diff mbox series

[RFC,1/6] bus: Extract simple-bus into self-contained driver

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

Commit Message

Stephen Boyd Jan. 8, 2025, 1:28 a.m. UTC
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

Comments

Rob Herring (Arm) Jan. 8, 2025, 2:11 p.m. UTC | #1
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
diff mbox series

Patch

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);