[PATCH v2 00/19] CAL fixes and improvements

[PATCH v2 00/19] CAL fixes and improvements

[Tomi Valkeinen]

Hi,

This is v2 of the series.

Rebased on top of linux-media.

I have resolved most of the comments I got for the v1, although there
were a few from Laurent where I did not agree or didn't know how to
solve the comment.

The series contains a few new patches:

"fix DMA memory corruption" which I previously had sent separately, but
it touches the same parts, so it's easier to have it as part of this
series.

"drop cal_runtime_get/put", which is just a cleanup.

"improve enable_irqs", which is also a cleanup.

 Tomi

Tomi Valkeinen (19):
  media: ti-vpe: cal: fix DMA memory corruption
  media: ti-vpe: cal: improve enable_irqs
  media: ti-vpe: cal: fix use of wrong macro
  media: ti-vpe: cal: use runtime_resume for errata handling
  media: ti-vpe: cal: drop cal_runtime_get/put
  media: ti-vpe: cal: catch error irqs and print errors
  media: ti-vpe: cal: print errors on timeouts
  media: ti-vpe: cal: simplify irq handling
  media: ti-vpe: cal: remove useless CAL_GEN_* macros
  media: ti-vpe: cal: remove useless IRQ defines
  media: ti-vpe: cal: use reg_write_field
  media: ti-vpe: cal: cleanup CIO power enable/disable
  media: ti-vpe: cal: fix dummy read to phy
  media: ti-vpe: cal: program number of lines properly
  media: ti-vpe: cal: set DMA max seg size
  media: ti-vpe: cal: move code to separate functions
  media: ti-vpe: cal: improve wait for CIO resetdone
  media: ti-vpe: cal: improve wait for stop-state
  media: ti-vpe: cal: fix stop state timeout

 drivers/media/platform/ti-vpe/cal.c      | 398 ++++++++++++++---------
 drivers/media/platform/ti-vpe/cal_regs.h |  21 +-
 2 files changed, 245 insertions(+), 174 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH v2 01/19] media: ti-vpe: cal: fix DMA memory corruption

[Tomi Valkeinen]

When the CAL driver stops streaming, it will shut everything down
without waiting for the current frame to finish. This leaves the CAL DMA
in a slightly undefined state, and when CAL DMA is enabled when the
stream is started the next time, the old DMA transfer will continue.

It is not clear if the old DMA transfer continues with the exact
settings of the original transfer, or is it a mix of old and new
settings, but in any case the end result is memory corruption as the
destination memory address is no longer valid.

I could not find any way to ensure that any old DMA transfer would be
discarded, except perhaps full CAL reset. But we cannot do a full reset
when one port is getting enabled, as that would reset both ports.

This patch tries to make sure that the DMA transfer is finished properly
when the stream is being stopped. I say "tries", as, as mentioned above,
I don't see a way to force the DMA transfer to finish. I believe this
fixes the corruptions for normal cases, but if for some reason the DMA
of the final frame would stall a lot, resulting in timeout in the code
waiting for the DMA to finish, we'll again end up with unfinished DMA
transfer. However, I don't know what could cause such a timeout.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: stable@vger.kernel.org
---
 drivers/media/platform/ti-vpe/cal.c | 32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 6c8f3702eac0..9dd6de14189b 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -412,6 +412,8 @@ struct cal_ctx {
 	struct cal_buffer	*cur_frm;
 	/* Pointer pointing to next v4l2_buffer */
 	struct cal_buffer	*next_frm;
+
+	bool dma_act;
 };
 
 static const struct cal_fmt *find_format_by_pix(struct cal_ctx *ctx,
@@ -942,6 +944,7 @@ static void csi2_lane_config(struct cal_ctx *ctx)
 
 static void csi2_ppi_enable(struct cal_ctx *ctx)
 {
+	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
 	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
 			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
 }
@@ -1204,15 +1207,25 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 		if (isportirqset(irqst2, 1)) {
 			ctx = dev->ctx[0];
 
+			spin_lock(&ctx->slock);
+			ctx->dma_act = false;
+
 			if (ctx->cur_frm != ctx->next_frm)
 				cal_process_buffer_complete(ctx);
+
+			spin_unlock(&ctx->slock);
 		}
 
 		if (isportirqset(irqst2, 2)) {
 			ctx = dev->ctx[1];
 
+			spin_lock(&ctx->slock);
+			ctx->dma_act = false;
+
 			if (ctx->cur_frm != ctx->next_frm)
 				cal_process_buffer_complete(ctx);
+
+			spin_unlock(&ctx->slock);
 		}
 	}
 
@@ -1228,6 +1241,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 			dma_q = &ctx->vidq;
 
 			spin_lock(&ctx->slock);
+			ctx->dma_act = true;
 			if (!list_empty(&dma_q->active) &&
 			    ctx->cur_frm == ctx->next_frm)
 				cal_schedule_next_buffer(ctx);
@@ -1239,6 +1253,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 			dma_q = &ctx->vidq;
 
 			spin_lock(&ctx->slock);
+			ctx->dma_act = true;
 			if (!list_empty(&dma_q->active) &&
 			    ctx->cur_frm == ctx->next_frm)
 				cal_schedule_next_buffer(ctx);
@@ -1711,10 +1726,27 @@ static void cal_stop_streaming(struct vb2_queue *vq)
 	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
 	struct cal_dmaqueue *dma_q = &ctx->vidq;
 	struct cal_buffer *buf, *tmp;
+	unsigned long timeout;
 	unsigned long flags;
 	int ret;
+	bool dma_act;
 
 	csi2_ppi_disable(ctx);
+
+	/* wait for stream and dma to finish */
+	dma_act = true;
+	timeout = jiffies + msecs_to_jiffies(500);
+	while (dma_act && time_before(jiffies, timeout)) {
+		msleep(50);
+
+		spin_lock_irqsave(&ctx->slock, flags);
+		dma_act = ctx->dma_act;
+		spin_unlock_irqrestore(&ctx->slock, flags);
+	}
+
+	if (dma_act)
+		ctx_err(ctx, "failed to disable dma cleanly\n");
+
 	disable_irqs(ctx);
 	csi2_phy_deinit(ctx);
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 01/19] media: ti-vpe: cal: fix DMA memory corruption

[Benoit Parrot]

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> When the CAL driver stops streaming, it will shut everything down
> without waiting for the current frame to finish. This leaves the CAL DMA
> in a slightly undefined state, and when CAL DMA is enabled when the
> stream is started the next time, the old DMA transfer will continue.
> 
> It is not clear if the old DMA transfer continues with the exact
> settings of the original transfer, or is it a mix of old and new
> settings, but in any case the end result is memory corruption as the
> destination memory address is no longer valid.
> 
> I could not find any way to ensure that any old DMA transfer would be
> discarded, except perhaps full CAL reset. But we cannot do a full reset
> when one port is getting enabled, as that would reset both ports.
> 
> This patch tries to make sure that the DMA transfer is finished properly
> when the stream is being stopped. I say "tries", as, as mentioned above,
> I don't see a way to force the DMA transfer to finish. I believe this
> fixes the corruptions for normal cases, but if for some reason the DMA
> of the final frame would stall a lot, resulting in timeout in the code
> waiting for the DMA to finish, we'll again end up with unfinished DMA
> transfer. However, I don't know what could cause such a timeout.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/platform/ti-vpe/cal.c | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 6c8f3702eac0..9dd6de14189b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -412,6 +412,8 @@ struct cal_ctx {
>  	struct cal_buffer	*cur_frm;
>  	/* Pointer pointing to next v4l2_buffer */
>  	struct cal_buffer	*next_frm;
> +
> +	bool dma_act;
>  };
>  
>  static const struct cal_fmt *find_format_by_pix(struct cal_ctx *ctx,
> @@ -942,6 +944,7 @@ static void csi2_lane_config(struct cal_ctx *ctx)
>  
>  static void csi2_ppi_enable(struct cal_ctx *ctx)
>  {
> +	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
>  	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
>  			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>  }
> @@ -1204,15 +1207,25 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  		if (isportirqset(irqst2, 1)) {
>  			ctx = dev->ctx[0];
>  
> +			spin_lock(&ctx->slock);
> +			ctx->dma_act = false;
> +
>  			if (ctx->cur_frm != ctx->next_frm)
>  				cal_process_buffer_complete(ctx);
> +
> +			spin_unlock(&ctx->slock);
>  		}
>  

This totally wrong.

>  		if (isportirqset(irqst2, 2)) {
>  			ctx = dev->ctx[1];
>  
> +			spin_lock(&ctx->slock);
> +			ctx->dma_act = false;
> +
>  			if (ctx->cur_frm != ctx->next_frm)
>  				cal_process_buffer_complete(ctx);
> +
> +			spin_unlock(&ctx->slock);
>  		}
>  	}
>  
> @@ -1228,6 +1241,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  			dma_q = &ctx->vidq;
>  
>  			spin_lock(&ctx->slock);
> +			ctx->dma_act = true;
>  			if (!list_empty(&dma_q->active) &&
>  			    ctx->cur_frm == ctx->next_frm)
>  				cal_schedule_next_buffer(ctx);
> @@ -1239,6 +1253,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  			dma_q = &ctx->vidq;
>  
>  			spin_lock(&ctx->slock);
> +			ctx->dma_act = true;
>  			if (!list_empty(&dma_q->active) &&
>  			    ctx->cur_frm == ctx->next_frm)
>  				cal_schedule_next_buffer(ctx);
> @@ -1711,10 +1726,27 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>  	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
>  	struct cal_dmaqueue *dma_q = &ctx->vidq;
>  	struct cal_buffer *buf, *tmp;
> +	unsigned long timeout;
>  	unsigned long flags;
>  	int ret;
> +	bool dma_act;
>  
>  	csi2_ppi_disable(ctx);
> +
> +	/* wait for stream and dma to finish */
> +	dma_act = true;
> +	timeout = jiffies + msecs_to_jiffies(500);
> +	while (dma_act && time_before(jiffies, timeout)) {
> +		msleep(50);
> +
> +		spin_lock_irqsave(&ctx->slock, flags);
> +		dma_act = ctx->dma_act;
> +		spin_unlock_irqrestore(&ctx->slock, flags);
> +	}
> +
> +	if (dma_act)
> +		ctx_err(ctx, "failed to disable dma cleanly\n");
> +
>  	disable_irqs(ctx);
>  	csi2_phy_deinit(ctx);
>  
> 

Benoit

Re: [PATCH v2 01/19] media: ti-vpe: cal: fix DMA memory corruption

[Benoit Parrot]

All,

Please ignore my previous email, it was an email client test.

Sorry for the noise.

Benoit

