[edk2-devel] [PATCH v2 1/3] Silicon/ARM/NeoverseN1Soc: Update PciExpressLib to enable CCIX port
Khasim Mohammed
khasim.mohammed at arm.com
Mon Dec 6 10:34:17 UTC 2021
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:
>
> 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.
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 get fixed PCIe address.
There is similar patches posted for Morello SOC,
https://edk2.groups.io/g/devel/message/84344?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Cchandni%2C20%2C2%2C0%2C87497024
https://edk2.groups.io/g/devel/message/84165?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ckhasim%2C20%2C2%2C0%2C87257273
>
> A question about the mDummyConfigData value in
> Silicon/ARM/NeoverseN1Soc/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.
>
> 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.Pandey at arm.com>
>> Signed-off-by: Khasim Syed Mohammed <khasim.mohammed at 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)"
Have spit this functionality in v3 version of patches.
>
> 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)
Have fixed this.
>
>
>> +
>> + 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 (#84398): https://edk2.groups.io/g/devel/message/84398
Mute This Topic: https://groups.io/mt/87257273/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20211206/68298451/attachment.htm>
More information about the edk2-devel-archive
mailing list