Re: [PATCH v5 1/8] power: sequencing: Add T-HEAD TH1520 GPU power sequencer driver

[Bartosz Golaszewski]

On Wed, Jun 18, 2025 at 12:22 PM Michal Wilczynski
<m.wilczynski@xxxxxxxxxxx> wrote:
>
> Introduce the pwrseq-thead-gpu driver, a power sequencer provider for
> the Imagination BXM-4-64 GPU on the T-HEAD TH1520 SoC. This driver
> controls an auxiliary device instantiated by the AON power domain.
>
> The TH1520 GPU requires a specific sequence to correctly initialize and
> power down its resources:
>  - Enable GPU clocks (core and sys).
>  - De-assert the GPU clock generator reset (clkgen_reset).
>  - Introduce a short hardware-required delay.
>  - De-assert the GPU core reset. The power-down sequence performs these
>    steps in reverse.
>
> Implement this sequence via the pwrseq_power_on and pwrseq_power_off
> callbacks.
>
> Crucially, the driver's match function is called when a consumer (the
> Imagination GPU driver) requests the "gpu-power" target. During this
> match, the sequencer uses clk_bulk_get() and
> reset_control_get_exclusive() on the consumer's device to obtain handles
> to the GPU's "core" and "sys" clocks, and the GPU core reset.  These,
> along with clkgen_reset obtained from parent aon node, allow it to
> perform the complete sequence.
>
> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> ---

Thanks, this looks much better now.

[snip]

> +
> +static int pwrseq_thead_gpu_disable(struct pwrseq_device *pwrseq)
> +{
> +       struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> +       if (!ctx->clks || !ctx->gpu_reset)
> +               return -ENODEV;
> +
> +       reset_control_assert(ctx->gpu_reset);
> +       reset_control_assert(ctx->clkgen_reset);

These can still fail, I suggest checking and propagating the return values.

> +       clk_bulk_disable_unprepare(ctx->num_clks, ctx->clks);
> +
> +       return 0;
> +}
> +

[snip]

> +
> +static int pwrseq_thead_gpu_match(struct pwrseq_device *pwrseq,
> +                                 struct device *dev)
> +{
> +       struct pwrseq_thead_gpu_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +       static const char *const clk_names[] = { "core", "sys" };
> +       struct of_phandle_args pwr_spec;
> +       int i, ret;
> +
> +       /* We only match the specific T-HEAD TH1520 GPU compatible */
> +       if (!of_device_is_compatible(dev->of_node, "thead,th1520-gpu"))
> +               return 0;
> +
> +       ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> +                                        "#power-domain-cells", 0, &pwr_spec);
> +       if (ret)
> +               return 0;
> +
> +       /* Additionally verify consumer device has AON as power-domain */
> +       if (pwr_spec.np != ctx->aon_node || pwr_spec.args[0] != TH1520_GPU_PD) {
> +               of_node_put(pwr_spec.np);
> +               return 0;
> +       }
> +
> +       of_node_put(pwr_spec.np);
> +
> +       if (ctx->gpu_reset || ctx->clks)
> +               return 1;
> +

One thing that bothers me is that this is still a fragile construct. I
know this cannot happen in this particular design but in theory, this
would not work if there were multiple simultaneous consumers of the
AON power domain. Maybe just to be sure: store the address of the
of_node of the consumer (preferably bumping its reference count) and
check it to make sure that once a consumer associated with this node
is connected, we no longer allow any other nodes? This way you could
also just drop this if replacing it with checking the existence of the
of_node.

[snip]

Bartosz