On 3/19/20 4:41 PM, Benoit Parrot wrote:
> Thanks for the patch.
> 
> On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
>> When the CAL driver stops streaming, it will shut everything down
>> without waiting for the current frame to finish. This leaves the CAL DMA
>> in a slightly undefined state, and when CAL DMA is enabled when the
>> stream is started the next time, the old DMA transfer will continue.
>>
>> It is not clear if the old DMA transfer continues with the exact
>> settings of the original transfer, or is it a mix of old and new
>> settings, but in any case the end result is memory corruption as the
>> destination memory address is no longer valid.
>>
>> I could not find any way to ensure that any old DMA transfer would be
>> discarded, except perhaps full CAL reset. But we cannot do a full reset
>> when one port is getting enabled, as that would reset both ports.
>>
>> This patch tries to make sure that the DMA transfer is finished properly
>> when the stream is being stopped. I say "tries", as, as mentioned above,
>> I don't see a way to force the DMA transfer to finish. I believe this
>> fixes the corruptions for normal cases, but if for some reason the DMA
>> of the final frame would stall a lot, resulting in timeout in the code
>> waiting for the DMA to finish, we'll again end up with unfinished DMA
>> transfer. However, I don't know what could cause such a timeout.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/media/platform/ti-vpe/cal.c | 32 +++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index 6c8f3702eac0..9dd6de14189b 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -412,6 +412,8 @@ struct cal_ctx {
>>  	struct cal_buffer	*cur_frm;
>>  	/* Pointer pointing to next v4l2_buffer */
>>  	struct cal_buffer	*next_frm;
>> +
>> +	bool dma_act;
>>  };
>>  
>>  static const struct cal_fmt *find_format_by_pix(struct cal_ctx *ctx,
>> @@ -942,6 +944,7 @@ static void csi2_lane_config(struct cal_ctx *ctx)
>>  
>>  static void csi2_ppi_enable(struct cal_ctx *ctx)
>>  {
>> +	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
>>  	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
>>  			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>>  }
>> @@ -1204,15 +1207,25 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>>  		if (isportirqset(irqst2, 1)) {
>>  			ctx = dev->ctx[0];
>>  
>> +			spin_lock(&ctx->slock);
>> +			ctx->dma_act = false;
>> +
>>  			if (ctx->cur_frm != ctx->next_frm)
>>  				cal_process_buffer_complete(ctx);
>> +
>> +			spin_unlock(&ctx->slock);
>>  		}
>>  
> 
> This totally wrong.
> 
>>  		if (isportirqset(irqst2, 2)) {
>>  			ctx = dev->ctx[1];
>>  
>> +			spin_lock(&ctx->slock);
>> +			ctx->dma_act = false;
>> +
>>  			if (ctx->cur_frm != ctx->next_frm)
>>  				cal_process_buffer_complete(ctx);
>> +
>> +			spin_unlock(&ctx->slock);
>>  		}
>>  	}
>>  
>> @@ -1228,6 +1241,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>>  			dma_q = &ctx->vidq;
>>  
>>  			spin_lock(&ctx->slock);
>> +			ctx->dma_act = true;
>>  			if (!list_empty(&dma_q->active) &&
>>  			    ctx->cur_frm == ctx->next_frm)
>>  				cal_schedule_next_buffer(ctx);
>> @@ -1239,6 +1253,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>>  			dma_q = &ctx->vidq;
>>  
>>  			spin_lock(&ctx->slock);
>> +			ctx->dma_act = true;
>>  			if (!list_empty(&dma_q->active) &&
>>  			    ctx->cur_frm == ctx->next_frm)
>>  				cal_schedule_next_buffer(ctx);
>> @@ -1711,10 +1726,27 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>>  	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
>>  	struct cal_dmaqueue *dma_q = &ctx->vidq;
>>  	struct cal_buffer *buf, *tmp;
>> +	unsigned long timeout;
>>  	unsigned long flags;
>>  	int ret;
>> +	bool dma_act;
>>  
>>  	csi2_ppi_disable(ctx);
>> +
>> +	/* wait for stream and dma to finish */
>> +	dma_act = true;
>> +	timeout = jiffies + msecs_to_jiffies(500);
>> +	while (dma_act && time_before(jiffies, timeout)) {
>> +		msleep(50);
>> +
>> +		spin_lock_irqsave(&ctx->slock, flags);
>> +		dma_act = ctx->dma_act;
>> +		spin_unlock_irqrestore(&ctx->slock, flags);
>> +	}
>> +
>> +	if (dma_act)
>> +		ctx_err(ctx, "failed to disable dma cleanly\n");
>> +
>>  	disable_irqs(ctx);
>>  	csi2_phy_deinit(ctx);
>>  
>>
> 
> Benoit
> 

Re: [PATCH v2 01/19] media: ti-vpe: cal: fix DMA memory corruption

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> When the CAL driver stops streaming, it will shut everything down
> without waiting for the current frame to finish. This leaves the CAL DMA
> in a slightly undefined state, and when CAL DMA is enabled when the
> stream is started the next time, the old DMA transfer will continue.
> 
> It is not clear if the old DMA transfer continues with the exact
> settings of the original transfer, or is it a mix of old and new
> settings, but in any case the end result is memory corruption as the
> destination memory address is no longer valid.
> 
> I could not find any way to ensure that any old DMA transfer would be
> discarded, except perhaps full CAL reset. But we cannot do a full reset
> when one port is getting enabled, as that would reset both ports.
> 
> This patch tries to make sure that the DMA transfer is finished properly
> when the stream is being stopped. I say "tries", as, as mentioned above,
> I don't see a way to force the DMA transfer to finish. I believe this
> fixes the corruptions for normal cases, but if for some reason the DMA
> of the final frame would stall a lot, resulting in timeout in the code
> waiting for the DMA to finish, we'll again end up with unfinished DMA
> transfer. However, I don't know what could cause such a timeout.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 6c8f3702eac0..9dd6de14189b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -412,6 +412,8 @@ struct cal_ctx {
>  	struct cal_buffer	*cur_frm;
>  	/* Pointer pointing to next v4l2_buffer */
>  	struct cal_buffer	*next_frm;
> +
> +	bool dma_act;
>  };
>  
>  static const struct cal_fmt *find_format_by_pix(struct cal_ctx *ctx,
> @@ -942,6 +944,7 @@ static void csi2_lane_config(struct cal_ctx *ctx)
>  
>  static void csi2_ppi_enable(struct cal_ctx *ctx)
>  {
> +	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
>  	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
>  			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>  }
> @@ -1204,15 +1207,25 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  		if (isportirqset(irqst2, 1)) {
>  			ctx = dev->ctx[0];
>  
> +			spin_lock(&ctx->slock);
> +			ctx->dma_act = false;
> +
>  			if (ctx->cur_frm != ctx->next_frm)
>  				cal_process_buffer_complete(ctx);
> +
> +			spin_unlock(&ctx->slock);
>  		}
>  
>  		if (isportirqset(irqst2, 2)) {
>  			ctx = dev->ctx[1];
>  
> +			spin_lock(&ctx->slock);
> +			ctx->dma_act = false;
> +
>  			if (ctx->cur_frm != ctx->next_frm)
>  				cal_process_buffer_complete(ctx);
> +
> +			spin_unlock(&ctx->slock);
>  		}
>  	}
>  
> @@ -1228,6 +1241,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  			dma_q = &ctx->vidq;
>  
>  			spin_lock(&ctx->slock);
> +			ctx->dma_act = true;
>  			if (!list_empty(&dma_q->active) &&
>  			    ctx->cur_frm == ctx->next_frm)
>  				cal_schedule_next_buffer(ctx);
> @@ -1239,6 +1253,7 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  			dma_q = &ctx->vidq;
>  
>  			spin_lock(&ctx->slock);
> +			ctx->dma_act = true;
>  			if (!list_empty(&dma_q->active) &&
>  			    ctx->cur_frm == ctx->next_frm)
>  				cal_schedule_next_buffer(ctx);
> @@ -1711,10 +1726,27 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>  	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
>  	struct cal_dmaqueue *dma_q = &ctx->vidq;
>  	struct cal_buffer *buf, *tmp;
> +	unsigned long timeout;
>  	unsigned long flags;
>  	int ret;
> +	bool dma_act;
>  
>  	csi2_ppi_disable(ctx);
> +
> +	/* wait for stream and dma to finish */
> +	dma_act = true;
> +	timeout = jiffies + msecs_to_jiffies(500);
> +	while (dma_act && time_before(jiffies, timeout)) {
> +		msleep(50);
> +
> +		spin_lock_irqsave(&ctx->slock, flags);
> +		dma_act = ctx->dma_act;
> +		spin_unlock_irqrestore(&ctx->slock, flags);
> +	}
> +
> +	if (dma_act)
> +		ctx_err(ctx, "failed to disable dma cleanly\n");
> +
>  	disable_irqs(ctx);
>  	csi2_phy_deinit(ctx);
>  
> 

[PATCH v2 02/19] media: ti-vpe: cal: improve enable_irqs

[Tomi Valkeinen]

IRQENABLE_SET registers are (usually) not meant to be read, only written
to. The current driver needlessly uses read-modify-write cycle to enable
IRQ bits.

The read-modify-write has no bad side effects here, but it's still
better to clean this up by only using write.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 9dd6de14189b..76d55c76d938 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -706,16 +706,16 @@ static void cal_quickdump_regs(struct cal_dev *dev)
  */
 static void enable_irqs(struct cal_ctx *ctx)
 {
+	u32 val;
+
 	/* Enable IRQ_WDMA_END 0/1 */
-	reg_write_field(ctx->dev,
-			CAL_HL_IRQENABLE_SET(2),
-			CAL_HL_IRQ_ENABLE,
-			CAL_HL_IRQ_MASK(ctx->csi2_port));
+	val = 0;
+	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), val);
 	/* Enable IRQ_WDMA_START 0/1 */
-	reg_write_field(ctx->dev,
-			CAL_HL_IRQENABLE_SET(3),
-			CAL_HL_IRQ_ENABLE,
-			CAL_HL_IRQ_MASK(ctx->csi2_port));
+	val = 0;
+	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), val);
 	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
 	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 02/19] media: ti-vpe: cal: improve enable_irqs

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> IRQENABLE_SET registers are (usually) not meant to be read, only written
> to. The current driver needlessly uses read-modify-write cycle to enable
> IRQ bits.
> 
> The read-modify-write has no bad side effects here, but it's still
> better to clean this up by only using write.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 9dd6de14189b..76d55c76d938 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -706,16 +706,16 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>   */
>  static void enable_irqs(struct cal_ctx *ctx)
>  {
> +	u32 val;
> +
>  	/* Enable IRQ_WDMA_END 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_SET(2),
> -			CAL_HL_IRQ_ENABLE,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	val = 0;
> +	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), val);
>  	/* Enable IRQ_WDMA_START 0/1 */
> -	reg_write_field(ctx->dev,
> -			CAL_HL_IRQENABLE_SET(3),
> -			CAL_HL_IRQ_ENABLE,
> -			CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	val = 0;
> +	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), val);
>  	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);
>  }
> 

[PATCH v2 03/19] media: ti-vpe: cal: fix use of wrong macro

[Tomi Valkeinen]

i913_errata() sets a bit to 1 in PHY_REG10, but for some reason uses
CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE for the bit value. The value of
that macro is 1, so it works, but is still wrong.

Fix this to 1.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 76d55c76d938..c418296df0f8 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -645,8 +645,7 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
 {
 	u32 reg10 = reg_read(dev->cc[port], CAL_CSI2_PHY_REG10);
 
-	set_field(&reg10, CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE,
-		  CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK);
+	set_field(&reg10, 1, CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK);
 
 	cal_dbg(1, dev, "CSI2_%d_REG10 = 0x%08x\n", port, reg10);
 	reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 03/19] media: ti-vpe: cal: fix use of wrong macro

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> i913_errata() sets a bit to 1 in PHY_REG10, but for some reason uses
> CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE for the bit value. The value of
> that macro is 1, so it works, but is still wrong.
> 
> Fix this to 1.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 76d55c76d938..c418296df0f8 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -645,8 +645,7 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
>  {
>  	u32 reg10 = reg_read(dev->cc[port], CAL_CSI2_PHY_REG10);
>  
> -	set_field(&reg10, CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE,
> -		  CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK);
> +	set_field(&reg10, 1, CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK);
>  
>  	cal_dbg(1, dev, "CSI2_%d_REG10 = 0x%08x\n", port, reg10);
>  	reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10);
> 

[PATCH v2 04/19] media: ti-vpe: cal: use runtime_resume for errata handling

[Tomi Valkeinen]

We need to do errata handling every time CAL is being enabled. The code
is currently in cal_runtime_get(), which is not the correct place for
it.

