[PATCH 0/3] Add AMBA bus probing support to ACPI

[PATCH 0/3] Add AMBA bus probing support to ACPI

[Graeme Gregory]

As discussed when Shannon Zhao sent a patch to add platform_device support
to pl061 driver. Russel and other maintainers prefered that ACPI learned
how to create AMBA devices rather than converting/adding platform_device
support to AMBA drivers.

http://comments.gmane.org/gmane.linux.ports.arm.kernel/431364

1) Adds basic AMBA device probing support to ACPI, it uses a whitelist of
device IDs as the number of AMBA devices is limited. Currently the two ids
present are those used in QEMU for arm64.

2) Adds the plumbing into ACPI probe sequence.

3) From ACPI pl011 is only defined (SBSA document) to be in SBSA mode which has
reduced functionality. There may be a better method to do this that I have
overlooked.

Thanks

Graeme

[PATCH 1/3] ACPI: amba bus probing support

[Graeme Gregory]

On ARM64 some devices use the AMBA device and not the platform bus for
probing so add support for this. Uses a dummy clock for apb_pclk as ACPI
does not have a suitable clock representation and to keep the core
AMBA bus code unchanged between probing methods.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 drivers/acpi/Makefile    |   1 +
 drivers/acpi/acpi_amba.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h     |   1 +
 3 files changed, 159 insertions(+)
 create mode 100644 drivers/acpi/acpi_amba.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b5e7cd8..b94aa9f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -43,6 +43,7 @@ acpi-y				+= pci_root.o pci_link.o pci_irq.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
+acpi-y				+= acpi_amba.o
 acpi-y				+= int340x_thermal.o
 acpi-y				+= power.o
 acpi-y				+= event.o
diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c
new file mode 100644
index 0000000..2f45b71
--- /dev/null
+++ b/drivers/acpi/acpi_amba.c
@@ -0,0 +1,157 @@
+
+/*
+ * ACPI support for platform bus type.
+ *
+ * Copyright (C) 2015, Linaro Ltd
+ * Authors: Graeme Gregory <graeme.gregory@linaro.org>
+ *
+ * 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.
+ */
+
+#include <linux/acpi.h>
+#include <linux/amba/bus.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#ifdef CONFIG_ARM_AMBA
+static const struct acpi_device_id amba_id_list[] = {
+	{"ARMH0011", 0}, /* PL011 SBSA Uart */
+	{"ARMH0061", 0}, /* PL061 GPIO Device */
+	{"", 0},
+};
+
+struct clk *amba_dummy_clk;
+
+void amba_register_dummy_clk(void)
+{
+	struct clk *clk;
+
+	/* If clock already registered */
+	if (amba_dummy_clk)
+		return;
+
+	clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL, CLK_IS_ROOT, 0);
+	clk_register_clkdev(clk, "apb_pclk", NULL);
+
+	amba_dummy_clk = clk;
+}
+
+/**
+ * acpi_create_amba_device - Create platform device for ACPI device node
+ * @adev: ACPI device node to create a AMBA device for.
+ *
+ * Create and register an AMBA device, populate its common
+ * resources and return a pointer to it.  Otherwise, return %NULL.
+ *
+ * Name of the AMBA device will be the same as @adev's.
+ */
+struct amba_device *acpi_create_amba_device(struct acpi_device *adev)
+{
+	struct amba_device *dev = NULL;
+	struct acpi_device *acpi_parent;
+	struct resource_entry *rentry;
+	struct list_head resource_list;
+	struct resource *resources = NULL;
+	bool address_found = false;
+	int ret, count, irq_no = 0;
+
+	/* If the ACPI node already has a physical device attached, skip it. */
+	if (adev->physical_node_count)
+		return NULL;
+
+	/* AMBA devices use amba_device not platform device */
+	if (acpi_match_device_ids(adev, amba_id_list))
+		return NULL;
+
+	amba_register_dummy_clk();
+
+	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 NULL;
+	}
+
+	INIT_LIST_HEAD(&resource_list);
+	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+	if (count < 0) {
+		return NULL;
+	} else if (count > 0) {
+		resources = kmalloc_array(count, sizeof(struct resource),
+				    GFP_KERNEL);
+		if (!resources) {
+			acpi_dev_free_resource_list(&resource_list);
+			return ERR_PTR(-ENOMEM);
+		}
+		count = 0;
+		list_for_each_entry(rentry, &resource_list, node) {
+			switch (resource_type(rentry->res)) {
+			case IORESOURCE_MEM:
+				if (!address_found) {
+					dev->res = *rentry->res;
+					address_found = true;
+				}
+				break;
+			case IORESOURCE_IRQ:
+				if (irq_no < AMBA_NR_IRQS)
+					dev->irq[irq_no++] = rentry->res->start;
+				break;
+			default:
+				dev_warn(&adev->dev, "Invalid resource\n");
+			}
+		}
+		acpi_dev_free_resource_list(&resource_list);
+	}
+
+	/*
+	 * If the ACPI node has a parent and that parent has a physical device
+	 * attached to it, that physical device should be the parent of the
+	 * platform device we are about to create.
+	 */
+	dev->dev.parent = NULL;
+	acpi_parent = adev->parent;
+	if (acpi_parent) {
+		struct acpi_device_physical_node *entry;
+		struct list_head *list;
+
+		mutex_lock(&acpi_parent->physical_node_lock);
+		list = &acpi_parent->physical_node_list;
+		if (!list_empty(list)) {
+			entry = list_first_entry(list,
+					struct acpi_device_physical_node,
+					node);
+			dev->dev.parent = entry->dev;
+		}
+		mutex_unlock(&acpi_parent->physical_node_lock);
+	}
+
+	dev_set_name(&dev->dev, "%s", dev_name(&adev->dev));
+	ACPI_COMPANION_SET(&dev->dev, adev);
+
+	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 dev;
+
+err_free:
+	amba_device_put(dev);
+	return NULL;
+}
+#else
+struct amba_device *acpi_create_amba_device(struct acpi_device *adev)
+{
+	return NULL;
+}
+#endif /* CONFIG_ARM_AMBA */
+EXPORT_SYMBOL_GPL(acpi_create_amba_device);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 84e7055..dc42ec0 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -457,6 +457,7 @@ int acpi_device_modalias(struct device *, char *, int);
 void acpi_walk_dep_device_list(acpi_handle handle);
 
 struct platform_device *acpi_create_platform_device(struct acpi_device *);
