[PATCH v5 0/2] ACPI: amba bus probing support

[PATCH v5 0/2] ACPI: amba bus probing support

[Aleksey Makarov]

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

This patchset

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 only id
present is of the GPIO device that is used in QEMU for arm64.

2) Adds the plumbing into ACPI probe sequence.

3) Factors out the code that finds the first physical node of an ACPI device.

v5:
- Introduce a new function acpi_get_first_physical_node() in a separate patch
  (Andy Shevchenko)
- Combine the patch "ACPI: amba bus probing support" with the one line patch
  "ACPI: scan add in amba probing"
- Drop patch "serial: amba-pl011: add ACPI support to AMBA probe" until maintainers
  decide on the further directions (see [1, 2])
- Don't initialize dev->dev.parent and set the device's name with amba_device_alloc()
  (Russell King)
- Fix minor bugs and style issues (Andy Shevchenko)

[1] https://lkml.kernel.org/g/20160106100700.GA19062@n2100.arm.linux.org.uk
[2] https://lkml.kernel.org/g/20160106110350.GB3599@xora-haswell.xora.org.uk

v4:
https://lkml.kernel.org/g/1450880383-29560-1-git-send-email-aleksey.makarov@linaro.org
- A memory leak has been fixed (Vladimir Zapolskiy)
- ACPI_COMPANION() -> has_acpi_companion() (Andy Shevchenko)
- pr_err() -> dev_err() (Andy Shevchenko)
- The call to amba_register_dummy_clk() has been moved to to acpi_amba_init()
  (Vladimir Zapolskiy)
- Return value has been fixed (Vladimir Zapolskiy)

v3:
https://lkml.kernel.org/g/1450716100-13688-1-git-send-email-aleksey.makarov@linaro.org
- Compilation without CONFIG_ARM_AMBA has been fixed

v2:
https://lkml.kernel.org/g/1450709399-7246-1-git-send-email-aleksey.makarov@linaro.org
- A new ACPI scan handler for AMBA devices has been implemented
- The order of `if` branches in amba-pl011.c has been changed
- A couple of `static`s have been added
- The compilation of the acpi_amba.c unit has made conditional
- A comment on SBSA UART has been added

v1:
https://lkml.kernel.org/g/1443609530-21524-1-git-send-email-graeme.gregory@linaro.org

Aleksey Makarov (1):
  ACPI: introduce a function to find the first physical device

Graeme Gregory (1):
  ACPI: amba bus probing support

 drivers/acpi/Makefile        |   1 +
 drivers/acpi/acpi_amba.c     | 122 +++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/acpi_platform.c |  18 +------
 drivers/acpi/bus.c           |  36 ++++++++-----
 drivers/acpi/internal.h      |   6 +++
 drivers/acpi/scan.c          |   1 +
 6 files changed, 156 insertions(+), 28 deletions(-)
 create mode 100644 drivers/acpi/acpi_amba.c

-- 
2.7.0

[PATCH v5 1/2] ACPI: introduce a function to find the first physical device

[Aleksey Makarov]

Factor out the code that finds the first physical device
of a given ACPI device.  It is used in several places.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/acpi_platform.c | 18 ++----------------
 drivers/acpi/bus.c           | 36 ++++++++++++++++++++++++------------
 drivers/acpi/internal.h      |  1 +
 3 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 296b7a1..c99aed0 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = {
 struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 {
 	struct platform_device *pdev = NULL;
-	struct acpi_device *acpi_parent;
 	struct platform_device_info pdevinfo;
 	struct resource_entry *rentry;
 	struct list_head resource_list;
@@ -83,21 +82,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 	 * platform device we are about to create.
 	 */
 	pdevinfo.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);
-			pdevinfo.parent = entry->dev;
-		}
-		mutex_unlock(&acpi_parent->physical_node_lock);
-	}
+	if (adev->parent)
+		pdevinfo.parent = acpi_get_first_physical_node(adev->parent);
 	pdevinfo.name = dev_name(&adev->dev);
 	pdevinfo.id = -1;
 	pdevinfo.res = resources;
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index a212cef..2048c69 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -478,24 +478,36 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device)
                              Device Matching
    -------------------------------------------------------------------------- */
 
