[PATCH 00/13] Discover and probe dependencies

[PATCH 00/13] Discover and probe dependencies

[Tomeu Vizoso]

Hi,

this is another attempt at preventing deferred probe from obscuring why your
devices aren't probing and from delaying to the end of the boot process the
probe of the device you care the most.

The major differences with my previous approach [0] are:

* Dependencies are probed before the target is probed, so we don't have nested
  probe() calls, avoiding a series of deadlock situations.

* Dependencies could be stored and reused for other purposes such as for
  passing resources to drivers ala devm_probe, or for warning when a device is
  going to be unbound and has dependencies active, etc.

* I have tried to keep it firmware-agnostic. The previous approach (on-demand
  probing) could be done like this as well, but would require adding fwnode
  APIs to all affected subsystems first.

I have only implemented the class.get_dependencies() callback for the GPIO
subsystem and for the host1x bus because that's all that was needed on my Tegra
Chromebook to avoid deferred probes, but if this approach is deemed worthwhile
I will add more implementations so that deferred probes are avoided on the
other boards I have access to.

[0] http://thread.gmane.org/gmane.linux.kernel.gpio/8465

Thanks,

Tomeu

Tomeu Vizoso (13):
  gpiolib: Fix docs for gpiochip_add_pingroup_range
  driver-core: defer all probes until late_initcall
  ARM: tegra: Add gpio-ranges property
  pinctrl: tegra: Only set the gpio range if needed
  driver core: fix docbook for device_private.device
  of/platform: Set fwnode field for new devices
  driver-core: Add class.get_dependencies() callback
  gpio: sysfs: implement class.get_dependencies()
  gpu: host1x: implement class.get_dependencies()
  driver-core: add for_each_class()
  device property: add fwnode_get_parent()
  device property: add fwnode_get_name()
  driver-core: probe dependencies before probing

 arch/arm/boot/dts/tegra114.dtsi |   1 +
 arch/arm/boot/dts/tegra124.dtsi |   1 +
 arch/arm/boot/dts/tegra20.dtsi  |   1 +
 arch/arm/boot/dts/tegra30.dtsi  |   1 +
 drivers/base/base.h             |   4 +-
 drivers/base/class.c            |  16 +++++
 drivers/base/dd.c               | 128 +++++++++++++++++++++++++++++++++++++++-
 drivers/base/property.c         |  38 ++++++++++++
 drivers/gpio/gpiolib-sysfs.c    |  81 +++++++++++++++++++++++++
 drivers/gpio/gpiolib.c          |   2 +-
 drivers/gpu/host1x/dev.c        |  47 +++++++++++++++
 drivers/of/platform.c           |   1 +
 drivers/pinctrl/pinctrl-tegra.c |  19 +++++-
 include/acpi/acpi_bus.h         |   5 ++
 include/linux/acpi.h            |   5 ++
 include/linux/device.h          |   6 ++
 include/linux/fwnode.h          |   5 ++
 include/linux/property.h        |   4 ++
 18 files changed, 361 insertions(+), 4 deletions(-)

-- 
2.4.1

[PATCH 01/13] gpiolib: Fix docs for gpiochip_add_pingroup_range

[Tomeu Vizoso]

gpiochip_add_pingroup_range() has a pctldev argument, not pinctrl.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 957ede5..dc44350 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -671,7 +671,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) {}
 /**
  * gpiochip_add_pingroup_range() - add a range for GPIO <-> pin mapping
  * @chip: the gpiochip to add the range for
- * @pinctrl: the dev_name() of the pin controller to map to
+ * @pctldev: the pin controller to map to
  * @gpio_offset: the start offset in the current gpio_chip number space
  * @pin_group: name of the pin group inside the pin controller
  */
-- 
2.4.1

[PATCH 01/13] gpiolib: Fix docs for gpiochip_add_pingroup_range

[Linus Walleij]

On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:

> gpiochip_add_pingroup_range() has a pctldev argument, not pinctrl.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Patch applied.

Yours,
Linus Walleij

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Tomeu Vizoso]

To decrease the chances of devices deferring their probes because of
dependencies not having probed yet because of their drivers not having
registered yet, delay all probing until the late initcall level.

This will allow us to avoid deferred probes completely later by probing
dependencies on demand, or by probing them in dependency order.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/dd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a638bbb..18438aa 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 	if (!device_is_registered(dev))
 		return -ENODEV;
 
+	/* Defer all probes until we start processing the queue */
+	if (!driver_deferred_probe_enable) {
+		driver_deferred_probe_add(dev);
+		return 0;
+	}
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
@@ -585,7 +591,7 @@ EXPORT_SYMBOL_GPL(device_attach);
 
 void device_initial_probe(struct device *dev)
 {
-	__device_attach(dev, true);
+	__device_attach(dev, driver_deferred_probe_enable);
 }
 
 static int __driver_attach(struct device *dev, void *data)
-- 
2.4.1

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rafael J. Wysocki]

On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
> To decrease the chances of devices deferring their probes because of
> dependencies not having probed yet because of their drivers not having
> registered yet, delay all probing until the late initcall level.
> 
> This will allow us to avoid deferred probes completely later by probing
> dependencies on demand, or by probing them in dependency order.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  drivers/base/dd.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index a638bbb..18438aa 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>  	if (!device_is_registered(dev))
>  		return -ENODEV;
>  
> +	/* Defer all probes until we start processing the queue */
> +	if (!driver_deferred_probe_enable) {
> +		driver_deferred_probe_add(dev);

Do I think correctly that this will effectively force everybody to use deferred
probing?

> +		return 0;
> +	}
> +
>  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>  		 drv->bus->name, __func__, dev_name(dev), drv->name);
>  
> @@ -585,7 +591,7 @@ EXPORT_SYMBOL_GPL(device_attach);
>  
>  void device_initial_probe(struct device *dev)
>  {
> -	__device_attach(dev, true);
> +	__device_attach(dev, driver_deferred_probe_enable);
>  }
>  
>  static int __driver_attach(struct device *dev, void *data)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Tomeu Vizoso]

On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
>> To decrease the chances of devices deferring their probes because of
>> dependencies not having probed yet because of their drivers not having
>> registered yet, delay all probing until the late initcall level.
>>
>> This will allow us to avoid deferred probes completely later by probing
>> dependencies on demand, or by probing them in dependency order.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>  drivers/base/dd.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index a638bbb..18438aa 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>>       if (!device_is_registered(dev))
>>               return -ENODEV;
>>
>> +     /* Defer all probes until we start processing the queue */
>> +     if (!driver_deferred_probe_enable) {
>> +             driver_deferred_probe_add(dev);
>
> Do I think correctly that this will effectively force everybody to use deferred
> probing?

Guess it depends on the meaning of "using deferred probing". It will
defer the probe of the first device to late_initcall (which will
happen much earlier in time than before), but afterwards all built-in
drivers will be available and depending on the order in which we try
to probe devices, none may actually ask to defer its probe.

But what this patch achieves has nothing to do with drivers returning
-EPROBE_DEFER, it just delays device probe until all built-in drivers
have been registered.

I could have avoided reusing any of the deferred probe code by
creating a new queue of devices that need probing, and by registering
a new late_initcall to start processing them, but because that is
always enabled unconditionally, it seemed silly to not reuse that code
that already does exactly that.

Thanks,

Tomeu

>> +             return 0;
>> +     }
>> +
>>       pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>>                drv->bus->name, __func__, dev_name(dev), drv->name);
>>
>> @@ -585,7 +591,7 @@ EXPORT_SYMBOL_GPL(device_attach);
>>
>>  void device_initial_probe(struct device *dev)
>>  {
>> -     __device_attach(dev, true);
>> +     __device_attach(dev, driver_deferred_probe_enable);
>>  }
>>
>>  static int __driver_attach(struct device *dev, void *data)
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rafael J. Wysocki]

On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
> >> To decrease the chances of devices deferring their probes because of
> >> dependencies not having probed yet because of their drivers not having
> >> registered yet, delay all probing until the late initcall level.
> >>
> >> This will allow us to avoid deferred probes completely later by probing
> >> dependencies on demand, or by probing them in dependency order.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> ---
> >>  drivers/base/dd.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> index a638bbb..18438aa 100644
> >> --- a/drivers/base/dd.c
> >> +++ b/drivers/base/dd.c
> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> >>       if (!device_is_registered(dev))
> >>               return -ENODEV;
> >>
> >> +     /* Defer all probes until we start processing the queue */
> >> +     if (!driver_deferred_probe_enable) {
> >> +             driver_deferred_probe_add(dev);
> >
> > Do I think correctly that this will effectively force everybody to use deferred
> > probing?
> 
> Guess it depends on the meaning of "using deferred probing". It will
> defer the probe of the first device to late_initcall (which will
> happen much earlier in time than before), but afterwards all built-in
> drivers will be available and depending on the order in which we try
> to probe devices, none may actually ask to defer its probe.

So this will break things like the PNP system driver which relies on probing
stuff at the fs_initcall stage for correctness.  It may also break other
things with similar assumptions.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rob Herring]

On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
>> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
>> >> To decrease the chances of devices deferring their probes because of
>> >> dependencies not having probed yet because of their drivers not having
>> >> registered yet, delay all probing until the late initcall level.
>> >>
>> >> This will allow us to avoid deferred probes completely later by probing
>> >> dependencies on demand, or by probing them in dependency order.
>> >>
>> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >> ---
>> >>  drivers/base/dd.c | 8 +++++++-
>> >>  1 file changed, 7 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> >> index a638bbb..18438aa 100644
>> >> --- a/drivers/base/dd.c
>> >> +++ b/drivers/base/dd.c
>> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>> >>       if (!device_is_registered(dev))
>> >>               return -ENODEV;
>> >>
>> >> +     /* Defer all probes until we start processing the queue */
>> >> +     if (!driver_deferred_probe_enable) {
>> >> +             driver_deferred_probe_add(dev);
>> >
>> > Do I think correctly that this will effectively force everybody to use deferred
>> > probing?
>>
>> Guess it depends on the meaning of "using deferred probing". It will
>> defer the probe of the first device to late_initcall (which will
>> happen much earlier in time than before), but afterwards all built-in
>> drivers will be available and depending on the order in which we try
>> to probe devices, none may actually ask to defer its probe.
>
> So this will break things like the PNP system driver which relies on probing
> stuff at the fs_initcall stage for correctness.  It may also break other
> things with similar assumptions.

Yes, but I think that this can be done for only OF based devices
rather than globally for all platform devices and solve that problem.
Matching is already dependent of the type of device.

Rob

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rafael J. Wysocki]

