[v2,0/4] clk: si5351: add option to adjust PLL without glitches

From: Alvin Šipraga <alsi@bang-olufsen.dk>

This series intends to address a problem I had when using the Si5351A as
a runtime adjustable audio bit clock. The basic issue is that the driver
in its current form unconditionally resets the PLL whenever adjusting
its rate. But this reset causes an unwanted ~1.4 ms LOW signal glitch in
the clock output.

As a remedy, a new property is added to control the reset behaviour of
the PLLs more precisely. In the process I also converted the bindings to
YAML.

Changes:

v1 -> v2:

- address Rob's comments on the two dt-bindings patches
- new patch to correct the clock node names in the only upstream device
  tree using si5351

Alvin Šipraga (4):
  dt-bindings: clock: si5351: convert to yaml
  ARM: dts: dove-cubox: fix si5351 node names
  dt-bindings: clock: si5351: add PLL reset mode property
  clk: si5351: allow PLLs to be adjusted without reset

 .../bindings/clock/silabs,si5351.txt          | 126 --------
 .../bindings/clock/silabs,si5351.yaml         | 277 ++++++++++++++++++
 arch/arm/boot/dts/marvell/dove-cubox.dts      |   4 +-
 drivers/clk/clk-si5351.c                      |  47 ++-
 include/linux/platform_data/si5351.h          |   2 +
 5 files changed, 325 insertions(+), 131 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/silabs,si5351.txt
 create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5351.yaml

On 04/10/2023 08:35, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> Correct the device tree to conform with the bindings. The node name and
> index should be separated with an @.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---

This is independent change, please do not mix it with this series. DTS
should go via ARM SOC, not drivers subsystem tree.

Best regards,
Krzysztof

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];
> };
>

On Wed, 04 Oct 2023 08:35:28 +0200, Alvin Šipraga wrote:
> Correct the device tree to conform with the bindings. The node name and
> index should be separated with an @.
> 
> 

Applied, thanks!

[2/4] ARM: dts: dove-cubox: fix si5351 node names
      https://git.kernel.org/krzk/linux-dt/c/2df26223650027602d017368f4e8dc1eff90404e

Best regards,