Message ID | 20200607183612.GA82210@ryzen.blueri.se |
---|---|
State | Accepted |
Commit | 673eb44e91bc0c06cb1e3f353f5d07b4f9e5a586 |
Headers | show |
Series | rockchip: correctly set vop0 or vop1 | expand |
Patrick Wildt <patrick at 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 at 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
On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: > Patrick Wildt <patrick at 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 at 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? Patrick
Patrick Wildt <patrick at blueri.se> writes: > On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: >> Patrick Wildt <patrick at 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 at 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
On Mon, Jun 08, 2020 at 02:24:32PM +0200, Arnaud Patard wrote: > Patrick Wildt <patrick at blueri.se> writes: > > > On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: > >> Patrick Wildt <patrick at 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 at 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.
+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 <patrick at blueri.se> writes: >> >>> On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: >>>> Patrick Wildt <patrick at 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 at 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. > >
On 2020/6/8 ??2:36, Patrick Wildt wrote: > 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 at blueri.se> I will take this fix for rk3288 and later you can send support for rk3399. Reviewed-by: Kever Yang <kever.yang at rock-chips.com> Thanks, - Kever > > 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)); > > rockchip_edp_wait_hpd(priv); > > >
Hi : On 6/27/20 8:56 PM, Kever Yang wrote: > +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 <patrick at blueri.se> writes: >>> >>>> On Mon, Jun 08, 2020 at 10:18:19AM +0200, Arnaud Patard wrote: >>>>> Patrick Wildt <patrick at 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 at 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, >>>> }; >>>> It's true that different soc have different grf register for selecting lcdc/vop, and so it is for other modules such as rockchip_gmac/pinctrl. The above code in linux kernel is a example for how? we handle this case. >>>> 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. >> >> > > >
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)); rockchip_edp_wait_hpd(priv);
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 at blueri.se>