diff mbox series

[v1,3/4] clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate

Message ID 20260719211319.982285-4-juanmanuellopezcarrillo@gmail.com (mailing list archive)
State New
Headers
Series clk: sunxi-ng: fix the A523/T527 GPU clock model, enable GPU DVFS |

Commit Message

Juan Manuel López Carrillo July 19, 2026, 9:13 p.m. UTC 
Register the existing sunxi-ng mux notifier on pll-gpu so the GPU mod
clock is parked on the fixed pll-periph0-600M output while the PLL is
being reprogrammed, and switched back once it has locked, following the
cpux precedent of other sunxi SoCs.  The mux switch is glitch-free per
the T527 manual (GPU_CLK_REG, "The clock selection supports glitch-free
switch").

Nothing retunes pll-gpu at runtime today (the mux does not propagate
rate requests and all standard OPPs resolve to pll-periph0 outputs),
but the higher speed-bin operating points (648-792 MHz) will need
pll-gpu as a live parent, and any direct clk_set_rate() on the PLL is
now safe.

Signed-off-by: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>
---
 drivers/clk/sunxi-ng/ccu-sun55i-a523.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

sashiko-bot@kernel.org July 19, 2026, 9:35 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The `pll_gpu_clk` retains the `CLK_SET_RATE_GATE` flag, which completely prevents the intended dynamic rate changes while the GPU is active.
- [High] Missing `clk_notifier_unregister()` on driver unbind/module unload leads to a Use-After-Free.
- [Medium] The return value of `ccu_mux_notifier_register()` is silently ignored.

Pre-existing issues:
- [High] Unhandled `ABORT_RATE_CHANGE` event in the mux notifier callback permanently parks the GPU on the bypass clock.
- [Medium] Out-of-bounds array read on `gpu_mux_table` via `ccu_mux_helper_set_parent()`.
--

--- Patch [3]: [PATCH v1 3/4] clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate ---
commit faed0fa03ac4f89885fa0cb42878fa743f6eb4a4
Author: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>

clk: sunxi-ng: sun55i-a523: reparent GPU while pll-gpu changes rate

This commit registers a mux notifier on pll-gpu to park the GPU mod clock
on a fixed output while the PLL is being reprogrammed. This is intended to
allow glitch-free rate changes for higher speed-bin operating points.

> diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> index 979e53e635228..4341eb66c0b75 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c

[ ... ]

> @@ -1698,6 +1709,10 @@ static int sun55i_a523_ccu_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/* Reparent the GPU during pll-gpu rate changes */
> +	ccu_mux_notifier_register(pll_gpu_clk.common.hw.clk,
> +				  &sun55i_a523_gpu_nb);
> +

[Severity: High]
Does pll_gpu_clk still have the CLK_SET_RATE_GATE flag set?

Looking at drivers/clk/sunxi-ng/ccu-sun55i-a523.c, pll_gpu_clk is defined
with:

    .hw.init = CLK_HW_INIT_PARENTS_DATA("pll-gpu", osc24M,
                                        &ccu_nkmp_ops,
                                        CLK_SET_RATE_GATE),

The core clock framework enforces this in clk_core_set_rate_nolock():

    if (clk_core_rate_is_protected(core))
        return -EBUSY;

If the GPU driver calls clk_set_rate() while the clock is active, will the
core framework return -EBUSY and prevent the rate change from happening,
rendering this new notifier unused?

[Severity: Medium]
What happens if ccu_mux_notifier_register fails and returns an error?

If there is a low memory condition and clk_notifier_register returns
-ENOMEM, the registration will fail but the probe continues. Will this
leave the GPU clock without the mandatory glitch-free bypass protection
during rate changes?

[Severity: High]
Is it possible this introduces a use-after-free during module unload?

sun55i_a523_ccu_driver does not appear to have a .remove callback to
unregister this notifier.

If the module is unloaded (e.g. via rmmod), the statically allocated
sun55i_a523_gpu_nb is freed. If a clock rate change occurs on pll-gpu
afterwards, or if the global clock notifier list is traversed, could this
crash the system?

[Severity: High]
This is a pre-existing issue, but how does the notifier handle an aborted
rate change?

In drivers/clk/sunxi-ng/ccu_mux.c, ccu_mux_notifier_cb only handles
PRE_RATE_CHANGE and POST_RATE_CHANGE:

    if (event == PRE_RATE_CHANGE) {
        ...
    } else if (event == POST_RATE_CHANGE) {
        ...

If a clock rate change fails downstream after PRE_RATE_CHANGE, the core
emits ABORT_RATE_CHANGE. Since this event is dropped, will the GPU clock
remain permanently stuck at the bypass rate of 600MHz?

[Severity: Medium]
This is a pre-existing issue, but can ccu_mux_helper_set_parent read out of
bounds during POST_RATE_CHANGE?

If the bootloader leaves the hardware mux register with an unmapped value
like 6 or 7, ccu_mux_helper_get_parent returns this raw index. Later,
during the notifier callback, ccu_mux_helper_set_parent blindly uses it:

    if (cm->table)
        index = cm->table[index];

Since gpu_mux_table only has 5 elements:

    static const u8 gpu_mux_table[] = { 0, 2, 3, 4, 5 };

Could this result in an out-of-bounds read and write junk back to the
hardware register?

>  	return 0;
>  }
diff mbox series

Patch

diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
index 979e53e63..4341eb66c 100644
--- a/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
+++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523.c
@@ -1667,6 +1667,17 @@  static const u32 pll_regs[] = {
 	SUN55I_A523_PLL_NPU_REG,
 };
 
+/*
+ * Park the GPU on the fixed pll-periph0-600M output while pll-gpu
+ * changes rate; the mux switch is glitch-free (T527 manual, GPU_CLK_REG).
+ */
+static struct ccu_mux_nb sun55i_a523_gpu_nb = {
+	.common		= &gpu_clk.common,
+	.cm		= &gpu_clk.mux,
+	.delay_us	= 1,
+	.bypass_index	= 1, /* pll-periph0-600M */
+};
+
 static int sun55i_a523_ccu_probe(struct platform_device *pdev)
 {
 	void __iomem *reg;
@@ -1698,6 +1709,10 @@  static int sun55i_a523_ccu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/* Reparent the GPU during pll-gpu rate changes */
+	ccu_mux_notifier_register(pll_gpu_clk.common.hw.clk,
+				  &sun55i_a523_gpu_nb);
+
 	return 0;
 }