[PATCH v5 RESEND 0/5] AMBA: add complete support for power domains

[PATCH v5 RESEND 0/5] AMBA: add complete support for power domains

[Marek Szyprowski]

(Old thread name: Exynos4210: fix power domain for MDMA1 device)

This patchset fixes mysterious boot hang on Exynos 4210 SoCs, when IOMMU
is enabled. There is no direct dependency between IOMMU devices and
MDMA1. However enabling IOMMU changes the device probe order, what
results in LCD0 power domain being turned off for some time. During that
time the registration of MDMA1 device happens, what results in system
hangs, because the common bus code tries to read PID/CID registers from
turned-off device.

Since the proposed change in the driver core had impact on existing
drivers (see '-next regression: "driver cohandle -EPROBE_DEFER from
bus_type.match()"' thread https://lkml.org/lkml/2015/12/17/390 ), I've
checked all the functions assigned to match callback of struct bus_type
objects and found that there are only 4 such functions that don't return
0/1 values:

1. arch/arm/common/sa1111.c: sa1111_match -> result of bitwise &
2. drivers/nvdimm/bus.c: nvdimm_bus_match -> result of test_bit()
3. drivers/sh/superhyway/superhyway.c: superhyway_bus_match -> error codes
     (this is really funny case, all errors are resolved to 'matched' case)
4. drivers/staging/unisys/visorbus/visorbus_main.c: visorbus_match -> 0 and
     some positive integer values, safe for now

The list of functions that I've examined has been generated by following
shell command:
$ git grep -Pp "\.match\s" | grep -A1 bus_type | grep "\.match\s" | tr -s "\t;&,=:" " " | cut -d" " -f1,3

Only the first two functions require potential fixing to ensure that
correct match will not result in negative return value, so the 2
additional patches have been added to v5 patchset.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Changelog:

v5:
- added 2 more patches to avoid regression with existing drivers (nvdimm and
  sa1111), for more information, see https://lkml.org/lkml/2015/12/17/390
- changed thread name to "AMBA: add complete support for power domains"

v4: https://lkml.org/lkml/2015/12/2/52
- fixed more issues pointed by Ulf Hansson and Russell King

v3: https://lkml.org/lkml/2015/12/1/334
- fixed issues pointed by Ulf Hansson
- dropped patch for exynos4210 dts, because it already got queued for merging

v2: https://lkml.org/lkml/2015/11/26/229
- added 2 patches from 'On-demand device probing' thread
  (https://lkml.org/lkml/2015/9/29/189), which move PID/CIR reading
  from amba_device_add() to amba_match()
- moved dev_pm_domain_attach() to amba_match(), which is allowed to
  return -EPROBE_DEFER

v1: http://www.spinics.net/lists/arm-kernel/msg463185.html
- initial version


Patch summary:

Dan Williams (1):
  drivers: nvdimm: ensure no negative value gets returned on positive
    match

Marek Szyprowski (2):
  ARM: sa1111: ensure no negative value gets returned on positive match
  ARM: amba: Properly handle devices with power domains

Tomeu Vizoso (2):
  driver core: handle -EPROBE_DEFER from bus_type.match()
  ARM: amba: Move reading of periphid to amba_match()

 Documentation/driver-model/porting.txt |   6 +-
 arch/arm/common/sa1111.c               |   2 +-
 drivers/amba/bus.c                     | 152 ++++++++++++++++++---------------
 drivers/base/dd.c                      |  24 +++++-
 drivers/nvdimm/bus.c                   |   2 +-
 include/linux/device.h                 |   7 +-
 6 files changed, 114 insertions(+), 79 deletions(-)

-- 
1.9.2

[PATCH v5 RESEND 1/5] drivers: nvdimm: ensure no negative value gets returned on positive match

[Marek Szyprowski]

From: Dan Williams <dan.j.williams@intel.com>

This patch ensures that existing bus match callbacks don't return
negative values (which might be interpreted as potential errors in the
future) in case of positive match.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/nvdimm/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 7e2c43f..2b2181c 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -62,7 +62,7 @@ static int nvdimm_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct nd_device_driver *nd_drv = to_nd_device_driver(drv);
 
-	return test_bit(to_nd_device_type(dev), &nd_drv->type);
+	return !!test_bit(to_nd_device_type(dev), &nd_drv->type);
 }
 
 static struct module *to_bus_provider(struct device *dev)
-- 
1.9.2

[PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets returned on positive match

[Marek Szyprowski]

This patch ensures that existing bus match callbacks don't return
negative values (which might be interpreted as potential errors in the
future) in case of positive match.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/common/sa1111.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index 3d22494..fb0a0a4 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, struct device_driver *_drv)
 	struct sa1111_dev *dev = SA1111_DEV(_dev);
 	struct sa1111_driver *drv = SA1111_DRV(_drv);
 
-	return dev->devid & drv->devid;
+	return !!(dev->devid & drv->devid);
 }
 
 static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
