[v5,2/4] clk: sunxi-ng: div: add read-only operation support

Add support for sunxi-ng read-only dividers. This will be
useful to the a733 oscillator detection logic.

Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/sunxi-ng/ccu_div.c | 30 ++++++++++++++++++++++++++++--
 drivers/clk/sunxi-ng/ccu_div.h |  1 +
 drivers/clk/sunxi-ng/ccu_mux.c |  2 +-
 drivers/clk/sunxi-ng/ccu_mux.h |  4 ++++
 4 files changed, 34 insertions(+), 3 deletions(-)
  

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing `.set_parent` in `ccu_rodiv_ops` leads to silent software-hardware clock tree desynchronization during reparenting.

Pre-existing issues:
- [High] Unvalidated hardware register read passed to `divider_ro_determine_rate()` can cause a divide-by-zero kernel panic.
--

--- Patch [2]: [PATCH v5 2/4] clk: sunxi-ng: div: add read-only operation support ---
commit d732b1bc291c593bb4873ef5f04edb2855137f30
Author: Jerome Brunet <jbrunet@baylibre.com>

clk: sunxi-ng: div: add read-only operation support

This commit adds support for read-only dividers in the sunxi-ng clock
driver. It introduces a new ccu_rodiv_ops structure and updates the rate
determination helper to handle the read-only flag.

> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> index 62d680ccb524b..93cb4cb93abe8 100644
> --- a/drivers/clk/sunxi-ng/ccu_div.c
> +++ b/drivers/clk/sunxi-ng/ccu_div.c
> @@ -20,8 +20,22 @@ static int ccu_div_determine_rate_helper(struct ccu_mux_internal *mux,
>  	if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
>  		req->rate *= cd->fixed_post_div;
>  
> -	ret = divider_determine_rate(&cd->common.hw, req, cd->div.table,
> -				     cd->div.width, cd->div.flags);
> +	if (cd->div.flags & CLK_DIVIDER_READ_ONLY) {
> +		unsigned long val;
> +		u32 reg;
> +
> +		reg = readl(cd->common.base + cd->common.reg);
> +		val = reg >> cd->div.shift;
> +		val &= (1 << cd->div.width) - 1;
> +
> +		ret = divider_ro_determine_rate(&cd->common.hw, req, cd->div.table,
> +						cd->div.width, cd->div.flags, val);

[Severity: High]
This is a pre-existing issue, but could this unvalidated register read cause 
a divide-by-zero kernel panic?

In ccu_div_determine_rate_helper(), the raw register value is read and
passed as 'val' to divider_ro_determine_rate(). If the register contains
an unexpected value from the bootloader (or 0 when CLK_DIVIDER_ONE_BASED
is set) that _get_div() maps to 0, divider_ro_determine_rate() lacks a 
zero-check before calculating:

    req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);

> +
> +	} else {

[ ... ]

> @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
>  	.set_rate	= ccu_div_set_rate,
>  };
>  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
> +
> +const struct clk_ops ccu_rodiv_ops = {
> +	.disable	= ccu_div_disable,
> +	.enable		= ccu_div_enable,
> +	.is_enabled	= ccu_div_is_enabled,
> +
> +	.get_parent	= ccu_div_get_parent,
> +
> +	.determine_rate	= ccu_div_determine_rate,
> +	.recalc_rate	= ccu_div_recalc_rate,
> +};

[Severity: High]
Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
desynchronization between the software clock tree and the hardware?

The struct provides reparenting capabilities via .get_parent and
.determine_rate, but omits .set_parent. If a rate request causes
ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
Common Clock Framework will apply this new parent in software but skip
updating the hardware since .set_parent is NULL. 

This would leave all downstream clock rate calculations incorrect.
  

On Fri, Jul 17, 2026 at 11:40 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Missing `.set_parent` in `ccu_rodiv_ops` leads to silent software-hardware clock tree desynchronization during reparenting.
>
> Pre-existing issues:
> - [High] Unvalidated hardware register read passed to `divider_ro_determine_rate()` can cause a divide-by-zero kernel panic.
> --
>
> --- Patch [2]: [PATCH v5 2/4] clk: sunxi-ng: div: add read-only operation support ---
> commit d732b1bc291c593bb4873ef5f04edb2855137f30
> Author: Jerome Brunet <jbrunet@baylibre.com>
>
> clk: sunxi-ng: div: add read-only operation support
>
> This commit adds support for read-only dividers in the sunxi-ng clock
> driver. It introduces a new ccu_rodiv_ops structure and updates the rate
> determination helper to handle the read-only flag.
>
> > diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
> > index 62d680ccb524b..93cb4cb93abe8 100644
> > --- a/drivers/clk/sunxi-ng/ccu_div.c
> > +++ b/drivers/clk/sunxi-ng/ccu_div.c
> > @@ -20,8 +20,22 @@ static int ccu_div_determine_rate_helper(struct ccu_mux_internal *mux,
> >       if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
> >               req->rate *= cd->fixed_post_div;
> >
> > -     ret = divider_determine_rate(&cd->common.hw, req, cd->div.table,
> > -                                  cd->div.width, cd->div.flags);
> > +     if (cd->div.flags & CLK_DIVIDER_READ_ONLY) {
> > +             unsigned long val;
> > +             u32 reg;
> > +
> > +             reg = readl(cd->common.base + cd->common.reg);
> > +             val = reg >> cd->div.shift;
> > +             val &= (1 << cd->div.width) - 1;
> > +
> > +             ret = divider_ro_determine_rate(&cd->common.hw, req, cd->div.table,
> > +                                             cd->div.width, cd->div.flags, val);
>
> [Severity: High]
> This is a pre-existing issue, but could this unvalidated register read cause
> a divide-by-zero kernel panic?
>
> In ccu_div_determine_rate_helper(), the raw register value is read and
> passed as 'val' to divider_ro_determine_rate(). If the register contains
> an unexpected value from the bootloader (or 0 when CLK_DIVIDER_ONE_BASED
> is set) that _get_div() maps to 0, divider_ro_determine_rate() lacks a
> zero-check before calculating:
>
>     req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);

Well that would be either an invalid hardware state or unpowered / gated
hardware. Neither is correct and should never happen and blowing up is
the correct response.

In any case we don't have CLK_DIVIDER_ONE_BASED dividers so we are fine.

> > +
> > +     } else {
>
> [ ... ]
>
> > @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
> >       .set_rate       = ccu_div_set_rate,
> >  };
> >  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
> > +
> > +const struct clk_ops ccu_rodiv_ops = {
> > +     .disable        = ccu_div_disable,
> > +     .enable         = ccu_div_enable,
> > +     .is_enabled     = ccu_div_is_enabled,
> > +
> > +     .get_parent     = ccu_div_get_parent,
> > +
> > +     .determine_rate = ccu_div_determine_rate,
> > +     .recalc_rate    = ccu_div_recalc_rate,
> > +};
>
> [Severity: High]
> Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
> desynchronization between the software clock tree and the hardware?
>
> The struct provides reparenting capabilities via .get_parent and
> .determine_rate, but omits .set_parent. If a rate request causes
> ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
> Common Clock Framework will apply this new parent in software but skip
> updating the hardware since .set_parent is NULL.

This is an issue. I'm not sure if what Sashiko says actually happens.
But 1. this is "read-only divider", not "read-only mux & divider",
so the .set_parent callback should be provided. And 2. this is using
the ccu_mux_determine_rate_helper, so it's possible a clk_set_rate()
call is going to cause a reparent.

I can add it while applying if there are no other issues.

ChenYu

> This would leave all downstream clock rate calculations incorrect.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a733-rtc-v5-0-3874cc26abf7@baylibre.com?part=2
>
  

On dim. 19 juil. 2026 at 18:59, Chen-Yu Tsai <wens@kernel.org> wrote:

