Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use

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

Hi Leif,

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Thursday, November 1, 2018 11:20 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 09/27] Silicon/NXP: Add headers for SoC-
> specific i.MX packages to use
> 
> On Fri, Sep 21, 2018 at 08:26:00AM +0000, Chris Co wrote:
> > This adds common headers for other NXP i.MX SoC packages.
> > More specifically, this adds i.MX-generic GPIO, IoMux, and Platform
> > definitions.
> >
> > 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/iMXPlatformPkg/Include/Platform.h | 67 ++++++++++++++
> > Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h  | 92
> > ++++++++++++++++++++  Silicon/NXP/iMXPlatformPkg/Include/iMXIoMux.h |
> > 24 +++++
> >  3 files changed, 183 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > new file mode 100644
> > index 000000000000..8a1e828f68ea
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/Platform.h
> > @@ -0,0 +1,67 @@
> > +/** @file
> > +*
> > +*  i.MX Platform specific defines for constructing ACPI tables
> > +*
> > +*  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%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&amp;sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%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.
> > +*
> > +**/
> > +
> > +#ifndef _PLATFORM_IMX_H_
> > +#define _PLATFORM_IMX_H_
> > +
> > +#include <IndustryStandard/Acpi50.h>
> > +
> > +#define EFI_ACPI_OEM_ID               {'M','C','R','S','F','T'} // OEMID 6 bytes
> > +#define EFI_ACPI_VENDOR_ID            SIGNATURE_32('N','X','P','I')
> > +#define EFI_ACPI_CSRT_REVISION        0x00000005
> > +#define EFI_ACPI_5_0_CSRT_REVISION    0x00000000
> > +
> > +// Resource Descriptor Types
> > +#define EFI_ACPI_CSRT_RD_TYPE_INTERRUPT 1 #define
> > +EFI_ACPI_CSRT_RD_TYPE_TIMER 2 #define EFI_ACPI_CSRT_RD_TYPE_DMA 3
> > +#define EFI_ACPI_CSRT_RD_TYPE_CACHE 4
> > +
> > +// Resource Descriptor Subtypes
> > +#define EFI_ACPI_CSRT_RD_SUBTYPE_INTERRUPT_LINES 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_INTERRUPT_CONTROLLER 1 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_TIMER 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_DMA_CHANNEL 0 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_DMA_CONTROLLER 1 #define
> > +EFI_ACPI_CSRT_RD_SUBTYPE_CACHE 0
> 
> If using EFI_ACPI prefix, these #defines really should be in edk2 MdePkg. And
> CSRT itself is, so that might not be a bad idea.
> 
> > +
> > +#pragma pack(push, 1)
> 
> I don't see this #pragma making any difference to the structs below, can it be
> dropped?
> 

The pragma pack is defensive. Without it, we rely on the compiler packing structures by default and this may not happen on 64 bit compiles.

I have addressed the remaining feedback and will resubmit with v2.

Thanks,
Chris

