[PATCH 0/4] clk: implement sync_state support

[PATCH 0/4] clk: implement sync_state support

[Brian Masney]

The existing support for disabling unused clks runs in the late initcall
stage, and it has been known for a long time that this is broken since
it runs too early in the boot up process. It doesn't work for kernel
modules, and it also doesn't work if all of the consumers haven't fully
probed yet. Folks have long recommended to boot certain platforms with
clk_ignore_unused to work around issues with disabling unused clks.

This series:
- Adds a generic clk_sync_state() callback that's initially empty.
- Adds a generic qcom_cc_sync_state() for qcom SoCs that need to
  interact with the Interconnect framework.
- Converts the 7 qcom drivers from using icc_sync_state() to
  qcom_cc_sync_state().
- Implement the framework-level sync state via clk_sync_state().

This approach maintains bisectability.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
Brian Masney (4):
      clk: introduce stub clk_sync_state()
      clk: qcom: common: introduce qcom_cc_sync_state()
      clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()
      clk: implement sync_state support

 drivers/clk/clk.c                | 75 +++++++++++++++++++++++++++++++++-------
 drivers/clk/qcom/apss-ipq5424.c  |  3 +-
 drivers/clk/qcom/clk-cbf-8996.c  |  5 ++-
 drivers/clk/qcom/common.c        |  9 +++++
 drivers/clk/qcom/common.h        |  1 +
 drivers/clk/qcom/gcc-ipq5332.c   |  3 +-
 drivers/clk/qcom/gcc-ipq5424.c   |  3 +-
 drivers/clk/qcom/gcc-ipq9574.c   |  3 +-
 drivers/clk/qcom/nsscc-ipq5424.c |  3 +-
 drivers/clk/qcom/nsscc-ipq9574.c |  3 +-
 include/linux/clk.h              | 14 ++++++++
 11 files changed, 95 insertions(+), 27 deletions(-)
---
base-commit: 08484c504b55a98bd100527fbe10a3caf55ff3ff
change-id: 20260602-clk-sync-state-c0539c5530f4

Best regards,
-- 
Brian Masney <bmasney@redhat.com>

[PATCH 1/4] clk: introduce stub clk_sync_state()

[Brian Masney]

Add a stub clk_sync_state() since this will be needed for some clk
drivers that already have a sync_state callback. This allows introducing
all of the necessary changes into the drivers before actually adding the
real clk_sync_state() implementation. Doing it this way ensures that the
tree stays bisectable, and the individual changes are small patches.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c   |  6 ++++++
 include/linux/clk.h | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 048adfa86a5d..9cb2b42d1be4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1557,6 +1557,12 @@ static int __init clk_disable_unused(void)
 }
 late_initcall_sync(clk_disable_unused);
 
+void clk_sync_state(struct device *dev)
+{
+	/* Will fill in */
+}
+EXPORT_SYMBOL_GPL(clk_sync_state);
+
 static int clk_core_determine_round_nolock(struct clk_core *core,
 					   struct clk_rate_request *req)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 998ba3f261da..31a0c9224c46 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -846,6 +846,20 @@ void devm_clk_put(struct device *dev, struct clk *clk);
  */
 
 