+struct amba_device *acpi_create_amba_device(struct acpi_device *);
 #define ACPI_PTR(_ptr)	(_ptr)
 
 #else	/* !CONFIG_ACPI */
-- 
2.4.3

[PATCH 2/3] ACPI: scan add call to probe amba devices

[Graeme Gregory]

Call the amba device creation function in the default enumeration path,
this is the same location platform devices are probed.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 drivers/acpi/scan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f834b8c..aa3184e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1633,8 +1633,10 @@ static void acpi_default_enumeration(struct acpi_device *device)
 	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
 			       &is_spi_i2c_slave);
 	acpi_dev_free_resource_list(&resource_list);
-	if (!is_spi_i2c_slave)
+	if (!is_spi_i2c_slave) {
+		acpi_create_amba_device(device);
 		acpi_create_platform_device(device);
+	}
 }
 
 static const struct acpi_device_id generic_device_ids[] = {
-- 
2.4.3

[PATCH 3/3] serial: amba-pl011: add ACPI support to AMBA probe

[Graeme Gregory]

In ACPI this device is only defined in SBSA mode so
if we are probing from ACPI use this mode.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 drivers/tty/serial/amba-pl011.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index fd27e98..55209aa 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2368,18 +2368,28 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	if (!uap)
 		return -ENOMEM;
 
-	uap->clk = devm_clk_get(&dev->dev, NULL);
-	if (IS_ERR(uap->clk))
-		return PTR_ERR(uap->clk);
-
-	uap->vendor = vendor;
-	uap->lcrh_rx = vendor->lcrh_rx;
-	uap->lcrh_tx = vendor->lcrh_tx;
-	uap->fifosize = vendor->get_fifosize(dev);
+	/* ACPI only defines SBSA variant */
+	if (!ACPI_COMPANION(&dev->dev)) {
+		uap->clk = devm_clk_get(&dev->dev, NULL);
+		if (IS_ERR(uap->clk))
+			return PTR_ERR(uap->clk);
+
+		uap->vendor = vendor;
+		uap->lcrh_rx = vendor->lcrh_rx;
+		uap->lcrh_tx = vendor->lcrh_tx;
+		uap->fifosize = vendor->get_fifosize(dev);
+		uap->port.ops = &amba_pl011_pops;
+		snprintf(uap->type, sizeof(uap->type), "PL011 rev%u",
+				amba_rev(dev));
+	 } else {
+		uap->vendor	= &vendor_sbsa;
+		uap->fifosize	= 32;
+		uap->port.ops	= &sbsa_uart_pops;
+		uap->fixed_baud = 115200;
+
+		snprintf(uap->type, sizeof(uap->type), "SBSA");
+	}
 	uap->port.irq = dev->irq[0];
-	uap->port.ops = &amba_pl011_pops;
-
-	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
 
 	ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);
 	if (ret)
