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> 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
>
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
>>
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
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
© 2016 - 2025 Red Hat, Inc.