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

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

2018-01-09 Thread Ard Biesheuvel

On 9 January 2018 at 04:50, Meenakshi Aggarwal
 wrote:
>
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Monday, January 08, 2018 8:35 PM
>> To: Meenakshi Aggarwal 
>> Cc: Leif Lindholm ; Kinney, Michael D
>> ; edk2-devel@lists.01.org; Udit Kumar
>> ; Varun Sethi 
>> 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
>>  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 
>> > ---
>> >  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 otherSome error occurs when executing this entry 
>> > point.
>> > +
>> > +**/
>> > +EFI_STATUS
>> > +EFIAPI
>> > +InitializeSataController (
>> > +  IN EFI_HANDLEImageHandle,
>> > +  IN EFI_SYSTEM_TABLE  *SystemTable
>> > +  )
>> > +{
>> > +  EFI_STATUS   Status;
>> > +  UINT32   NumSataController;
>> > +  UINTNControllerAddr;
>> > +
>> > +  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 0x8000 to the address 0x20140520 (dcsr).
>> > +  //
>> > +  if (PcdGetBool (PcdSataErratumA010635)) {
>> > +BeMmioWrite32 ((UINTN)DCSR_SATA_ERRATA, 0x8000);
>> > +  }
>> > +
>> > +  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 (
>> > +,
>> > +TPL_CALLBACK,
>> > +PciIoRegistrationEvent,
>> > +(VOID *)ControllerAddr,
>> > +);
>>
>> What is the point of this?

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

2018-01-08 Thread Meenakshi Aggarwal



> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, January 08, 2018 8:35 PM
> To: Meenakshi Aggarwal 
> Cc: Leif Lindholm ; Kinney, Michael D
> ; edk2-devel@lists.01.org; Udit Kumar
> ; Varun Sethi 
> 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
>  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 
> > ---
> >  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
> >
> > diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.c
> b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
> > new file mode 100644
> > index 000..bac390b
> > --- /dev/null
> > +++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
> > @@ -0,0 +1,285 @@
> > +/** @file
> > +  This driver module adds SATA controller support.
> > +
> > +  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=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C1bf888
> dbc6b34f8646fe08d556a93d5a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636510207254570536=hU2o5igZuy5SDt5emEUmAqhSn1gW9
> H40OgvmH8gMn9k%3D=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.
> > +
> > + **/
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "SataInit.h"
> > +
> > +STATIC VOID*mDriverEventRegistration;
> > +
> > +/**
> > +  Read AHCI Operation register.
> > +
> > +  @param  PciIoThe PCI IO protocol instance.
> > +  @param  Offset   The operation register offset.
> > +
> > +  @return  The register content read.
> > +**/
> > +
> > +UINT32
> > +EFIAPI
> > +AhciReadReg (
> > +  IN  EFI_PCI_IO_PROTOCOL  *PciIo,
> > +  IN  UINT32   Offset
> > +  )
> > +{
> > +  UINT32   Data;
> > +
> > +  ASSERT (PciIo != NULL);
> > +
> > +  Data = 0;
> > +
> > +  PciIo->Mem.Read (
> > +  PciIo,
> > +  EfiPciIoWidthUint32,
> > +  AHCI_BAR_INDEX,
> > +  (UINT64) Offset,
> > +  1,
> > +  
> > +  );
> > +
> > +  return Data;
> > +}
> > +
> > +/**
> > +  Write AHCI Operation register.
> > +
> > +  @param PciIo The PCI IO protocol instance.
> > +  @param OffsetThe operation register offset.
> > +  @param Data  The data used to write down.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +AhciWriteReg (
> > +  IN EFI_PCI_IO_PROTOCOL   *PciIo,
> > +  IN UINT32Offset,
> > +  IN UINT32Data
> > +  )
> > +{
> > +  ASSERT (PciIo != NULL);
> > +
> > +  PciIo->Mem.Write (
> > +   PciIo,
> > +   EfiPciIoWidthUint32,
> > +   AHCI_BAR_INDEX,
> > +   (UINT64) Offset,
> > +   1,
> > +   
> > +   );
> > +
> > +  return;
> > +}
> > +
> > +STATIC
> > +VOID
> > +PciIoRegistrationEvent (
> > +  IN  EFI_EVENTEvent,
> > +  IN  VOID *Context
> > +  )
> > +{
> > +  EFI_STATUS   Status;
> > +  UINTNHandleCount;
> > +  UINTNAddress;
> > +  UINT32   Count;
> > +  UINT32   Data;
> > +  UINT8PciClass;
> > +  UINT8PciSubClass;
> > +  EFI_PCI_IO_PROTOCOL  *PciIo;
> > +  EFI_HANDLE   *HandleBuf;
> > +
> > +  PciIo =

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

2018-01-08 Thread Ard Biesheuvel

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
 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 
> ---
>  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
>
> diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.c 
> b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
> new file mode 100644
> index 000..bac390b
> --- /dev/null
> +++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
> @@ -0,0 +1,285 @@
> +/** @file
> +  This driver module adds SATA controller support.
> +
> +  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
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> + **/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "SataInit.h"
> +
> +STATIC VOID*mDriverEventRegistration;
> +
> +/**
> +  Read AHCI Operation register.
> +
> +  @param  PciIoThe PCI IO protocol instance.
> +  @param  Offset   The operation register offset.
> +
> +  @return  The register content read.
> +**/
> +
> +UINT32
> +EFIAPI
> +AhciReadReg (
> +  IN  EFI_PCI_IO_PROTOCOL  *PciIo,
> +  IN  UINT32   Offset
> +  )
> +{
> +  UINT32   Data;
> +
> +  ASSERT (PciIo != NULL);
> +
> +  Data = 0;
> +
> +  PciIo->Mem.Read (
> +  PciIo,
> +  EfiPciIoWidthUint32,
> +  AHCI_BAR_INDEX,
> +  (UINT64) Offset,
> +  1,
> +  
> +  );
> +
> +  return Data;
> +}
> +
> +/**
> +  Write AHCI Operation register.
> +
> +  @param PciIo The PCI IO protocol instance.
> +  @param OffsetThe operation register offset.
> +  @param Data  The data used to write down.
> +
> +**/
> +VOID
> +EFIAPI
> +AhciWriteReg (
> +  IN EFI_PCI_IO_PROTOCOL   *PciIo,
> +  IN UINT32Offset,
> +  IN UINT32Data
> +  )
> +{
> +  ASSERT (PciIo != NULL);
> +
> +  PciIo->Mem.Write (
> +   PciIo,
> +   EfiPciIoWidthUint32,
> +   AHCI_BAR_INDEX,
> +   (UINT64) Offset,
> +   1,
> +   
> +   );
> +
> +  return;
> +}
> +
> +STATIC
> +VOID
> +PciIoRegistrationEvent (
> +  IN  EFI_EVENTEvent,
> +  IN  VOID *Context
> +  )
> +{
> +  EFI_STATUS   Status;
> +  UINTNHandleCount;
> +  UINTNAddress;
> +  UINT32   Count;
> +  UINT32   Data;
> +  UINT8PciClass;
> +  UINT8PciSubClass;
> +  EFI_PCI_IO_PROTOCOL  *PciIo;
> +  EFI_HANDLE   *HandleBuf;
> +
> +  PciIo = NULL;
> +
> +  Status = gBS->LocateHandleBuffer (
> +  ByProtocol,
> +  ,
> +  NULL,
> +  ,
> +  );
> +  if (EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_ERROR, "Sata controller is not able to locate 
> gEfiPciIoProtocolGuid 0x%x\n",
> +Status));
> +return;
> +  }
> +
> +  for (Count = 0; Count < HandleCount; Count++) {
> +Status = gBS->OpenProtocol (
> +HandleBuf[Count],
> +,
> +(VOID **) ,
> +NULL,
> +NULL,
> +EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +if (EFI_ERROR (Status)) {
> +  continue;
> +}
> +
> +//
> +// Now further check the PCI header: Base class (offset 0x0B) and
> +// Sub Class (offset 0x0A). This controller should be an Ide controller
> +//
> +Status = PciIo->Pci.Read (
> +  PciIo,
> +

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

2018-01-08 Thread Meenakshi Aggarwal

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

diff --git a/Platform/NXP/Drivers/SataInitDxe/SataInit.c 
b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
new file mode 100644
index 000..bac390b
--- /dev/null
+++ b/Platform/NXP/Drivers/SataInitDxe/SataInit.c
@@ -0,0 +1,285 @@
+/** @file
+  This driver module adds SATA controller support.
+
+  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
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+ **/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "SataInit.h"
+
+STATIC VOID*mDriverEventRegistration;
+
+/**
+  Read AHCI Operation register.
+
+  @param  PciIoThe PCI IO protocol instance.
+  @param  Offset   The operation register offset.
+
+  @return  The register content read.
+**/
+
+UINT32
+EFIAPI
+AhciReadReg (
+  IN  EFI_PCI_IO_PROTOCOL  *PciIo,
+  IN  UINT32   Offset
+  )
+{
+  UINT32   Data;
+
+  ASSERT (PciIo != NULL);
+
+  Data = 0;
+
+  PciIo->Mem.Read (
+  PciIo,
+  EfiPciIoWidthUint32,
+  AHCI_BAR_INDEX,
+  (UINT64) Offset,
+  1,
+  
+  );
+
+  return Data;
+}
+
+/**
+  Write AHCI Operation register.
+
+  @param PciIo The PCI IO protocol instance.
+  @param OffsetThe operation register offset.
+  @param Data  The data used to write down.
+
+**/
+VOID
+EFIAPI
+AhciWriteReg (
+  IN EFI_PCI_IO_PROTOCOL   *PciIo,
+  IN UINT32Offset,
+  IN UINT32Data
+  )
+{
+  ASSERT (PciIo != NULL);
+
+  PciIo->Mem.Write (
+   PciIo,
+   EfiPciIoWidthUint32,
+   AHCI_BAR_INDEX,
+   (UINT64) Offset,
+   1,
+   
+   );
+
+  return;
+}
+
+STATIC
+VOID
+PciIoRegistrationEvent (
+  IN  EFI_EVENTEvent,
+  IN  VOID *Context
+  )
+{
+  EFI_STATUS   Status;
+  UINTNHandleCount;
+  UINTNAddress;
+  UINT32   Count;
+  UINT32   Data;
+  UINT8PciClass;
+  UINT8PciSubClass;
+  EFI_PCI_IO_PROTOCOL  *PciIo;
+  EFI_HANDLE   *HandleBuf;
+
+  PciIo = NULL;
+
+  Status = gBS->LocateHandleBuffer (
+  ByProtocol,
+  ,
+  NULL,
+  ,
+  );
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_ERROR, "Sata controller is not able to locate 
gEfiPciIoProtocolGuid 0x%x\n",
+Status));
+return;
+  }
+
+  for (Count = 0; Count < HandleCount; Count++) {
+Status = gBS->OpenProtocol (
+HandleBuf[Count],
+,
+(VOID **) ,
+NULL,
+NULL,
+EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+if (EFI_ERROR (Status)) {
+  continue;
+}
+
+//
+// Now further check the PCI header: Base class (offset 0x0B) and
+// Sub Class (offset 0x0A). This controller should be an Ide controller
+//
+Status = PciIo->Pci.Read (
+  PciIo,
+  EfiPciIoWidthUint8,
+  PCI_CLASSCODE_OFFSET + 2,
+  1,
+  
+  );
+if (EFI_ERROR (Status)) {
+  continue;
+}
+
+Status = PciIo->Pci.Read (
+  PciIo,
+  EfiPciIoWidthUint8,
+  PCI_CLASSCODE_OFFSET + 1,
+  1,
+  
+  );
+if (EFI_ERROR (Status)) {
+  continue;
+

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information