[PATCH v2 0/2] Enable support for error detection in CSI2RX

[PATCH v2 0/2] Enable support for error detection in CSI2RX

[Yemike Abhilash Chandra]

This patch series enables the csi2rx_err_irq interrupt to record any errors
that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS
ioctl, which outputs the current device status to the kernel log.

The IRQ handler records any errors encountered during streaming.
Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve
the latest status.

Changelog:
Changes in v2:
- Address Krzysztof's review comment to remove flexibility while adding
  interrupts.
- Address Jai's review comment to drop support VIDIOC_LOG_STATUS on a
  video node.
- Address Jai's review comment to get interrupt at probe instead of
  start_stream.
- Address Jai's review comment to change dev_warn to dev_dbg when there
  is no interrupt defined in DT.

V1: https://lore.kernel.org/all/20250212131244.1397722-1-y-abhilashchandra@ti.com/

Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/fb887bda738da91ea2a24690cd7b0818
Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/18347df18041e43b78e9738263d77aa9


Yemike Abhilash Chandra (2):
  dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for
    cdns-csi2rx
  media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add
    support for VIDIOC_LOG_STATUS

 .../bindings/media/cdns,csi2rx.yaml           |  10 ++
 drivers/media/platform/cadence/cdns-csi2rx.c  | 102 +++++++++++++++++-
 2 files changed, 111 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Yemike Abhilash Chandra]

The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
Enabling these interrupts will provide additional information about a CSI
packet or an individual frame. So, add support for optional interrupts
and interrupt-names properties.

[0]: http://www.ti.com/lit/pdf/spruil1

Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---

Changes in v2:
- Address Krzysztof's review comment to remove flexibility while adding
  interrupts.

 .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
index 2008a47c0580..f335429cbde9 100644
--- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
+++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
@@ -24,6 +24,16 @@ properties:
   reg:
     maxItems: 1
 
+  interrupts:
+    minItems: 3
+    maxItems: 3
+
+  interrupt-names:
+    items:
+      - const: info
+      - const: error
+      - const: monitor
+
   clocks:
     items:
       - description: CSI2Rx system clock
-- 
2.34.1

[PATCH v2 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Yemike Abhilash Chandra]

Enable the csi2rx_err_irq interrupt to record any errors during streaming
and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
ioctl can be invoked from user space to retrieve the device status,
including details about any errors.

Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---

Changes in v2:
- Address Jai's review comment to get interrupt at probe instead of
  start_stream.
- Address Jai's review comment to change dev_warn to dev_dbg when there
  is no interrupt defined in DT.

 drivers/media/platform/cadence/cdns-csi2rx.c | 102 ++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 4d64df829e75..79f0c31eaf50 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -57,6 +57,28 @@
 #define CSI2RX_LANES_MAX	4
 #define CSI2RX_STREAMS_MAX	4
 
