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

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)

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.

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