Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1313250 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/2] Add AMBA bus probing support to ACPI Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-20 15:40 +0100
    [PATCH v6 2/2] ACPI: amba bus probing support Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-20 15:40 +0100
    [PATCH v6 1/2] ACPI: introduce a function to find the first physical device Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-20 15:40 +0100
      Re: [PATCH v6 1/2] ACPI: introduce a function to find the first  physical device Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-20 16:20 +0100
        Re: [PATCH v6 1/2] ACPI: introduce a function to find the first  physical device Aleksey Makarov <aleksey.makarov@linaro.org> - 2016-01-20 18:10 +0100
          Re: [PATCH v6 1/2] ACPI: introduce a function to find the first  physical device Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-20 18:30 +0100

#1313250 — [PATCH v6 0/2] Add AMBA bus probing support to ACPI

As discussed when Shannon Zhao sent a patch to add platform_device support
to pl061 driver. Russell 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.

v6:
- Fix style issues (Andy Shevchenko)
- Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

v5:
https://lkml.kernel.org/g/1452518790-27053-1-git-send-email-aleksey.makarov@linaro.org
- 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 |  19 +------
 drivers/acpi/bus.c           |  33 ++++++++----
 drivers/acpi/internal.h      |   6 +++
 drivers/acpi/scan.c          |   1 +
 6 files changed, 154 insertions(+), 28 deletions(-)
 create mode 100644 drivers/acpi/acpi_amba.c

-- 
2.7.0

[toc] | [next] | [standalone]

#1313254 — [PATCH v6 2/2] ACPI: amba bus probing support

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.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
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 265eb90..f632ef1 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

[toc] | [prev] | [next] | [standalone]

#1313261 — [PATCH v6 1/2] ACPI: introduce a function to find the first physical device

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

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
 drivers/acpi/acpi_platform.c | 19 ++-----------------
 drivers/acpi/bus.c           | 33 ++++++++++++++++++++++-----------
 drivers/acpi/internal.h      |  1 +
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 296b7a1..c3af108 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;
@@ -82,22 +81,8 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 	 * attached to it, that physical device should be the parent of the
 	 * 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);
-	}
+	pdevinfo.parent = adev->parent ?
+		acpi_get_first_physical_node(adev->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..832b26d 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -478,24 +478,35 @@ 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 *node = NULL;
 
 	mutex_lock(physical_node_lock);
-	if (list_empty(&adev->physical_node_list)) {
-		adev = NULL;
-	} else {
-		const struct acpi_device_physical_node *node;
 
+	if (!list_empty(&adev->physical_node_list))
 		node = list_first_entry(&adev->physical_node_list,
-					struct acpi_device_physical_node, node);
-		if (node->dev != dev)
-			adev = NULL;
-	}
+				struct acpi_device_physical_node, node)->dev;
+
 	mutex_unlock(physical_node_lock);
-	return adev;
+
+	return node;
+}
+
+static struct acpi_device *acpi_primary_dev_companion(struct acpi_device *adev,
+						      const struct device *dev)
+{
+	const struct device *node = acpi_get_first_physical_node(adev);
+
+	if (node && node == 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

[toc] | [prev] | [next] | [standalone]

#1313296 — Re: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device

On Wed, Jan 20, 2016 at 4:29 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.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

Hmm… Sorry, didn't notice one style issue and there is one is matter
of taste below.

> --- 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[] = {

> +       pdevinfo.parent = adev->parent ?
> +               acpi_get_first_physical_node(adev->parent) : NULL;

Matter of taste, but I believe if-else looks better here even when
consumes +2 LOC.
Or, does it fit 80? How wide then?

> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -478,24 +478,35 @@ 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

'node' -> 'device node'
Name of the function is wrong.

> + * @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 *node = NULL;
>
>         mutex_lock(physical_node_lock);
> -       if (list_empty(&adev->physical_node_list)) {
> -               adev = NULL;
> -       } else {
> -               const struct acpi_device_physical_node *node;
>
> +       if (!list_empty(&adev->physical_node_list))
>                 node = list_first_entry(&adev->physical_node_list,
> -                                       struct acpi_device_physical_node, node);
> -               if (node->dev != dev)
> -                       adev = NULL;
> -       }
> +                               struct acpi_device_physical_node, node)->dev;

I didn't notice this '->dev' thingy. I supposed that the function
returns struct acpi_device_physical_node *, not struct device *.

Currently the name is not aligned with returned value.

> +
>         mutex_unlock(physical_node_lock);
> -       return adev;
> +
> +       return node;
> +}


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1313381 — Re: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device

Hi Andy,

On 20.01.2016 21:12, Andy Shevchenko wrote:
> On Wed, Jan 20, 2016 at 4:29 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.
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
>
> Hmm… Sorry, didn't notice one style issue and there is one is matter
> of taste below.
>
>> --- 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[] = {
>
>> +       pdevinfo.parent = adev->parent ?
>> +               acpi_get_first_physical_node(adev->parent) : NULL;
>
> Matter of taste, but I believe if-else looks better here even when
> consumes +2 LOC.
> Or, does it fit 80? How wide then?

It does not fit 80 chars.  And I would prefer to leave ?: here.

>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -478,24 +478,35 @@ 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
>
> 'node' -> 'device node'
> Name of the function is wrong.

I will fix the name of function.  The type of returned value is clear 
from the function definition.

>> + * @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 *node = NULL;
>>
>>          mutex_lock(physical_node_lock);
>> -       if (list_empty(&adev->physical_node_list)) {
>> -               adev = NULL;
>> -       } else {
>> -               const struct acpi_device_physical_node *node;
>>
>> +       if (!list_empty(&adev->physical_node_list))
>>                  node = list_first_entry(&adev->physical_node_list,
>> -                                       struct acpi_device_physical_node, node);
>> -               if (node->dev != dev)
>> -                       adev = NULL;
>> -       }
>> +                               struct acpi_device_physical_node, node)->dev;
>
> I didn't notice this '->dev' thingy. I supposed that the function
> returns struct acpi_device_physical_node *, not struct device *.
>
> Currently the name is not aligned with returned value.

It is aligned with the returned value (but not with the type of returned 
value).  So I would prefer to leave it as is.

Thank you for review.
Aleksey Makarov

>
>> +
>>          mutex_unlock(physical_node_lock);
>> -       return adev;
>> +
>> +       return node;
>> +}
>
>

[toc] | [prev] | [next] | [standalone]

#1313397 — Re: [PATCH v6 1/2] ACPI: introduce a function to find the first physical device

On Wed, Jan 20, 2016 at 7:00 PM, Aleksey Makarov
<aleksey.makarov@linaro.org> wrote:
> On 20.01.2016 21:12, Andy Shevchenko wrote:
>>
>> On Wed, Jan 20, 2016 at 4:29 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.
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

For all your comments, I have to withdraw my tag. You may return it
back if Rafael and / or Mika will be okay with these changes.
Sorry for inconvenience.

>>> Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
>>
>>
>> Hmm… Sorry, didn't notice one style issue and there is one is matter
>> of taste below.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web