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

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

[Sergei Shtylyov]

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

According to the latest revisions of the R-Car gen3 manual, the LVDS mode
must be set before the LVDS I/O pins are enabled, not after --  fix  the
gen3 LVDS startup sequence accordingly...

While  at it,  also fix the comment  preceding the first LVDCR0 write in
the R-Car gen2 startup code that still talks about hardcoding the LVDS
mode 0...

Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 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
@@ -60,8 +60,8 @@ static void rcar_du_lvdsenc_start_gen2(s
 	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
 
 	/*
-	 * 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)
@@ -106,6 +106,9 @@ static void rcar_du_lvdsenc_start_gen3(s
 
 	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
 
+	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
+	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
+
 	/* Turn all the channels on. */
 	rcar_lvds_write(lvds, LVDCR1,
 			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
@@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
 	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
 	 * delay and turn the output on.
 	 */
-	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
+
+	lvdcr0 = | LVDCR0_PLLON;
 	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
 
 	lvdcr0 |= LVDCR0_PWD;

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

[Sergei Shtylyov]

On 01/12/2018 11:12 PM, Sergei Shtylyov wrote:

> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> must be set before the LVDS I/O pins are enabled, not after --  fix  the
> gen3 LVDS startup sequence accordingly...
> 
> While  at it,  also fix the comment  preceding the first LVDCR0 write in
> the R-Car gen2 startup code that still talks about hardcoding the LVDS
> mode 0...
> 
> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 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
[...]
> @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
>  	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
>  	 * delay and turn the output on.
>  	 */
> -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> +
> +	lvdcr0 = | LVDCR0_PLLON;

    Dunno how it slipped in -- I've surely built and tested the resulting 
kernel... should be |=, of course. Laurent, should I repost?

[...]

MBR, Sergei

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

[Sergei Shtylyov]

On 01/12/2018 11:12 PM, Sergei Shtylyov wrote:

> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> must be set before the LVDS I/O pins are enabled, not after --  fix  the
> gen3 LVDS startup sequence accordingly...
> 
> While  at it,  also fix the comment  preceding the first LVDCR0 write in
> the R-Car gen2 startup code that still talks about hardcoding the LVDS
> mode 0...
> 
> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 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
[...]
> @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
>  	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
>  	 * delay and turn the output on.
>  	 */
> -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> +
> +	lvdcr0 = | LVDCR0_PLLON;

    Dunno how it slipped in -- I've surely built and tested the resulting 
kernel... should be |=, of course. Laurent, should I repost?

[...]

MBR, Sergei


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

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

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Friday, 12 January 2018 22:12:04 EET Sergei Shtylyov wrote:
> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> must be set before the LVDS I/O pins are enabled, not after --  fix  the
> gen3 LVDS startup sequence accordingly...
> 
> While  at it,  also fix the comment  preceding the first LVDCR0 write in
> the R-Car gen2 startup code that still talks about hardcoding the LVDS
> mode 0...

How about fixing that in patch 2/2 that touches the Gen2 initialization 
sequence ? I think I'd even go as far as squashing both patches, I don't think 
there's a need to split them.

> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Is this really needed ? Does it fix a problem you've experienced, or is it 
theoretical only ? The mode shouldn't matter before the LVDS internal logic is 
turned on. Unless there's a real issue I'm not sure we should make the code 
more complex.

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 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
> @@ -60,8 +60,8 @@ static void rcar_du_lvdsenc_start_gen2(s
>  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> 
>  	/*
> -	 * 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)
> @@ -106,6 +106,9 @@ static void rcar_du_lvdsenc_start_gen3(s
> 
>  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> 
> +	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> +
>  	/* Turn all the channels on. */
>  	rcar_lvds_write(lvds, LVDCR1,
>  			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
>  	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
>  	 * delay and turn the output on.
>  	 */
> -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> +
> +	lvdcr0 = | LVDCR0_PLLON;
>  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> 
>  	lvdcr0 |= LVDCR0_PWD;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sergei,

On Friday, 12 January 2018 23:51:35 EET Laurent Pinchart wrote:
> On Friday, 12 January 2018 22:12:04 EET Sergei Shtylyov wrote:
> > According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> > must be set before the LVDS I/O pins are enabled, not after --  fix  the
> > gen3 LVDS startup sequence accordingly...
> > 
> > While  at it,  also fix the comment  preceding the first LVDCR0 write in
> > the R-Car gen2 startup code that still talks about hardcoding the LVDS
> > mode 0...
> 
> How about fixing that in patch 2/2 that touches the Gen2 initialization
> sequence ? I think I'd even go as far as squashing both patches, I don't
> think there's a need to split them.
> 
> > Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Is this really needed ? Does it fix a problem you've experienced, or is it
> theoretical only ? The mode shouldn't matter before the LVDS internal logic
> is turned on. Unless there's a real issue I'm not sure we should make the
> code more complex.

Furthermore the datasheet states

"3. This refers to settings other than those that are concerned with LVDS-IF 
startup. These items may be set while waiting for the conditions of step 6 to 
be met."

Doesn't this mean that the mode and input selector don't have to be set as the 
very first step, but can be programmed at any point before starting the LVDS 
encoder through the PWD bit (on Gen3) or the PLLON bit (on Gen2) ?

> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
> >  1 file changed, 7 insertions(+), 3 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
> > @@ -60,8 +60,8 @@ static void rcar_du_lvdsenc_start_gen2(s
> >  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> >  	
> >  	/*
> > -	 * 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)
> > @@ -106,6 +106,9 @@ static void rcar_du_lvdsenc_start_gen3(s
> > 
> >  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> > 
> > +	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> > +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> > +
> >  	/* Turn all the channels on. */
> >  	rcar_lvds_write(lvds, LVDCR1,
> >  			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> > @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
> >  	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
> >  	 * delay and turn the output on.
> >  	 */
> > -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> > +
> > +	lvdcr0 = | LVDCR0_PLLON;
> >  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >  	
> >  	lvdcr0 |= LVDCR0_PWD;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sergei,

On Friday, 12 January 2018 23:51:35 EET Laurent Pinchart wrote:
> On Friday, 12 January 2018 22:12:04 EET Sergei Shtylyov wrote:
> > According to the latest revisions of the R-Car gen3 manual, the LVDS mode
> > must be set before the LVDS I/O pins are enabled, not after --  fix  the
> > gen3 LVDS startup sequence accordingly...
> > 
> > While  at it,  also fix the comment  preceding the first LVDCR0 write in
> > the R-Car gen2 startup code that still talks about hardcoding the LVDS
> > mode 0...
> 
> How about fixing that in patch 2/2 that touches the Gen2 initialization
> sequence ? I think I'd even go as far as squashing both patches, I don't
> think there's a need to split them.
> 
> > Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Is this really needed ? Does it fix a problem you've experienced, or is it
> theoretical only ? The mode shouldn't matter before the LVDS internal logic
> is turned on. Unless there's a real issue I'm not sure we should make the
> code more complex.

Furthermore the datasheet states

"3. This refers to settings other than those that are concerned with LVDS-IF 
startup. These items may be set while waiting for the conditions of step 6 to 
be met."

Doesn't this mean that the mode and input selector don't have to be set as the 
very first step, but can be programmed at any point before starting the LVDS 
encoder through the PWD bit (on Gen3) or the PLLON bit (on Gen2) ?

> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c |   10 +++++++---
> >  1 file changed, 7 insertions(+), 3 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
> > @@ -60,8 +60,8 @@ static void rcar_du_lvdsenc_start_gen2(s
> >  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> >  	
> >  	/*
> > -	 * 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)
> > @@ -106,6 +106,9 @@ static void rcar_du_lvdsenc_start_gen3(s
> > 
> >  	rcar_lvds_write(lvds, LVDPLLCR, pllcr);
> > 
> > +	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> > +	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> > +
> >  	/* Turn all the channels on. */
> >  	rcar_lvds_write(lvds, LVDCR1,
> >  			LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) |
> > @@ -115,7 +118,8 @@ static void rcar_du_lvdsenc_start_gen3(s
> >  	 * Turn the PLL on, set it to LVDS normal mode, wait for the startup
> >  	 * delay and turn the output on.
> >  	 */
> > -	lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_PLLON;
> > +
> > +	lvdcr0 = | LVDCR0_PLLON;
> >  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >  	
> >  	lvdcr0 |= LVDCR0_PWD;

-- 
Regards,

Laurent Pinchart

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

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

[Sergei Shtylyov]

Hello!

On 1/13/2018 12:51 AM, Laurent Pinchart wrote:

> On Friday, 12 January 2018 22:12:04 EET Sergei Shtylyov wrote:
>> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
>> must be set before the LVDS I/O pins are enabled, not after --  fix  the
>> gen3 LVDS startup sequence accordingly...
>>
>> While  at it,  also fix the comment  preceding the first LVDCR0 write in
>> the R-Car gen2 startup code that still talks about hardcoding the LVDS
>> mode 0...
> 
> How about fixing that in patch 2/2 that touches the Gen2 initialization
> sequence ? I think I'd even go as far as squashing both patches, I don't think
> there's a need to split them.

    No. Fixing both issues with 1 patch was intentional because they both were 
introduced by the same commit (see Fixes:).

>> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Is this really needed ? Does it fix a problem you've experienced, or is it
> theoretical only ?

    No, just another check against the manual - there were some patches in the 
history of this file that did the same thing (sync to the manual). See e.g.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=85e8f8d175caa6a39f4c4e11dd4d0ab038f43324

> The mode shouldn't matter before the LVDS internal logic is
> turned on. Unless there's a real issue I'm not sure we should make the code
> more complex.

    Up to you, of course...

[...]

MBR,Sergei

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

[Sergei Shtylyov]

On 1/13/2018 1:15 AM, Laurent Pinchart wrote:

>>> According to the latest revisions of the R-Car gen3 manual, the LVDS mode
>>> must be set before the LVDS I/O pins are enabled, not after --  fix  the
>>> gen3 LVDS startup sequence accordingly...
>>>
>>> While  at it,  also fix the comment  preceding the first LVDCR0 write in
>>> the R-Car gen2 startup code that still talks about hardcoding the LVDS
>>> mode 0...
>>
>> How about fixing that in patch 2/2 that touches the Gen2 initialization
>> sequence ? I think I'd even go as far as squashing both patches, I don't
>> think there's a need to split them.
>>
>>> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode selection")
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> Is this really needed ? Does it fix a problem you've experienced, or is it
>> theoretical only ? The mode shouldn't matter before the LVDS internal logic
>> is turned on. Unless there's a real issue I'm not sure we should make the
>> code more complex.
> 
> Furthermore the datasheet states
> 
> "3. This refers to settings other than those that are concerned with LVDS-IF
> startup. These items may be set while waiting for the conditions of step 6 to
> be met."

    Ah, I hadn't paid much attention to this note. Howeve, it seems quite 
vague to me... and there's no condition in step 6. ;-)

> Doesn't this mean that the mode and input selector don't have to be set as the
> very first step, but can be programmed at any point before starting the LVDS
> encoder through the PWD bit (on Gen3) or the PLLON bit (on Gen2) ?

    Frankly speaking, I don't know how to interpret that note...

[...]

MBR, Sergei

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

[Laurent Pinchart]

Hi Sergei,

On Saturday, 13 January 2018 11:25:31 EET Sergei Shtylyov wrote:
> On 1/13/2018 1:15 AM, Laurent Pinchart wrote:
> >>> According to the latest revisions of the R-Car gen3 manual, the LVDS
> >>> mode must be set before the LVDS I/O pins are enabled, not after --  fix 
> >>> the gen3 LVDS startup sequence accordingly...
> >>> 
> >>> While  at it,  also fix the comment  preceding the first LVDCR0 write in
> >>> the R-Car gen2 startup code that still talks about hardcoding the LVDS
> >>> mode 0...
> >> 
> >> How about fixing that in patch 2/2 that touches the Gen2 initialization
> >> sequence ? I think I'd even go as far as squashing both patches, I don't
> >> think there's a need to split them.
> >> 
> >>> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode
> >>> selection")
> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> 
> >> Is this really needed ? Does it fix a problem you've experienced, or is
> >> it theoretical only ? The mode shouldn't matter before the LVDS internal
> >> logic is turned on. Unless there's a real issue I'm not sure we should
> >> make the code more complex.
> > 
> > Furthermore the datasheet states
> > 
> > "3. This refers to settings other than those that are concerned with
> > LVDS-IF startup. These items may be set while waiting for the conditions
> > of step 6 to be met."
> 
> Ah, I hadn't paid much attention to this note. Howeve, it seems quite
> vague to me... and there's no condition in step 6. ;-)

Lots of bits and pieces are lost in translation yes :-)

> > Doesn't this mean that the mode and input selector don't have to be set as
> > the very first step, but can be programmed at any point before starting
> > the LVDS encoder through the PWD bit (on Gen3) or the PLLON bit (on Gen2)
> > ?
> 
> Frankly speaking, I don't know how to interpret that note...

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

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

[Laurent Pinchart]

Hi Sergei,

On Tuesday, 16 January 2018 17:42:41 EET Laurent Pinchart wrote:
> On Saturday, 13 January 2018 11:25:31 EET Sergei Shtylyov wrote:
> > On 1/13/2018 1:15 AM, Laurent Pinchart wrote:
> >>>> According to the latest revisions of the R-Car gen3 manual, the LVDS
> >>>> mode must be set before the LVDS I/O pins are enabled, not after -- 
> >>>> fix the gen3 LVDS startup sequence accordingly...
> >>>> 
> >>>> While  at it,  also fix the comment  preceding the first LVDCR0 write
> >>>> in the R-Car gen2 startup code that still talks about hardcoding the
> >>>> LVDS mode 0...
> >>> 
> >>> How about fixing that in patch 2/2 that touches the Gen2 initialization
> >>> sequence ? I think I'd even go as far as squashing both patches, I
> >>> don't think there's a need to split them.
> >>> 
> >>>> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode
> >>>> selection")
> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> 
> >>> Is this really needed ? Does it fix a problem you've experienced, or is
> >>> it theoretical only ? The mode shouldn't matter before the LVDS
> >>> internal logic is turned on. Unless there's a real issue I'm not sure we
> >>> should make the code more complex.
> >> 
> >> Furthermore the datasheet states
> >> 
> >> "3. This refers to settings other than those that are concerned with
> >> LVDS-IF startup. These items may be set while waiting for the conditions
> >> of step 6 to be met."
> > 
> > Ah, I hadn't paid much attention to this note. Howeve, it seems quite
> > vague to me... and there's no condition in step 6. ;-)
> 
> Lots of bits and pieces are lost in translation yes :-)
> 
> >> Doesn't this mean that the mode and input selector don't have to be set
> >> as the very first step, but can be programmed at any point before
> >> starting the LVDS encoder through the PWD bit (on Gen3) or the PLLON bit
> >> (on Gen2) ?
> > 
> > Frankly speaking, I don't know how to interpret that note...
> 
> 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.

I've received feedback, and while it wasn't clear what the not really means, 
Renesas recommends following the documented startup sequence (I'm still not 
sure it's really needed, but that's a different story). I'll thus rebase this 
patch and repost it, and take it in my tree if you're fine with the result.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sergei,

On Tuesday, 16 January 2018 17:42:41 EET Laurent Pinchart wrote:
> On Saturday, 13 January 2018 11:25:31 EET Sergei Shtylyov wrote:
> > On 1/13/2018 1:15 AM, Laurent Pinchart wrote:
> >>>> According to the latest revisions of the R-Car gen3 manual, the LVDS
> >>>> mode must be set before the LVDS I/O pins are enabled, not after -- 
> >>>> fix the gen3 LVDS startup sequence accordingly...
> >>>> 
> >>>> While  at it,  also fix the comment  preceding the first LVDCR0 write
> >>>> in the R-Car gen2 startup code that still talks about hardcoding the
> >>>> LVDS mode 0...
> >>> 
> >>> How about fixing that in patch 2/2 that touches the Gen2 initialization
> >>> sequence ? I think I'd even go as far as squashing both patches, I
> >>> don't think there's a need to split them.
> >>> 
> >>>> Fixes: e947eccbeba4 ("drm: rcar-du: Add support for LVDS mode
> >>>> selection")
> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>> 
> >>> Is this really needed ? Does it fix a problem you've experienced, or is
> >>> it theoretical only ? The mode shouldn't matter before the LVDS
> >>> internal logic is turned on. Unless there's a real issue I'm not sure we
> >>> should make the code more complex.
> >> 
> >> Furthermore the datasheet states
> >> 
> >> "3. This refers to settings other than those that are concerned with
> >> LVDS-IF startup. These items may be set while waiting for the conditions
> >> of step 6 to be met."
> > 
> > Ah, I hadn't paid much attention to this note. Howeve, it seems quite
> > vague to me... and there's no condition in step 6. ;-)
> 
> Lots of bits and pieces are lost in translation yes :-)
> 
> >> Doesn't this mean that the mode and input selector don't have to be set
> >> as the very first step, but can be programmed at any point before
> >> starting the LVDS encoder through the PWD bit (on Gen3) or the PLLON bit
> >> (on Gen2) ?
> > 
> > Frankly speaking, I don't know how to interpret that note...
> 
> 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.

I've received feedback, and while it wasn't clear what the not really means, 
Renesas recommends following the documented startup sequence (I'm still not 
sure it's really needed, but that's a different story). I'll thus rebase this 
patch and repost it, and take it in my tree if you're fine with the result.

-- 
Regards,

Laurent Pinchart

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