On Monday, June 22, 2015 07:07:08 PM Rob Herring wrote:
> On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
> >> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
> >> >> To decrease the chances of devices deferring their probes because of
> >> >> dependencies not having probed yet because of their drivers not having
> >> >> registered yet, delay all probing until the late initcall level.
> >> >>
> >> >> This will allow us to avoid deferred probes completely later by probing
> >> >> dependencies on demand, or by probing them in dependency order.
> >> >>
> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> >> ---
> >> >>  drivers/base/dd.c | 8 +++++++-
> >> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> >> index a638bbb..18438aa 100644
> >> >> --- a/drivers/base/dd.c
> >> >> +++ b/drivers/base/dd.c
> >> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> >> >>       if (!device_is_registered(dev))
> >> >>               return -ENODEV;
> >> >>
> >> >> +     /* Defer all probes until we start processing the queue */
> >> >> +     if (!driver_deferred_probe_enable) {
> >> >> +             driver_deferred_probe_add(dev);
> >> >
> >> > Do I think correctly that this will effectively force everybody to use deferred
> >> > probing?
> >>
> >> Guess it depends on the meaning of "using deferred probing". It will
> >> defer the probe of the first device to late_initcall (which will
> >> happen much earlier in time than before), but afterwards all built-in
> >> drivers will be available and depending on the order in which we try
> >> to probe devices, none may actually ask to defer its probe.
> >
> > So this will break things like the PNP system driver which relies on probing
> > stuff at the fs_initcall stage for correctness.  It may also break other
> > things with similar assumptions.
> 
> Yes, but I think that this can be done for only OF based devices
> rather than globally for all platform devices and solve that problem.
> Matching is already dependent of the type of device.

Well, the current patch is not OF-only, though.

Rafael

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Tomeu Vizoso]

On 23 June 2015 at 16:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, June 22, 2015 07:07:08 PM Rob Herring wrote:
>> On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
>> >> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
>> >> >> To decrease the chances of devices deferring their probes because of
>> >> >> dependencies not having probed yet because of their drivers not having
>> >> >> registered yet, delay all probing until the late initcall level.
>> >> >>
>> >> >> This will allow us to avoid deferred probes completely later by probing
>> >> >> dependencies on demand, or by probing them in dependency order.
>> >> >>
>> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >> >> ---
>> >> >>  drivers/base/dd.c | 8 +++++++-
>> >> >>  1 file changed, 7 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> >> >> index a638bbb..18438aa 100644
>> >> >> --- a/drivers/base/dd.c
>> >> >> +++ b/drivers/base/dd.c
>> >> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>> >> >>       if (!device_is_registered(dev))
>> >> >>               return -ENODEV;
>> >> >>
>> >> >> +     /* Defer all probes until we start processing the queue */
>> >> >> +     if (!driver_deferred_probe_enable) {
>> >> >> +             driver_deferred_probe_add(dev);
>> >> >
>> >> > Do I think correctly that this will effectively force everybody to use deferred
>> >> > probing?
>> >>
>> >> Guess it depends on the meaning of "using deferred probing". It will
>> >> defer the probe of the first device to late_initcall (which will
>> >> happen much earlier in time than before), but afterwards all built-in
>> >> drivers will be available and depending on the order in which we try
>> >> to probe devices, none may actually ask to defer its probe.
>> >
>> > So this will break things like the PNP system driver which relies on probing
>> > stuff at the fs_initcall stage for correctness.  It may also break other
>> > things with similar assumptions.
>>
>> Yes, but I think that this can be done for only OF based devices
>> rather than globally for all platform devices and solve that problem.
>> Matching is already dependent of the type of device.
>
> Well, the current patch is not OF-only, though.

Yeah, I'm currently looking at only delaying probing of devices
created from OF data.

Note that calculating dependencies and trying to probe them before
they are needed can be done independently of this patch, but it isn't
that useful because devices will still defer their probes because the
drivers of some dependencies won't have been registered until
late_initcall.

Regards,

Tomeu

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rafael J. Wysocki]

On Tuesday, June 23, 2015 04:17:29 PM Tomeu Vizoso wrote:
> On 23 June 2015 at 16:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, June 22, 2015 07:07:08 PM Rob Herring wrote:
> >> On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
> >> >> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> >> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
> >> >> >> To decrease the chances of devices deferring their probes because of
> >> >> >> dependencies not having probed yet because of their drivers not having
> >> >> >> registered yet, delay all probing until the late initcall level.
> >> >> >>
> >> >> >> This will allow us to avoid deferred probes completely later by probing
> >> >> >> dependencies on demand, or by probing them in dependency order.
> >> >> >>
> >> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> >> >> ---
> >> >> >>  drivers/base/dd.c | 8 +++++++-
> >> >> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> >> >> index a638bbb..18438aa 100644
> >> >> >> --- a/drivers/base/dd.c
> >> >> >> +++ b/drivers/base/dd.c
> >> >> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> >> >> >>       if (!device_is_registered(dev))
> >> >> >>               return -ENODEV;
> >> >> >>
> >> >> >> +     /* Defer all probes until we start processing the queue */
> >> >> >> +     if (!driver_deferred_probe_enable) {
> >> >> >> +             driver_deferred_probe_add(dev);
> >> >> >
> >> >> > Do I think correctly that this will effectively force everybody to use deferred
> >> >> > probing?
> >> >>
> >> >> Guess it depends on the meaning of "using deferred probing". It will
> >> >> defer the probe of the first device to late_initcall (which will
> >> >> happen much earlier in time than before), but afterwards all built-in
> >> >> drivers will be available and depending on the order in which we try
> >> >> to probe devices, none may actually ask to defer its probe.
> >> >
> >> > So this will break things like the PNP system driver which relies on probing
> >> > stuff at the fs_initcall stage for correctness.  It may also break other
> >> > things with similar assumptions.
> >>
> >> Yes, but I think that this can be done for only OF based devices
> >> rather than globally for all platform devices and solve that problem.
> >> Matching is already dependent of the type of device.
> >
> > Well, the current patch is not OF-only, though.
> 
> Yeah, I'm currently looking at only delaying probing of devices
> created from OF data.

I'm not sure if tying it hard to OF is not too restrictive.

Maybe we can use some general opt-in mechanism that OF will just always use?

In fact, we have a similar problem in ACPI where we have the _DEP object which
is used by firmware to describe dependencies between devices.

> Note that calculating dependencies and trying to probe them before
> they are needed can be done independently of this patch, but it isn't
> that useful because devices will still defer their probes because the
> drivers of some dependencies won't have been registered until
> late_initcall.

I see.

Rafael

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Tomeu Vizoso]

On 23 June 2015 at 16:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, June 23, 2015 04:17:29 PM Tomeu Vizoso wrote:
>> On 23 June 2015 at 16:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Monday, June 22, 2015 07:07:08 PM Rob Herring wrote:
>> >> On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >> > On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
>> >> >> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >> >> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
>> >> >> >> To decrease the chances of devices deferring their probes because of
>> >> >> >> dependencies not having probed yet because of their drivers not having
>> >> >> >> registered yet, delay all probing until the late initcall level.
>> >> >> >>
>> >> >> >> This will allow us to avoid deferred probes completely later by probing
>> >> >> >> dependencies on demand, or by probing them in dependency order.
>> >> >> >>
>> >> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >> >> >> ---
>> >> >> >>  drivers/base/dd.c | 8 +++++++-
>> >> >> >>  1 file changed, 7 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> >> >> >> index a638bbb..18438aa 100644
>> >> >> >> --- a/drivers/base/dd.c
>> >> >> >> +++ b/drivers/base/dd.c
>> >> >> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
>> >> >> >>       if (!device_is_registered(dev))
>> >> >> >>               return -ENODEV;
>> >> >> >>
>> >> >> >> +     /* Defer all probes until we start processing the queue */
>> >> >> >> +     if (!driver_deferred_probe_enable) {
>> >> >> >> +             driver_deferred_probe_add(dev);
>> >> >> >
>> >> >> > Do I think correctly that this will effectively force everybody to use deferred
>> >> >> > probing?
>> >> >>
>> >> >> Guess it depends on the meaning of "using deferred probing". It will
>> >> >> defer the probe of the first device to late_initcall (which will
>> >> >> happen much earlier in time than before), but afterwards all built-in
>> >> >> drivers will be available and depending on the order in which we try
>> >> >> to probe devices, none may actually ask to defer its probe.
>> >> >
>> >> > So this will break things like the PNP system driver which relies on probing
>> >> > stuff at the fs_initcall stage for correctness.  It may also break other
>> >> > things with similar assumptions.
>> >>
>> >> Yes, but I think that this can be done for only OF based devices
>> >> rather than globally for all platform devices and solve that problem.
>> >> Matching is already dependent of the type of device.
>> >
>> > Well, the current patch is not OF-only, though.
>>
>> Yeah, I'm currently looking at only delaying probing of devices
>> created from OF data.
>
> I'm not sure if tying it hard to OF is not too restrictive.
>
> Maybe we can use some general opt-in mechanism that OF will just always use?

Would it help if buses called fwnode_driver_match_device() instead of
the existing OF and ACPI variants and we did it in there? I'm still
not sure of how fwnode is used in machines with ACPI.

But that would be quite a bit of work that I think should be left for
a later series because otherwise this one is going to balloon in size
really quickly.

> In fact, we have a similar problem in ACPI where we have the _DEP object which
> is used by firmware to describe dependencies between devices.

I would expect that classes/subsystems would be able to use that data
in their class.get_dependencies() callback, if the passed fwnode is a
ACPI node.

Regards,

Tomeu

>> Note that calculating dependencies and trying to probe them before
>> they are needed can be done independently of this patch, but it isn't
>> that useful because devices will still defer their probes because the
>> drivers of some dependencies won't have been registered until
>> late_initcall.
>
> I see.
>
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 02/13] driver-core: defer all probes until late_initcall

[Rafael J. Wysocki]

On Tuesday, June 23, 2015 04:37:57 PM Tomeu Vizoso wrote:
> On 23 June 2015 at 16:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, June 23, 2015 04:17:29 PM Tomeu Vizoso wrote:
> >> On 23 June 2015 at 16:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Monday, June 22, 2015 07:07:08 PM Rob Herring wrote:
> >> >> On Fri, Jun 19, 2015 at 6:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> >> > On Friday, June 19, 2015 03:36:46 PM Tomeu Vizoso wrote:
> >> >> >> On 18 June 2015 at 23:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> >> >> > On Wednesday, June 17, 2015 03:42:12 PM Tomeu Vizoso wrote:
> >> >> >> >> To decrease the chances of devices deferring their probes because of
> >> >> >> >> dependencies not having probed yet because of their drivers not having
> >> >> >> >> registered yet, delay all probing until the late initcall level.
> >> >> >> >>
> >> >> >> >> This will allow us to avoid deferred probes completely later by probing
> >> >> >> >> dependencies on demand, or by probing them in dependency order.
> >> >> >> >>
> >> >> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> >> >> >> ---
> >> >> >> >>  drivers/base/dd.c | 8 +++++++-
> >> >> >> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> >> >> >>
> >> >> >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> >> >> >> index a638bbb..18438aa 100644
> >> >> >> >> --- a/drivers/base/dd.c
> >> >> >> >> +++ b/drivers/base/dd.c
> >> >> >> >> @@ -407,6 +407,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
> >> >> >> >>       if (!device_is_registered(dev))
> >> >> >> >>               return -ENODEV;
> >> >> >> >>
> >> >> >> >> +     /* Defer all probes until we start processing the queue */
> >> >> >> >> +     if (!driver_deferred_probe_enable) {
> >> >> >> >> +             driver_deferred_probe_add(dev);
> >> >> >> >
> >> >> >> > Do I think correctly that this will effectively force everybody to use deferred
> >> >> >> > probing?
> >> >> >>
> >> >> >> Guess it depends on the meaning of "using deferred probing". It will
> >> >> >> defer the probe of the first device to late_initcall (which will
> >> >> >> happen much earlier in time than before), but afterwards all built-in
> >> >> >> drivers will be available and depending on the order in which we try
> >> >> >> to probe devices, none may actually ask to defer its probe.
> >> >> >
> >> >> > So this will break things like the PNP system driver which relies on probing
> >> >> > stuff at the fs_initcall stage for correctness.  It may also break other
> >> >> > things with similar assumptions.
> >> >>
> >> >> Yes, but I think that this can be done for only OF based devices
> >> >> rather than globally for all platform devices and solve that problem.
> >> >> Matching is already dependent of the type of device.
> >> >
> >> > Well, the current patch is not OF-only, though.
> >>
> >> Yeah, I'm currently looking at only delaying probing of devices
> >> created from OF data.
> >
> > I'm not sure if tying it hard to OF is not too restrictive.
> >
> > Maybe we can use some general opt-in mechanism that OF will just always use?
> 
> Would it help if buses called fwnode_driver_match_device() instead of
> the existing OF and ACPI variants and we did it in there?

Probably it would, but I'd need to see the actual patch. :-)

> I'm still not sure of how fwnode is used in machines with ACPI.

I'm not sure what you mean.  On ACPI systems struct fwnode_handle is embedded
in struct acpi_device and there is a pointer from struct device to that field
in the companion ACPI device object.

> But that would be quite a bit of work that I think should be left for
> a later series because otherwise this one is going to balloon in size
> really quickly.

Well, I'd prefer not to leave anything to a "later series" that may never be
submitted ...

> > In fact, we have a similar problem in ACPI where we have the _DEP object which
> > is used by firmware to describe dependencies between devices.
> 
> I would expect that classes/subsystems would be able to use that data
> in their class.get_dependencies() callback, if the passed fwnode is a
> ACPI node.

Yes, something like that.

But the point is that this really isn't OF-specific.

Thanks,
Rafael

[PATCH 03/13] ARM: tegra: Add gpio-ranges property

[Tomeu Vizoso]

Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
explicit.

This currently will add a duplicated entry in the map from pins to gpios
in the pinmux controller but it should be harmless and will be fixed in a
later commit.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

---

v2:	* Change number of gpios to 251 to match pinctrl-tegra124.c
	* Add note about duplicated mapping
	* Add the property to all the other Tegra SoCs
---
 arch/arm/boot/dts/tegra114.dtsi | 1 +
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 arch/arm/boot/dts/tegra20.dtsi  | 1 +
 arch/arm/boot/dts/tegra30.dtsi  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index f58a3d9..7e1e171 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -234,6 +234,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 246>;
 	};
 
 	apbmisc at 70000800 {
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 87318a7..4779df3 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -255,6 +255,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 251>;
 	};
 
 	apbdma: dma at 0,60020000 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index f444b67..b44277c 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -244,6 +244,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 224>;
 	};
 
 	apbmisc at 70000800 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 782b11b..28c547f 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -349,6 +349,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 248>;
 	};
 
 	apbmisc at 70000800 {
-- 
2.4.1

[PATCH 03/13] ARM: tegra: Add gpio-ranges property

[Mark Brown]

On Wed, Jun 17, 2015 at 03:42:13PM +0200, Tomeu Vizoso wrote:
> Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
> explicit.

Can I suggest splitting this sort of patch out of the patch series (and
similarly the documentation fix you had), or putting them at the end or
something?  It's pretty confusing that early patches in the series are
a bit tangential rather than being dependencies for the rest of it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150617/2a9dcd54/attachment.sig>

[PATCH 03/13] ARM: tegra: Add gpio-ranges property

[Tomeu Vizoso]

On 17 June 2015 at 19:25, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 17, 2015 at 03:42:13PM +0200, Tomeu Vizoso wrote:
>> Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
>> explicit.
>
> Can I suggest splitting this sort of patch out of the patch series (and
> similarly the documentation fix you had), or putting them at the end or
> something?  It's pretty confusing that early patches in the series are
> a bit tangential rather than being dependencies for the rest of it.

Agreed, sorry about that.

Thanks,

Tomeu

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Tomeu Vizoso]

If the gpio DT node has the gpio-ranges property, the range will be
added by the gpio core and doesn't need to be added by the pinctrl
driver.

By having the gpio-ranges property, we have an explicit dependency from
the gpio node to the pinctrl node and we can stop using the deprecated
pinctrl_add_gpio_range() function.

Note that when the GPIO device gets probed before the associated
princtrl device, the gpio core actually won't register the gpio range.
Thus, this patch is only safe to be merged after we have in place a way
to assure that gpio devices are probed after their associated pinctrl
devices (such as ordered probing).

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/pinctrl/pinctrl-tegra.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index 0f982b8..0fd7fd2 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -624,6 +624,22 @@ static struct pinctrl_desc tegra_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+static bool gpio_node_has_range(void)
+{
+	struct device_node *np;
+	bool has_prop = false;
+
+	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra30-gpio");
+	if (!np)
+		return has_prop;
+
+	has_prop = of_find_property(np, "gpio-ranges", NULL);
+
+	of_node_put(np);
+
+	return has_prop;
+}
+
 int tegra_pinctrl_probe(struct platform_device *pdev,
 			const struct tegra_pinctrl_soc_data *soc_data)
 {
@@ -708,7 +724,8 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 		return PTR_ERR(pmx->pctl);
 	}
 
-	pinctrl_add_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
+	if (!gpio_node_has_range())
+		pinctrl_add_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
 
 	platform_set_drvdata(pdev, pmx);
 
-- 
2.4.1

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Linus Walleij]

On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:

> If the gpio DT node has the gpio-ranges property, the range will be
> added by the gpio core and doesn't need to be added by the pinctrl
> driver.
>
> By having the gpio-ranges property, we have an explicit dependency from
> the gpio node to the pinctrl node and we can stop using the deprecated
> pinctrl_add_gpio_range() function.
>
> Note that when the GPIO device gets probed before the associated
> princtrl device, the gpio core actually won't register the gpio range.
> Thus, this patch is only safe to be merged after we have in place a way
> to assure that gpio devices are probed after their associated pinctrl
> devices (such as ordered probing).
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

This doesn't look like it would hurt, but need Stephen's opinion
on it, and I think he's on vacation. Would check with next-in-line
Tegra maintainer, Thierry/Alexandre?

Yours,
Linus Walleij

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Tomeu Vizoso]

On 13 July 2015 at 22:14, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>
>> If the gpio DT node has the gpio-ranges property, the range will be
>> added by the gpio core and doesn't need to be added by the pinctrl
>> driver.
>>
>> By having the gpio-ranges property, we have an explicit dependency from
>> the gpio node to the pinctrl node and we can stop using the deprecated
>> pinctrl_add_gpio_range() function.
>>
>> Note that when the GPIO device gets probed before the associated
>> princtrl device, the gpio core actually won't register the gpio range.
>> Thus, this patch is only safe to be merged after we have in place a way
>> to assure that gpio devices are probed after their associated pinctrl
>> devices (such as ordered probing).
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> This doesn't look like it would hurt, but need Stephen's opinion
> on it, and I think he's on vacation. Would check with next-in-line
> Tegra maintainer, Thierry/Alexandre?

Sorry about that, but I have split these changes out into their own
series after people complained about it.

Have just sent a new version which already has Stephen's ack:

https://lkml.kernel.org/g/1436862596-27730-1-git-send-email-tomeu.vizoso at collabora.com

Thanks,

Tomeu

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Alexandre Courbot]

On Tue, Jul 14, 2015 at 5:34 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 13 July 2015 at 22:14, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
>> <tomeu.vizoso@collabora.com> wrote:
>>
>>> If the gpio DT node has the gpio-ranges property, the range will be
>>> added by the gpio core and doesn't need to be added by the pinctrl
>>> driver.
>>>
>>> By having the gpio-ranges property, we have an explicit dependency from
>>> the gpio node to the pinctrl node and we can stop using the deprecated
>>> pinctrl_add_gpio_range() function.
>>>
>>> Note that when the GPIO device gets probed before the associated
>>> princtrl device, the gpio core actually won't register the gpio range.
>>> Thus, this patch is only safe to be merged after we have in place a way
>>> to assure that gpio devices are probed after their associated pinctrl
>>> devices (such as ordered probing).
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> This doesn't look like it would hurt, but need Stephen's opinion
>> on it, and I think he's on vacation. Would check with next-in-line
>> Tegra maintainer, Thierry/Alexandre?
>
> Sorry about that, but I have split these changes out into their own
> series after people complained about it.
>
> Have just sent a new version which already has Stephen's ack:
>
> https://lkml.kernel.org/g/1436862596-27730-1-git-send-email-tomeu.vizoso at collabora.com