-- 
2.4.3

Re: [PATCH 0/3] Add AMBA bus probing support to ACPI

[Al Stone]

On 09/30/2015 04:38 AM, Graeme Gregory wrote:
> As discussed when Shannon Zhao sent a patch to add platform_device support
> to pl061 driver. Russel and other maintainers prefered that ACPI learned
> how to create AMBA devices rather than converting/adding platform_device
> support to AMBA drivers.
> 
> http://comments.gmane.org/gmane.linux.ports.arm.kernel/431364
> 
> 1) Adds basic AMBA device probing support to ACPI, it uses a whitelist of
> device IDs as the number of AMBA devices is limited. Currently the two ids
> present are those used in QEMU for arm64.
> 
> 2) Adds the plumbing into ACPI probe sequence.
> 
> 3) From ACPI pl011 is only defined (SBSA document) to be in SBSA mode which has
> reduced functionality. There may be a better method to do this that I have
> overlooked.
> 
> Thanks
> 
> Graeme
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Any comments on these patches?  It's been awful quiet....

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/3] Add AMBA bus probing support to ACPI

[Rafael J. Wysocki]

On Wed, Oct 21, 2015 at 6:41 PM, Al Stone <ahs3@redhat.com> wrote:
> On 09/30/2015 04:38 AM, Graeme Gregory wrote:
>> As discussed when Shannon Zhao sent a patch to add platform_device support
>> to pl061 driver. Russel and other maintainers prefered that ACPI learned
>> how to create AMBA devices rather than converting/adding platform_device
>> support to AMBA drivers.
>>
>> http://comments.gmane.org/gmane.linux.ports.arm.kernel/431364
>>
>> 1) Adds basic AMBA device probing support to ACPI, it uses a whitelist of
>> device IDs as the number of AMBA devices is limited. Currently the two ids
>> present are those used in QEMU for arm64.
>>
>> 2) Adds the plumbing into ACPI probe sequence.
>>
>> 3) From ACPI pl011 is only defined (SBSA document) to be in SBSA mode which has
>> reduced functionality. There may be a better method to do this that I have
>> overlooked.
>>
>> Thanks
>>
>> Graeme
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Any comments on these patches?  It's been awful quiet....

That's because of my limited review bandwidth.

I'm kind of in the middle of travel now, sorry about that.

Thanks,
Rafael

Re: [PATCH 0/3] Add AMBA bus probing support to ACPI

[Al Stone]

On 10/21/2015 04:29 PM, Rafael J. Wysocki wrote:
> On Wed, Oct 21, 2015 at 6:41 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 09/30/2015 04:38 AM, Graeme Gregory wrote:
>>> As discussed when Shannon Zhao sent a patch to add platform_device support
>>> to pl061 driver. Russel and other maintainers prefered that ACPI learned
>>> how to create AMBA devices rather than converting/adding platform_device
>>> support to AMBA drivers.
>>>
>>> http://comments.gmane.org/gmane.linux.ports.arm.kernel/431364
>>>
>>> 1) Adds basic AMBA device probing support to ACPI, it uses a whitelist of
>>> device IDs as the number of AMBA devices is limited. Currently the two ids
>>> present are those used in QEMU for arm64.
>>>
>>> 2) Adds the plumbing into ACPI probe sequence.
>>>
>>> 3) From ACPI pl011 is only defined (SBSA document) to be in SBSA mode which has
>>> reduced functionality. There may be a better method to do this that I have
>>> overlooked.
>>>
>>> Thanks
>>>
>>> Graeme
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> Any comments on these patches?  It's been awful quiet....
> 
> That's because of my limited review bandwidth.
> 
> I'm kind of in the middle of travel now, sorry about that.
> 
> Thanks,
> Rafael
> 