+/*
+ * clk_sync_state - sync_state callback to disable unused clocks
+ * @dev: the clock provider device whose unused clocks should be disabled
+ *
+ * It is called by the driver core once all consumers of @dev have probed,
+ * and disables any clocks belonging to @dev that are unused at that point.
+ *
+ * If a clock provider doesn't have a sync_state callback, then the framework
+ * will set up clk_sync_state() on your drivers behalf. If your driver needs
+ * a sync_state callback, then that callback also needs to call
+ * clk_sync_state().
+ */
+void clk_sync_state(struct device *dev);
+
 /**
  * clk_round_rate - adjust a rate to the exact rate a clock can provide
  * @clk: clock source

-- 
2.54.0

[PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney]

Several qcom clk providers currently have a sync_state helper set to
icc_sync_state(). With an upcoming change to the clk framework, if
sync_state is not defined for the device, then the clk framework sets it
to clk_sync_state().

Clk providers that require their own sync_state will need to call the
framework level clk_sync_state(). Let's introduce a new common helper
qcom_cc_sync_state() that calls icc_sync_state() and clk_sync_state().

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/qcom/common.c | 9 +++++++++
 drivers/clk/qcom/common.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index eec369d2173b..31382c49c948 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -3,12 +3,14 @@
  * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/clk.h>
 #include <linux/export.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
 #include <linux/clk-provider.h>
 #include <linux/interconnect-clk.h>
+#include <linux/interconnect-provider.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset-controller.h>
 #include <linux/of.h>
@@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
 }
 EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
 
+void qcom_cc_sync_state(struct device *dev)
+{
+	icc_sync_state(dev);
+	clk_sync_state(dev);
+}
+EXPORT_SYMBOL_GPL(qcom_cc_sync_state);
+
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("QTI Common Clock module");
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 6f2406f8839e..ee448163b1fc 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -88,5 +88,6 @@ extern int qcom_cc_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc);
 extern int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
 				  const struct qcom_cc_desc *desc);
+extern void qcom_cc_sync_state(struct device *dev);
 
 #endif

-- 
2.54.0

[PATCH 3/4] clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()

[Brian Masney]

Convert all of the qcom clk drivers from icc_sync_state() to
qcom_cc_sync_state().

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/qcom/apss-ipq5424.c  | 3 +--
 drivers/clk/qcom/clk-cbf-8996.c  | 5 ++---
 drivers/clk/qcom/gcc-ipq5332.c   | 3 +--
 drivers/clk/qcom/gcc-ipq5424.c   | 3 +--
 drivers/clk/qcom/gcc-ipq9574.c   | 3 +--
 drivers/clk/qcom/nsscc-ipq5424.c | 3 +--
 drivers/clk/qcom/nsscc-ipq9574.c | 3 +--
 7 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/qcom/apss-ipq5424.c b/drivers/clk/qcom/apss-ipq5424.c
index 1662c83058bc..70991c3a9a58 100644
--- a/drivers/clk/qcom/apss-ipq5424.c
+++ b/drivers/clk/qcom/apss-ipq5424.c
@@ -7,7 +7,6 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
-#include <linux/interconnect-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -248,7 +247,7 @@ static struct platform_driver apss_ipq5424_driver = {
 	.driver = {
 		.name   = "apss-ipq5424-clk",
 		.of_match_table = apss_ipq5424_match_table,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 
diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
index 0b40ed601f9a..0d920bd5a992 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -6,7 +6,6 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/interconnect-clk.h>
-#include <linux/interconnect-provider.h>
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -249,7 +248,7 @@ static void qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
 
 	icc_clk_unregister(provider);
 }
-#define qcom_msm8996_cbf_icc_sync_state icc_sync_state
+#define qcom_msm8996_cbf_icc_sync_state qcom_cc_sync_state
 #else
 static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev,  struct clk_hw *cbf_hw)
 {
@@ -258,7 +257,7 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev,  struct c
 	return 0;
 }
 #define qcom_msm8996_cbf_icc_remove(pdev) { }
-#define qcom_msm8996_cbf_icc_sync_state NULL
+#define qcom_msm8996_cbf_icc_sync_state clk_sync_state
 #endif
 
 static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
index 9246e97d785a..adec25aa9c27 100644
--- a/drivers/clk/qcom/gcc-ipq5332.c
+++ b/drivers/clk/qcom/gcc-ipq5332.c
@@ -4,7 +4,6 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/interconnect-provider.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -3307,7 +3306,7 @@ static struct platform_driver gcc_ipq5332_driver = {
 	.driver = {
 		.name = "gcc-ipq5332",
 		.of_match_table = gcc_ipq5332_match_table,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 
diff --git a/drivers/clk/qcom/gcc-ipq5424.c b/drivers/clk/qcom/gcc-ipq5424.c
index 35af6ffeeb85..9bf3bb6f8904 100644
--- a/drivers/clk/qcom/gcc-ipq5424.c
+++ b/drivers/clk/qcom/gcc-ipq5424.c
@@ -5,7 +5,6 @@
  */
 
 #include <linux/clk-provider.h>
-#include <linux/interconnect-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -3322,7 +3321,7 @@ static struct platform_driver gcc_ipq5424_driver = {
 	.driver = {
 		.name   = "qcom,gcc-ipq5424",
 		.of_match_table = gcc_ipq5424_match_table,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 
diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
index 6dc86e686de4..9edcfd8afb77 100644
--- a/drivers/clk/qcom/gcc-ipq9574.c
+++ b/drivers/clk/qcom/gcc-ipq9574.c
@@ -5,7 +5,6 @@
 
 #include <linux/clk-provider.h>
 #include <linux/interconnect-clk.h>
-#include <linux/interconnect-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -4134,7 +4133,7 @@ static struct platform_driver gcc_ipq9574_driver = {
 	.driver = {
 		.name   = "qcom,gcc-ipq9574",
 		.of_match_table = gcc_ipq9574_match_table,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 
diff --git a/drivers/clk/qcom/nsscc-ipq5424.c b/drivers/clk/qcom/nsscc-ipq5424.c
index 5893c7146180..253d625ebf35 100644
--- a/drivers/clk/qcom/nsscc-ipq5424.c
+++ b/drivers/clk/qcom/nsscc-ipq5424.c
@@ -6,7 +6,6 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
-#include <linux/interconnect-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -1331,7 +1330,7 @@ static struct platform_driver nss_cc_ipq5424_driver = {
 		.name = "qcom,ipq5424-nsscc",
 		.of_match_table = nss_cc_ipq5424_match_table,
 		.pm = &nss_cc_ipq5424_pm_ops,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 module_platform_driver(nss_cc_ipq5424_driver);
diff --git a/drivers/clk/qcom/nsscc-ipq9574.c b/drivers/clk/qcom/nsscc-ipq9574.c
index c8b11b04a7c2..be8fbf5edda4 100644
--- a/drivers/clk/qcom/nsscc-ipq9574.c
+++ b/drivers/clk/qcom/nsscc-ipq9574.c
@@ -7,7 +7,6 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/err.h>
-#include <linux/interconnect-provider.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -3100,7 +3099,7 @@ static struct platform_driver nss_cc_ipq9574_driver = {
 		.name = "qcom,nsscc-ipq9574",
 		.of_match_table = nss_cc_ipq9574_match_table,
 		.pm = &nss_cc_ipq9574_pm_ops,
-		.sync_state = icc_sync_state,
+		.sync_state = qcom_cc_sync_state,
 	},
 };
 