Move the code to cal_runtime_resume, which is called every time CAL is
enabled.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 36 ++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index c418296df0f8..4fe37f284b54 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -653,20 +653,7 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
 
 static int cal_runtime_get(struct cal_dev *dev)
 {
-	int r;
-
-	r = pm_runtime_get_sync(&dev->pdev->dev);
-
-	if (dev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) {
-		/*
-		 * Apply errata on both port eveytime we (re-)enable
-		 * the clock
-		 */
-		i913_errata(dev, 0);
-		i913_errata(dev, 1);
-	}
-
-	return r;
+	return pm_runtime_get_sync(&dev->pdev->dev);
 }
 
 static inline void cal_runtime_put(struct cal_dev *dev)
@@ -2409,11 +2396,32 @@ static const struct of_device_id cal_of_match[] = {
 MODULE_DEVICE_TABLE(of, cal_of_match);
 #endif
 
+static int cal_runtime_resume(struct device *dev)
+{
+	struct cal_dev *caldev = dev_get_drvdata(dev);
+
+	if (caldev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) {
+		/*
+		 * Apply errata on both port everytime we (re-)enable
+		 * the clock
+		 */
+		i913_errata(caldev, 0);
+		i913_errata(caldev, 1);
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops cal_pm_ops = {
+	.runtime_resume = cal_runtime_resume,
+};
+
 static struct platform_driver cal_pdrv = {
 	.probe		= cal_probe,
 	.remove		= cal_remove,
 	.driver		= {
 		.name	= CAL_MODULE_NAME,
+		.pm	= &cal_pm_ops,
 		.of_match_table = of_match_ptr(cal_of_match),
 	},
 };
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 04/19] media: ti-vpe: cal: use runtime_resume for errata handling

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> We need to do errata handling every time CAL is being enabled. The code
> is currently in cal_runtime_get(), which is not the correct place for
> it.
> 
> Move the code to cal_runtime_resume, which is called every time CAL is
> enabled.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 36 ++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index c418296df0f8..4fe37f284b54 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -653,20 +653,7 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
>  
>  static int cal_runtime_get(struct cal_dev *dev)
>  {
> -	int r;
> -
> -	r = pm_runtime_get_sync(&dev->pdev->dev);
> -
> -	if (dev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) {
> -		/*
> -		 * Apply errata on both port eveytime we (re-)enable
> -		 * the clock
> -		 */
> -		i913_errata(dev, 0);
> -		i913_errata(dev, 1);
> -	}
> -
> -	return r;
> +	return pm_runtime_get_sync(&dev->pdev->dev);
>  }
>  
>  static inline void cal_runtime_put(struct cal_dev *dev)
> @@ -2409,11 +2396,32 @@ static const struct of_device_id cal_of_match[] = {
>  MODULE_DEVICE_TABLE(of, cal_of_match);
>  #endif
>  
> +static int cal_runtime_resume(struct device *dev)
> +{
> +	struct cal_dev *caldev = dev_get_drvdata(dev);
> +
> +	if (caldev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) {
> +		/*
> +		 * Apply errata on both port everytime we (re-)enable
> +		 * the clock
> +		 */
> +		i913_errata(caldev, 0);
> +		i913_errata(caldev, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cal_pm_ops = {
> +	.runtime_resume = cal_runtime_resume,
> +};
> +
>  static struct platform_driver cal_pdrv = {
>  	.probe		= cal_probe,
>  	.remove		= cal_remove,
>  	.driver		= {
>  		.name	= CAL_MODULE_NAME,
> +		.pm	= &cal_pm_ops,
>  		.of_match_table = of_match_ptr(cal_of_match),
>  	},
>  };
> 

[PATCH v2 05/19] media: ti-vpe: cal: drop cal_runtime_get/put

[Tomi Valkeinen]

Now that cal_runtime_get and cal_runtime_put are only direct wrappers to
pm_runtime_get/put, we can drop cal_runtime_get and cal_runtime_put.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 4fe37f284b54..4f9dee3474ba 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -651,16 +651,6 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
 	reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10);
 }
 
-static int cal_runtime_get(struct cal_dev *dev)
-{
-	return pm_runtime_get_sync(&dev->pdev->dev);
-}
-
-static inline void cal_runtime_put(struct cal_dev *dev)
-{
-	pm_runtime_put_sync(&dev->pdev->dev);
-}
-
 static void cal_quickdump_regs(struct cal_dev *dev)
 {
 	cal_info(dev, "CAL Registers @ 0x%pa:\n", &dev->res->start);
@@ -1666,7 +1656,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err;
 	}
 
-	cal_runtime_get(ctx->dev);
+	pm_runtime_get_sync(&ctx->dev->pdev->dev);
 
 	csi2_ctx_config(ctx);
 	pix_proc_config(ctx);
@@ -1681,7 +1671,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret) {
 		v4l2_subdev_call(ctx->sensor, core, s_power, 0);
 		ctx_err(ctx, "stream on failed in subdev\n");
-		cal_runtime_put(ctx->dev);
+		pm_runtime_put_sync(&ctx->dev->pdev->dev);
 		goto err;
 	}
 
@@ -1761,7 +1751,7 @@ static void cal_stop_streaming(struct vb2_queue *vq)
 	ctx->next_frm = NULL;
 	spin_unlock_irqrestore(&ctx->slock, flags);
 
-	cal_runtime_put(ctx->dev);
+	pm_runtime_put_sync(&ctx->dev->pdev->dev);
 }
 
 static const struct vb2_ops cal_video_qops = {
@@ -2316,14 +2306,14 @@ static int cal_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
-	ret = cal_runtime_get(dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret)
 		goto runtime_disable;
 
 	/* Just check we can actually access the module */
 	cal_get_hwinfo(dev);
 
-	cal_runtime_put(dev);
+	pm_runtime_put_sync(&pdev->dev);
 
 	return 0;
 
@@ -2351,7 +2341,7 @@ static int cal_remove(struct platform_device *pdev)
 
 	cal_dbg(1, dev, "Removing %s\n", CAL_MODULE_NAME);
 
-	cal_runtime_get(dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	for (i = 0; i < CAL_NUM_CONTEXT; i++) {
 		ctx = dev->ctx[i];
@@ -2367,7 +2357,7 @@ static int cal_remove(struct platform_device *pdev)
 		}
 	}
 
-	cal_runtime_put(dev);
+	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
 	return 0;
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 05/19] media: ti-vpe: cal: drop cal_runtime_get/put

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Now that cal_runtime_get and cal_runtime_put are only direct wrappers to
> pm_runtime_get/put, we can drop cal_runtime_get and cal_runtime_put.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 4fe37f284b54..4f9dee3474ba 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -651,16 +651,6 @@ static void i913_errata(struct cal_dev *dev, unsigned int port)
>  	reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10);
>  }
>  
> -static int cal_runtime_get(struct cal_dev *dev)
> -{
> -	return pm_runtime_get_sync(&dev->pdev->dev);
> -}
> -
> -static inline void cal_runtime_put(struct cal_dev *dev)
> -{
> -	pm_runtime_put_sync(&dev->pdev->dev);
> -}
> -
>  static void cal_quickdump_regs(struct cal_dev *dev)
>  {
>  	cal_info(dev, "CAL Registers @ 0x%pa:\n", &dev->res->start);
> @@ -1666,7 +1656,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		goto err;
>  	}
>  
> -	cal_runtime_get(ctx->dev);
> +	pm_runtime_get_sync(&ctx->dev->pdev->dev);
>  
>  	csi2_ctx_config(ctx);
>  	pix_proc_config(ctx);
> @@ -1681,7 +1671,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret) {
>  		v4l2_subdev_call(ctx->sensor, core, s_power, 0);
>  		ctx_err(ctx, "stream on failed in subdev\n");
> -		cal_runtime_put(ctx->dev);
> +		pm_runtime_put_sync(&ctx->dev->pdev->dev);
>  		goto err;
>  	}
>  
> @@ -1761,7 +1751,7 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>  	ctx->next_frm = NULL;
>  	spin_unlock_irqrestore(&ctx->slock, flags);
>  
> -	cal_runtime_put(ctx->dev);
> +	pm_runtime_put_sync(&ctx->dev->pdev->dev);
>  }
>  
>  static const struct vb2_ops cal_video_qops = {
> @@ -2316,14 +2306,14 @@ static int cal_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> -	ret = cal_runtime_get(dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  	if (ret)
>  		goto runtime_disable;
>  
>  	/* Just check we can actually access the module */
>  	cal_get_hwinfo(dev);
>  
> -	cal_runtime_put(dev);
> +	pm_runtime_put_sync(&pdev->dev);
>  
>  	return 0;
>  
> @@ -2351,7 +2341,7 @@ static int cal_remove(struct platform_device *pdev)
>  
>  	cal_dbg(1, dev, "Removing %s\n", CAL_MODULE_NAME);
>  
> -	cal_runtime_get(dev);
> +	pm_runtime_get_sync(&pdev->dev);
>  
>  	for (i = 0; i < CAL_NUM_CONTEXT; i++) {
>  		ctx = dev->ctx[i];
> @@ -2367,7 +2357,7 @@ static int cal_remove(struct platform_device *pdev)
>  		}
>  	}
>  
> -	cal_runtime_put(dev);
> +	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
>  	return 0;
> 

Reviewed-by: Benoit Parrot <bparrot@ti.com>

[PATCH v2 06/19] media: ti-vpe: cal: catch error irqs and print errors

[Tomi Valkeinen]

CAL reports various errors via IRQs, which are not handled at all by the
current driver. Add code to enable and catch those IRQs and print
errors. This will make it much easier to notice and debug issues with
sensors.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
 drivers/media/platform/ti-vpe/cal_regs.h |  6 ++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 4f9dee3474ba..838215a3f230 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -684,6 +684,21 @@ static void enable_irqs(struct cal_ctx *ctx)
 {
 	u32 val;
 
+	const u32 cio_err_mask =
+		CAL_CSI2_COMPLEXIO_IRQ_LANE_ERRORS_MASK |
+		CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK |
+		CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK |
+		CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK;
+
+	/* Enable CIO error irqs */
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1),
+		  CAL_HL_IRQ_CIO_MASK(ctx->csi2_port));
+	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_IRQENABLE(ctx->csi2_port),
+		  cio_err_mask);
+
+	/* Always enable OCPO error */
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), CAL_HL_IRQ_OCPO_ERR_MASK);
+
 	/* Enable IRQ_WDMA_END 0/1 */
 	val = 0;
 	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
@@ -700,6 +715,12 @@ static void disable_irqs(struct cal_ctx *ctx)
 {
 	u32 val;
 
+	/* Disable CIO error irqs */
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(1),
+		  CAL_HL_IRQ_CIO_MASK(ctx->csi2_port));
+	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_IRQENABLE(ctx->csi2_port),
+		  0);
+
 	/* Disable IRQ_WDMA_END 0/1 */
 	val = 0;
 	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
@@ -1171,7 +1192,30 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 	struct cal_dev *dev = (struct cal_dev *)data;
 	struct cal_ctx *ctx;
 	struct cal_dmaqueue *dma_q;
