diff mbox series

[v2] rcar-csi2: Propagate the FLD signal for NTSC and PAL

Message ID 20190308235157.26357-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State New, archived
Headers show
Series [v2] rcar-csi2: Propagate the FLD signal for NTSC and PAL | expand

Commit Message

Niklas Söderlund March 8, 2019, 11:51 p.m. UTC 
Depending on which video standard is used the driver needs to setup the
hardware to correctly handle fields. If stream is identified as NTSC
or PAL setup field detection and propagate the field detection signal.

Later versions of the datasheet have been updated to make it clear
that FLD register should be set to 0 when dealing with non-interlaced
field formats.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

---

Hi,

This patch depends on [PATCH v2 0/2] rcar-csi2: Use standby mode instead of resetting

Comments

Laurent Pinchart March 11, 2019, 9:09 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Sat, Mar 09, 2019 at 12:51:57AM +0100, Niklas Söderlund wrote:
> Depending on which video standard is used the driver needs to setup the
> hardware to correctly handle fields. If stream is identified as NTSC
> or PAL setup field detection and propagate the field detection signal.
> 
> Later versions of the datasheet have been updated to make it clear
> that FLD register should be set to 0 when dealing with non-interlaced
> field formats.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> ---
> 
> Hi,
> 
> This patch depends on [PATCH v2 0/2] rcar-csi2: Use standby mode instead of resetting
> 
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 7a1c9b549e0fffc6..d9b29dbbcc2949de 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -475,7 +475,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>  	unsigned int i;
>  	int mbps, ret;
>  
> @@ -507,6 +507,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>  
> +	if (priv->mf.field != V4L2_FIELD_NONE &&

Shouldn't this be

	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {

If the CSI-2 receiver gets a top/bottom-only or sequential field order I
would expect it not to toggle the field signal.

> +	    (priv->mf.height == 240 || priv->mf.height == 288)) {

I think you can drop this part of the check.

> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> +
> +		if (priv->mf.height == 240)
> +			fld |= FLD_FLD_NUM(2);
> +		else
> +			fld |= FLD_FLD_NUM(1);

How does this work ? Looking at the datasheet, I was expecting
FLD_DET_SEL field to be set to 01 in order for the field signal to
toggle every frame.

>+	}
> +
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << priv->lanes) - 1;
>  
> @@ -519,8 +529,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	rcsi2_write(priv, PHTC_REG, 0);
>  
>  	/* Configure */
> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> +	rcsi2_write(priv, FLD_REG, fld);
>  	rcsi2_write(priv, VCDT_REG, vcdt);
>  	if (vcdt2)
>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
Niklas Söderlund March 11, 2019, 9:45 p.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2019-03-11 11:09:01 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, Mar 09, 2019 at 12:51:57AM +0100, Niklas Söderlund wrote:
> > Depending on which video standard is used the driver needs to setup the
> > hardware to correctly handle fields. If stream is identified as NTSC
> > or PAL setup field detection and propagate the field detection signal.
> > 
> > Later versions of the datasheet have been updated to make it clear
> > that FLD register should be set to 0 when dealing with non-interlaced
> > field formats.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > ---
> > 
> > Hi,
> > 
> > This patch depends on [PATCH v2 0/2] rcar-csi2: Use standby mode instead of resetting
> > 
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 7a1c9b549e0fffc6..d9b29dbbcc2949de 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -475,7 +475,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> > +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >  	unsigned int i;
> >  	int mbps, ret;
> >  
> > @@ -507,6 +507,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >  
> > +	if (priv->mf.field != V4L2_FIELD_NONE &&
> 
> Shouldn't this be
> 
> 	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> 
> If the CSI-2 receiver gets a top/bottom-only or sequential field order I
> would expect it not to toggle the field signal.

For some reason I mixed all interlaced formats in to the mix while now 
it's clear ALTERNATE is the only one which make sens, thanks!

> 
> > +	    (priv->mf.height == 240 || priv->mf.height == 288)) {
> 
> I think you can drop this part of the check.

I added it to guard so this special case only would trigger for PAL and 
NTSC resolutions. But I think I agree with you that I might be over 
cautious.

> 
> > +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> > +
> > +		if (priv->mf.height == 240)
> > +			fld |= FLD_FLD_NUM(2);
> > +		else
> > +			fld |= FLD_FLD_NUM(1);
> 
> How does this work ? Looking at the datasheet, I was expecting
> FLD_DET_SEL field to be set to 01 in order for the field signal to
> toggle every frame.

I thought so too then I read 26.4.5 FLD Signal which fits what is done 
in the BSP code and fits with how the hardware behaves.

> 
> >+	}
> > +
> >  	phycnt = PHYCNT_ENABLECLK;
> >  	phycnt |= (1 << priv->lanes) - 1;
> >  
> > @@ -519,8 +529,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	rcsi2_write(priv, PHTC_REG, 0);
> >  
> >  	/* Configure */
> > -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> > -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> > +	rcsi2_write(priv, FLD_REG, fld);
> >  	rcsi2_write(priv, VCDT_REG, vcdt);
> >  	if (vcdt2)
> >  		rcsi2_write(priv, VCDT2_REG, vcdt2);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart March 11, 2019, 10:10 p.m. UTC | #3 
Hi Niklas,

