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

2013-02-19 Thread Jason Gunthorpe
On Sun, Feb 17, 2013 at 10:49:08AM +, 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@020e {
   compatible = fsl,imx6q-iomuxc-gpr, syscon;
   reg = 0x020e 0x38;
   };
  
   iomuxc: iomuxc@020e {
   compatible = fsl,imx6q-iomuxc;
   reg = 0x020e 0x4000;
   };
  
   With the patch in place, pinctrl-imx6q fails to register like below.
  
   syscon 20e.iomuxc: syscon regmap start 0x20e end 0x20e3fff 
   registered
   imx6q-pinctrl 20e.iomuxc: can't request region for resource [mem 
   0x020e-0x020e3fff]
   imx6q-pinctrl: probe of 20e.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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


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

2013-02-17 Thread 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?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


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

2013-02-17 Thread Grant Likely
On Sun, Feb 17, 2013 at 10:19 AM, Russell King - ARM Linux
li...@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@020e {
  compatible = fsl,imx6q-iomuxc-gpr, syscon;
  reg = 0x020e 0x38;
  };
 
  iomuxc: iomuxc@020e {
  compatible = fsl,imx6q-iomuxc;
  reg = 0x020e 0x4000;
  };
 
  With the patch in place, pinctrl-imx6q fails to register like below.
 
  syscon 20e.iomuxc: syscon regmap start 0x20e end 0x20e3fff 
  registered
  imx6q-pinctrl 20e.iomuxc: can't request region for resource [mem 
  0x020e-0x020e3fff]
  imx6q-pinctrl: probe of 20e.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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


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

2013-02-17 Thread Fabio Estevam
On Sun, Feb 17, 2013 at 4:43 AM, Shawn Guo shawn@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 +, 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.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


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

2013-02-16 Thread Shawn Guo
On Fri, Jan 18, 2013 at 01:40:00AM +, 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@020e {
compatible = fsl,imx6q-iomuxc-gpr, syscon;
reg = 0x020e 0x38;
};

iomuxc: iomuxc@020e {
compatible = fsl,imx6q-iomuxc;
reg = 0x020e 0x4000;
};

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

syscon 20e.iomuxc: syscon regmap start 0x20e end 0x20e3fff registered
imx6q-pinctrl 20e.iomuxc: can't request region for resource [mem 
0x020e-0x020e3fff]
imx6q-pinctrl: probe of 20e.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.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


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

2013-02-16 Thread Shawn Guo
On Sun, Feb 17, 2013 at 11:03:35AM +0800, Shawn Guo wrote:
 On Fri, Jan 18, 2013 at 01:40:00AM +, 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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


[PATCH 2/2] of: use platform_device_add

2013-01-17 Thread 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 jguntho...@obsidianresearch.com
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Grant Likely grant.lik...@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 = 0xUL;
 #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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev