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.web09.1560.1639590170144070171 for ; Wed, 15 Dec 2021 09:42:50 -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 6C9A01435; Wed, 15 Dec 2021 09:42:43 -0800 (PST) Received: from [10.34.129.49] (unknown [10.34.129.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2F94C3F5A1; Wed, 15 Dec 2021 09:42:42 -0800 (PST) Subject: Re: [edk2-devel] [PATCH v4 1/3] Silicon/ARM/NeoverseN1Soc: Port PCI Segment Library To: devel@edk2.groups.io, khasim.mohammed@arm.com Cc: nd@arm.com, Sami.mujawar@arm.com References: <20211214194356.21005-1-khasim.mohammed@arm.com> <20211214194356.21005-2-khasim.mohammed@arm.com> From: "PierreGondois" Message-ID: Date: Wed, 15 Dec 2021 18:42:45 +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: <20211214194356.21005-2-khasim.mohammed@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Hi Khassim, Thanks for the new serie. I have some comments: On 12/14/21 8:43 PM, Khasim Mohammed via groups.io wrote: > The BasePCISegment Library in MdePkg doesn't allow configuring > multiple segments required for PCIe and CCIX root port > enumeration. Therefore, a custom PCI Segment library is adapted > from SynQuacerPciSegmentLib and ported for N1Sdp. > > In addition to this, the hardware has few other limitations which affects > the access to the PCIe root port: > 1. ECAM space is not contiguous, root port ECAM (BDF = 0:0:0) is isolated > from rest of the downstream hierarchy ECAM space. > 2. Root port ECAM space is not capable of 8bit/16bit writes. > 3. A slave error is generated when host accesses the configuration > space of non-available device or unimplemented function on a > given bus. > > The description of the workarounds included for these limitations can > be found in the corresponding files of this patch. > > Change-Id: I0a124b0ea2fb7a8ee652de2d66b977d848c509b4 The 'Change-Id' should be removed I think. > Signed-off-by: Khasim Syed Mohammed > --- > .../Library/PciSegmentLib/PciSegmentLib.c | 1643 +++++++++++++++++ > .../Library/PciSegmentLib/PciSegmentLib.inf | 38 + > 2 files changed, 1681 insertions(+) > create mode 100644 Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c > create mode 100644 Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.inf > > diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c b/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c > new file mode 100644 > index 0000000000..3a3cf3008a > --- /dev/null > +++ b/Silicon/ARM/NeoverseN1Soc/Library/PciSegmentLib/PciSegmentLib.c > @@ -0,0 +1,1643 @@ > +/** @file > + PCI Segment Library for N1SDP SoC with multiple RCs > + > + Having two distinct root complexes is not supported by the standard > + set of PciLib/PciExpressLib/PciSegmentLib, this PciSegmentLib > + reimplements the functionality to support multiple root ports on > + different segment numbers. > + > + On the NeoverseN1Soc, a slave error is generated when host accesses the > + configuration space of non-available device or unimplemented function on a > + given bus. So this library introduces a workaround using IsBdfValid(), > + to return 0xFFFFFFFF for all such access. > + > + In addition to this, the hardware has two other limitations which affect > + access to the PCIe root port: > + 1. ECAM space is not contiguous, root port ECAM (BDF = 0:0:0) is isolated > + from rest of the downstream hierarchy ECAM space. > + 2. Root port ECAM space is not capable of 8bit/16bit writes. > + The description of the workarounds included for these limitations can > + be found in the comments below. > + > + Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.
> + Copyright (c) 2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include I think the library can be removed now. > +#include > +#include > + > +typedef enum { > + PciCfgWidthUint8 = 0, > + PciCfgWidthUint16, > + PciCfgWidthUint32, > + PciCfgWidthMax > +} PCI_CFG_WIDTH; > + > +/** > + Assert the validity of a PCI Segment address. > + A valid PCI Segment address should not contain 1's in bits 28..31 and 48..63 > + > + @param A The address to validate. > + @param M Additional bits to assert to be zero. > +**/ > +#define ASSERT_INVALID_PCI_SEGMENT_ADDRESS(A,M) \ > + ASSERT (((A) & (0xffff0000f0000000ULL | (M))) == 0) > + > +#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. > + > + @param A The address to validate. > + > +**/ > +#define ASSERT_INVALID_PCI_ADDRESS(A) \ > + ASSERT (((A) & ~0xffffffff) == 0) I think the macro is not used anymore. > + > +#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 > +#define BDF_TABLE_HEADER_COUNT 2 > +#define BDF_TABLE_HEADER_SIZE 8 > + > +/* 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) > +#define GET_FUNC_NUM(Address) (((Address) >> 12) & 0x07) > +#define GET_REG_NUM(Address) ((Address) & 0xFFF) > + > +CONST STATIC UINTN mDummyConfigData = 0xFFFFFFFF; > + > +/** > + Check if the requested PCI address is a valid BDF address. > + > + 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 The base address of PCI Express. > + > +**/ > +STATIC > +UINTN > +IsBdfValid ( > + IN UINTN Address > + ) > +{ > + UINT8 Bus; > + UINT8 Device; > + UINT8 Function; > + UINTN BdfCount; > + UINTN BdfValue; > + UINTN Count; > + UINTN TableBase; > + 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; > + } > + > + BdfCount = MmioRead32 (TableBase + BDF_TABLE_ENTRY_SIZE); > + > + /* 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; > + } > + > + if (Count == (BdfCount + BDF_TABLE_HEADER_COUNT)) { > + return mDummyConfigData; > + } else { > + return PciAddress; > + } > +} > + > +/** > + Get the physical address of a configuration space register. > + > + Implement a workaround to avoid generation of slave errors from the bus. That > + is, retrieve the PCI Express Base Address via a PCD entry, add the incomming > + address with that base address and check whether this converted address > + points to a accessible BDF. If it is not accessible, return the address > + of a dummy location so that a read from it does not cause a slave error. > + > + In addition to this, implement a workaround for accessing the root port's > + configuration space. The root port configuration space is not contiguous > + with the rest of the downstream hierarchy configuration space. So determine > + whether the specified address is for the root port and use a different base > + address for it. > + > + @param Address The address that encodes the PCI Bus, Device, Function and > + Register. > + > + @return Physical address of the configuration register that corresponds to the > + PCI configuration register specified by input parameter 'Address'. > + > +**/ > +STATIC > +VOID* > +GetPciExpressAddress ( > + IN UINTN Address > + ) > +{ > + BOOLEAN CheckRootPort; > + UINT8 Bus; > + UINT8 Device; > + UINT8 Function; > + UINT16 Register; > + UINTN ConfigAddress; > + > + Bus = GET_BUS_NUM (Address); > + Device = GET_DEV_NUM (Address); > + Function = GET_FUNC_NUM (Address); > + Register = GET_REG_NUM (Address); > + > + CheckRootPort = (BOOLEAN) (Bus == 0) && (Device == 0) && (Function == 0); > + > + if (GET_PCIE_BASE_ADDRESS (Address) == > + FixedPcdGet64 (PcdPcieExpressBaseAddress)) { > + if (CheckRootPort == TRUE) { > + ConfigAddress = (UINTN) (PcdGet32 (PcdPcieRootPortConfigBaseAddress) + > + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)); > + } else { > + ConfigAddress = (UINTN) (PcdGet64 (PcdPcieExpressBaseAddress) + > + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)); > + } > + } else { > + if (CheckRootPort == TRUE) { > + ConfigAddress = (UINTN) (PcdGet32 (PcdCcixRootPortConfigBaseAddress) + > + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)); > + } else { > + ConfigAddress = (UINTN) PcdGet32 (PcdCcixExpressBaseAddress + > + EFI_PCIE_ADDRESS (Bus, Device, Function, Register)); > + } > + } > + > + if (CheckRootPort == FALSE) { > + if (IsBdfValid (Address) == mDummyConfigData) { > + ConfigAddress = (UINTN) &mDummyConfigData; > + } > + } > + > + return (VOID *)ConfigAddress; > +} > + > +/** > + Function to return PCIe Physical Address for different RCs. > + If address is invalid, then ASSERT(). > + > + @param Address Address passed from bus layer. > + > + @return Return PCIe base address. > + > +**/ > +STATIC > +UINT64 > +PciSegmentLibGetConfigBase ( > + IN UINT64 Address > + ) > +{ > + switch ((UINT16)(Address >> 32)) {+ case 0: > + return FixedPcdGet32 (PcdPcieExpressBaseAddress); > + case 1: > + return FixedPcdGet32 (PcdCcixExpressBaseAddress); > + default: > + ASSERT (FALSE); > + } > + return 0; > +} > + > +/** > + Internal worker function to read a PCI configuration register. > + > + @param Address The address that encodes the PCI Bus, Device, Function > + and Register. > + @param Width The width of data to read > + > + @return The value read from the PCI configuration register. > +**/ > +STATIC > +UINT32 > +PciSegmentLibReadWorker ( > + IN UINT64 Address, > + IN PCI_CFG_WIDTH Width > + ) > +{ > + UINT64 Addr; > + UINT64 Base; > + > + Base = PciSegmentLibGetConfigBase (Address); > + Addr = (UINT64)GetPciExpressAddress ((UINT32) Address + Base); I think PciSegmentLibGetConfigBase() could be removed. Another macro to get the Pci segment should be created as: #define GET_SEGV_NUM(Address) (((Address) >> 32) & 0xFFFF) Cf the bit layout at MdePkg/Include/Library/PciSegmentLib.h And then in GetPciExpressAddress(), we get the Pci Segment value and choose which PcdXXXBaseAddress value to use. Similar comment for the PciSegmentLibWriteWorker() function. > + > + switch (Width) { > + case PciCfgWidthUint8: > + return MmioRead8 (Addr); > + case PciCfgWidthUint16: > + return MmioRead16 (Addr); > + case PciCfgWidthUint32: > + return MmioRead32 (Addr); > + default: > + ASSERT (FALSE); > + } > + > + return 0; > +} > + > +/** > + Internal worker function to write to a PCI configuration register. > + > + @param Address The address that encodes the PCI Bus, Device, Function > + and Register. > + @param Width The width of data to write > + @param Data The value to write. > + > + @return The value written to the PCI configuration register. > +**/ > +STATIC > +UINT32 > +PciSegmentLibWriteWorker ( > + IN UINT64 Address, > + IN PCI_CFG_WIDTH Width, > + IN UINT32 Data > + ) > +{ > + UINT8 Bus, Device, Function; > + UINT8 Offset; > + UINT32 WData; > + UINT64 Addr; > + UINT64 Base; > + > + Base = PciSegmentLibGetConfigBase (Address); > + Addr = (UINTN) Address + Base; > + > + Bus = GET_BUS_NUM (Addr); > + Device = GET_DEV_NUM (Addr); > + Function = GET_FUNC_NUM (Addr); > + > + // 8-bit and 16-bit writes to root port config space is not supported due to > + // a hardware limitation. As a workaround, perform a read-update-write > + // sequence on the whole 32-bit word of the root port config register such > + // that only the specified 8-bits of that word are updated. > + > + switch (Width) { > + case PciCfgWidthUint8: > + if ((Bus == 0) && (Device == 0) && (Function == 0)) { > + Offset = Addr & 0x3; > + Addr &= 0xFFFFFFFC; > + WData = MmioRead32 ((UINTN) GetPciExpressAddress (Addr)); > + WData &= ~(0xFF << (8 * Offset)); > + WData |= (Data << (8 * Offset)); > + MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), WData); > + return Data; > + } > + MmioWrite8 ((UINTN) GetPciExpressAddress (Addr), Data); > + break; > + case PciCfgWidthUint16: > + if ((Bus == 0) && (Device == 0) && (Function == 0)) { > + Offset = Addr & 0x3; > + Addr &= 0xFFFFFFFC; > + WData = MmioRead32 ((UINTN) GetPciExpressAddress (Addr)); > + WData &= ~(0xFFFF << (8 * Offset)); > + WData |= (Data << (8 * Offset)); > + MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), WData); > + return Data; > + } Some of the code for the 2 cases with 8/16 bits seems similar, is it possible to factorize it ? We could also store the result of GetPciExpressAddress (Addr) once for the whole function. Also, is there an 'else' statement missing ? It seems that when not writing to a root complex, we are writing 2 times. > + MmioWrite16 ((UINTN) GetPciExpressAddress (Addr), Data); > + break; > + case PciCfgWidthUint32: > + MmioWrite32 ((UINTN) GetPciExpressAddress (Addr), Data); > + break; > + default: > + ASSERT (FALSE); > + } > + > + return Data; > +} > + [SNIP]