| 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
| ||
|
Message-ID: <55e606c5-9ac0-4e0b-8506-5f88a6fc540e@samsung.com>
Date: Fri, 22 Aug 2025 12:04:18 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Matt Coster <Matt.Coster@...tec.com>
Cc: 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>, Bartosz Golaszewski <brgl@...ev.pl>, Philipp Zabel
<p.zabel@...gutronix.de>, Frank Binns <Frank.Binns@...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>, Drew
Fustini <fustini@...nel.org>, "linux-riscv@...ts.infradead.org"
<linux-riscv@...ts.infradead.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-pm@...r.kernel.org"
<linux-pm@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
<dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v13 1/4] drm/imagination: Use pwrseq for TH1520 GPU
power management
On 8/22/25 11:28, Matt Coster wrote:
> On 21/08/2025 23:20, Michal Wilczynski wrote:
>> Update the Imagination PVR DRM driver to leverage the pwrseq framework
>> for managing the complex power sequence of the GPU on the T-HEAD TH1520
>> SoC.
>>
>> To cleanly separate platform-specific logic from the generic driver,
>> this patch introduces an `init` callback to the `pwr_power_sequence_ops`
>> struct. This allows for different power management strategies to be
>> selected at probe time based on the device's compatible string.
>>
>> A `pvr_device_data` struct, associated with each compatible in the
>> of_device_id table, points to the appropriate ops table (manual or
>> pwrseq).
>>
>> At probe time, the driver now calls the `->init()` op. For pwrseq-based
>> platforms, this callback calls `devm_pwrseq_get("gpu-power")`, deferring
>> probe if the sequencer is not yet available. For other platforms, it
>> falls back to the existing manual clock and reset handling. The runtime
>> PM callbacks continue to call the appropriate functions via the ops
>> table.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
>
> Reviewed-by: Matt Coster <matt.coster@...tec.com>
>
> Would you like me to take the non-DTS changes via drm-misc-next?
Yeah I think this would be appropriate.
Thanks !
>
> Cheers,
> Matt
>
>> ---
>> drivers/gpu/drm/imagination/pvr_device.c | 22 +----
>> drivers/gpu/drm/imagination/pvr_device.h | 17 ++++
>> drivers/gpu/drm/imagination/pvr_drv.c | 23 ++++-
>> drivers/gpu/drm/imagination/pvr_power.c | 158 +++++++++++++++++++++++--------
>> drivers/gpu/drm/imagination/pvr_power.h | 15 +++
>> 5 files changed, 176 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>> index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..294b6019b4155bb7fdb7de73ccf7fa8ad867811f 100644
>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>> @@ -23,6 +23,7 @@
>> #include <linux/firmware.h>
>> #include <linux/gfp.h>
>> #include <linux/interrupt.h>
>> +#include <linux/of.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/reset.h>
>> @@ -121,21 +122,6 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
>> return 0;
>> }
>>
>> -static int pvr_device_reset_init(struct pvr_device *pvr_dev)
>> -{
>> - struct drm_device *drm_dev = from_pvr_device(pvr_dev);
>> - struct reset_control *reset;
>> -
>> - reset = devm_reset_control_get_optional_exclusive(drm_dev->dev, NULL);
>> - if (IS_ERR(reset))
>> - return dev_err_probe(drm_dev->dev, PTR_ERR(reset),
>> - "failed to get gpu reset line\n");
>> -
>> - pvr_dev->reset = reset;
>> -
>> - return 0;
>> -}
>> -
>> /**
>> * pvr_device_process_active_queues() - Process all queue related events.
>> * @pvr_dev: PowerVR device to check
>> @@ -618,6 +604,9 @@ pvr_device_init(struct pvr_device *pvr_dev)
>> struct device *dev = drm_dev->dev;
>> int err;
>>
>> + /* Get the platform-specific data based on the compatible string. */
>> + pvr_dev->device_data = of_device_get_match_data(dev);
>> +
>> /*
>> * Setup device parameters. We do this first in case other steps
>> * depend on them.
>> @@ -631,8 +620,7 @@ pvr_device_init(struct pvr_device *pvr_dev)
>> if (err)
>> return err;
>>
>> - /* Get the reset line for the GPU */
>> - err = pvr_device_reset_init(pvr_dev);
>> + err = pvr_dev->device_data->pwr_ops->init(pvr_dev);
>> if (err)
>> return err;
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
>> index 7cb01c38d2a9c3fc71effe789d4dfe54eddd93ee..ab8f56ae15df6c2888feb16b1d87b59510961936 100644
>> --- a/drivers/gpu/drm/imagination/pvr_device.h
>> +++ b/drivers/gpu/drm/imagination/pvr_device.h
>> @@ -37,6 +37,9 @@ struct clk;
>> /* Forward declaration from <linux/firmware.h>. */
>> struct firmware;
>>
>> +/* Forward declaration from <linux/pwrseq/consumer.h> */
>> +struct pwrseq_desc;
>> +
>> /**
>> * struct pvr_gpu_id - Hardware GPU ID information for a PowerVR device
>> * @b: Branch ID.
>> @@ -57,6 +60,14 @@ struct pvr_fw_version {
>> u16 major, minor;
>> };
>>
>> +/**
>> + * struct pvr_device_data - Platform specific data associated with a compatible string.
>> + * @pwr_ops: Pointer to a structure with platform-specific power functions.
>> + */
>> +struct pvr_device_data {
>> + const struct pvr_power_sequence_ops *pwr_ops;
>> +};
>> +
>> /**
>> * struct pvr_device - powervr-specific wrapper for &struct drm_device
>> */
>> @@ -98,6 +109,9 @@ struct pvr_device {
>> /** @fw_version: Firmware version detected at runtime. */
>> struct pvr_fw_version fw_version;
>>
>> + /** @device_data: Pointer to platform-specific data. */
>> + const struct pvr_device_data *device_data;
>> +
>> /** @regs_resource: Resource representing device control registers. */
>> struct resource *regs_resource;
>>
>> @@ -148,6 +162,9 @@ struct pvr_device {
>> */
>> struct reset_control *reset;
>>
>> + /** @pwrseq: Pointer to a power sequencer, if one is used. */
>> + struct pwrseq_desc *pwrseq;
>> +
>> /** @irq: IRQ number. */
>> int irq;
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
>> index b058ec183bb30ab5c3db17ebaadf2754520a2a1f..916b40ced7eb0408fe985ba1b83b3be2eb024bae 100644
>> --- a/drivers/gpu/drm/imagination/pvr_drv.c
>> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
>> @@ -1480,15 +1480,33 @@ static void pvr_remove(struct platform_device *plat_dev)
>> pvr_power_domains_fini(pvr_dev);
>> }
>>
>> +static const struct pvr_device_data pvr_device_data_manual = {
>> + .pwr_ops = &pvr_power_sequence_ops_manual,
>> +};
>> +
>> +static const struct pvr_device_data pvr_device_data_pwrseq = {
>> + .pwr_ops = &pvr_power_sequence_ops_pwrseq,
>> +};
>> +
>> static const struct of_device_id dt_match[] = {
>> - { .compatible = "img,img-rogue", .data = NULL },
>> + {
>> + .compatible = "thead,th1520-gpu",
>> + .data = &pvr_device_data_pwrseq,
>> + },
>> + {
>> + .compatible = "img,img-rogue",
>> + .data = &pvr_device_data_manual,
>> + },
>>
>> /*
>> * This legacy compatible string was introduced early on before the more generic
>> * "img,img-rogue" was added. Keep it around here for compatibility, but never use
>> * "img,img-axe" in new devicetrees.
>> */
>> - { .compatible = "img,img-axe", .data = NULL },
>> + {
>> + .compatible = "img,img-axe",
>> + .data = &pvr_device_data_manual,
>> + },
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, dt_match);
>> @@ -1513,4 +1531,5 @@ MODULE_DESCRIPTION(PVR_DRIVER_DESC);
>> MODULE_LICENSE("Dual MIT/GPL");
>> MODULE_IMPORT_NS("DMA_BUF");
>> MODULE_FIRMWARE("powervr/rogue_33.15.11.3_v1.fw");
>> +MODULE_FIRMWARE("powervr/rogue_36.52.104.182_v1.fw");
>> MODULE_FIRMWARE("powervr/rogue_36.53.104.796_v1.fw");
>> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
>> index 187a07e0bd9adb2f0713ac2c8e091229f4027354..c6e7ff9e935d3b348eff6953c633c72410fdf507 100644
>> --- a/drivers/gpu/drm/imagination/pvr_power.c
>> +++ b/drivers/gpu/drm/imagination/pvr_power.c
>> @@ -18,6 +18,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/pm_domain.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/pwrseq/consumer.h>
>> #include <linux/reset.h>
>> #include <linux/timer.h>
>> #include <linux/types.h>
>> @@ -234,6 +235,118 @@ pvr_watchdog_init(struct pvr_device *pvr_dev)
>> return 0;
>> }
>>
>> +static int pvr_power_init_manual(struct pvr_device *pvr_dev)
>> +{
>> + struct drm_device *drm_dev = from_pvr_device(pvr_dev);
>> + struct reset_control *reset;
>> +
>> + reset = devm_reset_control_get_optional_exclusive(drm_dev->dev, NULL);
>> + if (IS_ERR(reset))
>> + return dev_err_probe(drm_dev->dev, PTR_ERR(reset),
>> + "failed to get gpu reset line\n");
>> +
>> + pvr_dev->reset = reset;
>> +
>> + return 0;
>> +}
>> +
>> +static int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev)
>> +{
>> + int err;
>> +
>> + err = clk_prepare_enable(pvr_dev->core_clk);
>> + if (err)
>> + return err;
>> +
>> + 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;
>> +
>> + /*
>> + * According to the hardware manual, a delay of at least 32 clock
>> + * cycles is required between de-asserting the clkgen reset and
>> + * de-asserting the GPU reset. Assuming a worst-case scenario with
>> + * a very high GPU clock frequency, a delay of 1 microsecond is
>> + * sufficient to ensure this requirement is met across all
>> + * feasible GPU clock speeds.
>> + */
>> + udelay(1);
>> +
>> + err = reset_control_deassert(pvr_dev->reset);
>> + if (err)
>> + goto err_mem_clk_disable;
>> +
>> + return 0;
>> +
>> +err_mem_clk_disable:
>> + clk_disable_unprepare(pvr_dev->mem_clk);
>> +
>> +err_sys_clk_disable:
>> + clk_disable_unprepare(pvr_dev->sys_clk);
>> +
>> +err_core_clk_disable:
>> + clk_disable_unprepare(pvr_dev->core_clk);
>> +
>> + return err;
>> +}
>> +
>> +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;
>> +}
>> +
>> +const struct pvr_power_sequence_ops pvr_power_sequence_ops_manual = {
>> + .init = pvr_power_init_manual,
>> + .power_on = pvr_power_on_sequence_manual,
>> + .power_off = pvr_power_off_sequence_manual,
>> +};
>> +
>> +static int pvr_power_init_pwrseq(struct pvr_device *pvr_dev)
>> +{
>> + struct device *dev = from_pvr_device(pvr_dev)->dev;
>> +
>> + pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>> + if (IS_ERR(pvr_dev->pwrseq)) {
>> + /*
>> + * This platform requires a sequencer. If we can't get it, we
>> + * must return the error (including -EPROBE_DEFER to wait for
>> + * the provider to appear)
>> + */
>> + return dev_err_probe(dev, PTR_ERR(pvr_dev->pwrseq),
>> + "Failed to get required power sequencer\n");
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev)
>> +{
>> + return pwrseq_power_on(pvr_dev->pwrseq);
>> +}
>> +
>> +static int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev)
>> +{
>> + return pwrseq_power_off(pvr_dev->pwrseq);
>> +}
>> +
>> +const struct pvr_power_sequence_ops pvr_power_sequence_ops_pwrseq = {
>> + .init = pvr_power_init_pwrseq,
>> + .power_on = pvr_power_on_sequence_pwrseq,
>> + .power_off = pvr_power_off_sequence_pwrseq,
>> +};
>> +
>> int
>> pvr_power_device_suspend(struct device *dev)
>> {
>> @@ -252,11 +365,7 @@ 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);
>> + err = pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
>>
>> err_drm_dev_exit:
>> drm_dev_exit(idx);
>> @@ -276,53 +385,22 @@ pvr_power_device_resume(struct device *dev)
>> if (!drm_dev_enter(drm_dev, &idx))
>> return -EIO;
>>
>> - err = clk_prepare_enable(pvr_dev->core_clk);
>> + err = pvr_dev->device_data->pwr_ops->power_on(pvr_dev);
>> 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->mem_clk);
>> - if (err)
>> - goto err_sys_clk_disable;
>> -
>> - /*
>> - * According to the hardware manual, a delay of at least 32 clock
>> - * cycles is required between de-asserting the clkgen reset and
>> - * de-asserting the GPU reset. Assuming a worst-case scenario with
>> - * a very high GPU clock frequency, a delay of 1 microsecond is
>> - * sufficient to ensure this requirement is met across all
>> - * feasible GPU clock speeds.
>> - */
>> - udelay(1);
>> -
>> - err = reset_control_deassert(pvr_dev->reset);
>> - if (err)
>> - goto err_mem_clk_disable;
>> -
>> if (pvr_dev->fw_dev.booted) {
>> err = pvr_power_fw_enable(pvr_dev);
>> if (err)
>> - goto err_reset_assert;
>> + goto err_power_off;
>> }
>>
>> drm_dev_exit(idx);
>>
>> return 0;
>>
>> -err_reset_assert:
>> - reset_control_assert(pvr_dev->reset);
>> -
>> -err_mem_clk_disable:
>> - clk_disable_unprepare(pvr_dev->mem_clk);
>> -
>> -err_sys_clk_disable:
>> - clk_disable_unprepare(pvr_dev->sys_clk);
>> -
>> -err_core_clk_disable:
>> - clk_disable_unprepare(pvr_dev->core_clk);
>> +err_power_off:
>> + pvr_dev->device_data->pwr_ops->power_off(pvr_dev);
>>
>> err_drm_dev_exit:
>> drm_dev_exit(idx);
>> diff --git a/drivers/gpu/drm/imagination/pvr_power.h b/drivers/gpu/drm/imagination/pvr_power.h
>> index ada85674a7ca762dcf92df40424230e1c3910342..b853d092242cc90cb98cf66100679a309055a1dc 100644
>> --- a/drivers/gpu/drm/imagination/pvr_power.h
>> +++ b/drivers/gpu/drm/imagination/pvr_power.h
>> @@ -41,4 +41,19 @@ pvr_power_put(struct pvr_device *pvr_dev)
>> int pvr_power_domains_init(struct pvr_device *pvr_dev);
>> void pvr_power_domains_fini(struct pvr_device *pvr_dev);
>>
>> +/**
>> + * struct pvr_power_sequence_ops - Platform specific power sequence operations.
>> + * @init: Pointer to the platform-specific initialization function.
>> + * @power_on: Pointer to the platform-specific power on function.
>> + * @power_off: Pointer to the platform-specific power off function.
>> + */
>> +struct pvr_power_sequence_ops {
>> + int (*init)(struct pvr_device *pvr_dev);
>> + int (*power_on)(struct pvr_device *pvr_dev);
>> + int (*power_off)(struct pvr_device *pvr_dev);
>> +};
>> +
>> +extern const struct pvr_power_sequence_ops pvr_power_sequence_ops_manual;
>> +extern const struct pvr_power_sequence_ops pvr_power_sequence_ops_pwrseq;
>> +
>> #endif /* PVR_POWER_H */
>>
>
>
Best regards,
--
Michal Wilczynski <m.wilczynski@...sung.com>
Powered by blists - more mailing lists