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 <[email protected]> > Reviewed-by: Bartosz Golaszewski <[email protected]> > Signed-off-by: Michal Wilczynski <[email protected]> > --- > 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: [email protected]
OpenPGP_signature.asc
Description: OpenPGP digital signature
