| Message ID | 20210902230442.1515531-1-saravanak@google.com |
|---|---|
| Headers | show |
| Series | Fix simple-bus issues with fw_devlink | expand |
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 >
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
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
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
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
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
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
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
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(+)