Re: [edk2-platforms 1/4] Silicon/NXP: Add GPIO driver support.

["Leif Lindholm" <leif@nuviainc.com>]

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 <pramod.kumar_1@nxp.com>
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>

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 <Uefi.h>
> +
> +/* 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 <Library/GpioLib.h>
> +#include <Library/IoAccessLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BaseLib.h>
> +
> +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
>