[PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control

[PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control

[Jacky Bai]

Add debugfs support for runtime tuning of the audio PLL K divider,
which enables fine-grained frequency adjustments for audio PLL.
This is used for:
  - Audio clock calibration and testing
  - Debugging audio synchronization issues

Two debug interfaces are exported to userspace:
  - delta_k: It is used to adjust the K divider in PLL based on small
    steps
  - pll_parameter: It is used for get PLL's current M-divider,
    P-divider, S-divider & K-divider setting in PLL register

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
Changes in v2:
- remove the examples from commit log.
- refine the comments blocks
- add delta_k read back support
- resolve the comments from Sashiko AI
- add prefix to audio_pll_debug_init API
- Link to v1: https://lore.kernel.org/r/20260512-imx8m_pll_debugfs-v1-1-e1e44b21be90@nxp.com
---
 drivers/clk/imx/clk-imx8mm.c  |   6 +++
 drivers/clk/imx/clk-pll14xx.c | 107 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk.h         |   1 +
 3 files changed, 114 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 319af4deec01c188524807d39dff92bbd08f3601..f080a4dcead359f9494b289ebd277ef2dfdf72cd 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -298,6 +298,7 @@ static struct clk_hw **hws;
 
 static int imx8mm_clocks_probe(struct platform_device *pdev)
 {
+	struct clk_hw *audio_pll_hws[2];
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	void __iomem *base;
@@ -610,6 +611,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 
 	imx_register_uart_clocks();
 
+	/* Add debug interface for audio PLLs */
+	audio_pll_hws[0] = hws[IMX8MM_AUDIO_PLL1];
+	audio_pll_hws[1] = hws[IMX8MM_AUDIO_PLL2];
+	imx_audio_pll_debug_init(audio_pll_hws, ARRAY_SIZE(audio_pll_hws));
+
 	return 0;
 
 unregister_hws:
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 39600ee22be3066683b562aa6f2a8b750d19c4cc..b00529cb196a22ad0444b1efdbc3b53d0e11c20b 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -8,11 +8,13 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/clk-provider.h>
+#include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/jiffies.h>
 
 #include "clk.h"
@@ -40,6 +42,8 @@ struct clk_pll14xx {
 	enum imx_pll14xx_type		type;
 	const struct imx_pll14xx_rate_table *rate_table;
 	int rate_count;
+	s16 delta_k;
+	spinlock_t lock;
 };
 
 #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
@@ -361,6 +365,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 {
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
 	struct imx_pll14xx_rate_table rate;
+	unsigned long flags;
 	u32 gnrl_ctl, div_ctl0;
 	int ret;
 
@@ -374,9 +379,13 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
 		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
 
+		spin_lock_irqsave(&pll->lock, flags);
+
 		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
 			       pll->base + DIV_CTL1);
 
+		spin_unlock_irqrestore(&pll->lock, flags);
+
 		return 0;
 	}
 
@@ -394,8 +403,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
 		   FIELD_PREP(SDIV_MASK, rate.sdiv);
 	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
 
+	spin_lock_irqsave(&pll->lock, flags);
+
 	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
 
+	spin_unlock_irqrestore(&pll->lock, flags);
+
 	/*
 	 * According to SPEC, t3 - t2 need to be greater than
 	 * 1us and 1/FREF, respectively.
@@ -508,6 +521,8 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
+	spin_lock_init(&pll->lock);
+
 	init.name = name;
 	init.flags = pll_clk->flags;
 	init.parent_names = &parent_name;
@@ -551,3 +566,95 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
 	return hw;
 }
 EXPORT_SYMBOL_GPL(imx_dev_clk_hw_pll14xx);
+
+/*
+ * Debugfs interface for Audio PLL runtime monitoring and control
+ *
+ * This interface allows dynamic adjustment of the Audio PLL
+ * K-divider for precise frequency tuning, particularly useful
+ * for audio applications.
+ *
+ * examples for the usage of the two interfaces:
+ *   1): Get the current PLL setting of dividers
+ *     cat /sys/kernel/debug/audio_pll_monitor/audio_pll1/pll_parameter
+ *
+ *   2): Adjust the K-divider by a small delta_k
+ *     echo 1 > /sys/kernel/debug/audio_pll_monitor/audio_pll1/delta_k;
+ */
+#ifdef CONFIG_DEBUG_FS
+static int pll_delta_k_get(void *data, u64 *val)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
+	*val = pll->delta_k;
+	return 0;
+}
+
+static int pll_delta_k_set(void *data, u64 val)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
+	unsigned long flags;
+	u32 div_ctl1;
+	s16 kdiv, delta_k;
+
+	delta_k = (s16)clamp_t(long, val, KDIV_MIN, KDIV_MAX);
+	pll->delta_k = delta_k;
+
+	spin_lock_irqsave(&pll->lock, flags);
+
+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
+	kdiv = (s16)FIELD_GET(KDIV_MASK, div_ctl1);
+	kdiv = (s16)clamp_t(s32, (s32)kdiv + delta_k, KDIV_MIN, KDIV_MAX);
+	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);
+
+	spin_unlock_irqrestore(&pll->lock, flags);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(delta_k_fops, pll_delta_k_get, pll_delta_k_set, "%lld\n");
+
+static int pll_setting_show(struct seq_file *s, void *data)
+{
+	struct clk_pll14xx *pll = to_clk_pll14xx(s->private);
+	u32 div_ctl0, div_ctl1;
+	u32 mdiv, pdiv, sdiv, kdiv;
+
+	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
+
+	mdiv = FIELD_GET(MDIV_MASK, div_ctl0);
+	pdiv = FIELD_GET(PDIV_MASK, div_ctl0);
+	sdiv = FIELD_GET(SDIV_MASK, div_ctl0);
+	kdiv = FIELD_GET(KDIV_MASK, div_ctl1);
+
+	seq_printf(s, "Mdiv: 0x%x; Pdiv: 0x%x; Sdiv: 0x%x; Kdiv: 0x%x\n",
+		   mdiv, pdiv, sdiv, kdiv);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(pll_setting);
+
+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int num_plls)
+{
+	struct dentry *rootdir, *audio_pll_dir;
+	const char *pll_name;
+	int i;
+
+	rootdir = debugfs_create_dir("audio_pll_monitor", NULL);
+
+	for (i = 0; i < num_plls; i++) {
+		if (!IS_ERR_OR_NULL(hws[i])) {
+			pll_name = clk_hw_get_name(hws[i]);
+			audio_pll_dir = debugfs_create_dir(pll_name, rootdir);
+			debugfs_create_file_unsafe("delta_k", 0600, audio_pll_dir,
+					hws[i], &delta_k_fops);
+			debugfs_create_file("pll_parameter", 0444, audio_pll_dir,
+					hws[i], &pll_setting_fops);
+		}
+	}
+}
+#else /* !CONFIG_DEBUG_FS */
+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int num_plls)
+{
+}
+#endif /* CONFIG_DEBUG_FS */
+EXPORT_SYMBOL_GPL(imx_audio_pll_debug_init);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index aa5202f284f3d1b7c1b4bf65e2329831832b43a5..16e0bafa0c9b99cb4eee37a216e0a274d27f11fc 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -487,4 +487,5 @@ struct clk_hw *imx_clk_gpr_mux(const char *name, const char *compatible,
 			       u32 reg, const char **parent_names,
 			       u8 num_parents, const u32 *mux_table, u32 mask);
 
