[PATCH v1 0/6] media: imx-pxp: Miscellaneous enhancements

[PATCH v1 0/6] media: imx-pxp: Miscellaneous enhancements

[Laurent Pinchart]

Hello,

This patch series brings miscellaneous enhancements to the imx-pxp
driver that I have kept in my tree for a too long time. As it's a
collection of miscellanous changes, please see individual patches for
details.

The patches are based on top of Michael's recent i.MX7D support for the
imx-pxp driver [1]. Michael, I can rebase this on top of your v2, or
feel free to take the patches in your tree already and include them in
v2 if you prefer.

[1] https://lore.kernel.org/linux-media/20230105134729.59542-1-m.tretter@pengutronix.de

Laurent Pinchart (6):
  media: imx-pxp: Sort headers alphabetically
  media: imx-pxp: Add media controller support
  media: imx-pxp: Pass pixel format value to find_format()
  media: imx-pxp: Implement frame size enumeration
  media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers
  media: imx-pxp: Use non-threaded IRQ

 drivers/media/platform/nxp/imx-pxp.c | 203 ++++++++++++++++++---------
 1 file changed, 136 insertions(+), 67 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v1 1/6] media: imx-pxp: Sort headers alphabetically

[Laurent Pinchart]

Sorting headers alphabetically helps locating duplicates, and make it
easier to figure out where to insert new headers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 04cc8df2a498..dcb04217288b 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -20,15 +20,15 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 
-#include <linux/platform_device.h>
-#include <media/v4l2-mem2mem.h>
-#include <media/v4l2-device.h>
-#include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-mem2mem.h>
 #include <media/videobuf2-dma-contig.h>
 
 #include "imx-pxp.h"
-- 
Regards,

Laurent Pinchart

[PATCH v1 2/6] media: imx-pxp: Add media controller support

[Laurent Pinchart]

Register a media device for the PXP, using the v4l2-mem2mem MC
infrastructure to populate the media graph. No media device operation is
implemented, the main use of the MC API is to allow consistent discovery
of media devices for userspace.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 37 ++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index dcb04217288b..132065c8b8b4 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -24,6 +24,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 
+#include <media/media-device.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -200,6 +201,9 @@ struct pxp_pdata {
 struct pxp_dev {
 	struct v4l2_device	v4l2_dev;
 	struct video_device	vfd;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device	mdev;
+#endif
 
 	struct clk		*clk;
 	void __iomem		*mmio;
@@ -1832,8 +1836,36 @@ static int pxp_probe(struct platform_device *pdev)
 		goto err_m2m;
 	}
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+	dev->mdev.dev = &pdev->dev;
+	strscpy(dev->mdev.model, MEM2MEM_NAME, sizeof(dev->mdev.model));
+	snprintf(dev->mdev.bus_info, sizeof(dev->mdev.bus_info), "platform:%s",
+		 dev_name(&pdev->dev));
+	media_device_init(&dev->mdev);
+	dev->v4l2_dev.mdev = &dev->mdev;
+
+	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
+						 MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize media device\n");
+		goto err_vfd;
+	}
+
+	ret = media_device_register(&dev->mdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register media device\n");
+		goto err_m2m_mc;
+	}
+#endif
+
 	return 0;
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+err_m2m_mc:
+	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
+err_vfd:
+	video_unregister_device(vfd);
+#endif
 err_m2m:
 	v4l2_m2m_release(dev->m2m_dev);
 err_v4l2:
@@ -1854,6 +1886,11 @@ static int pxp_remove(struct platform_device *pdev)
 	clk_disable_unprepare(dev->clk);
 
 	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	media_device_unregister(&dev->mdev);
+	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
+#endif
 	video_unregister_device(&dev->vfd);
 	v4l2_m2m_release(dev->m2m_dev);
 	v4l2_device_unregister(&dev->v4l2_dev);
-- 
Regards,

Laurent Pinchart

[PATCH v1 3/6] media: imx-pxp: Pass pixel format value to find_format()

[Laurent Pinchart]

The find_format() function looks up format information for a given pixel
format. It takes a v4l2_format pointer, but only uses the contained
pixel format value. To prepare it for being used by callers that don't
have v4l2_format, modify it to take the pixel format value directly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 132065c8b8b4..fd4c055c01eb 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -176,14 +176,14 @@ enum {
 	V4L2_M2M_DST = 1,
 };
 
