Message-ID: <CAPDyKFpi6_CD++a9sbGBvJCuBSQS6YcpNttkRQhQMTWy1yyrRg@mail.gmail.com>
Date: Thu, 8 May 2025 13:13:19 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, "Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>, 
	Pavel Machek <pavel@...nel.org>, 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>, 
	m.szyprowski@...sung.com, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 3/4] pmdomain: thead: Add GPU-specific clock and reset
 handling for TH1520

On Wed, 30 Apr 2025 at 14:17, Michal Wilczynski
<m.wilczynski@...sung.com> wrote:
>
>
>
> On 4/25/25 10:50, Ulf Hansson wrote:
> > + Bartosz
> >
> > On Mon, 14 Apr 2025 at 20:53, Michal Wilczynski
> > <m.wilczynski@...sung.com> wrote:
> >>
> >> Extend the TH1520 power domain driver to manage GPU related clocks and
> >> resets via generic PM domain start/stop callbacks.
> >>
> >> The TH1520 GPU requires a special sequence to correctly initialize:
> >> - Enable the GPU clocks
> >> - Deassert the GPU clkgen reset
> >> - Delay for a few cycles to satisfy hardware requirements
> >> - Deassert the GPU core reset
> >>
> >> This sequence is SoC-specific and must be abstracted away from the
> >> Imagination GPU driver, which expects only a standard single reset
> >> interface. Following discussions with kernel maintainers [1], this
> >> logic is placed inside a PM domain, rather than polluting the clock or
> >> reset frameworks, or the GPU driver itself.
> >
> > Speaking about special sequences for power-on/off devices like this
> > one, that's a known common problem. We actually have a generic
> > subsystem for this now, drivers/power/sequencing/*.
> >
> > Perhaps it's worth having a look at that, it should allow us to
> > abstract things, so the GPU driver can stay more portable.
> >
> > Kind regards
> > Uffe
>
>
> Hi Ulf, Bartosz,
>
> Thank you very much for your suggestion, Ulf. I took a look at the
> drivers/power/sequencing/ API and agree that it seems like a suitable
> framework to model the specific power-on/off sequence required for the
> TH1520 GPU, allowing for better abstraction than embedding the logic
> directly in genpd callbacks.
>
> My plan is to refactor the series based on this approach. Here's how I
> envision the implementation:
>
> 1) Provider (th1520-pm-domains.c): This driver will register as both a
> generic power domain provider and a power sequencer provider for the GPU
> domain.
>
> 2) pwrseq target Definition: A pwrseq target will be defined within the
> provider to encapsulate the required sequence (enable clocks, deassert
> clkgen reset, delay, deassert GPU core reset). The target will be
> named using the GPU's first compatible string with a "-power" suffix.
>
> Example GPU DT node, adhering to convention introduced here [1].
> gpu: gpu@...f400000 {
>         compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
>                      "img,img-bxm", "img-rogue";
> };

I don't think the power-domain provider node is the correct place for
this, from DT point of view.

Instead, I would try to follow the same approach as
Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml,
which uses a separate device-node to describe the pwrseq provider.

>
> [1] - https://lore.kernel.org/all/20250410-sets-bxs-4-64-patch-v1-v6-1-eda620c5865f@imgtec.com/#t
>
> 3) Consumer (drm/imagination): In its probe function, the driver will
> read the first compatible string of the device node. It will then
> attempt devm_pwrseq_get(dev, compatible_string_with_suffix) (e.g.
> devm_pwrseq_get(dev, "thead,th1520-gpu-power")). The result

Make sense, but we should probably use a more generic target-name,
such as "gpu-power" or something along those lines.

> pvr_dev->pwrseq_desc will be stored (it will be NULL if no suitable
> provider/target is found, or a valid descriptor if successful). The
> driver will still acquire its necessary clock/reset handles via
> devm_*_get in probe for potential use outside of RPM (like devfreq).
>
> 4) Consumer Runtime PM Callbacks
> (pvr_power_device_resume/suspend): These functions will check if
> pvr_dev->pwrseq_desc is valid. If valid: Call pwrseq_power_on() in
> resume and pwrseq_power_off() in suspend. The driver will not perform
> its own clock/reset enabling/disabling for resources managed by the
> sequence. If NULL: Execute the existing fallback logic (manually
> enabling/disabling clocks/resets using the handles acquired in probe).
> Unconditional logic (like FW booting/shutdown) will remain within the
> RPM callbacks, executed after successful power on or before power off,
> respectively.
>
> 5) Resource Handling (via genpd callbacks): To allow the provider
> (th1520-pm-domains.c) to access resources defined in the consumer's DT
> node (specifically the clocks and gpu_reset needed in the sequence), I
> plan to keep the attach_dev / detach_dev genpd callbacks as implemented
> in the previous patch version. attach_dev will acquire the consumer's
> resources (using the consumer_dev pointer) and store the handles in the
> provider's context. The pwrseq unit callbacks will then access these
> stored handles via the shared context. detach_dev will release these
> resources. This seems necessary as the pwrseq API itself doesn't
> currently provide a direct hook for the provider to get the consumer's
> device pointer or manage its resources across the pwrseq_get/put
> lifecycle.

If we have a separate node describing the pwrseq, as
Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml,
it's should be rather easy to hook up and pwrseq driver for it too, as
drivers/power/sequencing/pwrseq-qcom-wcn.c does it.

>
> This approach uses the pwrseq framework for the sequence logic as
> suggested, keeps the generic consumer driver free of SoC-specific
> sequence details (by using the compatible string lookup for this), and
> retains the genpd attach/detach mechanism to handle cross-node resource
> dependencies.
>
> Please let me know if this plan sounds reasonable.
>
> Thanks !

[...]

That said, Bartosz is better with guidance around pwrseq
providers/consumers. The above is just my view of it - and I might
have missed something.

Kind regards
Uffe

Powered by blists - more mailing lists