[PATCH 1/2] powerpc/5200: Fix size to request_mem_region() call

[PATCH 1/2] powerpc/5200: Fix size to request_mem_region() call

[Grant Likely]

The Bestcomm driver requests a memory region larger than the one
described in the device tree. This is due to an extra undocumented field
in the bestcomm register structure. This hasn't been a problem up to
now, but there is a patch pending to make the DT platform_bus support
code use platform_device_add() which tightens the rules and provides
extra checks for drivers to stay within the specified register regions.

Alternately, I could have removed the extra field from the structure,
but I'm not sure if it is still needed for resume to work. Better be
safe and leave it in.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anatolij Gustschin <agust@denx.de>
---
 arch/powerpc/sysdev/bestcomm/bestcomm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/bestcomm/bestcomm.c b/arch/powerpc/sysdev/bestcomm/bestcomm.c
index d913063..81c3314 100644
--- a/arch/powerpc/sysdev/bestcomm/bestcomm.c
+++ b/arch/powerpc/sysdev/bestcomm/bestcomm.c
@@ -414,7 +414,7 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
 		goto error_sramclean;
 	}
 
-	if (!request_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma),
+	if (!request_mem_region(res_bcom.start, resource_size(&res_bcom),
 				DRIVER_NAME)) {
 		printk(KERN_ERR DRIVER_NAME ": "
 			"Can't request registers region\n");
-- 
1.7.10.4

[PATCH 2/2] of: use platform_device_add

[Grant Likely]

This allows platform_device_add a chance to call insert_resource on all
of the resources from OF. At a minimum this fills in proc/iomem and
presumably makes resource tracking and conflict detection work better.
However, it has the side effect of moving all OF generated platform
devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
break userspace because userspace is not supposed to depend on the full
path (because userspace always does what it is supposed to, right?).

This may cause breakage if either:
1) any two nodes in a given device tree have overlapping & staggered
   regions (ie. 0x80..0xbf and 0xa0..0xdf; where one is not contained
   within the other). In this case one of the devices will fail to
   register and an exception will be needed in platform_device_add() to
   complain but not fail.
2) any device calls request_mem_region() on a region larger than
   specified in the device tree. In this case the device node may be
   wrong, or the driver is overreaching. In either case I'd like to know
   about any problems and fix them.

Please test. Despite the above, I'm still fairly confident that this
patch is in good shape. I'd like to put it into linux-next, but would
appreciate some bench testing from others before I do; particularly on
PowerPC machines.

