* [PATCH] of: Have of_device_add call platform_device_add rather than device_add
@ 2012-11-21 7:24 Jason Gunthorpe
2012-11-21 15:51 ` Grant Likely
0 siblings, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2012-11-21 7:24 UTC (permalink / raw)
To: linux-kernel, Grant Likely, Rob Herring, Greg Kroah-Hartman
Cc: devicetree-discuss
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 7:24 [PATCH] of: Have of_device_add call platform_device_add rather than device_add Jason Gunthorpe
@ 2012-11-21 15:51 ` Grant Likely
2012-11-21 16:05 ` Grant Likely
2012-11-21 17:44 ` Jason Gunthorpe
0 siblings, 2 replies; 10+ messages in thread
From: Grant Likely @ 2012-11-21 15:51 UTC (permalink / raw)
To: Jason Gunthorpe, linux-kernel, Rob Herring, Greg Kroah-Hartman
Cc: devicetree-discuss
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 15:51 ` Grant Likely
@ 2012-11-21 16:05 ` Grant Likely
2012-11-21 17:44 ` Jason Gunthorpe
1 sibling, 0 replies; 10+ messages in thread
From: Grant Likely @ 2012-11-21 16:05 UTC (permalink / raw)
To: Jason Gunthorpe, Linux Kernel Mailing List, Rob Herring,
Greg Kroah-Hartman
Cc: devicetree-discuss, Benjamin Herrenschmidt
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 15:51 ` Grant Likely
2012-11-21 16:05 ` Grant Likely
@ 2012-11-21 17:44 ` Jason Gunthorpe
2012-11-21 18:07 ` Grant Likely
1 sibling, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2012-11-21 17:44 UTC (permalink / raw)
To: Grant Likely
Cc: linux-kernel, Rob Herring, Greg Kroah-Hartman, devicetree-discuss
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 17:44 ` Jason Gunthorpe
@ 2012-11-21 18:07 ` Grant Likely
2012-11-21 18:14 ` Jason Gunthorpe
0 siblings, 1 reply; 10+ messages in thread
From: Grant Likely @ 2012-11-21 18:07 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 18:07 ` Grant Likely
@ 2012-11-21 18:14 ` Jason Gunthorpe
2012-11-22 15:36 ` Grant Likely
0 siblings, 1 reply; 10+ messages in thread
From: Jason Gunthorpe @ 2012-11-21 18:14 UTC (permalink / raw)
To: Grant Likely
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-21 18:14 ` Jason Gunthorpe
@ 2012-11-22 15:36 ` Grant Likely
2012-11-22 17:30 ` Jason Gunthorpe
0 siblings, 1 reply; 10+ messages in thread
From: Grant Likely @ 2012-11-22 15:36 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-22 15:36 ` Grant Likely
@ 2012-11-22 17:30 ` Jason Gunthorpe
2012-11-26 14:30 ` Grant Likely
2012-11-26 15:28 ` Grant Likely
0 siblings, 2 replies; 10+ messages in thread
From: Jason Gunthorpe @ 2012-11-22 17:30 UTC (permalink / raw)
To: Grant Likely
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-22 17:30 ` Jason Gunthorpe
@ 2012-11-26 14:30 ` Grant Likely
2012-11-26 15:28 ` Grant Likely
1 sibling, 0 replies; 10+ messages in thread
From: Grant Likely @ 2012-11-26 14:30 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: Have of_device_add call platform_device_add rather than device_add
2012-11-22 17:30 ` Jason Gunthorpe
2012-11-26 14:30 ` Grant Likely
@ 2012-11-26 15:28 ` Grant Likely
1 sibling, 0 replies; 10+ messages in thread
From: Grant Likely @ 2012-11-26 15:28 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Linux Kernel Mailing List, Rob Herring, Greg Kroah-Hartman,
devicetree-discuss
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-26 16:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-21 7:24 [PATCH] of: Have of_device_add call platform_device_add rather than device_add Jason Gunthorpe
2012-11-21 15:51 ` Grant Likely
2012-11-21 16:05 ` Grant Likely
2012-11-21 17:44 ` Jason Gunthorpe
2012-11-21 18:07 ` Grant Likely
2012-11-21 18:14 ` Jason Gunthorpe
2012-11-22 15:36 ` Grant Likely
2012-11-22 17:30 ` Jason Gunthorpe
2012-11-26 14:30 ` Grant Likely
2012-11-26 15:28 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).