Re: [platforms: PATCH v2 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,


wt., 15 sty 2019 o 00:32 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Thu, Jan 10, 2019 at 02:44:35AM +0100, Marcin Wojtas wrote:
> > Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> > one in AP806 and two in each south bridge hardware blocks.
> >
> > This patch introduces support for them. The new driver implements
> > a generic EMBEDDED_GPIO protocol.
> >
> > In order to ease description of used GPIO pins and controllers
> > of the Armada 7k8k platforms, add a common enum type.
> >
> > Based on original work of Jing Hua <jinghua@marvell.com>.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf |  44 +++
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h   |  49 +++
> >  Silicon/Marvell/Include/Protocol/MvGpio.h            |  10 +
> >  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c   | 372 ++++++++++++++++++++
> >  4 files changed, 475 insertions(+)
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> >  create mode 100644 Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> >
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > new file mode 100644
> > index 0000000..5ff9130
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> > @@ -0,0 +1,44 @@
> > +## @file
> > +#
> > +#  Copyright (c) 2017, Marvell International Ltd. All rights reserved.<BR>
> > +#
> > +#  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 at
> > +#  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.
> > +#
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x0001001A
> > +  BASE_NAME                      = MvGpioDxe
> > +  FILE_GUID                      = 706eb761-b3b5-4f41-8558-5fd9217c0079
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = MvGpioEntryPoint
> > +
> > +[Sources]
> > +  MvGpioDxe.c
> > +  MvGpioDxe.h
> > +
> > +[Packages]
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/Marvell/Marvell.dec
> > +
> > +[LibraryClasses]
> > +  ArmadaSoCDescLib
> > +  DebugLib
> > +  MemoryAllocationLib
> > +  UefiDriverEntryPoint
> > +  UefiLib
> > +
> > +[Protocols]
> > +  gEmbeddedGpioProtocolGuid
> > +  gMarvellBoardDescProtocolGuid
> > +
> > +[Depex]
> > +  TRUE
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > new file mode 100644
> > index 0000000..4e5422b
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> > @@ -0,0 +1,49 @@
> > +/**
> > +*
> > +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> > +*
> > +*  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 at
> > +*  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.
> > +*
> > +**/
> > +#ifndef __MV_GPIO_H__
> > +#define __MV_GPIO_H__
> > +
> > +
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/DevicePathLib.h>
> > +#include <Library/IoLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/PcdLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiLib.h>
> > +
> > +#include <Protocol/BoardDesc.h>
> > +#include <Protocol/MvGpio.h>
> > +
> > +#include <Uefi/UefiBaseType.h>
> > +
> > +#define MV_GPIO_SIGNATURE        SIGNATURE_32 ('G', 'P', 'I', 'O')
>
> Unaddressed feedback from last time. But rereading it makes it clear
> my feedback was too imprecise, apologies. The point was that "GPIO" is
> not a very good choice of key to distinguish _this_ GPIO driver from
> other GPIO drivers. (As well as the MV_ prefix.)
>

Sure, I will modify it to be more specific signature.

> One more comment below, but it does not require code changes.
> If you can just change the signature, I'm happy with this patch.
>
> > +
> > +// Marvell MV_GPIO Controller Registers
> > +#define MV_GPIO_DATA_OUT_REG     (0x0)
> > +#define MV_GPIO_OUT_EN_REG       (0x4)
> > +#define MV_GPIO_BLINK_EN_REG     (0x8)
> > +#define MV_GPIO_DATA_IN_POL_REG  (0xc)
> > +#define MV_GPIO_DATA_IN_REG      (0x10)
> > +
> > +typedef struct {
> > +  EMBEDDED_GPIO     GpioProtocol;
> > +  GPIO_CONTROLLER  *SoCGpio;
> > +  UINTN             GpioDeviceCount;
> > +  UINTN             Signature;
> > +  EFI_HANDLE        Handle;
> > +} MV_GPIO;
> > +
> > +#endif // __MV_GPIO_H__
> > diff --git a/Silicon/Marvell/Include/Protocol/MvGpio.h b/Silicon/Marvell/Include/Protocol/MvGpio.h
> > index c9f1007..3319b79 100644
> > --- a/Silicon/Marvell/Include/Protocol/MvGpio.h
> > +++ b/Silicon/Marvell/Include/Protocol/MvGpio.h
> > @@ -27,6 +27,16 @@ typedef enum {
> >    MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> >  } MV_GPIO_DRIVER_TYPE;
> >
> > +typedef enum {
> > +  MV_GPIO_AP806_CONTROLLER0,
> > +  MV_GPIO_CP0_CONTROLLER0,
> > +  MV_GPIO_CP0_CONTROLLER1,
> > +  MV_GPIO_CP1_CONTROLLER0,
> > +  MV_GPIO_CP1_CONTROLLER1,
> > +  MV_GPIO_CP2_CONTROLLER0,
> > +  MV_GPIO_CP2_CONTROLLER1,
> > +} MV_GPIO_SOC_CONTROLLER_TYPE;
> > +
> >  typedef struct {
> >    VENDOR_DEVICE_PATH        Header;
> >    MV_GPIO_DRIVER_TYPE       GpioDriverType;
> > diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> > new file mode 100644
> > index 0000000..4d5e530
> > --- /dev/null
> > +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> > @@ -0,0 +1,372 @@
> > +/**
> > +*
> > +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> > +*
> > +*  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 at
> > +*  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 "MvGpioDxe.h"
> > +
> > +STATIC MV_GPIO *mGpioInstance;
> > +
> > +STATIC MV_GPIO_DEVICE_PATH mDevicePathTemplate = {
> > +  {
> > +    {
> > +      HARDWARE_DEVICE_PATH,
> > +      HW_VENDOR_DP,
> > +      {
> > +        (UINT8) (sizeof (VENDOR_DEVICE_PATH) +
> > +                 sizeof (MV_GPIO_DRIVER_TYPE)),
> > +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH) +
> > +                 sizeof (MV_GPIO_DRIVER_TYPE)) >> 8),
> > +      },
> > +    },
> > +    EFI_CALLER_ID_GUID
> > +  },
> > +  MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> > +  {
> > +    END_DEVICE_PATH_TYPE,
> > +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +    {
> > +      sizeof(EFI_DEVICE_PATH_PROTOCOL),
> > +      0
> > +    }
> > +  }
> > +};
> > +
> > +#if !defined(MDEPKG_NDEBUG)
> > +/**
> > +
> > +Routine Description:
> > +
> > +  Verifies if controller index / GPIO pin values
> > +  are within proper boundaries.
> > +
> > +Arguments:
> > +
> > +  ControllerIndex - index of controller
> > +  GpioPin - which pin to read
> > +
> > +Returns:
> > +
> > +  EFI_SUCCESS           - GPIO pin / controller index are proper
> > +  EFI_INVALID_PARAMETER - GPIO pin / controller index is out of range
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +MvGpioValidate (
> > +  IN UINTN ControllerIndex,
> > +  IN UINTN GpioPin
> > +  )
> > +{
> > +  if (ControllerIndex >= mGpioInstance->GpioDeviceCount) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Invalid GPIO ControllerIndex: %d\n",
> > +      __FUNCTION__,
> > +      ControllerIndex));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (GpioPin >= mGpioInstance->SoCGpio[ControllerIndex].InternalGpioCount) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: GPIO pin #%d not available in Controller#%d\n",
> > +      __FUNCTION__,
> > +      GpioPin,
> > +      ControllerIndex));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +#endif
> > +
> > +/**
> > +
> > +Routine Description:
> > +
> > +  Gets the mode (function) of a GPIO pin
> > +
> > +Arguments:
> > +
> > +  This  - pointer to protocol
> > +  Gpio  - which pin
> > +  Mode  - pointer to output mode value
> > +
> > +Returns:
> > +
> > +  EFI_SUCCESS           - mode value retrieved
> > +  EFI_INVALID_PARAMETER - Mode is a null pointer or Gpio pin is out of range
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +MvGpioGetMode (
>
> Is this a refactored version of MvGpioGetFunction?
>
> > +  IN  EMBEDDED_GPIO       *This,
> > +  IN  EMBEDDED_GPIO_PIN    Gpio,
> > +  OUT EMBEDDED_GPIO_MODE  *Mode
> > +  )
> > +{
> > +  UINTN ControllerIndex;
> > +  UINTN BaseAddress;
> > +  UINTN GpioPin;
> > +
> > +  GpioPin = GPIO_PIN (Gpio);
> > +  ControllerIndex = GPIO_PORT (Gpio);
> > +
> > +  ASSERT_EFI_ERROR (MvGpioValidate (ControllerIndex, GpioPin));
> > +
> > +  BaseAddress = mGpioInstance->SoCGpio[ControllerIndex].RegisterBase;
> > +
> > +  if (MmioRead32 (BaseAddress + MV_GPIO_OUT_EN_REG) & (1 << GpioPin)) {
> > +    *Mode = GPIO_MODE_INPUT;
> > +  } else {
> > +    if (MmioRead32 (BaseAddress + MV_GPIO_DATA_IN_REG) & (1 << GpioPin)) {
> > +      *Mode = GPIO_MODE_OUTPUT_1;
> > +    } else {
> > +      *Mode = GPIO_MODE_OUTPUT_0;
>
> Ah, right, it's the change to EMBEDDED_GPIO that means we have two
> output modes to return instead of just input or output.
> Could I just ask that you're a bit more explicit about such things in
> the cover letter? Would have saved me a couple of minutes of head
> scratching.
>

Well, in the cover letter I wrote:
"The biggest change is dropping custom GPIO protocol and start using
the generic EMBEDDED_GPIO with all its types."

And in the commit log of both drivers:
"The new driver implements a generic EMBEDDED_GPIO protocol."

Wasn't it explicit enough? :)

Best regards,
Marcin

> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +
> > +Routine Description:
> > +
> > +  Gets the state of a GPIO pin
> > +
> > +Arguments:
> > +
> > +  This  - pointer to protocol
> > +  Gpio  - which pin to read
> > +  Value - state of the pin
> > +
> > +Returns:
> > +
> > +  EFI_SUCCESS           - GPIO state returned in Value
> > +  EFI_INVALID_PARAMETER - Value is NULL pointer or Gpio pin is out of range
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +MvGpioGet (
> > +  IN  EMBEDDED_GPIO      *This,
> > +  IN  EMBEDDED_GPIO_PIN   Gpio,
> > +  OUT UINTN              *Value
> > +  )
> > +{
> > +  UINTN ControllerIndex;
> > +  UINTN BaseAddress;
> > +  UINTN GpioPin;
> > +
> > +  GpioPin = GPIO_PIN (Gpio);
> > +  ControllerIndex = GPIO_PORT (Gpio);
> > +
> > +  ASSERT_EFI_ERROR (MvGpioValidate (ControllerIndex, GpioPin));
> > +
> > +  BaseAddress = mGpioInstance->SoCGpio[ControllerIndex].RegisterBase;
> > +
> > +  if (MmioRead32 (BaseAddress + MV_GPIO_DATA_IN_REG) & (1 << GpioPin)) {
> > +    *Value = 1;
> > +  } else {
> > +    *Value = 0;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +
> > +Routine Description:
> > +
> > +  Sets the state of a GPIO pin
> > +
> > +Arguments:
> > +
> > +  This  - pointer to protocol
> > +  Gpio  - which pin to modify
> > +  Mode  - mode to set
> > +
> > +Returns:
> > +
> > +  EFI_SUCCESS           - GPIO set as requested
> > +  EFI_UNSUPPORTED       - Mode is not supported
> > +  EFI_INVALID_PARAMETER - Gpio pin is out of range
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +MvGpioSet (
> > +  IN EMBEDDED_GPIO       *This,
> > +  IN EMBEDDED_GPIO_PIN    Gpio,
> > +  IN EMBEDDED_GPIO_MODE   Mode
> > +  )
> > +{
> > +  UINTN ControllerIndex;
> > +  UINTN BaseAddress;
> > +  UINTN GpioPin;
> > +
> > +  GpioPin = GPIO_PIN (Gpio);
> > +  ControllerIndex = GPIO_PORT (Gpio);
> > +
> > +  ASSERT_EFI_ERROR (MvGpioValidate (ControllerIndex, GpioPin));
> > +
> > +  BaseAddress = mGpioInstance->SoCGpio[ControllerIndex].RegisterBase;
> > +
> > +  switch (Mode) {
> > +  case GPIO_MODE_OUTPUT_0:
> > +    MmioAnd32 (BaseAddress + MV_GPIO_DATA_OUT_REG, ~(1 << GpioPin));
> > +    MmioAnd32 (BaseAddress + MV_GPIO_OUT_EN_REG, ~(1 << GpioPin));
> > +    break;
> > +
> > +  case GPIO_MODE_OUTPUT_1:
> > +    MmioOr32 (BaseAddress + MV_GPIO_DATA_OUT_REG, (1 << GpioPin));
> > +    MmioAnd32 (BaseAddress + MV_GPIO_OUT_EN_REG, ~(1 << GpioPin));
> > +    break;
> > +
> > +  case GPIO_MODE_INPUT:
> > +    MmioOr32 (BaseAddress + MV_GPIO_OUT_EN_REG, (1 << GpioPin));
> > +    break;
> > +
> > +  default:
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > +
> > +Routine Description:
> > +
> > +  Sets the pull-up / pull-down resistor of a GPIO pin
> > +
> > +Arguments:
> > +
> > +  This  - pointer to protocol
> > +  Gpio  - which pin
> > +  Direction - pull-up, pull-down, or none
> > +
> > +Returns:
> > +
> > +  EFI_UNSUPPORTED - Can not perform the requested operation
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +MvGpioSetPull (
> > +  IN  EMBEDDED_GPIO       *This,
> > +  IN  EMBEDDED_GPIO_PIN   Gpio,
> > +  IN  EMBEDDED_GPIO_PULL  Direction
> > +  )
> > +{
> > +  return EFI_UNSUPPORTED;
> > +}
> > +
> > +STATIC
> > +VOID
> > +MvGpioInitProtocol (
> > +  IN EMBEDDED_GPIO *GpioProtocol
> > +  )
> > +{
> > +  GpioProtocol->Get     = MvGpioGet;
> > +  GpioProtocol->Set     = MvGpioSet;
> > +  GpioProtocol->GetMode = MvGpioGetMode;
> > +  GpioProtocol->SetPull = MvGpioSetPull;
> > +}
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +MvGpioEntryPoint (
> > +  IN EFI_HANDLE       ImageHandle,
> > +  IN EFI_SYSTEM_TABLE *SystemTable
> > +  )
> > +{
> > +  MARVELL_BOARD_DESC_PROTOCOL *MvBoardProtocol;
> > +  MV_BOARD_GPIO_DESCRIPTION *GpioDescription;
> > +  MV_GPIO_DEVICE_PATH *GpioDevicePath;
> > +  EFI_STATUS Status;
> > +
> > +  GpioDevicePath = AllocateCopyPool (sizeof (MV_GPIO_DEVICE_PATH),
> > +                     &mDevicePathTemplate);
> > +  if (GpioDevicePath == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  mGpioInstance = AllocateZeroPool (sizeof (MV_GPIO));
> > +  if (mGpioInstance == NULL) {
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    goto ErrGpioInstanceAlloc;
> > +  }
> > +
> > +  /* Obtain list of available controllers */
> > +  Status = gBS->LocateProtocol (&gMarvellBoardDescProtocolGuid,
> > +                  NULL,
> > +                  (VOID **)&MvBoardProtocol);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot locate BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    goto ErrLocateBoardDesc;
> > +  }
> > +
> > +  Status = MvBoardProtocol->GpioDescriptionGet (MvBoardProtocol,
> > +             &GpioDescription);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Cannot get GPIO board desc from BoardDesc protocol\n",
> > +      __FUNCTION__));
> > +    goto ErrLocateBoardDesc;
> > +  }
> > +
> > +  mGpioInstance->Signature = MV_GPIO_SIGNATURE;
> > +  mGpioInstance->GpioDeviceCount = GpioDescription->GpioDeviceCount;
> > +  mGpioInstance->SoCGpio = AllocateCopyPool (sizeof (GPIO_CONTROLLER) *
> > +                             GpioDescription->GpioDeviceCount,
> > +                             GpioDescription->SoCGpio);
> > +  if (mGpioInstance->SoCGpio == NULL) {
> > +    DEBUG ((DEBUG_ERROR,
> > +      "%a: Fail to copy GPIO SoC controllers description\n",
> > +      __FUNCTION__));
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    goto ErrAllocateSoCGpio;
> > +  }
> > +
> > +  MvGpioInitProtocol (&mGpioInstance->GpioProtocol);
> > +
> > +  Status = gBS->InstallMultipleProtocolInterfaces (&(mGpioInstance->Handle),
> > +                  &gEmbeddedGpioProtocolGuid,
> > +                  &(mGpioInstance->GpioProtocol),
> > +                  &gEfiDevicePathProtocolGuid,
> > +                  (EFI_DEVICE_PATH_PROTOCOL *)GpioDevicePath,
> > +                  NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    goto ErrInstallProtocols;
> > +  }
> > +
> > +  gBS->FreePool (GpioDescription->SoCGpio);
> > +  gBS->FreePool (GpioDescription);
> > +
> > +  return EFI_SUCCESS;
> > +
> > +ErrInstallProtocols:
> > +  gBS->FreePool (mGpioInstance->SoCGpio);
> > +
> > +ErrAllocateSoCGpio:
> > +  gBS->FreePool (GpioDescription->SoCGpio);
> > +  gBS->FreePool (GpioDescription);
> > +
> > +ErrLocateBoardDesc:
> > +  gBS->FreePool (mGpioInstance);
> > +
> > +ErrGpioInstanceAlloc:
> > +  gBS->FreePool (GpioDevicePath);
> > +
> > +  return Status;
> > +}
> > --
> > 2.7.4
> >