+void imx_audio_pll_debug_init(struct clk_hw **hws, unsigned int num_plls);
 #endif

---
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
change-id: 20260421-imx8m_pll_debugfs-246d0fbbb617

Best regards,
-- 
Jacky Bai <ping.bai@nxp.com>

Re: [PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Data race on `pll->delta_k` assignment outside lock.
- [Medium] Torn reads of PLL state in `pll_setting_show` due to missing locks.
- [Medium] Type truncation in `clamp_t` bypasses upper bounds on 32-bit platforms.
--

commit f3539cad304e6bd44f1d2c105a546db8ccf75dd1
Author: Jacky Bai <ping.bai@nxp.com>

clk: imx: Add audio PLL debugfs for K-divider control

Add debugfs support for runtime tuning of the audio PLL K divider,
enabling fine-grained frequency adjustments for audio PLL. Two debug
interfaces are exported to userspace: delta_k and pll_parameter.

> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
> index 39600ee22be30..b00529cb196a2 100644
> --- a/drivers/clk/imx/clk-pll14xx.c
> +++ b/drivers/clk/imx/clk-pll14xx.c
[ ... ]
> @@ -551,3 +566,95 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
[ ... ]
> +static int pll_delta_k_set(void *data, u64 val)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(data);
> +	unsigned long flags;
> +	u32 div_ctl1;
> +	s16 kdiv, delta_k;
> +
> +	delta_k = (s16)clamp_t(long, val, KDIV_MIN, KDIV_MAX);

Is it possible for this cast to long to cause truncation issues on 32-bit
platforms?

If a user writes a large positive value like 4294967295 (0xFFFFFFFF) to the
debugfs file, wouldn't it be truncated and sign-extended to -1?

Since -1 falls within the valid range, it seems this would bypass the max
bound check and apply an incorrect delta. Does this need to use s64 or long
long for the clamp type?

> +	pll->delta_k = delta_k;
> +
> +	spin_lock_irqsave(&pll->lock, flags);

Could this lockless assignment lead to state inconsistencies?

If two threads concurrently write to delta_k, could their execution interleave
such that both update pll->delta_k locklessly, but the lock is acquired
sequentially?

This might result in the hardware accumulating both changes while pll->delta_k
only reflects the last lockless assignment. Should the update of pll->delta_k
be moved inside the lock?

> +
> +	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> +	kdiv = (s16)FIELD_GET(KDIV_MASK, div_ctl1);
> +	kdiv = (s16)clamp_t(s32, (s32)kdiv + delta_k, KDIV_MIN, KDIV_MAX);
> +	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);
> +
> +	spin_unlock_irqrestore(&pll->lock, flags);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(delta_k_fops, pll_delta_k_get, pll_delta_k_set, "%lld\n");
> +
> +static int pll_setting_show(struct seq_file *s, void *data)
> +{
> +	struct clk_pll14xx *pll = to_clk_pll14xx(s->private);
> +	u32 div_ctl0, div_ctl1;
> +	u32 mdiv, pdiv, sdiv, kdiv;
> +
> +	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> +	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);