-static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
-						      const struct device *dev)
+/**
+ * acpi_device_fix_parent - Get first physical node of an ACPI device
+ * @adev: ACPI device in question
+ */
+struct device *acpi_get_first_physical_node(
+	struct acpi_device *adev)
 {
 	struct mutex *physical_node_lock = &adev->physical_node_lock;
+	struct device *ret = NULL;
 
 	mutex_lock(physical_node_lock);
-	if (list_empty(&adev->physical_node_list)) {
-		adev = NULL;
-	} else {
-		const struct acpi_device_physical_node *node;
 
-		node = list_first_entry(&adev->physical_node_list,
-					struct acpi_device_physical_node, node);
-		if (node->dev != dev)
-			adev = NULL;
-	}
+	if (!list_empty(&adev->physical_node_list))
+		ret = list_first_entry(&adev->physical_node_list,
+				struct acpi_device_physical_node, node)->dev;
+
 	mutex_unlock(physical_node_lock);
-	return adev;
+
+	return ret;
+}
+
+static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
+						      const struct device *dev)
+{
+	const struct device *pdev = acpi_get_first_physical_node(adev);
+
+	if (pdev && pdev == dev)
+		return adev;
+
+	return NULL;
 }
 
 /**
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 11d87bf..ee9312ba 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -98,6 +98,7 @@ bool acpi_device_is_present(struct acpi_device *adev);
 bool acpi_device_is_battery(struct acpi_device *adev);
 bool acpi_device_is_first_physical_node(struct acpi_device *adev,
 					const struct device *dev);
+struct device *acpi_get_first_physical_node(struct acpi_device *adev);
 
 /* --------------------------------------------------------------------------
                      Device Matching and Notification
-- 
2.7.0

[PATCH v5 2/2] ACPI: amba bus probing support

[Aleksey Makarov]

From: Graeme Gregory <graeme.gregory@linaro.org>

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.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/Makefile    |   1 +
 drivers/acpi/acpi_amba.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h  |   5 ++
 drivers/acpi/scan.c      |   1 +
 4 files changed, 129 insertions(+)
 create mode 100644 drivers/acpi/acpi_amba.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 675eaf3..3cf732f 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-$(CONFIG_ARM_AMBA)	+= 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..2a61b54
--- /dev/null
+++ b/drivers/acpi/acpi_amba.c
@@ -0,0 +1,122 @@
+
+/*
+ * ACPI support for platform bus type.
+ *
+ * Copyright (C) 2015, Linaro Ltd
+ * Author: 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>
+
+#include "internal.h"
+
+static const struct acpi_device_id amba_id_list[] = {
+	{"ARMH0061", 0}, /* PL061 GPIO Device */
+	{"", 0},
+};
+
+static void amba_register_dummy_clk(void)
+{
+	static struct clk *amba_dummy_clk;
+
+	/* If clock already registered */
+	if (amba_dummy_clk)
+		return;
+
+	amba_dummy_clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL,
+						CLK_IS_ROOT, 0);
+	clk_register_clkdev(amba_dummy_clk, "apb_pclk", NULL);
+}
+
+static int amba_handler_attach(struct acpi_device *adev,
+				const struct acpi_device_id *id)
+{
+	struct amba_device *dev;
+	struct resource_entry *rentry;
+	struct list_head resource_list;
+	bool address_found = false;
+	int irq_no = 0;
+	int ret;
+
+	/* If the ACPI node already has a physical device attached, skip it. */
+	if (adev->physical_node_count)
+		return 0;
+
+	dev = amba_device_alloc(dev_name(&adev->dev), 0, 0);
+	if (!dev) {
+		dev_err(&adev->dev, "%s(): amba_device_alloc() failed\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&resource_list);
+	ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
+	if (ret < 0)
+		goto err_free;
+
+	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");
+			break;
+		}
+	}
+
+	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 amba device we are about to create.
+	 */
+	if (adev->parent)
+		dev->dev.parent = acpi_get_first_physical_node(adev->parent);
+
+	ACPI_COMPANION_SET(&dev->dev, adev);
+
+	ret = amba_device_add(dev, &iomem_resource);
+	if (ret) {
+		dev_err(&adev->dev, "%s(): amba_device_add() failed (%d)\n",
+		       __func__, ret);
+		goto err_free;
+	}
+
+	return 1;
+
+err_free:
+	amba_device_put(dev);
+	return ret;
+}
+
+static struct acpi_scan_handler amba_handler = {
+	.ids = amba_id_list,
+	.attach = amba_handler_attach,
+};
+
+void __init acpi_amba_init(void)
+{
+	amba_register_dummy_clk();
+	acpi_scan_add_handler(&amba_handler);
+}
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index ee9312ba..6086f66 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -29,6 +29,11 @@ void acpi_processor_init(void);
 void acpi_platform_init(void);
 void acpi_pnp_init(void);
 void acpi_int340x_thermal_init(void);
