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

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

On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote:
> From: jinghua <jinghua@marvell.com>
> 
> Marvell Armada 7k/8k SoCs comprise integrated GPIO controllers,
> one in AP and two in each possible CP hardware blocks.
> 
> This patch introduces support for them, which is a producer of
> MARVELL_GPIO_PROTOCOL, which add necessary routines.
> Hardware description of the controllers is placed in MvHwDescLib.c,
> same as other devices.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf |  43 +++
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h   |  52 ++++
>  Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c   | 298 ++++++++++++++++++++
>  3 files changed, 393 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..2d56433
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.inf
> @@ -0,0 +1,43 @@
> +## @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]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmadaSoCDescLib
> +  DebugLib
> +  MemoryAllocationLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +
> +[Protocols]
> +  gMarvellBoardDescProtocolGuid
> +  gMarvellGpioProtocolGuid
> +
> +[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..48744dc
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.h
> @@ -0,0 +1,52 @@
> +/**
> +*
> +*  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 GPIO_SIGNATURE                   SIGNATURE_32 ('G', 'P', 'I', 'O')

Needs more MV.

> +
> +#ifndef BIT
> +#define BIT(nr)                          (1 << (nr))
> +#endif

OK, you are using this with non-constants.
But please drop the #ifndef and submit a patch to add a BIT macro to
MdePkg/Include/Base.h. (And drop this if you get it accepted :)

> +
> +// Marvell GPIO Controller Registers
> +#define GPIO_DATA_OUT_REG                (0x0)
> +#define GPIO_OUT_EN_REG                  (0x4)
> +#define GPIO_BLINK_EN_REG                (0x8)
> +#define GPIO_DATA_IN_POL_REG             (0xc)
> +#define GPIO_DATA_IN_REG                 (0x10)

Needs more MV.

> +
> +typedef struct {
> +  MARVELL_GPIO_PROTOCOL   GpioProtocol;
> +  MV_BOARD_GPIO_DESC      *Desc;
> +  UINTN                   Signature;
> +  EFI_HANDLE              Handle;
> +} MV_GPIO;
> +
> +#endif // __MV_GPIO_H__
> diff --git a/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> new file mode 100644
> index 0000000..fc2d416
> --- /dev/null
> +++ b/Silicon/Marvell/Drivers/Gpio/MvGpioDxe/MvGpioDxe.c
> @@ -0,0 +1,298 @@
> +/**
> +*
> +*  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 mGpioDevicePathTemplate = {

(Again, not used outside this file, doesn't really need the Gpio bit
in the name. Up to you.)

> +  {
> +    {
> +      HARDWARE_DEVICE_PATH,
> +      HW_VENDOR_DP,
> +      {
> +        (UINT8) (sizeof (VENDOR_DEVICE_PATH) +
> +                 sizeof (MARVELL_GPIO_DRIVER_TYPE)),
> +        (UINT8) ((sizeof (VENDOR_DEVICE_PATH) +
> +                 sizeof (MARVELL_GPIO_DRIVER_TYPE)) >> 8),
> +      },
> +    },
> +    EFI_CALLER_ID_GUID
> +  },
> +  GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> +  {
> +    END_DEVICE_PATH_TYPE,
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    {
> +      sizeof(EFI_DEVICE_PATH_PROTOCOL),
> +      0
> +    }
> +  }
> +};
> +
> +STATIC
> +EFI_STATUS
> +MvGpioValidate (
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin
> +  )
> +{

Please add a comment header for this function explaining what it does.
(I will start becoming more strict on this in general, but I know I've
been lax in the past, so I'll ease into it.)

> +  if (ControllerIndex >= mGpioInstance->Desc->GpioDevCount) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Invalid GPIO ControllerIndex: %d\n",
> +      __FUNCTION__,
> +      ControllerIndex));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (GpioPin >= mGpioInstance->Desc->SoC[ControllerIndex].GpioPinCount) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: GPIO pin #%d not available in Controller#%d\n",
> +      __FUNCTION__,
> +      GpioPin,
> +      ControllerIndex));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvGpioDirectionOutput (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  IN  BOOLEAN Value
> +  )
> +{
> +  UINTN BaseAddress;
> +  EFI_STATUS Status;
> +
> +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to set output for pin #%d\n",
> +      __FUNCTION__,
> +      GpioPin));
> +    return Status;
> +  }

So, I'm on the fence here. Do we need to validate that we're not
trying to write pins that don't exist all the time?

I could see why it may be useful for DEBUG builds and new board
bringup, but even then - could it be a helper function that is called
from an ASSERT and only included ifndef MDEPKG_NDEBUG?

> +
> +  BaseAddress = mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> +
> +  MmioAndThenOr32 (BaseAddress + GPIO_DATA_OUT_REG,
> +    ~BIT (GpioPin),
> +    (Value) << GpioPin);

Would be cleaner to have a BITPOSITION macro to match the BIT one.

> +
> +  MmioAnd32 (BaseAddress + GPIO_OUT_EN_REG, ~BIT (GpioPin));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvGpioDirectionInput (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin
> +  )
> +{
> +  UINTN BaseAddress;
> +  EFI_STATUS Status;
> +
> +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to set input for pin #%d\n",
> +      __FUNCTION__,
> +      GpioPin));
> +    return Status;
> +  }
> +
> +  BaseAddress = mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> +
> +  MmioOr32 (BaseAddress + GPIO_OUT_EN_REG, BIT (GpioPin));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvGpioGetFunction (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  OUT MARVELL_GPIO_MODE *Mode
> +  )
> +{
> +  UINT32 RegVal;
> +  UINTN BaseAddress;
> +  EFI_STATUS Status;
> +
> +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to get function of pin #%d\n",
> +      __FUNCTION__,
> +      GpioPin));
> +    return Status;
> +  }
> +
> +  BaseAddress = mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> +
> +  RegVal = MmioRead32 (BaseAddress + GPIO_OUT_EN_REG);
> +  *Mode = ((RegVal & BIT (GpioPin)) ? GPIO_MODE_INPUT : GPIO_MODE_OUTPUT);

Not a fan of this. This is abusing the fact that the programmer knows
the numeric values of the enum members. At which point, why not nuke
the ternary overhead and go
  *Mode = (RegVal & BIT (GpioPin)) >> BIT (GpioPin);
?

An alternative interpretation is that the enum is the software view
only, and that the bit meaning is hardware dependent. In that case, we
additionally need a #define for that.

> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvGpioGetValue (
> +  IN     MARVELL_GPIO_PROTOCOL *This,
> +  IN     UINTN ControllerIndex,
> +  IN     UINTN GpioPin,
> +  IN OUT BOOLEAN *Value
> +  )
> +{
> +  UINTN BaseAddress;
> +  EFI_STATUS Status;
> +
> +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to get value of pin #%d\n",
> +      __FUNCTION__,
> +      GpioPin));
> +    return Status;
> +  }
> +
> +  BaseAddress = mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> +
> +  *Value = !!(MmioRead32 (BaseAddress + GPIO_DATA_IN_REG) & BIT (GpioPin));

Please don't !!.
If necessary, please shift.

/
    Leif

> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MvGpioSetValue (
> +  IN  MARVELL_GPIO_PROTOCOL *This,
> +  IN  UINTN ControllerIndex,
> +  IN  UINTN GpioPin,
> +  IN  BOOLEAN Value
> +  )
> +{
> +  UINTN BaseAddress;
> +  EFI_STATUS Status;
> +
> +  Status = MvGpioValidate (ControllerIndex, GpioPin);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to get value of pin #%d\n",
> +      __FUNCTION__,
> +      GpioPin));
> +    return Status;
> +  }
> +
> +  BaseAddress = mGpioInstance->Desc->SoC[ControllerIndex].GpioBaseAddress;
> +
> +  MmioAndThenOr32 (BaseAddress + GPIO_DATA_OUT_REG,
> +    ~BIT (GpioPin),
> +    Value << GpioPin);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +MvGpioInitProtocol (
> +  IN MARVELL_GPIO_PROTOCOL *GpioProtocol
> +  )
> +{
> +  GpioProtocol->DirectionInput  = MvGpioDirectionInput;
> +  GpioProtocol->DirectionOutput = MvGpioDirectionOutput;
> +  GpioProtocol->GetFunction     = MvGpioGetFunction;
> +  GpioProtocol->GetValue        = MvGpioGetValue;
> +  GpioProtocol->SetValue        = MvGpioSetValue;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +MvGpioEntryPoint (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  MARVELL_BOARD_DESC_PROTOCOL *BoardDescProtocol;
> +  MV_GPIO_DEVICE_PATH *GpioDevicePath;
> +  MV_BOARD_GPIO_DESC *Desc;
> +  EFI_STATUS Status;
> +
> +  GpioDevicePath = AllocateCopyPool (sizeof (MV_GPIO_DEVICE_PATH),
> +                     &mGpioDevicePathTemplate);
> +  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 **)&BoardDescProtocol);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Cannot locate BoardDesc protocol\n",
> +      __FUNCTION__));
> +    goto ErrLocateBoardDesc;
> +  }
> +
> +  Status = BoardDescProtocol->BoardDescGpioGet (BoardDescProtocol, &Desc);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Cannot get GPIO board desc from BoardDesc protocol\n",
> +      __FUNCTION__));
> +    goto ErrLocateBoardDesc;
> +  }
> +
> +  mGpioInstance->Signature = GPIO_SIGNATURE;
> +  mGpioInstance->Desc = Desc;
> +
> +  MvGpioInitProtocol (&mGpioInstance->GpioProtocol);
> +
> +  Status = gBS->InstallMultipleProtocolInterfaces (&(mGpioInstance->Handle),
> +                  &gMarvellGpioProtocolGuid,
> +                  &(mGpioInstance->GpioProtocol),
> +                  &gEfiDevicePathProtocolGuid,
> +                  (EFI_DEVICE_PATH_PROTOCOL *)GpioDevicePath,
> +                  NULL);
> +  if (EFI_ERROR (Status)) {
> +    goto ErrLocateBoardDesc;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +ErrLocateBoardDesc:
> +  gBS->FreePool (mGpioInstance);
> +
> +ErrGpioInstanceAlloc:
> +  gBS->FreePool (GpioDevicePath);
> +
> +  return Status;
> +}
> -- 
> 2.7.4
>