Alper Nebi Yasak <alpernebiya...@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.pat...@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
