Re: [PATCH edk2-platforms v2 2/2] LS1046 : Enable support of SATA controller

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, January 08, 2018 8:42 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms v2 2/2] LS1046 : Enable support of SATA
> controller
> 
> On 8 January 2018 at 15:55, Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com> wrote:
> > Enable support of SATA drives on ls1046 board.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > ---
> >  Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dsc                 |  8 ++++++++
> >  Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.fdf                 | 12
> ++++++++++++
> >  .../NXP/LS1046aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf |  2 ++
> >  .../NXP/LS1046aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c    |  8
> ++++++++
> >  Silicon/NXP/LS1046A/LS1046A.dsc                              |  5 +++++
> >  5 files changed, 35 insertions(+)
> >
> > diff --git a/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dsc
> b/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dsc
> > index 9d2482b..93fc848 100644
> > --- a/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dsc
> > +++ b/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dsc
> > @@ -63,6 +63,13 @@
> >    #
> >    gNxpQoriqLsTokenSpaceGuid.PcdI2cSlaveAddress|0x51
> >
> > +  #
> > +  # Errata Pcds
> > +  #
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|TRUE
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554|TRUE
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635|TRUE
> > +
> >
> ##########################################################
> ######################
> >  #
> >  # Components Section - list of all EDK II Modules needed by this Platform
> > @@ -71,3 +78,4 @@
> >  [Components.common]
> >    edk2-platforms/Platform/NXP/Drivers/WatchDog/WatchDogDxe.inf
> >    edk2-platforms/Platform/NXP/Drivers/I2cDxe/I2cDxe.inf
> > +  edk2-platforms/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
> 
> This looks wrong to me. Your .dsc/.fdf files should not contain these
> edk2-platforms prefixes. Instead, you should set your PACKAGES_PATH
> correctly to include your edk2-platforms directory.
> 
OK, We will remove this from .dsc/.fdf files.
My concern is as there are already a lot of patches are under review so it will be 
Better if review gets completed once, then we will share the updated in next revision of patch
As this needs to be change in multiple patches.

There is one more comment from you on keeping shred Drivers and Library in Silicon/NXP directory.
In this case also, this will need a rework in all patches sent till date.

So once review comments been recieved we will made the changes in next revision of patch.

> > diff --git a/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.fdf
> b/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.fdf
> > index 169cef0..23b46ad 100644
> > --- a/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.fdf
> > +++ b/Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.fdf
> > @@ -142,6 +142,18 @@ READ_LOCK_STATUS   = TRUE
> >
> >    INF
> MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariableRuntime
> Dxe.inf
> >
> > +  #
> > +  # AHCI Support
> > +  #
> > +  INF MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > +  INF MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
> > +  INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > +  INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > +  INF MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> > +  INF
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
> DeviceDxe.inf
> > +
> > +  INF edk2-platforms/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
> > +
> 
> Same here
> 
> >    # FAT filesystem + GPT/MBR partitioning
> >    #
> >    INF
> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
> > diff --git
> a/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> b/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > index 13a0ffb..002294e 100644
> > ---
> a/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > +++
> b/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf
> > @@ -68,3 +68,5 @@
> >    gNxpQoriqLsTokenSpaceGuid.PcdDram3Size
> >    gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr
> >    gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize
> > diff --git
> a/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> b/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > index 7022528..4b04ff5 100644
> > --- a/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > +++
> b/Platform/NXP/LS1046aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c
> > @@ -49,6 +49,8 @@
> >  #define DRAM3_SIZE                FixedPcdGet64 (PcdDram3Size)
> >  #define QSPI_REGION_BASE_ADDR     FixedPcdGet64
> (PcdQspiRegionBaseAddr)
> >  #define QSPI_REGION_SIZE          FixedPcdGet64 (PcdQspiRegionSize)
> > +#define DCSR_BASE_ADDR            FixedPcdGet64 (PcdDcsrBaseAddr)
> > +#define DCSR_SIZE                 FixedPcdGet64 (PcdDcsrSize)
> >
> >
> >  /**
> > @@ -169,6 +171,12 @@ ArmPlatformGetVirtualMemoryMap (
> >    VirtualMemoryTable[Index].Length       = QSPI_REGION_SIZE;
> >    VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >
> > +  // DCSR Space
> > +  VirtualMemoryTable[++Index].PhysicalBase = DCSR_BASE_ADDR;
> > +  VirtualMemoryTable[Index].VirtualBase  = DCSR_BASE_ADDR;
> > +  VirtualMemoryTable[Index].Length       = DCSR_SIZE;
> > +  VirtualMemoryTable[Index].Attributes   =
> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> > +
> >    // End of Table
> >    VirtualMemoryTable[++Index].PhysicalBase = 0;
> >    VirtualMemoryTable[Index].VirtualBase  = 0;
> > diff --git a/Silicon/NXP/LS1046A/LS1046A.dsc
> b/Silicon/NXP/LS1046A/LS1046A.dsc
> > index 4e7230a..33c57ad 100644
> > --- a/Silicon/NXP/LS1046A/LS1046A.dsc
> > +++ b/Silicon/NXP/LS1046A/LS1046A.dsc
> > @@ -74,5 +74,10 @@
> >    gNxpQoriqLsTokenSpaceGuid.PcdI2c2BaseAddr|0x021A0000
> >    gNxpQoriqLsTokenSpaceGuid.PcdI2c3BaseAddr|0x021B0000
> >    gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController|4
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr|0x20000000
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize|0x04000000
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x3200000
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x10000
> > +  gNxpQoriqLsTokenSpaceGuid.PcdNumSataController|0x1
> >
> >  ##
> > --
> > 1.9.1
> >