[PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[ryang]

The current behavior is that clk_round_rate would return the same clock
rate passed to it for valid PLL configurations. This change will return
the exact rate the PLL will provide in accordance with clk API.

Signed-off-by: ryang <decatf@gmail.com>
---
 drivers/clk/tegra/clk-pll.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 17a058c3bbc1..36014a6ec42e 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
 		return -EINVAL;
 	}
 
-	cfg->output_rate >>= p_div;
+	if (cfg->m == 0) {
+		cfg->output_rate = 0;
+	} else {
+		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
+		cfg->output_rate >>= p_div;
+	}
 
 	if (pll->params->pdiv_tohw) {
 		ret = _p_div_to_hw(hw, 1 << p_div);
-- 
2.17.1

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang <decatf@gmail.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 17a058c3bbc1..36014a6ec42e 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
>  		return -EINVAL;
>  	}
>  
> -	cfg->output_rate >>= p_div;
> +	if (cfg->m == 0) {
> +		cfg->output_rate = 0;

I think a WARN_ON() is appropriate here. the input divider should never be 0.

Peter.

> +	} else {
> +		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> +		cfg->output_rate >>= p_div;
> +	}
>  
>  	if (pll->params->pdiv_tohw) {
>  		ret = _p_div_to_hw(hw, 1 << p_div);
> -- 
> 2.17.1
> 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang <decatf@gmail.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 17a058c3bbc1..36014a6ec42e 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
>  		return -EINVAL;
>  	}
>  
> -	cfg->output_rate >>= p_div;
> +	if (cfg->m == 0) {
> +		cfg->output_rate = 0;

I think a WARN_ON() is appropriate here. the input divider should never be 0.

Peter.

> +	} else {
> +		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> +		cfg->output_rate >>= p_div;
> +	}
>  
>  	if (pll->params->pdiv_tohw) {
>  		ret = _p_div_to_hw(hw, 1 << p_div);
> -- 
> 2.17.1
> 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[r yang]

On Mon, Sep 24, 2018 at 11:08:03AM +0300, Peter De Schrijver wrote:
> On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> > The current behavior is that clk_round_rate would return the same clock
> > rate passed to it for valid PLL configurations. This change will return
> > the exact rate the PLL will provide in accordance with clk API.
> > 
> > Signed-off-by: ryang <decatf@gmail.com>
> > ---
> >  drivers/clk/tegra/clk-pll.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > index 17a058c3bbc1..36014a6ec42e 100644
> > --- a/drivers/clk/tegra/clk-pll.c
> > +++ b/drivers/clk/tegra/clk-pll.c
> > @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
> >  		return -EINVAL;
> >  	}
> >  
> > -	cfg->output_rate >>= p_div;
> > +	if (cfg->m == 0) {
> > +		cfg->output_rate = 0;
> 
> I think a WARN_ON() is appropriate here. the input divider should never be 0.
> 
> Peter.
> 

Should it return -EINVAL (or some error) too? _calc_rate is also in the
clk_set_rate code path. I think we want to avoid programming the
register to 0 input divider all together?

> > +	} else {
> > +		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> > +		cfg->output_rate >>= p_div;
> > +	}
> >  
> >  	if (pll->params->pdiv_tohw) {
> >  		ret = _p_div_to_hw(hw, 1 << p_div);
> > -- 
> > 2.17.1
> > 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Mon, Sep 24, 2018 at 03:18:04PM -0400, r yang wrote:
> On Mon, Sep 24, 2018 at 11:08:03AM +0300, Peter De Schrijver wrote:
> > On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> > > The current behavior is that clk_round_rate would return the same clock
> > > rate passed to it for valid PLL configurations. This change will return
> > > the exact rate the PLL will provide in accordance with clk API.
> > > 
> > > Signed-off-by: ryang <decatf@gmail.com>
> > > ---
> > >  drivers/clk/tegra/clk-pll.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > > index 17a058c3bbc1..36014a6ec42e 100644
> > > --- a/drivers/clk/tegra/clk-pll.c
> > > +++ b/drivers/clk/tegra/clk-pll.c
> > > @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	cfg->output_rate >>= p_div;
> > > +	if (cfg->m == 0) {
> > > +		cfg->output_rate = 0;
> > 
> > I think a WARN_ON() is appropriate here. the input divider should never be 0.
> > 
> > Peter.
> > 
> 
> Should it return -EINVAL (or some error) too? _calc_rate is also in the
> clk_set_rate code path. I think we want to avoid programming the
> register to 0 input divider all together?
> 

Yes. writing 0 to the input divider is usually not allowed. In some cases it's
equivalent to writing 1, but better not count on that.

Peter.

> > > +	} else {
> > > +		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> > > +		cfg->output_rate >>= p_div;
> > > +	}
> > >  
> > >  	if (pll->params->pdiv_tohw) {
> > >  		ret = _p_div_to_hw(hw, 1 << p_div);
> > > -- 
> > > 2.17.1
> > > 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Mon, Sep 24, 2018 at 03:18:04PM -0400, r yang wrote:
> On Mon, Sep 24, 2018 at 11:08:03AM +0300, Peter De Schrijver wrote:
> > On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> > > The current behavior is that clk_round_rate would return the same clock
> > > rate passed to it for valid PLL configurations. This change will return
> > > the exact rate the PLL will provide in accordance with clk API.
> > > 
> > > Signed-off-by: ryang <decatf@gmail.com>
> > > ---
> > >  drivers/clk/tegra/clk-pll.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > > index 17a058c3bbc1..36014a6ec42e 100644
> > > --- a/drivers/clk/tegra/clk-pll.c
> > > +++ b/drivers/clk/tegra/clk-pll.c
> > > @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct tegra_clk_pll_freq_table *cfg,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	cfg->output_rate >>= p_div;
> > > +	if (cfg->m == 0) {
> > > +		cfg->output_rate = 0;
> > 
> > I think a WARN_ON() is appropriate here. the input divider should never be 0.
> > 
> > Peter.
> > 
> 
> Should it return -EINVAL (or some error) too? _calc_rate is also in the
> clk_set_rate code path. I think we want to avoid programming the
> register to 0 input divider all together?
> 

Yes. writing 0 to the input divider is usually not allowed. In some cases it's
equivalent to writing 1, but better not count on that.

Peter.

> > > +	} else {
> > > +		cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> > > +		cfg->output_rate >>= p_div;
> > > +	}
> > >  
> > >  	if (pll->params->pdiv_tohw) {
> > >  		ret = _p_div_to_hw(hw, 1 << p_div);
> > > -- 
> > > 2.17.1
> > > 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Thierry Reding]

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

On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang <decatf@gmail.com>
> ---
>  drivers/clk/tegra/clk-pll.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

With Peter's comment addressed:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Dmitry Osipenko]

On 9/22/18 1:01 AM, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang <decatf@gmail.com>

A minor comment.. [0] require a real name, it's not even obvious whether
you're "Ryan G" or "R Yang".

[0]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1