[patch 0/8] RFC: Pinebook pro EDP support

[patch 0/8] RFC: Pinebook pro EDP support

[Arnaud Patard]

This patchset add support for the rk3399 edp. It has been tested on the pinebook
pro devices. The only missing part is a hack used to get stable edp output after
a warn reset, which is possibly specific to this device. I'm not sure if it's suitable
for merge.

The changes have been written by studying the linux code, since I didn't find any
manual for theses part of the RK3399 SoC.

On the linux kernel side, on recent kernels, it needs commit "pwm: rockchip: Keep enabled PWMs
running while probing" otherwise the pinebook pro will freeze when probing
the display.

The kernel is also randomly failing to display something on my device. When this
occurs, the kernel has this following message:
rockchip-pm-domain ff310000.power-management:power-controller: failed to set idle on domain 'pd_vopl', val=0
I've yet to find what's the issue.

Arnaud

[patch 1/8] RFC: drivers/video/rockchip/rk_vop.c: Use endpoint compatible string to find VOP mode

[Arnaud Patard]

The current code is using an hard coded enum and the of node reg value of
endpoint to find out if the endpoint is mipi/hdmi/lvds/edp/dp. The order
is different between rk3288, rk3399 vop little, rk3399 vop big.

A possible solution would be to make sure that the rk3288.dtsi and 
rk3399.dtsi files have "expected" reg value or an other solution is
to find the kind of endpoint by comparing the endpoint compatible value.

This patch is implementing the more flexible second solution.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>

Index: u-boot/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
===================================================================
--- u-boot.orig/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
+++ u-boot/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
@@ -85,26 +85,13 @@ enum {
 	LB_RGB_1280X8 = 0x5
 };
 
-#if defined(CONFIG_ROCKCHIP_RK3399)
 enum vop_modes {
 	VOP_MODE_EDP = 0,
 	VOP_MODE_MIPI,
 	VOP_MODE_HDMI,
-	VOP_MODE_MIPI1,
-	VOP_MODE_DP,
-	VOP_MODE_NONE,
-};
-#else
-enum vop_modes {
-	VOP_MODE_EDP = 0,
-	VOP_MODE_HDMI,
 	VOP_MODE_LVDS,
-	VOP_MODE_MIPI,
-	VOP_MODE_NONE,
-	VOP_MODE_AUTO_DETECT,
-	VOP_MODE_UNKNOWN,
+	VOP_MODE_DP,
 };
-#endif
 
 /* VOP_VERSION_INFO */
 #define M_FPGA_VERSION (0xffff << 16)