On Mon, Mar 11, 2019 at 10:45:59PM +0100, Niklas Söderlund wrote:
> On 2019-03-11 11:09:01 +0200, Laurent Pinchart wrote:
> > On Sat, Mar 09, 2019 at 12:51:57AM +0100, Niklas Söderlund wrote:
> >> Depending on which video standard is used the driver needs to setup the
> >> hardware to correctly handle fields. If stream is identified as NTSC
> >> or PAL setup field detection and propagate the field detection signal.
> >> 
> >> Later versions of the datasheet have been updated to make it clear
> >> that FLD register should be set to 0 when dealing with non-interlaced
> >> field formats.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ++++++++++++---
> >>  1 file changed, 12 insertions(+), 3 deletions(-)
> >> 
> >> ---
> >> 
> >> Hi,
> >> 
> >> This patch depends on [PATCH v2 0/2] rcar-csi2: Use standby mode instead of resetting
> >> 
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> index 7a1c9b549e0fffc6..d9b29dbbcc2949de 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -475,7 +475,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> >>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  {
> >>  	const struct rcar_csi2_format *format;
> >> -	u32 phycnt, vcdt = 0, vcdt2 = 0;
> >> +	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> >>  	unsigned int i;
> >>  	int mbps, ret;
> >>  
> >> @@ -507,6 +507,16 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >>  	}
> >>  
> >> +	if (priv->mf.field != V4L2_FIELD_NONE &&
> > 
> > Shouldn't this be
> > 
> > 	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > 
> > If the CSI-2 receiver gets a top/bottom-only or sequential field order I
> > would expect it not to toggle the field signal.
> 
> For some reason I mixed all interlaced formats in to the mix while now 
> it's clear ALTERNATE is the only one which make sens, thanks!
> 
> > 
> >> +	    (priv->mf.height == 240 || priv->mf.height == 288)) {
> > 
> > I think you can drop this part of the check.
> 
> I added it to guard so this special case only would trigger for PAL and 
> NTSC resolutions. But I think I agree with you that I might be over 
> cautious.
> 
> > 
> >> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
> >> +
> >> +		if (priv->mf.height == 240)
> >> +			fld |= FLD_FLD_NUM(2);
> >> +		else
> >> +			fld |= FLD_FLD_NUM(1);
> > 
> > How does this work ? Looking at the datasheet, I was expecting
> > FLD_DET_SEL field to be set to 01 in order for the field signal to
> > toggle every frame.
> 
> I thought so too then I read 26.4.5 FLD Signal which fits what is done 
> in the BSP code and fits with how the hardware behaves.

Do we have a guarantee that all alternate sources will cycle the frame
number between 1 and 2 ? If not I think you should select based on the
LSB.

> > >+	}
> >> +
> >>  	phycnt = PHYCNT_ENABLECLK;
> >>  	phycnt |= (1 << priv->lanes) - 1;
> >>  
> >> @@ -519,8 +529,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>  	rcsi2_write(priv, PHTC_REG, 0);
> >>  
> >>  	/* Configure */
> >> -	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> >> -		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> >> +	rcsi2_write(priv, FLD_REG, fld);
> >>  	rcsi2_write(priv, VCDT_REG, vcdt);
> >>  	if (vcdt2)
> >>  		rcsi2_write(priv, VCDT2_REG, vcdt2);
Niklas Söderlund March 12, 2019, 12:35 a.m. UTC | #4 
Hi Laurent,

On 2019-03-12 00:10:23 +0200, Laurent Pinchart wrote:
> > >> +		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | 
> > >> FLD_FLD_EN;
> > >> +
> > >> +		if (priv->mf.height == 240)
> > >> +			fld |= FLD_FLD_NUM(2);
> > >> +		else
> > >> +			fld |= FLD_FLD_NUM(1);
> > > 
> > > How does this work ? Looking at the datasheet, I was expecting
> > > FLD_DET_SEL field to be set to 01 in order for the field signal to
> > > toggle every frame.
> > 
> > I thought so too then I read 26.4.5 FLD Signal which fits what is done 
> > in the BSP code and fits with how the hardware behaves.
> 
> Do we have a guarantee that all alternate sources will cycle the frame
> number between 1 and 2 ? If not I think you should select based on the
> LSB.
> 

I can't imagine we have such guarantees and experimenting with 
FLD_DET_SEL set to 01 one works as expected. I will do so in next 
version. Thanks for finding this.
diff mbox series

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 7a1c9b549e0fffc6..d9b29dbbcc2949de 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -475,7 +475,7 @@  static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
-	u32 phycnt, vcdt = 0, vcdt2 = 0;
+	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
 	unsigned int i;
 	int mbps, ret;
 
@@ -507,6 +507,16 @@  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}
 
+	if (priv->mf.field != V4L2_FIELD_NONE &&
+	    (priv->mf.height == 240 || priv->mf.height == 288)) {
+		fld =  FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN;
+
+		if (priv->mf.height == 240)
+			fld |= FLD_FLD_NUM(2);
+		else
+			fld |= FLD_FLD_NUM(1);
+	}
+
 	phycnt = PHYCNT_ENABLECLK;
 	phycnt |= (1 << priv->lanes) - 1;
 
@@ -519,8 +529,7 @@  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	rcsi2_write(priv, PHTC_REG, 0);
 
 	/* Configure */
-	rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
-		    FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
+	rcsi2_write(priv, FLD_REG, fld);
 	rcsi2_write(priv, VCDT_REG, vcdt);
 	if (vcdt2)
 		rcsi2_write(priv, VCDT2_REG, vcdt2);