From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH v2 1/3] Silicon/ARM/NeoverseN1Soc: Update PciExpressLib to enable CCIX port To: PierreGondois ,devel@edk2.groups.io From: "Khasim Mohammed" X-Originating-Location: Bengaluru, Karnataka, IN (217.140.105.53) X-Originating-Platform: Linux Firefox 72 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Mon, 06 Dec 2021 02:34:17 -0800 References: In-Reply-To: Message-ID: <21656.1638786857050381937@groups.io> Content-Type: multipart/alternative; boundary="1d1s3JhcxXicofgfTFab" --1d1s3JhcxXicofgfTFab Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Pierre, I have addressed all the suggested changes and posted v3 version of patches= . On Tue, Nov 30, 2021 at 09:10 AM, PierreGondois wrote: >=20 > Hi Khasim, >=20 > It seems that: > -PciHostBridgeDxe requires the PciSegmentLib. > -The MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf > implementation requires the PciLib. > -The MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf require= s > 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. I tried to do this, but unfortunately wasn't successful. We are reusing the= standard version of PciExpressLib.c and modifying the GetPci functionality= to return the updated base address. If we remove the inf entry from custom= implementation then it will leverage the default one and it will fail to g= et fixed PCIe address. There is similar patches posted for Morello SOC, https://edk2.groups.io/g/devel/message/84344?p=3D%2C%2C%2C20%2C0%2C0%2C0%3A= %3Arecentpostdate%2Fsticky%2C%2Cchandni%2C20%2C2%2C0%2C87497024 https://edk2.groups.io/g/devel/message/84165?p=3D%2C%2C%2C20%2C0%2C0%2C0%3A= %3Arecentpostdate%2Fsticky%2C%2Ckhasim%2C20%2C2%2C0%2C87257273 >=20 > A question about the mDummyConfigData value in > Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLi= b.c, > shouldn't it be a const ? Have fixed this in v3 version of patches. >=20 > I still made some comments inline in this patch. >=20 > Regards, >=20 > Pierre >=20 >=20 > On 11/23/21 1:26 PM, Khasim Mohammed via groups.io wrote: >=20 >> Update the PciExpressLib to enable CCIX port as PCIe root host by >> validating the PCIe addresses and introducing corresponding PCD >> entries. >>=20 >> Change-Id: I0d1167b86e53a3781f59c4d68a3b2e61add4317e >> Signed-off-by: Deepak Pandey >> Signed-off-by: Khasim Syed Mohammed >> --- >> .../PciExpressLib.c | 127 ++++++++++++------ >> .../PciExpressLib.inf | 7 +- >> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec | 5 +- >> 3 files changed, 94 insertions(+), 45 deletions(-) >>=20 >> diff --git >> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.c >> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.c >>=20 >> index bb0246b4a9..3abe0a2d6b 100644 >> --- >> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.c >>=20 >> +++ >> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.c >>=20 >> @@ -20,7 +20,7 @@ >> The description of the workarounds included for these limitations can >> be found in the comments below. >>=20 >> - Copyright (c) 2020, ARM Limited. All rights reserved. >> + Copyright (c) 2020 - 2021, ARM Limited. All rights reserved.
>>=20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >>=20 >> @@ -36,15 +36,29 @@ >> #include >> #include >>=20 >> +#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 conta= in >> 1's >> - only in the low 28 bits. >> + Assert the validity of a PCI address. A valid PCI address should conta= in >> 1's. >>=20 >> @param A The address to validate. >>=20 >> **/ >> #define ASSERT_INVALID_PCI_ADDRESS(A) \ >> - ASSERT (((A) & ~0xfffffff) =3D=3D 0) >> + ASSERT (((A) & ~0xffffffff) =3D=3D 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) >>=20 >> /* Root port Entry, BDF Entries Count */ >> #define BDF_TABLE_ENTRY_SIZE 4 >> @@ -53,6 +67,7 @@ >>=20 >> /* BDF table offsets for PCIe */ >> #define PCIE_BDF_TABLE_OFFSET 0 >> +#define CCIX_BDF_TABLE_OFFSET (16 * 1024) >>=20 >> #define GET_BUS_NUM(Address) (((Address) >> 20) & 0x7F) >> #define GET_DEV_NUM(Address) (((Address) >> 15) & 0x1F) >> @@ -113,49 +128,64 @@ PciExpressRegisterForRuntimeAccess ( >> } >>=20 >> /** >> - Check if the requested PCI address can be safely accessed. >> + Check if the requested PCI address is a valid BDF address. >>=20 >> - SCP performs the initial bus scan, prepares a table of valid BDF >> addresses >> - and shares them through non-trusted SRAM. This function validates if t= he >>=20 >> - 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 >>=20 >> - 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 t= he >> 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 bu= s >> fault >> + that happens when accessing unavailable PCI device due to RTL bug. >>=20 >> @param Address The address that encodes the PCI Bus, Device, Function an= d >> Register. >>=20 >> - @return TRUE BDF can be accessed, valid. >> - @return FALSE BDF should not be accessed, invalid. >> + @return The base address of PCI Express. >>=20 >> **/ >> 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 =3D GET_BUS_NUM (Address); >> + Device =3D GET_DEV_NUM (Address); >> + Function =3D GET_FUNC_NUM (Address); >> + >> + PciAddress =3D EFI_PCIE_ADDRESS (Bus, Device, Function, 0); >> + >> + if (GET_PCIE_BASE_ADDRESS (Address) =3D=3D >> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) { >> + TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFS= ET; >> + } else { >> + TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFS= ET; >> + } >>=20 >> - ConfigBase =3D Address & ~0xFFF; >> - TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OFFS= ET; >> BdfCount =3D MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE); >> - BdfEntry =3D TableBase + BDF_TABLE_HEADER_SIZE; >> - >> - /* Skip the header & check remaining entry */ >> - for (Count =3D 0; Count < BdfCount; Count++, BdfEntry +=3D >> BDF_TABLE_ENTRY_SIZE) { >> - BdfValue =3D MmioRead32 (BdfEntry); >> - if (BdfValue =3D=3D ConfigBase) { >> - return TRUE; >> - } >> + >> + /* Start from the second entry */ >> + for (Count =3D BDF_TABLE_HEADER_COUNT; >> + Count < (BdfCount + BDF_TABLE_HEADER_COUNT); >> + Count++) { >> + BdfValue =3D MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE)); >> + if (BdfValue =3D=3D PciAddress) >> + break; >> } >>=20 >> - return FALSE; >> + if (Count =3D=3D (BdfCount + BDF_TABLE_HEADER_COUNT)) { >> + return mDummyConfigData; >> + } else { >> + return PciAddress; >> + } >> } >>=20 >> /** >> @@ -186,20 +216,35 @@ GetPciExpressAddress ( >> IN UINTN Address >> ) >> { >> - UINT8 Bus, Device, Function; >> - UINTN ConfigAddress; >> - >> - Bus =3D GET_BUS_NUM (Address); >> - Device =3D GET_DEV_NUM (Address); >> + UINT8 Bus; >> + UINT8 Device; >> + UINT8 Function; >> + UINT16 Register; >> + UINTN ConfigAddress; >> + >> + // Get the EFI notation >> + Bus =3D GET_BUS_NUM (Address); >> + Device =3D GET_DEV_NUM (Address); >> Function =3D GET_FUNC_NUM (Address); >> - >> - if ((Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D 0)) { >> - ConfigAddress =3D PcdGet32 (PcdPcieRootPortConfigBaseAddress) + Addres= s; >> - } else { >> - ConfigAddress =3D PcdGet64 (PcdPciExpressBaseAddress) + Address; >> - if (!IsBdfValid(Address)) { >> - ConfigAddress =3D (UINTN)&mDummyConfigData; >> - } >> + Register =3D GET_REG_NUM (Address); >> + >> + ConfigAddress =3D (UINTN) >> + ((GET_PCIE_BASE_ADDRESS (Address) =3D=3D >> + FixedPcdGet64 (PcdPcieExpressBaseAddress)) ? >> + (((Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D 0)) ? >> + PcdGet32 (PcdPcieRootPortConfigBaseAddress >> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)): >> + PcdGet64 (PcdPcieExpressBaseAddress >> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register))) : >> + (((Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D 0)) ? >> + PcdGet32 (PcdCcixRootPortConfigBaseAddress >> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)): >> + PcdGet32 (PcdCcixExpressBaseAddress >> + + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)))); >=20 > 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=C2=A0 "(Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D = 0)" Have spit this functionality in v3 version of patches. >=20 > I m not sure why this is valid, but shouldn't the brackets of: >=20 > PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Device, > Function, Register)) >=20 > be like: >=20 > PcdGet64 (PcdPcieExpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device, > Function, Register) >=20 > (Same question for other PcdGet64() calls in the assignement) Have fixed this. >=20 >=20 >> + >> + if (!((Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D 0))) { >> + if (IsBdfValid (Address) =3D=3D mDummyConfigData) >> + ConfigAddress =3D (UINTN) &mDummyConfigData; >> } >>=20 >> return (VOID *)ConfigAddress; >> diff --git >> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.inf >> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.inf >>=20 >> index acb6fb6219..eac981e460 100644 >> --- >> a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.inf >>=20 >> +++ >> b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpres= sLib.inf >>=20 >> @@ -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.
>> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> @@ -43,6 +43,8 @@ >> Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec >>=20 >> [FixedPcd] >> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseAddress >> + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSize >> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress >> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize >>=20 >> @@ -53,4 +55,5 @@ >> PcdLib >>=20 >> [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 >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT64|0x= 00000011 >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|0x00= 000012 >>=20 >> + >> gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UIN= T64|0x00000013 >>=20 >>=20 >> # CCIX >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0x00000016 >> @@ -53,8 +54,8 @@ >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusMin|0|UINT32|0x00000018 >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixExpressBaseAddress|0x68000000|UIN= T32|0x00000019 >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A >> - >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x00000= 01B >>=20 >> - gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x000000= 1C >>=20 >> + >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x000= 0001B >>=20 >> + >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x000000= 1C >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|0= x00000001D >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|0x00= 00001E >>=20 >> gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT32|0= x00000001F >>=20 >=20 > --1d1s3JhcxXicofgfTFab Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Pierre,

I have addressed all the suggested changes and posted= v3 version of patches.

On Tue, Nov 30, 2021 at 09:10 AM, Pierre= Gondois wrote:
Hi Khasim,

It seems that:
-PciHostBridgeDxe req= uires the PciSegmentLib.
-The MdePkg/Library/BasePciSegmentLibPci/Base= PciSegmentLibPci.inf implementation requires the PciLib.
-The MdePkg/L= ibrary/BasePciLibPciExpress/BasePciLibPciExpress.inf requires the PciExpres= sLib, wich is implemented in Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1So= cPciExpressLib/
-Since the patch 2 of the serie is now implementing a = PciSegmentLib, isn't it possible to move the PCI hacks from Silicon/ARM/Neo= verseN1Soc/Library/NeoverseN1SocPciExpressLib/ to this new library ? The Si= licon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/ library would t= hen not be needed anymore and could be removed.
I tried to do this, but unfortunately wasn't successful. We are reusing the= standard version of PciExpressLib.c and modifying the GetPci functionality= to return the updated base address. If we remove the inf entry from custom= implementation then it will leverage the default one and it will fail to g= et fixed PCIe address.

There is similar patches posted for Morel= lo SOC,
https://edk2.groups.io/g/devel/message/84344?p=3D%2C%2C%2C20%= 2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Cchandni%2C20%2C2%2C0%2C874970= 24
https://edk2.groups.io/g/devel/m= essage/84165?p=3D%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2C= khasim%2C20%2C2%2C0%2C87257273


A question about the mDummyConfigData value in Silicon/ARM/Neov= erseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c, shouldn't it = be a const ?
Have fixed this in v3 version of patches.

I still made some comments inline in this patch.

Reg= ards,

Pierre


On 11/23/21 1:26 PM, Khasim Mohamm= ed 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<= br />Signed-off-by: Deepak Pandey <Deepak.Pandey@arm.com>
Signed= -off-by: Khasim Syed Mohammed <khasim.mohammed@arm.com>
---
.../PciExpressLib.c | 127 ++++++++++++------
.../PciExpressLib.inf | = 7 +-
Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec | 5 +-
3 files c= hanged, 94 insertions(+), 45 deletions(-)

diff --git a/Silicon/A= RM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c b/Silic= on/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
index bb0246b4a9..3abe0a2d6b 100644
--- a/Silicon/ARM/NeoverseN1Soc/= Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
+++ b/Silicon/ARM/N= eoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c
@@ -20= ,7 +20,7 @@
The description of the workarounds included for these limi= tations 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-Identif= ier: BSD-2-Clause-Patent

@@ -36,15 +36,29 @@
#include <L= ibrary/PcdLib.h>
#include <NeoverseN1Soc.h>

+#defi= ne 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 lo= w 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) =3D=3D 0)
+ ASSERT (((A) & ~0xffffffff) =3D=3D 0)+
+#define EFI_PCIE_ADDRESS(bus, dev, func, reg) \
+ (UINT64) = ( \
+ (((UINTN) bus) << BUS_OFFSET) | \
+ (((UINTN) dev) &l= t;< DEV_OFFSET) | \
+ (((UINTN) func) << FUNC_OFFSET) | \
+ (((UINTN) (reg)) < REG_OFFSET ? \
+ ((UINTN) (reg)) : (UINT64) = (LShiftU64 ((UINT64) (reg), 32))))
+
+#define GET_PCIE_BASE_ADDRE= SS(Address) (Address & 0xF8000000)

/* Root port Entry, BDF E= ntries Count */
#define BDF_TABLE_ENTRY_SIZE 4
@@ -53,6 +67,7 @@<= br />
/* BDF table offsets for PCIe */
#define PCIE_BDF_TABLE_OFF= SET 0
+#define CCIX_BDF_TABLE_OFFSET (16 * 1024)

#define GE= T_BUS_NUM(Address) (((Address) >> 20) & 0x7F)
#define GET_DE= V_NUM(Address) (((Address) >> 15) & 0x1F)
@@ -113,49 +128,64= @@ PciExpressRegisterForRuntimeAccess (
}

/**
- Check= if the requested PCI address can be safely accessed.
+ Check if the r= equested 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
- reque= sted 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 t= o
- avoid bus fault that occurs when accessing unavailable PCI device = due to
- hardware bug.
+ SCP performs the initial bus scan and pr= epares a table of valid BDF addresses
+ and shares them through non-tr= usted SRAM. This function validates if the PCI
+ address from any PCI = request falls within the table of valid entries. If not,
+ this functi= on will return 0xFFFFFFFF. This is a workaround to avoid bus fault
+ t= hat 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 bas= e address of PCI Express.

**/
STATIC
-BOOLEAN
+UI= NTN
IsBdfValid (
- IN UINTN Address
+ IN UINTN Address
= )
{
+ UINT8 Bus;
+ UINT8 Device;
+ UINT8 Function;
UINTN BdfCount;
UINTN BdfValue;
- UINTN BdfEntry;
UINTN Cou= nt;
UINTN TableBase;
- UINTN ConfigBase;
+ UINTN PciAddress;=
+
+ Bus =3D GET_BUS_NUM (Address);
+ Device =3D GET_DEV_NUM= (Address);
+ Function =3D GET_FUNC_NUM (Address);
+
+ PciAd= dress =3D EFI_PCIE_ADDRESS (Bus, Device, Function, 0);
+
+ if (GE= T_PCIE_BASE_ADDRESS (Address) =3D=3D
+ FixedPcdGet64 (PcdPcieExpressBa= seAddress)) {
+ TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCI= E_BDF_TABLE_OFFSET;
+ } else {
+ TableBase =3D NEOVERSEN1SOC_NON_= SECURE_SRAM_BASE + CCIX_BDF_TABLE_OFFSET;
+ }

- ConfigBase = =3D Address & ~0xFFF;
- TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRA= M_BASE + PCIE_BDF_TABLE_OFFSET;
BdfCount =3D MmioRead32 (TableBase + B= DF_TABLE_ENTRY_SIZE);
- BdfEntry =3D TableBase + BDF_TABLE_HEADER_SIZE= ;
-
- /* Skip the header & check remaining entry */
- fo= r (Count =3D 0; Count < BdfCount; Count++, BdfEntry +=3D BDF_TABLE_ENTRY= _SIZE) {
- BdfValue =3D MmioRead32 (BdfEntry);
- if (BdfValue =3D= =3D ConfigBase) {
- return TRUE;
- }
+
+ /* Start from = the second entry */
+ for (Count =3D BDF_TABLE_HEADER_COUNT;
+ Co= unt < (BdfCount + BDF_TABLE_HEADER_COUNT);
+ Count++) {
+ BdfV= alue =3D MmioRead32 (TableBase + (Count * BDF_TABLE_ENTRY_SIZE));
+ if= (BdfValue =3D=3D PciAddress)
+ break;
}

- return FALS= E;
+ if (Count =3D=3D (BdfCount + BDF_TABLE_HEADER_COUNT)) {
+ re= turn mDummyConfigData;
+ } else {
+ return PciAddress;
+ }}

/**
@@ -186,20 +216,35 @@ GetPciExpressAddress (
IN UINTN Address
)
{
- UINT8 Bus, Device, Function;
- = UINTN ConfigAddress;
-
- Bus =3D GET_BUS_NUM (Address);
- De= vice =3D GET_DEV_NUM (Address);
+ UINT8 Bus;
+ UINT8 Device;
+ UINT8 Function;
+ UINT16 Register;
+ UINTN ConfigAddress;
+
+ // Get the EFI notation
+ Bus =3D GET_BUS_NUM (Address);
+ Device =3D GET_DEV_NUM (Address);
Function =3D GET_FUNC_NUM (Addre= ss);
-
- if ((Bus =3D=3D 0) && (Device =3D=3D 0) &&am= p; (Function =3D=3D 0)) {
- ConfigAddress =3D PcdGet32 (PcdPcieRootPor= tConfigBaseAddress) + Address;
- } else {
- ConfigAddress =3D Pcd= Get64 (PcdPciExpressBaseAddress) + Address;
- if (!IsBdfValid(Address)= ) {
- ConfigAddress =3D (UINTN)&mDummyConfigData;
- }
+ = Register =3D GET_REG_NUM (Address);
+
+ ConfigAddress =3D (UINTN)=
+ ((GET_PCIE_BASE_ADDRESS (Address) =3D=3D
+ FixedPcdGet64 (PcdP= cieExpressBaseAddress)) ?
+ (((Bus =3D=3D 0) && (Device =3D=3D= 0) && (Function =3D=3D 0)) ?
+ PcdGet32 (PcdPcieRootPortConfi= gBaseAddress
+ + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)):<= br />+ PcdGet64 (PcdPcieExpressBaseAddress
+ + EFI_PCIE_ADDRESS (Bus, = Device, Function, Register))) :
+ (((Bus =3D=3D 0) && (Device = =3D=3D 0) && (Function =3D=3D 0)) ?
+ PcdGet32 (PcdCcixRootPor= tConfigBaseAddress
+ + EFI_PCIE_ADDRESS (Bus, Device, Function, Regist= er)):
+ PcdGet32 (PcdCcixExpressBaseAddress
+ + EFI_PCIE_ADDRESS = (Bus, Device, Function, Register))));
Is it possible to split it in multiple statement ? Since the case of the ro= ot bridge is special and checked multiple times, we could store the result = of  "(Bus =3D=3D 0) && (Device =3D=3D 0) && (Function = =3D=3D 0)"
Have spit this functionality in v3 version of patches.

I m not sure why this is valid, but shouldn't the brackets of:<= br />
PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Dev= ice, Function, Register))

be like:

PcdGet64 (PcdPcieE= xpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)

(Same question for other PcdGet64() calls in the assignement) Have fixed this.

+
+ if (!((Bus =3D=3D 0) && (Device =3D=3D 0) &= ;& (Function =3D=3D 0))) {
+ if (IsBdfValid (Address) =3D=3D mDumm= yConfigData)
+ ConfigAddress =3D (UINTN) &mDummyConfigData;
}=

return (VOID *)ConfigAddress;
diff --git a/Silicon/ARM/Neo= verseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf b/Silicon/A= RM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
= index acb6fb6219..eac981e460 100644
--- a/Silicon/ARM/NeoverseN1Soc/Li= brary/NeoverseN1SocPciExpressLib/PciExpressLib.inf
+++ b/Silicon/ARM/N= eoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf
@@ -= 21,7 +21,7 @@
# 2. Root port ECAM space is not capable of 8bit/16bit w= rites.
# This library includes workaround for these limitations as wel= l.
#
-# 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.PcdCcixRootPortConfigBase= Address
+ gArmNeoverseN1SocTokenSpaceGuid.PcdCcixRootPortConfigBaseSiz= e
gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseAddress
gArmNeoverseN1SocTokenSpaceGuid.PcdPcieRootPortConfigBaseSize

= @@ -53,4 +55,5 @@
PcdLib

[Pcd]
- gEfiMdePkgTokenSpaceG= uid.PcdPciExpressBaseAddress ## CONSUMES
+ gArmNeoverseN1SocTokenSpace= Guid.PcdCcixExpressBaseAddress ## CONSUMES
+ gArmNeoverseN1SocTokenSpa= ceGuid.PcdPcieExpressBaseAddress
diff --git a/Silicon/ARM/NeoverseN1So= c/NeoverseN1Soc.dec b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
inde= x eea2d58402..5ec3c32539 100644
--- a/Silicon/ARM/NeoverseN1Soc/Neover= seN1Soc.dec
+++ b/Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
@@ = -46,6 +46,7 @@
gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64MaxBase|0x= 28FFFFFFFF|UINT64|0x00000010
gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMm= io64Size|0x2000000000|UINT64|0x00000011
gArmNeoverseN1SocTokenSpaceGui= d.PcdPcieMmio64Translation|0x0|UINT64|0x00000012
+ gArmNeoverseN1SocTo= kenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000|UINT64|0x00000013
# CCIX
gArmNeoverseN1SocTokenSpaceGuid.PcdCcixBusCount|18|UINT32|0= x00000016
@@ -53,8 +54,8 @@
gArmNeoverseN1SocTokenSpaceGuid.PcdCc= ixBusMin|0|UINT32|0x00000018
gArmNeoverseN1SocTokenSpaceGuid.PcdCcixEx= pressBaseAddress|0x68000000|UINT32|0x00000019
gArmNeoverseN1SocTokenSp= aceGuid.PcdCcixIoBase|0x0|UINT32|0x0000001A
- gArmNeoverseN1SocTokenSp= aceGuid.PcdCcixIoMaxBase|0x01FFFF|UINT32|0x0000001B
- gArmNeoverseN1So= cTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000001C
+ gArmNeovers= eN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0x0000001B
+ g= ArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x0000001CgArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT32|= 0x00000001D
gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x692000= 00|UINT32|0x0000001E
gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxB= ase|0x6D1FFFFF|UINT32|0x00000001F
--1d1s3JhcxXicofgfTFab--