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

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

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