-static struct pxp_fmt *find_format(struct v4l2_format *f)
+static struct pxp_fmt *find_format(unsigned int pixelformat)
 {
 	struct pxp_fmt *fmt;
 	unsigned int k;
 
 	for (k = 0; k < NUM_FORMATS; k++) {
 		fmt = &formats[k];
-		if (fmt->fourcc == f->fmt.pix.pixelformat)
+		if (fmt->fourcc == pixelformat)
 			break;
 	}
 
@@ -1268,10 +1268,10 @@ static int pxp_try_fmt_vid_cap(struct file *file, void *priv,
 	struct pxp_fmt *fmt;
 	struct pxp_ctx *ctx = file2ctx(file);
 
-	fmt = find_format(f);
+	fmt = find_format(f->fmt.pix.pixelformat);
 	if (!fmt) {
 		f->fmt.pix.pixelformat = formats[0].fourcc;
-		fmt = find_format(f);
+		fmt = find_format(f->fmt.pix.pixelformat);
 	}
 	if (!(fmt->types & MEM2MEM_CAPTURE)) {
 		v4l2_err(&ctx->dev->v4l2_dev,
@@ -1296,10 +1296,10 @@ static int pxp_try_fmt_vid_out(struct file *file, void *priv,
 	struct pxp_fmt *fmt;
 	struct pxp_ctx *ctx = file2ctx(file);
 
-	fmt = find_format(f);
+	fmt = find_format(f->fmt.pix.pixelformat);
 	if (!fmt) {
 		f->fmt.pix.pixelformat = formats[0].fourcc;
-		fmt = find_format(f);
+		fmt = find_format(f->fmt.pix.pixelformat);
 	}
 	if (!(fmt->types & MEM2MEM_OUTPUT)) {
 		v4l2_err(&ctx->dev->v4l2_dev,
@@ -1332,7 +1332,7 @@ static int pxp_s_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
 		return -EBUSY;
 	}
 
-	q_data->fmt		= find_format(f);
+	q_data->fmt		= find_format(f->fmt.pix.pixelformat);
 	q_data->width		= f->fmt.pix.width;
 	q_data->height		= f->fmt.pix.height;
 	q_data->bytesperline	= f->fmt.pix.bytesperline;
-- 
Regards,

Laurent Pinchart

[PATCH v1 4/6] media: imx-pxp: Implement frame size enumeration

[Laurent Pinchart]

Implement support for the VIDIOC_ENUM_FRAMESIZES ioctl.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index fd4c055c01eb..68f838e3069d 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -1391,6 +1391,26 @@ static int pxp_s_fmt_vid_out(struct file *file, void *priv,
 	return 0;
 }
 
+static int pxp_enum_framesizes(struct file *file, void *fh,
+			       struct v4l2_frmsizeenum *fsize)
+{
+	if (fsize->index > 0)
+		return -EINVAL;
+
+	if (!find_format(fsize->pixel_format))
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
+	fsize->stepwise.min_width = MIN_W;
+	fsize->stepwise.max_width = MAX_W;
+	fsize->stepwise.step_width = ALIGN_W;
+	fsize->stepwise.min_height = MIN_H;
+	fsize->stepwise.max_height = MAX_H;
+	fsize->stepwise.step_height = ALIGN_H;
+
+	return 0;
+}
+
 static u8 pxp_degrees_to_rot_mode(u32 degrees)
 {
 	switch (degrees) {
@@ -1459,6 +1479,8 @@ static const struct v4l2_ioctl_ops pxp_ioctl_ops = {
 	.vidioc_try_fmt_vid_out	= pxp_try_fmt_vid_out,
 	.vidioc_s_fmt_vid_out	= pxp_s_fmt_vid_out,
 
+	.vidioc_enum_framesizes	= pxp_enum_framesizes,
+
 	.vidioc_reqbufs		= v4l2_m2m_ioctl_reqbufs,
 	.vidioc_querybuf	= v4l2_m2m_ioctl_querybuf,
 	.vidioc_qbuf		= v4l2_m2m_ioctl_qbuf,
-- 
Regards,

Laurent Pinchart

[PATCH v1 5/6] media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers

[Laurent Pinchart]

Add pxp_read() and pxp_write() functions to wrap readl() and writel()
respectively. This can be useful for debugging register accesses.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 118 +++++++++++++++------------
 1 file changed, 64 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index 68f838e3069d..e4d7a6339929 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -253,6 +253,16 @@ static struct pxp_q_data *get_q_data(struct pxp_ctx *ctx,
 		return &ctx->q_data[V4L2_M2M_DST];
 }
 
+static inline u32 pxp_read(struct pxp_dev *dev, u32 reg)
+{
+	return readl(dev->mmio + reg);
+}
+
+static inline void pxp_write(struct pxp_dev *dev, u32 reg, u32 value)
+{
+	writel(value, dev->mmio + reg);
+}
+
 static u32 pxp_v4l2_pix_fmt_to_ps_format(u32 v4l2_pix_fmt)
 {
 	switch (v4l2_pix_fmt) {
@@ -505,11 +515,11 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
 				csc1_coef = csc1_coef_smpte240m_lim;
 		}
 
-		writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
-		writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
-		writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
+		pxp_write(dev, HW_PXP_CSC1_COEF0, csc1_coef[0]);
+		pxp_write(dev, HW_PXP_CSC1_COEF1, csc1_coef[1]);
+		pxp_write(dev, HW_PXP_CSC1_COEF2, csc1_coef[2]);
 	} else {
-		writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + HW_PXP_CSC1_COEF0);
+		pxp_write(dev, HW_PXP_CSC1_COEF0, BM_PXP_CSC1_COEF0_BYPASS);
 	}
 
 	if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) &&
@@ -725,15 +735,15 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
 				    BP_PXP_CSC2_CTRL_CSC_MODE;
 		}
 
-		writel(csc2_ctrl, dev->mmio + HW_PXP_CSC2_CTRL);
-		writel(csc2_coef[0], dev->mmio + HW_PXP_CSC2_COEF0);
-		writel(csc2_coef[1], dev->mmio + HW_PXP_CSC2_COEF1);
-		writel(csc2_coef[2], dev->mmio + HW_PXP_CSC2_COEF2);
-		writel(csc2_coef[3], dev->mmio + HW_PXP_CSC2_COEF3);
-		writel(csc2_coef[4], dev->mmio + HW_PXP_CSC2_COEF4);
-		writel(csc2_coef[5], dev->mmio + HW_PXP_CSC2_COEF5);
+		pxp_write(dev, HW_PXP_CSC2_CTRL, csc2_ctrl);
+		pxp_write(dev, HW_PXP_CSC2_COEF0, csc2_coef[0]);
+		pxp_write(dev, HW_PXP_CSC2_COEF1, csc2_coef[1]);
+		pxp_write(dev, HW_PXP_CSC2_COEF2, csc2_coef[2]);
+		pxp_write(dev, HW_PXP_CSC2_COEF3, csc2_coef[3]);
+		pxp_write(dev, HW_PXP_CSC2_COEF4, csc2_coef[4]);
+		pxp_write(dev, HW_PXP_CSC2_COEF5, csc2_coef[5]);
 	} else {
-		writel(BM_PXP_CSC2_CTRL_BYPASS, dev->mmio + HW_PXP_CSC2_CTRL);
+		pxp_write(dev, HW_PXP_CSC2_CTRL, BM_PXP_CSC2_CTRL_BYPASS);
 	}
 }
 
@@ -820,8 +830,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
 	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
 	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
 
-	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
-	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
+	pxp_write(dev, HW_PXP_DATA_PATH_CTRL0, ctrl0);
+	pxp_write(dev, HW_PXP_DATA_PATH_CTRL1, ctrl1);
 }
 
 static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
@@ -977,48 +987,48 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
 		   BF_PXP_PS_SCALE_XSCALE(xscale);
 	ps_offset = BF_PXP_PS_OFFSET_YOFFSET(0) | BF_PXP_PS_OFFSET_XOFFSET(0);
 
-	writel(ctrl, dev->mmio + HW_PXP_CTRL);
+	pxp_write(dev, HW_PXP_CTRL, ctrl);
 	/* skip STAT */
-	writel(out_ctrl, dev->mmio + HW_PXP_OUT_CTRL);
-	writel(out_buf, dev->mmio + HW_PXP_OUT_BUF);
-	writel(out_buf2, dev->mmio + HW_PXP_OUT_BUF2);
-	writel(out_pitch, dev->mmio + HW_PXP_OUT_PITCH);
-	writel(out_lrc, dev->mmio + HW_PXP_OUT_LRC);
-	writel(out_ps_ulc, dev->mmio + HW_PXP_OUT_PS_ULC);
-	writel(out_ps_lrc, dev->mmio + HW_PXP_OUT_PS_LRC);
-	writel(as_ulc, dev->mmio + HW_PXP_OUT_AS_ULC);
-	writel(as_lrc, dev->mmio + HW_PXP_OUT_AS_LRC);
-	writel(ps_ctrl, dev->mmio + HW_PXP_PS_CTRL);
-	writel(ps_buf, dev->mmio + HW_PXP_PS_BUF);
-	writel(ps_ubuf, dev->mmio + HW_PXP_PS_UBUF);
-	writel(ps_vbuf, dev->mmio + HW_PXP_PS_VBUF);
-	writel(ps_pitch, dev->mmio + HW_PXP_PS_PITCH);
-	writel(0x00ffffff, dev->mmio + HW_PXP_PS_BACKGROUND_0);
-	writel(ps_scale, dev->mmio + HW_PXP_PS_SCALE);
-	writel(ps_offset, dev->mmio + HW_PXP_PS_OFFSET);
+	pxp_write(dev, HW_PXP_OUT_CTRL, out_ctrl);
+	pxp_write(dev, HW_PXP_OUT_BUF, out_buf);
+	pxp_write(dev, HW_PXP_OUT_BUF2, out_buf2);
+	pxp_write(dev, HW_PXP_OUT_PITCH, out_pitch);
+	pxp_write(dev, HW_PXP_OUT_LRC, out_lrc);
+	pxp_write(dev, HW_PXP_OUT_PS_ULC, out_ps_ulc);
+	pxp_write(dev, HW_PXP_OUT_PS_LRC, out_ps_lrc);
+	pxp_write(dev, HW_PXP_OUT_AS_ULC, as_ulc);
+	pxp_write(dev, HW_PXP_OUT_AS_LRC, as_lrc);
+	pxp_write(dev, HW_PXP_PS_CTRL, ps_ctrl);
+	pxp_write(dev, HW_PXP_PS_BUF, ps_buf);
+	pxp_write(dev, HW_PXP_PS_UBUF, ps_ubuf);
+	pxp_write(dev, HW_PXP_PS_VBUF, ps_vbuf);
+	pxp_write(dev, HW_PXP_PS_PITCH, ps_pitch);
+	pxp_write(dev, HW_PXP_PS_BACKGROUND_0, 0x00ffffff);
+	pxp_write(dev, HW_PXP_PS_SCALE, ps_scale);
+	pxp_write(dev, HW_PXP_PS_OFFSET, ps_offset);
 	/* disable processed surface color keying */
-	writel(0x00ffffff, dev->mmio + HW_PXP_PS_CLRKEYLOW_0);
-	writel(0x00000000, dev->mmio + HW_PXP_PS_CLRKEYHIGH_0);
+	pxp_write(dev, HW_PXP_PS_CLRKEYLOW_0, 0x00ffffff);
+	pxp_write(dev, HW_PXP_PS_CLRKEYHIGH_0, 0x00000000);
 
 	/* disable alpha surface color keying */
-	writel(0x00ffffff, dev->mmio + HW_PXP_AS_CLRKEYLOW_0);
-	writel(0x00000000, dev->mmio + HW_PXP_AS_CLRKEYHIGH_0);
+	pxp_write(dev, HW_PXP_AS_CLRKEYLOW_0, 0x00ffffff);
+	pxp_write(dev, HW_PXP_AS_CLRKEYHIGH_0, 0x00000000);
 
 	/* setup CSC */
 	pxp_setup_csc(ctx);
 
 	/* bypass LUT */
-	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
+	pxp_write(dev, HW_PXP_LUT_CTRL, BM_PXP_LUT_CTRL_BYPASS);
 
 	pxp_set_data_path(ctx);
 
-	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
+	pxp_write(dev, HW_PXP_IRQ_MASK, 0xffff);
 
 	/* ungate, enable PS/AS/OUT and PXP operation */
-	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
-	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
-	       BM_PXP_CTRL_ENABLE_ROTATE0 |
-	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
+	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_IRQ_ENABLE);
+	pxp_write(dev, HW_PXP_CTRL_SET,
+		  BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
+		  BM_PXP_CTRL_ENABLE_ROTATE0 | BM_PXP_CTRL_ENABLE_PS_AS_OUT);
 
 	return 0;
 }
@@ -1091,23 +1101,23 @@ static irqreturn_t pxp_irq_handler(int irq, void *dev_id)
 	struct pxp_dev *dev = dev_id;
 	u32 stat;
 
-	stat = readl(dev->mmio + HW_PXP_STAT);
+	stat = pxp_read(dev, HW_PXP_STAT);
 
 	if (stat & BM_PXP_STAT_IRQ0) {
 		/* we expect x = 0, y = height, irq0 = 1 */
 		if (stat & ~(BM_PXP_STAT_BLOCKX | BM_PXP_STAT_BLOCKY |
 			     BM_PXP_STAT_IRQ0))
 			dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
-		writel(BM_PXP_STAT_IRQ0, dev->mmio + HW_PXP_STAT_CLR);
+		pxp_write(dev, HW_PXP_STAT_CLR, BM_PXP_STAT_IRQ0);
 
 		pxp_job_finish(dev);
 	} else {
-		u32 irq = readl(dev->mmio + HW_PXP_IRQ);
+		u32 irq = pxp_read(dev, HW_PXP_IRQ);
 
 		dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
 		dprintk(dev, "%s: irq = 0x%08x\n", __func__, irq);
 
-		writel(irq, dev->mmio + HW_PXP_IRQ_CLR);
+		pxp_write(dev, HW_PXP_IRQ_CLR, irq);
 	}
 
 	return IRQ_HANDLED;
@@ -1753,25 +1763,25 @@ static int pxp_soft_reset(struct pxp_dev *dev)
 	int ret;
 	u32 val;
 
-	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
-	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
+	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
+	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
 
-	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
+	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
 
 	ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
 				 val & BM_PXP_CTRL_CLKGATE, 0, 100);
 	if (ret < 0)
 		return ret;
 
-	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
-	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
+	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
+	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
 
 	return 0;
 }
 
 static u32 pxp_read_version(struct pxp_dev *dev)
 {
-	return readl(dev->mmio + HW_PXP_VERSION);
+	return pxp_read(dev, HW_PXP_VERSION);
 }
 
 static int pxp_probe(struct platform_device *pdev)
@@ -1902,8 +1912,8 @@ static int pxp_remove(struct platform_device *pdev)
 {
 	struct pxp_dev *dev = platform_get_drvdata(pdev);
 
-	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
-	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
+	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_CLKGATE);
+	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
 
 	clk_disable_unprepare(dev->clk);
 
-- 
Regards,

Laurent Pinchart

[PATCH v1 6/6] media: imx-pxp: Use non-threaded IRQ

[Laurent Pinchart]

There's no need to use threaded IRQs with the PXP, as the interrupt
handler doesn't need to sleep and doesn't perform any time-consuming
operation. Switch to regular IRQ handler.

This fixes lockups of the PXP noticed on i.MX7, where the PXP would stop
generating interrupts after a variable number of frames (from a few
dozens to a few hundreds). The root cause is however unknown.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index e4d7a6339929..28a222c8fe86 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -1814,8 +1814,8 @@ static int pxp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&dev->irqlock);
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, pxp_irq_handler,
-			IRQF_ONESHOT, dev_name(&pdev->dev), dev);
+	ret = devm_request_irq(&pdev->dev, irq, pxp_irq_handler,
+			       IRQF_TRIGGER_HIGH, dev_name(&pdev->dev), dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
 		return ret;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 5/6] media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers

[Alexander Stein]

Hi Laurent,

Am Freitag, 6. Januar 2023, 14:32:26 CET schrieb Laurent Pinchart:
> Add pxp_read() and pxp_write() functions to wrap readl() and writel()
> respectively. This can be useful for debugging register accesses.

I know this is just sending old patches, but how about using regmap-mmio 
instead? This gives you access in debugfs for register values.

Best regards,
Alexander

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 118 +++++++++++++++------------
>  1 file changed, 64 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c
> b/drivers/media/platform/nxp/imx-pxp.c index 68f838e3069d..e4d7a6339929
> 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -253,6 +253,16 @@ static struct pxp_q_data *get_q_data(struct pxp_ctx
> *ctx, return &ctx->q_data[V4L2_M2M_DST];
>  }
> 
> +static inline u32 pxp_read(struct pxp_dev *dev, u32 reg)
> +{
> +	return readl(dev->mmio + reg);
> +}
> +
> +static inline void pxp_write(struct pxp_dev *dev, u32 reg, u32 value)
> +{
> +	writel(value, dev->mmio + reg);
> +}
> +
>  static u32 pxp_v4l2_pix_fmt_to_ps_format(u32 v4l2_pix_fmt)
>  {
>  	switch (v4l2_pix_fmt) {
> @@ -505,11 +515,11 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  				csc1_coef = csc1_coef_smpte240m_lim;
>  		}
> 
> -		writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
> -		writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
> -		writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
> +		pxp_write(dev, HW_PXP_CSC1_COEF0, csc1_coef[0]);
> +		pxp_write(dev, HW_PXP_CSC1_COEF1, csc1_coef[1]);
> +		pxp_write(dev, HW_PXP_CSC1_COEF2, csc1_coef[2]);
>  	} else {
> -		writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + 
HW_PXP_CSC1_COEF0);
> +		pxp_write(dev, HW_PXP_CSC1_COEF0, 
BM_PXP_CSC1_COEF0_BYPASS);
>  	}
> 
>  	if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) 
&&
> @@ -725,15 +735,15 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  				    BP_PXP_CSC2_CTRL_CSC_MODE;
>  		}
> 
> -		writel(csc2_ctrl, dev->mmio + HW_PXP_CSC2_CTRL);
> -		writel(csc2_coef[0], dev->mmio + HW_PXP_CSC2_COEF0);
> -		writel(csc2_coef[1], dev->mmio + HW_PXP_CSC2_COEF1);
> -		writel(csc2_coef[2], dev->mmio + HW_PXP_CSC2_COEF2);
> -		writel(csc2_coef[3], dev->mmio + HW_PXP_CSC2_COEF3);
> -		writel(csc2_coef[4], dev->mmio + HW_PXP_CSC2_COEF4);
> -		writel(csc2_coef[5], dev->mmio + HW_PXP_CSC2_COEF5);
> +		pxp_write(dev, HW_PXP_CSC2_CTRL, csc2_ctrl);
> +		pxp_write(dev, HW_PXP_CSC2_COEF0, csc2_coef[0]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF1, csc2_coef[1]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF2, csc2_coef[2]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF3, csc2_coef[3]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF4, csc2_coef[4]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF5, csc2_coef[5]);
>  	} else {
> -		writel(BM_PXP_CSC2_CTRL_BYPASS, dev->mmio + 
HW_PXP_CSC2_CTRL);
> +		pxp_write(dev, HW_PXP_CSC2_CTRL, BM_PXP_CSC2_CTRL_BYPASS);
>  	}
>  }
> 
> @@ -820,8 +830,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
>  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
>  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> 
> -	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> -	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL0, ctrl0);
> +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL1, ctrl1);
>  }
> 
>  static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
> @@ -977,48 +987,48 @@ static int pxp_start(struct pxp_ctx *ctx, struct
> vb2_v4l2_buffer *in_vb, BF_PXP_PS_SCALE_XSCALE(xscale);
>  	ps_offset = BF_PXP_PS_OFFSET_YOFFSET(0) | 
BF_PXP_PS_OFFSET_XOFFSET(0);
> 
> -	writel(ctrl, dev->mmio + HW_PXP_CTRL);
> +	pxp_write(dev, HW_PXP_CTRL, ctrl);
>  	/* skip STAT */
> -	writel(out_ctrl, dev->mmio + HW_PXP_OUT_CTRL);
> -	writel(out_buf, dev->mmio + HW_PXP_OUT_BUF);
> -	writel(out_buf2, dev->mmio + HW_PXP_OUT_BUF2);
> -	writel(out_pitch, dev->mmio + HW_PXP_OUT_PITCH);
> -	writel(out_lrc, dev->mmio + HW_PXP_OUT_LRC);
> -	writel(out_ps_ulc, dev->mmio + HW_PXP_OUT_PS_ULC);
> -	writel(out_ps_lrc, dev->mmio + HW_PXP_OUT_PS_LRC);
> -	writel(as_ulc, dev->mmio + HW_PXP_OUT_AS_ULC);
> -	writel(as_lrc, dev->mmio + HW_PXP_OUT_AS_LRC);
> -	writel(ps_ctrl, dev->mmio + HW_PXP_PS_CTRL);
> -	writel(ps_buf, dev->mmio + HW_PXP_PS_BUF);
> -	writel(ps_ubuf, dev->mmio + HW_PXP_PS_UBUF);
> -	writel(ps_vbuf, dev->mmio + HW_PXP_PS_VBUF);
> -	writel(ps_pitch, dev->mmio + HW_PXP_PS_PITCH);
> -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_BACKGROUND_0);
> -	writel(ps_scale, dev->mmio + HW_PXP_PS_SCALE);
> -	writel(ps_offset, dev->mmio + HW_PXP_PS_OFFSET);
> +	pxp_write(dev, HW_PXP_OUT_CTRL, out_ctrl);
> +	pxp_write(dev, HW_PXP_OUT_BUF, out_buf);
> +	pxp_write(dev, HW_PXP_OUT_BUF2, out_buf2);
> +	pxp_write(dev, HW_PXP_OUT_PITCH, out_pitch);
> +	pxp_write(dev, HW_PXP_OUT_LRC, out_lrc);
> +	pxp_write(dev, HW_PXP_OUT_PS_ULC, out_ps_ulc);
> +	pxp_write(dev, HW_PXP_OUT_PS_LRC, out_ps_lrc);
> +	pxp_write(dev, HW_PXP_OUT_AS_ULC, as_ulc);
> +	pxp_write(dev, HW_PXP_OUT_AS_LRC, as_lrc);
> +	pxp_write(dev, HW_PXP_PS_CTRL, ps_ctrl);
> +	pxp_write(dev, HW_PXP_PS_BUF, ps_buf);
> +	pxp_write(dev, HW_PXP_PS_UBUF, ps_ubuf);
> +	pxp_write(dev, HW_PXP_PS_VBUF, ps_vbuf);
> +	pxp_write(dev, HW_PXP_PS_PITCH, ps_pitch);
> +	pxp_write(dev, HW_PXP_PS_BACKGROUND_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_PS_SCALE, ps_scale);
> +	pxp_write(dev, HW_PXP_PS_OFFSET, ps_offset);
>  	/* disable processed surface color keying */
> -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_CLRKEYLOW_0);
> -	writel(0x00000000, dev->mmio + HW_PXP_PS_CLRKEYHIGH_0);
> +	pxp_write(dev, HW_PXP_PS_CLRKEYLOW_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_PS_CLRKEYHIGH_0, 0x00000000);
> 
>  	/* disable alpha surface color keying */
> -	writel(0x00ffffff, dev->mmio + HW_PXP_AS_CLRKEYLOW_0);
> -	writel(0x00000000, dev->mmio + HW_PXP_AS_CLRKEYHIGH_0);
> +	pxp_write(dev, HW_PXP_AS_CLRKEYLOW_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_AS_CLRKEYHIGH_0, 0x00000000);
> 
>  	/* setup CSC */
>  	pxp_setup_csc(ctx);
> 
>  	/* bypass LUT */
> -	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
> +	pxp_write(dev, HW_PXP_LUT_CTRL, BM_PXP_LUT_CTRL_BYPASS);
> 
>  	pxp_set_data_path(ctx);
> 
> -	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
> +	pxp_write(dev, HW_PXP_IRQ_MASK, 0xffff);
> 
>  	/* ungate, enable PS/AS/OUT and PXP operation */
> -	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
> -	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> -	       BM_PXP_CTRL_ENABLE_ROTATE0 |
> -	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_IRQ_ENABLE);
> +	pxp_write(dev, HW_PXP_CTRL_SET,
> +		  BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> +		  BM_PXP_CTRL_ENABLE_ROTATE0 | 
BM_PXP_CTRL_ENABLE_PS_AS_OUT);
> 
>  	return 0;
>  }
> @@ -1091,23 +1101,23 @@ static irqreturn_t pxp_irq_handler(int irq, void
> *dev_id) struct pxp_dev *dev = dev_id;
>  	u32 stat;
> 
> -	stat = readl(dev->mmio + HW_PXP_STAT);
> +	stat = pxp_read(dev, HW_PXP_STAT);
> 
>  	if (stat & BM_PXP_STAT_IRQ0) {
>  		/* we expect x = 0, y = height, irq0 = 1 */
>  		if (stat & ~(BM_PXP_STAT_BLOCKX | BM_PXP_STAT_BLOCKY |
>  			     BM_PXP_STAT_IRQ0))
>  			dprintk(dev, "%s: stat = 0x%08x\n", __func__, 
stat);
> -		writel(BM_PXP_STAT_IRQ0, dev->mmio + HW_PXP_STAT_CLR);
> +		pxp_write(dev, HW_PXP_STAT_CLR, BM_PXP_STAT_IRQ0);
> 
>  		pxp_job_finish(dev);
>  	} else {
> -		u32 irq = readl(dev->mmio + HW_PXP_IRQ);
> +		u32 irq = pxp_read(dev, HW_PXP_IRQ);
> 
>  		dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
>  		dprintk(dev, "%s: irq = 0x%08x\n", __func__, irq);
> 
> -		writel(irq, dev->mmio + HW_PXP_IRQ_CLR);
> +		pxp_write(dev, HW_PXP_IRQ_CLR, irq);
>  	}
> 
>  	return IRQ_HANDLED;
> @@ -1753,25 +1763,25 @@ static int pxp_soft_reset(struct pxp_dev *dev)
>  	int ret;
>  	u32 val;
> 
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> 
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> 
>  	ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
>  				 val & BM_PXP_CTRL_CLKGATE, 0, 100);
>  	if (ret < 0)
>  		return ret;
> 
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> 
>  	return 0;
>  }
> 
>  static u32 pxp_read_version(struct pxp_dev *dev)
>  {
> -	return readl(dev->mmio + HW_PXP_VERSION);
> +	return pxp_read(dev, HW_PXP_VERSION);
>  }
> 
>  static int pxp_probe(struct platform_device *pdev)
> @@ -1902,8 +1912,8 @@ static int pxp_remove(struct platform_device *pdev)
>  {
>  	struct pxp_dev *dev = platform_get_drvdata(pdev);
> 
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_CLKGATE);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> 
>  	clk_disable_unprepare(dev->clk);

