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, ®s->analog_ctl_2);
- writel(REF_CLK_24M, ®s->pll_reg_1);
- u32 reg;
- reg = REF_CLK_24M;
- if (chip_type == RK3288_DP)
reg ^= REF_CLK_MASK;- writel(reg, ®s->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, ®s->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