-- 
2.54.0

[PATCH 4/4] clk: implement sync_state support

[Brian Masney]

The existing support for disabling unused clks runs in the late initcall
stage, and it has been known for a long time that this is broken since
it runs too early in the boot up process. It doesn't work for kernel
modules, and it also doesn't work if all of the consumers haven't fully
probed yet. Folks have long recommended to boot certain platforms with
clk_ignore_unused to work around issues with disabling unused clks.

Let's go ahead and add a framework-level sync_state callback for the clk
subsystem. If a driver doesn't have a sync_state callback configured,
which is the 99+% use case today, then let's set it up to use the
clk_sync_state() introduced in this commit so that no driver changes
are needed.

At the time of this writing, there are currently only 7 clk drivers that
implement sync_state, and all are Qualcomm SoCs where they interact with
the interconnect framework via icc_sync_state(). A shared helper has
been created for this platform that calls clk_sync_state(). It is
expected that any new clk drivers that want to implement their own
sync_state will also need to call clk_sync_state() at the end of their
custom sync_state callback.

There will be several stages of disabling unused clks:

- The first phase will be executed at late_initcall and it will only
  disable unused clks that do not have a struct dev.
- The sync_state callback will be invoked for each clk driver once all
  consumers have probed.

This is based on previous attempts by Saravana Kannan and Abel Vesa
that are linked below.

This change was tested on a Thinkpad x13s laptop.

    [    0.308051] clk: Disabling unused clocks not associated with a device
    [    6.541069] qcom_aoss_qmp c300000.power-management: clk: Disabling unused clocks
    [    6.843310] qcom-qmp-pcie-phy 1c24000.phy: clk: Disabling unused clocks
    [    7.604556] qcom-qmp-pcie-phy 1c14000.phy: clk: Disabling unused clocks
    [    8.446161] qcom-qmp-usb-phy 88f1000.phy: clk: Disabling unused clocks
    [    8.446293] qcom-qmp-usb-phy 88ef000.phy: clk: Disabling unused clocks
    [    8.546067] qcom-qmp-combo-phy 88eb000.phy: clk: Disabling unused clocks
    [    8.546203] qcom-qmp-combo-phy 8903000.phy: clk: Disabling unused clocks
    [    8.546254] qcom-edp-phy aec5a00.phy: clk: Disabling unused clocks
    [   15.436834] qcom-cpufreq-hw 18591000.cpufreq: clk: Disabling unused clocks
    [   15.436953] clk-rpmh 18200000.rsc:clock-controller: clk: Disabling unused clocks
    [   15.723348] qcom-qmp-pcie-phy 1c06000.phy: clk: Disabling unused clocks
    [   21.063241] q6prm-lpass-clock 3000000.remoteproc:glink-edge:gpr:service@2:clock-controller: clk: Disabling unused clocks
    [   21.081996] va_macro 3370000.codec: clk: Disabling unused clocks
    [   21.092740] rx_macro 3200000.rxmacro: clk: Disabling unused clocks
    [   21.118261] wsa_macro 3240000.codec: clk: Disabling unused clocks
    [   21.128758] tx_macro 3220000.txmacro: clk: Disabling unused clocks

Signed-off-by: Brian Masney <bmasney@redhat.com>
Link: https://www.youtube.com/watch?v=tXYzM8yLIQA
Link: https://lore.kernel.org/all/20210407034456.516204-1-saravanak@google.com/
Link: https://lore.kernel.org/all/20221227204528.1899863-1-abel.vesa@linaro.org/
---
 drivers/clk/clk.c | 71 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9cb2b42d1be4..ab3f8becda0c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1442,14 +1442,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
 	clk_core_unprepare_lock(core);
 }
 
-static void __init clk_unprepare_unused_subtree(struct clk_core *core)
+/*
+ * Returns true if @core should be skipped during an unused-clock sweep for
+ * @dev.  When @dev is NULL the sweep is the global late_initcall pass; when
+ * @dev is non-NULL the sweep is a per-device sync_state pass.
+ */
+static bool clk_core_skip_unused(struct clk_core *core, struct device *dev)
+{
+	/*
+	 * At late_initcall, skip clocks that belong to a device — they will be
+	 * handled at sync_state time.
+	 */
+	if (!dev && core->dev)
+		return true;
+
+	/* When called from sync_state, only process clocks for this device. */
+	if (dev && core->dev != dev)
+		return true;
+
+	return false;
+}
+
+static void clk_unprepare_unused_subtree(struct clk_core *core,
+					 struct device *dev)
 {
 	struct clk_core *child;
 
 	lockdep_assert_held(&prepare_lock);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		clk_unprepare_unused_subtree(child);
+		clk_unprepare_unused_subtree(child, dev);
+
+	if (clk_core_skip_unused(core, dev))
+		return;
 
 	if (core->prepare_count)
 		return;
@@ -1467,7 +1492,8 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	}
 }
 
