[RESEND PATCH 0/3] clk: si5351: PLL reset fixes

[RESEND PATCH 0/3] clk: si5351: PLL reset fixes

[Sergej Sawazki]

The Si5351 clock generator has up to 8 output clocks and 2 PLLs. In order
to get a deterministic phase offset relationship between the output clocks,
it is necessary to reset the PLLs is certain scenarios.

This patch-set:
 * fixes a regression and adds resetting the PLL before enabling the outputs
 * adds a dt-property for enabling/disabling the PLL reset
 * adds a debug message for PLL reset (it is helpful during debugging,
   probably no longer required?)

Based on clk-next.

Best regards,
Sergej

Sergej Sawazki (3):
  clk: si5351: Apply PLL soft reset before enabling the outputs
  clk: si5351: Add DT property to enable PLL reset
  clk: si5351: Add a debug message for PLL reset

 .../devicetree/bindings/clock/silabs,si5351.txt    |  1 +
 drivers/clk/clk-si5351.c                           | 40 ++++++++++++++++++++--
 include/linux/platform_data/si5351.h               |  2 ++
 3 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.7.4

[RESEND PATCH 1/3] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sergej Sawazki]

The "Si5351A/B/C Data Sheet" states to apply a PLL soft reset before
enabling the output clocks [1]. This is required to get a deterministic
phase relationship between the output clocks.

Without resetting the PLL, the phase relationship between the clocks is
unpredictable. Fix this by resetting the PLL in si5351_clkout_prepare().

