[PATCH 1/3] clk: qcom: lcc-msm8960: check regmap_read/regmap_write return values in probe

[PATCH 1/3] clk: qcom: lcc-msm8960: check regmap_read/regmap_write return values in probe

[Herman van Hazendonk]

The PLL4 L-register read in probe was used to select between the 393 MHz
and 492 MHz frequency plans without checking whether the underlying
regmap operation succeeded; a silent failure would leave the rcg
structures pointing at whatever default they had at startup (the 393
MHz plan) and the chosen plan could be wrong for the running PLL,
producing incorrect audio clock rates without any diagnostic.

The unconditional write to register 0xc4 that arms the LPASS Primary
PLL mux on PLL4 had the same problem: a bus-level failure would leave
the mux at its default (PXO) and every downstream LCC clock would be
sourced from the wrong parent without a warning.

Use dev_err_probe() in both spots so the error is surfaced (and the
deferred-probe state machine handles the EPROBE_DEFER-from-bus-arbiter
case correctly) and the driver does not bind in a known-bad
configuration.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/lcc-msm8960.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/lcc-msm8960.c b/drivers/clk/qcom/lcc-msm8960.c
index 7cba2ce3e408..44321c01f957 100644
--- a/drivers/clk/qcom/lcc-msm8960.c
+++ b/drivers/clk/qcom/lcc-msm8960.c
@@ -453,6 +453,7 @@ MODULE_DEVICE_TABLE(of, lcc_msm8960_match_table);
 static int lcc_msm8960_probe(struct platform_device *pdev)
 {
 	u32 val;
+	int ret;
 	struct regmap *regmap;
 
 	/* patch for the cxo <-> pxo difference */
@@ -468,7 +469,10 @@ static int lcc_msm8960_probe(struct platform_device *pdev)
 		return PTR_ERR(regmap);
 
 	/* Use the correct frequency plan depending on speed of PLL4 */
-	regmap_read(regmap, 0x4, &val);
+	ret = regmap_read(regmap, 0x4, &val);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to read PLL4 L register\n");
 	if (val == 0x12) {
 		slimbus_src.freq_tbl = clk_tbl_aif_osr_492;
 		mi2s_osr_src.freq_tbl = clk_tbl_aif_osr_492;
@@ -479,7 +483,10 @@ static int lcc_msm8960_probe(struct platform_device *pdev)
 		pcm_src.freq_tbl = clk_tbl_pcm_492;
 	}
 	/* Enable PLL4 source on the LPASS Primary PLL Mux */
-	regmap_write(regmap, 0xc4, 0x1);
+	ret = regmap_write(regmap, 0xc4, 0x1);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to select PLL4 on LPASS Primary PLL Mux\n");
 
 	return qcom_cc_really_probe(&pdev->dev, &lcc_msm8960_desc, regmap);
 }
-- 
2.43.0

Re: [PATCH 1/3] clk: qcom: lcc-msm8960: check regmap_read/regmap_write return values in probe

[Konrad Dybcio]

On 6/2/26 6:49 AM, Herman van Hazendonk wrote:
> The PLL4 L-register read in probe was used to select between the 393 MHz
> and 492 MHz frequency plans without checking whether the underlying
> regmap operation succeeded; a silent failure would leave the rcg
> structures pointing at whatever default they had at startup (the 393
> MHz plan) and the chosen plan could be wrong for the running PLL,
> producing incorrect audio clock rates without any diagnostic.
> 
> The unconditional write to register 0xc4 that arms the LPASS Primary
> PLL mux on PLL4 had the same problem: a bus-level failure would leave
> the mux at its default (PXO) and every downstream LCC clock would be
> sourced from the wrong parent without a warning.
> 
> Use dev_err_probe() in both spots so the error is surfaced (and the
> deferred-probe state machine handles the EPROBE_DEFER-from-bus-arbiter
> case correctly) and the driver does not bind in a known-bad
> configuration.
> 
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
> ---

Can this happen in practice?

LCC seems to be a device on the MMIO bus..

Konrad

[PATCH 2/3] clk: qcom: lcc-msm8960: serialise probe and block sysfs rebind

[Herman van Hazendonk]