The change looks ok, but why limit it to Tegra? It seems like it could
apply to any driver that supports gpio-range. Or is it because this
will only work with drivers that use ordered probing?

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Tomeu Vizoso]

On 15 July 2015 at 05:17, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Tue, Jul 14, 2015 at 5:34 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> On 13 July 2015 at 22:14, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
>>> <tomeu.vizoso@collabora.com> wrote:
>>>
>>>> If the gpio DT node has the gpio-ranges property, the range will be
>>>> added by the gpio core and doesn't need to be added by the pinctrl
>>>> driver.
>>>>
>>>> By having the gpio-ranges property, we have an explicit dependency from
>>>> the gpio node to the pinctrl node and we can stop using the deprecated
>>>> pinctrl_add_gpio_range() function.
>>>>
>>>> Note that when the GPIO device gets probed before the associated
>>>> princtrl device, the gpio core actually won't register the gpio range.
>>>> Thus, this patch is only safe to be merged after we have in place a way
>>>> to assure that gpio devices are probed after their associated pinctrl
>>>> devices (such as ordered probing).
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>
>>> This doesn't look like it would hurt, but need Stephen's opinion
>>> on it, and I think he's on vacation. Would check with next-in-line
>>> Tegra maintainer, Thierry/Alexandre?
>>
>> Sorry about that, but I have split these changes out into their own
>> series after people complained about it.
>>
>> Have just sent a new version which already has Stephen's ack:
>>
>> https://lkml.kernel.org/g/1436862596-27730-1-git-send-email-tomeu.vizoso at collabora.com
>
> The change looks ok, but why limit it to Tegra? It seems like it could
> apply to any driver that supports gpio-range. Or is it because this
> will only work with drivers that use ordered probing?

It has to be SoC specific because we need to find out if a node
representing the in-SoC gpio controller is present, and has the
gpio-ranges property.

I guess we could move the code to the pinctrl core and parametrize the
property name but given that pinctrl_add_gpio_range is deprecated and
that we are doing this only for DT compatibility, I'm not sure it
would be a good idea.

Regards,

Tomeu

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Linus Walleij]

On Tue, Jul 14, 2015 at 10:34 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 13 July 2015 at 22:14, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
>> <tomeu.vizoso@collabora.com> wrote:
>>
>>> If the gpio DT node has the gpio-ranges property, the range will be
>>> added by the gpio core and doesn't need to be added by the pinctrl
>>> driver.
>>>
>>> By having the gpio-ranges property, we have an explicit dependency from
>>> the gpio node to the pinctrl node and we can stop using the deprecated
>>> pinctrl_add_gpio_range() function.
>>>
>>> Note that when the GPIO device gets probed before the associated
>>> princtrl device, the gpio core actually won't register the gpio range.
>>> Thus, this patch is only safe to be merged after we have in place a way
>>> to assure that gpio devices are probed after their associated pinctrl
>>> devices (such as ordered probing).
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> This doesn't look like it would hurt, but need Stephen's opinion
>> on it, and I think he's on vacation. Would check with next-in-line
>> Tegra maintainer, Thierry/Alexandre?
>
> Sorry about that, but I have split these changes out into their own
> series after people complained about it.
>
> Have just sent a new version which already has Stephen's ack:
>
> https://lkml.kernel.org/g/1436862596-27730-1-git-send-email-tomeu.vizoso at collabora.com

I don't see Stephen's ACK on them, where is it? It's not in the
patch and I don't see it as response to the patches.

Yours,
Linus Walleij

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Tomeu Vizoso]

On 17 July 2015 at 10:04, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jul 14, 2015 at 10:34 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> On 13 July 2015 at 22:14, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, Jun 17, 2015 at 3:42 PM, Tomeu Vizoso
>>> <tomeu.vizoso@collabora.com> wrote:
>>>
>>>> If the gpio DT node has the gpio-ranges property, the range will be
>>>> added by the gpio core and doesn't need to be added by the pinctrl
>>>> driver.
>>>>
>>>> By having the gpio-ranges property, we have an explicit dependency from
>>>> the gpio node to the pinctrl node and we can stop using the deprecated
>>>> pinctrl_add_gpio_range() function.
>>>>
>>>> Note that when the GPIO device gets probed before the associated
>>>> princtrl device, the gpio core actually won't register the gpio range.
>>>> Thus, this patch is only safe to be merged after we have in place a way
>>>> to assure that gpio devices are probed after their associated pinctrl
>>>> devices (such as ordered probing).
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>
>>> This doesn't look like it would hurt, but need Stephen's opinion
>>> on it, and I think he's on vacation. Would check with next-in-line
>>> Tegra maintainer, Thierry/Alexandre?
>>
>> Sorry about that, but I have split these changes out into their own
>> series after people complained about it.
>>
>> Have just sent a new version which already has Stephen's ack:
>>
>> https://lkml.kernel.org/g/1436862596-27730-1-git-send-email-tomeu.vizoso at collabora.com
>
> I don't see Stephen's ACK on them, where is it? It's not in the
> patch and I don't see it as response to the patches.

I see it in:

https://lkml.kernel.org/g/1436862596-27730-4-git-send-email-tomeu.vizoso at collabora.com

Which was given in:

https://lkml.kernel.org/g/559D8E36.50209 at wwwdotorg.org

But we still need an ack for patch 1/3 before both 2/3 and 3/3 can be
merged, because if we don't defer the probe when the pinctrl isn't
found and just skip adding the gpio-range, there won't be no
gpio-range set at all if the pinctrl device probes after the gpiochip
device. Merging 2/3 now would be fine as long as we don't merge 3/3
before 1/3.

Thanks,

Tomeu

[PATCH 04/13] pinctrl: tegra: Only set the gpio range if needed

[Linus Walleij]

On Fri, Jul 17, 2015 at 10:19 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 17 July 2015 at 10:04, Linus Walleij <linus.walleij@linaro.org> wrote:

>> I don't see Stephen's ACK on them, where is it? It's not in the
>> patch and I don't see it as response to the patches.
>
> I see it in:
>
> https://lkml.kernel.org/g/1436862596-27730-4-git-send-email-tomeu.vizoso at collabora.com
>
> Which was given in:
>
> https://lkml.kernel.org/g/559D8E36.50209 at wwwdotorg.org

Hm strange I only see v1, that's why I'm confused....

> But we still need an ack for patch 1/3 before both 2/3 and 3/3 can be
> merged, because if we don't defer the probe when the pinctrl isn't
> found and just skip adding the gpio-range, there won't be no
> gpio-range set at all if the pinctrl device probes after the gpiochip
> device. Merging 2/3 now would be fine as long as we don't merge 3/3
> before 1/3.

I'll hold it back for now, bug me when you think I can merge
1/3 and 2/3.

Yours,
Linus Walleij

[PATCH 05/13] driver core: fix docbook for device_private.device

[Tomeu Vizoso]

This field refers to the public device struct, not to classes.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/base.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index fd3347d..29c985e 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -63,7 +63,7 @@ struct driver_private {
  *	binding of drivers which were unable to get all the resources needed by
  *	the device; typically because it depends on another driver getting
  *	probed first.
- * @device - pointer back to the struct class that this structure is
+ * @device - pointer back to the struct device that this structure is
  * associated with.
  *
  * Nothing outside of the driver core should ever touch these fields.
-- 
2.4.1

[PATCH 06/13] of/platform: Set fwnode field for new devices

[Tomeu Vizoso]

When allocating a new platform device, set the fwnode field in the
struct device to point to the device_node.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/of/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ddf8e42..e880f55 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -139,6 +139,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	}
 
 	dev->dev.of_node = of_node_get(np);
+	dev->dev.fwnode = &dev->dev.of_node->fwnode;
 	dev->dev.parent = parent ? : &platform_bus;
 
 	if (bus_id)
-- 
2.4.1

[PATCH 06/13] of/platform: Set fwnode field for new devices

[Mark Brown]

On Wed, Jun 17, 2015 at 03:42:16PM +0200, Tomeu Vizoso wrote:

> When allocating a new platform device, set the fwnode field in the
> struct device to point to the device_node.

Why?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150617/65482507/attachment.sig>

[PATCH 07/13] driver-core: Add class.get_dependencies() callback

[Tomeu Vizoso]

Classes can implement this callback to provide a list of dependencies
for a firmware node. These dependencies can be used to probe devices in
order and to give proper warnings when dependencies cannot be fulfilled.

This functionality is implemented in a class callback because subsystems
implement the bindings that define how dependencies are expressed in the
firmware.

