[RFC PATCH 0/6] qcom: Add an SoC PM driver for sc7180 using PM domains

[RFC PATCH 0/6] qcom: Add an SoC PM driver for sc7180 using PM domains

[Stephen Boyd]

This series implements an SoC PM driver for Qualcomm's SC7180 SoC that
creates devices for the nodes that make up the soc node and attaches PM
domains to those devices before the devices are added to the platform
bus. The overall plan is to remove power management logic from various
platform drivers and consolidate it into this SoC PM driver using PM
domains. This series isn't there yet, as I haven't had the time to do
much beyond this starting part.

This is a followup to my presentation at OSSEU in 2024[1]. I'm sending
it out so that the early pieces can land while we work through the PM
domain parts which I worry is going to get annoying rather quickly.

TODO:
 * Populate more child devices and attach more pm domains to test out
   more stuff
 * Set power state of PM domains to match on/off state of resources like
   clks, regulators, etc.
 * Investigate setting runtime PM state of devices before they're added
   to platform bus
 * Remove PM code from drivers using the platform_data non-NULL trick
 * Make multiple domains? Perhaps clk domain, regulator domain,
   interconnect domain, etc?
 * Provide a way for runtime active devices out of boot to be powered
   down when a driver isn't attached

[1] https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google

Stephen Boyd (6):
  bus: Extract simple-bus into self-contained driver
  dt-bindings: bus: Add qcom,soc-sc7180 SoC
  bus: Add basic sc7180 bus driver
  of: Extract alloc/add functions from of_platform_device_create_pdata()
  bus: qcom-sc7180: Attach pm domain to watchdog device
  arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

 .../bindings/bus/qcom,soc-sc7180.yaml         |  76 ++++++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |   2 +-
 drivers/bus/Kconfig                           |  26 +++
 drivers/bus/Makefile                          |   6 +
 drivers/bus/qcom/Kconfig                      |  16 ++
 drivers/bus/qcom/Makefile                     |   3 +
 drivers/bus/qcom/qcom-sc7180.c                | 173 ++++++++++++++++++
 drivers/bus/simple-bus.c                      |  79 ++++++++
 drivers/bus/simple-pm-bus.c                   |   2 +
 drivers/of/platform.c                         | 130 +++++++++++--
 include/linux/of_platform.h                   |  14 ++
 11 files changed, 511 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml
 create mode 100644 drivers/bus/qcom/Kconfig
 create mode 100644 drivers/bus/qcom/Makefile
 create mode 100644 drivers/bus/qcom/qcom-sc7180.c
 create mode 100644 drivers/bus/simple-bus.c


base-commit: 3c48780d48df029cf9d5f42b8971663e6fb975ae
-- 
https://chromeos.dev

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

[Stephen Boyd]

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.
+
+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);
-- 
https://chromeos.dev

[RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Stephen Boyd]

Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up
of multiple devices that have their own bindings, therefore this binding
is for a "bus" that is the SoC node.

TODO: Document all child nodes. This is woefully incomplete but at least
shows what is involved with describing an SoC node in dt schema.

Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: <devicetree@vger.kernel.org>
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>
---
 .../bindings/bus/qcom,soc-sc7180.yaml         | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml

diff --git a/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml b/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml
new file mode 100644
index 000000000000..56f8b75ecdab
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/qcom,soc-sc7180.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/qcom,soc-sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SC7180 SoC
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+  - Konrad Dybcio <konradybcio@kernel.org>
+
+description:
+  Qualcomm's SC7180 System on a Chip (SoC).
+
+properties:
+  compatible:
+    items:
+      - const: qcom,soc-sc7180
+      - const: simple-bus
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 2
+
+  clock-controller@100000:
+    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#
+
+  watchdog@17c10000:
+    $ref: /schemas/watchdog/qcom-wdt.yaml#
+
+required:
+  - compatible
+  - '#address-cells'
+  - '#size-cells'
+  - clock-controller@100000
+  - watchdog@17c10000
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        compatible = "qcom,soc-sc7180", "simple-bus";
+
+        // TODO: Is it possible to ignore the details?
+        clock-controller@100000 {
+          compatible = "qcom,gcc-sc7180";
+          reg = <0 0x00100000 0 0x1f0000>;
+          clocks = <&rpmhcc_RPMH_CXO_CLK>,
+                   <&rpmhcc_RPMH_CXO_CLK_A>,
+                   <&sleep_clk>;
+          clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk";
+          #clock-cells = <1>;
+          #reset-cells = <1>;
+          #power-domain-cells = <1>;
+          power-domains = <&rpmhpd_SC7180_CX>;
+        };
+
+        watchdog@17c10000 {
+          compatible = "qcom,apss-wdt-sc7180", "qcom,kpss-wdt";
+          reg = <0 0x17c10000 0 0x1000>;
+          clocks = <&sleep_clk>;
+          interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>;
+        };
+    };
+
+...
-- 
https://chromeos.dev

[RFC PATCH 3/6] bus: Add basic sc7180 bus driver

[Stephen Boyd]

Add a bus driver that does nothing besides populate devices as a child
of the soc device.

Cc: Rob Herring <robh@kernel.org>
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            |  3 ++
 drivers/bus/Makefile           |  3 ++
 drivers/bus/qcom/Kconfig       | 16 +++++++++++
 drivers/bus/qcom/Makefile      |  3 ++
 drivers/bus/qcom/qcom-sc7180.c | 51 ++++++++++++++++++++++++++++++++++
 5 files changed, 76 insertions(+)
 create mode 100644 drivers/bus/qcom/Kconfig
 create mode 100644 drivers/bus/qcom/Makefile
 create mode 100644 drivers/bus/qcom/qcom-sc7180.c

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 7c2aa1350578..69963f0f02f3 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -284,6 +284,9 @@ config OF_SIMPLE_BUS
 	  symbol if ALLOW_SIMPLE_BUS_OVERRIDE is set and there isn't another
 	  driver for the simple-bus compatible.
 
+# SoC specific drivers
+source "drivers/bus/qcom/Kconfig"
+
 source "drivers/bus/fsl-mc/Kconfig"
 source "drivers/bus/mhi/Kconfig"
 
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index f3968221d704..796dd0515578 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -40,6 +40,9 @@ obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
 
 obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
 
+# SoC specific drivers
+obj-y				+= qcom/
+
 # Must be last for driver registration ordering
 obj-$(CONFIG_OF_SIMPLE_BUS)	+= simple-bus.o
 
