[PATCH 01/16] media: ti-vpe: cal: fix use of wrong macro

[PATCH 01/16] 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>
---
 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 e44b34dfac1a..4b584c419e98 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

[PATCH 02/16] 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>
---
 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 4b584c419e98..b4a9f4d16ce4 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)
@@ -2397,11 +2384,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 eveytime 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 02/16] media: ti-vpe: cal: use runtime_resume for errata handling

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:07PM +0200, 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>
> ---
>  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 4b584c419e98..b4a9f4d16ce4 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)
> @@ -2397,11 +2384,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 eveytime we (re-)enable

s/eveytime/everytime/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Now that cal_runtime_get() and cal_runtime_put() are just wrappers
around pm_runtime_get_sync() and pm_runtime_put_sync(), how about
dropping the wrappers ?

> +		 * 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),
>  	},
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/16] media: ti-vpe: cal: use runtime_resume for errata handling

[Tomi Valkeinen]

On 16/03/2020 14:28, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:07PM +0200, 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>
>> ---
>>   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 4b584c419e98..b4a9f4d16ce4 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)
>> @@ -2397,11 +2384,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 eveytime we (re-)enable
> 
> s/eveytime/everytime/
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Now that cal_runtime_get() and cal_runtime_put() are just wrappers
> around pm_runtime_get_sync() and pm_runtime_put_sync(), how about
> dropping the wrappers ?

Yep, I'll add a patch for that.

  Tomi

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

[PATCH 03/16] 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>
---
 drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
 drivers/media/platform/ti-vpe/cal_regs.h |  3 ++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index b4a9f4d16ce4..f6ce0558752a 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -692,6 +692,21 @@ static void cal_quickdump_regs(struct cal_dev *dev)
  */
 static void enable_irqs(struct cal_ctx *ctx)
 {
+	const u32 cio_err_mask =
+		((1 << 20) - 1) |	/* lane errors */
+		BIT(27) |		/* FIFO_OVR */
+		BIT(28) |		/* SHORT_PACKET */
+		BIT(30);		/* ECC_NO_CORRECTION */
+
+	/* 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 OCP error */
+	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), BIT(6));
+
 	/* Enable IRQ_WDMA_END 0/1 */
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
 	/* Enable IRQ_WDMA_START 0/1 */
@@ -702,6 +717,12 @@ static void enable_irqs(struct cal_ctx *ctx)
 
 static void disable_irqs(struct cal_ctx *ctx)
 {
+	/* 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 */
 	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
 	/* Disable IRQ_WDMA_START 0/1 */
@@ -1169,7 +1190,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 & BIT(6))
+			dev_err_ratelimited(&dev->pdev->dev, "OCP 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..a29198cc3efe 100644
--- a/drivers/media/platform/ti-vpe/cal_regs.h
+++ b/drivers/media/platform/ti-vpe/cal_regs.h
@@ -158,6 +158,9 @@
 #define CAL_HL_IRQ_ENABLED				0x1
 #define CAL_HL_IRQ_PENDING				0x1
 
+#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
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Hans Verkuil]

Hi Tomi,

On 3/13/20 12:41 PM, 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.

Can you rebase your series to the media_tree master branch? Other recently
merged patches from Benoit now conflict with at least this patch.

I reviewed this series and it looks good otherwise (just one other small comment
about a confusing log message), so once I have a rebased version I can make
a PR for it.

Regards,

	Hans

> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
>  drivers/media/platform/ti-vpe/cal_regs.h |  3 ++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index b4a9f4d16ce4..f6ce0558752a 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -692,6 +692,21 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>   */
>  static void enable_irqs(struct cal_ctx *ctx)
>  {
> +	const u32 cio_err_mask =
> +		((1 << 20) - 1) |	/* lane errors */
> +		BIT(27) |		/* FIFO_OVR */
> +		BIT(28) |		/* SHORT_PACKET */
> +		BIT(30);		/* ECC_NO_CORRECTION */
> +
> +	/* 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 OCP error */
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), BIT(6));
> +
>  	/* Enable IRQ_WDMA_END 0/1 */
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
>  	/* Enable IRQ_WDMA_START 0/1 */
> @@ -702,6 +717,12 @@ static void enable_irqs(struct cal_ctx *ctx)
>  
>  static void disable_irqs(struct cal_ctx *ctx)
>  {
> +	/* 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 */
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
>  	/* Disable IRQ_WDMA_START 0/1 */
> @@ -1169,7 +1190,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 & BIT(6))
> +			dev_err_ratelimited(&dev->pdev->dev, "OCP 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..a29198cc3efe 100644
> --- a/drivers/media/platform/ti-vpe/cal_regs.h
> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
> @@ -158,6 +158,9 @@
>  #define CAL_HL_IRQ_ENABLED				0x1
>  #define CAL_HL_IRQ_PENDING				0x1
>  
> +#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
> 

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Hans Verkuil]

