Hi Leif,
> -----Original Message-----
> From: Leif Lindholm <[email protected]>
> Sent: Thursday, November 1, 2018 11:20 AM
> To: Chris Co <[email protected]>
> Cc: [email protected]; Ard Biesheuvel <[email protected]>;
> Michael D Kinney <[email protected]>
> 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 <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>
> > Cc: Leif Lindholm <[email protected]>
> > Cc: Michael D Kinney <[email protected]>
> > ---
> > 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&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%3D&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&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%3D&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&data=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C53adf4afe364416fab0c08d64026b170%7C72f988bf8
> 6f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636766932265064004&sdata=lEf
> %2Bq2l
> > +nuxJ3cw%2BL91rhCTJ2e3jzWZfvgZZY8cyHswY%3D&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
> >
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