Could this sequence result in torn reads if called concurrently with a rate
change?

Looking at clk_pll1443x_set_rate(), the hardware registers are updated
sequentially. DIV_CTL0 is written, followed by a locked write to DIV_CTL1:

    div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
    writel_relaxed(div_ctl0, pll->base + DIV_CTL0);

    spin_lock_irqsave(&pll->lock, flags);
    writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
    spin_unlock_irqrestore(&pll->lock, flags);

If pll_setting_show() runs concurrently without holding pll->lock, could it
interleave between those two writes, reading the updated dividers from
DIV_CTL0 but the stale K divider from DIV_CTL1?

> +
> +	mdiv = FIELD_GET(MDIV_MASK, div_ctl0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-imx8m_pll_debugfs-v2-1-20e1d88526b0@nxp.com?part=1

Re: [PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control

[Peng Fan]

On Tue, May 19, 2026 at 05:19:18PM +0800, Jacky Bai wrote:
>Add debugfs support for runtime tuning of the audio PLL K divider,
>which enables fine-grained frequency adjustments for audio PLL.
>This is used for:
>  - Audio clock calibration and testing
>  - Debugging audio synchronization issues
>
>Two debug interfaces are exported to userspace:
>  - delta_k: It is used to adjust the K divider in PLL based on small
>    steps
>  - pll_parameter: It is used for get PLL's current M-divider,
>    P-divider, S-divider & K-divider setting in PLL register
>
>Signed-off-by: Jacky Bai <ping.bai@nxp.com>
>---
>Changes in v2:
>- remove the examples from commit log.
>- refine the comments blocks
>- add delta_k read back support
>- resolve the comments from Sashiko AI
>- add prefix to audio_pll_debug_init API
>- Link to v1: https://lore.kernel.org/r/20260512-imx8m_pll_debugfs-v1-1-e1e44b21be90@nxp.com
>---
> drivers/clk/imx/clk-imx8mm.c  |   6 +++
> drivers/clk/imx/clk-pll14xx.c | 107 ++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/imx/clk.h         |   1 +
> 3 files changed, 114 insertions(+)
>
>diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
>index 319af4deec01c188524807d39dff92bbd08f3601..f080a4dcead359f9494b289ebd277ef2dfdf72cd 100644
>--- a/drivers/clk/imx/clk-imx8mm.c
>+++ b/drivers/clk/imx/clk-imx8mm.c
>@@ -298,6 +298,7 @@ static struct clk_hw **hws;
> 
> static int imx8mm_clocks_probe(struct platform_device *pdev)
> {
>+	struct clk_hw *audio_pll_hws[2];
> 	struct device *dev = &pdev->dev;
> 	struct device_node *np = dev->of_node;
> 	void __iomem *base;
>@@ -610,6 +611,11 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> 
> 	imx_register_uart_clocks();
> 
>+	/* Add debug interface for audio PLLs */
>+	audio_pll_hws[0] = hws[IMX8MM_AUDIO_PLL1];
>+	audio_pll_hws[1] = hws[IMX8MM_AUDIO_PLL2];
>+	imx_audio_pll_debug_init(audio_pll_hws, ARRAY_SIZE(audio_pll_hws));
>+
> 	return 0;
> 
> unregister_hws:
>diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
>index 39600ee22be3066683b562aa6f2a8b750d19c4cc..b00529cb196a22ad0444b1efdbc3b53d0e11c20b 100644
>--- a/drivers/clk/imx/clk-pll14xx.c
>+++ b/drivers/clk/imx/clk-pll14xx.c
>@@ -8,11 +8,13 @@
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> #include <linux/clk-provider.h>
>+#include <linux/debugfs.h>
> #include <linux/err.h>
> #include <linux/export.h>
> #include <linux/io.h>
> #include <linux/iopoll.h>
> #include <linux/slab.h>
>+#include <linux/spinlock.h>
> #include <linux/jiffies.h>
> 
> #include "clk.h"
>@@ -40,6 +42,8 @@ struct clk_pll14xx {
> 	enum imx_pll14xx_type		type;
> 	const struct imx_pll14xx_rate_table *rate_table;
> 	int rate_count;
>+	s16 delta_k;
>+	spinlock_t lock;
> };
> 
> #define to_clk_pll14xx(_hw) container_of(_hw, struct clk_pll14xx, hw)
>@@ -361,6 +365,7 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
> {
> 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
> 	struct imx_pll14xx_rate_table rate;
>+	unsigned long flags;
> 	u32 gnrl_ctl, div_ctl0;
> 	int ret;
> 
>@@ -374,9 +379,13 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
> 		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
> 		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> 
>+		spin_lock_irqsave(&pll->lock, flags);

Should this lock also protect the setting to DIV_CTL0?

>+
> 		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
> 			       pll->base + DIV_CTL1);
> 
>+		spin_unlock_irqrestore(&pll->lock, flags);
>+
> 		return 0;
> 	}
> 
>@@ -394,8 +403,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw, unsigned long drate,
> 		   FIELD_PREP(SDIV_MASK, rate.sdiv);
> 	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> 
>+	spin_lock_irqsave(&pll->lock, flags);

Ditto.

>+
> 	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base + DIV_CTL1);
> 
>+	spin_unlock_irqrestore(&pll->lock, flags);
>+
> 	/*
> 	 * According to SPEC, t3 - t2 need to be greater than
> 	 * 1us and 1/FREF, respectively.
>@@ -508,6 +521,8 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
> 	if (!pll)
> 		return ERR_PTR(-ENOMEM);
> 
>+	spin_lock_init(&pll->lock);
>+
> 	init.name = name;
> 	init.flags = pll_clk->flags;
> 	init.parent_names = &parent_name;
>@@ -551,3 +566,95 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct device *dev, const char *name,
> 	return hw;
> }
> EXPORT_SYMBOL_GPL(imx_dev_clk_hw_pll14xx);
>+
>+/*
>+ * Debugfs interface for Audio PLL runtime monitoring and control
>+ *
>+ * This interface allows dynamic adjustment of the Audio PLL
>+ * K-divider for precise frequency tuning, particularly useful
>+ * for audio applications.
>+ *
>+ * examples for the usage of the two interfaces:
>+ *   1): Get the current PLL setting of dividers
>+ *     cat /sys/kernel/debug/audio_pll_monitor/audio_pll1/pll_parameter
>+ *
>+ *   2): Adjust the K-divider by a small delta_k
>+ *     echo 1 > /sys/kernel/debug/audio_pll_monitor/audio_pll1/delta_k;
>+ */
>+#ifdef CONFIG_DEBUG_FS
>+static int pll_delta_k_get(void *data, u64 *val)
>+{
>+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
>+	*val = pll->delta_k;
>+	return 0;
>+}
>+
>+static int pll_delta_k_set(void *data, u64 val)
>+{
>+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
>+	unsigned long flags;
>+	u32 div_ctl1;
>+	s16 kdiv, delta_k;
>+
>+	delta_k = (s16)clamp_t(long, val, KDIV_MIN, KDIV_MAX);
>+	pll->delta_k = delta_k;
>+
>+	spin_lock_irqsave(&pll->lock, flags);
>+
>+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
>+	kdiv = (s16)FIELD_GET(KDIV_MASK, div_ctl1);
>+	kdiv = (s16)clamp_t(s32, (s32)kdiv + delta_k, KDIV_MIN, KDIV_MAX);
>+	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);
>+
>+	spin_unlock_irqrestore(&pll->lock, flags);
>+
>+	return 0;
>+}
>+DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(delta_k_fops, pll_delta_k_get, pll_delta_k_set, "%lld\n");
>+
>+static int pll_setting_show(struct seq_file *s, void *data)
>+{
>+	struct clk_pll14xx *pll = to_clk_pll14xx(s->private);
>+	u32 div_ctl0, div_ctl1;
>+	u32 mdiv, pdiv, sdiv, kdiv;
>+
>+	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
>+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);