On 3/16/20 11:06 AM, Hans Verkuil wrote:
> Hi Tomi,
> 
> On 3/13/20 12:41 PM, 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.
> 
> Can you rebase your series to the media_tree master branch? Other recently
> merged patches from Benoit now conflict with at least this patch.
> 
> I reviewed this series and it looks good otherwise (just one other small comment
> about a confusing log message), so once I have a rebased version I can make
> a PR for it.

Just to confirm: this series has been tested with a real sensor, right? If so,
please add a Tested-by line as well.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
>>  drivers/media/platform/ti-vpe/cal_regs.h |  3 ++
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index b4a9f4d16ce4..f6ce0558752a 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -692,6 +692,21 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>>   */
>>  static void enable_irqs(struct cal_ctx *ctx)
>>  {
>> +	const u32 cio_err_mask =
>> +		((1 << 20) - 1) |	/* lane errors */
>> +		BIT(27) |		/* FIFO_OVR */
>> +		BIT(28) |		/* SHORT_PACKET */
>> +		BIT(30);		/* ECC_NO_CORRECTION */
>> +
>> +	/* 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 OCP error */
>> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), BIT(6));
>> +
>>  	/* Enable IRQ_WDMA_END 0/1 */
>>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
>>  	/* Enable IRQ_WDMA_START 0/1 */
>> @@ -702,6 +717,12 @@ static void enable_irqs(struct cal_ctx *ctx)
>>  
>>  static void disable_irqs(struct cal_ctx *ctx)
>>  {
>> +	/* 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 */
>>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
>>  	/* Disable IRQ_WDMA_START 0/1 */
>> @@ -1169,7 +1190,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 & BIT(6))
>> +			dev_err_ratelimited(&dev->pdev->dev, "OCP 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..a29198cc3efe 100644
>> --- a/drivers/media/platform/ti-vpe/cal_regs.h
>> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
>> @@ -158,6 +158,9 @@
>>  #define CAL_HL_IRQ_ENABLED				0x1
>>  #define CAL_HL_IRQ_PENDING				0x1
>>  
>> +#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
>>
> 

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Tomi Valkeinen]

On 16/03/2020 12:51, Hans Verkuil wrote:
> On 3/16/20 11:06 AM, Hans Verkuil wrote:
>> Hi Tomi,
>>
>> On 3/13/20 12:41 PM, 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.
>>
>> Can you rebase your series to the media_tree master branch? Other recently
>> merged patches from Benoit now conflict with at least this patch.
>>
>> I reviewed this series and it looks good otherwise (just one other small comment
>> about a confusing log message), so once I have a rebased version I can make
>> a PR for it.
> 
> Just to confirm: this series has been tested with a real sensor, right? If so,
> please add a Tested-by line as well.

Yes, I'm using OV5640.

I thought tested-by by the author is implicit, unless otherwise clearly noted. But if it's the 
custom with linux-media, I can add those.

  Tomi

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

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Hans Verkuil]

On 3/16/20 1:00 PM, Tomi Valkeinen wrote:
> On 16/03/2020 12:51, Hans Verkuil wrote:
>> On 3/16/20 11:06 AM, Hans Verkuil wrote:
>>> Hi Tomi,
>>>
>>> On 3/13/20 12:41 PM, 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.
>>>
>>> Can you rebase your series to the media_tree master branch? Other recently
>>> merged patches from Benoit now conflict with at least this patch.
>>>
>>> I reviewed this series and it looks good otherwise (just one other small comment
>>> about a confusing log message), so once I have a rebased version I can make
>>> a PR for it.
>>
>> Just to confirm: this series has been tested with a real sensor, right? If so,
>> please add a Tested-by line as well.
> 
> Yes, I'm using OV5640.
> 
> I thought tested-by by the author is implicit, unless otherwise clearly noted. But if it's the 
> custom with linux-media, I can add those.

