+Andy Yan for this topic,
Hi Patrick and Arnaud,
I would like to leave this patch until the code fits for all the socs,
Thanks,
- Kever
On 2020/6/8 下午8:39, Patrick Wildt wrote:
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.