It also fixes a regression introduced in commit 6dc669a22c77 ("clk: si5351:
Add PLL soft reset") that causes a disruption on platforms where clocks
derived from different PLLs do different things by resetting only the PLL
which the output clock is derived from.

References:
[1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
    Figure 12 ("I2C Programming Procedure")

Fixes: 6dc669a22c77 ("clk: si5351: Add PLL soft reset")
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Rabeeh Khoury <rabeeh@solid-run.com>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Sergej Sawazki <sergej@taudac.com>
---
 drivers/clk/clk-si5351.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 2492442..46bbc95 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -898,6 +898,21 @@ static int _si5351_clkout_set_disable_state(
 	return 0;
 }
 
+void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
+{
+	u8 val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
+
+	switch (val & SI5351_CLK_INPUT_MASK) {
+	case SI5351_CLK_INPUT_XTAL:
+	case SI5351_CLK_INPUT_CLKIN:
+		return;  /* PLL not used, no need to reset */
+	}
+
+	si5351_reg_write(drvdata, SI5351_PLL_RESET,
+			 (val & SI5351_CLK_PLL_SELECT) ? SI5351_PLL_RESET_B :
+			 SI5351_PLL_RESET_A);
+}
+
 static int si5351_clkout_prepare(struct clk_hw *hw)
 {
 	struct si5351_hw_data *hwdata =
@@ -905,6 +920,14 @@ static int si5351_clkout_prepare(struct clk_hw *hw)
 
 	si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
 			SI5351_CLK_POWERDOWN, 0);
+
+	/*
+	 * Reset the PLLs before enabling the outputs to get a deterministic
+	 * phase relationship between the output clocks. Otherwise, the phase
+	 * offset beween the clocks is unpredictable.
+	 */
+	_si5351_clkout_reset_pll(hwdata->drvdata, hwdata->num);
+
 	si5351_set_bits(hwdata->drvdata, SI5351_OUTPUT_ENABLE_CTRL,
 			(1 << hwdata->num), 0);
 	return 0;
@@ -1095,8 +1118,7 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 	 * Do a pll soft reset on both plls, needed in some cases to get
 	 * all outputs running.
 	 */
-	si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
-			 SI5351_PLL_RESET_A | SI5351_PLL_RESET_B);
+	_si5351_clkout_reset_pll(hwdata->drvdata, hwdata->num);
 
 	dev_dbg(&hwdata->drvdata->client->dev,
 		"%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
-- 
2.7.4

[RESEND PATCH 2/3] clk: si5351: Add DT property to enable PLL reset

[Sergej Sawazki]

Add optional output clock DT property to enable PLL reset when a clock
output is enabled or the clock rate is changed.

Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Rabeeh Khoury <rabeeh@solid-run.com>
Signed-off-by: Sergej Sawazki <sergej@taudac.com>
---
 Documentation/devicetree/bindings/clock/silabs,si5351.txt |  1 +
 drivers/clk/clk-si5351.c                                  | 11 ++++++++++-
 include/linux/platform_data/si5351.h                      |  2 ++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/silabs,si5351.txt b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
index 28b2830..157ee02 100644
--- a/Documentation/devicetree/bindings/clock/silabs,si5351.txt
+++ b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
@@ -45,6 +45,7 @@ Optional child node properties:
 - silabs,multisynth-source: source pll A(0) or B(1) of corresponding multisynth
   divider.
 - silabs,pll-master: boolean, multisynth can change pll frequency.
+- silabs,pll-reset: boolean, clock output can reset its pll.
 - silabs,disable-state : clock output disable state, shall be
   0 = clock output is driven LOW when disabled
   1 = clock output is driven HIGH when disabled
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 46bbc95..aa8d8d9 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -900,7 +900,13 @@ static int _si5351_clkout_set_disable_state(
 
 void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
 {
-	u8 val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
+	struct si5351_platform_data *pdata = drvdata->client->dev.platform_data;
+	u8 val;
+
+	if (!pdata->clkout[num].pll_reset)
+		return;  /* Resetting PLL disabled */
+
+	val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
 
 	switch (val & SI5351_CLK_INPUT_MASK) {
 	case SI5351_CLK_INPUT_XTAL:
@@ -1321,6 +1327,9 @@ static int si5351_dt_parse(struct i2c_client *client,
 
 		pdata->clkout[num].pll_master =
 			of_property_read_bool(child, "silabs,pll-master");
+
+		pdata->clkout[num].pll_reset =
+			of_property_read_bool(child, "silabs,pll-reset");
 	}
 	client->dev.platform_data = pdata;
 
diff --git a/include/linux/platform_data/si5351.h b/include/linux/platform_data/si5351.h
index 533d980..da346f22 100644
--- a/include/linux/platform_data/si5351.h
+++ b/include/linux/platform_data/si5351.h
@@ -85,6 +85,7 @@ enum si5351_disable_state {
  * @multisynth_src: multisynth source clock
  * @clkout_src: clkout source clock
  * @pll_master: if true, clkout can also change pll rate
+ * @pll_reset: if true, clkout can reset its pll
  * @drive: output drive strength
  * @rate: initial clkout rate, or default if 0
  */
@@ -94,6 +95,7 @@ struct si5351_clkout_config {
 	enum si5351_drive_strength drive;
 	enum si5351_disable_state disable_state;
 	bool pll_master;
+	bool pll_reset;
 	unsigned long rate;
 };
 
-- 
2.7.4

[RESEND PATCH 3/3] clk: si5351: Add a debug message for PLL reset

[Sergej Sawazki]

Print a debug message when resetting a PLL. It is useful to figure out
what PLL is resetted and when.

Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Rabeeh Khoury <rabeeh@solid-run.com>
Signed-off-by: Sergej Sawazki <sergej@taudac.com>
---
 drivers/clk/clk-si5351.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index aa8d8d9..7a122c7 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -917,6 +917,10 @@ void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
 	si5351_reg_write(drvdata, SI5351_PLL_RESET,
 			 (val & SI5351_CLK_PLL_SELECT) ? SI5351_PLL_RESET_B :
 			 SI5351_PLL_RESET_A);
+
+	dev_dbg(&drvdata->client->dev, "%s - %s: pll = %d\n",
+		__func__, clk_hw_get_name(&drvdata->clkout[num].hw),
+		(val & SI5351_CLK_PLL_SELECT) ? 1 : 0);
 }
 
 static int si5351_clkout_prepare(struct clk_hw *hw)
-- 
2.7.4

Re: [RESEND PATCH 1/3] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sebastian Hesselbarth]

On 02.09.2017 11:57, Sergej Sawazki wrote:
> The "Si5351A/B/C Data Sheet" states to apply a PLL soft reset before
> enabling the output clocks [1]. This is required to get a deterministic
> phase relationship between the output clocks.
> 
> Without resetting the PLL, the phase relationship between the clocks is
> unpredictable. Fix this by resetting the PLL in si5351_clkout_prepare().
> 
> It also fixes a regression introduced in commit 6dc669a22c77 ("clk: si5351:
> Add PLL soft reset") that causes a disruption on platforms where clocks
> derived from different PLLs do different things by resetting only the PLL
> which the output clock is derived from.
> 
> References:
> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
>     Figure 12 ("I2C Programming Procedure")
> 
> Fixes: 6dc669a22c77 ("clk: si5351: Add PLL soft reset")
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> ---
>  drivers/clk/clk-si5351.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 2492442..46bbc95 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -898,6 +898,21 @@ static int _si5351_clkout_set_disable_state(
>  	return 0;
>  }
>  
> +void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
> +{
> +	u8 val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
> +
> +	switch (val & SI5351_CLK_INPUT_MASK) {
> +	case SI5351_CLK_INPUT_XTAL:
> +	case SI5351_CLK_INPUT_CLKIN:
> +		return;  /* PLL not used, no need to reset */
> +	}
> +
> +	si5351_reg_write(drvdata, SI5351_PLL_RESET,
> +			 (val & SI5351_CLK_PLL_SELECT) ? SI5351_PLL_RESET_B :
> +			 SI5351_PLL_RESET_A);
> +}
> +
>  static int si5351_clkout_prepare(struct clk_hw *hw)
>  {
>  	struct si5351_hw_data *hwdata =
> @@ -905,6 +920,14 @@ static int si5351_clkout_prepare(struct clk_hw *hw)
>  
>  	si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
>  			SI5351_CLK_POWERDOWN, 0);
> +
> +	/*
> +	 * Reset the PLLs before enabling the outputs to get a deterministic
> +	 * phase relationship between the output clocks. Otherwise, the phase
> +	 * offset beween the clocks is unpredictable.
> +	 */
> +	_si5351_clkout_reset_pll(hwdata->drvdata, hwdata->num);

Sergej,

sorry for the late reply. In general, I am fine with refactoring the PLL
reset into it's own function. But instead of adding the function here
unconditionally and changing driver behavior for all users, please first
add the property to conditionally reset the PLLs on clkout changes and
use it to conditionally call above reset function.

> +
>  	si5351_set_bits(hwdata->drvdata, SI5351_OUTPUT_ENABLE_CTRL,
>  			(1 << hwdata->num), 0);
>  	return 0;
> @@ -1095,8 +1118,7 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
>  	 * Do a pll soft reset on both plls, needed in some cases to get
>  	 * all outputs running.
>  	 */
> -	si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
> -			 SI5351_PLL_RESET_A | SI5351_PLL_RESET_B);
> +	_si5351_clkout_reset_pll(hwdata->drvdata, hwdata->num);

Above reset (a) has been removed by Russell's fix for the PLL reset
issue on different clock outputs and (b) does not match the original
behavior. Original behavior was to reset both PLLs but the function
does only reset one PLL.

In any way, the patch should not mess with this line at all.

Sebastian

>  
>  	dev_dbg(&hwdata->drvdata->client->dev,
>  		"%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
> 

Re: [RESEND PATCH 2/3] clk: si5351: Add DT property to enable PLL reset

[Sebastian Hesselbarth]

On 02.09.2017 11:57, Sergej Sawazki wrote:
> Add optional output clock DT property to enable PLL reset when a clock
> output is enabled or the clock rate is changed.

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

But please move it to the first patch applied, then parse the property
in the driver and finally add the conditional reset protected by the
parsed property.

Sebastian

> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> ---
>  Documentation/devicetree/bindings/clock/silabs,si5351.txt |  1 +
>  drivers/clk/clk-si5351.c                                  | 11 ++++++++++-
>  include/linux/platform_data/si5351.h                      |  2 ++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/silabs,si5351.txt b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> index 28b2830..157ee02 100644
> --- a/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> +++ b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
> @@ -45,6 +45,7 @@ Optional child node properties:
>  - silabs,multisynth-source: source pll A(0) or B(1) of corresponding multisynth
>    divider.
>  - silabs,pll-master: boolean, multisynth can change pll frequency.
> +- silabs,pll-reset: boolean, clock output can reset its pll.
>  - silabs,disable-state : clock output disable state, shall be
>    0 = clock output is driven LOW when disabled
>    1 = clock output is driven HIGH when disabled
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 46bbc95..aa8d8d9 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -900,7 +900,13 @@ static int _si5351_clkout_set_disable_state(
>  
>  void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
>  {
> -	u8 val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
> +	struct si5351_platform_data *pdata = drvdata->client->dev.platform_data;
> +	u8 val;
> +
> +	if (!pdata->clkout[num].pll_reset)
> +		return;  /* Resetting PLL disabled */
> +
> +	val = si5351_reg_read(drvdata, SI5351_CLK0_CTRL + num);
>  
>  	switch (val & SI5351_CLK_INPUT_MASK) {
>  	case SI5351_CLK_INPUT_XTAL:
> @@ -1321,6 +1327,9 @@ static int si5351_dt_parse(struct i2c_client *client,
>  
>  		pdata->clkout[num].pll_master =
>  			of_property_read_bool(child, "silabs,pll-master");
> +
> +		pdata->clkout[num].pll_reset =
> +			of_property_read_bool(child, "silabs,pll-reset");
>  	}
>  	client->dev.platform_data = pdata;
>  
> diff --git a/include/linux/platform_data/si5351.h b/include/linux/platform_data/si5351.h
> index 533d980..da346f22 100644
> --- a/include/linux/platform_data/si5351.h
> +++ b/include/linux/platform_data/si5351.h
> @@ -85,6 +85,7 @@ enum si5351_disable_state {
>   * @multisynth_src: multisynth source clock
>   * @clkout_src: clkout source clock
>   * @pll_master: if true, clkout can also change pll rate
> + * @pll_reset: if true, clkout can reset its pll
>   * @drive: output drive strength
>   * @rate: initial clkout rate, or default if 0
>   */
> @@ -94,6 +95,7 @@ struct si5351_clkout_config {
>  	enum si5351_drive_strength drive;
>  	enum si5351_disable_state disable_state;
>  	bool pll_master;
> +	bool pll_reset;
>  	unsigned long rate;
>  };
>  
> 

Re: [RESEND PATCH 3/3] clk: si5351: Add a debug message for PLL reset

[Sebastian Hesselbarth]

On 02.09.2017 11:57, Sergej Sawazki wrote:
> Print a debug message when resetting a PLL. It is useful to figure out
> what PLL is resetted and when.

I am fine with the debug message but please squash it into the patch
adding _si5351_clkout_reset_pll.

Sebastian

> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> ---
>  drivers/clk/clk-si5351.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index aa8d8d9..7a122c7 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -917,6 +917,10 @@ void _si5351_clkout_reset_pll(struct si5351_driver_data *drvdata, int num)
>  	si5351_reg_write(drvdata, SI5351_PLL_RESET,
>  			 (val & SI5351_CLK_PLL_SELECT) ? SI5351_PLL_RESET_B :
>  			 SI5351_PLL_RESET_A);
> +
> +	dev_dbg(&drvdata->client->dev, "%s - %s: pll = %d\n",
> +		__func__, clk_hw_get_name(&drvdata->clkout[num].hw),
> +		(val & SI5351_CLK_PLL_SELECT) ? 1 : 0);
>  }
>  
>  static int si5351_clkout_prepare(struct clk_hw *hw)
>