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.