-- 
1.9.2

[PATCH v5 RESEND 2/5] ARM: sa1111: ensure no negative value gets returned on positive match

[Ulf Hansson]

On 10 February 2016 at 11:47, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> This patch ensures that existing bus match callbacks don't return
> negative values (which might be interpreted as potential errors in the
> future) in case of positive match.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

FYI:
This one has actually been acked by Russell.
https://lkml.org/lkml/2015/12/23/111

Kind regards
Uffe

> ---
>  arch/arm/common/sa1111.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> index 3d22494..fb0a0a4 100644
> --- a/arch/arm/common/sa1111.c
> +++ b/arch/arm/common/sa1111.c
> @@ -1290,7 +1290,7 @@ static int sa1111_match(struct device *_dev, struct device_driver *_drv)
>         struct sa1111_dev *dev = SA1111_DEV(_dev);
>         struct sa1111_driver *drv = SA1111_DRV(_drv);
>
> -       return dev->devid & drv->devid;
> +       return !!(dev->devid & drv->devid);
>  }
>
>  static int sa1111_bus_suspend(struct device *dev, pm_message_t state)
> --
> 1.9.2
>

[PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from bus_type.match()

[Marek Szyprowski]

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Allow implementations of the match() callback in struct bus_type to
return errors and if it's -EPROBE_DEFER then queue the device for
deferred probing.

This is useful to buses such as AMBA in which devices are registered
before their matching information can be retrieved from the HW
(typically because a clock driver hasn't probed yet).

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
[changed if-else code structure, adjusted documentation to match the code,
extended comments]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/driver-model/porting.txt |  6 ++++--
 drivers/base/dd.c                      | 24 ++++++++++++++++++++++--
 include/linux/device.h                 |  7 +++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-model/porting.txt b/Documentation/driver-model/porting.txt
index 92d86f7..453053f 100644
--- a/Documentation/driver-model/porting.txt
+++ b/Documentation/driver-model/porting.txt
@@ -340,8 +340,10 @@ comparison:
 
   int (*match)(struct device * dev, struct device_driver * drv);
 
-match should return '1' if the driver supports the device, and '0'
-otherwise. 
+match should return positive value if the driver supports the device,
+and zero otherwise. It may also return error code (for example
+-EPROBE_DEFER) if determining that given driver supports the device is
+not possible.
 
 When a device is registered, the bus's list of drivers is iterated
 over. bus->match() is called for each one until a match is found. 
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 7399be7..7c3f1f1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -544,6 +544,7 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	struct device_attach_data *data = _data;
 	struct device *dev = data->dev;
 	bool async_allowed;
+	int ret;
 
 	/*
 	 * Check if device has already been claimed. This may
@@ -554,8 +555,17 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
 	if (dev->driver)
 		return -EBUSY;
 
-	if (!driver_match_device(drv, dev))
+	ret = driver_match_device(drv, dev);
+	if (ret == 0) {
+		/* no match */
 		return 0;
+	} else if (ret == -EPROBE_DEFER) {
+		dev_dbg(dev, "Device match requests probe deferral\n");
+		driver_deferred_probe_add(dev);
+	} else if (ret < 0) {
+		dev_dbg(dev, "Bus failed to match device: %d", ret);
+		return ret;
+	} /* ret > 0 means positive match */
 
 	async_allowed = driver_allows_async_probing(drv);
 
@@ -675,6 +685,7 @@ void device_initial_probe(struct device *dev)
 static int __driver_attach(struct device *dev, void *data)
 {
 	struct device_driver *drv = data;
+	int ret;
 
 	/*
 	 * Lock device and try to bind to it. We drop the error
@@ -686,8 +697,17 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */
 
-	if (!driver_match_device(drv, dev))
+	ret = driver_match_device(drv, dev);
+	if (ret == 0) {
+		/* no match */
 		return 0;
+	} else if (ret == -EPROBE_DEFER) {
+		dev_dbg(dev, "Device match requests probe deferral\n");
+		driver_deferred_probe_add(dev);
+	} else if (ret < 0) {
+		dev_dbg(dev, "Bus failed to match device: %d", ret);
+		return ret;
+	} /* ret > 0 means positive match */
 
 	if (dev->parent)	/* Needed for USB */
 		device_lock(dev->parent);
diff --git a/include/linux/device.h b/include/linux/device.h
index f627ba2..75a2cde 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -70,8 +70,11 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @dev_groups:	Default attributes of the devices on the bus.
  * @drv_groups: Default attributes of the device drivers on the bus.
  * @match:	Called, perhaps multiple times, whenever a new device or driver
- *		is added for this bus. It should return a nonzero value if the
- *		given device can be handled by the given driver.
+ *		is added for this bus. It should return a positive value if the
+ *		given device can be handled by the given driver and zero
+ *		otherwise. It may also return error code if determining that
+ *		the driver supports the device is not possible. In case of
+ *		-EPROBE_DEFER it will queue the device for deferred probing.
  * @uevent:	Called when a device is added, removed, or a few other things
  *		that generate uevents to add the environment variables.
  * @probe:	Called when a new device or driver add to this bus, and callback
-- 
1.9.2

[PATCH v5 RESEND 3/5] driver core: handle -EPROBE_DEFER from bus_type.match()

[Greg Kroah-Hartman]

On Wed, Feb 10, 2016 at 11:47:28AM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> Allow implementations of the match() callback in struct bus_type to
> return errors and if it's -EPROBE_DEFER then queue the device for
> deferred probing.
> 
> This is useful to buses such as AMBA in which devices are registered
> before their matching information can be retrieved from the HW
> (typically because a clock driver hasn't probed yet).
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> [changed if-else code structure, adjusted documentation to match the code,
> extended comments]
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/driver-model/porting.txt |  6 ++++--
>  drivers/base/dd.c                      | 24 ++++++++++++++++++++++--
>  include/linux/device.h                 |  7 +++++--
>  3 files changed, 31 insertions(+), 6 deletions(-)


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Marek Szyprowski]

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Reading the periphid when the Primecell device is registered means that
the apb pclk must be available by then or the device won't be registered
at all.

By reading the periphid in amba_match() we can return -EPROBE_DEFER if
the apb pclk isn't there yet and the device will be retried later.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
[minor code adjustments, removed forward declaration, added missing
comment]
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c | 146 +++++++++++++++++++++++++++--------------------------
 1 file changed, 75 insertions(+), 71 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f009936..643127f 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -24,6 +24,71 @@
 
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
+static int amba_get_enable_pclk(struct amba_device *pcdev)
+{
+	int ret;
+
+	pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
+	if (IS_ERR(pcdev->pclk))
+		return PTR_ERR(pcdev->pclk);
+
+	ret = clk_prepare_enable(pcdev->pclk);
+	if (ret)
+		clk_put(pcdev->pclk);
+
+	return ret;
+}
+
+static void amba_put_disable_pclk(struct amba_device *pcdev)
+{
+	clk_disable_unprepare(pcdev->pclk);
+	clk_put(pcdev->pclk);
+}
+
+static int amba_read_periphid(struct amba_device *dev)
+{
+	u32 size;
+	void __iomem *tmp;
+	int i, ret = 0;
+
+	/*
+	 * Dynamically calculate the size of the resource
+	 * and use this for iomap
+	 */
+	size = resource_size(&dev->res);
+	tmp = ioremap(dev->res.start, size);
+	if (!tmp)
+		return -ENOMEM;
+
+	ret = amba_get_enable_pclk(dev);
+	if (ret == 0) {
+		u32 pid, cid;
+
+		/*
+		 * Read pid and cid based on size of resource
+		 * they are located at end of region
+		 */
+		for (pid = 0, i = 0; i < 4; i++)
+			pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) <<
+				(i * 8);
+		for (cid = 0, i = 0; i < 4; i++)
+			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
+				(i * 8);
+
+		amba_put_disable_pclk(dev);
+
+		if (cid == AMBA_CID || cid == CORESIGHT_CID)
+			dev->periphid = pid;
+
+		if (!dev->periphid)
+			ret = -ENODEV;
+	}
+
+	iounmap(tmp);
+
+	return ret;
+}
+
 static const struct amba_id *
 amba_lookup(const struct amba_id *table, struct amba_device *dev)
 {
@@ -43,11 +108,19 @@ static int amba_match(struct device *dev, struct device_driver *drv)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(drv);
+	int ret;
 
 	/* When driver_override is set, only bind to the matching driver */
 	if (pcdev->driver_override)
 		return !strcmp(pcdev->driver_override, drv->name);
 
+	/* Do plug-n-play if no hard-coded primecell ID has been provided */
+	if (!pcdev->periphid) {
+		ret = amba_read_periphid(pcdev);
+		if (ret)
+			return ret;
+	}
+
 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
 
@@ -204,27 +277,6 @@ static int __init amba_init(void)
 
 postcore_initcall(amba_init);
 
-static int amba_get_enable_pclk(struct amba_device *pcdev)
-{
-	int ret;
-
-	pcdev->pclk = clk_get(&pcdev->dev, "apb_pclk");
-	if (IS_ERR(pcdev->pclk))
-		return PTR_ERR(pcdev->pclk);
-
-	ret = clk_prepare_enable(pcdev->pclk);
-	if (ret)
-		clk_put(pcdev->pclk);
-
-	return ret;
-}
-
-static void amba_put_disable_pclk(struct amba_device *pcdev)
-{
-	clk_disable_unprepare(pcdev->pclk);
-	clk_put(pcdev->pclk);
-}
-
 /*
  * These are the device model conversion veneers; they convert the
  * device model structures to our more specific structures.
@@ -341,15 +393,12 @@ static void amba_device_release(struct device *dev)
  *	@dev: AMBA device allocated by amba_device_alloc
  *	@parent: resource parent for this devices resources
  *
- *	Claim the resource, and read the device cell ID if not already
- *	initialized.  Register the AMBA device with the Linux device
+ *	Claim the resource, and register the AMBA device with the Linux device
  *	manager.
  */
 int amba_device_add(struct amba_device *dev, struct resource *parent)
 {
-	u32 size;
-	void __iomem *tmp;
-	int i, ret;
+	int ret;
 
 	WARN_ON(dev->irq[0] == (unsigned int)-1);
 	WARN_ON(dev->irq[1] == (unsigned int)-1);
@@ -358,51 +407,6 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 	if (ret)
 		goto err_out;
 
-	/* Hard-coded primecell ID instead of plug-n-play */
-	if (dev->periphid != 0)
-		goto skip_probe;
-
-	/*
-	 * Dynamically calculate the size of the resource
-	 * and use this for iomap
-	 */
-	size = resource_size(&dev->res);
-	tmp = ioremap(dev->res.start, size);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto err_release;
-	}
-
-	ret = amba_get_enable_pclk(dev);
-	if (ret == 0) {
-		u32 pid, cid;
-
-		/*
-		 * Read pid and cid based on size of resource
-		 * they are located at end of region
-		 */
-		for (pid = 0, i = 0; i < 4; i++)
-			pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) <<
-				(i * 8);
-		for (cid = 0, i = 0; i < 4; i++)
-			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
-				(i * 8);
-
-		amba_put_disable_pclk(dev);
-
-		if (cid == AMBA_CID || cid == CORESIGHT_CID)
-			dev->periphid = pid;
-
-		if (!dev->periphid)
-			ret = -ENODEV;
-	}
-
-	iounmap(tmp);
-
-	if (ret)
-		goto err_release;
-
- skip_probe:
 	ret = device_add(&dev->dev);
 	if (ret)
 		goto err_release;
-- 
1.9.2

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Russell King - ARM Linux]

