Re: [PATCH/RFC 10/15] drm: rcar-du: lvds: Set LVEN and LVRES bits together on D3

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

Hi Jacopo,

On Fri, Mar 08, 2019 at 05:25:12PM +0100, Jacopo Mondi wrote:
> On Thu, Mar 07, 2019 at 01:23:40AM +0200, Laurent Pinchart wrote:
> > On the D3 SoC the LVDS PHY must be enabled in the same register write
> > that enables the LVDS output. Skip writing the LVEN bit independently
> > on that platform, it will be set by the write that sets LVRES.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index b1abe737dc05..5ac92ee15be0 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -475,9 +475,13 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> >  	}
> >
> >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) {
> > -		/* Turn on the LVDS PHY. */
> > +		/*
> > +		 * Turn on the LVDS PHY. On D3, the LVEN and LVRES bit must be
> > +		 * set at the same time, so don't write the register yet.
> > +		 */
> >  		lvdcr0 |= LVDCR0_LVEN;
> > -		rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> > +		if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_PWD))
> 
> This is quite obscure, and works because D3 is the only SoC that has
> (quirks & RCAR_LVDS_QUIRK_GEN3_LVEN) and (!(quirks & RCAR_LVDS_QUIRK_PWD)).
> 
> I guess there are not many ways around this.

We could add a model field to the info structure, or another quirk, but
I'd rather not add yet another if it's not needed for now. I agree it's
not very nice though, it bothered me too when I wrote the code.

> > +			rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> 
> I have verified this against the latest v1.50 datasheet, and it
> matches what's reported in section 37.3.7 so please add my:
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> I would like just to add that the same section prescribes a precise
> order in which LVDS0 and LVDS1 have to be configured when working with
> vertical stripe output. Is that enforced in this series?

It is, the companion is enabled before and disabled after the master for
this reason. My code initially violated the constraint, and the HDMI
output remained blank.

Note that figure 37.9 describes a sequence where register writes are
interleaved. As it's titled "The sample setting of the vertical stripe
output", I've considered it as a sample only.

> >  	}
> >
> >  	if (!(lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)) {

-- 
Regards,

Laurent Pinchart