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

Groups > linux.kernel > #1331053 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to  amba_match() Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-10 11:50 +0100
    Re: [PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to  amba_match() Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-15 19:00 +0100
      Re: [PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to  amba_match() Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-16 17:40 +0100
      Re: [PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to  amba_match() Marek Szyprowski <m.szyprowski@samsung.com> - 2016-02-17 09:00 +0100
        Re: [PATCH v5 RESEND 4/5] ARM: amba: Move reading of periphid to  amba_match() Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-17 21:20 +0100

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

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

[toc] | [next] | [standalone]

#1334669

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.

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

#1335615

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.

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

#1336118

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

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

#1336729

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web