From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::144; helo=mail-it1-x144.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E7C3121198CC8 for ; Tue, 4 Dec 2018 08:40:28 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id i145so16374437ita.4 for ; Tue, 04 Dec 2018 08:40:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Zg8LFbft59gOowo+WJnzcZaEKaog7jAuWE1aro3fovg=; b=kMurZRDGYxkQv5KnCGG1hbBDQgLOVuhR0AvCIMg/LAv9Nm5/261TQS+6yMNlgNSTBY /Ka0ADpGhLsNkbxh+IrIePfeRQ/HmMDtpLxAfcYiwDT64UD3LyoN/MhFLg15uMyZsBeo hMi3VVYyTISV6KMcAkyDjA15FyxoorlvxWKd0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Zg8LFbft59gOowo+WJnzcZaEKaog7jAuWE1aro3fovg=; b=Hk+YpinF62i6nboS/GqBzHXJVPcDgpS/3GvTUEnZNBK2NFWPMKkLKDuTgjhsUbDS6z TlZOJysC/9spjyWNzv9JllEiUJjPl49AmqOgdapOh2NRIy4XmqAWL2TGiPcXGmeKxmzL y2zxsp8jZ/R3QE7aOfBlaIyOb2xXh1dkmPGy5Di7ckqFOVagIl+gAo72vx/Az/nBAYKU GQkQMCbReNwI8ohuiovJPEUKci2Io9TUfzMiK61ulsQnY7Q3eNg23bACchAREZfJ4X4S snV5y6rIedtMZnvxbd/00VJZO0BQZouh+6Q8XxBFuPI8fq2sVD32cGPPTd1jespYEfJr klQA== X-Gm-Message-State: AA+aEWbsWBYIVTk1qXQ63/waJECYsM8nTsbpnq+Z6p8IduswzmsSQL2D 84ncnBmUEIH2ok2cxzSPVh1WK/3WLbzrZQFar5rgOBN7MM4= X-Google-Smtp-Source: AFSGD/VQNDwSOz37NFa8yw/pjdeekBpQ0R19wDu9sTXvoiHELzoSkNKqRwm/t6/b5sOQfJoEiGlaU4etwrce18yv2pU= X-Received: by 2002:a24:710:: with SMTP id f16mr10870811itf.121.1543941628099; Tue, 04 Dec 2018 08:40:28 -0800 (PST) MIME-Version: 1.0 References: <1540000661-1956-1-git-send-email-mw@semihalf.com> <1540000661-1956-9-git-send-email-mw@semihalf.com> <20181204163726.4j5pm4cbwlmb5oea@bivouac.eciton.net> In-Reply-To: <20181204163726.4j5pm4cbwlmb5oea@bivouac.eciton.net> From: Ard Biesheuvel Date: Tue, 4 Dec 2018 17:40:14 +0100 Message-ID: To: Leif Lindholm Cc: Marcin Wojtas , "edk2-devel@lists.01.org" , Nadav Haklai , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Hua Jing Subject: Re: [platforms: PATCH 08/12] Marvell/Drivers: MvGpioDxe: Introduce platform GPIO driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Dec 2018 16:40:29 -0000 Content-Type: text/plain; charset="UTF-8" On Tue, 4 Dec 2018 at 17:37, Leif Lindholm wrote: > > On Sat, Oct 20, 2018 at 03:57:37AM +0200, Marcin Wojtas wrote: > > From: jinghua > > > > 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 > > --- > > 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.
> > +# > > +# 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > + > > +#include > > + > > +#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)