+#define CSI2RX_ERROR_IRQS_REG			0x28
+#define CSI2RX_ERROR_IRQS_MASK_REG		0x2C
+
+#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ	BIT(19)
+#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ	BIT(18)
+#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ	BIT(17)
+#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ	BIT(16)
+#define CSI2RX_FRONT_TRUNC_HDR_IRQ		BIT(12)
+#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ	BIT(11)
+#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ		BIT(10)
+#define CSI2RX_SP_INVALID_RCVD_IRQ		BIT(9)
+#define CSI2RX_DATA_ID_IRQ			BIT(7)
+#define CSI2RX_HEADER_CORRECTED_ECC_IRQ	BIT(6)
+#define CSI2RX_HEADER_ECC_IRQ			BIT(5)
+#define CSI2RX_PAYLOAD_CRC_IRQ			BIT(4)
+
+#define CSI2RX_ECC_ERRORS		GENMASK(7, 4)
+#define CSI2RX_PACKET_ERRORS		GENMASK(12, 9)
+#define CSI2RX_STREAM_ERRORS		GENMASK(19, 16)
+#define CSI2RX_ERRORS			(CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
+					CSI2RX_STREAM_ERRORS)
+
 enum csi2rx_pads {
 	CSI2RX_PAD_SINK,
 	CSI2RX_PAD_SOURCE_STREAM0,
@@ -71,6 +93,28 @@ struct csi2rx_fmt {
 	u8				bpp;
 };
 
+struct csi2rx_event {
+	u32 mask;
+	const char *name;
+};
+
+static const struct csi2rx_event csi2rx_events[] = {
+	{ CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
+	{ CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
+	{ CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
+	{ CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
+	{ CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
+	{ CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
+	{ CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
+	{ CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
+	{ CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
+	{ CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
+	{ CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
+	{ CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
+};
+
+#define CSI2RX_NUM_EVENTS		ARRAY_SIZE(csi2rx_events)
+
 struct csi2rx_priv {
 	struct device			*dev;
 	unsigned int			count;
@@ -95,6 +139,7 @@ struct csi2rx_priv {
 	u8				max_lanes;
 	u8				max_streams;
 	bool				has_internal_dphy;
+	u32				events[CSI2RX_NUM_EVENTS];
 
 	struct v4l2_subdev		subdev;
 	struct v4l2_async_notifier	notifier;
@@ -124,6 +169,24 @@ static const struct csi2rx_fmt formats[] = {
 	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
 };
 
+static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
+{
+	struct csi2rx_priv *csi2rx = dev_id;
+	int i;
+	u32 error_status;
+
+	error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
+
+	for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
+		if (error_status & csi2rx_events[i].mask)
+			csi2rx->events[i]++;
+
+	writel(CSI2RX_ERRORS & error_status,
+	       csi2rx->base + CSI2RX_ERROR_IRQS_REG);
+
+	return IRQ_HANDLED;
+}
+
 static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
 {
 	unsigned int i;
@@ -218,6 +281,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
 	reset_control_deassert(csi2rx->p_rst);
 	csi2rx_reset(csi2rx);
 
+	writel(CSI2RX_ERRORS, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
+
 	reg = csi2rx->num_lanes << 8;
 	for (i = 0; i < csi2rx->num_lanes; i++) {
 		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
@@ -330,6 +395,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
 	reset_control_assert(csi2rx->sys_rst);
 	clk_disable_unprepare(csi2rx->sys_clk);
 
+	writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
+
 	for (i = 0; i < csi2rx->max_streams; i++) {
 		writel(CSI2RX_STREAM_CTRL_STOP,
 		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
@@ -361,6 +428,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
 	}
 }
 
+static int csi2rx_log_status(struct v4l2_subdev *sd)
+{
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
+	unsigned int i;
+
+	for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
+		if (csi2rx->events[i])
+			dev_info(csi2rx->dev, "%s events: %d\n",
+				 csi2rx_events[i].name,
+				 csi2rx->events[i]);
+	}
+
+	return 0;
+}
+
 static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
@@ -466,7 +548,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
 	.s_stream	= csi2rx_s_stream,
 };
 
+static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
+	.log_status	= csi2rx_log_status,
+};
+
 static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
+	.core		= &csi2rx_core_ops,
 	.video		= &csi2rx_video_ops,
 	.pad		= &csi2rx_pad_ops,
 };
@@ -665,7 +752,7 @@ static int csi2rx_probe(struct platform_device *pdev)
 {
 	struct csi2rx_priv *csi2rx;
 	unsigned int i;
-	int ret;
+	int irq, ret;
 
 	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
 	if (!csi2rx)
@@ -703,6 +790,19 @@ static int csi2rx_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup;
 
+	irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
+
+	if (irq < 0) {
+		dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
+	} else {
+		ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
+					"csi2rx-irq", csi2rx);
+		if (ret) {
+			dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
+			return ret;
+		}
+	}
+
 	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
 	if (ret)
 		goto err_cleanup;
-- 
2.34.1

Re: [PATCH v2 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 8667 bytes --]

Hi Abhilash,

On Mon, Feb 17, 2025 at 06:30:13PM +0530, Yemike Abhilash Chandra wrote:
> Enable the csi2rx_err_irq interrupt to record any errors during streaming
> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
> ioctl can be invoked from user space to retrieve the device status,
> including details about any errors.
> 
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> 
> Changes in v2:
> - Address Jai's review comment to get interrupt at probe instead of
>   start_stream.
> - Address Jai's review comment to change dev_warn to dev_dbg when there
>   is no interrupt defined in DT.
> 
>  drivers/media/platform/cadence/cdns-csi2rx.c | 102 ++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 4d64df829e75..79f0c31eaf50 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -57,6 +57,28 @@
>  #define CSI2RX_LANES_MAX	4
>  #define CSI2RX_STREAMS_MAX	4
>  
> +#define CSI2RX_ERROR_IRQS_REG			0x28
> +#define CSI2RX_ERROR_IRQS_MASK_REG		0x2C
> +
> +#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ	BIT(19)
> +#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ	BIT(18)
> +#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ	BIT(17)
> +#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ	BIT(16)
> +#define CSI2RX_FRONT_TRUNC_HDR_IRQ		BIT(12)
> +#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ	BIT(11)
> +#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ		BIT(10)
> +#define CSI2RX_SP_INVALID_RCVD_IRQ		BIT(9)
> +#define CSI2RX_DATA_ID_IRQ			BIT(7)
> +#define CSI2RX_HEADER_CORRECTED_ECC_IRQ	BIT(6)
> +#define CSI2RX_HEADER_ECC_IRQ			BIT(5)
> +#define CSI2RX_PAYLOAD_CRC_IRQ			BIT(4)
> +
> +#define CSI2RX_ECC_ERRORS		GENMASK(7, 4)
> +#define CSI2RX_PACKET_ERRORS		GENMASK(12, 9)
> +#define CSI2RX_STREAM_ERRORS		GENMASK(19, 16)
> +#define CSI2RX_ERRORS			(CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
> +					CSI2RX_STREAM_ERRORS)
> +
>  enum csi2rx_pads {
>  	CSI2RX_PAD_SINK,
>  	CSI2RX_PAD_SOURCE_STREAM0,
> @@ -71,6 +93,28 @@ struct csi2rx_fmt {
>  	u8				bpp;
>  };
>  
> +struct csi2rx_event {
> +	u32 mask;
> +	const char *name;
> +};
> +
> +static const struct csi2rx_event csi2rx_events[] = {
> +	{ CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
> +	{ CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
> +	{ CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
> +	{ CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
> +	{ CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
> +	{ CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
> +	{ CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
> +	{ CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
> +	{ CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
> +	{ CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
> +	{ CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
> +	{ CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
> +};
> +
> +#define CSI2RX_NUM_EVENTS		ARRAY_SIZE(csi2rx_events)
> +
>  struct csi2rx_priv {
>  	struct device			*dev;
>  	unsigned int			count;
> @@ -95,6 +139,7 @@ struct csi2rx_priv {
>  	u8				max_lanes;
>  	u8				max_streams;
>  	bool				has_internal_dphy;
> +	u32				events[CSI2RX_NUM_EVENTS];
>  
>  	struct v4l2_subdev		subdev;
>  	struct v4l2_async_notifier	notifier;
> @@ -124,6 +169,24 @@ static const struct csi2rx_fmt formats[] = {
>  	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
>  };
>  
> +static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
> +{
> +	struct csi2rx_priv *csi2rx = dev_id;
> +	int i;
> +	u32 error_status;
> +
> +	error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> +	for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
> +		if (error_status & csi2rx_events[i].mask)
> +			csi2rx->events[i]++;
> +
> +	writel(CSI2RX_ERRORS & error_status,
> +	       csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
>  {
>  	unsigned int i;
> @@ -218,6 +281,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	reset_control_deassert(csi2rx->p_rst);
>  	csi2rx_reset(csi2rx);
>  
> +	writel(CSI2RX_ERRORS, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
> +

Because all events are masked here, I receive a flood of interrupts on my 
SK-AM62A due to Stream1 overflow events.

[  328.931479] cdns_csi2rx.30101000.csi-bridge: =================  START STATUS  =================
[  328.940213] cdns-csi2rx 30101000.csi-bridge: Overflow of the Stream 1 FIFO detected events: 108078
[  328.949179] cdns_csi2rx.30101000.csi-bridge: ==================  END STATUS  ==================

$ cat /proc/interrupts | grep csi
559:     108078          0          0          0     GICv3 175 Level     csi2rx-irq

I don't think Stream1 (pad2 of cdns-csi2rx) is connected to anything in the 
hardware, at least from what I can see in the AM62A TRM [1].

It should be possible to figure out which pads of cdns-csi2rx subdev have 
active links to other subdevs, and only enable events for the corresponding 
Stream port on the hardware.

This also exposes another issue in the csi2rx_start() function where we start 
streaming on all source pads, ignoring if it is actually linked to any subdev 
in the pipeline:

    for (i = 0; i < csi2rx->max_streams; i++) {
    ...
		writel(CSI2RX_STREAM_CTRL_START,
		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
    }

I believe that should be fixed as a separate patch, probably along with the v3 
of v4l2 streams API support [2].

[1] https://www.ti.com/lit/pdf/spruj16
[2] https://lore.kernel.org/linux-media/20240627-multistream-v2-0-6ae96c54c1c3@ti.com/

>  	reg = csi2rx->num_lanes << 8;
>  	for (i = 0; i < csi2rx->num_lanes; i++) {
>  		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
> @@ -330,6 +395,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	reset_control_assert(csi2rx->sys_rst);
>  	clk_disable_unprepare(csi2rx->sys_clk);
>  
> +	writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
> +
>  	for (i = 0; i < csi2rx->max_streams; i++) {
>  		writel(CSI2RX_STREAM_CTRL_STOP,
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> @@ -361,6 +428,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	}
>  }
>  
> +static int csi2rx_log_status(struct v4l2_subdev *sd)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +	unsigned int i;
> +
> +	for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
> +		if (csi2rx->events[i])
> +			dev_info(csi2rx->dev, "%s events: %d\n",
> +				 csi2rx_events[i].name,
> +				 csi2rx->events[i]);
> +	}
> +
> +	return 0;
> +}
> +
>  static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>  {
>  	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> @@ -466,7 +548,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
>  	.s_stream	= csi2rx_s_stream,
>  };
>  
> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
> +	.log_status	= csi2rx_log_status,
> +};
> +
>  static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> +	.core		= &csi2rx_core_ops,
>  	.video		= &csi2rx_video_ops,
>  	.pad		= &csi2rx_pad_ops,
>  };
> @@ -665,7 +752,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  {
>  	struct csi2rx_priv *csi2rx;
>  	unsigned int i;
> -	int ret;
> +	int irq, ret;
>  
>  	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
>  	if (!csi2rx)
> @@ -703,6 +790,19 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>  
> +	irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
> +

What about the other two interrupts that are required in DT?

> +	if (irq < 0) {
> +		dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
> +	} else {
> +		ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
> +					"csi2rx-irq", csi2rx);
> +		if (ret) {
> +			dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
>  	if (ret)
>  		goto err_cleanup;
> -- 
> 2.34.1
> 

Thanks,
Jai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

Hi Abhilash,

On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
> Enabling these interrupts will provide additional information about a CSI
> packet or an individual frame. So, add support for optional interrupts
> and interrupt-names properties.
> 
> [0]: http://www.ti.com/lit/pdf/spruil1
> 
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> 
> Changes in v2:
> - Address Krzysztof's review comment to remove flexibility while adding
>   interrupts.
> 
>  .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> index 2008a47c0580..f335429cbde9 100644
> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> @@ -24,6 +24,16 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  interrupts:
> +    minItems: 3
> +    maxItems: 3
> +
> +  interrupt-names:
> +    items:
> +      - const: info
> +      - const: error
> +      - const: monitor
> +

How many interrupt lines are actually exposed by the Cadence IP?

It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF 
Hardware Requests" it looks like there are three separate interrupt lines 
CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC. And 
four lines for the ASF submodule (?) that are not routed to GIC.

This does not match with the error, info and monitor line names mentioned 
here.

In my understanding which interrupt lines are actually integrated will vary 
from SoC to SoC, so please check what are the actual interrupt line names 
exposed by the Cadence IP. Maybe Maxime knows more.

But I don't think it is correct to make all 3 mandatory together, as some 
vendors may only integrate the error interrupt ignoring the rest.

CC: Changhuang Liang <changhuang.liang@starfivetech.com>

>    clocks:
>      items:
>        - description: CSI2Rx system clock
> -- 
> 2.34.1
> 

Thanks,
Jai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Krzysztof Kozlowski]

On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
> Enabling these interrupts will provide additional information about a CSI
> packet or an individual frame. So, add support for optional interrupts
> and interrupt-names properties.
> 
> [0]: http://www.ti.com/lit/pdf/spruil1
> 
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> 
> Changes in v2:
> - Address Krzysztof's review comment to remove flexibility while adding
>   interrupts.
> 
>  .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> index 2008a47c0580..f335429cbde9 100644
> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> @@ -24,6 +24,16 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  interrupts:
> +    minItems: 3

Drop, see other bindings.


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

回复: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Changhuang Liang]

Hi Jai, Abhilash

> Hi Abhilash,
> 
> On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
> > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
> > Enabling these interrupts will provide additional information about a
> > CSI packet or an individual frame. So, add support for optional
> > interrupts and interrupt-names properties.
> >
> > [0]: http://www.ti.com/lit/pdf/spruil1
> >
> > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> > ---
> >
> > Changes in v2:
> > - Address Krzysztof's review comment to remove flexibility while adding
> >   interrupts.
> >
> >  .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10
> ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > index 2008a47c0580..f335429cbde9 100644
> > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > @@ -24,6 +24,16 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > +  interrupts:
> > +    minItems: 3
> > +    maxItems: 3
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: info
> > +      - const: error
> > +      - const: monitor
> > +
> 
> How many interrupt lines are actually exposed by the Cadence IP?

I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes, 
please help correct them.

> It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF
> Hardware Requests" it looks like there are three separate interrupt lines
> CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC.
> And four lines for the ASF submodule (?) that are not routed to GIC.
> 
> This does not match with the error, info and monitor line names mentioned
> here.
> 
> In my understanding which interrupt lines are actually integrated will vary
> from SoC to SoC, so please check what are the actual interrupt line names
> exposed by the Cadence IP. Maybe Maxime knows more.
> 
> But I don't think it is correct to make all 3 mandatory together, as some
> vendors may only integrate the error interrupt ignoring the rest.

Agreed.

Best Regards,
Changhuang

Re: 回复: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

On Tue, Feb 18, 2025 at 09:35:50AM +0000, Changhuang Liang wrote:
> Hi Jai, Abhilash
> 
> > Hi Abhilash,
> > 
> > On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
> > > The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
> > > Enabling these interrupts will provide additional information about a
> > > CSI packet or an individual frame. So, add support for optional
> > > interrupts and interrupt-names properties.
> > >
> > > [0]: http://www.ti.com/lit/pdf/spruil1
> > >
> > > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Address Krzysztof's review comment to remove flexibility while adding
> > >   interrupts.
> > >
> > >  .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10
> > ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > > b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > > index 2008a47c0580..f335429cbde9 100644
> > > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > > @@ -24,6 +24,16 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > >
> > > +  interrupts:
> > > +    minItems: 3
> > > +    maxItems: 3
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: info
> > > +      - const: error
> > > +      - const: monitor
> > > +
> > 
> > How many interrupt lines are actually exposed by the Cadence IP?
> 
> I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes, 
> please help correct them.
> 

Thank you Changhuang for the quick confirmation.
This seems to match CSI_ERR_IRQ and CSI_IRQ in TI's integration.

> > It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF
> > Hardware Requests" it looks like there are three separate interrupt lines
> > CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC.
> > And four lines for the ASF submodule (?) that are not routed to GIC.
> > 

Abhilash,

What is unclear to me is where the third CSI_LEVEL interrupt line is coming 
from?

The TRM description of the CSI_LEVEL interrupt says it is for PSI_L overflow 
or VP0/VP1 frame/line mismatch, both of which are outside the Cadence CSI2RX, 
instead belonging to the TI wrapper hardware, which has its own driver.

> > This does not match with the error, info and monitor line names mentioned
> > here.
> > 
> > In my understanding which interrupt lines are actually integrated will vary
> > from SoC to SoC, so please check what are the actual interrupt line names
> > exposed by the Cadence IP. Maybe Maxime knows more.
> > 
> > But I don't think it is correct to make all 3 mandatory together, as some
> > vendors may only integrate the error interrupt ignoring the rest.
> 
> Agreed.
> 

I think by default there should only be two optional interrupt lines for 
cdns-csi2rx, "irq" and "err_irq" which is common across vendors.

If this third TI-specific CSI_LEVEL interrupt *has* to be handled by 
cdns-csi2rx driver, it would be better to create conditional bindings and 
match data in the driver such that the irq is only requested if 
"ti,j721e-csi2rx" compatible is present.

> Best Regards,
> Changhuang

Thanks,
Jai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Changhuang Liang]

> Enable the csi2rx_err_irq interrupt to record any errors during streaming
> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
> ioctl can be invoked from user space to retrieve the device status,
> including details about any errors.
>
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---

[...]

> @@ -665,7 +752,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  {
>  	struct csi2rx_priv *csi2rx;
>  	unsigned int i;
> -	int ret;
> +	int irq, ret;
>
>  	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
>  	if (!csi2rx)
> @@ -703,6 +790,19 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>
> +	irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");

Can use the "pdev" directly ?

> +	if (irq < 0) {
> +		dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
> +	} else {
> +		ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
> +					"csi2rx-irq", csi2rx);
> +		if (ret) {
> +			dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
>  	if (ret)
>  		goto err_cleanup;
> --
> 2.34.1
>

Re: 回复: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Yemike Abhilash Chandra]

Hi Jai,Changhuang,

Thanks for the reviews.

On 18/02/25 21:07, Jai Luthra wrote:
> On Tue, Feb 18, 2025 at 09:35:50AM +0000, Changhuang Liang wrote:
>> Hi Jai, Abhilash
>>
>>> Hi Abhilash,
>>>
>>> On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
>>>> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
>>>> Enabling these interrupts will provide additional information about a
>>>> CSI packet or an individual frame. So, add support for optional
>>>> interrupts and interrupt-names properties.
>>>>
>>>> [0]: http://www.ti.com/lit/pdf/spruil1
>>>>
>>>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Address Krzysztof's review comment to remove flexibility while adding
>>>>    interrupts.
>>>>
>>>>   .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10
>>> ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> index 2008a47c0580..f335429cbde9 100644
>>>> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> @@ -24,6 +24,16 @@ properties:
>>>>     reg:
>>>>       maxItems: 1
>>>>
>>>> +  interrupts:
>>>> +    minItems: 3
>>>> +    maxItems: 3
>>>> +
>>>> +  interrupt-names:
>>>> +    items:
>>>> +      - const: info
>>>> +      - const: error
>>>> +      - const: monitor
>>>> +
>>>
>>> How many interrupt lines are actually exposed by the Cadence IP?
>>
>> I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes,
>> please help correct them.
>>
> 
> Thank you Changhuang for the quick confirmation.
> This seems to match CSI_ERR_IRQ and CSI_IRQ in TI's integration.
> 
>>> It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF
>>> Hardware Requests" it looks like there are three separate interrupt lines
>>> CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC.
>>> And four lines for the ASF submodule (?) that are not routed to GIC.
>>>
> 
> Abhilash,
> 
> What is unclear to me is where the third CSI_LEVEL interrupt line is coming
> from?
> 
> The TRM description of the CSI_LEVEL interrupt says it is for PSI_L overflow
> or VP0/VP1 frame/line mismatch, both of which are outside the Cadence CSI2RX,
> instead belonging to the TI wrapper hardware, which has its own driver.

Yes, we will update the device tree bindings accordingly. We are 
planning to handle the TI-specific interrupt line separately in another 
series.

> 
>>> This does not match with the error, info and monitor line names mentioned
>>> here.
>>>
>>> In my understanding which interrupt lines are actually integrated will vary
>>> from SoC to SoC, so please check what are the actual interrupt line names
>>> exposed by the Cadence IP. Maybe Maxime knows more.
>>>
>>> But I don't think it is correct to make all 3 mandatory together, as some
>>> vendors may only integrate the error interrupt ignoring the rest.
>>
>> Agreed.
>>
> 
> I think by default there should only be two optional interrupt lines for
> cdns-csi2rx, "irq" and "err_irq" which is common across vendors.
> 
> If this third TI-specific CSI_LEVEL interrupt *has* to be handled by
> cdns-csi2rx driver, it would be better to create conditional bindings and
> match data in the driver such that the irq is only requested if
> "ti,j721e-csi2rx" compatible is present.

I will correct this in v3 by introducing bindings that allow vendors
the flexibility to use either two or three interrupt lines.

Thanks and Regards,
Yemike Abhilash Chandra

> 
>> Best Regards,
>> Changhuang
> 
> Thanks,
> Jai

Re: [PATCH v2 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Yemike Abhilash Chandra]

Hi Jai,
Thanks for the review.

On 18/02/25 13:39, Jai Luthra wrote:
> Hi Abhilash,
> 
> On Mon, Feb 17, 2025 at 06:30:13PM +0530, Yemike Abhilash Chandra wrote:
>> Enable the csi2rx_err_irq interrupt to record any errors during streaming
>> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
>> ioctl can be invoked from user space to retrieve the device status,
>> including details about any errors.
>>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>>
>> Changes in v2:
>> - Address Jai's review comment to get interrupt at probe instead of
>>    start_stream.
>> - Address Jai's review comment to change dev_warn to dev_dbg when there
>>    is no interrupt defined in DT.
>>
>>   drivers/media/platform/cadence/cdns-csi2rx.c | 102 ++++++++++++++++++-
>>   1 file changed, 101 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>> index 4d64df829e75..79f0c31eaf50 100644
>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>> @@ -57,6 +57,28 @@
>>   #define CSI2RX_LANES_MAX	4
>>   #define CSI2RX_STREAMS_MAX	4
>>   
>> +#define CSI2RX_ERROR_IRQS_REG			0x28
>> +#define CSI2RX_ERROR_IRQS_MASK_REG		0x2C
>> +
>> +#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ	BIT(19)
>> +#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ	BIT(18)
>> +#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ	BIT(17)
>> +#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ	BIT(16)
>> +#define CSI2RX_FRONT_TRUNC_HDR_IRQ		BIT(12)
>> +#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ	BIT(11)
>> +#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ		BIT(10)
>> +#define CSI2RX_SP_INVALID_RCVD_IRQ		BIT(9)
>> +#define CSI2RX_DATA_ID_IRQ			BIT(7)
>> +#define CSI2RX_HEADER_CORRECTED_ECC_IRQ	BIT(6)
>> +#define CSI2RX_HEADER_ECC_IRQ			BIT(5)
>> +#define CSI2RX_PAYLOAD_CRC_IRQ			BIT(4)
>> +
>> +#define CSI2RX_ECC_ERRORS		GENMASK(7, 4)
>> +#define CSI2RX_PACKET_ERRORS		GENMASK(12, 9)
>> +#define CSI2RX_STREAM_ERRORS		GENMASK(19, 16)
>> +#define CSI2RX_ERRORS			(CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
>> +					CSI2RX_STREAM_ERRORS)
>> +
>>   enum csi2rx_pads {
>>   	CSI2RX_PAD_SINK,
>>   	CSI2RX_PAD_SOURCE_STREAM0,
>> @@ -71,6 +93,28 @@ struct csi2rx_fmt {
>>   	u8				bpp;
>>   };
>>   
>> +struct csi2rx_event {
>> +	u32 mask;
>> +	const char *name;
>> +};
>> +
>> +static const struct csi2rx_event csi2rx_events[] = {
>> +	{ CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
>> +	{ CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
>> +	{ CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
>> +	{ CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
>> +	{ CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
>> +	{ CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
>> +	{ CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
>> +	{ CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
>> +	{ CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
>> +	{ CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
>> +	{ CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
>> +	{ CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
>> +};
>> +
>> +#define CSI2RX_NUM_EVENTS		ARRAY_SIZE(csi2rx_events)
>> +
>>   struct csi2rx_priv {
>>   	struct device			*dev;
>>   	unsigned int			count;
>> @@ -95,6 +139,7 @@ struct csi2rx_priv {
>>   	u8				max_lanes;
>>   	u8				max_streams;
>>   	bool				has_internal_dphy;
>> +	u32				events[CSI2RX_NUM_EVENTS];
>>   
>>   	struct v4l2_subdev		subdev;
>>   	struct v4l2_async_notifier	notifier;
>> @@ -124,6 +169,24 @@ static const struct csi2rx_fmt formats[] = {
>>   	{ .code	= MEDIA_BUS_FMT_BGR888_1X24,  .bpp = 24, },
>>   };
>>   
>> +static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct csi2rx_priv *csi2rx = dev_id;
>> +	int i;
>> +	u32 error_status;
>> +
>> +	error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
>> +
>> +	for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
>> +		if (error_status & csi2rx_events[i].mask)
>> +			csi2rx->events[i]++;
>> +
>> +	writel(CSI2RX_ERRORS & error_status,
>> +	       csi2rx->base + CSI2RX_ERROR_IRQS_REG);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>   static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
>>   {
>>   	unsigned int i;
>> @@ -218,6 +281,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>   	reset_control_deassert(csi2rx->p_rst);
>>   	csi2rx_reset(csi2rx);
>>   
>> +	writel(CSI2RX_ERRORS, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
>> +
> 
> Because all events are masked here, I receive a flood of interrupts on my
> SK-AM62A due to Stream1 overflow events.
> 
> [  328.931479] cdns_csi2rx.30101000.csi-bridge: =================  START STATUS  =================
> [  328.940213] cdns-csi2rx 30101000.csi-bridge: Overflow of the Stream 1 FIFO detected events: 108078
> [  328.949179] cdns_csi2rx.30101000.csi-bridge: ==================  END STATUS  ==================
> 
> $ cat /proc/interrupts | grep csi
> 559:     108078          0          0          0     GICv3 175 Level     csi2rx-irq
> 
> I don't think Stream1 (pad2 of cdns-csi2rx) is connected to anything in the
> hardware, at least from what I can see in the AM62A TRM [1].
> 
> It should be possible to figure out which pads of cdns-csi2rx subdev have
> active links to other subdevs, and only enable events for the corresponding
> Stream port on the hardware.
> 

I will fix this in v3 so that the stream overflow bits in the
mask are set only when the corresponding pad/stream is active.

Thanks and Regards,
Yemike Abhilash Chandra

> This also exposes another issue in the csi2rx_start() function where we start
> streaming on all source pads, ignoring if it is actually linked to any subdev
> in the pipeline:
> 
>      for (i = 0; i < csi2rx->max_streams; i++) {
>      ...
> 		writel(CSI2RX_STREAM_CTRL_START,
> 		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>      }
> 
> I believe that should be fixed as a separate patch, probably along with the v3
> of v4l2 streams API support [2].
> 
> [1] https://www.ti.com/lit/pdf/spruj16
> [2] https://lore.kernel.org/linux-media/20240627-multistream-v2-0-6ae96c54c1c3@ti.com/
> 
>>   	reg = csi2rx->num_lanes << 8;
>>   	for (i = 0; i < csi2rx->num_lanes; i++) {
>>   		reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
>> @@ -330,6 +395,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>>   	reset_control_assert(csi2rx->sys_rst);
>>   	clk_disable_unprepare(csi2rx->sys_clk);
>>   
>> +	writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
>> +
>>   	for (i = 0; i < csi2rx->max_streams; i++) {
>>   		writel(CSI2RX_STREAM_CTRL_STOP,
>>   		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>> @@ -361,6 +428,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>>   	}
>>   }
>>   
>> +static int csi2rx_log_status(struct v4l2_subdev *sd)
>> +{
>> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
>> +		if (csi2rx->events[i])
>> +			dev_info(csi2rx->dev, "%s events: %d\n",
>> +				 csi2rx_events[i].name,
>> +				 csi2rx->events[i]);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>>   {
>>   	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> @@ -466,7 +548,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
>>   	.s_stream	= csi2rx_s_stream,
>>   };
>>   
>> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>> +	.log_status	= csi2rx_log_status,
>> +};
>> +
>>   static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
>> +	.core		= &csi2rx_core_ops,
>>   	.video		= &csi2rx_video_ops,
>>   	.pad		= &csi2rx_pad_ops,
>>   };
>> @@ -665,7 +752,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>   {
>>   	struct csi2rx_priv *csi2rx;
>>   	unsigned int i;
>> -	int ret;
>> +	int irq, ret;
>>   
>>   	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
>>   	if (!csi2rx)
>> @@ -703,6 +790,19 @@ static int csi2rx_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_cleanup;
>>   
>> +	irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
>> +
> 
> What about the other two interrupts that are required in DT?
> 
>> +	if (irq < 0) {
>> +		dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
>> +	} else {
>> +		ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
>> +					"csi2rx-irq", csi2rx);
>> +		if (ret) {
>> +			dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
>>   	if (ret)
>>   		goto err_cleanup;
>> -- 
>> 2.34.1
>>
> 
> Thanks,
> Jai

Re: [PATCH v2 2/2] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Yemike Abhilash Chandra]

Hi Changhuang,
Thanks for the review.

On 19/02/25 08:32, Changhuang Liang wrote:
>> Enable the csi2rx_err_irq interrupt to record any errors during streaming
>> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
>> ioctl can be invoked from user space to retrieve the device status,
>> including details about any errors.
>>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
> 
> [...]
> 
>> @@ -665,7 +752,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>   {
>>   	struct csi2rx_priv *csi2rx;
>>   	unsigned int i;
>> -	int ret;
>> +	int irq, ret;
>>
>>   	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
>>   	if (!csi2rx)
>> @@ -703,6 +790,19 @@ static int csi2rx_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_cleanup;
>>
>> +	irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
> 
> Can use the "pdev" directly ?

Yes, will correct this in v3.

Thanks and Regards,
Yemike Abhilash Chandra.

> 
>> +	if (irq < 0) {
>> +		dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
>> +	} else {
>> +		ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
>> +					"csi2rx-irq", csi2rx);
>> +		if (ret) {
>> +			dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
>>   	if (ret)
>>   		goto err_cleanup;
>> --
>> 2.34.1
>>