>
>> > +
>> > +     } else {
>>
>> [ ... ]
>>
>> > @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
>> >       .set_rate       = ccu_div_set_rate,
>> >  };
>> >  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
>> > +
>> > +const struct clk_ops ccu_rodiv_ops = {
>> > +     .disable        = ccu_div_disable,
>> > +     .enable         = ccu_div_enable,
>> > +     .is_enabled     = ccu_div_is_enabled,
>> > +
>> > +     .get_parent     = ccu_div_get_parent,
>> > +
>> > +     .determine_rate = ccu_div_determine_rate,
>> > +     .recalc_rate    = ccu_div_recalc_rate,
>> > +};
>>
>> [Severity: High]
>> Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
>> desynchronization between the software clock tree and the hardware?
>>
>> The struct provides reparenting capabilities via .get_parent and
>> .determine_rate, but omits .set_parent. If a rate request causes
>> ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
>> Common Clock Framework will apply this new parent in software but skip
>> updating the hardware since .set_parent is NULL.
>
> This is an issue. I'm not sure if what Sashiko says actually happens.

I think it could. I've defenitely made a mistake here.

> But 1. this is "read-only divider", not "read-only mux & divider",

The correct way to choose between the 2 is CLK_SET_RATE_NO_REPARENT I think.

> so the .set_parent callback should be provided. And 2. this is using
> the ccu_mux_determine_rate_helper, so it's possible a clk_set_rate()
> call is going to cause a reparent.
>
> I can add it while applying if there are no other issues.

As you prefer, I don't mind sending another version in a few days
(giving some review time to patch #1)

>
> ChenYu
>
>> This would leave all downstream clock rate calculations incorrect.
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a733-rtc-v5-0-3874cc26abf7@baylibre.com?part=2
>>
  

On Sun, Jul 19, 2026 at 9:29 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On dim. 19 juil. 2026 at 18:59, Chen-Yu Tsai <wens@kernel.org> wrote:
>
> >
> >> > +
> >> > +     } else {
> >>
> >> [ ... ]
> >>
> >> > @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
> >> >       .set_rate       = ccu_div_set_rate,
> >> >  };
> >> >  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
> >> > +
> >> > +const struct clk_ops ccu_rodiv_ops = {
> >> > +     .disable        = ccu_div_disable,
> >> > +     .enable         = ccu_div_enable,
> >> > +     .is_enabled     = ccu_div_is_enabled,
> >> > +
> >> > +     .get_parent     = ccu_div_get_parent,
> >> > +
> >> > +     .determine_rate = ccu_div_determine_rate,
> >> > +     .recalc_rate    = ccu_div_recalc_rate,
> >> > +};
> >>
> >> [Severity: High]
> >> Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
> >> desynchronization between the software clock tree and the hardware?
> >>
> >> The struct provides reparenting capabilities via .get_parent and
> >> .determine_rate, but omits .set_parent. If a rate request causes
> >> ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
> >> Common Clock Framework will apply this new parent in software but skip
> >> updating the hardware since .set_parent is NULL.
> >
> > This is an issue. I'm not sure if what Sashiko says actually happens.
>
> I think it could. I've defenitely made a mistake here.
>
> > But 1. this is "read-only divider", not "read-only mux & divider",
>
> The correct way to choose between the 2 is CLK_SET_RATE_NO_REPARENT I think.

For standard muxes, just dropping the .set_parent and .determine_rate
callbacks works. The core will just pass the determine_rate request
to the current parent. In fact that is what the basic clk-mux does.
However we have to deal with the pre-dividers, so we need the
.determine_rate callback.

On the other hand CLK_SET_RATE_NO_REPARENT just says that the rate change
cannot reparent the clock. And it requires the ops to actually check for
it. (In the past we didn't always check ...). So we should still drop the
.set_parent op for clarity.

But I think the main thing is that the naming and your intention in
this patch is that the divider is read-only, not the mux.

> > so the .set_parent callback should be provided. And 2. this is using
> > the ccu_mux_determine_rate_helper, so it's possible a clk_set_rate()
> > call is going to cause a reparent.
> >
> > I can add it while applying if there are no other issues.
>
> As you prefer, I don't mind sending another version in a few days
> (giving some review time to patch #1)

That also works if you want to do it. It does help with preserving
history.


Thanks
ChenYu