-static void __init clk_disable_unused_subtree(struct clk_core *core)
+static void clk_disable_unused_subtree(struct clk_core *core,
+				       struct device *dev)
 {
 	struct clk_core *child;
 	unsigned long flags;
@@ -1475,7 +1501,10 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 	lockdep_assert_held(&prepare_lock);
 
 	hlist_for_each_entry(child, &core->children, child_node)
-		clk_disable_unused_subtree(child);
+		clk_disable_unused_subtree(child, dev);
+
+	if (clk_core_skip_unused(core, dev))
+		return;
 
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
 		clk_core_prepare_enable(core->parent);
@@ -1508,7 +1537,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 		clk_core_disable_unprepare(core->parent);
 }
 
-static bool clk_ignore_unused __initdata;
+static bool clk_ignore_unused;
 static int __init clk_ignore_unused_setup(char *__unused)
 {
 	clk_ignore_unused = true;
@@ -1516,7 +1545,7 @@ static int __init clk_ignore_unused_setup(char *__unused)
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
 
-static int __init clk_disable_unused(void)
+static int __clk_disable_unused(struct device *dev)
 {
 	struct clk_core *core;
 	int ret;
@@ -1526,7 +1555,10 @@ static int __init clk_disable_unused(void)
 		return 0;
 	}
 
-	pr_info("clk: Disabling unused clocks\n");
+	if (dev)
+		dev_info(dev, "clk: Disabling unused clocks\n");
+	else
+		pr_info("clk: Disabling unused clocks not associated with a device\n");
 
 	ret = clk_pm_runtime_get_all();
 	if (ret)
@@ -1538,16 +1570,16 @@ static int __init clk_disable_unused(void)
 	clk_prepare_lock();
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, dev);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, dev);
 
 	clk_prepare_unlock();
 
@@ -1555,11 +1587,16 @@ static int __init clk_disable_unused(void)
 
 	return 0;
 }
+
+static int __init clk_disable_unused(void)
+{
+	return __clk_disable_unused(NULL);
+}
 late_initcall_sync(clk_disable_unused);
 
 void clk_sync_state(struct device *dev)
 {
-	/* Will fill in */
+	__clk_disable_unused(dev);
 }
 EXPORT_SYMBOL_GPL(clk_sync_state);
 
@@ -4345,8 +4382,16 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	core->dev = dev;
 	clk_pm_runtime_init(core);
 	core->of_node = np;
-	if (dev && dev->driver)
+	if (dev && dev->driver) {
 		core->owner = dev->driver->owner;
+
+		/*
+		 * If a clk provider sets their own sync_state, then it needs to
+		 * also call clk_sync_state(). dev_set_drv_sync_state() won't
+		 * overwrite the sync_state callback.
+		 */
+		dev_set_drv_sync_state(dev, clk_sync_state);
+	}
 	core->hw = hw;
 	core->flags = init->flags;
 	core->num_parents = init->num_parents;

-- 
2.54.0

Re: [PATCH 3/4] clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()

[Jens Glathe]

On 03.06.26 16:21, Brian Masney wrote:
> Convert all of the qcom clk drivers from icc_sync_state() to
> qcom_cc_sync_state().
>
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
[...]
>   
> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> index 0b40ed601f9a..0d920bd5a992 100644
> --- a/drivers/clk/qcom/clk-cbf-8996.c
> +++ b/drivers/clk/qcom/clk-cbf-8996.c
> @@ -6,7 +6,6 @@
>   #include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/interconnect-clk.h>
> -#include <linux/interconnect-provider.h>
>   #include <linux/of.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> @@ -249,7 +248,7 @@ static void qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
>   
>
Hi Brian, thank you for the patch set. To successfully build there seems 
to be a header missing. I tested this in my tree on Thinkbook 16 G7 QOY 
and Ideapad Slim3x 15Q8X10 without adverse effects, looking good.

jglathe@tb16-jg:~$ sudo dmesg|grep "unused clocks"
[    0.492547] clk: Disabling unused clocks not associated with a device
[    1.178818] clk-rpmh 17500000.rsc:clock-controller: clk: Disabling 
unused clocks
[    1.314479] qcom-edp-phy aec5a00.phy: clk: Disabling unused clocks
[    1.327608] tcsrcc-x1e80100 1fc0000.clock-controller: clk: Disabling 
unused clocks
[    1.544869] qcom-qmp-combo-phy fdf000.phy: clk: Disabling unused clocks
[    1.614767] qcom-qmp-usb-phy 88e5000.phy: clk: Disabling unused clocks
[    1.616222] qcom-qmp-usb-phy 88e3000.phy: clk: Disabling unused clocks
[    1.690273] qcom-qmp-combo-phy fd5000.phy: clk: Disabling unused clocks
[    1.726354] qcom-qmp-pcie-phy 1bfc000.phy: clk: Disabling unused clocks
[   10.956342] qcom-qmp-pcie-phy 1c0e000.phy: clk: Disabling unused clocks
[   15.858926] q6prm-lpass-clock 
6800000.remoteproc:glink-edge:gpr:service@2:clock-controller: clk: 
Disabling unused clocks
[   15.867120] rx_macro 6ac0000.codec: clk: Disabling unused clocks
[   15.867990] wsa_macro 6aa0000.codec: clk: Disabling unused clocks
[   15.868589] va_macro 6d44000.codec: clk: Disabling unused clocks
[   15.970465] wsa_macro 6b00000.codec: clk: Disabling unused clocks
[   15.973614] tx_macro 6ae0000.codec: clk: Disabling unused clocks

Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

