[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 at nxp.com>
> >