[PATCH v2 0/4] Exynos4210: fix power domain for MDMA1 device

[PATCH v2 0/4] Exynos4210: fix power domain for MDMA1 device

[Marek Szyprowski]

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.

The main change from v1 is reusing patches, which move PID/CIR reading
from amba_device_add() to amba_match() and adding power domain support
there. This way -EPROBE_DEFER error code can be handled properly.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Changelog:
v2:
- 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:

Marek Szyprowski (2):
  ARM: dts: exynos4210: MDMA1 device belongs to LCD0 power domain
  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()

 arch/arm/boot/dts/exynos4210.dtsi |  4 ++
 drivers/amba/bus.c                | 94 ++++++++++++++++++++++-----------------
 drivers/base/dd.c                 | 24 +++++++++-
 include/linux/device.h            |  2 +-
 4 files changed, 79 insertions(+), 45 deletions(-)

-- 
1.9.2

[PATCH v2 1/4] ARM: dts: exynos4210: MDMA1 device belongs to LCD0 power domain

[Marek Szyprowski]

On Exynos 4210 MDMA1 device belongs to LCD0 power domain, so add proper
power-domains property. On Exynos 4x12, it belongs to TOP power domain,
which is always enabled, thus require no assignment in exynos4x12.dtsi.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index b7474cf2..aac0f17 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -271,6 +271,10 @@
 		     <0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
 };
 
+&mdma1 {
+	power-domains = <&pd_lcd0>;
+};
+
 &pmu_system_controller {
 	clock-names = "clkout0", "clkout1", "clkout2", "clkout3",
 			"clkout4", "clkout8", "clkout9";
-- 
1.9.2

[PATCH v2 1/4] ARM: dts: exynos4210: MDMA1 device belongs to LCD0 power domain

[Krzysztof Kozlowski]

On 26.11.2015 21:49, Marek Szyprowski wrote:
> On Exynos 4210 MDMA1 device belongs to LCD0 power domain, so add proper
> power-domains property. On Exynos 4x12, it belongs to TOP power domain,
> which is always enabled, thus require no assignment in exynos4x12.dtsi.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4210.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)

Makes sense. I suppose the rest of the patchset does not depends on it
directly so it can go through samsung-soc tree (otherwise please let us
know).

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

[PATCH v2 2/4] driver core: handle -EPROBE_DEFER from bus_type.match()

[Marek Szyprowski]

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

Lets 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>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/dd.c      | 24 ++++++++++++++++++++++--
 include/linux/device.h |  2 +-
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a641cf3..a20c119 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -490,6 +490,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
@@ -500,8 +501,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)
 		return 0;
+	else if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			dev_dbg(dev, "Device match requests probe deferral\n");
+			driver_deferred_probe_add(dev);
+		} else
+			dev_warn(dev, "Bus failed to match device: %d", ret);
+		return ret;
+	}
 
 	async_allowed = driver_allows_async_probing(drv);
 
@@ -621,6 +631,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
@@ -632,8 +643,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)
+		return 0;
+	else if (ret < 0) {
+		if (ret == -EPROBE_DEFER) {
+			dev_dbg(dev, "Device match requests probe deferral\n");
+			driver_deferred_probe_add(dev);
+		} else
+			dev_warn(dev, "Bus failed to match device: %d", ret);
 		return 0;
+	}
 
 	if (dev->parent)	/* Needed for USB */
 		device_lock(dev->parent);
diff --git a/include/linux/device.h b/include/linux/device.h
index b8f411b..d4e7d1f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -70,7 +70,7 @@ 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
+ *		is added for this bus. It should return a positive value if the
  *		given device can be handled by the given driver.
  * @uevent:	Called when a device is added, removed, or a few other things
  *		that generate uevents to add the environment variables.
-- 
1.9.2

[PATCH v2 2/4] driver core: handle -EPROBE_DEFER from bus_type.match()

[Ulf Hansson]

On 26 November 2015 at 13:49, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> Lets implementations of the match() callback in struct bus_type to

/s/Lets/Allow

