Re: [PATCH] clk: sunxi-ng: div: implement set_rate_and_parent

[Chen-Yu Tsai]

On Sun, Jul 19, 2026 at 11:31 PM Aureal
<juanmanuellopezcarrillo@xxxxxxxxx> wrote:
>
> From: Juan Manuel López Carrillo <juanmanuellopezcarrillo@xxxxxxxxx>
>
> When a rate change on a ccu_div clock also switches its parent, the
> clk core, in the absence of a .set_rate_and_parent op, programs the
> parent first and the divider second.  If the new parent is faster than
> the old one, the clock transiently runs at new_parent_rate/old_divider
> between the two register writes, overshooting both the old and the
> requested rate and potentially violating the consumer's maximum
> allowed frequency.
>
> This is not theoretical.  On the Allwinner A523/T527 the GPU clock is
> a ccu_div muxing between pll-gpu and the fixed pll-periph0 outputs:
> going from 600 MHz (pll-periph0-600M, M=1) down to 400 or 200 MHz
> (both derived from pll-periph0-800M) makes the Mali G57 run at 800 MHz
> for the window between the two writes, 33% above the vendor's maximum
> operating point.  Observed and validated on an Orange Pi 4A (T527)
> with a downstream GPU devfreq setup; mainline does not yet describe
> GPU OPPs for this SoC, but any ccu_div consumer whose set_rate ends up
> crossing parents is affected.
>
> Implement .set_rate_and_parent with the same ordering rule as
> clk_composite_set_rate_and_parent(): if keeping the current divider
> while switching the mux would overshoot the requested rate, program
> the divider first, otherwise switch the mux first.  The intermediate
> rate then never exceeds both the old and the new rate.
>
> Clocks using a prediv feature keep the historical mux-then-divider
> order: the prediv helpers look the prediv up by the current parent,
> which is ambiguous while both are changing.
>
> Fixes: e9b93213103f ("clk: sunxi-ng: Add divider")
> Signed-off-by: Juan Manuel López Carrillo <juanmanuellopezcarrillo@xxxxxxxxx>

Reviewed-by: Chen-Yu Tsai <wens@xxxxxxxxxx>

So the patch itself is OK, but I think there is also something wrong
with the GPU mod clock. The GPU clock is not a standard divider, but
actually a fractional one. The formula for the output is

    Clock Source * ((16-M)/16)

with M being 0~15.

And we're also missing a notifier to switch the GPU clock away from
the PLL while the PLL is being changed.


Do you plan on implementing GPU DVFS on mainline?

> ---
> Note: this touches the same area of ccu_div.c as patch 2/4 of the
> pending series "clk: sun6i-rtc: Add support for Allwinner A733 SoC" v5
> (<20260717-a733-rtc-v5-2-3874cc26abf7@xxxxxxxxxxxx>), which adds
> ccu_rodiv_ops right after ccu_div_ops.  The conflict is trivial
> (context only); happy to rebase on top of it if it lands first.

That is manageable. Thanks for the heads up.


ChenYu

>
>  drivers/clk/sunxi-ng/ccu_div.c | 36 ++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> index 62d680ccb..9024bcd8c 100644
> --- a/drivers/clk/sunxi-ng/ccu_div.c
> +++ b/drivers/clk/sunxi-ng/ccu_div.c
> @@ -130,6 +130,41 @@ static int ccu_div_set_parent(struct clk_hw *hw, u8 index)
>         return ccu_mux_helper_set_parent(&cd->common, &cd->mux, index);
>  }
>
> +static int ccu_div_set_rate_and_parent(struct clk_hw *hw, unsigned long rate,
> +                                      unsigned long parent_rate, u8 index)
> +{
> +       struct ccu_div *cd = hw_to_ccu_div(hw);
> +
> +       /*
> +        * The predivider helpers look it up through the current parent,
> +        * which is ambiguous while both the parent and the divider are
> +        * changing, so keep the mux-then-divider order the core would
> +        * have used for those clocks.
> +        */
> +       if (cd->common.features & (CCU_FEATURE_VARIABLE_PREDIV |
> +                                  CCU_FEATURE_FIXED_PREDIV |
> +                                  CCU_FEATURE_ALL_PREDIV)) {
> +               ccu_div_set_parent(hw, index);
> +               return ccu_div_set_rate(hw, rate, parent_rate);
> +       }
> +
> +       /*
> +        * Same ordering rule as clk_composite_set_rate_and_parent(): if
> +        * switching the mux with the current divider would overshoot the
> +        * requested rate, program the divider first, so the intermediate
> +        * rate never exceeds both the old and the new rate.
> +        */
> +       if (ccu_div_recalc_rate(hw, parent_rate) > rate) {
> +               ccu_div_set_rate(hw, rate, parent_rate);
> +               ccu_div_set_parent(hw, index);
> +       } else {
> +               ccu_div_set_parent(hw, index);
> +               ccu_div_set_rate(hw, rate, parent_rate);
> +       }
> +
> +       return 0;
> +}
> +
>  const struct clk_ops ccu_div_ops = {
>         .disable        = ccu_div_disable,
>         .enable         = ccu_div_enable,
> @@ -141,5 +176,6 @@ const struct clk_ops ccu_div_ops = {
>         .determine_rate = ccu_div_determine_rate,
>         .recalc_rate    = ccu_div_recalc_rate,
>         .set_rate       = ccu_div_set_rate,
> +       .set_rate_and_parent = ccu_div_set_rate_and_parent,
>  };
>  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
>
> base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
> --
> 2.47.3
>