with best regards,

Jens

---
 From ac259285bc626800d5f1216bd5b276ada99295bd Mon Sep 17 00:00:00 2001
From: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
Date: Thu, 4 Jun 2026 22:03:20 +0200
Subject: [PATCH] clk: qcom: clk-cbf-8996: Include common.h for
  qcom_cc_sync_state

After switching to qcom_cc_sync_state in the clk sync_state series,
clk-cbf-8996.c was missing the declaration. Fix build error by
including common.h.

Fixes: 20260603-clk-sync-state series

Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
---
  drivers/clk/qcom/clk-cbf-8996.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/clk/qcom/clk-cbf-8996.c 
b/drivers/clk/qcom/clk-cbf-8996.c
index 0d920bd5a9929..6abc70ebf6f19 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -10,6 +10,7 @@
  #include <linux/module.h>
  #include <linux/platform_device.h>
  #include <linux/regmap.h>
+#include "common.h"

  #include <dt-bindings/interconnect/qcom,msm8996-cbf.h>

-- 

2.53.0

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Dmitry Baryshkov]

On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> Several qcom clk providers currently have a sync_state helper set to
> icc_sync_state(). With an upcoming change to the clk framework, if
> sync_state is not defined for the device, then the clk framework sets it
> to clk_sync_state().
> 
> Clk providers that require their own sync_state will need to call the
> framework level clk_sync_state(). Let's introduce a new common helper
> qcom_cc_sync_state() that calls icc_sync_state() and clk_sync_state().
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/qcom/common.c | 9 +++++++++
>  drivers/clk/qcom/common.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index eec369d2173b..31382c49c948 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -3,12 +3,14 @@
>   * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
>  #include <linux/interconnect-clk.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset-controller.h>
>  #include <linux/of.h>
> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>  
> +void qcom_cc_sync_state(struct device *dev)
> +{
> +	icc_sync_state(dev);

Only if desc->icc_hws != 0, otherwise it will mess the interconnect
internals. You might need to set drvdata to desc.

> +	clk_sync_state(dev);
> +}
> +EXPORT_SYMBOL_GPL(qcom_cc_sync_state);
> +
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("QTI Common Clock module");
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 6f2406f8839e..ee448163b1fc 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -88,5 +88,6 @@ extern int qcom_cc_probe(struct platform_device *pdev,
>  			 const struct qcom_cc_desc *desc);
>  extern int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>  				  const struct qcom_cc_desc *desc);
> +extern void qcom_cc_sync_state(struct device *dev);
>  
>  #endif
> 
> -- 
> 2.54.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()

[Dmitry Baryshkov]

On Wed, Jun 03, 2026 at 10:21:48AM -0400, Brian Masney wrote:
> Convert all of the qcom clk drivers from icc_sync_state() to
> qcom_cc_sync_state().
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/qcom/apss-ipq5424.c  | 3 +--
>  drivers/clk/qcom/clk-cbf-8996.c  | 5 ++---
>  drivers/clk/qcom/gcc-ipq5332.c   | 3 +--
>  drivers/clk/qcom/gcc-ipq5424.c   | 3 +--
>  drivers/clk/qcom/gcc-ipq9574.c   | 3 +--
>  drivers/clk/qcom/nsscc-ipq5424.c | 3 +--
>  drivers/clk/qcom/nsscc-ipq9574.c | 3 +--
>  7 files changed, 8 insertions(+), 15 deletions(-)
> 
> @@ -249,7 +248,7 @@ static void qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
>  
>  	icc_clk_unregister(provider);
>  }
> -#define qcom_msm8996_cbf_icc_sync_state icc_sync_state
> +#define qcom_msm8996_cbf_icc_sync_state qcom_cc_sync_state

This patch will need to be reworked after adding a check to icc_hws. For
example, the CBF driver would need to call both functions manually. It's
not using the qcom_cc_really_probe() anyway, so it shouldn't use
qcom_cc_sync_state().

>  #else
>  static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev,  struct clk_hw *cbf_hw)
>  {
> @@ -258,7 +257,7 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev,  struct c
>  	return 0;
>  }
>  #define qcom_msm8996_cbf_icc_remove(pdev) { }
> -#define qcom_msm8996_cbf_icc_sync_state NULL
> +#define qcom_msm8996_cbf_icc_sync_state clk_sync_state
>  #endif
>  
>  static int qcom_msm8996_cbf_probe(struct platform_device *pdev)

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Val Packett]

On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
>> Several qcom clk providers currently have a sync_state helper set to
>> icc_sync_state(). With an upcoming change to the clk framework, if
>> sync_state is not defined for the device, then the clk framework sets it
>> to clk_sync_state().
>> [..]
>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>>   
>> +void qcom_cc_sync_state(struct device *dev)
>> +{
>> +	icc_sync_state(dev);
> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> internals. You might need to set drvdata to desc.

Hmm…

Currently icc_sync_state does not seem to use the dev argument at all.

How would something get messed up, now or whenever icc_sync_state 
changes? o.0


Thanks,
~val

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Dmitry Baryshkov]