On Wed, Feb 10, 2016 at 11:47:29AM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> Reading the periphid when the Primecell device is registered means that
> the apb pclk must be available by then or the device won't be registered
> at all.
> 
> By reading the periphid in amba_match() we can return -EPROBE_DEFER if
> the apb pclk isn't there yet and the device will be retried later.

I've just realised, we can't do this.  We need to read the peripheral
ID at registration time, because that's published to userspace via
(a) a sysfs attribute, and (b) as part of the uevent, which will be
used by udev to locate the driver module.

So, this will have the side effect of breaking systems which have
AMBA primecell devices configured as modules.

Sorry, I can't apply this.  We can't regress existing platforms for
the sake of introducing new platforms to this code.

-- 
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.

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Russell King - ARM Linux]

On Mon, Feb 15, 2016 at 05:52:50PM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 10, 2016 at 11:47:29AM +0100, Marek Szyprowski wrote:
> > From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > 
> > Reading the periphid when the Primecell device is registered means that
> > the apb pclk must be available by then or the device won't be registered
> > at all.
> > 
> > By reading the periphid in amba_match() we can return -EPROBE_DEFER if
> > the apb pclk isn't there yet and the device will be retried later.
> 
> I've just realised, we can't do this.  We need to read the peripheral
> ID at registration time, because that's published to userspace via
> (a) a sysfs attribute, and (b) as part of the uevent, which will be
> used by udev to locate the driver module.
> 
> So, this will have the side effect of breaking systems which have
> AMBA primecell devices configured as modules.
> 
> Sorry, I can't apply this.  We can't regress existing platforms for
> the sake of introducing new platforms to this code.

I just re-applied the patch set to the amba branch, and then dropped
the last two patches because of this issue.  Please ignore the "applied"
notification for the last two patches from the patch system.

-- 
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.

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Marek Szyprowski]

Hello,

On 2016-02-15 18:52, Russell King - ARM Linux wrote:
> On Wed, Feb 10, 2016 at 11:47:29AM +0100, Marek Szyprowski wrote:
>> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> Reading the periphid when the Primecell device is registered means that
>> the apb pclk must be available by then or the device won't be registered
>> at all.
>>
>> By reading the periphid in amba_match() we can return -EPROBE_DEFER if
>> the apb pclk isn't there yet and the device will be retried later.
> I've just realised, we can't do this.  We need to read the peripheral
> ID at registration time, because that's published to userspace via
> (a) a sysfs attribute, and (b) as part of the uevent, which will be
> used by udev to locate the driver module.
>
> So, this will have the side effect of breaking systems which have
> AMBA primecell devices configured as modules.
>
> Sorry, I can't apply this.  We can't regress existing platforms for
> the sake of introducing new platforms to this code.

Then the only solution right now I see is to get back to v1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388199.html
which at least handles correctly device registration when power domain 
driver
is available. You pointed that the patch cannot be applied, because failure
of dev_pm_domain_attach() will be fatal for device registration. Right now
lack of such call is fatal for the whole system, so there is really not a
big difference. Please also note that amba_get_enable_pclk() calls 
clk_get(),
which also might return -EPROBE_DEFER, which already breaks device
registration the same way.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Russell King - ARM Linux]

