[PATCH v2 0/22] On-demand device probing

[PATCH v2 0/22] On-demand device probing

[Tomeu Vizoso]

Hello,

I have a problem with the panel on my Tegra Chromebook taking longer
than expected to be ready during boot (Stéphane Marchesin reported what
is basically the same issue in [0]), and have looked into ordered
probing as a better way of solving this than moving nodes around in the
DT or playing with initcall levels and linking order.

While reading the thread [1] that Alexander Holler started with his
series to make probing order deterministic, it occurred to me that it
should be possible to achieve the same by probing devices as they are
referenced by other devices.

This basically reuses the information that is already implicit in the
probe() implementations, saving us from refactoring existing drivers or
adding information to DTBs.

During review of v1 of this series Linus Walleij suggested that it
should be the device driver core to make sure that dependencies are
ready before probing a device. I gave this idea a try [2] but Mark Brown
pointed out to the logic duplication between the resource acquisition
and dependency discovery code paths (though I think it's fairly minor).

To address that code duplication I experimented with Arnd's devm_probe
[3] concept of having drivers declare their dependencies instead of
acquiring them during probe, and while it worked [4], I don't think we
end up winning anything when compared to just probing devices on-demand
from resource getters.

One remaining objection is to the "sprinkling" of calls to
fwnode_ensure_device() in the resource getters of each subsystem, but I
think it's the right thing to do given that the storage of resources is
currently subsystem-specific.

We could avoid the above by moving resource storage into the core, but I
don't think there's a compelling case for that.

I have tested this on boards with Tegra, iMX.6, Exynos and OMAP SoCs,
and these patches were enough to eliminate all the deferred probes
(except one in PandaBoard because omap_dma_system doesn't have a
firmware node as of yet).

With this series I get the kernel to output to the panel in 0.5s,
instead of 2.8s.

Regards,

Tomeu

[0] http://lists.freedesktop.org/archives/dri-devel/2014-August/066527.html

[1] https://lkml.org/lkml/2014/5/12/452

[2] https://lkml.org/lkml/2015/6/17/305

[3] http://article.gmane.org/gmane.linux.ports.arm.kernel/277689

[4] https://lkml.org/lkml/2015/7/21/441

Changes in v2:
- Move delay to platform.c
- Use set_primary_fwnode()
- Use of_node_full_name()
- Move the logic for finding a platform device from its firmware node to
  of/platform.c as it's not needed for ACPI nodes.
- Add acpi_dev_get_device()
- Add fwnode_ensure_device() so the mechanism for probing devices on
  demand is independent of the firmware format.
- Acquire regulator device lock before returning from regulator_dev_lookup()

Tomeu Vizoso (22):
  platform: delay device-driver matches until late_initcall
  of/platform: Set fwnode field for new devices
  device property: add fwnode_get_name()
  of/platform: add of_platform_device_find()
  ACPI: add acpi_dev_get_device()
  device property: add fwnode_ensure_device()
  gpio: Probe GPIO drivers on demand
  gpio: Probe pinctrl devices on demand
  regulator: core: Reduce critical area in _regulator_get
  regulator: core: Probe regulators on demand
  drm: Probe panels on demand
  drm/tegra: Probe dpaux devices on demand
  i2c: core: Probe i2c master devices on demand
  pwm: Probe PWM chip devices on demand
  backlight: Probe backlight devices on demand
  usb: phy: Probe phy devices on demand
  clk: Probe clk providers on demand
  pinctrl: Probe pinctrl devices on demand
  phy: core: Probe phy providers on demand
  dma: of: Probe DMA controllers on demand
  power-supply: Probe power supplies on demand
  ASoC: core: Probe components on demand

 drivers/base/platform.c             | 28 +++++++++++
 drivers/base/property.c             | 73 +++++++++++++++++++++++++++
 drivers/clk/clk.c                   |  3 ++
 drivers/dma/of-dma.c                |  2 +
 drivers/gpio/gpiolib-of.c           |  4 ++
 drivers/gpu/drm/drm_panel.c         |  2 +
 drivers/gpu/drm/tegra/dpaux.c       |  2 +
 drivers/i2c/i2c-core.c              |  2 +
 drivers/of/platform.c               | 61 +++++++++++++++++++++++
 drivers/phy/phy-core.c              |  2 +
 drivers/pinctrl/devicetree.c        |  1 +
 drivers/power/power_supply_core.c   |  2 +
 drivers/pwm/core.c                  |  2 +
 drivers/regulator/core.c            | 99 +++++++++++++++++++++----------------
 drivers/usb/phy/phy.c               |  2 +
 drivers/video/backlight/backlight.c |  2 +
 include/linux/acpi.h                | 10 ++++
 include/linux/of_platform.h         |  1 +
 include/linux/property.h            |  4 ++
 sound/soc/soc-core.c                |  6 ++-
 20 files changed, 264 insertions(+), 44 deletions(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 01/22] platform: delay device-driver matches until late_initcall

[Tomeu Vizoso]

Delay matches of platform devices until late_initcall, when we are sure
that all built-in drivers have been registered already.  This is needed
to prevent deferred probes because of some drivers not having registered
yet.

The reason why only platform devices are delayed is that some other
devices are expected to be probed earlier than late_initcall, for
example, the system PNP driver needs to probe its devices in
fs_initcall.

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

Changes in v2:
- Move delay to platform.c

 drivers/base/platform.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 063f0ab15259..fcf654678e27 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -33,6 +33,8 @@
 /* For automatically allocated device IDs */
 static DEFINE_IDA(platform_devid_ida);
 
+static bool enable_matches;
+
 struct device platform_bus = {
 	.init_name	= "platform",
 };
@@ -839,6 +841,15 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct platform_driver *pdrv = to_platform_driver(drv);
 
+	/*
+	 * Delay matches of platform devices until late_initcall, when we are
+	 * sure that all built-in drivers have been registered already. This
+	 * is needed to prevent deferred probes because of some drivers
+	 * not having registered yet.
+	 */
+	if (!enable_matches)
+		return false;
+
 	/* When driver_override is set, only bind to the matching driver */
 	if (pdev->driver_override)
 		return !strcmp(pdev->driver_override, drv->name);
@@ -859,6 +870,23 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 	return (strcmp(pdev->name, drv->name) == 0);
 }
 
+static int __probe_device(struct device *dev, void *data)
+{
+	device_initial_probe(dev);
+
+	return 0;
+}
+
+static int probe_delayed_matches_initcall(void)
+{
+	enable_matches = true;
+
+	bus_for_each_dev(&platform_bus_type, NULL, NULL, __probe_device);
+
+	return 0;
+}
+late_initcall(probe_delayed_matches_initcall);
+
 #ifdef CONFIG_PM_SLEEP
 
 static int platform_legacy_suspend(struct device *dev, pm_message_t mesg)
-- 
2.4.3

Re: [PATCH v2 01/22] platform: delay device-driver matches until late_initcall

[Rob Herring]

On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> Delay matches of platform devices until late_initcall, when we are sure
> that all built-in drivers have been registered already.  This is needed
> to prevent deferred probes because of some drivers not having registered
> yet.
>
> The reason why only platform devices are delayed is that some other
> devices are expected to be probed earlier than late_initcall, for
> example, the system PNP driver needs to probe its devices in
> fs_initcall.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v2:
> - Move delay to platform.c
>
>  drivers/base/platform.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 063f0ab15259..fcf654678e27 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -33,6 +33,8 @@
>  /* For automatically allocated device IDs */
>  static DEFINE_IDA(platform_devid_ida);
>
> +static bool enable_matches;
> +
>  struct device platform_bus = {
>         .init_name      = "platform",
>  };
> @@ -839,6 +841,15 @@ static int platform_match(struct device *dev, struct device_driver *drv)
>         struct platform_device *pdev = to_platform_device(dev);
>         struct platform_driver *pdrv = to_platform_driver(drv);
>
> +       /*
> +        * Delay matches of platform devices until late_initcall, when we are
> +        * sure that all built-in drivers have been registered already. This
> +        * is needed to prevent deferred probes because of some drivers
> +        * not having registered yet.
> +        */
> +       if (!enable_matches)
> +               return false;
> +

Having this as a global makes me nervous. I think it would be better
to be DT specific or per device some how. Perhaps use OF_POPULATED_BUS
flag as an additional test.

There could be non-DT platforms that rely on the initcall ordering and
moving all probes to late_initcall could change the ordering. I'm not
sure though.

Rob

Re: [PATCH v2 01/22] platform: delay device-driver matches until late_initcall

[Tomeu Vizoso]

