[v3,05/17] drm/imagination: Get GPU resources

Commit Message

Sarah Walker June 13, 2023, 2:47 p.m. UTC

Acquire clock, regulator and register resources, and enable/map as
appropriate.

Signed-off-by: Sarah Walker <[e-mail]>
---
 drivers/gpu/drm/imagination/Makefile     |   1 +
 drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
 drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
 4 files changed, 496 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/imagination/pvr_device.c

Comments

Andrew Davis June 13, 2023, 6:12 p.m. UTC | #1

On 6/13/23 9:47 AM, Sarah Walker wrote:
> Acquire clock, regulator and register resources, and enable/map as
> appropriate.
> 
> Signed-off-by: Sarah Walker <[e-mail]>
> ---
>   drivers/gpu/drm/imagination/Makefile     |   1 +
>   drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
>   drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
>   drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
>   4 files changed, 496 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> 
> diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
> index 62ccf0ccbd51..186f920d615b 100644
> --- a/drivers/gpu/drm/imagination/Makefile
> +++ b/drivers/gpu/drm/imagination/Makefile
> @@ -4,6 +4,7 @@
>   subdir-ccflags-y := -I$(srctree)/$(src)
>   
>   powervr-y := \
> +	pvr_device.o \
>   	pvr_drv.o \
>   
>   obj-$(CONFIG_DRM_POWERVR) += powervr.o
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> new file mode 100644
> index 000000000000..790c36cebec1
> --- /dev/null
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> +
> +#include "pvr_device.h"
> +
> +#include <drm/drm_print.h>
> +
> +#include <linux/clk.h>
> +#include <linux/compiler_attributes.h>
> +#include <linux/compiler_types.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +
> +/**
> + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> + * control registers.
> + * @pvr_dev: Target PowerVR device.
> + *
> + * Sets struct pvr_device->regs.
> + *
> + * This method of mapping the device control registers into memory ensures that
> + * they are unmapped when the driver is detached (i.e. no explicit cleanup is
> + * required).
> + *
> + * Return:
> + *  * 0 on success, or
> + *  * Any error returned by devm_platform_ioremap_resource().
> + */
> +static int
> +pvr_device_reg_init(struct pvr_device *pvr_dev)
> +{
> +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +	struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> +	struct resource *regs_resource;
> +	void __iomem *regs;
> +	int err;
> +
> +	pvr_dev->regs_resource = NULL;
> +	pvr_dev->regs = NULL;
> +
> +	regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
> +	if (IS_ERR(regs)) {
> +		err = PTR_ERR(regs);
> +		drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> +			err);
> +		return err;
> +	}
> +
> +	pvr_dev->regs = regs;
> +	pvr_dev->regs_resource = regs_resource;
> +
> +	return 0;
> +}
> +
> +/**
> + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> + * control registers.
> + * @pvr_dev: Target PowerVR device.
> + *
> + * This is essentially a no-op, since pvr_device_reg_init() already ensures that
> + * struct pvr_device->regs is unmapped when the device is detached. This
> + * function just sets struct pvr_device->regs to %NULL.
> + */
> +static __always_inline void
> +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> +{
> +	pvr_dev->regs = NULL;

This function isn't needed, kinda defeats the purpose of using devm_*()
if you go an manually have no-op functions to unwind it..

> +}
> +
> +/**
> + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> + * struct pvr_device->mem_clk.
> + *
> + * Three clocks are required by the PowerVR device: core, sys and mem. On
> + * return, this function guarantees that the clocks are in one of the following
> + * states:
> + *
> + *  * All successfully initialized,
> + *  * Core errored, sys and mem uninitialized,
> + *  * Core deinitialized, sys errored, mem uninitialized, or
> + *  * Core and sys deinitialized, mem errored.
> + *
> + * Return:
> + *  * 0 on success,
> + *  * Any error returned by devm_clk_get(), or
> + *  * Any error returned by clk_prepare_enable().
> + */
> +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> +{
> +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +	struct clk *core_clk;
> +	struct clk *sys_clk;
> +	struct clk *mem_clk;
> +	int err;
> +
> +	pvr_dev->core_clk = NULL;
> +	pvr_dev->sys_clk = NULL;
> +	pvr_dev->mem_clk = NULL;

You could NULL these out on the error path, but is that even needed, looks
like if this functions fails we bail on the whole init where this is all
deallocated (plus NULL'd out again in that path).

> +
> +	core_clk = devm_clk_get(drm_dev->dev, "core");
> +	if (IS_ERR(core_clk)) {
> +		err = PTR_ERR(core_clk);
> +		drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
> +		goto err_out;
> +	}
> +
> +	sys_clk = devm_clk_get(drm_dev->dev, "sys");
> +	if (IS_ERR(sys_clk))
> +		sys_clk = NULL;
> +
> +	mem_clk = devm_clk_get(drm_dev->dev, "mem");
> +	if (IS_ERR(mem_clk))
> +		mem_clk = NULL;
> +
> +	err = clk_prepare(core_clk);
> +	if (err)
> +		goto err_out;
> +
> +	if (sys_clk) {
> +		err = clk_prepare(sys_clk);
> +		if (err)
> +			goto err_deinit_core_clk;
> +	}
> +
> +	if (mem_clk) {
> +		err = clk_prepare(mem_clk);
> +		if (err)
> +			goto err_deinit_sys_clk;
> +	}
> +
> +	pvr_dev->core_clk = core_clk;
> +	pvr_dev->sys_clk = sys_clk;
> +	pvr_dev->mem_clk = mem_clk;
> +
> +	return 0;
> +
> +err_deinit_sys_clk:
> +	if (sys_clk)
> +		clk_disable_unprepare(sys_clk);
> +err_deinit_core_clk:
> +	clk_disable_unprepare(core_clk);
> +err_out:
> +	return err;
> +}
> +
> +/**
> + * pvr_device_clk_fini() - Deinitialize clocks required by a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + */
> +static void
> +pvr_device_clk_fini(struct pvr_device *pvr_dev)
> +{
> +	if (pvr_dev->mem_clk)
> +		clk_unprepare(pvr_dev->mem_clk);

Using devm_clk_get_(optional)_prepared() in the init function above might allow
these to be unprepaired for you, in that case this function also can be dropped.

> +	if (pvr_dev->sys_clk)
> +		clk_unprepare(pvr_dev->sys_clk);
> +	clk_unprepare(pvr_dev->core_clk);
> +
> +	pvr_dev->core_clk = NULL;
> +	pvr_dev->sys_clk = NULL;
> +	pvr_dev->mem_clk = NULL;
> +}
> +
> +/**
> + * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * The regulator is not a required devicetree property. If it is not present then this function will
> + * succeed, but &pvr_device->regulator will be %NULL.
> + *
> + * Returns:
> + *  * 0 on success, or
> + *  * Any error returned by devm_regulator_get().
> + */
> +static int
> +pvr_device_regulator_init(struct pvr_device *pvr_dev)
> +{
> +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +	struct regulator *regulator;
> +	int err;
> +
> +	regulator = devm_regulator_get(drm_dev->dev, "power");
> +	if (IS_ERR(regulator)) {
> +		err = PTR_ERR(regulator);
> +		/* Regulator is not required, so ENODEV is allowed here. */

There is devm_regulator_get_optional(), might be what you want here.

Also using dev_err_probe() would check for probe defer here.

> +		if (err != -ENODEV)
> +			goto err_out;
> +		regulator = NULL;
> +	}
> +
> +	pvr_dev->regulator = regulator;
> +
> +	return 0;
> +
> +err_out:
> +	return err;
> +}
> +
> +/**
> + * pvr_device_init() - Initialize a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * If this function returns successfully, the device will have been fully
> + * initialized. Otherwise, any parts of the device initialized before an error
> + * occurs will be de-initialized before returning.
> + *
> + * NOTE: The initialization steps currently taken are the bare minimum required
> + *       to read from the control registers. The device is unlikely to function
> + *       until further initialization steps are added. [This note should be
> + *       removed when that happens.]
> + *
> + * Return:
> + *  * 0 on success,
> + *  * Any error returned by pvr_device_reg_init(),
> + *  * Any error returned by pvr_device_clk_init(), or
> + *  * Any error returned by pvr_device_gpu_init().
> + */
> +int
> +pvr_device_init(struct pvr_device *pvr_dev)
> +{
> +	int err;
> +
> +	/* Enable and initialize clocks required for the device to operate. */
> +	err = pvr_device_clk_init(pvr_dev);
> +	if (err)
> +		goto err_out;
> +
> +	err = pvr_device_regulator_init(pvr_dev);
> +	if (err)
> +		goto err_device_clk_fini;
> +
> +	/* Map the control registers into memory. */
> +	err = pvr_device_reg_init(pvr_dev);
> +	if (err)
> +		goto err_device_reg_fini;
> +
> +	return 0;
> +
> +err_device_reg_fini:
> +	pvr_device_reg_fini(pvr_dev);
> +
> +err_device_clk_fini:
> +	pvr_device_clk_fini(pvr_dev);
> +
> +err_out:
> +	return err;
> +}
> +
> +/**
> + * pvr_device_fini() - Deinitialize a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + */
> +void
> +pvr_device_fini(struct pvr_device *pvr_dev)
> +{
> +	/*
> +	 * Deinitialization stages are performed in reverse order compared to
> +	 * the initialization stages in pvr_device_init().
> +	 */
> +	pvr_device_reg_fini(pvr_dev);
> +	pvr_device_clk_fini(pvr_dev);
> +}
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index 3d2865d726b8..c0dd0562844c 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.h
> +++ b/drivers/gpu/drm/imagination/pvr_device.h
> @@ -11,9 +11,25 @@
>   #include <linux/bits.h>
>   #include <linux/compiler_attributes.h>
>   #include <linux/compiler_types.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
>   #include <linux/kernel.h>
> +#include <linux/math.h>
> +#include <linux/mutex.h>
> +#include <linux/timer.h>
>   #include <linux/types.h>
>   #include <linux/wait.h>
> +#include <linux/workqueue.h>
> +#include <linux/xarray.h>
> +
> +/* Forward declaration from <linux/clk.h>. */
> +struct clk;
> +
> +/* Forward declaration from <linux/firmware.h>. */
> +struct firmware;
> +
> +/* Forward declaration from <linux/regulator/consumer.h>. */
> +struct regulator;
>   
>   /**
>    * struct pvr_device - powervr-specific wrapper for &struct drm_device
> @@ -26,6 +42,29 @@ struct pvr_device {
>   	 * from_pvr_device().
>   	 */
>   	struct drm_device base;
> +
> +	/** @regs_resource: Resource representing device control registers. */
> +	struct resource *regs_resource;
> +
> +	/**
> +	 * @regs: Device control registers.
> +	 *
> +	 * These are mapped into memory when the device is initialized; that
> +	 * location is where this pointer points.
> +	 */
> +	void __iomem *regs;
> +
> +	/** @core_clk: General core clock. */
> +	struct clk *core_clk;
> +
> +	/** @sys_clk: System bus clock. */
> +	struct clk *sys_clk;
> +
> +	/** @mem_clk: Memory clock. */
> +	struct clk *mem_clk;
> +
> +	/** @regulator: Power regulator. */
> +	struct regulator *regulator;
>   };
>   
>   /**
> @@ -72,6 +111,181 @@ to_pvr_file(struct drm_file *file)
>   	return file->driver_priv;
>   }
>   
> +int pvr_device_init(struct pvr_device *pvr_dev);
> +void pvr_device_fini(struct pvr_device *pvr_dev);
> +
> +int
> +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);
> +
> +/**
> + * PVR_CR_READ32() - Read a 32-bit register from a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + *
> + * This macro is a wrapper around pvr_cr_read32(). It applies ROGUE_CR_ prefix
> + * to the provided @reg name, making it behave comparably to the
> + * PVR_CR_FIELD_GET() macro.
> + *
> + * Return: The value of the requested register.
> + */
> +#define PVR_CR_READ32(pvr_dev, reg) pvr_cr_read32(pvr_dev, ROGUE_CR_##reg)

Not a huge fan of all these oneline functions/macros, does this really add anyhing?
All it does IMHO is add two levels of indirection we have to track back though
to figure out what it actually is doing, when calling ioread32() directly would
have been just as clean and more obvious.

Andrew

> +
> +/**
> + * PVR_CR_READ64() - Read a 64-bit register from a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + *
> + * This macro is a wrapper around pvr_cr_read64(). It applies ROGUE_CR_ prefix
> + * to the provided @reg name, making it behave comparably to the
> + * PVR_CR_FIELD_GET() macro.
> + *
> + * Return: The value of the requested register.
> + */
> +#define PVR_CR_READ64(pvr_dev, reg) pvr_cr_read64(pvr_dev, ROGUE_CR_##reg)
> +
> +/**
> + * PVR_CR_WRITE32() - Write to a 32-bit register in a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + * @val: Value to write.
> + *
> + * This macro is a wrapper around pvr_cr_write32(). It applies ROGUE_CR_
> + * prefix to the provided @reg name, making it behave comparably to the
> + * PVR_CR_FIELD_GET() macro.
> + */
> +#define PVR_CR_WRITE32(pvr_dev, reg, val) \
> +	pvr_cr_write32(pvr_dev, ROGUE_CR_##reg, val)
> +
> +/**
> + * PVR_CR_WRITE64() - Write to a 64-bit register in a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + * @val: Value to write.
> + *
> + * This macro is a wrapper around pvr_cr_write64(). It applies ROGUE_CR_
> + * prefix to the provided @reg name, making it behave comparably to the
> + * PVR_CR_FIELD_GET() macro.
> + */
> +#define PVR_CR_WRITE64(pvr_dev, reg, val) \
> +	pvr_cr_write64(pvr_dev, ROGUE_CR_##reg, val)
> +
> +/**
> + * PVR_CR_FIELD_GET() - Extract a single field from a PowerVR control register
> + * @val: Value of the target register.
> + * @field: Field specifier, as defined in "pvr_rogue_cr_defs.h".
> + *
> + * Return: The extracted field.
> + */
> +#define PVR_CR_FIELD_GET(val, field) FIELD_GET(~ROGUE_CR_##field##_CLRMSK, val)
> +
> +/**
> + * pvr_cr_read32() - Read a 32-bit register from a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + *
> + * The wrapper macro PVR_CR_READ32() may be used instead to simplify the
> + * selection of @reg.
> + *
> + * Return: The value of the requested register.
> + */
> +static __always_inline u32
> +pvr_cr_read32(struct pvr_device *pvr_dev, u32 reg)
> +{
> +	return ioread32(pvr_dev->regs + reg);
> +}
> +
> +/**
> + * pvr_cr_read64() - Read a 64-bit register from a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + *
> + * The wrapper macro PVR_CR_READ64() may be used instead to simplify the
> + * selection of @reg.
> + *
> + * Return: The value of the requested register.
> + */
> +static __always_inline u64
> +pvr_cr_read64(struct pvr_device *pvr_dev, u32 reg)
> +{
> +	return ioread64(pvr_dev->regs + reg);
> +}
> +
> +/**
> + * pvr_cr_write32() - Write to a 32-bit register in a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + * @val: Value to write.
> + *
> + * The wrapper macro PVR_CR_WRITE32() may be used instead to simplify the
> + * selection of @reg.
> + */
> +static __always_inline void
> +pvr_cr_write32(struct pvr_device *pvr_dev, u32 reg, u32 val)
> +{
> +	iowrite32(val, pvr_dev->regs + reg);
> +}
> +
> +/**
> + * pvr_cr_write64() - Write to a 64-bit register in a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + * @reg: Target register.
> + * @val: Value to write.
> + *
> + * The wrapper macro PVR_CR_WRITE64() may be used instead to simplify the
> + * selection of @reg.
> + */
> +static __always_inline void
> +pvr_cr_write64(struct pvr_device *pvr_dev, u32 reg, u64 val)
> +{
> +	iowrite64(val, pvr_dev->regs + reg);
> +}
> +
> +/**
> + * pvr_cr_poll_reg32() - Wait for a 32-bit register to match a given value by
> + *                       polling
> + * @pvr_dev: Target PowerVR device.
> + * @reg_addr: Address of register.
> + * @reg_value: Expected register value (after masking).
> + * @reg_mask: Mask of bits valid for comparison with @reg_value.
> + * @timeout_usec: Timeout length, in us.
> + *
> + * Returns:
> + *  * 0 on success, or
> + *  * -%ETIMEDOUT on timeout.
> + */
> +static __always_inline int
> +pvr_cr_poll_reg32(struct pvr_device *pvr_dev, u32 reg_addr, u32 reg_value,
> +		  u32 reg_mask, u64 timeout_usec)
> +{
> +	u32 value;
> +
> +	return readl_poll_timeout(pvr_dev->regs + reg_addr, value,
> +		(value & reg_mask) == reg_value, 0, timeout_usec);
> +}
> +
> +/**
> + * pvr_cr_poll_reg64() - Wait for a 64-bit register to match a given value by
> + *                       polling
> + * @pvr_dev: Target PowerVR device.
> + * @reg_addr: Address of register.
> + * @reg_value: Expected register value (after masking).
> + * @reg_mask: Mask of bits valid for comparison with @reg_value.
> + * @timeout_usec: Timeout length, in us.
> + *
> + * Returns:
> + *  * 0 on success, or
> + *  * -%ETIMEDOUT on timeout.
> + */
> +static __always_inline int
> +pvr_cr_poll_reg64(struct pvr_device *pvr_dev, u32 reg_addr, u64 reg_value,
> +		  u64 reg_mask, u64 timeout_usec)
> +{
> +	u64 value;
> +
> +	return readq_poll_timeout(pvr_dev->regs + reg_addr, value,
> +		(value & reg_mask) == reg_value, 0, timeout_usec);
> +}
> +
>   /**
>    * DOC: IOCTL validation helpers
>    *
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index e203a2370f55..48a870715426 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -485,12 +485,19 @@ pvr_probe(struct platform_device *plat_dev)
>   
>   	platform_set_drvdata(plat_dev, drm_dev);
>   
> -	err = drm_dev_register(drm_dev, 0);
> +	err = pvr_device_init(pvr_dev);
>   	if (err)
>   		goto err_out;
>   
> +	err = drm_dev_register(drm_dev, 0);
> +	if (err)
> +		goto err_device_fini;
> +
>   	return 0;
>   
> +err_device_fini:
> +	pvr_device_fini(pvr_dev);
> +
>   err_out:
>   	return err;
>   }
> @@ -499,8 +506,10 @@ static int
>   pvr_remove(struct platform_device *plat_dev)
>   {
>   	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> +	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
>   
>   	drm_dev_unregister(drm_dev);
> +	pvr_device_fini(pvr_dev);
>   
>   	return 0;
>   }

Frank Binns June 16, 2023, 11:23 a.m. UTC | #2

Hi Andrew,

On Tue, 2023-06-13 at 13:12 -0500, Andrew Davis wrote:
> On 6/13/23 9:47 AM, Sarah Walker wrote:
> > Acquire clock, regulator and register resources, and enable/map as
> > appropriate.
> > 
> > Signed-off-by: Sarah Walker <[e-mail]>
> > ---
> >   drivers/gpu/drm/imagination/Makefile     |   1 +
> >   drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
> >   drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
> >   drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
> >   4 files changed, 496 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
> > index 62ccf0ccbd51..186f920d615b 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -4,6 +4,7 @@
> >   subdir-ccflags-y := -I$(srctree)/$(src)
> >   
> >   powervr-y := \
> > +	pvr_device.o \
> >   	pvr_drv.o \
> >   
> >   obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> > new file mode 100644
> > index 000000000000..790c36cebec1
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#include "pvr_device.h"
> > +
> > +#include <drm/drm_print.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/compiler_attributes.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/stddef.h>
> > +#include <linux/types.h>
> > +
> > +/**
> > + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->regs.
> > + *
> > + * This method of mapping the device control registers into memory ensures that
> > + * they are unmapped when the driver is detached (i.e. no explicit cleanup is
> > + * required).
> > + *
> > + * Return:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_platform_ioremap_resource().
> > + */
> > +static int
> > +pvr_device_reg_init(struct pvr_device *pvr_dev)
> > +{
> > +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +	struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> > +	struct resource *regs_resource;
> > +	void __iomem *regs;
> > +	int err;
> > +
> > +	pvr_dev->regs_resource = NULL;
> > +	pvr_dev->regs = NULL;
> > +
> > +	regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
> > +	if (IS_ERR(regs)) {
> > +		err = PTR_ERR(regs);
> > +		drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> > +			err);
> > +		return err;
> > +	}
> > +
> > +	pvr_dev->regs = regs;
> > +	pvr_dev->regs_resource = regs_resource;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * This is essentially a no-op, since pvr_device_reg_init() already ensures that
> > + * struct pvr_device->regs is unmapped when the device is detached. This
> > + * function just sets struct pvr_device->regs to %NULL.
> > + */
> > +static __always_inline void
> > +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> > +{
> > +	pvr_dev->regs = NULL;
> 
> This function isn't needed, kinda defeats the purpose of using devm_*()
> if you go an manually have no-op functions to unwind it..

Thank you for the suggestions. We'll fix this, as well as your other suggestions
below.

Thanks
Frank

> 
> > +}
> > +
> > +/**
> > + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> > + * struct pvr_device->mem_clk.
> > + *
> > + * Three clocks are required by the PowerVR device: core, sys and mem. On
> > + * return, this function guarantees that the clocks are in one of the following
> > + * states:
> > + *
> > + *  * All successfully initialized,
> > + *  * Core errored, sys and mem uninitialized,
> > + *  * Core deinitialized, sys errored, mem uninitialized, or
> > + *  * Core and sys deinitialized, mem errored.
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by devm_clk_get(), or
> > + *  * Any error returned by clk_prepare_enable().
> > + */
> > +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> > +{
> > +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +	struct clk *core_clk;
> > +	struct clk *sys_clk;
> > +	struct clk *mem_clk;
> > +	int err;
> > +
> > +	pvr_dev->core_clk = NULL;
> > +	pvr_dev->sys_clk = NULL;
> > +	pvr_dev->mem_clk = NULL;
> 
> You could NULL these out on the error path, but is that even needed, looks
> like if this functions fails we bail on the whole init where this is all
> deallocated (plus NULL'd out again in that path).
> 
> > +
> > +	core_clk = devm_clk_get(drm_dev->dev, "core");
> > +	if (IS_ERR(core_clk)) {
> > +		err = PTR_ERR(core_clk);
> > +		drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
> > +		goto err_out;
> > +	}
> > +
> > +	sys_clk = devm_clk_get(drm_dev->dev, "sys");
> > +	if (IS_ERR(sys_clk))
> > +		sys_clk = NULL;
> > +
> > +	mem_clk = devm_clk_get(drm_dev->dev, "mem");
> > +	if (IS_ERR(mem_clk))
> > +		mem_clk = NULL;
> > +
> > +	err = clk_prepare(core_clk);
> > +	if (err)
> > +		goto err_out;
> > +
> > +	if (sys_clk) {
> > +		err = clk_prepare(sys_clk);
> > +		if (err)
> > +			goto err_deinit_core_clk;
> > +	}
> > +
> > +	if (mem_clk) {
> > +		err = clk_prepare(mem_clk);
> > +		if (err)
> > +			goto err_deinit_sys_clk;
> > +	}
> > +
> > +	pvr_dev->core_clk = core_clk;
> > +	pvr_dev->sys_clk = sys_clk;
> > +	pvr_dev->mem_clk = mem_clk;
> > +
> > +	return 0;
> > +
> > +err_deinit_sys_clk:
> > +	if (sys_clk)
> > +		clk_disable_unprepare(sys_clk);
> > +err_deinit_core_clk:
> > +	clk_disable_unprepare(core_clk);
> > +err_out:
> > +	return err;
> > +}
> > +
> > +/**
> > + * pvr_device_clk_fini() - Deinitialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + */
> > +static void
> > +pvr_device_clk_fini(struct pvr_device *pvr_dev)
> > +{
> > +	if (pvr_dev->mem_clk)
> > +		clk_unprepare(pvr_dev->mem_clk);
> 
> Using devm_clk_get_(optional)_prepared() in the init function above might allow
> these to be unprepaired for you, in that case this function also can be dropped.
> 
> > +	if (pvr_dev->sys_clk)
> > +		clk_unprepare(pvr_dev->sys_clk);
> > +	clk_unprepare(pvr_dev->core_clk);
> > +
> > +	pvr_dev->core_clk = NULL;
> > +	pvr_dev->sys_clk = NULL;
> > +	pvr_dev->mem_clk = NULL;
> > +}
> > +
> > +/**
> > + * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * The regulator is not a required devicetree property. If it is not present then this function will
> > + * succeed, but &pvr_device->regulator will be %NULL.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_regulator_get().
> > + */
> > +static int
> > +pvr_device_regulator_init(struct pvr_device *pvr_dev)
> > +{
> > +	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +	struct regulator *regulator;
> > +	int err;
> > +
> > +	regulator = devm_regulator_get(drm_dev->dev, "power");
> > +	if (IS_ERR(regulator)) {
> > +		err = PTR_ERR(regulator);
> > +		/* Regulator is not required, so ENODEV is allowed here. */
> 
> There is devm_regulator_get_optional(), might be what you want here.
> 
> Also using dev_err_probe() would check for probe defer here.
> 
> > +		if (err != -ENODEV)
> > +			goto err_out;
> > +		regulator = NULL;
> > +	}
> > +
> > +	pvr_dev->regulator = regulator;
> > +
> > +	return 0;
> > +
> > +err_out:
> > +	return err;
> > +}
> > +
> > +/**
> > + * pvr_device_init() - Initialize a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * If this function returns successfully, the device will have been fully
> > + * initialized. Otherwise, any parts of the device initialized before an error
> > + * occurs will be de-initialized before returning.
> > + *
> > + * NOTE: The initialization steps currently taken are the bare minimum required
> > + *       to read from the control registers. The device is unlikely to function
> > + *       until further initialization steps are added. [This note should be
> > + *       removed when that happens.]
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by pvr_device_reg_init(),
> > + *  * Any error returned by pvr_device_clk_init(), or
> > + *  * Any error returned by pvr_device_gpu_init().
> > + */
> > +int
> > +pvr_device_init(struct pvr_device *pvr_dev)
> > +{
> > +	int err;
> > +
> > +	/* Enable and initialize clocks required for the device to operate. */
> > +	err = pvr_device_clk_init(pvr_dev);
> > +	if (err)
> > +		goto err_out;
> > +
> > +	err = pvr_device_regulator_init(pvr_dev);
> > +	if (err)
> > +		goto err_device_clk_fini;
> > +
> > +	/* Map the control registers into memory. */
> > +	err = pvr_device_reg_init(pvr_dev);
> > +	if (err)
> > +		goto err_device_reg_fini;
> > +
> > +	return 0;
> > +
> > +err_device_reg_fini:
> > +	pvr_device_reg_fini(pvr_dev);
> > +
> > +err_device_clk_fini:
> > +	pvr_device_clk_fini(pvr_dev);
> > +
> > +err_out:
> > +	return err;
> > +}
> > +
> > +/**
> > + * pvr_device_fini() - Deinitialize a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + */
> > +void
> > +pvr_device_fini(struct pvr_device *pvr_dev)
> > +{
> > +	/*
> > +	 * Deinitialization stages are performed in reverse order compared to
> > +	 * the initialization stages in pvr_device_init().
> > +	 */
> > +	pvr_device_reg_fini(pvr_dev);
> > +	pvr_device_clk_fini(pvr_dev);
> > +}
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> > index 3d2865d726b8..c0dd0562844c 100644
> > --- a/drivers/gpu/drm/imagination/pvr_device.h
> > +++ b/drivers/gpu/drm/imagination/pvr_device.h
> > @@ -11,9 +11,25 @@
> >   #include <linux/bits.h>
> >   #include <linux/compiler_attributes.h>
> >   #include <linux/compiler_types.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> >   #include <linux/kernel.h>
> > +#include <linux/math.h>
> > +#include <linux/mutex.h>
> > +#include <linux/timer.h>
> >   #include <linux/types.h>
> >   #include <linux/wait.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/xarray.h>
> > +
> > +/* Forward declaration from <linux/clk.h>. */
> > +struct clk;
> > +
> > +/* Forward declaration from <linux/firmware.h>. */
> > +struct firmware;
> > +
> > +/* Forward declaration from <linux/regulator/consumer.h>. */
> > +struct regulator;
> >   
> >   /**
> >    * struct pvr_device - powervr-specific wrapper for &struct drm_device
> > @@ -26,6 +42,29 @@ struct pvr_device {
> >   	 * from_pvr_device().
> >   	 */
> >   	struct drm_device base;
> > +
> > +	/** @regs_resource: Resource representing device control registers. */
> > +	struct resource *regs_resource;
> > +
> > +	/**
> > +	 * @regs: Device control registers.
> > +	 *
> > +	 * These are mapped into memory when the device is initialized; that
> > +	 * location is where this pointer points.
> > +	 */
> > +	void __iomem *regs;
> > +
> > +	/** @core_clk: General core clock. */
> > +	struct clk *core_clk;
> > +
> > +	/** @sys_clk: System bus clock. */
> > +	struct clk *sys_clk;
> > +
> > +	/** @mem_clk: Memory clock. */
> > +	struct clk *mem_clk;
> > +
> > +	/** @regulator: Power regulator. */
> > +	struct regulator *regulator;
> >   };
> >   
> >   /**
> > @@ -72,6 +111,181 @@ to_pvr_file(struct drm_file *file)
> >   	return file->driver_priv;
> >   }
> >   
> > +int pvr_device_init(struct pvr_device *pvr_dev);
> > +void pvr_device_fini(struct pvr_device *pvr_dev);
> > +
> > +int
> > +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);
> > +
> > +/**
> > + * PVR_CR_READ32() - Read a 32-bit register from a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + *
> > + * This macro is a wrapper around pvr_cr_read32(). It applies ROGUE_CR_ prefix
> > + * to the provided @reg name, making it behave comparably to the
> > + * PVR_CR_FIELD_GET() macro.
> > + *
> > + * Return: The value of the requested register.
> > + */
> > +#define PVR_CR_READ32(pvr_dev, reg) pvr_cr_read32(pvr_dev, ROGUE_CR_##reg)
> 
> Not a huge fan of all these oneline functions/macros, does this really add anyhing?
> All it does IMHO is add two levels of indirection we have to track back though
> to figure out what it actually is doing, when calling ioread32() directly would
> have been just as clean and more obvious.
> 
> Andrew
> 
> > +
> > +/**
> > + * PVR_CR_READ64() - Read a 64-bit register from a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + *
> > + * This macro is a wrapper around pvr_cr_read64(). It applies ROGUE_CR_ prefix
> > + * to the provided @reg name, making it behave comparably to the
> > + * PVR_CR_FIELD_GET() macro.
> > + *
> > + * Return: The value of the requested register.
> > + */
> > +#define PVR_CR_READ64(pvr_dev, reg) pvr_cr_read64(pvr_dev, ROGUE_CR_##reg)
> > +
> > +/**
> > + * PVR_CR_WRITE32() - Write to a 32-bit register in a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + * @val: Value to write.
> > + *
> > + * This macro is a wrapper around pvr_cr_write32(). It applies ROGUE_CR_
> > + * prefix to the provided @reg name, making it behave comparably to the
> > + * PVR_CR_FIELD_GET() macro.
> > + */
> > +#define PVR_CR_WRITE32(pvr_dev, reg, val) \
> > +	pvr_cr_write32(pvr_dev, ROGUE_CR_##reg, val)
> > +
> > +/**
> > + * PVR_CR_WRITE64() - Write to a 64-bit register in a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + * @val: Value to write.
> > + *
> > + * This macro is a wrapper around pvr_cr_write64(). It applies ROGUE_CR_
> > + * prefix to the provided @reg name, making it behave comparably to the
> > + * PVR_CR_FIELD_GET() macro.
> > + */
> > +#define PVR_CR_WRITE64(pvr_dev, reg, val) \
> > +	pvr_cr_write64(pvr_dev, ROGUE_CR_##reg, val)
> > +
> > +/**
> > + * PVR_CR_FIELD_GET() - Extract a single field from a PowerVR control register
> > + * @val: Value of the target register.
> > + * @field: Field specifier, as defined in "pvr_rogue_cr_defs.h".
> > + *
> > + * Return: The extracted field.
> > + */
> > +#define PVR_CR_FIELD_GET(val, field) FIELD_GET(~ROGUE_CR_##field##_CLRMSK, val)
> > +
> > +/**
> > + * pvr_cr_read32() - Read a 32-bit register from a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + *
> > + * The wrapper macro PVR_CR_READ32() may be used instead to simplify the
> > + * selection of @reg.
> > + *
> > + * Return: The value of the requested register.
> > + */
> > +static __always_inline u32
> > +pvr_cr_read32(struct pvr_device *pvr_dev, u32 reg)
> > +{
> > +	return ioread32(pvr_dev->regs + reg);
> > +}
> > +
> > +/**
> > + * pvr_cr_read64() - Read a 64-bit register from a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + *
> > + * The wrapper macro PVR_CR_READ64() may be used instead to simplify the
> > + * selection of @reg.
> > + *
> > + * Return: The value of the requested register.
> > + */
> > +static __always_inline u64
> > +pvr_cr_read64(struct pvr_device *pvr_dev, u32 reg)
> > +{
> > +	return ioread64(pvr_dev->regs + reg);
> > +}
> > +
> > +/**
> > + * pvr_cr_write32() - Write to a 32-bit register in a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + * @val: Value to write.
> > + *
> > + * The wrapper macro PVR_CR_WRITE32() may be used instead to simplify the
> > + * selection of @reg.
> > + */
> > +static __always_inline void
> > +pvr_cr_write32(struct pvr_device *pvr_dev, u32 reg, u32 val)
> > +{
> > +	iowrite32(val, pvr_dev->regs + reg);
> > +}
> > +
> > +/**
> > + * pvr_cr_write64() - Write to a 64-bit register in a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg: Target register.
> > + * @val: Value to write.
> > + *
> > + * The wrapper macro PVR_CR_WRITE64() may be used instead to simplify the
> > + * selection of @reg.
> > + */
> > +static __always_inline void
> > +pvr_cr_write64(struct pvr_device *pvr_dev, u32 reg, u64 val)
> > +{
> > +	iowrite64(val, pvr_dev->regs + reg);
> > +}
> > +
> > +/**
> > + * pvr_cr_poll_reg32() - Wait for a 32-bit register to match a given value by
> > + *                       polling
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg_addr: Address of register.
> > + * @reg_value: Expected register value (after masking).
> > + * @reg_mask: Mask of bits valid for comparison with @reg_value.
> > + * @timeout_usec: Timeout length, in us.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * -%ETIMEDOUT on timeout.
> > + */
> > +static __always_inline int
> > +pvr_cr_poll_reg32(struct pvr_device *pvr_dev, u32 reg_addr, u32 reg_value,
> > +		  u32 reg_mask, u64 timeout_usec)
> > +{
> > +	u32 value;
> > +
> > +	return readl_poll_timeout(pvr_dev->regs + reg_addr, value,
> > +		(value & reg_mask) == reg_value, 0, timeout_usec);
> > +}
> > +
> > +/**
> > + * pvr_cr_poll_reg64() - Wait for a 64-bit register to match a given value by
> > + *                       polling
> > + * @pvr_dev: Target PowerVR device.
> > + * @reg_addr: Address of register.
> > + * @reg_value: Expected register value (after masking).
> > + * @reg_mask: Mask of bits valid for comparison with @reg_value.
> > + * @timeout_usec: Timeout length, in us.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * -%ETIMEDOUT on timeout.
> > + */
> > +static __always_inline int
> > +pvr_cr_poll_reg64(struct pvr_device *pvr_dev, u32 reg_addr, u64 reg_value,
> > +		  u64 reg_mask, u64 timeout_usec)
> > +{
> > +	u64 value;
> > +
> > +	return readq_poll_timeout(pvr_dev->regs + reg_addr, value,
> > +		(value & reg_mask) == reg_value, 0, timeout_usec);
> > +}
> > +
> >   /**
> >    * DOC: IOCTL validation helpers
> >    *
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> > index e203a2370f55..48a870715426 100644
> > --- a/drivers/gpu/drm/imagination/pvr_drv.c
> > +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> > @@ -485,12 +485,19 @@ pvr_probe(struct platform_device *plat_dev)
> >   
> >   	platform_set_drvdata(plat_dev, drm_dev);
> >   
> > -	err = drm_dev_register(drm_dev, 0);
> > +	err = pvr_device_init(pvr_dev);
> >   	if (err)
> >   		goto err_out;
> >   
> > +	err = drm_dev_register(drm_dev, 0);
> > +	if (err)
> > +		goto err_device_fini;
> > +
> >   	return 0;
> >   
> > +err_device_fini:
> > +	pvr_device_fini(pvr_dev);
> > +
> >   err_out:
> >   	return err;
> >   }
> > @@ -499,8 +506,10 @@ static int
> >   pvr_remove(struct platform_device *plat_dev)
> >   {
> >   	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> > +	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
> >   
> >   	drm_dev_unregister(drm_dev);
> > +	pvr_device_fini(pvr_dev);
> >   
> >   	return 0;
> >   }

Maxime Ripard July 7, 2023, 12:47 p.m. UTC | #3

On Tue, Jun 13, 2023 at 03:47:48PM +0100, Sarah Walker wrote:
> Acquire clock, regulator and register resources, and enable/map as
> appropriate.
>
> Signed-off-by: Sarah Walker <[e-mail]>
> ---
>  drivers/gpu/drm/imagination/Makefile     |   1 +
>  drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
>  drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
>  drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
>  4 files changed, 496 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
>
> diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
> index 62ccf0ccbd51..186f920d615b 100644
> --- a/drivers/gpu/drm/imagination/Makefile
> +++ b/drivers/gpu/drm/imagination/Makefile
> @@ -4,6 +4,7 @@
>  subdir-ccflags-y := -I$(srctree)/$(src)
>
>  powervr-y := \
> +     pvr_device.o \
>       pvr_drv.o \
>
>  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> new file mode 100644
> index 000000000000..790c36cebec1
> --- /dev/null
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> +
> +#include "pvr_device.h"
> +
> +#include <drm/drm_print.h>
> +
> +#include <linux/clk.h>
> +#include <linux/compiler_attributes.h>
> +#include <linux/compiler_types.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +
> +/**
> + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> + * control registers.
> + * @pvr_dev: Target PowerVR device.
> + *
> + * Sets struct pvr_device->regs.
> + *
> + * This method of mapping the device control registers into memory ensures that
> + * they are unmapped when the driver is detached (i.e. no explicit cleanup is
> + * required).
> + *
> + * Return:
> + *  * 0 on success, or
> + *  * Any error returned by devm_platform_ioremap_resource().
> + */
> +static int
> +pvr_device_reg_init(struct pvr_device *pvr_dev)
> +{
> +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +     struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> +     struct resource *regs_resource;
> +     void __iomem *regs;
> +     int err;
> +
> +     pvr_dev->regs_resource = NULL;
> +     pvr_dev->regs = NULL;
> +
> +     regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
> +     if (IS_ERR(regs)) {
> +             err = PTR_ERR(regs);
> +             drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> +                     err);
> +             return err;
> +     }
> +
> +     pvr_dev->regs = regs;
> +     pvr_dev->regs_resource = regs_resource;

Do you actually need the resources somewhere?

> +
> +     return 0;
> +}
> +
> +/**
> + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> + * control registers.
> + * @pvr_dev: Target PowerVR device.
> + *
> + * This is essentially a no-op, since pvr_device_reg_init() already ensures that
> + * struct pvr_device->regs is unmapped when the device is detached. This
> + * function just sets struct pvr_device->regs to %NULL.
> + */
> +static __always_inline void
> +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> +{
> +     pvr_dev->regs = NULL;

But if you do, I guess clearing the regs_resource pointer would be nice too.

> +}
> +
> +/**
> + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> + * struct pvr_device->mem_clk.
> + *
> + * Three clocks are required by the PowerVR device: core, sys and mem. On
> + * return, this function guarantees that the clocks are in one of the following
> + * states:
> + *
> + *  * All successfully initialized,
> + *  * Core errored, sys and mem uninitialized,
> + *  * Core deinitialized, sys errored, mem uninitialized, or
> + *  * Core and sys deinitialized, mem errored.
> + *
> + * Return:
> + *  * 0 on success,
> + *  * Any error returned by devm_clk_get(), or
> + *  * Any error returned by clk_prepare_enable().
> + */
> +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> +{
> +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +     struct clk *core_clk;
> +     struct clk *sys_clk;
> +     struct clk *mem_clk;
> +     int err;
> +
> +     pvr_dev->core_clk = NULL;
> +     pvr_dev->sys_clk = NULL;
> +     pvr_dev->mem_clk = NULL;
> +
> +     core_clk = devm_clk_get(drm_dev->dev, "core");
> +     if (IS_ERR(core_clk)) {
> +             err = PTR_ERR(core_clk);
> +             drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
> +             goto err_out;
> +     }
> +
> +     sys_clk = devm_clk_get(drm_dev->dev, "sys");
> +     if (IS_ERR(sys_clk))
> +             sys_clk = NULL;
> +
> +     mem_clk = devm_clk_get(drm_dev->dev, "mem");
> +     if (IS_ERR(mem_clk))
> +             mem_clk = NULL;

If those two are optionals, you can use devm_clk_get_optional. This has
the advantage of only ignoring the case where the clock isn't there, but
not the other error conditions.

> +
> +     err = clk_prepare(core_clk);
> +     if (err)
> +             goto err_out;
> +
> +     if (sys_clk) {
> +             err = clk_prepare(sys_clk);

It's valid to call clk_prepare(NULL), which will be a nop. I think you
can remove the check on the null pointer.

Also do you need to split prepare and enable? The usual reason to do so
is that prepare can sleep and thus can't be called in an atomic context,
but enable can. I can't think of a case where that would happen for a
GPU though, so you should probably do both at once?

You should also consider using the devm variants there like
devm_clk_get_optional_prepared and similar, depending on how you address
the comments above. This will greatly simplify your exit / cleanup path.

> +             if (err)
> +                     goto err_deinit_core_clk;
> +     }
> +
> +     if (mem_clk) {
> +             err = clk_prepare(mem_clk);
> +             if (err)
> +                     goto err_deinit_sys_clk;
> +     }
> +
> +     pvr_dev->core_clk = core_clk;
> +     pvr_dev->sys_clk = sys_clk;
> +     pvr_dev->mem_clk = mem_clk;
> +
> +     return 0;
> +
> +err_deinit_sys_clk:
> +     if (sys_clk)
> +             clk_disable_unprepare(sys_clk);
> +err_deinit_core_clk:
> +     clk_disable_unprepare(core_clk);

Since the clocks haven't been enabled yet, this will create an enable
count imbalance.

> +/**
> + * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * The regulator is not a required devicetree property. If it is not present then this function will
> + * succeed, but &pvr_device->regulator will be %NULL.
> + *
> + * Returns:
> + *  * 0 on success, or
> + *  * Any error returned by devm_regulator_get().
> + */
> +static int
> +pvr_device_regulator_init(struct pvr_device *pvr_dev)
> +{
> +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> +     struct regulator *regulator;
> +     int err;
> +
> +     regulator = devm_regulator_get(drm_dev->dev, "power");
> +     if (IS_ERR(regulator)) {
> +             err = PTR_ERR(regulator);
> +             /* Regulator is not required, so ENODEV is allowed here. */
> +             if (err != -ENODEV)
> +                     goto err_out;

This is what regulator_get_optional does :)

> +/**
> + * pvr_device_init() - Initialize a PowerVR device
> + * @pvr_dev: Target PowerVR device.
> + *
> + * If this function returns successfully, the device will have been fully
> + * initialized. Otherwise, any parts of the device initialized before an error
> + * occurs will be de-initialized before returning.
> + *
> + * NOTE: The initialization steps currently taken are the bare minimum required
> + *       to read from the control registers. The device is unlikely to function
> + *       until further initialization steps are added. [This note should be
> + *       removed when that happens.]
> + *
> + * Return:
> + *  * 0 on success,
> + *  * Any error returned by pvr_device_reg_init(),
> + *  * Any error returned by pvr_device_clk_init(), or
> + *  * Any error returned by pvr_device_gpu_init().
> + */
> +int
> +pvr_device_init(struct pvr_device *pvr_dev)
> +{
> +     int err;
> +
> +     /* Enable and initialize clocks required for the device to operate. */
> +     err = pvr_device_clk_init(pvr_dev);
> +     if (err)
> +             goto err_out;
> +
> +     err = pvr_device_regulator_init(pvr_dev);
> +     if (err)
> +             goto err_device_clk_fini;
> +
> +     /* Map the control registers into memory. */
> +     err = pvr_device_reg_init(pvr_dev);
> +     if (err)
> +             goto err_device_reg_fini;
> +
> +     return 0;
> +
> +err_device_reg_fini:
> +     pvr_device_reg_fini(pvr_dev);

I think you got that one wrong, I don't think you should call
pvr_device_reg_fini if pvr_device_reg_init failed?

But switching to devm will solve this too.

>  /**
>   * struct pvr_device - powervr-specific wrapper for &struct drm_device
> @@ -26,6 +42,29 @@ struct pvr_device {
>        * from_pvr_device().
>        */
>       struct drm_device base;
> +
> +     /** @regs_resource: Resource representing device control registers. */
> +     struct resource *regs_resource;
> +
> +     /**
> +      * @regs: Device control registers.
> +      *
> +      * These are mapped into memory when the device is initialized; that
> +      * location is where this pointer points.
> +      */
> +     void __iomem *regs;
> +
> +     /** @core_clk: General core clock. */
> +     struct clk *core_clk;
> +
> +     /** @sys_clk: System bus clock. */
> +     struct clk *sys_clk;
> +
> +     /** @mem_clk: Memory clock. */
> +     struct clk *mem_clk;

It's not really a review but more of a suggestion: the semantics around
the clocks vary from one vendor to another, so having a bit more context
here in what those clocks are used for by the hardware and how we should
use them in the driver would be great.

> +     /** @regulator: Power regulator. */
> +     struct regulator *regulator;
>  };
>
>  /**
> @@ -72,6 +111,181 @@ to_pvr_file(struct drm_file *file)
>       return file->driver_priv;
>  }
>
> +int pvr_device_init(struct pvr_device *pvr_dev);
> +void pvr_device_fini(struct pvr_device *pvr_dev);
> +
> +int
> +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);

Do we really need a wrapper around clk_get_rate(pvr_dev->core_clk) ?

Also, this function is defined in patch 7.

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index e203a2370f55..48a870715426 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -485,12 +485,19 @@ pvr_probe(struct platform_device *plat_dev)
>
>       platform_set_drvdata(plat_dev, drm_dev);
>
> -     err = drm_dev_register(drm_dev, 0);
> +     err = pvr_device_init(pvr_dev);
>       if (err)
>               goto err_out;
>
> +     err = drm_dev_register(drm_dev, 0);
> +     if (err)
> +             goto err_device_fini;
> +
>       return 0;
>
> +err_device_fini:
> +     pvr_device_fini(pvr_dev);
> +
>  err_out:
>       return err;
>  }
> @@ -499,8 +506,10 @@ static int
>  pvr_remove(struct platform_device *plat_dev)
>  {
>       struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> +     struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
>
>       drm_dev_unregister(drm_dev);
> +     pvr_device_fini(pvr_dev);
>
>       return 0;
>  }

There's one gotcha you'll need to consider: DRM devices are unregistered
and freed not when the device goes away but when the last (userspace)
application closed its file descriptor.

This means that you have an undefined window during which devm will have
run (and thus cleaned up the resources) but the driver is still live and
can still get called.

So you need to protect all device resources access (registers, clocks
and regulator in your case I guess?) by calls to
drm_dev_enter/drm_dev_exit and use drm_dev_unplug instead of
drm_dev_unregister.

Maxime

Frank Binns July 14, 2023, 1:39 p.m. UTC | #4

Hi Maxime,

On Fri, 2023-07-07 at 14:47 +0200, Maxime Ripard wrote:
> On Tue, Jun 13, 2023 at 03:47:48PM +0100, Sarah Walker wrote:
> > Acquire clock, regulator and register resources, and enable/map as
> > appropriate.
> > 
> > Signed-off-by: Sarah Walker <[e-mail]>
> > ---
> >  drivers/gpu/drm/imagination/Makefile     |   1 +
> >  drivers/gpu/drm/imagination/pvr_device.c | 271 +++++++++++++++++++++++
> >  drivers/gpu/drm/imagination/pvr_device.h | 214 ++++++++++++++++++
> >  drivers/gpu/drm/imagination/pvr_drv.c    |  11 +-
> >  4 files changed, 496 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/imagination/pvr_device.c
> > 
> > diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
> > index 62ccf0ccbd51..186f920d615b 100644
> > --- a/drivers/gpu/drm/imagination/Makefile
> > +++ b/drivers/gpu/drm/imagination/Makefile
> > @@ -4,6 +4,7 @@
> >  subdir-ccflags-y := -I$(srctree)/$(src)
> > 
> >  powervr-y := \
> > +     pvr_device.o \
> >       pvr_drv.o \
> > 
> >  obj-$(CONFIG_DRM_POWERVR) += powervr.o
> > diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> > new file mode 100644
> > index 000000000000..790c36cebec1
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imagination/pvr_device.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/* Copyright (c) 2022 Imagination Technologies Ltd. */
> > +
> > +#include "pvr_device.h"
> > +
> > +#include <drm/drm_print.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/compiler_attributes.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/stddef.h>
> > +#include <linux/types.h>
> > +
> > +/**
> > + * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->regs.
> > + *
> > + * This method of mapping the device control registers into memory ensures that
> > + * they are unmapped when the driver is detached (i.e. no explicit cleanup is
> > + * required).
> > + *
> > + * Return:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_platform_ioremap_resource().
> > + */
> > +static int
> > +pvr_device_reg_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
> > +     struct resource *regs_resource;
> > +     void __iomem *regs;
> > +     int err;
> > +
> > +     pvr_dev->regs_resource = NULL;
> > +     pvr_dev->regs = NULL;
> > +
> > +     regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
> > +     if (IS_ERR(regs)) {
> > +             err = PTR_ERR(regs);
> > +             drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
> > +                     err);
> > +             return err;
> > +     }
> > +
> > +     pvr_dev->regs = regs;
> > +     pvr_dev->regs_resource = regs_resource;
> 
> Do you actually need the resources somewhere?

This is needed when setting up the boot data for the MIPS firmware processor in
patch 11 (drm/imagination: Implement MIPS firmware processor and MMU support).
We'll move it to that patch instead.

> 
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
> > + * control registers.
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * This is essentially a no-op, since pvr_device_reg_init() already ensures that
> > + * struct pvr_device->regs is unmapped when the device is detached. This
> > + * function just sets struct pvr_device->regs to %NULL.
> > + */
> > +static __always_inline void
> > +pvr_device_reg_fini(struct pvr_device *pvr_dev)
> > +{
> > +     pvr_dev->regs = NULL;
> 
> But if you do, I guess clearing the regs_resource pointer would be nice too.

pvr_device_reg_fini() will be gone in the next version.

> 
> > +}
> > +
> > +/**
> > + * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
> > + * struct pvr_device->mem_clk.
> > + *
> > + * Three clocks are required by the PowerVR device: core, sys and mem. On
> > + * return, this function guarantees that the clocks are in one of the following
> > + * states:
> > + *
> > + *  * All successfully initialized,
> > + *  * Core errored, sys and mem uninitialized,
> > + *  * Core deinitialized, sys errored, mem uninitialized, or
> > + *  * Core and sys deinitialized, mem errored.
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by devm_clk_get(), or
> > + *  * Any error returned by clk_prepare_enable().
> > + */
> > +static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct clk *core_clk;
> > +     struct clk *sys_clk;
> > +     struct clk *mem_clk;
> > +     int err;
> > +
> > +     pvr_dev->core_clk = NULL;
> > +     pvr_dev->sys_clk = NULL;
> > +     pvr_dev->mem_clk = NULL;
> > +
> > +     core_clk = devm_clk_get(drm_dev->dev, "core");
> > +     if (IS_ERR(core_clk)) {
> > +             err = PTR_ERR(core_clk);
> > +             drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
> > +             goto err_out;
> > +     }
> > +
> > +     sys_clk = devm_clk_get(drm_dev->dev, "sys");
> > +     if (IS_ERR(sys_clk))
> > +             sys_clk = NULL;
> > +
> > +     mem_clk = devm_clk_get(drm_dev->dev, "mem");
> > +     if (IS_ERR(mem_clk))
> > +             mem_clk = NULL;
> 
> If those two are optionals, you can use devm_clk_get_optional. This has
> the advantage of only ignoring the case where the clock isn't there, but
> not the other error conditions.

Ack

> 
> > +
> > +     err = clk_prepare(core_clk);
> > +     if (err)
> > +             goto err_out;
> > +
> > +     if (sys_clk) {
> > +             err = clk_prepare(sys_clk);
> 
> It's valid to call clk_prepare(NULL), which will be a nop. I think you
> can remove the check on the null pointer.

Ack

> 
> Also do you need to split prepare and enable? The usual reason to do so
> is that prepare can sleep and thus can't be called in an atomic context,
> but enable can. I can't think of a case where that would happen for a
> GPU though, so you should probably do both at once?

No, we don't need the split, so we'll fix this.

> 
> You should also consider using the devm variants there like
> devm_clk_get_optional_prepared and similar, depending on how you address
> the comments above. This will greatly simplify your exit / cleanup path.

Ack

> 
> > +             if (err)
> > +                     goto err_deinit_core_clk;
> > +     }
> > +
> > +     if (mem_clk) {
> > +             err = clk_prepare(mem_clk);
> > +             if (err)
> > +                     goto err_deinit_sys_clk;
> > +     }
> > +
> > +     pvr_dev->core_clk = core_clk;
> > +     pvr_dev->sys_clk = sys_clk;
> > +     pvr_dev->mem_clk = mem_clk;
> > +
> > +     return 0;
> > +
> > +err_deinit_sys_clk:
> > +     if (sys_clk)
> > +             clk_disable_unprepare(sys_clk);
> > +err_deinit_core_clk:
> > +     clk_disable_unprepare(core_clk);
> 
> Since the clocks haven't been enabled yet, this will create an enable
> count imbalance.

Ack

> 
> > +/**
> > + * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * The regulator is not a required devicetree property. If it is not present then this function will
> > + * succeed, but &pvr_device->regulator will be %NULL.
> > + *
> > + * Returns:
> > + *  * 0 on success, or
> > + *  * Any error returned by devm_regulator_get().
> > + */
> > +static int
> > +pvr_device_regulator_init(struct pvr_device *pvr_dev)
> > +{
> > +     struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> > +     struct regulator *regulator;
> > +     int err;
> > +
> > +     regulator = devm_regulator_get(drm_dev->dev, "power");
> > +     if (IS_ERR(regulator)) {
> > +             err = PTR_ERR(regulator);
> > +             /* Regulator is not required, so ENODEV is allowed here. */
> > +             if (err != -ENODEV)
> > +                     goto err_out;
> 
> This is what regulator_get_optional does :)

Thanks for point it out :)

> 
> > +/**
> > + * pvr_device_init() - Initialize a PowerVR device
> > + * @pvr_dev: Target PowerVR device.
> > + *
> > + * If this function returns successfully, the device will have been fully
> > + * initialized. Otherwise, any parts of the device initialized before an error
> > + * occurs will be de-initialized before returning.
> > + *
> > + * NOTE: The initialization steps currently taken are the bare minimum required
> > + *       to read from the control registers. The device is unlikely to function
> > + *       until further initialization steps are added. [This note should be
> > + *       removed when that happens.]
> > + *
> > + * Return:
> > + *  * 0 on success,
> > + *  * Any error returned by pvr_device_reg_init(),
> > + *  * Any error returned by pvr_device_clk_init(), or
> > + *  * Any error returned by pvr_device_gpu_init().
> > + */
> > +int
> > +pvr_device_init(struct pvr_device *pvr_dev)
> > +{
> > +     int err;
> > +
> > +     /* Enable and initialize clocks required for the device to operate. */
> > +     err = pvr_device_clk_init(pvr_dev);
> > +     if (err)
> > +             goto err_out;
> > +
> > +     err = pvr_device_regulator_init(pvr_dev);
> > +     if (err)
> > +             goto err_device_clk_fini;
> > +
> > +     /* Map the control registers into memory. */
> > +     err = pvr_device_reg_init(pvr_dev);
> > +     if (err)
> > +             goto err_device_reg_fini;
> > +
> > +     return 0;
> > +
> > +err_device_reg_fini:
> > +     pvr_device_reg_fini(pvr_dev);
> 
> I think you got that one wrong, I don't think you should call
> pvr_device_reg_fini if pvr_device_reg_init failed?
> 
> But switching to devm will solve this too.

Ack

> 
> >  /**
> >   * struct pvr_device - powervr-specific wrapper for &struct drm_device
> > @@ -26,6 +42,29 @@ struct pvr_device {
> >        * from_pvr_device().
> >        */
> >       struct drm_device base;
> > +
> > +     /** @regs_resource: Resource representing device control registers. */
> > +     struct resource *regs_resource;
> > +
> > +     /**
> > +      * @regs: Device control registers.
> > +      *
> > +      * These are mapped into memory when the device is initialized; that
> > +      * location is where this pointer points.
> > +      */
> > +     void __iomem *regs;
> > +
> > +     /** @core_clk: General core clock. */
> > +     struct clk *core_clk;
> > +
> > +     /** @sys_clk: System bus clock. */
> > +     struct clk *sys_clk;
> > +
> > +     /** @mem_clk: Memory clock. */
> > +     struct clk *mem_clk;
> 
> It's not really a review but more of a suggestion: the semantics around
> the clocks vary from one vendor to another, so having a bit more context
> here in what those clocks are used for by the hardware and how we should
> use them in the driver would be great.

We'll flesh this out a bit more in the next version.

> 
> > +     /** @regulator: Power regulator. */
> > +     struct regulator *regulator;
> >  };
> > 
> >  /**
> > @@ -72,6 +111,181 @@ to_pvr_file(struct drm_file *file)
> >       return file->driver_priv;
> >  }
> > 
> > +int pvr_device_init(struct pvr_device *pvr_dev);
> > +void pvr_device_fini(struct pvr_device *pvr_dev);
> > +
> > +int
> > +pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);
> 
> Do we really need a wrapper around clk_get_rate(pvr_dev->core_clk) ?

No, we'll drop this.

> 
> Also, this function is defined in patch 7.
> 
> > diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> > index e203a2370f55..48a870715426 100644
> > --- a/drivers/gpu/drm/imagination/pvr_drv.c
> > +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> > @@ -485,12 +485,19 @@ pvr_probe(struct platform_device *plat_dev)
> > 
> >       platform_set_drvdata(plat_dev, drm_dev);
> > 
> > -     err = drm_dev_register(drm_dev, 0);
> > +     err = pvr_device_init(pvr_dev);
> >       if (err)
> >               goto err_out;
> > 
> > +     err = drm_dev_register(drm_dev, 0);
> > +     if (err)
> > +             goto err_device_fini;
> > +
> >       return 0;
> > 
> > +err_device_fini:
> > +     pvr_device_fini(pvr_dev);
> > +
> >  err_out:
> >       return err;
> >  }
> > @@ -499,8 +506,10 @@ static int
> >  pvr_remove(struct platform_device *plat_dev)
> >  {
> >       struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> > +     struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
> > 
> >       drm_dev_unregister(drm_dev);
> > +     pvr_device_fini(pvr_dev);
> > 
> >       return 0;
> >  }
> 
> There's one gotcha you'll need to consider: DRM devices are unregistered
> and freed not when the device goes away but when the last (userspace)
> application closed its file descriptor.
> 
> This means that you have an undefined window during which devm will have
> run (and thus cleaned up the resources) but the driver is still live and
> can still get called.
> 
> So you need to protect all device resources access (registers, clocks
> and regulator in your case I guess?) by calls to
> drm_dev_enter/drm_dev_exit and use drm_dev_unplug instead of
> drm_dev_unregister.

Thanks for highlighting this. We'll make sure we use these functions in the next
version.

Thanks
Frank

> 
> Maxime

Patch

diff --git a/drivers/gpu/drm/imagination/Makefile b/drivers/gpu/drm/imagination/Makefile
index 62ccf0ccbd51..186f920d615b 100644
--- a/drivers/gpu/drm/imagination/Makefile
+++ b/drivers/gpu/drm/imagination/Makefile
@@ -4,6 +4,7 @@ 
 subdir-ccflags-y := -I$(srctree)/$(src)
 
 powervr-y := \
+	pvr_device.o \
 	pvr_drv.o \
 
 obj-$(CONFIG_DRM_POWERVR) += powervr.o
diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
new file mode 100644
index 000000000000..790c36cebec1
--- /dev/null
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -0,0 +1,271 @@ 
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/* Copyright (c) 2022 Imagination Technologies Ltd. */
+
+#include "pvr_device.h"
+
+#include <drm/drm_print.h>
+
+#include <linux/clk.h>
+#include <linux/compiler_attributes.h>
+#include <linux/compiler_types.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/gfp.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+/**
+ * pvr_device_reg_init() - Initialize kernel access to a PowerVR device's
+ * control registers.
+ * @pvr_dev: Target PowerVR device.
+ *
+ * Sets struct pvr_device->regs.
+ *
+ * This method of mapping the device control registers into memory ensures that
+ * they are unmapped when the driver is detached (i.e. no explicit cleanup is
+ * required).
+ *
+ * Return:
+ *  * 0 on success, or
+ *  * Any error returned by devm_platform_ioremap_resource().
+ */
+static int
+pvr_device_reg_init(struct pvr_device *pvr_dev)
+{
+	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+	struct platform_device *plat_dev = to_platform_device(drm_dev->dev);
+	struct resource *regs_resource;
+	void __iomem *regs;
+	int err;
+
+	pvr_dev->regs_resource = NULL;
+	pvr_dev->regs = NULL;
+
+	regs = devm_platform_get_and_ioremap_resource(plat_dev, 0, &regs_resource);
+	if (IS_ERR(regs)) {
+		err = PTR_ERR(regs);
+		drm_err(drm_dev, "failed to ioremap gpu registers (err=%d)\n",
+			err);
+		return err;
+	}
+
+	pvr_dev->regs = regs;
+	pvr_dev->regs_resource = regs_resource;
+
+	return 0;
+}
+
+/**
+ * pvr_device_reg_fini() - Deinitialize kernel access to a PowerVR device's
+ * control registers.
+ * @pvr_dev: Target PowerVR device.
+ *
+ * This is essentially a no-op, since pvr_device_reg_init() already ensures that
+ * struct pvr_device->regs is unmapped when the device is detached. This
+ * function just sets struct pvr_device->regs to %NULL.
+ */
+static __always_inline void
+pvr_device_reg_fini(struct pvr_device *pvr_dev)
+{
+	pvr_dev->regs = NULL;
+}
+
+/**
+ * pvr_device_clk_init() - Initialize clocks required by a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ *
+ * Sets struct pvr_device->core_clk, struct pvr_device->sys_clk and
+ * struct pvr_device->mem_clk.
+ *
+ * Three clocks are required by the PowerVR device: core, sys and mem. On
+ * return, this function guarantees that the clocks are in one of the following
+ * states:
+ *
+ *  * All successfully initialized,
+ *  * Core errored, sys and mem uninitialized,
+ *  * Core deinitialized, sys errored, mem uninitialized, or
+ *  * Core and sys deinitialized, mem errored.
+ *
+ * Return:
+ *  * 0 on success,
+ *  * Any error returned by devm_clk_get(), or
+ *  * Any error returned by clk_prepare_enable().
+ */
+static int pvr_device_clk_init(struct pvr_device *pvr_dev)
+{
+	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+	struct clk *core_clk;
+	struct clk *sys_clk;
+	struct clk *mem_clk;
+	int err;
+
+	pvr_dev->core_clk = NULL;
+	pvr_dev->sys_clk = NULL;
+	pvr_dev->mem_clk = NULL;
+
+	core_clk = devm_clk_get(drm_dev->dev, "core");
+	if (IS_ERR(core_clk)) {
+		err = PTR_ERR(core_clk);
+		drm_err(drm_dev, "failed to get core clock (err=%d)\n", err);
+		goto err_out;
+	}
+
+	sys_clk = devm_clk_get(drm_dev->dev, "sys");
+	if (IS_ERR(sys_clk))
+		sys_clk = NULL;
+
+	mem_clk = devm_clk_get(drm_dev->dev, "mem");
+	if (IS_ERR(mem_clk))
+		mem_clk = NULL;
+
+	err = clk_prepare(core_clk);
+	if (err)
+		goto err_out;
+
+	if (sys_clk) {
+		err = clk_prepare(sys_clk);
+		if (err)
+			goto err_deinit_core_clk;
+	}
+
+	if (mem_clk) {
+		err = clk_prepare(mem_clk);
+		if (err)
+			goto err_deinit_sys_clk;
+	}
+
+	pvr_dev->core_clk = core_clk;
+	pvr_dev->sys_clk = sys_clk;
+	pvr_dev->mem_clk = mem_clk;
+
+	return 0;
+
+err_deinit_sys_clk:
+	if (sys_clk)
+		clk_disable_unprepare(sys_clk);
+err_deinit_core_clk:
+	clk_disable_unprepare(core_clk);
+err_out:
+	return err;
+}
+
+/**
+ * pvr_device_clk_fini() - Deinitialize clocks required by a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ */
+static void
+pvr_device_clk_fini(struct pvr_device *pvr_dev)
+{
+	if (pvr_dev->mem_clk)
+		clk_unprepare(pvr_dev->mem_clk);
+	if (pvr_dev->sys_clk)
+		clk_unprepare(pvr_dev->sys_clk);
+	clk_unprepare(pvr_dev->core_clk);
+
+	pvr_dev->core_clk = NULL;
+	pvr_dev->sys_clk = NULL;
+	pvr_dev->mem_clk = NULL;
+}
+
+/**
+ * pvr_device_regulator_init() - Initialise regulator required by a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ *
+ * The regulator is not a required devicetree property. If it is not present then this function will
+ * succeed, but &pvr_device->regulator will be %NULL.
+ *
+ * Returns:
+ *  * 0 on success, or
+ *  * Any error returned by devm_regulator_get().
+ */
+static int
+pvr_device_regulator_init(struct pvr_device *pvr_dev)
+{
+	struct drm_device *drm_dev = from_pvr_device(pvr_dev);
+	struct regulator *regulator;
+	int err;
+
+	regulator = devm_regulator_get(drm_dev->dev, "power");
+	if (IS_ERR(regulator)) {
+		err = PTR_ERR(regulator);
+		/* Regulator is not required, so ENODEV is allowed here. */
+		if (err != -ENODEV)
+			goto err_out;
+		regulator = NULL;
+	}
+
+	pvr_dev->regulator = regulator;
+
+	return 0;
+
+err_out:
+	return err;
+}
+
+/**
+ * pvr_device_init() - Initialize a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ *
+ * If this function returns successfully, the device will have been fully
+ * initialized. Otherwise, any parts of the device initialized before an error
+ * occurs will be de-initialized before returning.
+ *
+ * NOTE: The initialization steps currently taken are the bare minimum required
+ *       to read from the control registers. The device is unlikely to function
+ *       until further initialization steps are added. [This note should be
+ *       removed when that happens.]
+ *
+ * Return:
+ *  * 0 on success,
+ *  * Any error returned by pvr_device_reg_init(),
+ *  * Any error returned by pvr_device_clk_init(), or
+ *  * Any error returned by pvr_device_gpu_init().
+ */
+int
+pvr_device_init(struct pvr_device *pvr_dev)
+{
+	int err;
+
+	/* Enable and initialize clocks required for the device to operate. */
+	err = pvr_device_clk_init(pvr_dev);
+	if (err)
+		goto err_out;
+
+	err = pvr_device_regulator_init(pvr_dev);
+	if (err)
+		goto err_device_clk_fini;
+
+	/* Map the control registers into memory. */
+	err = pvr_device_reg_init(pvr_dev);
+	if (err)
+		goto err_device_reg_fini;
+
+	return 0;
+
+err_device_reg_fini:
+	pvr_device_reg_fini(pvr_dev);
+
+err_device_clk_fini:
+	pvr_device_clk_fini(pvr_dev);
+
+err_out:
+	return err;
+}
+
+/**
+ * pvr_device_fini() - Deinitialize a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ */
+void
+pvr_device_fini(struct pvr_device *pvr_dev)
+{
+	/*
+	 * Deinitialization stages are performed in reverse order compared to
+	 * the initialization stages in pvr_device_init().
+	 */
+	pvr_device_reg_fini(pvr_dev);
+	pvr_device_clk_fini(pvr_dev);
+}
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 3d2865d726b8..c0dd0562844c 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -11,9 +11,25 @@ 
 #include <linux/bits.h>
 #include <linux/compiler_attributes.h>
 #include <linux/compiler_types.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/mutex.h>
+#include <linux/timer.h>
 #include <linux/types.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
+#include <linux/xarray.h>
+
+/* Forward declaration from <linux/clk.h>. */
+struct clk;
+
+/* Forward declaration from <linux/firmware.h>. */
+struct firmware;
+
+/* Forward declaration from <linux/regulator/consumer.h>. */
+struct regulator;
 
 /**
  * struct pvr_device - powervr-specific wrapper for &struct drm_device
@@ -26,6 +42,29 @@  struct pvr_device {
 	 * from_pvr_device().
 	 */
 	struct drm_device base;
+
+	/** @regs_resource: Resource representing device control registers. */
+	struct resource *regs_resource;
+
+	/**
+	 * @regs: Device control registers.
+	 *
+	 * These are mapped into memory when the device is initialized; that
+	 * location is where this pointer points.
+	 */
+	void __iomem *regs;
+
+	/** @core_clk: General core clock. */
+	struct clk *core_clk;
+
+	/** @sys_clk: System bus clock. */
+	struct clk *sys_clk;
+
+	/** @mem_clk: Memory clock. */
+	struct clk *mem_clk;
+
+	/** @regulator: Power regulator. */
+	struct regulator *regulator;
 };
 
 /**
@@ -72,6 +111,181 @@  to_pvr_file(struct drm_file *file)
 	return file->driver_priv;
 }
 
+int pvr_device_init(struct pvr_device *pvr_dev);
+void pvr_device_fini(struct pvr_device *pvr_dev);
+
+int
+pvr_device_clk_core_get_freq(struct pvr_device *pvr_dev, u32 *freq_out);
+
+/**
+ * PVR_CR_READ32() - Read a 32-bit register from a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ *
+ * This macro is a wrapper around pvr_cr_read32(). It applies ROGUE_CR_ prefix
+ * to the provided @reg name, making it behave comparably to the
+ * PVR_CR_FIELD_GET() macro.
+ *
+ * Return: The value of the requested register.
+ */
+#define PVR_CR_READ32(pvr_dev, reg) pvr_cr_read32(pvr_dev, ROGUE_CR_##reg)
+
+/**
+ * PVR_CR_READ64() - Read a 64-bit register from a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ *
+ * This macro is a wrapper around pvr_cr_read64(). It applies ROGUE_CR_ prefix
+ * to the provided @reg name, making it behave comparably to the
+ * PVR_CR_FIELD_GET() macro.
+ *
+ * Return: The value of the requested register.
+ */
+#define PVR_CR_READ64(pvr_dev, reg) pvr_cr_read64(pvr_dev, ROGUE_CR_##reg)
+
+/**
+ * PVR_CR_WRITE32() - Write to a 32-bit register in a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ * @val: Value to write.
+ *
+ * This macro is a wrapper around pvr_cr_write32(). It applies ROGUE_CR_
+ * prefix to the provided @reg name, making it behave comparably to the
+ * PVR_CR_FIELD_GET() macro.
+ */
+#define PVR_CR_WRITE32(pvr_dev, reg, val) \
+	pvr_cr_write32(pvr_dev, ROGUE_CR_##reg, val)
+
+/**
+ * PVR_CR_WRITE64() - Write to a 64-bit register in a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ * @val: Value to write.
+ *
+ * This macro is a wrapper around pvr_cr_write64(). It applies ROGUE_CR_
+ * prefix to the provided @reg name, making it behave comparably to the
+ * PVR_CR_FIELD_GET() macro.
+ */
+#define PVR_CR_WRITE64(pvr_dev, reg, val) \
+	pvr_cr_write64(pvr_dev, ROGUE_CR_##reg, val)
+
+/**
+ * PVR_CR_FIELD_GET() - Extract a single field from a PowerVR control register
+ * @val: Value of the target register.
+ * @field: Field specifier, as defined in "pvr_rogue_cr_defs.h".
+ *
+ * Return: The extracted field.
+ */
+#define PVR_CR_FIELD_GET(val, field) FIELD_GET(~ROGUE_CR_##field##_CLRMSK, val)
+
+/**
+ * pvr_cr_read32() - Read a 32-bit register from a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ *
+ * The wrapper macro PVR_CR_READ32() may be used instead to simplify the
+ * selection of @reg.
+ *
+ * Return: The value of the requested register.
+ */
+static __always_inline u32
+pvr_cr_read32(struct pvr_device *pvr_dev, u32 reg)
+{
+	return ioread32(pvr_dev->regs + reg);
+}
+
+/**
+ * pvr_cr_read64() - Read a 64-bit register from a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ *
+ * The wrapper macro PVR_CR_READ64() may be used instead to simplify the
+ * selection of @reg.
+ *
+ * Return: The value of the requested register.
+ */
+static __always_inline u64
+pvr_cr_read64(struct pvr_device *pvr_dev, u32 reg)
+{
+	return ioread64(pvr_dev->regs + reg);
+}
+
+/**
+ * pvr_cr_write32() - Write to a 32-bit register in a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ * @val: Value to write.
+ *
+ * The wrapper macro PVR_CR_WRITE32() may be used instead to simplify the
+ * selection of @reg.
+ */
+static __always_inline void
+pvr_cr_write32(struct pvr_device *pvr_dev, u32 reg, u32 val)
+{
+	iowrite32(val, pvr_dev->regs + reg);
+}
+
+/**
+ * pvr_cr_write64() - Write to a 64-bit register in a PowerVR device
+ * @pvr_dev: Target PowerVR device.
+ * @reg: Target register.
+ * @val: Value to write.
+ *
+ * The wrapper macro PVR_CR_WRITE64() may be used instead to simplify the
+ * selection of @reg.
+ */
+static __always_inline void
+pvr_cr_write64(struct pvr_device *pvr_dev, u32 reg, u64 val)
+{
+	iowrite64(val, pvr_dev->regs + reg);
+}
+
+/**
+ * pvr_cr_poll_reg32() - Wait for a 32-bit register to match a given value by
+ *                       polling
+ * @pvr_dev: Target PowerVR device.
+ * @reg_addr: Address of register.
+ * @reg_value: Expected register value (after masking).
+ * @reg_mask: Mask of bits valid for comparison with @reg_value.
+ * @timeout_usec: Timeout length, in us.
+ *
+ * Returns:
+ *  * 0 on success, or
+ *  * -%ETIMEDOUT on timeout.
+ */
+static __always_inline int
+pvr_cr_poll_reg32(struct pvr_device *pvr_dev, u32 reg_addr, u32 reg_value,
+		  u32 reg_mask, u64 timeout_usec)
+{
+	u32 value;
+
+	return readl_poll_timeout(pvr_dev->regs + reg_addr, value,
+		(value & reg_mask) == reg_value, 0, timeout_usec);
+}
+
+/**
+ * pvr_cr_poll_reg64() - Wait for a 64-bit register to match a given value by
+ *                       polling
+ * @pvr_dev: Target PowerVR device.
+ * @reg_addr: Address of register.
+ * @reg_value: Expected register value (after masking).
+ * @reg_mask: Mask of bits valid for comparison with @reg_value.
+ * @timeout_usec: Timeout length, in us.
+ *
+ * Returns:
+ *  * 0 on success, or
+ *  * -%ETIMEDOUT on timeout.
+ */
+static __always_inline int
+pvr_cr_poll_reg64(struct pvr_device *pvr_dev, u32 reg_addr, u64 reg_value,
+		  u64 reg_mask, u64 timeout_usec)
+{
+	u64 value;
+
+	return readq_poll_timeout(pvr_dev->regs + reg_addr, value,
+		(value & reg_mask) == reg_value, 0, timeout_usec);
+}
+
 /**
  * DOC: IOCTL validation helpers
  *
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index e203a2370f55..48a870715426 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -485,12 +485,19 @@  pvr_probe(struct platform_device *plat_dev)
 
 	platform_set_drvdata(plat_dev, drm_dev);
 
-	err = drm_dev_register(drm_dev, 0);
+	err = pvr_device_init(pvr_dev);
 	if (err)
 		goto err_out;
 
+	err = drm_dev_register(drm_dev, 0);
+	if (err)
+		goto err_device_fini;
+
 	return 0;
 
+err_device_fini:
+	pvr_device_fini(pvr_dev);
+
 err_out:
 	return err;
 }
@@ -499,8 +506,10 @@  static int
 pvr_remove(struct platform_device *plat_dev)
 {
 	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
+	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
 
 	drm_dev_unregister(drm_dev);
+	pvr_device_fini(pvr_dev);
 
 	return 0;
 }