On Tue, Sep 7, 2021 at 2:01 AM Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > On Mon, Sep 6, 2021 at 12:54 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > > > Hi Saravana, > > > > Thanks for your patch! > > > > CC linux-pm, Lee (mfd) > > > > On Sat, Sep 4, 2021 at 2:05 AM Saravana Kannan <saravanak@xxxxxxxxxx> 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@xxxxxxxxxx/ > > > > > > [1] - https://lore.kernel.org/lkml/CAPDyKFo9Bxremkb1dDrr4OcXSpE0keVze94Cm=zrkOVxHHxBmQ@xxxxxxxxxxxxxx/ > > > Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx> > > > Tested-by: Saravana Kannan <saravanak@xxxxxxxxxx> > > > > > --- 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
