diff mbox

[v4,04/22] of: add function to allow probing a device from a OF node

Message ID 1441628627-5143-5-git-send-email-tomeu.vizoso@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomeu Vizoso Sept. 7, 2015, 12:23 p.m. UTC
Walks the OF tree up and finds the closest ancestor that has a struct
device associated with it, probing it if isn't bound to a driver yet.

The above should ensure that the dependency represented by the passed OF
node is available, because probing a device should cause its descendants
to be probed as well (when they get registered).

Subsystems can use this when looking up resources for drivers, to reduce
the chances of deferred probes because of the probing order of devices.

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

Changes in v4:
- Rename of_platform_probe to of_device_probe
- Use device_node.device instead of device_node.platform_dev

Changes in v3:
- Set and use device_node.platform_dev instead of reversing the logic to
  find the platform device that encloses a device node.
- Drop the fwnode API to probe firmware nodes and add OF-only API for
  now. I think this same scheme could be used for machines with ACPI,
  but I haven't been able to find one that had to defer its probes because
  of the device probe order.

Changes in v2: None

 drivers/of/device.c       | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c     |  2 ++
 include/linux/of_device.h |  3 +++
 3 files changed, 63 insertions(+)

Comments

Rob Herring (Arm) Sept. 9, 2015, 1:29 a.m. UTC | #1
On 09/07/2015 07:23 AM, Tomeu Vizoso wrote:
> Walks the OF tree up and finds the closest ancestor that has a struct
> device associated with it, probing it if isn't bound to a driver yet.
> 
> The above should ensure that the dependency represented by the passed OF
> node is available, because probing a device should cause its descendants
> to be probed as well (when they get registered).
> 
> Subsystems can use this when looking up resources for drivers, to reduce
> the chances of deferred probes because of the probing order of devices.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Looks pretty good to me. One comment below.

[...]

> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index baf04d7249bd..f089d95ac961 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -269,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>  		goto err_free;
>  	}
>  
> +	node->device = &dev->dev;
> +

This seems oddly placed. Can you move to patch 3?

>  	return dev;
>  
>  err_free:
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index cc7dd687a89d..da8d489e73ad 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -40,6 +40,7 @@ extern ssize_t of_device_get_modalias(struct device *dev,
>  
>  extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
>  extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
> +extern void of_device_probe(struct device_node *np);
>  
>  static inline void of_device_node_put(struct device *dev)
>  {
> @@ -84,6 +85,8 @@ static inline int of_device_uevent_modalias(struct device *dev,
>  	return -ENODEV;
>  }
>  
> +static inline void of_device_probe(struct device_node *np) { }
> +
>  static inline void of_device_node_put(struct device *dev) { }
>  
>  static inline const struct of_device_id *__of_match_device(
>
Tomeu Vizoso Sept. 10, 2015, 2:53 p.m. UTC | #2
On 9 September 2015 at 03:29, Rob Herring <robh@kernel.org> wrote:
> On 09/07/2015 07:23 AM, Tomeu Vizoso wrote:
>> Walks the OF tree up and finds the closest ancestor that has a struct
>> device associated with it, probing it if isn't bound to a driver yet.
>>
>> The above should ensure that the dependency represented by the passed OF
>> node is available, because probing a device should cause its descendants
>> to be probed as well (when they get registered).
>>
>> Subsystems can use this when looking up resources for drivers, to reduce
>> the chances of deferred probes because of the probing order of devices.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> Looks pretty good to me. One comment below.
>
> [...]
>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index baf04d7249bd..f089d95ac961 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -269,6 +269,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>>               goto err_free;
>>       }
>>
>> +     node->device = &dev->dev;
>> +
>
> This seems oddly placed. Can you move to patch 3?

Sure, no problem. Besides this, are you ok with the rest of the series?

Regards,

Tomeu

>>       return dev;
>>
>>  err_free:
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index cc7dd687a89d..da8d489e73ad 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -40,6 +40,7 @@ extern ssize_t of_device_get_modalias(struct device *dev,
>>
>>  extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
>>  extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
>> +extern void of_device_probe(struct device_node *np);
>>
>>  static inline void of_device_node_put(struct device *dev)
>>  {
>> @@ -84,6 +85,8 @@ static inline int of_device_uevent_modalias(struct device *dev,
>>       return -ENODEV;
>>  }
>>
>> +static inline void of_device_probe(struct device_node *np) { }
>> +
>>  static inline void of_device_node_put(struct device *dev) { }
>>
>>  static inline const struct of_device_id *__of_match_device(
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown Sept. 11, 2015, 12:08 p.m. UTC | #3
On Mon, Sep 07, 2015 at 02:23:29PM +0200, Tomeu Vizoso wrote:
> Walks the OF tree up and finds the closest ancestor that has a struct
> device associated with it, probing it if isn't bound to a driver yet.

> The above should ensure that the dependency represented by the passed OF
> node is available, because probing a device should cause its descendants
> to be probed as well (when they get registered).

I'm still not seeing how this works for MFDs where the MFD binding is
present directly in DT.
Tomeu Vizoso Sept. 16, 2015, 8:17 a.m. UTC | #4
On 11 September 2015 at 14:08, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 07, 2015 at 02:23:29PM +0200, Tomeu Vizoso wrote:
>> Walks the OF tree up and finds the closest ancestor that has a struct
>> device associated with it, probing it if isn't bound to a driver yet.
>
>> The above should ensure that the dependency represented by the passed OF
>> node is available, because probing a device should cause its descendants
>> to be probed as well (when they get registered).
>
> I'm still not seeing how this works for MFDs where the MFD binding is
> present directly in DT.

The same problem should happen with simple-bus nodes such as
nvidia,tegra124-host1x in which children devices are represented in
the DT (and registered right after their parent) and depend on their
parent for their operation.

Looked at why it wasn't being a problem in my tests and Thierry
mentioned that tegra_host1x_driver takes care of the synchronization
between the bus and their children. So children would make use of the
bus only once it has finished probing and is ready to work (the init
and exit callbacks in host1x_client_ops signal when the bus is safe to
use).

AFAICS, this is a must currently for correct operation in simple-bus
and simple-mfd situations, because probing order is currently very
unpredictable and it's totally possible for the probing of a child to
start before the probing of its parent has finished (if async probing
is enabled or if a module is loaded that registers a child's driver at
the wrong time).

I would prefer if the core would take care of making sure that parents
are always probed before their children, but the unconditional locking
of the parent device stands in the way.

Regards,

Tomeu
Mark Brown Sept. 16, 2015, 7:35 p.m. UTC | #5
On Wed, Sep 16, 2015 at 10:17:43AM +0200, Tomeu Vizoso wrote:

> AFAICS, this is a must currently for correct operation in simple-bus
> and simple-mfd situations, because probing order is currently very
> unpredictable and it's totally possible for the probing of a child to
> start before the probing of its parent has finished (if async probing
> is enabled or if a module is loaded that registers a child's driver at
> the wrong time).

Hrm.  That's worrying for devices that rely on peering into their parent
device for things...

> I would prefer if the core would take care of making sure that parents
> are always probed before their children, but the unconditional locking
> of the parent device stands in the way.

In the MFD case this is another reason for not putting the device
structure into the DT - if it isn't then the ordering is controlled by
the device being instantiated.
Geert Uytterhoeven Oct. 21, 2015, 11:29 a.m. UTC | #6
Hi Tomeu,

On Mon, Sep 7, 2015 at 2:23 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> Walks the OF tree up and finds the closest ancestor that has a struct
> device associated with it, probing it if isn't bound to a driver yet.
>
> The above should ensure that the dependency represented by the passed OF
> node is available, because probing a device should cause its descendants
> to be probed as well (when they get registered).
>
> Subsystems can use this when looking up resources for drivers, to reduce
> the chances of deferred probes because of the probing order of devices.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v4:
> - Rename of_platform_probe to of_device_probe
> - Use device_node.device instead of device_node.platform_dev
>
> Changes in v3:
> - Set and use device_node.platform_dev instead of reversing the logic to
>   find the platform device that encloses a device node.
> - Drop the fwnode API to probe firmware nodes and add OF-only API for
>   now. I think this same scheme could be used for machines with ACPI,
>   but I haven't been able to find one that had to defer its probes because
>   of the device probe order.
>
> Changes in v2: None
>
>  drivers/of/device.c       | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/of/platform.c     |  2 ++
>  include/linux/of_device.h |  3 +++
>  3 files changed, 63 insertions(+)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 8b91ea241b10..c32ac7b6fbe2 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -286,3 +286,61 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>
>         return 0;
>  }
> +
> +/**
> + * of_device_probe() - Probe device associated with OF node
> + * @np: node to probe
> + *
> + * Probe the device associated with the passed device node.
> + */
> +void of_device_probe(struct device_node *np)
> +{
> +       struct device_node *target;
> +       struct device *dev = NULL;
> +
> +       if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS))
> +               return;
> +
> +       if (!np)
> +               return;
> +
> +       of_node_get(np);
> +
> +       /* Find the closest ancestor that has a device associated */
> +       for (target = np;
> +            !of_node_is_root(target);
> +            target = of_get_next_parent(target))
> +               if (target->device) {
> +                       dev = target->device;
> +                       break;
> +               }
> +
> +       of_node_put(target);
> +
> +       if (!dev) {
> +               pr_warn("Couldn't find a device for node '%s'\n",
> +                       of_node_full_name(np));

This is triggered for all devices with clocks instantiated through
CLK_OF_DECLARE().
E.g. on r8a7791/koelsch ("... | sort | uniq -c"):

      1 Couldn't find a device for node '/clocks/audio_clk_a'
      1 Couldn't find a device for node '/clocks/audio_clk_b'
      1 Couldn't find a device for node '/clocks/audio_clk_c'
      2 Couldn't find a device for node '/clocks/cpg_clocks@e6150000'
      1 Couldn't find a device for node '/clocks/m2_clk'
      2 Couldn't find a device for node '/clocks/mstp0_clks@e6150130'
     34 Couldn't find a device for node '/clocks/mstp10_clks@e6150998'
      2 Couldn't find a device for node '/clocks/mstp1_clks@e6150134'
      2 Couldn't find a device for node '/clocks/mstp2_clks@e6150138'
     10 Couldn't find a device for node '/clocks/mstp3_clks@e615013c'
      3 Couldn't find a device for node '/clocks/mstp5_clks@e6150144'
      8 Couldn't find a device for node '/clocks/mstp7_clks@e615014c'
      4 Couldn't find a device for node '/clocks/mstp8_clks@e6150990'
     22 Couldn't find a device for node '/clocks/mstp9_clks@e6150994'
      1 Couldn't find a device for node '/clocks/pcie_bus_clk'

Corresponding DTS is arch/arm/boot/dts/r8a7791.dtsi.

> +               return;
> +       }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox

Patch

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 8b91ea241b10..c32ac7b6fbe2 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -286,3 +286,61 @@  int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 
 	return 0;
 }
+
+/**
+ * of_device_probe() - Probe device associated with OF node
+ * @np: node to probe
+ *
+ * Probe the device associated with the passed device node.
+ */
+void of_device_probe(struct device_node *np)
+{
+	struct device_node *target;
+	struct device *dev = NULL;
+
+	if (!of_root || !of_node_check_flag(of_root, OF_POPULATED_BUS))
+		return;
+
+	if (!np)
+		return;
+
+	of_node_get(np);
+
+	/* Find the closest ancestor that has a device associated */
+	for (target = np;
+	     !of_node_is_root(target);
+	     target = of_get_next_parent(target))
+		if (target->device) {
+			dev = target->device;
+			break;
+		}
+
+	of_node_put(target);
+
+	if (!dev) {
+		pr_warn("Couldn't find a device for node '%s'\n",
+			of_node_full_name(np));
+		return;
+	}
+
+	/*
+	 * 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;
+
+	/*
+	 * Probing a device should cause its descendants to be probed as
+	 * well, which includes the passed device node.
+	 */
+	if (device_attach(dev) != 1)
+		/*
+		 * This cannot be a warning for now because clock nodes have a
+		 * compatible string but the clock framework doesn't follow
+		 * the device/driver model yet.
+		 */
+		dev_dbg(dev, "Probe failed for %s\n", of_node_full_name(np));
+}
+EXPORT_SYMBOL_GPL(of_device_probe);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index baf04d7249bd..f089d95ac961 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -269,6 +269,8 @@  static struct amba_device *of_amba_device_create(struct device_node *node,
 		goto err_free;
 	}
 
+	node->device = &dev->dev;
+
 	return dev;
 
 err_free:
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index cc7dd687a89d..da8d489e73ad 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -40,6 +40,7 @@  extern ssize_t of_device_get_modalias(struct device *dev,
 
 extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
 extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
+extern void of_device_probe(struct device_node *np);
 
 static inline void of_device_node_put(struct device *dev)
 {
@@ -84,6 +85,8 @@  static inline int of_device_uevent_modalias(struct device *dev,
 	return -ENODEV;
 }
 
+static inline void of_device_probe(struct device_node *np) { }
+
 static inline void of_device_node_put(struct device *dev) { }
 
 static inline const struct of_device_id *__of_match_device(