rockchip: correctly set vop0 or vop1

[Andy Yan]

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.
>>
>>
>
>
>