(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
From: Dan Williams <[email protected]>
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 <[email protected]>
Signed-off-by: Marek Szyprowski <[email protected]>
---
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
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 <[email protected]>
---
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
From: Tomeu Vizoso <[email protected]>
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 <[email protected]>
[changed if-else code structure, adjusted documentation to match the code,
extended comments]
Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
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
From: Tomeu Vizoso <[email protected]>
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 <[email protected]>
[minor code adjustments, removed forward declaration, added missing
comment]
Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
---
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
On Wed, Dec 23, 2015 at 11:59:25AM +0100, Marek Szyprowski 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.
This actually can't return a negative number - only valid devid bits
are 0 to 9 inclusive, and this isn't going to ever change. So the
patch isn't strictly necessary, but is good from the point of view
of ensuring consistency. Hence:
> Signed-off-by: Marek Szyprowski <[email protected]>
Acked-by: Russell King <[email protected]>
> ---
> 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
>
--
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.