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@xxxxxxxxxxxxxxx] > Sent: Tuesday, November 12, 2013 4:41 AM > To: Anderson, Brandon > Cc: ssg.sos.patches; linaro-acpi@xxxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx > 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@xxxxxxx> > > --- > > 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@xxxxxxx> > > + * > > + * 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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