It's not custom, but this series has a lot of low-level changes and so it's good
to have an explicit confirmation that this has been tested.

I wouldn't have asked if a v2 wasn't needed anyway.

Regards,

	Hans

> 
>   Tomi
> 

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:08PM +0200, 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>
> ---
>  drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
>  drivers/media/platform/ti-vpe/cal_regs.h |  3 ++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index b4a9f4d16ce4..f6ce0558752a 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -692,6 +692,21 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>   */
>  static void enable_irqs(struct cal_ctx *ctx)
>  {
> +	const u32 cio_err_mask =
> +		((1 << 20) - 1) |	/* lane errors */
> +		BIT(27) |		/* FIFO_OVR */
> +		BIT(28) |		/* SHORT_PACKET */
> +		BIT(30);		/* ECC_NO_CORRECTION */

Could you create macros for those bits ?

> +
> +	/* 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 OCP error */
> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), BIT(6));

And for this bit too.

> +
>  	/* Enable IRQ_WDMA_END 0/1 */
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
>  	/* Enable IRQ_WDMA_START 0/1 */
> @@ -702,6 +717,12 @@ static void enable_irqs(struct cal_ctx *ctx)
>  
>  static void disable_irqs(struct cal_ctx *ctx)
>  {
> +	/* 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 */
>  	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
>  	/* Disable IRQ_WDMA_START 0/1 */
> @@ -1169,7 +1190,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 & BIT(6))
> +			dev_err_ratelimited(&dev->pdev->dev, "OCP 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..a29198cc3efe 100644
> --- a/drivers/media/platform/ti-vpe/cal_regs.h
> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
> @@ -158,6 +158,9 @@
>  #define CAL_HL_IRQ_ENABLED				0x1
>  #define CAL_HL_IRQ_PENDING				0x1
>  
> +#define CAL_HL_IRQ_CIO_MASK(i)			BIT(16 + (i-1) * 8)

This should be

	BIT(16 + ((i)-1) * 8)

> +#define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + (i-1) * 8)

Same here.

> +
>  #define CAL_PIX_PROC_EN_MASK			BIT(0)
>  #define CAL_PIX_PROC_EXTRACT_MASK		GENMASK(4, 1)
>  #define CAL_PIX_PROC_EXTRACT_B6				0x0

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/16] media: ti-vpe: cal: catch error irqs and print errors

[Tomi Valkeinen]

On 16/03/2020 14:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:08PM +0200, 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>
>> ---
>>   drivers/media/platform/ti-vpe/cal.c      | 46 +++++++++++++++++++++++-
>>   drivers/media/platform/ti-vpe/cal_regs.h |  3 ++
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index b4a9f4d16ce4..f6ce0558752a 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -692,6 +692,21 @@ static void cal_quickdump_regs(struct cal_dev *dev)
>>    */
>>   static void enable_irqs(struct cal_ctx *ctx)
>>   {
>> +	const u32 cio_err_mask =
>> +		((1 << 20) - 1) |	/* lane errors */
>> +		BIT(27) |		/* FIFO_OVR */
>> +		BIT(28) |		/* SHORT_PACKET */
>> +		BIT(30);		/* ECC_NO_CORRECTION */
> 
> Could you create macros for those bits ?
> 
>> +
>> +	/* 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 OCP error */
>> +	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(1), BIT(6));
> 
> And for this bit too.

Yep, will do.

