[PATCH] clk: fractional-divider: check parent rate only for general approximation

[PATCH] clk: fractional-divider: check parent rate only for general approximation

[Katsuhiro Suzuki]

Custom approximation of fractional-divider may not need parent clock
rate checking. For example Rockchip SoCs work fine using grand parent
clock rate even if target rate is greater than parent.

This patch removes parent clock rate check from custom approximation.

For detailied example, clock tree of Rockchip I2S audio hardware.
  - A i2s1_div is integer divider can divide input clock 1/1 ~ 1/16.
    Initialize divider value is 1.
  - A i2s1_frac is fractional divider can divide input to x/y, x and
    y are 16bit integer.
  - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.

CPLL --> | selector | ---> i2s1_div -+--> | selector | --> I2S1 MCLK
GPLL --> |          | ,--------------'    |          |
                      `--> i2s1_frac ---> |          |

Clock mux system try to choose suitable one from i2s1_div and
i2s1_frac for master clock (MCLK) of I2S1.

Bad scenario as follows:
  - Try to set MCLK to 8.192MHz (32kHz audio replay)
    Candidate setting is
    - i2s1_div : GPLL / 60           = 8.192MHz
    i2s1_div is same as target clock rate, so mux choose this.
    i2s1_div output rate is changed 491.52MHz -> 8.192MHz

  - After that try to set to 11.2896MHz (44.1kHz audio replay)
    Candidate settings are
    - i2s1_div : CPLL / 107 = 11.214945MHz
    - i2s1_frac: i2s1_div   = 8.192MHz
      This is because clk_fd_round_rate() thinks target rate
      (11.2896MHz) is higher than parent rate (i2s1_div = 8.192MHz)
      and returns parent clock rate.
    Clock mux system choose i2s1_div, but this clock rate is not
    acceptable for I2S driver, so users cannot replay audio.

Expected behavior is:
  - Try to set master clock to 11.2896MHz (44.1kHz audio replay)
    Candidate settings are
    - i2s1_div : CPLL / 107          = 11.214945MHz
    - i2s1_frac: i2s1_div * 147/6400 = 11.2896MHz
                 (change i2s1_div to GPLL / 1 = 491.52MHz)
      If apply this commit, clk_fd_round_rate() calls custom
      approximate function of Rockchip even if target rate is higher
      than parent. Custom function changes both grand parent
      (i2s1_div) and parent (i2s_frac) settings at same time.
    Clock mux system can choose i2s1_frac and audio works fine.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
---
 drivers/clk/clk-fractional-divider.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 545dceec0bbf..b0fc5509e0ff 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long m, n;
 	u64 ret;
 
-	if (!rate || rate >= *parent_rate)
+	if (!rate)
 		return *parent_rate;
 
-	if (fd->approximation)
+	if (fd->approximation) {
 		fd->approximation(hw, rate, parent_rate, &m, &n);
-	else
+	} else {
+		if (rate >= *parent_rate)
+			return *parent_rate;
+
 		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
+	}
 
 	ret = (u64)*parent_rate * m;
 	do_div(ret, n);
-- 
2.20.1

Re: [PATCH] clk: fractional-divider: check parent rate only for general approximation

[Stephen Boyd]

+Heiko because it mentions Rockchip

Quoting Katsuhiro Suzuki (2019-01-30 15:50:22)
> Custom approximation of fractional-divider may not need parent clock
> rate checking. For example Rockchip SoCs work fine using grand parent
> clock rate even if target rate is greater than parent.
> 
> This patch removes parent clock rate check from custom approximation.
> 
> For detailied example, clock tree of Rockchip I2S audio hardware.

s/detailied/detailed/

>   - A i2s1_div is integer divider can divide input clock 1/1 ~ 1/16.
>     Initialize divider value is 1.

s/Initialize/Initial/

>   - A i2s1_frac is fractional divider can divide input to x/y, x and
>     y are 16bit integer.
>   - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
> 
> CPLL --> | selector | ---> i2s1_div -+--> | selector | --> I2S1 MCLK
> GPLL --> |          | ,--------------'    |          |
>                       `--> i2s1_frac ---> |          |
> 
> Clock mux system try to choose suitable one from i2s1_div and
> i2s1_frac for master clock (MCLK) of I2S1.
> 
> Bad scenario as follows:
>   - Try to set MCLK to 8.192MHz (32kHz audio replay)
>     Candidate setting is
>     - i2s1_div : GPLL / 60           = 8.192MHz
>     i2s1_div is same as target clock rate, so mux choose this.
>     i2s1_div output rate is changed 491.52MHz -> 8.192MHz
> 
>   - After that try to set to 11.2896MHz (44.1kHz audio replay)
>     Candidate settings are
>     - i2s1_div : CPLL / 107 = 11.214945MHz
>     - i2s1_frac: i2s1_div   = 8.192MHz
>       This is because clk_fd_round_rate() thinks target rate
>       (11.2896MHz) is higher than parent rate (i2s1_div = 8.192MHz)
>       and returns parent clock rate.
>     Clock mux system choose i2s1_div, but this clock rate is not
>     acceptable for I2S driver, so users cannot replay audio.
> 
> Expected behavior is:
>   - Try to set master clock to 11.2896MHz (44.1kHz audio replay)
>     Candidate settings are
>     - i2s1_div : CPLL / 107          = 11.214945MHz
>     - i2s1_frac: i2s1_div * 147/6400 = 11.2896MHz
>                  (change i2s1_div to GPLL / 1 = 491.52MHz)
>       If apply this commit, clk_fd_round_rate() calls custom
>       approximate function of Rockchip even if target rate is higher
>       than parent. Custom function changes both grand parent
>       (i2s1_div) and parent (i2s_frac) settings at same time.