sashiko-bot brings a valid concern, you may need to protect the read operation.

[1]https://lore.kernel.org/all/20260519095044.996B7C2BCB3@smtp.kernel.org/


Regards
Peng

>+
>+	mdiv = FIELD_GET(MDIV_MASK, div_ctl0);
>+	pdiv = FIELD_GET(PDIV_MASK, div_ctl0);
>+	sdiv = FIELD_GET(SDIV_MASK, div_ctl0);
>+	kdiv = FIELD_GET(KDIV_MASK, div_ctl1);
>+
>+	seq_printf(s, "Mdiv: 0x%x; Pdiv: 0x%x; Sdiv: 0x%x; Kdiv: 0x%x\n",
>+		   mdiv, pdiv, sdiv, kdiv);
>+
>+	return 0;
>+}
>+DEFINE_SHOW_ATTRIBUTE(pll_setting);
>+
>+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int num_plls)
>+{
>+	struct dentry *rootdir, *audio_pll_dir;
>+	const char *pll_name;
>+	int i;
>+
>+	rootdir = debugfs_create_dir("audio_pll_monitor", NULL);
>+
>+	for (i = 0; i < num_plls; i++) {
>+		if (!IS_ERR_OR_NULL(hws[i])) {
>+			pll_name = clk_hw_get_name(hws[i]);
>+			audio_pll_dir = debugfs_create_dir(pll_name, rootdir);
>+			debugfs_create_file_unsafe("delta_k", 0600, audio_pll_dir,
>+					hws[i], &delta_k_fops);
>+			debugfs_create_file("pll_parameter", 0444, audio_pll_dir,
>+					hws[i], &pll_setting_fops);
>+		}
>+	}
>+}
>+#else /* !CONFIG_DEBUG_FS */
>+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int num_plls)
>+{
>+}
>+#endif /* CONFIG_DEBUG_FS */
>+EXPORT_SYMBOL_GPL(imx_audio_pll_debug_init);
>diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
>index aa5202f284f3d1b7c1b4bf65e2329831832b43a5..16e0bafa0c9b99cb4eee37a216e0a274d27f11fc 100644
>--- a/drivers/clk/imx/clk.h
>+++ b/drivers/clk/imx/clk.h
>@@ -487,4 +487,5 @@ struct clk_hw *imx_clk_gpr_mux(const char *name, const char *compatible,
> 			       u32 reg, const char **parent_names,
> 			       u8 num_parents, const u32 *mux_table, u32 mask);
> 
>+void imx_audio_pll_debug_init(struct clk_hw **hws, unsigned int num_plls);
> #endif
>
>---
>base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
>change-id: 20260421-imx8m_pll_debugfs-246d0fbbb617
>
>Best regards,
>-- 
>Jacky Bai <ping.bai@nxp.com>
>

