[{"id":1762234,"web_url":"http://patchwork.ozlabs.org/comment/1762234/","msgid":"","list_archive_url":null,"date":"2017-09-03T09:46:14","subject":"Re: [U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for\n\tIntel Tangier","submitter":{"id":64981,"url":"http://patchwork.ozlabs.org/api/people/64981/","name":"Bin Meng","email":"bmeng.cn@gmail.com"},"content":"Hi Andy,\n\nOn Wed, Aug 30, 2017 at 11:04 PM, Andy Shevchenko\n wrote:\n> Intel Tangier SoC is a part of Intel Merrifield platform which doesn't\n> utilize ACPI by default. Here is an attempt to unleash ACPI flexibility\n> power on Intel Merrifield based platforms.\n>\n> The change brings minimum support of the devices that found on\n> Intel Merrifield based end user device.\n>\n> Signed-off-by: Andy Shevchenko \n> ---\n> arch/x86/cpu/tangier/Makefile | 1 +\n> arch/x86/cpu/tangier/acpi.c | 86 ++++++\n> .../include/asm/arch-tangier/acpi/global_nvs.asl | 16 ++\n> .../x86/include/asm/arch-tangier/acpi/platform.asl | 31 +++\n> .../include/asm/arch-tangier/acpi/southcluster.asl | 306 +++++++++++++++++++++\n> arch/x86/include/asm/arch-tangier/global_nvs.h | 22 ++\n> 6 files changed, 462 insertions(+)\n> create mode 100644 arch/x86/cpu/tangier/acpi.c\n> create mode 100644 arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl\n> create mode 100644 arch/x86/include/asm/arch-tangier/acpi/platform.asl\n> create mode 100644 arch/x86/include/asm/arch-tangier/acpi/southcluster.asl\n> create mode 100644 arch/x86/include/asm/arch-tangier/global_nvs.h\n>\n\nGenerally it looks good. Some comments below.\n\n> diff --git a/arch/x86/cpu/tangier/Makefile b/arch/x86/cpu/tangier/Makefile\n> index d146b3f5c2..92cfa555ed 100644\n> --- a/arch/x86/cpu/tangier/Makefile\n> +++ b/arch/x86/cpu/tangier/Makefile\n> @@ -5,3 +5,4 @@\n> #\n>\n> obj-y += car.o tangier.o sdram.o\n> +obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o\n> diff --git a/arch/x86/cpu/tangier/acpi.c b/arch/x86/cpu/tangier/acpi.c\n> new file mode 100644\n> index 0000000000..fb15ce40ad\n> --- /dev/null\n> +++ b/arch/x86/cpu/tangier/acpi.c\n> @@ -0,0 +1,86 @@\n> +/*\n> + * Copyright (c) 2017 Intel Corporation\n> + *\n> + * Partially based on acpi.c for other x86 platforms\n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs,\n> + void *dsdt)\n> +{\n> + struct acpi_table_header *header = &(fadt->header);\n> +\n> + memset((void *)fadt, 0, sizeof(struct acpi_fadt));\n> +\n> + acpi_fill_header(header, \"FACP\");\n> + header->length = sizeof(struct acpi_fadt);\n> + header->revision = 6;\n> +\n> + fadt->firmware_ctrl = (u32)facs;\n> + fadt->dsdt = (u32)dsdt;\n> + fadt->preferred_pm_profile = ACPI_PM_UNSPECIFIED;\n> +\n> + fadt->iapc_boot_arch = ACPI_FADT_VGA_NOT_PRESENT |\n> + ACPI_FADT_NO_PCIE_ASPM_CONTROL;\n> + fadt->flags =\n> + ACPI_FADT_WBINVD |\n> + ACPI_FADT_POWER_BUTTON | ACPI_FADT_SLEEP_BUTTON |\n> + ACPI_FADT_SEALED_CASE | ACPI_FADT_HEADLESS |\n> + ACPI_FADT_HW_REDUCED_ACPI;\n> +\n> + fadt->minor_revision = 2;\n> +\n> + fadt->x_firmware_ctl_l = (u32)facs;\n> + fadt->x_firmware_ctl_h = 0;\n> + fadt->x_dsdt_l = (u32)dsdt;\n> + fadt->x_dsdt_h = 0;\n> +\n> + header->checksum = table_compute_checksum(fadt, header->length);\n> +}\n> +\n> +u32 acpi_fill_madt(u32 current)\n> +{\n> + current += acpi_create_madt_lapics(current);\n> +\n> + current += acpi_create_madt_ioapic((struct acpi_madt_ioapic *)current,\n> + io_apic_read(IO_APIC_ID) >> 24, IO_APIC_ADDR, 0);\n> +\n> + return current;\n> +}\n> +\n> +u32 acpi_fill_mcfg(u32 current)\n> +{\n> + current += acpi_create_mcfg_mmconfig\n> + ((struct acpi_mcfg_mmconfig *)current,\n> + 0x3f500000, 0x0, 0x0, 0x0);\n\nWhat is 0x3f500000? Can we define something in asm/arch/iomap.h (like\nBaytrail) and use it here? And I see the end_bus_number is set to zero\nhere, why? Is this table a faked one? How about completely drop this\ntable? Does Linux boot without this table?\n\n> +\n> + return current;\n> +}\n> +\n> +void acpi_create_gnvs(struct acpi_global_nvs *gnvs)\n> +{\n> + struct udevice *dev;\n> + int ret;\n> +\n> + /* at least we have one processor */\n> + gnvs->pcnt = 1;\n> +\n> + /* override the processor count with actual number */\n> + ret = uclass_find_first_device(UCLASS_CPU, &dev);\n> + if (ret == 0 && dev != NULL) {\n> + ret = cpu_get_count(dev);\n> + if (ret > 0)\n> + gnvs->pcnt = ret;\n> + }\n> +}\n> diff --git a/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl b/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl\n> new file mode 100644\n> index 0000000000..84fffbe140\n> --- /dev/null\n> +++ b/arch/x86/include/asm/arch-tangier/acpi/global_nvs.asl\n> @@ -0,0 +1,16 @@\n> +/*\n> + * Copyright (c) 2017 Intel Corporation\n> + *\n> + * Partially based on global_nvs.asl for other x86 platforms\n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +#include \n> +\n> +OperationRegion(GNVS, SystemMemory, ACPI_GNVS_ADDR, ACPI_GNVS_SIZE)\n> +Field(GNVS, ByteAcc, NoLock, Preserve)\n> +{\n> + Offset (0x00),\n> + PCNT, 2, /* processor count */\n\n2 is weird here. Why only two bits? I believe it should be 8 per your\nglobal_nvs.h defines.\n\n> +}\n> diff --git a/arch/x86/include/asm/arch-tangier/acpi/platform.asl b/arch/x86/include/asm/arch-tangier/acpi/platform.asl\n> new file mode 100644\n> index 0000000000..a57b7cb319\n> --- /dev/null\n> +++ b/arch/x86/include/asm/arch-tangier/acpi/platform.asl\n> @@ -0,0 +1,31 @@\n> +/*\n> + * Copyright (c) 2017 Intel Corporation\n> + *\n> + * Partially based on platform.asl for other x86 platforms\n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +#include \n> +\n> +/*\n> + * The _PTS method (Prepare To Sleep) is called before the OS is\n> + * entering a sleep state. The sleep state number is passed in Arg0.\n> + */\n> +Method(_PTS, 1)\n> +{\n> +}\n> +\n> +/* The _WAK method is called on system wakeup */\n> +Method(_WAK, 1)\n> +{\n> + Return (Package() {0, 0})\n> +}\n> +\n> +/* ACPI global NVS */\n> +#include \"global_nvs.asl\"\n> +\n> +Scope (\\_SB)\n> +{\n> + #include \"southcluster.asl\"\n> +}\n> diff --git a/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl\n> new file mode 100644\n> index 0000000000..d3a9b114cb\n> --- /dev/null\n> +++ b/arch/x86/include/asm/arch-tangier/acpi/southcluster.asl\n> @@ -0,0 +1,306 @@\n> +/*\n> + * Copyright (c) 2017 Intel Corporation\n> + *\n> + * Partially based on southcluster.asl for other x86 platforms\n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +Device (PCI0)\n> +{\n> + Name (_HID, EISAID(\"PNP0A08\")) /* PCIe */\n> + Name (_CID, EISAID(\"PNP0A03\")) /* PCI */\n> +\n> + Name (_ADR, 0)\n> + Name (_BBN, 0)\n> +\n> + Name (MCRS, ResourceTemplate()\n> + {\n> + /* Bus Numbers */\n> + WordBusNumber(ResourceProducer, MinFixed, MaxFixed, PosDecode,\n> + 0x0000, 0x0000, 0x00ff, 0x0000, 0x0100, , , PB00)\n> +\n> + /* IO Region 0 */\n> + WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,\n> + 0x0000, 0x0000, 0x0cf7, 0x0000, 0x0cf8, , , PI00)\n> +\n> + /* PCI Config Space */\n> + IO(Decode16, 0x0cf8, 0x0cf8, 0x0001, 0x0008)\n> +\n> + /* IO Region 1 */\n> + WordIO(ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,\n> + 0x0000, 0x0d00, 0xffff, 0x0000, 0xf300, , , PI01)\n> +\n> + /* GPIO Low Memory Region */\n> + DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,\n> + Cacheable, ReadWrite,\n> + 0x00000000, 0x000ddcc0, 0x000ddccf, 0x00000000,\n> + 0x00000010, , , GP00)\n> +\n> + /* PSH Memory Region 0 */\n> + DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,\n> + Cacheable, ReadWrite,\n> + 0x00000000, 0x04819000, 0x04898fff, 0x00000000,\n> + 0x00080000, , , PSH0)\n> +\n> + /* PSH Memory Region 1 */\n> + DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,\n> + Cacheable, ReadWrite,\n> + 0x00000000, 0x04919000, 0x04920fff, 0x00000000,\n> + 0x00008000, , , PSH1)\n> +\n> + /* SST Memory Region */\n> + DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,\n> + Cacheable, ReadWrite,\n> + 0x00000000, 0x05e00000, 0x05ffffff, 0x00000000,\n> + 0x00200000, , , SST0)\n> +\n> + /* PCI Memory Region */\n> + DWordMemory(ResourceProducer, PosDecode, MinFixed, MaxFixed,\n> + Cacheable, ReadWrite,\n> + 0x00000000, 0x80000000, 0xffffffff, 0x00000000,\n> + 0x80000000, , , PMEM)\n> + })\n> +\n> + Method (_CRS, 0, Serialized)\n> + {\n> + Return (MCRS)\n> + }\n> +\n> + Method (_OSC, 4)\n> + {\n> + /* Check for proper GUID */\n> + If (LEqual(Arg0, ToUUID(\"33db4d5b-1ff7-401c-9657-7441c03dd766\"))) {\n> + /* Let OS control everything */\n> + Return (Arg3)\n> + } Else {\n> + /* Unrecognized UUID */\n> + CreateDWordField(Arg3, 0, CDW1)\n> + Or(CDW1, 4, CDW1)\n> + Return (Arg3)\n> + }\n> + }\n> +\n> + Device (SDHC)\n> + {\n> + Name (_ADR, 0x00010003)\n> + Name (_DEP, Package (0x01)\n> + {\n> + GPIO\n> + })\n> + Name (PSTS, Zero)\n> +\n> + Method (_STA)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> +\n> + Method (_PS3, 0, NotSerialized)\n> + {\n> + }\n> +\n> + Method (_PS0, 0, NotSerialized)\n> + {\n> + If (PSTS == Zero)\n> + {\n\nnits: can we do something similar to U-Boot coding style with these If/Else?\n\n> + If (^^GPIO.AVBL == One)\n> + {\n> + ^^GPIO.WFD3 = One\n> + PSTS = One\n> + }\n> + }\n> + }\n> +\n> + /* BCM43340 */\n> + Device (BRC1)\n> + {\n> + Name (_ADR, One)\n\nnits: since it represents an address, I would use 0x01 instead of One\n\n> + Name (_DEP, Package (0x01)\n> + {\n> + GPIO\n> + })\n> +\n> + Method (_STA)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> +\n> + Method (_RMV, 0, NotSerialized)\n> + {\n> + Return (Zero)\n> + }\n> +\n> + Method (_PS3, 0, NotSerialized)\n> + {\n> + If (^^^GPIO.AVBL == One)\n> + {\n> + ^^^GPIO.WFD3 = Zero\n> + PSTS = Zero\n> + }\n> + }\n> +\n> + Method (_PS0, 0, NotSerialized)\n> + {\n> + If (PSTS == Zero)\n> + {\n> + If (^^^GPIO.AVBL == One)\n> + {\n> + ^^^GPIO.WFD3 = One\n> + PSTS = One\n> + }\n> + }\n> + }\n> + }\n> +\n> + Device (BRC2)\n> + {\n> + Name (_ADR, 0x02)\n> + Method (_STA, 0, NotSerialized)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> +\n> + Method (_RMV, 0, NotSerialized)\n> + {\n> + Return (Zero)\n> + }\n> + }\n> + }\n> +\n> + Device (SPI5)\n> + {\n> + Name (_ADR, 0x00070001)\n> + Name (_DEP, Package (0x01)\n> + {\n> + GPIO\n> + })\n> + Name (RBUF, ResourceTemplate()\n> + {\n> + GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,\n> + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 91 }\n\nnits: can we use relative path here, like \"GPIO\"?\n\n> + GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,\n> + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 92 }\n> + GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,\n> + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 93 }\n> + GpioIo(Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,\n> + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 94 }\n> + })\n> +\n> + Method (_CRS, 0, NotSerialized)\n> + {\n> + Return (RBUF)\n> + }\n> +\n> + /*\n> + * See\n> + * http://www.kernel.org/doc/Documentation/acpi/gpio-properties.txt\n> + * for more information about GPIO bindings.\n> + */\n> + Name (_DSD, Package () {\n> + ToUUID(\"daffd814-6eba-4d8c-8a91-bc9bbf4aa301\"),\n> + Package () {\n> + Package () {\n> + \"cs-gpios\", Package () {\n> + ^SPI5, 0, 0, 0,\n> + ^SPI5, 1, 0, 0,\n> + ^SPI5, 2, 0, 0,\n> + ^SPI5, 3, 0, 0,\n> + },\n> + },\n> + }\n> + })\n> +\n> + Method (_STA, 0, NotSerialized)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> + }\n> +\n> + Device (I2C1)\n> + {\n> + Name (_ADR, 0x00080000)\n> +\n> + Method (_STA, 0, NotSerialized)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> + }\n> +\n> + Device (GPIO)\n> + {\n> + Name (_ADR, 0x000c0000)\n> + Name (_DEP, Package (0x01)\n> + {\n> + \\_SB.FLIS\n> + })\n\nLooks _DEP method is supported only for Operation Regions as I read\nfrom ACPI spec? Does it work here?\n\n> +\n> + Method (_STA)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> +\n> + Name (AVBL, Zero)\n> + Method (_REG, 2, NotSerialized)\n> + {\n> + If (Arg0 == 0x08)\n\nI assume 0x08 here means GeneralPurposeIO which is one of Operation\nRegion Address Space Identifiers Values? If yes, could we add\nsomething in arch/x86/include/asm/acpi/ and use macro here?\n\n> + {\n> + AVBL = Arg1\n> + }\n> + }\n> +\n> + OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x04)\n> + Field (GPOP, ByteAcc, NoLock, Preserve)\n> + {\n> + Connection (\n> + GpioIo(Exclusive, PullDefault, 0, 0, IoRestrictionOutputOnly,\n> + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 56 }\n> + ),\n> + WFD3, 1,\n> + }\n> + }\n> +\n> + Device (PWM0)\n> + {\n> + Name (_ADR, 0x00170000)\n> +\n> + Method (_STA, 0, NotSerialized)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> + }\n> +}\n> +\n> +Device (FLIS)\n> +{\n> + Name (_HID, \"PRP0001\")\n> + Name (_DDN, \"Intel Merrifield Family-Level Interface Shim\")\n> + Name (RBUF, ResourceTemplate()\n> + {\n> + Memory32Fixed(ReadWrite, 0xFF0C0000, 0x00008000, )\n> + PinGroup(\"spi5\", ResourceProducer, ) { 90, 91, 92, 93, 94, 95, 96 }\n> + PinGroup(\"uart0\", ResourceProducer, ) { 115, 116, 117, 118 }\n> + PinGroup(\"uart1\", ResourceProducer, ) { 119, 120, 121, 122 }\n> + PinGroup(\"uart2\", ResourceProducer, ) { 123, 124, 125, 126 }\n> + PinGroup(\"pwm0\", ResourceProducer, ) { 144 }\n> + PinGroup(\"pwm1\", ResourceProducer, ) { 145 }\n> + PinGroup(\"pwm2\", ResourceProducer, ) { 132 }\n> + PinGroup(\"pwm3\", ResourceProducer, ) { 133 }\n> + })\n> +\n> + Method (_CRS, 0, NotSerialized)\n> + {\n> + Return (RBUF)\n> + }\n> +\n> + Name (_DSD, Package () {\n> + ToUUID(\"daffd814-6eba-4d8c-8a91-bc9bbf4aa301\"),\n\nIs this new UUID supposed to describe pinctrl? I don't see it in\ninclude/acpi/acuuid.h in the kernel tree.\n\n> + Package () {\n> + Package () {\"compatible\", Package () {\"intel,merrifield-pinctrl\"}},\n\nIs the 2nd Package() necessary? Like just below, is it OK?\n\nPackage () {\"compatible\", \"intel,merrifield-pinctrl\"},\n\nI believe the intel,merrifield-pinctrl driver is an OF driver now, but\nI don't see such string in current kernel tree. ACPI v6.2 has native\nsupport of pinctrl, but kernel is not ready for it, so this is a\ntemporary placeholder?\n\n> + }\n> + })\n> +\n> + Method (_STA, 0, NotSerialized)\n> + {\n> + Return (STA_VISIBLE)\n> + }\n> +}\n> diff --git a/arch/x86/include/asm/arch-tangier/global_nvs.h b/arch/x86/include/asm/arch-tangier/global_nvs.h\n> new file mode 100644\n> index 0000000000..8ab5cf2aa2\n> --- /dev/null\n> +++ b/arch/x86/include/asm/arch-tangier/global_nvs.h\n> @@ -0,0 +1,22 @@\n> +/*\n> + * Copyright (c) 2017 Intel Corporation\n> + *\n> + * Partially based on global_nvs.h for other x86 platforms\n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + */\n> +\n> +#ifndef _GLOBAL_NVS_H_\n> +#define _GLOBAL_NVS_H_\n> +\n> +struct __packed acpi_global_nvs {\n> + u8 pcnt; /* processor count */\n> +\n> + /*\n> + * Add padding so sizeof(struct acpi_global_nvs) == 0x100.\n> + * This must match the size defined in the global_nvs.asl.\n> + */\n> + u8 rsvd[255];\n> +};\n> +\n> +#endif /* _GLOBAL_NVS_H_ */\n> --\n\nRegards,\nBin","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"StGNNb9n\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xlSmP1Kjrz9s71\n\tfor ;\n\tSun, 3 Sep 2017 19:46:27 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 45576C21E1C; Sun, 3 Sep 2017 09:46:22 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id C23FFC21C59;\n\tSun, 3 Sep 2017 09:46:17 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 9F004C21C59; Sun, 3 Sep 2017 09:46:16 +0000 (UTC)","from mail-wm0-f66.google.com (mail-wm0-f66.google.com\n\t[74.125.82.66])\n\tby lists.denx.de (Postfix) with ESMTPS id 2F363C21C58\n\tfor ; Sun, 3 Sep 2017 09:46:16 +0000 (UTC)","by mail-wm0-f66.google.com with SMTP id p17so3866485wmd.3\n\tfor ; Sun, 03 Sep 2017 02:46:16 -0700 (PDT)","by 10.223.135.121 with HTTP; Sun, 3 Sep 2017 02:46:14 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID\n\tautolearn=unavailable autolearn_force=no version=3.4.0","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=UO9vrWzv7uvI3T+7IG5pC+JUdDQG0+YeojQcUpVGVAE=;\n\tb=StGNNb9nV2wXps/0RKo06oqyb5a5SACmrFPTZkkOfQp050mhop8CbGWaaRCq/CzJ55\n\t+mLsaw2HAFhsGqK9mJ4Y4lNHGS+3Yu9nIv1QAi3E9yzvkF4/wGFjtJawNlMV8d8Wu3nK\n\tcw4pLvB4g8Fqbc55zdsEW6KcASljL6Iki+wwAjG1hVOQ2alLLrF/362VOxoGG9eyKGhD\n\t5gH/4lS9ABeLsVFRLjnlqnPAVGol0m3Pqd9/qQpiUvf5n49QJCOKEdNlU8QokqWJAdbq\n\tPoP06PzefdMAjRoHHP7lZHJBPTNBCTYOkFZr/KCp+svixZZ+UxQEOdx+uUuTvNVr49t3\n\tW17A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=UO9vrWzv7uvI3T+7IG5pC+JUdDQG0+YeojQcUpVGVAE=;\n\tb=b9f9l0xxc3sJRkm2aJcQwcr/0uKPCI3aEssMdm8XwHVDYQTVPpIINIR1ZA5INJpbYG\n\truiMCxxCsU4DQYRgNA7o3hQEDJgB8vfi56h0ifQdelifrh5K8v4geMLknn7DAgENhmsA\n\tVfxpGcqw0o5llS8CtXbHpaA3bPTXDUpJfVgf1dw5EETjTA+MpJt68yhDMUzTXX5v6R6c\n\tArTLjtSXTnYhJRtlp/rg8lmuLoODt+kgZ3mSTvlgQ+gk3MO+8j+obwA+7hpcuCpgdkDY\n\tfCGbZZGVTASo6GHomrApcVUONOrs8WTAnXny9b8wpTE/5AWVqialz7pwrw9rkRo6xcm6\n\tDnKQ==","X-Gm-Message-State":"AHPjjUgVARdP9H0ylOkOp0WVhCkp4dgZScdgAFe5yqgt4Cox8WwsNh08\n\tF+4fYXW6mEzHWbSkw/j1pZtc4uHTog==","X-Google-Smtp-Source":"ADKCNb7knWJw8QOwQS1jpXiDxS5M3uo64vAY7Gnb1FM3w+tnCen8nmUS6R9jgwhuzkXi/9OSWcliVYu6sE3UVsp6yF8=","X-Received":"by 10.28.20.18 with SMTP id 18mr1974068wmu.34.1504431975598; Sun,\n\t03 Sep 2017 02:46:15 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170830150459.67452-2-andriy.shevchenko@linux.intel.com>","References":"<20170830150459.67452-1-andriy.shevchenko@linux.intel.com>\n\t<20170830150459.67452-2-andriy.shevchenko@linux.intel.com>","From":"Bin Meng ","Date":"Sun, 3 Sep 2017 17:46:14 +0800","Message-ID":"","To":"Andy Shevchenko ","Cc":"U-Boot Mailing List , Stefan Roese ,\n\tFerry Toth ","Subject":"Re: [U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for\n\tIntel Tangier","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1778269,"web_url":"http://patchwork.ozlabs.org/comment/1778269/","msgid":"<1506943566.16112.222.camel@linux.intel.com>","list_archive_url":null,"date":"2017-10-02T11:26:06","subject":"Re: [U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for\n\tIntel Tangier","submitter":{"id":8583,"url":"http://patchwork.ozlabs.org/api/people/8583/","name":"Andy Shevchenko","email":"andriy.shevchenko@linux.intel.com"},"content":"On Sun, 2017-09-03 at 17:46 +0800, Bin Meng wrote:\n> Hi Andy,\n> \n> On Wed, Aug 30, 2017 at 11:04 PM, Andy Shevchenko\n> wrote:\n> > Intel Tangier SoC is a part of Intel Merrifield platform which\n> > doesn't\n> > utilize ACPI by default. Here is an attempt to unleash ACPI\n> > flexibility\n> > power on Intel Merrifield based platforms.\n> > \n> > The change brings minimum support of the devices that found on\n> > Intel Merrifield based end user device.\n> > \n\n> Generally it looks good. Some comments below.\n\nThanks for review, my answers below.\n\n> > +u32 acpi_fill_mcfg(u32 current)\n> > +{\n> > + current += acpi_create_mcfg_mmconfig\n> > + ((struct acpi_mcfg_mmconfig *)current,\n> > + 0x3f500000, 0x0, 0x0, 0x0);\n> \n> What is 0x3f500000?\n\nNice question (I have in my local branch an additional commit with FIXME\nnear to this line)!\n\nThe root cause is a hardware design here. There are two (okay, on those\nplatforms up to four) *real* PCI devices. The rest have PCI\n*programming* interface but being not PCI at the same time. This is a\nmagic address of so called PCI Shim for those (non-PCI) devices which\nfiled by firmware.\n\n> Can we define something in asm/arch/iomap.h (like\n> Baytrail) and use it here?\n\nIt comes from SFI, so, the best approach is to parse SFI for that.\n\nI would not do this right now (will take a lot of time for not much\nbenefit), though can put as TODO.\n\n> And I see the end_bus_number is set to zero\n> here, why?\n\nSee above.\n\n> Is this table a faked one?\n\nIt's based on SFI.\n\n> How about completely drop this\n> table? Does Linux boot without this table?\n\nIt might start booting (as initial part), but it will lose an ability to\nenumerate almost all devices in SoC. Basically user will not get even\nconsole.\n\n> > +\n> > + return current;\n> > +}\n> > \n\n> > +OperationRegion(GNVS, SystemMemory, ACPI_GNVS_ADDR, ACPI_GNVS_SIZE)\n> > +Field(GNVS, ByteAcc, NoLock, Preserve)\n> > +{\n> > + Offset (0x00),\n> > + PCNT, 2, /* processor count */\n> \n> 2 is weird here. Why only two bits? I believe it should be 8 per your\n> global_nvs.h defines.\n\nAh, I misread what that number means. Will fix.\n\n> > + Method (_PS0, 0, NotSerialized)\n> > + {\n> > + If (PSTS == Zero)\n> > + {\n> \n> nits: can we do something similar to U-Boot coding style with these\n> If/Else?\n\nI would rather follow iasl style for ASL.\n\nThough if it's obliged to use U-Boot style over all files, I can switch.\n\n> \n> > + If (^^GPIO.AVBL == One)\n> > + {\n> > + ^^GPIO.WFD3 = One\n> > + PSTS = One\n> > + }\n> > + }\n> > + }\n> > +\n> > + /* BCM43340 */\n> > + Device (BRC1)\n> > + {\n> > + Name (_ADR, One)\n> \n> nits: since it represents an address, I would use 0x01 instead of One\n\nI have no strong opinion here, ACPI spec says\n\n\"The use of this operator can reduce AML code size, since it is\nrepresented by a one-byte AML opcode.\"\n\nSame for Zero.\n\nThough to be consistent with the rest, I will change it.\n\n> + Device (BRC2)\n> > + {\n> > + Name (_ADR, 0x02)\n> > \n\n> > + Name (RBUF, ResourceTemplate()\n> > + {\n> > + GpioIo(Exclusive, PullUp, 0, 0,\n> > IoRestrictionOutputOnly,\n> > + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 91 }\n> \n> nits: can we use relative path here, like \"GPIO\"?\n\nWhile spec says:\n\n\"ResourceSource can be a fully-qualified name, a relative name or a name\nsegment that utilizes the namespace search rules.\"\n\nI would rather follow common practice, i.e. to use fully-qualified name\nfor GPIO and Pin control resources.\n\n> \n> > + GpioIo(Exclusive, PullUp, 0, 0,\n> > IoRestrictionOutputOnly,\n> > + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 92 }\n> > + GpioIo(Exclusive, PullUp, 0, 0,\n> > IoRestrictionOutputOnly,\n> > + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 93 }\n> > + GpioIo(Exclusive, PullUp, 0, 0,\n> > IoRestrictionOutputOnly,\n> > + \"\\\\_SB.PCI0.GPIO\", 0, ResourceConsumer, , ) { 94 }\n> > \n\n> > + Device (GPIO)\n> > + {\n> > + Name (_ADR, 0x000c0000)\n> > + Name (_DEP, Package (0x01)\n> > + {\n> > + \\_SB.FLIS\n> > + })\n> \n> Looks _DEP method is supported only for Operation Regions as I read\n> from ACPI spec? Does it work here?\n\nNo, it doesn't (yet?). I would move it out to my debugging stuff.\n\n> > +\n> > + Method (_STA)\n> > + {\n> > + Return (STA_VISIBLE)\n> > + }\n> > +\n> > + Name (AVBL, Zero)\n> > + Method (_REG, 2, NotSerialized)\n> > + {\n> > + If (Arg0 == 0x08)\n> \n> I assume 0x08 here means GeneralPurposeIO which is one of Operation\n> Region Address Space Identifiers Values? If yes, could we add\n> something in arch/x86/include/asm/acpi/ and use macro here?\n\nThis part I got based on disassemble from iasl. Moreover, there are two\nscopes for this value: inside OperationRegion() and outside. Since iasl\ndoes not replace it with such (existing) value, I would follow it rather\nthan creating a potential collision with names and scopes.\n\n> > + Name (_DSD, Package () {\n> > + ToUUID(\"daffd814-6eba-4d8c-8a91-bc9bbf4aa301\"),\n> \n> Is this new UUID supposed to describe pinctrl? I don't see it in\n> include/acpi/acuuid.h in the kernel tree.\n\nNope, this UUID is supposed to describe device properties.\n\nPlease, refer to 6.2.5 _DSD (Device Specific Data) in the spec.\n\n> > + Package () {\n> > + Package () {\"compatible\", Package ()\n> > {\"intel,merrifield-pinctrl\"}},\n> \n> Is the 2nd Package() necessary? Like just below, is it OK?\n> Package () {\"compatible\", \"intel,merrifield-pinctrl\"},\n\nIn this case yes, we have only one value for the \"compatible\" key.\nI will change it.\n\n> I believe the intel,merrifield-pinctrl driver is an OF driver now,\n\nNo.\n\nThe only way for now to get it enumerated via ACPI is to apply\ncompatible string.\n\n> but I don't see such string in current kernel tree.\n\nI sent recently a v2 which adds such binding to the kernel sources.\n\n> ACPI v6.2 has native\n> support of pinctrl, but kernel is not ready for it, so this is a\n> temporary placeholder?\n\nThe first part of sentence has nothing to do with the question at the\nsecond.\n\nKernel is not ready for pin control glue layer (ACPICA already handles\nthat), but it is orthogonal to compatible strings.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y5KjZ3LwDz9t4X\n\tfor ;\n\tMon, 2 Oct 2017 22:30:57 +1100 (AEDT)","by lists.denx.de (Postfix, from userid 105)\n\tid 8832CC21E3D; Mon, 2 Oct 2017 11:30:49 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id ADC11C21C41;\n\tMon, 2 Oct 2017 11:30:46 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 39022C21C41; Mon, 2 Oct 2017 11:30:46 +0000 (UTC)","from mga11.intel.com (mga11.intel.com [192.55.52.93])\n\tby lists.denx.de (Postfix) with ESMTPS id CCF02C21C29\n\tfor ; Mon, 2 Oct 2017 11:30:42 +0000 (UTC)","from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t02 Oct 2017 04:30:39 -0700","from smile.fi.intel.com (HELO smile) ([10.237.72.86])\n\tby FMSMGA003.fm.intel.com with ESMTP; 02 Oct 2017 04:30:37 -0700"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=none autolearn=unavailable\n\tautolearn_force=no version=3.4.0","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,469,1500966000\"; d=\"scan'208\";a=\"905832388\"","Message-ID":"<1506943566.16112.222.camel@linux.intel.com>","From":"Andy Shevchenko ","To":"Bin Meng ","Date":"Mon, 02 Oct 2017 14:26:06 +0300","In-Reply-To":"","References":"<20170830150459.67452-1-andriy.shevchenko@linux.intel.com>\n\t<20170830150459.67452-2-andriy.shevchenko@linux.intel.com>\n\t","Organization":"Intel Finland Oy","X-Mailer":"Evolution 3.26.0-1 ","Mime-Version":"1.0","Cc":"U-Boot Mailing List , Stefan Roese ,\n\tFerry Toth ","Subject":"Re: [U-Boot] [PATCH v1 1/2] x86: tangier: Enable ACPI support for\n\tIntel Tangier","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}}]