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_.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.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_.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_.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.org]
Sent: Wednesday, October 23, 2013 7:50 AM
To: Hanjun Guo
Cc: Anderson, Brandon; 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'
On Wed, Oct 23, 2013 at 07:52:30PM +0800, Hanjun Guo wrote:
> On 2013-10-22 8:33, brandon.anderson@amd.com wrote:
> > From: Brandon Anderson <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.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.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.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.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