diff mbox series

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

Message ID 20250217130013.2802293-3-y-abhilashchandra@ti.com (mailing list archive)
State New
Headers show
Series Enable support for error detection in CSI2RX | expand

Commit Message

Yemike Abhilash Chandra Feb. 17, 2025, 1 p.m. UTC 
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(-)

Comments

Jai Luthra Feb. 18, 2025, 8:09 a.m. UTC | #1 
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
Changhuang Liang Feb. 19, 2025, 3:02 a.m. UTC | #2 
> 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
>
Yemike Abhilash Chandra Feb. 21, 2025, 5:11 a.m. UTC | #3 
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
Yemike Abhilash Chandra Feb. 21, 2025, 5:13 a.m. UTC | #4 
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
>>
diff mbox series

Patch

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;