On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote:
> From: jinghua <jing...@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 <m...@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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
