Re: [PATCH v2 4/4] clk: si5351: allow PLLs to be adjusted without reset

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

Hi Alvin,

thanks for the patch. In general, I am fine with the change as default behavior is to do what it did before.

So,
Acked-by: <sebastian.hesselbarth@gmail.com>
for the functional changes.

For the DT changes you'll need Rob, Stephen, Michael's approval more than mine.

However, as Jacob and Sergej already noticed on their patches, I cannot spend enough time for maintaining the driver anymore.

Is there anyone volunteering to pick maintainership up?

Regards,
  Sebastian

(Hopefully plain/text now)


Am 4. Oktober 2023 08:35:30 MESZ schrieb "Alvin Šipraga" <alvin@pqrs.dk>:
>From: Alvin Šipraga <alsi@bang-olufsen.dk>
>
>Introduce a new PLL reset mode flag which controls whether or not to
>reset a PLL after adjusting its rate. The mode can be configured through
>platform data or device tree.
>
>Since commit 6dc669a22c77 ("clk: si5351: Add PLL soft reset"), the
>driver unconditionally resets a PLL whenever its rate is adjusted.
>The rationale was that a PLL reset was required to get three outputs
>working at the same time. Before this change, the driver never reset the
>PLLs.
>
>Commit b26ff127c52c ("clk: si5351: Apply PLL soft reset before enabling
>the outputs") subsequently introduced an option to reset the PLL when
>enabling a clock output that sourced it. Here, the rationale was that
>this is required to get a deterministic phase relationship between
>multiple output clocks.
>
>This clearly shows that it is useful to reset the PLLs in applications
>where multiple clock outputs are used. However, the Si5351 also allows
>for glitch-free rate adjustment of its PLLs if one avoids resetting the
>PLL. In our audio application where a single Si5351 clock output is used
>to supply a runtime adjustable bit clock, this unconditional PLL reset
>behaviour introduces unwanted glitches in the clock output.
>
>It would appear that the problem being solved in the former commit
>may be solved by using the optional device tree property introduced in
>the latter commit, obviating the need for an unconditional PLL reset
>after rate adjustment. But it's not OK to break the default behaviour of
>the driver, and it cannot be assumed that all device trees are using the
>property introduced in the latter commit. Hence, the new behaviour is
>made opt-in.
>
>Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>Cc: Rabeeh Khoury <rabeeh@solid-run.com>
>Cc: Jacob Siverskog <jacob@teenage.engineering>
>Cc: Sergej Sawazki <sergej@taudac.com>
>Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
>---
> drivers/clk/clk-si5351.c             | 47 ++++++++++++++++++++++++++--
> include/linux/platform_data/si5351.h |  2 ++
> 2 files changed, 46 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>index 00fb9b09e030..95d7afb8cfc6 100644
>--- a/drivers/clk/clk-si5351.c
>+++ b/drivers/clk/clk-si5351.c
>@@ -506,6 +506,8 @@ static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> {
> 	struct si5351_hw_data *hwdata =
> 		container_of(hw, struct si5351_hw_data, hw);
>+	struct si5351_platform_data *pdata =
>+		hwdata->drvdata->client->dev.platform_data;
> 	u8 reg = (hwdata->num == 0) ? SI5351_PLLA_PARAMETERS :
> 		SI5351_PLLB_PARAMETERS;
> 
>@@ -518,9 +520,10 @@ static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> 		(hwdata->params.p2 == 0) ? SI5351_CLK_INTEGER_MODE : 0);
> 
> 	/* Do a pll soft reset on the affected pll */
>-	si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
>-			 hwdata->num == 0 ? SI5351_PLL_RESET_A :
>-					    SI5351_PLL_RESET_B);
>+	if (pdata->pll_reset[hwdata->num])
>+		si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
>+				 hwdata->num == 0 ? SI5351_PLL_RESET_A :
>+						    SI5351_PLL_RESET_B);
> 
> 	dev_dbg(&hwdata->drvdata->client->dev,
> 		"%s - %s: p1 = %lu, p2 = %lu, p3 = %lu, parent_rate = %lu, rate = %lu\n",
>@@ -1222,6 +1225,44 @@ static int si5351_dt_parse(struct i2c_client *client,
> 		}
> 	}
> 
>+	/*
>+	 * Parse PLL reset mode. For compatibility with older device trees, the
>+	 * default is to always reset a PLL after setting its rate.
>+	 */
>+	pdata->pll_reset[0] = true;
>+	pdata->pll_reset[1] = true;
>+
>+	of_property_for_each_u32(np, "silabs,pll-reset-mode", prop, p, num) {
>+		if (num >= 2) {
>+			dev_err(&client->dev,
>+				"invalid pll %d on pll-reset-mode prop\n", num);
>+			return -EINVAL;
>+		}
>+
>+		p = of_prop_next_u32(prop, p, &val);
>+		if (!p) {
>+			dev_err(&client->dev,
>+				"missing pll-reset-mode for pll %d\n", num);
>+			return -EINVAL;
>+		}
>+
>+		switch (val) {
>+		case 0:
>+			/* Reset PLL whenever its rate is adjusted */
>+			pdata->pll_reset[num] = true;
>+			break;
>+		case 1:
>+			/* Don't reset PLL whenever its rate is adjusted */
>+			pdata->pll_reset[num] = false;
>+			break;
>+		default:
>+			dev_err(&client->dev,
>+				"invalid pll-reset-mode %d for pll %d\n", val,
>+				num);
>+			return -EINVAL;
>+		}
>+	}
>+
> 	/* per clkout properties */
> 	for_each_child_of_node(np, child) {
> 		if (of_property_read_u32(child, "reg", &num)) {
>diff --git a/include/linux/platform_data/si5351.h b/include/linux/platform_data/si5351.h
>index c71a2dd66143..5f412a615532 100644
>--- a/include/linux/platform_data/si5351.h
>+++ b/include/linux/platform_data/si5351.h
>@@ -105,10 +105,12 @@ struct si5351_clkout_config {
>  * @clk_xtal: xtal input clock
>  * @clk_clkin: clkin input clock
>  * @pll_src: array of pll source clock setting
>+ * @pll_reset: array indicating if plls should be reset after setting the rate
>  * @clkout: array of clkout configuration
>  */
> struct si5351_platform_data {
> 	enum si5351_pll_src pll_src[2];
>+	bool pll_reset[2];
> 	struct si5351_clkout_config clkout[8];
> };
>