This patch series requires the fixed-clock patches from Hanjun Guo.
Provides an ACPI module to probe ARM AMBA devices that have a hardware ID, which is used with struct amba_device to match up the appropriate driver. This currently uses the _DSM table for key/value pairs, which may need to be changed to use _PRP depending on the results of separate discussions.
See example of the required DSDT definition format below.
V2: Minor changes to address comments about #ifdef format and _DSM table tests.
Brandon Anderson (4): early fixed-clock AMBA bus ACPI implementation Add ACPI to AMBA SPI driver remove unneeded sections of DTS definition
arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 + arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 + drivers/acpi/acpi_platform.c | 2 + drivers/amba/Makefile | 2 +- drivers/amba/acpi.c | 338 +++++++++++++++++++++ drivers/clk/clk-fixed-rate.c | 2 +- drivers/spi/spi-pl022.c | 53 ++++ include/linux/amba/acpi.h | 29 ++ 9 files changed, 443 insertions(+), 5 deletions(-) create mode 100644 drivers/amba/acpi.c create mode 100644 include/linux/amba/acpi.h
An AMBA device will be probed once two things have occured: 1) ACPI has registered a device, and 2) the driver has registered itself as an AMBA driver. Since several drivers register themselves very early on, the probe will happen as soon as ACPI registers the device. If a device depends on a clock, the clock must be probed before ACPI registers the device. This means that the clock definition must be encountered first in the DSDT file, and the clock driver must already be loaded when this happens.
There are other potential solutions, with this solution registering the clock driver very early in the boot process to avoid changes to other drivers.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com --- drivers/clk/clk-fixed-rate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c index 226cefb..778291e 100644 --- a/drivers/clk/clk-fixed-rate.c +++ b/drivers/clk/clk-fixed-rate.c @@ -174,5 +174,5 @@ static int __init fixed_clk_init(void) * fixed clock will used for AMBA bus, UART and etc, so it should be * initialized early enough. */ -subsys_initcall(fixed_clk_init); +postcore_initcall(fixed_clk_init); #endif
This AMBA bus ACPI module provides a generic handler for compatible devices. It depends on a DSDT definition as provided in the related '0/4' email.
It uses the same common code as the device tree method to probe for a hardware ID and match up a driver for each device.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com --- drivers/acpi/acpi_platform.c | 2 + drivers/amba/Makefile | 2 +- drivers/amba/acpi.c | 338 ++++++++++++++++++++++++++++++++++++++++++ include/linux/amba/acpi.h | 29 ++++ 4 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 drivers/amba/acpi.c create mode 100644 include/linux/amba/acpi.h
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c index 37b8af8..fdfa990 100644 --- a/drivers/acpi/acpi_platform.c +++ b/drivers/acpi/acpi_platform.c @@ -36,6 +36,8 @@ static const struct acpi_device_id acpi_platform_device_ids[] = { { "LINA0007" }, /* armv8 pmu */ { "LINA0008" }, /* Fixed clock */
+ { "AMBA0000" }, + { } };
diff --git a/drivers/amba/Makefile b/drivers/amba/Makefile index 66e81c2..6d088e7 100644 --- a/drivers/amba/Makefile +++ b/drivers/amba/Makefile @@ -1,2 +1,2 @@ -obj-$(CONFIG_ARM_AMBA) += bus.o +obj-$(CONFIG_ARM_AMBA) += bus.o acpi.o obj-$(CONFIG_TEGRA_AHB) += tegra-ahb.o diff --git a/drivers/amba/acpi.c b/drivers/amba/acpi.c new file mode 100644 index 0000000..7b33535 --- /dev/null +++ b/drivers/amba/acpi.c @@ -0,0 +1,338 @@ +/* + * AMBA Connector Resource for ACPI + * + * Copyright (C) 2013 Advanced Micro Devices, Inc. + * + * Author: Brandon Anderson brandon.anderson@amd.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifdef CONFIG_ACPI + +#include <linux/module.h> +#include <linux/amba/bus.h> +#include <linux/platform_device.h> +#include <linux/acpi.h> +#include <linux/clkdev.h> +#include <linux/amba/acpi.h> + +struct acpi_amba_bus_info { + struct platform_device *pdev; + struct clk *clk; + char *clk_name; +}; + +/* UUID: a706b112-bf0b-48d2-9fa3-95591a3c4c06 (randomly generated) */ +static const char acpi_amba_dsm_uuid[] = { + 0xa7, 0x06, 0xb1, 0x12, 0xbf, 0x0b, 0x48, 0xd2, + 0x9f, 0xa3, 0x95, 0x59, 0x1a, 0x3c, 0x4c, 0x06 +}; + +/* acpi_amba_dsm_lookup() + * + * Helper to parse through ACPI _DSM object for a device. Each entry + * has three fields. + */ +int acpi_amba_dsm_lookup(acpi_handle handle, + const char *tag, int index, + struct acpi_amba_dsm_entry *entry) +{ + acpi_status status; + struct acpi_object_list input; + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object params[4]; + union acpi_object *obj; + int len, match_count, i; + + /* invalidate output in case there's no entry to supply */ + entry->key = NULL; + entry->value = NULL; + + input.count = 4; + params[0].type = ACPI_TYPE_BUFFER; /* UUID */ + params[0].buffer.length = sizeof(acpi_amba_dsm_uuid); + params[0].buffer.pointer = (char *)acpi_amba_dsm_uuid; + params[1].type = ACPI_TYPE_INTEGER; /* Revision */ + params[1].integer.value = 1; + params[2].type = ACPI_TYPE_INTEGER; /* Function # */ + params[2].integer.value = 1; + params[3].type = ACPI_TYPE_PACKAGE; /* Arguments */ + params[3].package.count = 0; + params[3].package.elements = NULL; + input.pointer = params; + + status = acpi_evaluate_object_typed(handle, "_DSM", + &input, &output, ACPI_TYPE_PACKAGE); + if (ACPI_FAILURE(status)) { + if (status != AE_NOT_FOUND) + pr_err("failed to get _DSM package for this device\n"); + return -ENOENT; + } + + obj = (union acpi_object *)output.pointer; + + /* parse 2 objects per entry */ + match_count = 0; + for (i = 0; (i + 2) <= obj->package.count; i += 2) { + /* key must be a string */ + len = obj->package.elements[i].string.length; + if (len <= 0) + continue; + + /* check to see if this is the entry to return */ + if (strncmp(tag, obj->package.elements[i].string.pointer, + len) != 0 || + match_count < index) { + match_count++; + continue; + } + + /* copy the key */ + entry->key = kmalloc(len + 1, GFP_KERNEL); + strncpy(entry->key, + obj->package.elements[i].string.pointer, + len); + entry->key[len] = '\0'; + + /* value is a string with space-delimited fields if necessary */ + len = obj->package.elements[i + 1].string.length; + if (len > 0) { + entry->value = kmalloc(len + 1, GFP_KERNEL); + strncpy(entry->value, + obj->package.elements[i+1].string.pointer, + len); + entry->value[len] = '\0'; + } + + break; + } + + kfree(output.pointer); + + if (entry->key == NULL) + return -ENOENT; + + return 0; +} +EXPORT_SYMBOL_GPL(acpi_amba_dsm_lookup); + +static int acpi_amba_add_resource(struct acpi_resource *ares, void *data) +{ + struct amba_device *dev = data; + struct resource r; + int irq_idx; + + switch (ares->type) { + case ACPI_RESOURCE_TYPE_FIXED_MEMORY32: + if (!acpi_dev_resource_memory(ares, &dev->res)) + pr_err("failed to map memory resource\n"); + break; + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: + for (irq_idx = 0; irq_idx < AMBA_NR_IRQS; irq_idx++) { + if (acpi_dev_resource_interrupt(ares, irq_idx, &r)) + dev->irq[irq_idx] = r.start; + else + break; + } + + break; + case ACPI_RESOURCE_TYPE_END_TAG: + /* ignore the end tag */ + break; + default: + /* log an error, but proceed with driver probe */ + pr_err("unhandled acpi resource type= %d\n", + ares->type); + break; + } + + return 1; /* Tell ACPI core that this resource has been handled */ +} + +static struct clk *acpi_amba_get_clk(acpi_handle handle, int index, + char **clk_name) +{ + struct acpi_amba_dsm_entry entry; + acpi_handle clk_handle; + struct acpi_device *adev, *clk_adev; + char *clk_path; + struct clk *clk = NULL; + int len; + + if (acpi_bus_get_device(handle, &adev)) { + pr_err("cannot get device from handle\n"); + return NULL; + } + + /* key=value format for clocks is: + * "clock-name"="apb_pclk \_SB.CLK0" + */ + if (acpi_amba_dsm_lookup(handle, "clock-name", index, &entry) != 0) + return NULL; + + /* look under the clock device for the clock that matches the entry */ + *clk_name = NULL; + len = strcspn(entry.value, " "); + if (len > 0 && (len + 1) < strlen(entry.value)) { + clk_path = entry.value + len + 1; + *clk_name = kmalloc(len + 1, GFP_KERNEL); + strncpy(*clk_name, entry.value, len); + (*clk_name)[len] = '\0'; + if (ACPI_FAILURE( + acpi_get_handle(NULL, clk_path, &clk_handle)) == 0 && + acpi_bus_get_device(clk_handle, &clk_adev) == 0) + clk = clk_get_sys(dev_name(&clk_adev->dev), *clk_name); + } else + pr_err("Invalid clock-name value format '%s' for %s\n", + entry.value, dev_name(&adev->dev)); + + kfree(entry.key); + kfree(entry.value); + + return clk; +} + +static void acpi_amba_register_clocks(struct acpi_device *adev, + struct clk *default_clk, const char *default_clk_name) +{ + struct clk *clk; + int i; + char *clk_name; + + if (default_clk) { + /* for amba_get_enable_pclk() ... */ + clk_register_clkdev(default_clk, default_clk_name, + dev_name(&adev->dev)); + /* for devm_clk_get() ... */ + clk_register_clkdev(default_clk, NULL, dev_name(&adev->dev)); + } + + for (i = 0;; i++) { + clk_name = NULL; + clk = acpi_amba_get_clk(ACPI_HANDLE(&adev->dev), i, &clk_name); + if (!clk) + break; + + clk_register_clkdev(clk, clk_name, dev_name(&adev->dev)); + + kfree(clk_name); + } +} + +/* acpi_amba_add_device() + * + * ACPI equivalent to of_amba_device_create() + */ +static acpi_status +acpi_amba_add_device(acpi_handle handle, + u32 lvl_not_used, void *data, void **not_used) +{ + struct list_head resource_list; + struct acpi_device *adev; + struct amba_device *dev; + int ret; + struct acpi_amba_bus_info *bus_info = data; + struct platform_device *bus_pdev = bus_info->pdev; + + if (acpi_bus_get_device(handle, &adev)) { + pr_err("%s: acpi_bus_get_device failed\n", __func__); + return AE_OK; + } + + pr_debug("Creating amba device %s\n", dev_name(&adev->dev)); + + dev = amba_device_alloc(NULL, 0, 0); + if (!dev) { + pr_err("%s(): amba_device_alloc() failed for %s\n", + __func__, dev_name(&adev->dev)); + return AE_CTRL_TERMINATE; + } + + /* setup generic device info */ + dev->dev.coherent_dma_mask = ~0; + dev->dev.parent = &bus_pdev->dev; + dev_set_name(&dev->dev, "%s", dev_name(&adev->dev)); + + /* setup amba-specific device info */ + dev->dma_mask = ~0; + + ACPI_HANDLE_SET(&dev->dev, handle); + ACPI_HANDLE_SET(&adev->dev, handle); + + INIT_LIST_HEAD(&resource_list); + acpi_dev_get_resources(adev, &resource_list, + acpi_amba_add_resource, dev); + acpi_dev_free_resource_list(&resource_list); + + /* Add clocks */ + acpi_amba_register_clocks(adev, bus_info->clk, bus_info->clk_name); + + /* Read AMBA hardware ID and add device to system. If a driver matching + * hardware ID has already been registered, bind this device to it. + * Otherwise, the platform subsystem will match up the hardware ID + * when the matching driver is registered. + */ + ret = amba_device_add(dev, &iomem_resource); + if (ret) { + pr_err("%s(): amba_device_add() failed (%d) for %s\n", + __func__, ret, dev_name(&adev->dev)); + goto err_free; + } + + return AE_OK; + +err_free: + amba_device_put(dev); + + return AE_OK; /* don't prevent other devices from being probed */ +} + +static int acpi_amba_bus_probe(struct platform_device *pdev) +{ + struct acpi_amba_bus_info bus_info; + bus_info.clk_name = NULL; + + /* see if there's a top-level clock to use as default for sub-devices */ + bus_info.clk = acpi_amba_get_clk(ACPI_HANDLE(&pdev->dev), 0, + &bus_info.clk_name); + + /* probe each device */ + bus_info.pdev = pdev; + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_HANDLE(&pdev->dev), 1, + acpi_amba_add_device, NULL, &bus_info, NULL); + + kfree(bus_info.clk_name); + + return 0; +} + +static const struct acpi_device_id amba_bus_acpi_match[] = { + { "AMBA0000", 0 }, + { }, +}; + +static struct platform_driver amba_bus_acpi_driver = { + .driver = { + .name = "amba-acpi", + .owner = THIS_MODULE, + .acpi_match_table = ACPI_PTR(amba_bus_acpi_match), + }, + .probe = acpi_amba_bus_probe, +}; + +static int __init acpi_amba_bus_init(void) +{ + return platform_driver_register(&amba_bus_acpi_driver); +} + +postcore_initcall(acpi_amba_bus_init); + +MODULE_AUTHOR("Brandon Anderson brandon.anderson@amd.com"); +MODULE_DESCRIPTION("ACPI Connector Resource for AMBA bus"); +MODULE_LICENSE("GPLv2"); +MODULE_ALIAS("platform:amba-acpi"); + +#endif /* CONFIG_ACPI */ diff --git a/include/linux/amba/acpi.h b/include/linux/amba/acpi.h new file mode 100644 index 0000000..40a72b2 --- /dev/null +++ b/include/linux/amba/acpi.h @@ -0,0 +1,29 @@ +/* + * AMBA bus ACPI helpers + * + * Copyright (C) 2013 Advanced Micro Devices, Inc. + * + * Author: Brandon Anderson brandon.anderson@amd.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#ifndef __ARM_AMBA_ACPI_H__ +#define __ARM_AMBA_ACPI_H__ + +#ifdef CONFIG_ACPI +#include <linux/acpi.h> + +struct acpi_amba_dsm_entry { + char *key; + char *value; +}; + +int acpi_amba_dsm_lookup(acpi_handle handle, + const char *tag, int index, + struct acpi_amba_dsm_entry *entry); + +#endif + +#endif
Neither Foundation nor RTSM have a SPI device, but here are the necessary driver changes as an example of how to use acpi_amba_dsm_lookup() to get non-standard parameters from ACPI.
This was tested by wiring up a SPI device into an RTSM Fast Model.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com --- drivers/spi/spi-pl022.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 9c511a9..1d0a8ec 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -43,6 +43,7 @@ #include <linux/gpio.h> #include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> +#include <linux/amba/acpi.h>
/* * This macro is used to define some register default values. @@ -2069,6 +2070,55 @@ pl022_platform_data_dt_get(struct device *dev) return pd; }
+#ifdef CONFIG_ACPI +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) +{ + struct pl022_ssp_controller *pd, *ret; + struct acpi_amba_dsm_entry entry; + + pd = devm_kzalloc(dev, sizeof(struct pl022_ssp_controller), GFP_KERNEL); + if (!pd) { + dev_err(dev, "cannot allocate platform data memory\n"); + return NULL; + } + ret = pd; + + pd->bus_id = -1; + pd->enable_dma = 1; + if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "num-cs", 0, &entry) == 0) { + if (kstrtou8(entry.value, 0, &pd->num_chipselect) != 0) { + dev_err(dev, "invalid 'num-cs' in ACPI definition\n"); + ret = NULL; + } + kfree(entry.key); + kfree(entry.value); + } + if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), + "autosuspend-delay", 0, &entry) == 0) { + if (kstrtoint(entry.value, 0, &pd->autosuspend_delay) != 0) { + dev_err(dev, "invalid 'autosuspend-delay' in ACPI definition\n"); + ret = NULL; + } + kfree(entry.key); + kfree(entry.value); + } + if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "rt", 0, &entry) == 0) { + pd->rt = (entry.value && strcmp(entry.value, "1") == 0); + kfree(entry.key); + kfree(entry.value); + } + + return ret; +} +#else +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) +{ + return NULL; +} +#endif + static int pl022_probe(struct amba_device *adev, const struct amba_id *id) { struct device *dev = &adev->dev; @@ -2081,6 +2131,9 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
dev_info(&adev->dev, "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid); + if (!platform_info && ACPI_HANDLE(dev)) + platform_info = acpi_pl022_get_platform_data(dev); + else if (!platform_info && IS_ENABLED(CONFIG_OF)) platform_info = pl022_platform_data_dt_get(dev);
Use ACPI definition instead of DTS definition for devices that are supported by AMBA bus ACPI module. The replacement ACPI definition was provided in the related '0/4' email.
Note that MMCI is compatible with AMBA bus ACPI module, but is not ported due to complex relations with sysreg and voltage regulator.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com --- arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +++++++--- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 ++++ arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 ++++++++ 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/foundation-v8-acpi.dts b/arch/arm64/boot/dts/foundation-v8-acpi.dts index 7f57c53..f0671eb 100644 --- a/arch/arm64/boot/dts/foundation-v8-acpi.dts +++ b/arch/arm64/boot/dts/foundation-v8-acpi.dts @@ -15,12 +15,16 @@
chosen { };
+ /* + * Removed for ACPI + * aliases { serial0 = &v2m_serial0; serial1 = &v2m_serial1; serial2 = &v2m_serial2; serial3 = &v2m_serial3; }; + */
cpus { #address-cells = <2>; @@ -196,6 +200,9 @@ reg = <0x010000 0x1000>; };
+ /* + * Removed for ACPI + * v2m_serial0: uart@090000 { compatible = "arm,pl011", "arm,primecell"; reg = <0x090000 0x1000>; @@ -227,9 +234,6 @@ clocks = <&v2m_clk24mhz>, <&v2m_clk24mhz>; clock-names = "uartclk", "apb_pclk"; }; - /* - * Removed for ACPI - * virtio_block@0130000 { compatible = "virtio,mmio"; reg = <0x130000 0x1000>; diff --git a/arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts b/arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts index 7f348eb..0953fd0 100644 --- a/arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts +++ b/arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts @@ -20,12 +20,16 @@
chosen { };
+ /* + * Removed for ACPI + * aliases { serial0 = &v2m_serial0; serial1 = &v2m_serial1; serial2 = &v2m_serial2; serial3 = &v2m_serial3; }; + */
cpus { #address-cells = <2>; diff --git a/arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi b/arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi index bbaaa8e..f20af14 100644 --- a/arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi +++ b/arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi @@ -71,6 +71,9 @@ #gpio-cells = <2>; };
+ /* + * Removed for ACPI + * v2m_sysctl: sysctl@020000 { compatible = "arm,sp810", "arm,primecell"; reg = <0x020000 0x1000>; @@ -87,6 +90,7 @@ clocks = <&v2m_clk24mhz>; clock-names = "apb_pclk"; }; + */
mmci@050000 { compatible = "arm,pl180", "arm,primecell"; @@ -100,6 +104,9 @@ clock-names = "mclk", "apb_pclk"; };
+ /* + * Removed for ACPI + * kmi@060000 { compatible = "arm,pl050", "arm,primecell"; reg = <0x060000 0x1000>; @@ -179,6 +186,7 @@ clocks = <&v2m_clk24mhz>; clock-names = "apb_pclk"; }; + */
clcd@1f0000 { compatible = "arm,pl111", "arm,primecell";
On Fri, Nov 22, 2013 at 10:12 AM, Brandon Anderson brandon.anderson@amd.com wrote:
An AMBA device will be probed once two things have occured: 1) ACPI has registered a device, and 2) the driver has registered itself as an AMBA driver. Since several drivers register themselves very early on, the probe will happen as soon as ACPI registers the device. If a device depends on a clock, the clock must be probed before ACPI registers the device. This means that the clock definition must be encountered first in the DSDT file, and the clock driver must already be loaded when this happens.
There are other potential solutions, with this solution registering the clock driver very early in the boot process to avoid changes to other drivers.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
You guys really need to start posting these patches to a wider audience than just linux-acpi. At least cc the clock maintainer on clock patches, and since these are ARM related, the linux-arm-kernel list should be cc:d too.
-Olof
Hi,
On Fri, Nov 22, 2013 at 10:12 AM, Brandon Anderson brandon.anderson@amd.com wrote:
This patch series requires the fixed-clock patches from Hanjun Guo.
I don't see them posted anywhere? Please don't post patches that don't apply to mainline (or to publicly posted dependencies).
arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 + arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 +
These files don't exist in mainline, nor in linux-next.
-Olof
Olof, I apologize for posting confusing patches to the linux-acpi list.
These patches are on top of the Linaro ACPI work, http://git.linaro.org/git-ro/arm/acpi/leg-kernel.git
Brandon
-----Original Message----- From: Olof Johansson [mailto:olof@lixom.net] Sent: Friday, November 22, 2013 1:04 PM To: Anderson, Brandon Cc: Russell King - ARM Linux; Rafael J. Wysocki; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org Subject: Re: [PATCH V2 0/4] ACPI/ARM: AMBA bus ACPI module
Hi,
On Fri, Nov 22, 2013 at 10:12 AM, Brandon Anderson brandon.anderson@amd.com wrote:
This patch series requires the fixed-clock patches from Hanjun Guo.
I don't see them posted anywhere? Please don't post patches that don't apply to mainline (or to publicly posted dependencies).
arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 + arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 +
These files don't exist in mainline, nor in linux-next.
-Olof
On Fri, Nov 22, 2013 at 6:12 PM, Brandon Anderson brandon.anderson@amd.com wrote:
Use ACPI definition instead of DTS definition for devices that are supported by AMBA bus ACPI module. The replacement ACPI definition was provided in the related '0/4' email.
Note that MMCI is compatible with AMBA bus ACPI module, but is not ported due to complex relations with sysreg and voltage regulator.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +++++++--- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 ++++ arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 ++++++++ 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/foundation-v8-acpi.dts b/arch/arm64/boot/dts/foundation-v8-acpi.dts index 7f57c53..f0671eb 100644 --- a/arch/arm64/boot/dts/foundation-v8-acpi.dts +++ b/arch/arm64/boot/dts/foundation-v8-acpi.dts @@ -15,12 +15,16 @@
chosen { };
/** Removed for ACPI* aliases { serial0 = &v2m_serial0; serial1 = &v2m_serial1; serial2 = &v2m_serial2; serial3 = &v2m_serial3; };*/
There's no point in commenting the sections out. If you're removing them, remove the outright.
g.
On Fri, Nov 22, 2013 at 6:12 PM, Brandon Anderson brandon.anderson@amd.com wrote:
Neither Foundation nor RTSM have a SPI device, but here are the necessary driver changes as an example of how to use acpi_amba_dsm_lookup() to get non-standard parameters from ACPI.
This was tested by wiring up a SPI device into an RTSM Fast Model.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
drivers/spi/spi-pl022.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 9c511a9..1d0a8ec 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -43,6 +43,7 @@ #include <linux/gpio.h> #include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> +#include <linux/amba/acpi.h>
/*
- This macro is used to define some register default values.
@@ -2069,6 +2070,55 @@ pl022_platform_data_dt_get(struct device *dev) return pd; }
+#ifdef CONFIG_ACPI +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) +{
struct pl022_ssp_controller *pd, *ret;struct acpi_amba_dsm_entry entry;pd = devm_kzalloc(dev, sizeof(struct pl022_ssp_controller), GFP_KERNEL);if (!pd) {dev_err(dev, "cannot allocate platform data memory\n");return NULL;}ret = pd;pd->bus_id = -1;pd->enable_dma = 1;if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "num-cs", 0, &entry) == 0) {if (kstrtou8(entry.value, 0, &pd->num_chipselect) != 0) {dev_err(dev, "invalid 'num-cs' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev),"autosuspend-delay", 0, &entry) == 0) {if (kstrtoint(entry.value, 0, &pd->autosuspend_delay) != 0) {dev_err(dev, "invalid 'autosuspend-delay' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "rt", 0, &entry) == 0) {pd->rt = (entry.value && strcmp(entry.value, "1") == 0);kfree(entry.key);kfree(entry.value);}return ret;+}
As discussed in the ACPI vs DT thread. The kinds of lookups here are identical to the DT property lookups except that the function name is different and the property name is needlessly different. That's madness. In all of the trivial cases the DT and ACPI lookup code should be identical. We need a property value lookup function that both DT and ACPI can use.
g.
-----Original Message----- From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely Sent: Friday, November 22, 2013 1:48 PM To: Anderson, Brandon Cc: Russell King - ARM Linux; Rafael J. Wysocki; linaro-acpi; ACPI Devel Mailing List Subject: Re: [Linaro-acpi] [PATCH V2 4/4] ACPI/ARM: Remove sections of DTS definition
On Fri, Nov 22, 2013 at 6:12 PM, Brandon Anderson brandon.anderson@amd.com wrote:
Use ACPI definition instead of DTS definition for devices that are supported by AMBA bus ACPI module. The replacement ACPI definition was provided in the related '0/4' email.
Note that MMCI is compatible with AMBA bus ACPI module, but is not ported due to complex relations with sysreg and voltage regulator.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
arch/arm64/boot/dts/foundation-v8-acpi.dts | 10 +++++++--- arch/arm64/boot/dts/rtsm_ve-aemv8a-acpi.dts | 4 ++++ arch/arm64/boot/dts/rtsm_ve-motherboard-acpi.dtsi | 8 ++++++++ 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/foundation-v8-acpi.dts b/arch/arm64/boot/dts/foundation-v8-acpi.dts index 7f57c53..f0671eb 100644 --- a/arch/arm64/boot/dts/foundation-v8-acpi.dts +++ b/arch/arm64/boot/dts/foundation-v8-acpi.dts @@ -15,12 +15,16 @@
chosen { };
/** Removed for ACPI* aliases { serial0 = &v2m_serial0; serial1 = &v2m_serial1; serial2 = &v2m_serial2; serial3 = &v2m_serial3; };*/
There's no point in commenting the sections out. If you're removing them, remove the outright.
g.
Acknowledged.
On 11/22/2013 12:52 PM, Grant Likely wrote:
On Fri, Nov 22, 2013 at 6:12 PM, Brandon Anderson brandon.anderson@amd.com wrote:
Neither Foundation nor RTSM have a SPI device, but here are the necessary driver changes as an example of how to use acpi_amba_dsm_lookup() to get non-standard parameters from ACPI.
This was tested by wiring up a SPI device into an RTSM Fast Model.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
drivers/spi/spi-pl022.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 9c511a9..1d0a8ec 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -43,6 +43,7 @@ #include <linux/gpio.h> #include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> +#include <linux/amba/acpi.h>
/*
- This macro is used to define some register default values.
@@ -2069,6 +2070,55 @@ pl022_platform_data_dt_get(struct device *dev) return pd; }
+#ifdef CONFIG_ACPI +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) +{
struct pl022_ssp_controller *pd, *ret;struct acpi_amba_dsm_entry entry;pd = devm_kzalloc(dev, sizeof(struct pl022_ssp_controller), GFP_KERNEL);if (!pd) {dev_err(dev, "cannot allocate platform data memory\n");return NULL;}ret = pd;pd->bus_id = -1;pd->enable_dma = 1;if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "num-cs", 0, &entry) == 0) {if (kstrtou8(entry.value, 0, &pd->num_chipselect) != 0) {dev_err(dev, "invalid 'num-cs' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev),"autosuspend-delay", 0, &entry) == 0) {if (kstrtoint(entry.value, 0, &pd->autosuspend_delay) != 0) {dev_err(dev, "invalid 'autosuspend-delay' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "rt", 0, &entry) == 0) {pd->rt = (entry.value && strcmp(entry.value, "1") == 0);kfree(entry.key);kfree(entry.value);}return ret;+}
As discussed in the ACPI vs DT thread. The kinds of lookups here are identical to the DT property lookups except that the function name is different and the property name is needlessly different. That's madness. In all of the trivial cases the DT and ACPI lookup code should be identical. We need a property value lookup function that both DT and ACPI can use.
g.
Agreed. I'm in the process of prototyping this code and my plan is to have something available for comment the week after next (unless I get _really_ bored Thanksgiving weekend...).
-----Original Message----- From: Al Stone [mailto:al.stone@linaro.org] Sent: Friday, November 22, 2013 2:13 PM To: Grant Likely; Anderson, Brandon Cc: Rafael J. Wysocki; Russell King - ARM Linux; ACPI Devel Mailing List; linaro-acpi Subject: Re: [Linaro-acpi] [PATCH V2 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver
On 11/22/2013 12:52 PM, Grant Likely wrote:
On Fri, Nov 22, 2013 at 6:12 PM, Brandon Anderson brandon.anderson@amd.com wrote:
Neither Foundation nor RTSM have a SPI device, but here are the necessary driver changes as an example of how to use acpi_amba_dsm_lookup() to get non-standard parameters from ACPI.
This was tested by wiring up a SPI device into an RTSM Fast Model.
Signed-off-by: Brandon Anderson brandon.anderson@amd.com
drivers/spi/spi-pl022.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 9c511a9..1d0a8ec 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -43,6 +43,7 @@ #include <linux/gpio.h> #include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> +#include <linux/amba/acpi.h>
/*
- This macro is used to define some register default values.
@@ -2069,6 +2070,55 @@ pl022_platform_data_dt_get(struct device *dev) return pd; }
+#ifdef CONFIG_ACPI +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) {
struct pl022_ssp_controller *pd, *ret;struct acpi_amba_dsm_entry entry;pd = devm_kzalloc(dev, sizeof(struct pl022_ssp_controller), GFP_KERNEL);if (!pd) {dev_err(dev, "cannot allocate platform data memory\n");return NULL;}ret = pd;pd->bus_id = -1;pd->enable_dma = 1;if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "num-cs", 0, &entry) == 0) {if (kstrtou8(entry.value, 0, &pd->num_chipselect) != 0) {dev_err(dev, "invalid 'num-cs' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev),"autosuspend-delay", 0, &entry) == 0) {if (kstrtoint(entry.value, 0, &pd->autosuspend_delay) != 0) {dev_err(dev, "invalid 'autosuspend-delay' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);}if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "rt", 0, &entry) == 0) {pd->rt = (entry.value && strcmp(entry.value, "1") == 0);kfree(entry.key);kfree(entry.value);}return ret;+}
As discussed in the ACPI vs DT thread. The kinds of lookups here are identical to the DT property lookups except that the function name is different and the property name is needlessly different. That's madness. In all of the trivial cases the DT and ACPI lookup code should be identical. We need a property value lookup function that both DT and ACPI can use.
g.
Agreed. I'm in the process of prototyping this code and my plan is to have something available for comment the week after next (unless I get _really_ bored Thanksgiving weekend...).
Great, Al!
I agree, and Russell King has made the same comment. This patch in the series can be dropped with no ill effect, as it is an example only. Or modified to use a common lookup function once it is available.
-- ciao, al ----------------------------------- Al Stone Software Engineer Linaro Enterprise Group al.stone@linaro.org -----------------------------------
On Fri, Nov 22, 2013 at 12:12:42PM -0600, Brandon Anderson wrote:
- status = acpi_evaluate_object_typed(handle, "_DSM",
&input, &output, ACPI_TYPE_PACKAGE);
We've got enough uses of _DSM that it'd be nice to have a generic helper function for setting them up. Not that this is a blocker.
- /* parse 2 objects per entry */
But if we're expecting anyone to use _DSM for DT properties, this really should be a generic function. Don't make it AMBA specific.
- /* key=value format for clocks is:
* "clock-name"="apb_pclk \\_SB.CLK0"*/- if (acpi_amba_dsm_lookup(handle, "clock-name", index, &entry) != 0)
return NULL;
Why use DT properties for this? If you're just pointing to something else in the ACPI namespace then add a method that returns the object.
On Fri, Nov 22, 2013 at 12:12:43PM -0600, Brandon Anderson wrote:
+#ifdef CONFIG_ACPI +static struct pl022_ssp_controller * +acpi_pl022_get_platform_data(struct device *dev) +{
- struct pl022_ssp_controller *pd, *ret;
- struct acpi_amba_dsm_entry entry;
- pd = devm_kzalloc(dev, sizeof(struct pl022_ssp_controller), GFP_KERNEL);
- if (!pd) {
dev_err(dev, "cannot allocate platform data memory\n");return NULL;- }
- ret = pd;
- pd->bus_id = -1;
- pd->enable_dma = 1;
- if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "num-cs", 0, &entry) == 0) {
if (kstrtou8(entry.value, 0, &pd->num_chipselect) != 0) {dev_err(dev, "invalid 'num-cs' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);- }
- if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev),
"autosuspend-delay", 0, &entry) == 0) {if (kstrtoint(entry.value, 0, &pd->autosuspend_delay) != 0) {dev_err(dev, "invalid 'autosuspend-delay' in ACPI definition\n");ret = NULL;}kfree(entry.key);kfree(entry.value);- }
- if (acpi_amba_dsm_lookup(ACPI_HANDLE(dev), "rt", 0, &entry) == 0) {
pd->rt = (entry.value && strcmp(entry.value, "1") == 0);kfree(entry.key);kfree(entry.value);- }
- return ret;
+}
I thought I'd already commented about this, and said if the properties are the same as the DT stuff, this should be done in a common way rather than modifing every driver.
For stuff like the above, it's really quite simple.
Rename the of_property_read_*() function to firmware_read_*() function and make it take a struct device.
Then you can hide the of_property_read_*() if there's an OF node present inside the firmware_read_*() function. If not, and it has an ACPI handle, then you can read it from ACPI.
That means the only modification that drivers see is a function rename and a _slight_ change of argument, which is a lot saner for driver authors to review rather than having to review an entire set of new parsing code.
On Friday, November 22, 2013 07:44:53 PM Anderson, Brandon wrote:
Olof, I apologize for posting confusing patches to the linux-acpi list.
These patches are on top of the Linaro ACPI work, http://git.linaro.org/git-ro/arm/acpi/leg-kernel.git
So here's a rule. I'm not going to look into any Linaro repositories, so please don't expect me to do that. If you want me to look at any code outside of the mainline kernel, please post it here.
Thanks, Rafael
On Fri, Nov 22, 2013 at 11:44 AM, Anderson, Brandon Brandon.Anderson@amd.com wrote:
Olof, I apologize for posting confusing patches to the linux-acpi list.
These patches are on top of the Linaro ACPI work, http://git.linaro.org/git-ro/arm/acpi/leg-kernel.git
Hi Brandon,
First, please don't top-post replies to emails on kernel mailing lists, we usually do in-line replies.
Posting patches that only apply to your forked/private kernel to a community-wide list is not appropriate. Upstream mailing lists should only see code targeted for upstream.
-Olof
-
Al Stone -
Anderson, Brandon -
Brandon Anderson
-
Grant Likely -
Matthew Garrett -
Olof Johansson -
Rafael J. Wysocki -
Russell King - ARM Linux