Re: [edk2] [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library
Hi Leif,
> -Original Message-
> From: Leif Lindholm
> Sent: Thursday, November 8, 2018 10:00 AM
> To: Chris Co
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney
> 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 +, 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
> > Cc: Ard Biesheuvel
> > Cc: Leif Lindholm
> > Cc: Michael D Kinney
> > ---
> > 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 ..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.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b
> f86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670sdata=
> vkaVmLhCg
> > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3Dreserved=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
> > +
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +// 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,
> > +
Re: [edk2] [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library
On Fri, Sep 21, 2018 at 08:26:03AM +, 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
> Cc: Ard Biesheuvel
> Cc: Leif Lindholm
> Cc: Michael D Kinney
> ---
> 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 ..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
> +
> +#include
> +#include
> +
> +#include
> +#include
> +
> +// 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_DIRDirection
> + )
> +{
> + 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) >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
[edk2] [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library
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
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Michael D Kinney
---
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 ..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
+
+#include
+#include
+
+#include
+#include
+
+// Muxing functions
+VOID
+ImxPadConfig (
+ IN IMX_PAD Pad,
+ IN IMX_PADCFG PadConfig
+ )
+{
+ // Configure Mux Control
+ MmioWrite32 (
+IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad),
+_IMX_PADCFG_MUX_CTL (PadConfig));
+
+ // 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_DIRDirection
+ )
+{
+ volatile IMX_GPIO_REGISTERS *gpioRegisters;
+
+ ASSERT (IoNumber < 32);
+
+ gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE;
+ if (Direction == IMX_GPIO_DIR_INPUT) {
+MmioAnd32 ((UINTN) >Banks[Bank - 1].GDIR, ~ (1 <<
IoNumber));
+ } else {
+MmioOr32 ((UINTN) >Banks[Bank - 1].GDIR, 1 << IoNumber);
+ }
+}
+
+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) >Banks[Bank - 1].DR, ~ (1 << IoNumber));
+ } else {
+MmioOr32 ((UINTN) >Banks[Bank - 1].DR, 1 << IoNumber);
+ }
+}
+
+IMX_GPIO_VALUE
+ImxGpioRead (
+ IN IMX_GPIO_BANK Bank,
+ IN UINT32 IoNumber
+ )
+{
+ volatile IMX_GPIO_REGISTERS *gpioRegisters;
+ UINT32Mask;
+ UINT32Psr;
+
+ ASSERT (IoNumber < 32);
+
+ gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE;
+ Mask = (1 << IoNumber);
+ Psr = MmioRead32 ((UINTN) >Banks[Bank - 1].PSR);
+
+ if (Psr & Mask) {
+return IMX_GPIO_HIGH;
+ } else {
+return IMX_GPIO_LOW;
+ }
+}
diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
new file mode 100644
index ..84bbbee5c1db
--- /dev/null
+++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf
@@ -0,0 +1,41 @@
+## @file
+#
+# Copyright (c) 2018 Microsoft Corporation. All rights