RE: [PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control

[Jacky Bai]

> Subject: Re: [PATCH v2] clk: imx: Add audio PLL debugfs for K-divider control
> 
[..]

> >@@ -374,9 +379,13 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw,
> unsigned long drate,
> > 		div_ctl0 |= FIELD_PREP(SDIV_MASK, rate.sdiv);
> > 		writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> >
> >+		spin_lock_irqsave(&pll->lock, flags);
> 
> Should this lock also protect the setting to DIV_CTL0?
>

I can add the DIV_CTL0 into the lock scope in next version.

The AI review agent's comment for the spinlock stuff is just let the code looks
like correct and useful, but useless in this audio pll debugfs real use case.

BR

> >+
> > 		writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv),
> > 			       pll->base + DIV_CTL1);
> >
> >+		spin_unlock_irqrestore(&pll->lock, flags);
> >+
> > 		return 0;
> > 	}
> >
> >@@ -394,8 +403,12 @@ static int clk_pll1443x_set_rate(struct clk_hw *hw,
> unsigned long drate,
> > 		   FIELD_PREP(SDIV_MASK, rate.sdiv);
> > 	writel_relaxed(div_ctl0, pll->base + DIV_CTL0);
> >
> >+	spin_lock_irqsave(&pll->lock, flags);
> 
> Ditto.
> 
> >+
> > 	writel_relaxed(FIELD_PREP(KDIV_MASK, rate.kdiv), pll->base +
> > DIV_CTL1);
> >
> >+	spin_unlock_irqrestore(&pll->lock, flags);
> >+
> > 	/*
> > 	 * According to SPEC, t3 - t2 need to be greater than
> > 	 * 1us and 1/FREF, respectively.
> >@@ -508,6 +521,8 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct
> device *dev, const char *name,
> > 	if (!pll)
> > 		return ERR_PTR(-ENOMEM);
> >
> >+	spin_lock_init(&pll->lock);
> >+
> > 	init.name = name;
> > 	init.flags = pll_clk->flags;
> > 	init.parent_names = &parent_name;
> >@@ -551,3 +566,95 @@ struct clk_hw *imx_dev_clk_hw_pll14xx(struct
> device *dev, const char *name,
> > 	return hw;
> > }
> > EXPORT_SYMBOL_GPL(imx_dev_clk_hw_pll14xx);
> >+
> >+/*
> >+ * Debugfs interface for Audio PLL runtime monitoring and control
> >+ *
> >+ * This interface allows dynamic adjustment of the Audio PLL
> >+ * K-divider for precise frequency tuning, particularly useful
> >+ * for audio applications.
> >+ *
> >+ * examples for the usage of the two interfaces:
> >+ *   1): Get the current PLL setting of dividers
> >+ *     cat
> /sys/kernel/debug/audio_pll_monitor/audio_pll1/pll_parameter
> >+ *
> >+ *   2): Adjust the K-divider by a small delta_k
> >+ *     echo 1 > /sys/kernel/debug/audio_pll_monitor/audio_pll1/delta_k;
> >+ */
> >+#ifdef CONFIG_DEBUG_FS
> >+static int pll_delta_k_get(void *data, u64 *val) {
> >+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
> >+	*val = pll->delta_k;
> >+	return 0;
> >+}
> >+
> >+static int pll_delta_k_set(void *data, u64 val) {
> >+	struct clk_pll14xx *pll = to_clk_pll14xx(data);
> >+	unsigned long flags;
> >+	u32 div_ctl1;
> >+	s16 kdiv, delta_k;
> >+
> >+	delta_k = (s16)clamp_t(long, val, KDIV_MIN, KDIV_MAX);
> >+	pll->delta_k = delta_k;
> >+
> >+	spin_lock_irqsave(&pll->lock, flags);
> >+
> >+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> >+	kdiv = (s16)FIELD_GET(KDIV_MASK, div_ctl1);
> >+	kdiv = (s16)clamp_t(s32, (s32)kdiv + delta_k, KDIV_MIN, KDIV_MAX);
> >+	writel_relaxed(FIELD_PREP(KDIV_MASK, kdiv), pll->base + DIV_CTL1);
> >+
> >+	spin_unlock_irqrestore(&pll->lock, flags);
> >+
> >+	return 0;
> >+}
> >+DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(delta_k_fops, pll_delta_k_get,
> >+pll_delta_k_set, "%lld\n");
> >+
> >+static int pll_setting_show(struct seq_file *s, void *data) {
> >+	struct clk_pll14xx *pll = to_clk_pll14xx(s->private);
> >+	u32 div_ctl0, div_ctl1;
> >+	u32 mdiv, pdiv, sdiv, kdiv;
> >+
> >+	div_ctl0 = readl_relaxed(pll->base + DIV_CTL0);
> >+	div_ctl1 = readl_relaxed(pll->base + DIV_CTL1);
> 
> sashiko-bot brings a valid concern, you may need to protect the read
> operation.
> 
> [1]https://lore.kernel.org/all/20260519095044.996B7C2BCB3@smtp.kernel.
> org/
> 
> 
> Regards
> Peng
> 
> >+
> >+	mdiv = FIELD_GET(MDIV_MASK, div_ctl0);
> >+	pdiv = FIELD_GET(PDIV_MASK, div_ctl0);
> >+	sdiv = FIELD_GET(SDIV_MASK, div_ctl0);
> >+	kdiv = FIELD_GET(KDIV_MASK, div_ctl1);
> >+
> >+	seq_printf(s, "Mdiv: 0x%x; Pdiv: 0x%x; Sdiv: 0x%x; Kdiv: 0x%x\n",
> >+		   mdiv, pdiv, sdiv, kdiv);
> >+
> >+	return 0;
> >+}
> >+DEFINE_SHOW_ATTRIBUTE(pll_setting);
> >+
> >+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int
> >+num_plls) {
> >+	struct dentry *rootdir, *audio_pll_dir;
> >+	const char *pll_name;
> >+	int i;
> >+
> >+	rootdir = debugfs_create_dir("audio_pll_monitor", NULL);
> >+
> >+	for (i = 0; i < num_plls; i++) {
> >+		if (!IS_ERR_OR_NULL(hws[i])) {
> >+			pll_name = clk_hw_get_name(hws[i]);
> >+			audio_pll_dir = debugfs_create_dir(pll_name, rootdir);
> >+			debugfs_create_file_unsafe("delta_k", 0600, audio_pll_dir,
> >+					hws[i], &delta_k_fops);
> >+			debugfs_create_file("pll_parameter", 0444, audio_pll_dir,
> >+					hws[i], &pll_setting_fops);
> >+		}
> >+	}
> >+}
> >+#else /* !CONFIG_DEBUG_FS */
> >+void imx_audio_pll_debug_init(struct clk_hw *hws[], unsigned int
> >+num_plls) { } #endif /* CONFIG_DEBUG_FS */
> >+EXPORT_SYMBOL_GPL(imx_audio_pll_debug_init);
> >diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index
> >aa5202f284f3d1b7c1b4bf65e2329831832b43a5..16e0bafa0c9b99cb4eee37
> a216e0a
> >274d27f11fc 100644
> >--- a/drivers/clk/imx/clk.h
> >+++ b/drivers/clk/imx/clk.h
> >@@ -487,4 +487,5 @@ struct clk_hw *imx_clk_gpr_mux(const char *name,
> const char *compatible,
> > 			       u32 reg, const char **parent_names,
> > 			       u8 num_parents, const u32 *mux_table, u32 mask);
> >
> >+void imx_audio_pll_debug_init(struct clk_hw **hws, unsigned int
> >+num_plls);
> > #endif
> >
> >---
> >base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
> >change-id: 20260421-imx8m_pll_debugfs-246d0fbbb617
> >
> >Best regards,
> >--
> >Jacky Bai <ping.bai@nxp.com>
> >