+#ifdef CONFIG_ARM_AMBA
+void acpi_amba_init(void);
+#else
+static inline void acpi_amba_init(void) {}
+#endif
 int acpi_sysfs_init(void);
 void acpi_container_init(void);
 void acpi_memory_hotplug_init(void);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 78d5f02..20c8cba 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1922,6 +1922,7 @@ int __init acpi_scan_init(void)
 	acpi_memory_hotplug_init();
 	acpi_pnp_init();
 	acpi_int340x_thermal_init();
+	acpi_amba_init();
 
 	acpi_scan_add_handler(&generic_device_handler);
 
-- 
2.7.0

Re: [PATCH v5 1/2] ACPI: introduce a function to find the first physical device

[Andy Shevchenko]

On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> Factor out the code that finds the first physical device
> of a given ACPI device.  It is used in several places.

My comments below.

Otherwise good look to me if Rafael has no objections.

FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -43,7 +43,6 @@ static const struct acpi_device_id forbidden_id_list[] = {
>  struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>  {
>         struct platform_device *pdev = NULL;
> -       struct acpi_device *acpi_parent;
>         struct platform_device_info pdevinfo;
>         struct resource_entry *rentry;
>         struct list_head resource_list;
> @@ -83,21 +82,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
>          * platform device we are about to create.
>          */
>         pdevinfo.parent = NULL;

No need to twice assign that, perhaps you may move to else branch of
the condition.

> -       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);
> -                       pdevinfo.parent = entry->dev;
> -               }
> -               mutex_unlock(&acpi_parent->physical_node_lock);
> -       }
> +       if (adev->parent)
> +               pdevinfo.parent = acpi_get_first_physical_node(adev->parent);

else
         pdevinfo.parent = NULL;


>         pdevinfo.name = dev_name(&adev->dev);
>         pdevinfo.id = -1;
>         pdevinfo.res = resources;
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cef..2048c69 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -478,24 +478,36 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device)
>                               Device Matching
>     -------------------------------------------------------------------------- */
>
> -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
> -                                                     const struct device *dev)
> +/**
> + * acpi_device_fix_parent - Get first physical node of an ACPI device
> + * @adev: ACPI device in question
> + */
> +struct device *acpi_get_first_physical_node(
> +       struct acpi_device *adev)
>  {
>         struct mutex *physical_node_lock = &adev->physical_node_lock;
> +       struct device *ret = NULL;

I would use ret -> node.

>
>         mutex_lock(physical_node_lock);
> -       if (list_empty(&adev->physical_node_list)) {
> -               adev = NULL;
> -       } else {
> -               const struct acpi_device_physical_node *node;
>
> -               node = list_first_entry(&adev->physical_node_list,
> -                                       struct acpi_device_physical_node, node);
> -               if (node->dev != dev)
> -                       adev = NULL;
> -       }
> +       if (!list_empty(&adev->physical_node_list))
> +               ret = list_first_entry(&adev->physical_node_list,
> +                               struct acpi_device_physical_node, node)->dev;
> +
>         mutex_unlock(physical_node_lock);
> -       return adev;
> +
> +       return ret;
> +}
> +
> +static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
> +                                                     const struct device *dev)
> +{
> +       const struct device *pdev = acpi_get_first_physical_node(adev);

Similar
pdev -> node.

> +
> +       if (pdev && pdev == dev)
> +               return adev;
> +
> +       return NULL;
>  }
>
>  /**
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 11d87bf..ee9312ba 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -98,6 +98,7 @@ bool acpi_device_is_present(struct acpi_device *adev);
>  bool acpi_device_is_battery(struct acpi_device *adev);
>  bool acpi_device_is_first_physical_node(struct acpi_device *adev,
>                                         const struct device *dev);
> +struct device *acpi_get_first_physical_node(struct acpi_device *adev);
>
>  /* --------------------------------------------------------------------------
>                       Device Matching and Notification
> --
> 2.7.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] ACPI: introduce a function to find the first physical device

[Andy Shevchenko]

+ One more thing

On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:

> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -478,24 +478,36 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device)
>                               Device Matching
>     -------------------------------------------------------------------------- */
>
> -static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
> -                                                     const struct device *dev)
> +/**
> + * acpi_device_fix_parent - Get first physical node of an ACPI device
> + * @adev: ACPI device in question
> + */

