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

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

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
> +*  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.
> +*
> +**/
> +
> +#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?

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