-	u32 irqst2, irqst3;
+	u32 irqst1, irqst2, irqst3;
+
+	irqst1 = reg_read(dev, CAL_HL_IRQSTATUS(1));
+	if (irqst1) {
+		int i;
+
+		reg_write(dev, CAL_HL_IRQSTATUS(1), irqst1);
+
+		if (irqst1 & CAL_HL_IRQ_OCPO_ERR_MASK)
+			dev_err_ratelimited(&dev->pdev->dev, "OCPO ERROR\n");
+
+		for (i = 1; i <= 2; ++i) {
+			if (irqst1 & CAL_HL_IRQ_CIO_MASK(i)) {
+				u32 cio_stat = reg_read(dev,
+							CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
+
+				dev_err_ratelimited(&dev->pdev->dev,
+						    "CIO%d error: %#08x\n", i, cio_stat);
+
+				reg_write(dev, CAL_CSI2_COMPLEXIO_IRQSTATUS(i),
+					  cio_stat);
+			}
+		}
+	}
 
 	/* Check which DMA just finished */
 	irqst2 = reg_read(dev, CAL_HL_IRQSTATUS(2));
diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
index 0b76d1186074..2d71f1e86e2a 100644
--- a/drivers/media/platform/ti-vpe/cal_regs.h
+++ b/drivers/media/platform/ti-vpe/cal_regs.h
@@ -158,6 +158,11 @@
 #define CAL_HL_IRQ_ENABLED				0x1
 #define CAL_HL_IRQ_PENDING				0x1
 
+#define CAL_HL_IRQ_OCPO_ERR_MASK		BIT(6)
+
+#define CAL_HL_IRQ_CIO_MASK(i)			BIT(16 + ((i)-1) * 8)
+#define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + ((i)-1) * 8)
+
 #define CAL_PIX_PROC_EN_MASK			BIT(0)
 #define CAL_PIX_PROC_EXTRACT_MASK		GENMASK(4, 1)
 #define CAL_PIX_PROC_EXTRACT_B6				0x0
@@ -414,6 +419,7 @@
 #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL3_MASK		BIT(17)
 #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL4_MASK		BIT(18)
 #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL5_MASK		BIT(19)
+#define CAL_CSI2_COMPLEXIO_IRQ_LANE_ERRORS_MASK		GENMASK(19, 0)
 #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM1_MASK		BIT(20)
 #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM2_MASK		BIT(21)
 #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM3_MASK		BIT(22)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 06/19] media: ti-vpe: cal: catch error irqs and print errors

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> CAL reports various errors via IRQs, which are not handled at all by the
> current driver. Add code to enable and catch those IRQs and print
> errors. This will make it much easier to notice and debug issues with
> sensors.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
>  drivers/media/platform/ti-vpe/cal_regs.h |  6 ++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 4f9dee3474ba..838215a3f230 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -684,6 +684,21 @@ static void enable_irqs(struct cal_ctx *ctx)
>  {
>  	u32 val;
>  
> +	const u32 cio_err_mask =
> +		CAL_CSI2_COMPLEXIO_IRQ_LANE_ERRORS_MASK |
> +		CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK |
> +		CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK |
> +		CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK;
> +
> +	/* Enable CIO error irqs */
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1),
> +		  CAL_HL_IRQ_CIO_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_IRQENABLE(ctx->csi2_port),
> +		  cio_err_mask);
> +
> +	/* Always enable OCPO error */
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), CAL_HL_IRQ_OCPO_ERR_MASK);
> +
>  	/* Enable IRQ_WDMA_END 0/1 */
>  	val = 0;
>  	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
> @@ -700,6 +715,12 @@ static void disable_irqs(struct cal_ctx *ctx)
>  {
>  	u32 val;
>  
> +	/* Disable CIO error irqs */
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(1),
> +		  CAL_HL_IRQ_CIO_MASK(ctx->csi2_port));
> +	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_IRQENABLE(ctx->csi2_port),
> +		  0);
> +
>  	/* Disable IRQ_WDMA_END 0/1 */
>  	val = 0;
>  	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
> @@ -1171,7 +1192,30 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  	struct cal_dev *dev = (struct cal_dev *)data;
>  	struct cal_ctx *ctx;
>  	struct cal_dmaqueue *dma_q;
> -	u32 irqst2, irqst3;
> +	u32 irqst1, irqst2, irqst3;
> +
> +	irqst1 = reg_read(dev, CAL_HL_IRQSTATUS(1));
> +	if (irqst1) {
> +		int i;
> +
> +		reg_write(dev, CAL_HL_IRQSTATUS(1), irqst1);
> +
> +		if (irqst1 & CAL_HL_IRQ_OCPO_ERR_MASK)
> +			dev_err_ratelimited(&dev->pdev->dev, "OCPO ERROR\n");
> +
> +		for (i = 1; i <= 2; ++i) {
> +			if (irqst1 & CAL_HL_IRQ_CIO_MASK(i)) {
> +				u32 cio_stat = reg_read(dev,
> +							CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
> +
> +				dev_err_ratelimited(&dev->pdev->dev,
> +						    "CIO%d error: %#08x\n", i, cio_stat);
> +
> +				reg_write(dev, CAL_CSI2_COMPLEXIO_IRQSTATUS(i),
> +					  cio_stat);
> +			}
> +		}
> +	}
>  
>  	/* Check which DMA just finished */
>  	irqst2 = reg_read(dev, CAL_HL_IRQSTATUS(2));
> diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
> index 0b76d1186074..2d71f1e86e2a 100644
> --- a/drivers/media/platform/ti-vpe/cal_regs.h
> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
> @@ -158,6 +158,11 @@
>  #define CAL_HL_IRQ_ENABLED				0x1
>  #define CAL_HL_IRQ_PENDING				0x1
>  
> +#define CAL_HL_IRQ_OCPO_ERR_MASK		BIT(6)
> +
> +#define CAL_HL_IRQ_CIO_MASK(i)			BIT(16 + ((i)-1) * 8)
> +#define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + ((i)-1) * 8)
> +
>  #define CAL_PIX_PROC_EN_MASK			BIT(0)
>  #define CAL_PIX_PROC_EXTRACT_MASK		GENMASK(4, 1)
>  #define CAL_PIX_PROC_EXTRACT_B6				0x0
> @@ -414,6 +419,7 @@
>  #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL3_MASK		BIT(17)
>  #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL4_MASK		BIT(18)
>  #define CAL_CSI2_COMPLEXIO_IRQ_ERRCONTROL5_MASK		BIT(19)
> +#define CAL_CSI2_COMPLEXIO_IRQ_LANE_ERRORS_MASK		GENMASK(19, 0)
>  #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM1_MASK		BIT(20)
>  #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM2_MASK		BIT(21)
>  #define CAL_CSI2_COMPLEXIO_IRQ_STATEULPM3_MASK		BIT(22)
> 

Reviewed-by: Benoit Parrot <bparrot@ti.com>

[PATCH v2 07/19] media: ti-vpe: cal: print errors on timeouts

[Tomi Valkeinen]

The driver does not print any errors on ComplexIO reset timeout or when
waiting for stop-state, making it difficult to debug and notice
problems.

Add error prints for these cases.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 838215a3f230..b070c56f8d80 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -844,6 +844,11 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
 		(i >= 250) ? "(timeout)" : "");
 
+	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=
+			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
+		ctx_err(ctx, "Timeout waiting for Complex IO reset done\n");
+
 	/* 4. G. Wait for all enabled lane to reach stop state */
 	for (i = 0; i < 10; i++) {
 		if (reg_read_field(ctx->dev,
@@ -857,6 +862,9 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 		ctx->csi2_port,
 		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
 		(i >= 10) ? "(timeout)" : "");
+	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
+			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
+		ctx_err(ctx, "Timeout waiting for stop state\n");
 
 	ctx_dbg(1, ctx, "CSI2_%d_REG1 = 0x%08x (Bit(31,28) should be set!)\n",
 		(ctx->csi2_port - 1), reg_read(ctx->cc, CAL_CSI2_PHY_REG1));
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 07/19] media: ti-vpe: cal: print errors on timeouts

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> The driver does not print any errors on ComplexIO reset timeout or when
> waiting for stop-state, making it difficult to debug and notice
> problems.
> 
> Add error prints for these cases.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 838215a3f230..b070c56f8d80 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -844,6 +844,11 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
>  		(i >= 250) ? "(timeout)" : "");
>  
> +	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=
> +			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
> +		ctx_err(ctx, "Timeout waiting for Complex IO reset done\n");
> +

Since you are promoting these from debug to error event then removing the related
ctx_dbg statement would make sense then.

I was using these as debug statement before but they are now useless if an error
trace is generated instead.

