* [PATCH v2 0/2] Fix simple-bus issues with fw_devlink
@ 2021-09-02 23:04 Saravana Kannan
2021-09-02 23:04 ` [PATCH v2 1/2] of: platform: Make sure bus only devices get probed Saravana Kannan
2021-09-02 23:04 ` [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan
0 siblings, 2 replies; 13+ messages in thread
From: Saravana Kannan @ 2021-09-02 23:04 UTC (permalink / raw)
To: Rob Herring, Frank Rowand
Cc: Saravana Kannan, Ulf Hansson, Geert Uytterhoeven, kernel-team,
linux-kernel, devicetree
Ulf reported an issue[1] with fw_devlink. This series tries to fix that
issue.
I replicated a similar set up on my end and I confirmed these patches work.
Ulf, mind testing this?
Thanks,
Saravana
[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Saravana Kannan (2):
of: platform: Make sure bus only devices get probed
drivers: bus: simple-pm-bus: Add support for probing simple bus only
devices
drivers/bus/simple-pm-bus.c | 7 ++++++
drivers/of/platform.c | 45 +++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+)
--
2.33.0.153.gba50c8fa24-goog
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-02 23:04 [PATCH v2 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan @ 2021-09-02 23:04 ` Saravana Kannan 2021-09-03 9:18 ` Ulf Hansson 2021-09-02 23:04 ` [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 1 sibling, 1 reply; 13+ messages in thread From: Saravana Kannan @ 2021-09-02 23:04 UTC (permalink / raw) To: Rob Herring, Frank Rowand Cc: Saravana Kannan, Ulf Hansson, Geert Uytterhoeven, kernel-team, linux-kernel, devicetree fw_devlink could end up creating device links for bus only devices. However, bus only devices don't get probed and can block probe() or sync_state() [1] call backs of other devices. To avoid this, set up these devices to get probed by the simple-pm-bus. [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ Signed-off-by: Saravana Kannan <saravanak@google.com> Tested-by: Saravana Kannan <saravanak@google.com> --- drivers/of/platform.c | 45 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 74afbb7a4f5e..b010e2310131 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -97,6 +97,48 @@ static void of_device_make_bus_id(struct device *dev) } } +/** + * of_match_only_simple_bus - Check if a device node is only a simple bus + * @np: device node to check + * + * A simple bus in this context is defined as a transparent bus whose child + * devices are automatically populated but has no hardware specific + * functionality. + * + * Returns true if the device node is only a simple bus and can never match + * with any other specific driver. Otherwise, returns false. + */ +static bool of_match_only_simple_bus(struct device_node *np) +{ + /* List of buses that don't have an explicit driver. */ + static const char * const of_simple_bus_table[] = { + "simple-bus", + "simple-mfd", + "isa", + "arm,amba-bus", + }; + const char *cp; + struct property *prop; + int i; + + prop = of_find_property(np, "compatible", NULL); + for (cp = of_prop_next_string(prop, NULL); cp; + cp = of_prop_next_string(prop, cp)) { + bool match = false; + const char *bus = of_simple_bus_table[i]; + + for (i = 0; i < ARRAY_SIZE(of_simple_bus_table); i++) + if (!of_compat_cmp(cp, bus, strlen(bus))) { + match = true; + break; + } + if (!match) + return false; + } + + return true; +} + /** * of_device_alloc - Allocate and initialize an of_device * @np: device node to assign to device @@ -143,6 +185,9 @@ struct platform_device *of_device_alloc(struct device_node *np, dev->dev.fwnode = &np->fwnode; dev->dev.parent = parent ? : &platform_bus; + if (of_match_only_simple_bus(np)) + dev->driver_override = "simple-pm-bus"; + if (bus_id) dev_set_name(&dev->dev, "%s", bus_id); else -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-02 23:04 ` [PATCH v2 1/2] of: platform: Make sure bus only devices get probed Saravana Kannan @ 2021-09-03 9:18 ` Ulf Hansson 2021-09-03 14:29 ` Geert Uytterhoeven 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2021-09-03 9:18 UTC (permalink / raw) To: Saravana Kannan Cc: Rob Herring, Frank Rowand, Geert Uytterhoeven, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > fw_devlink could end up creating device links for bus only devices. > However, bus only devices don't get probed and can block probe() or > sync_state() [1] call backs of other devices. To avoid this, set up > these devices to get probed by the simple-pm-bus. > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > Signed-off-by: Saravana Kannan <saravanak@google.com> > Tested-by: Saravana Kannan <saravanak@google.com> Again, this looks like a nice solution to the problem. One question though. The Kconfig SIMPLE_PM_BUS, should probably be "default y" - or something along those lines to make sure fw_devlink works as expected. Kind regards Uffe > --- > drivers/of/platform.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 74afbb7a4f5e..b010e2310131 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -97,6 +97,48 @@ static void of_device_make_bus_id(struct device *dev) > } > } > > +/** > + * of_match_only_simple_bus - Check if a device node is only a simple bus > + * @np: device node to check > + * > + * A simple bus in this context is defined as a transparent bus whose child > + * devices are automatically populated but has no hardware specific > + * functionality. > + * > + * Returns true if the device node is only a simple bus and can never match > + * with any other specific driver. Otherwise, returns false. > + */ > +static bool of_match_only_simple_bus(struct device_node *np) > +{ > + /* List of buses that don't have an explicit driver. */ > + static const char * const of_simple_bus_table[] = { > + "simple-bus", > + "simple-mfd", > + "isa", > + "arm,amba-bus", > + }; > + const char *cp; > + struct property *prop; > + int i; > + > + prop = of_find_property(np, "compatible", NULL); > + for (cp = of_prop_next_string(prop, NULL); cp; > + cp = of_prop_next_string(prop, cp)) { > + bool match = false; > + const char *bus = of_simple_bus_table[i]; > + > + for (i = 0; i < ARRAY_SIZE(of_simple_bus_table); i++) > + if (!of_compat_cmp(cp, bus, strlen(bus))) { > + match = true; > + break; > + } > + if (!match) > + return false; > + } > + > + return true; > +} > + > /** > * of_device_alloc - Allocate and initialize an of_device > * @np: device node to assign to device > @@ -143,6 +185,9 @@ struct platform_device *of_device_alloc(struct device_node *np, > dev->dev.fwnode = &np->fwnode; > dev->dev.parent = parent ? : &platform_bus; > > + if (of_match_only_simple_bus(np)) > + dev->driver_override = "simple-pm-bus"; > + > if (bus_id) > dev_set_name(&dev->dev, "%s", bus_id); > else > -- > 2.33.0.153.gba50c8fa24-goog > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-03 9:18 ` Ulf Hansson @ 2021-09-03 14:29 ` Geert Uytterhoeven 2021-09-03 17:09 ` Saravana Kannan 2021-09-07 10:35 ` Ulf Hansson 0 siblings, 2 replies; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-03 14:29 UTC (permalink / raw) To: Ulf Hansson Cc: Saravana Kannan, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML Hi Ulf, On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > fw_devlink could end up creating device links for bus only devices. > > However, bus only devices don't get probed and can block probe() or > > sync_state() [1] call backs of other devices. To avoid this, set up > > these devices to get probed by the simple-pm-bus. > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Tested-by: Saravana Kannan <saravanak@google.com> > > Again, this looks like a nice solution to the problem. > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > "default y" - or something along those lines to make sure fw_devlink > works as expected. I would love for SIMPLE_PM_BUS to go away, and all of its functionality to be usurped by the standard simple-bus handling. In the modern world, everything uses power management and Runtime PM, and the distinction between "simple-bus" and "simple-pm-bus" is purely artificial. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-03 14:29 ` Geert Uytterhoeven @ 2021-09-03 17:09 ` Saravana Kannan 2021-09-03 19:06 ` Geert Uytterhoeven 2021-09-07 10:35 ` Ulf Hansson 1 sibling, 1 reply; 13+ messages in thread From: Saravana Kannan @ 2021-09-03 17:09 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Ulf Hansson, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > fw_devlink could end up creating device links for bus only devices. > > > However, bus only devices don't get probed and can block probe() or > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > these devices to get probed by the simple-pm-bus. > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > Again, this looks like a nice solution to the problem. > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > "default y" - or something along those lines to make sure fw_devlink > > works as expected. > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > to be usurped by the standard simple-bus handling. What if SIMPLE_PM_BUS *is* the standard simple-bus handling? Insert "I'm the captain now" meme. I think SIMPLE_PM_BUS config should just go away and we should compile the driver in if CONFIG_OF is selected. -Saravana ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-03 17:09 ` Saravana Kannan @ 2021-09-03 19:06 ` Geert Uytterhoeven 2021-09-03 19:08 ` Saravana Kannan 0 siblings, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-03 19:06 UTC (permalink / raw) To: Saravana Kannan Cc: Ulf Hansson, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML Hi Saravana, On Fri, Sep 3, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote: > On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > > fw_devlink could end up creating device links for bus only devices. > > > > However, bus only devices don't get probed and can block probe() or > > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > > these devices to get probed by the simple-pm-bus. > > > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > > > Again, this looks like a nice solution to the problem. > > > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > > "default y" - or something along those lines to make sure fw_devlink > > > works as expected. > > > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > > to be usurped by the standard simple-bus handling. > > What if SIMPLE_PM_BUS *is* the standard simple-bus handling? > Insert "I'm the captain now" meme. No objections from my side. In fact that's how I wanted it to be from the beginning, but the DT people wanted a separate compatible value (and a separate driver). The only difference is the presence of pm_runtime_*() calls. > I think SIMPLE_PM_BUS config should just go away and we should compile > the driver in if CONFIG_OF is selected. I think there would be no need for a separate driver, if the standard simple-bus handling would take care of it. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-03 19:06 ` Geert Uytterhoeven @ 2021-09-03 19:08 ` Saravana Kannan 0 siblings, 0 replies; 13+ messages in thread From: Saravana Kannan @ 2021-09-03 19:08 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Ulf Hansson, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, Sep 3, 2021 at 12:06 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Saravana, > > On Fri, Sep 3, 2021 at 7:09 PM Saravana Kannan <saravanak@google.com> wrote: > > On Fri, Sep 3, 2021 at 7:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > > > fw_devlink could end up creating device links for bus only devices. > > > > > However, bus only devices don't get probed and can block probe() or > > > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > > > these devices to get probed by the simple-pm-bus. > > > > > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > > > > > Again, this looks like a nice solution to the problem. > > > > > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > > > "default y" - or something along those lines to make sure fw_devlink > > > > works as expected. > > > > > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > > > to be usurped by the standard simple-bus handling. > > > > What if SIMPLE_PM_BUS *is* the standard simple-bus handling? > > Insert "I'm the captain now" meme. > > No objections from my side. In fact that's how I wanted it to be > from the beginning, but the DT people wanted a separate compatible > value (and a separate driver). The only difference is the presence > of pm_runtime_*() calls. > > > I think SIMPLE_PM_BUS config should just go away and we should compile > > the driver in if CONFIG_OF is selected. > > I think there would be no need for a separate driver, if the standard > simple-bus handling would take care of it. I'm not sure about the history there. For now, I'm just extending this driver to take care of simple-bus too. -Saravana ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-03 14:29 ` Geert Uytterhoeven 2021-09-03 17:09 ` Saravana Kannan @ 2021-09-07 10:35 ` Ulf Hansson 2021-09-07 10:52 ` Geert Uytterhoeven 1 sibling, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2021-09-07 10:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Saravana Kannan, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > fw_devlink could end up creating device links for bus only devices. > > > However, bus only devices don't get probed and can block probe() or > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > these devices to get probed by the simple-pm-bus. > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > Again, this looks like a nice solution to the problem. > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > "default y" - or something along those lines to make sure fw_devlink > > works as expected. > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > to be usurped by the standard simple-bus handling. > > In the modern world, everything uses power management and Runtime > PM, and the distinction between "simple-bus" and "simple-pm-bus" > is purely artificial. I think it's not that easy, but maybe I am wrong. Today we have an opt-in way of supporting runtime PM (and power management). In most cases it's up to drivers or subsystem level code to decide if runtime PM should be enabled for the device. Would it really be okay to enable runtime PM for all of them? Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-07 10:35 ` Ulf Hansson @ 2021-09-07 10:52 ` Geert Uytterhoeven 2021-09-07 11:02 ` Ulf Hansson 0 siblings, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-07 10:52 UTC (permalink / raw) To: Ulf Hansson Cc: Saravana Kannan, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML Hi Ulf, On Tue, Sep 7, 2021 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > > fw_devlink could end up creating device links for bus only devices. > > > > However, bus only devices don't get probed and can block probe() or > > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > > these devices to get probed by the simple-pm-bus. > > > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > > > Again, this looks like a nice solution to the problem. > > > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > > "default y" - or something along those lines to make sure fw_devlink > > > works as expected. > > > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > > to be usurped by the standard simple-bus handling. > > > > In the modern world, everything uses power management and Runtime > > PM, and the distinction between "simple-bus" and "simple-pm-bus" > > is purely artificial. > > I think it's not that easy, but maybe I am wrong. > > Today we have an opt-in way of supporting runtime PM (and power > management). In most cases it's up to drivers or subsystem level code > to decide if runtime PM should be enabled for the device. > > Would it really be okay to enable runtime PM for all of them? You're talking about the software policy side. From my PoV, the issue is that this decision is leaked into DT, through the different compatible values ("simple-pm-bus" vs. "simple-bus"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] of: platform: Make sure bus only devices get probed 2021-09-07 10:52 ` Geert Uytterhoeven @ 2021-09-07 11:02 ` Ulf Hansson 0 siblings, 0 replies; 13+ messages in thread From: Ulf Hansson @ 2021-09-07 11:02 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Saravana Kannan, Rob Herring, Frank Rowand, Android Kernel Team, Linux Kernel Mailing List, DTML On Tue, 7 Sept 2021 at 12:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ulf, > > On Tue, Sep 7, 2021 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 3 Sept 2021 at 16:29, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Fri, Sep 3, 2021 at 11:19 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > > > fw_devlink could end up creating device links for bus only devices. > > > > > However, bus only devices don't get probed and can block probe() or > > > > > sync_state() [1] call backs of other devices. To avoid this, set up > > > > > these devices to get probed by the simple-pm-bus. > > > > > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > > Tested-by: Saravana Kannan <saravanak@google.com> > > > > > > > > Again, this looks like a nice solution to the problem. > > > > > > > > One question though. The Kconfig SIMPLE_PM_BUS, should probably be > > > > "default y" - or something along those lines to make sure fw_devlink > > > > works as expected. > > > > > > I would love for SIMPLE_PM_BUS to go away, and all of its functionality > > > to be usurped by the standard simple-bus handling. > > > > > > In the modern world, everything uses power management and Runtime > > > PM, and the distinction between "simple-bus" and "simple-pm-bus" > > > is purely artificial. > > > > I think it's not that easy, but maybe I am wrong. > > > > Today we have an opt-in way of supporting runtime PM (and power > > management). In most cases it's up to drivers or subsystem level code > > to decide if runtime PM should be enabled for the device. > > > > Would it really be okay to enable runtime PM for all of them? > > You're talking about the software policy side. > > From my PoV, the issue is that this decision is leaked into DT, through > the different compatible values ("simple-pm-bus" vs. "simple-bus"). Yes, I do agree with you there. On the other hand, it's probably not the only place where it's used as "software configuration", so I don't have a big issue with it. Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-02 23:04 [PATCH v2 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan 2021-09-02 23:04 ` [PATCH v2 1/2] of: platform: Make sure bus only devices get probed Saravana Kannan @ 2021-09-02 23:04 ` Saravana Kannan 2021-09-03 9:14 ` Ulf Hansson 1 sibling, 1 reply; 13+ messages in thread From: Saravana Kannan @ 2021-09-02 23:04 UTC (permalink / raw) To: Rob Herring, Frank Rowand Cc: Saravana Kannan, Ulf Hansson, Geert Uytterhoeven, kernel-team, linux-kernel, devicetree The OF platform code sets up simple bus only devices (i.e. devices that won't match with any other driver) to get probed by the simple-pm-bus to keep fw_devlink from blocking probe() or sync_state() [1] callbacks of other devices. There's no need to populate the child devices since the OF platform code would do that anyway, so return early for these simple bus only devices. [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ Signed-off-by: Saravana Kannan <saravanak@google.com> Tested-by: Saravana Kannan <saravanak@google.com> --- drivers/bus/simple-pm-bus.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c index 01a3d0cd08ed..91d52021b7f9 100644 --- a/drivers/bus/simple-pm-bus.c +++ b/drivers/bus/simple-pm-bus.c @@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev) const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev); struct device_node *np = pdev->dev.of_node; + /* + * These are transparent bus devices (not simple-pm-bus matches) that + * get populated automatically. So, don't need to do anything more. + */ + if (pdev->driver_override) + return 0; + dev_dbg(&pdev->dev, "%s\n", __func__); pm_runtime_enable(&pdev->dev); -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-02 23:04 ` [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan @ 2021-09-03 9:14 ` Ulf Hansson 2021-09-03 17:10 ` Saravana Kannan 0 siblings, 1 reply; 13+ messages in thread From: Ulf Hansson @ 2021-09-03 9:14 UTC (permalink / raw) To: Saravana Kannan Cc: Rob Herring, Frank Rowand, Geert Uytterhoeven, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > The OF platform code sets up simple bus only devices (i.e. devices that > won't match with any other driver) to get probed by the simple-pm-bus to > keep fw_devlink from blocking probe() or sync_state() [1] callbacks of > other devices. There's no need to populate the child devices since the > OF platform code would do that anyway, so return early for these simple > bus only devices. This looks like a neat solution to our problem. Although, a few comments below. > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > Signed-off-by: Saravana Kannan <saravanak@google.com> > Tested-by: Saravana Kannan <saravanak@google.com> > --- > drivers/bus/simple-pm-bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > index 01a3d0cd08ed..91d52021b7f9 100644 > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev); > struct device_node *np = pdev->dev.of_node; > > + /* > + * These are transparent bus devices (not simple-pm-bus matches) that > + * get populated automatically. So, don't need to do anything more. > + */ > + if (pdev->driver_override) > + return 0; You need the same kind of check in simple_pm_bus_remove(), to avoid pm_runtime_disable(). At least for consistency. > + > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_enable(&pdev->dev); > -- > 2.33.0.153.gba50c8fa24-goog > It also looks like we should flip the order of the patches in the series, to keep things bisectable. Kind regards Uffe ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-03 9:14 ` Ulf Hansson @ 2021-09-03 17:10 ` Saravana Kannan 0 siblings, 0 replies; 13+ messages in thread From: Saravana Kannan @ 2021-09-03 17:10 UTC (permalink / raw) To: Ulf Hansson Cc: Rob Herring, Frank Rowand, Geert Uytterhoeven, Android Kernel Team, Linux Kernel Mailing List, DTML On Fri, Sep 3, 2021 at 2:15 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 3 Sept 2021 at 01:04, Saravana Kannan <saravanak@google.com> wrote: > > > > The OF platform code sets up simple bus only devices (i.e. devices that > > won't match with any other driver) to get probed by the simple-pm-bus to > > keep fw_devlink from blocking probe() or sync_state() [1] callbacks of > > other devices. There's no need to populate the child devices since the > > OF platform code would do that anyway, so return early for these simple > > bus only devices. > > This looks like a neat solution to our problem. Although, a few comments below. > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Tested-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/bus/simple-pm-bus.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > > index 01a3d0cd08ed..91d52021b7f9 100644 > > --- a/drivers/bus/simple-pm-bus.c > > +++ b/drivers/bus/simple-pm-bus.c > > @@ -19,6 +19,13 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > const struct of_dev_auxdata *lookup = dev_get_platdata(&pdev->dev); > > struct device_node *np = pdev->dev.of_node; > > > > + /* > > + * These are transparent bus devices (not simple-pm-bus matches) that > > + * get populated automatically. So, don't need to do anything more. > > + */ > > + if (pdev->driver_override) > > + return 0; > > You need the same kind of check in simple_pm_bus_remove(), to avoid > pm_runtime_disable(). At least for consistency. Ack. > > > + > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > pm_runtime_enable(&pdev->dev); > > -- > > 2.33.0.153.gba50c8fa24-goog > > > > It also looks like we should flip the order of the patches in the > series, to keep things bisectable. Sorry I didn't get this. I'm not causing any compilation issues. Why does this matter for bisecting? -Saravana ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-07 11:02 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-02 23:04 [PATCH v2 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan 2021-09-02 23:04 ` [PATCH v2 1/2] of: platform: Make sure bus only devices get probed Saravana Kannan 2021-09-03 9:18 ` Ulf Hansson 2021-09-03 14:29 ` Geert Uytterhoeven 2021-09-03 17:09 ` Saravana Kannan 2021-09-03 19:06 ` Geert Uytterhoeven 2021-09-03 19:08 ` Saravana Kannan 2021-09-07 10:35 ` Ulf Hansson 2021-09-07 10:52 ` Geert Uytterhoeven 2021-09-07 11:02 ` Ulf Hansson 2021-09-02 23:04 ` [PATCH v2 2/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 2021-09-03 9:14 ` Ulf Hansson 2021-09-03 17:10 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).