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

[Leif Lindholm <leif.lindholm@linaro.org>]

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

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.

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