Re: [PATCH edk2-platforms 10/16] Silicon/NXP: PciSegmentLib: Add ECAM config support for PCIe LS Controller

["Wasim Khan (OSS)" <wasim.khan@oss.nxp.com>]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Friday, May 22, 2020 3:06 PM
> To: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>; devel@edk2.groups.io;
> Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Vabhav Sharma
> <vabhav.sharma@nxp.com>; Varun Sethi <V.Sethi@nxp.com>;
> leif@nuviainc.com; jon@solid-run.com
> Cc: Wasim Khan <wasim.khan@nxp.com>
> Subject: Re: [PATCH edk2-platforms 10/16] Silicon/NXP: PciSegmentLib: Add
> ECAM config support for PCIe LS Controller
> 
> On 5/22/20 1:02 AM, Wasim Khan wrote:
> > From: Wasim Khan <wasim.khan@nxp.com>
> >
> > PCIe Layerscape controller can be enabled for ECAM style configuration
> > access using CFG SHIFT Feature.
> >
> > Check for PcdPciCfgShiftEnable to decide the configuration access
> > scheme to be used with PCIe LS controller.
> >
> > Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> > ---
> >   Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf |  3 +++
> >   Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c   | 20
> ++++++++++++++++----
> >   2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
> > b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
> > index a36e79239b33..936213dc8a9d 100755
> > --- a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
> > +++ b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.inf
> > @@ -30,3 +30,6 @@ [LibraryClasses]
> >
> >   [FixedPcd]
> >     gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr
> > +
> > +[Pcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable
> > diff --git a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
> > b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
> > index ecd36971b753..552a425c6832 100755
> > --- a/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
> > +++ b/Silicon/NXP/Library/PciSegmentLib/PciSegmentLib.c
> > @@ -34,6 +34,8 @@ typedef enum {
> >   #define ASSERT_INVALID_PCI_SEGMENT_ADDRESS(A,M) \
> >     ASSERT (((A) & (0xffff0000f0000000ULL | (M))) == 0)
> >
> > +static BOOLEAN CfgShiftEnable;
> > +
> 
> This is a compile time constant, right? Please try to avoid using runtime
> read/write variables in that case, since it defeats the compiler's ability to
> remove code that is never executed.

We are initializing CfgShiftEnable with PcdPciCfgShiftEnable (dynamic PCD). 
We indent to enable/disable this support at run time so that without recompiling the firmware, we can turn on/off this feature.

> 
> 
> >   STATIC
> >   UINT64
> >   PciLsCfgTarget (
> > @@ -88,11 +90,20 @@ PciLsGetConfigBase (
> >   {
> >     UINT32 CfgAddr;
> >
> > -  CfgAddr = (UINT16)Offset;
> > -  if (Bus) {
> > -    return PciLsCfgTarget (PCI_SEG0_DBI_BASE + PCI_DBI_SIZE_DIFF *
> Segment, Address, Segment, Bus, Offset);
> > +  if (CfgShiftEnable) {
> > +    CfgAddr = (UINT32)Address;
> > +    if (Bus) {
> > +      return PCI_SEG0_MMIO_MEMBASE + PCI_BASE_DIFF * Segment +
> CfgAddr;
> > +    } else {
> > +      return PCI_SEG0_DBI_BASE + PCI_DBI_SIZE_DIFF * Segment + CfgAddr;
> > +    }
> >     } else {
> > -    return PCI_SEG0_DBI_BASE + PCI_DBI_SIZE_DIFF * Segment + CfgAddr;
> > +    CfgAddr = (UINT16)Offset;
> > +    if (Bus) {
> > +      return PciLsCfgTarget (PCI_SEG0_DBI_BASE + PCI_DBI_SIZE_DIFF *
> Segment, Address, Segment, Bus, Offset);
> > +    } else {
> > +      return PCI_SEG0_DBI_BASE + PCI_DBI_SIZE_DIFF * Segment + CfgAddr;
> > +    }
> >     }
> >   }
> >
> > @@ -608,5 +619,6 @@ PciSegLibInit (
> >     IN EFI_SYSTEM_TABLE  *SystemTable
> >     )
> >   {
> > +  CfgShiftEnable = CFG_SHIFT_ENABLE;
> >     return EFI_SUCCESS;
> >   }
> >