[PATCH v2 0/6] media: ti, cdns: Multiple pixel support and misc fixes

[PATCH v2 0/6] media: ti, cdns: Multiple pixel support and misc fixes

[Jai Luthra]

Hi,

The first four patches in this series are miscellaneous fixes and
improvements in the Cadence and TI CSI-RX drivers around probing, fwnode
and link creation.

The last two patches add support for transmitting multiple pixels per
clock on the internal bus between Cadence CSI-RX bridge and TI CSI-RX
wrapper. As this internal bus is 32-bit wide, the maximum number of
pixels that can be transmitted per cycle depend upon the format's bit
width. Secondly, the downstream element must support unpacking of
multiple pixels.

Thus we export a module function that can be used by the downstream
driver to negotiate the pixels per cycle on the output pixel stream of
the Cadence bridge.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Changes in v2:
- Rebase on v6.15-rc1
- Fix lkp warnings in PATCH 5/6 missing header for FIELD_PREP
- Add R-By tags from Devarsh and Changhuang
- Link to v1: https://lore.kernel.org/r/20250324-probe_fixes-v1-0-5cd5b9e1cfac@ideasonboard.com

---
Jai Luthra (6):
      media: ti: j721e-csi2rx: Use devm_of_platform_populate
      media: ti: j721e-csi2rx: Use fwnode_get_named_child_node
      media: ti: j721e-csi2rx: Fix source subdev link creation
      media: cadence: csi2rx: Implement get_fwnode_pad op
      media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle
      media: ti: j721e-csi2rx: Support multiple pixels per clock

 drivers/media/platform/cadence/cdns-csi2rx.c       | 76 +++++++++++++++++-----
 drivers/media/platform/cadence/cdns-csi2rx.h       | 19 ++++++
 drivers/media/platform/ti/Kconfig                  |  3 +-
 .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 66 ++++++++++++++-----
 4 files changed, 129 insertions(+), 35 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250314-probe_fixes-7e0ec33c7fee

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH v2 1/6] media: ti: j721e-csi2rx: Use devm_of_platform_populate

[Jai Luthra]

Ensure that we clean up the platform bus when we remove this driver.

This fixes a crash seen when reloading the module for the child device
with the parent not yet reloaded.

Fixes: b4a3d877dc92 ("media: ti: Add CSI2RX support for J721E")
Cc: stable@vger.kernel.org
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index 6412a00be8eab89548950dd21b3b3ec02dafa5b4..a066024bf745450e2ba01d06c0fec4e6bdbfa97e 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -1118,7 +1118,7 @@ static int ti_csi2rx_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_vb2q;
 
-	ret = of_platform_populate(csi->dev->of_node, NULL, NULL, csi->dev);
+	ret = devm_of_platform_populate(csi->dev);
 	if (ret) {
 		dev_err(csi->dev, "Failed to create children: %d\n", ret);
 		goto err_subdev;

-- 
2.49.0

[PATCH v2 2/6] media: ti: j721e-csi2rx: Use fwnode_get_named_child_node

[Jai Luthra]

Simplify notifier registration logic. Instead of first getting the
device node, get the fwnode of the child directly.

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index a066024bf745450e2ba01d06c0fec4e6bdbfa97e..6d406925e092660cb67c04cc2a7e1e10c14e295e 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 #include <media/mipi-csi2.h>
 #include <media/v4l2-device.h>
@@ -450,25 +451,23 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
 {
 	struct fwnode_handle *fwnode;
 	struct v4l2_async_connection *asc;
-	struct device_node *node;
 	int ret;
 
-	node = of_get_child_by_name(csi->dev->of_node, "csi-bridge");
-	if (!node)
+	fwnode = fwnode_get_named_child_node(csi->dev->fwnode, "csi-bridge");
+	if (!fwnode)
 		return -EINVAL;
 
-	fwnode = of_fwnode_handle(node);
-	if (!fwnode) {
-		of_node_put(node);
-		return -EINVAL;
-	}
-
 	v4l2_async_nf_init(&csi->notifier, &csi->v4l2_dev);
 	csi->notifier.ops = &csi_async_notifier_ops;
 
 	asc = v4l2_async_nf_add_fwnode(&csi->notifier, fwnode,
 				       struct v4l2_async_connection);
-	of_node_put(node);
+	/*
+	 * Calling v4l2_async_nf_add_fwnode grabs a refcount,
+	 * so drop the one we got in fwnode_get_named_child_node
+	 */
+	fwnode_handle_put(fwnode);
+
 	if (IS_ERR(asc)) {
 		v4l2_async_nf_cleanup(&csi->notifier);
 		return PTR_ERR(asc);

-- 
2.49.0

[PATCH v2 3/6] media: ti: j721e-csi2rx: Fix source subdev link creation

[Jai Luthra]

We don't use OF ports and remote-endpoints to connect the CSI2RX bridge
and this device in the device tree, thus it is wrong to use
v4l2_create_fwnode_links_to_pad() to create the media graph link between
the two.

It works out on accident, as neither the source nor the sink implement
the .get_fwnode_pad() callback, and the framework helper falls back on
using the first source and sink pads to create the link between them.

Instead, manually create the media link from the first source pad of the
bridge to the first sink pad of the J721E CSI2RX.

Fixes: b4a3d877dc92 ("media: ti: Add CSI2RX support for J721E")
Cc: stable@vger.kernel.org
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index 6d406925e092660cb67c04cc2a7e1e10c14e295e..ad51d033b6725426550578bdac1bae8443458f13 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -53,6 +53,8 @@
 #define DRAIN_TIMEOUT_MS		50
 #define DRAIN_BUFFER_SIZE		SZ_32K
 
+#define CSI2RX_BRIDGE_SOURCE_PAD	1
+
 struct ti_csi2rx_fmt {
 	u32				fourcc;	/* Four character code. */
 	u32				code;	/* Mbus code. */
@@ -427,8 +429,9 @@ static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
 	if (ret)
 		return ret;
 
-	ret = v4l2_create_fwnode_links_to_pad(csi->source, &csi->pad,
-					      MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	ret = media_create_pad_link(&csi->source->entity, CSI2RX_BRIDGE_SOURCE_PAD,
+				    &vdev->entity, csi->pad.index,
+				    MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
 
 	if (ret) {
 		video_unregister_device(vdev);

-- 
2.49.0

[PATCH v2 4/6] media: cadence: csi2rx: Implement get_fwnode_pad op

[Jai Luthra]

Use v4l2_subdev_get_fwnode_pad_1_to_1() as the get_fwnode_pad operation.
Cadence CSI2RX maps port numbers and pad indices 1:1.

Reviewed-by: Changhuang Liang <changhuang.liang@starfivetech.com>
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index cebcae196eeccc65548d2c8e14bcba4799415beb..608298c72462031515d9ad01c6b267bf7375a5bf 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -479,6 +479,7 @@ static const struct v4l2_subdev_internal_ops csi2rx_internal_ops = {
 
 static const struct media_entity_operations csi2rx_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,
+	.get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
 };
 
 static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,

-- 
2.49.0

[PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle

[Jai Luthra]

The output pixel interface is a parallel bus (32 bits), which
supports sending multiple pixels (1, 2 or 4) per clock cycle for
smaller pixel widths like RAW8-RAW16.

Dual-pixel and Quad-pixel modes can be a requirement if the export rate
of the Cadence IP in Single-pixel mode maxes out before the maximum
supported DPHY-RX frequency, which is the case with TI's integration of
this IP [1].

So, we export a function that lets the downstream hardware block request
a higher pixel-per-clock on a particular output pad.

We check if we can support the requested pixels per clock given the
known maximum for the currently configured format. If not, we set it
to the highest feasible value and return this value to the caller.

[1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM

Link: https://www.ti.com/lit/pdf/spruj16
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
 drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
 2 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Cadence Design Systems Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -22,6 +23,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "cdns-csi2rx.h"
+
 #define CSI2RX_DEVICE_CFG_REG			0x000
 
 #define CSI2RX_SOFT_RESET_REG			0x004
@@ -53,6 +56,8 @@
 
 #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
 #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
+#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK		GENMASK(5, 4)
+#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)			((n) >> 1)
 
 #define CSI2RX_LANES_MAX	4
 #define CSI2RX_STREAMS_MAX	4
@@ -68,7 +73,10 @@ enum csi2rx_pads {
 
 struct csi2rx_fmt {
 	u32				code;
+	/* width of a single pixel on CSI-2 bus */
 	u8				bpp;
+	/* max pixels per clock supported on output bus */
+	u8				max_pixels;
 };
 
 struct csi2rx_priv {
@@ -90,6 +98,7 @@ struct csi2rx_priv {
 	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
 	struct phy			*dphy;
 
+	u8				num_pixels[CSI2RX_STREAMS_MAX];
 	u8				lanes[CSI2RX_LANES_MAX];
 	u8				num_lanes;
 	u8				max_lanes;
@@ -106,22 +115,22 @@ struct csi2rx_priv {
 };
 
 static const struct csi2rx_fmt formats[] = {
-	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
-	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
-	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
-	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
-	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
+	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
+	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
+	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
+	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
+	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
 };
 
 static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
@@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
 
 		reset_control_deassert(csi2rx->pixel_rst[i]);
 
-		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
-		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
+		reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
+		reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
+				  csi2rx->num_pixels[i]);
+		writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
 
 		/*
 		 * Enable one virtual channel. When multiple virtual channels
@@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
 	return csi2rx_set_fmt(subdev, state, &format);
 }
 
+int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
+			      u8 *ppc)
+{
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+	const struct csi2rx_fmt *csi_fmt;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
+
+	if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_and_get_active_state(subdev);
+	fmt = v4l2_subdev_state_get_format(state, pad);
+	csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
+
+	/* Reduce requested PPC if it is too high */
+	*ppc = min(*ppc, csi_fmt->max_pixels);
+
+	v4l2_subdev_unlock_state(state);
+
+	csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
+		CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
+
+	return ret;
+}
+EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);
+
 static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
 	.enum_mbus_code	= csi2rx_enum_mbus_code,
 	.get_fmt	= v4l2_subdev_get_fmt,
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h
new file mode 100644
index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#ifndef CDNS_CSI2RX_H
+#define CDNS_CSI2RX_H
+
+#include <media/v4l2-subdev.h>
+
+/**
+ * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
+ *
+ * @subdev: point to &struct v4l2_subdev
+ * @pad: pad number of the source pad
+ * @ppc: pointer to requested pixel-per-clock value
+ *
+ * Returns 0 on success, negative error code otherwise.
+ */
+int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
+			      u8 *ppc);
+
+#endif

-- 
2.49.0

[PATCH v2 6/6] media: ti: j721e-csi2rx: Support multiple pixels per clock

[Jai Luthra]

Add support for negotiating the highest possible pixel mode (from
single, dual, quad) with the Cadence CSI2RX bridge. This is required to
drain the Cadence stream FIFOs without overflowing when the source is
operating at a high link-frequency [1].

Also, update the Kconfig as this introduces a hard build-time dependency
on the Cadence CSI2RX driver, even for a COMPILE_TEST.

[1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM

Link: https://www.ti.com/lit/pdf/spruj16
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/platform/ti/Kconfig                  |  3 +-
 .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 38 ++++++++++++++++++++--
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/ti/Kconfig b/drivers/media/platform/ti/Kconfig
index bab998c4179aca3b07372782b9be7de340cb8d45..3bc4aa35887e6edc9fa8749d9956a67714c59001 100644
--- a/drivers/media/platform/ti/Kconfig
+++ b/drivers/media/platform/ti/Kconfig
@@ -67,7 +67,8 @@ config VIDEO_TI_J721E_CSI2RX
 	tristate "TI J721E CSI2RX wrapper layer driver"
 	depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API
 	depends on MEDIA_SUPPORT && MEDIA_CONTROLLER
-	depends on (PHY_CADENCE_DPHY_RX && VIDEO_CADENCE_CSI2RX) || COMPILE_TEST
+	depends on VIDEO_CADENCE_CSI2RX
+	depends on PHY_CADENCE_DPHY_RX || COMPILE_TEST
 	depends on ARCH_K3 || COMPILE_TEST
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_FWNODE
diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index ad51d033b6725426550578bdac1bae8443458f13..425324c3d6802cfda79d116d3967b61a2e7a015a 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -21,6 +21,8 @@
 #include <media/v4l2-mc.h>
 #include <media/videobuf2-dma-contig.h>
 
+#include "../../cadence/cdns-csi2rx.h"
+
 #define TI_CSI2RX_MODULE_NAME		"j721e-csi2rx"
 
 #define SHIM_CNTL			0x10
@@ -29,6 +31,7 @@
 #define SHIM_DMACNTX			0x20
 #define SHIM_DMACNTX_EN			BIT(31)
 #define SHIM_DMACNTX_YUV422		GENMASK(27, 26)
+#define SHIM_DMACNTX_DUAL_PCK_CFG	BIT(24)
 #define SHIM_DMACNTX_SIZE		GENMASK(21, 20)
 #define SHIM_DMACNTX_FMT		GENMASK(5, 0)
 #define SHIM_DMACNTX_YUV422_MODE_11	3
@@ -40,6 +43,7 @@
 #define SHIM_PSI_CFG0_SRC_TAG		GENMASK(15, 0)
 #define SHIM_PSI_CFG0_DST_TAG		GENMASK(31, 16)
 
+#define TI_CSI2RX_MAX_PIX_PER_CLK	4
 #define PSIL_WORD_SIZE_BYTES		16
 /*
  * There are no hard limits on the width or height. The DMA engine can handle
@@ -110,6 +114,7 @@ struct ti_csi2rx_dev {
 	struct v4l2_format		v_fmt;
 	struct ti_csi2rx_dma		dma;
 	u32				sequence;
+	u8				pix_per_clk;
 };
 
 static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = {
@@ -485,6 +490,26 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
 	return 0;
 }
 
+/* Request maximum possible pixels per clock from the bridge */
+static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
+{
+	struct media_pad *pad;
+	int ret;
+	u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;
+
+	pad = media_entity_remote_source_pad_unique(&csi->vdev.entity);
+	if (!pad)
+		return;
+
+	ret = cdns_csi2rx_negotiate_ppc(csi->source, pad->index, &ppc);
+	if (ret) {
+		dev_warn(csi->dev, "NUM_PIXELS negotiation failed: %d\n", ret);
+		csi->pix_per_clk = 1;
+	} else {
+		csi->pix_per_clk = ppc;
+	}
+}
+
 static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
 {
 	const struct ti_csi2rx_fmt *fmt;
@@ -496,6 +521,9 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
 	reg = SHIM_CNTL_PIX_RST;
 	writel(reg, csi->shim + SHIM_CNTL);
 
+	/* Negotiate pixel count from the source */
+	ti_csi2rx_request_max_ppc(csi);
+
 	reg = SHIM_DMACNTX_EN;
 	reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
 
@@ -524,14 +552,18 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
 	case V4L2_PIX_FMT_YVYU:
 		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
 				  SHIM_DMACNTX_YUV422_MODE_11);
+		/* Multiple pixels are handled differently for packed YUV */
+		if (csi->pix_per_clk == 2)
+			reg |= SHIM_DMACNTX_DUAL_PCK_CFG;
+		reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
 		break;
 	default:
-		/* Ignore if not YUV 4:2:2 */
+		/* By default we change the shift size for multiple pixels */
+		reg |= FIELD_PREP(SHIM_DMACNTX_SIZE,
+				  fmt->size + (csi->pix_per_clk >> 1));
 		break;
 	}
 
-	reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
-
 	writel(reg, csi->shim + SHIM_DMACNTX);
 
 	reg = FIELD_PREP(SHIM_PSI_CFG0_SRC_TAG, 0) |

-- 
2.49.0

Re: [PATCH v2 0/6] media: ti, cdns: Multiple pixel support and misc fixes

[Yemike Abhilash Chandra]

Hi Jai,
Thanks for the patch series.

On 10/04/25 12:18, Jai Luthra wrote:
> Hi,
> 
> The first four patches in this series are miscellaneous fixes and
> improvements in the Cadence and TI CSI-RX drivers around probing, fwnode
> and link creation.
> 
> The last two patches add support for transmitting multiple pixels per
> clock on the internal bus between Cadence CSI-RX bridge and TI CSI-RX
> wrapper. As this internal bus is 32-bit wide, the maximum number of
> pixels that can be transmitted per cycle depend upon the format's bit
> width. Secondly, the downstream element must support unpacking of
> multiple pixels.
> 
> Thus we export a module function that can be used by the downstream
> driver to negotiate the pixels per cycle on the output pixel stream of
> the Cadence bridge.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---

For the entire series,

Tested-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com> [on AM68SK]

> Changes in v2:
> - Rebase on v6.15-rc1
> - Fix lkp warnings in PATCH 5/6 missing header for FIELD_PREP
> - Add R-By tags from Devarsh and Changhuang
> - Link to v1: https://lore.kernel.org/r/20250324-probe_fixes-v1-0-5cd5b9e1cfac@ideasonboard.com
> 
> ---
> Jai Luthra (6):
>        media: ti: j721e-csi2rx: Use devm_of_platform_populate
>        media: ti: j721e-csi2rx: Use fwnode_get_named_child_node
>        media: ti: j721e-csi2rx: Fix source subdev link creation
>        media: cadence: csi2rx: Implement get_fwnode_pad op
>        media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle
>        media: ti: j721e-csi2rx: Support multiple pixels per clock
> 
>   drivers/media/platform/cadence/cdns-csi2rx.c       | 76 +++++++++++++++++-----
>   drivers/media/platform/cadence/cdns-csi2rx.h       | 19 ++++++
>   drivers/media/platform/ti/Kconfig                  |  3 +-
>   .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 66 ++++++++++++++-----
>   4 files changed, 129 insertions(+), 35 deletions(-)
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250314-probe_fixes-7e0ec33c7fee
> 
> Best regards,

Re: [PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle

[Sakari Ailus]

Hi Jai,

Thanks for the patchset.

On Thu, Apr 10, 2025 at 12:19:03PM +0530, Jai Luthra wrote:
> The output pixel interface is a parallel bus (32 bits), which
> supports sending multiple pixels (1, 2 or 4) per clock cycle for
> smaller pixel widths like RAW8-RAW16.
> 
> Dual-pixel and Quad-pixel modes can be a requirement if the export rate
> of the Cadence IP in Single-pixel mode maxes out before the maximum
> supported DPHY-RX frequency, which is the case with TI's integration of
> this IP [1].
> 
> So, we export a function that lets the downstream hardware block request
> a higher pixel-per-clock on a particular output pad.
> 
> We check if we can support the requested pixels per clock given the
> known maximum for the currently configured format. If not, we set it
> to the highest feasible value and return this value to the caller.
> 
> [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> 
> Link: https://www.ti.com/lit/pdf/spruj16
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
>  drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
>  2 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2017 Cadence Design Systems Inc.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> @@ -22,6 +23,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "cdns-csi2rx.h"
> +
>  #define CSI2RX_DEVICE_CFG_REG			0x000
>  
>  #define CSI2RX_SOFT_RESET_REG			0x004
> @@ -53,6 +56,8 @@
>  
>  #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
>  #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)

Not a fault of this patch but this should use BIT(). Or at the very least
(1U << 8). I.e. this isn't a bug but the pattern is bad. It'd be nice to
fix this in a separate patch.

> +#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK		GENMASK(5, 4)
> +#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)			((n) >> 1)
>  
>  #define CSI2RX_LANES_MAX	4
>  #define CSI2RX_STREAMS_MAX	4
> @@ -68,7 +73,10 @@ enum csi2rx_pads {
>  
>  struct csi2rx_fmt {
>  	u32				code;
> +	/* width of a single pixel on CSI-2 bus */
>  	u8				bpp;
> +	/* max pixels per clock supported on output bus */
> +	u8				max_pixels;
>  };
>  
>  struct csi2rx_priv {
> @@ -90,6 +98,7 @@ struct csi2rx_priv {
>  	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
>  	struct phy			*dphy;
>  
> +	u8				num_pixels[CSI2RX_STREAMS_MAX];
>  	u8				lanes[CSI2RX_LANES_MAX];
>  	u8				num_lanes;
>  	u8				max_lanes;
> @@ -106,22 +115,22 @@ struct csi2rx_priv {
>  };
>  
>  static const struct csi2rx_fmt formats[] = {
> -	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
> -	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
> -	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
> -	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
> -	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
> +	{ .code	= MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
> +	{ .code	= MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
> +	{ .code	= MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
> +	{ .code	= MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
> +	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
>  };
>  
>  static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> @@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  
>  		reset_control_deassert(csi2rx->pixel_rst[i]);
>  
> -		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> -		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +		reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
> +		reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
> +				  csi2rx->num_pixels[i]);
> +		writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));

I'd write this as:

		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
		       FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
				  csi2rx->num_pixels[i]),
		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));

But up to you.

>  
>  		/*
>  		 * Enable one virtual channel. When multiple virtual channels
> @@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
>  	return csi2rx_set_fmt(subdev, state, &format);
>  }
>  
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> +			      u8 *ppc)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +	const struct csi2rx_fmt *csi_fmt;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;

ret is redundant.

> +
> +	if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(subdev);
> +	fmt = v4l2_subdev_state_get_format(state, pad);
> +	csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
> +
> +	/* Reduce requested PPC if it is too high */
> +	*ppc = min(*ppc, csi_fmt->max_pixels);
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
> +		CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);

EXPORT_SYMBOL_GPL(). Or maybe use a namespace?

> +
>  static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
>  	.enum_mbus_code	= csi2rx_enum_mbus_code,
>  	.get_fmt	= v4l2_subdev_get_fmt,
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h

I wonder if it'd be better to put this under include/media.

> new file mode 100644
> index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef CDNS_CSI2RX_H
> +#define CDNS_CSI2RX_H
> +
> +#include <media/v4l2-subdev.h>
> +
> +/**
> + * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
> + *
> + * @subdev: point to &struct v4l2_subdev
> + * @pad: pad number of the source pad
> + * @ppc: pointer to requested pixel-per-clock value
> + *
> + * Returns 0 on success, negative error code otherwise.
> + */
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> +			      u8 *ppc);
> +
> +#endif
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 6/6] media: ti: j721e-csi2rx: Support multiple pixels per clock

[Sakari Ailus]

Hi Jai,

On Thu, Apr 10, 2025 at 12:19:04PM +0530, Jai Luthra wrote:
> Add support for negotiating the highest possible pixel mode (from
> single, dual, quad) with the Cadence CSI2RX bridge. This is required to
> drain the Cadence stream FIFOs without overflowing when the source is
> operating at a high link-frequency [1].
> 
> Also, update the Kconfig as this introduces a hard build-time dependency
> on the Cadence CSI2RX driver, even for a COMPILE_TEST.
> 
> [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> 
> Link: https://www.ti.com/lit/pdf/spruj16
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

This creates a dependency between the two drivers.

Can you confirm that the TI device only exists in conjunction with the
cadence HW block?

> ---
>  drivers/media/platform/ti/Kconfig                  |  3 +-
>  .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 38 ++++++++++++++++++++--
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/Kconfig b/drivers/media/platform/ti/Kconfig
> index bab998c4179aca3b07372782b9be7de340cb8d45..3bc4aa35887e6edc9fa8749d9956a67714c59001 100644
> --- a/drivers/media/platform/ti/Kconfig
> +++ b/drivers/media/platform/ti/Kconfig
> @@ -67,7 +67,8 @@ config VIDEO_TI_J721E_CSI2RX
>  	tristate "TI J721E CSI2RX wrapper layer driver"
>  	depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API
>  	depends on MEDIA_SUPPORT && MEDIA_CONTROLLER
> -	depends on (PHY_CADENCE_DPHY_RX && VIDEO_CADENCE_CSI2RX) || COMPILE_TEST
> +	depends on VIDEO_CADENCE_CSI2RX
> +	depends on PHY_CADENCE_DPHY_RX || COMPILE_TEST
>  	depends on ARCH_K3 || COMPILE_TEST
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_FWNODE
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index ad51d033b6725426550578bdac1bae8443458f13..425324c3d6802cfda79d116d3967b61a2e7a015a 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -21,6 +21,8 @@
>  #include <media/v4l2-mc.h>
>  #include <media/videobuf2-dma-contig.h>
>  
> +#include "../../cadence/cdns-csi2rx.h"
> +
>  #define TI_CSI2RX_MODULE_NAME		"j721e-csi2rx"
>  
>  #define SHIM_CNTL			0x10
> @@ -29,6 +31,7 @@
>  #define SHIM_DMACNTX			0x20
>  #define SHIM_DMACNTX_EN			BIT(31)
>  #define SHIM_DMACNTX_YUV422		GENMASK(27, 26)
> +#define SHIM_DMACNTX_DUAL_PCK_CFG	BIT(24)
>  #define SHIM_DMACNTX_SIZE		GENMASK(21, 20)
>  #define SHIM_DMACNTX_FMT		GENMASK(5, 0)
>  #define SHIM_DMACNTX_YUV422_MODE_11	3
> @@ -40,6 +43,7 @@
>  #define SHIM_PSI_CFG0_SRC_TAG		GENMASK(15, 0)
>  #define SHIM_PSI_CFG0_DST_TAG		GENMASK(31, 16)
>  
> +#define TI_CSI2RX_MAX_PIX_PER_CLK	4
>  #define PSIL_WORD_SIZE_BYTES		16
>  /*
>   * There are no hard limits on the width or height. The DMA engine can handle
> @@ -110,6 +114,7 @@ struct ti_csi2rx_dev {
>  	struct v4l2_format		v_fmt;
>  	struct ti_csi2rx_dma		dma;
>  	u32				sequence;
> +	u8				pix_per_clk;
>  };
>  
>  static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = {
> @@ -485,6 +490,26 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
>  	return 0;
>  }
>  
> +/* Request maximum possible pixels per clock from the bridge */
> +static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
> +{
> +	struct media_pad *pad;
> +	int ret;
> +	u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;

Could you make this look like a reverse Christmas tree?

> +
> +	pad = media_entity_remote_source_pad_unique(&csi->vdev.entity);
> +	if (!pad)
> +		return;
> +
> +	ret = cdns_csi2rx_negotiate_ppc(csi->source, pad->index, &ppc);
> +	if (ret) {
> +		dev_warn(csi->dev, "NUM_PIXELS negotiation failed: %d\n", ret);
> +		csi->pix_per_clk = 1;
> +	} else {
> +		csi->pix_per_clk = ppc;
> +	}
> +}
> +
>  static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
>  {
>  	const struct ti_csi2rx_fmt *fmt;
> @@ -496,6 +521,9 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
>  	reg = SHIM_CNTL_PIX_RST;
>  	writel(reg, csi->shim + SHIM_CNTL);
>  
> +	/* Negotiate pixel count from the source */
> +	ti_csi2rx_request_max_ppc(csi);
> +
>  	reg = SHIM_DMACNTX_EN;
>  	reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
>  
> @@ -524,14 +552,18 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
>  	case V4L2_PIX_FMT_YVYU:
>  		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
>  				  SHIM_DMACNTX_YUV422_MODE_11);
> +		/* Multiple pixels are handled differently for packed YUV */
> +		if (csi->pix_per_clk == 2)
> +			reg |= SHIM_DMACNTX_DUAL_PCK_CFG;
> +		reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
>  		break;
>  	default:
> -		/* Ignore if not YUV 4:2:2 */
> +		/* By default we change the shift size for multiple pixels */
> +		reg |= FIELD_PREP(SHIM_DMACNTX_SIZE,
> +				  fmt->size + (csi->pix_per_clk >> 1));
>  		break;
>  	}
>  
> -	reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
> -
>  	writel(reg, csi->shim + SHIM_DMACNTX);
>  
>  	reg = FIELD_PREP(SHIM_PSI_CFG0_SRC_TAG, 0) |
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple pixels per clock cycle

[Jai Luthra]

Hi Sakari,

Thanks for the review.

Quoting Sakari Ailus (2025-06-25 23:00:22)
> Hi Jai,
> 
> Thanks for the patchset.
> 
> On Thu, Apr 10, 2025 at 12:19:03PM +0530, Jai Luthra wrote:
> > The output pixel interface is a parallel bus (32 bits), which
> > supports sending multiple pixels (1, 2 or 4) per clock cycle for
> > smaller pixel widths like RAW8-RAW16.
> > 
> > Dual-pixel and Quad-pixel modes can be a requirement if the export rate
> > of the Cadence IP in Single-pixel mode maxes out before the maximum
> > supported DPHY-RX frequency, which is the case with TI's integration of
> > this IP [1].
> > 
> > So, we export a function that lets the downstream hardware block request
> > a higher pixel-per-clock on a particular output pad.
> > 
> > We check if we can support the requested pixels per clock given the
> > known maximum for the currently configured format. If not, we set it
> > to the highest feasible value and return this value to the caller.
> > 
> > [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> > 
> > Link: https://www.ti.com/lit/pdf/spruj16
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
> >  drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
> >  2 files changed, 76 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2017 Cadence Design Systems Inc.
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/io.h>
> > @@ -22,6 +23,8 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >  
> > +#include "cdns-csi2rx.h"
> > +
> >  #define CSI2RX_DEVICE_CFG_REG                        0x000
> >  
> >  #define CSI2RX_SOFT_RESET_REG                        0x004
> > @@ -53,6 +56,8 @@
> >  
> >  #define CSI2RX_STREAM_CFG_REG(n)             (CSI2RX_STREAM_BASE(n) + 0x00c)
> >  #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF                (1 << 8)
> 
> Not a fault of this patch but this should use BIT(). Or at the very least
> (1U << 8). I.e. this isn't a bug but the pattern is bad. It'd be nice to
> fix this in a separate patch.
> 

Ah, that's my bad, rest of the driver does already use BIT().. will fix in
v3.

> > +#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK            GENMASK(5, 4)
> > +#define CSI2RX_STREAM_CFG_NUM_PIXELS(n)                      ((n) >> 1)
> >  
> >  #define CSI2RX_LANES_MAX     4
> >  #define CSI2RX_STREAMS_MAX   4
> > @@ -68,7 +73,10 @@ enum csi2rx_pads {
> >  
> >  struct csi2rx_fmt {
> >       u32                             code;
> > +     /* width of a single pixel on CSI-2 bus */
> >       u8                              bpp;
> > +     /* max pixels per clock supported on output bus */
> > +     u8                              max_pixels;
> >  };
> >  
> >  struct csi2rx_priv {
> > @@ -90,6 +98,7 @@ struct csi2rx_priv {
> >       struct reset_control            *pixel_rst[CSI2RX_STREAMS_MAX];
> >       struct phy                      *dphy;
> >  
> > +     u8                              num_pixels[CSI2RX_STREAMS_MAX];
> >       u8                              lanes[CSI2RX_LANES_MAX];
> >       u8                              num_lanes;
> >       u8                              max_lanes;
> > @@ -106,22 +115,22 @@ struct csi2rx_priv {
> >  };
> >  
> >  static const struct csi2rx_fmt formats[] = {
> > -     { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, },
> > -     { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
> > -     { .code = MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, },
> > -     { .code = MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, },
> > -     { .code = MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
> > +     { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_Y8_1X8,     .bpp = 8, .max_pixels = 4, },
> > +     { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
> > +     { .code = MEDIA_BUS_FMT_RGB565_1X16,  .bpp = 16, .max_pixels = 1, },
> > +     { .code = MEDIA_BUS_FMT_RGB888_1X24,  .bpp = 24, .max_pixels = 1, },
> > +     { .code = MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, .max_pixels = 1, },
> >  };
> >  
> >  static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> > @@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> >  
> >               reset_control_deassert(csi2rx->pixel_rst[i]);
> >  
> > -             writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> > -                    csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> > +             reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
> > +             reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
> > +                               csi2rx->num_pixels[i]);
> > +             writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> 
> I'd write this as:
> 
>                 writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
>                        FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
>                                   csi2rx->num_pixels[i]),
>                        csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> 
> But up to you.
> 

Will do in v3.

> >  
> >               /*
> >                * Enable one virtual channel. When multiple virtual channels
> > @@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
> >       return csi2rx_set_fmt(subdev, state, &format);
> >  }
> >  
> > +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> > +                           u8 *ppc)
> > +{
> > +     struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > +     const struct csi2rx_fmt *csi_fmt;
> > +     struct v4l2_subdev_state *state;
> > +     struct v4l2_mbus_framefmt *fmt;
> > +     int ret = 0;
> 
> ret is redundant.
> 

Good catch, will fix.

> > +
> > +     if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
> > +             return -EINVAL;
> > +
> > +     state = v4l2_subdev_lock_and_get_active_state(subdev);
> > +     fmt = v4l2_subdev_state_get_format(state, pad);
> > +     csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
> > +
> > +     /* Reduce requested PPC if it is too high */
> > +     *ppc = min(*ppc, csi_fmt->max_pixels);
> > +
> > +     v4l2_subdev_unlock_state(state);
> > +
> > +     csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
> > +             CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);
> 
> EXPORT_SYMBOL_GPL(). Or maybe use a namespace?
> 

Ah I wasn't aware of different namespaces. I think a module-specific one as
documented here [1] might make the most sense in this case. Will try that,
else will use _GPL.

[1] https://docs.kernel.org/core-api/symbol-namespaces.html#using-the-export-symbol-gpl-for-modules-macro

> > +
> >  static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> >       .enum_mbus_code = csi2rx_enum_mbus_code,
> >       .get_fmt        = v4l2_subdev_get_fmt,
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h
> 
> I wonder if it'd be better to put this under include/media.
> 

I was unsure about it, as while these are two separate drivers it's
essentially the same "device".. but I don't see a harm in keeping this
under include/media. Will do in v3.

> > new file mode 100644
> > index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
> > --- /dev/null
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +#ifndef CDNS_CSI2RX_H
> > +#define CDNS_CSI2RX_H
> > +
> > +#include <media/v4l2-subdev.h>
> > +
> > +/**
> > + * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
> > + *
> > + * @subdev: point to &struct v4l2_subdev
> > + * @pad: pad number of the source pad
> > + * @ppc: pointer to requested pixel-per-clock value
> > + *
> > + * Returns 0 on success, negative error code otherwise.
> > + */
> > +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> > +                           u8 *ppc);
> > +
> > +#endif
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks,
Jai

Re: [PATCH v2 6/6] media: ti: j721e-csi2rx: Support multiple pixels per clock

[Jai Luthra]

Hi Sakari,

Quoting Sakari Ailus (2025-06-25 23:02:39)
> Hi Jai,
> 
> On Thu, Apr 10, 2025 at 12:19:04PM +0530, Jai Luthra wrote:
> > Add support for negotiating the highest possible pixel mode (from
> > single, dual, quad) with the Cadence CSI2RX bridge. This is required to
> > drain the Cadence stream FIFOs without overflowing when the source is
> > operating at a high link-frequency [1].
> > 
> > Also, update the Kconfig as this introduces a hard build-time dependency
> > on the Cadence CSI2RX driver, even for a COMPILE_TEST.
> > 
> > [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
> > 
> > Link: https://www.ti.com/lit/pdf/spruj16
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> 
> This creates a dependency between the two drivers.
> 
> Can you confirm that the TI device only exists in conjunction with the
> cadence HW block?
> 

Yes, the main job of TI device is to unwrap the Cadence-specific pixel
stream to something the TI K3 DMA engine can understand. So these are (and
will be) always paired together in all K3 SoCs that support CSI2RX.

> > ---
> >  drivers/media/platform/ti/Kconfig                  |  3 +-
> >  .../media/platform/ti/j721e-csi2rx/j721e-csi2rx.c  | 38 ++++++++++++++++++++--
> >  2 files changed, 37 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/ti/Kconfig b/drivers/media/platform/ti/Kconfig
> > index bab998c4179aca3b07372782b9be7de340cb8d45..3bc4aa35887e6edc9fa8749d9956a67714c59001 100644
> > --- a/drivers/media/platform/ti/Kconfig
> > +++ b/drivers/media/platform/ti/Kconfig
> > @@ -67,7 +67,8 @@ config VIDEO_TI_J721E_CSI2RX
> >       tristate "TI J721E CSI2RX wrapper layer driver"
> >       depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API
> >       depends on MEDIA_SUPPORT && MEDIA_CONTROLLER
> > -     depends on (PHY_CADENCE_DPHY_RX && VIDEO_CADENCE_CSI2RX) || COMPILE_TEST
> > +     depends on VIDEO_CADENCE_CSI2RX
> > +     depends on PHY_CADENCE_DPHY_RX || COMPILE_TEST
> >       depends on ARCH_K3 || COMPILE_TEST
> >       select VIDEOBUF2_DMA_CONTIG
> >       select V4L2_FWNODE
> > diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> > index ad51d033b6725426550578bdac1bae8443458f13..425324c3d6802cfda79d116d3967b61a2e7a015a 100644
> > --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> > +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> > @@ -21,6 +21,8 @@
> >  #include <media/v4l2-mc.h>
> >  #include <media/videobuf2-dma-contig.h>
> >  
> > +#include "../../cadence/cdns-csi2rx.h"
> > +
> >  #define TI_CSI2RX_MODULE_NAME                "j721e-csi2rx"
> >  
> >  #define SHIM_CNTL                    0x10
> > @@ -29,6 +31,7 @@
> >  #define SHIM_DMACNTX                 0x20
> >  #define SHIM_DMACNTX_EN                      BIT(31)
> >  #define SHIM_DMACNTX_YUV422          GENMASK(27, 26)
> > +#define SHIM_DMACNTX_DUAL_PCK_CFG    BIT(24)
> >  #define SHIM_DMACNTX_SIZE            GENMASK(21, 20)
> >  #define SHIM_DMACNTX_FMT             GENMASK(5, 0)
> >  #define SHIM_DMACNTX_YUV422_MODE_11  3
> > @@ -40,6 +43,7 @@
> >  #define SHIM_PSI_CFG0_SRC_TAG                GENMASK(15, 0)
> >  #define SHIM_PSI_CFG0_DST_TAG                GENMASK(31, 16)
> >  
> > +#define TI_CSI2RX_MAX_PIX_PER_CLK    4
> >  #define PSIL_WORD_SIZE_BYTES         16
> >  /*
> >   * There are no hard limits on the width or height. The DMA engine can handle
> > @@ -110,6 +114,7 @@ struct ti_csi2rx_dev {
> >       struct v4l2_format              v_fmt;
> >       struct ti_csi2rx_dma            dma;
> >       u32                             sequence;
> > +     u8                              pix_per_clk;
> >  };
> >  
> >  static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = {
> > @@ -485,6 +490,26 @@ static int ti_csi2rx_notifier_register(struct ti_csi2rx_dev *csi)
> >       return 0;
> >  }
> >  
> > +/* Request maximum possible pixels per clock from the bridge */
> > +static void ti_csi2rx_request_max_ppc(struct ti_csi2rx_dev *csi)
> > +{
> > +     struct media_pad *pad;
> > +     int ret;
> > +     u8 ppc = TI_CSI2RX_MAX_PIX_PER_CLK;
> 
> Could you make this look like a reverse Christmas tree?
> 

Will do.

> > +
> > +     pad = media_entity_remote_source_pad_unique(&csi->vdev.entity);
> > +     if (!pad)
> > +             return;
> > +
> > +     ret = cdns_csi2rx_negotiate_ppc(csi->source, pad->index, &ppc);
> > +     if (ret) {
> > +             dev_warn(csi->dev, "NUM_PIXELS negotiation failed: %d\n", ret);
> > +             csi->pix_per_clk = 1;
> > +     } else {
> > +             csi->pix_per_clk = ppc;
> > +     }
> > +}
> > +
> >  static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
> >  {
> >       const struct ti_csi2rx_fmt *fmt;
> > @@ -496,6 +521,9 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
> >       reg = SHIM_CNTL_PIX_RST;
> >       writel(reg, csi->shim + SHIM_CNTL);
> >  
> > +     /* Negotiate pixel count from the source */
> > +     ti_csi2rx_request_max_ppc(csi);
> > +
> >       reg = SHIM_DMACNTX_EN;
> >       reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_dt);
> >  
> > @@ -524,14 +552,18 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
> >       case V4L2_PIX_FMT_YVYU:
> >               reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
> >                                 SHIM_DMACNTX_YUV422_MODE_11);
> > +             /* Multiple pixels are handled differently for packed YUV */
> > +             if (csi->pix_per_clk == 2)
> > +                     reg |= SHIM_DMACNTX_DUAL_PCK_CFG;
> > +             reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
> >               break;
> >       default:
> > -             /* Ignore if not YUV 4:2:2 */
> > +             /* By default we change the shift size for multiple pixels */
> > +             reg |= FIELD_PREP(SHIM_DMACNTX_SIZE,
> > +                               fmt->size + (csi->pix_per_clk >> 1));
> >               break;
> >       }
> >  
> > -     reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
> > -
> >       writel(reg, csi->shim + SHIM_DMACNTX);
> >  
> >       reg = FIELD_PREP(SHIM_PSI_CFG0_SRC_TAG, 0) |
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks,
Jai