Index: u-boot/drivers/video/rockchip/rk_vop.c
===================================================================
--- u-boot.orig/drivers/video/rockchip/rk_vop.c
+++ u-boot/drivers/video/rockchip/rk_vop.c
@@ -235,12 +235,11 @@ static int rk_display_init(struct udevic
 	struct clk clk;
 	enum video_log2_bpp l2bpp;
 	ofnode remote;
+	const char *compat;
 
 	debug("%s(%s, %lu, %s)\n", __func__,
 	      dev_read_name(dev), fbbase, ofnode_get_name(ep_node));
 
-	vop_id = ofnode_read_s32_default(ep_node, "reg", -1);
-	debug("vop_id=%d\n", vop_id);
 	ret = ofnode_read_u32(ep_node, "remote-endpoint", &remote_phandle);
 	if (ret)
 		return ret;
@@ -282,6 +281,28 @@ static int rk_display_init(struct udevic
 		if (disp)
 			break;
 	};
+	compat = ofnode_get_property(remote, "compatible", NULL);
+	if (!compat) {
+		debug("%s(%s): Failed to find compatible property\n",
+		      __func__, dev_read_name(dev));
+		return -EINVAL;
+	}
+	if (strstr(compat, "edp")) {
+		vop_id = VOP_MODE_EDP;
+	} else if (strstr(compat, "mipi")) {
+		vop_id = VOP_MODE_MIPI;
+	} else if (strstr(compat, "hdmi")) {
+		vop_id = VOP_MODE_HDMI;
+	} else if (strstr(compat, "cdn-dp")) {
+		vop_id = VOP_MODE_DP;
+	} else if (strstr(compat, "lvds")) {
+		vop_id = VOP_MODE_LVDS;
+	} else {
+		debug("%s(%s): Failed to find vop mode for %s\n",
+		      __func__, dev_read_name(dev), compat);
+		return -EINVAL;
+	}
+	debug("vop_id=%d\n", vop_id);
 
 	disp_uc_plat = dev_get_uclass_platdata(disp);
 	debug("Found device '%s', disp_uc_priv=%p\n", disp->name, disp_uc_plat);

[patch 1/8] RFC: drivers/video/rockchip/rk_vop.c: Use endpoint compatible string to find VOP mode

[Kever Yang]

Add Andy for this patch review.


Thanks,

- Kever

On 2020/9/26 ??2:36, Arnaud Patard (Rtp) wrote:
> The current code is using an hard coded enum and the of node reg value of
> endpoint to find out if the endpoint is mipi/hdmi/lvds/edp/dp. The order
> is different between rk3288, rk3399 vop little, rk3399 vop big.
>
> A possible solution would be to make sure that the rk3288.dtsi and
> rk3399.dtsi files have "expected" reg value or an other solution is
> to find the kind of endpoint by comparing the endpoint compatible value.
>
> This patch is implementing the more flexible second solution.
>
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>
> Index: u-boot/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
> ===================================================================
> --- u-boot.orig/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
> +++ u-boot/arch/arm/include/asm/arch-rockchip/vop_rk3288.h
> @@ -85,26 +85,13 @@ enum {
>   	LB_RGB_1280X8 = 0x5
>   };
>   
> -#if defined(CONFIG_ROCKCHIP_RK3399)
>   enum vop_modes {
>   	VOP_MODE_EDP = 0,
>   	VOP_MODE_MIPI,
>   	VOP_MODE_HDMI,
> -	VOP_MODE_MIPI1,
> -	VOP_MODE_DP,
> -	VOP_MODE_NONE,
> -};
> -#else
> -enum vop_modes {
> -	VOP_MODE_EDP = 0,
> -	VOP_MODE_HDMI,
>   	VOP_MODE_LVDS,
> -	VOP_MODE_MIPI,
> -	VOP_MODE_NONE,
> -	VOP_MODE_AUTO_DETECT,
> -	VOP_MODE_UNKNOWN,
> +	VOP_MODE_DP,
>   };
> -#endif
>   
>   /* VOP_VERSION_INFO */
>   #define M_FPGA_VERSION (0xffff << 16)
> Index: u-boot/drivers/video/rockchip/rk_vop.c
> ===================================================================
> --- u-boot.orig/drivers/video/rockchip/rk_vop.c
> +++ u-boot/drivers/video/rockchip/rk_vop.c
> @@ -235,12 +235,11 @@ static int rk_display_init(struct udevic
>   	struct clk clk;
>   	enum video_log2_bpp l2bpp;
>   	ofnode remote;
> +	const char *compat;
>   
>   	debug("%s(%s, %lu, %s)\n", __func__,
>   	      dev_read_name(dev), fbbase, ofnode_get_name(ep_node));
>   
> -	vop_id = ofnode_read_s32_default(ep_node, "reg", -1);
> -	debug("vop_id=%d\n", vop_id);
>   	ret = ofnode_read_u32(ep_node, "remote-endpoint", &remote_phandle);
>   	if (ret)
>   		return ret;
> @@ -282,6 +281,28 @@ static int rk_display_init(struct udevic
>   		if (disp)
>   			break;
>   	};
> +	compat = ofnode_get_property(remote, "compatible", NULL);
> +	if (!compat) {
> +		debug("%s(%s): Failed to find compatible property\n",
> +		      __func__, dev_read_name(dev));
> +		return -EINVAL;
> +	}
> +	if (strstr(compat, "edp")) {
> +		vop_id = VOP_MODE_EDP;
> +	} else if (strstr(compat, "mipi")) {
> +		vop_id = VOP_MODE_MIPI;
> +	} else if (strstr(compat, "hdmi")) {
> +		vop_id = VOP_MODE_HDMI;
> +	} else if (strstr(compat, "cdn-dp")) {
> +		vop_id = VOP_MODE_DP;
> +	} else if (strstr(compat, "lvds")) {
> +		vop_id = VOP_MODE_LVDS;
> +	} else {
> +		debug("%s(%s): Failed to find vop mode for %s\n",
> +		      __func__, dev_read_name(dev), compat);
> +		return -EINVAL;
> +	}
> +	debug("vop_id=%d\n", vop_id);
>   
>   	disp_uc_plat = dev_get_uclass_platdata(disp);
>   	debug("Found device '%s', disp_uc_priv=%p\n", disp->name, disp_uc_plat);
>
>
>
>

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

[Arnaud Patard]

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
===================================================================
--- 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
+
+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;
+};
+
 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];
 };
 
-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);
+
 
 	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;
+		}
 	}
-
 	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);
 
 	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)
 
 /* line_map */
 #define LANE3_MAP_LOGIC_LANE_0			(0x0 << 6)

[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@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)
> 
> 

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

[Arnaud Patard]

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

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

[Alper Nebi Yasak]

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.

[patch 3/8] RFC: drivers/video/rockchip/rk_edp.c: Change interrupt polarity configuration

[Arnaud Patard]

The linux code is setting polarity configuration to 3 but
uboot code is setting it to 1. Change the configuration to match the
linux configuration

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/drivers/video/rockchip/rk_edp.c
===================================================================
--- u-boot.orig/drivers/video/rockchip/rk_edp.c
+++ u-boot/drivers/video/rockchip/rk_edp.c
@@ -100,10 +100,13 @@ static void rk_edp_init_refclk(struct rk
 	       &regs->dp_reserv2);
 }
 
+#define INT_POL1                                (0x1 << 1)
+#define INT_POL0                                (0x1 << 0)
+
 static void rk_edp_init_interrupt(struct rk3288_edp *regs)
 {
 	/* Set interrupt pin assertion polarity as high */
-	writel(INT_POL, &regs->int_ctl);
+	writel(INT_POL0 | INT_POL1, &regs->int_ctl);
 
 	/* Clear pending registers */
 	writel(0xff, &regs->common_int_sta_1);

[patch 3/8] RFC: drivers/video/rockchip/rk_edp.c: Change interrupt polarity configuration

[Alper Nebi Yasak]

On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
> The linux code is setting polarity configuration to 3 but
> uboot code is setting it to 1. Change the configuration to match the
> linux configuration
FYI, coreboot does the same as existing code, but Linux support for this
is bound to be better than both coreboot and U-Boot.

> 
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: u-boot/drivers/video/rockchip/rk_edp.c
> ===================================================================
> --- u-boot.orig/drivers/video/rockchip/rk_edp.c
> +++ u-boot/drivers/video/rockchip/rk_edp.c
> @@ -100,10 +100,13 @@ static void rk_edp_init_refclk(struct rk
>  	       &regs->dp_reserv2);
>  }
>  
> +#define INT_POL1                                (0x1 << 1)
> +#define INT_POL0                                (0x1 << 0)
> +

INT_POL is defined at arch/arm/include/asm/arch-rockchip/edp_rk3288.h,
so these would probably go there.