Re: [PATCH v1 5/6] media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers

[Laurent Pinchart]

Hi Alexander,

On Fri, Jan 06, 2023 at 03:50:37PM +0100, Alexander Stein wrote:
> Am Freitag, 6. Januar 2023, 14:32:26 CET schrieb Laurent Pinchart:
> > Add pxp_read() and pxp_write() functions to wrap readl() and writel()
> > respectively. This can be useful for debugging register accesses.
> 
> I know this is just sending old patches, but how about using regmap-mmio 
> instead? This gives you access in debugfs for register values.

I'm fine with that. Notice how easy it would be on top of this patch, as
you will then only need to modify pxp_read() and pxp_write() ;-)

My i.MX7D board is now at the bottom of a box so I don't plan to work on
this myself, but I would review a patch.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 118 +++++++++++++++------------
> >  1 file changed, 64 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c
> > b/drivers/media/platform/nxp/imx-pxp.c index 68f838e3069d..e4d7a6339929
> > 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -253,6 +253,16 @@ static struct pxp_q_data *get_q_data(struct pxp_ctx
> > *ctx, return &ctx->q_data[V4L2_M2M_DST];
> >  }
> > 
> > +static inline u32 pxp_read(struct pxp_dev *dev, u32 reg)
> > +{
> > +	return readl(dev->mmio + reg);
> > +}
> > +
> > +static inline void pxp_write(struct pxp_dev *dev, u32 reg, u32 value)
> > +{
> > +	writel(value, dev->mmio + reg);
> > +}
> > +
> >  static u32 pxp_v4l2_pix_fmt_to_ps_format(u32 v4l2_pix_fmt)
> >  {
> >  	switch (v4l2_pix_fmt) {
> > @@ -505,11 +515,11 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> >  				csc1_coef = csc1_coef_smpte240m_lim;
> >  		}
> > 
> > -		writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
> > -		writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
> > -		writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
> > +		pxp_write(dev, HW_PXP_CSC1_COEF0, csc1_coef[0]);
> > +		pxp_write(dev, HW_PXP_CSC1_COEF1, csc1_coef[1]);
> > +		pxp_write(dev, HW_PXP_CSC1_COEF2, csc1_coef[2]);
> >  	} else {
> > -		writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + 
> HW_PXP_CSC1_COEF0);
> > +		pxp_write(dev, HW_PXP_CSC1_COEF0, 
> BM_PXP_CSC1_COEF0_BYPASS);
> >  	}
> > 
> >  	if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) 
> &&
> > @@ -725,15 +735,15 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> >  				    BP_PXP_CSC2_CTRL_CSC_MODE;
> >  		}
> > 
> > -		writel(csc2_ctrl, dev->mmio + HW_PXP_CSC2_CTRL);
> > -		writel(csc2_coef[0], dev->mmio + HW_PXP_CSC2_COEF0);
> > -		writel(csc2_coef[1], dev->mmio + HW_PXP_CSC2_COEF1);
> > -		writel(csc2_coef[2], dev->mmio + HW_PXP_CSC2_COEF2);
> > -		writel(csc2_coef[3], dev->mmio + HW_PXP_CSC2_COEF3);
> > -		writel(csc2_coef[4], dev->mmio + HW_PXP_CSC2_COEF4);
> > -		writel(csc2_coef[5], dev->mmio + HW_PXP_CSC2_COEF5);
> > +		pxp_write(dev, HW_PXP_CSC2_CTRL, csc2_ctrl);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF0, csc2_coef[0]);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF1, csc2_coef[1]);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF2, csc2_coef[2]);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF3, csc2_coef[3]);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF4, csc2_coef[4]);
> > +		pxp_write(dev, HW_PXP_CSC2_COEF5, csc2_coef[5]);
> >  	} else {
> > -		writel(BM_PXP_CSC2_CTRL_BYPASS, dev->mmio + 
> HW_PXP_CSC2_CTRL);
> > +		pxp_write(dev, HW_PXP_CSC2_CTRL, BM_PXP_CSC2_CTRL_BYPASS);
> >  	}
> >  }
> > 
> > @@ -820,8 +830,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> >  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> >  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> > 
> > -	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> > -	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> > +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL0, ctrl0);
> > +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL1, ctrl1);
> >  }
> > 
> >  static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
> > @@ -977,48 +987,48 @@ static int pxp_start(struct pxp_ctx *ctx, struct
> > vb2_v4l2_buffer *in_vb, BF_PXP_PS_SCALE_XSCALE(xscale);
> >  	ps_offset = BF_PXP_PS_OFFSET_YOFFSET(0) | 
> BF_PXP_PS_OFFSET_XOFFSET(0);
> > 
> > -	writel(ctrl, dev->mmio + HW_PXP_CTRL);
> > +	pxp_write(dev, HW_PXP_CTRL, ctrl);
> >  	/* skip STAT */
> > -	writel(out_ctrl, dev->mmio + HW_PXP_OUT_CTRL);
> > -	writel(out_buf, dev->mmio + HW_PXP_OUT_BUF);
> > -	writel(out_buf2, dev->mmio + HW_PXP_OUT_BUF2);
> > -	writel(out_pitch, dev->mmio + HW_PXP_OUT_PITCH);
> > -	writel(out_lrc, dev->mmio + HW_PXP_OUT_LRC);
> > -	writel(out_ps_ulc, dev->mmio + HW_PXP_OUT_PS_ULC);
> > -	writel(out_ps_lrc, dev->mmio + HW_PXP_OUT_PS_LRC);
> > -	writel(as_ulc, dev->mmio + HW_PXP_OUT_AS_ULC);
> > -	writel(as_lrc, dev->mmio + HW_PXP_OUT_AS_LRC);
> > -	writel(ps_ctrl, dev->mmio + HW_PXP_PS_CTRL);
> > -	writel(ps_buf, dev->mmio + HW_PXP_PS_BUF);
> > -	writel(ps_ubuf, dev->mmio + HW_PXP_PS_UBUF);
> > -	writel(ps_vbuf, dev->mmio + HW_PXP_PS_VBUF);
> > -	writel(ps_pitch, dev->mmio + HW_PXP_PS_PITCH);
> > -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_BACKGROUND_0);
> > -	writel(ps_scale, dev->mmio + HW_PXP_PS_SCALE);
> > -	writel(ps_offset, dev->mmio + HW_PXP_PS_OFFSET);
> > +	pxp_write(dev, HW_PXP_OUT_CTRL, out_ctrl);
> > +	pxp_write(dev, HW_PXP_OUT_BUF, out_buf);
> > +	pxp_write(dev, HW_PXP_OUT_BUF2, out_buf2);
> > +	pxp_write(dev, HW_PXP_OUT_PITCH, out_pitch);
> > +	pxp_write(dev, HW_PXP_OUT_LRC, out_lrc);
> > +	pxp_write(dev, HW_PXP_OUT_PS_ULC, out_ps_ulc);
> > +	pxp_write(dev, HW_PXP_OUT_PS_LRC, out_ps_lrc);
> > +	pxp_write(dev, HW_PXP_OUT_AS_ULC, as_ulc);
> > +	pxp_write(dev, HW_PXP_OUT_AS_LRC, as_lrc);
> > +	pxp_write(dev, HW_PXP_PS_CTRL, ps_ctrl);
> > +	pxp_write(dev, HW_PXP_PS_BUF, ps_buf);
> > +	pxp_write(dev, HW_PXP_PS_UBUF, ps_ubuf);
> > +	pxp_write(dev, HW_PXP_PS_VBUF, ps_vbuf);
> > +	pxp_write(dev, HW_PXP_PS_PITCH, ps_pitch);
> > +	pxp_write(dev, HW_PXP_PS_BACKGROUND_0, 0x00ffffff);
> > +	pxp_write(dev, HW_PXP_PS_SCALE, ps_scale);
> > +	pxp_write(dev, HW_PXP_PS_OFFSET, ps_offset);
> >  	/* disable processed surface color keying */
> > -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_CLRKEYLOW_0);
> > -	writel(0x00000000, dev->mmio + HW_PXP_PS_CLRKEYHIGH_0);
> > +	pxp_write(dev, HW_PXP_PS_CLRKEYLOW_0, 0x00ffffff);
> > +	pxp_write(dev, HW_PXP_PS_CLRKEYHIGH_0, 0x00000000);
> > 
> >  	/* disable alpha surface color keying */
> > -	writel(0x00ffffff, dev->mmio + HW_PXP_AS_CLRKEYLOW_0);
> > -	writel(0x00000000, dev->mmio + HW_PXP_AS_CLRKEYHIGH_0);
> > +	pxp_write(dev, HW_PXP_AS_CLRKEYLOW_0, 0x00ffffff);
> > +	pxp_write(dev, HW_PXP_AS_CLRKEYHIGH_0, 0x00000000);
> > 
> >  	/* setup CSC */
> >  	pxp_setup_csc(ctx);
> > 
> >  	/* bypass LUT */
> > -	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
> > +	pxp_write(dev, HW_PXP_LUT_CTRL, BM_PXP_LUT_CTRL_BYPASS);
> > 
> >  	pxp_set_data_path(ctx);
> > 
> > -	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
> > +	pxp_write(dev, HW_PXP_IRQ_MASK, 0xffff);
> > 
> >  	/* ungate, enable PS/AS/OUT and PXP operation */
> > -	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
> > -	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> > -	       BM_PXP_CTRL_ENABLE_ROTATE0 |
> > -	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
> > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_IRQ_ENABLE);
> > +	pxp_write(dev, HW_PXP_CTRL_SET,
> > +		  BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> > +		  BM_PXP_CTRL_ENABLE_ROTATE0 | 
> BM_PXP_CTRL_ENABLE_PS_AS_OUT);
> > 
> >  	return 0;
> >  }
> > @@ -1091,23 +1101,23 @@ static irqreturn_t pxp_irq_handler(int irq, void
> > *dev_id) struct pxp_dev *dev = dev_id;
> >  	u32 stat;
> > 
> > -	stat = readl(dev->mmio + HW_PXP_STAT);
> > +	stat = pxp_read(dev, HW_PXP_STAT);
> > 
> >  	if (stat & BM_PXP_STAT_IRQ0) {
> >  		/* we expect x = 0, y = height, irq0 = 1 */
> >  		if (stat & ~(BM_PXP_STAT_BLOCKX | BM_PXP_STAT_BLOCKY |
> >  			     BM_PXP_STAT_IRQ0))
> >  			dprintk(dev, "%s: stat = 0x%08x\n", __func__, 
> stat);
> > -		writel(BM_PXP_STAT_IRQ0, dev->mmio + HW_PXP_STAT_CLR);
> > +		pxp_write(dev, HW_PXP_STAT_CLR, BM_PXP_STAT_IRQ0);
> > 
> >  		pxp_job_finish(dev);
> >  	} else {
> > -		u32 irq = readl(dev->mmio + HW_PXP_IRQ);
> > +		u32 irq = pxp_read(dev, HW_PXP_IRQ);
> > 
> >  		dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
> >  		dprintk(dev, "%s: irq = 0x%08x\n", __func__, irq);
> > 
> > -		writel(irq, dev->mmio + HW_PXP_IRQ_CLR);
> > +		pxp_write(dev, HW_PXP_IRQ_CLR, irq);
> >  	}
> > 
> >  	return IRQ_HANDLED;
> > @@ -1753,25 +1763,25 @@ static int pxp_soft_reset(struct pxp_dev *dev)
> >  	int ret;
> >  	u32 val;
> > 
> > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> > 
> > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> > 
> >  	ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
> >  				 val & BM_PXP_CTRL_CLKGATE, 0, 100);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> > 
> >  	return 0;
> >  }
> > 
> >  static u32 pxp_read_version(struct pxp_dev *dev)
> >  {
> > -	return readl(dev->mmio + HW_PXP_VERSION);
> > +	return pxp_read(dev, HW_PXP_VERSION);
> >  }
> > 
> >  static int pxp_probe(struct platform_device *pdev)
> > @@ -1902,8 +1912,8 @@ static int pxp_remove(struct platform_device *pdev)
> >  {
> >  	struct pxp_dev *dev = platform_get_drvdata(pdev);
> > 
> > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
> > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_CLKGATE);
> > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> > 
> >  	clk_disable_unprepare(dev->clk);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/6] media: imx-pxp: Sort headers alphabetically

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:22 +0200, Laurent Pinchart wrote:
> Sorting headers alphabetically helps locating duplicates, and make it
> easier to figure out where to insert new headers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 04cc8df2a498..dcb04217288b 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -20,15 +20,15 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/platform_device.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  
> -#include <linux/platform_device.h>
> -#include <media/v4l2-mem2mem.h>
> -#include <media/v4l2-device.h>
> -#include <media/v4l2-ioctl.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mem2mem.h>
>  #include <media/videobuf2-dma-contig.h>
>  
>  #include "imx-pxp.h"
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 2/6] media: imx-pxp: Add media controller support

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:23 +0200, Laurent Pinchart wrote:
> Register a media device for the PXP, using the v4l2-mem2mem MC
> infrastructure to populate the media graph. No media device operation is
> implemented, the main use of the MC API is to allow consistent discovery
> of media devices for userspace.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 37 ++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index dcb04217288b..132065c8b8b4 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -24,6 +24,7 @@
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  
> +#include <media/media-device.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -200,6 +201,9 @@ struct pxp_pdata {
>  struct pxp_dev {
>  	struct v4l2_device	v4l2_dev;
>  	struct video_device	vfd;
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct media_device	mdev;
> +#endif
>  
>  	struct clk		*clk;
>  	void __iomem		*mmio;
> @@ -1832,8 +1836,36 @@ static int pxp_probe(struct platform_device *pdev)
>  		goto err_m2m;
>  	}
>  
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	dev->mdev.dev = &pdev->dev;
> +	strscpy(dev->mdev.model, MEM2MEM_NAME, sizeof(dev->mdev.model));
> +	snprintf(dev->mdev.bus_info, sizeof(dev->mdev.bus_info), "platform:%s",
> +		 dev_name(&pdev->dev));

v4l2-compliance gives the following warning:

	warn: v4l2-compliance.cpp(642): media bus_info 'platform:30700000.pxp' differs from V4L2 bus_info 'platform:pxp'

I would change this patch to use the same name as in the V4L2 bus_info. Do you
have a reason for including the address in the bus_info?

Michael

> +	media_device_init(&dev->mdev);
> +	dev->v4l2_dev.mdev = &dev->mdev;
> +
> +	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
> +						 MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to initialize media device\n");
> +		goto err_vfd;
> +	}
> +
> +	ret = media_device_register(&dev->mdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register media device\n");
> +		goto err_m2m_mc;
> +	}
> +#endif
> +
>  	return 0;
>  
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +err_m2m_mc:
> +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> +err_vfd:
> +	video_unregister_device(vfd);
> +#endif
>  err_m2m:
>  	v4l2_m2m_release(dev->m2m_dev);
>  err_v4l2:
> @@ -1854,6 +1886,11 @@ static int pxp_remove(struct platform_device *pdev)
>  	clk_disable_unprepare(dev->clk);
>  
>  	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	media_device_unregister(&dev->mdev);
> +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> +#endif
>  	video_unregister_device(&dev->vfd);
>  	v4l2_m2m_release(dev->m2m_dev);
>  	v4l2_device_unregister(&dev->v4l2_dev);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 3/6] media: imx-pxp: Pass pixel format value to find_format()

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:24 +0200, Laurent Pinchart wrote:
> The find_format() function looks up format information for a given pixel
> format. It takes a v4l2_format pointer, but only uses the contained
> pixel format value. To prepare it for being used by callers that don't
> have v4l2_format, modify it to take the pixel format value directly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 132065c8b8b4..fd4c055c01eb 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -176,14 +176,14 @@ enum {
>  	V4L2_M2M_DST = 1,
>  };
>  
> -static struct pxp_fmt *find_format(struct v4l2_format *f)
> +static struct pxp_fmt *find_format(unsigned int pixelformat)
>  {
>  	struct pxp_fmt *fmt;
>  	unsigned int k;
>  
>  	for (k = 0; k < NUM_FORMATS; k++) {
>  		fmt = &formats[k];
> -		if (fmt->fourcc == f->fmt.pix.pixelformat)
> +		if (fmt->fourcc == pixelformat)
>  			break;
>  	}
>  
> @@ -1268,10 +1268,10 @@ static int pxp_try_fmt_vid_cap(struct file *file, void *priv,
>  	struct pxp_fmt *fmt;
>  	struct pxp_ctx *ctx = file2ctx(file);
>  
> -	fmt = find_format(f);
> +	fmt = find_format(f->fmt.pix.pixelformat);
>  	if (!fmt) {
>  		f->fmt.pix.pixelformat = formats[0].fourcc;
> -		fmt = find_format(f);
> +		fmt = find_format(f->fmt.pix.pixelformat);
>  	}
>  	if (!(fmt->types & MEM2MEM_CAPTURE)) {
>  		v4l2_err(&ctx->dev->v4l2_dev,
> @@ -1296,10 +1296,10 @@ static int pxp_try_fmt_vid_out(struct file *file, void *priv,
>  	struct pxp_fmt *fmt;
>  	struct pxp_ctx *ctx = file2ctx(file);
>  
> -	fmt = find_format(f);
> +	fmt = find_format(f->fmt.pix.pixelformat);
>  	if (!fmt) {
>  		f->fmt.pix.pixelformat = formats[0].fourcc;
> -		fmt = find_format(f);
> +		fmt = find_format(f->fmt.pix.pixelformat);
>  	}
>  	if (!(fmt->types & MEM2MEM_OUTPUT)) {
>  		v4l2_err(&ctx->dev->v4l2_dev,
> @@ -1332,7 +1332,7 @@ static int pxp_s_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
>  		return -EBUSY;
>  	}
>  
> -	q_data->fmt		= find_format(f);
> +	q_data->fmt		= find_format(f->fmt.pix.pixelformat);
>  	q_data->width		= f->fmt.pix.width;
>  	q_data->height		= f->fmt.pix.height;
>  	q_data->bytesperline	= f->fmt.pix.bytesperline;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 4/6] media: imx-pxp: Implement frame size enumeration

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:25 +0200, Laurent Pinchart wrote:
> Implement support for the VIDIOC_ENUM_FRAMESIZES ioctl.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index fd4c055c01eb..68f838e3069d 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -1391,6 +1391,26 @@ static int pxp_s_fmt_vid_out(struct file *file, void *priv,
>  	return 0;
>  }
>  
> +static int pxp_enum_framesizes(struct file *file, void *fh,
> +			       struct v4l2_frmsizeenum *fsize)
> +{
> +	if (fsize->index > 0)
> +		return -EINVAL;
> +
> +	if (!find_format(fsize->pixel_format))
> +		return -EINVAL;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> +	fsize->stepwise.min_width = MIN_W;
> +	fsize->stepwise.max_width = MAX_W;
> +	fsize->stepwise.step_width = ALIGN_W;

fsize->stepwise.step_width = 1 << ALIGN_W;

ALIGN_W isn't the steps in pixels, but the exponent for a 2^align alignment.

> +	fsize->stepwise.min_height = MIN_H;
> +	fsize->stepwise.max_height = MAX_H;
> +	fsize->stepwise.step_height = ALIGN_H;

fsize->stepwise.step_height = 1 << ALIGN_H;

Michael

> +
> +	return 0;
> +}
> +
>  static u8 pxp_degrees_to_rot_mode(u32 degrees)
>  {
>  	switch (degrees) {
> @@ -1459,6 +1479,8 @@ static const struct v4l2_ioctl_ops pxp_ioctl_ops = {
>  	.vidioc_try_fmt_vid_out	= pxp_try_fmt_vid_out,
>  	.vidioc_s_fmt_vid_out	= pxp_s_fmt_vid_out,
>  
> +	.vidioc_enum_framesizes	= pxp_enum_framesizes,
> +
>  	.vidioc_reqbufs		= v4l2_m2m_ioctl_reqbufs,
>  	.vidioc_querybuf	= v4l2_m2m_ioctl_querybuf,
>  	.vidioc_qbuf		= v4l2_m2m_ioctl_qbuf,
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 5/6] media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers

[Michael Tretter]

On Fri, 06 Jan 2023 20:44:29 +0200, Laurent Pinchart wrote:
> On Fri, Jan 06, 2023 at 03:50:37PM +0100, Alexander Stein wrote:
> > Am Freitag, 6. Januar 2023, 14:32:26 CET schrieb Laurent Pinchart:
> > > Add pxp_read() and pxp_write() functions to wrap readl() and writel()
> > > respectively. This can be useful for debugging register accesses.
> > 
> > I know this is just sending old patches, but how about using regmap-mmio 
> > instead? This gives you access in debugfs for register values.
> 
> I'm fine with that. Notice how easy it would be on top of this patch, as
> you will then only need to modify pxp_read() and pxp_write() ;-)

Almost. There is one instance of readl_poll_timeout() that needs to be
converted too.

> 
> My i.MX7D board is now at the bottom of a box so I don't plan to work on
> this myself, but I would review a patch.

I will add a patch to my series.

Michael

> 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/nxp/imx-pxp.c | 118 +++++++++++++++------------
> > >  1 file changed, 64 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-pxp.c
> > > b/drivers/media/platform/nxp/imx-pxp.c index 68f838e3069d..e4d7a6339929
> > > 100644
> > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > @@ -253,6 +253,16 @@ static struct pxp_q_data *get_q_data(struct pxp_ctx
> > > *ctx, return &ctx->q_data[V4L2_M2M_DST];
> > >  }
> > > 
> > > +static inline u32 pxp_read(struct pxp_dev *dev, u32 reg)
> > > +{
> > > +	return readl(dev->mmio + reg);
> > > +}
> > > +
> > > +static inline void pxp_write(struct pxp_dev *dev, u32 reg, u32 value)
> > > +{
> > > +	writel(value, dev->mmio + reg);
> > > +}
> > > +
> > >  static u32 pxp_v4l2_pix_fmt_to_ps_format(u32 v4l2_pix_fmt)
> > >  {
> > >  	switch (v4l2_pix_fmt) {
> > > @@ -505,11 +515,11 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> > >  				csc1_coef = csc1_coef_smpte240m_lim;
> > >  		}
> > > 
> > > -		writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
> > > -		writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
> > > -		writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
> > > +		pxp_write(dev, HW_PXP_CSC1_COEF0, csc1_coef[0]);
> > > +		pxp_write(dev, HW_PXP_CSC1_COEF1, csc1_coef[1]);
> > > +		pxp_write(dev, HW_PXP_CSC1_COEF2, csc1_coef[2]);
> > >  	} else {
> > > -		writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + 
> > HW_PXP_CSC1_COEF0);
> > > +		pxp_write(dev, HW_PXP_CSC1_COEF0, 
> > BM_PXP_CSC1_COEF0_BYPASS);
> > >  	}
> > > 
> > >  	if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) 
> > &&
> > > @@ -725,15 +735,15 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
> > >  				    BP_PXP_CSC2_CTRL_CSC_MODE;
> > >  		}
> > > 
> > > -		writel(csc2_ctrl, dev->mmio + HW_PXP_CSC2_CTRL);
> > > -		writel(csc2_coef[0], dev->mmio + HW_PXP_CSC2_COEF0);
> > > -		writel(csc2_coef[1], dev->mmio + HW_PXP_CSC2_COEF1);
> > > -		writel(csc2_coef[2], dev->mmio + HW_PXP_CSC2_COEF2);
> > > -		writel(csc2_coef[3], dev->mmio + HW_PXP_CSC2_COEF3);
> > > -		writel(csc2_coef[4], dev->mmio + HW_PXP_CSC2_COEF4);
> > > -		writel(csc2_coef[5], dev->mmio + HW_PXP_CSC2_COEF5);
> > > +		pxp_write(dev, HW_PXP_CSC2_CTRL, csc2_ctrl);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF0, csc2_coef[0]);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF1, csc2_coef[1]);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF2, csc2_coef[2]);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF3, csc2_coef[3]);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF4, csc2_coef[4]);
> > > +		pxp_write(dev, HW_PXP_CSC2_COEF5, csc2_coef[5]);
> > >  	} else {
> > > -		writel(BM_PXP_CSC2_CTRL_BYPASS, dev->mmio + 
> > HW_PXP_CSC2_CTRL);
> > > +		pxp_write(dev, HW_PXP_CSC2_CTRL, BM_PXP_CSC2_CTRL_BYPASS);
> > >  	}
> > >  }
> > > 
> > > @@ -820,8 +830,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> > >  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> > >  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> > > 
> > > -	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> > > -	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> > > +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL0, ctrl0);
> > > +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL1, ctrl1);
> > >  }
> > > 
> > >  static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
> > > @@ -977,48 +987,48 @@ static int pxp_start(struct pxp_ctx *ctx, struct
> > > vb2_v4l2_buffer *in_vb, BF_PXP_PS_SCALE_XSCALE(xscale);
> > >  	ps_offset = BF_PXP_PS_OFFSET_YOFFSET(0) | 
> > BF_PXP_PS_OFFSET_XOFFSET(0);
> > > 
> > > -	writel(ctrl, dev->mmio + HW_PXP_CTRL);
> > > +	pxp_write(dev, HW_PXP_CTRL, ctrl);
> > >  	/* skip STAT */
> > > -	writel(out_ctrl, dev->mmio + HW_PXP_OUT_CTRL);
> > > -	writel(out_buf, dev->mmio + HW_PXP_OUT_BUF);
> > > -	writel(out_buf2, dev->mmio + HW_PXP_OUT_BUF2);
> > > -	writel(out_pitch, dev->mmio + HW_PXP_OUT_PITCH);
> > > -	writel(out_lrc, dev->mmio + HW_PXP_OUT_LRC);
> > > -	writel(out_ps_ulc, dev->mmio + HW_PXP_OUT_PS_ULC);
> > > -	writel(out_ps_lrc, dev->mmio + HW_PXP_OUT_PS_LRC);
> > > -	writel(as_ulc, dev->mmio + HW_PXP_OUT_AS_ULC);
> > > -	writel(as_lrc, dev->mmio + HW_PXP_OUT_AS_LRC);
> > > -	writel(ps_ctrl, dev->mmio + HW_PXP_PS_CTRL);
> > > -	writel(ps_buf, dev->mmio + HW_PXP_PS_BUF);
> > > -	writel(ps_ubuf, dev->mmio + HW_PXP_PS_UBUF);
> > > -	writel(ps_vbuf, dev->mmio + HW_PXP_PS_VBUF);
> > > -	writel(ps_pitch, dev->mmio + HW_PXP_PS_PITCH);
> > > -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_BACKGROUND_0);
> > > -	writel(ps_scale, dev->mmio + HW_PXP_PS_SCALE);
> > > -	writel(ps_offset, dev->mmio + HW_PXP_PS_OFFSET);
> > > +	pxp_write(dev, HW_PXP_OUT_CTRL, out_ctrl);
> > > +	pxp_write(dev, HW_PXP_OUT_BUF, out_buf);
> > > +	pxp_write(dev, HW_PXP_OUT_BUF2, out_buf2);
> > > +	pxp_write(dev, HW_PXP_OUT_PITCH, out_pitch);
> > > +	pxp_write(dev, HW_PXP_OUT_LRC, out_lrc);
> > > +	pxp_write(dev, HW_PXP_OUT_PS_ULC, out_ps_ulc);
> > > +	pxp_write(dev, HW_PXP_OUT_PS_LRC, out_ps_lrc);
> > > +	pxp_write(dev, HW_PXP_OUT_AS_ULC, as_ulc);
> > > +	pxp_write(dev, HW_PXP_OUT_AS_LRC, as_lrc);
> > > +	pxp_write(dev, HW_PXP_PS_CTRL, ps_ctrl);
> > > +	pxp_write(dev, HW_PXP_PS_BUF, ps_buf);
> > > +	pxp_write(dev, HW_PXP_PS_UBUF, ps_ubuf);
> > > +	pxp_write(dev, HW_PXP_PS_VBUF, ps_vbuf);
> > > +	pxp_write(dev, HW_PXP_PS_PITCH, ps_pitch);
> > > +	pxp_write(dev, HW_PXP_PS_BACKGROUND_0, 0x00ffffff);
> > > +	pxp_write(dev, HW_PXP_PS_SCALE, ps_scale);
> > > +	pxp_write(dev, HW_PXP_PS_OFFSET, ps_offset);
> > >  	/* disable processed surface color keying */
> > > -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_CLRKEYLOW_0);
> > > -	writel(0x00000000, dev->mmio + HW_PXP_PS_CLRKEYHIGH_0);
> > > +	pxp_write(dev, HW_PXP_PS_CLRKEYLOW_0, 0x00ffffff);
> > > +	pxp_write(dev, HW_PXP_PS_CLRKEYHIGH_0, 0x00000000);
> > > 
> > >  	/* disable alpha surface color keying */
> > > -	writel(0x00ffffff, dev->mmio + HW_PXP_AS_CLRKEYLOW_0);
> > > -	writel(0x00000000, dev->mmio + HW_PXP_AS_CLRKEYHIGH_0);
> > > +	pxp_write(dev, HW_PXP_AS_CLRKEYLOW_0, 0x00ffffff);
> > > +	pxp_write(dev, HW_PXP_AS_CLRKEYHIGH_0, 0x00000000);
> > > 
> > >  	/* setup CSC */
> > >  	pxp_setup_csc(ctx);
> > > 
> > >  	/* bypass LUT */
> > > -	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
> > > +	pxp_write(dev, HW_PXP_LUT_CTRL, BM_PXP_LUT_CTRL_BYPASS);
> > > 
> > >  	pxp_set_data_path(ctx);
> > > 
> > > -	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
> > > +	pxp_write(dev, HW_PXP_IRQ_MASK, 0xffff);
> > > 
> > >  	/* ungate, enable PS/AS/OUT and PXP operation */
> > > -	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
> > > -	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> > > -	       BM_PXP_CTRL_ENABLE_ROTATE0 |
> > > -	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
> > > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_IRQ_ENABLE);
> > > +	pxp_write(dev, HW_PXP_CTRL_SET,
> > > +		  BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> > > +		  BM_PXP_CTRL_ENABLE_ROTATE0 | 
> > BM_PXP_CTRL_ENABLE_PS_AS_OUT);
> > > 
> > >  	return 0;
> > >  }
> > > @@ -1091,23 +1101,23 @@ static irqreturn_t pxp_irq_handler(int irq, void
> > > *dev_id) struct pxp_dev *dev = dev_id;
> > >  	u32 stat;
> > > 
> > > -	stat = readl(dev->mmio + HW_PXP_STAT);
> > > +	stat = pxp_read(dev, HW_PXP_STAT);
> > > 
> > >  	if (stat & BM_PXP_STAT_IRQ0) {
> > >  		/* we expect x = 0, y = height, irq0 = 1 */
> > >  		if (stat & ~(BM_PXP_STAT_BLOCKX | BM_PXP_STAT_BLOCKY |
> > >  			     BM_PXP_STAT_IRQ0))
> > >  			dprintk(dev, "%s: stat = 0x%08x\n", __func__, 
> > stat);
> > > -		writel(BM_PXP_STAT_IRQ0, dev->mmio + HW_PXP_STAT_CLR);
> > > +		pxp_write(dev, HW_PXP_STAT_CLR, BM_PXP_STAT_IRQ0);
> > > 
> > >  		pxp_job_finish(dev);
> > >  	} else {
> > > -		u32 irq = readl(dev->mmio + HW_PXP_IRQ);
> > > +		u32 irq = pxp_read(dev, HW_PXP_IRQ);
> > > 
> > >  		dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
> > >  		dprintk(dev, "%s: irq = 0x%08x\n", __func__, irq);
> > > 
> > > -		writel(irq, dev->mmio + HW_PXP_IRQ_CLR);
> > > +		pxp_write(dev, HW_PXP_IRQ_CLR, irq);
> > >  	}
> > > 
> > >  	return IRQ_HANDLED;
> > > @@ -1753,25 +1763,25 @@ static int pxp_soft_reset(struct pxp_dev *dev)
> > >  	int ret;
> > >  	u32 val;
> > > 
> > > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> > > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> > > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> > > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> > > 
> > > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> > > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> > > 
> > >  	ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
> > >  				 val & BM_PXP_CTRL_CLKGATE, 0, 100);
> > >  	if (ret < 0)
> > >  		return ret;
> > > 
> > > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> > > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> > > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> > > +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
> > > 
> > >  	return 0;
> > >  }
> > > 
> > >  static u32 pxp_read_version(struct pxp_dev *dev)
> > >  {
> > > -	return readl(dev->mmio + HW_PXP_VERSION);
> > > +	return pxp_read(dev, HW_PXP_VERSION);
> > >  }
> > > 
> > >  static int pxp_probe(struct platform_device *pdev)
> > > @@ -1902,8 +1912,8 @@ static int pxp_remove(struct platform_device *pdev)
> > >  {
> > >  	struct pxp_dev *dev = platform_get_drvdata(pdev);
> > > 
> > > -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
> > > -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> > > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_CLKGATE);
> > > +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
> > > 
> > >  	clk_disable_unprepare(dev->clk);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH v1 5/6] media: imx-pxp: Introduce pxp_read() and pxp_write() wrappers

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:26 +0200, Laurent Pinchart wrote:
> Add pxp_read() and pxp_write() functions to wrap readl() and writel()
> respectively. This can be useful for debugging register accesses.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 118 +++++++++++++++------------
>  1 file changed, 64 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index 68f838e3069d..e4d7a6339929 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -253,6 +253,16 @@ static struct pxp_q_data *get_q_data(struct pxp_ctx *ctx,
>  		return &ctx->q_data[V4L2_M2M_DST];
>  }
>  
> +static inline u32 pxp_read(struct pxp_dev *dev, u32 reg)
> +{
> +	return readl(dev->mmio + reg);
> +}
> +
> +static inline void pxp_write(struct pxp_dev *dev, u32 reg, u32 value)
> +{
> +	writel(value, dev->mmio + reg);
> +}
> +
>  static u32 pxp_v4l2_pix_fmt_to_ps_format(u32 v4l2_pix_fmt)
>  {
>  	switch (v4l2_pix_fmt) {
> @@ -505,11 +515,11 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  				csc1_coef = csc1_coef_smpte240m_lim;
>  		}
>  
> -		writel(csc1_coef[0], dev->mmio + HW_PXP_CSC1_COEF0);
> -		writel(csc1_coef[1], dev->mmio + HW_PXP_CSC1_COEF1);
> -		writel(csc1_coef[2], dev->mmio + HW_PXP_CSC1_COEF2);
> +		pxp_write(dev, HW_PXP_CSC1_COEF0, csc1_coef[0]);
> +		pxp_write(dev, HW_PXP_CSC1_COEF1, csc1_coef[1]);
> +		pxp_write(dev, HW_PXP_CSC1_COEF2, csc1_coef[2]);
>  	} else {
> -		writel(BM_PXP_CSC1_COEF0_BYPASS, dev->mmio + HW_PXP_CSC1_COEF0);
> +		pxp_write(dev, HW_PXP_CSC1_COEF0, BM_PXP_CSC1_COEF0_BYPASS);
>  	}
>  
>  	if (!pxp_v4l2_pix_fmt_is_yuv(ctx->q_data[V4L2_M2M_SRC].fmt->fourcc) &&
> @@ -725,15 +735,15 @@ static void pxp_setup_csc(struct pxp_ctx *ctx)
>  				    BP_PXP_CSC2_CTRL_CSC_MODE;
>  		}
>  
> -		writel(csc2_ctrl, dev->mmio + HW_PXP_CSC2_CTRL);
> -		writel(csc2_coef[0], dev->mmio + HW_PXP_CSC2_COEF0);
> -		writel(csc2_coef[1], dev->mmio + HW_PXP_CSC2_COEF1);
> -		writel(csc2_coef[2], dev->mmio + HW_PXP_CSC2_COEF2);
> -		writel(csc2_coef[3], dev->mmio + HW_PXP_CSC2_COEF3);
> -		writel(csc2_coef[4], dev->mmio + HW_PXP_CSC2_COEF4);
> -		writel(csc2_coef[5], dev->mmio + HW_PXP_CSC2_COEF5);
> +		pxp_write(dev, HW_PXP_CSC2_CTRL, csc2_ctrl);
> +		pxp_write(dev, HW_PXP_CSC2_COEF0, csc2_coef[0]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF1, csc2_coef[1]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF2, csc2_coef[2]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF3, csc2_coef[3]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF4, csc2_coef[4]);
> +		pxp_write(dev, HW_PXP_CSC2_COEF5, csc2_coef[5]);
>  	} else {
> -		writel(BM_PXP_CSC2_CTRL_BYPASS, dev->mmio + HW_PXP_CSC2_CTRL);
> +		pxp_write(dev, HW_PXP_CSC2_CTRL, BM_PXP_CSC2_CTRL_BYPASS);
>  	}
>  }
>  
> @@ -820,8 +830,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
>  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
>  	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
>  
> -	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> -	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
> +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL0, ctrl0);
> +	pxp_write(dev, HW_PXP_DATA_PATH_CTRL1, ctrl1);
>  }
>  
>  static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
> @@ -977,48 +987,48 @@ static int pxp_start(struct pxp_ctx *ctx, struct vb2_v4l2_buffer *in_vb,
>  		   BF_PXP_PS_SCALE_XSCALE(xscale);
>  	ps_offset = BF_PXP_PS_OFFSET_YOFFSET(0) | BF_PXP_PS_OFFSET_XOFFSET(0);
>  
> -	writel(ctrl, dev->mmio + HW_PXP_CTRL);
> +	pxp_write(dev, HW_PXP_CTRL, ctrl);
>  	/* skip STAT */
> -	writel(out_ctrl, dev->mmio + HW_PXP_OUT_CTRL);
> -	writel(out_buf, dev->mmio + HW_PXP_OUT_BUF);
> -	writel(out_buf2, dev->mmio + HW_PXP_OUT_BUF2);
> -	writel(out_pitch, dev->mmio + HW_PXP_OUT_PITCH);
> -	writel(out_lrc, dev->mmio + HW_PXP_OUT_LRC);
> -	writel(out_ps_ulc, dev->mmio + HW_PXP_OUT_PS_ULC);
> -	writel(out_ps_lrc, dev->mmio + HW_PXP_OUT_PS_LRC);
> -	writel(as_ulc, dev->mmio + HW_PXP_OUT_AS_ULC);
> -	writel(as_lrc, dev->mmio + HW_PXP_OUT_AS_LRC);
> -	writel(ps_ctrl, dev->mmio + HW_PXP_PS_CTRL);
> -	writel(ps_buf, dev->mmio + HW_PXP_PS_BUF);
> -	writel(ps_ubuf, dev->mmio + HW_PXP_PS_UBUF);
> -	writel(ps_vbuf, dev->mmio + HW_PXP_PS_VBUF);
> -	writel(ps_pitch, dev->mmio + HW_PXP_PS_PITCH);
> -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_BACKGROUND_0);
> -	writel(ps_scale, dev->mmio + HW_PXP_PS_SCALE);
> -	writel(ps_offset, dev->mmio + HW_PXP_PS_OFFSET);
> +	pxp_write(dev, HW_PXP_OUT_CTRL, out_ctrl);
> +	pxp_write(dev, HW_PXP_OUT_BUF, out_buf);
> +	pxp_write(dev, HW_PXP_OUT_BUF2, out_buf2);
> +	pxp_write(dev, HW_PXP_OUT_PITCH, out_pitch);
> +	pxp_write(dev, HW_PXP_OUT_LRC, out_lrc);
> +	pxp_write(dev, HW_PXP_OUT_PS_ULC, out_ps_ulc);
> +	pxp_write(dev, HW_PXP_OUT_PS_LRC, out_ps_lrc);
> +	pxp_write(dev, HW_PXP_OUT_AS_ULC, as_ulc);
> +	pxp_write(dev, HW_PXP_OUT_AS_LRC, as_lrc);
> +	pxp_write(dev, HW_PXP_PS_CTRL, ps_ctrl);
> +	pxp_write(dev, HW_PXP_PS_BUF, ps_buf);
> +	pxp_write(dev, HW_PXP_PS_UBUF, ps_ubuf);
> +	pxp_write(dev, HW_PXP_PS_VBUF, ps_vbuf);
> +	pxp_write(dev, HW_PXP_PS_PITCH, ps_pitch);
> +	pxp_write(dev, HW_PXP_PS_BACKGROUND_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_PS_SCALE, ps_scale);
> +	pxp_write(dev, HW_PXP_PS_OFFSET, ps_offset);
>  	/* disable processed surface color keying */
> -	writel(0x00ffffff, dev->mmio + HW_PXP_PS_CLRKEYLOW_0);
> -	writel(0x00000000, dev->mmio + HW_PXP_PS_CLRKEYHIGH_0);
> +	pxp_write(dev, HW_PXP_PS_CLRKEYLOW_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_PS_CLRKEYHIGH_0, 0x00000000);
>  
>  	/* disable alpha surface color keying */
> -	writel(0x00ffffff, dev->mmio + HW_PXP_AS_CLRKEYLOW_0);
> -	writel(0x00000000, dev->mmio + HW_PXP_AS_CLRKEYHIGH_0);
> +	pxp_write(dev, HW_PXP_AS_CLRKEYLOW_0, 0x00ffffff);
> +	pxp_write(dev, HW_PXP_AS_CLRKEYHIGH_0, 0x00000000);
>  
>  	/* setup CSC */
>  	pxp_setup_csc(ctx);
>  
>  	/* bypass LUT */
> -	writel(BM_PXP_LUT_CTRL_BYPASS, dev->mmio + HW_PXP_LUT_CTRL);
> +	pxp_write(dev, HW_PXP_LUT_CTRL, BM_PXP_LUT_CTRL_BYPASS);
>  
>  	pxp_set_data_path(ctx);
>  
> -	writel(0xffff, dev->mmio + HW_PXP_IRQ_MASK);
> +	pxp_write(dev, HW_PXP_IRQ_MASK, 0xffff);
>  
>  	/* ungate, enable PS/AS/OUT and PXP operation */
> -	writel(BM_PXP_CTRL_IRQ_ENABLE, dev->mmio + HW_PXP_CTRL_SET);
> -	writel(BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> -	       BM_PXP_CTRL_ENABLE_ROTATE0 |
> -	       BM_PXP_CTRL_ENABLE_PS_AS_OUT, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_IRQ_ENABLE);
> +	pxp_write(dev, HW_PXP_CTRL_SET,
> +		  BM_PXP_CTRL_ENABLE | BM_PXP_CTRL_ENABLE_CSC2 |
> +		  BM_PXP_CTRL_ENABLE_ROTATE0 | BM_PXP_CTRL_ENABLE_PS_AS_OUT);
>  
>  	return 0;
>  }
> @@ -1091,23 +1101,23 @@ static irqreturn_t pxp_irq_handler(int irq, void *dev_id)
>  	struct pxp_dev *dev = dev_id;
>  	u32 stat;
>  
> -	stat = readl(dev->mmio + HW_PXP_STAT);
> +	stat = pxp_read(dev, HW_PXP_STAT);
>  
>  	if (stat & BM_PXP_STAT_IRQ0) {
>  		/* we expect x = 0, y = height, irq0 = 1 */
>  		if (stat & ~(BM_PXP_STAT_BLOCKX | BM_PXP_STAT_BLOCKY |
>  			     BM_PXP_STAT_IRQ0))
>  			dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
> -		writel(BM_PXP_STAT_IRQ0, dev->mmio + HW_PXP_STAT_CLR);
> +		pxp_write(dev, HW_PXP_STAT_CLR, BM_PXP_STAT_IRQ0);
>  
>  		pxp_job_finish(dev);
>  	} else {
> -		u32 irq = readl(dev->mmio + HW_PXP_IRQ);
> +		u32 irq = pxp_read(dev, HW_PXP_IRQ);
>  
>  		dprintk(dev, "%s: stat = 0x%08x\n", __func__, stat);
>  		dprintk(dev, "%s: irq = 0x%08x\n", __func__, irq);
>  
> -		writel(irq, dev->mmio + HW_PXP_IRQ_CLR);
> +		pxp_write(dev, HW_PXP_IRQ_CLR, irq);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -1753,25 +1763,25 @@ static int pxp_soft_reset(struct pxp_dev *dev)
>  	int ret;
>  	u32 val;
>  
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
>  
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
>  
>  	ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
>  				 val & BM_PXP_CTRL_CLKGATE, 0, 100);
>  	if (ret < 0)
>  		return ret;
>  
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_SFTRST);
> +	pxp_write(dev, HW_PXP_CTRL_CLR, BM_PXP_CTRL_CLKGATE);
>  
>  	return 0;
>  }
>  
>  static u32 pxp_read_version(struct pxp_dev *dev)
>  {
> -	return readl(dev->mmio + HW_PXP_VERSION);
> +	return pxp_read(dev, HW_PXP_VERSION);
>  }
>  
>  static int pxp_probe(struct platform_device *pdev)
> @@ -1902,8 +1912,8 @@ static int pxp_remove(struct platform_device *pdev)
>  {
>  	struct pxp_dev *dev = platform_get_drvdata(pdev);
>  
> -	writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_SET);
> -	writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_SET);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_CLKGATE);
> +	pxp_write(dev, HW_PXP_CTRL_SET, BM_PXP_CTRL_SFTRST);
>  
>  	clk_disable_unprepare(dev->clk);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 6/6] media: imx-pxp: Use non-threaded IRQ