v2: Remove powerpc special-case

Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/of/platform.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b80891b..3c3e3ca 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -203,6 +203,7 @@ struct platform_device *of_platform_device_create_pdata(
 					struct device *parent)
 {
 	struct platform_device *dev;
+	int rc;
 
 	if (!of_device_is_available(np))
 		return NULL;
@@ -214,16 +215,24 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
+	dev->name = dev_name(&dev->dev);
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
+	dev->dev.id = PLATFORM_DEVID_NONE;
+	/* device_add will assume that this device is on the same node as
+	 * the parent. If there is no parent defined, set the node
+	 * explicitly */
+	if (!parent)
+		set_dev_node(&dev->dev, of_node_to_nid(np));
 
 	/* We do not fill the DMA ops for platform devices by default.
 	 * This is currently the responsibility of the platform code
 	 * to do such, possibly using a device notifier
 	 */
 
-	if (of_device_add(dev) != 0) {
+	rc = platform_device_add(dev);
+	if (rc) {
+		dev_err(&dev->dev, "device registration failed\n");
 		platform_device_put(dev);
 		return NULL;
 	}
-- 
1.7.10.4

Re: [PATCH 2/2] of: use platform_device_add

[Shawn Guo]

On Fri, Jan 18, 2013 at 01:40:00AM +0000, Grant Likely wrote:
> This allows platform_device_add a chance to call insert_resource on all
> of the resources from OF. At a minimum this fills in proc/iomem and
> presumably makes resource tracking and conflict detection work better.
> However, it has the side effect of moving all OF generated platform
> devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
> break userspace because userspace is not supposed to depend on the full
> path (because userspace always does what it is supposed to, right?).
> 
> This may cause breakage if either:
> 1) any two nodes in a given device tree have overlapping & staggered
>    regions (ie. 0x80..0xbf and 0xa0..0xdf; where one is not contained
>    within the other). In this case one of the devices will fail to
>    register and an exception will be needed in platform_device_add() to
>    complain but not fail.

Grant,

The patch introduce a regression on imx6q boot.  The IOMUXC block on
imx6q is special.  It acts not only a pin controller but also a system
controller with a bunch of system level registers in there.  That's why
we currently have the following two nodes in imx6q device tree with the
same start "reg" address, which work with drivers/mfd/syscon.c and
drivers/pinctrl/pinctrl-imx6q.c respectively.

	gpr: iomuxc-gpr@020e0000 {
		compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
		reg = <0x020e0000 0x38>;
	};

	iomuxc: iomuxc@020e0000 {
		compatible = "fsl,imx6q-iomuxc";
		reg = <0x020e0000 0x4000>;
	};

With the patch in place, pinctrl-imx6q fails to register like below.

syscon 20e0000.iomuxc: syscon regmap start 0x20e0000 end 0x20e3fff registered
imx6q-pinctrl 20e0000.iomuxc: can't request region for resource [mem 0x020e0000-0x020e3fff]
imx6q-pinctrl: probe of 20e0000.iomuxc failed with error -16

Shawn

> 2) any device calls request_mem_region() on a region larger than
>    specified in the device tree. In this case the device node may be
>    wrong, or the driver is overreaching. In either case I'd like to know
>    about any problems and fix them.

Re: [PATCH 2/2] of: use platform_device_add

[Shawn Guo]

On Sun, Feb 17, 2013 at 11:03:35AM +0800, Shawn Guo wrote:
> On Fri, Jan 18, 2013 at 01:40:00AM +0000, Grant Likely wrote:
> > This allows platform_device_add a chance to call insert_resource on all
> > of the resources from OF. At a minimum this fills in proc/iomem and
> > presumably makes resource tracking and conflict detection work better.
> > However, it has the side effect of moving all OF generated platform
> > devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
> > break userspace because userspace is not supposed to depend on the full
> > path (because userspace always does what it is supposed to, right?).
> > 
> > This may cause breakage if either:
> > 1) any two nodes in a given device tree have overlapping & staggered
> >    regions (ie. 0x80..0xbf and 0xa0..0xdf; where one is not contained
> >    within the other). In this case one of the devices will fail to
> >    register and an exception will be needed in platform_device_add() to
> >    complain but not fail.
> 
> Grant,
> 
> The patch introduce a regression on imx6q boot.

It also breaks all of_amba_device users.

of_amba_device_create() --> amba_device_add() --> request_resource()
and fails.

Shawn

Re: [PATCH 2/2] of: use platform_device_add

[Russell King - ARM Linux]

On Sun, Feb 17, 2013 at 03:43:20PM +0800, Shawn Guo wrote:
> It also breaks all of_amba_device users.
> 
> of_amba_device_create() --> amba_device_add() --> request_resource()
> and fails.

Presumably that's because we no longer know what the parent resource
is supposed to be?

Re: [PATCH 2/2] of: use platform_device_add

[Grant Likely]

