Same parts of DT being probed twice

Same parts of DT being probed twice

[Daniel Drake]

Hi,

We're working on enabling the HDD LED on the XO-1.5 laptop using
gpio-leds and its automatic interaction with the device tree. I have
it working locally, but there is something not quite right.

We have modified the device tree to add /pci/isa/gpios

gpios then has a child "gpio-leds" which has a child "hdd" as
described in Documentation/devicetree/bindings/gpio/gpio-leds.txt

This means that we must extend the "matches" list passed to
of_platform_bus_probe() in arch/x86/platform/olpc/olpc_dt.c so that
the search goes deep enough into the tree to find the gpio-leds
element and create a device for it. Specifically we must add:

	{ .compatible = "pci" },
	{ .compatible = "isa" },
	{ .compatible = "via,vx855-gpio" },
	{ .compatible = "gpio-leds" },

This works, but it causes huge duplication of the whole pci and isa
trees in /sys/devices. These devices were already created because
arch/x86/kernel/devicetree.c unconditionally calls
of_platform_bus_probe() for pci and isa devices (see add_bus_probe()
and ce4100_ids), but not for vx855-gpio and gpio-leds.

Whats the best way to solve this?

Thanks,
Daniel

Re: Same parts of DT being probed twice

[Rob Herring]

Daniel,

On 07/21/2011 09:46 AM, Daniel Drake wrote:
> Hi,
> 
> We're working on enabling the HDD LED on the XO-1.5 laptop using
> gpio-leds and its automatic interaction with the device tree. I have
> it working locally, but there is something not quite right.
> 
> We have modified the device tree to add /pci/isa/gpios
> 
> gpios then has a child "gpio-leds" which has a child "hdd" as
> described in Documentation/devicetree/bindings/gpio/gpio-leds.txt
> 
> This means that we must extend the "matches" list passed to
> of_platform_bus_probe() in arch/x86/platform/olpc/olpc_dt.c so that
> the search goes deep enough into the tree to find the gpio-leds
> element and create a device for it. Specifically we must add:
> 
> 	{ .compatible = "pci" },
> 	{ .compatible = "isa" },
> 	{ .compatible = "via,vx855-gpio" },
> 	{ .compatible = "gpio-leds" },
> 
> This works, but it causes huge duplication of the whole pci and isa
> trees in /sys/devices. These devices were already created because
> arch/x86/kernel/devicetree.c unconditionally calls
> of_platform_bus_probe() for pci and isa devices (see add_bus_probe()
> and ce4100_ids), but not for vx855-gpio and gpio-leds.
> 
> Whats the best way to solve this?
> 
I think you want to move up the gpio-leds to top level of the dts. The
leds are part of your board not part of the gpio controller, so they
should be separate. On the other hand, i2c slaves are typically under
the controller node, so perhaps gpio controller drivers need to scan for
sub devices like leds.

Rob

Re: Same parts of DT being probed twice

[Grant Likely]

On Thu, Jul 21, 2011 at 03:46:02PM +0100, Daniel Drake wrote:
> Hi,
> 
> We're working on enabling the HDD LED on the XO-1.5 laptop using
> gpio-leds and its automatic interaction with the device tree. I have
> it working locally, but there is something not quite right.
> 
> We have modified the device tree to add /pci/isa/gpios
> 
> gpios then has a child "gpio-leds" which has a child "hdd" as
> described in Documentation/devicetree/bindings/gpio/gpio-leds.txt
> 
> This means that we must extend the "matches" list passed to
> of_platform_bus_probe() in arch/x86/platform/olpc/olpc_dt.c so that
> the search goes deep enough into the tree to find the gpio-leds
> element and create a device for it. Specifically we must add:
> 
> 	{ .compatible = "pci" },
> 	{ .compatible = "isa" },
> 	{ .compatible = "via,vx855-gpio" },
> 	{ .compatible = "gpio-leds" },
> 
> This works, but it causes huge duplication of the whole pci and isa
> trees in /sys/devices. These devices were already created because
> arch/x86/kernel/devicetree.c unconditionally calls
> of_platform_bus_probe() for pci and isa devices (see add_bus_probe()
> and ce4100_ids), but not for vx855-gpio and gpio-leds.

of_platform_bus_probe() from the root of the tree is the wrong
approach for registering 'deep' devices.  of_platform_bus_probe() only
knows how to deal with platform_devices.  The moment something
non-trivial appears in between, it no longer works.  What you really
want from the description you've given is the following struct device
hierarchy:

platform_device(representing the pci bus)
   --->pci_dev(representing the isa bridge)
      --->isa_device(representing the "via,vx855-gpio")

To make this work, of_platform_bus_probe() creates the pci bus
instance.  The pci bus /driver/ is responsible for creating the isa
bridge, and similarly the isa driver must be responsible for creating
the gpio controller instance.  Most of the pci support code that you
need should already be there, but I haven't looked at what ISA is
doing.

The gpio-leds node should be at the root of the tree, and everything
gets a lot easier if you switch to using the new of_platform_populate()
instead of of_platform_bus_probe().

g.


> 
> Whats the best way to solve this?
> 
> Thanks,
> Daniel
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Same parts of DT being probed twice

[Daniel Drake]

On 22 July 2011 18:35, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> of_platform_bus_probe() from the root of the tree is the wrong
> approach for registering 'deep' devices.  of_platform_bus_probe() only
> knows how to deal with platform_devices.  The moment something
> non-trivial appears in between, it no longer works.  What you really
> want from the description you've given is the following struct device
> hierarchy:
>
> platform_device(representing the pci bus)
>   --->pci_dev(representing the isa bridge)
>      --->isa_device(representing the "via,vx855-gpio")
>
> To make this work, of_platform_bus_probe() creates the pci bus
> instance.  The pci bus /driver/ is responsible for creating the isa
> bridge, and similarly the isa driver must be responsible for creating
> the gpio controller instance.  Most of the pci support code that you
> need should already be there, but I haven't looked at what ISA is
> doing.

OK, thanks for the explanation.

In this case we don't need to create a platform_device for the vx855
gpio device - it is already handled through probing of the PCI bus
which is discovered through other means. I only added it in the list
as an item inbetween the root and gpio-leds.

> The gpio-leds node should be at the root of the tree, and everything
> gets a lot easier if you switch to using the new of_platform_populate()
> instead of of_platform_bus_probe().

That sounds more sensible. I'll check with our firmware guy when he
gets back from his break and continue from there.

However, even after fixing that, we will still be left with the
generic x86 code probing pci and isa. This creates a huge array of
platform devices (which are useless, or are devices that we already
have other device instances for), and seems to take quite a while
during boot. Is commit 9079b35364e75 wrong in some way? It has your
acked-by, but seems to go against what you wrote above.

Thanks,
Daniel