On Tue, 4 Dec 2018 at 17:37, Leif Lindholm <[email protected]> wrote: > > On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote: > > From: jinghua <[email protected]> > > > > 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 <[email protected]> > > --- > > 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) _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