Adds struct fwnode_dependency, to be used to represent lists of
dependencies of fwnodes.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 include/linux/device.h | 6 ++++++
 include/linux/fwnode.h | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 5a31bf3..c52f290 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -367,6 +367,10 @@ int subsys_virtual_register(struct bus_type *subsys,
  * @dev_release: Called to release the device.
  * @suspend:	Used to put the device to sleep mode, usually to a low power
  *		state.
+ * @get_dependencies: Returns a list of struct fwnode_dependency containing
+ *		the dependencies of the passed firmware node. The class is
+ *		expected to inspect the firmware node and extract dependencies
+ *		from its properties, based on bindings documentation.
  * @resume:	Used to bring the device from the sleep mode.
  * @ns_type:	Callbacks so sysfs can detemine namespaces.
  * @namespace:	Namespace of the device belongs to this class.
@@ -397,6 +401,8 @@ struct class {
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
 
+	struct list_head *(*get_dependencies)(struct fwnode_handle *fwnode);
+
 	const struct kobj_ns_type_operations *ns_type;
 	const void *(*namespace)(struct device *dev);
 
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 0408545..68ab558 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -24,4 +24,9 @@ struct fwnode_handle {
 	struct fwnode_handle *secondary;
 };
 
+struct fwnode_dependency {
+	struct fwnode_handle *fwnode;
+	struct list_head dependency;
+};
+
 #endif
-- 
2.4.1

[PATCH 08/13] gpio: sysfs: implement class.get_dependencies()

[Tomeu Vizoso]

So the GPIO subsystem can be queried about the dependencies of nodes
that consume GPIOs, as specified in bindings/gpio/gpio.txt.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpio/gpiolib-sysfs.c | 81 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b57ed8e..d0a7fb1 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -7,6 +7,7 @@
 #include <linux/interrupt.h>
 #include <linux/kdev_t.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include "gpiolib.h"
 
@@ -515,6 +516,85 @@ done:
 	return status ? : len;
 }
 
+static bool strends(const char *str, const char *postfix)
+{
+	if (strlen(str) < strlen(postfix))
+		return false;
+
+	return strcmp(str + strlen(str) - strlen(postfix), postfix) == 0;
+}
+
+static void add_dependency(struct fwnode_handle *fwnode,
+			   struct list_head *list)
+{
+	struct fwnode_dependency *dep;
+
+	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
+	if (!dep)
+		return;
+
+	INIT_LIST_HEAD(&dep->dependency);
+	dep->fwnode = fwnode;
+
+	list_add_tail(&dep->dependency, list);
+}
+
+struct list_head *gpio_get_dependencies(struct fwnode_handle *fwnode)
+{
+	struct device_node *np = of_node(fwnode);
+	struct list_head *list = NULL;
+	struct property *pp;
+	struct of_phandle_args pspec;
+	int count, i, ret;
+
+	if (!is_of_node(fwnode))
+		return NULL;
+
+	np = of_node(fwnode);
+	if (!np)
+		return NULL;
+
+	list = kzalloc(sizeof(*list), GFP_KERNEL);
+	if (!list)
+		return NULL;
+
+	INIT_LIST_HEAD(list);
+
+	for_each_property_of_node(np, pp) {
+		if (strcmp(pp->name, "gpio") &&
+		    !strends(pp->name, "-gpios") &&
+		    !strends(pp->name, "-gpio"))
+			continue;
+
+		count = of_count_phandle_with_args(np, pp->name,
+						   "#gpio-cells");
+		for (i = 0; i < count; i++) {
+			ret = of_parse_phandle_with_args(np, pp->name,
+							 "#gpio-cells", i,
+							 &pspec);
+			if (ret || !pspec.np)
+				continue;
+
+			add_dependency(&pspec.np->fwnode, list);
+
+			of_node_put(pspec.np);
+		}
+	}
+
+	for (i = 0;; i++) {
+		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+						       i, &pspec);
+		if (ret)
+			break;
+
+		add_dependency(&pspec.np->fwnode, list);
+
+		of_node_put(pspec.np);
+	}
+
+	return list;
+}
+
 static struct class_attribute gpio_class_attrs[] = {
 	__ATTR(export, 0200, NULL, export_store),
 	__ATTR(unexport, 0200, NULL, unexport_store),
@@ -526,6 +606,7 @@ static struct class gpio_class = {
 	.owner =	THIS_MODULE,
 
 	.class_attrs =	gpio_class_attrs,
+	.get_dependencies = gpio_get_dependencies,
 };
 
 
-- 
2.4.1

[PATCH 08/13] gpio: sysfs: implement class.get_dependencies()

[Mark Brown]

On Wed, Jun 17, 2015 at 03:42:18PM +0200, Tomeu Vizoso wrote:

> +static bool strends(const char *str, const char *postfix)
> +{
> +	if (strlen(str) < strlen(postfix))
> +		return false;
> +
> +	return strcmp(str + strlen(str) - strlen(postfix), postfix) == 0;
> +}

This is named like (and looks like) a generic fuction, shouldn't it be
in string.h or something?

> +static void add_dependency(struct fwnode_handle *fwnode,
> +			   struct list_head *list)
> +{
> +	struct fwnode_dependency *dep;
> +
> +	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
> +	if (!dep)
> +		return;
> +
> +	INIT_LIST_HEAD(&dep->dependency);
> +	dep->fwnode = fwnode;
> +
> +	list_add_tail(&dep->dependency, list);
> +}

Might be worth putting this in generic code, it looks pretty generic?  I
have to say I'm unclear what frees the returned list.

> +	if (!is_of_node(fwnode))
> +		return NULL;
> +
> +	np = of_node(fwnode);
> +	if (!np)
> +		return NULL;

Presumably the first check could be dropped?

> +	list = kzalloc(sizeof(*list), GFP_KERNEL);
> +	if (!list)
> +		return NULL;

Might it make sense for the core to allocate the head of the list and
just ask the classes to add to the list?  We're going to want to merge
the dependencies from multiple subsystems and that saves allocating
heads that may never get anything added to them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150617/9551bc96/attachment.sig>

[PATCH 08/13] gpio: sysfs: implement class.get_dependencies()

[Tomeu Vizoso]

On 17 June 2015 at 19:40, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 17, 2015 at 03:42:18PM +0200, Tomeu Vizoso wrote:
>
>> +static bool strends(const char *str, const char *postfix)
>> +{
>> +     if (strlen(str) < strlen(postfix))
>> +             return false;
>> +
>> +     return strcmp(str + strlen(str) - strlen(postfix), postfix) == 0;
>> +}
>
> This is named like (and looks like) a generic fuction, shouldn't it be
> in string.h or something?

Yeah, will put it there.

>> +static void add_dependency(struct fwnode_handle *fwnode,
>> +                        struct list_head *list)
>> +{
>> +     struct fwnode_dependency *dep;
>> +
>> +     dep = kzalloc(sizeof(*dep), GFP_KERNEL);
>> +     if (!dep)
>> +             return;
>> +
>> +     INIT_LIST_HEAD(&dep->dependency);
>> +     dep->fwnode = fwnode;
>> +
>> +     list_add_tail(&dep->dependency, list);
>> +}
>
> Might be worth putting this in generic code, it looks pretty generic?  I
> have to say I'm unclear what frees the returned list.

Agreed.

>> +     if (!is_of_node(fwnode))
>> +             return NULL;
>> +
>> +     np = of_node(fwnode);
>> +     if (!np)
>> +             return NULL;
>
> Presumably the first check could be dropped?

That's right.

>> +     list = kzalloc(sizeof(*list), GFP_KERNEL);
>> +     if (!list)
>> +             return NULL;
>
> Might it make sense for the core to allocate the head of the list and
> just ask the classes to add to the list?  We're going to want to merge
> the dependencies from multiple subsystems and that saves allocating
> heads that may never get anything added to them.

Yes, I have gone with that advice and it looks better that way.

Thanks,

Tomeu

[PATCH 09/13] gpu: host1x: implement class.get_dependencies()

[Tomeu Vizoso]

So others can find out dependencies of host1x clients, as specified in
bindings/gpu/nvidia,tegra20-host1x.txt.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpu/host1x/dev.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 53d3d1d..c86ef88 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -212,10 +212,57 @@ static struct platform_driver tegra_host1x_driver = {
 	.remove = host1x_remove,
 };
 
+static void add_dependency(struct fwnode_handle *fwnode,
+			   const char *property,
+			   struct list_head *list)
+{
+	struct fwnode_dependency *dep;
+	struct device_node *np;
+
+	np = of_parse_phandle(of_node(fwnode), property, 0);
+	if (!np)
+		return;
+
+	dep = kzalloc(sizeof(*dep), GFP_KERNEL);
+	if (!dep)
+		return;
+
+	INIT_LIST_HEAD(&dep->dependency);
+	dep->fwnode = &np->fwnode;
+
+	list_add_tail(&dep->dependency, list);
+}
+
+struct list_head *host1x_get_dependencies(struct fwnode_handle *fwnode)
+{
+	struct list_head *list = NULL;
+
+	list = kzalloc(sizeof(*list), GFP_KERNEL);
+	if (!list)
+		return NULL;
+
+	INIT_LIST_HEAD(list);
+
+	add_dependency(fwnode, "nvidia,dpaux", list);
+	add_dependency(fwnode, "nvidia,panel", list);
+
+	return list;
+}
+
+static struct class host1x_class = {
+	.name = "tegra-host1x",
+
+	.get_dependencies = host1x_get_dependencies,
+};
+
 static int __init tegra_host1x_init(void)
 {
 	int err;
 
+	err = class_register(&host1x_class);
+	if (err < 0)
+		return err;
+
 	err = bus_register(&host1x_bus_type);
 	if (err < 0)
 		return err;
-- 
2.4.1

[PATCH 10/13] driver-core: add for_each_class()

[Tomeu Vizoso]

The purpose of this function is to allow other parts of the driver core
to iterate over the currently registered classes.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/base.h  |  2 ++
 drivers/base/class.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 29c985e..be81c88 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -149,3 +149,5 @@ extern int devtmpfs_init(void);
 #else
 static inline int devtmpfs_init(void) { return 0; }
 #endif
+
+extern void for_each_class(void (*fn)(struct class *, void *), void *data);
diff --git a/drivers/base/class.c b/drivers/base/class.c
index 6e81088..f4114a4 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -583,6 +583,22 @@ void class_compat_remove_link(struct class_compat *cls, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(class_compat_remove_link);
 
+void for_each_class(void (*fn)(struct class *, void *), void *data)
+{
+	struct subsys_private *cp;
+	struct kobject *k;
+
+	if (!class_kset)
+		return;
+
+	list_for_each_entry(k, &class_kset->list, entry) {
+		cp = to_subsys_private(k);
+		class_get(cp->class);
+		fn(cp->class, data);
+		class_put(cp->class);
+	}
+}
+
 int __init classes_init(void)
 {
 	class_kset = kset_create_and_add("class", NULL, NULL);
-- 
2.4.1

[PATCH 11/13] device property: add fwnode_get_parent()

[Tomeu Vizoso]

So we can query the parent of a fwnode without having to resort to API
that is specific to a firmware data format.

Also adds a acpi_get_parent_dev() function to retrieve the parent
of an acpi_device. acpi_get_parent() already existed but it works with
acpi_handles.

The interface covers both ACPI and Device Trees.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/property.c  | 23 +++++++++++++++++++++++
 include/acpi/acpi_bus.h  |  5 +++++
 include/linux/acpi.h     |  5 +++++
 include/linux/property.h |  2 ++
 4 files changed, 35 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 1d0b116..28645a9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -519,3 +519,26 @@ unsigned int device_get_child_node_count(struct device *dev)
 	return count;
 }
 EXPORT_SYMBOL_GPL(device_get_child_node_count);
+
+/**
+ * fwnode_get_parent - return the parent node of a device node
+ * @fwnode: Device node to find the parent node of
+ */
+struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode)
+{
+	if (is_of_node(fwnode)) {
+		struct device_node *node = of_node(fwnode);
+
+		if (node->parent)
+			return &node->parent->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		struct acpi_device *node;
+
+		node = acpi_get_parent_dev(acpi_node(fwnode));
+		if (node)
+			return acpi_fwnode_handle(node);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_parent);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 5dec08d..bf8ef1a 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -401,6 +401,11 @@ static inline void *acpi_driver_data(struct acpi_device *d)
 	return d->driver_data;
 }
 
+static inline struct acpi_device *acpi_get_parent_dev(struct acpi_device *adev)
+{
+	return adev->parent;
+}
+
 #define to_acpi_device(d)	container_of(d, struct acpi_device, dev)
 #define to_acpi_driver(d)	container_of(d, struct acpi_driver, drv)
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a4acb55..d9dfacc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -825,6 +825,11 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev,
 	return NULL;
 }
 
+static inline struct acpi_device *acpi_get_parent_dev(struct acpi_device *adev)
+{
+	return NULL;
+}
+
 #endif
 
 #endif	/*_LINUX_ACPI_H*/
diff --git a/include/linux/property.h b/include/linux/property.h
index de8bdf4..01e0483 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -63,6 +63,8 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode,
 struct fwnode_handle *device_get_next_child_node(struct device *dev,
 						 struct fwnode_handle *child);
 
+struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
+
 #define device_for_each_child_node(dev, child) \
 	for (child = device_get_next_child_node(dev, NULL); child; \
 	     child = device_get_next_child_node(dev, child))
-- 
2.4.1

[PATCH 12/13] device property: add fwnode_get_name()

[Tomeu Vizoso]

Getting a textual representation of a device node can be very useful for
debugging.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/property.c  | 15 +++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 28645a9..05e45f8 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -542,3 +542,18 @@ struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode)
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_parent);
+
+/**
+ * fwnode_get_name - return the name of a device node
+ * @fwnode: Device node to find the name of
+ */
+const char *fwnode_get_name(struct fwnode_handle *fwnode)
+{
+	if (is_of_node(fwnode))
+		return of_node(fwnode)->full_name;
+	else if (is_acpi_node(fwnode))
+		return acpi_dev_name(acpi_node(fwnode));
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_name);
diff --git a/include/linux/property.h b/include/linux/property.h
index 01e0483..7b7c0e8 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -65,6 +65,8 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
 
 struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
 
