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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.