[PATCH] of: Have of_device_add call platform_device_add rather than device_add

[PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Jason Gunthorpe]

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.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/of/device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Tested on PPC32 and ARM32 embedded kernels.

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 4c74e4f..a5b67dc 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -62,7 +62,7 @@ int of_device_add(struct platform_device *ofdev)
 	if (!ofdev->dev.parent)
 		set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
 
-	return device_add(&ofdev->dev);
+	return platform_device_add(ofdev);
 }
 
 int of_device_register(struct platform_device *pdev)
-- 
1.7.4.1

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Wed, 21 Nov 2012 00:24:48 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/of/device.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Tested on PPC32 and ARM32 embedded kernels.
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 4c74e4f..a5b67dc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -62,7 +62,7 @@ int of_device_add(struct platform_device *ofdev)
>  	if (!ofdev->dev.parent)
>  		set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
>  
> -	return device_add(&ofdev->dev);
> +	return platform_device_add(ofdev);
>  }
>  
>  int of_device_register(struct platform_device *pdev)

This has the side effect of moving all devices at the root of the tree
from /sys/devices/ to /sys/devices/platform. It also has the possibility
of breaking if any devices get registered with overlapping regions. I
think there are some powerpc 5200 boards that do this, and I'm not sure
about the larger Power boxen.

I've got a more nuanced version of this patch that I'm trying to get
published today for review. I'll add you to the cc list.

g.

> -- 
> 1.7.4.1
> 

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

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Wed, Nov 21, 2012 at 3:51 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, 21 Nov 2012 00:24:48 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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.
>>
>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> ---
>>  drivers/of/device.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> Tested on PPC32 and ARM32 embedded kernels.
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 4c74e4f..a5b67dc 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -62,7 +62,7 @@ int of_device_add(struct platform_device *ofdev)
>>       if (!ofdev->dev.parent)
>>               set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
>>
>> -     return device_add(&ofdev->dev);
>> +     return platform_device_add(ofdev);
>>  }
>>
>>  int of_device_register(struct platform_device *pdev)
>
> This has the side effect of moving all devices at the root of the tree
> from /sys/devices/ to /sys/devices/platform. It also has the possibility
> of breaking if any devices get registered with overlapping regions. I
> think there are some powerpc 5200 boards that do this, and I'm not sure
> about the larger Power boxen.
>
> I've got a more nuanced version of this patch that I'm trying to get
> published today for review. I'll add you to the cc list.

However, while on this topic;

Ben. Do you have any objections to registering all OF generated
platform devices under /sys/devices/platform instead of /sys/devices?
I don't much like the platform directory, but it would make the code
more consistent for DT and non-DT users.

g.

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

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Jason Gunthorpe]

On Wed, Nov 21, 2012 at 03:51:04PM +0000, Grant Likely wrote:
> On Wed, 21 Nov 2012 00:24:48 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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.
> > 
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> >  drivers/of/device.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > Tested on PPC32 and ARM32 embedded kernels.
> > 
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index 4c74e4f..a5b67dc 100644
> > +++ b/drivers/of/device.c
> > @@ -62,7 +62,7 @@ int of_device_add(struct platform_device *ofdev)
> >  	if (!ofdev->dev.parent)
> >  		set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
> >  
> > -	return device_add(&ofdev->dev);
> > +	return platform_device_add(ofdev);
> >  }
> >  
> >  int of_device_register(struct platform_device *pdev)
> 
> This has the side effect of moving all devices at the root of the tree
> from /sys/devices/ to /sys/devices/platform. It also has the possibility
> of breaking if any devices get registered with overlapping regions. I
> think there are some powerpc 5200 boards that do this, and I'm not sure
> about the larger Power boxen.

Okay, I'll try to test your patch.

I know sensible overlapping seems to work:

e0000000-e7ffffff : PCIe 0 MEM
  e0000000-e000ffff : 0000:00:01.0
    e0000000-e0000fff : /pex@e0000000/chip@0/chip_control@0
      e0000008-e000000b : dat
        e0000008-e000000b : dat
      e000000c-e000000f : set
        e000000c-e000000f : set
      e0000010-e0000013 : dirin
        e0000010-e0000013 : dirin

Which is nesting the generic gpio driver under a larger region..

And:
f1010100-f101013f : /internal@f1000000/gpio@10100
  f1010100-f1010103 : /internal@f1000000/chip_cfg@0

Which is nesting a register set for one OF platform_device under
another.

It seems to be handled automatically.

My motivations are two fold:
 - Having a mostly empty /proc/iomem isn't great for diagnostics
 - I'll send a patch today that adds some more code to
   platform_device_add

