[PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[Enzo Adriano]

Some sunxi-ng MP clocks have an M divider but no P divider. The A523
MBUS, IOMMU and DRAM clocks use this layout and also require the update
bit when changing their rate.

ccu_mp_set_rate() unconditionally builds and applies a mask for the P
field. With a zero-width P field this produces an invalid GENMASK()
range and can clear bits outside a P divider, including the clock gate.

The callback also ignores CCU_FEATURE_UPDATE_BIT, so hardware that
requires the update bit may not latch the new divider value.

Only update the P field when it exists, and set CCU_SUNXI_UPDATE_BIT for
MP clocks carrying the feature. This matches the existing sunxi-ng div,
mux and gate helper behavior.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/20260712081341.9D1431F00A3D@smtp.kernel.org
Fixes: 6702d17f54a8 ("clk: sunxi-ng: a523: add video mod clocks")
Assisted-by: Codex:gpt-5
Signed-off-by: Enzo Adriano <enzo.adriano.code@gmail.com>
---
Based on clk-next 8cdeaa50eae8 (Linux 7.2-rc2).
Tested with strict checkpatch and an arm64 W=1 build of ccu_mp.o.
No hardware runtime claim is made.

 drivers/clk/sunxi-ng/ccu_mp.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
index 7cdb0eedc69b..aa6cb20447f1 100644
--- a/drivers/clk/sunxi-ng/ccu_mp.c
+++ b/drivers/clk/sunxi-ng/ccu_mp.c
@@ -237,12 +237,17 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	reg = readl(cmp->common.base + cmp->common.reg);
 	reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
-	reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
+	if (cmp->p.width)
+		reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
+	if (cmp->common.features & CCU_FEATURE_UPDATE_BIT)
+		reg |= CCU_SUNXI_UPDATE_BIT;
 	reg |= (m - cmp->m.offset) << cmp->m.shift;
-	if (shift)
-		reg |= ilog2(p) << cmp->p.shift;
-	else
-		reg |= (p - cmp->p.offset) << cmp->p.shift;
+	if (cmp->p.width) {
+		if (shift)
+			reg |= ilog2(p) << cmp->p.shift;
+		else
+			reg |= (p - cmp->p.offset) << cmp->p.shift;
+	}
 
 	writel(reg, cmp->common.base + cmp->common.reg);
 
-- 
2.53.0

Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch resolves an invalid GENMASK evaluation for 0-width P dividers but fails to apply the same check for 0-width M dividers, leaving a register corruption bug intact.
- [High] When an unreachable low clock rate is requested, `ccu_mp_find_best` fails to find a suitable divider and returns `m = 0` and `p = 0`, leading to integer underflows that severely corrupt the CCU hardware register.
--

--- Patch [1]: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers ---
commit e8027d463bc82dfb3ec7ba4b45f1688fa59d1694
Author: Enzo Adriano <enzo.adriano.code@gmail.com>

clk: sunxi-ng: ccu_mp: fix clocks without P dividers

This commit modifies the sunxi-ng MP clock driver to conditionally apply the
P divider mask only when the P field is present. It also ensures the update
bit is set if required by the hardware. This prevents invalid GENMASK
evaluations and missing hardware updates.

Closes: https://lore.kernel.org/r/20260712081341.9D1431F00A3D@smtp.kernel.org

> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
> index 7cdb0eedc69b5..aa6cb20447f19 100644
> --- a/drivers/clk/sunxi-ng/ccu_mp.c
> +++ b/drivers/clk/sunxi-ng/ccu_mp.c
> @@ -237,12 +237,17 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
>  
>  	reg = readl(cmp->common.base + cmp->common.reg);
>  	reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);

[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing the
invalid GENMASK for 0-width P dividers, should we apply the same check for
M dividers?

If an MP clock has no M divider (m.width == 0), the GENMASK will underflow
and produce an all-ones mask. The ~GENMASK operation then zeroes out the
entire hardware register, clearing critical fields like the clock gate.

> -	reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->p.width)
> +		reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->common.features & CCU_FEATURE_UPDATE_BIT)
> +		reg |= CCU_SUNXI_UPDATE_BIT;
>  	reg |= (m - cmp->m.offset) << cmp->m.shift;