[Michael Tretter]

On Fri, 06 Jan 2023 15:32:27 +0200, Laurent Pinchart wrote:
> There's no need to use threaded IRQs with the PXP, as the interrupt
> handler doesn't need to sleep and doesn't perform any time-consuming
> operation. Switch to regular IRQ handler.
> 
> This fixes lockups of the PXP noticed on i.MX7, where the PXP would stop
> generating interrupts after a variable number of frames (from a few
> dozens to a few hundreds). The root cause is however unknown.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  drivers/media/platform/nxp/imx-pxp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index e4d7a6339929..28a222c8fe86 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -1814,8 +1814,8 @@ static int pxp_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&dev->irqlock);
>  
> -	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, pxp_irq_handler,
> -			IRQF_ONESHOT, dev_name(&pdev->dev), dev);
> +	ret = devm_request_irq(&pdev->dev, irq, pxp_irq_handler,
> +			       IRQF_TRIGGER_HIGH, dev_name(&pdev->dev), dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to request irq: %d\n", ret);
>  		return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH v1 2/6] media: imx-pxp: Add media controller support

[Laurent Pinchart]

Hi Michael,

On Thu, Jan 12, 2023 at 02:58:20PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 15:32:23 +0200, Laurent Pinchart wrote:
> > Register a media device for the PXP, using the v4l2-mem2mem MC
> > infrastructure to populate the media graph. No media device operation is
> > implemented, the main use of the MC API is to allow consistent discovery
> > of media devices for userspace.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 37 ++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index dcb04217288b..132065c8b8b4 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> >  
> > +#include <media/media-device.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-event.h>
> > @@ -200,6 +201,9 @@ struct pxp_pdata {
> >  struct pxp_dev {
> >  	struct v4l2_device	v4l2_dev;
> >  	struct video_device	vfd;
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	struct media_device	mdev;
> > +#endif
> >  
> >  	struct clk		*clk;
> >  	void __iomem		*mmio;
> > @@ -1832,8 +1836,36 @@ static int pxp_probe(struct platform_device *pdev)
> >  		goto err_m2m;
> >  	}
> >  
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	dev->mdev.dev = &pdev->dev;
> > +	strscpy(dev->mdev.model, MEM2MEM_NAME, sizeof(dev->mdev.model));
> > +	snprintf(dev->mdev.bus_info, sizeof(dev->mdev.bus_info), "platform:%s",
> > +		 dev_name(&pdev->dev));
> 
> v4l2-compliance gives the following warning:
> 
> 	warn: v4l2-compliance.cpp(642): media bus_info 'platform:30700000.pxp' differs from V4L2 bus_info 'platform:pxp'
> 
> I would change this patch to use the same name as in the V4L2 bus_info. Do you
> have a reason for including the address in the bus_info?

Actually, I should drop bus_info here. The documentation of
media_device_init() states

 * The bus_info field is set by media_device_init() for PCI and platform devices
 * if the field begins with '\0'.

media_device_init() calls media_set_bus_info() which is defined as

/**
 * media_set_bus_info() - Set bus_info field
 *
 * @bus_info:		Variable where to write the bus info (char array)
 * @bus_info_size:	Length of the bus_info
 * @dev:		Related struct device
 *
 * Sets bus information based on &dev. This is currently done for PCI and
 * platform devices. dev is required to be non-NULL for this to happen.
 *
 * This function is not meant to be called from drivers.
 */
static inline void
media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
{
	if (!dev)
		strscpy(bus_info, "no bus info", bus_info_size);
	else if (dev_is_platform(dev))
		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
	else if (dev_is_pci(dev))
		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
}

v4l_querycap() also calls media_set_bus_info(). Including the device
name is thus the recommended option.

I'll include in v2 another patch that drops the bus_info from the
video_device to make v4l2-compliance happy.

> > +	media_device_init(&dev->mdev);
> > +	dev->v4l2_dev.mdev = &dev->mdev;
> > +
> > +	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
> > +						 MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to initialize media device\n");
> > +		goto err_vfd;
> > +	}
> > +
> > +	ret = media_device_register(&dev->mdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to register media device\n");
> > +		goto err_m2m_mc;
> > +	}
> > +#endif
> > +
> >  	return 0;
> >  
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +err_m2m_mc:
> > +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> > +err_vfd:
> > +	video_unregister_device(vfd);
> > +#endif
> >  err_m2m:
> >  	v4l2_m2m_release(dev->m2m_dev);
> >  err_v4l2:
> > @@ -1854,6 +1886,11 @@ static int pxp_remove(struct platform_device *pdev)
> >  	clk_disable_unprepare(dev->clk);
> >  
> >  	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	media_device_unregister(&dev->mdev);
> > +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> > +#endif
> >  	video_unregister_device(&dev->vfd);
> >  	v4l2_m2m_release(dev->m2m_dev);
> >  	v4l2_device_unregister(&dev->v4l2_dev);

-- 
Regards,

Laurent Pinchart