rockchip: correctly set vop0 or vop1

[Patrick Wildt <patrick@blueri.se>]

On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote:
> Patrick Wildt <patrick@blueri.se> writes:
> 
> > On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote:
> >> Patrick Wildt <patrick@blueri.se> writes:
> >> 
> >> Hi,
> >> 
> >> > The EDP_LCDC_SEL bit has to be set correctly to select vop0 or
> >> > vop1, but so far we have set it in both conditions, which is not
> >> > correct.
> >> >
> >> > Can someone verify this is the correct way round?  vop1 -> set,
> >> > vop0 -> clear?
> >> >
> >> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >> >
> >> > diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
> >> > index 92188be9275..000bd481408 100644
> >> > --- a/drivers/video/rockchip/rk_edp.c
> >> > +++ b/drivers/video/rockchip/rk_edp.c
> >> > @@ -1062,7 +1062,8 @@ static int rk_edp_probe(struct udevice *dev)
> >> >  	rk_setreg(&priv->grf->soc_con12, 1 << 4);
> >> >  
> >> >  	/* select epd signal from vop0 or vop1 */
> >> > -	rk_setreg(&priv->grf->soc_con6, (vop_id == 1) ? (1 << 5) : (1 << 5));
> >> > +	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
> >> > +	    (vop_id == 1) ? (1 << 5) : (0 << 5));
> >> 
> >> While working on PBP EDP support, found this too but I'm not sure it's
> >> fine or not. For rk3399, my (not yet published) patch is doing:
> >> 
> >> +       if (vop_id == 0)
> >> +               rk_clrreg(&priv->grf->soc_con20, (1 << 5));
> >> +       else
> >> +               rk_setreg(&priv->grf->soc_con20, (1 << 5));
> >> 
> >> I believe that the rk3288 may need similar treatment but I've yet to
> >> look at the rk3288 manual.
> >> 
> >> Arnaud
> >
> > Yes, it does.  If you look at the linux code, they have:
> >
> > static const struct rockchip_dp_chip_data rk3399_edp = {
> >         .lcdsel_grf_reg = RK3399_GRF_SOC_CON20,
> >         .lcdsel_big = HIWORD_UPDATE(0, RK3399_EDP_LCDC_SEL),
> >         .lcdsel_lit = HIWORD_UPDATE(RK3399_EDP_LCDC_SEL, RK3399_EDP_LCDC_SEL),
> >         .chip_type = RK3399_EDP,
> > };
> >
> > static const struct rockchip_dp_chip_data rk3288_dp = {
> >         .lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
> >         .lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
> >         .lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
> >         .chip_type = RK3288_DP,
> > };
> >
> > which indicates that for rk3399 *and* rk3288 the bit has to be set to
> > select "lit".  Now your diff looks equivalent to mine, apart from using
> > a different operation to achieve the same goal.
> >
> > The linux code does
> >
> >         ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> >         if (ret < 0)
> >                 return;
> >
> >         if (ret)
> >                 val = dp->data->lcdsel_lit;
> >         else
> >                 val = dp->data->lcdsel_big;
> >
> > Assuming that endpoint_id essentiall returns vop id 0 or vop id 1, this
> > would mean that vop1 -> lit -> set bit and vop0 -> big -> clr bit.
> >
> > That said, my diff seems to be fine, and your RK3399 code as well.  Do
> > you agree?
> 
> According to the code you've shown, it should be fine for rk3288 I guess
> but not for rk3399. Please note that it's grf soc_con6 register for rk3288
> but grf soc_con20 for rk3399.
> 
> Arnaud

Exactly, which is why you had that if defined() in your diff, to compile
one part of the code for RK3288, and the other for RK3399. :)  The bit
though happens to be the same.