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

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

On Tue, 4 Dec 2018 at 17:37, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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.
>

Or cast to (BOOLEAN)