>> +
>>   	/* Enable IRQ_WDMA_END 0/1 */
>>   	reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1));
>>   	/* Enable IRQ_WDMA_START 0/1 */
>> @@ -702,6 +717,12 @@ static void enable_irqs(struct cal_ctx *ctx)
>>   
>>   static void disable_irqs(struct cal_ctx *ctx)
>>   {
>> +	/* 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 */
>>   	reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1));
>>   	/* Disable IRQ_WDMA_START 0/1 */
>> @@ -1169,7 +1190,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 & BIT(6))
>> +			dev_err_ratelimited(&dev->pdev->dev, "OCP 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..a29198cc3efe 100644
>> --- a/drivers/media/platform/ti-vpe/cal_regs.h
>> +++ b/drivers/media/platform/ti-vpe/cal_regs.h
>> @@ -158,6 +158,9 @@
>>   #define CAL_HL_IRQ_ENABLED				0x1
>>   #define CAL_HL_IRQ_PENDING				0x1
>>   
>> +#define CAL_HL_IRQ_CIO_MASK(i)			BIT(16 + (i-1) * 8)
> 
> This should be
> 
> 	BIT(16 + ((i)-1) * 8)
> 
>> +#define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + (i-1) * 8)
> 
> Same here.

Right. Thanks!

  Tomi

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

[PATCH 04/16] 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>
---
 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 f6ce0558752a..1499d443f414 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -842,6 +842,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,
@@ -855,6 +860,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

[PATCH 05/16] 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>
---
 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 1499d443f414..0888d6aac3f4 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1226,64 +1226,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

[PATCH 06/16] 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>
---
 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 0888d6aac3f4..cd788c6687cb 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -775,10 +775,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",
@@ -787,8 +785,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,
@@ -851,8 +848,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);
 	}
@@ -949,13 +945,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)
@@ -1030,7 +1026,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)));
@@ -1050,7 +1046,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 a29198cc3efe..532d4a95740a 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 06/16] media: ti-vpe: cal: remove useless CAL_GEN_* macros

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:11PM +0200, Tomi Valkeinen wrote:
> These macros only obfuscate the code, so drop them.
> 
> Signed-off-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 0888d6aac3f4..cd788c6687cb 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -775,10 +775,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",
> @@ -787,8 +785,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,
> @@ -851,8 +848,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);
>  	}
> @@ -949,13 +945,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)
> @@ -1030,7 +1026,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)));
> @@ -1050,7 +1046,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 a29198cc3efe..532d4a95740a 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
>  *********************************************************************/

-- 
Regards,

Laurent Pinchart

[PATCH 07/16] media: ti-vpe: cal: remove unused defines

[Tomi Valkeinen]

Remove a bunch of IRQ defines that are never used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/platform/ti-vpe/cal_regs.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
index 532d4a95740a..b27c0445b8d5 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_CIO_MASK(i)			BIT(16 + (i-1) * 8)
 #define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + (i-1) * 8)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 07/16] media: ti-vpe: cal: remove unused defines

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:12PM +0200, Tomi Valkeinen wrote:
> Remove a bunch of IRQ defines that are never used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/ti-vpe/cal_regs.h | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h
> index 532d4a95740a..b27c0445b8d5 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_CIO_MASK(i)			BIT(16 + (i-1) * 8)
>  #define CAL_HL_IRQ_VC_MASK(i)			BIT(17 + (i-1) * 8)

-- 
Regards,

Laurent Pinchart

[PATCH 08/16] 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>
---
 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 cd788c6687cb..ebea5fa28691 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -759,10 +759,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)));
@@ -784,18 +783,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++) {
@@ -867,13 +864,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++) {
@@ -890,10 +885,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 08/16] media: ti-vpe: cal: use reg_write_field

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:13PM +0200, Tomi Valkeinen wrote:
> Simplify the code by using reg_write_field() where trivially possible.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I wonder if it would make sense to use regmap.

> ---
>  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 cd788c6687cb..ebea5fa28691 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -759,10 +759,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)));
> @@ -784,18 +783,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++) {
> @@ -867,13 +864,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++) {
> @@ -890,10 +885,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++) {

-- 
Regards,

Laurent Pinchart

[PATCH 09/16] 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>
---
 drivers/media/platform/ti-vpe/cal.c | 68 ++++++++++++++---------------
 1 file changed, 32 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index ebea5fa28691..771ad7c14c96 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -731,11 +731,40 @@ static void disable_irqs(struct cal_ctx *ctx)
 	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
 }
 
