linux-kernel - Re: [PATCH v4 4/8] drm/imagination: Use pwrseq for TH1520 GPU power management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMRc=MfdBd6HBwM4F1TcjDvwbOJ03kxgRk4hJQ8HFK7Wz2XBAg@mail.gmail.com>
Date: Mon, 16 Jun 2025 11:40:50 +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 4/8] drm/imagination: Use pwrseq for TH1520 GPU power management

On Sat, Jun 14, 2025 at 8:09 PM Michal Wilczynski
<m.wilczynski@...sung.com> wrote:
>
> Update the Imagination PVR DRM driver to leverage the pwrseq framework
> for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.
>
> To cleanly handle the TH1520's specific power requirements in the
> generic driver, this patch implements the "driver match data" pattern. A
> has_pwrseq flag in a new pvr_soc_data struct is now associated with
> thead,th1520-gpu compatible string in the of_device_id table.
>
> At probe time, the driver checks this flag. If true, it calls
> devm_pwrseq_get("gpu-power"), requiring a valid sequencer and deferring
> probe on failure. In this mode, all power and reset control is delegated
> to the pwrseq provider. If the flag is false, the driver skips this
> logic and falls back to its standard manual power management. Clock
> handles are still acquired directly by this driver in both cases for
> other purposes like devfreq.
>
> The runtime PM callbacks, pvr_power_device_resume() and
> pvr_power_device_suspend(), are modified to call pwrseq_power_on() and
> pwrseq_power_off() respectively when the sequencer is present.  A helper
> function, pvr_power_off_sequence_manual(), is introduced to encapsulate
> the manual power-down logic.
>
> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---

[snip]

>
> +static int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev)
> +{
> +       int err;
> +
> +       err = reset_control_assert(pvr_dev->reset);
> +
> +       clk_disable_unprepare(pvr_dev->mem_clk);
> +       clk_disable_unprepare(pvr_dev->sys_clk);
> +       clk_disable_unprepare(pvr_dev->core_clk);
> +
> +       return err;
> +}
> +
>  int
>  pvr_power_device_suspend(struct device *dev)
>  {
> @@ -252,11 +266,10 @@ pvr_power_device_suspend(struct device *dev)
>                         goto err_drm_dev_exit;
>         }
>
> -       clk_disable_unprepare(pvr_dev->mem_clk);
> -       clk_disable_unprepare(pvr_dev->sys_clk);
> -       clk_disable_unprepare(pvr_dev->core_clk);
> -
> -       err = reset_control_assert(pvr_dev->reset);
> +       if (pvr_dev->pwrseq)
> +               err = pwrseq_power_off(pvr_dev->pwrseq);
> +       else
> +               err = pvr_power_off_sequence_manual(pvr_dev);
>
>  err_drm_dev_exit:
>         drm_dev_exit(idx);
> @@ -276,44 +289,55 @@ pvr_power_device_resume(struct device *dev)
>         if (!drm_dev_enter(drm_dev, &idx))
>                 return -EIO;
>
> -       err = clk_prepare_enable(pvr_dev->core_clk);
> -       if (err)
> -               goto err_drm_dev_exit;
> +       if (pvr_dev->pwrseq) {
> +               err = pwrseq_power_on(pvr_dev->pwrseq);
> +               if (err)
> +                       goto err_drm_dev_exit;
> +       } else {
> +               err = clk_prepare_enable(pvr_dev->core_clk);
> +               if (err)
> +                       goto err_drm_dev_exit;
>
> -       err = clk_prepare_enable(pvr_dev->sys_clk);
> -       if (err)
> -               goto err_core_clk_disable;
> +               err = clk_prepare_enable(pvr_dev->sys_clk);
> +               if (err)
> +                       goto err_core_clk_disable;
>
> -       err = clk_prepare_enable(pvr_dev->mem_clk);
> -       if (err)
> -               goto err_sys_clk_disable;
> +               err = clk_prepare_enable(pvr_dev->mem_clk);
> +               if (err)
> +                       goto err_sys_clk_disable;
>

In order to decrease the number of if-elses, would it make sense to
put the "manual" and "pwrseq" operations into their own separate
functions and then store addresses of these functions in the device
match data struct as function pointers (instead of the has_pwrseq
flag)? This way we'd just call them directly.

Bart