> 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>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/base/dd.c      | 24 ++++++++++++++++++++++--
>  include/linux/device.h |  2 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a641cf3..a20c119 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -490,6 +490,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
> @@ -500,8 +501,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)
>                 return 0;
> +       else if (ret < 0) {

Depending on what happens with the added dev_warn() below, perhaps a
switch statement can make it a bit clearer, instead of these messy if
clauses?

> +               if (ret == -EPROBE_DEFER) {
> +                       dev_dbg(dev, "Device match requests probe deferral\n");
> +                       driver_deferred_probe_add(dev);
> +               } else
> +                       dev_warn(dev, "Bus failed to match device: %d", ret);

Greg commented on this before, as it may introduce some noise [1].

I started browsing various bus?s implementation of the ->match()
callback. A quick search tells me that most implementations are
following the Documentation/driver-model/porting.txt, which means
returning 0 or 1. Actually I couldn't find anyone returning any other
value, though it was a quick search.

On the other hand, include/linux/device.h states a "non-zero" value is
allowed to be return, so there's a tiny conflict between the code and
the documentation. I guess we should fix that!?

No matter what, I realize that it could be useful to print a message
when receiving a negative error code, maybe dev_dbg() could be
sufficient?

> +               return ret;
> +       }
>
>         async_allowed = driver_allows_async_probing(drv);
>
> @@ -621,6 +631,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
> @@ -632,8 +643,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)
> +               return 0;
> +       else if (ret < 0) {
> +               if (ret == -EPROBE_DEFER) {
> +                       dev_dbg(dev, "Device match requests probe deferral\n");
> +                       driver_deferred_probe_add(dev);
> +               } else
> +                       dev_warn(dev, "Bus failed to match device: %d", ret);
>                 return 0;
> +       }
>
>         if (dev->parent)        /* Needed for USB */
>                 device_lock(dev->parent);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b8f411b..d4e7d1f 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -70,7 +70,7 @@ 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
> + *             is added for this bus. It should return a positive value if the
>   *             given device can be handled by the given driver.
>   * @uevent:    Called when a device is added, removed, or a few other things
>   *             that generate uevents to add the environment variables.
> --
> 1.9.2
>

Kind regards
Uffe

[1]
https://lkml.org/lkml/2015/10/17/24

[PATCH v2 2/4] driver core: handle -EPROBE_DEFER from bus_type.match()

[Marek Szyprowski]

Hello,

On 2015-11-30 14:36, Ulf Hansson wrote:
> On 26 November 2015 at 13:49, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> Lets implementations of the match() callback in struct bus_type to
> /s/Lets/Allow
>
>> 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>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/base/dd.c      | 24 ++++++++++++++++++++++--
>>   include/linux/device.h |  2 +-
>>   2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index a641cf3..a20c119 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -490,6 +490,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
>> @@ -500,8 +501,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)
>>                  return 0;
>> +       else if (ret < 0) {
> Depending on what happens with the added dev_warn() below, perhaps a
> switch statement can make it a bit clearer, instead of these messy if
> clauses?

Frankly, I have no idea how to convert this to switch statement and make
the code easier to read. Please note that we have the following 4 cases:

ret > 0: positive match
ret == 0: negative match
ret == -EPROBE_DEFER: deferred probe
ret < 0: other, unknown error

Another way to encode this logic is the following code:

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);
         return ret;
} else if (ret < 0) {
         dev_dbg(dev, "Bus failed to match device: %d", ret);
         return ret;
} /* ret > 0 means positive match */


>> +               if (ret == -EPROBE_DEFER) {
>> +                       dev_dbg(dev, "Device match requests probe deferral\n");
>> +                       driver_deferred_probe_add(dev);
>> +               } else
>> +                       dev_warn(dev, "Bus failed to match device: %d", ret);
> Greg commented on this before, as it may introduce some noise [1].
>
> I started browsing various bus?s implementation of the ->match()
> callback. A quick search tells me that most implementations are
> following the Documentation/driver-model/porting.txt, which means
> returning 0 or 1. Actually I couldn't find anyone returning any other
> value, though it was a quick search.
>
> On the other hand, include/linux/device.h states a "non-zero" value is
> allowed to be return, so there's a tiny conflict between the code and
> the documentation. I guess we should fix that!?

Okay, I will update the documentation as well.

> No matter what, I realize that it could be useful to print a message
> when receiving a negative error code, maybe dev_dbg() could be
> sufficient?
>
>> +               return ret;
>> +       }
>>
>>          async_allowed = driver_allows_async_probing(drv);
>>
>> @@ -621,6 +631,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
>> @@ -632,8 +643,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)
>> +               return 0;
>> +       else if (ret < 0) {
>> +               if (ret == -EPROBE_DEFER) {
>> +                       dev_dbg(dev, "Device match requests probe deferral\n");
>> +                       driver_deferred_probe_add(dev);
>> +               } else
>> +                       dev_warn(dev, "Bus failed to match device: %d", ret);
>>                  return 0;
>> +       }
>>
>>          if (dev->parent)        /* Needed for USB */
>>                  device_lock(dev->parent);
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index b8f411b..d4e7d1f 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -70,7 +70,7 @@ 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
>> + *             is added for this bus. It should return a positive value if the
>>    *             given device can be handled by the given driver.
>>    * @uevent:    Called when a device is added, removed, or a few other things
>>    *             that generate uevents to add the environment variables.
>> --
>> 1.9.2

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

