[patch 2/8] RFC: drivers/video/rockchip/rk_edp.c: Add rk3399 support

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 23/10/2020 11:49, Arnaud Patard (Rtp) wrote:
> Alper Nebi Yasak <alpernebiyasak@gmail.com> writes:
>> I think instead of supporting both devices in the same driver, it'd be
>> cleaner to split chip-specific parts into rk3288_edp.c and rk3399_edp.c
>> like the other ones for vop, hdmi, etc; the common parts would stay here
>> in rk_edp.c, and maybe a new rk_edp.h.
> 
> From what I understand the 3288 and 3399 are using the same HW IP and
> there's no other rk SoC using it. At the beginning of this work I used
> #ifdef but in the end I found it was making the code harder to read (and
> checkpatch.pl unhappy iirc). I'm also not convinced that splitting for
> only two SoCs worths it.
> 
> btw, code inside #ifdef tends to bitrot so using runtime detection
> will at least give build testing.

I don't think anything has to be in #ifdef, chip-specific files would be
conditionally built and linked in based on Kconfig. The chip-specific
versions would be called first on driver probe, then they'd use the
common parts as they see fit.

As you said it might not be worth splitting it, I don't really know.

>>> @@ -1037,16 +1062,17 @@ static int rk_edp_probe(struct udevice *
>>>  	int vop_id = uc_plat->source_id;
>>>  	debug("%s, uc_plat=%p, vop_id=%u\n", __func__, uc_plat, vop_id);
>>>  
>>> -	ret = clk_get_by_index(dev, 1, &clk);
>>> -	if (ret >= 0) {
>>> -		ret = clk_set_rate(&clk, 0);
>>> -		clk_free(&clk);
>>> -	}
>>> -	if (ret) {
>>> -		debug("%s: Failed to set EDP clock: ret=%d\n", __func__, ret);
>>> -		return ret;
>>> +	if (edp_data->chip_type == RK3288_DP) {
>>> +		ret = clk_get_by_index(dev, 1, &clk);
>>> +		if (ret >= 0) {
>>> +			ret = clk_set_rate(&clk, 0);
>>> +			clk_free(&clk);
>>> +		}
>>> +		if (ret) {
>>> +			debug("%s: Failed to set EDP clock: ret=%d\n", __func__, ret);
>>> +			return ret;
>>> +		}
>>
>> Both these clocks don't look like they're unique to rk3288 but the
>> current code that sets them definitely is, could be split out to
>> chip-specific drivers.
>>
>> Maybe you could get and set the clocks by name for both devices in the
>> common part while checking at least one of the equivalent clocks were
>> set (but the clock driver currently ignores some clocks and e.g. sets
>> them to a known constant).
>>
> 
> I've been wondering a lot about this clock stuff and in the end, choose
> to not modify current behaviour. For instance, I'm not sure to
> understand why the clock rate is set to 0 and in linux, there's no
> difference between 3288 and 3399 clocks handling. I really think that if
> the clock part has to change, it has to be in a different patchset than
> here.

I think this is SCLK_EDP_24M, looks like the rk3288 clock driver can
configure it for 24M but not set it to a specific rate, so it ignores
the parameter and pretends it set what you gave it. On rk3399 it'd be
trying to set PCLK_EDP_CTRL (?), which its clock driver doesn't know
about.

I agree clock changes would be better in a different patchset (unless
you'd go with the chip-specific files and want to maximize the common
parts).

>>> @@ -232,8 +232,9 @@ check_member(rk3288_edp, pll_reg_5, 0xa0
>>>  #define PD_CH0					(0x1 << 0)
>>>  
>>>  /* pll_reg_1 */
>>> -#define REF_CLK_24M				(0x1 << 1)
>>> -#define REF_CLK_27M				(0x0 << 1)
>>> +#define REF_CLK_24M				(0x1 << 0)
>>> +#define REF_CLK_27M				(0x0 << 0)
>>> +#define REF_CLK_MASK				(0x1 << 0)
>>
>> AFAIK the old definitions were already wrong and just happened to work
>> because both set bit-0 to 0, which chooses 24M on rk3288, which is what
>> was requested anyway. As I said above, you might define the two
>> constants here differently with #if defined(CONFIG_ROCKCHIP_RK3399).
> 
> why using two set of constants ? there's no difference on linux for
> this between rk3399 and rk3288. I'm only fixing things according to
> what's done in linux. See
> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.{c,h}.

I assumed the meaning of the bit was explicitly chosen to be different
for the two chips, so I though that'd match the hardware better
semantically. But Linux commit 7bdc07208693 ("drm/bridge: analogix_dp:
some rockchip chips need to flip REF_CLK bit setting") which adds that
bit flipping code says it's due to a "IC PHY layout mistake" so I guess
I was wrong about that.