On 30 July 2015 at 05:20, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> Delay matches of platform devices until late_initcall, when we are sure
>> that all built-in drivers have been registered already.  This is needed
>> to prevent deferred probes because of some drivers not having registered
>> yet.
>>
>> The reason why only platform devices are delayed is that some other
>> devices are expected to be probed earlier than late_initcall, for
>> example, the system PNP driver needs to probe its devices in
>> fs_initcall.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v2:
>> - Move delay to platform.c
>>
>>  drivers/base/platform.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 063f0ab15259..fcf654678e27 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -33,6 +33,8 @@
>>  /* For automatically allocated device IDs */
>>  static DEFINE_IDA(platform_devid_ida);
>>
>> +static bool enable_matches;
>> +
>>  struct device platform_bus = {
>>         .init_name      = "platform",
>>  };
>> @@ -839,6 +841,15 @@ static int platform_match(struct device *dev, struct device_driver *drv)
>>         struct platform_device *pdev = to_platform_device(dev);
>>         struct platform_driver *pdrv = to_platform_driver(drv);
>>
>> +       /*
>> +        * Delay matches of platform devices until late_initcall, when we are
>> +        * sure that all built-in drivers have been registered already. This
>> +        * is needed to prevent deferred probes because of some drivers
>> +        * not having registered yet.
>> +        */
>> +       if (!enable_matches)
>> +               return false;
>> +
>
> Having this as a global makes me nervous. I think it would be better
> to be DT specific or per device some how. Perhaps use OF_POPULATED_BUS
> flag as an additional test.

I see no problem with restricting this to platform devices with an
of_node (or a fwnode if we still want to address machines with ACPI).

> There could be non-DT platforms that rely on the initcall ordering and
> moving all probes to late_initcall could change the ordering. I'm not
> sure though.

Yeah, I'm not sure how much that could be a problem. Maybe if a
non-platform device has a match and probes before a platform device
that has been delayed and is a dependency of it. That could be a
problem in platforms that don't do on-demand probing because of the
lack of firmware data.

Thanks,

Tomeu

> Rob
> --
> 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/

[PATCH v2 02/22] 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.

This is needed by platform-independent code that needs to do something
with devices based on the data provided by the firmware.

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

Changes in v2:
- Use set_primary_fwnode()

 drivers/of/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ddf8e42c9367..ff27494cda8c 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);
+	set_primary_fwnode(&dev->dev, &dev->dev.of_node->fwnode);
 	dev->dev.parent = parent ? : &platform_bus;
 
 	if (bus_id)
-- 
2.4.3

[PATCH v2 03/22] 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>
---

Changes in v2:
- Use of_node_full_name()

 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 f3f6d167f3f1..efa74803af30 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -533,3 +533,18 @@ bool device_dma_is_coherent(struct device *dev)
 	return coherent;
 }
 EXPORT_SYMBOL_GPL(device_dma_is_coherent);
+
+/**
+ * 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_full_name(to_of_node(fwnode));
+	else if (is_acpi_node(fwnode))
+		return acpi_dev_name(to_acpi_node(fwnode));
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_name);
diff --git a/include/linux/property.h b/include/linux/property.h
index 76ebde9c11d4..826f156f7288 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);
 
+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.3

[PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

>From an arbitrary node in the tree, find the enclosing node that
corresponds to a platform device, as registered by
of_platform_populate().

This can be used to find out what device needs to be probed so the
dependency described by a given node is made available.

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

Changes in v2:
- Move the logic for finding a platform device from its firmware node to
  of/platform.c as it's not needed for ACPI nodes.

 drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_platform.h |  1 +
 2 files changed, 61 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ff27494cda8c..89c5cd513027 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
 }
 EXPORT_SYMBOL_GPL(of_platform_depopulate);
 
+static bool of_is_platform(struct device_node *np)
+{
+	int count;
+
+	count = of_property_count_strings(np, "compatible");
+
+	/* The node has to have a compatible string */
+	if (!count)
+		return false;
+
+	/* But it cannot be just a simple memory-mapped bus */
+	if (count == 1 && of_match_node(of_default_bus_match_table, np))
+		return false;
+
+	/* But AMBA devices aren't platform devices */
+	if (of_device_is_compatible(np, "arm,primecell"))
+		return false;
+
+	/* Node is immediately below root */
+	if (!np->parent || !np->parent->parent)
+		return true;
+
+	/* If it's a node in a simple memory-mapped bus */
+	if (of_match_node(of_default_bus_match_table, np->parent))
+		return true;
+
+	return false;
+}
+
+/**
+ * of_platform_device_find() - Find nearest ancestor that is a platform device
+ * @np: node to find platform device for
+ *
+ * Walks the tree up and finds the closest ancestor that has match data and
+ * either is at the root of the tree or is a child of a simple memory mapped
+ * bus.
+ *
+ * Returns such a device, or NULL if none could be found.
+ */
+struct device *of_platform_device_find(struct device_node *np)
+{
+	struct device_node *target;
+	struct platform_device *pdev;
+
+	of_node_get(np);
+
+	for (target = np;
+	     !of_node_is_root(target);
+	     target = of_get_next_parent(target))
+		if (of_is_platform(target))
+			break;
+
+	pdev = of_find_device_by_node(target);
+
+	of_node_put(target);
+
+	return pdev ? &pdev->dev : NULL;
+}
+EXPORT_SYMBOL_GPL(of_platform_device_find);
+
 #ifdef CONFIG_OF_DYNAMIC
 static int of_platform_notify(struct notifier_block *nb,
 				unsigned long action, void *arg)
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 611a691145c4..baefc941fdb8 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -58,6 +58,7 @@ extern struct platform_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
 extern struct platform_device *of_find_device_by_node(struct device_node *np);
+extern struct device *of_platform_device_find(struct device_node *np);
 
 /* Platform devices and busses creation */
 extern struct platform_device *of_platform_device_create(struct device_node *np,
-- 
2.4.3

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Rob Herring]

On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> From an arbitrary node in the tree, find the enclosing node that
> corresponds to a platform device, as registered by
> of_platform_populate().
>
> This can be used to find out what device needs to be probed so the
> dependency described by a given node is made available.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>
> Changes in v2:
> - Move the logic for finding a platform device from its firmware node to
>   of/platform.c as it's not needed for ACPI nodes.
>
>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_platform.h |  1 +
>  2 files changed, 61 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ff27494cda8c..89c5cd513027 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>  }
>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>
> +static bool of_is_platform(struct device_node *np)
> +{
> +       int count;
> +
> +       count = of_property_count_strings(np, "compatible");
> +
> +       /* The node has to have a compatible string */
> +       if (!count)
> +               return false;
> +
> +       /* But it cannot be just a simple memory-mapped bus */
> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
> +               return false;
> +
> +       /* But AMBA devices aren't platform devices */
> +       if (of_device_is_compatible(np, "arm,primecell"))
> +               return false;
> +
> +       /* Node is immediately below root */
> +       if (!np->parent || !np->parent->parent)
> +               return true;
> +
> +       /* If it's a node in a simple memory-mapped bus */
> +       if (of_match_node(of_default_bus_match_table, np->parent))
> +               return true;

This seems really fragile. What about platform devices which are
children of MFDs but are not "simple-mfd"?

Does of_find_device_by_node not work for you? That is probably not the
most efficient search, but we could fix that. We could add struct
device ptr to struct device_node and check without searching for
example.

Rob

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> From an arbitrary node in the tree, find the enclosing node that
>> corresponds to a platform device, as registered by
>> of_platform_populate().
>>
>> This can be used to find out what device needs to be probed so the
>> dependency described by a given node is made available.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v2:
>> - Move the logic for finding a platform device from its firmware node to
>>   of/platform.c as it's not needed for ACPI nodes.
>>
>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_platform.h |  1 +
>>  2 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index ff27494cda8c..89c5cd513027 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>  }
>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>
>> +static bool of_is_platform(struct device_node *np)
>> +{
>> +       int count;
>> +
>> +       count = of_property_count_strings(np, "compatible");
>> +
>> +       /* The node has to have a compatible string */
>> +       if (!count)
>> +               return false;
>> +
>> +       /* But it cannot be just a simple memory-mapped bus */
>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>> +               return false;
>> +
>> +       /* But AMBA devices aren't platform devices */
>> +       if (of_device_is_compatible(np, "arm,primecell"))
>> +               return false;
>> +
>> +       /* Node is immediately below root */
>> +       if (!np->parent || !np->parent->parent)
>> +               return true;
>> +
>> +       /* If it's a node in a simple memory-mapped bus */
>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>> +               return true;
>
> This seems really fragile.

I think this finding logic matches the logic for registering platform
devices in of_platform_populate and also what is documented in
Documentation/devicetree/usage-model.txt.

> What about platform devices which are
> children of MFDs but are not "simple-mfd"?

This code should deal fine with those (the boards I tested with do
have them). It probes the mfd master, and that in turn will call
mfd_add_devices causing the target device to be probed.

> Does of_find_device_by_node not work for you?

Well, the dependencies aren't always platform devices, that's why I
need to go up the tree until I find a node that corresponds to a
platform device that I can query and probe.

If I had a way to get, say, a i2c device from its fwnode then I would
just need to make sure that a device's parent is probed before probing
it and everything would be cleaner in the OF case.

> That is probably not the
> most efficient search, but we could fix that. We could add struct
> device ptr to struct device_node and check without searching for
> example.

That would be great, but I thought there was an issue with a OF node
being able to be related to more than one struct device (but I haven't
found this myself yet).

Thanks,

Tomeu

> Rob
> --
> 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/

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Rob Herring]

On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>> <tomeu.vizoso@collabora.com> wrote:
>>> From an arbitrary node in the tree, find the enclosing node that
>>> corresponds to a platform device, as registered by
>>> of_platform_populate().
>>>
>>> This can be used to find out what device needs to be probed so the
>>> dependency described by a given node is made available.
>>>
>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Move the logic for finding a platform device from its firmware node to
>>>   of/platform.c as it's not needed for ACPI nodes.
>>>
>>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/of_platform.h |  1 +
>>>  2 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index ff27494cda8c..89c5cd513027 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>>  }
>>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>>
>>> +static bool of_is_platform(struct device_node *np)
>>> +{
>>> +       int count;
>>> +
>>> +       count = of_property_count_strings(np, "compatible");
>>> +
>>> +       /* The node has to have a compatible string */
>>> +       if (!count)
>>> +               return false;
>>> +
>>> +       /* But it cannot be just a simple memory-mapped bus */
>>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>>> +               return false;
>>> +
>>> +       /* But AMBA devices aren't platform devices */
>>> +       if (of_device_is_compatible(np, "arm,primecell"))
>>> +               return false;
>>> +
>>> +       /* Node is immediately below root */
>>> +       if (!np->parent || !np->parent->parent)
>>> +               return true;
>>> +
>>> +       /* If it's a node in a simple memory-mapped bus */
>>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>>> +               return true;
>>
>> This seems really fragile.
>
> I think this finding logic matches the logic for registering platform
> devices in of_platform_populate and also what is documented in
> Documentation/devicetree/usage-model.txt.

Right. So now we have that logic in 2 places. One is descending from
the root and one is walking up from the child. That's an opportunity
for some mismatch.

>> What about platform devices which are
>> children of MFDs but are not "simple-mfd"?
>
> This code should deal fine with those (the boards I tested with do
> have them). It probes the mfd master, and that in turn will call
> mfd_add_devices causing the target device to be probed.

I don't see how this function would ever return true in this case
unless you put the MFD at the root level. The only other way to return
true is matching on of_default_bus_match_table for the parent (i.e.
the MFD).


>> Does of_find_device_by_node not work for you?
>
> Well, the dependencies aren't always platform devices, that's why I
> need to go up the tree until I find a node that corresponds to a
> platform device that I can query and probe.
>
> If I had a way to get, say, a i2c device from its fwnode then I would
> just need to make sure that a device's parent is probed before probing
> it and everything would be cleaner in the OF case.

If you have the struct device from the device_node, then you should be
able to do this, right?

>> That is probably not the
>> most efficient search, but we could fix that. We could add struct
>> device ptr to struct device_node and check without searching for
>> example.
>
> That would be great, but I thought there was an issue with a OF node
> being able to be related to more than one struct device (but I haven't
> found this myself yet).

I think it pretty much should be one to one. I'm not aware of any
examples where that is not the case. This function would already be
broken if you could have more than one struct device.

There is an issue where you could have 2 drivers match the same node,
but on different compatible strings. But that's a different issue.

Rob

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 28 July 2015 at 17:31, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>> <tomeu.vizoso@collabora.com> wrote:
>>>> From an arbitrary node in the tree, find the enclosing node that
>>>> corresponds to a platform device, as registered by
>>>> of_platform_populate().
>>>>
>>>> This can be used to find out what device needs to be probed so the
>>>> dependency described by a given node is made available.
>>>>
>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Move the logic for finding a platform device from its firmware node to
>>>>   of/platform.c as it's not needed for ACPI nodes.
>>>>
>>>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/of_platform.h |  1 +
>>>>  2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index ff27494cda8c..89c5cd513027 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>>>
>>>> +static bool of_is_platform(struct device_node *np)
>>>> +{
>>>> +       int count;
>>>> +
>>>> +       count = of_property_count_strings(np, "compatible");
>>>> +
>>>> +       /* The node has to have a compatible string */
>>>> +       if (!count)
>>>> +               return false;
>>>> +
>>>> +       /* But it cannot be just a simple memory-mapped bus */
>>>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>>>> +               return false;
>>>> +
>>>> +       /* But AMBA devices aren't platform devices */
>>>> +       if (of_device_is_compatible(np, "arm,primecell"))
>>>> +               return false;
>>>> +
>>>> +       /* Node is immediately below root */
>>>> +       if (!np->parent || !np->parent->parent)
>>>> +               return true;
>>>> +
>>>> +       /* If it's a node in a simple memory-mapped bus */
>>>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>>>> +               return true;
>>>
>>> This seems really fragile.
>>
>> I think this finding logic matches the logic for registering platform
>> devices in of_platform_populate and also what is documented in
>> Documentation/devicetree/usage-model.txt.
>
> Right. So now we have that logic in 2 places. One is descending from
> the root and one is walking up from the child. That's an opportunity
> for some mismatch.

Definitely.

>>> What about platform devices which are
>>> children of MFDs but are not "simple-mfd"?
>>
>> This code should deal fine with those (the boards I tested with do
>> have them). It probes the mfd master, and that in turn will call
>> mfd_add_devices causing the target device to be probed.
>
> I don't see how this function would ever return true in this case
> unless you put the MFD at the root level. The only other way to return
> true is matching on of_default_bus_match_table for the parent (i.e.
> the MFD).

Oops, you are right.

>>> Does of_find_device_by_node not work for you?
>>
>> Well, the dependencies aren't always platform devices, that's why I
>> need to go up the tree until I find a node that corresponds to a
>> platform device that I can query and probe.
>>
>> If I had a way to get, say, a i2c device from its fwnode then I would
>> just need to make sure that a device's parent is probed before probing
>> it and everything would be cleaner in the OF case.
>
> If you have the struct device from the device_node, then you should be
> able to do this, right?

Yes, if I could go back from the device_node to the struct device that
was registered from it, for all buses, then all this would be much
simpler and more robust. It would basically work like in the ACPI
case.

I will play with this idea.

>>> That is probably not the
>>> most efficient search, but we could fix that. We could add struct
>>> device ptr to struct device_node and check without searching for
>>> example.
>>
>> That would be great, but I thought there was an issue with a OF node
>> being able to be related to more than one struct device (but I haven't
>> found this myself yet).
>
> I think it pretty much should be one to one. I'm not aware of any
> examples where that is not the case. This function would already be
> broken if you could have more than one struct device.

Well, for platform devices we currently know that there can only be
one struct device for a given device_node, but that's not so clear for
other devices.

> There is an issue where you could have 2 drivers match the same node,
> but on different compatible strings. But that's a different issue.

Yup.

Thanks,

Tomeu

> Rob
> --
> 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/

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 29 July 2015 at 08:14, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> On 28 July 2015 at 17:31, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
>> <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>>> On 28 July 2015 at 15:39, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>>> <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>>>>> From an arbitrary node in the tree, find the enclosing node that
>>>>> corresponds to a platform device, as registered by
>>>>> of_platform_populate().
>>>>>
>>>>> This can be used to find out what device needs to be probed so the
>>>>> dependency described by a given node is made available.
>>>>>
>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Move the logic for finding a platform device from its firmware node to
>>>>>   of/platform.c as it's not needed for ACPI nodes.
>>>>>
>>>>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/of_platform.h |  1 +
>>>>>  2 files changed, 61 insertions(+)
>>>>>
>>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>>> index ff27494cda8c..89c5cd513027 100644
>>>>> --- a/drivers/of/platform.c
>>>>> +++ b/drivers/of/platform.c
>>>>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>>>>
>>>>> +static bool of_is_platform(struct device_node *np)
>>>>> +{
>>>>> +       int count;
>>>>> +
>>>>> +       count = of_property_count_strings(np, "compatible");
>>>>> +
>>>>> +       /* The node has to have a compatible string */
>>>>> +       if (!count)
>>>>> +               return false;
>>>>> +
>>>>> +       /* But it cannot be just a simple memory-mapped bus */
>>>>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>>>>> +               return false;
>>>>> +
>>>>> +       /* But AMBA devices aren't platform devices */
>>>>> +       if (of_device_is_compatible(np, "arm,primecell"))
>>>>> +               return false;
>>>>> +
>>>>> +       /* Node is immediately below root */
>>>>> +       if (!np->parent || !np->parent->parent)
>>>>> +               return true;
>>>>> +
>>>>> +       /* If it's a node in a simple memory-mapped bus */
>>>>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>>>>> +               return true;
>>>>
>>>> This seems really fragile.
>>>
>>> I think this finding logic matches the logic for registering platform
>>> devices in of_platform_populate and also what is documented in
>>> Documentation/devicetree/usage-model.txt.
>>
>> Right. So now we have that logic in 2 places. One is descending from
>> the root and one is walking up from the child. That's an opportunity
>> for some mismatch.
>
> Definitely.
>
>>>> What about platform devices which are
>>>> children of MFDs but are not "simple-mfd"?
>>>
>>> This code should deal fine with those (the boards I tested with do
>>> have them). It probes the mfd master, and that in turn will call
>>> mfd_add_devices causing the target device to be probed.
>>
>> I don't see how this function would ever return true in this case
>> unless you put the MFD at the root level. The only other way to return
>> true is matching on of_default_bus_match_table for the parent (i.e.
>> the MFD).
>
> Oops, you are right.
>
>>>> Does of_find_device_by_node not work for you?
>>>
>>> Well, the dependencies aren't always platform devices, that's why I
>>> need to go up the tree until I find a node that corresponds to a
>>> platform device that I can query and probe.
>>>
>>> If I had a way to get, say, a i2c device from its fwnode then I would
>>> just need to make sure that a device's parent is probed before probing
>>> it and everything would be cleaner in the OF case.
>>
>> If you have the struct device from the device_node, then you should be
>> able to do this, right?
>
> Yes, if I could go back from the device_node to the struct device that
> was registered from it, for all buses, then all this would be much
> simpler and more robust. It would basically work like in the ACPI
> case.
>
> I will play with this idea.
>
>>>> That is probably not the
>>>> most efficient search, but we could fix that. We could add struct
>>>> device ptr to struct device_node and check without searching for
>>>> example.
>>>
>>> That would be great, but I thought there was an issue with a OF node
>>> being able to be related to more than one struct device (but I haven't
>>> found this myself yet).
>>
>> I think it pretty much should be one to one. I'm not aware of any
>> examples where that is not the case. This function would already be
>> broken if you could have more than one struct device.
>
> Well, for platform devices we currently know that there can only be
> one struct device for a given device_node, but that's not so clear for
> other devices.

Just found this case:

http://lxr.free-electrons.com/source/drivers/spi/spi-tegra114.c#L1124

Looks like SPI master devices point to the same device_node as the
platform device that registers them.

Regards,

Tomeu

>> There is an issue where you could have 2 drivers match the same node,
>> but on different compatible strings. But that's a different issue.
>
> Yup.
>
> Thanks,
>
> Tomeu
>
>> Rob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 29 July 2015 at 13:20, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 29 July 2015 at 08:14, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> On 28 July 2015 at 17:31, Rob Herring <robherring2@gmail.com> wrote:
>>> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
>>> <tomeu.vizoso@collabora.com> wrote:
>>>> On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
>>>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>>>> <tomeu.vizoso@collabora.com> wrote:
>>>>>> From an arbitrary node in the tree, find the enclosing node that
>>>>>> corresponds to a platform device, as registered by
>>>>>> of_platform_populate().
>>>>>>
>>>>>> This can be used to find out what device needs to be probed so the
>>>>>> dependency described by a given node is made available.
>>>>>>
>>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Move the logic for finding a platform device from its firmware node to
>>>>>>   of/platform.c as it's not needed for ACPI nodes.
>>>>>>
>>>>>>  drivers/of/platform.c       | 60 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/linux/of_platform.h |  1 +
>>>>>>  2 files changed, 61 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>>>> index ff27494cda8c..89c5cd513027 100644
>>>>>> --- a/drivers/of/platform.c
>>>>>> +++ b/drivers/of/platform.c
>>>>>> @@ -501,6 +501,66 @@ void of_platform_depopulate(struct device *parent)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(of_platform_depopulate);
>>>>>>
>>>>>> +static bool of_is_platform(struct device_node *np)
>>>>>> +{
>>>>>> +       int count;
>>>>>> +
>>>>>> +       count = of_property_count_strings(np, "compatible");
>>>>>> +
>>>>>> +       /* The node has to have a compatible string */
>>>>>> +       if (!count)
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* But it cannot be just a simple memory-mapped bus */
>>>>>> +       if (count == 1 && of_match_node(of_default_bus_match_table, np))
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* But AMBA devices aren't platform devices */
>>>>>> +       if (of_device_is_compatible(np, "arm,primecell"))
>>>>>> +               return false;
>>>>>> +
>>>>>> +       /* Node is immediately below root */
>>>>>> +       if (!np->parent || !np->parent->parent)
>>>>>> +               return true;
>>>>>> +
>>>>>> +       /* If it's a node in a simple memory-mapped bus */
>>>>>> +       if (of_match_node(of_default_bus_match_table, np->parent))
>>>>>> +               return true;
>>>>>
>>>>> This seems really fragile.
>>>>
>>>> I think this finding logic matches the logic for registering platform
>>>> devices in of_platform_populate and also what is documented in
>>>> Documentation/devicetree/usage-model.txt.
>>>
>>> Right. So now we have that logic in 2 places. One is descending from
>>> the root and one is walking up from the child. That's an opportunity
>>> for some mismatch.
>>
>> Definitely.
>>
>>>>> What about platform devices which are
>>>>> children of MFDs but are not "simple-mfd"?
>>>>
>>>> This code should deal fine with those (the boards I tested with do
>>>> have them). It probes the mfd master, and that in turn will call
>>>> mfd_add_devices causing the target device to be probed.
>>>
>>> I don't see how this function would ever return true in this case
>>> unless you put the MFD at the root level. The only other way to return
>>> true is matching on of_default_bus_match_table for the parent (i.e.
>>> the MFD).
>>
>> Oops, you are right.
>>
>>>>> Does of_find_device_by_node not work for you?
>>>>
>>>> Well, the dependencies aren't always platform devices, that's why I
>>>> need to go up the tree until I find a node that corresponds to a
>>>> platform device that I can query and probe.
>>>>
>>>> If I had a way to get, say, a i2c device from its fwnode then I would
>>>> just need to make sure that a device's parent is probed before probing
>>>> it and everything would be cleaner in the OF case.
>>>
>>> If you have the struct device from the device_node, then you should be
>>> able to do this, right?
>>
>> Yes, if I could go back from the device_node to the struct device that
>> was registered from it, for all buses, then all this would be much
>> simpler and more robust. It would basically work like in the ACPI
>> case.
>>
>> I will play with this idea.
>>
>>>>> That is probably not the
>>>>> most efficient search, but we could fix that. We could add struct
>>>>> device ptr to struct device_node and check without searching for
>>>>> example.
>>>>
>>>> That would be great, but I thought there was an issue with a OF node
>>>> being able to be related to more than one struct device (but I haven't
>>>> found this myself yet).
>>>
>>> I think it pretty much should be one to one. I'm not aware of any
>>> examples where that is not the case. This function would already be
>>> broken if you could have more than one struct device.
>>
>> Well, for platform devices we currently know that there can only be
>> one struct device for a given device_node, but that's not so clear for
>> other devices.
>
> Just found this case:
>
> http://lxr.free-electrons.com/source/drivers/spi/spi-tegra114.c#L1124
>
> Looks like SPI master devices point to the same device_node as the
> platform device that registers them.

And finally found the email that warned me against it:

http://lkml.kernel.org/g/20140514140534.897F8C4153D@trevor.secretlab.ca

Regards,

Tomeu

> Regards,
>
> Tomeu
>
>>> There is an issue where you could have 2 drivers match the same node,
>>> but on different compatible strings. But that's a different issue.
>>
>> Yup.
>>
>> Thanks,
>>
>> Tomeu
>>
>>> Rob
>>> --
>>> 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/

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Rob Herring]

On Wed, Jul 29, 2015 at 6:20 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 29 July 2015 at 08:14, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> On 28 July 2015 at 17:31, Rob Herring <robherring2@gmail.com> wrote:
>>> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
>>> <tomeu.vizoso@collabora.com> wrote:
>>>> On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
>>>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>>>> <tomeu.vizoso@collabora.com> wrote:
>>>>>> From an arbitrary node in the tree, find the enclosing node that
>>>>>> corresponds to a platform device, as registered by
>>>>>> of_platform_populate().

[...]

>>>> If I had a way to get, say, a i2c device from its fwnode then I would
>>>> just need to make sure that a device's parent is probed before probing
>>>> it and everything would be cleaner in the OF case.
>>>
>>> If you have the struct device from the device_node, then you should be
>>> able to do this, right?
>>
>> Yes, if I could go back from the device_node to the struct device that
>> was registered from it, for all buses, then all this would be much
>> simpler and more robust. It would basically work like in the ACPI
>> case.
>>
>> I will play with this idea.
>>
>>>>> That is probably not the
>>>>> most efficient search, but we could fix that. We could add struct
>>>>> device ptr to struct device_node and check without searching for
>>>>> example.
>>>>
>>>> That would be great, but I thought there was an issue with a OF node
>>>> being able to be related to more than one struct device (but I haven't
>>>> found this myself yet).
>>>
>>> I think it pretty much should be one to one. I'm not aware of any
>>> examples where that is not the case. This function would already be
>>> broken if you could have more than one struct device.
>>
>> Well, for platform devices we currently know that there can only be
>> one struct device for a given device_node, but that's not so clear for
>> other devices.
>
> Just found this case:
>
> http://lxr.free-electrons.com/source/drivers/spi/spi-tegra114.c#L1124
>
> Looks like SPI master devices point to the same device_node as the
> platform device that registers them.

I don't think this is a problem. The device ptr would only point to
the platform device. Nothing else is going to know about the ptr,
modify it nor expect that it points to the same struct device that
contains the of_node ptr.

So I think any instances of struct device like this are ones you don't
care about for purposes of probe dependencies.

Rob

Re: [PATCH v2 04/22] of/platform: add of_platform_device_find()

[Tomeu Vizoso]

On 29 July 2015 at 17:27, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Jul 29, 2015 at 6:20 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> On 29 July 2015 at 08:14, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> On 28 July 2015 at 17:31, Rob Herring <robherring2@gmail.com> wrote:
>>>> On Tue, Jul 28, 2015 at 8:54 AM, Tomeu Vizoso
>>>> <tomeu.vizoso@collabora.com> wrote:
>>>>> On 28 July 2015 at 15:39, Rob Herring <robherring2@gmail.com> wrote:
>>>>>> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
>>>>>> <tomeu.vizoso@collabora.com> wrote:
>>>>>>> From an arbitrary node in the tree, find the enclosing node that
>>>>>>> corresponds to a platform device, as registered by
>>>>>>> of_platform_populate().
>
> [...]
>
>>>>> If I had a way to get, say, a i2c device from its fwnode then I would
>>>>> just need to make sure that a device's parent is probed before probing
>>>>> it and everything would be cleaner in the OF case.
>>>>
>>>> If you have the struct device from the device_node, then you should be
>>>> able to do this, right?
>>>
>>> Yes, if I could go back from the device_node to the struct device that
>>> was registered from it, for all buses, then all this would be much
>>> simpler and more robust. It would basically work like in the ACPI
>>> case.
>>>
>>> I will play with this idea.
>>>
>>>>>> That is probably not the
>>>>>> most efficient search, but we could fix that. We could add struct
>>>>>> device ptr to struct device_node and check without searching for
>>>>>> example.
>>>>>
>>>>> That would be great, but I thought there was an issue with a OF node
>>>>> being able to be related to more than one struct device (but I haven't
>>>>> found this myself yet).
>>>>
>>>> I think it pretty much should be one to one. I'm not aware of any
>>>> examples where that is not the case. This function would already be
>>>> broken if you could have more than one struct device.
>>>
>>> Well, for platform devices we currently know that there can only be
>>> one struct device for a given device_node, but that's not so clear for
>>> other devices.
>>
>> Just found this case:
>>
>> http://lxr.free-electrons.com/source/drivers/spi/spi-tegra114.c#L1124
>>
>> Looks like SPI master devices point to the same device_node as the
>> platform device that registers them.
>
> I don't think this is a problem. The device ptr would only point to
> the platform device. Nothing else is going to know about the ptr,
> modify it nor expect that it points to the same struct device that
> contains the of_node ptr.
>
> So I think any instances of struct device like this are ones you don't
> care about for purposes of probe dependencies.

Ok, I think I got it now. This is what I came up with and works fine
on all the boards I'm testing with:

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 89c5cd513027..e14518b5e1ce 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -192,6 +192,8 @@ static struct platform_device
*of_platform_device_create_pdata(
         goto err_clear_flag;
     }

+    np->platform_dev = dev;
+
     return dev;

 err_clear_flag:
@@ -501,59 +503,29 @@ void of_platform_depopulate(struct device *parent)
 }
 EXPORT_SYMBOL_GPL(of_platform_depopulate);

 /**
  * of_platform_device_find() - Find nearest ancestor that is a platform device
  * @np: node to find platform device for
  *
- * Walks the tree up and finds the closest ancestor that has match data and
- * either is at the root of the tree or is a child of a simple memory mapped
- * bus.
+ * Walks the OF tree up and finds the closest ancestor that has a platform
+ * device associated with it.
  *
  * Returns such a device, or NULL if none could be found.
  */
 struct device *of_platform_device_find(struct device_node *np)
 {
     struct device_node *target;
-    struct platform_device *pdev;
+    struct platform_device *pdev = NULL;

     of_node_get(np);

     for (target = np;
          !of_node_is_root(target);
          target = of_get_next_parent(target))
-        if (of_is_platform(target))
+        if (target->platform_dev) {
+            pdev = target->platform_dev;
             break;
-
-    pdev = of_find_device_by_node(target);
+        }

     of_node_put(target);

Thanks,

Tomeu

> Rob
> --
> 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/

[PATCH v2 05/22] ACPI: add acpi_dev_get_device()

[Tomeu Vizoso]

This function is just a getter for struct acpi_device.dev and is needed
in the implementation of the fwnode API when building with !CONFIG_ACPI.

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

Changes in v2:
- Add acpi_dev_get_device()

 include/linux/acpi.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 01e6770d8e27..d1ad6c20c7e4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -78,6 +78,11 @@ static inline void acpi_preset_companion(struct device *dev,
 	ACPI_COMPANION_SET(dev, acpi_find_child_device(parent, addr, NULL));
 }
 
+static inline struct device *acpi_dev_get_device(struct acpi_device *adev)
+{
+	return &adev->dev;
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return dev_name(&adev->dev);
@@ -476,6 +481,11 @@ static inline bool has_acpi_companion(struct device *dev)
 	return false;
 }
 
+static inline struct device *acpi_dev_get_device(struct acpi_device *adev)
+{
+	return NULL;
+}
+
 static inline const char *acpi_dev_name(struct acpi_device *adev)
 {
 	return NULL;
-- 
2.4.3

Re: [PATCH v2 05/22] ACPI: add acpi_dev_get_device()

[Rob Herring]

On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
<tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> This function is just a getter for struct acpi_device.dev and is needed
> in the implementation of the fwnode API when building with !CONFIG_ACPI.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> ---
>
> Changes in v2:
> - Add acpi_dev_get_device()
>
>  include/linux/acpi.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 01e6770d8e27..d1ad6c20c7e4 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -78,6 +78,11 @@ static inline void acpi_preset_companion(struct device *dev,
>         ACPI_COMPANION_SET(dev, acpi_find_child_device(parent, addr, NULL));
>  }
>
> +static inline struct device *acpi_dev_get_device(struct acpi_device *adev)

get usually implies reference counting. Perhaps acpi_dev_to_device or
acpi_dev_find_device.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 06/22] device property: add fwnode_ensure_device()

[Tomeu Vizoso]

Checks if the device associated with this firmware node has been already
probed, and probes it if not.

It can be used by resource getters to make sure that the requested
resource is available, if at all possible.

For OF nodes, it finds the platform device that encloses this node and
tries to probe it.

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

Changes in v2:
- Add fwnode_ensure_device() so the mechanism for probing devices on
  demand is independent of the firmware format.

 drivers/base/property.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 60 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index efa74803af30..a63d9bc3d8f7 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -15,8 +15,11 @@
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/property.h>
 
+#include "base.h"
+
 /**
  * device_add_property_set - Add a collection of properties to a device object.
  * @dev: Device to add properties to.
@@ -548,3 +551,58 @@ const char *fwnode_get_name(struct fwnode_handle *fwnode)
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_name);
+
+/**
+ * fwnode_ensure_device - probes the device associated to this firmware node
+ * @fwnode: Firmware node whose device is to be probed
+ *
+ * Checks if the device associated with this firmware node has been already
+ * probed, and probes it if not.  It can be used by resource getters to make
+ * sure that the requested resource is available, if at all possible.
+ */
+void fwnode_ensure_device(struct fwnode_handle *fwnode)
+{
+	struct device *dev = NULL;
+
+	/*
+	 * In general, OF nodes don't have a single device associated to them,
+	 * but those directly beneath the root of the tree or that are
+	 * children of simple memory mapped bus nodes do have just one
+	 * platform device. See Documentation/devicetree/usage-model.txt for
+	 * a more extended explanation.
+	 *
+	 * of_platform_device_find() will return such a platform device and
+	 * probing it should cause the target device to be probed as well
+	 * because platform devices are expected to register their children
+	 * when probing.
+	 */
+	if (is_of_node(fwnode))
+		dev = of_platform_device_find(to_of_node(fwnode));
+	else if (is_acpi_node(fwnode))
+		dev = acpi_dev_get_device(to_acpi_node(fwnode));
+
+	if (!dev) {
+		/*
+		 * Cannot be a warning because some fwnodes will have
+		 * matching data but aren't really supposed to be probed.
+		 */
+		pr_debug("Couldn't find device for %s\n",
+			 fwnode_get_name(fwnode));
+		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;
+
+	bus_probe_device(dev);
+
+	if (!dev->driver)
+		pr_warn("Probe failed for %s (%s)\n",
+			fwnode_get_name(fwnode), dev_name(dev));
+}
+EXPORT_SYMBOL_GPL(fwnode_ensure_device);
diff --git a/include/linux/property.h b/include/linux/property.h
index 826f156f7288..6b99296bcad7 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -168,4 +168,6 @@ void device_add_property_set(struct device *dev, struct property_set *pset);
 
 bool device_dma_is_coherent(struct device *dev);
 
+void fwnode_ensure_device(struct fwnode_handle *fwnode);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.4.3

[PATCH v2 07/22] gpio: Probe GPIO drivers on demand

[Tomeu Vizoso]

When looking up a gpiochip through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/gpio/gpiolib-of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index fa6e3c8823d6..31a6b18a6542 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -95,6 +95,8 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
 		return ERR_PTR(ret);
 	}
 
+	fwnode_ensure_device(&gg_data.gpiospec.np->fwnode);
+
 	gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
 
 	of_node_put(gg_data.gpiospec.np);
-- 
2.4.3

[PATCH v2 08/22] gpio: Probe pinctrl devices on demand

[Tomeu Vizoso]

When looking up a pin controller through its firmware node, probe it if
it hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/gpio/gpiolib-of.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 31a6b18a6542..2a6379c1fea7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -358,6 +358,8 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 		if (ret)
 			break;
 
+		fwnode_ensure_device(&pinspec.np->fwnode);
+
 		pctldev = of_pinctrl_get(pinspec.np);
 		if (!pctldev)
 			return -EPROBE_DEFER;
-- 
2.4.3

[PATCH v2 09/22] regulator: core: Reduce critical area in _regulator_get

[Tomeu Vizoso]

...by moving the locking of regulator_list_mutex into
regulator_dev_lookup(), where it is iterated over.

The regulator device lock gets acquired before returning, and the caller
is responsible for releasing it after it's done with the regulator
device.

In _regulator_get() the regulator_list_mutex mutex is held for most of
the function, but is only strictly needed to protect the list lookups.

This change would be useful if for example regulator devices could be
registered on demand when a driver requests them. regulator_register()
could end up being called from within _regulator_get while the lock on
regulator_list_mutex is being held, causing a deadlock.

This backtrace illustrates the situation described above:

(regulator_register) from [<c05efe64>]
(devm_regulator_register+0x48/0x84)
(devm_regulator_register) from [<c05f0b20>]
(reg_fixed_voltage_probe+0x214/0x35c)
(reg_fixed_voltage_probe) from [<c06cc7fc>]
(platform_drv_probe+0x54/0xbc)
(platform_drv_probe) from [<c06caac8>] (driver_probe_device+0x184/0x2c4)
(driver_probe_device) from [<c06cac58>] (__device_attach+0x50/0x54)
(__device_attach) from [<c06c8eac>] (bus_for_each_drv+0x70/0xa4)
(bus_for_each_drv) from [<c06ca900>] (device_attach+0x90/0xa4)
(device_attach) from [<c06c9eb4>] (bus_probe_device+0x94/0xb8)
(bus_probe_device) from [<c06c7de8>] (device_add+0x384/0x580)
(device_add) from [<c095c104>] (of_device_add+0x44/0x4c)
(of_device_add) from [<c095c968>]
...
(regulator_dev_lookup) from [<c05ee7c0>] (_regulator_get+0x8c/0x26c)
(_regulator_get) from [<c05ee9c0>] (regulator_get+0x20/0x24)
(regulator_get) from [<c05efb1c>] (_devm_regulator_get+0xa4/0xc8)
(_devm_regulator_get) from [<c05efb5c>] (devm_regulator_get+0x1c/0x20)
(devm_regulator_get) from [<c06ba870>] (tegra_hdmi_probe+0xe0/0x278)
(tegra_hdmi_probe) from [<c06cc7fc>] (platform_drv_probe+0x54/0xbc)

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

Changes in v2:
- Acquire regulator device lock before returning from regulator_dev_lookup()

 drivers/regulator/core.c | 98 +++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 613034667b93..65a3f28d452c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -110,6 +110,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
 static void _regulator_put(struct regulator *regulator);
+static int _regulator_enable(struct regulator *regulator, bool do_lock);
 
 static const char *rdev_get_name(struct regulator_dev *rdev)
 {
@@ -1212,6 +1213,7 @@ static void unset_regulator_supplies(struct regulator_dev *rdev)
 
 #define REG_STR_SIZE	64
 
+/* rdev->mutex held by caller */
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name)
@@ -1224,7 +1226,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	if (regulator == NULL)
 		return NULL;
 
-	mutex_lock(&rdev->mutex);
+	lockdep_assert_held_once(&rdev->mutex);
 	regulator->rdev = rdev;
 	list_add(&regulator->list, &rdev->consumer_list);
 
@@ -1276,12 +1278,10 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
-	mutex_unlock(&rdev->mutex);
 	return regulator;
 overflow_err:
 	list_del(&regulator->list);
 	kfree(regulator);
-	mutex_unlock(&rdev->mutex);
 	return NULL;
 }
 
@@ -1320,6 +1320,7 @@ static void regulator_supply_alias(struct device **dev, const char **supply)
 	}
 }
 
+/* Caller has to release r->mutex once done with r */
 static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 						  const char *supply,
 						  int *ret)
@@ -1335,10 +1336,15 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev && dev->of_node) {
 		node = of_get_regulator(dev, supply);
 		if (node) {
+			mutex_lock(&regulator_list_mutex);
 			list_for_each_entry(r, &regulator_list, list)
 				if (r->dev.parent &&
-					node == r->dev.of_node)
+					node == r->dev.of_node) {
+					mutex_lock(&r->mutex);
+					mutex_unlock(&regulator_list_mutex);
 					return r;
+				}
+			mutex_unlock(&regulator_list_mutex);
 			*ret = -EPROBE_DEFER;
 			return NULL;
 		} else {
@@ -1356,9 +1362,14 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev)
 		devname = dev_name(dev);
 
+	mutex_lock(&regulator_list_mutex);
 	list_for_each_entry(r, &regulator_list, list)
-		if (strcmp(rdev_get_name(r), supply) == 0)
+		if (strcmp(rdev_get_name(r), supply) == 0) {
+			mutex_lock(&r->mutex);
+			mutex_unlock(&regulator_list_mutex);
 			return r;
+		}
+	mutex_unlock(&regulator_list_mutex);
 
 	list_for_each_entry(map, &regulator_map_list, list) {
 		/* If the mapping has a device set up it must match */
@@ -1400,6 +1411,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (!r) {
 		if (have_full_constraints()) {
 			r = dummy_regulator_rdev;
+			mutex_lock(&r->mutex);
 		} else {
 			dev_err(dev, "Failed to resolve %s-supply for %s\n",
 				rdev->supply_name, rdev->desc->name);
@@ -1410,23 +1422,25 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	/* Recursively resolve the supply of the supply */
 	ret = regulator_resolve_supply(r);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = set_supply(rdev, r);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	/* Cascade always-on state to supply */
 	if (_regulator_is_enabled(rdev)) {
-		ret = regulator_enable(rdev->supply);
+		ret = _regulator_enable(rdev->supply, false);
 		if (ret < 0) {
 			if (rdev->supply)
 				_regulator_put(rdev->supply);
-			return ret;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	mutex_unlock(&r->mutex);
+	return ret;
 }
 
 /* Internal regulator request function */
@@ -1451,8 +1465,6 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 	else
 		ret = -EPROBE_DEFER;
 
-	mutex_lock(&regulator_list_mutex);
-
 	rdev = regulator_dev_lookup(dev, id, &ret);
 	if (rdev)
 		goto found;
@@ -1478,13 +1490,13 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 			devname, id);
 
 		rdev = dummy_regulator_rdev;
+		mutex_lock(&rdev->mutex);
 		goto found;
 	/* Don't log an error when called from regulator_get_optional() */
 	} else if (!have_full_constraints() || exclusive) {
 		dev_warn(dev, "dummy supplies not allowed\n");
 	}
 
-	mutex_unlock(&regulator_list_mutex);
 	return regulator;
 
 found:
@@ -1526,8 +1538,7 @@ found:
 	}
 
 out:
-	mutex_unlock(&regulator_list_mutex);
-
+	mutex_unlock(&rdev->mutex);
 	return regulator;
 }
 
