* [PATCH 2/2] of: use platform_device_add
[not found] ` <1358473200-17886-2-git-send-email-grant.likely@secretlab.ca>
@ 2013-02-17 3:03 ` Shawn Guo
2013-02-17 7:43 ` Shawn Guo
0 siblings, 1 reply; 6+ messages in thread
From: Shawn Guo @ 2013-02-17 3:03 UTC (permalink / raw)
To: linux-arm-kernel
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 at 020e0000 {
compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
reg = <0x020e0000 0x38>;
};
iomuxc: iomuxc at 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] of: use platform_device_add
2013-02-17 3:03 ` [PATCH 2/2] of: use platform_device_add Shawn Guo
@ 2013-02-17 7:43 ` Shawn Guo
2013-02-17 10:19 ` Russell King - ARM Linux
2013-02-17 13:18 ` Fabio Estevam
0 siblings, 2 replies; 6+ messages in thread
From: Shawn Guo @ 2013-02-17 7:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] of: use platform_device_add
2013-02-17 7:43 ` Shawn Guo
@ 2013-02-17 10:19 ` Russell King - ARM Linux
2013-02-17 10:49 ` Grant Likely
2013-02-17 13:18 ` Fabio Estevam
1 sibling, 1 reply; 6+ messages in thread
From: Russell King - ARM Linux @ 2013-02-17 10:19 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] of: use platform_device_add
2013-02-17 10:19 ` Russell King - ARM Linux
@ 2013-02-17 10:49 ` Grant Likely
2013-02-19 19:03 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Grant Likely @ 2013-02-17 10:49 UTC (permalink / raw)
To: linux-arm-kernel
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 at 020e0000 {
> > compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > reg = <0x020e0000 0x38>;
> > };
> >
> > iomuxc: iomuxc at 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] of: use platform_device_add
2013-02-17 7:43 ` Shawn Guo
2013-02-17 10:19 ` Russell King - ARM Linux
@ 2013-02-17 13:18 ` Fabio Estevam
1 sibling, 0 replies; 6+ messages in thread
From: Fabio Estevam @ 2013-02-17 13:18 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] of: use platform_device_add
2013-02-17 10:49 ` Grant Likely
@ 2013-02-19 19:03 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2013-02-19 19:03 UTC (permalink / raw)
To: linux-arm-kernel
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 at 020e0000 {
> > > compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > > reg = <0x020e0000 0x38>;
> > > };
> > >
> > > iomuxc: iomuxc at 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-19 19:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1358473200-17886-1-git-send-email-grant.likely@secretlab.ca>
[not found] ` <1358473200-17886-2-git-send-email-grant.likely@secretlab.ca>
2013-02-17 3:03 ` [PATCH 2/2] of: use platform_device_add Shawn Guo
2013-02-17 7:43 ` Shawn Guo
2013-02-17 10:19 ` Russell King - ARM Linux
2013-02-17 10:49 ` Grant Likely
2013-02-19 19:03 ` Jason Gunthorpe
2013-02-17 13:18 ` Fabio Estevam
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).