[PATCH v8 09/16] media: cadence: csi2rx: Soft reset the streams before starting capture

Jai Luthra posted 16 patches 2 years, 6 months ago
There is a newer version of this series
[PATCH v8 09/16] media: cadence: csi2rx: Soft reset the streams before starting capture
Posted by Jai Luthra 2 years, 6 months ago
From: Pratyush Yadav <p.yadav@ti.com>

This resets the stream state machines and FIFOs, giving them a clean
slate. On J721E if the streams are not reset before starting the
capture, the captured frame gets wrapped around vertically on every run
after the first.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
Signed-off-by: Jai Luthra <j-luthra@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v7->v8: No change

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

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 2a80c66fb547..30cdc260b46a 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -40,6 +40,7 @@
 #define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
 
 #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
 #define CSI2RX_STREAM_CTRL_START			BIT(0)
 
 #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
@@ -138,12 +139,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
 
 static void csi2rx_reset(struct csi2rx_priv *csi2rx)
 {
+	unsigned int i;
+
 	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
 	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
 
 	udelay(10);
 
 	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+	/* Reset individual streams. */
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		writel(CSI2RX_STREAM_CTRL_SOFT_RST,
+		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+		usleep_range(10, 20);
+		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+	}
 }
 
 static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)

-- 
2.41.0
Re: [PATCH v8 09/16] media: cadence: csi2rx: Soft reset the streams before starting capture
Posted by Tomi Valkeinen 2 years, 6 months ago
On 31/07/2023 11:29, Jai Luthra wrote:
> From: Pratyush Yadav <p.yadav@ti.com>
> 
> This resets the stream state machines and FIFOs, giving them a clean
> slate. On J721E if the streams are not reset before starting the
> capture, the captured frame gets wrapped around vertically on every run
> after the first.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> v7->v8: No change
> 
>   drivers/media/platform/cadence/cdns-csi2rx.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 2a80c66fb547..30cdc260b46a 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -40,6 +40,7 @@
>   #define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
>   
>   #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
> +#define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
>   #define CSI2RX_STREAM_CTRL_START			BIT(0)
>   
>   #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
> @@ -138,12 +139,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
>   
>   static void csi2rx_reset(struct csi2rx_priv *csi2rx)
>   {
> +	unsigned int i;
> +
>   	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
>   	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
>   
>   	udelay(10);
>   
>   	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> +	/* Reset individual streams. */
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		writel(CSI2RX_STREAM_CTRL_SOFT_RST,
> +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +		usleep_range(10, 20);
> +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +	}

Do you have to do it like this? Or would it be fine to set the reset bit 
for all stream regs, then sleep, then clear the reset bit from all 
stream regs? Or going even further, can you set the 
CSI2RX_SOFT_RESET_REG and all CSI2RX_STREAM_CTRL_REG regs, then sleep, 
and then clear them all?

  Tomi
Re: [PATCH v8 09/16] media: cadence: csi2rx: Soft reset the streams before starting capture
Posted by Jai Luthra 2 years, 6 months ago
On Aug 01, 2023 at 17:16:41 +0300, Tomi Valkeinen wrote:
> On 31/07/2023 11:29, Jai Luthra wrote:
> > From: Pratyush Yadav <p.yadav@ti.com>
> > 
> > This resets the stream state machines and FIFOs, giving them a clean
> > slate. On J721E if the streams are not reset before starting the
> > capture, the captured frame gets wrapped around vertically on every run
> > after the first.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > v7->v8: No change
> > 
> >   drivers/media/platform/cadence/cdns-csi2rx.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 2a80c66fb547..30cdc260b46a 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -40,6 +40,7 @@
> >   #define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
> >   #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
> > +#define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
> >   #define CSI2RX_STREAM_CTRL_START			BIT(0)
> >   #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
> > @@ -138,12 +139,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> >   static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> >   {
> > +	unsigned int i;
> > +
> >   	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> >   	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
> >   	udelay(10);
> >   	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> > +
> > +	/* Reset individual streams. */
> > +	for (i = 0; i < csi2rx->max_streams; i++) {
> > +		writel(CSI2RX_STREAM_CTRL_SOFT_RST,
> > +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +		usleep_range(10, 20);
> > +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +	}
> 
> Do you have to do it like this? Or would it be fine to set the reset bit for
> all stream regs, then sleep, then clear the reset bit from all stream regs?
> Or going even further, can you set the CSI2RX_SOFT_RESET_REG and all
> CSI2RX_STREAM_CTRL_REG regs, then sleep, and then clear them all?

You're right I think that should work, and would be much cleaner. Will 
fix.

> 
>  Tomi
> 

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145