[Severity: High]
This is a pre-existing issue, but can this operation corrupt the hardware
register if an extremely low rate is requested?

If a driver or user requests a rate lower than the minimum achievable rate,
ccu_mp_find_best() will skip all rates:

drivers/clk/sunxi-ng/ccu_mp.c:ccu_mp_find_best() {
    ...
	if (tmp_rate > rate)
		continue;
    ...
}

This causes it to return the initial values for m and p as 0. Back in
ccu_mp_set_rate(), an m of 0 will cause m - cmp->m.offset to underflow
(for example, 0 - 1 = 0xFFFFFFFF). This value is then left-shifted and
OR'd into the register, overwriting adjacent hardware fields with 1s.

> -	if (shift)
> -		reg |= ilog2(p) << cmp->p.shift;
> -	else
> -		reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	if (cmp->p.width) {
> +		if (shift)
> +			reg |= ilog2(p) << cmp->p.shift;
> +		else
> +			reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	}
>  
>  	writel(reg, cmp->common.base + cmp->common.reg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722005643.1208942-1-enzo.adriano.code@gmail.com?part=1

Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[Andre Przywara]

Hi,

On 7/22/26 02:56, Enzo Adriano wrote:
> Some sunxi-ng MP clocks have an M divider but no P divider. The A523
> MBUS, IOMMU and DRAM clocks use this layout and also require the update
> bit when changing their rate.
> 
> ccu_mp_set_rate() unconditionally builds and applies a mask for the P
> field. With a zero-width P field this produces an invalid GENMASK()
> range and can clear bits outside a P divider, including the clock gate.
> 
> The callback also ignores CCU_FEATURE_UPDATE_BIT, so hardware that
> requires the update bit may not latch the new divider value.
> 
> Only update the P field when it exists, and set CCU_SUNXI_UPDATE_BIT for
> MP clocks carrying the feature. This matches the existing sunxi-ng div,
> mux and gate helper behavior.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/20260712081341.9D1431F00A3D@smtp.kernel.org
> Fixes: 6702d17f54a8 ("clk: sunxi-ng: a523: add video mod clocks")
> Assisted-by: Codex:gpt-5
> Signed-off-by: Enzo Adriano <enzo.adriano.code@gmail.com>
> ---
> Based on clk-next 8cdeaa50eae8 (Linux 7.2-rc2).
> Tested with strict checkpatch and an arm64 W=1 build of ccu_mp.o.
> No hardware runtime claim is made.

Does that mean it's not tested on hardware?

> 
>   drivers/clk/sunxi-ng/ccu_mp.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
> index 7cdb0eedc69b..aa6cb20447f1 100644
> --- a/drivers/clk/sunxi-ng/ccu_mp.c
> +++ b/drivers/clk/sunxi-ng/ccu_mp.c
> @@ -237,12 +237,17 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
>   
>   	reg = readl(cmp->common.base + cmp->common.reg);
>   	reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
> -	reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->p.width)
> +		reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +	if (cmp->common.features & CCU_FEATURE_UPDATE_BIT)
> +		reg |= CCU_SUNXI_UPDATE_BIT;
>   	reg |= (m - cmp->m.offset) << cmp->m.shift;
> -	if (shift)
> -		reg |= ilog2(p) << cmp->p.shift;
> -	else
> -		reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	if (cmp->p.width) {

Can you merge that into the upper conditional branch? So that it's just 
one if statement?
And then apply the same treatment to the M divider, which is set to 0 by 
some A523 clocks (hstimer and r-timer).

Cheers,
Andre

> +		if (shift)
> +			reg |= ilog2(p) << cmp->p.shift;
> +		else
> +			reg |= (p - cmp->p.offset) << cmp->p.shift;
> +	}
>   
>   	writel(reg, cmp->common.base + cmp->common.reg);
>   

Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[Chen-Yu Tsai]

On Wed, Jul 22, 2026 at 8:56 AM Enzo Adriano
<enzo.adriano.code@gmail.com> wrote:
>
> Some sunxi-ng MP clocks have an M divider but no P divider. The A523
> MBUS, IOMMU and DRAM clocks use this layout and also require the update
> bit when changing their rate.
>
> ccu_mp_set_rate() unconditionally builds and applies a mask for the P
> field. With a zero-width P field this produces an invalid GENMASK()
> range and can clear bits outside a P divider, including the clock gate.
>
> The callback also ignores CCU_FEATURE_UPDATE_BIT, so hardware that
> requires the update bit may not latch the new divider value.
>
> Only update the P field when it exists, and set CCU_SUNXI_UPDATE_BIT for
> MP clocks carrying the feature. This matches the existing sunxi-ng div,
> mux and gate helper behavior.

No. If the clock only has one divider, then it should use the single divider
clk class. That one also supports CCU_FEATURE_UPDATE_BIT.

ChenYu

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/20260712081341.9D1431F00A3D@smtp.kernel.org
> Fixes: 6702d17f54a8 ("clk: sunxi-ng: a523: add video mod clocks")
> Assisted-by: Codex:gpt-5
> Signed-off-by: Enzo Adriano <enzo.adriano.code@gmail.com>
> ---
> Based on clk-next 8cdeaa50eae8 (Linux 7.2-rc2).
> Tested with strict checkpatch and an arm64 W=1 build of ccu_mp.o.
> No hardware runtime claim is made.
>
>  drivers/clk/sunxi-ng/ccu_mp.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
> index 7cdb0eedc69b..aa6cb20447f1 100644
> --- a/drivers/clk/sunxi-ng/ccu_mp.c
> +++ b/drivers/clk/sunxi-ng/ccu_mp.c
> @@ -237,12 +237,17 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
>
>         reg = readl(cmp->common.base + cmp->common.reg);
>         reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
> -       reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +       if (cmp->p.width)
> +               reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> +       if (cmp->common.features & CCU_FEATURE_UPDATE_BIT)
> +               reg |= CCU_SUNXI_UPDATE_BIT;
>         reg |= (m - cmp->m.offset) << cmp->m.shift;
> -       if (shift)
> -               reg |= ilog2(p) << cmp->p.shift;
> -       else
> -               reg |= (p - cmp->p.offset) << cmp->p.shift;
> +       if (cmp->p.width) {
> +               if (shift)
> +                       reg |= ilog2(p) << cmp->p.shift;
> +               else
> +                       reg |= (p - cmp->p.offset) << cmp->p.shift;
> +       }
>
>         writel(reg, cmp->common.base + cmp->common.reg);
>
> --
> 2.53.0

Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[Enzo Adriano]

Hi Andre, Chen-Yu,

Correct, this was not tested on hardware; it was build-tested only.

Thanks for the review. Chen-Yu's point is right: these one-divider clocks
should use the single-divider clock class, which already supports
CCU_FEATURE_UPDATE_BIT. I'll drop this generic ccu_mp patch; there will be
no v2.

This analysis was done with AI assistance and each finding was checked
against the cited sources.

Regards,
Enzo

Re: [PATCH] clk: sunxi-ng: ccu_mp: fix clocks without P dividers

[Chen-Yu Tsai]

On Thu, Jul 23, 2026 at 1:37 AM Enzo Adriano
<enzo.adriano.code@gmail.com> wrote:
>
> Hi Andre, Chen-Yu,
>
> Correct, this was not tested on hardware; it was build-tested only.

For future patches, if you only build-tested it (which is a hard requirement),
please mark the patches with "RFT" (so the subject prefix is "[PATCH RFT]")
and note in the footer that it needs to be tested on actual hardware.


ChenYu

> Thanks for the review. Chen-Yu's point is right: these one-divider clocks
> should use the single-divider clock class, which already supports
> CCU_FEATURE_UPDATE_BIT. I'll drop this generic ccu_mp patch; there will be
> no v2.
>
> This analysis was done with AI assistance and each finding was checked
> against the cited sources.
>
> Regards,
> Enzo