Thank you, Graeme. This is exactly the clarification that I needed.
Are there any other requirements for the GUID implementation? Is it appropriate to calculate a single GUID with uuidgen to use in both the AMBA bus ACPI module and any device drivers that need to access these key/value pairs?
Brandon
From: Graeme Gregory [mailto:graeme.gregory@linaro.org] Sent: Friday, October 25, 2013 5:37 AM To: Anderson, Brandon Cc: Hanjun Guo; linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.org; lenb@kernel.org; rjw@sisk.pl; naresh.bhat@linaro.org; Suthikulpanit, Suravee Subject: Re: [RFC PATCH 2/3] ACPI: Prototype of AMBA bus 'connector resource'
The decision coming out of the Kernel Plumbers is that these should be key=value types.
Certain characters like " " and ";" are not allowed in ACPI namespace so they can be used as seperators.
Method(_DSM, 4, NotSerialized) { Store (Package (6) { "clock-name", "KMIREFCLK \_SB_.CLK0file:///\\_SB_.CLK0", }, Local0) Return (Local0) }
And of course the _DSM has to check for the GUID of the key/value pairs data type before returning the package. It does not do it unconditionally.
Thanks
Graeme
On 23 October 2013 16:32, Anderson, Brandon <Brandon.Anderson@amd.commailto:Brandon.Anderson@amd.com> wrote: Thank you Hanjun and Graeme for reviewing the clock definition.
Some devices have multiple clocks and they need to be labeled, so I propose the following format:
Method(_DSM, 4, NotSerialized) { Store (Package (6) { "clock-name", "KMIREFCLK", "\_SB_.CLK0file:///\\_SB_.CLK0", }, Local0) Return (Local0) }
Other custom parameters are required (for MMCI driver in particular), and they will fit into this format as well:
Method(_DSM, 4, NotSerialized) { Store (Package (9) { "clock-name", "mclk", "\_SB_.CLK0file:///\\_SB_.CLK0", "max-frequency", 12000000, "", "vmmc-supply", "", "", }, Local0)
Return (Local0) }
Three fields must be specified for each entry (each line above is an entry). Is this acceptable?
Brandon
-----Original Message----- From: Graeme Gregory [mailto:graeme.gregory@linaro.orgmailto:graeme.gregory@linaro.org] Sent: Wednesday, October 23, 2013 7:50 AM To: Hanjun Guo Cc: Anderson, Brandon; linaro-acpi@lists.linaro.orgmailto:linaro-acpi@lists.linaro.org; linux-acpi@vger.kernel.orgmailto:linux-acpi@vger.kernel.org; lenb@kernel.orgmailto:lenb@kernel.org; rjw@sisk.plmailto:rjw@sisk.pl; naresh.bhat@linaro.orgmailto:naresh.bhat@linaro.org; Suthikulpanit, Suravee Subject: Re: [RFC PATCH 2/3] ACPI: Prototype of AMBA bus 'connector resource'
On Wed, Oct 23, 2013 at 07:52:30PM +0800, Hanjun Guo wrote:
On 2013-10-22 8:33, brandon.anderson@amd.commailto:brandon.anderson@amd.com wrote:
From: Brandon Anderson <brandon.anderson@amd.commailto:brandon.anderson@amd.com>
Add AMBA bus 'connector resource' module to call amba_device_add() for each device under the top-level 'AMBA0000' entry. Clock handling is yet to be implemented.
Signed-off-by: Brandon Anderson <brandon.anderson@amd.commailto:brandon.anderson@amd.com>
drivers/acpi/acpi_platform.c | 2 + drivers/amba/Makefile | 2 +- drivers/amba/acpi.c | 162 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 drivers/amba/acpi.c
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..04efcbd --- /dev/null +++ b/drivers/amba/acpi.c @@ -0,0 +1,162 @@ +/*
- AMBA Connector Resource for ACPI
- Copyright (C) 2013 Advanced Micro Devices, Inc.
- Author: Brandon Anderson <brandon.anderson@amd.commailto: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>
+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("%s: failed to map memory resource\n", __func__);}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;default:pr_debug("%s: unhandled acpi resource type= %d\n", __func__,ares->type);break;- }
- return 1; /* Tell ACPI core to skip this resource */ }
+static void acpi_amba_register_clk(struct acpi_device *adev) {
- /* TODO: Retrieve clock details from ACPI and register the appropriate
clock under this device for amba subsystem and drivers to+reference */ }
+/* 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 platform_device *pdata = data;
- 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 = pdata->dev.parent;
- //dev->dev.platform_data = platform_data; //FIXME: is this needed by any drivers?
- 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);
- 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_clk(adev);
- /* 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_CTRL_TERMINATE;
+}
+static int acpi_amba_bus_probe(struct platform_device *pdev) {
- // see if there's a top-level clock to use as default for sub-devices
- //TODO
Hmm, how about add _DSM under each device object which need the clock? I mean some thing like (not the real ASL code): Device (SER0) { .... Method(_DSM, 4, NotSerialized) { clock path = "\_SB.SMB.CLK0file:///\\_SB.SMB.CLK0" } } Then we can get the clock ACPI handle from the path, then ACPI dev, if we we attach the pointer of clock struct to that ACPI handle, we can directly get the clock struct.
\_SB.SMB.CLK0file:///\\_SB.SMB.CLK0 -> clock ACPI handle -> clock ACPI dev -> get the clk data
What do you think?
This is certainly the form we should be using based on ACPI vs DT discussion at kernel plumbers conference.
Graeme