On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote: > Patrick Wildt <patr...@blueri.se> writes: > > > On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: > >> Patrick Wildt <patr...@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 <patr...@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.