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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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|0x00000315
>     gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
>     gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x00000317
> +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|FALSE|BOOLEAN|0x00000318
>   
>   [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|0x00000600
>     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/NonDiscoverablePciDeviceDxe.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.

> +  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?

> +
> +#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?

> +#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?

> +#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

> +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;
> +}
>