>  	/* 4. G. Wait for all enabled lane to reach stop state */
>  	for (i = 0; i < 10; i++) {
>  		if (reg_read_field(ctx->dev,
> @@ -857,6 +862,9 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  		ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
>  		(i >= 10) ? "(timeout)" : "");

Same here.

But with that, 

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> +	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
> +			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
> +		ctx_err(ctx, "Timeout waiting for stop state\n");
>  
>  	ctx_dbg(1, ctx, "CSI2_%d_REG1 = 0x%08x (Bit(31,28) should be set!)\n",
>  		(ctx->csi2_port - 1), reg_read(ctx->cc, CAL_CSI2_PHY_REG1));
> 

[PATCH v2 08/19] media: ti-vpe: cal: simplify irq handling

[Tomi Valkeinen]

Instead of having identical code block to handle irqs for the two CAL
ports, we can have a for loop and a single code block.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 68 +++++++++++------------------
 1 file changed, 25 insertions(+), 43 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index b070c56f8d80..ba82f0887875 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1228,64 +1228,46 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
 	/* Check which DMA just finished */
 	irqst2 = reg_read(dev, CAL_HL_IRQSTATUS(2));
 	if (irqst2) {
+		int i;
+
 		/* Clear Interrupt status */
 		reg_write(dev, CAL_HL_IRQSTATUS(2), irqst2);
 
-		/* Need to check both port */
-		if (isportirqset(irqst2, 1)) {
-			ctx = dev->ctx[0];
-
-			spin_lock(&ctx->slock);
-			ctx->dma_act = false;
-
-			if (ctx->cur_frm != ctx->next_frm)
-				cal_process_buffer_complete(ctx);
-
-			spin_unlock(&ctx->slock);
-		}
-
-		if (isportirqset(irqst2, 2)) {
-			ctx = dev->ctx[1];
+		for (i = 1; i <= 2; ++i) {
+			if (isportirqset(irqst2, i)) {
+				ctx = dev->ctx[i - 1];
 
-			spin_lock(&ctx->slock);
-			ctx->dma_act = false;
+				spin_lock(&ctx->slock);
+				ctx->dma_act = false;
 
-			if (ctx->cur_frm != ctx->next_frm)
-				cal_process_buffer_complete(ctx);
+				if (ctx->cur_frm != ctx->next_frm)
+					cal_process_buffer_complete(ctx);
 
-			spin_unlock(&ctx->slock);
+				spin_unlock(&ctx->slock);
+			}
 		}
 	}
 
 	/* Check which DMA just started */
 	irqst3 = reg_read(dev, CAL_HL_IRQSTATUS(3));
 	if (irqst3) {
+		int i;
+
 		/* Clear Interrupt status */
 		reg_write(dev, CAL_HL_IRQSTATUS(3), irqst3);
 
-		/* Need to check both port */
-		if (isportirqset(irqst3, 1)) {
-			ctx = dev->ctx[0];
-			dma_q = &ctx->vidq;
-
-			spin_lock(&ctx->slock);
-			ctx->dma_act = true;
-			if (!list_empty(&dma_q->active) &&
-			    ctx->cur_frm == ctx->next_frm)
-				cal_schedule_next_buffer(ctx);
-			spin_unlock(&ctx->slock);
-		}
-
-		if (isportirqset(irqst3, 2)) {
-			ctx = dev->ctx[1];
-			dma_q = &ctx->vidq;
-
-			spin_lock(&ctx->slock);
-			ctx->dma_act = true;
-			if (!list_empty(&dma_q->active) &&
-			    ctx->cur_frm == ctx->next_frm)
-				cal_schedule_next_buffer(ctx);
-			spin_unlock(&ctx->slock);
+		for (i = 1; i <= 2; ++i) {
+			if (isportirqset(irqst3, i)) {
+				ctx = dev->ctx[i - 1];
+				dma_q = &ctx->vidq;
+
+				spin_lock(&ctx->slock);
+				ctx->dma_act = true;
+				if (!list_empty(&dma_q->active) &&
+				    ctx->cur_frm == ctx->next_frm)
+					cal_schedule_next_buffer(ctx);
+				spin_unlock(&ctx->slock);
+			}
 		}
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 08/19] media: ti-vpe: cal: simplify irq handling

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Instead of having identical code block to handle irqs for the two CAL
> ports, we can have a for loop and a single code block.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 68 +++++++++++------------------
>  1 file changed, 25 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index b070c56f8d80..ba82f0887875 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1228,64 +1228,46 @@ static irqreturn_t cal_irq(int irq_cal, void *data)
>  	/* Check which DMA just finished */
>  	irqst2 = reg_read(dev, CAL_HL_IRQSTATUS(2));
>  	if (irqst2) {
> +		int i;
> +
>  		/* Clear Interrupt status */
>  		reg_write(dev, CAL_HL_IRQSTATUS(2), irqst2);
>  
> -		/* Need to check both port */
> -		if (isportirqset(irqst2, 1)) {
> -			ctx = dev->ctx[0];
> -
> -			spin_lock(&ctx->slock);
> -			ctx->dma_act = false;
> -
> -			if (ctx->cur_frm != ctx->next_frm)
> -				cal_process_buffer_complete(ctx);
> -
> -			spin_unlock(&ctx->slock);
> -		}
> -
> -		if (isportirqset(irqst2, 2)) {
> -			ctx = dev->ctx[1];
> +		for (i = 1; i <= 2; ++i) {
> +			if (isportirqset(irqst2, i)) {
> +				ctx = dev->ctx[i - 1];
>  
> -			spin_lock(&ctx->slock);
> -			ctx->dma_act = false;
> +				spin_lock(&ctx->slock);
> +				ctx->dma_act = false;
>  
> -			if (ctx->cur_frm != ctx->next_frm)
> -				cal_process_buffer_complete(ctx);
> +				if (ctx->cur_frm != ctx->next_frm)
> +					cal_process_buffer_complete(ctx);
>  
> -			spin_unlock(&ctx->slock);
> +				spin_unlock(&ctx->slock);
> +			}
>  		}
>  	}
>  
>  	/* Check which DMA just started */
>  	irqst3 = reg_read(dev, CAL_HL_IRQSTATUS(3));
>  	if (irqst3) {
> +		int i;
> +
>  		/* Clear Interrupt status */
>  		reg_write(dev, CAL_HL_IRQSTATUS(3), irqst3);
>  
> -		/* Need to check both port */
> -		if (isportirqset(irqst3, 1)) {
> -			ctx = dev->ctx[0];
> -			dma_q = &ctx->vidq;
> -
> -			spin_lock(&ctx->slock);
> -			ctx->dma_act = true;
> -			if (!list_empty(&dma_q->active) &&
> -			    ctx->cur_frm == ctx->next_frm)
> -				cal_schedule_next_buffer(ctx);
> -			spin_unlock(&ctx->slock);
> -		}
> -
> -		if (isportirqset(irqst3, 2)) {
> -			ctx = dev->ctx[1];
> -			dma_q = &ctx->vidq;
> -
> -			spin_lock(&ctx->slock);
> -			ctx->dma_act = true;
> -			if (!list_empty(&dma_q->active) &&
> -			    ctx->cur_frm == ctx->next_frm)
> -				cal_schedule_next_buffer(ctx);
> -			spin_unlock(&ctx->slock);
> +		for (i = 1; i <= 2; ++i) {
> +			if (isportirqset(irqst3, i)) {
> +				ctx = dev->ctx[i - 1];
> +				dma_q = &ctx->vidq;
> +
> +				spin_lock(&ctx->slock);
> +				ctx->dma_act = true;
> +				if (!list_empty(&dma_q->active) &&
> +				    ctx->cur_frm == ctx->next_frm)
> +					cal_schedule_next_buffer(ctx);
> +				spin_unlock(&ctx->slock);
> +			}
>  		}
>  	}
>  
> 

Reviewed-by: Benoit Parrot <bparrot@ti.com>

[PATCH v2 09/19] media: ti-vpe: cal: remove useless CAL_GEN_* macros

[Tomi Valkeinen]

These macros only obfuscate the code, so drop them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c      | 20 ++++++++------------
 drivers/media/platform/ti-vpe/cal_regs.h |  9 ---------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index ba82f0887875..53072afbaf4e 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -777,10 +777,8 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 
 	/* 3.B. Program Stop States */
 	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
-	set_field(&val, CAL_GEN_ENABLE,
-		  CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
-	set_field(&val, CAL_GEN_DISABLE,
-		  CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
+	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
+	set_field(&val, 0, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
 	set_field(&val, 407, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
 	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
 	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop States\n",
@@ -789,8 +787,7 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 
 	/* 4. Force FORCERXMODE */
 	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
-	set_field(&val, CAL_GEN_ENABLE,
-		  CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
+	set_field(&val, 1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
 	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
 	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Force RXMODE\n",
 		ctx->csi2_port,
@@ -853,8 +850,7 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 	for (i = 0; i < 10; i++) {
 		if (reg_read_field(ctx->dev,
 				   CAL_CSI2_TIMING(ctx->csi2_port),
-				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) ==
-		    CAL_GEN_DISABLE)
+				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
 			break;
 		usleep_range(1000, 1100);
 	}
@@ -951,13 +947,13 @@ static void csi2_ppi_enable(struct cal_ctx *ctx)
 {
 	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
 	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
-			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
+			1, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
 }
 
 static void csi2_ppi_disable(struct cal_ctx *ctx)
 {
 	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
-			CAL_GEN_DISABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
+			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
 }
 
 static void csi2_ctx_config(struct cal_ctx *ctx)
@@ -1032,7 +1028,7 @@ static void pix_proc_config(struct cal_ctx *ctx)
 	set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK);
 	set_field(&val, pack, CAL_PIX_PROC_PACK_MASK);
 	set_field(&val, ctx->csi2_port, CAL_PIX_PROC_CPORT_MASK);
-	set_field(&val, CAL_GEN_ENABLE, CAL_PIX_PROC_EN_MASK);
+	set_field(&val, 1, CAL_PIX_PROC_EN_MASK);
 	reg_write(ctx->dev, CAL_PIX_PROC(ctx->csi2_port), val);
 	ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->csi2_port,
 		reg_read(ctx->dev, CAL_PIX_PROC(ctx->csi2_port)));
@@ -1052,7 +1048,7 @@ static void cal_wr_dma_config(struct cal_ctx *ctx,
 		  CAL_WR_DMA_CTRL_MODE_MASK);
 	set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR,
 		  CAL_WR_DMA_CTRL_PATTERN_MASK);
-	set_field(&val, CAL_GEN_ENABLE, CAL_WR_DMA_CTRL_STALL_RD_MASK);
+	set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK);
 	reg_write(ctx->dev, CAL_WR_DMA_CTRL(ctx->csi2_port), val);
 	ctx_dbg(3, ctx, "CAL_WR_DMA_CTRL(%d) = 0x%08x\n", ctx->csi2_port,
 		reg_read(ctx->dev, CAL_WR_DMA_CTRL(ctx->csi2_port)));
diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
index 2d71f1e86e2a..3b3150aaf343 100644
--- a/drivers/media/platform/ti-vpe/cal_regs.h
+++ b/drivers/media/platform/ti-vpe/cal_regs.h
@@ -100,15 +100,6 @@
 /* CAL Control Module Core Camerrx Control register offsets */
 #define CM_CTRL_CORE_CAMERRX_CONTROL	0x000
 
-/*********************************************************************
-* Generic value used in various field below
-*********************************************************************/
-
-#define CAL_GEN_DISABLE			0
-#define CAL_GEN_ENABLE			1
-#define CAL_GEN_FALSE			0
-#define CAL_GEN_TRUE			1
-
 /*********************************************************************
 * Field Definition Macros
 *********************************************************************/
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 09/19] media: ti-vpe: cal: remove useless CAL_GEN_* macros

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> These macros only obfuscate the code, so drop them.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c      | 20 ++++++++------------
>  drivers/media/platform/ti-vpe/cal_regs.h |  9 ---------
>  2 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index ba82f0887875..53072afbaf4e 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -777,10 +777,8 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  
>  	/* 3.B. Program Stop States */
>  	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
> -	set_field(&val, CAL_GEN_ENABLE,
> -		  CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
> -	set_field(&val, CAL_GEN_DISABLE,
> -		  CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
> +	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
> +	set_field(&val, 0, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
>  	set_field(&val, 407, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
>  	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
>  	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop States\n",
> @@ -789,8 +787,7 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  
>  	/* 4. Force FORCERXMODE */
>  	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
> -	set_field(&val, CAL_GEN_ENABLE,
> -		  CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
> +	set_field(&val, 1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
>  	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
>  	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Force RXMODE\n",
>  		ctx->csi2_port,
> @@ -853,8 +850,7 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  	for (i = 0; i < 10; i++) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_TIMING(ctx->csi2_port),
> -				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) ==
> -		    CAL_GEN_DISABLE)
> +				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
>  			break;
>  		usleep_range(1000, 1100);
>  	}
> @@ -951,13 +947,13 @@ static void csi2_ppi_enable(struct cal_ctx *ctx)
>  {
>  	reg_write(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port), BIT(3));
>  	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
> -			CAL_GEN_ENABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
> +			1, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>  }
>  
>  static void csi2_ppi_disable(struct cal_ctx *ctx)
>  {
>  	reg_write_field(ctx->dev, CAL_CSI2_PPI_CTRL(ctx->csi2_port),
> -			CAL_GEN_DISABLE, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
> +			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>  }
>  
>  static void csi2_ctx_config(struct cal_ctx *ctx)
> @@ -1032,7 +1028,7 @@ static void pix_proc_config(struct cal_ctx *ctx)
>  	set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK);
>  	set_field(&val, pack, CAL_PIX_PROC_PACK_MASK);
>  	set_field(&val, ctx->csi2_port, CAL_PIX_PROC_CPORT_MASK);
> -	set_field(&val, CAL_GEN_ENABLE, CAL_PIX_PROC_EN_MASK);
> +	set_field(&val, 1, CAL_PIX_PROC_EN_MASK);
>  	reg_write(ctx->dev, CAL_PIX_PROC(ctx->csi2_port), val);
>  	ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_PIX_PROC(ctx->csi2_port)));
> @@ -1052,7 +1048,7 @@ static void cal_wr_dma_config(struct cal_ctx *ctx,
>  		  CAL_WR_DMA_CTRL_MODE_MASK);
>  	set_field(&val, CAL_WR_DMA_CTRL_PATTERN_LINEAR,
>  		  CAL_WR_DMA_CTRL_PATTERN_MASK);
> -	set_field(&val, CAL_GEN_ENABLE, CAL_WR_DMA_CTRL_STALL_RD_MASK);
> +	set_field(&val, 1, CAL_WR_DMA_CTRL_STALL_RD_MASK);
>  	reg_write(ctx->dev, CAL_WR_DMA_CTRL(ctx->csi2_port), val);
>  	ctx_dbg(3, ctx, "CAL_WR_DMA_CTRL(%d) = 0x%08x\n", ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_WR_DMA_CTRL(ctx->csi2_port)));
> diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
> index 2d71f1e86e2a..3b3150aaf343 100644
> --- a/drivers/media/platform/ti-vpe/cal_regs.h
> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
> @@ -100,15 +100,6 @@
>  /* CAL Control Module Core Camerrx Control register offsets */
>  #define CM_CTRL_CORE_CAMERRX_CONTROL	0x000
>  
> -/*********************************************************************
> -* Generic value used in various field below
> -*********************************************************************/
> -
> -#define CAL_GEN_DISABLE			0
> -#define CAL_GEN_ENABLE			1
> -#define CAL_GEN_FALSE			0
> -#define CAL_GEN_TRUE			1
> -
>  /*********************************************************************
>  * Field Definition Macros
>  *********************************************************************/
> 

[PATCH v2 10/19] media: ti-vpe: cal: remove useless IRQ defines

[Tomi Valkeinen]

Remove a bunch of IRQ defines, of which only CAL_HL_IRQ_ENABLE and
CAL_HL_IRQ_CLEAR are used, and these defines only end up obfuscating
code.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c      | 8 ++++----
 drivers/media/platform/ti-vpe/cal_regs.h | 6 ------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 53072afbaf4e..979f9027a232 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -701,11 +701,11 @@ static void enable_irqs(struct cal_ctx *ctx)
 
 	/* Enable IRQ_WDMA_END 0/1 */
 	val = 0;
-	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), val);
 	/* Enable IRQ_WDMA_START 0/1 */
 	val = 0;
-	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), val);
 	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
 	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);
@@ -723,11 +723,11 @@ static void disable_irqs(struct cal_ctx *ctx)
 
 	/* Disable IRQ_WDMA_END 0/1 */
 	val = 0;
-	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), val);
 	/* Disable IRQ_WDMA_START 0/1 */
 	val = 0;
