Re: [PATCH v3 1/2] drivers: bus: simple-pm-bus: Add support for probing simple bus only devices

On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan [off-list ref] wrote:

On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven [off-list ref] wrote:

quoted

Hi Saravana,

Thanks for your patch!

CC linux-pm, Lee (mfd)

On Sat, Sep 4, 2021 at 2:05 AM Saravana Kannan [off-list ref] wrote:

quoted

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/ (local)

quoted

quoted

[1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@mail.gmail.com/ (local)
Signed-off-by: Saravana Kannan <redacted>
Tested-by: Saravana Kannan <redacted>

quoted

--- 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.

quoted

quoted

+                       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.

quoted

quoted

+       }

        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.

quoted

quoted

+       { .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[].

quoted

[*] 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

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