diff --git a/drivers/bus/qcom/Kconfig b/drivers/bus/qcom/Kconfig
new file mode 100644
index 000000000000..f4c5d05ec9ca
--- /dev/null
+++ b/drivers/bus/qcom/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menuconfig QCOM_SOC_BUS
+	tristate "Qualcomm SoC Bus Drivers"
+	depends on ARCH_QCOM || COMPILE_TEST
+
+if QCOM_SOC_BUS
+
+config QCOM_SOC_BUS_SC7180
+	tristate "Qualcomm SC7180 SoC Bus"
+	depends on ALLOW_SIMPLE_BUS_OVERRIDE
+	depends on OF_SIMPLE_BUS || !OF_SIMPLE_BUS
+	help
+	  Support for the Qualcomm SC7180 SoC bus.
+
+endif
diff --git a/drivers/bus/qcom/Makefile b/drivers/bus/qcom/Makefile
new file mode 100644
index 000000000000..5d41ad61fead
--- /dev/null
+++ b/drivers/bus/qcom/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_QCOM_SOC_BUS_SC7180)		+= qcom-sc7180.o
diff --git a/drivers/bus/qcom/qcom-sc7180.c b/drivers/bus/qcom/qcom-sc7180.c
new file mode 100644
index 000000000000..a615cf5a2129
--- /dev/null
+++ b/drivers/bus/qcom/qcom-sc7180.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SoC bus driver for Qualcomm SC7180 SoCs
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+static int qcom_soc_sc7180_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	return of_platform_populate(np, NULL, NULL, dev);
+}
+
+static const struct of_device_id qcom_soc_sc7180_match[] = {
+	{ .compatible = "qcom,soc-sc7180", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_soc_sc7180_match);
+
+static struct platform_driver qcom_soc_sc7180_driver = {
+	.probe = qcom_soc_sc7180_probe,
+	.driver = {
+		.name = "qcom-soc-sc7180",
+		.of_match_table = qcom_soc_sc7180_match,
+		.suppress_bind_attrs = true,
+	},
+};
+
+static int __init qcom_soc_sc7180_driver_init(void)
+{
+	return platform_driver_register(&qcom_soc_sc7180_driver);
+}
+/* Register before simple-bus driver. */
+arch_initcall(qcom_soc_sc7180_driver_init);
+
+static void __exit qcom_soc_sc7180_driver_exit(void)
+{
+	platform_driver_unregister(&qcom_soc_sc7180_driver);
+}
+module_exit(qcom_soc_sc7180_driver_exit);
+
+MODULE_DESCRIPTION("Qualcomm SC7180 SoC Driver");
+MODULE_LICENSE("GPL");
-- 
https://chromeos.dev

[RFC PATCH 4/6] of: Extract alloc/add functions from of_platform_device_create_pdata()

[Stephen Boyd]

Allow drivers to modify the 'struct device' for a device node by
splitting of_platform_device_create_pdata() into two functions. The
first function, of_platform_device_alloc(), allocates the platform
device and the second function, of_platform_device_add(), adds the
platform device to the platform bus. SoC power management drivers can
use these APIs to allocate a platform device for a node underneath the
soc node, attach pmdomains and/or set the device as runtime PM active,
and finally add the platform device to the platform bus.

Cc: Rob Herring <robh@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: <devicetree@vger.kernel.org>
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/of/platform.c       | 80 ++++++++++++++++++++++++++++++-------
 include/linux/of_platform.h | 14 +++++++
 2 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 63a80c30d515..d8ee2d38a382 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -138,6 +138,66 @@ struct platform_device *of_device_alloc(struct device_node *np,
 }
 EXPORT_SYMBOL(of_device_alloc);
 
+/**
+ * of_platform_device_alloc - Alloc and initialize an of_device
+ * @np: pointer to node to create device for
+ * @bus_id: name to assign device
+ * @parent: Linux device model parent device.
+ *
+ * Return: Pointer to created platform device, or NULL if a device was not
+ * allocated.  Unavailable devices will not get allocated.
+ */
+struct platform_device *
+of_platform_device_alloc(struct device_node *np, const char *bus_id,
+			  struct device *parent)
+{
+	struct platform_device *ofdev;
+
+	pr_debug("alloc platform device: %pOF\n", np);
+
+	if (!of_device_is_available(np) ||
+	    of_node_test_and_set_flag(np, OF_POPULATED))
+		return NULL;
+
+	ofdev = of_device_alloc(np, bus_id, parent);
+	if (!ofdev) {
+		of_node_clear_flag(np, OF_POPULATED);
+		return ofdev;
+	}
+
+	ofdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	if (!ofdev->dev.dma_mask)
+		ofdev->dev.dma_mask = &ofdev->dev.coherent_dma_mask;
+	ofdev->dev.bus = &platform_bus_type;
+	of_msi_configure(&ofdev->dev, ofdev->dev.of_node);
+
+	return ofdev;
+}
+EXPORT_SYMBOL(of_platform_device_alloc);
+
+/**
+ * of_platform_device_add - Add an of_device to the platform bus
+ * @ofdev: of_device to add
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int of_platform_device_add(struct platform_device *ofdev)
+{
+	struct device_node *np = ofdev->dev.of_node;
+	int ret;
+
+	pr_debug("adding platform device: %pOF\n", np);
+
+	ret = of_device_add(ofdev);
+	if (ret) {
+		platform_device_put(ofdev);
+		of_node_clear_flag(np, OF_POPULATED);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(of_platform_device_add);
+
 /**
  * of_platform_device_create_pdata - Alloc, initialize and register an of_device
  * @np: pointer to node to create device for
@@ -154,29 +214,19 @@ static struct platform_device *of_platform_device_create_pdata(
 					void *platform_data,
 					struct device *parent)
 {
+	int ret;
 	struct platform_device *dev;
 
 	pr_debug("create platform device: %pOF\n", np);
 
-	if (!of_device_is_available(np) ||
-	    of_node_test_and_set_flag(np, OF_POPULATED))
+	dev = of_platform_device_alloc(np, bus_id, parent);
+	if (!dev)
 		return NULL;
 
-	dev = of_device_alloc(np, bus_id, parent);
-	if (!dev)
-		goto err_clear_flag;
-
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	if (!dev->dev.dma_mask)
-		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
-	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
-	of_msi_configure(&dev->dev, dev->dev.of_node);
-
-	if (of_device_add(dev) != 0) {
-		platform_device_put(dev);
+	ret = of_platform_device_add(dev);
+	if (ret)
 		goto err_clear_flag;
-	}
 
 	return dev;
 
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 17471ef8e092..e55c1371b560 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -71,6 +71,10 @@ extern int of_platform_bus_probe(struct device_node *root,
 
 #ifdef CONFIG_OF_ADDRESS
 /* Platform devices and busses creation */
+extern struct platform_device *of_platform_device_alloc(struct device_node *np,
+							const char *bus_id,
+							struct device *parent);
+extern int of_platform_device_add(struct platform_device *ofdev);
 extern struct platform_device *of_platform_device_create(struct device_node *np,
 						   const char *bus_id,
 						   struct device *parent);
@@ -91,6 +95,16 @@ extern int devm_of_platform_populate(struct device *dev);
 extern void devm_of_platform_depopulate(struct device *dev);
 #else
 /* Platform devices and busses creation */
+static inline struct platform_device *of_platform_device_alloc(struct device_node *np,
+							       const char *bus_id,
+							       struct device *parent)
+{
+	return NULL;
+}
+static inline int of_platform_device_add(struct platform_device *ofdev)
+{
+	return -ENODEV;
+}
 static inline struct platform_device *of_platform_device_create(struct device_node *np,
 								const char *bus_id,
 								struct device *parent)
-- 
https://chromeos.dev

[RFC PATCH 5/6] bus: qcom-sc7180: Attach pm domain to watchdog device

[Stephen Boyd]

Find the watchdog device described as a child node of the sc7180 SoC
node and attach a generic pm domain to the device before registering the
device with the platform bus. The domain simply gets the clk and turns
it on when the pm domain is powered on and turns it off when the pm
domain is powered off.

Cc: Rob Herring <robh@kernel.org>
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/qcom/qcom-sc7180.c | 122 +++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/bus/qcom/qcom-sc7180.c b/drivers/bus/qcom/qcom-sc7180.c
index a615cf5a2129..7dfe6b32efef 100644
--- a/drivers/bus/qcom/qcom-sc7180.c
+++ b/drivers/bus/qcom/qcom-sc7180.c
@@ -3,18 +3,140 @@
  * SoC bus driver for Qualcomm SC7180 SoCs
  */
 
+#include <linux/cleanup.h>
+#include <linux/clk.h>
 #include <linux/device.h>
+#include <linux/dev_printk.h>
 #include <linux/init.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_domain.h>
+
+struct qcom_soc_pm_domain {
+	struct clk *clk;
+	struct generic_pm_domain pd;
+};
+
+static struct qcom_soc_pm_domain *
+gpd_to_qcom_soc_pm_domain(struct generic_pm_domain *gpd)
+{
+	return container_of(gpd, struct qcom_soc_pm_domain, pd);
+}
+
+static struct qcom_soc_pm_domain *pd_to_qcom_soc_pm_domain(struct dev_pm_domain *pd)
+{
+	struct generic_pm_domain *gpd;
+
+	gpd = container_of(pd, struct generic_pm_domain, domain);
+
+	return gpd_to_qcom_soc_pm_domain(gpd);
+}
+
+static struct qcom_soc_pm_domain *dev_to_qcom_soc_pm_domain(struct device *dev)
+{
+	struct dev_pm_domain *pd;
+
+	pd = dev->pm_domain;
+	if (!pd)
+		return NULL;
+
+	return pd_to_qcom_soc_pm_domain(pd);
+}
+
+static struct platform_device *
+qcom_soc_alloc_device(struct platform_device *socdev, const char *compatible)
+{
+	struct device_node *np __free(device_node);
+
+	np = of_get_compatible_child(socdev->dev.of_node, compatible);
+
+	return of_platform_device_alloc(np, NULL, &socdev->dev);
+}
+
+static int qcom_soc_domain_activate(struct device *dev)
+{
+	struct qcom_soc_pm_domain *soc_domain;
+
+	dev_info(dev, "Activating device\n");
+	soc_domain = dev_to_qcom_soc_pm_domain(dev);
+
+	soc_domain->clk = devm_clk_get(dev, NULL);
+
+	return PTR_ERR_OR_ZERO(soc_domain->clk);
+}
+
+static int qcom_soc_domain_power_on(struct generic_pm_domain *domain)
+{
+	struct qcom_soc_pm_domain *soc_domain;
+
+	pr_info("Powering on device\n");
+	soc_domain = gpd_to_qcom_soc_pm_domain(domain);
+
+	return clk_prepare_enable(soc_domain->clk);
+}
+
+static int qcom_soc_domain_power_off(struct generic_pm_domain *domain)
+{
+	struct qcom_soc_pm_domain *soc_domain;
+
+	pr_info("Powering off device\n");
+	soc_domain = gpd_to_qcom_soc_pm_domain(domain);
+
+	clk_disable_unprepare(soc_domain->clk);
+
+	return 0;
+}
+
+static int qcom_soc_add_clk_domain(struct platform_device *socdev,
+				   struct platform_device *pdev)
+{
+	struct qcom_soc_pm_domain *domain;
+	struct generic_pm_domain *pd;
+	int ret;
+
+	domain = devm_kzalloc(&socdev->dev, sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	pd = &domain->pd;
+	pd->name = "wdog";
+	ret = pm_genpd_init(pd, NULL, false);
+	if (ret)
+		return ret;
+
+	/* TODO: Wrap this in a generic_pm_domain function similar to power_on() */
+	pd->domain.activate = qcom_soc_domain_activate;
+	pd->power_on = qcom_soc_domain_power_on;
+	pd->power_off = qcom_soc_domain_power_off;
+
+	dev_info(&socdev->dev, "adding pm domain for %s\n", dev_name(&pdev->dev));
+	dev_pm_domain_set(&pdev->dev, &pd->domain);
+
+	return 0;
+}
 
 static int qcom_soc_sc7180_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct platform_device *sdev;
+	int ret;
+
+	sdev = qcom_soc_alloc_device(pdev, "qcom,apss-wdt-sc7180");
+	if (!sdev)
+		return dev_err_probe(dev, -ENODEV, "Failed to alloc sdev\n");
+
+	ret = qcom_soc_add_clk_domain(pdev, sdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add clk domain to sdev\n");
+
+	ret = of_platform_device_add(sdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add sdev to bus\n");
 
 	return of_platform_populate(np, NULL, NULL, dev);
 }
-- 
https://chromeos.dev

[RFC PATCH 6/6] arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

[Stephen Boyd]

Allow an SoC driver to probe for these devices. Add the SoC specific
compatible to the soc node. Leave the original simple-bus compatible in
place so that everything keeps working.

Cc: Rob Herring <robh@kernel.org>
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>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 76fe314d2ad5..257890a193e6 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -782,7 +782,7 @@ soc: soc@0 {
 		#size-cells = <2>;
 		ranges = <0 0 0 0 0x10 0>;
 		dma-ranges = <0 0 0 0 0x10 0>;
-		compatible = "simple-bus";
+		compatible = "qcom,soc-sc7180", "simple-bus";
 
 		gcc: clock-controller@100000 {
 			compatible = "qcom,gcc-sc7180";
-- 
https://chromeos.dev

Re: [RFC PATCH 6/6] arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

[Dmitry Baryshkov]

On Tue, Jan 07, 2025 at 05:28:43PM -0800, Stephen Boyd wrote:
> Allow an SoC driver to probe for these devices. Add the SoC specific
> compatible to the soc node. Leave the original simple-bus compatible in
> place so that everything keeps working.
> 
> Cc: Rob Herring <robh@kernel.org>
> 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>
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 76fe314d2ad5..257890a193e6 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -782,7 +782,7 @@ soc: soc@0 {
>  		#size-cells = <2>;
>  		ranges = <0 0 0 0 0x10 0>;
>  		dma-ranges = <0 0 0 0 0x10 0>;
> -		compatible = "simple-bus";
> +		compatible = "qcom,soc-sc7180", "simple-bus";

If the new driver requires this compatible, it will break compatibility
with older DT files (and it should be avoided).

>  
>  		gcc: clock-controller@100000 {
>  			compatible = "qcom,gcc-sc7180";
> -- 
> https://chromeos.dev
> 

-- 
With best wishes
Dmitry

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

[Rob Herring]

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

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

[Stephen Boyd]

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.

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

[Konrad Dybcio]

On 8.01.2025 11:44 PM, Stephen Boyd wrote:
> 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>
>>> ---

[...]

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

I think it even makes logical sense for the /soc node's compatible to
be.. you know.. the model of the SoC we're modeling

Konrad

Re: [RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Konrad Dybcio]

On 8.01.2025 2:28 AM, Stephen Boyd wrote:
> Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up
> of multiple devices that have their own bindings, therefore this binding
> is for a "bus" that is the SoC node.
> 
> TODO: Document all child nodes. This is woefully incomplete but at least
> shows what is involved with describing an SoC node in dt schema.

I'm not sure I'm a fan, because...

[...]

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: qcom,soc-sc7180
> +      - const: simple-bus
> +
> +  '#address-cells':
> +    const: 2
> +
> +  '#size-cells':
> +    const: 2
> +
> +  clock-controller@100000:
> +    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#
> +
> +  watchdog@17c10000:
> +    $ref: /schemas/watchdog/qcom-wdt.yaml#
> +
> +required:
> +  - compatible
> +  - '#address-cells'
> +  - '#size-cells'
> +  - clock-controller@100000
> +  - watchdog@17c10000
> +
> +additionalProperties: false

..this approach will make any dt node addition under /soc require
an additional bindings change, which sounds like absolute madness

I think additionalProperties: true would be sufficient here, like in
Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml

Konrad

Re: [RFC PATCH 4/6] of: Extract alloc/add functions from of_platform_device_create_pdata()

[Konrad Dybcio]

On 8.01.2025 2:28 AM, Stephen Boyd wrote:
> Allow drivers to modify the 'struct device' for a device node by
> splitting of_platform_device_create_pdata() into two functions. The
> first function, of_platform_device_alloc(), allocates the platform
> device and the second function, of_platform_device_add(), adds the
> platform device to the platform bus. SoC power management drivers can
> use these APIs to allocate a platform device for a node underneath the
> soc node, attach pmdomains and/or set the device as runtime PM active,
> and finally add the platform device to the platform bus.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: <devicetree@vger.kernel.org>
> 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>
> ---

[...]

> +/**
> + * of_platform_device_add - Add an of_device to the platform bus
> + * @ofdev: of_device to add
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int of_platform_device_add(struct platform_device *ofdev)
> +{
> +	struct device_node *np = ofdev->dev.of_node;
> +	int ret;
> +
> +	pr_debug("adding platform device: %pOF\n", np);
> +
> +	ret = of_device_add(ofdev);
> +	if (ret) {
> +		platform_device_put(ofdev);
> +		of_node_clear_flag(np, OF_POPULATED);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(of_platform_device_add);
> +
>  /**
>   * of_platform_device_create_pdata - Alloc, initialize and register an of_device
>   * @np: pointer to node to create device for
> @@ -154,29 +214,19 @@ static struct platform_device *of_platform_device_create_pdata(
>  					void *platform_data,
>  					struct device *parent)
>  {
> +	int ret;
>  	struct platform_device *dev;

ultranit: you kept the reverse order above, please keep ret last too

Konrad

Re: [RFC PATCH 6/6] arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

[Konrad Dybcio]

On 8.01.2025 2:02 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 07, 2025 at 05:28:43PM -0800, Stephen Boyd wrote:
>> Allow an SoC driver to probe for these devices. Add the SoC specific
>> compatible to the soc node. Leave the original simple-bus compatible in
>> place so that everything keeps working.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> 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>
>> ---
>>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index 76fe314d2ad5..257890a193e6 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -782,7 +782,7 @@ soc: soc@0 {
>>  		#size-cells = <2>;
>>  		ranges = <0 0 0 0 0x10 0>;
>>  		dma-ranges = <0 0 0 0 0x10 0>;
>> -		compatible = "simple-bus";
>> +		compatible = "qcom,soc-sc7180", "simple-bus";
> 
> If the new driver requires this compatible, it will break compatibility
> with older DT files (and it should be avoided).

IIUC the intent here is to provide backwards compatibility through checking
for sth like IS_SOCPM_MANAGED(), sorta like HAS_ACPI_COMPANION(). In that
case, power sequencing would be done by the socpm driver, whereas if it
doesn't hold, the resources would be toggled by the device driver

Konrad

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

[Stephen Boyd]

Quoting Konrad Dybcio (2025-01-09 06:02:17)
> On 8.01.2025 11:44 PM, Stephen Boyd wrote:
> > 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>
> >>> ---
>
> [...]
>
> > 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.
>
> I think it even makes logical sense for the /soc node's compatible to
> be.. you know.. the model of the SoC we're modeling
>

Sure. The annoying thing is that we'll have to enable the new kernel
driver to keep things working if we don't have some way to fallback to
how it used to work before. I was trying to do that with this patch but
it's easier to just force folks using new DT to use new kernels so
probably we should just do that.

Re: [RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Stephen Boyd]

Quoting Konrad Dybcio (2025-01-09 06:05:14)
> On 8.01.2025 2:28 AM, Stephen Boyd wrote:
> > Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up
> > of multiple devices that have their own bindings, therefore this binding
> > is for a "bus" that is the SoC node.
> >
> > TODO: Document all child nodes. This is woefully incomplete but at least
> > shows what is involved with describing an SoC node in dt schema.
>
> I'm not sure I'm a fan, because...
>
> [...]
>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: qcom,soc-sc7180
> > +      - const: simple-bus
> > +
> > +  '#address-cells':
> > +    const: 2
> > +
> > +  '#size-cells':
> > +    const: 2
> > +
> > +  clock-controller@100000:
> > +    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#
> > +
> > +  watchdog@17c10000:
> > +    $ref: /schemas/watchdog/qcom-wdt.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +  - clock-controller@100000
> > +  - watchdog@17c10000
> > +
> > +additionalProperties: false
>
> ..this approach will make any dt node addition under /soc require
> an additional bindings change, which sounds like absolute madness

We should pretty much know what nodes go under here though, because it's
a chip that exists and doesn't change after the fact. I agree that it
will be annoying during early development when everyone is modifying the
same file to add their node, but that problem also exists with the dts
files today so it doesn't seem like total madness. It's also nice to be
able to look at one file and quickly find all the schemas for the SoC
used, like a table of contents almost or a memory map for the chip.

One thing that I find annoying is that you have to put the whole soc
node and child nodes in the example. Maybe we can omit the example
because there are so many child nodes.

>
> I think additionalProperties: true would be sufficient here, like in
> Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml
>

Ok. That binding looks to be for the efuse properties of the SoC node
itself? I was hoping to find another example of this "describe the whole
SoC" sort of binding but that doesn't match. Is there one already out
there? Should I move this binding to bindings/soc/qcom instead of
bindings/bus?

Re: [RFC PATCH 6/6] arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 03:10:33PM +0100, Konrad Dybcio wrote:
> On 8.01.2025 2:02 PM, Dmitry Baryshkov wrote:
> > On Tue, Jan 07, 2025 at 05:28:43PM -0800, Stephen Boyd wrote:
> >> Allow an SoC driver to probe for these devices. Add the SoC specific
> >> compatible to the soc node. Leave the original simple-bus compatible in
> >> place so that everything keeps working.
> >>
> >> Cc: Rob Herring <robh@kernel.org>
> >> 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>
> >> ---
> >>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> index 76fe314d2ad5..257890a193e6 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> @@ -782,7 +782,7 @@ soc: soc@0 {
> >>  		#size-cells = <2>;
> >>  		ranges = <0 0 0 0 0x10 0>;
> >>  		dma-ranges = <0 0 0 0 0x10 0>;
> >> -		compatible = "simple-bus";
> >> +		compatible = "qcom,soc-sc7180", "simple-bus";
> > 
> > If the new driver requires this compatible, it will break compatibility
> > with older DT files (and it should be avoided).
> 
> IIUC the intent here is to provide backwards compatibility through checking
> for sth like IS_SOCPM_MANAGED(), sorta like HAS_ACPI_COMPANION(). In that
> case, power sequencing would be done by the socpm driver, whereas if it
> doesn't hold, the resources would be toggled by the device driver

I think that this way we end up having PM code both in the device driver
and in the socpm. Ideally in my opinion we should be able to migrate all
pm code to socpm, keeping compat with old DT files. In the end, if this
is the only change to the SoC.dtsi, then we should be able to live
without this compat change.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Konrad Dybcio]

On 9.01.2025 10:51 PM, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2025-01-09 06:05:14)
>> On 8.01.2025 2:28 AM, Stephen Boyd wrote:
>>> Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up
>>> of multiple devices that have their own bindings, therefore this binding
>>> is for a "bus" that is the SoC node.
>>>
>>> TODO: Document all child nodes. This is woefully incomplete but at least
>>> shows what is involved with describing an SoC node in dt schema.
>>
>> I'm not sure I'm a fan, because...
>>
>> [...]
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: qcom,soc-sc7180
>>> +      - const: simple-bus
>>> +
>>> +  '#address-cells':
>>> +    const: 2
>>> +
>>> +  '#size-cells':
>>> +    const: 2
>>> +
>>> +  clock-controller@100000:
>>> +    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#
>>> +
>>> +  watchdog@17c10000:
>>> +    $ref: /schemas/watchdog/qcom-wdt.yaml#
>>> +
>>> +required:
>>> +  - compatible
>>> +  - '#address-cells'
>>> +  - '#size-cells'
>>> +  - clock-controller@100000
>>> +  - watchdog@17c10000
>>> +
>>> +additionalProperties: false
>>
>> ..this approach will make any dt node addition under /soc require
>> an additional bindings change, which sounds like absolute madness
> 
> We should pretty much know what nodes go under here though, because it's
> a chip that exists and doesn't change after the fact. I agree that it
> will be annoying during early development when everyone is modifying the
> same file to add their node, but that problem also exists with the dts
> files today so it doesn't seem like total madness. It's also nice to be
> able to look at one file and quickly find all the schemas for the SoC
> used, like a table of contents almost or a memory map for the chip.
> 
> One thing that I find annoying is that you have to put the whole soc
> node and child nodes in the example. Maybe we can omit the example
> because there are so many child nodes.

I guess I could get behind both your and my points.. My main worry is
the day-1-support-1234-long-patch-series where 5-10% of nodes is likely
to need some remodeling, with some hw needing to be re-described in a
different way before getting merged.

Rebasing that will be an even bigger mess, but I suppose it's doable..

The same stands for the every-now-and-then occasion when we decide that
e.g. block X is not really a clock-controller, but rather a power-manager
or something.. If someone wants to rely on stable bindings in their
non-Linux OS project, requiring constant node names is one more potential
point of failure.

>> I think additionalProperties: true would be sufficient here, like in
>> Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml
>>
> 
> Ok. That binding looks to be for the efuse properties of the SoC node
> itself? I was hoping to find another example of this "describe the whole
> SoC" sort of binding but that doesn't match. Is there one already out
> there? Should I move this binding to bindings/soc/qcom instead of
> bindings/bus?

I don't think anyone has done that in the past.. maaaaybe
Documentation/devicetree/bindings/bus/st,stm32mp25-rifsc.yaml
gets close with a loose "anything with a @unit-address" as "Peripherals",
but that's not what you're looking for..

As for the directory, it seems to be all over the place for other uses of
"xyz", "simple-bus":

Documentation/devicetree/bindings/arm/arm,realview.yaml
Documentation/devicetree/bindings/arm/arm,vexpress-juno.yaml
Documentation/devicetree/bindings/arm/microchip,sparx5.yaml
Documentation/devicetree/bindings/bus/fsl,spba-bus.yaml
Documentation/devicetree/bindings/bus/st,stm32-etzpc.yaml
Documentation/devicetree/bindings/bus/st,stm32mp25-rifsc.yaml
Documentation/devicetree/bindings/net/marvell,dfx-server.yaml
Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,qe.yaml
Documentation/devicetree/bindings/soc/imx/fsl,aips-bus.yaml
Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml

Konrad

Re: [RFC PATCH 6/6] arm64: dts: qcom: sc7180: Add SoC specific compatible to soc node

[Konrad Dybcio]

On 10.01.2025 12:45 AM, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 03:10:33PM +0100, Konrad Dybcio wrote:
>> On 8.01.2025 2:02 PM, Dmitry Baryshkov wrote:
>>> On Tue, Jan 07, 2025 at 05:28:43PM -0800, Stephen Boyd wrote:
>>>> Allow an SoC driver to probe for these devices. Add the SoC specific
>>>> compatible to the soc node. Leave the original simple-bus compatible in
>>>> place so that everything keeps working.
>>>>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> 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>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>>> index 76fe314d2ad5..257890a193e6 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>>> @@ -782,7 +782,7 @@ soc: soc@0 {
>>>>  		#size-cells = <2>;
>>>>  		ranges = <0 0 0 0 0x10 0>;
>>>>  		dma-ranges = <0 0 0 0 0x10 0>;
>>>> -		compatible = "simple-bus";
>>>> +		compatible = "qcom,soc-sc7180", "simple-bus";
>>>
>>> If the new driver requires this compatible, it will break compatibility
>>> with older DT files (and it should be avoided).
>>
>> IIUC the intent here is to provide backwards compatibility through checking
>> for sth like IS_SOCPM_MANAGED(), sorta like HAS_ACPI_COMPANION(). In that
>> case, power sequencing would be done by the socpm driver, whereas if it
>> doesn't hold, the resources would be toggled by the device driver
> 
> I think that this way we end up having PM code both in the device driver
> and in the socpm. Ideally in my opinion we should be able to migrate all
> pm code to socpm, keeping compat with old DT files. In the end, if this
> is the only change to the SoC.dtsi, then we should be able to live
> without this compat change.

We should be able to do that with a dynamic overlay, I suppose.. which we
could drop after some time (probably a rather large amount of time)

Konrad

Re: [RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Rob Herring]

On Thu, Jan 09, 2025 at 01:51:12PM -0800, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2025-01-09 06:05:14)
> > On 8.01.2025 2:28 AM, Stephen Boyd wrote:
> > > Document the Qualcomm SC7180 System on a Chip (SoC). This SoC is made up
> > > of multiple devices that have their own bindings, therefore this binding
> > > is for a "bus" that is the SoC node.
> > >
> > > TODO: Document all child nodes. This is woefully incomplete but at least
> > > shows what is involved with describing an SoC node in dt schema.
> >
> > I'm not sure I'm a fan, because...
> >
> > [...]
> >
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: qcom,soc-sc7180
> > > +      - const: simple-bus
> > > +
> > > +  '#address-cells':
> > > +    const: 2
> > > +
> > > +  '#size-cells':
> > > +    const: 2
> > > +
> > > +  clock-controller@100000:
> > > +    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#

This makes the above schema be applied twice. Once here and then when 
the compatible matches. That can be avoided by just listing a 
compatible. The QCom display bindings follow that style.

> > > +
> > > +  watchdog@17c10000:
> > > +    $ref: /schemas/watchdog/qcom-wdt.yaml#
> > > +
> > > +required:
> > > +  - compatible
> > > +  - '#address-cells'
> > > +  - '#size-cells'
> > > +  - clock-controller@100000
> > > +  - watchdog@17c10000
> > > +
> > > +additionalProperties: false
> >
> > ..this approach will make any dt node addition under /soc require
> > an additional bindings change, which sounds like absolute madness
> 
> We should pretty much know what nodes go under here though, because it's
> a chip that exists and doesn't change after the fact. I agree that it
> will be annoying during early development when everyone is modifying the
> same file to add their node, but that problem also exists with the dts
> files today so it doesn't seem like total madness. It's also nice to be
> able to look at one file and quickly find all the schemas for the SoC
> used, like a table of contents almost or a memory map for the chip.

I don't really care for listing everything either.

We could just generate all the schemas used. Either "give me all the 
schemas matching some compatible patter" or "give me all the schemas 
used to validate the DTB". The latter was possible on a per node basis, 
but I think I dropped that when I changed how we select schemas to 
apply.

Speaking of memory maps, I would like a tool which could dump memory map 
from .dts. My primary reason is to find overlapping regions.

> One thing that I find annoying is that you have to put the whole soc
> node and child nodes in the example. Maybe we can omit the example
> because there are so many child nodes.
> 
> >
> > I think additionalProperties: true would be sufficient here, like in
> > Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml
> >

No. You can do:

additionalProperties:
  type: object

Or a patternProperties entry requiring '@' in the name.


> Ok. That binding looks to be for the efuse properties of the SoC node
> itself? I was hoping to find another example of this "describe the whole
> SoC" sort of binding but that doesn't match. Is there one already out
> there? Should I move this binding to bindings/soc/qcom instead of
> bindings/bus?

bindings/bus

The 'soc' nodes here aren't really for the whole SoC. Cpus aren't in 
the soc node. They are for buses. We should allow for there to be more 
than 1 for instance.

Rob

Re: [RFC PATCH 5/6] bus: qcom-sc7180: Attach pm domain to watchdog device

[Rob Herring]

On Tue, Jan 07, 2025 at 05:28:42PM -0800, Stephen Boyd wrote:
> Find the watchdog device described as a child node of the sc7180 SoC
> node and attach a generic pm domain to the device before registering the
> device with the platform bus. The domain simply gets the clk and turns
> it on when the pm domain is powered on and turns it off when the pm
> domain is powered off.
> 
> Cc: Rob Herring <robh@kernel.org>
> 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/qcom/qcom-sc7180.c | 122 +++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/drivers/bus/qcom/qcom-sc7180.c b/drivers/bus/qcom/qcom-sc7180.c
> index a615cf5a2129..7dfe6b32efef 100644
> --- a/drivers/bus/qcom/qcom-sc7180.c
> +++ b/drivers/bus/qcom/qcom-sc7180.c
> @@ -3,18 +3,140 @@
>   * SoC bus driver for Qualcomm SC7180 SoCs
>   */
>  
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
>  #include <linux/device.h>
> +#include <linux/dev_printk.h>
>  #include <linux/init.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_domain.h>
> +
> +struct qcom_soc_pm_domain {
> +	struct clk *clk;
> +	struct generic_pm_domain pd;
> +};
> +
> +static struct qcom_soc_pm_domain *
> +gpd_to_qcom_soc_pm_domain(struct generic_pm_domain *gpd)
> +{
> +	return container_of(gpd, struct qcom_soc_pm_domain, pd);
> +}
> +
> +static struct qcom_soc_pm_domain *pd_to_qcom_soc_pm_domain(struct dev_pm_domain *pd)
> +{
> +	struct generic_pm_domain *gpd;
> +
> +	gpd = container_of(pd, struct generic_pm_domain, domain);
> +
> +	return gpd_to_qcom_soc_pm_domain(gpd);
> +}
> +
> +static struct qcom_soc_pm_domain *dev_to_qcom_soc_pm_domain(struct device *dev)
> +{
> +	struct dev_pm_domain *pd;
> +
> +	pd = dev->pm_domain;
> +	if (!pd)
> +		return NULL;
> +
> +	return pd_to_qcom_soc_pm_domain(pd);
> +}
> +
> +static struct platform_device *
> +qcom_soc_alloc_device(struct platform_device *socdev, const char *compatible)
> +{
> +	struct device_node *np __free(device_node);
> +
> +	np = of_get_compatible_child(socdev->dev.of_node, compatible);
> +
> +	return of_platform_device_alloc(np, NULL, &socdev->dev);
> +}
> +
> +static int qcom_soc_domain_activate(struct device *dev)
> +{
> +	struct qcom_soc_pm_domain *soc_domain;
> +
> +	dev_info(dev, "Activating device\n");
> +	soc_domain = dev_to_qcom_soc_pm_domain(dev);
> +
> +	soc_domain->clk = devm_clk_get(dev, NULL);
> +
> +	return PTR_ERR_OR_ZERO(soc_domain->clk);
> +}
> +
> +static int qcom_soc_domain_power_on(struct generic_pm_domain *domain)
> +{
> +	struct qcom_soc_pm_domain *soc_domain;
> +
> +	pr_info("Powering on device\n");
> +	soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> +
> +	return clk_prepare_enable(soc_domain->clk);
> +}
> +
> +static int qcom_soc_domain_power_off(struct generic_pm_domain *domain)
> +{
> +	struct qcom_soc_pm_domain *soc_domain;
> +
> +	pr_info("Powering off device\n");
> +	soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> +
> +	clk_disable_unprepare(soc_domain->clk);

How's this going to scale when there are multiple clocks and it's not 
just turn on/off all the clocks in any order? Or when there's ordering 
requirements between different resources.

I'm pretty sure I've seen attempts to order clock entries in DT based on 
the order they want to enable them.

> +
> +	return 0;
> +}
> +
> +static int qcom_soc_add_clk_domain(struct platform_device *socdev,
> +				   struct platform_device *pdev)
> +{
> +	struct qcom_soc_pm_domain *domain;
> +	struct generic_pm_domain *pd;
> +	int ret;
> +
> +	domain = devm_kzalloc(&socdev->dev, sizeof(*domain), GFP_KERNEL);
> +	if (!domain)
> +		return -ENOMEM;
> +
> +	pd = &domain->pd;
> +	pd->name = "wdog";
> +	ret = pm_genpd_init(pd, NULL, false);
> +	if (ret)
> +		return ret;
> +
> +	/* TODO: Wrap this in a generic_pm_domain function similar to power_on() */
> +	pd->domain.activate = qcom_soc_domain_activate;
> +	pd->power_on = qcom_soc_domain_power_on;
> +	pd->power_off = qcom_soc_domain_power_off;
> +
> +	dev_info(&socdev->dev, "adding pm domain for %s\n", dev_name(&pdev->dev));
> +	dev_pm_domain_set(&pdev->dev, &pd->domain);
> +
> +	return 0;
> +}
>  
>  static int qcom_soc_sc7180_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	struct platform_device *sdev;
> +	int ret;
> +
> +	sdev = qcom_soc_alloc_device(pdev, "qcom,apss-wdt-sc7180");

We're going to have to have an explicit call for every child node?

> +	if (!sdev)
> +		return dev_err_probe(dev, -ENODEV, "Failed to alloc sdev\n");
> +
> +	ret = qcom_soc_add_clk_domain(pdev, sdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add clk domain to sdev\n");
> +
> +	ret = of_platform_device_add(sdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add sdev to bus\n");
>  
>  	return of_platform_populate(np, NULL, NULL, dev);
>  }
> -- 
> https://chromeos.dev
> 

Re: [RFC PATCH 2/6] dt-bindings: bus: Add qcom,soc-sc7180 SoC

[Stephen Boyd]

Quoting Rob Herring (2025-01-10 05:58:43)
> On Thu, Jan 09, 2025 at 01:51:12PM -0800, Stephen Boyd wrote:
> > Quoting Konrad Dybcio (2025-01-09 06:05:14)
> > > On 8.01.2025 2:28 AM, Stephen Boyd wrote:
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 2
> > > > +
> > > > +  clock-controller@100000:
> > > > +    $ref: /schemas/clock/qcom,gcc-sc7180.yaml#
>
> This makes the above schema be applied twice. Once here and then when
> the compatible matches. That can be avoided by just listing a
> compatible. The QCom display bindings follow that style.

Cool thanks!

>
> > > > +
> > > > +  watchdog@17c10000:
> > > > +    $ref: /schemas/watchdog/qcom-wdt.yaml#
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - '#address-cells'
> > > > +  - '#size-cells'
> > > > +  - clock-controller@100000
> > > > +  - watchdog@17c10000
> > > > +
> > > > +additionalProperties: false
> > >
> > > ..this approach will make any dt node addition under /soc require
> > > an additional bindings change, which sounds like absolute madness
> >
> > We should pretty much know what nodes go under here though, because it's
> > a chip that exists and doesn't change after the fact. I agree that it
> > will be annoying during early development when everyone is modifying the
> > same file to add their node, but that problem also exists with the dts
> > files today so it doesn't seem like total madness. It's also nice to be
> > able to look at one file and quickly find all the schemas for the SoC
> > used, like a table of contents almost or a memory map for the chip.
>
> I don't really care for listing everything either.
>
> We could just generate all the schemas used. Either "give me all the
> schemas matching some compatible patter" or "give me all the schemas
> used to validate the DTB". The latter was possible on a per node basis,
> but I think I dropped that when I changed how we select schemas to
> apply.

It is good enough to list compatible and properties like address-cells
and size-cells and then have patternProperties requiring '@' in the
name?

>
> Speaking of memory maps, I would like a tool which could dump memory map
> from .dts. My primary reason is to find overlapping regions.

Sounds cool. I don't have any need for that though so I'm not going to
jump at writing such a tool.

>
> > One thing that I find annoying is that you have to put the whole soc
> > node and child nodes in the example. Maybe we can omit the example
> > because there are so many child nodes.
> >
> > >
> > > I think additionalProperties: true would be sufficient here, like in
> > > Documentation/devicetree/bindings/soc/imx/imx8m-soc.yaml
> > >
>
> No. You can do:
>
> additionalProperties:
>   type: object
>
> Or a patternProperties entry requiring '@' in the name.

This is to make sure only child nodes can be added, right? I like
checking for '@' in the name so that random nodes can't be added that
don't have a reg property.

Re: [RFC PATCH 5/6] bus: qcom-sc7180: Attach pm domain to watchdog device

[Stephen Boyd]

Quoting Rob Herring (2025-01-10 06:09:34)
> > diff --git a/drivers/bus/qcom/qcom-sc7180.c b/drivers/bus/qcom/qcom-sc7180.c
> > index a615cf5a2129..7dfe6b32efef 100644
> > --- a/drivers/bus/qcom/qcom-sc7180.c
> > +++ b/drivers/bus/qcom/qcom-sc7180.c
> > @@ -3,18 +3,140 @@
> >   * SoC bus driver for Qualcomm SC7180 SoCs
> >   */
> >
> > +#include <linux/cleanup.h>
> > +#include <linux/clk.h>
> >  #include <linux/device.h>
> > +#include <linux/dev_printk.h>
[...]
> > +
> > +static int qcom_soc_domain_power_on(struct generic_pm_domain *domain)
> > +{
> > +     struct qcom_soc_pm_domain *soc_domain;
> > +
> > +     pr_info("Powering on device\n");
> > +     soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> > +
> > +     return clk_prepare_enable(soc_domain->clk);
> > +}
> > +
> > +static int qcom_soc_domain_power_off(struct generic_pm_domain *domain)
> > +{
> > +     struct qcom_soc_pm_domain *soc_domain;
> > +
> > +     pr_info("Powering off device\n");
> > +     soc_domain = gpd_to_qcom_soc_pm_domain(domain);
> > +
> > +     clk_disable_unprepare(soc_domain->clk);
>
> How's this going to scale when there are multiple clocks and it's not
> just turn on/off all the clocks in any order? Or when there's ordering
> requirements between different resources.

We'll need different power_on/power_off functions when the ordering
requirements are different. It would be similar to how we have different
clk_ops for different types of clks.

>
> I'm pretty sure I've seen attempts to order clock entries in DT based on
> the order they want to enable them.

Yes, I've seen that too. The order in DT shouldn't matter. The SoC PM
driver will know what order of operations to take, including between
different resources like resets, interconnects, power-domains, etc.
That's the general idea of this driver, it will coordinate all the power
for devices in the soc node, because it's written for that particular
SoC. For example, if there are ordering requirements it can get clks by
name and "do the right thing".

Another goal I have is to power off devices that aren't bound to a
driver, and/or never will be. If we can figure out the runtime PM state
of devices before adding them to the platform bus it would allow us to
power those devices off at runtime or during system suspend if userspace
isn't actively trying to power down devices. Maybe to do that we'll have
to be notified by subsystem frameworks when a provider is registered and
then once all the providers are registered, get the PM resources like
clks and interconnects, etc., figure out if they're on/off, set the
runtime PM state of the device to match, and finally add the device to
the bus. Then we can extend the driver PM core to allow userspace to
turn off devices that aren't bound to a driver, because we've moved the
SoC PM glue out of each driver into this one driver that both adds the
devices to the system and manages the device power.

If a node is status = "disabled" I'd like to still get all the PM
resources for that device and either put them into one overall SoC PM
domain, or in an "unused device" domain that we can have userspace tell
the kernel it's safe to power down those devices that were left in a
(semi-)powered state by the bootloader. Obviously I haven't gotten to
this point yet, but it's another TODO item. We could also populate those
devices but never let them be bound to a driver because they're marked
disabled in DT. Then we don't have to do anything different from devices
that are ok to use, but we waste kernel memory. Either way, the PM
resources for disabled devices need to be dealt with.

>
> > +
> > +     return 0;
> > +}
> > +
> > +static int qcom_soc_add_clk_domain(struct platform_device *socdev,
> > +                                struct platform_device *pdev)
> > +{
> > +     struct qcom_soc_pm_domain *domain;
> > +     struct generic_pm_domain *pd;
> > +     int ret;
> > +
> > +     domain = devm_kzalloc(&socdev->dev, sizeof(*domain), GFP_KERNEL);
> > +     if (!domain)
> > +             return -ENOMEM;
> > +
> > +     pd = &domain->pd;
> > +     pd->name = "wdog";
> > +     ret = pm_genpd_init(pd, NULL, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* TODO: Wrap this in a generic_pm_domain function similar to power_on() */
> > +     pd->domain.activate = qcom_soc_domain_activate;
> > +     pd->power_on = qcom_soc_domain_power_on;
> > +     pd->power_off = qcom_soc_domain_power_off;
> > +
> > +     dev_info(&socdev->dev, "adding pm domain for %s\n", dev_name(&pdev->dev));
> > +     dev_pm_domain_set(&pdev->dev, &pd->domain);
> > +
> > +     return 0;
> > +}
> >
> >  static int qcom_soc_sc7180_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct device_node *np = dev->of_node;
> > +     struct platform_device *sdev;
> > +     int ret;
> > +
> > +     sdev = qcom_soc_alloc_device(pdev, "qcom,apss-wdt-sc7180");
>
> We're going to have to have an explicit call for every child node?

Probably! Or we can populate the "complicated" ones that require
something different and then populate the rest of the nodes that didn't
get populated explicitly with some sort of loop over non-populated nodes
that attaches a simple pm domain that does generic on/off sequencing. I
haven't gotten that far to know if populating everything explicitly will
be a problem.