On Tue, Nov 22, 2022 at 10:50:30AM +0200, Tomi Valkeinen wrote: > On 17/11/2022 17:46, Kieran Bingham wrote: > > Quoting Tomi Valkeinen (2022-11-17 12:25:46) > >> From: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx> > >> > >> Add DSI support for r8a779g0. The main differences to r8a779a0 are in > >> the PLL and PHTW setups. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 484 +++++++++++++++---- > >> drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h | 6 +- > >> 2 files changed, 384 insertions(+), 106 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> index a7f2b7f66a17..723c35726c38 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c > >> @@ -9,6 +9,7 @@ > >> #include <linux/delay.h> > >> #include <linux/io.h> > >> #include <linux/iopoll.h> > >> +#include <linux/math64.h> > >> #include <linux/module.h> > >> #include <linux/of.h> > >> #include <linux/of_device.h> > >> @@ -28,6 +29,20 @@ > >> #include "rcar_mipi_dsi.h" > >> #include "rcar_mipi_dsi_regs.h" > >> > >> +#define MHZ(v) ((v) * 1000000u) > >> + > >> +enum rcar_mipi_dsi_hw_model { > >> + RCAR_DSI_R8A779A0, > >> + RCAR_DSI_R8A779G0, > >> +}; > >> + > >> +struct rcar_mipi_dsi_device_info { > >> + enum rcar_mipi_dsi_hw_model model; > >> + const struct dsi_clk_config *clk_cfg; > >> + u8 clockset2_m_offset; > >> + u8 clockset2_n_offset; > >> +}; > >> + > >> struct rcar_mipi_dsi { > >> struct device *dev; > >> const struct rcar_mipi_dsi_device_info *info; > >> @@ -50,6 +65,17 @@ struct rcar_mipi_dsi { > >> unsigned int lanes; > >> }; > >> > >> +struct dsi_setup_info { > >> + unsigned long hsfreq; > >> + u16 hsfreqrange; > >> + > >> + unsigned long fout; > >> + u16 m; > >> + u16 n; > >> + u16 vclk_divider; > >> + const struct dsi_clk_config *clkset; > >> +}; > >> + > >> static inline struct rcar_mipi_dsi * > >> bridge_to_rcar_mipi_dsi(struct drm_bridge *bridge) > >> { > >> @@ -62,22 +88,6 @@ host_to_rcar_mipi_dsi(struct mipi_dsi_host *host) > >> return container_of(host, struct rcar_mipi_dsi, host); > >> } > >> > >> -static const u32 phtw[] = { > >> - 0x01020114, 0x01600115, /* General testing */ > >> - 0x01030116, 0x0102011d, /* General testing */ > >> - 0x011101a4, 0x018601a4, /* 1Gbps testing */ > >> - 0x014201a0, 0x010001a3, /* 1Gbps testing */ > >> - 0x0101011f, /* 1Gbps testing */ > >> -}; > >> - > >> -static const u32 phtw2[] = { > >> - 0x010c0130, 0x010c0140, /* General testing */ > >> - 0x010c0150, 0x010c0180, /* General testing */ > >> - 0x010c0190, > >> - 0x010a0160, 0x010a0170, > >> - 0x01800164, 0x01800174, /* 1Gbps testing */ > >> -}; > >> - > >> static const u32 hsfreqrange_table[][2] = { > >> { 80000000U, 0x00 }, { 90000000U, 0x10 }, { 100000000U, 0x20 }, > >> { 110000000U, 0x30 }, { 120000000U, 0x01 }, { 130000000U, 0x11 }, > >> @@ -103,24 +113,53 @@ static const u32 hsfreqrange_table[][2] = { > >> { /* sentinel */ }, > >> }; > >> > >> -struct vco_cntrl_value { > >> +struct dsi_clk_config { > >> u32 min_freq; > >> u32 max_freq; > >> - u16 value; > >> + u8 vco_cntrl; > >> + u8 cpbias_cntrl; > >> + u8 gmp_cntrl; > >> + u8 int_cntrl; > >> + u8 prop_cntrl; > >> }; > >> > >> -static const struct vco_cntrl_value vco_cntrl_table[] = { > >> - { .min_freq = 40000000U, .max_freq = 55000000U, .value = 0x3f }, > >> - { .min_freq = 52500000U, .max_freq = 80000000U, .value = 0x39 }, > >> - { .min_freq = 80000000U, .max_freq = 110000000U, .value = 0x2f }, > >> - { .min_freq = 105000000U, .max_freq = 160000000U, .value = 0x29 }, > >> - { .min_freq = 160000000U, .max_freq = 220000000U, .value = 0x1f }, > >> - { .min_freq = 210000000U, .max_freq = 320000000U, .value = 0x19 }, > >> - { .min_freq = 320000000U, .max_freq = 440000000U, .value = 0x0f }, > >> - { .min_freq = 420000000U, .max_freq = 660000000U, .value = 0x09 }, > >> - { .min_freq = 630000000U, .max_freq = 1149000000U, .value = 0x03 }, > >> - { .min_freq = 1100000000U, .max_freq = 1152000000U, .value = 0x01 }, > >> - { .min_freq = 1150000000U, .max_freq = 1250000000U, .value = 0x01 }, > >> +static const struct dsi_clk_config dsi_clk_cfg_r8a779a0[] = { > >> + { 40000000u, 55000000u, 0x3f, 0x10, 0x01, 0x00, 0x0b }, > >> + { 52500000u, 80000000u, 0x39, 0x10, 0x01, 0x00, 0x0b }, > >> + { 80000000u, 110000000u, 0x2f, 0x10, 0x01, 0x00, 0x0b }, > >> + { 105000000u, 160000000u, 0x29, 0x10, 0x01, 0x00, 0x0b }, > >> + { 160000000u, 220000000u, 0x1f, 0x10, 0x01, 0x00, 0x0b }, > >> + { 210000000u, 320000000u, 0x19, 0x10, 0x01, 0x00, 0x0b }, > >> + { 320000000u, 440000000u, 0x0f, 0x10, 0x01, 0x00, 0x0b }, > >> + { 420000000u, 660000000u, 0x09, 0x10, 0x01, 0x00, 0x0b }, > >> + { 630000000u, 1149000000u, 0x03, 0x10, 0x01, 0x00, 0x0b }, > >> + { 1100000000u, 1152000000u, 0x01, 0x10, 0x01, 0x00, 0x0b }, > >> + { 1150000000u, 1250000000u, 0x01, 0x10, 0x01, 0x00, 0x0c }, > > > > Sigh ... is it this one 0x0c value that means we need to keep all these > > entries repeated ? :-S > > > > If it weren't for that, it seems we could keep just two sets of > >> + u8 cpbias_cntrl; > >> + u8 gmp_cntrl; > >> + u8 int_cntrl; > >> + u8 prop_cntrl; > > > > One for each of the 9a0, and the 9g0... > > > > But this is fine, and I guess the implication is there may be other > > future differences to come in other platforms. > > > > It could be refactored then when we have more visibility. > > Yes, it's not so nice. And afaiu some of these values should really be > solved dynamically in the code. But the docs list these tables and don't > explain how to come up with those values, so... I think having these > tables is the safest way. We could drop the cpbias_cntrl, gmp_cntrl and int_cntrl fields and set them based on the IP version. > >> @@ -427,8 +671,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi, > >> dev_warn(dsi->dev, "unsupported format"); > >> return -EINVAL; > >> } > >> - vclkset |= VCLKSET_COLOR_RGB | VCLKSET_DIV(setup_info.div) > >> - | VCLKSET_LANE(dsi->lanes - 1); > >> + > >> + vclkset |= VCLKSET_COLOR_RGB | VCLKSET_LANE(dsi->lanes - 1); > >> + > >> + switch (dsi->info->model) { > >> + case RCAR_DSI_R8A779A0: > >> + vclkset |= VCLKSET_DIV_R8A779A0(__ffs(setup_info.vclk_divider)); > >> + break; > >> + > >> + case RCAR_DSI_R8A779G0: > >> + vclkset |= VCLKSET_DIV_R8A779G0(__ffs(setup_info.vclk_divider) - 1); > > > > Why is this a -1 here ? Seems an odd difference compared to the A0. > > You need to ask the HW designers =). That's how the register is on G0. > Field value of 0 means divided by 2. -- Regards, Laurent Pinchart