Hi Khasim,
It seems that:
-PciHostBridgeDxe requires the PciSegmentLib.
-The MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf
implementation requires the PciLib.
-The MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf requires the
PciExpressLib, wich is implemented in
Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/
-Since the patch 2 of the serie is now implementing a PciSegmentLib, isn't it
possible to move the PCI hacks from
Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ to this new
library ? The Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/
library would then not be needed anymore and could be removed.
A question about the mDummyConfigData value in
Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c,
shouldn't it be a const ?
I still made some comments inline in this patch.
Regards,
Pierre
On 11/23/21 1:26 PM, Khasim Mohammed via groups.io wrote:
> Update the PciExpressLib to enable CCIX port as PCIe root host by
> validating the PCIe addresses and introducing corresponding PCD
> entries.
>
> Change-Id: I0d1167b86e53a3781f59c4d68a3b2e61add4317e
> Signed-off-by: Deepak Pandey <deepak.pan...@arm.com>
> Signed-off-by: Khasim Syed Mohammed <khasim.moham...@arm.com>
> ---
> .../PciExpressLib.c | 127 ++++++++++++------
> .../PciExpressLib.inf | 7 +-
> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec | 5 +-
> 3 files changed, 94 insertions(+), 45 deletions(-)
>
> diff --git
> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
>
> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> index bb0246b4a9..3abe0a2d6b 100644
> ---
> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> +++
> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
> @@ -20,7 +20,7 @@
> The description of the workarounds included for these limitations can
> be found in the comments below.
>
> - Copyright (c) 2020, ARM Limited. All rights reserved.
> + Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -36,15 +36,29 @@
> #include <Library/PcdLib.h>
> #include <NeoverseN1Soc.h>
>
> +#define BUS_OFFSET 20
> +#define DEV_OFFSET 15
> +#define FUNC_OFFSET 12
> +#define REG_OFFSET 4096
> +
> /**
> - Assert the validity of a PCI address. A valid PCI address should contain
> 1's
> - only in the low 28 bits.
> + Assert the validity of a PCI address. A valid PCI address should contain
> 1's.
>
> @param A The address to validate.
>
> **/
> #define ASSERT_INVALID_PCI_ADDRESS(A) \
> - ASSERT (((A) & ~0xfffffff) == 0)
> + ASSERT (((A) & ~0xffffffff) == 0)
> +
> +#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
> + (UINT64) ( \
> + (((UINTN) bus) << BUS_OFFSET) | \
> + (((UINTN) dev) << DEV_OFFSET) | \
> + (((UINTN) func) << FUNC_OFFSET) | \
> + (((UINTN) (reg)) < REG_OFFSET ? \
> + ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) (reg), 32))))
> +
> +#define GET_PCIE_BASE_ADDRESS(Address) (Address & 0xF8000000)
>
> /* Root port Entry, BDF Entries Count */
> #define BDF_TABLE_ENTRY_SIZE 4
> @@ -53,6 +67,7 @@
>
> /* BDF table offsets for PCIe */
> #define PCIE_BDF_TABLE_OFFSET 0
> +#define CCIX_BDF_TABLE_OFFSET (16 * 1024)
>
> #define GET_BUS_NUM(Address) (((Address) >> 20) & 0x7F)
> #define GET_DEV_NUM(Address) (((Address) >> 15) & 0x1F)
> @@ -113,49 +128,64 @@ PciExpressRegisterForRuntimeAccess (
> }
>
> /**
> - Check if the requested PCI address can be safely accessed.
> + Check if the requested PCI address is a valid BDF address.
>
> - SCP performs the initial bus scan, prepares a table of valid BDF addresses
> - and shares them through non-trusted SRAM. This function validates if the
> - requested PCI address belongs to a valid BDF by checking the table of valid
> - entries. If not, this function will return false. This is a workaround to
> - avoid bus fault that occurs when accessing unavailable PCI device due to
> - hardware bug.
> + SCP performs the initial bus scan and prepares a table of valid BDF
> addresses
> + and shares them through non-trusted SRAM. This function validates if the
> PCI
> + address from any PCI request falls within the table of valid entries. If
> not,
> + this function will return 0xFFFFFFFF. This is a workaround to avoid bus
> fault
> + that happens when accessing unavailable PCI device due to RTL bug.
>
> @param Address The address that encodes the PCI Bus, Device, Function and
> Register.
>
> - @return TRUE BDF can be accessed, valid.
> - @return FALSE BDF should not be accessed, invalid.
> + @return The base address of PCI Express.
>
> **/
> STATIC
> -BOOLEAN
> +UINTN
> IsBdfValid (
> - IN UINTN Address
> + IN UINTN Address
> )
> {
> + UINT8 Bus;
> + UINT8 Device;
> + UINT8 Function;
> UINTN BdfCount;
> UINTN BdfValue;
> - UINTN BdfEntry;
> UINTN Count;
> UINTN TableBase;
> - UINTN ConfigBase;
> + UINTN PciAddress;
> +
> + Bus = GET_BUS_NUM (Address);
> + Device = GET_DEV_NUM (Address);
> + Function = GET_FUNC_NUM (Address);
> +
> + PciAddress = EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
> +
> + if (GET_PCIE_BASE_ADDRESS (Address) ==
> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) {
> + TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
> + } else {
> + TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
> + }
>
> - ConfigBase = Address & ~0xFFF;
> - TableBase = NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFSET;
> BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE);
> - BdfEntry = TableBase + BDF_TABLE_HEADER_SIZE;
> -
> - /* Skip the header & check remaining entry */
> - for (Count = 0; Count < BdfCount; Count++, BdfEntry +=
> BDF_TABLE_ENTRY_SIZE) {
> - BdfValue = MmioRead32 (BdfEntry);
> - if (BdfValue == ConfigBase) {
> - return TRUE;
> - }
> +
> + /* Start from the second entry */
> + for (Count = BDF_TABLE_HEADER_COUNT;
> + Count < (BdfCount + BDF_TABLE_HEADER_COUNT);
> + Count++) {
> + BdfValue = MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
> + if (BdfValue == PciAddress)
> + break;
> }
>
> - return FALSE;
> + if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) {
> + return mDummyConfigData;
> + } else {
> + return PciAddress;
> + }
> }
>
> /**
> @@ -186,20 +216,35 @@ GetPciExpressAddress (
> IN UINTN Address
> )
> {
> - UINT8 Bus, Device, Function;
> - UINTN ConfigAddress;
> -
> - Bus = GET_BUS_NUM (Address);
> - Device = GET_DEV_NUM (Address);
> + UINT8 Bus;
> + UINT8 Device;
> + UINT8 Function;
> + UINT16 Register;
> + UINTN ConfigAddress;
> +
> + // Get the EFI notation
> + Bus = GET_BUS_NUM (Address);
> + Device = GET_DEV_NUM (Address);
> Function = GET_FUNC_NUM (Address);
> -
> - if ((Bus == 0) && (Device == 0) && (Function == 0)) {
> - ConfigAddress = PcdGet32 (PcdPcieRootPortConfigBaseAddress) + Address;
> - } else {
> - ConfigAddress = PcdGet64 (PcdPciExpressBaseAddress) + Address;
> - if (!IsBdfValid(Address)) {
> - ConfigAddress = (UINTN)&mDummyConfigData;
> - }
> + Register = GET_REG_NUM (Address);
> +
> + ConfigAddress = (UINTN)
> + ((GET_PCIE_BASE_ADDRESS (Address) ==
> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) ?
> + (((Bus == 0) && (Device == 0) && (Function == 0)) ?
> + PcdGet32 (PcdPcieRootPortConfigBaseAddress
> + + EFI_PCIE_ADDRESS (Bus, Device, Function,
> Register)):
> + PcdGet64 (PcdPcieExpressBaseAddress
> + + EFI_PCIE_ADDRESS (Bus, Device, Function,
> Register))) :
> + (((Bus == 0) && (Device == 0) && (Function == 0)) ?
> + PcdGet32 (PcdCcixRootPortConfigBaseAddress
> + + EFI_PCIE_ADDRESS (Bus, Device, Function,
> Register)):
> + PcdGet32 (PcdCcixExpressBaseAddress
> + + EFI_PCIE_ADDRESS (Bus, Device, Function,
> Register))));
Is it possible to split it in multiple statement ? Since the case of the root
bridge is special and checked multiple times, we could store the result ofÂ
"(Bus == 0) && (Device == 0) && (Function == 0)"
I m not sure why this is valid, but shouldn't the brackets of:
PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Device, Function,
Register))
be like:
PcdGet64 (PcdPcieExpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device, Function,
Register)
(Same question for other PcdGet64() calls in the assignement)
> +
> + if (!((Bus == 0) && (Device == 0) && (Function == 0))) {
> + if (IsBdfValid (Address) == mDummyConfigData)
> + ConfigAddress = (UINTN) &mDummyConfigData;
> }
>
> return (VOID *)ConfigAddress;
> diff --git
> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
>
> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> index acb6fb6219..eac981e460 100644
> ---
> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> +++
> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
> @@ -21,7 +21,7 @@
> # 2. Root port ECAM space is not capable of 8bit/16bit writes.
> # This library includes workaround for these limitations as well.
> #
> -# Copyright (c) 2020, ARM Limited. All rights reserved.
> +# Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> @@ -43,6 +43,8 @@
> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>
> [FixedPcd]
> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseAddress
> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSize
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize
>
> @@ -53,4 +55,5 @@
> PcdLib
>
> [Pcd]
> - gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress ## CONSUMES
> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress ## CONSUMES
> + gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress
> diff --git a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> index eea2d58402..5ec3c32539 100644
> --- a/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> +++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
> @@ -46,6 +46,7 @@
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64MaxBase|0x28FFFFFFFF|UINT64|0x00000010
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT64|0x00000011
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|0x00000012
> +
> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UINT64|0x00000013
>
> # CCIX
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0x00000016
> @@ -53,8 +54,8 @@
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusMin|0|UINT32|0x00000018
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress|0x68000000|UINT32|0x00000019
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A
> - gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x0000001B
> - gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000001C
> +
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x0000001B
> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x0000001C
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|0x00000001D
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|0x0000001E
>
> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT32|0x00000001F
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84165): https://edk2.groups.io/g/devel/message/84165
Mute This Topic: https://groups.io/mt/87257273/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
