Message-ID: <a265a20e-8908-40d8-b4e0-2c8b8f773742@imgtec.com>
Date: Tue, 24 Jun 2025 13:53:55 +0000
From: Matt Coster <Matt.Coster@...tec.com>
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>,
        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>,
        Bartosz Golaszewski
	<bartosz.golaszewski@...aro.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 v6 4/8] drm/imagination: Use pwrseq for TH1520 GPU power
 management

On 23/06/2025 12:42, Michal Wilczynski 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.
> The pvr_soc_data struct, associated with a compatible string in the
> of_device_id table, now holds pointers to platform-specific power_on and
> power_off functions.
> 
> At probe time, the driver inspects the assigned power_on function
> pointer. If it points to the pwrseq variant, the driver calls
> devm_pwrseq_get("gpu-power"), requiring a valid sequencer and deferring
> probe on failure. Otherwise, it falls back to its standard manual reset
> initialization.
> 
> The runtime PM callbacks, pvr_power_device_resume() and
> pvr_power_device_suspend(), call the power_on and power_off function
> pointers. Helper functions for both manual and pwrseq-based sequences
> are introduced to support this.

Hi Michal,

My apologies for not responding to previous revisions of this series. In
general, my main earlier complaints were already addressed by others and
the series generally looks good to me.

Just a few notes from me in this and subsequent patches.

> 
> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
> ---
>  drivers/gpu/drm/imagination/Kconfig      |   1 +
>  drivers/gpu/drm/imagination/pvr_device.c |  31 +++++++--
>  drivers/gpu/drm/imagination/pvr_device.h |  19 ++++++
>  drivers/gpu/drm/imagination/pvr_drv.c    |  30 ++++++++-
>  drivers/gpu/drm/imagination/pvr_power.c  | 112 ++++++++++++++++++++-----------
>  drivers/gpu/drm/imagination/pvr_power.h  |   6 ++
>  6 files changed, 152 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
> index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..5f9fff43d6baadc42ebf48d91729bfbf27e06caa 100644
> --- a/drivers/gpu/drm/imagination/Kconfig
> +++ b/drivers/gpu/drm/imagination/Kconfig
> @@ -11,6 +11,7 @@ config DRM_POWERVR
>  	select DRM_SCHED
>  	select DRM_GPUVM
>  	select FW_LOADER
> +	select POWER_SEQUENCING

Given this is (currently) the only platform that requires this
dependency, I'm not super enthusiastic about selecting it here. Given
the modular way the pwrseq code has been written below (which I really
like!), would it make sense to make that code conditional on
CONFIG_POWER_SEQUENCING rather than pulling it in here by default?

>  	help
>  	  Choose this option if you have a system that has an Imagination
>  	  Technologies PowerVR (Series 6 or later) or IMG GPU.
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..c1c24c441c821ccce59f7cd3f14544a91ef54ea9 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -23,8 +23,10 @@
>  #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/pwrseq/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/stddef.h>
> @@ -618,6 +620,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->soc_data = of_device_get_match_data(dev);
> +
>  	/*
>  	 * Setup device parameters. We do this first in case other steps
>  	 * depend on them.
> @@ -631,10 +636,28 @@ 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);
> -	if (err)
> -		return err;
> +	/*
> +	 * For platforms that require it, get the power sequencer.
> +	 * For all others, perform manual reset initialization.
> +	 */
> +	if (pvr_dev->soc_data->power_on == pvr_power_on_sequence_pwrseq) {

Not a huge fan of this check. Semantically it doesn't make a lot of
sense – why would we only care about the power_on callback specifically?
See my comment below.

> +		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");
> +		}
> +	} else {
> +		/* This platform does not use a sequencer, init reset manually. */
> +		err = pvr_device_reset_init(pvr_dev);
> +		if (err)
> +			return err;
> +	}
>  
>  	/* Explicitly power the GPU so we can access control registers before the FW is booted. */
>  	err = pm_runtime_resume_and_get(dev);
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index 7cb01c38d2a9c3fc71effe789d4dfe54eddd93ee..3f35025e84efac031d3261c849ef9fe105466423 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,16 @@ struct pvr_fw_version {
>  	u16 major, minor;
>  };
>  
> +/**
> + * struct pvr_soc_data - Platform specific data associated with a compatible string.
> + * @power_on: Pointer to the platform-specific power on function.
> + * @power_off: Pointer to the platform-specific power off function.
> + */
> +struct pvr_soc_data {

Nit: can we make this struct pvr_device_data? It's being used as the
top-level struct of_device_id.data value, which means it may contain
more than just SoC-specific details later on.

> +	int (*power_on)(struct pvr_device *pvr_dev);
> +	int (*power_off)(struct pvr_device *pvr_dev);

I'd prefer to see these two functions extracted to a separate struct so
_that_ structure can be defined as a constant for *_pwrseq and *_manual
variants and have _those constants_ used in the dt_match table (and, for
example, the check above).

> +};
> +
>  /**
>   * struct pvr_device - powervr-specific wrapper for &struct drm_device
>   */
> @@ -98,6 +111,9 @@ struct pvr_device {
>  	/** @fw_version: Firmware version detected at runtime. */
>  	struct pvr_fw_version fw_version;
>  
> +	/** @soc_data: Pointer to platform-specific quirk data. */
> +	const struct pvr_soc_data *soc_data;

With the type rename above, I guess this would fit better as something
like compatible_data or maybe runtime_data? Naming is hard :)

> +
>  	/** @regs_resource: Resource representing device control registers. */
>  	struct resource *regs_resource;
>  
> @@ -148,6 +164,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..97ccf4a73964ed3752ed1a798231c41cc5c70030 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1481,14 +1481,39 @@ static void pvr_remove(struct platform_device *plat_dev)
>  }
>  
>  static const struct of_device_id dt_match[] = {
> -	{ .compatible = "img,img-rogue", .data = NULL },
> +	{
> +		.compatible = "thead,th1520-gpu",
> +		.data =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_pwrseq,
> +				.power_off = pvr_power_off_sequence_pwrseq,
> +			},
> +	},
> +	{
> +		.compatible = "img,img-rogue",
> +		.data =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_manual,
> +				.power_off = pvr_power_off_sequence_manual,
> +			},
> +	},

Can you define a "default" instance of this so it can be reused below?

>  
>  	/*
>  	 * 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 =
> +			&(struct pvr_soc_data)
> +			{
> +				.power_on = pvr_power_on_sequence_manual,
> +				.power_off = pvr_power_off_sequence_manual,
> +			},
> +	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);
> @@ -1513,4 +1538,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 41f5d89e78b854cf6993838868a4416a220b490a..49b66856b9916b1d13efcc3db739de9be2de56b6 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,71 @@ pvr_watchdog_init(struct pvr_device *pvr_dev)
>  	return 0;
>  }
>  
> +int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev)
> +{
> +	return pwrseq_power_on(pvr_dev->pwrseq);
> +}
> +
> +int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev)
> +{
> +	return pwrseq_power_off(pvr_dev->pwrseq);
> +}

I'm not sure what the standard method of gracefully handling unsupported
configurations at runtime is, but I suppose we could just have
alternative stub versions of these two functions that error out if
!CONFIG_POWER_SEQUENCING.

> +
> +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);

Nit: there were blank lines before each label before.

> +
> +	return err;
> +}
> +
> +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 +318,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->soc_data->power_off(pvr_dev);
>  
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
> @@ -276,54 +338,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->soc_data->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->soc_data->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..d91d5f3f39b61f81121357f4187b1a6e09b3dec0 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.h
> +++ b/drivers/gpu/drm/imagination/pvr_power.h
> @@ -41,4 +41,10 @@ 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);
>  
> +/* Power sequence functions */
> +int pvr_power_on_sequence_manual(struct pvr_device *pvr_dev);
> +int pvr_power_off_sequence_manual(struct pvr_device *pvr_dev);
> +int pvr_power_on_sequence_pwrseq(struct pvr_device *pvr_dev);
> +int pvr_power_off_sequence_pwrseq(struct pvr_device *pvr_dev);

Perhaps the extracted structure of function pointers could be declared
here as e.g. struct pvr_power_sequence_ops, allowing these functions to
be contained to pvr_power.c.

Cheers,
Matt

> +
>  #endif /* PVR_POWER_H */
> 


-- 
Matt Coster
E: matt.coster@...tec.com


Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists