| Message ID | 1358473200-17886-2-git-send-email-grant.likely@secretlab.ca (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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: > > 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
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?
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.
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.
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
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; }
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(-)