Can you indicate what that function is? Is this merged upstream or
something you're developing now?

>     Clock mux system can choose i2s1_frac and audio works fine.

I think this last paragraph from "If apply this commit" can be
unindented from the expected behavior section.

> 
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> ---
>  drivers/clk/clk-fractional-divider.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 545dceec0bbf..b0fc5509e0ff 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>         unsigned long m, n;
>         u64 ret;
>  
> -       if (!rate || rate >= *parent_rate)
> +       if (!rate)
>                 return *parent_rate;

Ok. I think it would be clearer if we had

	unsigned long flags = clk_hw_get_flags()

	if (!rate ||
	    !(flags & CLK_SET_PARENT_RATE) && rate >= *parent_rate)

indicating that the parent of the clk isn't expected to change rate so
we can only achieve that much frequency. Plus some sort of comment to
this effect would be helpful too.

We could also introduce macros to check clk flags. Then it would read
nicer:

	if (!rate || clk_hw_can_set_parent_rate(hw) && rate >= ...)

Re: [PATCH] clk: fractional-divider: check parent rate only for general approximation

[Katsuhiro Suzuki]

Hello Stephen,

Thanks a lot for your review. I choose clk_hw_can_set_parent_rate()
macro and fix typo and patch description. And I post v2 patch.

Best Regards,
Katsuhiro Suzuki