On Sun, Feb 17, 2013 at 10:19 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Feb 17, 2013 at 03:43:20PM +0800, Shawn Guo wrote:
> > The patch introduce a regression on imx6q boot.  The IOMUXC block on
> > imx6q is special.  It acts not only a pin controller but also a system
> > controller with a bunch of system level registers in there.  That's why
> > we currently have the following two nodes in imx6q device tree with the
> > same start "reg" address, which work with drivers/mfd/syscon.c and
> > drivers/pinctrl/pinctrl-imx6q.c respectively.
> >
> >         gpr: iomuxc-gpr@020e0000 {
> >                 compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> >                 reg = <0x020e0000 0x38>;
> >         };
> >
> >         iomuxc: iomuxc@020e0000 {
> >                 compatible = "fsl,imx6q-iomuxc";
> >                 reg = <0x020e0000 0x4000>;
> >         };
> >
> > With the patch in place, pinctrl-imx6q fails to register like below.
> >
> > syscon 20e0000.iomuxc: syscon regmap start 0x20e0000 end 0x20e3fff registered
> > imx6q-pinctrl 20e0000.iomuxc: can't request region for resource [mem 0x020e0000-0x020e3fff]
> > imx6q-pinctrl: probe of 20e0000.iomuxc failed with error -16

Strictly you're not supposed to do that with the device tree. There
shouldn't be two nodes using the same overlapping memory region unless
they are parent/child. That rule has been around for a long time, but
the core never checked for it. What /should/ happen is the two drivers
should be cooperating for the register region and only one device
driver probe sets up both behaviours.

However, neither is it okay to just break the existing device trees.
Best thing to do I think is to deprecate one of the nodes.

>> It also breaks all of_amba_device users.
>>
>> of_amba_device_create() --> amba_device_add() --> request_resource()
>> and fails.
>
> Presumably that's because we no longer know what the parent resource
> is supposed to be?

Hmmm, it looks that way, yes. Currently the OF code is using
iomem_resource as the parent for all amba_device_add() calls
(driver/of/platform.c), but if the parent node gets registered as a
platform device and it has the resources then the parenthood chain
doesn't match up. It isn't immediately obvious to me how to fix this.
I'm going to drop the patch from my tree. I could use some help
figuring out what the correct behaviour really should be here.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] of: use platform_device_add

[Jason Gunthorpe]

On Sun, Feb 17, 2013 at 10:49:08AM +0000, Grant Likely wrote:
> > > The patch introduce a regression on imx6q boot.  The IOMUXC block on
> > > imx6q is special.  It acts not only a pin controller but also a system
> > > controller with a bunch of system level registers in there.  That's why
> > > we currently have the following two nodes in imx6q device tree with the
> > > same start "reg" address, which work with drivers/mfd/syscon.c and
> > > drivers/pinctrl/pinctrl-imx6q.c respectively.
> > >
> > >         gpr: iomuxc-gpr@020e0000 {
> > >                 compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > >                 reg = <0x020e0000 0x38>;
> > >         };
> > >
> > >         iomuxc: iomuxc@020e0000 {
> > >                 compatible = "fsl,imx6q-iomuxc";
> > >                 reg = <0x020e0000 0x4000>;
> > >         };
> > >
> > > With the patch in place, pinctrl-imx6q fails to register like below.
> > >
> > > syscon 20e0000.iomuxc: syscon regmap start 0x20e0000 end 0x20e3fff registered
> > > imx6q-pinctrl 20e0000.iomuxc: can't request region for resource [mem 0x020e0000-0x020e3fff]
> > > imx6q-pinctrl: probe of 20e0000.iomuxc failed with error -16
> 
> Strictly you're not supposed to do that with the device tree. There
> shouldn't be two nodes using the same overlapping memory region unless
> they are parent/child. That rule has been around for a long time, but
> the core never checked for it. What /should/ happen is the two drivers
> should be cooperating for the register region and only one device
> driver probe sets up both behaviours.

This case was something we both discussed when this patch was first
brough up, and both our tests seemed like it was OK.. What is going on
here that these non-staggered regions are failing? It looks like the
platform devices registered but the devm_request_and_iormap failed?

