[PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sergej Sawazki]

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

Without the PLL reset, the phase offset beween the clocks is unpredictable.

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

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 255d0fe..6cca425 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -905,6 +905,15 @@ 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_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
+			 SI5351_PLL_RESET_A | SI5351_PLL_RESET_B);
+
 	si5351_set_bits(hwdata->drvdata, SI5351_OUTPUT_ENABLE_CTRL,
 			(1 << hwdata->num), 0);
 	return 0;
-- 
2.7.4

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Stephen Boyd]

On 07/25, Sergej Sawazki wrote:
> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
> before enabling the outputs [1]. This is required to get a deterministic
> phase relationship between the output clocks.
> 
> Without the PLL reset, the phase offset beween the clocks is unpredictable.
> 
> References:
> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
>     Figure 12 ("I2C Programming Procedure")
> 
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> ---

This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
reset, 2015-11-20)? But I think that commit was causing some
problem for Russell King and there was going to be a patch to
change it but nothing has materialized on the list. Unless this
is that patch?

Does the other reset in this driver need to be removed? At the
least, it may be a good idea to combine the two places where
CLK_POWERDOWN is cleared to also have this reset part.

>  drivers/clk/clk-si5351.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 255d0fe..6cca425 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -905,6 +905,15 @@ 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_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
> +			 SI5351_PLL_RESET_A | SI5351_PLL_RESET_B);
> +
>  	si5351_set_bits(hwdata->drvdata, SI5351_OUTPUT_ENABLE_CTRL,
>  			(1 << hwdata->num), 0);
>  	return 0;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sebastian Hesselbarth]

On 26.07.2017 03:11, Stephen Boyd wrote:
> On 07/25, Sergej Sawazki wrote:
>> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
>> before enabling the outputs [1]. This is required to get a deterministic
>> phase relationship between the output clocks.
>>
>> Without the PLL reset, the phase offset beween the clocks is unpredictable.
>>
>> References:
>> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
>>     Figure 12 ("I2C Programming Procedure")
>>
>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
>> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
>> ---
> 
> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
> reset, 2015-11-20)? But I think that commit was causing some
> problem for Russell King and there was going to be a patch to
> change it but nothing has materialized on the list. Unless this
> is that patch?

Sergej, Stephen,

resetting both PLLs in this driver will not happen as it does have
an influence on the other PLL and all clocks on it.

I understand that some of the functions of the clk gen will not be
available with this driver but it is not the use case of this driver.

So, NAK on this one.

The patch you are talking about is still pending but I think I just
send it in a few days.

Sebastian

> Does the other reset in this driver need to be removed? At the
> least, it may be a good idea to combine the two places where
> CLK_POWERDOWN is cleared to also have this reset part.
> 
>>  drivers/clk/clk-si5351.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>> index 255d0fe..6cca425 100644
>> --- a/drivers/clk/clk-si5351.c
>> +++ b/drivers/clk/clk-si5351.c
>> @@ -905,6 +905,15 @@ 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_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
>> +			 SI5351_PLL_RESET_A | SI5351_PLL_RESET_B);
>> +
>>  	si5351_set_bits(hwdata->drvdata, SI5351_OUTPUT_ENABLE_CTRL,
>>  			(1 << hwdata->num), 0);
>>  	return 0;
> 

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Russell King - ARM Linux]

On Tue, Jul 25, 2017 at 06:11:12PM -0700, Stephen Boyd wrote:
> On 07/25, Sergej Sawazki wrote:
> > The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
> > before enabling the outputs [1]. This is required to get a deterministic
> > phase relationship between the output clocks.
> > 
> > Without the PLL reset, the phase offset beween the clocks is unpredictable.
> > 
> > References:
> > [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
> >     Figure 12 ("I2C Programming Procedure")
> > 
> > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> > Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> > ---
> 
> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
> reset, 2015-11-20)? But I think that commit was causing some
> problem for Russell King and there was going to be a patch to
> change it but nothing has materialized on the list. Unless this
> is that patch?

Did I not send that patch?  Sorry, I can't keep track of everything.

8<====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] clk: si5351: fix PLL reset

Changing the audio sample rate on the SolidRun Cubox disrupts the video
output.  The Si5351 provides both the video clock (using PLLA on output
0) and the audio clock (using PLLB on output 2).

When the rate of clock output 2 is changed, it reconfigures PLLB, which
results in both PLLA and PLLB being reset.  The reset of PLLA causes
clock output 0 to be disrupted, thereby causing a loss of sync by the
attached display device.

Hence, each time the audio sample rate changes (eg, when a video player
starts up, or when starting to play music) the video display momentarily
blanks while the Si5351 settles down.  Prior to the commit below, this
behaviour did not happen.

Fix this by only resetting only the PLL which has been changed.

Fixes: 6dc669a22c77 ("clk: si5351: Add PLL soft reset")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/clk/clk-si5351.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 412a1fb5b6aa..48961ac1ced5 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -520,6 +520,11 @@ static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 		SI5351_CLK_INTEGER_MODE,
 		(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);
+
 	dev_dbg(&hwdata->drvdata->client->dev,
 		"%s - %s: p1 = %lu, p2 = %lu, p3 = %lu, parent_rate = %lu, rate = %lu\n",
 		__func__, clk_hw_get_name(hw),
@@ -1092,13 +1097,6 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 	si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
 			SI5351_CLK_POWERDOWN, 0);
 
-	/*
-	 * 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);
-
 	dev_dbg(&hwdata->drvdata->client->dev,
 		"%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
 		__func__, clk_hw_get_name(hw), (1 << rdiv),
-- 
2.7.4

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sergej Sawazki]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]

Am 26.07.2017 um 06:43 schrieb Sebastian Hesselbarth:
> On 26.07.2017 03:11, Stephen Boyd wrote:
>> On 07/25, Sergej Sawazki wrote:
>>> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
>>> before enabling the outputs [1]. This is required to get a deterministic
>>> phase relationship between the output clocks.
>>>
>>> Without the PLL reset, the phase offset beween the clocks is unpredictable.
>>>
>>> References:
>>> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
>>>     Figure 12 ("I2C Programming Procedure")
>>>
>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
>>> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
>>> ---
>> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
>> reset, 2015-11-20)? But I think that commit was causing some
>> problem for Russell King and there was going to be a patch to
>> change it but nothing has materialized on the list. Unless this
>> is that patch?
> Sergej, Stephen,
>
> resetting both PLLs in this driver will not happen as it does have
> an influence on the other PLL and all clocks on it.
>
> I understand that some of the functions of the clk gen will not be
> available with this driver but it is not the use case of this driver.
>
> So, NAK on this one.
>
> The patch you are talking about is still pending but I think I just
> send it in a few days.
>
> Sebastian

Sebastian, Stephen,

On my setup, the Si5351 provides audio bit and frame clocks. Without
resetting the PLLs before enabling the output clocks the phase offset
between the clocks is unpredictable, the clocks are not aligned, this
corrupts the audio stream.

I agree, resetting both PLLs is not a good idea. Only one PLL should
be resetted (the one that the output clocks are connected to).

I am not changing the rates, I am only enabling/disabling the outputs
and changing the clkin source. So resetting the PLL in set_rate() does
not help me.

Sergej

 

[-- Attachment #2: Type: text/html, Size: 2937 bytes --]

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sergej Sawazki]

Sorry for the HTML stuff in my previous message, please ignore it.

Am 26.07.2017 um 06:43 schrieb Sebastian Hesselbarth:
> On 26.07.2017 03:11, Stephen Boyd wrote:
>> On 07/25, Sergej Sawazki wrote:
>>> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
>>> before enabling the outputs [1]. This is required to get a deterministic
>>> phase relationship between the output clocks.
>>>
>>> Without the PLL reset, the phase offset beween the clocks is unpredictable.
>>>
>>> References:
>>> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
>>>     Figure 12 ("I2C Programming Procedure")
>>>
>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
>>> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
>>> ---
>> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
>> reset, 2015-11-20)? But I think that commit was causing some
>> problem for Russell King and there was going to be a patch to
>> change it but nothing has materialized on the list. Unless this
>> is that patch?
> Sergej, Stephen,
> 
> resetting both PLLs in this driver will not happen as it does have
> an influence on the other PLL and all clocks on it.
> 
> I understand that some of the functions of the clk gen will not be
> available with this driver but it is not the use case of this driver.
> 
> So, NAK on this one.
> 
> The patch you are talking about is still pending but I think I just
> send it in a few days.
> 
> Sebastian
> 

Sebastian, Stephen,

On my setup, the Si5351 provides audio bit and frame clocks. Without
resetting the PLLs before enabling the output clocks the phase offset
between the clocks is unpredictable, the clocks are not aligned, this
corrupts the audio stream.

I agree, resetting both PLLs is not a good idea. Only one PLL should
be resetted (the one that the output clocks are connected to).

I am not changing the rates, I am only enabling/disabling the outputs
and changing the clkin source. So resetting the PLL in set_rate() does
not help me.

Sergej
 

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Russell King - ARM Linux]

On Thu, Jul 27, 2017 at 01:10:58AM +0200, Sergej Sawazki wrote:
> Am 26.07.2017 um 06:43 schrieb Sebastian Hesselbarth:
> > On 26.07.2017 03:11, Stephen Boyd wrote:
> >> On 07/25, Sergej Sawazki wrote:
> >>> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
> >>> before enabling the outputs [1]. This is required to get a deterministic
> >>> phase relationship between the output clocks.
> >>>
> >>> Without the PLL reset, the phase offset beween the clocks is unpredictable.
> >>>
> >>> References:
> >>> [1] https://www.silabs.com/Support%20Documents/TechnicalDocs/Si5351-B.pdf
> >>>     Figure 12 ("I2C Programming Procedure")
> >>>
> >>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> >>> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> >>> Signed-off-by: Sergej Sawazki <sergej@taudac.com>
> >>> ---
> >> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
> >> reset, 2015-11-20)? But I think that commit was causing some
> >> problem for Russell King and there was going to be a patch to
> >> change it but nothing has materialized on the list. Unless this
> >> is that patch?
> > Sergej, Stephen,
> >
> > resetting both PLLs in this driver will not happen as it does have
> > an influence on the other PLL and all clocks on it.
> >
> > I understand that some of the functions of the clk gen will not be
> > available with this driver but it is not the use case of this driver.
> >
> > So, NAK on this one.
> >
> > The patch you are talking about is still pending but I think I just
> > send it in a few days.
> >
> > Sebastian
> 
> Sebastian, Stephen,
> 
> On my setup, the Si5351 provides audio bit and frame clocks. Without
> resetting the PLLs before enabling the output clocks the phase offset
> between the clocks is unpredictable, the clocks are not aligned, this
> corrupts the audio stream.
> 
> I agree, resetting both PLLs is not a good idea. Only one PLL should
> be resetted (the one that the output clocks are connected to).
> 
> I am not changing the rates, I am only enabling/disabling the outputs
> and changing the clkin source. So resetting the PLL in set_rate() does
> not help me.

However you may wish to do this, you must not do it in a way that
regresses other platforms.

In other words, if you need to reset both PLLs, and that disrupts
other platforms, resetting the PLLs needs to be configurable by
some mechanism, so the platforms that need it can have it, but
those that don't (and are positively harmed by it) need it can
remain unaffected.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sebastian Hesselbarth]

On 26.07.2017 10:18, Russell King - ARM Linux wrote:
> On Tue, Jul 25, 2017 at 06:11:12PM -0700, Stephen Boyd wrote:
>> This is similar to commit 6dc669a22c77 (clk: si5351: Add PLL soft
>> reset, 2015-11-20)? But I think that commit was causing some
>> problem for Russell King and there was going to be a patch to
>> change it but nothing has materialized on the list. Unless this
>> is that patch?
> 
> Did I not send that patch?  Sorry, I can't keep track of everything.

Russell,

I sent you a suggestion for this patch back then, you must have missed
it.

However,

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

for the below patch.

Thanks!

Sebastian

> 8<====
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] clk: si5351: fix PLL reset
> 
> Changing the audio sample rate on the SolidRun Cubox disrupts the video
> output.  The Si5351 provides both the video clock (using PLLA on output
> 0) and the audio clock (using PLLB on output 2).
> 
> When the rate of clock output 2 is changed, it reconfigures PLLB, which
> results in both PLLA and PLLB being reset.  The reset of PLLA causes
> clock output 0 to be disrupted, thereby causing a loss of sync by the
> attached display device.
> 
> Hence, each time the audio sample rate changes (eg, when a video player
> starts up, or when starting to play music) the video display momentarily
> blanks while the Si5351 settles down.  Prior to the commit below, this
> behaviour did not happen.
> 
> Fix this by only resetting only the PLL which has been changed.
> 
> Fixes: 6dc669a22c77 ("clk: si5351: Add PLL soft reset")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/clk/clk-si5351.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index 412a1fb5b6aa..48961ac1ced5 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -520,6 +520,11 @@ static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  		SI5351_CLK_INTEGER_MODE,
>  		(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);
> +
>  	dev_dbg(&hwdata->drvdata->client->dev,
>  		"%s - %s: p1 = %lu, p2 = %lu, p3 = %lu, parent_rate = %lu, rate = %lu\n",
>  		__func__, clk_hw_get_name(hw),
> @@ -1092,13 +1097,6 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
>  	si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
>  			SI5351_CLK_POWERDOWN, 0);
>  
> -	/*
> -	 * 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);
> -
>  	dev_dbg(&hwdata->drvdata->client->dev,
>  		"%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
>  		__func__, clk_hw_get_name(hw), (1 << rdiv),
> 

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sebastian Hesselbarth]

On 27.07.2017 11:11, Russell King - ARM Linux wrote:
> On Thu, Jul 27, 2017 at 01:10:58AM +0200, Sergej Sawazki wrote:
>> Am 26.07.2017 um 06:43 schrieb Sebastian Hesselbarth:
>>> On 26.07.2017 03:11, Stephen Boyd wrote:
>>>> On 07/25, Sergej Sawazki wrote:
>>>>> The "Si5351A/B/C Data Sheet" states to apply a PLLA and PLLB soft reset
>>>>> before enabling the outputs [1]. This is required to get a deterministic
>>>>> phase relationship between the output clocks.
>>>>>
>>>>> Without the PLL reset, the phase offset beween the clocks is unpredictable.
[...]
>>>
>>> resetting both PLLs in this driver will not happen as it does have
>>> an influence on the other PLL and all clocks on it.
>>>
>>> I understand that some of the functions of the clk gen will not be
>>> available with this driver but it is not the use case of this driver.
>>>
>>> So, NAK on this one.
>>>
>>> The patch you are talking about is still pending but I think I just
>>> send it in a few days.
>>
>> On my setup, the Si5351 provides audio bit and frame clocks. Without
>> resetting the PLLs before enabling the output clocks the phase offset
>> between the clocks is unpredictable, the clocks are not aligned, this
>> corrupts the audio stream.

Sergej,

if the two clocks you are generating are directly related to each other.
why aren't you using _one_ PLL and derive both clocks from the same PLL?

This should solve your alignment issues.

>> I agree, resetting both PLLs is not a good idea. Only one PLL should
>> be resetted (the one that the output clocks are connected to).
>>
>> I am not changing the rates, I am only enabling/disabling the outputs
>> and changing the clkin source. So resetting the PLL in set_rate() does
>> not help me.
> 
> However you may wish to do this, you must not do it in a way that
> regresses other platforms.
> 
> In other words, if you need to reset both PLLs, and that disrupts
> other platforms, resetting the PLLs needs to be configurable by
> some mechanism, so the platforms that need it can have it, but
> those that don't (and are positively harmed by it) need it can
> remain unaffected.

I copy that. If there is the need for resetting both PLLs, we do need
another DT property that tells the driver to do so.

Resetting both PLLs does interrupt all clocks and that does cause a
regression on systems where clocks derived from both PLLs do different
things.

Sebastian

Re: [PATCH] clk: si5351: Apply PLL soft reset before enabling the outputs

[Sergej Sawazki]

Am 28.07.2017 um 09:33 schrieb Sebastian Hesselbarth:
> On 27.07.2017 11:11, Russell King - ARM Linux wrote:
>> On Thu, Jul 27, 2017 at 01:10:58AM +0200, Sergej Sawazki wrote:
>>> Am 26.07.2017 um 06:43 schrieb Sebastian Hesselbarth:[...]
>>> On my setup, the Si5351 provides audio bit and frame clocks. Without
>>> resetting the PLLs before enabling the output clocks the phase offset
>>> between the clocks is unpredictable, the clocks are not aligned, this
>>> corrupts the audio stream.
> 
> Sergej,
> 
> if the two clocks you are generating are directly related to each other.
> why aren't you using _one_ PLL and derive both clocks from the same PLL?
> 
> This should solve your alignment issues.
> 
[...]

Sebastian,

I am using _one_ PLL for all my clocks, still, the phase relationship 
between the clocks is random on each activation.

The only way I was able to fix it, is to reset the corresponding PLL.

Sergej