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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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