+const char *fwnode_get_name(struct fwnode_handle *fwnode);
+
 #define device_for_each_child_node(dev, child) \
 	for (child = device_get_next_child_node(dev, NULL); child; \
 	     child = device_get_next_child_node(dev, child))
-- 
2.4.1

[PATCH 13/13] driver-core: probe dependencies before probing

[Tomeu Vizoso]

Before actually probing a device, find out what dependencies it has and
do our best to ensure that they are available at this point.

This is accomplished by finding out what platform devices need to be
probed so the dependencies are available.

If any dependencies are still unavailable after that (most probably a
missing driver or an error in the HW description from the firmware), we
print a nice error message so that people don't have to add a zillion of
printks to find out why a device asked for its probe to be deferred.

Dependencies are discovered with the help of the subsystems that already
implement the firmware bindings that specify the naming scheme of
properties that point to other nodes, via class.get_deps().

Currently the dependencies list is discarded but it could be stored for
later usage.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/dd.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 18438aa..8a64a29 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,9 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -54,6 +57,121 @@ static LIST_HEAD(deferred_probe_active_list);
 static struct workqueue_struct *deferred_wq;
 static atomic_t deferred_trigger_count = ATOMIC_INIT(0);
 
+static bool device_is_bound(struct device *dev)
+{
+	return klist_node_attached(&dev->p->knode_driver);
+}
+
+static int fwnode_match(struct device *dev, void *data)
+{
+	return dev->fwnode == data;
+}
+
+static bool fwnode_is_bound(struct fwnode_handle *fwnode)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&platform_bus_type, NULL, fwnode, fwnode_match);
+
+	/* Check whether device is bound or is being probed right now */
+	return dev ? dev->driver : false;
+}
+
+static struct fwnode_handle *get_enclosing_platform_dev(
+						struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *iter, *node = NULL;
+
+	for (iter = fwnode;
+	     iter && fwnode_get_parent(iter);
+	     iter = fwnode_get_parent(iter)) {
+		struct fwnode_handle *parent = fwnode_get_parent(iter);
+
+		/* Only nodes with the compatible property can be probed */
+		if (fwnode_property_present(iter, "compatible"))
+			node = iter;
+
+		/* Stop just below root or if the parent is bound already */
+		if (!fwnode_get_parent(parent) ||
+		    fwnode_is_bound(parent))
+			break;
+	}
+
+	return node;
+}
+
+static bool check_dependency(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *target;
+	struct device *dev;
+
+	if (!fwnode)
+		return true;
+
+	target = get_enclosing_platform_dev(fwnode);
+	if (!target)
+		return true;
+
+	dev = bus_find_device(&platform_bus_type, NULL, target, fwnode_match);
+	if (!dev) {
+		pr_debug("Couldn't find device for %s\n",
+			 fwnode_get_name(fwnode));
+		return false;
+	}
+
+	/*
+	 * Device is bound or is being probed right now. If we have bad luck
+	 * and the dependency isn't ready when it's needed, deferred probe
+	 * will save us.
+	 */
+	if (dev->driver)
+		return true;
+
+	bus_probe_device(dev);
+
+	/* If the dependency hasn't finished probing, we'll want a warning */
+	return device_is_bound(dev);
+}
+
+static void check_dependencies_per_class(struct class *class, void *data)
+{
+	struct fwnode_handle *fwnode = data;
+	struct list_head *deps;
+	struct fwnode_dependency *dep, *tmp;
+
+	if (!class->get_dependencies)
+		return;
+
+	deps = class->get_dependencies(fwnode);
+	if (!deps)
+		return;
+
+	list_for_each_entry_safe(dep, tmp, deps, dependency) {
+		if (!check_dependency(dep->fwnode))
+			pr_debug("Dependency '%s' not available\n",
+				 fwnode_get_name(dep->fwnode));
+
+		list_del(&dep->dependency);
+		kfree(dep);
+	}
+
+	kfree(deps);
+}
+
+static void check_dependencies(struct device *dev)
+{
+	if (dev->parent && !check_dependency(dev->parent->fwnode))
+		pr_debug("Parent '%s' of device '%s' not available\n",
+			 dev_name(dev->parent), dev_name(dev));
+
+	if (!dev->fwnode) {
+		pr_debug("Device '%s' doesn't have a fwnode\n", dev_name(dev));
+		return;
+	}
+
+	for_each_class(check_dependencies_per_class, dev->fwnode);
+}
+
 /*
  * deferred_probe_work_func() - Retry probing devices in the active list.
  */
@@ -413,6 +531,8 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
 		return 0;
 	}
 
+	check_dependencies(dev);
+
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
-- 
2.4.1

[PATCH 13/13] driver-core: probe dependencies before probing

[Mark Brown]

On Wed, Jun 17, 2015 at 03:42:23PM +0200, Tomeu Vizoso wrote:
> Before actually probing a device, find out what dependencies it has and
> do our best to ensure that they are available at this point.

> This is accomplished by finding out what platform devices need to be
> probed so the dependencies are available.

...and then trying to probe them first.

> If any dependencies are still unavailable after that (most probably a
> missing driver or an error in the HW description from the firmware), we
> print a nice error message so that people don't have to add a zillion of
> printks to find out why a device asked for its probe to be deferred.

So, I think I like this approach though I've not done a full pass
through and I'm not sure how expensive it gets (there's definitely room
for optimisation as the patch notes).  I'm not 100% sure I see what
prints this error message you're referring to (I'm just seeing debug
prints).

> +static struct fwnode_handle *get_enclosing_platform_dev(
> +						struct fwnode_handle *fwnode)

Only platform devices?

> +static void check_dependencies_per_class(struct class *class, void *data)
> +{
> +	struct fwnode_handle *fwnode = data;
> +	struct list_head *deps;
> +	struct fwnode_dependency *dep, *tmp;
> +
> +	if (!class->get_dependencies)
> +		return;
> +
> +	deps = class->get_dependencies(fwnode);
> +	if (!deps)
> +		return;
> +
> +	list_for_each_entry_safe(dep, tmp, deps, dependency) {
> +		if (!check_dependency(dep->fwnode))
> +			pr_debug("Dependency '%s' not available\n",
> +				 fwnode_get_name(dep->fwnode));
> +
> +		list_del(&dep->dependency);
> +		kfree(dep);
> +	}
> +
> +	kfree(deps);

OK, so the caller is responsible for freeing everything and the class
must allocate - this definitely suggests that 

I'm not sure there's any benefit in having deps be dynamically allocated
here, just put it on the stack and iterate through the list - the
iteration is going to be cheap if we get nothing back (probably the
common case) and probably cheaper than the alloc/free.

One thing here is that I was under the impression classes were supposed
to be going away...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150617/2865540c/attachment.sig>

[PATCH 13/13] driver-core: probe dependencies before probing

[Tomeu Vizoso]

On 17 June 2015 at 20:13, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 17, 2015 at 03:42:23PM +0200, Tomeu Vizoso wrote:
>> Before actually probing a device, find out what dependencies it has and
>> do our best to ensure that they are available at this point.
>
>> This is accomplished by finding out what platform devices need to be
>> probed so the dependencies are available.
>
> ...and then trying to probe them first.
>
>> If any dependencies are still unavailable after that (most probably a
>> missing driver or an error in the HW description from the firmware), we
>> print a nice error message so that people don't have to add a zillion of
>> printks to find out why a device asked for its probe to be deferred.
>
> So, I think I like this approach though I've not done a full pass
> through and I'm not sure how expensive it gets (there's definitely room
> for optimisation as the patch notes).

Have measured it and the overhead doesn't seem to be much, in the
version that I'm close to send.

> I'm not 100% sure I see what
> prints this error message you're referring to (I'm just seeing debug
> prints).

Right, so far I have left them as debug messages because I have so far
tested the series on just one platform and I'm not sure if there
wouldn't be lots of noise in others.

>> +static struct fwnode_handle *get_enclosing_platform_dev(
>> +                                             struct fwnode_handle *fwnode)
>
> Only platform devices?

Yes, this code assumes that devices on other buses will be registered
and probed when their enclosing platform devices are.

