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


Leif Lindholm
 

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

Join devel@edk2.groups.io to automatically receive all group messages.