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 <[email protected]> > --- > 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; > } > -- Yixun Lan (dlan)