>> +static void check_dependencies_per_class(struct class *class, void *data)
>> +{
>> +     struct fwnode_handle *fwnode = data;
>> +     struct list_head *deps;
>> +     struct fwnode_dependency *dep, *tmp;
>> +
>> +     if (!class->get_dependencies)
>> +             return;
>> +
>> +     deps = class->get_dependencies(fwnode);
>> +     if (!deps)
>> +             return;
>> +
>> +     list_for_each_entry_safe(dep, tmp, deps, dependency) {
>> +             if (!check_dependency(dep->fwnode))
>> +                     pr_debug("Dependency '%s' not available\n",
>> +                              fwnode_get_name(dep->fwnode));
>> +
>> +             list_del(&dep->dependency);
>> +             kfree(dep);
>> +     }
>> +
>> +     kfree(deps);
>
> OK, so the caller is responsible for freeing everything and the class
> must allocate - this definitely suggests that
>
> I'm not sure there's any benefit in having deps be dynamically allocated
> here, just put it on the stack and iterate through the list - the
> iteration is going to be cheap if we get nothing back (probably the
> common case) and probably cheaper than the alloc/free.

Have done this and I like it more.

> One thing here is that I was under the impression classes were supposed
> to be going away...

Actually, while looking at more firmware node properties to parse for
dependencies, I found a rather common case in which the bindings are
implemented by individual drivers and not subsystems. Some examples
are nvidia,dpaux, nvidia,panel and nvidia,ddc-i2c-bus.

So in my next version I have dropped class callbacks and have gone
with a way for classes, drivers, whatever to just register a function
to extract dependencies.

Thanks,

Tomeu

[PATCH 00/13] Discover and probe dependencies

[Andrzej Hajda]

Hi Tomeu,

I have few comments about the design.

On 06/17/2015 03:42 PM, Tomeu Vizoso wrote:
> Hi,
>
> this is another attempt at preventing deferred probe from obscuring why your
> devices aren't probing and from delaying to the end of the boot process the
> probe of the device you care the most.
>
> The major differences with my previous approach [0] are:
>
> * Dependencies are probed before the target is probed, so we don't have nested
>   probe() calls, avoiding a series of deadlock situations.
>
> * Dependencies could be stored and reused for other purposes such as for
>   passing resources to drivers ala devm_probe, or for warning when a device is
>   going to be unbound and has dependencies active, etc.

With this approach we should assume many things, for example:
1. Dependencies are explicitly described in firmware (dts/dtb).
    It will not work for example with lookup tables present in
gpios/clocks/regulators.
2. Provider create/register their resources only during probe.
    It is not always the case - for example componentized drivers in
probe often
    calls only component_add, the real initialization is performed in
bind callback.
3. Dependencies are mandatory, ie without it driver will not be able to
successfully finish
the probe.
    It should be not true. Sometimes device will require given resource
only in specific
    scenario, or it can still probe successfully and ask for the
resource later.
    I can also imagine that firmware can describe more information than
given driver require,
    some resources even if they are present in the dts, will be not
requested by the driver, it
    can be the case of drivers providing limited functionality, or just
obsolete bindings.
  

I have also more general design objection, which should not be
necessarily true:
Device node describes piece of the hardware which should be mainly
interpreted by the driver.
Parsing it in external code [1] violates this idea. Additionally we will
have the same information
parsed and interpreted in two different places (discovery framework and
the driver),
it does not look good to me.
But as I said earlier it is just my opinion, not a solid evidence :)

[1]: I know that for example clk_get is also located in external
framework but currently the driver
decides that it should call clk_get, my_private_clk_get,
other_framework_clk_get or do not call it at all,
this framework assumes that it will be always clk_get, or at least
something compatible with it at binding level.

Regards
Andrzej


>
> * I have tried to keep it firmware-agnostic. The previous approach (on-demand
>   probing) could be done like this as well, but would require adding fwnode
>   APIs to all affected subsystems first.
>
> I have only implemented the class.get_dependencies() callback for the GPIO
> subsystem and for the host1x bus because that's all that was needed on my Tegra
> Chromebook to avoid deferred probes, but if this approach is deemed worthwhile
> I will add more implementations so that deferred probes are avoided on the
> other boards I have access to.
>
> [0] http://thread.gmane.org/gmane.linux.kernel.gpio/8465
>
> Thanks,
>
> Tomeu
>
> Tomeu Vizoso (13):
>   gpiolib: Fix docs for gpiochip_add_pingroup_range
>   driver-core: defer all probes until late_initcall
>   ARM: tegra: Add gpio-ranges property
>   pinctrl: tegra: Only set the gpio range if needed
>   driver core: fix docbook for device_private.device
>   of/platform: Set fwnode field for new devices
>   driver-core: Add class.get_dependencies() callback
>   gpio: sysfs: implement class.get_dependencies()
>   gpu: host1x: implement class.get_dependencies()
>   driver-core: add for_each_class()
>   device property: add fwnode_get_parent()
>   device property: add fwnode_get_name()
>   driver-core: probe dependencies before probing
>
>  arch/arm/boot/dts/tegra114.dtsi |   1 +
>  arch/arm/boot/dts/tegra124.dtsi |   1 +
>  arch/arm/boot/dts/tegra20.dtsi  |   1 +
>  arch/arm/boot/dts/tegra30.dtsi  |   1 +
>  drivers/base/base.h             |   4 +-
>  drivers/base/class.c            |  16 +++++
>  drivers/base/dd.c               | 128 +++++++++++++++++++++++++++++++++++++++-
>  drivers/base/property.c         |  38 ++++++++++++
>  drivers/gpio/gpiolib-sysfs.c    |  81 +++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c          |   2 +-
>  drivers/gpu/host1x/dev.c        |  47 +++++++++++++++
>  drivers/of/platform.c           |   1 +
>  drivers/pinctrl/pinctrl-tegra.c |  19 +++++-
>  include/acpi/acpi_bus.h         |   5 ++
>  include/linux/acpi.h            |   5 ++
>  include/linux/device.h          |   6 ++
>  include/linux/fwnode.h          |   5 ++
>  include/linux/property.h        |   4 ++
>  18 files changed, 361 insertions(+), 4 deletions(-)
>

[PATCH 00/13] Discover and probe dependencies

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 11:42:01AM +0200, Andrzej Hajda wrote:
> 2. Provider create/register their resources only during probe.
>     It is not always the case - for example componentized drivers in
> probe often
>     calls only component_add, the real initialization is performed in
> bind callback.

bind is called in _a_ probe callback, but it may not be the probe callback
associated with the device.  (It'll be the final component's callback.)

I'm willing to be less critical of componentised drivers claiming their
resources in ->probe _iff_ they separate their data structures, like:

	struct foo_priv {
		void *base;
		struct clk *clk;
		struct dma_chan *chan;
		... other resource data ...;
		struct foo_runtime {
			... runtime data ...
		} rt;
	};

and then, in their bind callback, they memset the foo_runtime structure
to zero, to ensure that the driver always re-binds in the same state as
the first bind.

I've seen too many lax drivers over the years that this is a point I'm
very insistant on: either componentised drivers don't do any resource
claiming in their probe function, or they take steps to ensure non-
resource struct members are properly separated such that they can
guarantee that they aren't going to accidentally use something from a
previous binding.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 00/13] Discover and probe dependencies

[Mark Brown]

On Thu, Jun 18, 2015 at 11:42:01AM +0200, Andrzej Hajda wrote:

There's something really messed up with how your mailer is word wrapping
paragraphs, might want to check it out - it looks like it's making lines
that are longer than 80 columns and then randomly breaking them 80
columns in rather than reflowing things.

> 3. Dependencies are mandatory, ie without it driver will not be able to
> successfully finish
> the probe.
>     It should be not true. Sometimes device will require given resource
> only in specific
>     scenario, or it can still probe successfully and ask for the
> resource later.
>     I can also imagine that firmware can describe more information than
> given driver require,
>     some resources even if they are present in the dts, will be not
> requested by the driver, it
>     can be the case of drivers providing limited functionality, or just
> obsolete bindings.

Well, this isn't clear - the model here seems to be that the
dependencies are those that are explicitly listed for a given platform.
For those our current expectation is that we will try to control them
and will defer probe until the resources that are mapped in on the
platform are present so this doesn't seem like a change.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150618/c2c55b2b/attachment-0001.sig>

[PATCH 00/13] Discover and probe dependencies

[Andrzej Hajda]

On 06/18/2015 12:36 PM, Mark Brown wrote:
> On Thu, Jun 18, 2015 at 11:42:01AM +0200, Andrzej Hajda wrote:
>
> There's something really messed up with how your mailer is word wrapping
> paragraphs, might want to check it out - it looks like it's making lines
> that are longer than 80 columns and then randomly breaking them 80
> columns in rather than reflowing things.
>
>> 3. Dependencies are mandatory, ie without it driver will not be able to
>> successfully finish
>> the probe.
>>     It should be not true. Sometimes device will require given resource
>> only in specific
>>     scenario, or it can still probe successfully and ask for the
>> resource later.
>>     I can also imagine that firmware can describe more information than
>> given driver require,
>>     some resources even if they are present in the dts, will be not
>> requested by the driver, it
>>     can be the case of drivers providing limited functionality, or just
>> obsolete bindings.
> Well, this isn't clear - the model here seems to be that the
> dependencies are those that are explicitly listed for a given platform.
> For those our current expectation is that we will try to control them
> and will defer probe until the resources that are mapped in on the
> platform are present so this doesn't seem like a change.
IMO current assumption is that we CAN TRY to control them
and we MAY defer probe until resource is present, nothing mandatory.

Lets look at more real example: we have HDMI encoder which can
use some video and audio resources provided by some video and audio
drivers. If we know that our machine will work without sound we can
disable audio drivers but we can expect video should still work, ie HDMI
driver should successfully probe even if audio resources are not available.

I had an impression that in this patchset the device wont be probed
until all dependencies are present, but after looking at the code it
does not seems to be true - in case dependency cannot be probed
warning will be printed but the probe will continue, so it should handle
scenario above properly. The only minor issue is that we will see
sometimes misleading message about missing dependencies.

Regards
Andrzej

[PATCH 00/13] Discover and probe dependencies

[Tomeu Vizoso]

