[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Jean-Francois Moine]

The best rate of a clock may be a bit greater than the requested one.
In such a case, the rate setting from a child clock was rejected.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
I don't know exactly why the rate constraint existed nor what can be
the impact of setting the rate of other clocks.

I had the problem when setting the PLL2 clock of the H3 (patch to come).
It has 4 outputs, so, it is composed of a base clock and 4 children
clocks pll2, pll2x2, pll2x4 and pll2x8 with a fixed factor (/4, /2, 1
and *2).
The pll2 clock rate may be only 24576000 (for the audio family 48000Hz)
or 22579200 (for the audio family 44100Hz).

Setting 24576000 asks for mul=86 and div=21,4 giving 24571428 as the
best rate, i.e. a bit slower than requested: good.

Setting 22579200 asks for mul=64 and div=17,4 giving 22588235, i.e.
a bit greater: then, the rate setting was rejected (no parent clock),
preventing audio streaming at 44100Hz.
---
 drivers/clk/sunxi/clk-factors.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index 59428db..d0774c2 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c
@@ -86,7 +86,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
 	int i, num_parents;
 	unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
 
-	/* find the parent that can help provide the fastest rate <= rate */
+	/* find the parent that can help provide the fastest rate */
 	num_parents = clk_hw_get_num_parents(hw);
 	for (i = 0; i < num_parents; i++) {
 		parent = clk_hw_get_parent_by_index(hw, i);
@@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
 		child_rate = clk_factors_round_rate(hw, req->rate,
 						    &parent_rate);
 
-		if (child_rate <= req->rate && child_rate > best_child_rate) {
+		if (child_rate > best_child_rate) {
 			best_parent = parent;
 			best = parent_rate;
 			best_child_rate = child_rate;
-- 
2.7.2

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Jean-Francois Moine]

Ping!

On Thu, 10 Mar 2016 08:15:02 +0100
Jean-Francois Moine <moinejf@free.fr> wrote:

> The best rate of a clock may be a bit greater than the requested one.
> In such a case, the rate setting from a child clock was rejected.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> I don't know exactly why the rate constraint existed nor what can be
> the impact of setting the rate of other clocks.
> 
> I had the problem when setting the PLL2 clock of the H3 (patch to come).
> It has 4 outputs, so, it is composed of a base clock and 4 children
> clocks pll2, pll2x2, pll2x4 and pll2x8 with a fixed factor (/4, /2, 1
> and *2).
> The pll2 clock rate may be only 24576000 (for the audio family 48000Hz)
> or 22579200 (for the audio family 44100Hz).
> 
> Setting 24576000 asks for mul=86 and div=21,4 giving 24571428 as the
> best rate, i.e. a bit slower than requested: good.
> 
> Setting 22579200 asks for mul=64 and div=17,4 giving 22588235, i.e.
> a bit greater: then, the rate setting was rejected (no parent clock),
> preventing audio streaming at 44100Hz.
> ---
>  drivers/clk/sunxi/clk-factors.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
> index 59428db..d0774c2 100644
> --- a/drivers/clk/sunxi/clk-factors.c
> +++ b/drivers/clk/sunxi/clk-factors.c
> @@ -86,7 +86,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
>  	int i, num_parents;
>  	unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
>  
> -	/* find the parent that can help provide the fastest rate <= rate */
> +	/* find the parent that can help provide the fastest rate */
>  	num_parents = clk_hw_get_num_parents(hw);
>  	for (i = 0; i < num_parents; i++) {
>  		parent = clk_hw_get_parent_by_index(hw, i);
> @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
>  		child_rate = clk_factors_round_rate(hw, req->rate,
>  						    &parent_rate);
>  
> -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> +		if (child_rate > best_child_rate) {
>  			best_parent = parent;
>  			best = parent_rate;
>  			best_child_rate = child_rate;
> -- 
> 2.7.2

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Maxime Ripard]

Hi,

On Thu, Mar 10, 2016 at 08:15:02AM +0100, Jean-Francois Moine wrote:
> The best rate of a clock may be a bit greater than the requested one.
> In such a case, the rate setting from a child clock was rejected.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> I don't know exactly why the rate constraint existed nor what can be
> the impact of setting the rate of other clocks.
> 
> I had the problem when setting the PLL2 clock of the H3 (patch to come).
> It has 4 outputs, so, it is composed of a base clock and 4 children
> clocks pll2, pll2x2, pll2x4 and pll2x8 with a fixed factor (/4, /2, 1
> and *2).
> The pll2 clock rate may be only 24576000 (for the audio family 48000Hz)
> or 22579200 (for the audio family 44100Hz).
> 
> Setting 24576000 asks for mul=86 and div=21,4 giving 24571428 as the
> best rate, i.e. a bit slower than requested: good.
> 
> Setting 22579200 asks for mul=64 and div=17,4 giving 22588235, i.e.
> a bit greater: then, the rate setting was rejected (no parent clock),
> preventing audio streaming at 44100Hz.
> ---
>  drivers/clk/sunxi/clk-factors.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
> index 59428db..d0774c2 100644
> --- a/drivers/clk/sunxi/clk-factors.c
> +++ b/drivers/clk/sunxi/clk-factors.c
> @@ -86,7 +86,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
>  	int i, num_parents;
>  	unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
>  
> -	/* find the parent that can help provide the fastest rate <= rate */
> +	/* find the parent that can help provide the fastest rate */
>  	num_parents = clk_hw_get_num_parents(hw);
>  	for (i = 0; i < num_parents; i++) {
>  		parent = clk_hw_get_parent_by_index(hw, i);
> @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
>  		child_rate = clk_factors_round_rate(hw, req->rate,
>  						    &parent_rate);
>  
> -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> +		if (child_rate > best_child_rate) {

I'm not sure this would work, since you'll end up picking the fastest
rate without considering whether it is the closest or not.

I guess what you want here is using the absolute difference between
the requested rate and the rate you're evaluating.

That being said, we had a similar discussion for SPI around a month
ago where we wanted a rate strictly lower than the requested one. I
guess it's time to add a flag to tell how you want to round.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160321/dd099234/attachment-0001.sig>

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Jean-Francois Moine]

On Mon, 21 Mar 2016 08:25:46 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> > -	/* find the parent that can help provide the fastest rate <= rate */
> > +	/* find the parent that can help provide the fastest rate */
> >  	num_parents = clk_hw_get_num_parents(hw);
> >  	for (i = 0; i < num_parents; i++) {
> >  		parent = clk_hw_get_parent_by_index(hw, i);
> > @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
> >  		child_rate = clk_factors_round_rate(hw, req->rate,
> >  						    &parent_rate);
> >  
> > -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> > +		if (child_rate > best_child_rate) {
> 
> I'm not sure this would work, since you'll end up picking the fastest
> rate without considering whether it is the closest or not.
> 
> I guess what you want here is using the absolute difference between
> the requested rate and the rate you're evaluating.
> 
> That being said, we had a similar discussion for SPI around a month
> ago where we wanted a rate strictly lower than the requested one. I
> guess it's time to add a flag to tell how you want to round.

You are right, I just removed half of the constraint, but I still wonder
why does this sequence introduced by the commit 862b728387aef3a37
(clk: sunxi: factors: automatic reparenting support) do
	"provide the fastest rate <= rate"
instead of
	"provide the closest rate" ?

Emilio?

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Maxime Ripard]

Hi,

On Mon, Mar 21, 2016 at 09:25:49AM +0100, Jean-Francois Moine wrote:
> On Mon, 21 Mar 2016 08:25:46 +0100
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > > -	/* find the parent that can help provide the fastest rate <= rate */
> > > +	/* find the parent that can help provide the fastest rate */
> > >  	num_parents = clk_hw_get_num_parents(hw);
> > >  	for (i = 0; i < num_parents; i++) {
> > >  		parent = clk_hw_get_parent_by_index(hw, i);
> > > @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
> > >  		child_rate = clk_factors_round_rate(hw, req->rate,
> > >  						    &parent_rate);
> > >  
> > > -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> > > +		if (child_rate > best_child_rate) {
> > 
> > I'm not sure this would work, since you'll end up picking the fastest
> > rate without considering whether it is the closest or not.
> > 
> > I guess what you want here is using the absolute difference between
> > the requested rate and the rate you're evaluating.
> > 
> > That being said, we had a similar discussion for SPI around a month
> > ago where we wanted a rate strictly lower than the requested one. I
> > guess it's time to add a flag to tell how you want to round.
> 
> You are right, I just removed half of the constraint, but I still wonder
> why does this sequence introduced by the commit 862b728387aef3a37
> (clk: sunxi: factors: automatic reparenting support) do
> 	"provide the fastest rate <= rate"
> instead of
> 	"provide the closest rate" ?

I guess it's a confusing wording, in this case where you try to have
the closest rate but below the requested rate, what you want is
actually the fastest rate that doesn't trip above the requested rate,
hence the comment I guess.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160329/6c5ef873/attachment.sig>

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Jean-Francois Moine]

On Tue, 29 Mar 2016 11:38:27 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Mon, Mar 21, 2016 at 09:25:49AM +0100, Jean-Francois Moine wrote:
> > On Mon, 21 Mar 2016 08:25:46 +0100
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > > > -	/* find the parent that can help provide the fastest rate <= rate */
> > > > +	/* find the parent that can help provide the fastest rate */
> > > >  	num_parents = clk_hw_get_num_parents(hw);
> > > >  	for (i = 0; i < num_parents; i++) {
> > > >  		parent = clk_hw_get_parent_by_index(hw, i);
> > > > @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
> > > >  		child_rate = clk_factors_round_rate(hw, req->rate,
> > > >  						    &parent_rate);
> > > >  
> > > > -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> > > > +		if (child_rate > best_child_rate) {
> > > 
> > > I'm not sure this would work, since you'll end up picking the fastest
> > > rate without considering whether it is the closest or not.
> > > 
> > > I guess what you want here is using the absolute difference between
> > > the requested rate and the rate you're evaluating.
> > > 
> > > That being said, we had a similar discussion for SPI around a month
> > > ago where we wanted a rate strictly lower than the requested one. I
> > > guess it's time to add a flag to tell how you want to round.
> > 
> > You are right, I just removed half of the constraint, but I still wonder
> > why does this sequence introduced by the commit 862b728387aef3a37
> > (clk: sunxi: factors: automatic reparenting support) do
> > 	"provide the fastest rate <= rate"
> > instead of
> > 	"provide the closest rate" ?
> 
> I guess it's a confusing wording, in this case where you try to have
> the closest rate but below the requested rate, what you want is
> actually the fastest rate that doesn't trip above the requested rate,
> hence the comment I guess.

This does not answer my question.

What I want to know is: will everyone be OK if I change the actual test
to "provide the closest rate"?

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Emilio López]

[Sorry for the delay, I meant to reply to this post a while back but I
forgot]

El 21/03/16 a las 05:25, Jean-Francois Moine escribi?:
> On Mon, 21 Mar 2016 08:25:46 +0100
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
>>> -	/* find the parent that can help provide the fastest rate <= rate */
>>> +	/* find the parent that can help provide the fastest rate */
>>>  	num_parents = clk_hw_get_num_parents(hw);
>>>  	for (i = 0; i < num_parents; i++) {
>>>  		parent = clk_hw_get_parent_by_index(hw, i);
>>> @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
>>>  		child_rate = clk_factors_round_rate(hw, req->rate,
>>>  						    &parent_rate);
>>>  
>>> -		if (child_rate <= req->rate && child_rate > best_child_rate) {
>>> +		if (child_rate > best_child_rate) {
>>
>> I'm not sure this would work, since you'll end up picking the fastest
>> rate without considering whether it is the closest or not.
>>
>> I guess what you want here is using the absolute difference between
>> the requested rate and the rate you're evaluating.
>>
>> That being said, we had a similar discussion for SPI around a month
>> ago where we wanted a rate strictly lower than the requested one. I
>> guess it's time to add a flag to tell how you want to round.
> 
> You are right, I just removed half of the constraint, but I still wonder
> why does this sequence introduced by the commit 862b728387aef3a37
> (clk: sunxi: factors: automatic reparenting support) do
> 	"provide the fastest rate <= rate"
> instead of
> 	"provide the closest rate" ?
> 
> Emilio?

Overclocking components is usually not a good default in my opinion. I
don't recall at the moment if there was some other justification apart
from playing it safe.

Cheers,
Emilio

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Maxime Ripard]

On Wed, Mar 30, 2016 at 05:49:47PM -0300, Emilio L?pez wrote:
> [Sorry for the delay, I meant to reply to this post a while back but I
> forgot]
> 
> El 21/03/16 a las 05:25, Jean-Francois Moine escribi?:
> > On Mon, 21 Mar 2016 08:25:46 +0100
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> >>> -	/* find the parent that can help provide the fastest rate <= rate */
> >>> +	/* find the parent that can help provide the fastest rate */
> >>>  	num_parents = clk_hw_get_num_parents(hw);
> >>>  	for (i = 0; i < num_parents; i++) {
> >>>  		parent = clk_hw_get_parent_by_index(hw, i);
> >>> @@ -100,7 +100,7 @@ static int clk_factors_determine_rate(struct clk_hw *hw,
> >>>  		child_rate = clk_factors_round_rate(hw, req->rate,
> >>>  						    &parent_rate);
> >>>  
> >>> -		if (child_rate <= req->rate && child_rate > best_child_rate) {
> >>> +		if (child_rate > best_child_rate) {
> >>
> >> I'm not sure this would work, since you'll end up picking the fastest
> >> rate without considering whether it is the closest or not.
> >>
> >> I guess what you want here is using the absolute difference between
> >> the requested rate and the rate you're evaluating.
> >>
> >> That being said, we had a similar discussion for SPI around a month
> >> ago where we wanted a rate strictly lower than the requested one. I
> >> guess it's time to add a flag to tell how you want to round.
> > 
> > You are right, I just removed half of the constraint, but I still wonder
> > why does this sequence introduced by the commit 862b728387aef3a37
> > (clk: sunxi: factors: automatic reparenting support) do
> > 	"provide the fastest rate <= rate"
> > instead of
> > 	"provide the closest rate" ?
> > 
> > Emilio?
> 
> Overclocking components is usually not a good default in my opinion. I
> don't recall at the moment if there was some other justification apart
> from playing it safe.

Yeah, I'd agree, it should be the exception rather than the norm. In
some cases that are very timings sensitive (audio or video), we
probably want to enforce something as close as possible to the
expected rate, even if it trips above the rate.

For all the other, I'd prefer to keep the current behaviour.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160414/c292b61a/attachment.sig>

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Jean-Francois Moine]

On Thu, 14 Apr 2016 19:24:00 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> > >> That being said, we had a similar discussion for SPI around a month
> > >> ago where we wanted a rate strictly lower than the requested one. I
> > >> guess it's time to add a flag to tell how you want to round.
> > > 
> > > You are right, I just removed half of the constraint, but I still wonder
> > > why does this sequence introduced by the commit 862b728387aef3a37
> > > (clk: sunxi: factors: automatic reparenting support) do
> > > 	"provide the fastest rate <= rate"
> > > instead of
> > > 	"provide the closest rate" ?
> > > 
> > > Emilio?
> > 
> > Overclocking components is usually not a good default in my opinion. I
> > don't recall at the moment if there was some other justification apart
> > from playing it safe.
> 
> Yeah, I'd agree, it should be the exception rather than the norm. In
> some cases that are very timings sensitive (audio or video), we
> probably want to enforce something as close as possible to the
> expected rate, even if it trips above the rate.
> 
> For all the other, I'd prefer to keep the current behaviour.

OK, then, as I have no solution for this clock, there will be no audio
44.1KHz from the H3...

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH] clk: sunxi: Accept a greater rate when setting a parent clock

[Maxime Ripard]

Hi,

On Thu, Apr 14, 2016 at 08:14:28PM +0200, Jean-Francois Moine wrote:
> On Thu, 14 Apr 2016 19:24:00 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > > >> That being said, we had a similar discussion for SPI around a month
> > > >> ago where we wanted a rate strictly lower than the requested one. I
> > > >> guess it's time to add a flag to tell how you want to round.
> > > > 
> > > > You are right, I just removed half of the constraint, but I still wonder
> > > > why does this sequence introduced by the commit 862b728387aef3a37
> > > > (clk: sunxi: factors: automatic reparenting support) do
> > > > 	"provide the fastest rate <= rate"
> > > > instead of
> > > > 	"provide the closest rate" ?
> > > > 
> > > > Emilio?
> > > 
> > > Overclocking components is usually not a good default in my opinion. I
> > > don't recall at the moment if there was some other justification apart
> > > from playing it safe.
> > 
> > Yeah, I'd agree, it should be the exception rather than the norm. In
> > some cases that are very timings sensitive (audio or video), we
> > probably want to enforce something as close as possible to the
> > expected rate, even if it trips above the rate.
> > 
> > For all the other, I'd prefer to keep the current behaviour.
> 
> OK, then, as I have no solution for this clock, there will be no audio
> 44.1KHz from the H3...

You have not read me. I already suggested in the quote above to add a
flag to tell how we should round the clock rate, the default remaining
to round it down.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160414/524b3ef2/attachment.sig>