Re: [PATCH v2 1/3] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [High] `ccu_maskdiv_set_rate` recalculates a different mask factor (`M`) =
than what `ccu_maskdiv_determine_rate` initially chose, silently programmin=
g the hardware to a lower frequency than negotiated.
- [High] `ccu_maskdiv_determine_rate` ignores the `CLK_SET_RATE_NO_REPARENT=
` flag, potentially causing unintended clock reparenting.
--

--- Patch [1]: [PATCH v2 1/3] clk: sunxi-ng: add cycle-masking divider (mas=
kdiv) clock type ---
commit cf0833bab086c6beae6c2b221f361043441cc718
Author: Juan Manuel L=C3=B3pez Carrillo <[email protected]>

clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type

This commit adds a cycle-masking divider for the A523/T527 GPU clock.
It masks M pulses out of every 2^width parent cycles instead of using a
linear divider. It also adds custom determine_rate and set_rate functions
to support this behavior.

> diff --git a/drivers/clk/sunxi-ng/ccu_maskdiv.c b/drivers/clk/sunxi-ng/cc=
u_maskdiv.c
> new file mode 100644
> index 0000000000000..4ad49d51405b3
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_maskdiv.c

[ ... ]

> +static int ccu_maskdiv_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
> +{
> +	struct ccu_maskdiv *cmd =3D hw_to_ccu_maskdiv(hw);
> +	unsigned long best_rate =3D 0, best_parent_rate =3D 0;
> +	struct clk_hw *best_parent =3D NULL;
> +	unsigned int best_m =3D UINT_MAX;
> +	unsigned int i;
> +
> +	for (i =3D 0; i < clk_hw_get_num_parents(hw); i++) {
> +		struct clk_hw *parent =3D clk_hw_get_parent_by_index(hw, i);

[Severity: High]
Does this iteration need to respect the CLK_SET_RATE_NO_REPARENT flag?

Unlike other sunxi-ng multiplexers that use ccu_mux_helper_determine_rate(),
this loops unconditionally across all available parents. If a driver specif=
ies
CLK_SET_RATE_NO_REPARENT, would this silently ignore it and potentially cau=
se
unintended clock reparenting?

> +		unsigned long parent_rate, new_rate;
> +		unsigned int m;

[ ... ]

> +static int ccu_maskdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct ccu_maskdiv *cmd =3D hw_to_ccu_maskdiv(hw);
> +	unsigned int m;
> +	unsigned long flags;
> +	u32 reg;
> +
> +	m =3D ccu_maskdiv_find_m(parent_rate, rate, cmd->width);

[Severity: High]
Does ccu_maskdiv_set_rate() end up programming a different mask factor (M)
than what ccu_maskdiv_determine_rate() initially chose?

Because ccu_maskdiv_calc_rate() truncates the remainder (e.g. 1000 * 13 / 16
=3D 812), feeding the result back into ccu_maskdiv_find_m() evaluates this
truncated output again. For example, 812 * 16 / 1000 =3D 12, instead of the
original 13.

Could this inappropriately increase M and silently program the hardware to
a lower frequency than what was negotiated?

> +
> +	spin_lock_irqsave(cmd->common.lock, flags);

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803180755.2887=
[email protected]?part=3D1