Re: [PATCH edk2-platforms 1/2] Silicon/NXP: Add SATA controller initialization driver

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

Hi Ard,

Reply inline.

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Friday, June 5, 2020 6:30 PM
> To: Meenakshi Aggarwal (OSS) <meenakshi.aggarwal@oss.nxp.com>;
> leif@nuviainc.com; michael.d.kinney@intel.com; devel@edk2.groups.io
> Cc: Varun Sethi <V.Sethi@nxp.com>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>
> Subject: Re: [PATCH edk2-platforms 1/2] Silicon/NXP: Add SATA controller
> initialization driver
> 
> On 6/5/20 6:02 PM, Meenakshi Aggarwal wrote:
> > Add support of SATA controller driver which performs controller
> > initialization and register itself as NonDiscoverableMmioDevice
> >
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > ---
> >   Silicon/NXP/NxpQoriqLs.dec                      |   6 +
> >   Silicon/NXP/NxpQoriqLs.dsc.inc                  |  10 ++
> >   Silicon/NXP/Drivers/SataInitDxe/SataInitDxe.inf |  46 ++++++++
> >   Silicon/NXP/Drivers/SataInitDxe/SataInit.h      |  27 +++++
> >   Silicon/NXP/Drivers/SataInitDxe/SataInit.c      | 116 ++++++++++++++++++++
> >   5 files changed, 205 insertions(+)
> >
> > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> > index 72c1744fc934..e83f282ace4e 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dec
> > +++ b/Silicon/NXP/NxpQoriqLs.dec
> > @@ -28,6 +28,7 @@ [PcdsFeatureFlag]
> >
> gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x0000
> 0315
> >
> gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
> >
> >
> gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x0000031
> 7
> > +
> > +
> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|FALSE|BOOLEAN|0x000
> 0
> > + 0318
> >
> >   [PcdsFixedAtBuild.common]
> >     # Pcds for PCI Express
> > @@ -41,6 +42,11 @@ [PcdsFixedAtBuild.common]
> >     gNxpQoriqLsTokenSpaceGuid.PcdUsbSize|0x0|UINT32|0x00000511
> >
> gNxpQoriqLsTokenSpaceGuid.PcdNumUsbController|0|UINT32|0x00000512
> >
> > +  # Pcds for SATA
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x0|UINT64|0x00000350
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x0|UINT32|0x00000351
> > +
> > +
> gNxpQoriqLsTokenSpaceGuid.PcdNumSataController|0x0|UINT32|0x00000352
> > +
> >   [PcdsDynamic.common]
> >
> gNxpQoriqLsTokenSpaceGuid.PcdPciCfgShiftEnable|FALSE|BOOLEAN|0x000006
> 00
> >
> > gNxpQoriqLsTokenSpaceGuid.PcdPciLsGen4Ctrl|FALSE|BOOLEAN|0x00000601
> > diff --git a/Silicon/NXP/NxpQoriqLs.dsc.inc
> > b/Silicon/NXP/NxpQoriqLs.dsc.inc index 1f0f272da6c7..399ac5de9176
> > 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dsc.inc
> > +++ b/Silicon/NXP/NxpQoriqLs.dsc.inc
> > @@ -101,6 +101,7 @@ [LibraryClasses.common]
> >
> >     PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
> >
> > MemoryInitPeiLib|Silicon/NXP/Library/MemoryInitPeiLib/MemoryInitPeiLib
> > .inf
> > +  UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
> >
> >   [LibraryClasses.common.SEC]
> >     PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> > @@ -378,6 +379,15 @@ [Components.common]
> >
> >
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDev
> > iceDxe.inf
> >
> >     #
> > +  # AHCI Support
> > +  #
> > +  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > +  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
> > +  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > +  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > +  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
> > +
> > +  #
> >     # Bds
> >     #
> >     MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> > diff --git a/Silicon/NXP/Drivers/SataInitDxe/SataInitDxe.inf
> > b/Silicon/NXP/Drivers/SataInitDxe/SataInitDxe.inf
> > new file mode 100644
> > index 000000000000..06b6c2ac6ff9
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/SataInitDxe/SataInitDxe.inf
> > @@ -0,0 +1,46 @@
> > +## @file
> > +#  Component description file for the Sata Controller initialization
> > +driver # #  Copyright 2017 NXP # #  SPDX-License-Identifier:
> > +BSD-2-Clause-Patent # ##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001A
> > +  BASE_NAME                      = SataInit
> > +  FILE_GUID                      = 021722D8-522B-4079-852A-FE44C2C13F49
> 
> Please don't reuse file GUIDs  but generate your own.
> 
Missed it, will replace

> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = InitializeSataController
> > +
> > +[Sources]
> > +  SataInit.c
> > +  SataInit.h
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  DebugLib
> > +  NonDiscoverableDeviceRegistrationLib
> > +  UefiBootServicesTableLib
> > +  UefiDriverEntryPoint
> > +  UefiLib
> > +
> > +[FixedPcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdNumSataController
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataSize
> > +
> > +[FeaturePcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185
> > +
> > +[Guids]
> > +  gEfiEndOfDxeEventGroupGuid
> > +
> > +[Depex]
> > +  TRUE
> > diff --git a/Silicon/NXP/Drivers/SataInitDxe/SataInit.h
> > b/Silicon/NXP/Drivers/SataInitDxe/SataInit.h
> > new file mode 100644
> > index 000000000000..edaf0bdda72c
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/SataInitDxe/SataInit.h
> > @@ -0,0 +1,27 @@
> > +/** @file
> > +  Header file for Sata Controller initialization driver.
> > +
> > +  Copyright 2017-2018, 2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > + **/
> > +
> > +#ifndef SATA_INIT_H_
> > +#define SATA_INIT_H_
> > +
> > +//
> > +// Offset for AHCI base address in PCI Header // #define
> > +PCI_AHCI_BASE_ADDRESS  0x24
> 
> Where is this used?
Missed during cleanup, will delete
> 
> > +
> > +#define SATA_PPCFG             0xA8
> > +#define SATA_PTC               0xC8
> > +#define SATA_PAXIC             0xC0
> > +
> > +#define PORT_PHYSICAL          0xA003FFFE
> > +#define PORT_TRANSPORT         0x08000025
> > +#define PORT_RXWM              0x08000029
> > +#define ENABLE_NONZERO_4MB_PRD 0x10000000
> > +
> 
> Do these really need a separate local header?
Hmm, we can keep in ".c" file
> 
> > +#endif
> > diff --git a/Silicon/NXP/Drivers/SataInitDxe/SataInit.c
> > b/Silicon/NXP/Drivers/SataInitDxe/SataInit.c
> > new file mode 100644
> > index 000000000000..c678b63652e7
> > --- /dev/null
> > +++ b/Silicon/NXP/Drivers/SataInitDxe/SataInit.c
> > @@ -0,0 +1,116 @@
> > +/** @file
> > +  This driver module adds SATA controller support.
> > +
> > +  Copyright 2017-2018, 2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > + **/
> > +
> 
> No need to #include <PiDxe.h> here?
I am not seeing  any error without this
> 
> > +#include <Library/DebugLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/NonDiscoverableDeviceRegistrationLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +
> > +#include "SataInit.h"
> > +
> > +/**
> > +  This function gets registered as a callback to perform USB
> > +controller intialization
> > +
> > +  @param  Event         Event whose notification function is being invoked.
> > +  @param  Context       Pointer to the notification function's context.
> > +
> > +**/
> 
> STATIC
ok
> 
> > +VOID
> > +EFIAPI
> > +SataEndOfDxeCallback (
> > +  IN EFI_EVENT  Event,
> > +  IN VOID       *Context
> > +  )
> > +{
> > +  EFI_STATUS               Status;
> > +  UINT32                   NumSataController;
> > +  UINT32                   Index;
> > +  UINT32                   Data;
> > +  UINTN                    ControllerAddr;
> > +
> > +  NumSataController = PcdGet32 (PcdNumSataController);
> > +
> > +  for (Index = 0; Index < NumSataController; Index++) {
> > +    ControllerAddr = PcdGet64 (PcdSataBaseAddr) +
> > +                     (Index * PcdGet32 (PcdSataSize));
> > +
> > +    //
> > +    // configuring Physical Control Layer parameters for Port 0
> > +    //
> > +    MmioWrite32 ((UINTN)(ControllerAddr + SATA_PPCFG),
> > + PORT_PHYSICAL);
> > +
> > +    //
> > +    // This register controls the configuration of the
> > +    // Transport Layer for  Port 0
> > +    // Errata Description : The default Rx watermark value may be insufficient
> > +    // for some hard drives and result in a false CRC or internal errors.
> > +    // Workaround: Change PTC[RXWM] field at offset 0xC8 to 0x29. Do not
> change
> > +    // the other reserved fields of the register.
> > +    //
> > +
> > +    Data = MmioRead32 ((UINTN)(ControllerAddr + SATA_PTC));
> > +    if (PcdGetBool (PcdSataErratumA009185)) {
> > +      Data |= PORT_RXWM;
> > +    } else {
> > +      Data |= PORT_TRANSPORT;
> > +    }
> > +    MmioWrite32 ((UINTN)(ControllerAddr + SATA_PTC), Data);
> > +
> > +    //
> > +    // Enable Non-Zero 4 MB PRD entries.
> > +    //
> > +    MmioOr32 ((UINTN)(ControllerAddr + SATA_PAXIC),
> > + ENABLE_NONZERO_4MB_PRD);
> > +
> > +    Status = RegisterNonDiscoverableMmioDevice (
> > +               NonDiscoverableDeviceTypeAhci,
> > +               NonDiscoverableDeviceDmaTypeNonCoherent,
> > +               NULL,
> > +               NULL,
> > +               1,
> > +               ControllerAddr, PcdGet32 (PcdSataSize)
> > +               );
> > +
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "Failed to register SATA device 0x%x, error 0x%r
> \n",
> > +        ControllerAddr, Status));
> > +    }
> > +  }
> > +}
> > +
> > +/**
> > +  The Entry Point of module. It follows the standard UEFI driver model.
> > +
> > +  @param[in] ImageHandle       The firmware allocated handle for the EFI
> image.
> > +  @param[in] SystemTable       A pointer to the EFI System Table.
> > +
> > +  @retval EFI_SUCCESS          The entry point is executed successfully.
> > +  @retval Other                Some error occurs when executing this entry point
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +InitializeSataController (
> > +  IN EFI_HANDLE            ImageHandle,
> > +  IN EFI_SYSTEM_TABLE      *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS               Status;
> > +  EFI_EVENT                EndOfDxeEvent;
> > +
> > +  Status = gBS->CreateEventEx (
> > +                  EVT_NOTIFY_SIGNAL,
> > +                  TPL_CALLBACK,
> > +                  SataEndOfDxeCallback,
> > +                  NULL,
> > +                  &gEfiEndOfDxeEventGroupGuid,
> > +                  &EndOfDxeEvent
> > +                  );
> > +
> > +  return Status;
> > +}
> >