> +struct device *acpi_get_first_physical_node(
> +       struct acpi_device *adev)

One line (even if it goes a bit over 80)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Russell King - ARM Linux]

On Mon, Jan 11, 2016 at 07:26:27PM +0600, Aleksey Makarov wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
> 
> 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.
> 
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

This looks fine to me, thanks.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Andy Shevchenko]

On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
>
> 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.
>

My comments below.

> +++ b/drivers/acpi/acpi_amba.c
> @@ -0,0 +1,122 @@
> +
> +/*
> + * ACPI support for platform bus type.
> + *
> + * Copyright (C) 2015, Linaro Ltd
> + * Author: 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>
> +
> +#include "internal.h"
> +
> +static const struct acpi_device_id amba_id_list[] = {
> +       {"ARMH0061", 0}, /* PL061 GPIO Device */
> +       {"", 0},
> +};
> +
> +static void amba_register_dummy_clk(void)
> +{
> +       static struct clk *amba_dummy_clk;
> +
> +       /* If clock already registered */
> +       if (amba_dummy_clk)
> +               return;
> +
> +       amba_dummy_clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL,
> +                                               CLK_IS_ROOT, 0);
> +       clk_register_clkdev(amba_dummy_clk, "apb_pclk", NULL);
> +}
> +
> +static int amba_handler_attach(struct acpi_device *adev,
> +                               const struct acpi_device_id *id)
> +{
> +       struct amba_device *dev;
> +       struct resource_entry *rentry;
> +       struct list_head resource_list;
> +       bool address_found = false;
> +       int irq_no = 0;
> +       int ret;
> +
> +       /* If the ACPI node already has a physical device attached, skip it. */
> +       if (adev->physical_node_count)
> +               return 0;
> +
> +       dev = amba_device_alloc(dev_name(&adev->dev), 0, 0);
> +       if (!dev) {
> +               dev_err(&adev->dev, "%s(): amba_device_alloc() failed\n",
> +                       __func__);
> +               return -ENOMEM;
> +       }
> +
> +       INIT_LIST_HEAD(&resource_list);
> +       ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> +       if (ret < 0)
> +               goto err_free;
> +
> +       list_for_each_entry(rentry, &resource_list, node) {
> +               switch (resource_type(rentry->res)) {
> +               case IORESOURCE_MEM:
> +                       if (!address_found) {
> +                               dev->res = *rentry->res;

dev->res is 0 before this one, right? Could you use this fact instead
of address_found flag?

> +                               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");

Why? Isn't possible to have other resources for the devices?

> +                       break;
> +               }
> +       }
> +
> +       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 amba device we are about to create.
> +        */
> +       if (adev->parent)
> +               dev->dev.parent = acpi_get_first_physical_node(adev->parent);
> +
> +       ACPI_COMPANION_SET(&dev->dev, adev);
> +
> +       ret = amba_device_add(dev, &iomem_resource);
> +       if (ret) {

ret < 0?

What to do if ret > 0? It will be considered as not error. Please,
check what function returns and adjust this.

> +               dev_err(&adev->dev, "%s(): amba_device_add() failed (%d)\n",
> +                      __func__, ret);
> +               goto err_free;
> +       }
> +
> +       return 1;
> +
> +err_free:
> +       amba_device_put(dev);
> +       return ret;
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Russell King - ARM Linux]

On Mon, Jan 11, 2016 at 05:13:20PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
> <aleksey.makarov@linaro.org> wrote:
> > From: Graeme Gregory <graeme.gregory@linaro.org>
> >
> > 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.
> >
> 
> My comments below.
> 
> > +++ b/drivers/acpi/acpi_amba.c
> > @@ -0,0 +1,122 @@
> > +
> > +/*
> > + * ACPI support for platform bus type.
> > + *
> > + * Copyright (C) 2015, Linaro Ltd
> > + * Author: 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>
> > +
> > +#include "internal.h"
> > +
> > +static const struct acpi_device_id amba_id_list[] = {
> > +       {"ARMH0061", 0}, /* PL061 GPIO Device */
> > +       {"", 0},
> > +};
> > +
> > +static void amba_register_dummy_clk(void)
> > +{
> > +       static struct clk *amba_dummy_clk;
> > +
> > +       /* If clock already registered */
> > +       if (amba_dummy_clk)
> > +               return;
> > +
> > +       amba_dummy_clk = clk_register_fixed_rate(NULL, "apb_pclk", NULL,
> > +                                               CLK_IS_ROOT, 0);
> > +       clk_register_clkdev(amba_dummy_clk, "apb_pclk", NULL);
> > +}
> > +
> > +static int amba_handler_attach(struct acpi_device *adev,
> > +                               const struct acpi_device_id *id)
> > +{
> > +       struct amba_device *dev;
> > +       struct resource_entry *rentry;
> > +       struct list_head resource_list;
> > +       bool address_found = false;
> > +       int irq_no = 0;
> > +       int ret;
> > +
> > +       /* If the ACPI node already has a physical device attached, skip it. */
> > +       if (adev->physical_node_count)
> > +               return 0;
> > +
> > +       dev = amba_device_alloc(dev_name(&adev->dev), 0, 0);
> > +       if (!dev) {
> > +               dev_err(&adev->dev, "%s(): amba_device_alloc() failed\n",
> > +                       __func__);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       INIT_LIST_HEAD(&resource_list);
> > +       ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
> > +       if (ret < 0)
> > +               goto err_free;
> > +
> > +       list_for_each_entry(rentry, &resource_list, node) {
> > +               switch (resource_type(rentry->res)) {
> > +               case IORESOURCE_MEM:
> > +                       if (!address_found) {
> > +                               dev->res = *rentry->res;
> 
> dev->res is 0 before this one, right? Could you use this fact instead
> of address_found flag?

amba_device_alloc() zero-initialises everything.  However, dev->res is
a struct resource, and I'd prefer _this_ method that the OT is using
to testing some random part of struct resource.

> > +                               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");
> 
> Why? Isn't possible to have other resources for the devices?

AMBA primecell devices have one memory region, and a number of
interrupts.  Other resource types don't make sense.

> > +                       break;
> > +               }
> > +       }
> > +
> > +       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 amba device we are about to create.
> > +        */
> > +       if (adev->parent)
> > +               dev->dev.parent = acpi_get_first_physical_node(adev->parent);
> > +
> > +       ACPI_COMPANION_SET(&dev->dev, adev);
> > +
> > +       ret = amba_device_add(dev, &iomem_resource);
> > +       if (ret) {
> 
> ret < 0?
> 
> What to do if ret > 0? It will be considered as not error. Please,
> check what function returns and adjust this.

Non-zero is treated as an error by amba_device_add().  Doing otherwise
here puts it at odds to the outcome of that function.  This code is fine.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Andy Shevchenko]

On Mon, Jan 11, 2016 at 5:27 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 11, 2016 at 05:13:20PM +0200, Andy Shevchenko wrote:
>> On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
>> <aleksey.makarov@linaro.org> wrote:
>> > From: Graeme Gregory <graeme.gregory@linaro.org>

>> > +static int amba_handler_attach(struct acpi_device *adev,
>> > +                               const struct acpi_device_id *id)
>> > +{
>> > +       struct amba_device *dev;
>> > +       struct resource_entry *rentry;
>> > +       struct list_head resource_list;
>> > +       bool address_found = false;
>> > +       int irq_no = 0;
>> > +       int ret;
>> > +
>> > +       /* If the ACPI node already has a physical device attached, skip it. */
>> > +       if (adev->physical_node_count)
>> > +               return 0;
>> > +
>> > +       dev = amba_device_alloc(dev_name(&adev->dev), 0, 0);
>> > +       if (!dev) {
>> > +               dev_err(&adev->dev, "%s(): amba_device_alloc() failed\n",
>> > +                       __func__);
>> > +               return -ENOMEM;
>> > +       }
>> > +
>> > +       INIT_LIST_HEAD(&resource_list);
>> > +       ret = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
>> > +       if (ret < 0)
>> > +               goto err_free;
>> > +
>> > +       list_for_each_entry(rentry, &resource_list, node) {
>> > +               switch (resource_type(rentry->res)) {
>> > +               case IORESOURCE_MEM:
>> > +                       if (!address_found) {
>> > +                               dev->res = *rentry->res;
>>
>> dev->res is 0 before this one, right? Could you use this fact instead
>> of address_found flag?
>
> amba_device_alloc() zero-initialises everything.  However, dev->res is
> a struct resource, and I'd prefer _this_ method that the OT is using
> to testing some random part of struct resource.

So, you mean resource->start = 0 is not enough reliable?

>> > +               default:
>> > +                       dev_warn(&adev->dev, "Invalid resource\n");
>>
>> Why? Isn't possible to have other resources for the devices?
>
> AMBA primecell devices have one memory region, and a number of
> interrupts.  Other resource types don't make sense.

But isn't warning on the other side too noisy?

>> > +       ret = amba_device_add(dev, &iomem_resource);
>> > +       if (ret) {
>>
>> ret < 0?
>>
>> What to do if ret > 0? It will be considered as not error. Please,
>> check what function returns and adjust this.
>
> Non-zero is treated as an error by amba_device_add().

> Doing otherwise
> here puts it at odds to the outcome of that function.  This code is fine.

Yes, and in this case ret > 0 should be converted to an appropriate
error code, otherwise ACPI core will consider this as a normal
execution, right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Russell King - ARM Linux]

On Mon, Jan 11, 2016 at 06:26:00PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 11, 2016 at 5:27 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 11, 2016 at 05:13:20PM +0200, Andy Shevchenko wrote:
> >> On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
> >> <aleksey.makarov@linaro.org> wrote:
> >> dev->res is 0 before this one, right? Could you use this fact instead
> >> of address_found flag?
> >
> > amba_device_alloc() zero-initialises everything.  However, dev->res is
> > a struct resource, and I'd prefer _this_ method that the OT is using
> > to testing some random part of struct resource.
> 
> So, you mean resource->start = 0 is not enough reliable?

I'd rather not make assumptions about what in a resource is valid
or not valid.

> >> > +               default:
> >> > +                       dev_warn(&adev->dev, "Invalid resource\n");
> >>
> >> Why? Isn't possible to have other resources for the devices?
> >
> > AMBA primecell devices have one memory region, and a number of
> > interrupts.  Other resource types don't make sense.
> 
> But isn't warning on the other side too noisy?

Why would it be "too noisy" ?  Isn't it saying that the ACPI is in
error to include more resource types that aren't part of specifying
the AMBA primecell device?  Maybe it should be dev_err(), because
it's technically an error...

Are you expecting people to create ACPI tables with a lot of rubbish
resources attached to these devices?

> Yes, and in this case ret > 0 should be converted to an appropriate
> error code, otherwise ACPI core will consider this as a normal
> execution, right?

You are assuming that it does return a positive non-zero value in the
first place.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v5 2/2] ACPI: amba bus probing support

[Andy Shevchenko]

On Mon, Jan 11, 2016 at 7:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 11, 2016 at 06:26:00PM +0200, Andy Shevchenko wrote:
>> On Mon, Jan 11, 2016 at 5:27 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jan 11, 2016 at 05:13:20PM +0200, Andy Shevchenko wrote:
>> >> On Mon, Jan 11, 2016 at 3:26 PM, Aleksey Makarov
>> >> <aleksey.makarov@linaro.org> wrote:
>> >> dev->res is 0 before this one, right? Could you use this fact instead
>> >> of address_found flag?
>> >
>> > amba_device_alloc() zero-initialises everything.  However, dev->res is
>> > a struct resource, and I'd prefer _this_ method that the OT is using
>> > to testing some random part of struct resource.
>>
>> So, you mean resource->start = 0 is not enough reliable?
>
> I'd rather not make assumptions about what in a resource is valid
> or not valid.

Fair enough.

>> >> > +               default:
>> >> > +                       dev_warn(&adev->dev, "Invalid resource\n");
>> >>
>> >> Why? Isn't possible to have other resources for the devices?
>> >
>> > AMBA primecell devices have one memory region, and a number of
>> > interrupts.  Other resource types don't make sense.
>>
>> But isn't warning on the other side too noisy?
>
> Why would it be "too noisy" ?  Isn't it saying that the ACPI is in
> error to include more resource types that aren't part of specifying
> the AMBA primecell device?  Maybe it should be dev_err(), because
> it's technically an error...
>
> Are you expecting people to create ACPI tables with a lot of rubbish
> resources attached to these devices?

I'm expecting broken ACPI tables coming from some vendors which is
often the case for some devices.
I would rather leave it as a warning or move to less verbose level.

>> Yes, and in this case ret > 0 should be converted to an appropriate
>> error code, otherwise ACPI core will consider this as a normal
>> execution, right?
>
> You are assuming that it does return a positive non-zero value in the
> first place.

Frankly, I didn't check what that function can return, but I'm sure
that if it will return positive value at some point this will print a
message and tell ACPI that everything okay when it's not.
So, here I suppose to have explicit check for that, or at least (if
you believe that above will never happen) to show that only negative
numbers are possible as error values.

However, I will not insist if Rafael is okay with the original code.

-- 
With Best Regards,
Andy Shevchenko