diff mbox series

rockchip: correctly set vop0 or vop1

Message ID 20200607183612.GA82210@ryzen.blueri.se
State Accepted
Commit 673eb44e91bc0c06cb1e3f353f5d07b4f9e5a586
Headers show
Series rockchip: correctly set vop0 or vop1 | expand

Commit Message

Patrick Wildt June 7, 2020, 6:36 p.m. UTC
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>

Comments

Arnaud Patard (Rtp) June 8, 2020, 8:18 a.m. UTC | #1
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
Patrick Wildt June 8, 2020, 11:10 a.m. UTC | #2
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
Arnaud Patard (Rtp) June 8, 2020, 12:24 p.m. UTC | #3
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
Patrick Wildt June 8, 2020, 12:39 p.m. UTC | #4
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.
Kever Yang June 27, 2020, 12:56 p.m. UTC | #5
+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.
>
>
Kever Yang June 28, 2020, 1:50 a.m. UTC | #6
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);
>   
>
>
Andy Yan June 28, 2020, 2:24 a.m. UTC | #7
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 mbox series

Patch

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);