Re: [PATCH 08/19] Silicon/NXP: Remove unnecessary PCDs

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Monday, February 10, 2020 11:02 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Varun Sethi <V.Sethi@nxp.com>;
> devel@edk2.groups.io
> Subject: Re: [PATCH 08/19] Silicon/NXP: Remove unnecessary PCDs
> 
> On Fri, Feb 07, 2020 at 18:13:17 +0530, Pankaj Bansal wrote:
> > There is no need to keep SOC specific PCDs defined for each SOC.
> 
> That sound like the definition of why we have SoC-specific Pcds, so I don't
> follow.
> 
> > we can do away with these PCDs.
> 
> After looking through this patchset, it looks like:
> 1) Initial implementation defined a bunch of things as Pcds which
>    really cannot vary between different platforms using the same SoC.

Yes.

> 2) This patch moves several things that can differ between platforms
>    into #defines, when they should be Pcds.
> 
> I am OK with the 1)s, but there are some you would need to convince me are not
> 2:s.

I have moved only those things to #define, which would be common for all platforms using the SOC.
I will explain more below.

> 
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> > ---
> >  .../Drivers/PlatformDxe/PlatformDxe.c         |  15 +--
> >  .../Drivers/PlatformDxe/PlatformDxe.inf       |   8 +-
> >  Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc  |   1 -
> >  .../Library/PlatformLib/ArmPlatformLib.inf    |  21 +---
> >  .../Library/PlatformLib/NxpQoriqLsMem.c       | 103 +++++++++---------
> >  Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf         |   6 -
> >  Silicon/NXP/Include/Chassis2/NxpSoc.h         |   2 +
> >  Silicon/NXP/LS1043A/Include/Soc.h             |  44 ++++++++
> >  Silicon/NXP/LS1043A/LS1043A.dsc.inc           |  32 ------
> >  Silicon/NXP/Library/SocLib/Chassis2/Soc.c     |   2 +-
> >  Silicon/NXP/Library/SocLib/LS1043aSocLib.inf  |   4 -
> >  Silicon/NXP/NxpQoriqLs.dec                    |  74 -------------
> >  12 files changed, 108 insertions(+), 204 deletions(-)  create mode
> > 100644 Silicon/NXP/LS1043A/Include/Soc.h
> >
> > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> > b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> > index f89dcdeff3..62c400eb1a 100644
> > --- a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> > +++ b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    LS1043 DXE platform driver.
> >
> > -  Copyright 2018-2019 NXP
> > +  Copyright 2018-2020 NXP
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -14,6 +14,7 @@
> >  #include <Library/PcdLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/UefiLib.h>
> > +#include <Soc.h>
> >
> >  #include <Protocol/NonDiscoverableDevice.h>
> >
> > @@ -22,7 +23,7 @@ typedef struct {
> >    UINT8 EndDesc;
> >  } ADDRESS_SPACE_DESCRIPTOR;
> >
> > -STATIC ADDRESS_SPACE_DESCRIPTOR mI2cDesc[FixedPcdGet64
> > (PcdNumI2cController)];
> > +STATIC ADDRESS_SPACE_DESCRIPTOR
> > +mI2cDesc[LS1043A_I2C_NUM_CONTROLLERS];
> 
> Sure, this one sounds like something not configurable in software or board
> design.
> 
> >
> >  STATIC
> >  EFI_STATUS
> > @@ -65,19 +66,19 @@ PopulateI2cInformation (  {
> >    UINT32 Index;
> >
> > -  for (Index = 0; Index < FixedPcdGet32 (PcdNumI2cController);
> > Index++) {
> > +  for (Index = 0; Index < ARRAY_SIZE (mI2cDesc); Index++) {
> >      mI2cDesc[Index].StartDesc.Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >      mI2cDesc[Index].StartDesc.Len = sizeof
> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> >      mI2cDesc[Index].StartDesc.ResType = ACPI_ADDRESS_SPACE_TYPE_MEM;
> >      mI2cDesc[Index].StartDesc.GenFlag = 0;
> >      mI2cDesc[Index].StartDesc.SpecificFlag = 0;
> >      mI2cDesc[Index].StartDesc.AddrSpaceGranularity = 32;
> > -    mI2cDesc[Index].StartDesc.AddrRangeMin = FixedPcdGet64
> (PcdI2c0BaseAddr) +
> > -                                             (Index * FixedPcdGet32 (PcdI2cSize));
> 
> As does this.
> 
> > +    mI2cDesc[Index].StartDesc.AddrRangeMin =
> LS1043A_I2C0_PHYS_ADDRESS +
> > +                                             (Index *
> > + LS1043A_I2C_SIZE);
> >      mI2cDesc[Index].StartDesc.AddrRangeMax =
> mI2cDesc[Index].StartDesc.AddrRangeMin +
> > -                                             FixedPcdGet32 (PcdI2cSize) - 1;
> > +                                             LS1043A_I2C_SIZE - 1;
> >      mI2cDesc[Index].StartDesc.AddrTranslationOffset = 0;
> > -    mI2cDesc[Index].StartDesc.AddrLen = FixedPcdGet32 (PcdI2cSize);
> > +    mI2cDesc[Index].StartDesc.AddrLen = LS1043A_I2C_SIZE;
> >
> >      mI2cDesc[Index].EndDesc = ACPI_END_TAG_DESCRIPTOR;
> >    }
> > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > index d689cf4db5..126a1174fa 100644
> > --- a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > +++ b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
> > @@ -2,7 +2,7 @@
> >  #
> >  #  Component description file for LS1043 DXE platform driver.
> >  #
> > -#  Copyright 2018-2019 NXP
> > +#  Copyright 2018-2020 NXP
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -25,6 +25,7 @@
> >    MdeModulePkg/MdeModulePkg.dec
> >    Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.dec
> >    Silicon/NXP/NxpQoriqLs.dec
> > +  Silicon/NXP/LS1043A/LS1043A.dec
> 
> Please insert sorted alphabetically.
> 
> >
> >  [LibraryClasses]
> >    BaseLib
> > @@ -43,10 +44,5 @@
> >    gEdkiiNonDiscoverableDeviceProtocolGuid        ## PRODUCES
> >    gDs1307RealTimeClockLibI2cMasterProtocolGuid   ## PRODUCES
> >
> > -[FixedPcd]
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController
> 
> Still good.
> 
> > -
> >  [Depex]
> >    TRUE
> > diff --git a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
> > b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
> > index 802cccdce6..74a1948fc6 100644
> > --- a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
> > +++ b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc
> > @@ -42,7 +42,6 @@
> >    #
> >    # Board Specific Pcds
> >    #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled|FALSE
> 
> But this sounds exactly like the kind of thing that would be configured differently
> for different boards. Can you please explain why this should be removed?

This PCD was defined because LS1043A (Chassis2 based SOC) has one Serdes.
Other Soc based on Chassis2 LS1046A, has two Serdes. So this Pcd is false for LS1043A
and true for LS1046A. Serdes block in Layerscape SOCs control which devices are enabled
and which devices are disabled ? e.g. (hypothetical example) with Serdes protocol 5,
out of 8 MACs, only 3 and 6 and enabled.

Going forward the plan is to move the Serdes handling
to SOC specific code and NOT to Chassis specific code. This is because the serdes
protocols are SOC specific and not chassis specific. i.e. both LS1046A and LS1043A can have
serdes protocol 5. But their meaning can be different for both

For the time being we have removed Serdes related code (in PATCH 07/19). We will introduce
It after PEI phase changes are merged.

> 
> >    gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv|0x1
> >
> >    #
> > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > index f7ae74afc6..054dc4d003 100644
> > ---
> > a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > +++ b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.in
> > +++ f
> > @@ -1,7 +1,7 @@
> >  #  @file
> >  #
> >  #  Copyright (c) 2016, Freescale Semiconductor, Inc. All rights reserved.
> > -#  Copyright 2017, 2019 NXP
> > +#  Copyright 2017, 2019-2020 NXP
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -20,6 +20,7 @@
> >    EmbeddedPkg/EmbeddedPkg.dec
> >    MdePkg/MdePkg.dec
> >    Silicon/NXP/NxpQoriqLs.dec
> > +  Silicon/NXP/LS1043A/LS1043A.dec
> 
> Insert sorted.
> 
> >
> >  [LibraryClasses]
> >    ArmLib
> > @@ -35,21 +36,3 @@
> >
> >  [FixedPcd]
> >    gArmTokenSpaceGuid.PcdArmPrimaryCore
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize
> > diff --git
> > a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > index c6c256da07..3a72c8bdd8 100644
> > --- a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > +++ b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > @@ -6,7 +6,7 @@
> >  *
> >  *  Copyright (c) 2011, ARM Limited. All rights reserved.
> >  *  Copyright (c) 2016, Freescale Semiconductor, Inc. All rights reserved.
> > -*  Copyright 2017, 2019 NXP
> > +*  Copyright 2017, 2019-2020 NXP
> >  *
> >  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  *
> > @@ -16,7 +16,7 @@
> >  #include <Library/DebugLib.h>
> >  #include <Library/PcdLib.h>
> >  #include <Library/MemoryAllocationLib.h> -#include <DramInfo.h>
> > +#include <Soc.h>
> >
> >  #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS          25
> >
> > @@ -38,7 +38,6 @@ ArmPlatformGetVirtualMemoryMap (  {
> >    UINTN                            Index;
> >    ARM_MEMORY_REGION_DESCRIPTOR     *VirtualMemoryTable;
> > -  DRAM_INFO                        DramInfo;
> >
> >    Index = 0;
> >
> > @@ -51,25 +50,21 @@ ArmPlatformGetVirtualMemoryMap (
> >      return;
> >    }
> >
> > -  if (GetDramBankInfo (&DramInfo)) {
> > -    DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n"));
> > -    return;
> > -  }
> > +  VirtualMemoryTable[Index].PhysicalBase =
> > + LS1043A_DRAM0_PHYS_ADDRESS;  VirtualMemoryTable[Index].VirtualBase
> = LS1043A_DRAM0_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_DRAM0_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >
> > -
> > -  for (Index = 0; Index < DramInfo.NumOfDrams; Index++) {
> > -    // DRAM1 (Must be 1st entry)
> > -    VirtualMemoryTable[Index].PhysicalBase =
> DramInfo.DramRegion[Index].BaseAddress;
> > -    VirtualMemoryTable[Index].VirtualBase  =
> DramInfo.DramRegion[Index].BaseAddress;
> > -    VirtualMemoryTable[Index].Length       = DramInfo.DramRegion[Index].Size;
> > -    VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> > -  }
> > +  VirtualMemoryTable[Index].PhysicalBase =
> > + LS1043A_DRAM1_PHYS_ADDRESS;  VirtualMemoryTable[Index].VirtualBase
> = LS1043A_DRAM1_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_DRAM1_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> 
> No changes to this file so far appear to have anything to do with what the
> commit message says the patch does.
> 
> >
> >    // CCSR Space
> > -  VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64
> > (PcdCcsrBaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64 (PcdCcsrBaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdCcsrSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_CCSR_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_CCSR_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_CCSR_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> 
> If these are not reconfigurable for different platforms, sure.

Yes. The SOC memory map is fixed in hardware. It doesn't change with Platform using SOC.

> 
> >
> >    // IFC region 1
> >    //
> > @@ -85,60 +80,60 @@ ArmPlatformGetVirtualMemoryMap (
> >    //             For write transactions from non-core masters (like system DMA),
> the address
> >    //                should be 16 byte aligned and the data size should be multiple of
> 16 bytes.
> >    //
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdIfcRegion1BaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdIfcRegion1BaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdIfcRegion1Size);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_IFC0_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_IFC0_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_IFC0_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >
> >    // QMAN SWP
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdQmanSwpBaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdQmanSwpBaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdQmanSwpSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> > +  VirtualMemoryTable[Index].PhysicalBase =
> > + LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].VirtualBase  =
> LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_QMAN_SW_PORTAL_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >    // BMAN SWP
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdBmanSwpBaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdBmanSwpBaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdBmanSwpSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> > +  VirtualMemoryTable[Index].PhysicalBase =
> > + LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].VirtualBase  =
> LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_QMAN_SW_PORTAL_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >    // IFC region 2
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdIfcRegion2BaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdIfcRegion2BaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdIfcRegion2Size);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_IFC1_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_IFC1_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_IFC1_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >    // PCIe1
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdPciExp1BaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdPciExp1BaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64
> (PcdPciExp1BaseSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI0_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_PCI0_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_PCI_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >    // PCIe2
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdPciExp2BaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdPciExp2BaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64
> (PcdPciExp2BaseSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI1_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_PCI1_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_PCI_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >    // PCIe3
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdPciExp3BaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdPciExp3BaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64
> (PcdPciExp3BaseSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI2_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_PCI2_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_PCI_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> 
> If these are not reconfigurable for different platforms, sure.
> >
> >    // QSPI region
> > -  VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64
> > (PcdQspiRegionBaseAddr);
> > -  VirtualMemoryTable[Index].VirtualBase  = FixedPcdGet64
> (PcdQspiRegionBaseAddr);
> > -  VirtualMemoryTable[Index].Length       = FixedPcdGet64 (PcdQspiRegionSize);
> > -  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> > +  VirtualMemoryTable[Index].PhysicalBase = LS1043A_QSPI_PHYS_ADDRESS;
> > + VirtualMemoryTable[Index].VirtualBase  = LS1043A_QSPI_PHYS_ADDRESS;
> > +  VirtualMemoryTable[Index].Length       = LS1043A_QSPI_SIZE;
> > +  VirtualMemoryTable[Index++].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> 
> If these are not reconfigurable for different platforms, sure.
> 
> >
> >    // End of Table
> > -  VirtualMemoryTable[++Index].PhysicalBase = 0;
> > +  VirtualMemoryTable[Index].PhysicalBase = 0;
> 
> Move that ++ back where it belongs - there is no functional change here.

I will remove this change and will keep functional changes only in this patch.

> 
> >    VirtualMemoryTable[Index].VirtualBase  = 0;
> >    VirtualMemoryTable[Index].Length       = 0;
> > -  VirtualMemoryTable[Index].Attributes   =
> (ARM_MEMORY_REGION_ATTRIBUTES)0;
> > +  VirtualMemoryTable[Index++].Attributes   =
> (ARM_MEMORY_REGION_ATTRIBUTES)0;
> 
> Move that ++ back where it belongs - there is no functional change here.
> 
> >
> > -  ASSERT ((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> > +  ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
> 
> Drop that +1 = - there is no functional change here.
> 
> If you want to submit separate patches for coding style changes, that's fine. But
> given that this set looks like it's making up for not synchronising internal
> development with the (very long) upstreaming effort so far, I am not interested
> in those until all functional changes are in.

I will remove this change and will keep functional changes only in this patch.

> 
> >
> >    *VirtualMemoryMap = VirtualMemoryTable;  } diff --git
> > a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > index 784139065f..fc4bb618fa 100644
> > --- a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> > @@ -49,11 +49,5 @@
> >    gEdkiiNonDiscoverableDeviceProtocolGuid    ## TO_START
> >    gEfiI2cMasterProtocolGuid                  ## BY_START
> >
> > -[Pcd]
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSize
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController
> > -
> >  [Depex]
> >    TRUE
> > diff --git a/Silicon/NXP/Include/Chassis2/NxpSoc.h
> > b/Silicon/NXP/Include/Chassis2/NxpSoc.h
> > index 74330b6205..6812beafe4 100644
> > --- a/Silicon/NXP/Include/Chassis2/NxpSoc.h
> > +++ b/Silicon/NXP/Include/Chassis2/NxpSoc.h
> > @@ -12,6 +12,8 @@
> >
> >  #define CLK_FREQ                   100000000
> >
> > +#define CHASSIS2_DCFG_ADDRESS      0x1EE0000
> > +
> >  /* SMMU Defintions */
> >  #define SMMU_BASE_ADDR             0x09000000
> >  #define SMMU_REG_SCR0              (SMMU_BASE_ADDR + 0x0)
> > diff --git a/Silicon/NXP/LS1043A/Include/Soc.h
> > b/Silicon/NXP/LS1043A/Include/Soc.h
> > new file mode 100644
> > index 0000000000..c1e00394af
> > --- /dev/null
> > +++ b/Silicon/NXP/LS1043A/Include/Soc.h
> > @@ -0,0 +1,44 @@
> > +/** @file
> > +
> > +  Copyright 2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +#ifndef __SOC_H__
> > +#define __SOC_H__
> > +
> > +/**
> > +  Soc Memory Map
> > +**/
> > +#define LS1043A_DRAM0_PHYS_ADDRESS   0x80000000
> > +#define LS1043A_DRAM0_SIZE           SIZE_2GB
> > +#define LS1043A_DRAM1_PHYS_ADDRESS   0x880000000
> > +#define LS1043A_DRAM1_SIZE           0x780000000 // 30 GB
> > +
> > +#define LS1043A_CCSR_PHYS_ADDRESS    0x1000000
> > +#define LS1043A_CCSR_SIZE            0xF000000
> > +
> > +#define LS1043A_IFC0_PHYS_ADDRESS    0x60000000
> > +#define LS1043A_IFC0_SIZE            SIZE_512MB
> > +#define LS1043A_IFC1_PHYS_ADDRESS    0x620000000
> > +#define LS1043A_IFC1_SIZE            0xE0000000 // 3.5 GB
> > +
> > +#define LS1043A_QSPI_PHYS_ADDRESS    0x40000000
> > +#define LS1043A_QSPI_SIZE            SIZE_512MB
> > +
> > +#define LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS    0x500000000
> > +#define LS1043A_QMAN_SW_PORTAL_SIZE            SIZE_128MB
> > +#define LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS    0x508000000
> > +#define LS1043A_BMAN_SW_PORTAL_SIZE            SIZE_128MB
> > +
> > +#define LS1043A_PCI0_PHYS_ADDRESS    0x4000000000
> > +#define LS1043A_PCI1_PHYS_ADDRESS    0x4800000000
> > +#define LS1043A_PCI2_PHYS_ADDRESS    0x5000000000
> > +#define LS1043A_PCI_SIZE             SIZE_32GB
> > +
> > +#define LS1043A_I2C0_PHYS_ADDRESS    0x2180000
> > +#define LS1043A_I2C_SIZE             0x10000
> > +#define LS1043A_I2C_NUM_CONTROLLERS  4
> > +
> > +#endif
> > diff --git a/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > b/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > index 754eff396a..7ebbb1a495 100644
> > --- a/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > +++ b/Silicon/NXP/LS1043A/LS1043A.dsc.inc
> > @@ -26,40 +26,8 @@
> >  [PcdsFixedAtBuild.common]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x021c0500
> >
> > -  #
> > -  # CCSR Address Space and other attached Memories
> > -  #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x01000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0F000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr|0x01EE1000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr|0x60000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size|0x20000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr|0x0620000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size|0x00E0000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcNandReservedSize|0x2EA
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0500000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0080000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0508000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize|0x0080000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr|0x4000000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize|0x800000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr|0x4800000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize|0x800000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr|0x5000000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize|0x800000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr|0x1570000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr|0x01EE0000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdWatchdog1BaseAddr|0x02AD0000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSdxcBaseAddr|0x01560000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr|0x02180000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSize|0x10000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController|4
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr|0x40000000
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize|0x20000000
> > -
> >    #
> >    # Big Endian IPs
> >    #
> >    gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian|TRUE
> > -  gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|TRUE
> >  ##
> > diff --git a/Silicon/NXP/Library/SocLib/Chassis2/Soc.c
> > b/Silicon/NXP/Library/SocLib/Chassis2/Soc.c
> > index 9baeb17ecf..a3dabc93d1 100644
> > --- a/Silicon/NXP/Library/SocLib/Chassis2/Soc.c
> > +++ b/Silicon/NXP/Library/SocLib/Chassis2/Soc.c
> > @@ -34,7 +34,7 @@ GetSysInfo (
> >    CCSR_GUR     *GurBase;
> >    UINTN        SysClk;
> >
> > -  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> > +  GurBase = (CCSR_GUR *)CHASSIS2_DCFG_ADDRESS;
> >    SysClk = CLK_FREQ;
> >
> >    SetMem (PtrSysInfo, sizeof (SYS_INFO), 0); diff --git
> > a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> > b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> > index fe77717337..d8707927b7 100644
> > --- a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> > +++ b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf
> > @@ -40,9 +40,5 @@
> >    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
> >    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> > -  gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr
> >    gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian
> > -  gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr
> >    gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv
> > -  gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled
> > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> > index 4a1cfb3e27..b478560450 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dec
> > +++ b/Silicon/NXP/NxpQoriqLs.dec
> > @@ -22,89 +22,15 @@
> >    gNxpNonDiscoverableI2cMasterGuid = { 0x5f2c099c, 0x54a3, 0x4dd4,
> > {0x9e, 0xc5, 0xe9, 0x12, 0x8c, 0x36, 0x81, 0x6a}}
> >
> >  [PcdsFixedAtBuild.common]
> > -  #
> > -  # Pcds for I2C Controller
> > -  #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed|0|UINT32|0x00000001
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController|0|UINT32|0x00000002
> > -
> > -  #
> > -  # Pcds for base address and size
> > -  #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr|0x0|UINT64|0x00000100
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPiFdSize|0x0|UINT32|0x00000101
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdPiFdBaseAddress|0x0|UINT64|0x00000102
> > -  gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr|0x0|UINT64|0x00000103
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdWatchdog1BaseAddr|0x0|UINT64|0x0000010
> 4
> > -  gNxpQoriqLsTokenSpaceGuid.PcdDdrBaseAddr|0x0|UINT64|0x00000105
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSdxcBaseAddr|0x0|UINT64|0x00000106
> > -  gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr|0x0|UINT64|0x00000107
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr|0x0|UINT64|0x00000108
> > -  gNxpQoriqLsTokenSpaceGuid.PcdI2cSize|0x0|UINT32|0x00000109
> > -  gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr|0x0|UINT64|0x0000010A
> > -  gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize|0x0|UINT64|0x0000010B
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x0|UINT32|0x0000010C
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x0|UINT32|0x0000010D
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0|UINT64|0x0000010E
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0|UINT64|0x0000010F
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0|UINT64|0x00000110
> > -  gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize|0x0|UINT64|0x00000111
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr|0x0|UINT64|0x00000112
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize|0x0|UINT64|0x00000113
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr|0x0|UINT64|0x00000114
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize|0x0|UINT64|0x00000115
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr|0x0|UINT64|0x00000116
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize|0x0|UINT64|0x00000117
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp4BaseAddr|0x0|UINT64|0x0000118
> > -  gNxpQoriqLsTokenSpaceGuid.PcdPciExp4BaseSize|0x0|UINT64|0x0000119
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr|0x0|UINT64|0x0000011
> A
> > -  gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize|0x0|UINT64|0x0000011B
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdQspiRegion2BaseAddr|0x0|UINT64|0x000001
> 1C
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdQspiRegion2Size|0x0|UINT64|0x0000011D
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdSystemMemoryExBase|0x0|UINT64|0x00000
> 11E
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdSystemMemoryExSize|0x0|UINT64|0x000001
> 1F
> > -  gNxpQoriqLsTokenSpaceGuid.PcdUsbBaseAddr|0x0|UINT32|0x00000120
> > -  gNxpQoriqLsTokenSpaceGuid.PcdUsbSize|0x0|UINT32|0x00000121
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x0|UINT64|0x00000122
> > -  gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0|UINT64|0x00000123
> > -
> > -  #
> > -  # IFC PCDs
> > -  #
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr|0x0|UINT64|0x0000019
> 0
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size|0x0|UINT64|0x00000191
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr|0x0|UINT64|0x0000019
> 2
> > -  gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size|0x0|UINT64|0x00000193
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdIfcNandReservedSize|0x0|UINT32|0x0000019
> 4
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdFlashDeviceBase64|0x0|UINT64|0x00000195
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdFlashReservedRegionBase64|0x0|UINT64|0x0
> 0
> > 000196
> > -
> > -  #
> > -  # NV Pcd
> > -  #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNvFdBase|0x0|UINT64|0x00000210
> > -  gNxpQoriqLsTokenSpaceGuid.PcdNvFdSize|0x0|UINT64|0x00000211
> 
> Not able to be different between different platforms using the same SoC?

This is a redundant Pcd. For Non Volatile Variables we already have PCDs like
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64

> 
> > -
> >    #
> >    # Platform PCDs
> >    #
> >    gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv|0x0|UINT32|0x00000250
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled|FALSE|BOOLEAN|0x0000025
> 1
> > -
> > -  #
> > -  # Clock PCDs
> > -  #
> > -  gNxpQoriqLsTokenSpaceGuid.PcdSysClk|0x0|UINT64|0x000002A0
> > -  gNxpQoriqLsTokenSpaceGuid.PcdDdrClk|0x0|UINT64|0x000002A1
> 
> And clocks also sound very much like something that can change per platform?

Yes Clocks can change. And also clocks are not fixed in all platforms. i.e. in most of the platforms based on the
switch settings, FPGA or Clock generator can be configured to provide different SYS clock / DDR Clock / SERDES clock etc.

Also we need the clock even when we don't have the support for PCD framework.
Which is why we have moved the clock functionality in ArmPlatformLib
Refer "NxpPlatformGetClock" in patch 14/19.

> 
> /
>     Leif
> 
> >
> >    #
> >    # Pcds to support Big Endian IPs
> >    #
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdMmcBigEndian|FALSE|BOOLEAN|0x0000310
> >
> gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian|FALSE|BOOLEAN|0x0000311
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x0000031
> 2
> > -
> >
> gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|FALSE|BOOLEAN|0x0000
> 031
> > 3
> > -
> gNxpQoriqLsTokenSpaceGuid.PcdIfcBigEndian|FALSE|BOOLEAN|0x00000314
> >
> >  [PcdsFeatureFlag]
> >
> >
> gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x0000
> 031
> > 5
> > --
> > 2.17.1
> >