| Message ID | 20250108012846.3275443-2-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qcom: Add an SoC PM driver for sc7180 using PM domains | expand |
On Tue, Jan 7, 2025 at 7:28 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Extract the simple bus into a self contained driver so that devices are > still populated when a node has two (or more) compatibles with the least > specific one being the generic "simple-bus". Allow the driver to be a > module so that in a fully modular build a driver module for the more > specific compatible will be loaded first before trying to match this > driver. > > Cc: Rob Herring <robh@kernel.org> > Cc: Saravana Kannan <saravanak@google.com> > Cc: <devicetree@vger.kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com> > Cc: Bjorn Andersson <andersson@kernel.org> > Cc: Konrad Dybcio <konradybcio@kernel.org> > Cc: <linux-arm-msm@vger.kernel.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > drivers/bus/Kconfig | 23 +++++++++++ > drivers/bus/Makefile | 3 ++ > drivers/bus/simple-bus.c | 79 +++++++++++++++++++++++++++++++++++++ > drivers/bus/simple-pm-bus.c | 2 + > drivers/of/platform.c | 50 +++++++++++++++++++++++ > 5 files changed, 157 insertions(+) > create mode 100644 drivers/bus/simple-bus.c > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index ff669a8ccad9..7c2aa1350578 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -261,6 +261,29 @@ config DA8XX_MSTPRI > configuration. Allows to adjust the priorities of all master > peripherals. > > +config ALLOW_SIMPLE_BUS_OVERRIDE > + bool "Allow simple-bus compatible OF nodes to match other drivers" > + depends on OF > + help > + Allow nodes with the "simple-bus" compatible to use a more specific > + driver which populates child devices itself. A kconfig option for this is not going to work. What does a distro kernel pick? > + > +config OF_SIMPLE_BUS > + tristate "OF Simple Bus Driver" > + depends on ALLOW_SIMPLE_BUS_OVERRIDE || COMPILE_TEST > + default ALLOW_SIMPLE_BUS_OVERRIDE > + help > + Driver for the "simple-bus" compatible nodes in DeviceTree. Child > + nodes are usually automatically populated on the platform bus when a > + node is compatible with "simple-bus". This driver maintains that > + feature but it fails probe to allow other drivers to try to probe > + with a more specific compatible if possible. > + > + Those other drivers depend on this kconfig symbol so that they match > + the builtin or modular status of this driver. Don't disable this > + symbol if ALLOW_SIMPLE_BUS_OVERRIDE is set and there isn't another > + driver for the simple-bus compatible. Giving users some rope to hang themselves? > + > source "drivers/bus/fsl-mc/Kconfig" > source "drivers/bus/mhi/Kconfig" > > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index cddd4984d6af..f3968221d704 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -40,5 +40,8 @@ obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > > obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o > > +# Must be last for driver registration ordering > +obj-$(CONFIG_OF_SIMPLE_BUS) += simple-bus.o > + > # MHI > obj-y += mhi/ > diff --git a/drivers/bus/simple-bus.c b/drivers/bus/simple-bus.c > new file mode 100644 > index 000000000000..3e39b9818566 > --- /dev/null > +++ b/drivers/bus/simple-bus.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Simple Bus Driver > + */ > + > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > + > +static struct platform_driver simple_bus_driver; > + > +static int has_specific_simple_bus_drv(struct device_driver *drv, void *dev) > +{ > + /* Skip if it's this simple bus driver */ > + if (drv == &simple_bus_driver.driver) > + return 0; > + > + if (of_driver_match_device(dev, drv)) { > + dev_dbg(dev, "Allowing '%s' to probe more specifically\n", drv->name); > + return 1; > + } > + > + return 0; > +} > + > +static int simple_bus_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > + struct device_node *np = dev->of_node; > + > + /* > + * If any other driver wants the device, leave the device to the other > + * driver. Only check drivers that come after this driver so that if an > + * earlier driver failed to probe we don't populate any devices, and > + * only check if there's a more specific compatible. > + */ > + if (of_property_match_string(np, "compatible", "simple-bus") != 0 && > + bus_for_each_drv(&platform_bus_type, &simple_bus_driver.driver, dev, > + has_specific_simple_bus_drv)) > + return -ENODEV; If this driver is built-in and the specific driver is a module, this doesn't work, right? I think we're going to have to have a list of specific compatibles that have or don't have a specific driver. Today, the list with specific drivers is short, but that could easily change if this becomes the way to handle things. We should consider if new platforms should just avoid 'simple-bus' and use something new. Rob
Quoting Rob Herring (2025-01-08 06:11:28) > On Tue, Jan 7, 2025 at 7:28 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Extract the simple bus into a self contained driver so that devices are > > still populated when a node has two (or more) compatibles with the least > > specific one being the generic "simple-bus". Allow the driver to be a > > module so that in a fully modular build a driver module for the more > > specific compatible will be loaded first before trying to match this > > driver. > > > > Cc: Rob Herring <robh@kernel.org> > > Cc: Saravana Kannan <saravanak@google.com> > > Cc: <devicetree@vger.kernel.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com> > > Cc: Bjorn Andersson <andersson@kernel.org> > > Cc: Konrad Dybcio <konradybcio@kernel.org> > > Cc: <linux-arm-msm@vger.kernel.org> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > --- > > drivers/bus/Kconfig | 23 +++++++++++ > > drivers/bus/Makefile | 3 ++ > > drivers/bus/simple-bus.c | 79 +++++++++++++++++++++++++++++++++++++ > > drivers/bus/simple-pm-bus.c | 2 + > > drivers/of/platform.c | 50 +++++++++++++++++++++++ > > 5 files changed, 157 insertions(+) > > create mode 100644 drivers/bus/simple-bus.c > > > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > > index ff669a8ccad9..7c2aa1350578 100644 > > --- a/drivers/bus/Kconfig > > +++ b/drivers/bus/Kconfig > > @@ -261,6 +261,29 @@ config DA8XX_MSTPRI > > configuration. Allows to adjust the priorities of all master > > peripherals. > > > > +config ALLOW_SIMPLE_BUS_OVERRIDE > > + bool "Allow simple-bus compatible OF nodes to match other drivers" > > + depends on OF > > + help > > + Allow nodes with the "simple-bus" compatible to use a more specific > > + driver which populates child devices itself. > > A kconfig option for this is not going to work. What does a distro kernel pick? I was thinking a distro kernel would set it to =Y > > > + > > +config OF_SIMPLE_BUS > > + tristate "OF Simple Bus Driver" > > + depends on ALLOW_SIMPLE_BUS_OVERRIDE || COMPILE_TEST > > + default ALLOW_SIMPLE_BUS_OVERRIDE > > + help > > + Driver for the "simple-bus" compatible nodes in DeviceTree. Child > > + nodes are usually automatically populated on the platform bus when a > > + node is compatible with "simple-bus". This driver maintains that > > + feature but it fails probe to allow other drivers to try to probe > > + with a more specific compatible if possible. > > + > > + Those other drivers depend on this kconfig symbol so that they match > > + the builtin or modular status of this driver. Don't disable this > > + symbol if ALLOW_SIMPLE_BUS_OVERRIDE is set and there isn't another > > + driver for the simple-bus compatible. > > Giving users some rope to hang themselves? Heh. > > > + > > source "drivers/bus/fsl-mc/Kconfig" > > source "drivers/bus/mhi/Kconfig" > > > > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > > index cddd4984d6af..f3968221d704 100644 > > --- a/drivers/bus/Makefile > > +++ b/drivers/bus/Makefile > > @@ -40,5 +40,8 @@ obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > > > > obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o > > > > +# Must be last for driver registration ordering > > +obj-$(CONFIG_OF_SIMPLE_BUS) += simple-bus.o > > + > > # MHI > > obj-y += mhi/ > > diff --git a/drivers/bus/simple-bus.c b/drivers/bus/simple-bus.c > > new file mode 100644 > > index 000000000000..3e39b9818566 > > --- /dev/null > > +++ b/drivers/bus/simple-bus.c > > @@ -0,0 +1,79 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Simple Bus Driver > > + */ > > + > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > + > > +static struct platform_driver simple_bus_driver; > > + > > +static int has_specific_simple_bus_drv(struct device_driver *drv, void *dev) > > +{ > > + /* Skip if it's this simple bus driver */ > > + if (drv == &simple_bus_driver.driver) > > + return 0; > > + > > + if (of_driver_match_device(dev, drv)) { > > + dev_dbg(dev, "Allowing '%s' to probe more specifically\n", drv->name); > > + return 1; > > + } > > + > > + return 0; > > +} > > + > > +static int simple_bus_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); > > + struct device_node *np = dev->of_node; > > + > > + /* > > + * If any other driver wants the device, leave the device to the other > > + * driver. Only check drivers that come after this driver so that if an > > + * earlier driver failed to probe we don't populate any devices, and > > + * only check if there's a more specific compatible. > > + */ > > + if (of_property_match_string(np, "compatible", "simple-bus") != 0 && > > + bus_for_each_drv(&platform_bus_type, &simple_bus_driver.driver, dev, > > + has_specific_simple_bus_drv)) > > + return -ENODEV; > > If this driver is built-in and the specific driver is a module, this > doesn't work, right? Yes. The thinking is that if the specific driver is a module we would make the simple bus driver be a module as well. > > I think we're going to have to have a list of specific compatibles > that have or don't have a specific driver. Today, the list with > specific drivers is short, but that could easily change if this > becomes the way to handle things. Are you talking about simple_pm_bus_probe()? I don't know how to make a list of specific compatibles work because we can't know if the more specific driver has been compiled or shipped as a module. We could get stuck waiting forever for the specific driver to be registered, leading to what looks like a non-working system because the simple-bus driver refused to probe. I decided to punt on that problem by relying on userspace to load the specific driver module before the simple-bus one. Or if the two drivers are builtin then the specific driver would be registered before the simple-bus driver via link ordering and it will work. > We should consider if new platforms > should just avoid 'simple-bus' and use something new. Yes, I think new platforms should entirely avoid "simple-bus" for their SoC node. One idea I had was to populate the devices for simple-bus and then remove them if a more specific driver registers to allow the specific driver to bind and do what it wants. That's not great because we would almost always cycle through deleting devices and repopulating them, unless we play tricks with initcall ordering. The benefit is that we don't have to do gymnastics to avoid the probe of the simple-bus driver. Maybe the best approach is to simply avoid all of this and drop the "simple-bus" compatible from the soc node? It introduces an annoying hurdle where you have to enable the new driver that does exactly the same thing as "simple-bus" does so you continue to have a working system, but it avoids the headaches of trying to make the fallback to "simple-bus" work and it would match how new DTs would be written. We could make the driver 'default ARCH_<SOC_VENDOR>' so that it gets built for olddefconfig users too.
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index ff669a8ccad9..7c2aa1350578 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -261,6 +261,29 @@ config DA8XX_MSTPRI configuration. Allows to adjust the priorities of all master peripherals. +config ALLOW_SIMPLE_BUS_OVERRIDE + bool "Allow simple-bus compatible OF nodes to match other drivers" + depends on OF + help + Allow nodes with the "simple-bus" compatible to use a more specific + driver which populates child devices itself. + +config OF_SIMPLE_BUS + tristate "OF Simple Bus Driver" + depends on ALLOW_SIMPLE_BUS_OVERRIDE || COMPILE_TEST + default ALLOW_SIMPLE_BUS_OVERRIDE + help + Driver for the "simple-bus" compatible nodes in DeviceTree. Child + nodes are usually automatically populated on the platform bus when a + node is compatible with "simple-bus". This driver maintains that + feature but it fails probe to allow other drivers to try to probe + with a more specific compatible if possible. + + Those other drivers depend on this kconfig symbol so that they match + the builtin or modular status of this driver. Don't disable this + symbol if ALLOW_SIMPLE_BUS_OVERRIDE is set and there isn't another + driver for the simple-bus compatible. + source "drivers/bus/fsl-mc/Kconfig" source "drivers/bus/mhi/Kconfig" diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index cddd4984d6af..f3968221d704 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -40,5 +40,8 @@ obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o obj-$(CONFIG_DA8XX_MSTPRI) += da8xx-mstpri.o +# Must be last for driver registration ordering +obj-$(CONFIG_OF_SIMPLE_BUS) += simple-bus.o + # MHI obj-y += mhi/ diff --git a/drivers/bus/simple-bus.c b/drivers/bus/simple-bus.c new file mode 100644 index 000000000000..3e39b9818566 --- /dev/null +++ b/drivers/bus/simple-bus.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Simple Bus Driver + */ + +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +static struct platform_driver simple_bus_driver; + +static int has_specific_simple_bus_drv(struct device_driver *drv, void *dev) +{ + /* Skip if it's this simple bus driver */ + if (drv == &simple_bus_driver.driver) + return 0; + + if (of_driver_match_device(dev, drv)) { + dev_dbg(dev, "Allowing '%s' to probe more specifically\n", drv->name); + return 1; + } + + return 0; +} + +static int simple_bus_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct of_dev_auxdata *lookup = dev_get_platdata(dev); + struct device_node *np = dev->of_node; + + /* + * If any other driver wants the device, leave the device to the other + * driver. Only check drivers that come after this driver so that if an + * earlier driver failed to probe we don't populate any devices, and + * only check if there's a more specific compatible. + */ + if (of_property_match_string(np, "compatible", "simple-bus") != 0 && + bus_for_each_drv(&platform_bus_type, &simple_bus_driver.driver, dev, + has_specific_simple_bus_drv)) + return -ENODEV; + + if (np) + of_platform_populate(np, NULL, lookup, dev); + + return 0; +} + +static const struct of_device_id simple_bus_of_match[] = { + { .compatible = "simple-bus", }, + { } +}; +MODULE_DEVICE_TABLE(of, simple_bus_of_match); + +static struct platform_driver simple_bus_driver = { + .probe = simple_bus_probe, + .driver = { + .name = "simple-bus", + .of_match_table = simple_bus_of_match, + }, +}; + +static int __init simple_bus_driver_init(void) +{ + return platform_driver_register(&simple_bus_driver); +} +arch_initcall(simple_bus_driver_init); + +static void __exit simple_bus_driver_exit(void) +{ + platform_driver_unregister(&simple_bus_driver); +} +module_exit(simple_bus_driver_exit); + +MODULE_DESCRIPTION("Simple Bus Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c index 5dea31769f9a..be9879aa80c1 100644 --- a/drivers/bus/simple-pm-bus.c +++ b/drivers/bus/simple-pm-bus.c @@ -118,7 +118,9 @@ static const struct dev_pm_ops simple_pm_bus_pm_ops = { static const struct of_device_id simple_pm_bus_of_match[] = { { .compatible = "simple-pm-bus", }, +#ifndef CONFIG_ALLOW_SIMPLE_BUS_OVERRIDE { .compatible = "simple-bus", .data = ONLY_BUS }, +#endif { .compatible = "simple-mfd", .data = ONLY_BUS }, { .compatible = "isa", .data = ONLY_BUS }, { .compatible = "arm,amba-bus", .data = ONLY_BUS }, diff --git a/drivers/of/platform.c b/drivers/of/platform.c index c6d8afb284e8..63a80c30d515 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -311,6 +311,54 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l return NULL; } +/** + * of_platform_should_populate_children() - Should child nodes be populated for a bus + * @bus: device node of the bus to populate children for + * @matches: match table for bus nodes + * + * This function is used to determine if child nodes should be populated as + * devices for a bus. That is usually the case, unless + * CONFIG_ALLOW_SIMPLE_BUS_OVERRIDE=y, in which case the simple-bus driver + * (CONFIG_OF_SIMPLE_BUS) will populate them. + * + * Return: True if child nodes should be populated as devices, false otherwise. + */ +static bool of_platform_should_populate_children(const struct of_device_id *matches, + struct device_node *bus) +{ + /* Not configured to allow simple-bus to be overridden. Skip. */ + if (!IS_ENABLED(CONFIG_ALLOW_SIMPLE_BUS_OVERRIDE)) + return true; + + /* The simple-bus driver will handle it. */ + if (IS_ENABLED(CONFIG_OF_SIMPLE_BUS)) + return false; + + if (!matches) + return true; + + /* + * Always populate if the matches aren't populating a "simple-bus" + * compatible node. + */ + for (; matches->name[0] || matches->type[0] || matches->compatible[0]; matches++) { + if (!strncmp(matches->compatible, "simple-bus", + ARRAY_SIZE(matches->compatible))) { + /* + * Always populate if "simple-bus" is the first + * compatible, so that CONFIG_OF_SIMPLE_BUS can be + * disabled while CONFIG_ALLOW_SIMPLE_BUS_OVERRIDE can + * be enabled. + */ + if (of_property_match_string(bus, "compatible", "simple-bus") != 0) + return false; + break; + } + } + + return true; +} + /** * of_platform_bus_create() - Create a device for a node and its children. * @bus: device node of the bus to instantiate @@ -370,6 +418,8 @@ static int of_platform_bus_create(struct device_node *bus, dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); if (!dev || !of_match_node(matches, bus)) return 0; + if (!of_platform_should_populate_children(matches, bus)) + return 0; for_each_child_of_node_scoped(bus, child) { pr_debug(" create child: %pOF\n", child);
Extract the simple bus into a self contained driver so that devices are still populated when a node has two (or more) compatibles with the least specific one being the generic "simple-bus". Allow the driver to be a module so that in a fully modular build a driver module for the more specific compatible will be loaded first before trying to match this driver. Cc: Rob Herring <robh@kernel.org> Cc: Saravana Kannan <saravanak@google.com> Cc: <devicetree@vger.kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com> Cc: Bjorn Andersson <andersson@kernel.org> Cc: Konrad Dybcio <konradybcio@kernel.org> Cc: <linux-arm-msm@vger.kernel.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/bus/Kconfig | 23 +++++++++++ drivers/bus/Makefile | 3 ++ drivers/bus/simple-bus.c | 79 +++++++++++++++++++++++++++++++++++++ drivers/bus/simple-pm-bus.c | 2 + drivers/of/platform.c | 50 +++++++++++++++++++++++ 5 files changed, 157 insertions(+) create mode 100644 drivers/bus/simple-bus.c