Re: [PATCH edk2-platforms v2 1/2] SATA : Added SATA controller driver.

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 9 January 2018 at 04:50, Meenakshi Aggarwal
<meenakshi.aggarwal@nxp.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, January 08, 2018 8:35 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 1/2] SATA : Added SATA controller
>> driver.
>>
>> Hi Meenakshi,
>>
>> This is looking much better - thanks for rewriting it. I do have some
>> comments below
>>
>> On 8 January 2018 at 15:55, Meenakshi Aggarwal
>> <meenakshi.aggarwal@nxp.com> wrote:
>> > This patch adds support of SATA controller, which
>> > Initialize SATA controller,
>> > apply platform specific errata and
>> > Register itself as NonDiscoverableMmioDevice
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
>> > ---
>> >  Platform/NXP/Drivers/SataInitDxe/SataInit.c      | 285
>> +++++++++++++++++++++++
>> >  Platform/NXP/Drivers/SataInitDxe/SataInit.h      |  36 +++
>> >  Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf |  52 +++++
>> >  Platform/NXP/NxpQoriqLs.dec                      |  14 +-
>> >  Platform/NXP/NxpQoriqLs.dsc                      |  13 ++
>> >  5 files changed, 398 insertions(+), 2 deletions(-)
>> >  create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInit.c
>> >  create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInit.h
>> >  create mode 100644 Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
>> >
....
>> > +/**
>> > +  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;
>> > +  UINT32                   NumSataController;
>> > +  UINTN                    ControllerAddr;
>> > +
>> > +  Status = EFI_SUCCESS;
>> > +  NumSataController = PcdGet32 (PcdNumSataController);
>> > +
>> > +  //
>> > +  // Impact : The SATA controller does not detect some hard drives reliably
>> with
>> > +  // the default SerDes register setting.
>> > +  // Workaround : write value 0x80104e20 to 0x1eb1300 (serdes 2)
>> > +  //
>> > +  if (PcdGetBool (PcdSataErratumA010554)) {
>> > +    BeMmioWrite32 ((UINTN)SERDES2_SATA_ERRATA, 0x80104e20);
>> > +  }
>> > +
>> > +  //
>> > +  // Impact : Device may see false CRC errors causing unreliable SATA
>> operation.
>> > +  // Workaround : write 0x80000000 to the address 0x20140520 (dcsr).
>> > +  //
>> > +  if (PcdGetBool (PcdSataErratumA010635)) {
>> > +    BeMmioWrite32 ((UINTN)DCSR_SATA_ERRATA, 0x80000000);
>> > +  }
>> > +
>> > +  while (NumSataController) {
>> > +    NumSataController--;
>> > +    ControllerAddr = PcdGet32 (PcdSataBaseAddr) +
>> > +                     (NumSataController * PcdGet32 (PcdSataSize));
>> > +
>> > +    Status = RegisterNonDiscoverableMmioDevice (
>> > +               NonDiscoverableDeviceTypeAhci,
>> > +               NonDiscoverableDeviceDmaTypeNonCoherent,
>> > +               NULL,
>> > +               NULL,
>> > +               1,
>> > +               ControllerAddr, PcdGet32 (PcdSataSize)
>> > +             );
>> > +
>> > +    if (EFI_ERROR (Status)) {
>> > +      DEBUG ((DEBUG_ERROR, "Failed to register SATA device (0x%x) with
>> error 0x%x \n",
>> > +                           ControllerAddr, Status));
>>
>> Please don't use if/else for the expected path: instead, return here
>> or goto the error/unwind code at the end of the function
>>
> In case of more than one controller, we cannot return/goto from here because there are chances that other controller might get register successfully.
> So used if-else, please suggest the correct way.
>

Then use 'continue'

In any case, all RegisterNonDiscoverableMmioDevice() does is install a
protocol on a new handle, so it is unlikely to fail. You can just
replace the error handling with ASSERT_EFI_ERROR().


>> > +    } else {
>> > +      //
>> > +      // Register a protocol registration notification callback on the driver
>> > +      // binding protocol so we can attempt to connect to it as soon as it
>> appears.
>> > +      //
>> > +      EfiCreateProtocolNotifyEvent (
>> > +        &gEfiPciIoProtocolGuid,
>> > +        TPL_CALLBACK,
>> > +        PciIoRegistrationEvent,
>> > +        (VOID *)ControllerAddr,
>> > +        &mDriverEventRegistration);
>>
>> What is the point of this? AhciReadReg()/AhciWriteReg() can access
>> ControllerAddr directly, so there is no reason to go through the PCI
>> I/O protocol.
>>
> OK, i will check this.
>> > +    }
>> > +  }
>> > +
>> > +  return Status;
>> > +}
>> > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.h
>> b/Platform/NXP/Drivers/SataInitDxe/SataInit.h
>> > new file mode 100644
>> > index 0000000..7fe6273
>> > --- /dev/null
>> > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.h
>> > @@ -0,0 +1,36 @@
>> > +/** @file
>> > +  Header file for Sata Controller initialization driver.
>> > +
>> > +  Copyright 2017 NXP
>> > +
>> > +  This program and the accompanying materials
>> > +  are licensed and made available under the terms and conditions of the
>> BSD License
>> > +  which accompanies this distribution. The full text of the license may be
>> found
>> > +
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
>> nsource.org%2Flicenses%2Fbsd-
>> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C1bf888
>> dbc6b34f8646fe08d556a93d5a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
>> %7C0%7C636510207254570536&sdata=hU2o5igZuy5SDt5emEUmAqhSn1gW9
>> H40OgvmH8gMn9k%3D&reserved=0
>> > +
>> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> BASIS,
>> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>> EXPRESS OR IMPLIED.
>> > +
>> > + **/
>> > +
>> > +#ifndef _SATA_INIT_H_
>> > +#define _SATA_INIT_H_
>> > +
>> > +
>> > +#define AHCI_BAR_INDEX         0x05
>> > +//
>> > +// Offset for AHCI base address in PCI Header
>> > +//
>> > +#define PCI_AHCI_BASE_ADDRESS  0x24
>> > +
>> > +#define SATA_PPCFG             0xA8
>> > +#define SATA_PTC               0xC8
>> > +
>> > +#define PORT_PHYSICAL          0xA003FFFE
>> > +#define PORT_TRANSPORT         0x08000025
>> > +#define PORT_RXWM              0x08000029
>> > +
>> > +#define DCSR_SATA_ERRATA       0x20140520
>> > +#define SERDES2_SATA_ERRATA    0x01eb1300
>> > +
>> > +#endif
>> > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
>> b/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
>> > new file mode 100644
>> > index 0000000..82535f4
>> > --- /dev/null
>> > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInitDxe.inf
>> > @@ -0,0 +1,52 @@
>> > +## @file
>> > +#  Component description file for the Sata Controller initialization driver
>> > +#
>> > +#  Copyright 2017 NXP
>> > +#
>> > +#  This program and the accompanying materials
>> > +#  are licensed and made available under the terms and conditions of the
>> BSD License
>> > +#  which accompanies this distribution. The full text of the license may be
>> found
>> > +#
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fope
>> nsource.org%2Flicenses%2Fbsd-
>> license.php&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C1bf888
>> dbc6b34f8646fe08d556a93d5a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
>> %7C0%7C636510207254570536&sdata=hU2o5igZuy5SDt5emEUmAqhSn1gW9
>> H40OgvmH8gMn9k%3D&reserved=0
>> > +#
>> > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> BASIS,
>> > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
>> EXPRESS OR IMPLIED.
>> > +#
>> > +##
>> > +
>> > +[Defines]
>> > +  INF_VERSION                    = 0x0001000A
>> > +  BASE_NAME                      = SataInit
>> > +  FILE_GUID                      = 021722D8-522B-4079-852A-FE44C2C13F49
>> > +  MODULE_TYPE                    = DXE_DRIVER
>> > +  VERSION_STRING                 = 1.0
>> > +  ENTRY_POINT                    = InitializeSataController
>> > +
>> > +[Sources]
>> > +  SataInit.c
>> > +
>> > +[Packages]
>> > +  MdePkg/MdePkg.dec
>> > +  MdeModulePkg/MdeModulePkg.dec
>> > +  Platform/NXP/NxpQoriqLs.dec
>> > +
>> > +[LibraryClasses]
>> > +  BeIoLib
>> > +  DebugLib
>> > +  NonDiscoverableDeviceRegistrationLib
>> > +  UefiBootServicesTableLib
>> > +  UefiDriverEntryPoint
>> > +  UefiLib
>> > +
>> > +[FixedPcd]
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdNumSataController
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataSize
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635
>> > +
>> > +[Protocols]
>> > +  gEfiPciIoProtocolGuid
>> > +
>> > +[Depex]
>> > +  TRUE
>> > diff --git a/Platform/NXP/NxpQoriqLs.dec b/Platform/NXP/NxpQoriqLs.dec
>> > index bd4273f..65d659e 100644
>> > --- a/Platform/NXP/NxpQoriqLs.dec
>> > +++ b/Platform/NXP/NxpQoriqLs.dec
>> > @@ -52,8 +52,8 @@
>> >    gNxpQoriqLsTokenSpaceGuid.PcdI2c1BaseAddr|0|UINT64|0x0000010E
>> >    gNxpQoriqLsTokenSpaceGuid.PcdI2c2BaseAddr|0|UINT64|0x0000010F
>> >    gNxpQoriqLsTokenSpaceGuid.PcdI2c3BaseAddr|0|UINT64|0x00000110
>> > -
>> gNxpQoriqLsTokenSpaceGuid.PcdSataController1BaseAddress|0x0|UINT32|
>> 0x00000111
>> > -
>> gNxpQoriqLsTokenSpaceGuid.PcdSataController2BaseAddress|0x0|UINT32|
>> 0x00000112
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x0|UINT32|0x00000111
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x0|UINT32|0x00000112
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0500000000|UINT6
>> 4|0x00000113
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0080000000|UINT64|0x0
>> 0000114
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0508000000|UINT64
>> |0x00000115
>> > @@ -83,6 +83,8 @@
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x01000000|UINT64|0x0000
>> 012D
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0F000000|UINT64|0x0000012E
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdDramMemSize|0x0|UINT64|0x0000012F
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr|0x0|UINT64|0x00000130
>> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize|0x0|UINT64|0x00000131
>> >
>> >    #
>> >    # DSPI Pcds
>> > @@ -156,6 +158,9 @@
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdErratumA008514|FALSE|BOOLEAN|0x0000
>> 0275
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdErratumA008336|FALSE|BOOLEAN|0x0000
>> 0276
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA009185|FALSE|BOOLEAN|0x
>> 00000277
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010554|FALSE|BOOLEAN|0x
>> 00000278
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA010635|FALSE|BOOLEAN|0x
>> 00000279
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdSataErratumA008402|FALSE|BOOLEAN|0x
>> 0000027A
>> >
>> >    #
>> >    # Test PCDs
>> > @@ -249,3 +254,8 @@
>> >    #
>> >    gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cBus|0|UINT32|0x0000330
>> >
>> gNxpQoriqLsTokenSpaceGuid.PcdSysEepromI2cAddress|0|UINT32|0x00003
>> 31
>> > +
>> > +  #
>> > +  # SATA Pcds
>> > +  #
>> > +
>> gNxpQoriqLsTokenSpaceGuid.PcdNumSataController|0x0|UINT32|0x000003
>> 40
>> > diff --git a/Platform/NXP/NxpQoriqLs.dsc b/Platform/NXP/NxpQoriqLs.dsc
>> > index 10eff06..c3c0eb1 100644
>> > --- a/Platform/NXP/NxpQoriqLs.dsc
>> > +++ b/Platform/NXP/NxpQoriqLs.dsc
>> > @@ -99,6 +99,8 @@
>> >    VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>> >    NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
>> >
>> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.i
>> nf
>> > +  UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>> > +
>> NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscove
>> rableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
>> >
>> >  [LibraryClasses.common.SEC]
>> >    PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
>> > @@ -144,6 +146,7 @@
>> >
>> SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementL
>> ib/DxeSecurityManagementLib.inf
>> >
>> PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerforma
>> nceLib.inf
>> >    MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
>> > +
>> NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscove
>> rableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
>> >
>>
>> Why do you need to add this twice?
> My fault, will update.
>>
>> >  [LibraryClasses.common.UEFI_APPLICATION]
>> >    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
>> > @@ -334,6 +337,16 @@
>> >    }
>> >
>> >    #
>> > +  # 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
>> > +
>> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
>> DeviceDxe.inf
>> > +
>> > +  #
>> >    # Architectural Protocols
>> >    #
>> >    ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>> > --
>> > 1.9.1
>> >