Thanks Grant,
Jason

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Wed, Nov 21, 2012 at 5:44 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Nov 21, 2012 at 03:51:04PM +0000, Grant Likely wrote:
>> On Wed, 21 Nov 2012 00:24:48 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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.
>> >
>> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> >  drivers/of/device.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > Tested on PPC32 and ARM32 embedded kernels.
>> >
>> > diff --git a/drivers/of/device.c b/drivers/of/device.c
>> > index 4c74e4f..a5b67dc 100644
>> > +++ b/drivers/of/device.c
>> > @@ -62,7 +62,7 @@ int of_device_add(struct platform_device *ofdev)
>> >     if (!ofdev->dev.parent)
>> >             set_dev_node(&ofdev->dev, of_node_to_nid(ofdev->dev.of_node));
>> >
>> > -   return device_add(&ofdev->dev);
>> > +   return platform_device_add(ofdev);
>> >  }
>> >
>> >  int of_device_register(struct platform_device *pdev)
>>
>> This has the side effect of moving all devices at the root of the tree
>> from /sys/devices/ to /sys/devices/platform. It also has the possibility
>> of breaking if any devices get registered with overlapping regions. I
>> think there are some powerpc 5200 boards that do this, and I'm not sure
>> about the larger Power boxen.
>
> Okay, I'll try to test your patch.
>
> I know sensible overlapping seems to work:
>
> e0000000-e7ffffff : PCIe 0 MEM
>   e0000000-e000ffff : 0000:00:01.0
>     e0000000-e0000fff : /pex@e0000000/chip@0/chip_control@0
>       e0000008-e000000b : dat
>         e0000008-e000000b : dat
>       e000000c-e000000f : set
>         e000000c-e000000f : set
>       e0000010-e0000013 : dirin
>         e0000010-e0000013 : dirin
>
> Which is nesting the generic gpio driver under a larger region..

Try two sibling nodes with overlapping addresses. There are powerpc
device trees doing that even though it isn't legal by the ofw and
epapr specs.

g.

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Jason Gunthorpe]

On Wed, Nov 21, 2012 at 06:07:46PM +0000, Grant Likely wrote:

> > Which is nesting the generic gpio driver under a larger region..
> 
> Try two sibling nodes with overlapping addresses. There are powerpc
> device trees doing that even though it isn't legal by the ofw and
> epapr specs.

