Message-ID: <CAMRc=Mdf9ZYXyzYttzJtnBXPANxn2UYvvdDZqdNaYZwiKZrTjw@mail.gmail.com>
Date: Mon, 16 Jun 2025 11:03:12 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, Fu Wei <wefu@...hat.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Frank Binns <frank.binns@...tec.com>, 
	Matt Coster <matt.coster@...tec.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v4 1/8] power: sequencing: Add T-HEAD TH1520 GPU power
 sequencer driver

On Sat, Jun 14, 2025 at 8:09 PM Michal Wilczynski
<m.wilczynski@...sung.com> 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 is
> an auxiliary driver instantiated by the AON power domain driver.

Just a technicality: this driver controls an auxiliary *device*
instantiated by the AON power domain driver.

>
> 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 devm_clk_bulk_get() and
> devm_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@...aro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---

[snip]

> +
> +static int pwrseq_thead_gpu_power_on(struct pwrseq_device *pwrseq)

Please follow the naming convention of the callbacks: this should be
pwrseq_thead_gpu_enable().

[snip]

> +
> +static int pwrseq_thead_gpu_power_off(struct pwrseq_device *pwrseq)

Same here.

[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);
> +
> +       /* Prevent multiple consumers from attaching */
> +       if (ctx->gpu_reset || ctx->clks)
> +               return -EBUSY;

Isn't it the whole point of pwrseq - to allow multiple consumers to
seamlessly attach to the provider and control the underlying resources
in a safe way? I think you should just not request the relevant
resources for the second time (really only applies to the exclusive
reset and even then it's not clear why it needs to be exclusive) but
still return 1 for a valid consumer and let pwrseq handle the
refcount? Also: can this even happen at all?

> +
> +       ctx->num_clks = ARRAY_SIZE(clk_names);
> +       ctx->clks = devm_kcalloc(dev, ctx->num_clks, sizeof(*ctx->clks),
> +                                GFP_KERNEL);
> +       if (!ctx->clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < ctx->num_clks; i++)
> +               ctx->clks[i].id = clk_names[i];
> +
> +       ret = devm_clk_bulk_get(dev, ctx->num_clks, ctx->clks);

This is interesting. I admit I had not considered the pwrseq provider
being able to acquire the resources from the consumer node at the time
of writing the subsystem. As the pwrseq framework aims at being as
flexible as possible, this is definitely something that we should
allow but the usage of devres here is problematic on at least two
levels. First: you're acquiring the resources from the struct device
of the consumer and so the devres entries are added to its devres
list. They will get released when the consumer device is detached and
the pwrseq provider may end up accessing them afterwards. Second: if
.match() fails or even returns 0, the resource is still acquired. Call
.match() enough times and you have the devres list needlessly
clobbered with unused resources.

You should stick to non-devres variants and make sure they are all
cleaned-up unless returning 1. (Note to self: these shouldn't be magic
values really). You can then release them in this driver's remove
callback.

> +       if (ret)
> +               return ret;
> +
> +       ctx->gpu_reset = devm_reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(ctx->gpu_reset))
> +               return PTR_ERR(ctx->gpu_reset);
> +
> +       return 1;
> +}
> +
> +static int pwrseq_thead_gpu_probe(struct auxiliary_device *adev,
> +                                 const struct auxiliary_device_id *id)
> +{
> +       struct device *dev = &adev->dev;
> +       struct device *parent_dev = dev->parent;
> +       struct pwrseq_thead_gpu_ctx *ctx;
> +       struct pwrseq_config config = {};
> +
> +       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->aon_node = parent_dev->of_node;
> +
> +       ctx->clkgen_reset =
> +               devm_reset_control_get_exclusive(parent_dev, "gpu-clkgen");
> +       if (IS_ERR(ctx->clkgen_reset))
> +               return dev_err_probe(
> +                       dev, PTR_ERR(ctx->clkgen_reset),
> +                       "Failed to get GPU clkgen reset from parent\n");
> +
> +       config.parent = dev;
> +       config.owner = THIS_MODULE;
> +       config.drvdata = ctx;
> +       config.match = pwrseq_thead_gpu_match;
> +       config.targets = pwrseq_thead_gpu_targets;
> +
> +       ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> +       if (IS_ERR(ctx->pwrseq))
> +               return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> +                                    "Failed to register power sequencer\n");
> +
> +       return 0;
> +}
> +
> +static const struct auxiliary_device_id pwrseq_thead_gpu_id_table[] = {
> +       { .name = "th1520_pm_domains.pwrseq-gpu" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, pwrseq_thead_gpu_id_table);
> +
> +static struct auxiliary_driver pwrseq_thead_gpu_driver = {
> +       .driver = {
> +               .name = "pwrseq-thead-gpu",
> +       },
> +       .probe = pwrseq_thead_gpu_probe,
> +       .id_table = pwrseq_thead_gpu_id_table,
> +};
> +module_auxiliary_driver(pwrseq_thead_gpu_driver);
> +
> +MODULE_AUTHOR("Michal Wilczynski <m.wilczynski@...sung.com>");
> +MODULE_DESCRIPTION("T-HEAD TH1520 GPU power sequencer driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>

Thanks!
Bartosz

Powered by blists - more mailing lists