Aha.  No worries, Rafael.  Just curious.  Thanks for checking in,
and safe travels!

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH 0/3] Add AMBA bus probing support to ACPI

[Shannon Zhao]

On 2015/9/30 18:38, Graeme Gregory wrote:
> As discussed when Shannon Zhao sent a patch to add platform_device support
> to pl061 driver. Russel and other maintainers prefered that ACPI learned
> how to create AMBA devices rather than converting/adding platform_device
> support to AMBA drivers.
>
> http://comments.gmane.org/gmane.linux.ports.arm.kernel/431364
>
> 1) Adds basic AMBA device probing support to ACPI, it uses a whitelist of
> device IDs as the number of AMBA devices is limited. Currently the two ids
> present are those used in QEMU for arm64.
>
> 2) Adds the plumbing into ACPI probe sequence.
>
> 3) From ACPI pl011 is only defined (SBSA document) to be in SBSA mode which has
> reduced functionality. There may be a better method to do this that I have
> overlooked.
>
Thanks. I test this patchset with PL061 on QEMU for arm64 through below 
patchset[1]. It works well.

Tested-by: Shannon Zhao <shannon.zhao@linaro.org>

[1] 
https://git.linaro.org/people/shannon.zhao/qemu.git/shortlog/refs/heads/PowerButton_v2

-- 
Shannon

Re: [PATCH 2/3] ACPI: scan add call to probe amba devices

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 11:38:49 AM Graeme Gregory wrote:
> Call the amba device creation function in the default enumeration path,
> this is the same location platform devices are probed.

First, I'm sorry for the glacially slow response here.

> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> ---
>  drivers/acpi/scan.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f834b8c..aa3184e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1633,8 +1633,10 @@ static void acpi_default_enumeration(struct acpi_device *device)
>  	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
>  			       &is_spi_i2c_slave);
>  	acpi_dev_free_resource_list(&resource_list);
> -	if (!is_spi_i2c_slave)
> +	if (!is_spi_i2c_slave) {
> +		acpi_create_amba_device(device);

Do you really have to add this here?

It would be much cleaner to use a new scan handler instead.

>  		acpi_create_platform_device(device);
> +	}
>  }
>  
>  static const struct acpi_device_id generic_device_ids[] = {
> 

Thanks,
Rafael

Re: [PATCH 3/3] serial: amba-pl011: add ACPI support to AMBA probe

[Rafael J. Wysocki]

On Wednesday, September 30, 2015 11:38:50 AM Graeme Gregory wrote:
> In ACPI this device is only defined in SBSA mode so
> if we are probing from ACPI use this mode.
> 
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index fd27e98..55209aa 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2368,18 +2368,28 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>  	if (!uap)
>  		return -ENOMEM;
>  
> -	uap->clk = devm_clk_get(&dev->dev, NULL);
> -	if (IS_ERR(uap->clk))
> -		return PTR_ERR(uap->clk);
> -
> -	uap->vendor = vendor;
> -	uap->lcrh_rx = vendor->lcrh_rx;
> -	uap->lcrh_tx = vendor->lcrh_tx;
> -	uap->fifosize = vendor->get_fifosize(dev);
> +	/* ACPI only defines SBSA variant */
> +	if (!ACPI_COMPANION(&dev->dev)) {

It would read more straightforward if you did

	if (ACPI_COMPANION(&dev->dev)) {
		<handle the ACPI case>
	} else {
		<handle the non-ACPI case>
	}

> +		uap->clk = devm_clk_get(&dev->dev, NULL);
> +		if (IS_ERR(uap->clk))
> +			return PTR_ERR(uap->clk);
> +
> +		uap->vendor = vendor;
> +		uap->lcrh_rx = vendor->lcrh_rx;
> +		uap->lcrh_tx = vendor->lcrh_tx;
> +		uap->fifosize = vendor->get_fifosize(dev);
> +		uap->port.ops = &amba_pl011_pops;
> +		snprintf(uap->type, sizeof(uap->type), "PL011 rev%u",
> +				amba_rev(dev));
> +	 } else {
> +		uap->vendor	= &vendor_sbsa;
> +		uap->fifosize	= 32;
> +		uap->port.ops	= &sbsa_uart_pops;
> +		uap->fixed_baud = 115200;
> +
> +		snprintf(uap->type, sizeof(uap->type), "SBSA");

This looks sort of heavy-handed.

Is this the only possible configuration of the device in the ACPI case?

> +	}
>  	uap->port.irq = dev->irq[0];
> -	uap->port.ops = &amba_pl011_pops;
> -
> -	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
>  
>  	ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);
>  	if (ret)

