| Message ID | 5aa2337bcb969568749c4728354164d5a6c88f27.1790975596.git.DaveWK225@protonmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tim Ouyang |
| Headers | show |
| Series | clk: spacemit: program the mux when setting a mix clock's rate | expand |
Also tested on the OrangePi R2S with upstream's K1 SDHCI driver from next: the SPL moves the eMMC io clock from 409.6 MHz to 204.8 MHz and loads U-Boot, which then runs the eMMC in HS200. The tree is upstream next plus the posted K1 fixes and board support that is not posted yet.
hi Dave, On 21:29 Fri 02 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. > > Write the chosen parent's mux index and the divider together and latch > both with one frequency change. Enable the new parent first and reparent > the clock device after, so the framework follows the hardware. 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> > --- > Tested on an OrangePi R2S booting from eMMC, where the SDH2 io clock > moves from the BootROM's 409.6 MHz to 204.8 MHz in the SPL, together > with "clk: spacemit: ccu_mix: fix inverted condition in > ccu_mix_trigger_fc()" from the K3 clock series, and on an OrangePi RV2 > booting from microSD. > > drivers/clk/spacemit/clk_mix.c | 54 +++++++++++++++++++++++++++++----- > 1 file changed, 46 insertions(+), 8 deletions(-) > > diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c > index a1158512a9..3521ab9374 100644 > --- a/drivers/clk/spacemit/clk_mix.c > +++ b/drivers/clk/spacemit/clk_mix.c > @@ -9,6 +9,7 @@ > */ > > #include <dm/device.h> > +#include <dm/device-internal.h> > #include <dm/uclass.h> > #include <div64.h> > #include <regmap.h> > @@ -136,26 +137,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate, > return best_rate; > } > > +/* > + * U-Boot's CCF has no determine_rate, so the parent is chosen here along with > + * the divider. The mux is programmed from the chosen parent rather than left > + * where it was found: init falls back to parents[0] when the hardware mux > + * selects a source this build does not list (the SPL lists only a subset), and > + * the clock would otherwise keep running from that source at a rate nobody > + * computed. Mux and divider are written together and latched by one frequency > + * change, and nothing is written when both 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; > + struct ccu_mux_config *mux = &mix->mux; > + struct clk *parent = NULL; > + unsigned long parent_rate; > + u32 target_div = 0, mask, val; > + int i, ret; > > - ccu_mix_calc_best_rate(clk, rate, NULL, NULL, &target_div); > + ccu_mix_calc_best_rate(clk, rate, &parent, &parent_rate, &target_div); > > - current_div = ccu_read(common, ctrl) >> div->shift; > - current_div &= (1 << div->width) - 1; > + mask = GENMASK(div->width + div->shift - 1, div->shift); > + val = target_div << div->shift; > + > + if (mux->width && parent) { > + for (i = 0; i < common->num_parents; i++) > + if (!strcmp(parent->dev->name, common->parents[i])) > + break; > + if (i == common->num_parents) > + return -EINVAL; > + mask |= GENMASK(mux->width + mux->shift - 1, mux->shift); > + val |= i << mux->shift; > + } else { > + parent = NULL; > + } > > - if (current_div == target_div) > + if ((ccu_read(common, ctrl) & mask) == val) > return 0; > > - mask = GENMASK(div->width + div->shift - 1, div->shift); > + if (parent && clk->dev->parent != parent->dev) { > + ret = clk_enable(parent); > + if (ret) > + return ret; > + } > .. this will enable parent, would be it ok to combine below device_reparent() together? to simply use clk_set_parent(clk, parent)? it make sense to me to reparent first then program the divider > - ccu_update(common, ctrl, mask, target_div << div->shift); > + ccu_update(common, ctrl, mask, val); > > - return ccu_mix_trigger_fc(clk); > + ret = ccu_mix_trigger_fc(clk); > + if (ret) > + return ret; > + .. > + if (parent && clk->dev->parent != parent->dev) > + return device_reparent(clk->dev, parent->dev); > + reparent clk > + return 0; > } >
Hi Yixun, On Sat, Oct 03, 2026 at 01:04:52AM +0000, Yixun Lan wrote: > .. this will enable parent, would be it ok to combine below > device_reparent() together? to simply use clk_set_parent(clk, > parent)? it make sense to me to reparent first then program > the divider Yes, that is better. clk_set_parent() only enables the new parent when the clock itself is enabled, which also fixes the unbalanced clk_enable() in this version, and it matches Linux, where the core calls ccu_mux_set_parent() and then ccu_mix_set_rate(), each latched by its own frequency change. The one difference from this version is that, between the two frequency changes, the clock briefly runs from the new parent with the old divider. Linux has the same window, so I'll keep it simple unless you would rather have the divider written first whenever that window would exceed both the old and the new rate. v2 now reparents through clk_set_parent() first, then writes the divider and triggers the frequency change, and still writes nothing when the mux and divider already match. It also reparents when the hardware mux selects a different source even if the framework already has the chosen parent. The parent/divider selection algorithm is unchanged. I have retested the integrated board-support lane on next bd2f5794b7e2, with OpenSBI v1.9 and your FC fix from k3/for-next: - OrangePi R2S: eMMC SPL boot, SDH2 I/O clock 204.8 MHz, U-Boot HS200 at 200 MHz/8-bit, EFI and FreeBSD boot, and an 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, and SSH works after boot. The R2S, RV2, generic K1 and RV2 SPI configurations build SPL and U-Boot proper without compiler warnings; the patch passes checkpatch. Generic K1 and RV2 SPI are build-only checks for this revision. I have not run before/after performance benchmarks. Thanks, Dave
diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c index a1158512a9..3521ab9374 100644 --- a/drivers/clk/spacemit/clk_mix.c +++ b/drivers/clk/spacemit/clk_mix.c @@ -9,6 +9,7 @@ */ #include <dm/device.h> +#include <dm/device-internal.h> #include <dm/uclass.h> #include <div64.h> #include <regmap.h> @@ -136,26 +137,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate, return best_rate; } +/* + * U-Boot's CCF has no determine_rate, so the parent is chosen here along with + * the divider. The mux is programmed from the chosen parent rather than left + * where it was found: init falls back to parents[0] when the hardware mux + * selects a source this build does not list (the SPL lists only a subset), and + * the clock would otherwise keep running from that source at a rate nobody + * computed. Mux and divider are written together and latched by one frequency + * change, and nothing is written when both 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; + struct ccu_mux_config *mux = &mix->mux; + struct clk *parent = NULL; + unsigned long parent_rate; + u32 target_div = 0, mask, val; + int i, ret; - ccu_mix_calc_best_rate(clk, rate, NULL, NULL, &target_div); + ccu_mix_calc_best_rate(clk, rate, &parent, &parent_rate, &target_div); - current_div = ccu_read(common, ctrl) >> div->shift; - current_div &= (1 << div->width) - 1; + mask = GENMASK(div->width + div->shift - 1, div->shift); + val = target_div << div->shift; + + if (mux->width && parent) { + for (i = 0; i < common->num_parents; i++) + if (!strcmp(parent->dev->name, common->parents[i])) + break; + if (i == common->num_parents) + return -EINVAL; + mask |= GENMASK(mux->width + mux->shift - 1, mux->shift); + val |= i << mux->shift; + } else { + parent = NULL; + } - if (current_div == target_div) + if ((ccu_read(common, ctrl) & mask) == val) return 0; - mask = GENMASK(div->width + div->shift - 1, div->shift); + if (parent && clk->dev->parent != parent->dev) { + ret = clk_enable(parent); + if (ret) + return ret; + } - ccu_update(common, ctrl, mask, target_div << div->shift); + ccu_update(common, ctrl, mask, val); - return ccu_mix_trigger_fc(clk); + ret = ccu_mix_trigger_fc(clk); + if (ret) + return ret; + + if (parent && clk->dev->parent != parent->dev) + return device_reparent(clk->dev, parent->dev); + + return 0; } static u8 ccu_mux_get_parent(struct clk *clk)
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. Write the chosen parent's mux index and the divider together and latch both with one frequency change. Enable the new parent first and reparent the clock device after, so the framework follows the hardware. 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> --- Tested on an OrangePi R2S booting from eMMC, where the SDH2 io clock moves from the BootROM's 409.6 MHz to 204.8 MHz in the SPL, together with "clk: spacemit: ccu_mix: fix inverted condition in ccu_mix_trigger_fc()" from the K3 clock series, and on an OrangePi RV2 booting from microSD. drivers/clk/spacemit/clk_mix.c | 54 +++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 8 deletions(-) base-commit: a000f2af240eaa996cec92f8d44bad96fdf3f898