Re: [edk2-devel] [edk2-platforms][PATCH v4 2/5] Silicon/ARM/N1SoC: Implement Neoverse N1 Soc specific PciExpressLib

["Leif Lindholm" <leif@nuviainc.com>]

On Tue, Jul 21, 2020 at 23:53:36 +0530, Thomas Abraham wrote:
> Hi Pranav,
> 
> On Sun, Jul 19, 2020 at 2:19 PM Pranav Madhu <pranav.madhu@arm.com> wrote:
> >
> > From: Deepak Pandey <deepak.pandey@arm.com>
> >
> > A slave error is generated when host accesses the config space of
> > non-available device or unimplemented function on a given bus. So
> > implement a Neoverse N1 SoC specific PciExpressLib library with a
> > workaround to return 0xffffffff for all such access.
> >
> > This library is inherited from MdePkg/Library/BasePciExpressLib and
> > based on commit 9344f0921518 of that library in the tianocore/edk2
> > project.
> >
> > In addition to this, the Neoverse N1 SoC has two other limitations which
> > affect 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.
> > This library includes workaround for these limitations as well.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Signed-off-by: Pranav Madhu <pranav.madhu@arm.com>
> > ---
> >  Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec                                    |    4 +
> >  Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.inf |   56 +
> >  Silicon/ARM/NeoverseN1Soc/Library/NeoverseN1SocPciExpressLib/PciExpressLib.c   | 1589 ++++++++++++++++++++
> >  3 files changed, 1649 insertions(+)
> >
> 
> <...>
> 
> > +UINT8
> > +EFIAPI
> > +PciExpressWrite8 (
> > +  IN      UINTN                     Address,
> > +  IN      UINT8                     Value
> > +  )
> > +{
> > +  UINT8 Bus, Device, Function;
> > +  UINT8 Offset;
> > +  UINT32 Data;
> > +
> > +  ASSERT_INVALID_PCI_ADDRESS (Address);
> > +
> > +  Bus = GET_BUS_NUM (Address);
> > +  Device = GET_DEV_NUM (Address);
> > +  Function = GET_FUNC_NUM (Address);
> > +
> > +  //
> > +  // 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.
> > +  //
> > +  if ((Bus == 0) && (Device == 0) && (Function == 0)) {
> > +    Offset = Address & 0x3;
> > +    Address &= 0xFFFFFFFC;
> > +    Data = MmioRead32 ((UINTN)GetPciExpressAddress (Address));
> 
> nit: There should have been a space before the GetPciExpressAddress.
> This is inconsistent with the rest of the file.

Actually, this is the correct way around.
If the rest of the file is different, that is what should change.

Regards,

Leif

> 
> Thanks,
> Thomas.
> 
> <...>