Thanks,
Rafael

Re: [PATCH 2/3] ACPI: scan add call to probe amba devices

[Graeme Gregory]

On Tue, Dec 01, 2015 at 03:16:53AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, September 30, 2015 11:38:49 AM Graeme Gregory wrote:
> > Call the amba device creation function in the default enumeration path,
> > this is the same location platform devices are probed.
> 
> First, I'm sorry for the glacially slow response here.
> 
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> > ---
> >  drivers/acpi/scan.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index f834b8c..aa3184e 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1633,8 +1633,10 @@ static void acpi_default_enumeration(struct acpi_device *device)
> >  	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
> >  			       &is_spi_i2c_slave);
> >  	acpi_dev_free_resource_list(&resource_list);
> > -	if (!is_spi_i2c_slave)
> > +	if (!is_spi_i2c_slave) {
> > +		acpi_create_amba_device(device);
> 
> Do you really have to add this here?
> 
> It would be much cleaner to use a new scan handler instead.
> 

I can certainly look into doing that, I cannot from a quick scan see any
reason not too. I just didn't think of it.

Graeme

Re: [PATCH 3/3] serial: amba-pl011: add ACPI support to AMBA probe

[Graeme Gregory]

On Tue, Dec 01, 2015 at 03:21:47AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, September 30, 2015 11:38:50 AM Graeme Gregory wrote:
> > In ACPI this device is only defined in SBSA mode so
> > if we are probing from ACPI use this mode.
> > 
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> > ---
> >  drivers/tty/serial/amba-pl011.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> > index fd27e98..55209aa 100644
> > --- a/drivers/tty/serial/amba-pl011.c
> > +++ b/drivers/tty/serial/amba-pl011.c
> > @@ -2368,18 +2368,28 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
> >  	if (!uap)
> >  		return -ENOMEM;
> >  
> > -	uap->clk = devm_clk_get(&dev->dev, NULL);
> > -	if (IS_ERR(uap->clk))
> > -		return PTR_ERR(uap->clk);
> > -
> > -	uap->vendor = vendor;
> > -	uap->lcrh_rx = vendor->lcrh_rx;
> > -	uap->lcrh_tx = vendor->lcrh_tx;
> > -	uap->fifosize = vendor->get_fifosize(dev);
> > +	/* ACPI only defines SBSA variant */
> > +	if (!ACPI_COMPANION(&dev->dev)) {
> 
> It would read more straightforward if you did
> 
> 	if (ACPI_COMPANION(&dev->dev)) {
> 		<handle the ACPI case>
> 	} else {
> 		<handle the non-ACPI case>
> 	}
> 

This was something I debated about whether to put the ACPI case or the
common case first. I can certainly reverse it.

> > +		uap->clk = devm_clk_get(&dev->dev, NULL);
> > +		if (IS_ERR(uap->clk))
> > +			return PTR_ERR(uap->clk);
> > +
> > +		uap->vendor = vendor;
> > +		uap->lcrh_rx = vendor->lcrh_rx;
> > +		uap->lcrh_tx = vendor->lcrh_tx;
> > +		uap->fifosize = vendor->get_fifosize(dev);
> > +		uap->port.ops = &amba_pl011_pops;
> > +		snprintf(uap->type, sizeof(uap->type), "PL011 rev%u",
> > +				amba_rev(dev));
> > +	 } else {
> > +		uap->vendor	= &vendor_sbsa;
> > +		uap->fifosize	= 32;
> > +		uap->port.ops	= &sbsa_uart_pops;
> > +		uap->fixed_baud = 115200;
> > +
> > +		snprintf(uap->type, sizeof(uap->type), "SBSA");
> 
> This looks sort of heavy-handed.
> 
> Is this the only possible configuration of the device in the ACPI case?
> 

As far as I can tell yes, but ARM haven't actually published a document
to state that as fact.

This does replace the platform_probe that Russel was unhappy about for
the ACPI case.

Graeme

Re: [PATCH 3/3] serial: amba-pl011: add ACPI support to AMBA probe

[Rafael J. Wysocki]

On Tuesday, December 01, 2015 12:21:00 PM Graeme Gregory wrote:
> On Tue, Dec 01, 2015 at 03:21:47AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, September 30, 2015 11:38:50 AM Graeme Gregory wrote:
> > > In ACPI this device is only defined in SBSA mode so
> > > if we are probing from ACPI use this mode.

[cut]

> > > +		uap->clk = devm_clk_get(&dev->dev, NULL);
> > > +		if (IS_ERR(uap->clk))
> > > +			return PTR_ERR(uap->clk);
> > > +
> > > +		uap->vendor = vendor;
> > > +		uap->lcrh_rx = vendor->lcrh_rx;
> > > +		uap->lcrh_tx = vendor->lcrh_tx;
> > > +		uap->fifosize = vendor->get_fifosize(dev);
> > > +		uap->port.ops = &amba_pl011_pops;
> > > +		snprintf(uap->type, sizeof(uap->type), "PL011 rev%u",
> > > +				amba_rev(dev));
> > > +	 } else {
> > > +		uap->vendor	= &vendor_sbsa;
> > > +		uap->fifosize	= 32;
> > > +		uap->port.ops	= &sbsa_uart_pops;
> > > +		uap->fixed_baud = 115200;
> > > +
> > > +		snprintf(uap->type, sizeof(uap->type), "SBSA");
> > 
> > This looks sort of heavy-handed.
> > 
> > Is this the only possible configuration of the device in the ACPI case?
> > 
> 
> As far as I can tell yes, but ARM haven't actually published a document
> to state that as fact.

At least a comment explaining what kind of information this is based on would
be useful.  Otherwise one has to wonder where this is coming from.

> This does replace the platform_probe that Russel was unhappy about for
> the ACPI case.

I can't really comment on that.

Thanks,
Rafael

Re: [PATCH 3/3] serial: amba-pl011: add ACPI support to AMBA probe

[Timur Tabi]

On Wed, Sep 30, 2015 at 5:38 AM, Graeme Gregory
<graeme.gregory@linaro.org> wrote:

> @@ -2368,18 +2368,28 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
>         if (!uap)
>                 return -ENOMEM;
>
> -       uap->clk = devm_clk_get(&dev->dev, NULL);
> -       if (IS_ERR(uap->clk))
> -               return PTR_ERR(uap->clk);
> -
> -       uap->vendor = vendor;
> -       uap->lcrh_rx = vendor->lcrh_rx;
> -       uap->lcrh_tx = vendor->lcrh_tx;
> -       uap->fifosize = vendor->get_fifosize(dev);
> +       /* ACPI only defines SBSA variant */
> +       if (!ACPI_COMPANION(&dev->dev)) {
> +               uap->clk = devm_clk_get(&dev->dev, NULL);
> +               if (IS_ERR(uap->clk))
> +                       return PTR_ERR(uap->clk);
> +
> +               uap->vendor = vendor;
> +               uap->lcrh_rx = vendor->lcrh_rx;
> +               uap->lcrh_tx = vendor->lcrh_tx;
> +               uap->fifosize = vendor->get_fifosize(dev);
> +               uap->port.ops = &amba_pl011_pops;
> +               snprintf(uap->type, sizeof(uap->type), "PL011 rev%u",
> +                               amba_rev(dev));
> +        } else {
> +               uap->vendor     = &vendor_sbsa;
> +               uap->fifosize   = 32;
> +               uap->port.ops   = &sbsa_uart_pops;
> +               uap->fixed_baud = 115200;
> +
> +               snprintf(uap->type, sizeof(uap->type), "SBSA");
> +       }
>         uap->port.irq = dev->irq[0];
> -       uap->port.ops = &amba_pl011_pops;
> -
> -       snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));

I'm confused.  We already have ACPI support in amba-pl011 driver, and
pl011_probe() is never called on an SBSA system.  That's what
sbsa_uart_probe() is for.  You even added this patch:

        drivers: PL011: add ACPI probing for SBSA UART

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.