On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
> 
> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> > On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> > > Several qcom clk providers currently have a sync_state helper set to
> > > icc_sync_state(). With an upcoming change to the clk framework, if
> > > sync_state is not defined for the device, then the clk framework sets it
> > > to clk_sync_state().
> > > [..]
> > > @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> > >   }
> > >   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> > > +void qcom_cc_sync_state(struct device *dev)
> > > +{
> > > +	icc_sync_state(dev);
> > Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> > internals. You might need to set drvdata to desc.
> 
> Hmm…
> 
> Currently icc_sync_state does not seem to use the dev argument at all.
> 
> How would something get messed up, now or whenever icc_sync_state changes?
> o.0

Yes :-(

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Konrad Dybcio]

On 6/3/26 4:21 PM, Brian Masney wrote:
> Several qcom clk providers currently have a sync_state helper set to
> icc_sync_state(). With an upcoming change to the clk framework, if
> sync_state is not defined for the device, then the clk framework sets it
> to clk_sync_state().
> 
> Clk providers that require their own sync_state will need to call the
> framework level clk_sync_state(). Let's introduce a new common helper
> qcom_cc_sync_state() that calls icc_sync_state() and clk_sync_state().
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/qcom/common.c | 9 +++++++++
>  drivers/clk/qcom/common.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index eec369d2173b..31382c49c948 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -3,12 +3,14 @@
>   * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
>  #include <linux/interconnect-clk.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset-controller.h>
>  #include <linux/of.h>
> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>  
> +void qcom_cc_sync_state(struct device *dev)
> +{
> +	icc_sync_state(dev);
> +	clk_sync_state(dev);
> +}

Do we expect more things to appear here? Currently it's only the qcom
clock controllers that use icc_sync_state, but I wouldn't be surprised
if that list grew

OTOH, as Dmitry suggested, we may need to check some of the
driver-specific data..

Konrad

Re: [PATCH 3/4] clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()

[Brian Masney]

Hi Jens,

On Sat, Jun 06, 2026 at 08:25:45AM +0200, Jens Glathe wrote:
> On 03.06.26 16:21, Brian Masney wrote:
> > Convert all of the qcom clk drivers from icc_sync_state() to
> > qcom_cc_sync_state().
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> [...]
> > diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> > index 0b40ed601f9a..0d920bd5a992 100644
> > --- a/drivers/clk/qcom/clk-cbf-8996.c
> > +++ b/drivers/clk/qcom/clk-cbf-8996.c
> > @@ -6,7 +6,6 @@
> >   #include <linux/clk.h>
> >   #include <linux/clk-provider.h>
> >   #include <linux/interconnect-clk.h>
> > -#include <linux/interconnect-provider.h>
> >   #include <linux/of.h>
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> > @@ -249,7 +248,7 @@ static void qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
> > 
> Hi Brian, thank you for the patch set. To successfully build there seems to
> be a header missing. I tested this in my tree on Thinkbook 16 G7 QOY and
> Ideapad Slim3x 15Q8X10 without adverse effects, looking good.
> 
> jglathe@tb16-jg:~$ sudo dmesg|grep "unused clocks"
> [    0.492547] clk: Disabling unused clocks not associated with a device
> [    1.178818] clk-rpmh 17500000.rsc:clock-controller: clk: Disabling unused
> clocks
> [    1.314479] qcom-edp-phy aec5a00.phy: clk: Disabling unused clocks
> [    1.327608] tcsrcc-x1e80100 1fc0000.clock-controller: clk: Disabling
> unused clocks
> [    1.544869] qcom-qmp-combo-phy fdf000.phy: clk: Disabling unused clocks
> [    1.614767] qcom-qmp-usb-phy 88e5000.phy: clk: Disabling unused clocks
> [    1.616222] qcom-qmp-usb-phy 88e3000.phy: clk: Disabling unused clocks
> [    1.690273] qcom-qmp-combo-phy fd5000.phy: clk: Disabling unused clocks
> [    1.726354] qcom-qmp-pcie-phy 1bfc000.phy: clk: Disabling unused clocks
> [   10.956342] qcom-qmp-pcie-phy 1c0e000.phy: clk: Disabling unused clocks
> [   15.858926] q6prm-lpass-clock
> 6800000.remoteproc:glink-edge:gpr:service@2:clock-controller: clk: Disabling
> unused clocks
> [   15.867120] rx_macro 6ac0000.codec: clk: Disabling unused clocks
> [   15.867990] wsa_macro 6aa0000.codec: clk: Disabling unused clocks
> [   15.868589] va_macro 6d44000.codec: clk: Disabling unused clocks
> [   15.970465] wsa_macro 6b00000.codec: clk: Disabling unused clocks
> [   15.973614] tx_macro 6ae0000.codec: clk: Disabling unused clocks
> 
> Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

Thanks for testing. I'm going to put the Tested-by on patch 4 that actually
adds the sync_state support when I post the new version unless I hear
from you otherwise.

Brian

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney]

Hi Dmitry,

