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

[Leif Lindholm <leif.lindholm@linaro.org>]

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
> +*  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 <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.

> +  )
> +{
> +  // 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
> +#  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                      = 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
>