[PATCH v2] clk: fractional-divider: check parent rate only if flag is set

[PATCH v2] clk: fractional-divider: check parent rate only if flag is set

[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 checks parent clock rate only if CLK_SET_RATE_PARENT flag
is set.

For detailed example, clock tree of Rockchip I2S audio hardware.
  - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
  - i2s1_div is integer divider can divide N (N is 1~128).
    Input clock is CPLL or GPLL. Initial divider value is N = 1.
    Ex) PLL = CPLL, N = 10, i2s1_div output rate is
      CPLL / 10 = 1.2GHz / 10 = 120MHz
  - i2s1_frac is fractional divider can divide input to x/y, x and
    y are 16bit integer.

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 candidate is exactly same as target clock rate, so mux
    choose this clock source. 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.

Above is current upstreamed behavior. 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 at same
                 time.

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 | 2 +-
 drivers/clk/clk.c                    | 8 ++++++++
 include/linux/clk-provider.h         | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 545dceec0bbf..fdfe2e423d15 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -79,7 +79,7 @@ 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 || (!clk_hw_can_set_rate_parent(hw) && rate >= *parent_rate))
 		return *parent_rate;
 
 	if (fd->approximation)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d2477a5058ac..070c0cb29ee8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -518,6 +518,14 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 }
 EXPORT_SYMBOL_GPL(clk_hw_set_rate_range);
 
+bool clk_hw_can_set_rate_parent(struct clk_hw *hw)
+{
+	unsigned long flags = clk_hw_get_flags(hw);
+
+	return flags & CLK_SET_RATE_PARENT;
+}
+EXPORT_SYMBOL_GPL(clk_hw_can_set_rate_parent);
+
 /*
  * Helper for finding best parent to provide a given frequency. This can be used
  * directly as a determine_rate callback (e.g. for a mux), or from a more
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index e443fa9fa859..693a51937ded 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -808,6 +808,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
+bool clk_hw_can_set_rate_parent(struct clk_hw *hw);
 
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
-- 
2.20.1

Re: [PATCH v2] clk: fractional-divider: check parent rate only if flag is set

[Heiko Stuebner]

Am Sonntag, 10. Februar 2019, 16:38:06 CET schrieb 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 checks parent clock rate only if CLK_SET_RATE_PARENT flag
> is set.
> 
> For detailed example, clock tree of Rockchip I2S audio hardware.
>   - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
>   - i2s1_div is integer divider can divide N (N is 1~128).
>     Input clock is CPLL or GPLL. Initial divider value is N = 1.
>     Ex) PLL = CPLL, N = 10, i2s1_div output rate is
>       CPLL / 10 = 1.2GHz / 10 = 120MHz
>   - i2s1_frac is fractional divider can divide input to x/y, x and
>     y are 16bit integer.
> 
> 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 candidate is exactly same as target clock rate, so mux
>     choose this clock source. 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.
> 
> Above is current upstreamed behavior. 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 at same
>                  time.
> 
> 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>

While it took me a bit to wrap my head around fractional dividers
again :-) ... this sounds definitly sensible to do, especially
as the clock driver should really take into account potentially
setting the parent rate if possible, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  drivers/clk/clk-fractional-divider.c | 2 +-
>  drivers/clk/clk.c                    | 8 ++++++++
>  include/linux/clk-provider.h         | 1 +
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 545dceec0bbf..fdfe2e423d15 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -79,7 +79,7 @@ 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 || (!clk_hw_can_set_rate_parent(hw) && rate >= *parent_rate))
>  		return *parent_rate;
>  
>  	if (fd->approximation)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d2477a5058ac..070c0cb29ee8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -518,6 +518,14 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_set_rate_range);
>  
> +bool clk_hw_can_set_rate_parent(struct clk_hw *hw)
> +{
> +	unsigned long flags = clk_hw_get_flags(hw);
> +
> +	return flags & CLK_SET_RATE_PARENT;
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_can_set_rate_parent);
> +
>  /*
>   * Helper for finding best parent to provide a given frequency. This can be used
>   * directly as a determine_rate callback (e.g. for a mux), or from a more
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index e443fa9fa859..693a51937ded 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -808,6 +808,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>  void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
>  void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>  			   unsigned long max_rate);
> +bool clk_hw_can_set_rate_parent(struct clk_hw *hw);
>  
>  static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
>  {
> 

Re: [PATCH v2] clk: fractional-divider: check parent rate only if flag is set

[Katsuhiro Suzuki]

Ping...

On 2019/02/11 0:38, 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 even if target rate is greater than parent.
> 
> This patch checks parent clock rate only if CLK_SET_RATE_PARENT flag
> is set.
> 
> For detailed example, clock tree of Rockchip I2S audio hardware.
>    - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
>    - i2s1_div is integer divider can divide N (N is 1~128).
>      Input clock is CPLL or GPLL. Initial divider value is N = 1.
>      Ex) PLL = CPLL, N = 10, i2s1_div output rate is
>        CPLL / 10 = 1.2GHz / 10 = 120MHz
>    - i2s1_frac is fractional divider can divide input to x/y, x and
>      y are 16bit integer.
> 
> 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 candidate is exactly same as target clock rate, so mux
>      choose this clock source. 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.
> 
> Above is current upstreamed behavior. 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 at same
>                   time.
> 
> 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 | 2 +-
>   drivers/clk/clk.c                    | 8 ++++++++
>   include/linux/clk-provider.h         | 1 +
>   3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 545dceec0bbf..fdfe2e423d15 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -79,7 +79,7 @@ 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 || (!clk_hw_can_set_rate_parent(hw) && rate >= *parent_rate))
>   		return *parent_rate;
>   
>   	if (fd->approximation)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d2477a5058ac..070c0cb29ee8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -518,6 +518,14 @@ void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>   }
>   EXPORT_SYMBOL_GPL(clk_hw_set_rate_range);
>   
> +bool clk_hw_can_set_rate_parent(struct clk_hw *hw)
> +{
> +	unsigned long flags = clk_hw_get_flags(hw);
> +
> +	return flags & CLK_SET_RATE_PARENT;
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_can_set_rate_parent);
> +
>   /*
>    * Helper for finding best parent to provide a given frequency. This can be used
>    * directly as a determine_rate callback (e.g. for a mux), or from a more
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index e443fa9fa859..693a51937ded 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -808,6 +808,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>   void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent);
>   void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>   			   unsigned long max_rate);
> +bool clk_hw_can_set_rate_parent(struct clk_hw *hw);
>   
>   static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
>   {
> 

Re: [PATCH v2] clk: fractional-divider: check parent rate only if flag is set

[Stephen Boyd]

Quoting Katsuhiro Suzuki (2019-02-10 07:38:06)
> 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 checks parent clock rate only if CLK_SET_RATE_PARENT flag
> is set.
> 
> For detailed example, clock tree of Rockchip I2S audio hardware.
>   - Clock rate of CPLL is 1.2GHz, GPLL is 491.52MHz.
>   - i2s1_div is integer divider can divide N (N is 1~128).
>     Input clock is CPLL or GPLL. Initial divider value is N = 1.
>     Ex) PLL = CPLL, N = 10, i2s1_div output rate is
>       CPLL / 10 = 1.2GHz / 10 = 120MHz
>   - i2s1_frac is fractional divider can divide input to x/y, x and
>     y are 16bit integer.
> 
> CPLL --> | selector | ---> i2s1_div -+--> | selector | --> I2S1 MCLK

Applied to clk-next but I made the function into a macro because I don't
see the benefit to exporting such a simple thing.