* rockchip: correctly set vop0 or vop1 @ 2020-06-07 18:36 Patrick Wildt 2020-06-08 8:18 ` Arnaud Patard 2020-06-28 1:50 ` Kever Yang 0 siblings, 2 replies; 8+ messages in thread From: Patrick Wildt @ 2020-06-07 18:36 UTC (permalink / raw) To: u-boot 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)); rockchip_edp_wait_hpd(priv); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-07 18:36 rockchip: correctly set vop0 or vop1 Patrick Wildt @ 2020-06-08 8:18 ` Arnaud Patard 2020-06-08 11:10 ` Patrick Wildt 2020-06-28 1:50 ` Kever Yang 1 sibling, 1 reply; 8+ messages in thread From: Arnaud Patard @ 2020-06-08 8:18 UTC (permalink / raw) To: u-boot 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@the rk3288 manual. Arnaud ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-08 8:18 ` Arnaud Patard @ 2020-06-08 11:10 ` Patrick Wildt 2020-06-08 12:24 ` Arnaud Patard 0 siblings, 1 reply; 8+ messages in thread From: Patrick Wildt @ 2020-06-08 11:10 UTC (permalink / raw) To: u-boot 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? Patrick ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-08 11:10 ` Patrick Wildt @ 2020-06-08 12:24 ` Arnaud Patard 2020-06-08 12:39 ` Patrick Wildt 0 siblings, 1 reply; 8+ messages in thread From: Arnaud Patard @ 2020-06-08 12:24 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-08 12:24 ` Arnaud Patard @ 2020-06-08 12:39 ` Patrick Wildt 2020-06-27 12:56 ` Kever Yang 0 siblings, 1 reply; 8+ messages in thread From: Patrick Wildt @ 2020-06-08 12:39 UTC (permalink / raw) To: u-boot 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-08 12:39 ` Patrick Wildt @ 2020-06-27 12:56 ` Kever Yang 2020-06-28 2:24 ` Andy Yan 0 siblings, 1 reply; 8+ messages in thread From: Kever Yang @ 2020-06-27 12:56 UTC (permalink / raw) To: u-boot +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@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. > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-27 12:56 ` Kever Yang @ 2020-06-28 2:24 ` Andy Yan 0 siblings, 0 replies; 8+ messages in thread From: Andy Yan @ 2020-06-28 2:24 UTC (permalink / raw) To: u-boot 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@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, >>>> }; >>>> 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. >> >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* rockchip: correctly set vop0 or vop1 2020-06-07 18:36 rockchip: correctly set vop0 or vop1 Patrick Wildt 2020-06-08 8:18 ` Arnaud Patard @ 2020-06-28 1:50 ` Kever Yang 1 sibling, 0 replies; 8+ messages in thread From: Kever Yang @ 2020-06-28 1:50 UTC (permalink / raw) To: u-boot 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@blueri.se> I will take this fix for rk3288 and later you can send support for rk3399. Reviewed-by: Kever Yang <kever.yang@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); > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-28 2:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-07 18:36 rockchip: correctly set vop0 or vop1 Patrick Wildt 2020-06-08 8:18 ` Arnaud Patard 2020-06-08 11:10 ` Patrick Wildt 2020-06-08 12:24 ` Arnaud Patard 2020-06-08 12:39 ` Patrick Wildt 2020-06-27 12:56 ` Kever Yang 2020-06-28 2:24 ` Andy Yan 2020-06-28 1:50 ` Kever Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox