[PATCH v2 0/12] Discover and probe dependencies

[PATCH v2 0/12] Discover and probe dependencies

[Tomeu Vizoso]

Hi,

this is version 2 of a series that probes devices in dependency order so
as to avoid deferred probes. While deferred probing is a powerful
solution that makes sure that you eventually get a working system at the
end of the boot, can make it very time consuming to find out why a
device didn't probe and can also introduce big delays in when a device
actually probes by sending it to the end of the deferred queue.

So far I have only tested on a Tegra124 Chromebook.

Thanks,

Tomeu

Changes in v2:
- Instead of delaying all probes until late_initcall, only delay matches
  of platform devices that have a firmware node attached.
- Allow bindings implementations register a function instead of using
  class callbacks, as not only subsystems implement firmware bindings.
- Move strends to string.h
- Document that consumers of backlight devices can use the 'backlight'
  property to hold a phandle to the backlight device.
- Allocate the list of dependencies and pass it to the function that
  fills it.

Tomeu Vizoso (12):
  device: property: delay device-driver matches
  device: property: find dependencies of a firmware node
  string: Introduce strends()
  gpio: register dependency parser for firmware nodes
  gpu: host1x: register dependency parser for firmware nodes
  backlight: Document consumers of backlight nodes
  backlight: register dependency parser for firmware nodes
  USB: EHCI: register dependency parser for firmware nodes
  regulator: register dependency parser for firmware nodes
  pwm: register dependency parser for firmware nodes
  ASoC: tegra: register dependency parser for firmware nodes
  driver-core: probe dependencies before probing

 .../bindings/video/backlight/backlight.txt         |  22 ++++
 drivers/base/dd.c                                  | 139 +++++++++++++++++++++
 drivers/base/property.c                            | 120 ++++++++++++++++++
 drivers/gpio/gpiolib.c                             |  54 ++++++++
 drivers/gpu/host1x/dev.c                           |  26 ++++
 drivers/pwm/core.c                                 |  28 +++++
 drivers/regulator/core.c                           |  27 ++++
 drivers/usb/host/ehci-tegra.c                      |  16 +++
 drivers/video/backlight/backlight.c                |  16 +++
 include/linux/fwnode.h                             |   5 +
 include/linux/property.h                           |  12 ++
 include/linux/string.h                             |  13 ++
 sound/soc/tegra/tegra_max98090.c                   |  42 ++++++-
 13 files changed, 519 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/backlight.txt

-- 
2.4.1

[PATCH v2 05/12] gpu: host1x: register dependency parser for firmware nodes

[Tomeu Vizoso]

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

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

Changes in v2: None

 drivers/gpu/host1x/dev.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 53d3d1d..5bb10b8 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -212,6 +212,29 @@ static struct platform_driver tegra_host1x_driver = {
 	.remove = host1x_remove,
 };
 
+static void add_dependency(struct fwnode_handle *fwnode,
+			   const char *property,
+			   struct list_head *deps)
+{
+	struct device_node *np;
+
+	np = of_parse_phandle(to_of_node(fwnode), property, 0);
+	if (!np)
+		return;
+
+	fwnode_add_dependency(&np->fwnode, deps);
+}
+
+static void host1x_get_dependencies(struct fwnode_handle *fwnode,
+				    struct list_head *deps)
+{
+	add_dependency(fwnode, "nvidia,dpaux", deps);
+	add_dependency(fwnode, "nvidia,panel", deps);
+	add_dependency(fwnode, "nvidia,ddc-i2c-bus", deps);
+	add_dependency(fwnode, "nvidia,hpd-gpio", deps);
+	add_dependency(fwnode, "ddc-i2c-bus", deps);
+}
+
 static int __init tegra_host1x_init(void)
 {
 	int err;
@@ -228,6 +251,8 @@ static int __init tegra_host1x_init(void)
 	if (err < 0)
 		goto unregister_host1x;
 
+	fwnode_add_dependency_parser(host1x_get_dependencies);
+
 	return 0;
 
 unregister_host1x:
@@ -240,6 +265,7 @@ module_init(tegra_host1x_init);
 
 static void __exit tegra_host1x_exit(void)
 {
+	fwnode_remove_dependency_parser(host1x_get_dependencies);
 	platform_driver_unregister(&tegra_mipi_driver);
 	platform_driver_unregister(&tegra_host1x_driver);
 	bus_unregister(&host1x_bus_type);
-- 
2.4.1

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

[PATCH v2 08/12] USB: EHCI: register dependency parser for firmware nodes

[Tomeu Vizoso]

So others can find out whether a firmware node depends on a phy as
specified in bindings/usb/nvidia,tegra20-ehci.txt.

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

Changes in v2: None

 drivers/usb/host/ehci-tegra.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 4031b37..3665eaa 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -589,6 +589,18 @@ static const struct ehci_driver_overrides tegra_overrides __initconst = {
 	.reset			= tegra_ehci_reset,
 };
 
+static void tegra_ehci_get_dependencies(struct fwnode_handle *fwnode,
+					struct list_head *deps)
+{
+	struct device_node *np;
+
+	np = of_parse_phandle(to_of_node(fwnode), "nvidia,phy", 0);
+	if (!np)
+		return;
+
+	fwnode_add_dependency(&np->fwnode, deps);
+}
+
 static int __init ehci_tegra_init(void)
 {
 	if (usb_disabled())
@@ -611,6 +623,8 @@ static int __init ehci_tegra_init(void)
 	tegra_ehci_hc_driver.unmap_urb_for_dma = tegra_ehci_unmap_urb_for_dma;
 	tegra_ehci_hc_driver.hub_control = tegra_ehci_hub_control;
 
+	fwnode_add_dependency_parser(tegra_ehci_get_dependencies);
+
 	return platform_driver_register(&tegra_ehci_driver);
 }
 module_init(ehci_tegra_init);
@@ -618,6 +632,8 @@ module_init(ehci_tegra_init);
 static void __exit ehci_tegra_cleanup(void)
 {
 	platform_driver_unregister(&tegra_ehci_driver);
+
+	fwnode_remove_dependency_parser(tegra_ehci_get_dependencies);
 }
 module_exit(ehci_tegra_cleanup);
 
-- 
2.4.1

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

[PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

So others can find out what dependencies a nvidia,tegra-audio-max98090
device has, as specified in
bindings/sound/nvidia,tegra-audio-max98090.txt.

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

Changes in v2: None

 sound/soc/tegra/tegra_max98090.c | 42 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..0f7cbf3 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -316,7 +316,47 @@ static struct platform_driver tegra_max98090_driver = {
 	.probe = tegra_max98090_probe,
 	.remove = tegra_max98090_remove,
 };
-module_platform_driver(tegra_max98090_driver);
+
+static void add_dependency(struct fwnode_handle *fwnode,
+			   const char *property,
+			   struct list_head *deps)
+{
+	struct device_node *np;
+
+	np = of_parse_phandle(to_of_node(fwnode), property, 0);
+	if (!np)
+		return;
+
+	fwnode_add_dependency(&np->fwnode, deps);
+}
+
+static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
+					    struct list_head *deps)
+{
+	add_dependency(fwnode, "nvidia,i2s-controller", deps);
+	add_dependency(fwnode, "nvidia,audio-codec", deps);
+}
+
+static int __init tegra_max98090_init(void)
+{
+	int err;
+
+	err = platform_driver_register(&tegra_max98090_driver);
+	if (err < 0)
+		return err;
+
+	fwnode_add_dependency_parser(tegra_max98090_get_dependencies);
+
+	return 0;
+}
+module_init(tegra_max98090_init);
+
+static void __exit tegra_max98090_exit(void)
+{
+	fwnode_remove_dependency_parser(tegra_max98090_get_dependencies);
+	platform_driver_unregister(&tegra_max98090_driver);
+}
+module_exit(tegra_max98090_exit);
 
 MODULE_AUTHOR("Stephen Warren <swarren@nvidia.com>");
 MODULE_DESCRIPTION("Tegra max98090 machine ASoC driver");
-- 
2.4.1

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

Re: [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

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

On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:

> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
> +					    struct list_head *deps)
> +{
> +	add_dependency(fwnode, "nvidia,i2s-controller", deps);
> +	add_dependency(fwnode, "nvidia,audio-codec", deps);
> +}

Why is this all being open coded in an individual driver (we already
know about and manage all these dependencies in the core...)?  If we're
going to do this I'd expect the interface for specifying DT nodes to the
core to be changed to support this.

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

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

On 1 July 2015 at 19:38, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
>
>> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
>> +                                         struct list_head *deps)
>> +{
>> +     add_dependency(fwnode, "nvidia,i2s-controller", deps);
>> +     add_dependency(fwnode, "nvidia,audio-codec", deps);
>> +}
>
> Why is this all being open coded in an individual driver (we already
> know about and manage all these dependencies in the core...)?  If we're
> going to do this I'd expect the interface for specifying DT nodes to the
> core to be changed to support this.

Are you thinking of changing drivers to acquire their resources
through Arnd's devm_probe (only that the resource table would have to
be in struct device_driver)?

https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel

Sounds like lots of fun, but that means that any given machine will
get ordered probe only after all the drivers it uses have been moved
to the new declarative API.

TBH, that seems really far away.

Regards,

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

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

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

On Mon, Jul 13, 2015 at 02:10:45PM +0200, Tomeu Vizoso wrote:
> On 1 July 2015 at 19:38, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:

> >> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
> >> +                                         struct list_head *deps)
> >> +{
> >> +     add_dependency(fwnode, "nvidia,i2s-controller", deps);
> >> +     add_dependency(fwnode, "nvidia,audio-codec", deps);
> >> +}

> > Why is this all being open coded in an individual driver (we already
> > know about and manage all these dependencies in the core...)?  If we're
> > going to do this I'd expect the interface for specifying DT nodes to the
> > core to be changed to support this.

> Are you thinking of changing drivers to acquire their resources
> through Arnd's devm_probe (only that the resource table would have to
> be in struct device_driver)?

> https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel

No, I'm looking at how we already have all the "did all my dependencies
appear" logic in the core based on data provided by the drivers.

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

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

On 13 July 2015 at 17:42, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 13, 2015 at 02:10:45PM +0200, Tomeu Vizoso wrote:
>> On 1 July 2015 at 19:38, Mark Brown <broonie@kernel.org> wrote:
>> > On Wed, Jul 01, 2015 at 11:41:06AM +0200, Tomeu Vizoso wrote:
>
>> >> +static void tegra_max98090_get_dependencies(struct fwnode_handle *fwnode,
>> >> +                                         struct list_head *deps)
>> >> +{
>> >> +     add_dependency(fwnode, "nvidia,i2s-controller", deps);
>> >> +     add_dependency(fwnode, "nvidia,audio-codec", deps);
>> >> +}
>
>> > Why is this all being open coded in an individual driver (we already
>> > know about and manage all these dependencies in the core...)?  If we're
>> > going to do this I'd expect the interface for specifying DT nodes to the
>> > core to be changed to support this.
>
>> Are you thinking of changing drivers to acquire their resources
>> through Arnd's devm_probe (only that the resource table would have to
>> be in struct device_driver)?
>
>> https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel
>
> No, I'm looking at how we already have all the "did all my dependencies
> appear" logic in the core based on data provided by the drivers.

Sorry, but I still don't get what you mean.

Information about dependencies is currently available only after
probe() starts executing, and used to decide whether we want to defer
the probe.

The goal of this series is to eliminate most or all of the deferred
probes by checking that all dependencies are available before probe()
is called.

Because currently we only have dependency information after probe()
starts executing, we have to make it available earlier. In this
particular version, in callbacks that are registered from the
initcalls that register subsystems, classes, drivers, etc. Whatever
knows how these dependencies are expressed in the firmware data.

I thought you were pointing out that the property names would be
duplicated, once in the probe() implementation and also in the
implementation of the get_dependencies callback.

A way to consolidate the code and remove that duplication would be
having a declarative API for expressing dependencies, which could be
used for both fetching dependencies and for preventing deferred
probes. That's why I mentioned devm_probe.

Thanks,

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

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

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

On Tue, Jul 14, 2015 at 09:34:22AM +0200, Tomeu Vizoso wrote:
> On 13 July 2015 at 17:42, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > No, I'm looking at how we already have all the "did all my dependencies
> > appear" logic in the core based on data provided by the drivers.

> Sorry, but I still don't get what you mean.

I'm not sure how I can be clearer here...  you're replacing something
that is currently pure data with open coding in each device.  That seems
like a step back in terms of ease of use.

> Information about dependencies is currently available only after
> probe() starts executing, and used to decide whether we want to defer
> the probe.

> The goal of this series is to eliminate most or all of the deferred
> probes by checking that all dependencies are available before probe()
> is called.

Right, but the way it does this is by moving code out of the core into
the drivers - currently drivers just tell the core what resources to
look up and the core then makes sure that they're all present.

> I thought you were pointing out that the property names would be
> duplicated, once in the probe() implementation and also in the
> implementation of the get_dependencies callback.

Yes, that is another part of issue with this approach - drivers now have
to specify things twice, once for this new interface and once for
actually looking things up.  That doesn't seem awesome and adding the
code into the individual drivers and then having to pull it out again
when the redundancy is removed is going to be an enormous amount of
churn.

> A way to consolidate the code and remove that duplication would be
> having a declarative API for expressing dependencies, which could be
> used for both fetching dependencies and for preventing deferred
> probes. That's why I mentioned devm_probe.

Part of what I'm saying here is that in ASoC we already have (at least
as far as the individual drivers are concerned) a declarative way of
specifying dependencies.  This new code should be able to make use of
that, if it can't and especially if none of the code can be shared
between drivers then that seems like the interface needs another spin.

I've not seen this devm_probe() code but the name sounds worryingly like
it might be fixing the wrong problem :/

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

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Tomeu Vizoso]

On 14 July 2015 at 13:07, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 14, 2015 at 09:34:22AM +0200, Tomeu Vizoso wrote:
>> On 13 July 2015 at 17:42, Mark Brown <broonie@kernel.org> wrote:
>
>> > No, I'm looking at how we already have all the "did all my dependencies
>> > appear" logic in the core based on data provided by the drivers.
>
>> Sorry, but I still don't get what you mean.
>
> I'm not sure how I can be clearer here...  you're replacing something
> that is currently pure data with open coding in each device.  That seems
> like a step back in terms of ease of use.

I could understand that if snd_soc_dai_link had a field with the
property name, and the core called of_parse_phandle on it, but
currently what I'm duplicating is:

    tegra_max98090_dai.cpu_of_node = of_parse_phandle(np,
            "nvidia,i2s-controller", 0);

with:

    add_dependency(fwnode, "nvidia,i2s-controller", deps);

Admittedly, we could add a cpu_fw_property field to snd_soc_dai_link
and have the core call of_parse_phandle itself.

But even then, the core doesn't know about a device's snd_soc_dai_link
until probe() is called and then it's too late for the purposes of
this series.

That's why I mentioned devm_probe, as it would add a common way to
specify the data needed to acquire resources in each driver, which
could be made available before probe() is called.

From the proof of concept that Arnd sent in
https://lkml.kernel.org/g/4742258.TBitC3hVuO@wuerfel :

struct foo_priv {
        spinlock_t lock;
        void __iomem *regs;
        int irq;
        struct gpio_desc *gpio;
        struct dma_chan *rxdma;
        struct dma_chan *txdma;
        bool oldstyle_dma;
};

/*
 * this lists all properties we access from the driver. The list
 * is interpreted by devm_probe() and can be programmatically
 * verified to match the binding.
 */
static const struct devm_probe foo_probe_list[] = {
        DEVM_ALLOC(foo_priv),
        DEVM_IOMAP(foo_priv, regs, 0, 0),
        DEVM_PROP_BOOL(foo_priv, oldstyle_dma, "foo,oldstyle-dma"),
        DEVM_DMA_SLAVE(foo_priv, rxdma, "rx");
        DEVM_DMA_SLAVE(foo_priv, txdma, "tx");
        DEVM_GPIO(foo_priv, gpio, 0);
        DEVM_IRQ_NAMED(foo_priv, irq, foo_irq_handler, "fifo", IRQF_SHARED),
        {},
};

Thanks,

Tomeu

>> Information about dependencies is currently available only after
>> probe() starts executing, and used to decide whether we want to defer
>> the probe.
>
>> The goal of this series is to eliminate most or all of the deferred
>> probes by checking that all dependencies are available before probe()
>> is called.
>
> Right, but the way it does this is by moving code out of the core into
> the drivers - currently drivers just tell the core what resources to
> look up and the core then makes sure that they're all present.
>
>> I thought you were pointing out that the property names would be
>> duplicated, once in the probe() implementation and also in the
>> implementation of the get_dependencies callback.
>
> Yes, that is another part of issue with this approach - drivers now have
> to specify things twice, once for this new interface and once for
> actually looking things up.  That doesn't seem awesome and adding the
> code into the individual drivers and then having to pull it out again
> when the redundancy is removed is going to be an enormous amount of
> churn.
>
>> A way to consolidate the code and remove that duplication would be
>> having a declarative API for expressing dependencies, which could be
>> used for both fetching dependencies and for preventing deferred
>> probes. That's why I mentioned devm_probe.
>
> Part of what I'm saying here is that in ASoC we already have (at least
> as far as the individual drivers are concerned) a declarative way of
> specifying dependencies.  This new code should be able to make use of
> that, if it can't and especially if none of the code can be shared
> between drivers then that seems like the interface needs another spin.
>
> I've not seen this devm_probe() code but the name sounds worryingly like
> it might be fixing the wrong problem :/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH v2 11/12] ASoC: tegra: register dependency parser for firmware nodes

[Mark Brown]

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

On Tue, Jul 14, 2015 at 02:47:04PM +0200, Tomeu Vizoso wrote:
> On 14 July 2015 at 13:07, Mark Brown <broonie@kernel.org> wrote:

> > I'm not sure how I can be clearer here...  you're replacing something
> > that is currently pure data with open coding in each device.  That seems
> > like a step back in terms of ease of use.

> I could understand that if snd_soc_dai_link had a field with the
> property name, and the core called of_parse_phandle on it, but
> currently what I'm duplicating is:

>     tegra_max98090_dai.cpu_of_node = of_parse_phandle(np,
>             "nvidia,i2s-controller", 0);

> with:

>     add_dependency(fwnode, "nvidia,i2s-controller", deps);

> Admittedly, we could add a cpu_fw_property field to snd_soc_dai_link
> and have the core call of_parse_phandle itself.

Yes, we could - that's really what should be happening here.  The other
bit of this is that we're doing it twice which isn't success.

> But even then, the core doesn't know about a device's snd_soc_dai_link
> until probe() is called and then it's too late for the purposes of
> this series.

That's not a good reason to encourage bad patterns in drivers.  At the
very least the drivers should be able to pass the same struct into both
places, having to open code the same thing in two places is going to be
error prone.

> That's why I mentioned devm_probe, as it would add a common way to
> specify the data needed to acquire resources in each driver, which
> could be made available before probe() is called.

That does avoid the duplication.  However there are issues with the
interface for enumerable buses, it doesn't solve the problem where
embedded systems need you to power up the device manually prior to the
device actually enumerating.  If we're doing early resource acquisition
we probably want to solve that too.

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