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] [day] [month] [year] [list]
Message-ID: <6176cae6-012e-4dc7-9445-058478bfe758@samsung.com>
Date: Mon, 16 Jun 2025 20:50:42 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Bartosz Golaszewski <brgl@...ev.pl>, Matt Coster
	<matt.coster@...tec.com>, Frank Binns <frank.binns@...tec.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 6/16/25 11:40, Bartosz Golaszewski wrote:
> 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.

Hi Bartosz,

Thanks for the suggestion. That sounds good. I can rework the patch to
use function pointers instead of the flag. 

Matt, as the maintainer of this code, do you have a preference on this?
Let me know what you think.

> 
> Bart
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>