[U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for Intel Tangier

Bin Meng bmeng.cn at gmail.com
Sun Sep 3 09:46:14 UTC 2017


Hi Andy,

On Wed, Aug 30, 2017 at 11:04 PM, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
> Intel Tangier SoC is a part of Intel Merrifield platform which doesn't
> utilize ACPI by default. Here is an attempt to unleash ACPI flexibility
> power on Intel Merrifield based platforms.
>
> The change brings minimum support of the devices that found on
> Intel Merrifield based end user device.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> ---
>  arch/x86/cpu/tangier/Makefile                      |   1 +
>  arch/x86/cpu/tangier/acpi.c                        |  86 ++++++
>  .../include/asm/arch-tangier/acpi/global_nvs.asl   |  16 ++
>  .../x86/include/asm/arch-tangier/acpi/platform.asl |  31 +++
>  .../include/asm/arch-tangier/acpi/southcluster.asl | 306 +++++++++++++++++++++
>  arch/x86/include/asm/arch-tangier/global_nvs.h     |  22 ++
>  6 files changed, 462 insertions(+)
>  create mode 100644 arch/x86/cpu/tangier/acpi.c
>  create mode 100644 arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl
>  create mode 100644 arch/x86/include/asm/arch-tangier/acpi/platform.asl
>  create mode 100644 arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
>  create mode 100644 arch/x86/include/asm/arch-tangier/global_nvs.h
>

Generally it looks good. Some comments below.

> diff --git a/arch/x86/cpu/tangier/Makefile b/arch/x86/cpu/tangier/Makefile
> index d146b3f5c2..92cfa555ed 100644
> --- a/arch/x86/cpu/tangier/Makefile
> +++ b/arch/x86/cpu/tangier/Makefile
> @@ -5,3 +5,4 @@
>  #
>
>  obj-y += car.o tangier.o sdram.o
> +obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o
> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c
> new file mode 100644
> index 0000000000..fb15ce40ad
> --- /dev/null
> +++ b/arch/x86/cpu/tangier/acpi.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Partially based on acpi.c for other x86 platforms
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <cpu.h>
> +#include <dm.h>
> +#include <dm/uclass-internal.h>
> +#include <asm/acpi_table.h>
> +#include <asm/ioapic.h>
> +#include <asm/mpspec.h>
> +#include <asm/tables.h>
> +#include <asm/arch/global_nvs.h>
> +
> +void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs,
> +                     void *dsdt)
> +{
> +       struct acpi_table_header *header = &(fadt->header);
> +
> +       memset((void *)fadt, 0, sizeof(struct acpi_fadt));
> +
> +       acpi_fill_header(header, "FACP");
> +       header->length = sizeof(struct acpi_fadt);
> +       header->revision = 6;
> +
> +       fadt->firmware_ctrl = (u32)facs;
> +       fadt->dsdt = (u32)dsdt;
> +       fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;
> +
> +       fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |
> +                              ACPI_FADT_NO_PCIE_ASPM_CONTROL;
> +       fadt->flags =
> +               ACPI_FADT_WBINVD |
> +               ACPI_FADT_POWER_BUTTON | ACPI_FADT_SLEEP_BUTTON |
> +               ACPI_FADT_SEALED_CASE | ACPI_FADT_HEADLESS |
> +               ACPI_FADT_HW_REDUCED_ACPI;
> +
> +       fadt->minor_revision = 2;
> +
> +       fadt->x_firmware_ctl_l = (u32)facs;
> +       fadt->x_firmware_ctl_h = 0;
> +       fadt->x_dsdt_l = (u32)dsdt;
> +       fadt->x_dsdt_h = 0;
> +
> +       header->checksum = table_compute_checksum(fadt, header->length);
> +}
> +
> +u32 acpi_fill_madt(u32 current)
> +{
> +       current += acpi_create_madt_lapics(current);
> +
> +       current += acpi_create_madt_ioapic((struct acpi_madt_ioapic *)current,
> +                       io_apic_read(IO_APIC_ID) >> 24, IO_APIC_ADDR, 0);
> +
> +       return current;
> +}
> +
> +u32 acpi_fill_mcfg(u32 current)
> +{
> +       current += acpi_create_mcfg_mmconfig
> +               ((struct acpi_mcfg_mmconfig *)current,
> +               0x3f500000, 0x0, 0x0, 0x0);

What is 0x3f500000? Can we define something in asm/arch/iomap.h (like
Baytrail) and use it here? And I see the end_bus_number is set to zero
here, why? Is this table a faked one? How about completely drop this
table? Does Linux boot without this table?

> +
> +       return current;
> +}
> +
> +void acpi_create_gnvs(struct acpi_global_nvs *gnvs)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       /* at least we have one processor */
> +       gnvs->pcnt = 1;
> +
> +       /* override the processor count with actual number */
> +       ret = uclass_find_first_device(UCLASS_CPU, &dev);
> +       if (ret == 0 && dev != NULL) {
> +               ret = cpu_get_count(dev);
> +               if (ret > 0)
> +                       gnvs->pcnt = ret;
> +       }
> +}
> diff --git a/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl b/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl
> new file mode 100644
> index 0000000000..84fffbe140
> --- /dev/null
> +++ b/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Partially based on global_nvs.asl for other x86 platforms
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <asm/acpi/global_nvs.h>
> +
> +OperationRegion(GNVS, SystemMemory, ACPI_GNVS_ADDR, ACPI_GNVS_SIZE)
> +Field(GNVS, ByteAcc, NoLock, Preserve)
> +{
> +    Offset (0x00),
> +    PCNT, 2,    /* processor count */

2 is weird here. Why only two bits? I believe it should be 8 per your
global_nvs.h defines.

> +}
> diff --git a/arch/x86/include/asm/arch-tangier/acpi/platform.asl b/arch/x86/include/asm/arch-tangier/acpi/platform.asl
> new file mode 100644
> index 0000000000..a57b7cb319
> --- /dev/null
> +++ b/arch/x86/include/asm/arch-tangier/acpi/platform.asl
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Partially based on platform.asl for other x86 platforms
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <asm/acpi/statdef.asl>
> +
> +/*
> + * The _PTS method (Prepare To Sleep) is called before the OS is
> + * entering a sleep state. The sleep state number is passed in Arg0.
> + */
> +Method(_PTS, 1)
> +{
> +}
> +
> +/* The _WAK method is called on system wakeup */
> +Method(_WAK, 1)
> +{
> +    Return (Package() {0, 0})
> +}
> +
> +/* ACPI global NVS */
> +#include "global_nvs.asl"
> +
> +Scope (\_SB)
> +{
> +    #include "southcluster.asl"
> +}
> diff --git a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
> new file mode 100644
> index 0000000000..d3a9b114cb
> --- /dev/null
> +++ b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl
> @@ -0,0 +1,306 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Partially based on southcluster.asl for other x86 platforms
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +Device (PCI0)
> +{
> +    Name (_HID, EISAID("PNP0A08"))    /* PCIe */
> +    Name (_CID, EISAID("PNP0A03"))    /* PCI */
> +
> +    Name (_ADR, 0)
> +    Name (_BBN, 0)
> +
> +    Name (MCRS, ResourceTemplate()
> +    {
> +        /* Bus Numbers */
> +        WordBusNumber(ResourceProducer, MinFixed, MaxFixed, PosDecode,
> +                0x0000, 0x0000, 0x00ff, 0x0000, 0x0100, , , PB00)
> +
> +        /* IO Region 0 */
> +        WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
> +                0x0000, 0x0000, 0x0cf7, 0x0000, 0x0cf8, , , PI00)
> +
> +        /* PCI Config Space */
> +        IO(Decode16, 0x0cf8, 0x0cf8, 0x0001, 0x0008)
> +
> +        /* IO Region 1 */
> +        WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
> +                0x0000, 0x0d00, 0xffff, 0x0000, 0xf300, , , PI01)
> +
> +        /* GPIO Low Memory Region */
> +        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,
> +                Cacheable, ReadWrite,
> +                0x00000000, 0x000ddcc0, 0x000ddccf, 0x00000000,
> +                0x00000010, , , GP00)
> +
> +        /* PSH Memory Region 0 */
> +        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,
> +                Cacheable, ReadWrite,
> +                0x00000000, 0x04819000, 0x04898fff, 0x00000000,
> +                0x00080000, , , PSH0)
> +
> +        /* PSH Memory Region 1 */
> +        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,
> +                Cacheable, ReadWrite,
> +                0x00000000, 0x04919000, 0x04920fff, 0x00000000,
> +                0x00008000, , , PSH1)
> +
> +        /* SST Memory Region */
> +        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,
> +                Cacheable, ReadWrite,
> +                0x00000000, 0x05e00000, 0x05ffffff, 0x00000000,
> +                0x00200000, , , SST0)
> +
> +        /* PCI Memory Region */
> +        DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,
> +                Cacheable, ReadWrite,
> +                0x00000000, 0x80000000, 0xffffffff, 0x00000000,
> +                0x80000000, , , PMEM)
> +    })
> +
> +    Method (_CRS, 0, Serialized)
> +    {
> +        Return (MCRS)
> +    }
> +
> +    Method (_OSC, 4)
> +    {
> +        /* Check for proper GUID */
> +        If (LEqual(Arg0, ToUUID("33db4d5b-1ff7-401c-9657-7441c03dd766"))) {
> +            /* Let OS control everything */
> +            Return (Arg3)
> +        } Else {
> +            /* Unrecognized UUID */
> +            CreateDWordField(Arg3, 0, CDW1)
> +            Or(CDW1, 4, CDW1)
> +            Return (Arg3)
> +        }
> +    }
> +
> +    Device (SDHC)
> +    {
> +        Name (_ADR, 0x00010003)
> +        Name (_DEP, Package (0x01)
> +        {
> +            GPIO
> +        })
> +        Name (PSTS, Zero)
> +
> +        Method (_STA)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +
> +        Method (_PS3, 0, NotSerialized)
> +        {
> +        }
> +
> +        Method (_PS0, 0, NotSerialized)
> +        {
> +            If (PSTS == Zero)
> +            {

nits: can we do something similar to U-Boot coding style with these If/Else?

> +                If (^^GPIO.AVBL == One)
> +                {
> +                    ^^GPIO.WFD3 = One
> +                    PSTS = One
> +                }
> +            }
> +        }
> +
> +        /* BCM43340 */
> +        Device (BRC1)
> +        {
> +            Name (_ADR, One)

nits: since it represents an address, I would use 0x01 instead of One

> +            Name (_DEP, Package (0x01)
> +            {
> +                GPIO
> +            })
> +
> +            Method (_STA)
> +            {
> +                Return (STA_VISIBLE)
> +            }
> +
> +            Method (_RMV, 0, NotSerialized)
> +            {
> +                Return (Zero)
> +            }
> +
> +            Method (_PS3, 0, NotSerialized)
> +            {
> +                If (^^^GPIO.AVBL == One)
> +                {
> +                    ^^^GPIO.WFD3 = Zero
> +                    PSTS = Zero
> +                }
> +            }
> +
> +            Method (_PS0, 0, NotSerialized)
> +            {
> +                If (PSTS == Zero)
> +                {
> +                    If (^^^GPIO.AVBL == One)
> +                    {
> +                        ^^^GPIO.WFD3 = One
> +                        PSTS = One
> +                    }
> +                }
> +            }
> +        }
> +
> +        Device (BRC2)
> +        {
> +            Name (_ADR, 0x02)
> +            Method (_STA, 0, NotSerialized)
> +            {
> +                Return (STA_VISIBLE)
> +            }
> +
> +            Method (_RMV, 0, NotSerialized)
> +            {
> +                Return (Zero)
> +            }
> +        }
> +    }
> +
> +    Device (SPI5)
> +    {
> +        Name (_ADR, 0x00070001)
> +        Name (_DEP, Package (0x01)
> +        {
> +            GPIO
> +        })
> +        Name (RBUF, ResourceTemplate()
> +        {
> +            GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
> +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 91 }

nits: can we use relative path here, like "GPIO"?

> +            GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
> +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 92 }
> +            GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
> +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 93 }
> +            GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
> +                "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 94 }
> +        })
> +
> +        Method (_CRS, 0, NotSerialized)
> +        {
> +            Return (RBUF)
> +        }
> +
> +        /*
> +         * See
> +         * http://www.kernel.org/doc/Documentation/acpi/gpio-properties.txt
> +         * for more information about GPIO bindings.
> +         */
> +        Name (_DSD, Package () {
> +            ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +            Package () {
> +                Package () {
> +                    "cs-gpios", Package () {
> +                        ^SPI5, 0, 0, 0,
> +                        ^SPI5, 1, 0, 0,
> +                        ^SPI5, 2, 0, 0,
> +                        ^SPI5, 3, 0, 0,
> +                    },
> +                },
> +            }
> +        })
> +
> +        Method (_STA, 0, NotSerialized)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +    }
> +
> +    Device (I2C1)
> +    {
> +        Name (_ADR, 0x00080000)
> +
> +        Method (_STA, 0, NotSerialized)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +    }
> +
> +    Device (GPIO)
> +    {
> +        Name (_ADR, 0x000c0000)
> +        Name (_DEP, Package (0x01)
> +        {
> +            \_SB.FLIS
> +        })

Looks _DEP method is supported only for Operation Regions as I read
from ACPI spec? Does it work here?

> +
> +        Method (_STA)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +
> +        Name (AVBL, Zero)
> +        Method (_REG, 2, NotSerialized)
> +        {
> +            If (Arg0 == 0x08)

I assume 0x08 here means GeneralPurposeIO which is one of Operation
Region Address Space Identifiers Values? If yes, could we add
something in arch/x86/include/asm/acpi/ and use macro here?

> +            {
> +                AVBL = Arg1
> +            }
> +        }
> +
> +        OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x04)
> +        Field (GPOP, ByteAcc, NoLock, Preserve)
> +        {
> +            Connection (
> +                GpioIo(Exclusive, PullDefault, 0, 0, IoRestrictionOutputOnly,
> +                    "\\_SB.PCI0.GPIO", 0, ResourceConsumer, , ) { 56 }
> +            ),
> +            WFD3, 1,
> +        }
> +    }
> +
> +    Device (PWM0)
> +    {
> +        Name (_ADR, 0x00170000)
> +
> +        Method (_STA, 0, NotSerialized)
> +        {
> +            Return (STA_VISIBLE)
> +        }
> +    }
> +}
> +
> +Device (FLIS)
> +{
> +    Name (_HID, "PRP0001")
> +    Name (_DDN, "Intel Merrifield Family-Level Interface Shim")
> +    Name (RBUF, ResourceTemplate()
> +    {
> +        Memory32Fixed(ReadWrite, 0xFF0C0000, 0x00008000, )
> +        PinGroup("spi5", ResourceProducer, ) { 90, 91, 92, 93, 94, 95, 96 }
> +        PinGroup("uart0", ResourceProducer, ) { 115, 116, 117, 118 }
> +        PinGroup("uart1", ResourceProducer, ) { 119, 120, 121, 122 }
> +        PinGroup("uart2", ResourceProducer, ) { 123, 124, 125, 126 }
> +        PinGroup("pwm0", ResourceProducer, ) { 144 }
> +        PinGroup("pwm1", ResourceProducer, ) { 145 }
> +        PinGroup("pwm2", ResourceProducer, ) { 132 }
> +        PinGroup("pwm3", ResourceProducer, ) { 133 }
> +    })
> +
> +    Method (_CRS, 0, NotSerialized)
> +    {
> +        Return (RBUF)
> +    }
> +
> +    Name (_DSD, Package () {
> +        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

Is this new UUID supposed to describe pinctrl? I don't see it in
include/acpi/acuuid.h in the kernel tree.

> +        Package () {
> +            Package () {"compatible", Package () {"intel,merrifield-pinctrl"}},

Is the 2nd Package() necessary? Like just below, is it OK?

Package () {"compatible", "intel,merrifield-pinctrl"},

I believe the intel,merrifield-pinctrl driver is an OF driver now, but
I don't see such string in current kernel tree. ACPI v6.2 has native
support of pinctrl, but kernel is not ready for it, so this is a
temporary placeholder?

> +        }
> +    })
> +
> +    Method (_STA, 0, NotSerialized)
> +    {
> +        Return (STA_VISIBLE)
> +    }
> +}
> diff --git a/arch/x86/include/asm/arch-tangier/global_nvs.h b/arch/x86/include/asm/arch-tangier/global_nvs.h
> new file mode 100644
> index 0000000000..8ab5cf2aa2
> --- /dev/null
> +++ b/arch/x86/include/asm/arch-tangier/global_nvs.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Partially based on global_nvs.h for other x86 platforms
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef _GLOBAL_NVS_H_
> +#define _GLOBAL_NVS_H_
> +
> +struct __packed acpi_global_nvs {
> +       u8      pcnt;           /* processor count */
> +
> +       /*
> +        * Add padding so sizeof(struct acpi_global_nvs) == 0x100.
> +        * This must match the size defined in the global_nvs.asl.
> +        */
> +       u8      rsvd[255];
> +};
> +
> +#endif /* _GLOBAL_NVS_H_ */
> --

Regards,
Bin


More information about the U-Boot mailing list