On 2019/02/07 4:40, Stephen Boyd wrote:
> +Heiko because it mentions Rockchip
> 
> Quoting Katsuhiro Suzuki (2019-01-30 15:50:22)
>> Custom approximation of fractional-divider may not need parent clock
>> rate checking. For example Rockchip SoCs work fine using grand parent
>> clock rate even if target rate is greater than parent.
>>
>> This patch removes parent clock rate check from custom approximation.
>>
>> For detailied example, clock tree of Rockchip I2S audio hardware.
> 
> s/detailied/detailed/
> 
>>    - A i2s1_div is integer divider can divide input clock 1/1 ~ 1/16.
>>      Initialize divider value is 1.
> 
> s/Initialize/Initial/
> 
>>    - A i2s1_frac is fractional divider can divide input to x/y, x and
>>      y are 16bit integer.
>>    - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
>>
>> CPLL --> | selector | ---> i2s1_div -+--> | selector | --> I2S1 MCLK
>> GPLL --> |          | ,--------------'    |          |
>>                        `--> i2s1_frac ---> |          |
>>
>> Clock mux system try to choose suitable one from i2s1_div and
>> i2s1_frac for master clock (MCLK) of I2S1.
>>
>> Bad scenario as follows:
>>    - Try to set MCLK to 8.192MHz (32kHz audio replay)
>>      Candidate setting is
>>      - i2s1_div : GPLL / 60           = 8.192MHz
>>      i2s1_div is same as target clock rate, so mux choose this.
>>      i2s1_div output rate is changed 491.52MHz -> 8.192MHz
>>
>>    - After that try to set to 11.2896MHz (44.1kHz audio replay)
>>      Candidate settings are
>>      - i2s1_div : CPLL / 107 = 11.214945MHz
>>      - i2s1_frac: i2s1_div   = 8.192MHz
>>        This is because clk_fd_round_rate() thinks target rate
>>        (11.2896MHz) is higher than parent rate (i2s1_div = 8.192MHz)
>>        and returns parent clock rate.
>>      Clock mux system choose i2s1_div, but this clock rate is not
>>      acceptable for I2S driver, so users cannot replay audio.
>>
>> Expected behavior is:
>>    - Try to set master clock to 11.2896MHz (44.1kHz audio replay)
>>      Candidate settings are
>>      - i2s1_div : CPLL / 107          = 11.214945MHz
>>      - i2s1_frac: i2s1_div * 147/6400 = 11.2896MHz
>>                   (change i2s1_div to GPLL / 1 = 491.52MHz)
>>        If apply this commit, clk_fd_round_rate() calls custom
>>        approximate function of Rockchip even if target rate is higher
>>        than parent. Custom function changes both grand parent
>>        (i2s1_div) and parent (i2s_frac) settings at same time.
> 
> Can you indicate what that function is? Is this merged upstream or
> something you're developing now?
> 
>>      Clock mux system can choose i2s1_frac and audio works fine.
> 
> I think this last paragraph from "If apply this commit" can be
> unindented from the expected behavior section.
> 
>>
>> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>> ---
>>   drivers/clk/clk-fractional-divider.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
>> index 545dceec0bbf..b0fc5509e0ff 100644
>> --- a/drivers/clk/clk-fractional-divider.c
>> +++ b/drivers/clk/clk-fractional-divider.c
>> @@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>>          unsigned long m, n;
>>          u64 ret;
>>   
>> -       if (!rate || rate >= *parent_rate)
>> +       if (!rate)
>>                  return *parent_rate;
> 
> Ok. I think it would be clearer if we had
> 
> 	unsigned long flags = clk_hw_get_flags()
> 
> 	if (!rate ||
> 	    !(flags & CLK_SET_PARENT_RATE) && rate >= *parent_rate)
> 
> indicating that the parent of the clk isn't expected to change rate so
> we can only achieve that much frequency. Plus some sort of comment to
> this effect would be helpful too.
> 
> We could also introduce macros to check clk flags. Then it would read
> nicer:
> 
> 	if (!rate || clk_hw_can_set_parent_rate(hw) && rate >= ...)
> 
> 
> 

[PATCH] clk: fractional-divider: check parent rate only for general approximation

[Katsuhiro Suzuki]

Custom approximation of fractional-divider may not need parent clock
rate checking. For example Rockchip SoCs work fine using grand parent
clock rate evne if target rate is greater than parent.

This patch removes parent clock rate check from custom approximation.

Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
---
 drivers/clk/clk-fractional-divider.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 545dceec0bbf..b0fc5509e0ff 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long m, n;
 	u64 ret;
 
-	if (!rate || rate >= *parent_rate)
+	if (!rate)
 		return *parent_rate;
 
-	if (fd->approximation)
+	if (fd->approximation) {
 		fd->approximation(hw, rate, parent_rate, &m, &n);
-	else
+	} else {
+		if (rate >= *parent_rate)
+			return *parent_rate;
+
 		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
+	}
 
 	ret = (u64)*parent_rate * m;
 	do_div(ret, n);
-- 
2.20.1

Re: [PATCH] clk: fractional-divider: check parent rate only for general approximation

[Katsuhiro Suzuki]

Ping...

On 2019/01/07 22:21, Katsuhiro Suzuki wrote:
> Custom approximation of fractional-divider may not need parent clock
> rate checking. For example Rockchip SoCs work fine using grand parent
> clock rate evne if target rate is greater than parent.
> 
> This patch removes parent clock rate check from custom approximation.
> 
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> ---
>   drivers/clk/clk-fractional-divider.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 545dceec0bbf..b0fc5509e0ff 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>   	unsigned long m, n;
>   	u64 ret;
>   
> -	if (!rate || rate >= *parent_rate)
> +	if (!rate)
>   		return *parent_rate;
>   
> -	if (fd->approximation)
> +	if (fd->approximation) {
>   		fd->approximation(hw, rate, parent_rate, &m, &n);
> -	else
> +	} else {
> +		if (rate >= *parent_rate)
> +			return *parent_rate;
> +
>   		clk_fd_general_approximation(hw, rate, parent_rate, &m, &n);
> +	}
>   
>   	ret = (u64)*parent_rate * m;
>   	do_div(ret, n);
> 

Re: [PATCH] clk: fractional-divider: check parent rate only for general approximation

[Stephen Boyd]

Quoting Katsuhiro Suzuki (2019-01-07 05:21:24)
> Custom approximation of fractional-divider may not need parent clock
> rate checking. For example Rockchip SoCs work fine using grand parent
> clock rate evne if target rate is greater than parent.
> 
> This patch removes parent clock rate check from custom approximation.
> 
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> ---
>  drivers/clk/clk-fractional-divider.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 545dceec0bbf..b0fc5509e0ff 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>         unsigned long m, n;
>         u64 ret;
>  
> -       if (!rate || rate >= *parent_rate)
> +       if (!rate)

Is your fraction multiplying the rate up? If it's a divider it shouldn't
be increasing the rate more than whatever the parent is supplying so
this looks odd. How does the grandparent matter here? Maybe you can show
the example in the commit text so we can all understand what's going on
here.

>                 return *parent_rate;
>  

Re: [PATCH] clk: fractional-divider: check parent rate only for general approximation

[Katsuhiro Suzuki]

Hello Stephen,

Thank you for your comment.
I'll add detail description and send it.

Best Regards,
Katsuhiro Suzuki

On 2019/01/30 6:55, Stephen Boyd wrote:
> Quoting Katsuhiro Suzuki (2019-01-07 05:21:24)
>> Custom approximation of fractional-divider may not need parent clock
>> rate checking. For example Rockchip SoCs work fine using grand parent
>> clock rate evne if target rate is greater than parent.
>>
>> This patch removes parent clock rate check from custom approximation.
>>
>> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>> ---
>>   drivers/clk/clk-fractional-divider.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
>> index 545dceec0bbf..b0fc5509e0ff 100644
>> --- a/drivers/clk/clk-fractional-divider.c
>> +++ b/drivers/clk/clk-fractional-divider.c
>> @@ -79,13 +79,17 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>>          unsigned long m, n;
>>          u64 ret;
>>   
>> -       if (!rate || rate >= *parent_rate)
>> +       if (!rate)
> 
> Is your fraction multiplying the rate up? If it's a divider it shouldn't
> be increasing the rate more than whatever the parent is supplying so
> this looks odd. How does the grandparent matter here? Maybe you can show
> the example in the commit text so we can all understand what's going on
> here.
> 
>>                  return *parent_rate;
>>   
> 
>