On Wed, Feb 17, 2016 at 08:52:36AM +0100, Marek Szyprowski wrote:
> Then the only solution right now I see is to get back to v1:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388199.html
> which at least handles correctly device registration when power domain
> driver is available.

... and which has the ability to break platforms if the PM domain is
not already available.

What's wrong with the patch in the link above _combined_ with a patch
which addresses the concern I have with that patch: build a list of
the failed-to-register devices, and retry them later - maybe from a
late_initcall(), or a similar mechanism?

My view is the risk to existing systems is _too_ high to apply either
this patch, or the patch you link to above, and I refuse to play the
"lets apply it and see if we break anything" lottery with this.

-- 
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.

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Ulf Hansson]

On 17 February 2016 at 21:08, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Feb 17, 2016 at 08:52:36AM +0100, Marek Szyprowski wrote:
>> Then the only solution right now I see is to get back to v1:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388199.html
>> which at least handles correctly device registration when power domain
>> driver is available.
>
> ... and which has the ability to break platforms if the PM domain is
> not already available.
>
> What's wrong with the patch in the link above _combined_ with a patch
> which addresses the concern I have with that patch: build a list of
> the failed-to-register devices, and retry them later - maybe from a
> late_initcall(), or a similar mechanism?

This will improve the robustness of the device registration process,
but I wonder if it's really worth the efforts of complicating the amba
device registration code.

The problem I see with such approach, is to know *when* shall we retry
to register the devices.

We will rely on the PM domain driver to be probed, as to have the
corresponding OF genpd provider registered, else the device
registration will continue to fail.
Now, I don't think there are PM domain drivers as removable modules
(yet), but in such cases a late_initcall won't help much.

>
> My view is the risk to existing systems is _too_ high to apply either
> this patch, or the patch you link to above, and I refuse to play the
> "lets apply it and see if we break anything" lottery with this.

I agree!

Although I think with some adjustments, perhaps we can move forward with v1?

The adjustments I think is needed:
Instead of propagating the error code from dev_pm_domain_attach(),
let's print a debug message and continue the device registration.
In that way, we shouldn't introduce regressions for cases where the OF
genpd provider hasn't yet been registered, but the issue Marek is
trying to solve for Exynos should be fixed.

What do you think?

Kind regards
Uffe

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Ulf Hansson]

- trimmed cc list

On 2 March 2016 at 11:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 17 February 2016 at 21:08, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Feb 17, 2016 at 08:52:36AM +0100, Marek Szyprowski wrote:
>>> Then the only solution right now I see is to get back to v1:
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388199.html
>>> which at least handles correctly device registration when power domain
>>> driver is available.
>>
>> ... and which has the ability to break platforms if the PM domain is
>> not already available.
>>
>> What's wrong with the patch in the link above _combined_ with a patch
>> which addresses the concern I have with that patch: build a list of
>> the failed-to-register devices, and retry them later - maybe from a
>> late_initcall(), or a similar mechanism?
>
> This will improve the robustness of the device registration process,
> but I wonder if it's really worth the efforts of complicating the amba
> device registration code.
>
> The problem I see with such approach, is to know *when* shall we retry
> to register the devices.
>
> We will rely on the PM domain driver to be probed, as to have the
> corresponding OF genpd provider registered, else the device
> registration will continue to fail.
> Now, I don't think there are PM domain drivers as removable modules
> (yet), but in such cases a late_initcall won't help much.
>
>>
>> My view is the risk to existing systems is _too_ high to apply either
>> this patch, or the patch you link to above, and I refuse to play the
>> "lets apply it and see if we break anything" lottery with this.
>
> I agree!
>
> Although I think with some adjustments, perhaps we can move forward with v1?
>
> The adjustments I think is needed:
> Instead of propagating the error code from dev_pm_domain_attach(),
> let's print a debug message and continue the device registration.
> In that way, we shouldn't introduce regressions for cases where the OF
> genpd provider hasn't yet been registered, but the issue Marek is
> trying to solve for Exynos should be fixed.
>
> What do you think?
>

Ping. Any news on this? Would be nice if we could agree on a way forward.

Kind regards
Uffe

[PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to amba_match()

[Marek Szyprowski]

Hi Ulf,

On 2016-04-12 13:39, Ulf Hansson wrote:
> - trimmed cc list
>
> On 2 March 2016 at 11:16, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 17 February 2016 at 21:08, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Wed, Feb 17, 2016 at 08:52:36AM +0100, Marek Szyprowski wrote:
>>>> Then the only solution right now I see is to get back to v1:
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/388199.html
>>>> which at least handles correctly device registration when power domain
>>>> driver is available.
>>> ... and which has the ability to break platforms if the PM domain is
>>> not already available.
>>>
>>> What's wrong with the patch in the link above _combined_ with a patch
>>> which addresses the concern I have with that patch: build a list of
>>> the failed-to-register devices, and retry them later - maybe from a
>>> late_initcall(), or a similar mechanism?
>> This will improve the robustness of the device registration process,
>> but I wonder if it's really worth the efforts of complicating the amba
>> device registration code.
>>
>> The problem I see with such approach, is to know *when* shall we retry
>> to register the devices.
>>
>> We will rely on the PM domain driver to be probed, as to have the
>> corresponding OF genpd provider registered, else the device
>> registration will continue to fail.
>> Now, I don't think there are PM domain drivers as removable modules
>> (yet), but in such cases a late_initcall won't help much.
>>
>>> My view is the risk to existing systems is _too_ high to apply either
>>> this patch, or the patch you link to above, and I refuse to play the
>>> "lets apply it and see if we break anything" lottery with this.
>> I agree!
>>
>> Although I think with some adjustments, perhaps we can move forward with v1?
>>
>> The adjustments I think is needed:
>> Instead of propagating the error code from dev_pm_domain_attach(),
>> let's print a debug message and continue the device registration.
>> In that way, we shouldn't introduce regressions for cases where the OF
>> genpd provider hasn't yet been registered, but the issue Marek is
>> trying to solve for Exynos should be fixed.
>>
>> What do you think?
>>
> Ping. Any news on this? Would be nice if we could agree on a way forward.

I'm sorry for the lack of any update for quite a long time, but I was 
terribly
busy with some internal stuff. I will send v6 in a few minutes based on the
late_initcall idea from Russell.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH v6] drivers: amba: properly handle devices with power domains

[Marek Szyprowski]

To read pid/cid registers, the probed device need to be properly turned on.
When it is inside a power domain, the bus code should ensure that the
given power domain is enabled before trying to access device's registers.
However in some cases power domain (or clocks) might not be yet available.
Returning EPROBE_DEFER is not a solution in such case, because callers
don't handle this special error code. Instead such devices are added to the
special list and their registration is retried in late_initcall, when
hopefully all resources are available.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/amba/bus.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 10 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f0099360039e..f0b711bb0372 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -336,16 +336,7 @@ static void amba_device_release(struct device *dev)
 	kfree(d);
 }
 
-/**
- *	amba_device_add - add a previously allocated AMBA device structure
- *	@dev: AMBA device allocated by amba_device_alloc
- *	@parent: resource parent for this devices resources
- *
- *	Claim the resource, and read the device cell ID if not already
- *	initialized.  Register the AMBA device with the Linux device
- *	manager.
- */
-int amba_device_add(struct amba_device *dev, struct resource *parent)
+static int amba_device_try_add(struct amba_device *dev, struct resource *parent)
 {
 	u32 size;
 	void __iomem *tmp;
@@ -373,6 +364,12 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 		goto err_release;
 	}
 
+	ret = dev_pm_domain_attach(&dev->dev, true);
+	if (ret == -EPROBE_DEFER) {
+		iounmap(tmp);
+		goto err_release;
+	}
+
 	ret = amba_get_enable_pclk(dev);
 	if (ret == 0) {
 		u32 pid, cid;
@@ -398,6 +395,7 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 	}
 
 	iounmap(tmp);
+	dev_pm_domain_detach(&dev->dev, true);
 
 	if (ret)
 		goto err_release;
@@ -421,8 +419,68 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
  err_out:
 	return ret;
 }