On 18 June 2015 at 15:14, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 06/18/2015 12:36 PM, Mark Brown wrote:
>> On Thu, Jun 18, 2015 at 11:42:01AM +0200, Andrzej Hajda wrote:
>>
>> There's something really messed up with how your mailer is word wrapping
>> paragraphs, might want to check it out - it looks like it's making lines
>> that are longer than 80 columns and then randomly breaking them 80
>> columns in rather than reflowing things.
>>
>>> 3. Dependencies are mandatory, ie without it driver will not be able to
>>> successfully finish
>>> the probe.
>>>     It should be not true. Sometimes device will require given resource
>>> only in specific
>>>     scenario, or it can still probe successfully and ask for the
>>> resource later.
>>>     I can also imagine that firmware can describe more information than
>>> given driver require,
>>>     some resources even if they are present in the dts, will be not
>>> requested by the driver, it
>>>     can be the case of drivers providing limited functionality, or just
>>> obsolete bindings.
>> Well, this isn't clear - the model here seems to be that the
>> dependencies are those that are explicitly listed for a given platform.
>> For those our current expectation is that we will try to control them
>> and will defer probe until the resources that are mapped in on the
>> platform are present so this doesn't seem like a change.
> IMO current assumption is that we CAN TRY to control them
> and we MAY defer probe until resource is present, nothing mandatory.
>
> Lets look at more real example: we have HDMI encoder which can
> use some video and audio resources provided by some video and audio
> drivers. If we know that our machine will work without sound we can
> disable audio drivers but we can expect video should still work, ie HDMI
> driver should successfully probe even if audio resources are not available.
>
> I had an impression that in this patchset the device wont be probed
> until all dependencies are present, but after looking at the code it
> does not seems to be true - in case dependency cannot be probed
> warning will be printed but the probe will continue, so it should handle
> scenario above properly.

Yeah, I think deferred probe is a very good way to make sure that we
get a working system at the end. It's just that as we probe devices in
a random order, we'd get a lot of noise if we printed something every
time a device failed to find a resource and deferred its probe.

You can see these series as just changing the order in which we probe
devices, to one in which dependencies are probed before the device
that depends on them.

> The only minor issue is that we will see
> sometimes misleading message about missing dependencies.

Yes, that's why they are currently pr_debug and not pr_warn. I think
that once we have tables specifying the resources needed by each
driver (ala devm_probe), we can know which are optional and print
appropriate warnings accordingly.

Thanks,

Tomeu

> Regards
> Andrzej
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 00/13] Discover and probe dependencies

[Russell King - ARM Linux]

On Thu, Jun 18, 2015 at 03:14:31PM +0200, Andrzej Hajda wrote:
> Lets look at more real example: we have HDMI encoder which can
> use some video and audio resources provided by some video and audio
> drivers. If we know that our machine will work without sound we can
> disable audio drivers but we can expect video should still work, ie
> HDMI driver should successfully probe even if audio resources are
> not available.

That already happens today, if you structure the driver appropriately.
I really don't buy into the crap argument that "all struct device must
be created by DT" - it is perfectly acceptable for a device driver to
declare and register its own sub-devices where it's appropraite for it
to do so.

Since audio _requires_ video on HDMI to work (audio fundamentally
depends on a working video setup), it is perfectly acceptable for a
HDMI video driver to register a struct device for its audio driver,
and to pass details that the audio driver may need.

What is not acceptable is to duplicate the HDMI drivers device_node
into the child devices: this creates a situation where the generic
device model can match the HDMI driver with its child device.  So
this is a big no no.

The model I refer to above is something which I have, and others have
implemented for HDMI devices, and it's a completely reasonable model.

Remember, DT is about describing the hardware.  If you have a HDMI
encoder, that's one hardware block, and there should be one entry
describing it in DT.  Just because in Linux we may decide to separate
them into two separate drivers, and therefore two separate struct
device's is an implementation detail, and is not a reason to medle
with the hardware model in DT.

I think you may need to pick a better example to illustrate your point. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 00/13] Discover and probe dependencies

[Alexander Holler]

Am 18.06.2015 um 16:49 schrieb Russell King - ARM Linux:

> I think you may need to pick a better example to illustrate your point. :)

It's easy to describe: Sort driver probe calls and forget about the 
whole device-stuff.

The real target is to sort the driver probe calls and ideally the whole 
device stuff could be ignored, leaving it as however it may work.

Unfortunately, that's not as easy to implement as it sounds. ;)

I've too hit the device-stuff very late in my driver-probe-ordering 
experiment and just did it somehow, because I already was at the limit 
of the time I wanted to spend. ;)

So to repeat myself, I think the first target has to be to identify 
drivers either without having to call their initcalls at all, or by 
making sure their initcalls just register and do nothing else (what I've 
called "well-done").

Regards,

Alexander Holler

[PATCH 00/13] Discover and probe dependencies

[Tomeu Vizoso]

On 18 June 2015 at 11:42, Andrzej Hajda <a.hajda@samsung.com> wrote:
> Hi Tomeu,
>
> I have few comments about the design.
>
> On 06/17/2015 03:42 PM, Tomeu Vizoso wrote:
>> Hi,
>>
>> this is another attempt at preventing deferred probe from obscuring why your
>> devices aren't probing and from delaying to the end of the boot process the
>> probe of the device you care the most.
>>
>> The major differences with my previous approach [0] are:
>>
>> * Dependencies are probed before the target is probed, so we don't have nested
>>   probe() calls, avoiding a series of deadlock situations.
>>
>> * Dependencies could be stored and reused for other purposes such as for
>>   passing resources to drivers ala devm_probe, or for warning when a device is
>>   going to be unbound and has dependencies active, etc.
>
> With this approach we should assume many things, for example:
> 1. Dependencies are explicitly described in firmware (dts/dtb).
>     It will not work for example with lookup tables present in
> gpios/clocks/regulators.

Yes, but my understanding is that we are moving towards having the hw
described in fwnode (so DT, ACPI and board files), and these
dependencies are part of the hw description.

> 2. Provider create/register their resources only during probe.
>     It is not always the case - for example componentized drivers in
> probe often
>     calls only component_add, the real initialization is performed in
> bind callback.

We don't really try to bind the actual provider, but the platform
device from which it derives, assuming that once that platform device
finishes probing, the actual provider will have been probed as well.

>From what I gather from reading drivers/of/platform.c this assumption
has to hold for DT-based machines, but I'm not so sure about others.

> 3. Dependencies are mandatory, ie without it driver will not be able to
> successfully finish
> the probe.
>     It should be not true. Sometimes device will require given resource
> only in specific
>     scenario, or it can still probe successfully and ask for the
> resource later.
>     I can also imagine that firmware can describe more information than
> given driver require,
>     some resources even if they are present in the dts, will be not
> requested by the driver, it
>     can be the case of drivers providing limited functionality, or just
> obsolete bindings.
>
>
> I have also more general design objection, which should not be
> necessarily true:
> Device node describes piece of the hardware which should be mainly
> interpreted by the driver.
> Parsing it in external code [1] violates this idea. Additionally we will
> have the same information
> parsed and interpreted in two different places (discovery framework and
> the driver),
> it does not look good to me.
> But as I said earlier it is just my opinion, not a solid evidence :)

Yeah, I see that concern, but actually the code that interprets
dependencies are the subsystem cores, not the drivers themselves.

It's true that this approach introduces some code duplication that the
on-demand series doesn't, but I'm not sure it would be a clear win to
refactor that duplication away.

> [1]: I know that for example clk_get is also located in external
> framework but currently the driver
> decides that it should call clk_get, my_private_clk_get,
> other_framework_clk_get or do not call it at all,
> this framework assumes that it will be always clk_get, or at least
> something compatible with it at binding level.

Yeah, in this series I assume that if the gpio bindings say that
phandles in properties with names ending in -gpios, then any phandles
in properties with that name scheme should point to gpiochips.

I have done some grepping and for this and all the other generic
subsystems this seems to hold true (there aren't properties that
follow any of those name schemes and that contain some other
information).

Regards,

Tomeu

> Regards
> Andrzej
>
>
>>
>> * I have tried to keep it firmware-agnostic. The previous approach (on-demand
>>   probing) could be done like this as well, but would require adding fwnode
>>   APIs to all affected subsystems first.
>>
>> I have only implemented the class.get_dependencies() callback for the GPIO
>> subsystem and for the host1x bus because that's all that was needed on my Tegra
>> Chromebook to avoid deferred probes, but if this approach is deemed worthwhile
>> I will add more implementations so that deferred probes are avoided on the
>> other boards I have access to.
>>
>> [0] http://thread.gmane.org/gmane.linux.kernel.gpio/8465
>>
>> Thanks,
>>
>> Tomeu
>>
>> Tomeu Vizoso (13):
>>   gpiolib: Fix docs for gpiochip_add_pingroup_range
>>   driver-core: defer all probes until late_initcall
>>   ARM: tegra: Add gpio-ranges property
>>   pinctrl: tegra: Only set the gpio range if needed
>>   driver core: fix docbook for device_private.device
>>   of/platform: Set fwnode field for new devices
>>   driver-core: Add class.get_dependencies() callback
>>   gpio: sysfs: implement class.get_dependencies()
>>   gpu: host1x: implement class.get_dependencies()
>>   driver-core: add for_each_class()
>>   device property: add fwnode_get_parent()
>>   device property: add fwnode_get_name()
>>   driver-core: probe dependencies before probing
>>
>>  arch/arm/boot/dts/tegra114.dtsi |   1 +
>>  arch/arm/boot/dts/tegra124.dtsi |   1 +
>>  arch/arm/boot/dts/tegra20.dtsi  |   1 +
>>  arch/arm/boot/dts/tegra30.dtsi  |   1 +
>>  drivers/base/base.h             |   4 +-
>>  drivers/base/class.c            |  16 +++++
>>  drivers/base/dd.c               | 128 +++++++++++++++++++++++++++++++++++++++-
>>  drivers/base/property.c         |  38 ++++++++++++
>>  drivers/gpio/gpiolib-sysfs.c    |  81 +++++++++++++++++++++++++
>>  drivers/gpio/gpiolib.c          |   2 +-
>>  drivers/gpu/host1x/dev.c        |  47 +++++++++++++++
>>  drivers/of/platform.c           |   1 +
>>  drivers/pinctrl/pinctrl-tegra.c |  19 +++++-
>>  include/acpi/acpi_bus.h         |   5 ++
>>  include/linux/acpi.h            |   5 ++
>>  include/linux/device.h          |   6 ++
>>  include/linux/fwnode.h          |   5 ++
>>  include/linux/property.h        |   4 ++
>>  18 files changed, 361 insertions(+), 4 deletions(-)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/