From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web12.5473.1601032364739473822 for ; Fri, 25 Sep 2020 04:12:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=qrMZXlQA; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id k18so2830852wmj.5 for ; Fri, 25 Sep 2020 04:12:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=xzpVybaBZlA3Ru93QPOevh4aXGEM+CoZtWKNquMGjO0=; b=qrMZXlQAel4AIr8NyjwVAHtj8FVmBfRlTZXr2bou0KR7xLsP5SNb1k8rznBB4rU8Ve iTloGljeUJdUBK0+TdcpfJr7Iev26E0wnfBL7X/Ji4Vu6svnJZPbEFScfxjUH9E4strX jXVdCpmBknzZOL6K48TQ9iBL6XmehaqArZl1ZtaGlzD9TcI8FtwS7zA0LqIZslR7rc44 boELqAVKW/e/GWYXxU+FkMjWPgWDGiM9a9P8lVMNjSCjuvLEA5jSMqhJQdL7SM6UwwJe q3P3qqTW9sPp/z9J6Vx5ku7g8mJ6/iPXHolzMFyENJniRTa5Cnd+0lumedNXgazCPsnw 9PTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=xzpVybaBZlA3Ru93QPOevh4aXGEM+CoZtWKNquMGjO0=; b=D2CQJxMAdXwQ3PrSJs61iwkY78/ewNhxWU26sP+QifmnplN3zpbQHVtLGFOCRYv4Ty DjNlmYLARZ9xLY32Kzn5bc/9c/A25OeWeeI47YcnjtGAMqQRxCAU/VvQms7hhy1TNL8L c4rdGS/7uXyJ3k2yUZTxNxGvw/xvU6hVfZQIJxptiuPKRXjp8NcQNUqtobuZeyJjVAAj VHzMlXOzLx0iILhAzZBgtJoTWsEvGApy5co8E7FDpmfRqfQGuDI/EtKJAxsUwmM7nnSK /kPNnf8HPea7sd4ZMSMoCbtN1u4VON3J8V6bD8HZOtVU19VvEgECdTrKe0C/3tPHHqgc +2uA== X-Gm-Message-State: AOAM530AVU8dWbz/nv2og9LE2Cs9hGQDK4AYb0tWNrvclaVB4LWDIGWv 83lYe+rofAO3eFAvLltrgcZyqA== X-Google-Smtp-Source: ABdhPJzzHc783EfE/gpfSp75uXHpj6ZgJVXcDf+KzxjPiCmliL3AhS1HAoCzb1H0xaajsbh7wF0+6A== X-Received: by 2002:a1c:28a:: with SMTP id 132mr2536407wmc.9.1601032363067; Fri, 25 Sep 2020 04:12:43 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id z13sm2485873wro.97.2020.09.25.04.12.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Sep 2020 04:12:42 -0700 (PDT) Date: Fri, 25 Sep 2020 12:12:35 +0100 From: "Leif Lindholm" To: Meenakshi Aggarwal Cc: ard.biesheuvel@arm.com, michael.d.kinney@intel.com, devel@edk2.groups.io, v.sethi@nxp.com, Pramod Kumar , Meenakshi Aggarwal Subject: Re: [edk2-platforms 1/4] Silicon/NXP: Add GPIO driver support. Message-ID: <20200925111235.GU5623@vanye> References: <1600187343-18732-1-git-send-email-meenakshi.aggarwal@oss.nxp.com> <1600187343-18732-2-git-send-email-meenakshi.aggarwal@oss.nxp.com> MIME-Version: 1.0 In-Reply-To: <1600187343-18732-2-git-send-email-meenakshi.aggarwal@oss.nxp.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Needs an actual commit message. What GPIO controller? If it does not have an explicit name, what family of devices is it in? On Tue, Sep 15, 2020 at 21:59:00 +0530, Meenakshi Aggarwal wrote: > Signed-off-by: Pramod Kumar > Signed-off-by: Meenakshi Aggarwal Only the poster can sign off that the post is being submitted in accordance with the Developer's Certificate of Origin: https://developercertificate.org/ If the author is different from the poster, that should be described in the patch metadata (i.e. Author: ). I have permitted (although I'm not a fan) Co-authored-by: tags, if that is what this is intended to describe. > --- > Silicon/NXP/Library/GpioLib/GpioLib.inf | 39 +++++ > Silicon/NXP/Include/Library/GpioLib.h | 110 +++++++++++++++ > Silicon/NXP/Library/GpioLib/GpioLib.c | 242 ++++++++++++++++++++++++++++++++ > 3 files changed, 391 insertions(+) > create mode 100644 Silicon/NXP/Library/GpioLib/GpioLib.inf > create mode 100644 Silicon/NXP/Include/Library/GpioLib.h > create mode 100644 Silicon/NXP/Library/GpioLib/GpioLib.c > > diff --git a/Silicon/NXP/Library/GpioLib/GpioLib.inf b/Silicon/NXP/Library/GpioLib/GpioLib.inf > new file mode 100644 > index 000000000000..7878d1d03db2 > --- /dev/null > +++ b/Silicon/NXP/Library/GpioLib/GpioLib.inf > @@ -0,0 +1,39 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = GpioLib > + FILE_GUID = addec2b8-d2e0-43c0-a277-41a8d42f3f4f > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = GpioLib > + > +[Sources.common] > + GpioLib.c > + > +[LibraryClasses] > + ArmLib > + BaseMemoryLib > + BaseLib Flip order of above two lines. > + IoAccessLib > + IoLib > + > +[Packages] > + ArmPlatformPkg/ArmPlatformPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + Silicon/NXP/NxpQoriqLs.dec > + > +[Pcd] > + gNxpQoriqLsTokenSpaceGuid.PcdNumGpioController > + gNxpQoriqLsTokenSpaceGuid.PcdGpioModuleBaseAddress > + gNxpQoriqLsTokenSpaceGuid.PcdGpioControllerOffset > + > +[FeaturePcd] > + gNxpQoriqLsTokenSpaceGuid.PcdGpioControllerBigEndian > diff --git a/Silicon/NXP/Include/Library/GpioLib.h b/Silicon/NXP/Include/Library/GpioLib.h > new file mode 100644 > index 000000000000..5821806226ee > --- /dev/null > +++ b/Silicon/NXP/Include/Library/GpioLib.h > @@ -0,0 +1,110 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef GPIO_H__ > +#define GPIO_H__ > + > +#include > + > +/* enum for GPIO number */ > +typedef enum _GPIO_BLOCK { > + GPIO1, > + GPIO2, > + GPIO3, > + GPIO4, > + GPIO_MAX > +} GPIO_BLOCK; > + > +/* enum for GPIO direction */ > +typedef enum _GPIO_DIRECTION { > + INPUT, > + OUTPUT > +} GPIO_DIRECTION; > + > +/* enum for GPIO state */ > +typedef enum _GPIO_STATE { > + LOW, > + HIGH > +} GPIO_VAL; > + > +/** > + SetDir Set GPIO direction as INPUT or OUTPUT > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Dir GPIO Direction as INPUT or OUTPUT > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetDir ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Dir > + ); > + > +/** > + GetDir Retrieve GPIO direction > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO Direction as INPUT or OUTPUT > + **/ > +UINT32 > +GetDir ( > + IN UINT8 Id, > + IN UINT32 Bit > + ); > + > + /** > + GetData Retrieve GPIO Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO value as HIGH or LOW > + **/ > +UINT32 > +GetData ( > + IN UINT8 Id, > + IN UINT32 Bit > + ); > + > +/** > + SetData Set GPIO data Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Data GPIO data value to set > + > + @retval GPIO value as HIGH or LOW > + **/ > +EFI_STATUS > +SetData ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Data > + ); > + > +/** > + SetOpenDrain Set GPIO as Open drain > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] OpenDrain Set as open drain > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetOpenDrain ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN OpenDrain > + ); > + > +#endif > diff --git a/Silicon/NXP/Library/GpioLib/GpioLib.c b/Silicon/NXP/Library/GpioLib/GpioLib.c > new file mode 100644 > index 000000000000..33cc45c2152b > --- /dev/null > +++ b/Silicon/NXP/Library/GpioLib/GpioLib.c > @@ -0,0 +1,242 @@ > +/** @file > + Which controller is this for. There should be a comment here sufficient for me to figure out where I should start looking for documentation. > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include > +#include > +#include > +#include > +#include > + > +STATIC MMIO_OPERATIONS *mGpioOps; > + > +/* Structure for GPIO Regsters */ > +typedef struct GpioRegs { > + UINT32 GpDir; > + UINT32 GpOdr; > + UINT32 GpData; > + UINT32 GpIer; > + UINT32 GpImr; > + UINT32 GpIcr; Are the above registers the official names as per the documentation? If so, this is fine even though it violates the CamelCase naming style, but please add a glossary entry to the top-of-file comment block. > +} GPIO_REGS; > + > +/** > + GetBaseAddr GPIO controller Base Address > + > + @param[in] Id GPIO controller number > + > + @retval GPIO controller Base Address, if found > + @retval NULL, if not a valid controller number > + > + **/ > +STATIC > +VOID * > +GetBaseAddr ( > + IN UINT8 Id > + ) > +{ > + > + UINTN GpioBaseAddr; > + UINTN MaxGpioController; > + > + mGpioOps = GetMmioOperations (FeaturePcdGet (PcdGpioControllerBigEndian)); > + > + MaxGpioController = PcdGet32 (PcdNumGpioController); > + > + if (Id < MaxGpioController) { > + GpioBaseAddr = PcdGet64 (PcdGpioModuleBaseAddress) + > + (Id * PcdGet64 (PcdGpioControllerOffset)); > + return (VOID *) GpioBaseAddr; No space after ). > + } > + else { Move to line above. > + DEBUG((DEBUG_ERROR, "Invalid Gpio Controller Id %d, Allowed Ids are %d-%d", > + Id, GPIO1, MaxGpioController)); > + return NULL; > + } > +} > + > +/** > + GetBitMask: Return Bit Mask > + > + @param[in] Bit Bit to create bitmask > + @retval Bitmask > + > + **/ > + > +STATIC > +UINT32 > +GetBitMask ( > + IN UINT32 Bit > + ) > +{ > + > + if (!FeaturePcdGet (PcdGpioControllerBigEndian)) { > + return (1 << Bit); > + } else { > + return (1 << (31 - Bit)); > + } The above confuses me greatly - endianness affects byte order, not bit order. Is this some compensation for PowerPC numbering bits backwards, and these reused blocks being affected by this when in big-endian mode? Needs a very descriptive comment. The current function comment block contains no information, it just keeps repeating the function name in different word order. > +} > + > + > +/** > + SetDir Set GPIO direction as INPUT or OUTPUT > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Dir GPIO Direction as INPUT or OUTPUT > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetDir ( SetDirection > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Dir > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; The variable should be called something descriptive. You are using it to read a specific value out of a specific register. In this instance, DirectionMask would be a better alternative. Please address accordingly in functions below, with appropriate descriptive names for each location. > + UINT32 Value; > + > + Regs = GetBaseAddr(Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpDir); > + > + if (Dir) { > + mGpioOps->Write32 ((UINTN)&Regs->GpDir, (Value | BitMask)); > + } > + else { > + mGpioOps->Write32 ((UINTN)&Regs->GpDir, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + GetDir Retrieve GPIO direction > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO Direction as INPUT or OUTPUT > + **/ > +UINT32 > +GetDir ( GetDirection > + IN UINT8 Id, > + IN UINT32 Bit > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 Value; > + UINT32 BitMask; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpDir); > + > + return (Value & BitMask); > +} > + > +/** > + GetData Retrieve GPIO Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO value as HIGH or LOW > + **/ > +UINT32 > +GetData ( > + IN UINT8 Id, > + IN UINT32 Bit > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 Value; > + UINT32 BitMask; > + > + Regs = (VOID *)GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Spurious extra blank line. > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpData); > + > + if (Value & BitMask) { > + return 1; > + } else { > + return 0; > + } return (Value & BitMask) == BitMask; ? > +} > + > +/** > + SetData Set GPIO data Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Data GPIO data value to set > + > + @retval GPIO value as HIGH or LOW > + **/ > +EFI_STATUS > +SetData ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Data > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; > + UINT32 Value; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpData); > + > + if (Data) { > + mGpioOps->Write32 ((UINTN)&Regs->GpData, (Value | BitMask)); > + } else { > + mGpioOps->Write32 ((UINTN)&Regs->GpData, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + SetOpenDrain Set GPIO as Open drain > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] OpenDrain Set as open drain > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetOpenDrain ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN OpenDrain > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; > + UINT32 Value; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); Missing space before (. > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpOdr); > + if (OpenDrain) { > + mGpioOps->Write32 ((UINTN)&Regs->GpOdr, (Value | BitMask)); > + } > + else { Move to line above. / Leif > + mGpioOps->Write32 ((UINTN)&Regs->GpOdr, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > -- > 1.9.1 >