The probe path mutates file-static clk_rcg structures:

  - The .freq_tbl pointer on slimbus_src, mi2s_osr_src, the four
    codec_i2s_*/spare_i2s_* osr_src entries, and pcm_src is selected
    from the PLL4 L-register read.

  - pxo_parent_data.fw_name/.name and lcc_pxo_pll4[0].fw_name/.name
    are switched to cxo for the MDM9615 compatible.

These are all file-static, and the registered clk_rcg objects are
referenced by qcom_cc_really_probe() through the same pointers. Two
hazards follow from that:

  1. Concurrent probe (a hypothetical second LCC instance, or sysfs
     bind racing the first probe) would race both the freq_tbl
     assignment and qcom_cc_really_probe()'s registration, leaving
     the clock tree in an inconsistent state with no diagnostic.

  2. Sysfs unbind + rebind would either race the previous instance's
     devm cleanup of those clocks (devm teardown runs after .remove
     returns, so concurrent probe-side mutation is observable), or,
     for a MDM9615 -> MSM8960 sequence, leave pxo_parent_data stuck
     on cxo for the new bind because remove cannot easily undo the
     compatible-specific patch.

Address both with a static bool guarded by a mutex (check-and-claim
atomically inside the locked scope so two concurrent probes cannot
both see the flag clear) and .suppress_bind_attrs = true on the
platform_driver. Sysfs bind/unbind is blocked entirely; module
unload is the only tear-down path and is serialised by the module
mutex, so no .remove callback is needed.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/lcc-msm8960.c | 93 +++++++++++++++++++++++++++++++---
 1 file changed, 85 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/qcom/lcc-msm8960.c b/drivers/clk/qcom/lcc-msm8960.c
index 44321c01f957..1e0873ce1cc2 100644
--- a/drivers/clk/qcom/lcc-msm8960.c
+++ b/drivers/clk/qcom/lcc-msm8960.c
@@ -6,6 +6,7 @@
 #include <linux/kernel.h>
 #include <linux/bitops.h>
 #include <linux/err.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -450,12 +451,38 @@ static const struct of_device_id lcc_msm8960_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, lcc_msm8960_match_table);
 
+/*
+ * The probe path mutates file-static clk_rcg structures (.freq_tbl
+ * pointers selected from the PLL4 L-register, and pxo_parent_data /
+ * lcc_pxo_pll4 for the MDM9615 cxo override). All clk_rcg objects in
+ * this file are registered by pointer through qcom_cc_really_probe(),
+ * so a second concurrent probe of the same driver would race both that
+ * registration and the freq_tbl assignment. Enforce single-instance
+ * binding with a static flag protected by an internal mutex.
+ */
+static bool lcc_msm8960_bound;
+static DEFINE_MUTEX(lcc_msm8960_bound_lock);
+
 static int lcc_msm8960_probe(struct platform_device *pdev)
 {
 	u32 val;
 	int ret;
 	struct regmap *regmap;
 
+	mutex_lock(&lcc_msm8960_bound_lock);
+	if (lcc_msm8960_bound) {
+		mutex_unlock(&lcc_msm8960_bound_lock);
+		return dev_err_probe(&pdev->dev, -EBUSY,
+			"only a single LCC instance is supported\n");
+	}
+	/*
+	 * Claim ownership inside the same locked region as the check
+	 * so a second concurrent probe cannot pass the check before
+	 * we set the flag.
+	 */
+	lcc_msm8960_bound = true;
+	mutex_unlock(&lcc_msm8960_bound_lock);
+
 	/* patch for the cxo <-> pxo difference */
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,lcc-mdm9615")) {
 		pxo_parent_data.fw_name = "cxo";
@@ -465,14 +492,18 @@ static int lcc_msm8960_probe(struct platform_device *pdev)
 	}
 
 	regmap = qcom_cc_map(pdev, &lcc_msm8960_desc);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto err_unclaim;
+	}
 
 	/* Use the correct frequency plan depending on speed of PLL4 */
 	ret = regmap_read(regmap, 0x4, &val);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "failed to read PLL4 L register\n");
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			      "failed to read PLL4 L register\n");
+		goto err_unclaim;
+	}
 	if (val == 0x12) {
 		slimbus_src.freq_tbl = clk_tbl_aif_osr_492;
 		mi2s_osr_src.freq_tbl = clk_tbl_aif_osr_492;
@@ -484,18 +515,64 @@ static int lcc_msm8960_probe(struct platform_device *pdev)
 	}
 	/* Enable PLL4 source on the LPASS Primary PLL Mux */
 	ret = regmap_write(regmap, 0xc4, 0x1);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			      "failed to select PLL4 on LPASS Primary PLL Mux\n");