[PATCH v2 2/4] driver core: handle -EPROBE_DEFER from bus_type.match()

[Ulf Hansson]

[...]

>>> @@ -500,8 +501,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)
>>>                  return 0;
>>> +       else if (ret < 0) {
>>
>> Depending on what happens with the added dev_warn() below, perhaps a
>> switch statement can make it a bit clearer, instead of these messy if
>> clauses?
>
>
> Frankly, I have no idea how to convert this to switch statement and make

You are right!

I was thinking that "1" was the only supported positive value,
according to the documentation.

> the code easier to read. Please note that we have the following 4 cases:
>
> ret > 0: positive match
> ret == 0: negative match
> ret == -EPROBE_DEFER: deferred probe
> ret < 0: other, unknown error
>
> Another way to encode this logic is the following code:
>
> 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);
>         return ret;
> } else if (ret < 0) {
>         dev_dbg(dev, "Bus failed to match device: %d", ret);
>         return ret;
> } /* ret > 0 means positive match */

Okay! To me this looks better!

[...]

Kind regards
Uffe

[PATCH v2 3/4] 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>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/amba/bus.c | 88 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f009936..72ebf9b 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -24,6 +24,8 @@
 
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
+static int read_periphid(struct amba_device *d, unsigned int *periphid);
+
 static const struct amba_id *
 amba_lookup(const struct amba_id *table, struct amba_device *dev)
 {
@@ -43,11 +45,22 @@ 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);
 
+	if (!pcdev->periphid) {
+		ret = read_periphid(pcdev, &pcdev->periphid);
+		if (ret) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to read periphid: %d",
+					ret);
+			return ret;
+		}
+	}
+
 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
 
@@ -336,44 +349,22 @@ 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 read_periphid(struct amba_device *d, unsigned int *periphid)
 {
 	u32 size;
 	void __iomem *tmp;
-	int i, ret;
-
-	WARN_ON(dev->irq[0] == (unsigned int)-1);
-	WARN_ON(dev->irq[1] == (unsigned int)-1);
-
-	ret = request_resource(parent, &dev->res);
-	if (ret)
-		goto err_out;
-
-	/* Hard-coded primecell ID instead of plug-n-play */
-	if (dev->periphid != 0)
-		goto skip_probe;
+	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) {
-		ret = -ENOMEM;
-		goto err_release;
-	}
+	size = resource_size(&d->res);
+	tmp = ioremap(d->res.start, size);
+	if (!tmp)
+		return -ENOMEM;
 
-	ret = amba_get_enable_pclk(dev);
+	ret = amba_get_enable_pclk(d);
 	if (ret == 0) {
 		u32 pid, cid;
 
@@ -388,37 +379,50 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
 			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
 				(i * 8);
 
-		amba_put_disable_pclk(dev);
+		amba_put_disable_pclk(d);
 
 		if (cid == AMBA_CID || cid == CORESIGHT_CID)
-			dev->periphid = pid;
+			*periphid = pid;
 
-		if (!dev->periphid)
+		if (!*periphid)
 			ret = -ENODEV;
 	}
 
 	iounmap(tmp);
 
+	return ret;
+}
+
+/**
+ *	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 register the AMBA device with the Linux device
+ *	manager.
+ */
+int amba_device_add(struct amba_device *dev, struct resource *parent)
+{
+	int ret;
+
+	WARN_ON(dev->irq[0] == (unsigned int)-1);
+	WARN_ON(dev->irq[1] == (unsigned int)-1);
+
+	ret = request_resource(parent, &dev->res);
 	if (ret)
-		goto err_release;
+		return ret;
 
- skip_probe:
 	ret = device_add(&dev->dev);
 	if (ret)
-		goto err_release;
+		return ret;
 
 	if (dev->irq[0])
 		ret = device_create_file(&dev->dev, &dev_attr_irq0);
 	if (ret == 0 && dev->irq[1])
 		ret = device_create_file(&dev->dev, &dev_attr_irq1);
-	if (ret == 0)
-		return ret;
-
-	device_unregister(&dev->dev);
+	if (ret)
+		device_unregister(&dev->dev);
 
- err_release:
-	release_resource(&dev->res);
- err_out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(amba_device_add);
-- 
1.9.2

[PATCH v2 3/4] ARM: amba: Move reading of periphid to amba_match()

[Ulf Hansson]

On 26 November 2015 at 13:49, Marek Szyprowski <m.szyprowski@samsung.com> 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.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/amba/bus.c | 88 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 46 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f009936..72ebf9b 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -24,6 +24,8 @@
>
>  #define to_amba_driver(d)      container_of(d, struct amba_driver, drv)
>
> +static int read_periphid(struct amba_device *d, unsigned int *periphid);

I would change this to:
static int amba_read_periphid(struct amba_device *dev);

That will actually also make the patch smaller.

> +
>  static const struct amba_id *
>  amba_lookup(const struct amba_id *table, struct amba_device *dev)
>  {
> @@ -43,11 +45,22 @@ 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);
>
> +       if (!pcdev->periphid) {
> +               ret = read_periphid(pcdev, &pcdev->periphid);
> +               if (ret) {
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(dev, "Failed to read periphid: %d",
> +                                       ret);
> +                       return ret;
> +               }
> +       }
> +
>         return amba_lookup(pcdrv->id_table, pcdev) != NULL;
>  }
>
> @@ -336,44 +349,22 @@ 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 read_periphid(struct amba_device *d, unsigned int *periphid)
>  {
>         u32 size;
>         void __iomem *tmp;
> -       int i, ret;
> -
> -       WARN_ON(dev->irq[0] == (unsigned int)-1);
> -       WARN_ON(dev->irq[1] == (unsigned int)-1);
> -
> -       ret = request_resource(parent, &dev->res);
> -       if (ret)
> -               goto err_out;
> -
> -       /* Hard-coded primecell ID instead of plug-n-play */

This line now belongs in amba_match(), please move it there instead of
just deleting it.

> -       if (dev->periphid != 0)
> -               goto skip_probe;
> +       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) {
> -               ret = -ENOMEM;
> -               goto err_release;
> -       }
> +       size = resource_size(&d->res);
> +       tmp = ioremap(d->res.start, size);
> +       if (!tmp)
> +               return -ENOMEM;
>
> -       ret = amba_get_enable_pclk(dev);
> +       ret = amba_get_enable_pclk(d);
>         if (ret == 0) {
>                 u32 pid, cid;
>
> @@ -388,37 +379,50 @@ int amba_device_add(struct amba_device *dev, struct resource *parent)
>                         cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
>                                 (i * 8);
>
> -               amba_put_disable_pclk(dev);
> +               amba_put_disable_pclk(d);
>
>                 if (cid == AMBA_CID || cid == CORESIGHT_CID)
> -                       dev->periphid = pid;
> +                       *periphid = pid;
>
> -               if (!dev->periphid)
> +               if (!*periphid)
>                         ret = -ENODEV;
>         }
>
>         iounmap(tmp);
>
> +       return ret;
> +}
> +
> +/**
> + *     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 register the AMBA device with the Linux device
> + *     manager.
> + */
> +int amba_device_add(struct amba_device *dev, struct resource *parent)
> +{
> +       int ret;
> +
> +       WARN_ON(dev->irq[0] == (unsigned int)-1);
> +       WARN_ON(dev->irq[1] == (unsigned int)-1);
> +
> +       ret = request_resource(parent, &dev->res);
>         if (ret)
> -               goto err_release;
> +               return ret;
>
> - skip_probe:
>         ret = device_add(&dev->dev);
>         if (ret)
> -               goto err_release;
> +               return ret;
>
>         if (dev->irq[0])
>                 ret = device_create_file(&dev->dev, &dev_attr_irq0);
>         if (ret == 0 && dev->irq[1])
>                 ret = device_create_file(&dev->dev, &dev_attr_irq1);
> -       if (ret == 0)
> -               return ret;
> -
> -       device_unregister(&dev->dev);
> +       if (ret)
> +               device_unregister(&dev->dev);
>
> - err_release:
> -       release_resource(&dev->res);
> - err_out:
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(amba_device_add);
> --
> 1.9.2
>

Kind regards
Uffe

[PATCH v2 4/4] 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>
---
 drivers/amba/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 72ebf9b..4fc1976 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -364,6 +364,10 @@ static int read_periphid(struct amba_device *d, unsigned int *periphid)
 	if (!tmp)
 		return -ENOMEM;
 
+	ret = dev_pm_domain_attach(&d->dev, true);
+	if (ret == -EPROBE_DEFER)
+		goto err_unmap;
+
 	ret = amba_get_enable_pclk(d);
 	if (ret == 0) {
 		u32 pid, cid;
@@ -388,6 +392,8 @@ static int read_periphid(struct amba_device *d, unsigned int *periphid)
 			ret = -ENODEV;
 	}
 
+	dev_pm_domain_detach(&d->dev, true);
+err_unmap:
 	iounmap(tmp);
 
 	return ret;
-- 
1.9.2