Both my examples were using sibling nodes in the OF tree.

	pex@e0000000 {
		device_type = "pci";
		ranges = <0x02000000 0x00000000 0x00000000  0xe0000000  0x0 0x8000000>;
		bus-range = <0x0 0xFF>;
		chip@0 {
			ranges = <0x02000000 0x00000000 0x00000000  0x02000000 0x00000000 0x00000000  0x0 0x8000000>;
			chip_control@0 {
				compatible = "orc,chip,control";
				assigned-addresses = <0x02000000 0x0 0x0  0x0 4096>;
			};

			gpio3: chip_gpio@8 {
			        #gpio-cells = <2>;
				compatible = "linux,basic-mmio-gpio";
				gpio-controller;
				reg-names = "dat", "set", "dirin";
				assigned-addresses = <0x02000000 0x0 0x8  0x0 4>,
				                     <0x02000000 0x0 0xc  0x0 4>,
				                     <0x02000000 0x0 0x10  0x0 4>;
			};

Non-conformant yes, but it is the simplest way to get linux to bind
two drivers to the same memory space.

Jason

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Wed, 21 Nov 2012 11:14:30 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Nov 21, 2012 at 06:07:46PM +0000, Grant Likely wrote:
> 
> > > Which is nesting the generic gpio driver under a larger region..
> > 
> > Try two sibling nodes with overlapping addresses. There are powerpc
> > device trees doing that even though it isn't legal by the ofw and
> > epapr specs.
> 
> Both my examples were using sibling nodes in the OF tree.
> 
> 	pex@e0000000 {
> 		device_type = "pci";
> 		ranges = <0x02000000 0x00000000 0x00000000  0xe0000000  0x0 0x8000000>;
> 		bus-range = <0x0 0xFF>;
> 		chip@0 {
> 			ranges = <0x02000000 0x00000000 0x00000000  0x02000000 0x00000000 0x00000000  0x0 0x8000000>;
> 			chip_control@0 {
> 				compatible = "orc,chip,control";
> 				assigned-addresses = <0x02000000 0x0 0x0  0x0 4096>;
> 			};
> 
> 			gpio3: chip_gpio@8 {
> 			        #gpio-cells = <2>;
> 				compatible = "linux,basic-mmio-gpio";
> 				gpio-controller;
> 				reg-names = "dat", "set", "dirin";
> 				assigned-addresses = <0x02000000 0x0 0x8  0x0 4>,
> 				                     <0x02000000 0x0 0xc  0x0 4>,
> 				                     <0x02000000 0x0 0x10  0x0 4>;
> 			};
> 
> Non-conformant yes, but it is the simplest way to get linux to bind
> two drivers to the same memory space.

Hmm... I've not tried it with assigned-address. I tried with two sibling
platform devices using just the 'reg' property. That the kernel will
complain about. For powerpc-only, the patch I posted allows the device
to get registered anyway even though the range incorrectly overlaps.

g.

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Jason Gunthorpe]

On Thu, Nov 22, 2012 at 03:36:21PM +0000, Grant Likely wrote:

> Hmm... I've not tried it with assigned-address. I tried with two sibling
> platform devices using just the 'reg' property. That the kernel will
> complain about. For powerpc-only, the patch I posted allows the device
> to get registered anyway even though the range incorrectly overlaps.

My second example was done with the reg property..

                gpio0: gpio@10100 {
                        compatible = "marvell,orion-gpio";
                        #gpio-cells = <2>;
                        gpio-controller;
                        reg = <0x10100 0x40>;
                }
                chip_cfg@0 {
                        compatible = "orc,chip_config";
                        // Doubles up on gpio0
                        reg = <0x10100 0x4>;
                };


f1010100-f101013f : /internal@f1000000/gpio@10100
  f1010100-f1010103 : /internal@f1000000/chip_cfg@0

What did you try? Maybe order matters?

Regards,
Jason

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Thu, 22 Nov 2012 10:30:20 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Nov 22, 2012 at 03:36:21PM +0000, Grant Likely wrote:
> 
> > Hmm... I've not tried it with assigned-address. I tried with two sibling
> > platform devices using just the 'reg' property. That the kernel will
> > complain about. For powerpc-only, the patch I posted allows the device
> > to get registered anyway even though the range incorrectly overlaps.
> 
> My second example was done with the reg property..
> 
>                 gpio0: gpio@10100 {
>                         compatible = "marvell,orion-gpio";
>                         #gpio-cells = <2>;
>                         gpio-controller;
>                         reg = <0x10100 0x40>;
>                 }
>                 chip_cfg@0 {
>                         compatible = "orc,chip_config";
>                         // Doubles up on gpio0
>                         reg = <0x10100 0x4>;
>                 };
> 
> 
> f1010100-f101013f : /internal@f1000000/gpio@10100
>   f1010100-f1010103 : /internal@f1000000/chip_cfg@0
> 
> What did you try? Maybe order matters?

It might. I tried on qemu with versatile. I've written a new test block
with different overlaps. Here's the block and the results:

     dummy@10201000 {
             compatible = "acme,test";
             reg = <0x10201000 0x1000>;
     };

     overlap@10200800 {
             compatible = "acme,test";
             reg = <0x10200800 0x1000>;
     };

     overlap@10201800 {
             compatible = "acme,test";
             reg = <0x10201800 0x1000>;
     };

     overlap@10201400 {
             compatible = "acme,test";
             reg = <0x10201400 0x800>;
     };

     overlap@10200c00 {
             compatible = "acme,test";
             reg = <0x10200c00 0x1800>;
     };

>From the kernel log:
10200800.overlap: failed to claim resource 0
10201800.overlap: failed to claim resource 0

# ls /sys/bus/platform/devices/
10002000.i2c      10010000.net      10201000.dummy    alarmtimer
10003000.intc     10140000.intc     10201400.overlap  amba.0
10008000.lcd      10200c00.overlap  34000000.flash    fpga.1

So, overlaps that are completely inside or completely outside the
already registered range don't appear to be detected. That may be a bug
(unless it is designed to work that way)

g.

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

Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add

[Grant Likely]

On Thu, 22 Nov 2012 10:30:20 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Thu, Nov 22, 2012 at 03:36:21PM +0000, Grant Likely wrote:
> 
> > Hmm... I've not tried it with assigned-address. I tried with two sibling
> > platform devices using just the 'reg' property. That the kernel will
> > complain about. For powerpc-only, the patch I posted allows the device
> > to get registered anyway even though the range incorrectly overlaps.
> 
> My second example was done with the reg property..
> 
>                 gpio0: gpio@10100 {
>                         compatible = "marvell,orion-gpio";
>                         #gpio-cells = <2>;
>                         gpio-controller;
>                         reg = <0x10100 0x40>;
>                 }
>                 chip_cfg@0 {
>                         compatible = "orc,chip_config";
>                         // Doubles up on gpio0
>                         reg = <0x10100 0x4>;
>                 };
> 
> 
> f1010100-f101013f : /internal@f1000000/gpio@10100
>   f1010100-f1010103 : /internal@f1000000/chip_cfg@0
> 
> What did you try? Maybe order matters?

(Sorry for not replying to my own mail; I'm doing this offline and my
sent mail doesn't show)

Looks like it is by design. With my dummy devices I see this:

10200c00-102023ff : /amba/overlap@10200c00
  10201000-10201fff : /amba/dummy@10201000
    10201400-10201bff : /amba/overlap@10201400

All three of those devices are siblings in the device tree.

g.