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

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.