Hi Ard,
Reply inline.
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheu...@arm.com>
> Sent: Friday, June 5, 2020 6:30 PM
> To: Meenakshi Aggarwal (OSS) <meenakshi.aggar...@oss.nxp.com>;
> l...@nuviainc.com; michael.d.kin...@intel.com; devel@edk2.groups.io
> Cc: Varun Sethi <v.se...@nxp.com>; Meenakshi Aggarwal
> <meenakshi.aggar...@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.aggar...@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;
> > +}
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60860): https://edk2.groups.io/g/devel/message/60860
Mute This Topic: https://groups.io/mt/74690750/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