+
+/*
+ * Registration of AMBA device require reading its pid and cid registers.
+ * To do this, the device must be turned on (if it is a part of power domain)
+ * and have clocks enabled. However in some cases those resources might not be
+ * yet available. Returning EPROBE_DEFER is not a solution in such case,
+ * because callers don't handle this special error code. Instead such devices
+ * are added to the special list and their registration is retried in
+ * late_initcall, when hopefully all resources are available.
+ */
+struct deferred_device {
+	struct amba_device *dev;
+	struct resource *parent;
+	struct list_head node;
+};
+
+static LIST_HEAD(deferred_devices);
+
+/**
+ *	amba_device_add - add a previously allocated AMBA device structure
+ *	@dev: AMBA device allocated by amba_device_alloc
+ *	@parent: resource parent for this devices resources
+ *
+ *	Claim the resource, and read the device cell ID if not already
+ *	initialized.  Register the AMBA device with the Linux device
+ *	manager.
+ */
+int amba_device_add(struct amba_device *dev, struct resource *parent)
+{
+	int ret = amba_device_try_add(dev, parent);
+
+	if (ret == -EPROBE_DEFER) {
+		struct deferred_device *ddev;
+
+		ddev = kmalloc(sizeof(*ddev), GFP_KERNEL);
+		ddev->dev = dev;
+		ddev->parent = parent;
+		list_add_tail(&ddev->node, &deferred_devices);
+		ret = 0;
+	}
+	return ret;
+}
 EXPORT_SYMBOL_GPL(amba_device_add);
 
+static int __init amba_deferred_device_init(void)
+{
+	struct deferred_device *ddev, *tmp;
+
+	list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
+		int ret = amba_device_try_add(ddev->dev, ddev->parent);
+
+		if (ret == -EPROBE_DEFER)
+			continue;
+
+		list_del_init(&ddev->node);
+		kfree(ddev);
+	}
+
+	return 0;
+}
+late_initcall(amba_deferred_device_init);
+
 static struct amba_device *
 amba_aphb_device_add(struct device *parent, const char *name,
 		     resource_size_t base, size_t size, int irq1, int irq2,
-- 
1.9.2

[PATCH v6] drivers: amba: properly handle devices with power domains

[Ulf Hansson]

[...]

>
> +static int __init amba_deferred_device_init(void)
> +{
> +       struct deferred_device *ddev, *tmp;
> +
> +       list_for_each_entry_safe(ddev, tmp, &deferred_devices, node) {
> +               int ret = amba_device_try_add(ddev->dev, ddev->parent);
> +
> +               if (ret == -EPROBE_DEFER)
> +                       continue;

What happens with devices that still fails to be added here? Should we
schedule a periodic work to re-try?

> +
> +               list_del_init(&ddev->node);
> +               kfree(ddev);
> +       }
> +
> +       return 0;
> +}
> +late_initcall(amba_deferred_device_init);
> +
>  static struct amba_device *
>  amba_aphb_device_add(struct device *parent, const char *name,
>                      resource_size_t base, size_t size, int irq1, int irq2,
> --
> 1.9.2
>

I assume there are other similar buses like AMBA that needs
enumeration before it can bind an appropriate driver for its device.

Perhaps that's a good reason to make this new "device add re-try"
mechanism a generic thing supported by the driver core?

Kind regards
Uffe

[PATCH v5 RESEND 5/5] ARM: amba: Properly handle devices with power domains

[Marek Szyprowski]

To read pid/cid registers, the probed device need to be properly turned on.
When it is inside a power domain, the bus code should ensure that the
given power domain is enabled before trying to access device's registers.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 643127f..570033b 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -60,6 +60,10 @@ static int amba_read_periphid(struct amba_device *dev)
 	if (!tmp)
 		return -ENOMEM;
 
+	ret = dev_pm_domain_attach(&dev->dev, true);
+	if (ret == -EPROBE_DEFER)
+		goto err_unmap;
+
 	ret = amba_get_enable_pclk(dev);
 	if (ret == 0) {
 		u32 pid, cid;
@@ -84,6 +88,8 @@ static int amba_read_periphid(struct amba_device *dev)
 			ret = -ENODEV;
 	}
 
+	dev_pm_domain_detach(&dev->dev, true);
+err_unmap:
 	iounmap(tmp);
 
 	return ret;
-- 
1.9.2