-	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
+	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), val);
 	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
 	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
index 3b3150aaf343..6a235abc25b1 100644
--- a/drivers/media/platform/ti-vpe/cal_regs.h
+++ b/drivers/media/platform/ti-vpe/cal_regs.h
@@ -142,12 +142,6 @@
 #define CAL_HL_IRQ_EOI_LINE_NUMBER_EOI0			0
 
 #define CAL_HL_IRQ_MASK(m)			BIT((m) - 1)
-#define CAL_HL_IRQ_NOACTION				0x0
-#define CAL_HL_IRQ_ENABLE				0x1
-#define CAL_HL_IRQ_CLEAR				0x1
-#define CAL_HL_IRQ_DISABLED				0x0
-#define CAL_HL_IRQ_ENABLED				0x1
-#define CAL_HL_IRQ_PENDING				0x1
 
 #define CAL_HL_IRQ_OCPO_ERR_MASK		BIT(6)
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 10/19] media: ti-vpe: cal: remove useless IRQ defines

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Remove a bunch of IRQ defines, of which only CAL_HL_IRQ_ENABLE and
> CAL_HL_IRQ_CLEAR are used, and these defines only end up obfuscating
> code.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c      | 8 ++++----
>  drivers/media/platform/ti-vpe/cal_regs.h | 6 ------
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 53072afbaf4e..979f9027a232 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -701,11 +701,11 @@ static void enable_irqs(struct cal_ctx *ctx)
>  
>  	/* Enable IRQ_WDMA_END 0/1 */
>  	val = 0;
> -	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), val);
>  	/* Enable IRQ_WDMA_START 0/1 */
>  	val = 0;
> -	set_field(&val, CAL_HL_IRQ_ENABLE, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), val);
>  	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);
> @@ -723,11 +723,11 @@ static void disable_irqs(struct cal_ctx *ctx)
>  
>  	/* Disable IRQ_WDMA_END 0/1 */
>  	val = 0;
> -	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), val);
>  	/* Disable IRQ_WDMA_START 0/1 */
>  	val = 0;
> -	set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
> +	set_field(&val, 1, CAL_HL_IRQ_MASK(ctx->csi2_port));
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), val);
>  	/* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
> diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
> index 3b3150aaf343..6a235abc25b1 100644
> --- a/drivers/media/platform/ti-vpe/cal_regs.h
> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
> @@ -142,12 +142,6 @@
>  #define CAL_HL_IRQ_EOI_LINE_NUMBER_EOI0			0
>  
>  #define CAL_HL_IRQ_MASK(m)			BIT((m) - 1)
> -#define CAL_HL_IRQ_NOACTION				0x0
> -#define CAL_HL_IRQ_ENABLE				0x1
> -#define CAL_HL_IRQ_CLEAR				0x1
> -#define CAL_HL_IRQ_DISABLED				0x0
> -#define CAL_HL_IRQ_ENABLED				0x1
> -#define CAL_HL_IRQ_PENDING				0x1
>  
>  #define CAL_HL_IRQ_OCPO_ERR_MASK		BIT(6)
>  
> 

[PATCH v2 11/19] media: ti-vpe: cal: use reg_write_field

[Tomi Valkeinen]

Simplify the code by using reg_write_field() where trivially possible.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 34 ++++++++++++-----------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 979f9027a232..5208dfde6fb5 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -761,10 +761,9 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 	camerarx_phy_enable(ctx);
 
 	/* 2. Reset complex IO - Do not wait for reset completion */
-	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
-	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL,
-		  CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
-	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
+	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL,
+			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
 	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x De-assert Complex IO Reset\n",
 		ctx->csi2_port,
 		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
@@ -786,18 +785,16 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
 
 	/* 4. Force FORCERXMODE */
-	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
-	set_field(&val, 1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
-	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
+	reg_write_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
+			1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
 	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Force RXMODE\n",
 		ctx->csi2_port,
 		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
 
 	/* E. Power up the PHY using the complex IO */
-	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
-	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
-		  CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
-	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
+	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
+			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
 
 	/* F. Wait for power up completion */
 	for (i = 0; i < 10; i++) {
@@ -869,13 +866,11 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 static void csi2_phy_deinit(struct cal_ctx *ctx)
 {
 	int i;
-	u32 val;
 
 	/* Power down the PHY using the complex IO */
-	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
-	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
-		  CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
-	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
+	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
+			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
 
 	/* Wait for power down completion */
 	for (i = 0; i < 10; i++) {
@@ -892,10 +887,9 @@ static void csi2_phy_deinit(struct cal_ctx *ctx)
 		(i >= 10) ? "(timeout)" : "");
 
 	/* Assert Comple IO Reset */
-	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
-	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
-		  CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
-	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
+	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
+			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
 
 	/* Wait for power down completion */
 	for (i = 0; i < 10; i++) {
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 11/19] media: ti-vpe: cal: use reg_write_field

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Simplify the code by using reg_write_field() where trivially possible.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 34 ++++++++++++-----------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 979f9027a232..5208dfde6fb5 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -761,10 +761,9 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  	camerarx_phy_enable(ctx);
>  
>  	/* 2. Reset complex IO - Do not wait for reset completion */
> -	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
> -	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL,
> -		  CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
> -	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
> +	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL,
> +			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
>  	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x De-assert Complex IO Reset\n",
>  		ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
> @@ -786,18 +785,16 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
>  
>  	/* 4. Force FORCERXMODE */
> -	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
> -	set_field(&val, 1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
> -	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
> +	reg_write_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
> +			1, CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK);
>  	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Force RXMODE\n",
>  		ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
>  
>  	/* E. Power up the PHY using the complex IO */
> -	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
> -	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
> -		  CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
> -	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
> +	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
> +			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
>  
>  	/* F. Wait for power up completion */
>  	for (i = 0; i < 10; i++) {
> @@ -869,13 +866,11 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  static void csi2_phy_deinit(struct cal_ctx *ctx)
>  {
>  	int i;
> -	u32 val;
>  
>  	/* Power down the PHY using the complex IO */
> -	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
> -	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
> -		  CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
> -	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
> +	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
> +			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
>  
>  	/* Wait for power down completion */
>  	for (i = 0; i < 10; i++) {
> @@ -892,10 +887,9 @@ static void csi2_phy_deinit(struct cal_ctx *ctx)
>  		(i >= 10) ? "(timeout)" : "");
>  
>  	/* Assert Comple IO Reset */
> -	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
> -	set_field(&val, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
> -		  CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
> -	reg_write(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port), val);
> +	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL,
> +			CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK);
>  
>  	/* Wait for power down completion */
>  	for (i = 0; i < 10; i++) {
> 

[PATCH v2 12/19] media: ti-vpe: cal: cleanup CIO power enable/disable

[Tomi Valkeinen]

Move the code to enable and disable ComplexIO power to its own function.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 67 +++++++++++++----------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 5208dfde6fb5..ed32b199aabe 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -733,11 +733,39 @@ static void disable_irqs(struct cal_ctx *ctx)
 	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
 }
 
+static void csi2_cio_power(struct cal_ctx *ctx, bool enable)
+{
+	u32 target_state;
+	unsigned int i;
+
+	target_state = enable ? CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON :
+		       CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF;
+
+	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+			target_state, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
+
+	for (i = 0; i < 10; i++) {
+		u32 current_state;
+
+		current_state = reg_read_field(ctx->dev,
+					       CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
+					       CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK);
+
+		if (current_state == target_state)
+			break;
+
+		usleep_range(1000, 1100);
+	}
+
+	if (i == 10)
+		ctx_err(ctx, "Failed to power %s complexio\n",
+			enable ? "up" : "down");
+}
+
 static void csi2_phy_config(struct cal_ctx *ctx);
 
 static void csi2_phy_init(struct cal_ctx *ctx)
 {
-	int i;
 	u32 val;
 
 	/* Steps
@@ -792,23 +820,7 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
 
 	/* E. Power up the PHY using the complex IO */
-	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
-			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
-			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
-
-	/* F. Wait for power up completion */
-	for (i = 0; i < 10; i++) {
-		if (reg_read_field(ctx->dev,
-				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
-				   CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK) ==
-		    CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_STATE_ON)
-			break;
-		usleep_range(1000, 1100);
-	}
-	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Powered UP %s\n",
-		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)),
-		(i >= 10) ? "(timeout)" : "");
+	csi2_cio_power(ctx, true);
 }
 
 static void csi2_wait_for_phy(struct cal_ctx *ctx)
@@ -867,24 +879,7 @@ static void csi2_phy_deinit(struct cal_ctx *ctx)
 {
 	int i;
 
-	/* Power down the PHY using the complex IO */
-	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
-			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
-			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
-
-	/* Wait for power down completion */
-	for (i = 0; i < 10; i++) {
-		if (reg_read_field(ctx->dev,
-				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
-				   CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK) ==
-		    CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_STATE_OFF)
-			break;
-		usleep_range(1000, 1100);
-	}
-	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Powered Down %s\n",
-		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)),
-		(i >= 10) ? "(timeout)" : "");
+	csi2_cio_power(ctx, false);
 
 	/* Assert Comple IO Reset */
 	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 12/19] media: ti-vpe: cal: cleanup CIO power enable/disable

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Move the code to enable and disable ComplexIO power to its own function.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 67 +++++++++++++----------------
>  1 file changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 5208dfde6fb5..ed32b199aabe 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -733,11 +733,39 @@ static void disable_irqs(struct cal_ctx *ctx)
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
>  }
>  
> +static void csi2_cio_power(struct cal_ctx *ctx, bool enable)
> +{
> +	u32 target_state;
> +	unsigned int i;
> +
> +	target_state = enable ? CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON :
> +		       CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF;
> +
> +	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +			target_state, CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
> +
> +	for (i = 0; i < 10; i++) {
> +		u32 current_state;
> +
> +		current_state = reg_read_field(ctx->dev,
> +					       CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> +					       CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK);
> +
> +		if (current_state == target_state)
> +			break;
> +
> +		usleep_range(1000, 1100);
> +	}
> +
> +	if (i == 10)
> +		ctx_err(ctx, "Failed to power %s complexio\n",
> +			enable ? "up" : "down");
> +}
> +
>  static void csi2_phy_config(struct cal_ctx *ctx);
>  
>  static void csi2_phy_init(struct cal_ctx *ctx)
>  {
> -	int i;
>  	u32 val;
>  
>  	/* Steps
> @@ -792,23 +820,7 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
>  
>  	/* E. Power up the PHY using the complex IO */
> -	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> -			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_ON,
> -			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
> -
> -	/* F. Wait for power up completion */
> -	for (i = 0; i < 10; i++) {
> -		if (reg_read_field(ctx->dev,
> -				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> -				   CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK) ==
> -		    CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_STATE_ON)
> -			break;
> -		usleep_range(1000, 1100);
> -	}
> -	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Powered UP %s\n",
> -		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)),
> -		(i >= 10) ? "(timeout)" : "");
> +	csi2_cio_power(ctx, true);
>  }
>  
>  static void csi2_wait_for_phy(struct cal_ctx *ctx)
> @@ -867,24 +879,7 @@ static void csi2_phy_deinit(struct cal_ctx *ctx)
>  {
>  	int i;
>  
> -	/* Power down the PHY using the complex IO */
> -	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> -			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_STATE_OFF,
> -			CAL_CSI2_COMPLEXIO_CFG_PWR_CMD_MASK);
> -
> -	/* Wait for power down completion */
> -	for (i = 0; i < 10; i++) {
> -		if (reg_read_field(ctx->dev,
> -				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> -				   CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_MASK) ==
> -		    CAL_CSI2_COMPLEXIO_CFG_PWR_STATUS_STATE_OFF)
> -			break;
> -		usleep_range(1000, 1100);
> -	}
> -	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Powered Down %s\n",
> -		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)),
> -		(i >= 10) ? "(timeout)" : "");
> +	csi2_cio_power(ctx, false);
>  
>  	/* Assert Comple IO Reset */
>  	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> 

[PATCH v2 13/19] media: ti-vpe: cal: fix dummy read to phy

[Tomi Valkeinen]

After ComplexIO reset, a dummy read to PHY is needed as per CAL spec to
finish the reset. Currently the driver reads a ComplexIO register, not
PHY register. Fix this.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index ed32b199aabe..e6f766ba3079 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -796,8 +796,8 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 		ctx->csi2_port,
 		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
 
-	/* Dummy read to allow SCP to complete */
-	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
+	/* Dummy read to allow SCP reset to complete */
+	reg_read(ctx->cc, CAL_CSI2_PHY_REG0);
 
 	/* 3.A. Program Phy Timing Parameters */
 	csi2_phy_config(ctx);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 13/19] media: ti-vpe: cal: fix dummy read to phy

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> After ComplexIO reset, a dummy read to PHY is needed as per CAL spec to
> finish the reset. Currently the driver reads a ComplexIO register, not
> PHY register. Fix this.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Nice catch.

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index ed32b199aabe..e6f766ba3079 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -796,8 +796,8 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  		ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
>  
> -	/* Dummy read to allow SCP to complete */
> -	val = reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port));
> +	/* Dummy read to allow SCP reset to complete */
> +	reg_read(ctx->cc, CAL_CSI2_PHY_REG0);
>  
>  	/* 3.A. Program Phy Timing Parameters */
>  	csi2_phy_config(ctx);
> 

[PATCH v2 14/19] media: ti-vpe: cal: program number of lines properly

[Tomi Valkeinen]

CAL_CSI2_CTX register has LINES field, which, according to the
documentation, should be programmed to the number of lines transmitted
by the camera. If the number of lines is unknown, it can be set to 0.
The driver sets the field to 0 for some reason, even if we know the
number of lines.

This patch sets the number of lines properly, which will allow the HW to
discard extra lines (if the sensor would send such for some reason),
and, according to documentation: "This leads to regular video timings
and avoids potential artifacts".

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index e6f766ba3079..a59931ba3ed7 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -962,8 +962,7 @@ static void csi2_ctx_config(struct cal_ctx *ctx)
 	set_field(&val, 0x1, CAL_CSI2_CTX_DT_MASK);
 	/* Virtual Channel from the CSI2 sensor usually 0! */
 	set_field(&val, ctx->virtual_channel, CAL_CSI2_CTX_VC_MASK);
-	/* NUM_LINES_PER_FRAME => 0 means auto detect */
-	set_field(&val, 0, CAL_CSI2_CTX_LINES_MASK);
+	set_field(&val, ctx->v_fmt.fmt.pix.height, CAL_CSI2_CTX_LINES_MASK);
 	set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK);
 	set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE,
 		  CAL_CSI2_CTX_PACK_MODE_MASK);
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 14/19] media: ti-vpe: cal: program number of lines properly

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> CAL_CSI2_CTX register has LINES field, which, according to the
> documentation, should be programmed to the number of lines transmitted
> by the camera. If the number of lines is unknown, it can be set to 0.
> The driver sets the field to 0 for some reason, even if we know the
> number of lines.
> 
> This patch sets the number of lines properly, which will allow the HW to
> discard extra lines (if the sensor would send such for some reason),
> and, according to documentation: "This leads to regular video timings
> and avoids potential artifacts".
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index e6f766ba3079..a59931ba3ed7 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -962,8 +962,7 @@ static void csi2_ctx_config(struct cal_ctx *ctx)
>  	set_field(&val, 0x1, CAL_CSI2_CTX_DT_MASK);
>  	/* Virtual Channel from the CSI2 sensor usually 0! */
>  	set_field(&val, ctx->virtual_channel, CAL_CSI2_CTX_VC_MASK);
> -	/* NUM_LINES_PER_FRAME => 0 means auto detect */
> -	set_field(&val, 0, CAL_CSI2_CTX_LINES_MASK);
> +	set_field(&val, ctx->v_fmt.fmt.pix.height, CAL_CSI2_CTX_LINES_MASK);
>  	set_field(&val, CAL_CSI2_CTX_ATT_PIX, CAL_CSI2_CTX_ATT_MASK);
>  	set_field(&val, CAL_CSI2_CTX_PACK_MODE_LINE,
>  		  CAL_CSI2_CTX_PACK_MODE_MASK);
> 

[PATCH v2 15/19] media: ti-vpe: cal: set DMA max seg size

[Tomi Valkeinen]

Set DMA max seg size correctly to get rid of warnings on 64 bit
platforms:

DMA-API: cal 6f03000.cal: mapping sg segment longer than device claims to support [len=720896] [max=65536]

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index a59931ba3ed7..b1b9616b12b6 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -2322,6 +2322,8 @@ static int cal_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
+
 	pm_runtime_enable(&pdev->dev);
 
 	ret = pm_runtime_get_sync(&pdev->dev);
@@ -2336,6 +2338,8 @@ static int cal_probe(struct platform_device *pdev)
 	return 0;
 
 runtime_disable:
+	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
+
 	pm_runtime_disable(&pdev->dev);
 	for (i = 0; i < CAL_NUM_CONTEXT; i++) {
 		ctx = dev->ctx[i];
@@ -2378,6 +2382,8 @@ static int cal_remove(struct platform_device *pdev)
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
+
 	return 0;
 }
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 15/19] media: ti-vpe: cal: set DMA max seg size

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Set DMA max seg size correctly to get rid of warnings on 64 bit
> platforms:
> 
> DMA-API: cal 6f03000.cal: mapping sg segment longer than device claims to support [len=720896] [max=65536]
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a59931ba3ed7..b1b9616b12b6 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -2322,6 +2322,8 @@ static int cal_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> +
>  	pm_runtime_enable(&pdev->dev);
>  
>  	ret = pm_runtime_get_sync(&pdev->dev);
> @@ -2336,6 +2338,8 @@ static int cal_probe(struct platform_device *pdev)
>  	return 0;
>  
>  runtime_disable:
> +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> +
>  	pm_runtime_disable(&pdev->dev);
>  	for (i = 0; i < CAL_NUM_CONTEXT; i++) {
>  		ctx = dev->ctx[i];
> @@ -2378,6 +2382,8 @@ static int cal_remove(struct platform_device *pdev)
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> +
>  	return 0;
>  }
>  
> 

[PATCH v2 16/19] media: ti-vpe: cal: move code to separate functions

[Tomi Valkeinen]

To make csi2_wait_for_phy() more readable, move code to separate
functions.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 38 ++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index b1b9616b12b6..ed68ad58121e 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -823,20 +823,10 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 	csi2_cio_power(ctx, true);
 }
 
-static void csi2_wait_for_phy(struct cal_ctx *ctx)
+static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
 {
 	int i;
 
-	/* Steps
-	 *  2. Wait for completion of reset
-	 *          Note if the external sensor is not sending byte clock,
-	 *          the reset will timeout
-	 *  4.Force FORCERXMODE
-	 *      G. Wait for all enabled lane to reach stop state
-	 *      H. Disable pull down using pad control
-	 */
-
-	/* 2. Wait for reset completion */
 	for (i = 0; i < 250; i++) {
 		if (reg_read_field(ctx->dev,
 				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
@@ -855,7 +845,12 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
 		ctx_err(ctx, "Timeout waiting for Complex IO reset done\n");
 
-	/* 4. G. Wait for all enabled lane to reach stop state */
+}
+
+static void csi2_wait_stop_state(struct cal_ctx *ctx)
+{
+	int i;
+
 	for (i = 0; i < 10; i++) {
 		if (reg_read_field(ctx->dev,
 				   CAL_CSI2_TIMING(ctx->csi2_port),
@@ -867,9 +862,28 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
 		ctx->csi2_port,
 		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
 		(i >= 10) ? "(timeout)" : "");
+
 	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
 			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
 		ctx_err(ctx, "Timeout waiting for stop state\n");
+}
+
+static void csi2_wait_for_phy(struct cal_ctx *ctx)
+{
+	/* Steps
+	 *  2. Wait for completion of reset
+	 *          Note if the external sensor is not sending byte clock,
+	 *          the reset will timeout
+	 *  4.Force FORCERXMODE
+	 *      G. Wait for all enabled lane to reach stop state
+	 *      H. Disable pull down using pad control
+	 */
+
+	/* 2. Wait for reset completion */
+	csi2_wait_complexio_reset(ctx);
+
+	/* 4. G. Wait for all enabled lane to reach stop state */
+	csi2_wait_stop_state(ctx);
 
 	ctx_dbg(1, ctx, "CSI2_%d_REG1 = 0x%08x (Bit(31,28) should be set!)\n",
 		(ctx->csi2_port - 1), reg_read(ctx->cc, CAL_CSI2_PHY_REG1));
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 16/19] media: ti-vpe: cal: move code to separate functions

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> To make csi2_wait_for_phy() more readable, move code to separate
> functions.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 38 ++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index b1b9616b12b6..ed68ad58121e 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -823,20 +823,10 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  	csi2_cio_power(ctx, true);
>  }
>  
> -static void csi2_wait_for_phy(struct cal_ctx *ctx)
> +static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>  {
>  	int i;
>  
> -	/* Steps
> -	 *  2. Wait for completion of reset
> -	 *          Note if the external sensor is not sending byte clock,
> -	 *          the reset will timeout
> -	 *  4.Force FORCERXMODE
> -	 *      G. Wait for all enabled lane to reach stop state
> -	 *      H. Disable pull down using pad control
> -	 */
> -
> -	/* 2. Wait for reset completion */
>  	for (i = 0; i < 250; i++) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
> @@ -855,7 +845,12 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
>  		ctx_err(ctx, "Timeout waiting for Complex IO reset done\n");
>  
> -	/* 4. G. Wait for all enabled lane to reach stop state */
> +}
> +
> +static void csi2_wait_stop_state(struct cal_ctx *ctx)
> +{
> +	int i;
> +
>  	for (i = 0; i < 10; i++) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_TIMING(ctx->csi2_port),
> @@ -867,9 +862,28 @@ static void csi2_wait_for_phy(struct cal_ctx *ctx)
>  		ctx->csi2_port,
>  		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
>  		(i >= 10) ? "(timeout)" : "");
> +
>  	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
>  			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
>  		ctx_err(ctx, "Timeout waiting for stop state\n");
> +}
> +
> +static void csi2_wait_for_phy(struct cal_ctx *ctx)
> +{
> +	/* Steps
> +	 *  2. Wait for completion of reset
> +	 *          Note if the external sensor is not sending byte clock,
> +	 *          the reset will timeout
> +	 *  4.Force FORCERXMODE
> +	 *      G. Wait for all enabled lane to reach stop state
> +	 *      H. Disable pull down using pad control
> +	 */
> +
> +	/* 2. Wait for reset completion */
> +	csi2_wait_complexio_reset(ctx);
> +
> +	/* 4. G. Wait for all enabled lane to reach stop state */
> +	csi2_wait_stop_state(ctx);
>  
>  	ctx_dbg(1, ctx, "CSI2_%d_REG1 = 0x%08x (Bit(31,28) should be set!)\n",
>  		(ctx->csi2_port - 1), reg_read(ctx->cc, CAL_CSI2_PHY_REG1));
> 

[PATCH v2 17/19] media: ti-vpe: cal: improve wait for CIO resetdone

[Tomi Valkeinen]

Sometimes there is a timeout when waiting for the 'ComplexIO Reset
Done'.  Testing shows that sometimes we need to wait more than what the
current code does. It is not clear how long this wait can be, but it is
based on how quickly the sensor provides a valid clock, and how quickly
CAL syncs to it.

Change the code to make it more obvious how long we'll wait, and set a
wider range for usleep_range. Increase the timeout to 750ms.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index ed68ad58121e..0dc7892881e7 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -825,20 +825,17 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 
 static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
 {
-	int i;
+	unsigned long timeout;
 
-	for (i = 0; i < 250; i++) {
+	timeout = jiffies + msecs_to_jiffies(750);
+	while (time_before(jiffies, timeout)) {
 		if (reg_read_field(ctx->dev,
 				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
 				   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) ==
 		    CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
 			break;
-		usleep_range(1000, 1100);
+		usleep_range(500, 5000);
 	}
-	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done (%d) %s\n",
-		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
-		(i >= 250) ? "(timeout)" : "");
 
 	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
 			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 17/19] media: ti-vpe: cal: improve wait for CIO resetdone

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Sometimes there is a timeout when waiting for the 'ComplexIO Reset
> Done'.  Testing shows that sometimes we need to wait more than what the
> current code does. It is not clear how long this wait can be, but it is
> based on how quickly the sensor provides a valid clock, and how quickly
> CAL syncs to it.
> 
> Change the code to make it more obvious how long we'll wait, and set a
> wider range for usleep_range. Increase the timeout to 750ms.

Hmm strange, I am pretty sure I had tried larger values in the past...

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index ed68ad58121e..0dc7892881e7 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -825,20 +825,17 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  
>  static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>  {
> -	int i;
> +	unsigned long timeout;
>  
> -	for (i = 0; i < 250; i++) {
> +	timeout = jiffies + msecs_to_jiffies(750);
> +	while (time_before(jiffies, timeout)) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
>  				   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) ==
>  		    CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED)
>  			break;
> -		usleep_range(1000, 1100);
> +		usleep_range(500, 5000);
>  	}
> -	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done (%d) %s\n",
> -		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
> -		(i >= 250) ? "(timeout)" : "");
>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
>  			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=
> 

[PATCH v2 18/19] media: ti-vpe: cal: improve wait for stop-state

[Tomi Valkeinen]

Sometimes there is a timeout when waiting for the Stop-State.  Testing
shows that sometimes we need to wait more than what the current code
does. It is not clear how long this wait can be, but it is based on how
quickly the sensor provides a valid clock, and how quickly CAL syncs to
it.

Change the code to make it more obvious how long we'll wait, and set a
wider range for usleep_range. Increase the timeout to 750ms.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 0dc7892881e7..0f90078ee8c2 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -846,19 +846,16 @@ static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
 
 static void csi2_wait_stop_state(struct cal_ctx *ctx)
 {
-	int i;
+	unsigned long timeout;
 
-	for (i = 0; i < 10; i++) {
+	timeout = jiffies + msecs_to_jiffies(750);
+	while (time_before(jiffies, timeout)) {
 		if (reg_read_field(ctx->dev,
 				   CAL_CSI2_TIMING(ctx->csi2_port),
 				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
 			break;
-		usleep_range(1000, 1100);
+		usleep_range(500, 5000);
 	}
-	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
-		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
-		(i >= 10) ? "(timeout)" : "");
 
 	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
 			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 18/19] media: ti-vpe: cal: improve wait for stop-state

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> Sometimes there is a timeout when waiting for the Stop-State.  Testing
> shows that sometimes we need to wait more than what the current code
> does. It is not clear how long this wait can be, but it is based on how
> quickly the sensor provides a valid clock, and how quickly CAL syncs to
> it.
> 
> Change the code to make it more obvious how long we'll wait, and set a
> wider range for usleep_range. Increase the timeout to 750ms.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Benoit Parrot <bparrot@ti.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 0dc7892881e7..0f90078ee8c2 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -846,19 +846,16 @@ static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>  
>  static void csi2_wait_stop_state(struct cal_ctx *ctx)
>  {
> -	int i;
> +	unsigned long timeout;
>  
> -	for (i = 0; i < 10; i++) {
> +	timeout = jiffies + msecs_to_jiffies(750);
> +	while (time_before(jiffies, timeout)) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_TIMING(ctx->csi2_port),
>  				   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) == 0)
>  			break;
> -		usleep_range(1000, 1100);
> +		usleep_range(500, 5000);
>  	}
> -	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
> -		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
> -		(i >= 10) ? "(timeout)" : "");
>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
>  			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)
> 

[PATCH v2 19/19] media: ti-vpe: cal: fix stop state timeout

[Tomi Valkeinen]

The stop-state timeout needs to be over 100us as per CSI spec. With the
CAL fclk of 266 MHZ on DRA76, with the current value the driver uses,
the timeout is 24us. Too small timeout will cause failure to enable the
streaming.

Also, the fclk can be different on other SoCs, as is the case with AM65x
where the fclk is 250 MHz.

This patch fixes the timeout by calculating it correctly based on the
fclk rate.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/ti-vpe/cal.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 0f90078ee8c2..d935c628597b 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -6,6 +6,7 @@
  * Benoit Parrot, <bparrot@ti.com>
  */
 
+#include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/ioctl.h>
@@ -340,6 +341,7 @@ static const struct cal_data am654_cal_data = {
  * all instances.
  */
 struct cal_dev {
+	struct clk		*fclk;
 	int			irq;
 	void __iomem		*base;
 	struct resource		*res;
@@ -767,6 +769,7 @@ static void csi2_phy_config(struct cal_ctx *ctx);
 static void csi2_phy_init(struct cal_ctx *ctx)
 {
 	u32 val;
+	u32 sscounter;
 
 	/* Steps
 	 *  1. Configure D-PHY mode and enable required lanes
@@ -803,10 +806,20 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 	csi2_phy_config(ctx);
 
 	/* 3.B. Program Stop States */
+	/*
+	 * The stop-state-counter is based on fclk cycles, and we always use
+	 * the x16 and x4 settings, so stop-state-timeout =
+	 * fclk-cycle * 16 * 4 * counter.
+	 *
+	 * Stop-state-timeout must be more than 100us as per CSI2 spec, so we
+	 * calculate a timeout that's 100us (rounding up).
+	 */
+	sscounter = DIV_ROUND_UP(clk_get_rate(ctx->dev->fclk), 10000 *  16 * 4);
+
 	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
 	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
-	set_field(&val, 0, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
-	set_field(&val, 407, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
+	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
+	set_field(&val, sscounter, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
 	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
 	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop States\n",
 		ctx->csi2_port,
@@ -2257,6 +2270,12 @@ static int cal_probe(struct platform_device *pdev)
 	/* save pdev pointer */
 	dev->pdev = pdev;
 
+	dev->fclk = devm_clk_get(&pdev->dev, "fck");
+	if (IS_ERR(dev->fclk)) {
+		dev_err(&pdev->dev, "cannot get CAL fclk\n");
+		return PTR_ERR(dev->fclk);
+	}
+
 	syscon_camerrx = syscon_regmap_lookup_by_phandle(parent,
 							 "ti,camerrx-control");
 	ret = of_property_read_u32_index(parent, "ti,camerrx-control", 1,
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 19/19] media: ti-vpe: cal: fix stop state timeout

[Benoit Parrot]

Tomi,

Thanks for the patch.

On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
> The stop-state timeout needs to be over 100us as per CSI spec. With the
> CAL fclk of 266 MHZ on DRA76, with the current value the driver uses,
> the timeout is 24us. Too small timeout will cause failure to enable the
> streaming.
> 
> Also, the fclk can be different on other SoCs, as is the case with AM65x
> where the fclk is 250 MHz.
> 
> This patch fixes the timeout by calculating it correctly based on the
> fclk rate.
> 

Isn't this in relation to the clock sourcing the PHY module which is fixed
at 96Mhz (LVDSRX_96M_GFCLK)?

Benoit

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 0f90078ee8c2..d935c628597b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -6,6 +6,7 @@
>   * Benoit Parrot, <bparrot@ti.com>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/ioctl.h>
> @@ -340,6 +341,7 @@ static const struct cal_data am654_cal_data = {
>   * all instances.
>   */
>  struct cal_dev {
> +	struct clk		*fclk;
>  	int			irq;
>  	void __iomem		*base;
>  	struct resource		*res;
> @@ -767,6 +769,7 @@ static void csi2_phy_config(struct cal_ctx *ctx);
>  static void csi2_phy_init(struct cal_ctx *ctx)
>  {
>  	u32 val;
> +	u32 sscounter;
>  
>  	/* Steps
>  	 *  1. Configure D-PHY mode and enable required lanes
> @@ -803,10 +806,20 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  	csi2_phy_config(ctx);
>  
>  	/* 3.B. Program Stop States */
> +	/*
> +	 * The stop-state-counter is based on fclk cycles, and we always use
> +	 * the x16 and x4 settings, so stop-state-timeout =
> +	 * fclk-cycle * 16 * 4 * counter.
> +	 *
> +	 * Stop-state-timeout must be more than 100us as per CSI2 spec, so we
> +	 * calculate a timeout that's 100us (rounding up).
> +	 */
> +	sscounter = DIV_ROUND_UP(clk_get_rate(ctx->dev->fclk), 10000 *  16 * 4);
> +
>  	val = reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port));
>  	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X16_IO1_MASK);
> -	set_field(&val, 0, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
> -	set_field(&val, 407, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
> +	set_field(&val, 1, CAL_CSI2_TIMING_STOP_STATE_X4_IO1_MASK);
> +	set_field(&val, sscounter, CAL_CSI2_TIMING_STOP_STATE_COUNTER_IO1_MASK);
>  	reg_write(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port), val);
>  	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop States\n",
>  		ctx->csi2_port,
> @@ -2257,6 +2270,12 @@ static int cal_probe(struct platform_device *pdev)
>  	/* save pdev pointer */
>  	dev->pdev = pdev;
>  
> +	dev->fclk = devm_clk_get(&pdev->dev, "fck");
> +	if (IS_ERR(dev->fclk)) {
> +		dev_err(&pdev->dev, "cannot get CAL fclk\n");
> +		return PTR_ERR(dev->fclk);
> +	}
> +
>  	syscon_camerrx = syscon_regmap_lookup_by_phandle(parent,
>  							 "ti,camerrx-control");
>  	ret = of_property_read_u32_index(parent, "ti,camerrx-control", 1,
> 

Re: [PATCH v2 19/19] media: ti-vpe: cal: fix stop state timeout

[Tomi Valkeinen]

On 20/03/2020 00:53, Benoit Parrot wrote:
> Tomi,
> 
> Thanks for the patch.
> 
> On 3/19/20 2:50 AM, Tomi Valkeinen wrote:
>> The stop-state timeout needs to be over 100us as per CSI spec. With the
>> CAL fclk of 266 MHZ on DRA76, with the current value the driver uses,
>> the timeout is 24us. Too small timeout will cause failure to enable the
>> streaming.
>>
>> Also, the fclk can be different on other SoCs, as is the case with AM65x
>> where the fclk is 250 MHz.
>>
>> This patch fixes the timeout by calculating it correctly based on the
>> fclk rate.
>>
> 
> Isn't this in relation to the clock sourcing the PHY module which is fixed
> at 96Mhz (LVDSRX_96M_GFCLK)?

It's not clearly said in the docs. In register descs, it's "L3 cycles", in the content it's 
"functional clock cycles" (without specifying which func clock).

As far as I see, this timeout is part of the CAL, not the PHY, so I think it's the CAL functional clock.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki