[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