@@ -1987,11 +1998,22 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 }
 
 /* locks held by regulator_enable() */
-static int _regulator_enable(struct regulator_dev *rdev)
+static int _regulator_enable(struct regulator *regulator, bool do_lock)
 {
-	int ret;
+	struct regulator_dev *rdev = regulator->rdev;
+	int ret = 0;
 
-	lockdep_assert_held_once(&rdev->mutex);
+	if (regulator->always_on)
+		return 0;
+
+	if (rdev->supply) {
+		ret = regulator_enable(rdev->supply);
+		if (ret != 0)
+			return ret;
+	}
+
+	if (do_lock)
+		mutex_lock(&rdev->mutex);
 
 	/* check voltage and requested load before enabling */
 	if (rdev->constraints &&
@@ -2002,23 +2024,32 @@ static int _regulator_enable(struct regulator_dev *rdev)
 		/* The regulator may on if it's not switchable or left on */
 		ret = _regulator_is_enabled(rdev);
 		if (ret == -EINVAL || ret == 0) {
-			if (!_regulator_can_change_status(rdev))
-				return -EPERM;
+			if (!_regulator_can_change_status(rdev)) {
+				ret = -EPERM;
+				goto out;
+			}
 
 			ret = _regulator_do_enable(rdev);
 			if (ret < 0)
-				return ret;
+				goto out;
 
 		} else if (ret < 0) {
 			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
-			return ret;
+			goto out;
 		}
 		/* Fallthrough on positive return values - already enabled */
 	}
 
 	rdev->use_count++;
 
-	return 0;
+out:
+	if (do_lock)
+		mutex_unlock(&rdev->mutex);
+
+	if (ret != 0 && rdev->supply)
+		regulator_disable(rdev->supply);
+
+	return ret;
 }
 
 /**
@@ -2034,26 +2065,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
  */
 int regulator_enable(struct regulator *regulator)
 {
-	struct regulator_dev *rdev = regulator->rdev;
-	int ret = 0;
-
-	if (regulator->always_on)
-		return 0;
-
-	if (rdev->supply) {
-		ret = regulator_enable(rdev->supply);
-		if (ret != 0)
-			return ret;
-	}
-
-	mutex_lock(&rdev->mutex);
-	ret = _regulator_enable(rdev);
-	mutex_unlock(&rdev->mutex);
-
-	if (ret != 0 && rdev->supply)
-		regulator_disable(rdev->supply);
-
-	return ret;
+	return _regulator_enable(regulator, true);
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
 
-- 
2.4.3

[PATCH v2 10/22] regulator: core: Probe regulators on demand

[Tomeu Vizoso]

When looking up a regulator through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/regulator/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 65a3f28d452c..568f095df351 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1336,6 +1336,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	if (dev && dev->of_node) {
 		node = of_get_regulator(dev, supply);
 		if (node) {
+			fwnode_ensure_device(&node->fwnode);
 			mutex_lock(&regulator_list_mutex);
 			list_for_each_entry(r, &regulator_list, list)
 				if (r->dev.parent &&
-- 
2.4.3

[PATCH v2 11/22] drm: Probe panels on demand

[Tomeu Vizoso]

When looking up a panel through its firmware node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/gpu/drm/drm_panel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e037b7..374a964c3842 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -80,6 +80,8 @@ struct drm_panel *of_drm_find_panel(struct device_node *np)
 {
 	struct drm_panel *panel;
 
+	fwnode_ensure_device(&np->fwnode);
+
 	mutex_lock(&panel_lock);
 
 	list_for_each_entry(panel, &panel_list, list) {
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 12/22] drm/tegra: Probe dpaux devices on demand

[Tomeu Vizoso]

When looking up a dpaux device through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/gpu/drm/tegra/dpaux.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index 07b26972f487..9a793158a571 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -394,6 +394,8 @@ struct tegra_dpaux *tegra_dpaux_find_by_of_node(struct device_node *np)
 {
 	struct tegra_dpaux *dpaux;
 
+	fwnode_ensure_device(&np->fwnode);
+
 	mutex_lock(&dpaux_lock);
 
 	list_for_each_entry(dpaux, &dpaux_list, list)
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 13/22] i2c: core: Probe i2c master devices on demand

[Tomeu Vizoso]

When looking up an i2c master through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/i2c/i2c-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e6d4935161e4..5520b413e3db 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1353,6 +1353,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 {
 	struct device *dev;
 
+	fwnode_ensure_device(&node->fwnode);
+
 	dev = bus_find_device(&i2c_bus_type, NULL, node,
 					 of_dev_node_match);
 	if (!dev)
-- 
2.4.3

Re: [PATCH v2 13/22] i2c: core: Probe i2c master devices on demand

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Tue, Jul 28, 2015 at 03:19:44PM +0200, Tomeu Vizoso wrote:
> When looking up an i2c master through its firmware node, probe it if it
> hasn't already.
> 
> The goal is to reduce deferred probes to a minimum, as it makes it very
> cumbersome to find out why a device failed to probe, and can introduce
> very big delays in when a critical device is probed.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

What is the status of this series? The boot time reduction sounds great.

> ---
> 
> Changes in v2: None
> 
>  drivers/i2c/i2c-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e6d4935161e4..5520b413e3db 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1353,6 +1353,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  {
>  	struct device *dev;
>  
> +	fwnode_ensure_device(&node->fwnode);

TBH, the function name doesn't tell me a lot. It ensures what?

> +
>  	dev = bus_find_device(&i2c_bus_type, NULL, node,
>  					 of_dev_node_match);
>  	if (!dev)
> -- 
> 2.4.3
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 13/22] i2c: core: Probe i2c master devices on demand

[Tomeu Vizoso]

On 9 August 2015 at 14:34, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Tue, Jul 28, 2015 at 03:19:44PM +0200, Tomeu Vizoso wrote:
>> When looking up an i2c master through its firmware node, probe it if it
>> hasn't already.
>>
>> The goal is to reduce deferred probes to a minimum, as it makes it very
>> cumbersome to find out why a device failed to probe, and can introduce
>> very big delays in when a critical device is probed.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>
> What is the status of this series?

See here for a summary:

http://lkml.kernel.org/g/1438870315-18689-1-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org

> The boot time reduction sounds great.

Note that this should not reduce total boot time substantially, as the
time wasted due to superfluous probe deferrals is really small when
compared to the time spent in delays due to hardware constraints. If
you want to reduce total boot time, async probing may be helpful in
some scenarios.

Regards,

Tomeu

>> ---
>>
>> Changes in v2: None
>>
>>  drivers/i2c/i2c-core.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index e6d4935161e4..5520b413e3db 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1353,6 +1353,8 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>>  {
>>       struct device *dev;
>>
>> +     fwnode_ensure_device(&node->fwnode);
>
> TBH, the function name doesn't tell me a lot. It ensures what?
>
>> +
>>       dev = bus_find_device(&i2c_bus_type, NULL, node,
>>                                        of_dev_node_match);
>>       if (!dev)
>> --
>> 2.4.3
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 14/22] pwm: Probe PWM chip devices on demand

[Tomeu Vizoso]

When looking up a PWM chip through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/pwm/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3a7769fe53de..4727cbf69d96 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -496,6 +496,8 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
 {
 	struct pwm_chip *chip;
 
+	fwnode_ensure_device(&np->fwnode);
+
 	mutex_lock(&pwm_lock);
 
 	list_for_each_entry(chip, &pwm_chips, list)
-- 
2.4.3

[PATCH v2 15/22] backlight: Probe backlight devices on demand

[Tomeu Vizoso]

When looking up a backlight device through its firmware node, probe it
if it hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/video/backlight/backlight.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index bddc8b17a4d8..f7151d916435 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -559,6 +559,8 @@ struct backlight_device *of_find_backlight_by_node(struct device_node *node)
 {
 	struct device *dev;
 
+	fwnode_ensure_device(&node->fwnode);
+
 	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
 
 	return dev ? to_backlight_device(dev) : NULL;
-- 
2.4.3

[PATCH v2 16/22] usb: phy: Probe phy devices on demand

[Tomeu Vizoso]

When looking up a phy through its firmware node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/usb/phy/phy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
index 98f75d2842b7..2d51c12d0726 100644
--- a/drivers/usb/phy/phy.c
+++ b/drivers/usb/phy/phy.c
@@ -196,6 +196,8 @@ struct  usb_phy *devm_usb_get_phy_by_node(struct device *dev,
 		goto err0;
 	}
 
+	fwnode_ensure_device(&node->fwnode);
+
 	spin_lock_irqsave(&phy_lock, flags);
 
 	phy = __of_usb_find_phy(node);
-- 
2.4.3

[PATCH v2 17/22] clk: Probe clk providers on demand

[Tomeu Vizoso]

When looking up a clock through its firmware node, probe it if it hasn't
already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7bb975747a2c..3cc951989a12 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2979,6 +2979,9 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 	if (!clkspec)
 		return ERR_PTR(-EINVAL);
 
+	if (clkspec->np)
+		fwnode_ensure_device(&clkspec->np->fwnode);
+
 	/* Check if we have such a provider in our array */
 	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(provider, &of_clk_providers, link) {
-- 
2.4.3

[PATCH v2 18/22] pinctrl: Probe pinctrl devices on demand

[Tomeu Vizoso]

When looking up a pin controller through its firmware node, probe it if
it hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/pinctrl/devicetree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fe04e748dfe4..490860f6374e 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -112,6 +112,7 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 
 	/* Find the pin controller containing np_config */
 	np_pctldev = of_node_get(np_config);
+	fwnode_ensure_device(&np_pctldev->fwnode);
 	for (;;) {
 		np_pctldev = of_get_next_parent(np_pctldev);
 		if (!np_pctldev || of_node_is_root(np_pctldev)) {
-- 
2.4.3

[PATCH v2 19/22] phy: core: Probe phy providers on demand

[Tomeu Vizoso]

When looking up a phy provider through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/phy/phy-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index fc48fac003a6..c27e7d3d86ee 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -363,6 +363,8 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
 	if (ret)
 		return ERR_PTR(-ENODEV);
 
+	fwnode_ensure_device(&args.np->fwnode);
+
 	mutex_lock(&phy_provider_mutex);
 	phy_provider = of_phy_provider_lookup(args.np);
 	if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner)) {
-- 
2.4.3

[PATCH v2 20/22] dma: of: Probe DMA controllers on demand

[Tomeu Vizoso]

When looking up a DMA controller through its firmware node, probe it if
it hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/dma/of-dma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 1e1f2986eba8..de411a6a63af 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -263,6 +263,8 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 		if (of_dma_match_channel(np, name, i, &dma_spec))
 			continue;
 
+		fwnode_ensure_device(&dma_spec.np->fwnode);
+
 		mutex_lock(&of_dma_lock);
 		ofdma = of_dma_find_controller(&dma_spec);
 
-- 
2.4.3

[PATCH v2 21/22] power-supply: Probe power supplies on demand

[Tomeu Vizoso]

When looking up a power supply through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 drivers/power/power_supply_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 456987c88baa..b51b0b850a17 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -206,6 +206,8 @@ static int power_supply_find_supply_from_node(struct device_node *supply_node)
 {
 	int error;
 
+	fwnode_ensure_device(&supply_node->fwnode);
+
 	/*
 	 * class_for_each_device() either returns its own errors or values
 	 * returned by __power_supply_find_supply_from_node().
-- 
2.4.3

[PATCH v2 22/22] ASoC: core: Probe components on demand

[Tomeu Vizoso]

When looking up a component through its firmware node, probe it if it
hasn't already.

The goal is to reduce deferred probes to a minimum, as it makes it very
cumbersome to find out why a device failed to probe, and can introduce
very big delays in when a critical device is probed.

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

Changes in v2: None

 sound/soc/soc-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c81aec9c872a..4c55da97f9ed 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -865,10 +865,12 @@ static const struct snd_soc_dai_ops null_dai_ops = {
 };
 
 static struct snd_soc_component *soc_find_component(
-	const struct device_node *of_node, const char *name)
+	struct device_node *of_node, const char *name)
 {
 	struct snd_soc_component *component;
 
+	fwnode_ensure_device(&of_node->fwnode);
+
 	lockdep_assert_held(&client_mutex);
 
 	list_for_each_entry(component, &component_list, list) {
@@ -890,6 +892,8 @@ static struct snd_soc_dai *snd_soc_find_dai(
 	struct snd_soc_dai *dai;
 	struct device_node *component_of_node;
 
+	fwnode_ensure_device(&dlc->of_node->fwnode);
+
 	lockdep_assert_held(&client_mutex);
 
 	/* Find CPU DAI from registered DAIs*/
-- 
2.4.3

Re: [PATCH v2 0/22] On-demand device probing

[Rafael J. Wysocki]

On Tuesday, July 28, 2015 03:19:31 PM Tomeu Vizoso wrote:
> Hello,
> 
> I have a problem with the panel on my Tegra Chromebook taking longer
> than expected to be ready during boot (Stéphane Marchesin reported what
> is basically the same issue in [0]), and have looked into ordered
> probing as a better way of solving this than moving nodes around in the
> DT or playing with initcall levels and linking order.
> 
> While reading the thread [1] that Alexander Holler started with his
> series to make probing order deterministic, it occurred to me that it
> should be possible to achieve the same by probing devices as they are
> referenced by other devices.
> 
> This basically reuses the information that is already implicit in the
> probe() implementations, saving us from refactoring existing drivers or
> adding information to DTBs.
> 
> During review of v1 of this series Linus Walleij suggested that it
> should be the device driver core to make sure that dependencies are
> ready before probing a device. I gave this idea a try [2] but Mark Brown
> pointed out to the logic duplication between the resource acquisition
> and dependency discovery code paths (though I think it's fairly minor).
> 
> To address that code duplication I experimented with Arnd's devm_probe
> [3] concept of having drivers declare their dependencies instead of
> acquiring them during probe, and while it worked [4], I don't think we
> end up winning anything when compared to just probing devices on-demand
> from resource getters.
> 
> One remaining objection is to the "sprinkling" of calls to
> fwnode_ensure_device() in the resource getters of each subsystem, but I
> think it's the right thing to do given that the storage of resources is
> currently subsystem-specific.
> 
> We could avoid the above by moving resource storage into the core, but I
> don't think there's a compelling case for that.
> 
> I have tested this on boards with Tegra, iMX.6, Exynos and OMAP SoCs,
> and these patches were enough to eliminate all the deferred probes
> (except one in PandaBoard because omap_dma_system doesn't have a
> firmware node as of yet).
> 
> With this series I get the kernel to output to the panel in 0.5s,
> instead of 2.8s.

Can you trim your CC list somewhat, please?

I'm definitely going to look at this, but not before then next week.
Sorry about that.

Thanks,
Rafael

Re: [PATCH v2 0/22] On-demand device probing

[Rob Herring]

On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> Hello,
>
> I have a problem with the panel on my Tegra Chromebook taking longer
> than expected to be ready during boot (Stéphane Marchesin reported what
> is basically the same issue in [0]), and have looked into ordered
> probing as a better way of solving this than moving nodes around in the
> DT or playing with initcall levels and linking order.
>
> While reading the thread [1] that Alexander Holler started with his
> series to make probing order deterministic, it occurred to me that it
> should be possible to achieve the same by probing devices as they are
> referenced by other devices.
>
> This basically reuses the information that is already implicit in the
> probe() implementations, saving us from refactoring existing drivers or
> adding information to DTBs.
>
> During review of v1 of this series Linus Walleij suggested that it
> should be the device driver core to make sure that dependencies are
> ready before probing a device. I gave this idea a try [2] but Mark Brown
> pointed out to the logic duplication between the resource acquisition
> and dependency discovery code paths (though I think it's fairly minor).
>
> To address that code duplication I experimented with Arnd's devm_probe
> [3] concept of having drivers declare their dependencies instead of
> acquiring them during probe, and while it worked [4], I don't think we
> end up winning anything when compared to just probing devices on-demand
> from resource getters.
>
> One remaining objection is to the "sprinkling" of calls to
> fwnode_ensure_device() in the resource getters of each subsystem, but I
> think it's the right thing to do given that the storage of resources is
> currently subsystem-specific.

Seems like a minor change to me.

> We could avoid the above by moving resource storage into the core, but I
> don't think there's a compelling case for that.
>
> I have tested this on boards with Tegra, iMX.6, Exynos and OMAP SoCs,
> and these patches were enough to eliminate all the deferred probes
> (except one in PandaBoard because omap_dma_system doesn't have a
> firmware node as of yet).
>
> With this series I get the kernel to output to the panel in 0.5s,
> instead of 2.8s.

Generally, I think this looks pretty good. It is simple and the error
path is simply falling back to deferred probe.

One overall comment is I'm not so sure if fwnode_ensure_device
shouldn't just be of_ensure_device. At least currently, it looks like
all the calling locations are DT specific functions anyway. There's
very little logic within the function to really benefit sharing with
ACPI. It is basically just a call to of_platform_device_find and then
bus_probe_device. I expect the get functions will always call into
DT/ACPI specific functions which can then call the firmware specific
device find function.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/22] On-demand device probing

[Tomeu Vizoso]

On 30 July 2015 at 05:06, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:19 AM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> Hello,
>>
>> I have a problem with the panel on my Tegra Chromebook taking longer
>> than expected to be ready during boot (Stéphane Marchesin reported what
>> is basically the same issue in [0]), and have looked into ordered
>> probing as a better way of solving this than moving nodes around in the
>> DT or playing with initcall levels and linking order.
>>
>> While reading the thread [1] that Alexander Holler started with his
>> series to make probing order deterministic, it occurred to me that it
>> should be possible to achieve the same by probing devices as they are
>> referenced by other devices.
>>
>> This basically reuses the information that is already implicit in the
>> probe() implementations, saving us from refactoring existing drivers or
>> adding information to DTBs.
>>
>> During review of v1 of this series Linus Walleij suggested that it
>> should be the device driver core to make sure that dependencies are
>> ready before probing a device. I gave this idea a try [2] but Mark Brown
>> pointed out to the logic duplication between the resource acquisition
>> and dependency discovery code paths (though I think it's fairly minor).
>>
>> To address that code duplication I experimented with Arnd's devm_probe
>> [3] concept of having drivers declare their dependencies instead of
>> acquiring them during probe, and while it worked [4], I don't think we
>> end up winning anything when compared to just probing devices on-demand
>> from resource getters.
>>
>> One remaining objection is to the "sprinkling" of calls to
>> fwnode_ensure_device() in the resource getters of each subsystem, but I
>> think it's the right thing to do given that the storage of resources is
>> currently subsystem-specific.
>
> Seems like a minor change to me.
>
>> We could avoid the above by moving resource storage into the core, but I
>> don't think there's a compelling case for that.
>>
>> I have tested this on boards with Tegra, iMX.6, Exynos and OMAP SoCs,
>> and these patches were enough to eliminate all the deferred probes
>> (except one in PandaBoard because omap_dma_system doesn't have a
>> firmware node as of yet).
>>
>> With this series I get the kernel to output to the panel in 0.5s,
>> instead of 2.8s.
>
> Generally, I think this looks pretty good. It is simple and the error
> path is simply falling back to deferred probe.
>
> One overall comment is I'm not so sure if fwnode_ensure_device
> shouldn't just be of_ensure_device. At least currently, it looks like
> all the calling locations are DT specific functions anyway. There's
> very little logic within the function to really benefit sharing with
> ACPI. It is basically just a call to of_platform_device_find and then
> bus_probe_device. I expect the get functions will always call into
> DT/ACPI specific functions which can then call the firmware specific
> device find function.

That's fine with me. I just went that way because I assumed the plan
was for subsystems to move to consume fw data through fwnode and drop
as much fw-specific code as possible.

But I have just looked at fwnode_get_named_gpiod and the OF and ACPI
code paths are so dissimilar that I guess that's not so and would be
better to do as you say.

Thanks,

Tomeu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel