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 <sarah.wal...@imgtec.com>
> > ---
> > 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,
> > ®s_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;
> > }