+static void csi2_cio_pwr(struct cal_ctx *ctx, bool enable)
+{
+	u32 target_state;
+	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
@@ -790,23 +819,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_pwr(ctx, true);
 }
 
 static void csi2_wait_for_phy(struct cal_ctx *ctx)
@@ -865,24 +878,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_pwr(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 09/16] media: ti-vpe: cal: cleanup CIO power enable/disable

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:14PM +0200, 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/ti-vpe/cal.c | 68 ++++++++++++++---------------
>  1 file changed, 32 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index ebea5fa28691..771ad7c14c96 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -731,11 +731,40 @@ static void disable_irqs(struct cal_ctx *ctx)
>  	reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);
>  }
>  
> +static void csi2_cio_pwr(struct cal_ctx *ctx, bool enable)

I think you can spell it out fully to csi2_cio_power.

> +{
> +	u32 target_state;
> +	int i;

i is never negative, it can be an unsued int.

> +
> +	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);

These two liens would hold on a single line.

> +
> +	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
> @@ -790,23 +819,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_pwr(ctx, true);
>  }
>  
>  static void csi2_wait_for_phy(struct cal_ctx *ctx)
> @@ -865,24 +878,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_pwr(ctx, false);
>  
>  	/* Assert Comple IO Reset */
>  	reg_write_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),

-- 
Regards,

Laurent Pinchart

[PATCH 10/16] 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>
---
 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 771ad7c14c96..b5fd90a1ec09 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -795,8 +795,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 10/16] media: ti-vpe: cal: fix dummy read to phy

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:15PM +0200, 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>

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 771ad7c14c96..b5fd90a1ec09 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -795,8 +795,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);

-- 
Regards,

Laurent Pinchart

[PATCH 11/16] 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>
---
 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 b5fd90a1ec09..832f37ebad0d 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -961,8 +961,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 11/16] media: ti-vpe: cal: program number of lines properly

[Laurent Pinchart]

On Fri, Mar 13, 2020 at 01:41:16PM +0200, 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".

And possibly buffer overflows !

> Signed-off-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 b5fd90a1ec09..832f37ebad0d 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -961,8 +961,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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 11/16] media: ti-vpe: cal: program number of lines properly

[Tomi Valkeinen]

On 16/03/2020 14:37, Laurent Pinchart wrote:
> On Fri, Mar 13, 2020 at 01:41:16PM +0200, 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".
> 
> And possibly buffer overflows !

There's a register in the DMA block which defines the max number of lines the DMA will transfer. So 
overflow should not be possible even without this patch.

  Tomi

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

[PATCH 12/16] 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>
---
 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 832f37ebad0d..64ea92dbfac5 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -2321,6 +2321,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 = cal_runtime_get(dev);
@@ -2335,6 +2337,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];
@@ -2377,6 +2381,8 @@ static int cal_remove(struct platform_device *pdev)
 	cal_runtime_put(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 12/16] media: ti-vpe: cal: set DMA max seg size

[Laurent Pinchart]

On Fri, Mar 13, 2020 at 01:41:17PM +0200, 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>
> ---
>  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 832f37ebad0d..64ea92dbfac5 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -2321,6 +2321,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 = cal_runtime_get(dev);
> @@ -2335,6 +2337,8 @@ static int cal_probe(struct platform_device *pdev)
>  	return 0;
>  
>  runtime_disable:
> +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> +

It's probably time to fix this horrible DMA API that requires such
cleanups, but that's out of scope for this patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	pm_runtime_disable(&pdev->dev);
>  	for (i = 0; i < CAL_NUM_CONTEXT; i++) {
>  		ctx = dev->ctx[i];
> @@ -2377,6 +2381,8 @@ static int cal_remove(struct platform_device *pdev)
>  	cal_runtime_put(dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH 13/16] 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>
---
 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 64ea92dbfac5..319312770eea 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -822,20 +822,10 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 	csi2_cio_pwr(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),
@@ -854,7 +844,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),
@@ -866,9 +861,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 13/16] media: ti-vpe: cal: move code to separate functions

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:18PM +0200, 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>
> ---
>  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 64ea92dbfac5..319312770eea 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -822,20 +822,10 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  	csi2_cio_pwr(ctx, true);
>  }
>  
> -static void csi2_wait_for_phy(struct cal_ctx *ctx)
> +static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>  {
>  	int i;

unsigned int ?

>  
> -	/* 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),
> @@ -854,7 +844,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;

Ditto.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	for (i = 0; i < 10; i++) {
>  		if (reg_read_field(ctx->dev,
>  				   CAL_CSI2_TIMING(ctx->csi2_port),
> @@ -866,9 +861,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));

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

On 16/03/2020 14:41, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:18PM +0200, 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>
>> ---
>>   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 64ea92dbfac5..319312770eea 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -822,20 +822,10 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>>   	csi2_cio_pwr(ctx, true);
>>   }
>>   
>> -static void csi2_wait_for_phy(struct cal_ctx *ctx)
>> +static void csi2_wait_complexio_reset(struct cal_ctx *ctx)
>>   {
>>   	int i;
> 
> unsigned int ?
> 
>>   
>> -	/* 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),
>> @@ -854,7 +844,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;
> 
> Ditto.

I'll leave these, as they were "int" also in the original code, and I remove these in the next patches.

  Tomi

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

[PATCH 14/16] media: ti-vpe: cal: improve wait for CIO resetdone

[Tomi Valkeinen]

Sometimes waiting for ComplexIO resetdone timeouts. 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>
---
 drivers/media/platform/ti-vpe/cal.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 319312770eea..929f9b3ca4f9 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -824,20 +824,21 @@ 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_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done\n",
 		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
-		(i >= 250) ? "(timeout)" : "");
+		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
 
 	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 14/16] media: ti-vpe: cal: improve wait for CIO resetdone

[Hans Verkuil]

On 3/13/20 12:41 PM, Tomi Valkeinen wrote:
> Sometimes waiting for ComplexIO resetdone timeouts. 

This sentence is hard to read. You probably mean:

Sometimes there is a timeout when waiting for the 'ComplexIO Reset Done'.

Regards,

	Hans

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>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 319312770eea..929f9b3ca4f9 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -824,20 +824,21 @@ 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_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done\n",
>  		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
> -		(i >= 250) ? "(timeout)" : "");
> +		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
>  			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=
> 

Re: [PATCH 14/16] media: ti-vpe: cal: improve wait for CIO resetdone

[Tomi Valkeinen]

On 16/03/2020 12:05, Hans Verkuil wrote:
> On 3/13/20 12:41 PM, Tomi Valkeinen wrote:
>> Sometimes waiting for ComplexIO resetdone timeouts.
> 
> This sentence is hard to read. You probably mean:
> 
> Sometimes there is a timeout when waiting for the 'ComplexIO Reset Done'.

Ah, indeed, it's confusing. And same with the next patch. I'll update the desc.

  Tomi

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

Re: [PATCH 14/16] media: ti-vpe: cal: improve wait for CIO resetdone

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:19PM +0200, Tomi Valkeinen wrote:
> Sometimes waiting for ComplexIO resetdone timeouts. 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>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 319312770eea..929f9b3ca4f9 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -824,20 +824,21 @@ 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);
>  	}

How about using readx_poll_timeout() ?

> -	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done (%d) %s\n",
> +
> +	ctx_dbg(3, ctx, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO Reset Done\n",
>  		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)), i,
> -		(i >= 250) ? "(timeout)" : "");
> +		reg_read(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port)));
>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_COMPLEXIO_CFG(ctx->csi2_port),
>  			   CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) !=

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/16] media: ti-vpe: cal: improve wait for CIO resetdone

[Tomi Valkeinen]

On 16/03/2020 14:43, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:19PM +0200, Tomi Valkeinen wrote:
>> Sometimes waiting for ComplexIO resetdone timeouts. 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>
>> ---
>>   drivers/media/platform/ti-vpe/cal.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index 319312770eea..929f9b3ca4f9 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -824,20 +824,21 @@ 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);
>>   	}
> 
> How about using readx_poll_timeout() ?

There's no function that directly fits readx_poll_timeout's accessor function, so I think using 
readl_poll_timeout and calculating the address manually would be the way to use *_poll_timeout.

But it does skip the register access functions the driver uses (reg_read/write).

So... It would be nice to use *_poll_timeout, but I'm not sure if it worth breaking the register 
access model the driver uses.

  Tomi

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

[PATCH 15/16] media: ti-vpe: cal: improve wait for stop-state

[Tomi Valkeinen]

Sometimes waiting for stop-state timeouts. 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>
---
 drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 929f9b3ca4f9..df5a4281838b 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -849,19 +849,19 @@ 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_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached\n",
 		ctx->csi2_port,
-		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
-		(i >= 10) ? "(timeout)" : "");
+		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
 
 	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 15/16] media: ti-vpe: cal: improve wait for stop-state

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:20PM +0200, Tomi Valkeinen wrote:
> Sometimes waiting for stop-state timeouts. 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>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 929f9b3ca4f9..df5a4281838b 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -849,19 +849,19 @@ 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);
>  	}

Same here, readx_poll_timeout() ?

> -	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
> +	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached\n",
>  		ctx->csi2_port,
> -		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
> -		(i >= 10) ? "(timeout)" : "");
> +		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));

That's not correct anymore, if the condition is false then the stop
state hasn't been reached.

Is this debug statement really useful ?

>  
>  	if (reg_read_field(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port),
>  			   CAL_CSI2_TIMING_FORCE_RX_MODE_IO1_MASK) != 0)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 15/16] media: ti-vpe: cal: improve wait for stop-state

[Tomi Valkeinen]

On 16/03/2020 14:45, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Mar 13, 2020 at 01:41:20PM +0200, Tomi Valkeinen wrote:
>> Sometimes waiting for stop-state timeouts. 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>
>> ---
>>   drivers/media/platform/ti-vpe/cal.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>> index 929f9b3ca4f9..df5a4281838b 100644
>> --- a/drivers/media/platform/ti-vpe/cal.c
>> +++ b/drivers/media/platform/ti-vpe/cal.c
>> @@ -849,19 +849,19 @@ 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);
>>   	}
> 
> Same here, readx_poll_timeout() ?
> 
>> -	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached %s\n",
>> +	ctx_dbg(3, ctx, "CAL_CSI2_TIMING(%d) = 0x%08x Stop State Reached\n",
>>   		ctx->csi2_port,
>> -		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)),
>> -		(i >= 10) ? "(timeout)" : "");
>> +		reg_read(ctx->dev, CAL_CSI2_TIMING(ctx->csi2_port)));
> 
> That's not correct anymore, if the condition is false then the stop
> state hasn't been reached.

It wasn't exactly correct earlier either, as it always said "stop state reached", although it had 
"(timeout)" too.

> Is this debug statement really useful ?

I did not find any of these debug prints useful in my debugging, but as this is not "my" driver, I 
didn't want to drop them.

But I'll drop the debug print here and in the previous patch. Benoit can comment if he wants them back.

  Tomi

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

[PATCH 16/16] 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>
---
 drivers/media/platform/ti-vpe/cal.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index df5a4281838b..e9dd405b8eb1 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;
@@ -766,6 +768,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
@@ -802,10 +805,13 @@ static void csi2_phy_init(struct cal_ctx *ctx)
 	csi2_phy_config(ctx);
 
 	/* 3.B. Program Stop States */
+	/* Must be more than 100us */
+	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,
@@ -2263,6 +2269,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 16/16] media: ti-vpe: cal: fix stop state timeout

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:21PM +0200, 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.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index df5a4281838b..e9dd405b8eb1 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;
> @@ -766,6 +768,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
> @@ -802,10 +805,13 @@ static void csi2_phy_init(struct cal_ctx *ctx)
>  	csi2_phy_config(ctx);
>  
>  	/* 3.B. Program Stop States */
> +	/* Must be more than 100us */

You may want to expand this comment to explain the calculation
(especially the * 16 * 4).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	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,
> @@ -2263,6 +2269,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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/16] media: ti-vpe: cal: fix use of wrong macro

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 01:41:06PM +0200, 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>

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 e44b34dfac1a..4b584c419e98 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);

-- 
Regards,

Laurent Pinchart