[PATCH 2/2] drm: rcar-du: lvds: fix LVDS startup on R-Car gen2

[PATCH 2/2] drm: rcar-du: lvds: fix LVDS startup on R-Car gen2

[Sergei Shtylyov]

[-- Attachment #1: drm-rcar-du-lvds-fix-LVDS-startup-on-R-Car-gen2.patch --]
[-- Type: text/plain, Size: 1728 bytes --]

According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
must be enabled and the bias crcuit  enabled after the LVDS I/O pins are
enabled, not before --  fix the gen2 LVDS startup sequence accordingly.

Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
===================================================================
--- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
+++ linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
@@ -59,11 +59,8 @@ static void rcar_du_lvdsenc_start_gen2(s
 
 	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
 
-	/*
-	 * Set the  LVDS mode, select the input, enable LVDS operation,
-	 * and turn bias circuitry on.
-	 */
-	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_BEN | LVDCR0_LVEN;
+	/* Select the input and set the LVDS mode. */
+	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
 	if (rcrtc->index == 2)
 		lvdcr0 |= LVDCR0_DUSEL;
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
@@ -73,6 +70,10 @@ static void rcar_du_lvdsenc_start_gen2(s
 			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
 			LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
 
+	/* Enable LVDS operation and turn bias circuitry on. */
+	lvdcr0 |= LVDCR0_BEN | LVDCR0_LVEN;
+	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+
 	/*
 	 * Turn the PLL on, wait for the startup delay, and turn the output
 	 * on.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: rcar-du: lvds: fix LVDS startup on R-Car gen2

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Friday, 12 January 2018 22:12:05 EET Sergei Shtylyov wrote:
> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
> must be enabled and the bias crcuit  enabled after the LVDS I/O pins are
> enabled, not before --  fix the gen2 LVDS startup sequence accordingly.
> 
> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Index: linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> ===================================================================
> --- linux.orig/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> +++ linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
> @@ -59,11 +59,8 @@ static void rcar_du_lvdsenc_start_gen2(s
> 
>  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> 
> -	/*
> -	 * Set the  LVDS mode, select the input, enable LVDS operation,
> -	 * and turn bias circuitry on.
> -	 */
> -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_BEN | LVDCR0_LVEN;
> +	/* Select the input and set the LVDS mode. */
> +	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
>  	if (rcrtc->index == 2)
>  		lvdcr0 |= LVDCR0_DUSEL;
>  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> @@ -73,6 +70,10 @@ static void rcar_du_lvdsenc_start_gen2(s
>  			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
>  			LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
> 
> +	/* Enable LVDS operation and turn bias circuitry on. */
> +	lvdcr0 |= LVDCR0_BEN | LVDCR0_LVEN;
> +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);

This change looks good, but for the reasons explained in my replies to patch 
1/2, I would set the mode here along with the BEN and LVEN bits.

>  	/*
>  	 * Turn the PLL on, wait for the startup delay, and turn the output
>  	 * on.

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

[Laurent Pinchart]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
must be enabled and the bias crcuit enabled after the LVDS I/O pins are
enabled, not before. Fix the gen2 LVDS startup sequence accordingly.

While at it, also fix the comment preceding the first LVDCR0 write that
still talks about hardcoding the LVDS mode 0.

Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
[Set the mode and input at the same time as the BEN and LVEN bits]
Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Hi Sergei,

For your convenience (and if you agree with bundling mode setup with the first
write as explained in my review of patch 1/2), here's the updated version of
patch 2/2 that I have taken in my development branch. If you're fine with it
I'll keep it, otherwise we can continue the review discussion.

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
index abbb7b25129a..b37c255c3d93 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
@@ -59,20 +59,20 @@ static void rcar_du_lvdsenc_start_gen2(struct rcar_du_lvdsenc *lvds,
 
 	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
 
+	/* Turn all the channels on. */
+	rcar_lvds_write(lvds, LVDCR1,
+			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
+			LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
+
 	/*
-	 * Select the input, hardcode mode 0, enable LVDS operation and turn
-	 * bias circuitry on.
+	 * Set the  LVDS mode, select the input, enable LVDS operation,
+	 * and turn bias circuitry on.
 	 */
 	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_BEN | LVDCR0_LVEN;
 	if (rcrtc->index == 2)
 		lvdcr0 |= LVDCR0_DUSEL;
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
 
-	/* Turn all the channels on. */
-	rcar_lvds_write(lvds, LVDCR1,
-			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
-			LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY);
-
 	/*
 	 * Turn the PLL on, wait for the startup delay, and turn the output
 	 * on.
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

[Sergei Shtylyov]

On 1/13/2018 2:10 AM, Laurent Pinchart wrote:

> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
> must be enabled and the bias crcuit enabled after the LVDS I/O pins are
> enabled, not before. Fix the gen2 LVDS startup sequence accordingly.
> 
> While at it, also fix the comment preceding the first LVDCR0 write that
> still talks about hardcoding the LVDS mode 0.

    Please do this in a separate commit then...

> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")

    You forgot to specify the other commit this one fixes -- I mean the 
comment fix.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> [Set the mode and input at the same time as the BEN and LVEN bits]
> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Hi Sergei,
> 
> For your convenience (and if you agree with bundling mode setup with the first
> write as explained in my review of patch 1/2), here's the updated version of
> patch 2/2 that I have taken in my development branch. If you're fine with it
> I'll keep it, otherwise we can continue the review discussion.

    As I said, I don't know how to interpret the note 3 in either manual.

[...]

MBR, Sergei

Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

[Laurent Pinchart]

Hi Sergei,

On Saturday, 13 January 2018 11:33:55 EET Sergei Shtylyov wrote:
> On 1/13/2018 2:10 AM, Laurent Pinchart wrote:
> > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> > According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
> > must be enabled and the bias crcuit enabled after the LVDS I/O pins are
> > enabled, not before. Fix the gen2 LVDS startup sequence accordingly.
> > 
> > While at it, also fix the comment preceding the first LVDCR0 write that
> > still talks about hardcoding the LVDS mode 0.
> 
> Please do this in a separate commit then...

The reason I added it here is that I think we don't need patch 1/2 from this 
series, and I found a bit overkill to split a comment fix to a separate patch 
when we have a patch that touches the code around the comment.

> > Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
> 
> You forgot to specify the other commit this one fixes -- I mean the comment
> fix.

Do we need to for a comment update ? It doesn't affect fix the behaviour of 
the driver or device, and I'd thus prefer to avoid giving the wrong impression 
that this patch fixes an bug introduced in a previous commit, otherwise it 
might end up being backported unnecessarily.

> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > [Set the mode and input at the same time as the BEN and LVEN bits]
> > Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >   drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > Hi Sergei,
> > 
> > For your convenience (and if you agree with bundling mode setup with the
> > first write as explained in my review of patch 1/2), here's the updated
> > version of patch 2/2 that I have taken in my development branch. If
> > you're fine with it I'll keep it, otherwise we can continue the review
> > discussion.
> 
> As I said, I don't know how to interpret the note 3 in either manual.

As explained in my latest reply to patch 1/2, my understanding is that the 
parameters can be programmed at any time before step 6. The fact that the 
current code works seems to confirm that interpretation. We could ask Renesas 
for a confirmation if you want.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

[Sergei Shtylyov]

On 01/16/2018 06:46 PM, Laurent Pinchart wrote:

>>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
>>> must be enabled and the bias crcuit enabled after the LVDS I/O pins are

    Oops, this needs fixing (note the typo!). Could you please change this 
passage to:

and the bias circuit must be enabled after the LVDS I/O pins are

?

>>> enabled, not before. Fix the gen2 LVDS startup sequence accordingly.
>>>
>>> While at it, also fix the comment preceding the first LVDCR0 write that
>>> still talks about hardcoding the LVDS mode 0.
>>
>> Please do this in a separate commit then...
> 
> The reason I added it here is that I think we don't need patch 1/2 from this
> series, and I found a bit overkill to split a comment fix to a separate patch
> when we have a patch that touches the code around the comment.

    OK, you're the maintainer of this driver, you know better. :-)

>>> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
>>
>> You forgot to specify the other commit this one fixes -- I mean the comment
>> fix.
> 
> Do we need to for a comment update ? It doesn't affect fix the behaviour of
> the driver or device, and I'd thus prefer to avoid giving the wrong impression
> that this patch fixes an bug introduced in a previous commit, otherwise it
> might end up being backported unnecessarily.

    OK, no dire need to backport indeed...

>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> [Set the mode and input at the same time as the BEN and LVEN bits]
>>> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>>
>>>    drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
>>>    1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> Hi Sergei,
>>>
>>> For your convenience (and if you agree with bundling mode setup with the
>>> first write as explained in my review of patch 1/2), here's the updated
>>> version of patch 2/2 that I have taken in my development branch. If
>>> you're fine with it I'll keep it, otherwise we can continue the review
>>> discussion.
>>
>> As I said, I don't know how to interpret the note 3 in either manual.

    Moreover, it seems to me that the notes don't match the start-up procedure 
anymore...

> As explained in my latest reply to patch 1/2, my understanding is that the
> parameters can be programmed at any time before step 6. The fact that the
> current code works seems to confirm that interpretation. We could ask Renesas
> for a confirmation if you want.

    Would be good to ask, indeed.

MBR, Sergei
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2

[Laurent Pinchart]

Hi Sergei,

On Tuesday, 16 January 2018 22:17:31 EET Sergei Shtylyov wrote:
> On 01/16/2018 06:46 PM, Laurent Pinchart wrote:
> >>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> 
> >>> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS
> >>> must be enabled and the bias crcuit enabled after the LVDS I/O pins are
> 
> Oops, this needs fixing (note the typo!). Could you please change this
> passage to:
> 
> and the bias circuit must be enabled after the LVDS I/O pins are
> 
> ?

Sure I'll fix that.

> >>> enabled, not before. Fix the gen2 LVDS startup sequence accordingly.
> >>> 
> >>> While at it, also fix the comment preceding the first LVDCR0 write that
> >>> still talks about hardcoding the LVDS mode 0.
> >> 
> >> Please do this in a separate commit then...
> > 
> > The reason I added it here is that I think we don't need patch 1/2 from
> > this series, and I found a bit overkill to split a comment fix to a
> > separate patch when we have a patch that touches the code around the
> > comment.
> 
> OK, you're the maintainer of this driver, you know better. :-)
> 
> >>> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")
> >> 
> >> You forgot to specify the other commit this one fixes -- I mean the
> >> comment fix.
> > 
> > Do we need to for a comment update ? It doesn't affect fix the behaviour
> > of the driver or device, and I'd thus prefer to avoid giving the wrong
> > impression that this patch fixes an bug introduced in a previous commit,
> > otherwise it might end up being backported unnecessarily.
> 
>     OK, no dire need to backport indeed...
> 
> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> Reviewed-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> [Set the mode and input at the same time as the BEN and LVEN bits]
> >>> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>    drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++-------
> >>>    1 file changed, 7 insertions(+), 7 deletions(-)
> >>> 
> >>> Hi Sergei,
> >>> 
> >>> For your convenience (and if you agree with bundling mode setup with the
> >>> first write as explained in my review of patch 1/2), here's the updated
> >>> version of patch 2/2 that I have taken in my development branch. If
> >>> you're fine with it I'll keep it, otherwise we can continue the review
> >>> discussion.
> >> 
> >> As I said, I don't know how to interpret the note 3 in either manual.
> 
> Moreover, it seems to me that the notes don't match the start-up procedure
> anymore...

How so ?

> > As explained in my latest reply to patch 1/2, my understanding is that the
> > parameters can be programmed at any time before step 6. The fact that the
> > current code works seems to confirm that interpretation. We could ask
> > Renesas for a confirmation if you want.
> 
> Would be good to ask, indeed.

I'll ask.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel