[PATCH 0/4] ACPI/ARM: AMBA bus ACPI module

[PATCH 0/4] ACPI/ARM: AMBA bus ACPI module

[Brandon Anderson]

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.

See required DSDT definition format below.


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                               |  339 +++++++++++++++++++++
 drivers/clk/clk-fixed-rate.c                      |    2 +-
 drivers/spi/spi-pl022.c                           |   49 +++
 include/linux/amba/acpi.h                         |   29 ++
 9 files changed, 440 insertions(+), 5 deletions(-)
 create mode 100644 drivers/amba/acpi.c
 create mode 100644 include/linux/amba/acpi.h

-- 
1.7.9.5

-----------

diff --git a/platforms/rtsm_ve-aemv8a.acpi/dsdt.asl b/platforms/rtsm_ve-aemv8a.acpi/dsdt.asl
index 0bcc94d..64e84a4 100644
--- a/platforms/rtsm_ve-aemv8a.acpi/dsdt.asl
+++ b/platforms/rtsm_ve-aemv8a.acpi/dsdt.asl
@@ -252,6 +252,33 @@ DefinitionBlock (
 			}
 		}
 
+		Device (CLK0) {
+			Name (_HID, "LINA0008")
+			Name (_UID, 0)
+
+			Method (FREQ, 0x0, NotSerialized) {
+				Return (24000000)
+			}
+		}
+
+		Device (CLK1) {
+			Name (_HID, "LINA0008")
+			Name (_UID, 1)
+
+			Method (FREQ, 0x0, NotSerialized) {
+				Return (1000000)
+			}
+		}
+
+		Device (CLK2) {
+			Name (_HID, "LINA0008")
+			Name (_UID, 2)
+
+			Method (FREQ, 0x0, NotSerialized) {
+				Return (32768)
+			}
+		}
+
 		Device (PMU0) {
 			Name (_HID, "LINA0007")
 			Name (_UID, 0)
@@ -263,5 +290,229 @@ DefinitionBlock (
 				Return (RBUF)
 			}
 		}
+
+		Method (DTGP, 5, NotSerialized)
+		{
+			If (LEqual (Arg0, Buffer (0x10)
+					{
+					/* UUID:	a706b112-bf0b-48d2-9fa3-95591a3c4c06 */
+					  /* 0000 */	0xa7, 0x06, 0xb1, 0x12, 0xbf, 0x0b, 0x48, 0xd2,
+					  /* 0008 */	0x9f, 0xa3, 0x95, 0x59, 0x1a, 0x3c, 0x4c, 0x06
+					}))
+			{
+				If (LEqual (Arg1, 0x01))
+				{
+					If (LEqual (Arg2, 0x00))
+					{
+						Store (Buffer (0x01)
+						{
+							0x03
+						}, Arg4)
+						Return (0x01)
+					}
+
+					If (LEqual (Arg2, 0x01))
+					{
+						Return (0x01)
+					}
+				}
+			}
+
+			Store (Buffer (0x01)
+			{
+				0x00
+			}, Arg4)
+			Return (0x00)
+		}
+
+		Device (AMBA) {
+			Name (_HID, "AMBA0000") /* the parallel to "arm,primecell" in DTS */
+			Name (_UID, 0)
+
+			/* Define 'apb_pclk' as a default clock source since it is
+			   common with devices below */
+			Method(_DSM, 4, NotSerialized) {
+				Store (Package (2)
+				{
+					"clock-name", "apb_pclk \\_SB.CLK0",
+				}, Local0)
+
+				DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
+
+				Return (Local0)
+			}
+
+			Device (SCT0) {
+				Name (_ADR, 0x1c020000)						/* SYSCTL */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c020000, 0x00001000)
+					})
+					Return (RBUF)
+				}
+				Method(_DSM, 4, Serialized) {
+					Store (Package (4)
+					{
+						"clock-name", "refclk \\_SB.CLK2",
+						"clock-name", "timclk \\_SB.CLK1",
+					}, Local0)
+
+					DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
+
+					Return (Local0)
+				}
+			}
+
+			Device (KMI0) {
+				Name (_ADR, 0x1c060000)
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c060000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {44}
+					})
+					Return (RBUF)
+				}
+				Method(_DSM, 4, Serialized) {
+					Store (Package (2)
+					{
+						"clock-name", "KMIREFCLK \\_SB.CLK0",
+					}, Local0)
+
+					DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
+
+					Return (Local0)
+				}
+			}
+
+			Device (KMI1) {
+				Name (_ADR, 0x1c070000)
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c070000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {45}
+					})
+					Return (RBUF)
+				}
+				Method(_DSM, 4, NotSerialized) {
+					Store (Package (2)
+					{
+						"clock-name", "KMIREFCLK \\_SB.CLK0",
+					}, Local0)
+
+					DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
+
+					Return (Local0)
+				}
+			}
+
+			Device (SER0) {
+				Name (_ADR, 0x1c090000)                         /* UART0 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c090000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {37}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (SER1) {
+				Name (_ADR, 0x1c0a0000)                         /* UART1 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c0a0000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {38}
+					})
+					Return (RBUF)
+				}
+			}
+			Device (SER2) {
+				Name (_ADR, 0x1c0b0000)                         /* UART2 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c0b0000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {39}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (SER3) {
+				Name (_ADR, 0x1c0c0000)                         /* UART3 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c0c0000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {40}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (AAC0) {
+				Name (_ADR, 0x1c040000)						/* AACI */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c040000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {43}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (WDT0) {
+				Name (_ADR, 0x1c0f0000)						/* WDT */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c0f0000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {32}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (TIM0) {
+				Name (_ADR, 0x1c110000)						/* TIMER01 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c110000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {34}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (TIM2) {
+				Name (_ADR, 0x1c120000)						/* TIMER23 */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c120000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {35}
+					})
+					Return (RBUF)
+				}
+			}
+
+			Device (RTC0) {
+				Name (_ADR, 0x1c170000)						/* RTC */
+				Method (_CRS, 0x0, Serialized) {
+					Name (RBUF, ResourceTemplate () {
+						Memory32Fixed (ReadWrite, 0x1c170000, 0x00001000)
+						Interrupt (ResourceConsumer, Edge, ActiveBoth,
+								Exclusive, , , ) {36}
+					})
+					Return (RBUF)
+				}
+			}
+		}
 	}
 }

[PATCH 1/4] ACPI/ARM: Load fixed-clk module early

[Brandon Anderson]

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
-- 
1.7.9.5

[PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module

[Brandon Anderson]

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          |  339 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/acpi.h    |   29 ++++
 4 files changed, 371 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..b8a9f57
--- /dev/null
+++ b/drivers/amba/acpi.c
@@ -0,0 +1,339 @@
+/*
+ * 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;
+
+	if (!acpi_has_method(handle, "_DSM"))
+		return -ENOENT;
+
+	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)) {
+		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
-- 
1.7.9.5

Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module

[Mika Westerberg]

On Mon, Nov 11, 2013 at 11:16:08AM -0600, Brandon Anderson wrote:
> 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          |  339 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/amba/acpi.h    |   29 ++++
>  4 files changed, 371 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..b8a9f57
> --- /dev/null
> +++ b/drivers/amba/acpi.c
> @@ -0,0 +1,339 @@
> +/*
> + * 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;
> +
> +	if (!acpi_has_method(handle, "_DSM"))
> +		return -ENOENT;

I think this is redundant because acpi_evaluate_object_typed() below
already checks if the given method exists.

> +
> +	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)) {
> +		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;
> +		}

Can't you use helpers already provided in drivers/acpi/resource.c?

> +
> +		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)
> +{

I wonder if you can leverate acpi_create_platform_device() here?

> +	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 */
> +}

Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module

[Mika Westerberg]

On Tue, Nov 12, 2013 at 05:17:51PM +0000, Anderson, Brandon wrote:
> Mika, thank you for reviewing! Responses below.
> 
> 
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
> Sent: Tuesday, November 12, 2013 4:41 AM
> To: Anderson, Brandon
> Cc: ssg.sos.patches; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module
> 
> On Mon, Nov 11, 2013 at 11:16:08AM -0600, Brandon Anderson wrote:
> > 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          |  339 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/amba/acpi.h    |   29 ++++
> >  4 files changed, 371 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..b8a9f57
> > --- /dev/null
> > +++ b/drivers/amba/acpi.c
> > @@ -0,0 +1,339 @@
> > +/*
> > + * 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;
> > +
> > +	if (!acpi_has_method(handle, "_DSM"))
> > +		return -ENOENT;
> 
> I think this is redundant because acpi_evaluate_object_typed() below already checks if the given method exists.
> 
> 
> I see your point. However, the _DSM method is optional, so this first check is whether or not it needs to be called,
> and the second is checking to ensure that the call was successful. The first is not an 'error', but the second is
> because it indicates a malformed DSDT. Should I drop the first check, even though that would require also losing the
> error message in the second check? The code should not print an error message when the method is not present.

I think that acpi_evaluate_object_typed() returns AE_NOT_EXISTS if the
method you are trying to evaluate does not exists.

> 
> 
> > +
> > +	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)) {
> > +		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;
> > +		}
> 
> Can't you use helpers already provided in drivers/acpi/resource.c?
> 
> 
> I'm curious if you are seeing something that I'm not, but I don't think so. If this code were filling out a struct platform_device, 
> then it might be able to. But it's filling out struct amba_device, and using different helpers would mean more code to convert
> the output of the helpers to the structure needed.

No, you are right. I misread the code - you are already using the resource
helpers I was suggesting.

> > +
> > +		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) {
> 
> I wonder if you can leverate acpi_create_platform_device() here?
> 
> 
> I don't believe so. This function sets up struct amba_device in order to
> use existing code path in drivers/amba/bus.c

OK.

Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module

[Tomasz Nowicki]

On 11.11.2013 18:16, Brandon Anderson wrote:
> 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          |  339 ++++++++++++++++++++++++++++++++++++++++++
>   include/linux/amba/acpi.h    |   29 ++++
>   4 files changed, 371 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..b8a9f57
> --- /dev/null
> +++ b/drivers/amba/acpi.c
> @@ -0,0 +1,339 @@
> +/*
> + * 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;
> +
> +	if (!acpi_has_method(handle, "_DSM"))
> +		return -ENOENT;
> +
> +	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)) {
> +		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;
> +	}
I am wondering, why you did not use function index arg of _DSM method 
(params[2].integer.value) for different properties like others drivers 
do. IMHO, it would be easier to parse (only one value as return value) 
and more compliant with ACPI spec. Does it sound reasonable for you?
> +
> +	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
>

Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module

[Tomasz Nowicki]

On 21.11.2013 18:30, Anderson, Brandon wrote:
>
>
> -----Original Message-----
> From: Tomasz Nowicki [mailto:tomasz.nowicki@linaro.org]
> Sent: Thursday, November 21, 2013 9:10 AM
> To: Anderson, Brandon; ssg.sos.patches; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 2/4] ACPI/ARM: Add AMBA bus ACPI module
>
> On 11.11.2013 18:16, Brandon Anderson wrote:
>> 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          |  339 ++++++++++++++++++++++++++++++++++++++++++
>>    include/linux/amba/acpi.h    |   29 ++++
>>    4 files changed, 371 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..b8a9f57
>> --- /dev/null
>> +++ b/drivers/amba/acpi.c
>> @@ -0,0 +1,339 @@
>> +/*
>> + * 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;
>> +
>> +	if (!acpi_has_method(handle, "_DSM"))
>> +		return -ENOENT;
>> +
>> +	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)) {
>> +		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;
>> +	}
> I am wondering, why you did not use function index arg of _DSM method
> (params[2].integer.value) for different properties like others drivers do. IMHO, it would be easier to parse (only one value as return value) and more compliant with ACPI spec. Does it sound reasonable for you?
>
> Are you suggesting that we define and use an index for each key instead of an ascii string? Graeme mentioned that the key/value pair paradigm came from the Kernel Plumbers, do we need to change this? Obviously I'd prefer to go with what I've already implemented and tested.
I see, since it is well discussed approach, that is fine with me.
>
> I have a V2 patch with minor changes ready to submit.
>
>> +
>> +	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
>>
>
>

[PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver

[Brandon Anderson]

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 |   49 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 9c511a9..75cff22 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,49 @@ 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;
+}
+#endif
+
 static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct device *dev = &adev->dev;
@@ -2081,6 +2125,11 @@ 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);
+#ifdef CONFIG_ACPI
+	if (!platform_info && ACPI_HANDLE(dev))
+		platform_info = acpi_pl022_get_platform_data(dev);
+	else
+#endif
 	if (!platform_info && IS_ENABLED(CONFIG_OF))
 		platform_info = pl022_platform_data_dt_get(dev);
 
-- 
1.7.9.5

Re: [PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver

[Mika Westerberg]

On Mon, Nov 11, 2013 at 11:16:09AM -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;
> +}
> +#endif

If you add dummy stub acpi_pl022_get_platform_data() here in case of
!CONFIG_ACPI...

> +
>  static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	struct device *dev = &adev->dev;
> @@ -2081,6 +2125,11 @@ 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);
> +#ifdef CONFIG_ACPI

You don't need this ugly #ifdef here.

> +	if (!platform_info && ACPI_HANDLE(dev))
> +		platform_info = acpi_pl022_get_platform_data(dev);
> +	else
> +#endif
>  	if (!platform_info && IS_ENABLED(CONFIG_OF))
>  		platform_info = pl022_platform_data_dt_get(dev);

Re: [PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver

[Mika Westerberg]

On Tue, Nov 12, 2013 at 05:26:39PM +0000, Anderson, Brandon wrote:
> 
> 
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
> Sent: Tuesday, November 12, 2013 4:43 AM
> To: Anderson, Brandon
> Cc: ssg.sos.patches; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver
> 
> On Mon, Nov 11, 2013 at 11:16:09AM -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;
> > +}
> > +#endif
> 
> If you add dummy stub acpi_pl022_get_platform_data() here in case of !CONFIG_ACPI...
> 
> > +
> >  static int pl022_probe(struct amba_device *adev, const struct amba_id 
> > *id)  {
> >  	struct device *dev = &adev->dev;
> > @@ -2081,6 +2125,11 @@ 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);
> > +#ifdef CONFIG_ACPI
> 
> You don't need this ugly #ifdef here.
> 
> 
> Ok, I understand the alternative. Is one way or the other required for
> patch acceptance?

I guess it is up to the maintainer to decide in the end.

> 
> 
> > +	if (!platform_info && ACPI_HANDLE(dev))
> > +		platform_info = acpi_pl022_get_platform_data(dev);
> > +	else
> > +#endif
> >  	if (!platform_info && IS_ENABLED(CONFIG_OF))
> >  		platform_info = pl022_platform_data_dt_get(dev);
> 

Re: [PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver

[Rafael J. Wysocki]

On Tuesday, November 12, 2013 09:14:45 PM Mika Westerberg wrote:
> On Tue, Nov 12, 2013 at 05:26:39PM +0000, Anderson, Brandon wrote:
> > 
> > 
> > -----Original Message-----
> > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
> > Sent: Tuesday, November 12, 2013 4:43 AM
> > To: Anderson, Brandon
> > Cc: ssg.sos.patches; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org
> > Subject: Re: [PATCH 3/4] ACPI/ARM: Add ACPI to AMBA SPI driver
> > 
> > On Mon, Nov 11, 2013 at 11:16:09AM -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;
> > > +}
> > > +#endif
> > 
> > If you add dummy stub acpi_pl022_get_platform_data() here in case of !CONFIG_ACPI...
> > 
> > > +
> > >  static int pl022_probe(struct amba_device *adev, const struct amba_id 
> > > *id)  {
> > >  	struct device *dev = &adev->dev;
> > > @@ -2081,6 +2125,11 @@ 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);
> > > +#ifdef CONFIG_ACPI
> > 
> > You don't need this ugly #ifdef here.
> > 
> > 
> > Ok, I understand the alternative. Is one way or the other required for
> > patch acceptance?
> 
> I guess it is up to the maintainer to decide in the end.

Well, if I were the maintainer in question, I'd ask that do be done the way
Mika suggested.

The same applies if my ACK under the patch is requisite (which I believe should
be the case here).

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 4/4] ACPI/ARM: Remove sections of DTS definition

[Brandon Anderson]

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";
-- 
1.7.9.5