clk: sunxi: Accept a greater rate when setting a parent clock
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Hi,
Quoted 39 lines · 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 <[email protected]>---
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(-)drivers/clk/sunxi/clk-factors.c +1
-1@@ -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);
drivers/clk/sunxi/clk-factors.c static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 5 lines
drivers/clk/sunxi/clk-factors.c +1
-1@@ -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
Signature · 10 lines
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 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Quoted 15 lines · On Mon, 21 Mar 2016 08:25:46 +0100 Maxime Ripard…
Review static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 11 lines
(quoted diff) +2
-2- /* 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.
Quoted 6 lines
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?
Signature · 2 lines
Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
PATCH 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Hi,
Quoted 25 lines · Jean-Francois Moine
Quoted 15 lines · On Mon, 21 Mar 2016 08:25:46 +0100 Maxime Ripard…
Review static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 11 lines
(quoted diff) +2
-2- /* 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.
Quoted 6 lines
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.
Quoted 6 lines
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
Signature · 10 lines
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 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Quoted 35 lines · On Tue, 29 Mar 2016 11:38:27 +0200 Maxime Ripard…
Hi,
Quoted 25 lines · Jean-Francois Moine
Quoted 15 lines · On Mon, 21 Mar 2016 08:25:46 +0100 Maxime Ripard…
Review static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 11 lines
(quoted diff) +2
-2- /* 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.
Quoted 6 lines
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.
Quoted 6 lines
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" ?
Quoted 4 lines
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"?
Signature · 2 lines
Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
PATCH 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
In reply to Jean-Francois Moine[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?:
Quoted 25 lines
Quoted 15 lines · On Mon, 21 Mar 2016 08:25:46 +0100 Maxime Ripard…
Review static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 11 lines
(quoted diff) +2
-2- /* 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.
Quoted 6 lines
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.
Quoted 8 lines
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 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Quoted 36 lines · 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?:
Quoted 32 lines
Quoted 15 lines · On Mon, 21 Mar 2016 08:25:46 +0100 Maxime Ripard…
Review static int clk_factors_determine_rate(struct clk_hw *hw, · line 103
Quoted 11 lines
(quoted diff) +2
-2- /* 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.
Quoted 6 lines
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" ?
Quoted 5 lines
Quoted 1 line
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
Signature · 10 lines
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 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Quoted 17 lines · On Thu, 14 Apr 2016 19:24:00 +0200 Maxime Ripard…
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
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" ?Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
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...
Signature · 2 lines
Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
PATCH 1/1 clk: sunxi: Accept a greater rate when setting a parent clock
Hi,
Quoted 25 lines · Jean-Francois Moine
Quoted 22 lines · On Thu, 14 Apr 2016 19:24:00 +0200 Maxime Ripard…
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
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" ?Quoted 5 lines
Quoted 1 line
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.
Quoted 4 lines
Quoted 1 line
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
Signature · 10 lines
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>