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

[Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org>]

Alper Nebi Yasak <alpernebiyasak@gmail.com> writes:

Hi,

> On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
>> According to linux commit "drm/rockchip: analogix_dp: add rk3399 eDP
>> support" (82872e42bb1501dd9e60ca430f4bae45a469aa64), rk3288 and rk3399
>> eDP IPs are nearly the same, the difference is in the grf register
>> (SOC_CON6 versus SOC_CON20). So, change the code to use the right
>> register on each IP.
>> 
>> The clocks don't seem to be the same, the eDP clock is not at index 1
>> on rk3399, so don't try changing the clock at index 1 to rate 0 on
>> rk3399.
>> 
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: u-boot/drivers/video/rockchip/rk_edp.c
>> ===================================================================
>
> 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.

>
>> --- u-boot.orig/drivers/video/rockchip/rk_edp.c
>> +++ u-boot/drivers/video/rockchip/rk_edp.c
>> @@ -17,11 +17,10 @@
>>  #include <asm/gpio.h>
>>  #include <asm/io.h>
>>  #include <asm/arch-rockchip/clock.h>
>> +#include <asm/arch-rockchip/hardware.h>
>>  #include <asm/arch-rockchip/edp_rk3288.h>
>>  #include <asm/arch-rockchip/grf_rk3288.h>
>> -#include <asm/arch-rockchip/hardware.h>
>> -#include <dt-bindings/clock/rk3288-cru.h>
>> -#include <linux/delay.h>
>> +#include <asm/arch-rockchip/grf_rk3399.h>
>>  
>>  #define MAX_CR_LOOP 5
>>  #define MAX_EQ_LOOP 5
>> @@ -37,18 +36,42 @@ static const char * const pre_emph_names
>>  #define DP_VOLTAGE_MAX         DP_TRAIN_VOLTAGE_SWING_1200
>>  #define DP_PRE_EMPHASIS_MAX    DP_TRAIN_PRE_EMPHASIS_9_5
>>  
>> +#define RK3288_GRF_SOC_CON6	0x025c
>> +#define RK3288_GRF_SOC_CON12	0x0274
>> +#define RK3399_GRF_SOC_CON20	0x6250
>> +#define RK3399_GRF_SOC_CON25	0x6264
>
> The chip-specific parts could cast the *grf to rk3288_grf/rk3399_grf
> struct pointer and use &grf->soc_con6, &grf->soc_con25 etc. in the
> chip-specific drivers.
>
>> +
>> +enum rockchip_dp_types {
>> +	RK3288_DP = 0,
>> +	RK3399_EDP
>> +};
>> +
>> +struct rockchip_dp_data {
>> +	unsigned long reg_vop_big_little;
>> +	unsigned long reg_vop_big_little_sel;
>> +	unsigned long reg_ref_clk_sel;
>> +	unsigned long ref_clk_sel_bit;
>> +	enum rockchip_dp_types chip_type;
>> +};
>
> These wouldn't be necessary as you could hard-code things per-chip, like
> the current code does.
>
>> +
>>  struct rk_edp_priv {
>>  	struct rk3288_edp *regs;
>> -	struct rk3288_grf *grf;
>> +	void *grf;
>>  	struct udevice *panel;
>>  	struct link_train link_train;
>>  	u8 train_set[4];
>>  };
>
> This would go to a rk_edp.h which would be included from both
> chip-specific .c files.
>
>>  
>> -static void rk_edp_init_refclk(struct rk3288_edp *regs)
>> +static void rk_edp_init_refclk(struct rk3288_edp *regs, enum rockchip_dp_types chip_type)
>>  {
>>  	writel(SEL_24M, &regs->analog_ctl_2);
>> -	writel(REF_CLK_24M, &regs->pll_reg_1);
>> +	u32 reg;
>> +
>> +	reg = REF_CLK_24M;
>> +	if (chip_type == RK3288_DP)
>> +		reg ^= REF_CLK_MASK;
>> +	writel(reg, &regs->pll_reg_1);
>> +
>
> You can delegate this to the chip-specific drivers with something like
> what rkvop_set_pin_polarity() does. Or you could keep it in the common
> code and just change the definition of the constants with #if based on
> the chip.
>
>>  
>>  	writel(LDO_OUTPUT_V_SEL_145 | KVCO_DEFALUT | CHG_PUMP_CUR_SEL_5US |
>>  	       V2L_CUR_SEL_1MA, &regs->pll_reg_2);
>> @@ -1023,6 +1046,8 @@ static int rk_edp_probe(struct udevice *
>>  	struct display_plat *uc_plat = dev_get_uclass_platdata(dev);
>>  	struct rk_edp_priv *priv = dev_get_priv(dev);
>>  	struct rk3288_edp *regs = priv->regs;
>> +	struct rockchip_dp_data *edp_data = (struct rockchip_dp_data *)dev_get_driver_data(dev);
>> +
>>  	struct clk clk;
>>  	int ret;
>>  
>> @@ -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.

>>  	}
>> -
>>  	ret = clk_get_by_index(uc_plat->src_dev, 0, &clk);
>>  	if (ret >= 0) {
>>  		ret = clk_set_rate(&clk, 192000000);
>> @@ -1059,15 +1085,17 @@ static int rk_edp_probe(struct udevice *
>>  	}
>>  
>>  	/* grf_edp_ref_clk_sel: from internal 24MHz or 27MHz clock */
>> -	rk_setreg(&priv->grf->soc_con12, 1 << 4);
>> +	rk_setreg(priv->grf + edp_data->reg_ref_clk_sel,
>> +		  edp_data->ref_clk_sel_bit);
>>  
>>  	/* select epd signal from vop0 or vop1 */
>> -	rk_clrsetreg(&priv->grf->soc_con6, (1 << 5),
>> -	    (vop_id == 1) ? (1 << 5) : (0 << 5));
>> +	rk_clrsetreg(priv->grf + edp_data->reg_vop_big_little,
>> +		     edp_data->reg_vop_big_little_sel,
>> +		     (vop_id == 1) ? edp_data->reg_vop_big_little_sel : 0);
>>  
>
> I think probe() parts upto here would be chip-specific and parts after
> this would be common, but there's also the panel thing at the top...
>
> (You'd also just change the some numbers here with chip-specific
> drivers.)
>
>>  	rockchip_edp_wait_hpd(priv);
>>  
>> -	rk_edp_init_refclk(regs);
>> +	rk_edp_init_refclk(regs, edp_data->chip_type);
>>  	rk_edp_init_interrupt(regs);
>>  	rk_edp_enable_sw_function(regs);
>>  	ret = rk_edp_init_analog_func(regs);
>> @@ -1083,8 +1111,25 @@ static const struct dm_display_ops dp_ro
>>  	.enable = rk_edp_enable,
>>  };
>>  
>> +static const struct rockchip_dp_data rk3399_edp = {
>> +	.reg_vop_big_little = RK3399_GRF_SOC_CON20,
>> +	.reg_vop_big_little_sel = BIT(5),
>> +	.reg_ref_clk_sel = RK3399_GRF_SOC_CON25,
>> +	.ref_clk_sel_bit = BIT(11),
>> +	.chip_type = RK3399_EDP,
>> +};
>> +
>> +static const struct rockchip_dp_data rk3288_dp = {
>> +	.reg_vop_big_little = RK3288_GRF_SOC_CON6,
>> +	.reg_vop_big_little_sel = BIT(5),
>> +	.reg_ref_clk_sel = RK3288_GRF_SOC_CON12,
>> +	.ref_clk_sel_bit = BIT(4),
>> +	.chip_type = RK3288_DP,
>> +};
>> +
>>  static const struct udevice_id rockchip_dp_ids[] = {
>> -	{ .compatible = "rockchip,rk3288-edp" },
>> +	{ .compatible = "rockchip,rk3288-edp", .data = (ulong)&rk3288_dp },
>> +	{ .compatible = "rockchip,rk3399-edp", .data = (ulong)&rk3399_edp },
>>  	{ }
>>  };
>>  
>> Index: u-boot/arch/arm/include/asm/arch-rockchip/edp_rk3288.h
>> ===================================================================
>> --- u-boot.orig/arch/arm/include/asm/arch-rockchip/edp_rk3288.h
>> +++ u-boot/arch/arm/include/asm/arch-rockchip/edp_rk3288.h
>> @@ -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}.


Arnaud