> >> It also breaks all of_amba_device users.
> >>
> >> of_amba_device_create() --> amba_device_add() --> request_resource()
> >> and fails.
> >
> > Presumably that's because we no longer know what the parent resource
> > is supposed to be?
> 
> Hmmm, it looks that way, yes. Currently the OF code is using
> iomem_resource as the parent for all amba_device_add() calls
> (driver/of/platform.c), but if the parent node gets registered as a
> platform device and it has the resources then the parenthood chain
> doesn't match up. It isn't immediately obvious to me how to fix this.
> I'm going to drop the patch from my tree. I could use some help
> figuring out what the correct behaviour really should be here.

I looked for a bit and it wasn't obvious to me where the colliding
request_resource was coming from. The DTs for amba busses seem to all
be placed under the root node, or within a simple bus, so there is not
parent platform device and the use of iomem_resource should still be OK?

Jason

Re: [PATCH 2/2] of: use platform_device_add

[Fabio Estevam]

On Sun, Feb 17, 2013 at 4:43 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Sun, Feb 17, 2013 at 11:03:35AM +0800, Shawn Guo wrote:
>> On Fri, Jan 18, 2013 at 01:40:00AM +0000, Grant Likely wrote:
>> > This allows platform_device_add a chance to call insert_resource on all
>> > of the resources from OF. At a minimum this fills in proc/iomem and
>> > presumably makes resource tracking and conflict detection work better.
>> > However, it has the side effect of moving all OF generated platform
>> > devices from /sys/devices to /sys/devices/platform/. It /shouldn't/
>> > break userspace because userspace is not supposed to depend on the full
>> > path (because userspace always does what it is supposed to, right?).
>> >
>> > This may cause breakage if either:
>> > 1) any two nodes in a given device tree have overlapping & staggered
>> >    regions (ie. 0x80..0xbf and 0xa0..0xdf; where one is not contained
>> >    within the other). In this case one of the devices will fail to
>> >    register and an exception will be needed in platform_device_add() to
>> >    complain but not fail.
>>
>> Grant,
>>
>> The patch introduce a regression on imx6q boot.
>
> It also breaks all of_amba_device users.
>
> of_amba_device_create() --> amba_device_add() --> request_resource()
> and fails.

Yes, correct: amba-pl011 does not register anymore after this patch,
which causes the serial console to be not functional.

Re: [PATCH 1/2] powerpc/5200: Fix size to request_mem_region() call

[Anatolij Gustschin]

Hi Grant,

On Fri, 18 Jan 2013 01:39:59 +0000
Grant Likely <grant.likely@secretlab.ca> wrote:

> The Bestcomm driver requests a memory region larger than the one
> described in the device tree. This is due to an extra undocumented field
> in the bestcomm register structure. This hasn't been a problem up to
> now, but there is a patch pending to make the DT platform_bus support
> code use platform_device_add() which tightens the rules and provides
> extra checks for drivers to stay within the specified register regions.
> 
> Alternately, I could have removed the extra field from the structure,
> but I'm not sure if it is still needed for resume to work. Better be
> safe and leave it in.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
>  arch/powerpc/sysdev/bestcomm/bestcomm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

There is a patch moving this driver to drivers/dma,

http://patchwork.ozlabs.org/patch/191153/

I've applied it to my 5xxx next branch.

> diff --git a/arch/powerpc/sysdev/bestcomm/bestcomm.c b/arch/powerpc/sysdev/bestcomm/bestcomm.c
> index d913063..81c3314 100644
> --- a/arch/powerpc/sysdev/bestcomm/bestcomm.c
> +++ b/arch/powerpc/sysdev/bestcomm/bestcomm.c
> @@ -414,7 +414,7 @@ static int mpc52xx_bcom_probe(struct platform_device *op)
>  		goto error_sramclean;
>  	}
>  
> -	if (!request_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma),
> +	if (!request_mem_region(res_bcom.start, resource_size(&res_bcom),
>  				DRIVER_NAME)) {
>  		printk(KERN_ERR DRIVER_NAME ": "
>  			"Can't request registers region\n");

similar change is needed for release_mem_region() in error path
and in driver's remove() function.

Thanks,

Anatolij