>  static void rk_edp_init_interrupt(struct rk3288_edp *regs)
>  {
>  	/* Set interrupt pin assertion polarity as high */
> -	writel(INT_POL, &regs->int_ctl);
> +	writel(INT_POL0 | INT_POL1, &regs->int_ctl);
>  
>  	/* Clear pending registers */
>  	writel(0xff, &regs->common_int_sta_1);
> 
> 

[patch 3/8] RFC: drivers/video/rockchip/rk_edp.c: Change interrupt polarity configuration

[Arnaud Patard]

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

> On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
>> The linux code is setting polarity configuration to 3 but
>> uboot code is setting it to 1. Change the configuration to match the
>> linux configuration
> FYI, coreboot does the same as existing code, but Linux support for this
> is bound to be better than both coreboot and U-Boot.
>
>> 
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: u-boot/drivers/video/rockchip/rk_edp.c
>> ===================================================================
>> --- u-boot.orig/drivers/video/rockchip/rk_edp.c
>> +++ u-boot/drivers/video/rockchip/rk_edp.c
>> @@ -100,10 +100,13 @@ static void rk_edp_init_refclk(struct rk
>>  	       &regs->dp_reserv2);
>>  }
>>  
>> +#define INT_POL1                                (0x1 << 1)
>> +#define INT_POL0                                (0x1 << 0)
>> +
>
> INT_POL is defined at arch/arm/include/asm/arch-rockchip/edp_rk3288.h,
> so these would probably go there.
>

I've been wondering were to put them tbh. I'll move them to edp_rk3288.h
then.

Arnaud

[patch 4/8] RFC: drivers/video/rockchip/rk_edp.c: Change clock rate

[Arnaud Patard]

The current code is setting the clock rate to 192000000, but
due to the current device-tree configuration and linux code,
it should rather be 100000000.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/drivers/video/rockchip/rk_edp.c
===================================================================
--- u-boot.orig/drivers/video/rockchip/rk_edp.c
+++ u-boot/drivers/video/rockchip/rk_edp.c
@@ -1075,7 +1078,7 @@ static int rk_edp_probe(struct udevice *
 	}
 	ret = clk_get_by_index(uc_plat->src_dev, 0, &clk);
 	if (ret >= 0) {
-		ret = clk_set_rate(&clk, 192000000);
+		ret = clk_set_rate(&clk, 100000000);
 		clk_free(&clk);
 	}
 	if (ret < 0) {

[patch 4/8] RFC: drivers/video/rockchip/rk_edp.c: Change clock rate

[Alper Nebi Yasak]

On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
> The current code is setting the clock rate to 192000000, but
> due to the current device-tree configuration and linux code,
> it should rather be 100000000.
> 

This looks like it's ACLK_VOP to me. FYI, coreboot sets it to 192 MHz
for rk3288 and 200 MHz for rk3399. But from what I can understand the
U-Boot rk3399 clock driver just ignores this call and sets it to 198 MHz
anyway while setting DCLK_VOP...

> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: u-boot/drivers/video/rockchip/rk_edp.c
> ===================================================================
> --- u-boot.orig/drivers/video/rockchip/rk_edp.c
> +++ u-boot/drivers/video/rockchip/rk_edp.c
> @@ -1075,7 +1078,7 @@ static int rk_edp_probe(struct udevice *
>  	}
>  	ret = clk_get_by_index(uc_plat->src_dev, 0, &clk);
>  	if (ret >= 0) {
> -		ret = clk_set_rate(&clk, 192000000);
> +		ret = clk_set_rate(&clk, 100000000);
>  		clk_free(&clk);
>  	}
>  	if (ret < 0) {
> 
> 

[patch 4/8] RFC: drivers/video/rockchip/rk_edp.c: Change clock rate

[Arnaud Patard]

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

> On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
>> The current code is setting the clock rate to 192000000, but
>> due to the current device-tree configuration and linux code,
>> it should rather be 100000000.
>> 
>
> This looks like it's ACLK_VOP to me. FYI, coreboot sets it to 192 MHz
> for rk3288 and 200 MHz for rk3399. But from what I can understand the
> U-Boot rk3399 clock driver just ignores this call and sets it to 198 MHz
> anyway while setting DCLK_VOP...

In a perfect world, the rate should not be hardcoded and everyone should
rely on devicetree. Again, fixing that is more for a different patchset
than rk3399 edp support imho.
If it makes things easier, I'll test my patchset on my PBP without this
change and possibly drop this patch.

Arnaud

[patch 5/8] RFC: drivers/video/rockchip/rk_vop.c: Reserve efi fb memory

[Arnaud Patard]

When booting with EFI and graphics, the memory used for framebuffer
has to be reserved, otherwise it may leads to kernel memory
overwrite.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/drivers/video/rockchip/rk_vop.c
===================================================================
--- u-boot.orig/drivers/video/rockchip/rk_vop.c
+++ u-boot/drivers/video/rockchip/rk_vop.c
@@ -20,6 +20,8 @@
 #include <asm/arch-rockchip/vop_rk3288.h>
 #include <dm/device-internal.h>
 #include <dm/uclass-internal.h>
+#include <efi.h>
+#include <efi_loader.h>
 #include <linux/bitops.h>
 #include <linux/err.h>
 #include <power/regulator.h>
@@ -394,6 +396,13 @@ int rk_vop_probe(struct udevice *dev)
 	if (!(gd->flags & GD_FLG_RELOC))
 		return 0;
 
+	plat->base = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size - plat->size;
+
+#if defined(CONFIG_EFI_LOADER)
+	debug("Adding to EFI map %d @ %lx\n", plat->size, plat->base);
+	efi_add_memory_map(plat->base, plat->size, EFI_RESERVED_MEMORY_TYPE);
+#endif
+
 	priv->regs = (struct rk3288_vop *)dev_read_addr(dev);
 
 	/*

[patch 6/8] RFC: rk3399-pinebook-pro-u-boot.dtsi: Enable edp

[Arnaud Patard]

- uboot rockchip edp code is looking for a rockchip,panel property
  for the edp dts node, so add it.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
===================================================================
--- u-boot.orig/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
+++ u-boot/arch/arm/dts/rk3399-pinebook-pro-u-boot.dtsi
@@ -49,3 +49,7 @@
 &vdd_log {
 	regulator-init-microvolt = <950000>;
 };
+
+&edp {
+	rockchip,panel = <&edp_panel>;
+};

[patch 7/8] RFC: configs/pinebook-pro-rk3399_defconfig: enable SYS_USB_EVENT_POLL_VIA_INT_QUEUE

[Arnaud Patard]

The default configuration will use SYS_USB_EVENT_POLL for handling the
usb keyboard and it makes the system really slow (eg slow keypress, 
loading kernel/initrd from grub-efi is taking ages).

Using CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE seems to be improving
things a lot, so use it.

Tested-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/configs/pinebook-pro-rk3399_defconfig
===================================================================
--- u-boot.orig/configs/pinebook-pro-rk3399_defconfig
+++ u-boot/configs/pinebook-pro-rk3399_defconfig
@@ -78,6 +78,7 @@ CONFIG_USB_OHCI_GENERIC=y
 CONFIG_USB_DWC3=y
 CONFIG_ROCKCHIP_USB2_PHY=y
 CONFIG_USB_KEYBOARD=y
+CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE=y
 CONFIG_USB_HOST_ETHER=y
 CONFIG_USB_ETHER_ASIX=y
 CONFIG_USB_ETHER_RTL8152=y

[patch 8/8] RFC: drivers/pwm/rk_pwm.c: Fix default polarity

[Arnaud Patard]

In the code, the default polarity is set to positive/positive,
which is neither normal polarity or inverted polarity. It's
only the hardware default. This leads to booting linux with
wrong polarity setting.

Update the code to use PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE
by default instead.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: u-boot/drivers/pwm/rk_pwm.c
===================================================================
--- u-boot.orig/drivers/pwm/rk_pwm.c
+++ u-boot/drivers/pwm/rk_pwm.c
@@ -146,7 +146,7 @@ static int rk_pwm_probe(struct udevice *
 	priv->data = (struct rockchip_pwm_data *)dev_get_driver_data(dev);
 
 	if (priv->data->supports_polarity)
-		priv->conf_polarity = PWM_DUTY_POSTIVE | PWM_INACTIVE_POSTIVE;
+		priv->conf_polarity = PWM_DUTY_POSTIVE | PWM_INACTIVE_NEGATIVE;
 
 	return 0;
 }

[patch 0/8] RFC: Pinebook pro EDP support

[Alper Nebi Yasak]

On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
> This patchset add support for the rk3399 edp. It has been tested on the pinebook
> pro devices. The only missing part is a hack used to get stable edp output after
> a warn reset, which is possibly specific to this device. I'm not sure if it's suitable
> for merge.
> 
> The changes have been written by studying the linux code, since I didn't find any
> manual for theses part of the RK3399 SoC.
> 
> On the linux kernel side, on recent kernels, it needs commit "pwm: rockchip: Keep enabled PWMs
> running while probing" otherwise the pinebook pro will freeze when probing
> the display.
> 
> The kernel is also randomly failing to display something on my device. When this
> occurs, the kernel has this following message:
> rockchip-pm-domain ff310000.power-management:power-controller: failed to set idle on domain 'pd_vopl', val=0
> I've yet to find what's the issue.

I've been trying to test your patches on a rk3399-gru-kevin with these
patches and as a result I've posted some patches for cros_ec_pwm as a
backlight [1], and have some yet-to-be-posted ones for kevin and bob
because I don't truly know if they work [2]. (I don't even have the
appropriate hardware to get a serial console / debug this board, thus
the motivation to get the screen working).

It should be possible for someone with a kevin or bob to tinker on my
test branch and get more conclusive outcomes than I did, maybe even get
everything fully working!

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=204353
[2] https://github.com/alpernebbi/u-boot/commits/rk3399-gru-kevin/wip


Anyway, here's how it goes for me. I start from the Chrome OS firmware
showing some white-background bitmap on the screen with the backlight
enabled. I press CTRL+L to tell it to chainload my U-Boot build. I don't
know if/how chainloading this way affects any of this.

AFAICT, the firmware doesn't clear or turn off the backlight before
doing so, because when VIDEO_ROCKCHIP_MAX_{X,Y}RES={3840,2160} the
screen doesn't clear. I have to set {2400,1600} as it's the resolution
of my panel. Maybe you should test this on Pinebook Pro as well, the
defaults are {1920,1080} (same as its panel), but DISPLAY_ROCKCHIP_HDMI
for example would change it.

With those initial conditions I see the the screen progressively
clearing to black then the backlight turns off after a while. I think
the clearing part hints this patchset is at least doing something.

I had also tried commenting out "enable-gpios" code in pwm_backlight.c
(when I didn't really know how to turn it on properly), that results in
the backlight turn on immediately after it turns off, where I see a
small white artifact (?) on the mid-right part of the bottom of the
screen, for a frame or so.

Overall, I never see any content on the display and it's always black.
Even unsetting CONFIG_SYS_WHITE_ON_BLACK doesn't change anything.

Wish I could've been more helpful, but all this is as far as I could
figure out right now.

[patch 0/8] RFC: Pinebook pro EDP support

[Arnaud Patard]

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

Hi,

> On 25/09/2020 21:36, Arnaud Patard (Rtp) wrote:
>> This patchset add support for the rk3399 edp. It has been tested on the pinebook
>> pro devices. The only missing part is a hack used to get stable edp output after
>> a warn reset, which is possibly specific to this device. I'm not sure if it's suitable
>> for merge.
>> 
>> The changes have been written by studying the linux code, since I didn't find any
>> manual for theses part of the RK3399 SoC.
>> 
>> On the linux kernel side, on recent kernels, it needs commit "pwm: rockchip: Keep enabled PWMs
>> running while probing" otherwise the pinebook pro will freeze when probing
>> the display.
>> 
>> The kernel is also randomly failing to display something on my device. When this
>> occurs, the kernel has this following message:
>> rockchip-pm-domain ff310000.power-management:power-controller: failed to set idle on domain 'pd_vopl', val=0
>> I've yet to find what's the issue.
>
> I've been trying to test your patches on a rk3399-gru-kevin with these
> patches and as a result I've posted some patches for cros_ec_pwm as a
> backlight [1], and have some yet-to-be-posted ones for kevin and bob
> because I don't truly know if they work [2]. (I don't even have the
> appropriate hardware to get a serial console / debug this board, thus
> the motivation to get the screen working).
>
> It should be possible for someone with a kevin or bob to tinker on my
> test branch and get more conclusive outcomes than I did, maybe even get
> everything fully working!
>
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=204353
> [2] https://github.com/alpernebbi/u-boot/commits/rk3399-gru-kevin/wip
>
>
> Anyway, here's how it goes for me. I start from the Chrome OS firmware
> showing some white-background bitmap on the screen with the backlight
> enabled. I press CTRL+L to tell it to chainload my U-Boot build. I don't
> know if/how chainloading this way affects any of this.
>
> AFAICT, the firmware doesn't clear or turn off the backlight before
> doing so, because when VIDEO_ROCKCHIP_MAX_{X,Y}RES={3840,2160} the
> screen doesn't clear. I have to set {2400,1600} as it's the resolution
> of my panel. Maybe you should test this on Pinebook Pro as well, the
> defaults are {1920,1080} (same as its panel), but DISPLAY_ROCKCHIP_HDMI
> for example would change it.

Here, it's set to 1920x1080, and I have CONFIG_DISPLAY_ROCKCHIP_HDMI
disabled, since there's no HDMI output on the PBP. Can you try with the
screen size set to the one of your panel and disable the HDMI output ?
I suspect it won't change anything, but worth trying.

>
> With those initial conditions I see the the screen progressively
> clearing to black then the backlight turns off after a while. I think
> the clearing part hints this patchset is at least doing something.

I see that you've added the needed rockchip,panel properties, so it's
possible it's related to the driver but I'm surprised that the
blacklight turns off. iirc, the driver is only turning it on.

>
> I had also tried commenting out "enable-gpios" code in pwm_backlight.c
> (when I didn't really know how to turn it on properly), that results in
> the backlight turn on immediately after it turns off, where I see a
> small white artifact (?) on the mid-right part of the bottom of the
> screen, for a frame or so.
>
> Overall, I never see any content on the display and it's always black.
> Even unsetting CONFIG_SYS_WHITE_ON_BLACK doesn't change anything.
>
> Wish I could've been more helpful, but all this is as far as I could
> figure out right now.

Some logs would be nice. If USB is working or if the laptop has
ethernet, maybe you could try netconsole (doc/README.NetConsole) with
CONSOLE_MUX. I've not used that for ages, but there's no reason for it
to be broken.

btw, I'm not sure what's the current practice in uboot m-l, but maybe we
can go on debugging your issue off-list ?

Arnaud

[patch 0/8] RFC: Pinebook pro EDP support

[Alper Nebi Yasak]

On 28/09/2020 09:41, Arnaud Patard (Rtp) wrote:
> Here, it's set to 1920x1080, and I have CONFIG_DISPLAY_ROCKCHIP_HDMI
> disabled, since there's no HDMI output on the PBP. Can you try with the
> screen size set to the one of your panel and disable the HDMI output ?
> I suspect it won't change anything, but worth trying.

Disabling DISPLAY_ROCKCHIP_HDMI (and those other than EDP) didn't change
anything with 2400x1600.

(What I was saying is if someone in the future enables it on PBP, the
eDP would likely break; but maybe that already happens with the existing
code in non-rk3399 devices. A minor issue, though.)

> I see that you've added the needed rockchip,panel properties, so it's
> possible it's related to the driver but I'm surprised that the
> blacklight turns off. iirc, the driver is only turning it on.

FWIW, commenting out panel_enable_backlight() in rk_edp.c or adding a
panel_set_backlight() after that call didn't get the backlight to stop
turning off or get it turned on before my loop in board_late_init().
Weird.

> Some logs would be nice. If USB is working or if the laptop has
> ethernet, maybe you could try netconsole (doc/README.NetConsole) with
> CONSOLE_MUX. I've not used that for ages, but there's no reason for it
> to be broken.

It doesn't have ethernet, I don't think USB works either -- assuming you
mean I'd use a USB-to-Ethernet adapter.

> btw, I'm not sure what's the current practice in uboot m-l, but maybe we
> can go on debugging your issue off-list ?

Sure. At least, I guess most of it would be spam-ish to the people
already in Cc.