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

[Alper Nebi Yasak]

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

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

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

>  
>  /* line_map */
>  #define LANE3_MAP_LOGIC_LANE_0			(0x0 << 6)
> 
>