* [PATCH v3 0/2] Fix simple-bus issues with fw_devlink @ 2021-09-04 0:05 Saravana Kannan 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 2021-09-04 0:05 ` [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS Saravana Kannan 0 siblings, 2 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-04 0:05 UTC (permalink / raw) To: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal Cc: Saravana Kannan, Ulf Hansson, Rob Herring, kernel-team, linux-arm-kernel, linux-kernel, linux-oxnas, linux-renesas-soc, linux-omap, linux-riscv 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: - A simple-bus only device is probed. - Another device listing simple-bus as a 2nd compatible string isn't probed. Ulf, mind testing this? v1->v2: - Switched to probing the simple-bus device instead of marking it as NEVER_PROBES. v2->v3: - Moved all the code into the simple-pm-bus driver - Addressed Ulf's comment about the remove() code missing a check. 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> Cc: Rob Herring <robh+dt@kernel.org> Saravana Kannan (2): drivers: bus: simple-pm-bus: Add support for probing simple bus only devices drivers: bus: Delete CONFIG_SIMPLE_PM_BUS arch/arm/configs/multi_v7_defconfig | 1 - arch/arm/configs/oxnas_v6_defconfig | 1 - arch/arm/configs/shmobile_defconfig | 1 - arch/arm/mach-omap2/Kconfig | 1 - arch/arm64/configs/defconfig | 1 - drivers/bus/Kconfig | 12 ----------- drivers/bus/Makefile | 2 +- drivers/bus/simple-pm-bus.c | 32 ++++++++++++++++++++++++++--- drivers/soc/canaan/Kconfig | 1 - 9 files changed, 30 insertions(+), 22 deletions(-) -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-04 0:05 [PATCH v3 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan @ 2021-09-04 0:05 ` Saravana Kannan 2021-09-06 7:53 ` Geert Uytterhoeven ` (2 more replies) 2021-09-04 0:05 ` [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS Saravana Kannan 1 sibling, 3 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-04 0:05 UTC (permalink / raw) To: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal Cc: Saravana Kannan, Ulf Hansson, Rob Herring, kernel-team, linux-arm-kernel, linux-kernel, linux-oxnas, linux-renesas-soc, linux-omap, linux-riscv 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, probe these devices using the simple-pm-bus driver. However, there are instances of devices that are not simple buses (they get probed by their specific drivers) that also list the "simple-bus" (or other bus only compatible strings) in their compatible property to automatically populate their child devices. We still want these devices to get probed by their specific drivers. So, we make sure this driver only probes devices that are only buses. [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 | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c index 01a3d0cd08ed..3e086a9f34cb 100644 --- a/drivers/bus/simple-pm-bus.c +++ b/drivers/bus/simple-pm-bus.c @@ -13,11 +13,26 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> - 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; + const struct device *dev = &pdev->dev; + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); + struct device_node *np = dev->of_node; + const struct of_device_id *match; + + match = of_match_device(dev->driver->of_match_table, dev); + + /* + * These are transparent bus devices (not simple-pm-bus matches) that + * have their child nodes populated automatically. So, don't need to + * do anything more. + */ + if (match && match->data) { + if (of_property_match_string(np, "compatible", match->compatible) == 0) + return 0; + else + return -ENODEV; + } dev_dbg(&pdev->dev, "%s\n", __func__); @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) static int simple_pm_bus_remove(struct platform_device *pdev) { + const void *data = of_device_get_match_data(&pdev->dev); + + if (data) + return 0; + dev_dbg(&pdev->dev, "%s\n", __func__); pm_runtime_disable(&pdev->dev); return 0; } +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ + static const struct of_device_id simple_pm_bus_of_match[] = { { .compatible = "simple-pm-bus", }, + { .compatible = "simple-bus", .data = ONLY_BUS }, + { .compatible = "simple-mfd", .data = ONLY_BUS }, + { .compatible = "isa", .data = ONLY_BUS }, + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan @ 2021-09-06 7:53 ` Geert Uytterhoeven 2021-09-07 7:01 ` Saravana Kannan 2021-09-07 10:41 ` Ulf Hansson 2021-09-09 11:01 ` Ulf Hansson 2 siblings, 1 reply; 18+ messages in thread From: Geert Uytterhoeven @ 2021-09-06 7:53 UTC (permalink / raw) To: Saravana Kannan Cc: Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones Hi Saravana, Thanks for your patch! CC linux-pm, Lee (mfd) On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > devices using the simple-pm-bus driver. > > However, there are instances of devices that are not simple buses (they > get probed by their specific drivers) that also list the "simple-bus" > (or other bus only compatible strings) in their compatible property to > automatically populate their child devices. We still want these devices > to get probed by their specific drivers. So, we make sure this driver > only probes devices that are only buses. Note that this can also be the case for buses declaring compatibility with "simple-pm-bus". However, at the moment, none of such device nodes in upstream DTS files have device-specific drivers. > [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> > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -13,11 +13,26 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > - > 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; > + const struct device *dev = &pdev->dev; > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > + struct device_node *np = dev->of_node; > + const struct of_device_id *match; > + > + match = of_match_device(dev->driver->of_match_table, dev); > + > + /* > + * These are transparent bus devices (not simple-pm-bus matches) that > + * have their child nodes populated automatically. So, don't need to > + * do anything more. > + */ > + if (match && match->data) { > + if (of_property_match_string(np, "compatible", match->compatible) == 0) Does this work as expected? Having multiple compatible values in a device node does not guarantee there exist a separate driver for any of the device-specific compatible values. > + return 0; > + else > + return -ENODEV; So if we get here, as both branches use "return", we skip the pm_runtime_enable() and of_platform_populate() below: - of_platform_populate() is handled for these buses by of_platform_default_populate(), so that's OK, - I'm wondering if any of the simple-mfd sub-devices use Runtime PM, but currently fail to save power because pm_runtime_enable() is never called for the MFD container, just like with simple-bus... > + } > > dev_dbg(&pdev->dev, "%s\n", __func__); > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > static int simple_pm_bus_remove(struct platform_device *pdev) > { > + const void *data = of_device_get_match_data(&pdev->dev); > + > + if (data) > + return 0; > + > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_disable(&pdev->dev); > return 0; > } > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > + > static const struct of_device_id simple_pm_bus_of_match[] = { > { .compatible = "simple-pm-bus", }, > + { .compatible = "simple-bus", .data = ONLY_BUS }, > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > + { .compatible = "isa", .data = ONLY_BUS }, #ifdef CONFIG_ARM_AMBA ? > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > { /* sentinel */ } This is now (almost[*]) the same as of_default_bus_match_table[] in drivers/of/platform.c. Perhaps it can be shared? [*] Especially if "simple-pm-bus" and "simple-bus" would be treated the same. > }; > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); 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] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-06 7:53 ` Geert Uytterhoeven @ 2021-09-07 7:01 ` Saravana Kannan 2021-09-09 0:15 ` Rob Herring 0 siblings, 1 reply; 18+ messages in thread From: Saravana Kannan @ 2021-09-07 7:01 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Saravana, > > Thanks for your patch! > > CC linux-pm, Lee (mfd) > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > devices using the simple-pm-bus driver. > > > > However, there are instances of devices that are not simple buses (they > > get probed by their specific drivers) that also list the "simple-bus" > > (or other bus only compatible strings) in their compatible property to > > automatically populate their child devices. We still want these devices > > to get probed by their specific drivers. So, we make sure this driver > > only probes devices that are only buses. > > Note that this can also be the case for buses declaring compatibility > with "simple-pm-bus". However, at the moment, none of such device > nodes in upstream DTS files have device-specific drivers. Not sure about mfd, but I want to make sure we don't confuse busses (which are typically added to a class) with these "simple bus" devices that are added to platform_bus. Also if these other buses are actually causing an issue, then then should implement their own stub driver or use try patch[2] if they are added to classes (devices on classes don't probe) [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > [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> > > > --- a/drivers/bus/simple-pm-bus.c > > +++ b/drivers/bus/simple-pm-bus.c > > @@ -13,11 +13,26 @@ > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > > > - > > 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; > > + const struct device *dev = &pdev->dev; > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > + struct device_node *np = dev->of_node; > > + const struct of_device_id *match; > > + > > + match = of_match_device(dev->driver->of_match_table, dev); > > + > > + /* > > + * These are transparent bus devices (not simple-pm-bus matches) that > > + * have their child nodes populated automatically. So, don't need to > > + * do anything more. > > + */ > > + if (match && match->data) { > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > Does this work as expected? Having multiple compatible values in a > device node does not guarantee there exist a separate driver for any > of the device-specific compatible values. Right, and if they are platform devices that are equivalent to simple-bus (meaning, they don't do anything in Linux and just have their devices populated) we can add those to this list too. > > > + return 0; > > + else > > + return -ENODEV; > > So if we get here, as both branches use "return", we skip the > pm_runtime_enable() and of_platform_populate() below: > - of_platform_populate() is handled for these buses by > of_platform_default_populate(), so that's OK, > - I'm wondering if any of the simple-mfd sub-devices use Runtime > PM, but currently fail to save power because pm_runtime_enable() > is never called for the MFD container, just like with simple-bus... But this doesn't affect simple-mfd though. > > > + } > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > > > static int simple_pm_bus_remove(struct platform_device *pdev) > > { > > + const void *data = of_device_get_match_data(&pdev->dev); > > + > > + if (data) > > + return 0; > > + > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > pm_runtime_disable(&pdev->dev); > > return 0; > > } > > > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > > + > > static const struct of_device_id simple_pm_bus_of_match[] = { > > { .compatible = "simple-pm-bus", }, > > + { .compatible = "simple-bus", .data = ONLY_BUS }, > > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > > + { .compatible = "isa", .data = ONLY_BUS }, > > #ifdef CONFIG_ARM_AMBA ? Not needed? If CONFIG_ARM_AMBA isn't enabled, the device wouldn't be created in the first place. > > > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > > { /* sentinel */ } > > This is now (almost[*]) the same as of_default_bus_match_table[] > in drivers/of/platform.c. Perhaps it can be shared? I wanted this table to be expandable as needed. That's why I'm intentionally not using of_default_bus_match_table[]. > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > the same. They are not treated the same way. -Saravana > > > }; > > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); > > 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] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-07 7:01 ` Saravana Kannan @ 2021-09-09 0:15 ` Rob Herring 2021-09-09 0:57 ` Saravana Kannan 0 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2021-09-09 0:15 UTC (permalink / raw) To: Saravana Kannan Cc: Geert Uytterhoeven, Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@google.com> wrote: > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Saravana, > > > > Thanks for your patch! > > > > CC linux-pm, Lee (mfd) > > > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > > devices using the simple-pm-bus driver. > > > > > > However, there are instances of devices that are not simple buses (they > > > get probed by their specific drivers) that also list the "simple-bus" > > > (or other bus only compatible strings) in their compatible property to > > > automatically populate their child devices. We still want these devices > > > to get probed by their specific drivers. So, we make sure this driver > > > only probes devices that are only buses. > > > > Note that this can also be the case for buses declaring compatibility > > with "simple-pm-bus". However, at the moment, none of such device > > nodes in upstream DTS files have device-specific drivers. > > Not sure about mfd, but I want to make sure we don't confuse busses > (which are typically added to a class) with these "simple bus" devices > that are added to platform_bus. Also if these other buses are actually > causing an issue, then then should implement their own stub driver or > use try patch[2] if they are added to classes (devices on classes > don't probe) > > [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > > > > [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> > > > > > --- a/drivers/bus/simple-pm-bus.c > > > +++ b/drivers/bus/simple-pm-bus.c > > > @@ -13,11 +13,26 @@ > > > #include <linux/platform_device.h> > > > #include <linux/pm_runtime.h> > > > > > > - > > > 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; > > > + const struct device *dev = &pdev->dev; > > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > > + struct device_node *np = dev->of_node; > > > + const struct of_device_id *match; > > > + > > > + match = of_match_device(dev->driver->of_match_table, dev); > > > + > > > + /* > > > + * These are transparent bus devices (not simple-pm-bus matches) that > > > + * have their child nodes populated automatically. So, don't need to > > > + * do anything more. > > > + */ > > > + if (match && match->data) { > > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > Does this work as expected? Having multiple compatible values in a > > device node does not guarantee there exist a separate driver for any > > of the device-specific compatible values. > > Right, and if they are platform devices that are equivalent to > simple-bus (meaning, they don't do anything in Linux and just have > their devices populated) we can add those to this list too. I think this needs to be a list of compatibles we have drivers for instead. A more specific compatible that the OS doesn't understand shouldn't cause a change in behavior and adding one would. I expect it to be a short list. We are guaranteed that of_match_device() returns the best match in the match list, so we really just need 1 list here with a boolean to bail or not. > > > + return 0; > > > + else > > > + return -ENODEV; > > > > So if we get here, as both branches use "return", we skip the > > pm_runtime_enable() and of_platform_populate() below: > > - of_platform_populate() is handled for these buses by > > of_platform_default_populate(), so that's OK, > > - I'm wondering if any of the simple-mfd sub-devices use Runtime > > PM, but currently fail to save power because pm_runtime_enable() > > is never called for the MFD container, just like with simple-bus... > > But this doesn't affect simple-mfd though. > > > > > > + } > > > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > > > > > static int simple_pm_bus_remove(struct platform_device *pdev) > > > { > > > + const void *data = of_device_get_match_data(&pdev->dev); > > > + > > > + if (data) > > > + return 0; > > > + > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > pm_runtime_disable(&pdev->dev); > > > return 0; > > > } > > > > > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > > > + > > > static const struct of_device_id simple_pm_bus_of_match[] = { > > > { .compatible = "simple-pm-bus", }, > > > + { .compatible = "simple-bus", .data = ONLY_BUS }, > > > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > > > + { .compatible = "isa", .data = ONLY_BUS }, > > > > #ifdef CONFIG_ARM_AMBA ? > > Not needed? If CONFIG_ARM_AMBA isn't enabled, the device wouldn't be > created in the first place. > > > > > > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > > > { /* sentinel */ } > > > > This is now (almost[*]) the same as of_default_bus_match_table[] > > in drivers/of/platform.c. Perhaps it can be shared? > > I wanted this table to be expandable as needed. That's why I'm > intentionally not using of_default_bus_match_table[]. > > > > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > > the same. > > They are not treated the same way. I think it would be better if they were. IOW, the core code stops descending into simple-bus, etc. nodes and they are populated here. Then we just get rid of of_default_bus_match_table. That could cause some issues with init ordering. As I recall the at91 gpio and pinctrl drivers are sensitive to this. The default call to of_platform_populate doesn't work on those systems because the devices get created later than when their machine specific call happens. It may have been a case of a parent probe assuming a child probe completed after of_platform_populate returns (also a problem for Qcom with DWC3). There's a fix for at91 somewhere in the git history after I broke it. I started trying to untangle things with at91, but never finished that. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-09 0:15 ` Rob Herring @ 2021-09-09 0:57 ` Saravana Kannan 2021-09-09 14:01 ` Rob Herring 0 siblings, 1 reply; 18+ messages in thread From: Saravana Kannan @ 2021-09-09 0:57 UTC (permalink / raw) To: Rob Herring Cc: Geert Uytterhoeven, Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Wed, Sep 8, 2021 at 5:16 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@google.com> wrote: > > > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > Hi Saravana, > > > > > > Thanks for your patch! > > > > > > CC linux-pm, Lee (mfd) > > > > > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > > > devices using the simple-pm-bus driver. > > > > > > > > However, there are instances of devices that are not simple buses (they > > > > get probed by their specific drivers) that also list the "simple-bus" > > > > (or other bus only compatible strings) in their compatible property to > > > > automatically populate their child devices. We still want these devices > > > > to get probed by their specific drivers. So, we make sure this driver > > > > only probes devices that are only buses. > > > > > > Note that this can also be the case for buses declaring compatibility > > > with "simple-pm-bus". However, at the moment, none of such device > > > nodes in upstream DTS files have device-specific drivers. > > > > Not sure about mfd, but I want to make sure we don't confuse busses > > (which are typically added to a class) with these "simple bus" devices > > that are added to platform_bus. Also if these other buses are actually > > causing an issue, then then should implement their own stub driver or > > use try patch[2] if they are added to classes (devices on classes > > don't probe) > > > > [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > > > > > > > [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> > > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > > +++ b/drivers/bus/simple-pm-bus.c > > > > @@ -13,11 +13,26 @@ > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > > > > > - > > > > 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; > > > > + const struct device *dev = &pdev->dev; > > > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > > > + struct device_node *np = dev->of_node; > > > > + const struct of_device_id *match; > > > > + > > > > + match = of_match_device(dev->driver->of_match_table, dev); > > > > + > > > > + /* > > > > + * These are transparent bus devices (not simple-pm-bus matches) that > > > > + * have their child nodes populated automatically. So, don't need to > > > > + * do anything more. > > > > + */ > > > > + if (match && match->data) { > > > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > > > Does this work as expected? Having multiple compatible values in a > > > device node does not guarantee there exist a separate driver for any > > > of the device-specific compatible values. > > > > Right, and if they are platform devices that are equivalent to > > simple-bus (meaning, they don't do anything in Linux and just have > > their devices populated) we can add those to this list too. > > I think this needs to be a list of compatibles we have drivers for > instead. I don't think a "denylist" (devices we shouldn't probe with this driver) would be a short list. As of today, literally any device that has children could add a "simple-bus" to the compatible property and get its child devices populated for free. If we use a denylist, we'll have to update it every time someone adds "simple-bus" as a general match to a DT node (new or otherwise) that isn't in the denylist. The list would blow up and be a maintenance headache. Also, a denylist won't capture any DT that isn't part of the kernel repo but depends on "simple-bus" to populate the device's child nodes. Keep in mind this could be true even for completely upstream drivers today. And on top of that, this will also break for downstream drivers and platforms in the development stage. The allowlist is much smaller and manageable. > A more specific compatible that the OS doesn't understand > shouldn't cause a change in behavior and adding one would. I think the amount of specific compatible strings that'll be added, but won't have drivers added to Linux AND would want to boot with Linux is much less likely than the amount of times we'd have to update a denylist. Also, if we do hit the cases you mention and we want those devices to get probed anyway, with my current allowlist approach, we could use "driver_override" to force this driver to match them. If you use a denylist like you said, there's no way you can get the simple-pm-bus to unbind and let the more specific driver to bind. > I expect it to be a short list. > > We are guaranteed that of_match_device() returns the best match in the > match list, so we really just need 1 list here with a boolean to bail > or not. > > > > > + return 0; > > > > + else > > > > + return -ENODEV; > > > > > > So if we get here, as both branches use "return", we skip the > > > pm_runtime_enable() and of_platform_populate() below: > > > - of_platform_populate() is handled for these buses by > > > of_platform_default_populate(), so that's OK, > > > - I'm wondering if any of the simple-mfd sub-devices use Runtime > > > PM, but currently fail to save power because pm_runtime_enable() > > > is never called for the MFD container, just like with simple-bus... > > > > But this doesn't affect simple-mfd though. > > > > > > > > > + } > > > > > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > > > > > > > static int simple_pm_bus_remove(struct platform_device *pdev) > > > > { > > > > + const void *data = of_device_get_match_data(&pdev->dev); > > > > + > > > > + if (data) > > > > + return 0; > > > > + > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > > > pm_runtime_disable(&pdev->dev); > > > > return 0; > > > > } > > > > > > > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > > > > + > > > > static const struct of_device_id simple_pm_bus_of_match[] = { > > > > { .compatible = "simple-pm-bus", }, > > > > + { .compatible = "simple-bus", .data = ONLY_BUS }, > > > > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > > > > + { .compatible = "isa", .data = ONLY_BUS }, > > > > > > #ifdef CONFIG_ARM_AMBA ? > > > > Not needed? If CONFIG_ARM_AMBA isn't enabled, the device wouldn't be > > created in the first place. > > > > > > > > > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > > > > { /* sentinel */ } > > > > > > This is now (almost[*]) the same as of_default_bus_match_table[] > > > in drivers/of/platform.c. Perhaps it can be shared? > > > > I wanted this table to be expandable as needed. That's why I'm > > intentionally not using of_default_bus_match_table[]. > > > > > > > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > > > the same. > > > > They are not treated the same way. > > I think it would be better if they were. IOW, the core code stops > descending into simple-bus, etc. nodes and they are populated here. > Then we just get rid of of_default_bus_match_table. Right, I would if we could. But we can't simply stop descending the simple-bus nodes in the core code because all the specific drivers that used to have their child devices populated automatically would stop working and would need to be updated to populate their child devices. And I'm sure there are a ton more downstream kernels and downstream DTs (that use upstream kernels) that we would break. If you really want to do that go for it, but I'd rather not do all this as part of trying to fix the issue Ulf reported that needs simple-bus only devices probed. > That could cause some issues with init ordering. As I recall the at91 > gpio and pinctrl drivers are sensitive to this. The default call to > of_platform_populate doesn't work on those systems because the devices > get created later than when their machine specific call happens. It > may have been a case of a parent probe assuming a child probe > completed after of_platform_populate returns (also a problem for Qcom > with DWC3). There's a fix for at91 somewhere in the git history after > I broke it. I started trying to untangle things with at91, but never > finished that. I think it'll cause a lot of issues if we stop descending simple-bus nodes in the core code. We're just scratching the surface here. -Saravana ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-09 0:57 ` Saravana Kannan @ 2021-09-09 14:01 ` Rob Herring 2021-09-09 18:11 ` Saravana Kannan 0 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2021-09-09 14:01 UTC (permalink / raw) To: Saravana Kannan Cc: Geert Uytterhoeven, Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Wed, Sep 8, 2021 at 7:58 PM Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Sep 8, 2021 at 5:16 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > > Hi Saravana, > > > > > > > > Thanks for your patch! > > > > > > > > CC linux-pm, Lee (mfd) > > > > > > > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > > > > devices using the simple-pm-bus driver. > > > > > > > > > > However, there are instances of devices that are not simple buses (they > > > > > get probed by their specific drivers) that also list the "simple-bus" > > > > > (or other bus only compatible strings) in their compatible property to > > > > > automatically populate their child devices. We still want these devices > > > > > to get probed by their specific drivers. So, we make sure this driver > > > > > only probes devices that are only buses. > > > > > > > > Note that this can also be the case for buses declaring compatibility > > > > with "simple-pm-bus". However, at the moment, none of such device > > > > nodes in upstream DTS files have device-specific drivers. > > > > > > Not sure about mfd, but I want to make sure we don't confuse busses > > > (which are typically added to a class) with these "simple bus" devices > > > that are added to platform_bus. Also if these other buses are actually > > > causing an issue, then then should implement their own stub driver or > > > use try patch[2] if they are added to classes (devices on classes > > > don't probe) > > > > > > [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > > > > > > > > > > [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> > > > > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > > > +++ b/drivers/bus/simple-pm-bus.c > > > > > @@ -13,11 +13,26 @@ > > > > > #include <linux/platform_device.h> > > > > > #include <linux/pm_runtime.h> > > > > > > > > > > - > > > > > 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; > > > > > + const struct device *dev = &pdev->dev; > > > > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > > > > + struct device_node *np = dev->of_node; > > > > > + const struct of_device_id *match; > > > > > + > > > > > + match = of_match_device(dev->driver->of_match_table, dev); > > > > > + > > > > > + /* > > > > > + * These are transparent bus devices (not simple-pm-bus matches) that > > > > > + * have their child nodes populated automatically. So, don't need to > > > > > + * do anything more. > > > > > + */ > > > > > + if (match && match->data) { > > > > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > > > > > Does this work as expected? Having multiple compatible values in a > > > > device node does not guarantee there exist a separate driver for any > > > > of the device-specific compatible values. > > > > > > Right, and if they are platform devices that are equivalent to > > > simple-bus (meaning, they don't do anything in Linux and just have > > > their devices populated) we can add those to this list too. > > > > I think this needs to be a list of compatibles we have drivers for > > instead. > > I don't think a "denylist" (devices we shouldn't probe with this > driver) would be a short list. As of today, literally any device that > has children could add a "simple-bus" to the compatible property and > get its child devices populated for free. If we use a denylist, we'll > have to update it every time someone adds "simple-bus" as a general > match to a DT node (new or otherwise) that isn't in the denylist. The > list would blow up and be a maintenance headache. > > Also, a denylist won't capture any DT that isn't part of the kernel > repo but depends on "simple-bus" to populate the device's child nodes. > Keep in mind this could be true even for completely upstream drivers > today. And on top of that, this will also break for downstream drivers > and platforms in the development stage. You've got this all backwards. The list would be compatibles for which they have their own driver. If they have their own driver, we know that because there is a driver in the kernel. If you have an out of tree bus driver, then I guess you shouldn't be claiming compatibility with 'simple-bus'. > The allowlist is much smaller and manageable. I count 140 cases of 'simple-bus' with another compatible. I find roughly 24 of those under drivers/ that have a driver (and look, there's at91 pinctrl/gpio). There's some more under arch/, but I'm not sure if they are drivers. This is what I ran: git grep -ho '".*", "simple-bus"' -- arch/ | cut -d' ' -f1 | grep -oE '".+"' | grep -v '"syscon"' | sort -u > buses.txt git grep -f buses.txt -- drivers/ I haven't looked at 'simple-mfd', but that was supposed to always mean 'no driver'. To put it another way, let's look at 3 possibilities: 'simple-bus' 'foo,has-no-driver', 'simple-bus' 'foo,has-a-driver', 'simple-bus' The first case is easy. The last 2 cases are not. We have no way to handle them differently without a list. > > A more specific compatible that the OS doesn't understand > > shouldn't cause a change in behavior and adding one would. Again, if a dt is modified from case 1 to case 2, there should not be a change in behavior. Presumably if Ulf changed his test in this way, it would again fail, right? > I think the amount of specific compatible strings that'll be added, > but won't have drivers added to Linux AND would want to boot with > Linux is much less likely than the amount of times we'd have to update > a denylist. > > Also, if we do hit the cases you mention and we want those devices to > get probed anyway, with my current allowlist approach, we could use > "driver_override" to force this driver to match them. If you use a > denylist like you said, there's no way you can get the simple-pm-bus > to unbind and let the more specific driver to bind. Where would we set "driver_override"? Aren't we going to end up with a driver_override list? [...] > > > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > > > > the same. > > > > > > They are not treated the same way. > > > > I think it would be better if they were. IOW, the core code stops > > descending into simple-bus, etc. nodes and they are populated here. > > Then we just get rid of of_default_bus_match_table. > > Right, I would if we could. But we can't simply stop descending the > simple-bus nodes in the core code because all the specific drivers > that used to have their child devices populated automatically would > stop working and would need to be updated to populate their child > devices. And I'm sure there are a ton more downstream kernels and > downstream DTs (that use upstream kernels) that we would break. > > If you really want to do that go for it, but I'd rather not do all > this as part of trying to fix the issue Ulf reported that needs > simple-bus only devices probed. Agreed. > > That could cause some issues with init ordering. As I recall the at91 > > gpio and pinctrl drivers are sensitive to this. The default call to > > of_platform_populate doesn't work on those systems because the devices > > get created later than when their machine specific call happens. It > > may have been a case of a parent probe assuming a child probe > > completed after of_platform_populate returns (also a problem for Qcom > > with DWC3). There's a fix for at91 somewhere in the git history after > > I broke it. I started trying to untangle things with at91, but never > > finished that. > > I think it'll cause a lot of issues if we stop descending simple-bus > nodes in the core code. We're just scratching the surface here. Perhaps, but if there's cases which are that fragile, we should fix them. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-09 14:01 ` Rob Herring @ 2021-09-09 18:11 ` Saravana Kannan 2021-09-10 15:21 ` Rob Herring 0 siblings, 1 reply; 18+ messages in thread From: Saravana Kannan @ 2021-09-09 18:11 UTC (permalink / raw) To: Rob Herring Cc: Geert Uytterhoeven, Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Thu, Sep 9, 2021 at 7:02 AM Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Sep 8, 2021 at 7:58 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Wed, Sep 8, 2021 at 5:16 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > > > > Hi Saravana, > > > > > > > > > > Thanks for your patch! > > > > > > > > > > CC linux-pm, Lee (mfd) > > > > > > > > > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > > > > > devices using the simple-pm-bus driver. > > > > > > > > > > > > However, there are instances of devices that are not simple buses (they > > > > > > get probed by their specific drivers) that also list the "simple-bus" > > > > > > (or other bus only compatible strings) in their compatible property to > > > > > > automatically populate their child devices. We still want these devices > > > > > > to get probed by their specific drivers. So, we make sure this driver > > > > > > only probes devices that are only buses. > > > > > > > > > > Note that this can also be the case for buses declaring compatibility > > > > > with "simple-pm-bus". However, at the moment, none of such device > > > > > nodes in upstream DTS files have device-specific drivers. > > > > > > > > Not sure about mfd, but I want to make sure we don't confuse busses > > > > (which are typically added to a class) with these "simple bus" devices > > > > that are added to platform_bus. Also if these other buses are actually > > > > causing an issue, then then should implement their own stub driver or > > > > use try patch[2] if they are added to classes (devices on classes > > > > don't probe) > > > > > > > > [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > > > > > > > > > > > > > [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> > > > > > > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > > > > +++ b/drivers/bus/simple-pm-bus.c > > > > > > @@ -13,11 +13,26 @@ > > > > > > #include <linux/platform_device.h> > > > > > > #include <linux/pm_runtime.h> > > > > > > > > > > > > - > > > > > > 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; > > > > > > + const struct device *dev = &pdev->dev; > > > > > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > > > > > + struct device_node *np = dev->of_node; > > > > > > + const struct of_device_id *match; > > > > > > + > > > > > > + match = of_match_device(dev->driver->of_match_table, dev); > > > > > > + > > > > > > + /* > > > > > > + * These are transparent bus devices (not simple-pm-bus matches) that > > > > > > + * have their child nodes populated automatically. So, don't need to > > > > > > + * do anything more. > > > > > > + */ > > > > > > + if (match && match->data) { > > > > > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > > > > > > > Does this work as expected? Having multiple compatible values in a > > > > > device node does not guarantee there exist a separate driver for any > > > > > of the device-specific compatible values. > > > > > > > > Right, and if they are platform devices that are equivalent to > > > > simple-bus (meaning, they don't do anything in Linux and just have > > > > their devices populated) we can add those to this list too. > > > > > > I think this needs to be a list of compatibles we have drivers for > > > instead. > > > > I don't think a "denylist" (devices we shouldn't probe with this > > driver) would be a short list. As of today, literally any device that > > has children could add a "simple-bus" to the compatible property and > > get its child devices populated for free. If we use a denylist, we'll > > have to update it every time someone adds "simple-bus" as a general > > match to a DT node (new or otherwise) that isn't in the denylist. The > > list would blow up and be a maintenance headache. > > > > Also, a denylist won't capture any DT that isn't part of the kernel > > repo but depends on "simple-bus" to populate the device's child nodes. > > Keep in mind this could be true even for completely upstream drivers > > today. And on top of that, this will also break for downstream drivers > > and platforms in the development stage. > > You've got this all backwards. I don't think so. I got what you are saying. > The list would be compatibles for which > they have their own driver. If they have their own driver, we know > that because there is a driver in the kernel. If you have an out of > tree bus driver, then I guess you shouldn't be claiming compatibility > with 'simple-bus'. But it isn't just out of tree drivers though. Take for example (a random compatible string with a driver in upstream) "qcom,sdm845-gpucc" that has DT nodes in a couple of DTS files. It's perfectly valid today today to add child nodes under it and have the child nodes get populated by making the compatible property: "qcom,sdm845-gpucc", "simple-bus" The only thing not upstreamed in that case is the DT file. And I'm fairly certain our position is not "if your DT isn't in the kernel repo we won't support it". In this situation we have no way of knowing we need to add "qcom,sdm845-gpucc" to the "it has a driver, don't probe it" list. And even if they upstream it, this list is going to blow up. And we are breaking backward compatibility that can't be fixed without a kernel update (no runtime fixes possible). > > The allowlist is much smaller and manageable. > > I count 140 cases of 'simple-bus' with another compatible. I find > roughly 24 of those under drivers/ that have a driver (and look, > there's at91 pinctrl/gpio). There's some more under arch/, but I'm not > sure if they are drivers. This is what I ran: > > git grep -ho '".*", "simple-bus"' -- arch/ | cut -d' ' -f1 | grep -oE > '".+"' | grep -v '"syscon"' | sort -u > buses.txt > git grep -f buses.txt -- drivers/ > > I haven't looked at 'simple-mfd', but that was supposed to always mean > 'no driver'. > > To put it another way, let's look at 3 possibilities: > > 'simple-bus' > 'foo,has-no-driver', 'simple-bus' > 'foo,has-a-driver', 'simple-bus' > > The first case is easy. The last 2 cases are not. We have no way to > handle them differently without a list. > > > > A more specific compatible that the OS doesn't understand > > > shouldn't cause a change in behavior and adding one would. > > Again, if a dt is modified from case 1 to case 2, there should not be > a change in behavior. My point is that if DT is modified from 1 to 2, we'll need to add "foo,has-no-driver" to the list we'll maintain. That preserves backward compatibility with existing DTs and if someone modifies the DT it's more likely that they can modify the kernel too. See more below. > Presumably if Ulf changed his test in this way, > it would again fail, right? > > > > I think the amount of specific compatible strings that'll be added, > > but won't have drivers added to Linux AND would want to boot with > > Linux is much less likely than the amount of times we'd have to update > > a denylist. > > > > Also, if we do hit the cases you mention and we want those devices to > > get probed anyway, with my current allowlist approach, we could use > > "driver_override" to force this driver to match them. If you use a > > denylist like you said, there's no way you can get the simple-pm-bus > > to unbind and let the more specific driver to bind. > > Where would we set "driver_override"? Aren't we going to end up with a > driver_override list? I'm not saying we maintain a list in the kernel. I'm saying if someone changed their DT to add 'foo,has-no-driver', but hit the unlikely case where they can update the DT but not the kernel, they can still get the 'foo,has-no-driver' to probe by using the driver_override file for that device in sysfs. TLDR: they can still get it to work without having to modify the kernel. > > [...] > > > > > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > > > > > the same. > > > > > > > > They are not treated the same way. > > > > > > I think it would be better if they were. IOW, the core code stops > > > descending into simple-bus, etc. nodes and they are populated here. > > > Then we just get rid of of_default_bus_match_table. > > > > Right, I would if we could. But we can't simply stop descending the > > simple-bus nodes in the core code because all the specific drivers > > that used to have their child devices populated automatically would > > stop working and would need to be updated to populate their child > > devices. And I'm sure there are a ton more downstream kernels and > > downstream DTs (that use upstream kernels) that we would break. > > > > If you really want to do that go for it, but I'd rather not do all > > this as part of trying to fix the issue Ulf reported that needs > > simple-bus only devices probed. > > Agreed. I'm confused. So you are okay with this patch to merge now? > > > That could cause some issues with init ordering. As I recall the at91 > > > gpio and pinctrl drivers are sensitive to this. The default call to > > > of_platform_populate doesn't work on those systems because the devices > > > get created later than when their machine specific call happens. It > > > may have been a case of a parent probe assuming a child probe > > > completed after of_platform_populate returns (also a problem for Qcom > > > with DWC3). There's a fix for at91 somewhere in the git history after > > > I broke it. I started trying to untangle things with at91, but never > > > finished that. > > > > I think it'll cause a lot of issues if we stop descending simple-bus > > nodes in the core code. We're just scratching the surface here. > > Perhaps, but if there's cases which are that fragile, we should fix them. Sure, but I'm not sure I want to open that can of works, -Saravana ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-09 18:11 ` Saravana Kannan @ 2021-09-10 15:21 ` Rob Herring 0 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2021-09-10 15:21 UTC (permalink / raw) To: Saravana Kannan Cc: Geert Uytterhoeven, Russell King, Neil Armstrong, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Ulf Hansson, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, open list:TI ETHERNET SWITCH DRIVER (CPSW), linux-riscv, Lee Jones On Thu, Sep 9, 2021 at 1:11 PM Saravana Kannan <saravanak@google.com> wrote: > TLDR: Read the end first... > On Thu, Sep 9, 2021 at 7:02 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Wed, Sep 8, 2021 at 7:58 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Wed, Sep 8, 2021 at 5:16 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > > > > > > Hi Saravana, > > > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > > CC linux-pm, Lee (mfd) > > > > > > > > > > > > On Sat, Sep 4, 2021 at 2:05 AM 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, probe these > > > > > > > devices using the simple-pm-bus driver. > > > > > > > > > > > > > > However, there are instances of devices that are not simple buses (they > > > > > > > get probed by their specific drivers) that also list the "simple-bus" > > > > > > > (or other bus only compatible strings) in their compatible property to > > > > > > > automatically populate their child devices. We still want these devices > > > > > > > to get probed by their specific drivers. So, we make sure this driver > > > > > > > only probes devices that are only buses. > > > > > > > > > > > > Note that this can also be the case for buses declaring compatibility > > > > > > with "simple-pm-bus". However, at the moment, none of such device > > > > > > nodes in upstream DTS files have device-specific drivers. > > > > > > > > > > Not sure about mfd, but I want to make sure we don't confuse busses > > > > > (which are typically added to a class) with these "simple bus" devices > > > > > that are added to platform_bus. Also if these other buses are actually > > > > > causing an issue, then then should implement their own stub driver or > > > > > use try patch[2] if they are added to classes (devices on classes > > > > > don't probe) > > > > > > > > > > [2] - https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/ > > > > > > > > > > > > > > > > > > [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> > > > > > > > > > > > > > --- a/drivers/bus/simple-pm-bus.c > > > > > > > +++ b/drivers/bus/simple-pm-bus.c > > > > > > > @@ -13,11 +13,26 @@ > > > > > > > #include <linux/platform_device.h> > > > > > > > #include <linux/pm_runtime.h> > > > > > > > > > > > > > > - > > > > > > > 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; > > > > > > > + const struct device *dev = &pdev->dev; > > > > > > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > > > > > > + struct device_node *np = dev->of_node; > > > > > > > + const struct of_device_id *match; > > > > > > > + > > > > > > > + match = of_match_device(dev->driver->of_match_table, dev); > > > > > > > + > > > > > > > + /* > > > > > > > + * These are transparent bus devices (not simple-pm-bus matches) that > > > > > > > + * have their child nodes populated automatically. So, don't need to > > > > > > > + * do anything more. > > > > > > > + */ > > > > > > > + if (match && match->data) { > > > > > > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > > > > > > > > > Does this work as expected? Having multiple compatible values in a > > > > > > device node does not guarantee there exist a separate driver for any > > > > > > of the device-specific compatible values. > > > > > > > > > > Right, and if they are platform devices that are equivalent to > > > > > simple-bus (meaning, they don't do anything in Linux and just have > > > > > their devices populated) we can add those to this list too. > > > > > > > > I think this needs to be a list of compatibles we have drivers for > > > > instead. > > > > > > I don't think a "denylist" (devices we shouldn't probe with this > > > driver) would be a short list. As of today, literally any device that > > > has children could add a "simple-bus" to the compatible property and > > > get its child devices populated for free. If we use a denylist, we'll > > > have to update it every time someone adds "simple-bus" as a general > > > match to a DT node (new or otherwise) that isn't in the denylist. The > > > list would blow up and be a maintenance headache. > > > > > > Also, a denylist won't capture any DT that isn't part of the kernel > > > repo but depends on "simple-bus" to populate the device's child nodes. > > > Keep in mind this could be true even for completely upstream drivers > > > today. And on top of that, this will also break for downstream drivers > > > and platforms in the development stage. > > > > You've got this all backwards. > > I don't think so. I got what you are saying. > > > The list would be compatibles for which > > they have their own driver. If they have their own driver, we know > > that because there is a driver in the kernel. If you have an out of > > tree bus driver, then I guess you shouldn't be claiming compatibility > > with 'simple-bus'. > > But it isn't just out of tree drivers though. Take for example (a > random compatible string with a driver in upstream) > "qcom,sdm845-gpucc" that has DT nodes in a couple of DTS files. It's > perfectly valid today today to add child nodes under it and have the > child nodes get populated by making the compatible property: > "qcom,sdm845-gpucc", "simple-bus" IMO, that is not valid and I'd reject adding 'simple-bus' to the schema (so we would know). Also, if we had only "qcom,sdm845-gpucc" to start with, then we probably have a driver already. > The only thing not upstreamed in that case is the DT file. And I'm > fairly certain our position is not "if your DT isn't in the kernel > repo we won't support it". In this situation we have no way of knowing > we need to add "qcom,sdm845-gpucc" to the "it has a driver, don't > probe it" list. And even if they upstream it, this list is going to > blow up. And we are breaking backward compatibility that can't be > fixed without a kernel update (no runtime fixes possible). > > > > The allowlist is much smaller and manageable. > > > > I count 140 cases of 'simple-bus' with another compatible. I find > > roughly 24 of those under drivers/ that have a driver (and look, > > there's at91 pinctrl/gpio). There's some more under arch/, but I'm not > > sure if they are drivers. This is what I ran: > > > > git grep -ho '".*", "simple-bus"' -- arch/ | cut -d' ' -f1 | grep -oE > > '".+"' | grep -v '"syscon"' | sort -u > buses.txt > > git grep -f buses.txt -- drivers/ > > > > I haven't looked at 'simple-mfd', but that was supposed to always mean > > 'no driver'. > > > > To put it another way, let's look at 3 possibilities: > > > > 'simple-bus' > > 'foo,has-no-driver', 'simple-bus' > > 'foo,has-a-driver', 'simple-bus' > > > > The first case is easy. The last 2 cases are not. We have no way to > > handle them differently without a list. > > > > > > A more specific compatible that the OS doesn't understand > > > > shouldn't cause a change in behavior and adding one would. > > > > Again, if a dt is modified from case 1 to case 2, there should not be > > a change in behavior. > > My point is that if DT is modified from 1 to 2, we'll need to add > "foo,has-no-driver" to the list we'll maintain. That preserves > backward compatibility with existing DTs and if someone modifies the > DT it's more likely that they can modify the kernel too. See more > below. I assume this list is just cases of I need to bind the simple-bus driver for devlinks, right? Being able to update the kernel is not a valid assumption. While the kernel sources may be updated, it is perfectly valid to update a dtb and not the kernel. This is what SUSE does (or was doing?). DTBs from the latest kernel (because that's the most complete h/w description, right?) and an LTS kernel. Think about updating your PC's OS and firmware. Do you want updating either of those independently to work? Probably so. It's the same thing with kernel and dtb(in firmware) updates. While specific platforms or environments (Android) may not care, in general we have to. However, we can cheat a little bit if the kernel update goes to stable kernels, but that should be the exception and I don't think we should rely on that. Would you want your PC to stop booting due to a firmware update without a stable kernel update? > > Presumably if Ulf changed his test in this way, > > it would again fail, right? > > > > > > > I think the amount of specific compatible strings that'll be added, > > > but won't have drivers added to Linux AND would want to boot with > > > Linux is much less likely than the amount of times we'd have to update > > > a denylist. > > > > > > Also, if we do hit the cases you mention and we want those devices to > > > get probed anyway, with my current allowlist approach, we could use > > > "driver_override" to force this driver to match them. If you use a > > > denylist like you said, there's no way you can get the simple-pm-bus > > > to unbind and let the more specific driver to bind. > > > > Where would we set "driver_override"? Aren't we going to end up with a > > driver_override list? > > I'm not saying we maintain a list in the kernel. I'm saying if someone > changed their DT to add 'foo,has-no-driver', but hit the unlikely case > where they can update the DT but not the kernel, they can still get > the 'foo,has-no-driver' to probe by using the driver_override file for > that device in sysfs. TLDR: they can still get it to work without > having to modify the kernel. That doesn't seem like a great experience. How's that going to work if you didn't boot up enough to get to sysfs? > > [...] > > > > > > > > [*] Especially if "simple-pm-bus" and "simple-bus" would be treated > > > > > > the same. > > > > > > > > > > They are not treated the same way. > > > > > > > > I think it would be better if they were. IOW, the core code stops > > > > descending into simple-bus, etc. nodes and they are populated here. > > > > Then we just get rid of of_default_bus_match_table. > > > > > > Right, I would if we could. But we can't simply stop descending the > > > simple-bus nodes in the core code because all the specific drivers > > > that used to have their child devices populated automatically would > > > stop working and would need to be updated to populate their child > > > devices. And I'm sure there are a ton more downstream kernels and > > > downstream DTs (that use upstream kernels) that we would break. > > > > > > If you really want to do that go for it, but I'd rather not do all > > > this as part of trying to fix the issue Ulf reported that needs > > > simple-bus only devices probed. > > > > Agreed. > > I'm confused. So you are okay with this patch to merge now? No, I'm okay with the separate step of removing descending 'simple-bus', etc. nodes out of of_platform_populate() to do it in the simple-bus driver. Backing up... The problem here is letting what 'simple-bus' (and simple-mfd) means change with the presence of another compatible and we can't determine if it changes or not. Has a driver or not doesn't really work. I think the only thing that works is 'simple-bus' can only mean child nodes have no dependency on the simple-bus node. If there is a dependency, then it is not a simple-bus. We might have some cases that violated this definition and they've gotten lucky so far. I think we just have to fix these cases as they arise. That's at least bounded by cases where we already have drivers and shouldn't have new cases introduced. I still like the idea that 'simple-bus' always binds to a driver, but I think for that to work we've got to support rebinding to a new driver when a better matching driver is found. We're never going to know for sure if/when there is a better match. There's a couple of cases where we need this. Rob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 2021-09-06 7:53 ` Geert Uytterhoeven @ 2021-09-07 10:41 ` Ulf Hansson 2021-09-08 22:02 ` Saravana Kannan 2021-09-09 11:01 ` Ulf Hansson 2 siblings, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2021-09-07 10:41 UTC (permalink / raw) To: Saravana Kannan Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Sat, 4 Sept 2021 at 02:05, 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, probe these > devices using the simple-pm-bus driver. > > However, there are instances of devices that are not simple buses (they > get probed by their specific drivers) that also list the "simple-bus" > (or other bus only compatible strings) in their compatible property to > automatically populate their child devices. We still want these devices > to get probed by their specific drivers. So, we make sure this driver > only probes devices that are only buses. > > [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> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> I will run some tests as soon as I can and let you know the results. Kind regards Uffe > --- > drivers/bus/simple-pm-bus.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > index 01a3d0cd08ed..3e086a9f34cb 100644 > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -13,11 +13,26 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > - > 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; > + const struct device *dev = &pdev->dev; > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > + struct device_node *np = dev->of_node; > + const struct of_device_id *match; > + > + match = of_match_device(dev->driver->of_match_table, dev); > + > + /* > + * These are transparent bus devices (not simple-pm-bus matches) that > + * have their child nodes populated automatically. So, don't need to > + * do anything more. > + */ > + if (match && match->data) { > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > + return 0; > + else > + return -ENODEV; > + } > > dev_dbg(&pdev->dev, "%s\n", __func__); > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > static int simple_pm_bus_remove(struct platform_device *pdev) > { > + const void *data = of_device_get_match_data(&pdev->dev); > + > + if (data) > + return 0; > + > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_disable(&pdev->dev); > return 0; > } > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > + > static const struct of_device_id simple_pm_bus_of_match[] = { > { .compatible = "simple-pm-bus", }, > + { .compatible = "simple-bus", .data = ONLY_BUS }, > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > + { .compatible = "isa", .data = ONLY_BUS }, > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); > -- > 2.33.0.153.gba50c8fa24-goog > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-07 10:41 ` Ulf Hansson @ 2021-09-08 22:02 ` Saravana Kannan 0 siblings, 0 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-08 22:02 UTC (permalink / raw) To: Ulf Hansson Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Tue, Sep 7, 2021 at 3:41 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Sat, 4 Sept 2021 at 02:05, 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, probe these > > devices using the simple-pm-bus driver. > > > > However, there are instances of devices that are not simple buses (they > > get probed by their specific drivers) that also list the "simple-bus" > > (or other bus only compatible strings) in their compatible property to > > automatically populate their child devices. We still want these devices > > to get probed by their specific drivers. So, we make sure this driver > > only probes devices that are only buses. > > > > [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> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > I will run some tests as soon as I can and let you know the results. Thanks. -Saravana > > Kind regards > Uffe > > > --- > > drivers/bus/simple-pm-bus.c | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > > index 01a3d0cd08ed..3e086a9f34cb 100644 > > --- a/drivers/bus/simple-pm-bus.c > > +++ b/drivers/bus/simple-pm-bus.c > > @@ -13,11 +13,26 @@ > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > > > - > > 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; > > + const struct device *dev = &pdev->dev; > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > + struct device_node *np = dev->of_node; > > + const struct of_device_id *match; > > + > > + match = of_match_device(dev->driver->of_match_table, dev); > > + > > + /* > > + * These are transparent bus devices (not simple-pm-bus matches) that > > + * have their child nodes populated automatically. So, don't need to > > + * do anything more. > > + */ > > + if (match && match->data) { > > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > > + return 0; > > + else > > + return -ENODEV; > > + } > > > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > > > static int simple_pm_bus_remove(struct platform_device *pdev) > > { > > + const void *data = of_device_get_match_data(&pdev->dev); > > + > > + if (data) > > + return 0; > > + > > dev_dbg(&pdev->dev, "%s\n", __func__); > > > > pm_runtime_disable(&pdev->dev); > > return 0; > > } > > > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > > + > > static const struct of_device_id simple_pm_bus_of_match[] = { > > { .compatible = "simple-pm-bus", }, > > + { .compatible = "simple-bus", .data = ONLY_BUS }, > > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > > + { .compatible = "isa", .data = ONLY_BUS }, > > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); > > -- > > 2.33.0.153.gba50c8fa24-goog > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 2021-09-06 7:53 ` Geert Uytterhoeven 2021-09-07 10:41 ` Ulf Hansson @ 2021-09-09 11:01 ` Ulf Hansson 2021-09-24 11:49 ` Ulf Hansson 2 siblings, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2021-09-09 11:01 UTC (permalink / raw) To: Saravana Kannan Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Sat, 4 Sept 2021 at 02:05, 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, probe these > devices using the simple-pm-bus driver. > > However, there are instances of devices that are not simple buses (they > get probed by their specific drivers) that also list the "simple-bus" > (or other bus only compatible strings) in their compatible property to > automatically populate their child devices. We still want these devices > to get probed by their specific drivers. So, we make sure this driver > only probes devices that are only buses. > > [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> Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/bus/simple-pm-bus.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c > index 01a3d0cd08ed..3e086a9f34cb 100644 > --- a/drivers/bus/simple-pm-bus.c > +++ b/drivers/bus/simple-pm-bus.c > @@ -13,11 +13,26 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > - > 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; > + const struct device *dev = &pdev->dev; > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > + struct device_node *np = dev->of_node; > + const struct of_device_id *match; > + > + match = of_match_device(dev->driver->of_match_table, dev); > + > + /* > + * These are transparent bus devices (not simple-pm-bus matches) that > + * have their child nodes populated automatically. So, don't need to > + * do anything more. > + */ > + if (match && match->data) { > + if (of_property_match_string(np, "compatible", match->compatible) == 0) > + return 0; > + else > + return -ENODEV; > + } > > dev_dbg(&pdev->dev, "%s\n", __func__); > > @@ -31,14 +46,25 @@ static int simple_pm_bus_probe(struct platform_device *pdev) > > static int simple_pm_bus_remove(struct platform_device *pdev) > { > + const void *data = of_device_get_match_data(&pdev->dev); > + > + if (data) > + return 0; > + > dev_dbg(&pdev->dev, "%s\n", __func__); > > pm_runtime_disable(&pdev->dev); > return 0; > } > > +#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */ > + > static const struct of_device_id simple_pm_bus_of_match[] = { > { .compatible = "simple-pm-bus", }, > + { .compatible = "simple-bus", .data = ONLY_BUS }, > + { .compatible = "simple-mfd", .data = ONLY_BUS }, > + { .compatible = "isa", .data = ONLY_BUS }, > + { .compatible = "arm,amba-bus", .data = ONLY_BUS }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match); > -- > 2.33.0.153.gba50c8fa24-goog > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-09 11:01 ` Ulf Hansson @ 2021-09-24 11:49 ` Ulf Hansson 2021-09-24 20:20 ` Saravana Kannan 0 siblings, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2021-09-24 11:49 UTC (permalink / raw) To: Saravana Kannan, Rob Herring Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Thu, 9 Sept 2021 at 13:01, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Sat, 4 Sept 2021 at 02:05, 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, probe these > > devices using the simple-pm-bus driver. > > > > However, there are instances of devices that are not simple buses (they > > get probed by their specific drivers) that also list the "simple-bus" > > (or other bus only compatible strings) in their compatible property to > > automatically populate their child devices. We still want these devices > > to get probed by their specific drivers. So, we make sure this driver > > only probes devices that are only buses. > > > > [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> > > Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Saravana, Rob, I have been following your latest discussion in this thread - and it looks like you guys are moving towards a consensus. Although, if there is anything I can do to help to complete this, just tell me and I will jump in immediately. [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices 2021-09-24 11:49 ` Ulf Hansson @ 2021-09-24 20:20 ` Saravana Kannan 0 siblings, 0 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-24 20:20 UTC (permalink / raw) To: Ulf Hansson Cc: Rob Herring, Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Fri, Sep 24, 2021 at 4:50 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 9 Sept 2021 at 13:01, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Sat, 4 Sept 2021 at 02:05, 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, probe these > > > devices using the simple-pm-bus driver. > > > > > > However, there are instances of devices that are not simple buses (they > > > get probed by their specific drivers) that also list the "simple-bus" > > > (or other bus only compatible strings) in their compatible property to > > > automatically populate their child devices. We still want these devices > > > to get probed by their specific drivers. So, we make sure this driver > > > only probes devices that are only buses. > > > > > > [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> > > > > Tested-by: Ulf Hansson <ulf.hansson@linaro.org> > > Saravana, Rob, > > I have been following your latest discussion in this thread - and it > looks like you guys are moving towards a consensus. > > Although, if there is anything I can do to help to complete this, just > tell me and I will jump in immediately. Looks like we settled on the allow list approach during the BoF in LPC. I'll send out a v4 with some tweaks. -Saravana ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS 2021-09-04 0:05 [PATCH v3 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan @ 2021-09-04 0:05 ` Saravana Kannan 2021-09-07 10:29 ` Ulf Hansson 2021-09-09 11:02 ` Ulf Hansson 1 sibling, 2 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-04 0:05 UTC (permalink / raw) To: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal Cc: Saravana Kannan, Ulf Hansson, Rob Herring, kernel-team, linux-arm-kernel, linux-kernel, linux-oxnas, linux-renesas-soc, linux-omap, linux-riscv The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to work with fw_devlink. So, always compile it in for CONFIG_OF and delete the config since it's no longer necessary. Signed-off-by: Saravana Kannan <saravanak@google.com> --- arch/arm/configs/multi_v7_defconfig | 1 - arch/arm/configs/oxnas_v6_defconfig | 1 - arch/arm/configs/shmobile_defconfig | 1 - arch/arm/mach-omap2/Kconfig | 1 - arch/arm64/configs/defconfig | 1 - drivers/bus/Kconfig | 12 ------------ drivers/bus/Makefile | 2 +- drivers/soc/canaan/Kconfig | 1 - 8 files changed, 1 insertion(+), 19 deletions(-) diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index d9abaae118dd..362720ae8d65 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -196,7 +196,6 @@ CONFIG_PCI_EPF_TEST=m CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y CONFIG_OMAP_OCP2SCP=y -CONFIG_SIMPLE_PM_BUS=y CONFIG_MTD=y CONFIG_MTD_CMDLINE_PARTS=y CONFIG_MTD_BLOCK=y diff --git a/arch/arm/configs/oxnas_v6_defconfig b/arch/arm/configs/oxnas_v6_defconfig index cae0db6b4eaf..de37f7e90999 100644 --- a/arch/arm/configs/oxnas_v6_defconfig +++ b/arch/arm/configs/oxnas_v6_defconfig @@ -46,7 +46,6 @@ CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y CONFIG_DMA_CMA=y CONFIG_CMA_SIZE_MBYTES=64 -CONFIG_SIMPLE_PM_BUS=y CONFIG_MTD=y CONFIG_MTD_CMDLINE_PARTS=y CONFIG_MTD_BLOCK=y diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig index d9a27e4e0914..18d2a960b2d2 100644 --- a/arch/arm/configs/shmobile_defconfig +++ b/arch/arm/configs/shmobile_defconfig @@ -40,7 +40,6 @@ CONFIG_PCI_RCAR_GEN2=y CONFIG_PCIE_RCAR_HOST=y CONFIG_DEVTMPFS=y CONFIG_DEVTMPFS_MOUNT=y -CONFIG_SIMPLE_PM_BUS=y CONFIG_MTD=y CONFIG_MTD_BLOCK=y CONFIG_MTD_CFI=y diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig index 7df8f5276ddf..02f2f3157f07 100644 --- a/arch/arm/mach-omap2/Kconfig +++ b/arch/arm/mach-omap2/Kconfig @@ -112,7 +112,6 @@ config ARCH_OMAP2PLUS select PM_GENERIC_DOMAINS select PM_GENERIC_DOMAINS_OF select RESET_CONTROLLER - select SIMPLE_PM_BUS select SOC_BUS select TI_SYSC select OMAP_IRQCHIP diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index f423d08b9a71..474b1f2e3f06 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -245,7 +245,6 @@ CONFIG_DEVTMPFS_MOUNT=y CONFIG_FW_LOADER_USER_HELPER=y CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y CONFIG_HISILICON_LPC=y -CONFIG_SIMPLE_PM_BUS=y CONFIG_FSL_MC_BUS=y CONFIG_TEGRA_ACONNECT=m CONFIG_GNSS=m diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index e7f7eee6ee9a..dc3801369488 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -141,18 +141,6 @@ config QCOM_EBI2 Interface 2, which can be used to connect things like NAND Flash, SRAM, ethernet adapters, FPGAs and LCD displays. -config SIMPLE_PM_BUS - tristate "Simple Power-Managed Bus Driver" - depends on OF && PM - help - Driver for transparent busses that don't need a real driver, but - where the bus controller is part of a PM domain, or under the control - of a functional clock, and thus relies on runtime PM for managing - this PM domain and/or clock. - An example of such a bus controller is the Renesas Bus State - Controller (BSC, sometimes called "LBSC within Bus Bridge", or - "External Bus Interface") as found on several Renesas ARM SoCs. - config SUN50I_DE2_BUS bool "Allwinner A64 DE2 Bus Driver" default ARM64 diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index 397e35392bff..86aacd36a56d 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -26,7 +26,7 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o -obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o +obj-$(CONFIG_OF) += simple-pm-bus.o obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o obj-$(CONFIG_TEGRA_GMI) += tegra-gmi.o obj-$(CONFIG_TI_PWMSS) += ti-pwmss.o diff --git a/drivers/soc/canaan/Kconfig b/drivers/soc/canaan/Kconfig index 8179b69518b4..853096b7e84c 100644 --- a/drivers/soc/canaan/Kconfig +++ b/drivers/soc/canaan/Kconfig @@ -5,7 +5,6 @@ config SOC_K210_SYSCTL depends on RISCV && SOC_CANAAN && OF default SOC_CANAAN select PM - select SIMPLE_PM_BUS select SYSCON select MFD_SYSCON help -- 2.33.0.153.gba50c8fa24-goog ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS 2021-09-04 0:05 ` [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS Saravana Kannan @ 2021-09-07 10:29 ` Ulf Hansson 2021-09-08 22:01 ` Saravana Kannan 2021-09-09 11:02 ` Ulf Hansson 1 sibling, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2021-09-07 10:29 UTC (permalink / raw) To: Saravana Kannan Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Sat, 4 Sept 2021 at 02:05, Saravana Kannan <saravanak@google.com> wrote: > > The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to > work with fw_devlink. So, always compile it in for CONFIG_OF and delete > the config since it's no longer necessary. > > Signed-off-by: Saravana Kannan <saravanak@google.com> Some comments, see below. Nevertheless, feel free to add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > arch/arm/configs/multi_v7_defconfig | 1 - > arch/arm/configs/oxnas_v6_defconfig | 1 - > arch/arm/configs/shmobile_defconfig | 1 - > arch/arm/mach-omap2/Kconfig | 1 - > arch/arm64/configs/defconfig | 1 - > drivers/bus/Kconfig | 12 ------------ > drivers/bus/Makefile | 2 +- > drivers/soc/canaan/Kconfig | 1 - > 8 files changed, 1 insertion(+), 19 deletions(-) Not sure what other people think (and it's not my call to make), but I would suggest to split this up in four pieces (drivers/bus, drivers/soc, arm, arm64) The important part is that the change in drivers/bus gets merged as part of your series, to make sure we don't break anything. > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > index d9abaae118dd..362720ae8d65 100644 > --- a/arch/arm/configs/multi_v7_defconfig > +++ b/arch/arm/configs/multi_v7_defconfig > @@ -196,7 +196,6 @@ CONFIG_PCI_EPF_TEST=m > CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > CONFIG_OMAP_OCP2SCP=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_CMDLINE_PARTS=y > CONFIG_MTD_BLOCK=y > diff --git a/arch/arm/configs/oxnas_v6_defconfig b/arch/arm/configs/oxnas_v6_defconfig > index cae0db6b4eaf..de37f7e90999 100644 > --- a/arch/arm/configs/oxnas_v6_defconfig > +++ b/arch/arm/configs/oxnas_v6_defconfig > @@ -46,7 +46,6 @@ CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > CONFIG_DMA_CMA=y > CONFIG_CMA_SIZE_MBYTES=64 > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_CMDLINE_PARTS=y > CONFIG_MTD_BLOCK=y > diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig > index d9a27e4e0914..18d2a960b2d2 100644 > --- a/arch/arm/configs/shmobile_defconfig > +++ b/arch/arm/configs/shmobile_defconfig > @@ -40,7 +40,6 @@ CONFIG_PCI_RCAR_GEN2=y > CONFIG_PCIE_RCAR_HOST=y > CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_BLOCK=y > CONFIG_MTD_CFI=y > diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig > index 7df8f5276ddf..02f2f3157f07 100644 > --- a/arch/arm/mach-omap2/Kconfig > +++ b/arch/arm/mach-omap2/Kconfig > @@ -112,7 +112,6 @@ config ARCH_OMAP2PLUS > select PM_GENERIC_DOMAINS > select PM_GENERIC_DOMAINS_OF > select RESET_CONTROLLER > - select SIMPLE_PM_BUS > select SOC_BUS > select TI_SYSC > select OMAP_IRQCHIP > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index f423d08b9a71..474b1f2e3f06 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -245,7 +245,6 @@ CONFIG_DEVTMPFS_MOUNT=y > CONFIG_FW_LOADER_USER_HELPER=y > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > CONFIG_HISILICON_LPC=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_FSL_MC_BUS=y > CONFIG_TEGRA_ACONNECT=m > CONFIG_GNSS=m > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index e7f7eee6ee9a..dc3801369488 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -141,18 +141,6 @@ config QCOM_EBI2 > Interface 2, which can be used to connect things like NAND Flash, > SRAM, ethernet adapters, FPGAs and LCD displays. > > -config SIMPLE_PM_BUS > - tristate "Simple Power-Managed Bus Driver" > - depends on OF && PM > - help > - Driver for transparent busses that don't need a real driver, but > - where the bus controller is part of a PM domain, or under the control > - of a functional clock, and thus relies on runtime PM for managing > - this PM domain and/or clock. > - An example of such a bus controller is the Renesas Bus State > - Controller (BSC, sometimes called "LBSC within Bus Bridge", or > - "External Bus Interface") as found on several Renesas ARM SoCs. > - > config SUN50I_DE2_BUS > bool "Allwinner A64 DE2 Bus Driver" > default ARM64 > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index 397e35392bff..86aacd36a56d 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -26,7 +26,7 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o > obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > -obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o > +obj-$(CONFIG_OF) += simple-pm-bus.o > obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o > obj-$(CONFIG_TEGRA_GMI) += tegra-gmi.o > obj-$(CONFIG_TI_PWMSS) += ti-pwmss.o > diff --git a/drivers/soc/canaan/Kconfig b/drivers/soc/canaan/Kconfig > index 8179b69518b4..853096b7e84c 100644 > --- a/drivers/soc/canaan/Kconfig > +++ b/drivers/soc/canaan/Kconfig > @@ -5,7 +5,6 @@ config SOC_K210_SYSCTL > depends on RISCV && SOC_CANAAN && OF > default SOC_CANAAN > select PM > - select SIMPLE_PM_BUS > select SYSCON > select MFD_SYSCON > help > -- > 2.33.0.153.gba50c8fa24-goog > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS 2021-09-07 10:29 ` Ulf Hansson @ 2021-09-08 22:01 ` Saravana Kannan 0 siblings, 0 replies; 18+ messages in thread From: Saravana Kannan @ 2021-09-08 22:01 UTC (permalink / raw) To: Ulf Hansson Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Tue, Sep 7, 2021 at 3:29 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Sat, 4 Sept 2021 at 02:05, Saravana Kannan <saravanak@google.com> wrote: > > > > The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to > > work with fw_devlink. So, always compile it in for CONFIG_OF and delete > > the config since it's no longer necessary. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > Some comments, see below. Nevertheless, feel free to add: > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks. > > Kind regards > Uffe > > > --- > > arch/arm/configs/multi_v7_defconfig | 1 - > > arch/arm/configs/oxnas_v6_defconfig | 1 - > > arch/arm/configs/shmobile_defconfig | 1 - > > arch/arm/mach-omap2/Kconfig | 1 - > > arch/arm64/configs/defconfig | 1 - > > drivers/bus/Kconfig | 12 ------------ > > drivers/bus/Makefile | 2 +- > > drivers/soc/canaan/Kconfig | 1 - > > 8 files changed, 1 insertion(+), 19 deletions(-) > > Not sure what other people think (and it's not my call to make), but I > would suggest to split this up in four pieces (drivers/bus, > drivers/soc, arm, arm64) > > The important part is that the change in drivers/bus gets merged as > part of your series, to make sure we don't break anything. I think it'll be better if it's one commit like this and we have Greg up the series. That way, there's no chance of broken trees anywhere. -Saravana > > > > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > > index d9abaae118dd..362720ae8d65 100644 > > --- a/arch/arm/configs/multi_v7_defconfig > > +++ b/arch/arm/configs/multi_v7_defconfig > > @@ -196,7 +196,6 @@ CONFIG_PCI_EPF_TEST=m > > CONFIG_DEVTMPFS=y > > CONFIG_DEVTMPFS_MOUNT=y > > CONFIG_OMAP_OCP2SCP=y > > -CONFIG_SIMPLE_PM_BUS=y > > CONFIG_MTD=y > > CONFIG_MTD_CMDLINE_PARTS=y > > CONFIG_MTD_BLOCK=y > > diff --git a/arch/arm/configs/oxnas_v6_defconfig b/arch/arm/configs/oxnas_v6_defconfig > > index cae0db6b4eaf..de37f7e90999 100644 > > --- a/arch/arm/configs/oxnas_v6_defconfig > > +++ b/arch/arm/configs/oxnas_v6_defconfig > > @@ -46,7 +46,6 @@ CONFIG_DEVTMPFS=y > > CONFIG_DEVTMPFS_MOUNT=y > > CONFIG_DMA_CMA=y > > CONFIG_CMA_SIZE_MBYTES=64 > > -CONFIG_SIMPLE_PM_BUS=y > > CONFIG_MTD=y > > CONFIG_MTD_CMDLINE_PARTS=y > > CONFIG_MTD_BLOCK=y > > diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig > > index d9a27e4e0914..18d2a960b2d2 100644 > > --- a/arch/arm/configs/shmobile_defconfig > > +++ b/arch/arm/configs/shmobile_defconfig > > @@ -40,7 +40,6 @@ CONFIG_PCI_RCAR_GEN2=y > > CONFIG_PCIE_RCAR_HOST=y > > CONFIG_DEVTMPFS=y > > CONFIG_DEVTMPFS_MOUNT=y > > -CONFIG_SIMPLE_PM_BUS=y > > CONFIG_MTD=y > > CONFIG_MTD_BLOCK=y > > CONFIG_MTD_CFI=y > > diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig > > index 7df8f5276ddf..02f2f3157f07 100644 > > --- a/arch/arm/mach-omap2/Kconfig > > +++ b/arch/arm/mach-omap2/Kconfig > > @@ -112,7 +112,6 @@ config ARCH_OMAP2PLUS > > select PM_GENERIC_DOMAINS > > select PM_GENERIC_DOMAINS_OF > > select RESET_CONTROLLER > > - select SIMPLE_PM_BUS > > select SOC_BUS > > select TI_SYSC > > select OMAP_IRQCHIP > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > > index f423d08b9a71..474b1f2e3f06 100644 > > --- a/arch/arm64/configs/defconfig > > +++ b/arch/arm64/configs/defconfig > > @@ -245,7 +245,6 @@ CONFIG_DEVTMPFS_MOUNT=y > > CONFIG_FW_LOADER_USER_HELPER=y > > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > > CONFIG_HISILICON_LPC=y > > -CONFIG_SIMPLE_PM_BUS=y > > CONFIG_FSL_MC_BUS=y > > CONFIG_TEGRA_ACONNECT=m > > CONFIG_GNSS=m > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > > index e7f7eee6ee9a..dc3801369488 100644 > > --- a/drivers/bus/Kconfig > > +++ b/drivers/bus/Kconfig > > @@ -141,18 +141,6 @@ config QCOM_EBI2 > > Interface 2, which can be used to connect things like NAND Flash, > > SRAM, ethernet adapters, FPGAs and LCD displays. > > > > -config SIMPLE_PM_BUS > > - tristate "Simple Power-Managed Bus Driver" > > - depends on OF && PM > > - help > > - Driver for transparent busses that don't need a real driver, but > > - where the bus controller is part of a PM domain, or under the control > > - of a functional clock, and thus relies on runtime PM for managing > > - this PM domain and/or clock. > > - An example of such a bus controller is the Renesas Bus State > > - Controller (BSC, sometimes called "LBSC within Bus Bridge", or > > - "External Bus Interface") as found on several Renesas ARM SoCs. > > - > > config SUN50I_DE2_BUS > > bool "Allwinner A64 DE2 Bus Driver" > > default ARM64 > > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > > index 397e35392bff..86aacd36a56d 100644 > > --- a/drivers/bus/Makefile > > +++ b/drivers/bus/Makefile > > @@ -26,7 +26,7 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > > obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o > > obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o > > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > > -obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o > > +obj-$(CONFIG_OF) += simple-pm-bus.o > > obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o > > obj-$(CONFIG_TEGRA_GMI) += tegra-gmi.o > > obj-$(CONFIG_TI_PWMSS) += ti-pwmss.o > > diff --git a/drivers/soc/canaan/Kconfig b/drivers/soc/canaan/Kconfig > > index 8179b69518b4..853096b7e84c 100644 > > --- a/drivers/soc/canaan/Kconfig > > +++ b/drivers/soc/canaan/Kconfig > > @@ -5,7 +5,6 @@ config SOC_K210_SYSCTL > > depends on RISCV && SOC_CANAAN && OF > > default SOC_CANAAN > > select PM > > - select SIMPLE_PM_BUS > > select SYSCON > > select MFD_SYSCON > > help > > -- > > 2.33.0.153.gba50c8fa24-goog > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS 2021-09-04 0:05 ` [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS Saravana Kannan 2021-09-07 10:29 ` Ulf Hansson @ 2021-09-09 11:02 ` Ulf Hansson 1 sibling, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2021-09-09 11:02 UTC (permalink / raw) To: Saravana Kannan Cc: Russell King, Neil Armstrong, Geert Uytterhoeven, Magnus Damm, Tony Lindgren, Catalin Marinas, Will Deacon, Damien Le Moal, Rob Herring, Android Kernel Team, Linux ARM, Linux Kernel Mailing List, linux-oxnas, Linux-Renesas, linux-omap, linux-riscv On Sat, 4 Sept 2021 at 02:05, Saravana Kannan <saravanak@google.com> wrote: > > The simple-pm-bus driver is mandatory for CONFIG_OF based platforms to > work with fw_devlink. So, always compile it in for CONFIG_OF and delete > the config since it's no longer necessary. > > Signed-off-by: Saravana Kannan <saravanak@google.com> Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > arch/arm/configs/multi_v7_defconfig | 1 - > arch/arm/configs/oxnas_v6_defconfig | 1 - > arch/arm/configs/shmobile_defconfig | 1 - > arch/arm/mach-omap2/Kconfig | 1 - > arch/arm64/configs/defconfig | 1 - > drivers/bus/Kconfig | 12 ------------ > drivers/bus/Makefile | 2 +- > drivers/soc/canaan/Kconfig | 1 - > 8 files changed, 1 insertion(+), 19 deletions(-) > > diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig > index d9abaae118dd..362720ae8d65 100644 > --- a/arch/arm/configs/multi_v7_defconfig > +++ b/arch/arm/configs/multi_v7_defconfig > @@ -196,7 +196,6 @@ CONFIG_PCI_EPF_TEST=m > CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > CONFIG_OMAP_OCP2SCP=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_CMDLINE_PARTS=y > CONFIG_MTD_BLOCK=y > diff --git a/arch/arm/configs/oxnas_v6_defconfig b/arch/arm/configs/oxnas_v6_defconfig > index cae0db6b4eaf..de37f7e90999 100644 > --- a/arch/arm/configs/oxnas_v6_defconfig > +++ b/arch/arm/configs/oxnas_v6_defconfig > @@ -46,7 +46,6 @@ CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > CONFIG_DMA_CMA=y > CONFIG_CMA_SIZE_MBYTES=64 > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_CMDLINE_PARTS=y > CONFIG_MTD_BLOCK=y > diff --git a/arch/arm/configs/shmobile_defconfig b/arch/arm/configs/shmobile_defconfig > index d9a27e4e0914..18d2a960b2d2 100644 > --- a/arch/arm/configs/shmobile_defconfig > +++ b/arch/arm/configs/shmobile_defconfig > @@ -40,7 +40,6 @@ CONFIG_PCI_RCAR_GEN2=y > CONFIG_PCIE_RCAR_HOST=y > CONFIG_DEVTMPFS=y > CONFIG_DEVTMPFS_MOUNT=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_MTD=y > CONFIG_MTD_BLOCK=y > CONFIG_MTD_CFI=y > diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig > index 7df8f5276ddf..02f2f3157f07 100644 > --- a/arch/arm/mach-omap2/Kconfig > +++ b/arch/arm/mach-omap2/Kconfig > @@ -112,7 +112,6 @@ config ARCH_OMAP2PLUS > select PM_GENERIC_DOMAINS > select PM_GENERIC_DOMAINS_OF > select RESET_CONTROLLER > - select SIMPLE_PM_BUS > select SOC_BUS > select TI_SYSC > select OMAP_IRQCHIP > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index f423d08b9a71..474b1f2e3f06 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -245,7 +245,6 @@ CONFIG_DEVTMPFS_MOUNT=y > CONFIG_FW_LOADER_USER_HELPER=y > CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y > CONFIG_HISILICON_LPC=y > -CONFIG_SIMPLE_PM_BUS=y > CONFIG_FSL_MC_BUS=y > CONFIG_TEGRA_ACONNECT=m > CONFIG_GNSS=m > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index e7f7eee6ee9a..dc3801369488 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -141,18 +141,6 @@ config QCOM_EBI2 > Interface 2, which can be used to connect things like NAND Flash, > SRAM, ethernet adapters, FPGAs and LCD displays. > > -config SIMPLE_PM_BUS > - tristate "Simple Power-Managed Bus Driver" > - depends on OF && PM > - help > - Driver for transparent busses that don't need a real driver, but > - where the bus controller is part of a PM domain, or under the control > - of a functional clock, and thus relies on runtime PM for managing > - this PM domain and/or clock. > - An example of such a bus controller is the Renesas Bus State > - Controller (BSC, sometimes called "LBSC within Bus Bridge", or > - "External Bus Interface") as found on several Renesas ARM SoCs. > - > config SUN50I_DE2_BUS > bool "Allwinner A64 DE2 Bus Driver" > default ARM64 > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index 397e35392bff..86aacd36a56d 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -26,7 +26,7 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > obj-$(CONFIG_QCOM_EBI2) += qcom-ebi2.o > obj-$(CONFIG_SUN50I_DE2_BUS) += sun50i-de2.o > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > -obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o > +obj-$(CONFIG_OF) += simple-pm-bus.o > obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o > obj-$(CONFIG_TEGRA_GMI) += tegra-gmi.o > obj-$(CONFIG_TI_PWMSS) += ti-pwmss.o > diff --git a/drivers/soc/canaan/Kconfig b/drivers/soc/canaan/Kconfig > index 8179b69518b4..853096b7e84c 100644 > --- a/drivers/soc/canaan/Kconfig > +++ b/drivers/soc/canaan/Kconfig > @@ -5,7 +5,6 @@ config SOC_K210_SYSCTL > depends on RISCV && SOC_CANAAN && OF > default SOC_CANAAN > select PM > - select SIMPLE_PM_BUS > select SYSCON > select MFD_SYSCON > help > -- > 2.33.0.153.gba50c8fa24-goog > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-09-24 20:20 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-04 0:05 [PATCH v3 0/2] Fix simple-bus issues with fw_devlink Saravana Kannan 2021-09-04 0:05 ` [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices Saravana Kannan 2021-09-06 7:53 ` Geert Uytterhoeven 2021-09-07 7:01 ` Saravana Kannan 2021-09-09 0:15 ` Rob Herring 2021-09-09 0:57 ` Saravana Kannan 2021-09-09 14:01 ` Rob Herring 2021-09-09 18:11 ` Saravana Kannan 2021-09-10 15:21 ` Rob Herring 2021-09-07 10:41 ` Ulf Hansson 2021-09-08 22:02 ` Saravana Kannan 2021-09-09 11:01 ` Ulf Hansson 2021-09-24 11:49 ` Ulf Hansson 2021-09-24 20:20 ` Saravana Kannan 2021-09-04 0:05 ` [PATCH v3 2/2] drivers: bus: Delete CONFIG_SIMPLE_PM_BUS Saravana Kannan 2021-09-07 10:29 ` Ulf Hansson 2021-09-08 22:01 ` Saravana Kannan 2021-09-09 11:02 ` Ulf Hansson
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).