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@xxxxxxxxx>
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@xxxxxxx>:
>From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>
>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@xxxxxxxxx>
>Cc: Rabeeh Khoury <rabeeh@xxxxxxxxxxxxx>
>Cc: Jacob Siverskog <jacob@teenage.engineering>
>Cc: Sergej Sawazki <sergej@xxxxxxxxxx>
>Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>---
> 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];
> };
>
