| Message ID | 25a05161c25d59a153acde52f798e5fcf570bd41.1790993264.git.DaveWK225@protonmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tim Ouyang |
| Headers | show |
| Series | [v2] clk: spacemit: program the mux when setting a mix clock's rate | expand |
Hi Dave, On 02:39 Sat 03 Oct , Dave Klotz wrote: > ccu_mix_set_rate() picks the best parent and divider but writes only > the divider. U-Boot's CCF has no determine_rate step that would reparent > the clock first, so the clock keeps running from whatever source the > mux selects, at a rate the caller never asked for. Worse, when the > hardware mux selects a source that this build's parent list does not > contain (the K1 SPL lists only pll1_d6_409p6 for the SDH clocks), init > registers the clock under parents[0] and the clock framework's idea of > the parent no longer matches the hardware. > > Reparent the clock to the chosen parent with clk_set_parent() before > programming the divider, as the clock core does in Linux. Do so also > when the framework already has that parent but the mux selects another > source. Nothing is written when the mux and divider already match, so a > clock left running at the requested rate by an earlier stage is not > disturbed. > > Signed-off-by: Dave Klotz <DaveWK225@protonmail.com> > --- > Changes in v2: > - Reparent through clk_set_parent() before programming the divider, as > suggested by Yixun Lan. CCF enables the new parent only when the clock > itself is enabled. > - Reparent also when only the hardware mux disagrees with the chosen > parent, including the SPL's single-parent clock registration case. > - Share parent-index lookup with ccu_mux_set_parent(). Keep the no-write > case when the mux and divider already match. > > v1: https://lists.u-boot-project.org/pipermail/u-boot/2026-October/633805.html > > The parent/divider selection algorithm is unchanged from v1. This > revision changes how the selected parent is applied. > > Retested on 2026-10-02, with the integrated board-support lane on next > bd2f5794b7e2, OpenSBI v1.9, and "clk: spacemit: ccu_mix: fix inverted > condition in ccu_mix_trigger_fc()" from Yixun's k3/for-next tree: > - OrangePi R2S: eMMC SPL boot, SDH2 I/O clock 204.8 MHz instead of the > BootROM's 409.6 MHz; U-Boot HS200 at 200 MHz/8-bit, normal EFI and > FreeBSD boot, and a final unattended reboot. > - OrangePi RV2: microSD SPL boot, U-Boot SD High Speed at 50 MHz, EFI and > FreeBSD boot with root on NVMe; two boots checked. > > Both CPU clusters remain at 1600 MHz on both boards, with SSH working > following boot. The R2S, RV2, generic K1 and RV2 SPI configurations build > SPL and U-Boot proper without compiler warnings; checkpatch passes. > Generic K1 and RV2 SPI are BUILD-ONLY for this revision. No before/after > performance benchmarks were run. > > drivers/clk/spacemit/clk_mix.c | 79 ++++++++++++++++++++++++---------- > 1 file changed, 56 insertions(+), 23 deletions(-) > > diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c > index a1158512a9..fc8e0fd41d 100644 > --- a/drivers/clk/spacemit/clk_mix.c > +++ b/drivers/clk/spacemit/clk_mix.c > @@ -136,14 +136,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate, > return best_rate; > } > .. > +static u8 ccu_mux_get_parent(struct clk *clk) This problem isn't introduced in this patch, but while review it.. I have difficulty to understand the function name with what it exactly do here.. I think it trys to retrieve current parent index where the hardware programmed to, so I think it would be better to rename as ccu_mux_current_parent_index() or ccu_mux_get_parent_index() > +{ > + struct ccu_mix *mix = clk_to_ccu_mix(clk); > + struct ccu_mux_config *mux = &mix->mux; .. > + u8 parent; I'd like to simply name it as 'index' instead of 'parent', we already have "struct clk *parent" which will brings confusion.. > + > + parent = ccu_read(&mix->common, ctrl) >> mux->shift; > + parent &= (1 << mux->width) - 1; > + > + return parent; > +} > + > +static int ccu_mux_parent_index(struct ccu_common *common, struct clk *parent) same reasone, how about ccu_mux_wanted_parent_index()? it's a parent index that expected to program to the hardware.. > +{ > + int i; > + > + for (i = 0; i < common->num_parents; i++) > + if (!strcmp(parent->dev->name, common->parents[i])) > + return i; > + > + return -EINVAL; > +} > + The patch looks good to me, so with or without changes Reviewed-by: Yixun Lan <dlan@kernel.org> -- Yixun Lan (dlan)
Hi Yixun, Thanks for the review. I've prepared v3 with ccu_mux_current_parent_index() and ccu_mux_wanted_parent_index(), updated every caller, and renamed the hardware-index local to index. There are no functional changes from v2, and I've added your Reviewed-by. Exact-base replay and the naming-only comparison pass. Both board configurations build successfully. During testing I found a separate overlapping DDR firmware copy in SPL; I've sent that fix separately. With it included, the combined firmware passed DDR initialization, FIT verification and automatic EFI/FreeBSD boot on both the 2 GiB R2S and the 4 GiB RV2. These are combined-firmware warm-boot results, not isolated tests of the clock patch. Cold boot of the corrected firmware remains untested. Thanks, Dave
diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c index a1158512a9..fc8e0fd41d 100644 --- a/drivers/clk/spacemit/clk_mix.c +++ b/drivers/clk/spacemit/clk_mix.c @@ -136,14 +136,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate, return best_rate; } +static u8 ccu_mux_get_parent(struct clk *clk) +{ + struct ccu_mix *mix = clk_to_ccu_mix(clk); + struct ccu_mux_config *mux = &mix->mux; + u8 parent; + + parent = ccu_read(&mix->common, ctrl) >> mux->shift; + parent &= (1 << mux->width) - 1; + + return parent; +} + +static int ccu_mux_parent_index(struct ccu_common *common, struct clk *parent) +{ + int i; + + for (i = 0; i < common->num_parents; i++) + if (!strcmp(parent->dev->name, common->parents[i])) + return i; + + return -EINVAL; +} + +/* + * U-Boot's CCF has no determine_rate step, so the parent giving the best rate + * is chosen here. As in Linux, the clock is reparented first and the divider + * programmed after. Reparent also when the framework already has the chosen + * parent but the mux selects another source: init registers a clock under + * parents[0] when the mux selects a source this build does not list (the SPL + * lists only a subset). Nothing is written when mux and divider already match, + * so a clock left running by an earlier stage is not disturbed. + */ static unsigned long ccu_mix_set_rate(struct clk *clk, unsigned long rate) { struct ccu_mix *mix = clk_to_ccu_mix(clk); struct ccu_common *common = &mix->common; struct ccu_div_config *div = &mix->div; - u32 current_div, target_div, mask; - - ccu_mix_calc_best_rate(clk, rate, NULL, NULL, &target_div); + struct ccu_mux_config *mux = &mix->mux; + struct clk *parent = NULL; + unsigned long parent_rate; + u32 current_div, target_div = 0, mask; + int index, ret; + + ccu_mix_calc_best_rate(clk, rate, &parent, &parent_rate, &target_div); + + if (mux->width && parent) { + index = ccu_mux_parent_index(common, parent); + if (index < 0) + return index; + + if (clk->dev->parent != parent->dev || + ccu_mux_get_parent(clk) != index) { + ret = clk_set_parent(clk, parent); + if (ret) + return ret; + } + } current_div = ccu_read(common, ctrl) >> div->shift; current_div &= (1 << div->width) - 1; @@ -158,35 +207,19 @@ static unsigned long ccu_mix_set_rate(struct clk *clk, unsigned long rate) return ccu_mix_trigger_fc(clk); } -static u8 ccu_mux_get_parent(struct clk *clk) -{ - struct ccu_mix *mix = clk_to_ccu_mix(clk); - struct ccu_mux_config *mux = &mix->mux; - u8 parent; - - parent = ccu_read(&mix->common, ctrl) >> mux->shift; - parent &= (1 << mux->width) - 1; - - return parent; -} - static int ccu_mux_set_parent(struct clk *clk, struct clk *parent) { struct ccu_common *common = clk_to_ccu_common(clk); struct ccu_mix *mix = clk_to_ccu_mix(clk); struct ccu_mux_config *mux = &mix->mux; u32 mask; - int i = 0; + int i; mask = GENMASK(mux->width + mux->shift - 1, mux->shift); - for (i = 0; i < common->num_parents; i++) { - if (!strcmp(parent->dev->name, common->parents[i])) - break; - } - - if (i == common->num_parents) - return -EINVAL; + i = ccu_mux_parent_index(common, parent); + if (i < 0) + return i; ccu_update(&mix->common, ctrl, mask, i << mux->shift);
ccu_mix_set_rate() picks the best parent and divider but writes only the divider. U-Boot's CCF has no determine_rate step that would reparent the clock first, so the clock keeps running from whatever source the mux selects, at a rate the caller never asked for. Worse, when the hardware mux selects a source that this build's parent list does not contain (the K1 SPL lists only pll1_d6_409p6 for the SDH clocks), init registers the clock under parents[0] and the clock framework's idea of the parent no longer matches the hardware. Reparent the clock to the chosen parent with clk_set_parent() before programming the divider, as the clock core does in Linux. Do so also when the framework already has that parent but the mux selects another source. Nothing is written when the mux and divider already match, so a clock left running at the requested rate by an earlier stage is not disturbed. Signed-off-by: Dave Klotz <DaveWK225@protonmail.com> --- Changes in v2: - Reparent through clk_set_parent() before programming the divider, as suggested by Yixun Lan. CCF enables the new parent only when the clock itself is enabled. - Reparent also when only the hardware mux disagrees with the chosen parent, including the SPL's single-parent clock registration case. - Share parent-index lookup with ccu_mux_set_parent(). Keep the no-write case when the mux and divider already match. v1: https://lists.u-boot-project.org/pipermail/u-boot/2026-October/633805.html The parent/divider selection algorithm is unchanged from v1. This revision changes how the selected parent is applied. Retested on 2026-10-02, with the integrated board-support lane on next bd2f5794b7e2, OpenSBI v1.9, and "clk: spacemit: ccu_mix: fix inverted condition in ccu_mix_trigger_fc()" from Yixun's k3/for-next tree: - OrangePi R2S: eMMC SPL boot, SDH2 I/O clock 204.8 MHz instead of the BootROM's 409.6 MHz; U-Boot HS200 at 200 MHz/8-bit, normal EFI and FreeBSD boot, and a final unattended reboot. - OrangePi RV2: microSD SPL boot, U-Boot SD High Speed at 50 MHz, EFI and FreeBSD boot with root on NVMe; two boots checked. Both CPU clusters remain at 1600 MHz on both boards, with SSH working following boot. The R2S, RV2, generic K1 and RV2 SPI configurations build SPL and U-Boot proper without compiler warnings; checkpatch passes. Generic K1 and RV2 SPI are BUILD-ONLY for this revision. No before/after performance benchmarks were run. drivers/clk/spacemit/clk_mix.c | 79 ++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 23 deletions(-)