From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.79419.1638292214541626961 for ; Tue, 30 Nov 2021 09:10:15 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1D48DED1; Tue, 30 Nov 2021 09:10:13 -0800 (PST) Received: from [10.34.125.4] (e126645.nice.arm.com [10.34.125.4]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D48C73F694; Tue, 30 Nov 2021 09:10:11 -0800 (PST) Subject: Re: [edk2-devel] [PATCH v2 1/3] Silicon/ARM/NeoverseN1Soc: Update PciExpressLib to enable CCIX port To: devel@edk2.groups.io, khasim.mohammed@arm.com Cc: nd@arm.com, Deepak Pandey , Sami.mujawar@arm.com References: <20211123122655.9516-1-khasim.mohammed@arm.com> <20211123122655.9516-2-khasim.mohammed@arm.com> From: "PierreGondois" Message-ID: Date: Tue, 30 Nov 2021 18:10:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211123122655.9516-2-khasim.mohammed@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Khasim, It seems that: -PciHostBridgeDxe requires the PciSegmentLib. -The MdePkg/Library/BasePciSegmentLibPci/BasePciSegmentLibPci.inf impleme= ntation requires the PciLib. -The MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf require= s the PciExpressLib, wich is implemented in Silicon/ARM/NeoverseN1Soc/Lib= rary/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/Librar= y/NeoverseN1SocPciExpressLib/ to this new library ? The Silicon/ARM/Neove= rseN1Soc/Library/NeoverseN1SocPciExpressLib/ library would then not be ne= eded anymore and could be removed. A question about the mDummyConfigData value in Silicon/ARM/NeoverseN1Soc/= Library/NeoverseN1SocPciExpressLib/PciExpressLib.c, shouldn't it be a con= st ? 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 > 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(-) > > diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressL= ib/PciExpressLib.c b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciEx= pressLib/PciExpressLib.c > index bb0246b4a9..3abe0a2d6b 100644 > --- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciE= xpressLib.c > +++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciE= xpressLib.c > @@ -20,7 +20,7 @@ > The description of the workarounds included for these limitations ca= n > 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 con= tain 1's > - only in the low 28 bits. > + Assert the validity of a PCI address. A valid PCI address should con= tain 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 add= resses > - and shares them through non-trusted SRAM. This function validates if= the > - requested PCI address belongs to a valid BDF by checking the table o= f valid > - entries. If not, this function will return false. This is a workarou= nd to > - avoid bus fault that occurs when accessing unavailable PCI device du= e 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. > =20 > @param Address The address that encodes the PCI Bus, Device, Functi= on and > 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_= OFFSET; > + } else { > + TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + CCIX_BDF_TABLE_= OFFSET; > + } > =20 > - ConfigBase =3D Address & ~0xFFF; > - TableBase =3D NEOVERSEN1SOC_NON_SECURE_SRAM_BASE + PCIE_BDF_TABLE_OF= FSET; > 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) + Ad= dress; > - } 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) && (Functi= on =3D=3D 0)) ? > + PcdGet32 (PcdPcieRootPortConfigBaseAddress > + + EFI_PCIE_ADDRESS (Bus, Device, Function, Reg= ister)): > + PcdGet64 (PcdPcieExpressBaseAddress > + + EFI_PCIE_ADDRESS (Bus, Device, Function, Reg= ister))) : > + (((Bus =3D=3D 0) && (Device =3D=3D 0) && (Functi= on =3D=3D 0)) ? > + PcdGet32 (PcdCcixRootPortConfigBaseAddress > + + EFI_PCIE_ADDRESS (Bus, Device, Function, Reg= ister)): > + PcdGet32 (PcdCcixExpressBaseAddress > + + EFI_PCIE_ADDRESS (Bus, Device, Function, Reg= ister)))); 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 res= ult of=C2=A0 "(Bus =3D=3D 0) && (Device =3D=3D 0) && (Function =3D=3D 0)" I m not sure why this is valid, but shouldn't the brackets of: PcdGet64 (PcdPcieExpressBaseAddress + EFI_PCIE_ADDRESS (Bus, Device, Func= tion, Register)) be like: PcdGet64 (PcdPcieExpressBaseAddress) + EFI_PCIE_ADDRESS (Bus, Device, Fun= ction, Register) (Same question for other PcdGet64() calls in the assignement) > + > + 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/NeoverseN1SocPciExpressL= ib/PciExpressLib.inf b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPci= ExpressLib/PciExpressLib.inf > index acb6fb6219..eac981e460 100644 > --- a/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciE= xpressLib.inf > +++ b/Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciE= xpressLib.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.
> # > # 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 ## CONSUM= ES > + 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|UI= NT64|0x00000010 > gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Size|0x2000000000|UINT6= 4|0x00000011 > gArmNeoverseN1SocTokenSpaceGuid.PcdPcieMmio64Translation|0x0|UINT64|= 0x00000012 > + gArmNeoverseN1SocTokenSpaceGuid.PcdPcieExpressBaseAddress|0x70000000= |UINT64|0x00000013 > =20 > # 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|0x0= 000001B > - gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x020000|UINT32|0x0000= 001C > + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoMaxBase|0x00FFFFFF|UINT32|0= x0000001B > + gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoSize|0x01000000|UINT32|0x00= 00001C > gArmNeoverseN1SocTokenSpaceGuid.PcdCcixIoTranslation|0x6D200000|UINT= 32|0x00000001D > gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32Base|0x69200000|UINT32|= 0x0000001E > gArmNeoverseN1SocTokenSpaceGuid.PcdCcixMmio32MaxBase|0x6D1FFFFF|UINT= 32|0x00000001F