+		goto err_unclaim;
+	}
+
+	ret = qcom_cc_really_probe(&pdev->dev, &lcc_msm8960_desc, regmap);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "failed to select PLL4 on LPASS Primary PLL Mux\n");
+		goto err_unclaim;
+
+	return 0;
+
+err_unclaim:
+	mutex_lock(&lcc_msm8960_bound_lock);
+	lcc_msm8960_bound = false;
+	mutex_unlock(&lcc_msm8960_bound_lock);
+	return ret;
+}
 
-	return qcom_cc_really_probe(&pdev->dev, &lcc_msm8960_desc, regmap);
+/*
+ * Clear the singleton bound flag so a future probe can succeed.
+ *
+ * .suppress_bind_attrs = true below blocks the sysfs bind/unbind path,
+ * which is the rebind race window we care about. However, the driver
+ * core can still call .remove via device removal (DT overlay removal,
+ * hot-unplug). Without clearing the flag here, the next probe of a
+ * fresh instance would fail with -EBUSY. The probe and remove on the
+ * same device are serialised by device_lock so this clear does not
+ * race the devm cleanup of the previous instance.
+ */
+static void lcc_msm8960_remove(struct platform_device *pdev)
+{
+	mutex_lock(&lcc_msm8960_bound_lock);
+	lcc_msm8960_bound = false;
+	mutex_unlock(&lcc_msm8960_bound_lock);
 }
 
 static struct platform_driver lcc_msm8960_driver = {
 	.probe		= lcc_msm8960_probe,
+	.remove		= lcc_msm8960_remove,
 	.driver		= {
 		.name	= "lcc-msm8960",
 		.of_match_table = lcc_msm8960_match_table,
+		/*
+		 * The probe path mutates file-static clk_rcg structures
+		 * (.freq_tbl pointers selected from the PLL4 L-register,
+		 * pxo_parent_data switched to cxo for MDM9615) that are
+		 * then registered by pointer through qcom_cc_really_probe().
+		 * A sysfs unbind + rebind would either race the previous
+		 * instance's devm teardown of those clocks or silently
+		 * leave pxo_parent_data stuck on its previous compatible's
+		 * setting (e.g. MDM9615 -> MSM8960 stuck on cxo). Block
+		 * sysfs bind / unbind to remove both hazards; non-sysfs
+		 * removal (DT overlay, hot-unplug) is serialised against
+		 * probe by device_lock so .remove just clears the singleton
+		 * flag and lets devm undo the clock registrations.
+		 */
+		.suppress_bind_attrs = true,
 	},
 };
 module_platform_driver(lcc_msm8960_driver);
-- 
2.43.0

[PATCH 3/3] clk: qcom: lcc-msm8960: re-apply PLL4 mux on resume

[Herman van Hazendonk]

The LPASS power domain on the MSM8x60 family is collapsed during
system sleep, which resets the LPASS Primary PLL Mux register at 0xc4
to its hardware default of 0 (PXO). The probe path arms this register
to 0x1 (PLL4) and the rest of the LCC clock tree assumes that
selection; after the first suspend/resume cycle every LCC clock
silently returns sourced from PXO at 27 MHz, with no diagnostic, and
audio produces wrong rates until the next reboot.

Add a resume PM op that re-asserts PLL4 on the mux. The single
register write is idempotent on platforms that do not exhibit the
power-collapse (the mux is already at 0x1 from probe), so it is safe
to run unconditionally for all compatibles. Stash the regmap in
drvdata during probe so resume can write without walking the
clock-provider tree.

dev_err / raw errno are used here (not dev_err_probe) because resume
cannot defer.

Use pm_ptr(), not pm_sleep_ptr(), to assign the dev_pm_ops pointer.
pm_ptr() is the correct macro for conditionally compiling the .pm
struct pointer based on CONFIG_PM; pm_sleep_ptr() is intended for
gating individual function pointers inside the struct on
CONFIG_PM_SLEEP.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/lcc-msm8960.c | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/clk/qcom/lcc-msm8960.c b/drivers/clk/qcom/lcc-msm8960.c
index 1e0873ce1cc2..7bfb396b09e2 100644
--- a/drivers/clk/qcom/lcc-msm8960.c
+++ b/drivers/clk/qcom/lcc-msm8960.c
@@ -10,6 +10,7 @@
 #include <linux/platform_device.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm.h>
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
 
