| Message ID | 20210702225145.2643303-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: switch dividers to .determine_rate | expand |
Quoting Martin Blumenstingl (2021-07-02 15:51:40) > .determine_rate is meant to replace .round_rate. The former comes with a > benefit which is especially relevant on 32-bit systems: since > .determine_rate uses an "unsigned long" (compared to a "signed long" > which is used by .round_rate) the maximum value on 32-bit systems > increases from 2^31 (or approx. 2.14GHz) to 2^32 (or approx. 4.29GHz). > > Implement .determine_rate in addition to .round_rate so drivers that are > using clk_divider_{ro_,}ops can benefit from this by default. Keep the > .round_rate callback for now since some drivers rely on > clk_divider_ops.round_rate being implemented. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- Applied to clk-next
Hi, Am 03.07.21 um 00:51 schrieb Martin Blumenstingl: > .determine_rate is meant to replace .round_rate. The former comes with a > benefit which is especially relevant on 32-bit systems: since > .determine_rate uses an "unsigned long" (compared to a "signed long" > which is used by .round_rate) the maximum value on 32-bit systems > increases from 2^31 (or approx. 2.14GHz) to 2^32 (or approx. 4.29GHz). > > Implement .determine_rate in addition to .round_rate so drivers that are > using clk_divider_{ro_,}ops can benefit from this by default. Keep the > .round_rate callback for now since some drivers rely on > clk_divider_ops.round_rate being implemented. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> This commit breaks composite clocks with multiple parents, since it adds a determine_rate callback, which is preferred over clock_round_rate in clk_composite_determine_rate in clk-composite.c and the "best-parent"-determination is only done for clock_round_rate-op there. There is no "best-parent"-determination in determine_rate in clk-divider which clk-compsite seems to expect - nor any multiple parents handling at all. That means that the composite will always stay at the same/initial parent clock (from the mux), without ever changing it (even if necessary). This breaks lot of clocks for Rockchip which intensively uses composites, i.e. those clocks will always stay at the initial parent, which in some cases is the XTAL clock and I strongly guess it is the same for other platforms, which use composite clocks having more than one parent (e.g. mediatek, ti ...) Example (RK3399) clk_sdio is set (initialized) with XTAL (24 MHz) as parent in u-boot. It will always stay at this parent, even if the mmc driver sets a rate of 200 MHz (fails, as the nature of things), which should switch it to any of its possible parent PLLs defined in mux_pll_src_cpll_gpll_npll_ppll_upll_24m_p (see clk-rk3399.c) - which never happens. Reverting this commit makes it work again: Unless there is a quick and obvious fix for that, I guess this should be done for 5.15 - even if the real issue is somewhere else. Alex > --- > drivers/clk/clk-divider.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 87ba4966b0e8..f6b2bf558486 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -446,6 +446,27 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, > divider->width, divider->flags); > } > > +static int clk_divider_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > +{ > + struct clk_divider *divider = to_clk_divider(hw); > + > + /* if read only, just return current value */ > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > + u32 val; > + > + val = clk_div_readl(divider) >> divider->shift; > + val &= clk_div_mask(divider->width); > + > + return divider_ro_determine_rate(hw, req, divider->table, > + divider->width, > + divider->flags, val); > + } > + > + return divider_determine_rate(hw, req, divider->table, divider->width, > + divider->flags); > +} > + > int divider_get_val(unsigned long rate, unsigned long parent_rate, > const struct clk_div_table *table, u8 width, > unsigned long flags) > @@ -501,6 +522,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, > const struct clk_ops clk_divider_ops = { > .recalc_rate = clk_divider_recalc_rate, > .round_rate = clk_divider_round_rate, > + .determine_rate = clk_divider_determine_rate, > .set_rate = clk_divider_set_rate, > }; > EXPORT_SYMBOL_GPL(clk_divider_ops); > @@ -508,6 +530,7 @@ EXPORT_SYMBOL_GPL(clk_divider_ops); > const struct clk_ops clk_divider_ro_ops = { > .recalc_rate = clk_divider_recalc_rate, > .round_rate = clk_divider_round_rate, > + .determine_rate = clk_divider_determine_rate, > }; > EXPORT_SYMBOL_GPL(clk_divider_ro_ops); >
Hi Alex, On Thu, Oct 14, 2021 at 11:55 AM Alex Bee <knaerzche@gmail.com> wrote: [...] > This breaks lot of clocks for Rockchip which intensively uses > composites, i.e. those clocks will always stay at the initial parent, > which in some cases is the XTAL clock and I strongly guess it is the > same for other platforms, which use composite clocks having more than > one parent (e.g. mediatek, ti ...) Sorry for that and thanks for bisecting this! > Example (RK3399) > clk_sdio is set (initialized) with XTAL (24 MHz) as parent in u-boot. > It will always stay at this parent, even if the mmc driver sets a rate > of 200 MHz (fails, as the nature of things), which should switch it to > any of its possible parent PLLs defined in > mux_pll_src_cpll_gpll_npll_ppll_upll_24m_p (see clk-rk3399.c) - which > never happens. My question to Stephen et. al. is: where is the correct place to solve this? What I came up with so far (in no particular order): 1) not using clk-composite from clock drivers and letting CCF take care of re-parenting clocks as needed (and as specified with CLK_SET_RATE_NO_REPARENT) 2) clk-composite.c: extending the logic so "rate" clocks with .determine_rate include the existing logic which only applies to .round_rate (which means clk-composite.c is then responsible for finding the best possible parent clock) 3) clk-divider.c: extending the logic here to account for clk_hws with multiple parents For 3) I am wondering whether this would even work because it seems that clk-composite uses multiple struct clk_hw. Letting the divider handle multiple parents means it would need to know about the information which is only available in mux_hw - whereas clk-composite currently passes rate_hw (struct clk_hw for the divider). I am happy to work on a patch for this if I can get some help with testing (since I don't have any board with Rockchip SoC). > Reverting this commit makes it work again: Unless there is a quick and > obvious fix for that, I guess this should be done for 5.15 - even if the > real issue is somewhere else. Reverting this patch is fine from the Amlogic SoC point of view. The main goal was to clean up / improve the CCF code. Nothing (that I am aware of) is going to break in Amlogic land if we revert this. Best regards, Martin
On Thu, Oct 14, 2021 at 2:11 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: [...] > > Reverting this commit makes it work again: Unless there is a quick and > > obvious fix for that, I guess this should be done for 5.15 - even if the > > real issue is somewhere else. > Reverting this patch is fine from the Amlogic SoC point of view. > The main goal was to clean up / improve the CCF code. > Nothing (that I am aware of) is going to break in Amlogic land if we > revert this. Unfortunately only now I realized that reverting this patch would also require reverting the other five patches in this series (since they depend on this one). For this reason I propose changing the order of the checks in clk-composite.c - see the attached patch (which I can send as a proper one once agreed that this is the way to go forward) Off-list Alex also suggested that I should use rate_ops.determine_rate if available. While I agree that this makes sense in general my plan is to do this in a follow-up patch. Changing the order of the conditions is needed anyways and it *should* fix the issue reported here (but I have no way of testing that unfortunately). Alex, it would be great if you (or someone with Rockchip boards) could test the attached patch and let me know if it fixes the reported problem. Best regards, Martin
Quoting Martin Blumenstingl (2021-10-14 14:34:54) > On Thu, Oct 14, 2021 at 2:11 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] > > > Reverting this commit makes it work again: Unless there is a quick and > > > obvious fix for that, I guess this should be done for 5.15 - even if the > > > real issue is somewhere else. > > Reverting this patch is fine from the Amlogic SoC point of view. > > The main goal was to clean up / improve the CCF code. > > Nothing (that I am aware of) is going to break in Amlogic land if we > > revert this. > Unfortunately only now I realized that reverting this patch would also > require reverting the other five patches in this series (since they > depend on this one). > For this reason I propose changing the order of the checks in > clk-composite.c - see the attached patch (which I can send as a proper > one once agreed that this is the way to go forward) > > Off-list Alex also suggested that I should use rate_ops.determine_rate > if available. > While I agree that this makes sense in general my plan is to do this > in a follow-up patch. > Changing the order of the conditions is needed anyways and it *should* > fix the issue reported here (but I have no way of testing that > unfortunately). > > Alex, it would be great if you (or someone with Rockchip boards) could > test the attached patch and let me know if it fixes the reported > problem. > I can't read your attached patch. Please send it inline.
Am 14.10.21 um 23:34 schrieb Martin Blumenstingl: > On Thu, Oct 14, 2021 at 2:11 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] >>> Reverting this commit makes it work again: Unless there is a quick and >>> obvious fix for that, I guess this should be done for 5.15 - even if the >>> real issue is somewhere else. >> Reverting this patch is fine from the Amlogic SoC point of view. >> The main goal was to clean up / improve the CCF code. >> Nothing (that I am aware of) is going to break in Amlogic land if we >> revert this. > Unfortunately only now I realized that reverting this patch would also > require reverting the other five patches in this series (since they > depend on this one). Indeed, that whole series would need have to reverted, which is clear (to me) when looking at the patches. > For this reason I propose changing the order of the checks in > clk-composite.c - see the attached patch (which I can send as a proper > one once agreed that this is the way to go forward) Yes, your patch papers over the actual issue (no best parent-selection in case determine_rate is used) and fixes it for now - as expected. But it is not a long-term solution, as we will be at the same point, as soon as round_rate gets removed from clk-divider. For me, who is semi-knowledged in clock-framework, it was relatively hard to figure out what was going on. "I'll do this later" has often been heard here. Though, I don't fully follow why moving the priority of determine_rate lower would have been necessary anyways: from what I understand determine_rate is expected to be the future-replacement of round_rate everywhere and should be preferred. I guess it's up to the maintainers on how to proceed. Alex > Off-list Alex also suggested that I should use rate_ops.determine_rate > if available. > While I agree that this makes sense in general my plan is to do this > in a follow-up patch. > Changing the order of the conditions is needed anyways and it *should* > fix the issue reported here (but I have no way of testing that > unfortunately). > > Alex, it would be great if you (or someone with Rockchip boards) could > test the attached patch and let me know if it fixes the reported > problem. > > > Best regards, > Martin
Quoting Alex Bee (2021-10-15 12:14:36) > > Am 14.10.21 um 23:34 schrieb Martin Blumenstingl: > > On Thu, Oct 14, 2021 at 2:11 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > [...] > >>> Reverting this commit makes it work again: Unless there is a quick and > >>> obvious fix for that, I guess this should be done for 5.15 - even if the > >>> real issue is somewhere else. > >> Reverting this patch is fine from the Amlogic SoC point of view. > >> The main goal was to clean up / improve the CCF code. > >> Nothing (that I am aware of) is going to break in Amlogic land if we > >> revert this. > > Unfortunately only now I realized that reverting this patch would also > > require reverting the other five patches in this series (since they > > depend on this one). > Indeed, that whole series would need have to reverted, which is clear > (to me) when looking at the patches. > > For this reason I propose changing the order of the checks in > > clk-composite.c - see the attached patch (which I can send as a proper > > one once agreed that this is the way to go forward) > > Yes, your patch papers over the actual issue (no best parent-selection > in case determine_rate is used) and fixes it for now - as expected. > > But it is not a long-term solution, as we will be at the same point, as > soon as round_rate gets removed from clk-divider. For me, who is > semi-knowledged in clock-framework, it was relatively hard to figure out > what was going on. "I'll do this later" has often been heard here. > > Though, I don't fully follow why moving the priority of determine_rate > lower would have been necessary anyways: from what I understand > determine_rate is expected to be the future-replacement of round_rate > everywhere and should be preferred. This is the only place I can see where we would have to keep round_rate around, to mesh together rate rounding with parent selection. We can probably stop using round_rate in the framework and only use it in the composite code. It's not a big deal but it's sort of annoying that we won't be able to fully remove round_rate from the clk_ops without resolving this. Long term it may make sense to get rid of the composite clk code entirely. It's pretty confusing code to read and encourages use of the "basic clk types" which I'd like to see be used via direct function calls instead of through clk_ops structures.
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 87ba4966b0e8..f6b2bf558486 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -446,6 +446,27 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate, divider->width, divider->flags); } +static int clk_divider_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_divider *divider = to_clk_divider(hw); + + /* if read only, just return current value */ + if (divider->flags & CLK_DIVIDER_READ_ONLY) { + u32 val; + + val = clk_div_readl(divider) >> divider->shift; + val &= clk_div_mask(divider->width); + + return divider_ro_determine_rate(hw, req, divider->table, + divider->width, + divider->flags, val); + } + + return divider_determine_rate(hw, req, divider->table, divider->width, + divider->flags); +} + int divider_get_val(unsigned long rate, unsigned long parent_rate, const struct clk_div_table *table, u8 width, unsigned long flags) @@ -501,6 +522,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate, const struct clk_ops clk_divider_ops = { .recalc_rate = clk_divider_recalc_rate, .round_rate = clk_divider_round_rate, + .determine_rate = clk_divider_determine_rate, .set_rate = clk_divider_set_rate, }; EXPORT_SYMBOL_GPL(clk_divider_ops); @@ -508,6 +530,7 @@ EXPORT_SYMBOL_GPL(clk_divider_ops); const struct clk_ops clk_divider_ro_ops = { .recalc_rate = clk_divider_recalc_rate, .round_rate = clk_divider_round_rate, + .determine_rate = clk_divider_determine_rate, }; EXPORT_SYMBOL_GPL(clk_divider_ro_ops);
.determine_rate is meant to replace .round_rate. The former comes with a benefit which is especially relevant on 32-bit systems: since .determine_rate uses an "unsigned long" (compared to a "signed long" which is used by .round_rate) the maximum value on 32-bit systems increases from 2^31 (or approx. 2.14GHz) to 2^32 (or approx. 4.29GHz). Implement .determine_rate in addition to .round_rate so drivers that are using clk_divider_{ro_,}ops can benefit from this by default. Keep the .round_rate callback for now since some drivers rely on clk_divider_ops.round_rate being implemented. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/clk/clk-divider.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)