Re: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library

[Chris Co <Christopher.Co@microsoft.com>]

Hi Leif,

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Thursday, November 8, 2018 10:00 AM
> To: Chris Co <Christopher.Co@microsoft.com>
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Michael D Kinney <michael.d.kinney@intel.com>
> Subject: Re: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX
> library
> 
> On Fri, Sep 21, 2018 at 08:26:03AM +0000, Chris Co wrote:
> > This adds support for initializing and manipulating the I/O Pads on
> > NXP i.MX6 SoCs.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > ---
> >  Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c      | 151
> ++++++++++++++++++++
> >  Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf |  41
> > ++++++
> >  2 files changed, 192 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > new file mode 100644
> > index 000000000000..7c0c7b54a2fe
> > --- /dev/null
> > +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c
> > @@ -0,0 +1,151 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018 Microsoft Corporation. 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
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b
> f86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670&amp;sdata=
> vkaVmLhCg
> > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3D&amp;reserved=0
> > +*
> > +*  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 <PiDxe.h>
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/IoLib.h>
> > +
> > +#include <iMX6.h>
> > +#include <iMX6IoMux.h>
> > +
> > +// Muxing functions
> > +VOID
> > +ImxPadConfig (
> > +  IN  IMX_PAD     Pad,
> > +  IN  IMX_PADCFG  PadConfig
> 
> I'll get back to reviewing patch 11 at some point, but that one is hard going. So
> I'll point out here what I'll mention then:
> Please just use UINT64. Typedefs are useful to reduce pointless repetition for
> structs, but here it just obscures what is programatically going on.
> 

I'm thinking to rework the PADCFG to actually use structs properly instead of custom-packing with macros. It seems the previous dev was very macro happy and I am the opposite...
The remaining feedback makes sense. Will address for v2.

Thanks,
Chris

> > +  )
> > +{
> > +  // Configure Mux Control
> > +  MmioWrite32 (
> > +    IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad),
> > +    _IMX_PADCFG_MUX_CTL (PadConfig));
> 
> It would be worth adding macros or simple accessor functions for these - there's
> a lot of text in this file that has no semantic value and needs to be manually
> filtered when reading.
> 
> I.e. IomuxWrite32 (Pad, ...), IomuxRead32 (Pad), ...
> 
> Other comment really belonging to 11:
> Please drop leading _ from macros. Such macros are intended for internal use
> by toolchains.
> 
> > +
> > +  // Configure Select Input Control
> > +  if (_IMX_PADCFG_SEL_INP (PadConfig) != 0) {
> > +    DEBUG ((DEBUG_INFO, "Setting INPUT_SELECT %x value %x\n",
> > +            _IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)),
> > +            _IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig))));
> > +
> > +    MmioWrite32 (
> > +      _IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)),
> > +      _IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig)));  }
> > +
> > +  // Configure Pad Control
> > +  MmioWrite32 (
> > +    IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad),
> > +    _IMX_PADCFG_PAD_CTL (PadConfig)); }
> > +
> > +VOID
> > +ImxPadDumpConfig (
> > +  IN  CHAR8 *SignalFriendlyName,
> > +  IN  IMX_PAD Pad
> > +  )
> > +{
> > +  IMX_IOMUXC_MUX_CTL  MuxCtl;
> > +  IMX_IOMUXC_PAD_CTL  PadCtl;
> > +
> > +  MuxCtl.AsUint32 = MmioRead32 (
> > +                      IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad));
> > +
> > +  DEBUG ((
> > +           DEBUG_INIT,
> > +           "- %a MUX_CTL(0x%p)=0x%08x: MUX_MODE:%d SION:%d | ",
> > +           SignalFriendlyName,
> > +           IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad),
> > +           MuxCtl.AsUint32,
> > +           MuxCtl.Fields.MUX_MODE,
> > +           MuxCtl.Fields.SION));
> > +
> > +  PadCtl.AsUint32 = MmioRead32 (
> > +                      IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad));
> > +
> > +  DEBUG ((
> > +           DEBUG_INIT,
> > +           "PAD_CTL(0x%p)=0x%08x: SRE:%d DSE:%d SPEED:%d ODE:%d PKE:%d
> PUE:%d PUS:%d HYS:%d\n",
> > +           IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad),
> > +           PadCtl.AsUint32,
> > +           PadCtl.Fields.SRE,
> > +           PadCtl.Fields.DSE,
> > +           PadCtl.Fields.SPEED,
> > +           PadCtl.Fields.ODE,
> > +           PadCtl.Fields.PKE,
> > +           PadCtl.Fields.PUE,
> > +           PadCtl.Fields.PUS,
> > +           PadCtl.Fields.HYS));
> > +}
> > +
> > +// GPIO functions
> > +VOID
> > +ImxGpioDirection (
> > +  IN  IMX_GPIO_BANK   Bank,
> > +  IN  UINT32          IoNumber,
> > +  IN  IMX_GPIO_DIR    Direction
> > +  )
> > +{
> > +  volatile IMX_GPIO_REGISTERS   *gpioRegisters;
> 
> What makes this pointer volatile?
> (Hint: drop it, it does nothing useful here.)
> 
> That initial 'g', following EDK2 naming rules, says that this is a variable in the
> global namespace, exported from this module. It should be GpioRegisters.
> 
> > +
> > +  ASSERT (IoNumber < 32);
> 
> That 32 needs a #define.
> 
> > +
> > +  gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE;  if
> > + (Direction == IMX_GPIO_DIR_INPUT) {
> > +    MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, ~ (1 <<
> > + IoNumber));
> 
> This 'Bank - 1' stuff looks a bit scary. Why aren't we passing the inde to use? A
> comment block before the function could explain what the inputs are meant to
> be.
> 
> > +  } else {
> > +    MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, 1 <<
> > + IoNumber);
> 
> Since we're doing 1 << IoNumber on all possible routes through this function,
> could we assign it to a temporary variable? And we're doing it to the same bank.
> 
> If I wrote this function, I'd probably do something more like
> 
>   UINTN DirectionRegister;
>   UINT32 Pin;
>   DirectionRegister = (UINTN)&((IMX_GPIO_REGISTERS *)IMX_GPIO_BASE)-
> >Banks[Bank - 1].GDIR;
>   Pin = 1 << IoNumber;
> 
>   if (Direction == IMX_GPIO_DIR_INPUT) {
>     MmioAnd32 (DirectionRegister, ~Pin);
>   } else {
>     MmioOr32 (DirectionRegister, Pin);
>   }
> 
> > +  }
> > +}
> > +
> > +VOID
> > +ImxGpioWrite (
> > +  IN  IMX_GPIO_BANK   Bank,
> > +  IN  UINT32          IoNumber,
> > +  IN  IMX_GPIO_VALUE  Value
> > +  )
> > +{
> > +  volatile IMX_GPIO_REGISTERS   *gpioRegisters;
> > +
> > +  ASSERT (IoNumber < 32);
> > +
> > +  gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE;  if (Value ==
> > + IMX_GPIO_LOW) {
> > +    MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, ~ (1 <<
> > + IoNumber));  } else {
> > +    MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, 1 <<
> > + IoNumber);  }
> 
> All the same comments as above.
> 
> > +}
> > +
> > +IMX_GPIO_VALUE
> > +ImxGpioRead (
> > +  IN  IMX_GPIO_BANK   Bank,
> > +  IN  UINT32          IoNumber
> > +  )
> > +{
> > +  volatile IMX_GPIO_REGISTERS   *gpioRegisters;
> > +  UINT32                        Mask;
> > +  UINT32                        Psr;
> > +
> > +  ASSERT (IoNumber < 32);
> > +
> > +  gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE;  Mask = (1 <<
> > + IoNumber);  Psr = MmioRead32 ((UINTN) &gpioRegisters->Banks[Bank -
> > + 1].PSR);
> > +
> > +  if (Psr & Mask) {
> > +    return IMX_GPIO_HIGH;
> > +  } else {
> > +    return IMX_GPIO_LOW;
> > +  }
> 
> Some of the same comments as above :)
> (I.e., those that apply.)
> 
> /
>     Leif
> 
> > +}
> > diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
> > b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
> > new file mode 100644
> > index 000000000000..84bbbee5c1db
> > --- /dev/null
> > +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
> > @@ -0,0 +1,41 @@
> > +## @file
> > +#
> > +#  Copyright (c) 2018 Microsoft Corporation. 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 #
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b
> f86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670&amp;sdata=
> vkaVmLhCg
> > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3D&amp;reserved=0
> > +#
> > +#  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                      = iMX6IoMuxLib
> > +  FILE_GUID                      = FA41BEF0-0666-4C07-9EC3-47F61C36EDBE
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = iMX6IoMuxLib
> > +
> > +[Packages]
> > +  ArmPkg/ArmPkg.dec
> > +  EmbeddedPkg/EmbeddedPkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/iMX6Pkg/iMX6Pkg.dec
> > +  Silicon/NXP/iMXPlatformPkg/iMXPlatformPkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseMemoryLib
> > +  DebugLib
> > +  IoLib
> > +  TimerLib
> > +
> > +[Sources.common]
> > +  iMX6IoMux.c
> > +
> > + [FixedPcd]
> > +  giMXPlatformTokenSpaceGuid.PcdGpioBankMemoryRange
> > --
> > 2.16.2.gvfs.1.33.gf5370f1
> >