On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
> > 
> > On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> > > On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> > > > Several qcom clk providers currently have a sync_state helper set to
> > > > icc_sync_state(). With an upcoming change to the clk framework, if
> > > > sync_state is not defined for the device, then the clk framework sets it
> > > > to clk_sync_state().
> > > > [..]
> > > > @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> > > > +void qcom_cc_sync_state(struct device *dev)
> > > > +{
> > > > +	icc_sync_state(dev);
> > > Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> > > internals. You might need to set drvdata to desc.
> > 
> > Hmm…
> > 
> > Currently icc_sync_state does not seem to use the dev argument at all.
> > 
> > How would something get messed up, now or whenever icc_sync_state changes?
> > o.0
> 
> Yes :-(

Sorry about the delayed response since I was out of town all last week.
Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
existed prior to my change, and I should label it as such with a Fixes
tag when I post my next version?

Brian

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Konrad Dybcio]

On 6/15/26 4:24 PM, Brian Masney wrote:
> Hi Dmitry,
> 
> On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
>> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
>>>
>>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
>>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
>>>>> Several qcom clk providers currently have a sync_state helper set to
>>>>> icc_sync_state(). With an upcoming change to the clk framework, if
>>>>> sync_state is not defined for the device, then the clk framework sets it
>>>>> to clk_sync_state().
>>>>> [..]
>>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>>>>> +void qcom_cc_sync_state(struct device *dev)
>>>>> +{
>>>>> +	icc_sync_state(dev);
>>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
>>>> internals. You might need to set drvdata to desc.
>>>
>>> Hmm…
>>>
>>> Currently icc_sync_state does not seem to use the dev argument at all.
>>>
>>> How would something get messed up, now or whenever icc_sync_state changes?
>>> o.0
>>
>> Yes :-(
> 
> Sorry about the delayed response since I was out of town all last week.
> Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
> existed prior to my change, and I should label it as such with a Fixes
> tag when I post my next version?

Up until this change, having icc_hws > 0 but lacking icc_sync_state
(or the reverse) would be be considered programmer error

Starting with patch 4, this gets assigned unconditionally, so there's
no prior bug to be fixed

Konrad

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney]

Hi Konrad / Dmitry,

On Mon, Jun 15, 2026 at 04:33:17PM +0200, Konrad Dybcio wrote:
> On 6/15/26 4:24 PM, Brian Masney wrote:
> > On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
> >> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
> >>>
> >>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> >>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> >>>>> Several qcom clk providers currently have a sync_state helper set to
> >>>>> icc_sync_state(). With an upcoming change to the clk framework, if
> >>>>> sync_state is not defined for the device, then the clk framework sets it
> >>>>> to clk_sync_state().
> >>>>> [..]
> >>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> >>>>>   }
> >>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> >>>>> +void qcom_cc_sync_state(struct device *dev)
> >>>>> +{
> >>>>> +	icc_sync_state(dev);
> >>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> >>>> internals. You might need to set drvdata to desc.
> >>>
> >>> Hmm…
> >>>
> >>> Currently icc_sync_state does not seem to use the dev argument at all.
> >>>
> >>> How would something get messed up, now or whenever icc_sync_state changes?
> >>> o.0
> >>
> >> Yes :-(
> > 
> > Sorry about the delayed response since I was out of town all last week.
> > Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
> > existed prior to my change, and I should label it as such with a Fixes
> > tag when I post my next version?
> 
> Up until this change, having icc_hws > 0 but lacking icc_sync_state
> (or the reverse) would be be considered programmer error

icc_hws > 0 but lacking icc_sync_state (or the reverse) makes sense as a
programmer error. However...

> Starting with patch 4, this gets assigned unconditionally, so there's
> no prior bug to be fixed

I don't see where that situation happens here. All of the places where
icc_sync_state() was previously called, the new code now calls
qcom_cc_sync_state() -> icc_sync_state(). (There is
qcom_msm8996_cbf_icc_sync_state() that needs to be modified.)

In patch 4 of this series, it sets up a framework level sync_state()
callback with dev_set_drv_sync_state(). If a sync_state already exists,
then that call will fail with -EBUSY, and it will leave the existing
sync_state() intact. So it's not calling sync_state twice. I will
clarify that on the comment.

Brian

Re: [PATCH 3/4] clk: qcom: convert from icc_sync_state() to qcom_cc_sync_state()

[Jens Glathe]

Hi Brian,

On 6/15/26 16:22, Brian Masney wrote:
> Thanks for testing. I'm going to put the Tested-by on patch 4 that actually
> adds the sync_state support when I post the new version unless I hear
> from you otherwise.

I would say the Tested-by should be on the whole patch series as its 
running here.

with best regards

Jens

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Konrad Dybcio]

On 6/15/26 4:48 PM, Brian Masney wrote:
> Hi Konrad / Dmitry,
> 
> On Mon, Jun 15, 2026 at 04:33:17PM +0200, Konrad Dybcio wrote:
>> On 6/15/26 4:24 PM, Brian Masney wrote:
>>> On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
>>>> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
>>>>>
>>>>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
>>>>>>> Several qcom clk providers currently have a sync_state helper set to
>>>>>>> icc_sync_state(). With an upcoming change to the clk framework, if
>>>>>>> sync_state is not defined for the device, then the clk framework sets it
>>>>>>> to clk_sync_state().
>>>>>>> [..]
>>>>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
>>>>>>> +void qcom_cc_sync_state(struct device *dev)
>>>>>>> +{
>>>>>>> +	icc_sync_state(dev);
>>>>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
>>>>>> internals. You might need to set drvdata to desc.
>>>>>
>>>>> Hmm…
>>>>>
>>>>> Currently icc_sync_state does not seem to use the dev argument at all.
>>>>>
>>>>> How would something get messed up, now or whenever icc_sync_state changes?
>>>>> o.0
>>>>
>>>> Yes :-(
>>>
>>> Sorry about the delayed response since I was out of town all last week.
>>> Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
>>> existed prior to my change, and I should label it as such with a Fixes
>>> tag when I post my next version?
>>
>> Up until this change, having icc_hws > 0 but lacking icc_sync_state
>> (or the reverse) would be be considered programmer error
> 
> icc_hws > 0 but lacking icc_sync_state (or the reverse) makes sense as a
> programmer error. However...
> 
>> Starting with patch 4, this gets assigned unconditionally, so there's
>> no prior bug to be fixed
> 
> I don't see where that situation happens here. All of the places where
> icc_sync_state() was previously called, the new code now calls
> qcom_cc_sync_state() -> icc_sync_state(). (There is
> qcom_msm8996_cbf_icc_sync_state() that needs to be modified.)
> 
> In patch 4 of this series, it sets up a framework level sync_state()
> callback with dev_set_drv_sync_state(). If a sync_state already exists,
> then that call will fail with -EBUSY, and it will leave the existing
> sync_state() intact. So it's not calling sync_state twice. I will
> clarify that on the comment.

Dmitry and I are referring to the situation where the clock driver isn't
an interconnect provider but icc_sync_state() still executes. That could
not have been the case before, since most clock drivers didn't come with
any sort of .sync_state()

Konrad

Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney]

Hi Konrad / Dmitry,

On Mon, Jun 15, 2026 at 04:51:35PM +0200, Konrad Dybcio wrote:
> On 6/15/26 4:48 PM, Brian Masney wrote:
> > On Mon, Jun 15, 2026 at 04:33:17PM +0200, Konrad Dybcio wrote:
> >> On 6/15/26 4:24 PM, Brian Masney wrote:
> >>> On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
> >>>> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
> >>>>>
> >>>>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> >>>>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> >>>>>>> Several qcom clk providers currently have a sync_state helper set to
> >>>>>>> icc_sync_state(). With an upcoming change to the clk framework, if
> >>>>>>> sync_state is not defined for the device, then the clk framework sets it
> >>>>>>> to clk_sync_state().
> >>>>>>> [..]
> >>>>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> >>>>>>>   }
> >>>>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> >>>>>>> +void qcom_cc_sync_state(struct device *dev)
> >>>>>>> +{
> >>>>>>> +	icc_sync_state(dev);
> >>>>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> >>>>>> internals. You might need to set drvdata to desc.
> >>>>>
> >>>>> Hmm…
> >>>>>
> >>>>> Currently icc_sync_state does not seem to use the dev argument at all.
> >>>>>
> >>>>> How would something get messed up, now or whenever icc_sync_state changes?
> >>>>> o.0
> >>>>
> >>>> Yes :-(
> >>>
> >>> Sorry about the delayed response since I was out of town all last week.
> >>> Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
> >>> existed prior to my change, and I should label it as such with a Fixes
> >>> tag when I post my next version?
> >>
> >> Up until this change, having icc_hws > 0 but lacking icc_sync_state
> >> (or the reverse) would be be considered programmer error
> > 
> > icc_hws > 0 but lacking icc_sync_state (or the reverse) makes sense as a
> > programmer error. However...
> > 
> >> Starting with patch 4, this gets assigned unconditionally, so there's
> >> no prior bug to be fixed
> > 
> > I don't see where that situation happens here. All of the places where
> > icc_sync_state() was previously called, the new code now calls
> > qcom_cc_sync_state() -> icc_sync_state(). (There is
> > qcom_msm8996_cbf_icc_sync_state() that needs to be modified.)
> > 
> > In patch 4 of this series, it sets up a framework level sync_state()
> > callback with dev_set_drv_sync_state(). If a sync_state already exists,
> > then that call will fail with -EBUSY, and it will leave the existing
> > sync_state() intact. So it's not calling sync_state twice. I will
> > clarify that on the comment.
> 
> Dmitry and I are referring to the situation where the clock driver isn't
> an interconnect provider but icc_sync_state() still executes. That could
> not have been the case before, since most clock drivers didn't come with
> any sort of .sync_state()

I'm sorry if I am being really dense here.

Let's ignore clk-cbf-8996.c since that has a separate issue. The other 6
drivers in this patch today all have this pattern:

static struct platform_driver foo_driver = {
	...
        .driver = {
		...
                .sync_state = icc_sync_state,
        },
};

I'm changing it to call qcom_cc_sync_state(), which will call
icc_sync_state(). So everywhere where icc_sync_state() is called today
it will still be called after the series is applied.

All of the other clk drivers will just call clk_sync_state() directly
that's set at the framework level.

Brian