From: Grant Likely <hidden> Date: 2013-01-18 01:40:12
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 <redacted>
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(-)
From: Grant Likely <hidden> Date: 2013-01-18 01:40:14
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 <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Rob Herring <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Grant Likely <redacted>
---
drivers/of/platform.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -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+*theparent.Ifthereisnoparentdefined,setthenode+*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.*Thisiscurrentlytheresponsibilityoftheplatformcode*todosuch,possiblyusingadevicenotifier*/-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);returnNULL;}
Hi Grant,
On Fri, 18 Jan 2013 01:39:59 +0000
Grant Likely [off-list ref] 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 <redacted>
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(-)
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.
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:
quoted
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
From: Grant Likely <hidden> Date: 2013-02-17 10:49:30
On Sun, Feb 17, 2013 at 10:19 AM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Feb 17, 2013 at 03:43:20PM +0800, Shawn Guo wrote:
quoted
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.
quoted
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.
On Sun, Feb 17, 2013 at 4:43 AM, Shawn Guo [off-list ref] wrote:
On Sun, Feb 17, 2013 at 11:03:35AM +0800, Shawn Guo wrote:
quoted
On Fri, Jan 18, 2013 at 01:40:00AM +0000, Grant Likely wrote:
quoted
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.
From: Jason Gunthorpe <hidden> Date: 2013-02-19 19:03:54
On Sun, Feb 17, 2013 at 10:49:08AM +0000, Grant Likely wrote:
quoted
quoted
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?
quoted
quoted
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