> > +//-------------------------------------------------------------------
> > +----- // CSRT Resource Group header 24 bytes long
> > +//-------------------------------------------------------------------
> > +-----
> > +typedef struct {
> > +  UINT32 Length;
> > +  UINT32 VendorID;
> > +  UINT32 SubVendorId;
> > +  UINT16 DeviceId;
> > +  UINT16 SubdeviceId;
> > +  UINT16 Revision;
> > +  UINT16 Reserved;
> > +  UINT32 SharedInfoLength;
> > +} EFI_ACPI_5_0_CSRT_RESOURCE_GROUP_HEADER;
> > +
> > +//-------------------------------------------------------------------
> > +----- // CSRT Resource Descriptor 12 bytes total
> > +//-------------------------------------------------------------------
> > +-----
> > +typedef struct {
> > +  UINT32 Length;
> > +  UINT16 ResourceType;
> > +  UINT16 ResourceSubType;
> > +  UINT32 UID;
> > +} EFI_ACPI_5_0_CSRT_RESOURCE_DESCRIPTOR_HEADER;
> > +#pragma pack (pop)
> > +
> > +#endif // !_PLATFORM_IMX_H_
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > new file mode 100644
> > index 000000000000..dce01f789058
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXGpio.h
> > @@ -0,0 +1,92 @@
> > +/** @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%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&amp;sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%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.
> > +*
> > +**/
> > +
> > +#ifndef _IMX_GPIO_H_
> > +#define _IMX_GPIO_H_
> > +
> > +#include <Library/PcdLib.h>
> > +
> > +typedef enum {
> > +  IMX_GPIO_LOW = 0,
> > +  IMX_GPIO_HIGH = 1
> > +} IMX_GPIO_VALUE;
> > +
> > +typedef enum {
> > +  IMX_GPIO_DIR_INPUT,
> > +  IMX_GPIO_DIR_OUTPUT
> > +} IMX_GPIO_DIR;
> > +
> > +typedef enum {
> > +  IMX_GPIO_BANK1 = 1,
> > +  IMX_GPIO_BANK2,
> > +  IMX_GPIO_BANK3,
> > +  IMX_GPIO_BANK4,
> > +  IMX_GPIO_BANK5,
> > +  IMX_GPIO_BANK6,
> > +  IMX_GPIO_BANK7,
> > +} IMX_GPIO_BANK;
> > +
> > +#pragma pack(push, 1)
> 
> I don't see what effect this is supposed to have, can it be dropped?
> 
> > +
> > +#define GPIO_RESERVED_SIZE \
> > +    ((FixedPcdGet32(PcdGpioBankMemoryRange) / 4) - 8)
> > +
> > +typedef struct {
> > +  UINT32 DR;                            // 0x00 GPIO data register (GPIO1_DR)
> > +  UINT32 GDIR;                          // 0x04 GPIO direction register (GPIO1_GDIR)
> > +  UINT32 PSR;                           // 0x08 GPIO pad status register (GPIO1_PSR)
> > +  UINT32 ICR1;                          // 0x0C GPIO interrupt configuration register1
> (GPIO1_ICR1)
> > +  UINT32 ICR2;                          // 0x10 GPIO interrupt configuration register2
> (GPIO1_ICR2)
> > +  UINT32 IMR;                           // 0x14 GPIO interrupt mask register
> (GPIO1_IMR)
> > +  UINT32 ISR;                           // 0x18 GPIO interrupt status register
> (GPIO1_ISR)
> > +  UINT32 EDGE_SEL;                      // 0x1C GPIO edge select register
> (GPIO1_EDGE_SEL)
> > +  UINT32 reserved[GPIO_RESERVED_SIZE]; } IMX_GPIO_BANK_REGISTERS;
> > +
> > +#pragma pack(pop)
> > +
> > +typedef struct {
> > +  IMX_GPIO_BANK_REGISTERS Banks[7];
> > +} IMX_GPIO_REGISTERS;
> > +
> > +/**
> > +    Set the specified GPIO to the specified direction.
> > +**/
> > +VOID
> > +ImxGpioDirection (
> > +  IMX_GPIO_BANK Bank,
> > +  UINT32 IoNumber,
> > +  IMX_GPIO_DIR Direction
> > +  );
> > +
> > +/**
> > +    Write a value to a GPIO pin.
> > +**/
> > +VOID
> > +ImxGpioWrite (
> > +  IMX_GPIO_BANK Bank,
> > +  UINT32 IoNumber,
> > +  IMX_GPIO_VALUE Value
> > +  );
> > +
> > +/**
> > +    Read a GPIO pin input value.
> > +**/
> > +IMX_GPIO_VALUE
> > +ImxGpioRead (
> > +  IMX_GPIO_BANK Bank,
> > +  UINT32 IoNumber
> > +  );
> > +
> > +#endif
> > diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXIoMux.h
> > b/Silicon/NXP/iMXPlatformPkg/Include/iMXIoMux.h
> > new file mode 100644
> > index 000000000000..7696af57d7ea
> > --- /dev/null
> > +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXIoMux.h
> > @@ -0,0 +1,24 @@
> > +/** @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%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&amp;sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%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.
> > +*
> > +**/
> > +
> > +#ifndef _IMX_IO_MUX_H_
> > +#define _IMX_IO_MUX_H_
> > +
> > +#define _IMX_PAD(CtlRegOffset, MuxRegOffset) \
> 
> No _ for macros. Why does one pad an iMX? Can the name be more
> descriptive? (applies to next few macros too).
> 
> /
>     Leif
> 
> > +  ((((CtlRegOffset) & 0xffff) << 16) | ((MuxRegOffset) & 0xffff))
> > +
> > +#define _IMX_PAD_CTL_OFFSET(ImxPadVal) ((ImxPadVal) >> 16) #define
> > +_IMX_PAD_MUX_OFFSET(ImxPadVal) ((ImxPadVal) & 0xffff)
> > +
> > +#endif // _IMX_IO_MUX_H_
> > --
> > 2.16.2.gvfs.1.33.gf5370f1
> >