@@ -525,6 +526,13 @@ static int lcc_msm8960_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unclaim;
 
+	/*
+	 * Stash the regmap so lcc_msm8960_resume() can re-apply the
+	 * LPASS Primary PLL mux selection without having to walk the
+	 * clock-provider tree. qcom_cc_really_probe() does not touch
+	 * platform drvdata, so this is safe.
+	 */
+	platform_set_drvdata(pdev, regmap);
 	return 0;
 
 err_unclaim:
@@ -552,12 +560,43 @@ static void lcc_msm8960_remove(struct platform_device *pdev)
 	mutex_unlock(&lcc_msm8960_bound_lock);
 }
 
+/*
+ * The LPASS power domain on at least the MSM8x60 family is collapsed
+ * during system sleep, which resets the LPASS Primary PLL Mux at 0xc4
+ * to its hardware default of 0 (PXO). Without re-asserting PLL4 here
+ * every LCC clock would silently come back sourced from PXO at 27 MHz
+ * and audio would produce wrong rates until the next reboot. The
+ * single-register write is idempotent on platforms that do not exhibit
+ * the collapse (the mux is already at 0x1 from probe), so it is safe
+ * to run unconditionally.
+ *
+ * Resume cannot defer; log with dev_err and propagate the raw errno
+ * rather than dev_err_probe (which silences EPROBE_DEFER).
+ */
+static int lcc_msm8960_resume(struct device *dev)
+{
+	struct regmap *regmap = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regmap_write(regmap, 0xc4, 0x1);
+	if (ret)
+		dev_err(dev,
+			"failed to re-select PLL4 on LPASS Primary PLL Mux on resume: %d\n",
+			ret);
+	return ret;
+}
+
+static const struct dev_pm_ops lcc_msm8960_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(NULL, lcc_msm8960_resume)
+};
+
 static struct platform_driver lcc_msm8960_driver = {
 	.probe		= lcc_msm8960_probe,
 	.remove		= lcc_msm8960_remove,
 	.driver		= {
 		.name	= "lcc-msm8960",
 		.of_match_table = lcc_msm8960_match_table,
+		.pm	= pm_ptr(&lcc_msm8960_pm_ops),
 		/*
 		 * The probe path mutates file-static clk_rcg structures
 		 * (.freq_tbl pointers selected from the PLL4 L-register,
-- 
2.43.0

Re: [PATCH 3/3] clk: qcom: lcc-msm8960: re-apply PLL4 mux on resume

[Konrad Dybcio]

On 6/2/26 6:50 AM, Herman van Hazendonk wrote:
> The LPASS power domain on the MSM8x60 family is collapsed during
> system sleep, which resets the LPASS Primary PLL Mux register at 0xc4
> to its hardware default of 0 (PXO). The probe path arms this register
> to 0x1 (PLL4) and the rest of the LCC clock tree assumes that
> selection; after the first suspend/resume cycle every LCC clock
> silently returns sourced from PXO at 27 MHz, with no diagnostic, and
> audio produces wrong rates until the next reboot.
> 
> Add a resume PM op that re-asserts PLL4 on the mux. The single
> register write is idempotent on platforms that do not exhibit the
> power-collapse (the mux is already at 0x1 from probe), so it is safe
> to run unconditionally for all compatibles. Stash the regmap in
> drvdata during probe so resume can write without walking the
> clock-provider tree.
> 
> dev_err / raw errno are used here (not dev_err_probe) because resume
> cannot defer.
> 
> Use pm_ptr(), not pm_sleep_ptr(), to assign the dev_pm_ops pointer.
> pm_ptr() is the correct macro for conditionally compiling the .pm
> struct pointer based on CONFIG_PM; pm_sleep_ptr() is intended for
> gating individual function pointers inside the struct on
> CONFIG_PM_SLEEP.
> 
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
> ---

This seems like more of a workaround, but the aforementioned PLL seems
to not be registered with Linux.. what drives it? Is there an ADSP on
this platform?

Konrad