diff mbox series

[v2,7/7] drm: rcar-du: dsi: Add r8a779g0 support

Message ID 20221123065946.40415-8-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series Renesas V4H DSI & DP output support | expand

Commit Message

Tomi Valkeinen Nov. 23, 2022, 6:59 a.m. UTC
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@ideasonboard.com>
---
 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(-)

Comments

Laurent Pinchart Nov. 29, 2022, 1:49 a.m. UTC | #1
Hi Tomi,

Thank you for the patch.

On Wed, Nov 23, 2022 at 08:59:46AM +0200, Tomi Valkeinen wrote:
> 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@ideasonboard.com>
> ---
>  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)

Isn't the U suffix usually spelled in uppercase ? Same below.

> +
> +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 },

Would MHZ(52.5) do the right thing ? If so, I'd use the macro through
those tables.

> +	{   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 },
> +	{ /* sentinel */ },
> +};
> +
> +static const struct dsi_clk_config dsi_clk_cfg_r8a779g0[] = {
> +	{   40000000u,   45310000u, 0x2b, 0x00, 0x00, 0x08, 0x0a },
> +	{   45310000u,   54660000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
> +	{   54660000u,   62500000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
> +	{   62500000u,   75000000u, 0x27, 0x00, 0x00, 0x08, 0x0a },
> +	{   75000000u,   90630000u, 0x23, 0x00, 0x00, 0x08, 0x0a },
> +	{   90630000u,  109370000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
> +	{  109370000u,  125000000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
> +	{  125000000u,  150000000u, 0x1f, 0x00, 0x00, 0x08, 0x0a },
> +	{  150000000u,  181250000u, 0x1b, 0x00, 0x00, 0x08, 0x0a },
> +	{  181250000u,  218750000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
> +	{  218750000u,  250000000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
> +	{  250000000u,  300000000u, 0x17, 0x00, 0x00, 0x08, 0x0a },
> +	{  300000000u,  362500000u, 0x13, 0x00, 0x00, 0x08, 0x0a },
> +	{  362500000u,  455480000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
> +	{  455480000u,  500000000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
> +	{  500000000u,  600000000u, 0x0f, 0x00, 0x00, 0x08, 0x0a },
> +	{  600000000u,  725000000u, 0x0b, 0x00, 0x00, 0x08, 0x0a },
> +	{  725000000u,  875000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
> +	{  875000000u, 1000000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
> +	{ 1000000000u, 1200000000u, 0x07, 0x00, 0x00, 0x08, 0x0a },
> +	{ 1200000000u, 1250000000u, 0x03, 0x00, 0x00, 0x08, 0x0a },
>  	{ /* sentinel */ },
>  };
>  
> @@ -144,7 +183,7 @@ static void rcar_mipi_dsi_set(struct rcar_mipi_dsi *dsi, u32 reg, u32 set)
>  	rcar_mipi_dsi_write(dsi, reg, rcar_mipi_dsi_read(dsi, reg) | set);
>  }
>  
> -static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
> +static int rcar_mipi_dsi_write_phtw(struct rcar_mipi_dsi *dsi, u32 phtw)
>  {
>  	u32 status;
>  	int ret;
> @@ -163,32 +202,231 @@ static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
>  	return ret;
>  }
>  
> +static int rcar_mipi_dsi_write_phtw_arr(struct rcar_mipi_dsi *dsi,
> +					const u32 *phtw, unsigned int size)
> +{
> +	for (unsigned int i = 0; i < size; i++) {
> +		int ret = rcar_mipi_dsi_write_phtw(dsi, phtw[i]);
> +
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +#define WRITE_PHTW(...)                                               \
> +	({                                                            \
> +		static const u32 phtw[] = { __VA_ARGS__ };            \
> +		int ret;                                              \
> +		ret = rcar_mipi_dsi_write_phtw_arr(dsi, phtw,         \
> +						   ARRAY_SIZE(phtw)); \
> +		ret;                                                  \
> +	})
> +
> +static int rcar_mipi_dsi_init_phtw_v3u(struct rcar_mipi_dsi *dsi)

You're mixing the R8A779* names and the short names. I'd use one of the
two and stick to it, probably the short name (up to you).

> +{
> +	return WRITE_PHTW(0x01020114, 0x01600115, 0x01030116, 0x0102011d,
> +			  0x011101a4, 0x018601a4, 0x014201a0, 0x010001a3,
> +			  0x0101011f);
> +}
> +
> +static int rcar_mipi_dsi_post_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
> +{
> +	return WRITE_PHTW(0x010c0130, 0x010c0140, 0x010c0150, 0x010c0180,
> +			  0x010c0190, 0x010a0160, 0x010a0170, 0x01800164,
> +			  0x01800174);
> +}
> +
> +static int rcar_mipi_dsi_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
> +				       const struct dsi_setup_info *setup_info)
> +{
> +	int ret;
> +
> +	if (setup_info->hsfreq < MHZ(450)) {
> +		ret = WRITE_PHTW(0x01010100, 0x011b01ac);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = WRITE_PHTW(0x01010100, 0x01030173, 0x01000174, 0x01500175,
> +			 0x01030176, 0x01040166, 0x010201ad);
> +	if (ret)
> +		return ret;
> +
> +	if (setup_info->hsfreq <= MHZ(1000))
> +		ret = WRITE_PHTW(0x01020100, 0x01910170, 0x01020171,
> +				 0x01110172);
> +	else if (setup_info->hsfreq <= MHZ(1500))
> +		ret = WRITE_PHTW(0x01020100, 0x01980170, 0x01030171,
> +				 0x01100172);
> +	else if (setup_info->hsfreq <= MHZ(2500))
> +		ret = WRITE_PHTW(0x01020100, 0x0144016b, 0x01000172);
> +	else
> +		return -EINVAL;
> +
> +	if (ret)
> +		return ret;
> +
> +	if (dsi->lanes <= 1) {
> +		ret = WRITE_PHTW(0x01070100, 0x010e010b);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (dsi->lanes <= 2) {
> +		ret = WRITE_PHTW(0x01090100, 0x010e010b);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (dsi->lanes <= 3) {
> +		ret = WRITE_PHTW(0x010b0100, 0x010e010b);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (setup_info->hsfreq <= MHZ(1500)) {
> +		ret = WRITE_PHTW(0x01010100, 0x01c0016e);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +rcar_mipi_dsi_post_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
> +				 const struct dsi_setup_info *setup_info)
> +{
> +	u32 status;
> +	int ret;
> +
> +	if (setup_info->hsfreq <= MHZ(1500)) {
> +		WRITE_PHTW(0x01020100, 0x00000180);
> +
> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> +					status & PHTR_TEST, 2000, 10000, false,
> +					dsi, PHTR);
> +		if (ret < 0) {
> +			dev_err(dsi->dev, "failed to test PHTR\n");
> +			return ret;
> +		}
> +
> +		WRITE_PHTW(0x01010100, 0x0100016e);
> +	}
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Hardware Setup
>   */
>  
> -struct dsi_setup_info {
> -	unsigned long fout;
> -	u16 vco_cntrl;
> -	u16 prop_cntrl;
> -	u16 hsfreqrange;
> -	u16 div;
> -	unsigned int m;
> -	unsigned int n;
> -};
> +static void rcar_mipi_dsi_pll_calc_r8a779a0(struct rcar_mipi_dsi *dsi,
> +					    struct clk *clk,
> +					    unsigned long fout_target,
> +					    struct dsi_setup_info *setup_info)
> +{
> +	unsigned int best_err = -1;
> +	unsigned long fin;
> +
> +	fin = clk_get_rate(clk);
> +
> +	for (unsigned int n = 3; n <= 8; n++) {
> +		unsigned long fpfd;
> +
> +		fpfd = fin / n;
> +
> +		if (fpfd < MHZ(2) || fpfd > MHZ(8))
> +			continue;
> +
> +		for (unsigned int m = 64; m <= 625; m++) {
> +			unsigned int err;
> +			u64 fout;
> +
> +			fout = (u64)fpfd * m;
> +
> +			if (fout < MHZ(320) || fout > MHZ(1250))
> +				continue;
> +
> +			fout = div64_u64(fout, setup_info->vclk_divider);
> +
> +			if (fout < setup_info->clkset->min_freq ||
> +			    fout > setup_info->clkset->max_freq)
> +				continue;
> +
> +			err = abs((long)(fout - fout_target) * 10000 /
> +				  (long)fout_target);
> +
> +			if (err < best_err) {
> +				setup_info->m = m;
> +				setup_info->n = n;
> +				setup_info->fout = (unsigned long)fout;
> +				best_err = err;
> +
> +				if (err == 0)
> +					return;
> +			}
> +		}
> +	}
> +}
> +
> +static void rcar_mipi_dsi_pll_calc_r8a779g0(struct rcar_mipi_dsi *dsi,
> +					    struct clk *clk,
> +					    unsigned long fout_target,
> +					    struct dsi_setup_info *setup_info)
> +{
> +	unsigned int best_err = -1;
> +	unsigned long fin;
> +
> +	fin = clk_get_rate(clk);

This could move to the caller.

> +
> +	for (unsigned int n = 1; n <= 8; n++) {
> +		unsigned long fpfd;
> +
> +		fpfd = fin / n;
> +
> +		if (fpfd < MHZ(8) || fpfd > MHZ(24))
> +			continue;
> +
> +		for (unsigned int m = 167; m <= 1000; m++) {
> +			unsigned int err;
> +			u64 fout;
> +
> +			fout = div64_u64((u64)fpfd * m, 2);
> +
> +			if (fout < MHZ(2000) || fout > MHZ(4000))
> +				continue;
> +
> +			fout = div64_u64(fout, setup_info->vclk_divider);
> +
> +			if (fout < setup_info->clkset->min_freq ||
> +			    fout > setup_info->clkset->max_freq)
> +				continue;
> +
> +			err = abs((long)(fout - fout_target) * 10000 /
> +				  (long)fout_target);

Add a blank line here, or remove it from the previous function.

> +			if (err < best_err) {
> +				setup_info->m = m;
> +				setup_info->n = n;
> +				setup_info->fout = (unsigned long)fout;
> +				best_err = err;
> +
> +				if (err == 0)
> +					return;
> +			}
> +		}
> +	}
> +}

This function could be parameterized, up to you.

>  
>  static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>  					  struct clk *clk, unsigned long target,
>  					  struct dsi_setup_info *setup_info)
>  {
>  
> -	const struct vco_cntrl_value *vco_cntrl;
> +	const struct dsi_clk_config *clkset;
>  	unsigned long fout_target;
> -	unsigned long fin, fout;
> -	unsigned long hsfreq;
> -	unsigned int best_err = -1;
> -	unsigned int divider;
> -	unsigned int n;
>  	unsigned int i;
>  	unsigned int err;
>  
> @@ -198,70 +436,53 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>  	 */
>  	fout_target = target * mipi_dsi_pixel_format_to_bpp(dsi->format)
>  		    / (2 * dsi->lanes);
> -	if (fout_target < 40000000 || fout_target > 1250000000)
> +	if (fout_target < MHZ(40) || fout_target > MHZ(1250))
>  		return;
>  
>  	/* Find vco_cntrl */
> -	for (vco_cntrl = vco_cntrl_table; vco_cntrl->min_freq != 0; vco_cntrl++) {
> -		if (fout_target > vco_cntrl->min_freq &&
> -		    fout_target <= vco_cntrl->max_freq) {
> -			setup_info->vco_cntrl = vco_cntrl->value;
> -			if (fout_target >= 1150000000)
> -				setup_info->prop_cntrl = 0x0c;
> -			else
> -				setup_info->prop_cntrl = 0x0b;
> +	for (clkset = dsi->info->clk_cfg; clkset->min_freq != 0; clkset++) {
> +		if (fout_target > clkset->min_freq &&
> +		    fout_target <= clkset->max_freq) {
> +			setup_info->clkset = clkset;
>  			break;
>  		}
>  	}
>  
> -	/* Add divider */
> -	setup_info->div = (setup_info->vco_cntrl & 0x30) >> 4;
> +	switch (dsi->info->model) {
> +	case RCAR_DSI_R8A779A0:
> +		setup_info->vclk_divider = 1 << ((clkset->vco_cntrl >> 4) & 0x3);

If you stored (clkset->vco_cntrl >> 4) & 0x3 in setup_info->vclk_divider
you wouldn't have to use __ffs() in rcar_mipi_dsi_startup(). You could
also drop the - 1 there, which would allow dropping one of the
switch(dsi->info->model). You can store the real divider value in
setup_info separately for rcar_mipi_dsi_pll_calc_r8a779a0(), or pass it
to the function.

> +		rcar_mipi_dsi_pll_calc_r8a779a0(dsi, clk, fout_target, setup_info);
> +		break;
> +
> +	case RCAR_DSI_R8A779G0:
> +		setup_info->vclk_divider = 1 << (((clkset->vco_cntrl >> 3) & 0x7) + 1);
> +		rcar_mipi_dsi_pll_calc_r8a779g0(dsi, clk, fout_target, setup_info);
> +		break;
> +
> +	default:
> +		return;
> +	}
>  
>  	/* Find hsfreqrange */
> -	hsfreq = fout_target * 2;
> +	setup_info->hsfreq = setup_info->fout * 2;
>  	for (i = 0; i < ARRAY_SIZE(hsfreqrange_table); i++) {
> -		if (hsfreqrange_table[i][0] >= hsfreq) {
> +		if (hsfreqrange_table[i][0] >= setup_info->hsfreq) {
>  			setup_info->hsfreqrange = hsfreqrange_table[i][1];
>  			break;
>  		}
>  	}
>  
> -	/*
> -	 * Calculate n and m for PLL clock
> -	 * Following the HW manual the ranges of n and m are
> -	 * n = [3-8] and m = [64-625]
> -	 */

I'd keep the comment in rcar_mipi_dsi_pll_calc_r8a779a0(), and add a
similar comment in rcar_mipi_dsi_pll_calc_r8a779g0().

> -	fin = clk_get_rate(clk);
> -	divider = 1 << setup_info->div;
> -	for (n = 3; n < 9; n++) {
> -		unsigned long fpfd;
> -		unsigned int m;
> -
> -		fpfd = fin / n;
> -
> -		for (m = 64; m < 626; m++) {
> -			fout = fpfd * m / divider;
> -			err = abs((long)(fout - fout_target) * 10000 /
> -				  (long)fout_target);
> -			if (err < best_err) {
> -				setup_info->m = m - 2;
> -				setup_info->n = n - 1;
> -				setup_info->fout = fout;
> -				best_err = err;
> -				if (err == 0)
> -					goto done;
> -			}
> -		}
> -	}
> +	err = abs((long)(setup_info->fout - fout_target) * 10000 / (long)fout_target);
>  
> -done:
>  	dev_dbg(dsi->dev,
> -		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL M/N/DIV %u/%u/%u\n",
> -		clk, fin, setup_info->fout, fout_target, best_err / 100,
> -		best_err % 100, setup_info->m, setup_info->n, setup_info->div);
> +		"Fout = %u * %lu / (2 * %u * %u) = %lu (target %lu Hz, error %d.%02u%%)\n",

Is the "2 *" valid on V3U too ?

> +		setup_info->m, clk_get_rate(clk), setup_info->n, setup_info->vclk_divider,

If you keep the clk_get_rate() call in this function you wouldn't have
to call it again here.

> +		setup_info->fout, fout_target,
> +		err / 100, err % 100);
> +
>  	dev_dbg(dsi->dev,
>  		"vco_cntrl = 0x%x\tprop_cntrl = 0x%x\thsfreqrange = 0x%x\n",
> -		setup_info->vco_cntrl, setup_info->prop_cntrl,
> +		clkset->vco_cntrl, clkset->prop_cntrl,
>  		setup_info->hsfreqrange);
>  }
>  
> @@ -324,7 +545,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  {
>  	struct dsi_setup_info setup_info = {};
>  	unsigned int timeout;
> -	int ret, i;
> +	int ret;
>  	int dsi_format;
>  	u32 phy_setup;
>  	u32 clockset2, clockset3;
> @@ -360,10 +581,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  	phy_setup |= PHYSETUP_HSFREQRANGE(setup_info.hsfreqrange);
>  	rcar_mipi_dsi_write(dsi, PHYSETUP, phy_setup);
>  
> -	for (i = 0; i < ARRAY_SIZE(phtw); i++) {
> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw[i]);
> +	switch (dsi->info->model) {
> +	case RCAR_DSI_R8A779A0:
> +		ret = rcar_mipi_dsi_init_phtw_v3u(dsi);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +
> +	case RCAR_DSI_R8A779G0:
> +		ret = rcar_mipi_dsi_init_phtw_v4h(dsi, &setup_info);
>  		if (ret < 0)
>  			return ret;
> +		break;
> +
> +	default:
> +		return -ENODEV;

This can't happen. Same below.

>  	}
>  
>  	/* PLL Clock Setting */
> @@ -371,12 +603,13 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  	rcar_mipi_dsi_set(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
>  	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
>  
> -	clockset2 = CLOCKSET2_M(setup_info.m) | CLOCKSET2_N(setup_info.n)
> -		  | CLOCKSET2_VCO_CNTRL(setup_info.vco_cntrl);
> -	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.prop_cntrl)
> -		  | CLOCKSET3_INT_CNTRL(0)
> -		  | CLOCKSET3_CPBIAS_CNTRL(0x10)
> -		  | CLOCKSET3_GMP_CNTRL(1);
> +	clockset2 = CLOCKSET2_M(setup_info.m - dsi->info->clockset2_m_offset)
> +		  | CLOCKSET2_N(setup_info.n - dsi->info->clockset2_n_offset)
> +		  | CLOCKSET2_VCO_CNTRL(setup_info.clkset->vco_cntrl);
> +	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.clkset->prop_cntrl)
> +		  | CLOCKSET3_INT_CNTRL(setup_info.clkset->int_cntrl)
> +		  | CLOCKSET3_CPBIAS_CNTRL(setup_info.clkset->cpbias_cntrl)
> +		  | CLOCKSET3_GMP_CNTRL(setup_info.clkset->gmp_cntrl);
>  	rcar_mipi_dsi_write(dsi, CLOCKSET2, clockset2);
>  	rcar_mipi_dsi_write(dsi, CLOCKSET3, clockset3);
>  
> @@ -407,10 +640,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>  		return -ETIMEDOUT;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(phtw2); i++) {
> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw2[i]);
> +	switch (dsi->info->model) {
> +	case RCAR_DSI_R8A779A0:
> +		ret = rcar_mipi_dsi_post_init_phtw_v3u(dsi);
>  		if (ret < 0)
>  			return ret;
> +		break;
> +
> +	case RCAR_DSI_R8A779G0:
> +		ret = rcar_mipi_dsi_post_init_phtw_v4h(dsi, &setup_info);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +
> +	default:
> +		return -ENODEV;
>  	}
>  
>  	/* Enable DOT clock */
> @@ -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);
> +		break;
> +
> +	default:
> +		return -ENODEV;
> +	}
>  
>  	rcar_mipi_dsi_write(dsi, VCLKSET, vclkset);
>  
> @@ -841,8 +1098,25 @@ static int rcar_mipi_dsi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct rcar_mipi_dsi_device_info r8a779a0_data = {
> +	.model = RCAR_DSI_R8A779A0,
> +	.clk_cfg = dsi_clk_cfg_r8a779a0,
> +	.clockset2_m_offset = 2,
> +	.clockset2_n_offset = 1,
> +

Extra blank line.

> +};
> +
> +static const struct rcar_mipi_dsi_device_info r8a779g0_data = {
> +	.model = RCAR_DSI_R8A779G0,
> +	.clk_cfg = dsi_clk_cfg_r8a779g0,
> +	.clockset2_m_offset = 0,
> +	.clockset2_n_offset = 1,

You could possibly drop clockset2_n_offset as it's identical for the two
variants, and there's no indication it would be different in another
version of the IP core.

> +

Here too.

> +};
> +
>  static const struct of_device_id rcar_mipi_dsi_of_table[] = {
> -	{ .compatible = "renesas,r8a779a0-dsi-csi2-tx" },
> +	{ .compatible = "renesas,r8a779a0-dsi-csi2-tx", .data = &r8a779a0_data },
> +	{ .compatible = "renesas,r8a779g0-dsi-csi2-tx", .data = &r8a779g0_data },
>  	{ }
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
> index 2eaca54636f3..608851340acf 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
> @@ -122,7 +122,8 @@
>  #define VCLKSET_CKEN			(1 << 16)
>  #define VCLKSET_COLOR_RGB		(0 << 8)
>  #define VCLKSET_COLOR_YCC		(1 << 8)
> -#define VCLKSET_DIV(x)			(((x) & 0x3) << 4)
> +#define VCLKSET_DIV_R8A779A0(x)		(((x) & 0x3) << 4)
> +#define VCLKSET_DIV_R8A779G0(x)		(((x) & 0x7) << 4)
>  #define VCLKSET_BPP_16			(0 << 2)
>  #define VCLKSET_BPP_18			(1 << 2)
>  #define VCLKSET_BPP_18L			(2 << 2)
> @@ -166,6 +167,9 @@
>  #define PHTW_CWEN			(1 << 8)
>  #define PHTW_TESTDIN_CODE(x)		(((x) & 0xff) << 0)
>  
> +#define PHTR				0x1038
> +#define PHTR_TEST			(1 << 16)
> +
>  #define PHTC				0x103c
>  #define PHTC_TESTCLR			(1 << 0)
>
Tomi Valkeinen Nov. 29, 2022, 11:30 a.m. UTC | #2
On 29/11/2022 03:49, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 23, 2022 at 08:59:46AM +0200, Tomi Valkeinen wrote:
>> 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@ideasonboard.com>
>> ---
>>   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)
> 
> Isn't the U suffix usually spelled in uppercase ? Same below.

I couldn't find any coding style guidelines on that. I like the lower 
case visually. The suffix stands out much clearer on 10000000u than on 
10000000U. But I can change it if you feel otherwise.

>> +
>> +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 },
> 
> Would MHZ(52.5) do the right thing ? If so, I'd use the macro through
> those tables.

That's a great idea. It had never occurred to me that we can use floats 
in the kernel code, as long as we convert to ints when the preprocessor 
is done.

>> +	{   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 },
>> +	{ /* sentinel */ },
>> +};
>> +
>> +static const struct dsi_clk_config dsi_clk_cfg_r8a779g0[] = {
>> +	{   40000000u,   45310000u, 0x2b, 0x00, 0x00, 0x08, 0x0a },
>> +	{   45310000u,   54660000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
>> +	{   54660000u,   62500000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
>> +	{   62500000u,   75000000u, 0x27, 0x00, 0x00, 0x08, 0x0a },
>> +	{   75000000u,   90630000u, 0x23, 0x00, 0x00, 0x08, 0x0a },
>> +	{   90630000u,  109370000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
>> +	{  109370000u,  125000000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
>> +	{  125000000u,  150000000u, 0x1f, 0x00, 0x00, 0x08, 0x0a },
>> +	{  150000000u,  181250000u, 0x1b, 0x00, 0x00, 0x08, 0x0a },
>> +	{  181250000u,  218750000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
>> +	{  218750000u,  250000000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
>> +	{  250000000u,  300000000u, 0x17, 0x00, 0x00, 0x08, 0x0a },
>> +	{  300000000u,  362500000u, 0x13, 0x00, 0x00, 0x08, 0x0a },
>> +	{  362500000u,  455480000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
>> +	{  455480000u,  500000000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
>> +	{  500000000u,  600000000u, 0x0f, 0x00, 0x00, 0x08, 0x0a },
>> +	{  600000000u,  725000000u, 0x0b, 0x00, 0x00, 0x08, 0x0a },
>> +	{  725000000u,  875000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
>> +	{  875000000u, 1000000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
>> +	{ 1000000000u, 1200000000u, 0x07, 0x00, 0x00, 0x08, 0x0a },
>> +	{ 1200000000u, 1250000000u, 0x03, 0x00, 0x00, 0x08, 0x0a },
>>   	{ /* sentinel */ },
>>   };
>>   
>> @@ -144,7 +183,7 @@ static void rcar_mipi_dsi_set(struct rcar_mipi_dsi *dsi, u32 reg, u32 set)
>>   	rcar_mipi_dsi_write(dsi, reg, rcar_mipi_dsi_read(dsi, reg) | set);
>>   }
>>   
>> -static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
>> +static int rcar_mipi_dsi_write_phtw(struct rcar_mipi_dsi *dsi, u32 phtw)
>>   {
>>   	u32 status;
>>   	int ret;
>> @@ -163,32 +202,231 @@ static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
>>   	return ret;
>>   }
>>   
>> +static int rcar_mipi_dsi_write_phtw_arr(struct rcar_mipi_dsi *dsi,
>> +					const u32 *phtw, unsigned int size)
>> +{
>> +	for (unsigned int i = 0; i < size; i++) {
>> +		int ret = rcar_mipi_dsi_write_phtw(dsi, phtw[i]);
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#define WRITE_PHTW(...)                                               \
>> +	({                                                            \
>> +		static const u32 phtw[] = { __VA_ARGS__ };            \
>> +		int ret;                                              \
>> +		ret = rcar_mipi_dsi_write_phtw_arr(dsi, phtw,         \
>> +						   ARRAY_SIZE(phtw)); \
>> +		ret;                                                  \
>> +	})
>> +
>> +static int rcar_mipi_dsi_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
> 
> You're mixing the R8A779* names and the short names. I'd use one of the
> two and stick to it, probably the short name (up to you).

Hmm that's true. The short names are easier to distinguish visually, so 
I'll use those.

>> +{
>> +	return WRITE_PHTW(0x01020114, 0x01600115, 0x01030116, 0x0102011d,
>> +			  0x011101a4, 0x018601a4, 0x014201a0, 0x010001a3,
>> +			  0x0101011f);
>> +}
>> +
>> +static int rcar_mipi_dsi_post_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
>> +{
>> +	return WRITE_PHTW(0x010c0130, 0x010c0140, 0x010c0150, 0x010c0180,
>> +			  0x010c0190, 0x010a0160, 0x010a0170, 0x01800164,
>> +			  0x01800174);
>> +}
>> +
>> +static int rcar_mipi_dsi_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
>> +				       const struct dsi_setup_info *setup_info)
>> +{
>> +	int ret;
>> +
>> +	if (setup_info->hsfreq < MHZ(450)) {
>> +		ret = WRITE_PHTW(0x01010100, 0x011b01ac);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = WRITE_PHTW(0x01010100, 0x01030173, 0x01000174, 0x01500175,
>> +			 0x01030176, 0x01040166, 0x010201ad);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (setup_info->hsfreq <= MHZ(1000))
>> +		ret = WRITE_PHTW(0x01020100, 0x01910170, 0x01020171,
>> +				 0x01110172);
>> +	else if (setup_info->hsfreq <= MHZ(1500))
>> +		ret = WRITE_PHTW(0x01020100, 0x01980170, 0x01030171,
>> +				 0x01100172);
>> +	else if (setup_info->hsfreq <= MHZ(2500))
>> +		ret = WRITE_PHTW(0x01020100, 0x0144016b, 0x01000172);
>> +	else
>> +		return -EINVAL;
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (dsi->lanes <= 1) {
>> +		ret = WRITE_PHTW(0x01070100, 0x010e010b);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (dsi->lanes <= 2) {
>> +		ret = WRITE_PHTW(0x01090100, 0x010e010b);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (dsi->lanes <= 3) {
>> +		ret = WRITE_PHTW(0x010b0100, 0x010e010b);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (setup_info->hsfreq <= MHZ(1500)) {
>> +		ret = WRITE_PHTW(0x01010100, 0x01c0016e);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +rcar_mipi_dsi_post_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
>> +				 const struct dsi_setup_info *setup_info)
>> +{
>> +	u32 status;
>> +	int ret;
>> +
>> +	if (setup_info->hsfreq <= MHZ(1500)) {
>> +		WRITE_PHTW(0x01020100, 0x00000180);
>> +
>> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
>> +					status & PHTR_TEST, 2000, 10000, false,
>> +					dsi, PHTR);
>> +		if (ret < 0) {
>> +			dev_err(dsi->dev, "failed to test PHTR\n");
>> +			return ret;
>> +		}
>> +
>> +		WRITE_PHTW(0x01010100, 0x0100016e);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   /* -----------------------------------------------------------------------------
>>    * Hardware Setup
>>    */
>>   
>> -struct dsi_setup_info {
>> -	unsigned long fout;
>> -	u16 vco_cntrl;
>> -	u16 prop_cntrl;
>> -	u16 hsfreqrange;
>> -	u16 div;
>> -	unsigned int m;
>> -	unsigned int n;
>> -};
>> +static void rcar_mipi_dsi_pll_calc_r8a779a0(struct rcar_mipi_dsi *dsi,
>> +					    struct clk *clk,
>> +					    unsigned long fout_target,
>> +					    struct dsi_setup_info *setup_info)
>> +{
>> +	unsigned int best_err = -1;
>> +	unsigned long fin;
>> +
>> +	fin = clk_get_rate(clk);
>> +
>> +	for (unsigned int n = 3; n <= 8; n++) {
>> +		unsigned long fpfd;
>> +
>> +		fpfd = fin / n;
>> +
>> +		if (fpfd < MHZ(2) || fpfd > MHZ(8))
>> +			continue;
>> +
>> +		for (unsigned int m = 64; m <= 625; m++) {
>> +			unsigned int err;
>> +			u64 fout;
>> +
>> +			fout = (u64)fpfd * m;
>> +
>> +			if (fout < MHZ(320) || fout > MHZ(1250))
>> +				continue;
>> +
>> +			fout = div64_u64(fout, setup_info->vclk_divider);
>> +
>> +			if (fout < setup_info->clkset->min_freq ||
>> +			    fout > setup_info->clkset->max_freq)
>> +				continue;
>> +
>> +			err = abs((long)(fout - fout_target) * 10000 /
>> +				  (long)fout_target);
>> +
>> +			if (err < best_err) {
>> +				setup_info->m = m;
>> +				setup_info->n = n;
>> +				setup_info->fout = (unsigned long)fout;
>> +				best_err = err;
>> +
>> +				if (err == 0)
>> +					return;
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +static void rcar_mipi_dsi_pll_calc_r8a779g0(struct rcar_mipi_dsi *dsi,
>> +					    struct clk *clk,
>> +					    unsigned long fout_target,
>> +					    struct dsi_setup_info *setup_info)
>> +{
>> +	unsigned int best_err = -1;
>> +	unsigned long fin;
>> +
>> +	fin = clk_get_rate(clk);
> 
> This could move to the caller.

Ok.

>> +
>> +	for (unsigned int n = 1; n <= 8; n++) {
>> +		unsigned long fpfd;
>> +
>> +		fpfd = fin / n;
>> +
>> +		if (fpfd < MHZ(8) || fpfd > MHZ(24))
>> +			continue;
>> +
>> +		for (unsigned int m = 167; m <= 1000; m++) {
>> +			unsigned int err;
>> +			u64 fout;
>> +
>> +			fout = div64_u64((u64)fpfd * m, 2);
>> +
>> +			if (fout < MHZ(2000) || fout > MHZ(4000))
>> +				continue;
>> +
>> +			fout = div64_u64(fout, setup_info->vclk_divider);
>> +
>> +			if (fout < setup_info->clkset->min_freq ||
>> +			    fout > setup_info->clkset->max_freq)
>> +				continue;
>> +
>> +			err = abs((long)(fout - fout_target) * 10000 /
>> +				  (long)fout_target);
> 
> Add a blank line here, or remove it from the previous function.

Ok.

>> +			if (err < best_err) {
>> +				setup_info->m = m;
>> +				setup_info->n = n;
>> +				setup_info->fout = (unsigned long)fout;
>> +				best_err = err;
>> +
>> +				if (err == 0)
>> +					return;
>> +			}
>> +		}
>> +	}
>> +}
> 
> This function could be parameterized, up to you.

I thought about it, but it would somewhat obfuscate the code. Now it's 
easier to look at the HW docs and read the function. If we get more PLL 
versions, then I think we'll have to create a parametrized version...

>>   
>>   static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>>   					  struct clk *clk, unsigned long target,
>>   					  struct dsi_setup_info *setup_info)
>>   {
>>   
>> -	const struct vco_cntrl_value *vco_cntrl;
>> +	const struct dsi_clk_config *clkset;
>>   	unsigned long fout_target;
>> -	unsigned long fin, fout;
>> -	unsigned long hsfreq;
>> -	unsigned int best_err = -1;
>> -	unsigned int divider;
>> -	unsigned int n;
>>   	unsigned int i;
>>   	unsigned int err;
>>   
>> @@ -198,70 +436,53 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>>   	 */
>>   	fout_target = target * mipi_dsi_pixel_format_to_bpp(dsi->format)
>>   		    / (2 * dsi->lanes);
>> -	if (fout_target < 40000000 || fout_target > 1250000000)
>> +	if (fout_target < MHZ(40) || fout_target > MHZ(1250))
>>   		return;
>>   
>>   	/* Find vco_cntrl */
>> -	for (vco_cntrl = vco_cntrl_table; vco_cntrl->min_freq != 0; vco_cntrl++) {
>> -		if (fout_target > vco_cntrl->min_freq &&
>> -		    fout_target <= vco_cntrl->max_freq) {
>> -			setup_info->vco_cntrl = vco_cntrl->value;
>> -			if (fout_target >= 1150000000)
>> -				setup_info->prop_cntrl = 0x0c;
>> -			else
>> -				setup_info->prop_cntrl = 0x0b;
>> +	for (clkset = dsi->info->clk_cfg; clkset->min_freq != 0; clkset++) {
>> +		if (fout_target > clkset->min_freq &&
>> +		    fout_target <= clkset->max_freq) {
>> +			setup_info->clkset = clkset;
>>   			break;
>>   		}
>>   	}
>>   
>> -	/* Add divider */
>> -	setup_info->div = (setup_info->vco_cntrl & 0x30) >> 4;
>> +	switch (dsi->info->model) {
>> +	case RCAR_DSI_R8A779A0:
>> +		setup_info->vclk_divider = 1 << ((clkset->vco_cntrl >> 4) & 0x3);
> 
> If you stored (clkset->vco_cntrl >> 4) & 0x3 in setup_info->vclk_divider
> you wouldn't have to use __ffs() in rcar_mipi_dsi_startup(). You could
> also drop the - 1 there, which would allow dropping one of the
> switch(dsi->info->model). You can store the real divider value in
> setup_info separately for rcar_mipi_dsi_pll_calc_r8a779a0(), or pass it
> to the function.
> 
>> +		rcar_mipi_dsi_pll_calc_r8a779a0(dsi, clk, fout_target, setup_info);
>> +		break;
>> +
>> +	case RCAR_DSI_R8A779G0:
>> +		setup_info->vclk_divider = 1 << (((clkset->vco_cntrl >> 3) & 0x7) + 1);
>> +		rcar_mipi_dsi_pll_calc_r8a779g0(dsi, clk, fout_target, setup_info);
>> +		break;
>> +
>> +	default:
>> +		return;
>> +	}
>>   
>>   	/* Find hsfreqrange */
>> -	hsfreq = fout_target * 2;
>> +	setup_info->hsfreq = setup_info->fout * 2;
>>   	for (i = 0; i < ARRAY_SIZE(hsfreqrange_table); i++) {
>> -		if (hsfreqrange_table[i][0] >= hsfreq) {
>> +		if (hsfreqrange_table[i][0] >= setup_info->hsfreq) {
>>   			setup_info->hsfreqrange = hsfreqrange_table[i][1];
>>   			break;
>>   		}
>>   	}
>>   
>> -	/*
>> -	 * Calculate n and m for PLL clock
>> -	 * Following the HW manual the ranges of n and m are
>> -	 * n = [3-8] and m = [64-625]
>> -	 */
> 
> I'd keep the comment in rcar_mipi_dsi_pll_calc_r8a779a0(), and add a
> similar comment in rcar_mipi_dsi_pll_calc_r8a779g0().

Well, I dropped the comment as it's just pointing the obvious: we are 
calculating PLL config, and the n and m ranges are easily visible in the 
code below. I can add them if you think they are useful.

>> -	fin = clk_get_rate(clk);
>> -	divider = 1 << setup_info->div;
>> -	for (n = 3; n < 9; n++) {
>> -		unsigned long fpfd;
>> -		unsigned int m;
>> -
>> -		fpfd = fin / n;
>> -
>> -		for (m = 64; m < 626; m++) {
>> -			fout = fpfd * m / divider;
>> -			err = abs((long)(fout - fout_target) * 10000 /
>> -				  (long)fout_target);
>> -			if (err < best_err) {
>> -				setup_info->m = m - 2;
>> -				setup_info->n = n - 1;
>> -				setup_info->fout = fout;
>> -				best_err = err;
>> -				if (err == 0)
>> -					goto done;
>> -			}
>> -		}
>> -	}
>> +	err = abs((long)(setup_info->fout - fout_target) * 10000 / (long)fout_target);
>>   
>> -done:
>>   	dev_dbg(dsi->dev,
>> -		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL M/N/DIV %u/%u/%u\n",
>> -		clk, fin, setup_info->fout, fout_target, best_err / 100,
>> -		best_err % 100, setup_info->m, setup_info->n, setup_info->div);
>> +		"Fout = %u * %lu / (2 * %u * %u) = %lu (target %lu Hz, error %d.%02u%%)\n",
> 
> Is the "2 *" valid on V3U too ?

Good catch. It is not.

>> +		setup_info->m, clk_get_rate(clk), setup_info->n, setup_info->vclk_divider,
> 
> If you keep the clk_get_rate() call in this function you wouldn't have
> to call it again here.

Yep.

>> +		setup_info->fout, fout_target,
>> +		err / 100, err % 100);
>> +
>>   	dev_dbg(dsi->dev,
>>   		"vco_cntrl = 0x%x\tprop_cntrl = 0x%x\thsfreqrange = 0x%x\n",
>> -		setup_info->vco_cntrl, setup_info->prop_cntrl,
>> +		clkset->vco_cntrl, clkset->prop_cntrl,
>>   		setup_info->hsfreqrange);
>>   }
>>   
>> @@ -324,7 +545,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>   {
>>   	struct dsi_setup_info setup_info = {};
>>   	unsigned int timeout;
>> -	int ret, i;
>> +	int ret;
>>   	int dsi_format;
>>   	u32 phy_setup;
>>   	u32 clockset2, clockset3;
>> @@ -360,10 +581,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>   	phy_setup |= PHYSETUP_HSFREQRANGE(setup_info.hsfreqrange);
>>   	rcar_mipi_dsi_write(dsi, PHYSETUP, phy_setup);
>>   
>> -	for (i = 0; i < ARRAY_SIZE(phtw); i++) {
>> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw[i]);
>> +	switch (dsi->info->model) {
>> +	case RCAR_DSI_R8A779A0:
>> +		ret = rcar_mipi_dsi_init_phtw_v3u(dsi);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +
>> +	case RCAR_DSI_R8A779G0:
>> +		ret = rcar_mipi_dsi_init_phtw_v4h(dsi, &setup_info);
>>   		if (ret < 0)
>>   			return ret;
>> +		break;
>> +
>> +	default:
>> +		return -ENODEV;
> 
> This can't happen. Same below.

I thought the compiler would warn about it, but at least mine doesn't, 
so I'll drop the default cases.

>>   	}
>>   
>>   	/* PLL Clock Setting */
>> @@ -371,12 +603,13 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>   	rcar_mipi_dsi_set(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
>>   	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
>>   
>> -	clockset2 = CLOCKSET2_M(setup_info.m) | CLOCKSET2_N(setup_info.n)
>> -		  | CLOCKSET2_VCO_CNTRL(setup_info.vco_cntrl);
>> -	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.prop_cntrl)
>> -		  | CLOCKSET3_INT_CNTRL(0)
>> -		  | CLOCKSET3_CPBIAS_CNTRL(0x10)
>> -		  | CLOCKSET3_GMP_CNTRL(1);
>> +	clockset2 = CLOCKSET2_M(setup_info.m - dsi->info->clockset2_m_offset)
>> +		  | CLOCKSET2_N(setup_info.n - dsi->info->clockset2_n_offset)
>> +		  | CLOCKSET2_VCO_CNTRL(setup_info.clkset->vco_cntrl);
>> +	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.clkset->prop_cntrl)
>> +		  | CLOCKSET3_INT_CNTRL(setup_info.clkset->int_cntrl)
>> +		  | CLOCKSET3_CPBIAS_CNTRL(setup_info.clkset->cpbias_cntrl)
>> +		  | CLOCKSET3_GMP_CNTRL(setup_info.clkset->gmp_cntrl);
>>   	rcar_mipi_dsi_write(dsi, CLOCKSET2, clockset2);
>>   	rcar_mipi_dsi_write(dsi, CLOCKSET3, clockset3);
>>   
>> @@ -407,10 +640,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
>>   		return -ETIMEDOUT;
>>   	}
>>   
>> -	for (i = 0; i < ARRAY_SIZE(phtw2); i++) {
>> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw2[i]);
>> +	switch (dsi->info->model) {
>> +	case RCAR_DSI_R8A779A0:
>> +		ret = rcar_mipi_dsi_post_init_phtw_v3u(dsi);
>>   		if (ret < 0)
>>   			return ret;
>> +		break;
>> +
>> +	case RCAR_DSI_R8A779G0:
>> +		ret = rcar_mipi_dsi_post_init_phtw_v4h(dsi, &setup_info);
>> +		if (ret < 0)
>> +			return ret;
>> +		break;
>> +
>> +	default:
>> +		return -ENODEV;
>>   	}
>>   
>>   	/* Enable DOT clock */
>> @@ -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);
>> +		break;
>> +
>> +	default:
>> +		return -ENODEV;
>> +	}
>>   
>>   	rcar_mipi_dsi_write(dsi, VCLKSET, vclkset);
>>   
>> @@ -841,8 +1098,25 @@ static int rcar_mipi_dsi_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct rcar_mipi_dsi_device_info r8a779a0_data = {
>> +	.model = RCAR_DSI_R8A779A0,
>> +	.clk_cfg = dsi_clk_cfg_r8a779a0,
>> +	.clockset2_m_offset = 2,
>> +	.clockset2_n_offset = 1,
>> +
> 
> Extra blank line.

Yep.

> 
>> +};
>> +
>> +static const struct rcar_mipi_dsi_device_info r8a779g0_data = {
>> +	.model = RCAR_DSI_R8A779G0,
>> +	.clk_cfg = dsi_clk_cfg_r8a779g0,
>> +	.clockset2_m_offset = 0,
>> +	.clockset2_n_offset = 1,
> 
> You could possibly drop clockset2_n_offset as it's identical for the two
> variants, and there's no indication it would be different in another
> version of the IP core.

Yep, I think that's fine.

  Tomi
Biju Das Nov. 29, 2022, 11:40 a.m. UTC | #3
> -----Original Message-----
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Sent: 29 November 2022 11:30
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>; Rob
> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Magnus Damm <magnus.damm@gmail.com>; dri-
> devel@lists.freedesktop.org; linux-renesas-soc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Andrzej Hajda
> <andrzej.hajda@intel.com>; Neil Armstrong <neil.armstrong@linaro.org>;
> Robert Foss <robert.foss@linaro.org>; Jonas Karlman <jonas@kwiboo.se>;
> Jernej Skrabec <jernej.skrabec@gmail.com>
> Subject: Re: [PATCH v2 7/7] drm: rcar-du: dsi: Add r8a779g0 support
> 
> On 29/11/2022 03:49, Laurent Pinchart wrote:
> > Hi Tomi,
> >
> > Thank you for the patch.
> >
> > On Wed, Nov 23, 2022 at 08:59:46AM +0200, Tomi Valkeinen wrote:
> >> 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@ideasonboard.com>
> >> ---
> >>   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)
> >
> > Isn't the U suffix usually spelled in uppercase ? Same below.
> 
> I couldn't find any coding style guidelines on that. I like the lower
> case visually. The suffix stands out much clearer on 10000000u than on
> 10000000U. But I can change it if you feel otherwise.

https://elixir.bootlin.com/linux/v6.1-rc7/source/include/linux/units.h#L11

Maybe you could add MHZ here??

Cheers,
Biju
Geert Uytterhoeven Nov. 29, 2022, 11:49 a.m. UTC | #4
Hi Biju,

On Tue, Nov 29, 2022 at 12:40 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> > On 29/11/2022 03:49, Laurent Pinchart wrote:
> > > On Wed, Nov 23, 2022 at 08:59:46AM +0200, Tomi Valkeinen wrote:
> > >> 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@ideasonboard.com>

> > >> --- 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)
> > >
> > > Isn't the U suffix usually spelled in uppercase ? Same below.
> >
> > I couldn't find any coding style guidelines on that. I like the lower
> > case visually. The suffix stands out much clearer on 10000000u than on
> > 10000000U. But I can change it if you feel otherwise.
>
> https://elixir.bootlin.com/linux/v6.1-rc7/source/include/linux/units.h#L11
>
> Maybe you could add MHZ here??

Or use the existing MEGA? The metric system is soooo nice! ;-)

BTW, looks like the people adding definitions like

    #define MICROHZ_PER_HZ 1000000UL
    #define MILLIHZ_PER_HZ 1000UL

    #define MILLIWATT_PER_WATT 1000UL
    #define MICROWATT_PER_MILLIWATT 1000UL
    #define MICROWATT_PER_WATT 1000000UL

didn't grasp the full power...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Laurent Pinchart Nov. 29, 2022, 12:05 p.m. UTC | #5
Hi Tomi,

On Tue, Nov 29, 2022 at 01:30:04PM +0200, Tomi Valkeinen wrote:
> On 29/11/2022 03:49, Laurent Pinchart wrote:
> > On Wed, Nov 23, 2022 at 08:59:46AM +0200, Tomi Valkeinen wrote:
> >> 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@ideasonboard.com>
> >> ---
> >>   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)
> > 
> > Isn't the U suffix usually spelled in uppercase ? Same below.
> 
> I couldn't find any coding style guidelines on that. I like the lower 
> case visually. The suffix stands out much clearer on 10000000u than on 
> 10000000U. But I can change it if you feel otherwise.

The driver uses U already and I like consistency. You can change all of
them to u if you want :-)

> >> +
> >> +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 },
> > 
> > Would MHZ(52.5) do the right thing ? If so, I'd use the macro through
> > those tables.
> 
> That's a great idea. It had never occurred to me that we can use floats 
> in the kernel code, as long as we convert to ints when the preprocessor 
> is done.
> 
> >> +	{   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 },
> >> +	{ /* sentinel */ },
> >> +};
> >> +
> >> +static const struct dsi_clk_config dsi_clk_cfg_r8a779g0[] = {
> >> +	{   40000000u,   45310000u, 0x2b, 0x00, 0x00, 0x08, 0x0a },
> >> +	{   45310000u,   54660000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
> >> +	{   54660000u,   62500000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
> >> +	{   62500000u,   75000000u, 0x27, 0x00, 0x00, 0x08, 0x0a },
> >> +	{   75000000u,   90630000u, 0x23, 0x00, 0x00, 0x08, 0x0a },
> >> +	{   90630000u,  109370000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  109370000u,  125000000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  125000000u,  150000000u, 0x1f, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  150000000u,  181250000u, 0x1b, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  181250000u,  218750000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  218750000u,  250000000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  250000000u,  300000000u, 0x17, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  300000000u,  362500000u, 0x13, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  362500000u,  455480000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  455480000u,  500000000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  500000000u,  600000000u, 0x0f, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  600000000u,  725000000u, 0x0b, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  725000000u,  875000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
> >> +	{  875000000u, 1000000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
> >> +	{ 1000000000u, 1200000000u, 0x07, 0x00, 0x00, 0x08, 0x0a },
> >> +	{ 1200000000u, 1250000000u, 0x03, 0x00, 0x00, 0x08, 0x0a },
> >>   	{ /* sentinel */ },
> >>   };
> >>   
> >> @@ -144,7 +183,7 @@ static void rcar_mipi_dsi_set(struct rcar_mipi_dsi *dsi, u32 reg, u32 set)
> >>   	rcar_mipi_dsi_write(dsi, reg, rcar_mipi_dsi_read(dsi, reg) | set);
> >>   }
> >>   
> >> -static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
> >> +static int rcar_mipi_dsi_write_phtw(struct rcar_mipi_dsi *dsi, u32 phtw)
> >>   {
> >>   	u32 status;
> >>   	int ret;
> >> @@ -163,32 +202,231 @@ static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
> >>   	return ret;
> >>   }
> >>   
> >> +static int rcar_mipi_dsi_write_phtw_arr(struct rcar_mipi_dsi *dsi,
> >> +					const u32 *phtw, unsigned int size)
> >> +{
> >> +	for (unsigned int i = 0; i < size; i++) {
> >> +		int ret = rcar_mipi_dsi_write_phtw(dsi, phtw[i]);
> >> +
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +#define WRITE_PHTW(...)                                               \
> >> +	({                                                            \
> >> +		static const u32 phtw[] = { __VA_ARGS__ };            \
> >> +		int ret;                                              \
> >> +		ret = rcar_mipi_dsi_write_phtw_arr(dsi, phtw,         \
> >> +						   ARRAY_SIZE(phtw)); \
> >> +		ret;                                                  \
> >> +	})
> >> +
> >> +static int rcar_mipi_dsi_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
> > 
> > You're mixing the R8A779* names and the short names. I'd use one of the
> > two and stick to it, probably the short name (up to you).
> 
> Hmm that's true. The short names are easier to distinguish visually, so 
> I'll use those.
> 
> >> +{
> >> +	return WRITE_PHTW(0x01020114, 0x01600115, 0x01030116, 0x0102011d,
> >> +			  0x011101a4, 0x018601a4, 0x014201a0, 0x010001a3,
> >> +			  0x0101011f);
> >> +}
> >> +
> >> +static int rcar_mipi_dsi_post_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
> >> +{
> >> +	return WRITE_PHTW(0x010c0130, 0x010c0140, 0x010c0150, 0x010c0180,
> >> +			  0x010c0190, 0x010a0160, 0x010a0170, 0x01800164,
> >> +			  0x01800174);
> >> +}
> >> +
> >> +static int rcar_mipi_dsi_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
> >> +				       const struct dsi_setup_info *setup_info)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (setup_info->hsfreq < MHZ(450)) {
> >> +		ret = WRITE_PHTW(0x01010100, 0x011b01ac);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	ret = WRITE_PHTW(0x01010100, 0x01030173, 0x01000174, 0x01500175,
> >> +			 0x01030176, 0x01040166, 0x010201ad);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (setup_info->hsfreq <= MHZ(1000))
> >> +		ret = WRITE_PHTW(0x01020100, 0x01910170, 0x01020171,
> >> +				 0x01110172);
> >> +	else if (setup_info->hsfreq <= MHZ(1500))
> >> +		ret = WRITE_PHTW(0x01020100, 0x01980170, 0x01030171,
> >> +				 0x01100172);
> >> +	else if (setup_info->hsfreq <= MHZ(2500))
> >> +		ret = WRITE_PHTW(0x01020100, 0x0144016b, 0x01000172);
> >> +	else
> >> +		return -EINVAL;
> >> +
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (dsi->lanes <= 1) {
> >> +		ret = WRITE_PHTW(0x01070100, 0x010e010b);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (dsi->lanes <= 2) {
> >> +		ret = WRITE_PHTW(0x01090100, 0x010e010b);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (dsi->lanes <= 3) {
> >> +		ret = WRITE_PHTW(0x010b0100, 0x010e010b);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	if (setup_info->hsfreq <= MHZ(1500)) {
> >> +		ret = WRITE_PHTW(0x01010100, 0x01c0016e);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int
> >> +rcar_mipi_dsi_post_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
> >> +				 const struct dsi_setup_info *setup_info)
> >> +{
> >> +	u32 status;
> >> +	int ret;
> >> +
> >> +	if (setup_info->hsfreq <= MHZ(1500)) {
> >> +		WRITE_PHTW(0x01020100, 0x00000180);
> >> +
> >> +		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
> >> +					status & PHTR_TEST, 2000, 10000, false,
> >> +					dsi, PHTR);
> >> +		if (ret < 0) {
> >> +			dev_err(dsi->dev, "failed to test PHTR\n");
> >> +			return ret;
> >> +		}
> >> +
> >> +		WRITE_PHTW(0x01010100, 0x0100016e);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   /* -----------------------------------------------------------------------------
> >>    * Hardware Setup
> >>    */
> >>   
> >> -struct dsi_setup_info {
> >> -	unsigned long fout;
> >> -	u16 vco_cntrl;
> >> -	u16 prop_cntrl;
> >> -	u16 hsfreqrange;
> >> -	u16 div;
> >> -	unsigned int m;
> >> -	unsigned int n;
> >> -};
> >> +static void rcar_mipi_dsi_pll_calc_r8a779a0(struct rcar_mipi_dsi *dsi,
> >> +					    struct clk *clk,
> >> +					    unsigned long fout_target,
> >> +					    struct dsi_setup_info *setup_info)
> >> +{
> >> +	unsigned int best_err = -1;
> >> +	unsigned long fin;
> >> +
> >> +	fin = clk_get_rate(clk);
> >> +
> >> +	for (unsigned int n = 3; n <= 8; n++) {
> >> +		unsigned long fpfd;
> >> +
> >> +		fpfd = fin / n;
> >> +
> >> +		if (fpfd < MHZ(2) || fpfd > MHZ(8))
> >> +			continue;
> >> +
> >> +		for (unsigned int m = 64; m <= 625; m++) {
> >> +			unsigned int err;
> >> +			u64 fout;
> >> +
> >> +			fout = (u64)fpfd * m;
> >> +
> >> +			if (fout < MHZ(320) || fout > MHZ(1250))
> >> +				continue;
> >> +
> >> +			fout = div64_u64(fout, setup_info->vclk_divider);
> >> +
> >> +			if (fout < setup_info->clkset->min_freq ||
> >> +			    fout > setup_info->clkset->max_freq)
> >> +				continue;
> >> +
> >> +			err = abs((long)(fout - fout_target) * 10000 /
> >> +				  (long)fout_target);
> >> +
> >> +			if (err < best_err) {
> >> +				setup_info->m = m;
> >> +				setup_info->n = n;
> >> +				setup_info->fout = (unsigned long)fout;
> >> +				best_err = err;
> >> +
> >> +				if (err == 0)
> >> +					return;
> >> +			}
> >> +		}
> >> +	}
> >> +}
> >> +
> >> +static void rcar_mipi_dsi_pll_calc_r8a779g0(struct rcar_mipi_dsi *dsi,
> >> +					    struct clk *clk,
> >> +					    unsigned long fout_target,
> >> +					    struct dsi_setup_info *setup_info)
> >> +{
> >> +	unsigned int best_err = -1;
> >> +	unsigned long fin;
> >> +
> >> +	fin = clk_get_rate(clk);
> > 
> > This could move to the caller.
> 
> Ok.
> 
> >> +
> >> +	for (unsigned int n = 1; n <= 8; n++) {
> >> +		unsigned long fpfd;
> >> +
> >> +		fpfd = fin / n;
> >> +
> >> +		if (fpfd < MHZ(8) || fpfd > MHZ(24))
> >> +			continue;
> >> +
> >> +		for (unsigned int m = 167; m <= 1000; m++) {
> >> +			unsigned int err;
> >> +			u64 fout;
> >> +
> >> +			fout = div64_u64((u64)fpfd * m, 2);
> >> +
> >> +			if (fout < MHZ(2000) || fout > MHZ(4000))
> >> +				continue;
> >> +
> >> +			fout = div64_u64(fout, setup_info->vclk_divider);
> >> +
> >> +			if (fout < setup_info->clkset->min_freq ||
> >> +			    fout > setup_info->clkset->max_freq)
> >> +				continue;
> >> +
> >> +			err = abs((long)(fout - fout_target) * 10000 /
> >> +				  (long)fout_target);
> > 
> > Add a blank line here, or remove it from the previous function.
> 
> Ok.
> 
> >> +			if (err < best_err) {
> >> +				setup_info->m = m;
> >> +				setup_info->n = n;
> >> +				setup_info->fout = (unsigned long)fout;
> >> +				best_err = err;
> >> +
> >> +				if (err == 0)
> >> +					return;
> >> +			}
> >> +		}
> >> +	}
> >> +}
> > 
> > This function could be parameterized, up to you.
> 
> I thought about it, but it would somewhat obfuscate the code. Now it's 
> easier to look at the HW docs and read the function. If we get more PLL 
> versions, then I think we'll have to create a parametrized version...
> 
> >>   
> >>   static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
> >>   					  struct clk *clk, unsigned long target,
> >>   					  struct dsi_setup_info *setup_info)
> >>   {
> >>   
> >> -	const struct vco_cntrl_value *vco_cntrl;
> >> +	const struct dsi_clk_config *clkset;
> >>   	unsigned long fout_target;
> >> -	unsigned long fin, fout;
> >> -	unsigned long hsfreq;
> >> -	unsigned int best_err = -1;
> >> -	unsigned int divider;
> >> -	unsigned int n;
> >>   	unsigned int i;
> >>   	unsigned int err;
> >>   
> >> @@ -198,70 +436,53 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
> >>   	 */
> >>   	fout_target = target * mipi_dsi_pixel_format_to_bpp(dsi->format)
> >>   		    / (2 * dsi->lanes);
> >> -	if (fout_target < 40000000 || fout_target > 1250000000)
> >> +	if (fout_target < MHZ(40) || fout_target > MHZ(1250))
> >>   		return;
> >>   
> >>   	/* Find vco_cntrl */
> >> -	for (vco_cntrl = vco_cntrl_table; vco_cntrl->min_freq != 0; vco_cntrl++) {
> >> -		if (fout_target > vco_cntrl->min_freq &&
> >> -		    fout_target <= vco_cntrl->max_freq) {
> >> -			setup_info->vco_cntrl = vco_cntrl->value;
> >> -			if (fout_target >= 1150000000)
> >> -				setup_info->prop_cntrl = 0x0c;
> >> -			else
> >> -				setup_info->prop_cntrl = 0x0b;
> >> +	for (clkset = dsi->info->clk_cfg; clkset->min_freq != 0; clkset++) {
> >> +		if (fout_target > clkset->min_freq &&
> >> +		    fout_target <= clkset->max_freq) {
> >> +			setup_info->clkset = clkset;
> >>   			break;
> >>   		}
> >>   	}
> >>   
> >> -	/* Add divider */
> >> -	setup_info->div = (setup_info->vco_cntrl & 0x30) >> 4;
> >> +	switch (dsi->info->model) {
> >> +	case RCAR_DSI_R8A779A0:
> >> +		setup_info->vclk_divider = 1 << ((clkset->vco_cntrl >> 4) & 0x3);
> > 
> > If you stored (clkset->vco_cntrl >> 4) & 0x3 in setup_info->vclk_divider
> > you wouldn't have to use __ffs() in rcar_mipi_dsi_startup(). You could
> > also drop the - 1 there, which would allow dropping one of the
> > switch(dsi->info->model). You can store the real divider value in
> > setup_info separately for rcar_mipi_dsi_pll_calc_r8a779a0(), or pass it
> > to the function.
> > 
> >> +		rcar_mipi_dsi_pll_calc_r8a779a0(dsi, clk, fout_target, setup_info);
> >> +		break;
> >> +
> >> +	case RCAR_DSI_R8A779G0:
> >> +		setup_info->vclk_divider = 1 << (((clkset->vco_cntrl >> 3) & 0x7) + 1);
> >> +		rcar_mipi_dsi_pll_calc_r8a779g0(dsi, clk, fout_target, setup_info);
> >> +		break;
> >> +
> >> +	default:
> >> +		return;
> >> +	}
> >>   
> >>   	/* Find hsfreqrange */
> >> -	hsfreq = fout_target * 2;
> >> +	setup_info->hsfreq = setup_info->fout * 2;
> >>   	for (i = 0; i < ARRAY_SIZE(hsfreqrange_table); i++) {
> >> -		if (hsfreqrange_table[i][0] >= hsfreq) {
> >> +		if (hsfreqrange_table[i][0] >= setup_info->hsfreq) {
> >>   			setup_info->hsfreqrange = hsfreqrange_table[i][1];
> >>   			break;
> >>   		}
> >>   	}
> >>   
> >> -	/*
> >> -	 * Calculate n and m for PLL clock
> >> -	 * Following the HW manual the ranges of n and m are
> >> -	 * n = [3-8] and m = [64-625]
> >> -	 */
> > 
> > I'd keep the comment in rcar_mipi_dsi_pll_calc_r8a779a0(), and add a
> > similar comment in rcar_mipi_dsi_pll_calc_r8a779g0().
> 
> Well, I dropped the comment as it's just pointing the obvious: we are 
> calculating PLL config, and the n and m ranges are easily visible in the 
> code below. I can add them if you think they are useful.

Up to you.

> >> -	fin = clk_get_rate(clk);
> >> -	divider = 1 << setup_info->div;
> >> -	for (n = 3; n < 9; n++) {
> >> -		unsigned long fpfd;
> >> -		unsigned int m;
> >> -
> >> -		fpfd = fin / n;
> >> -
> >> -		for (m = 64; m < 626; m++) {
> >> -			fout = fpfd * m / divider;
> >> -			err = abs((long)(fout - fout_target) * 10000 /
> >> -				  (long)fout_target);
> >> -			if (err < best_err) {
> >> -				setup_info->m = m - 2;
> >> -				setup_info->n = n - 1;
> >> -				setup_info->fout = fout;
> >> -				best_err = err;
> >> -				if (err == 0)
> >> -					goto done;
> >> -			}
> >> -		}
> >> -	}
> >> +	err = abs((long)(setup_info->fout - fout_target) * 10000 / (long)fout_target);
> >>   
> >> -done:
> >>   	dev_dbg(dsi->dev,
> >> -		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL M/N/DIV %u/%u/%u\n",
> >> -		clk, fin, setup_info->fout, fout_target, best_err / 100,
> >> -		best_err % 100, setup_info->m, setup_info->n, setup_info->div);
> >> +		"Fout = %u * %lu / (2 * %u * %u) = %lu (target %lu Hz, error %d.%02u%%)\n",
> > 
> > Is the "2 *" valid on V3U too ?
> 
> Good catch. It is not.
> 
> >> +		setup_info->m, clk_get_rate(clk), setup_info->n, setup_info->vclk_divider,
> > 
> > If you keep the clk_get_rate() call in this function you wouldn't have
> > to call it again here.
> 
> Yep.
> 
> >> +		setup_info->fout, fout_target,
> >> +		err / 100, err % 100);
> >> +
> >>   	dev_dbg(dsi->dev,
> >>   		"vco_cntrl = 0x%x\tprop_cntrl = 0x%x\thsfreqrange = 0x%x\n",
> >> -		setup_info->vco_cntrl, setup_info->prop_cntrl,
> >> +		clkset->vco_cntrl, clkset->prop_cntrl,
> >>   		setup_info->hsfreqrange);
> >>   }
> >>   
> >> @@ -324,7 +545,7 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
> >>   {
> >>   	struct dsi_setup_info setup_info = {};
> >>   	unsigned int timeout;
> >> -	int ret, i;
> >> +	int ret;
> >>   	int dsi_format;
> >>   	u32 phy_setup;
> >>   	u32 clockset2, clockset3;
> >> @@ -360,10 +581,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
> >>   	phy_setup |= PHYSETUP_HSFREQRANGE(setup_info.hsfreqrange);
> >>   	rcar_mipi_dsi_write(dsi, PHYSETUP, phy_setup);
> >>   
> >> -	for (i = 0; i < ARRAY_SIZE(phtw); i++) {
> >> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw[i]);
> >> +	switch (dsi->info->model) {
> >> +	case RCAR_DSI_R8A779A0:
> >> +		ret = rcar_mipi_dsi_init_phtw_v3u(dsi);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		break;
> >> +
> >> +	case RCAR_DSI_R8A779G0:
> >> +		ret = rcar_mipi_dsi_init_phtw_v4h(dsi, &setup_info);
> >>   		if (ret < 0)
> >>   			return ret;
> >> +		break;
> >> +
> >> +	default:
> >> +		return -ENODEV;
> > 
> > This can't happen. Same below.
> 
> I thought the compiler would warn about it, but at least mine doesn't, 
> so I'll drop the default cases.

smatch or other static checkers may complain. You can add a default case
to one of the labels to avoid that.

> >>   	}
> >>   
> >>   	/* PLL Clock Setting */
> >> @@ -371,12 +603,13 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
> >>   	rcar_mipi_dsi_set(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
> >>   	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
> >>   
> >> -	clockset2 = CLOCKSET2_M(setup_info.m) | CLOCKSET2_N(setup_info.n)
> >> -		  | CLOCKSET2_VCO_CNTRL(setup_info.vco_cntrl);
> >> -	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.prop_cntrl)
> >> -		  | CLOCKSET3_INT_CNTRL(0)
> >> -		  | CLOCKSET3_CPBIAS_CNTRL(0x10)
> >> -		  | CLOCKSET3_GMP_CNTRL(1);
> >> +	clockset2 = CLOCKSET2_M(setup_info.m - dsi->info->clockset2_m_offset)
> >> +		  | CLOCKSET2_N(setup_info.n - dsi->info->clockset2_n_offset)
> >> +		  | CLOCKSET2_VCO_CNTRL(setup_info.clkset->vco_cntrl);
> >> +	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.clkset->prop_cntrl)
> >> +		  | CLOCKSET3_INT_CNTRL(setup_info.clkset->int_cntrl)
> >> +		  | CLOCKSET3_CPBIAS_CNTRL(setup_info.clkset->cpbias_cntrl)
> >> +		  | CLOCKSET3_GMP_CNTRL(setup_info.clkset->gmp_cntrl);
> >>   	rcar_mipi_dsi_write(dsi, CLOCKSET2, clockset2);
> >>   	rcar_mipi_dsi_write(dsi, CLOCKSET3, clockset3);
> >>   
> >> @@ -407,10 +640,21 @@ static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
> >>   		return -ETIMEDOUT;
> >>   	}
> >>   
> >> -	for (i = 0; i < ARRAY_SIZE(phtw2); i++) {
> >> -		ret = rcar_mipi_dsi_phtw_test(dsi, phtw2[i]);
> >> +	switch (dsi->info->model) {
> >> +	case RCAR_DSI_R8A779A0:
> >> +		ret = rcar_mipi_dsi_post_init_phtw_v3u(dsi);
> >>   		if (ret < 0)
> >>   			return ret;
> >> +		break;
> >> +
> >> +	case RCAR_DSI_R8A779G0:
> >> +		ret = rcar_mipi_dsi_post_init_phtw_v4h(dsi, &setup_info);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		break;
> >> +
> >> +	default:
> >> +		return -ENODEV;
> >>   	}
> >>   
> >>   	/* Enable DOT clock */
> >> @@ -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);
> >> +		break;
> >> +
> >> +	default:
> >> +		return -ENODEV;
> >> +	}
> >>   
> >>   	rcar_mipi_dsi_write(dsi, VCLKSET, vclkset);
> >>   
> >> @@ -841,8 +1098,25 @@ static int rcar_mipi_dsi_remove(struct platform_device *pdev)
> >>   	return 0;
> >>   }
> >>   
> >> +static const struct rcar_mipi_dsi_device_info r8a779a0_data = {
> >> +	.model = RCAR_DSI_R8A779A0,
> >> +	.clk_cfg = dsi_clk_cfg_r8a779a0,
> >> +	.clockset2_m_offset = 2,
> >> +	.clockset2_n_offset = 1,
> >> +
> > 
> > Extra blank line.
> 
> Yep.
> 
> >> +};
> >> +
> >> +static const struct rcar_mipi_dsi_device_info r8a779g0_data = {
> >> +	.model = RCAR_DSI_R8A779G0,
> >> +	.clk_cfg = dsi_clk_cfg_r8a779g0,
> >> +	.clockset2_m_offset = 0,
> >> +	.clockset2_n_offset = 1,
> > 
> > You could possibly drop clockset2_n_offset as it's identical for the two
> > variants, and there's no indication it would be different in another
> > version of the IP core.
> 
> Yep, I think that's fine.
Tomi Valkeinen Nov. 29, 2022, 12:59 p.m. UTC | #6
On 29/11/2022 03:49, Laurent Pinchart wrote:

>> @@ -198,70 +436,53 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>>   	 */
>>   	fout_target = target * mipi_dsi_pixel_format_to_bpp(dsi->format)
>>   		    / (2 * dsi->lanes);
>> -	if (fout_target < 40000000 || fout_target > 1250000000)
>> +	if (fout_target < MHZ(40) || fout_target > MHZ(1250))
>>   		return;
>>   
>>   	/* Find vco_cntrl */
>> -	for (vco_cntrl = vco_cntrl_table; vco_cntrl->min_freq != 0; vco_cntrl++) {
>> -		if (fout_target > vco_cntrl->min_freq &&
>> -		    fout_target <= vco_cntrl->max_freq) {
>> -			setup_info->vco_cntrl = vco_cntrl->value;
>> -			if (fout_target >= 1150000000)
>> -				setup_info->prop_cntrl = 0x0c;
>> -			else
>> -				setup_info->prop_cntrl = 0x0b;
>> +	for (clkset = dsi->info->clk_cfg; clkset->min_freq != 0; clkset++) {
>> +		if (fout_target > clkset->min_freq &&
>> +		    fout_target <= clkset->max_freq) {
>> +			setup_info->clkset = clkset;
>>   			break;
>>   		}
>>   	}
>>   
>> -	/* Add divider */
>> -	setup_info->div = (setup_info->vco_cntrl & 0x30) >> 4;
>> +	switch (dsi->info->model) {
>> +	case RCAR_DSI_R8A779A0:
>> +		setup_info->vclk_divider = 1 << ((clkset->vco_cntrl >> 4) & 0x3);
> 
> If you stored (clkset->vco_cntrl >> 4) & 0x3 in setup_info->vclk_divider
> you wouldn't have to use __ffs() in rcar_mipi_dsi_startup(). You could
> also drop the - 1 there, which would allow dropping one of the
> switch(dsi->info->model). You can store the real divider value in
> setup_info separately for rcar_mipi_dsi_pll_calc_r8a779a0(), or pass it
> to the function.

That's true. The reason I chose this approach was to keep dsi_setup_info 
"neutral", containing only the logical values, and the register specific 
tinkering is done only where the register is written. Mixing the logical 
and the register values in the old dsi_setup_info was confusing, and 
implementing your suggestion would again go that direction. But as you 
noticed, this is uglified a bit by the need to get the divider from the 
vco_cntrl.

We could store the logical divider in the dsi_clk_config table, though, 
which would remove the need for the above code.

  Tomi
diff mbox series

Patch

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 },
+	{ /* sentinel */ },
+};
+
+static const struct dsi_clk_config dsi_clk_cfg_r8a779g0[] = {
+	{   40000000u,   45310000u, 0x2b, 0x00, 0x00, 0x08, 0x0a },
+	{   45310000u,   54660000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
+	{   54660000u,   62500000u, 0x28, 0x00, 0x00, 0x08, 0x0a },
+	{   62500000u,   75000000u, 0x27, 0x00, 0x00, 0x08, 0x0a },
+	{   75000000u,   90630000u, 0x23, 0x00, 0x00, 0x08, 0x0a },
+	{   90630000u,  109370000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
+	{  109370000u,  125000000u, 0x20, 0x00, 0x00, 0x08, 0x0a },
+	{  125000000u,  150000000u, 0x1f, 0x00, 0x00, 0x08, 0x0a },
+	{  150000000u,  181250000u, 0x1b, 0x00, 0x00, 0x08, 0x0a },
+	{  181250000u,  218750000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
+	{  218750000u,  250000000u, 0x18, 0x00, 0x00, 0x08, 0x0a },
+	{  250000000u,  300000000u, 0x17, 0x00, 0x00, 0x08, 0x0a },
+	{  300000000u,  362500000u, 0x13, 0x00, 0x00, 0x08, 0x0a },
+	{  362500000u,  455480000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
+	{  455480000u,  500000000u, 0x10, 0x00, 0x00, 0x08, 0x0a },
+	{  500000000u,  600000000u, 0x0f, 0x00, 0x00, 0x08, 0x0a },
+	{  600000000u,  725000000u, 0x0b, 0x00, 0x00, 0x08, 0x0a },
+	{  725000000u,  875000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
+	{  875000000u, 1000000000u, 0x08, 0x00, 0x00, 0x08, 0x0a },
+	{ 1000000000u, 1200000000u, 0x07, 0x00, 0x00, 0x08, 0x0a },
+	{ 1200000000u, 1250000000u, 0x03, 0x00, 0x00, 0x08, 0x0a },
 	{ /* sentinel */ },
 };
 
@@ -144,7 +183,7 @@  static void rcar_mipi_dsi_set(struct rcar_mipi_dsi *dsi, u32 reg, u32 set)
 	rcar_mipi_dsi_write(dsi, reg, rcar_mipi_dsi_read(dsi, reg) | set);
 }
 
-static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
+static int rcar_mipi_dsi_write_phtw(struct rcar_mipi_dsi *dsi, u32 phtw)
 {
 	u32 status;
 	int ret;
@@ -163,32 +202,231 @@  static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
 	return ret;
 }
 
+static int rcar_mipi_dsi_write_phtw_arr(struct rcar_mipi_dsi *dsi,
+					const u32 *phtw, unsigned int size)
+{
+	for (unsigned int i = 0; i < size; i++) {
+		int ret = rcar_mipi_dsi_write_phtw(dsi, phtw[i]);
+
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+#define WRITE_PHTW(...)                                               \
+	({                                                            \
+		static const u32 phtw[] = { __VA_ARGS__ };            \
+		int ret;                                              \
+		ret = rcar_mipi_dsi_write_phtw_arr(dsi, phtw,         \
+						   ARRAY_SIZE(phtw)); \
+		ret;                                                  \
+	})
+
+static int rcar_mipi_dsi_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
+{
+	return WRITE_PHTW(0x01020114, 0x01600115, 0x01030116, 0x0102011d,
+			  0x011101a4, 0x018601a4, 0x014201a0, 0x010001a3,
+			  0x0101011f);
+}
+
+static int rcar_mipi_dsi_post_init_phtw_v3u(struct rcar_mipi_dsi *dsi)
+{
+	return WRITE_PHTW(0x010c0130, 0x010c0140, 0x010c0150, 0x010c0180,
+			  0x010c0190, 0x010a0160, 0x010a0170, 0x01800164,
+			  0x01800174);
+}
+
+static int rcar_mipi_dsi_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
+				       const struct dsi_setup_info *setup_info)
+{
+	int ret;
+
+	if (setup_info->hsfreq < MHZ(450)) {
+		ret = WRITE_PHTW(0x01010100, 0x011b01ac);
+		if (ret)
+			return ret;
+	}
+
+	ret = WRITE_PHTW(0x01010100, 0x01030173, 0x01000174, 0x01500175,
+			 0x01030176, 0x01040166, 0x010201ad);
+	if (ret)
+		return ret;
+
+	if (setup_info->hsfreq <= MHZ(1000))
+		ret = WRITE_PHTW(0x01020100, 0x01910170, 0x01020171,
+				 0x01110172);
+	else if (setup_info->hsfreq <= MHZ(1500))
+		ret = WRITE_PHTW(0x01020100, 0x01980170, 0x01030171,
+				 0x01100172);
+	else if (setup_info->hsfreq <= MHZ(2500))
+		ret = WRITE_PHTW(0x01020100, 0x0144016b, 0x01000172);
+	else
+		return -EINVAL;
+
+	if (ret)
+		return ret;
+
+	if (dsi->lanes <= 1) {
+		ret = WRITE_PHTW(0x01070100, 0x010e010b);
+		if (ret)
+			return ret;
+	}
+
+	if (dsi->lanes <= 2) {
+		ret = WRITE_PHTW(0x01090100, 0x010e010b);
+		if (ret)
+			return ret;
+	}
+
+	if (dsi->lanes <= 3) {
+		ret = WRITE_PHTW(0x010b0100, 0x010e010b);
+		if (ret)
+			return ret;
+	}
+
+	if (setup_info->hsfreq <= MHZ(1500)) {
+		ret = WRITE_PHTW(0x01010100, 0x01c0016e);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+rcar_mipi_dsi_post_init_phtw_v4h(struct rcar_mipi_dsi *dsi,
+				 const struct dsi_setup_info *setup_info)
+{
+	u32 status;
+	int ret;
+
+	if (setup_info->hsfreq <= MHZ(1500)) {
+		WRITE_PHTW(0x01020100, 0x00000180);
+
+		ret = read_poll_timeout(rcar_mipi_dsi_read, status,
+					status & PHTR_TEST, 2000, 10000, false,
+					dsi, PHTR);
+		if (ret < 0) {
+			dev_err(dsi->dev, "failed to test PHTR\n");
+			return ret;
+		}
+
+		WRITE_PHTW(0x01010100, 0x0100016e);
+	}
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Hardware Setup
  */
 
-struct dsi_setup_info {
-	unsigned long fout;
-	u16 vco_cntrl;
-	u16 prop_cntrl;
-	u16 hsfreqrange;
-	u16 div;
-	unsigned int m;
-	unsigned int n;
-};
+static void rcar_mipi_dsi_pll_calc_r8a779a0(struct rcar_mipi_dsi *dsi,
+					    struct clk *clk,
+					    unsigned long fout_target,
+					    struct dsi_setup_info *setup_info)
+{
+	unsigned int best_err = -1;
+	unsigned long fin;
+
+	fin = clk_get_rate(clk);
+
+	for (unsigned int n = 3; n <= 8; n++) {
+		unsigned long fpfd;
+
+		fpfd = fin / n;
+
+		if (fpfd < MHZ(2) || fpfd > MHZ(8))
+			continue;
+
+		for (unsigned int m = 64; m <= 625; m++) {
+			unsigned int err;
+			u64 fout;
+
+			fout = (u64)fpfd * m;
+
+			if (fout < MHZ(320) || fout > MHZ(1250))
+				continue;
+
+			fout = div64_u64(fout, setup_info->vclk_divider);
+
+			if (fout < setup_info->clkset->min_freq ||
+			    fout > setup_info->clkset->max_freq)
+				continue;
+
+			err = abs((long)(fout - fout_target) * 10000 /
+				  (long)fout_target);
+
+			if (err < best_err) {
+				setup_info->m = m;
+				setup_info->n = n;
+				setup_info->fout = (unsigned long)fout;
+				best_err = err;
+
+				if (err == 0)
+					return;
+			}
+		}
+	}
+}
+
+static void rcar_mipi_dsi_pll_calc_r8a779g0(struct rcar_mipi_dsi *dsi,
+					    struct clk *clk,
+					    unsigned long fout_target,
+					    struct dsi_setup_info *setup_info)
+{
+	unsigned int best_err = -1;
+	unsigned long fin;
+
+	fin = clk_get_rate(clk);
+
+	for (unsigned int n = 1; n <= 8; n++) {
+		unsigned long fpfd;
+
+		fpfd = fin / n;
+
+		if (fpfd < MHZ(8) || fpfd > MHZ(24))
+			continue;
+
+		for (unsigned int m = 167; m <= 1000; m++) {
+			unsigned int err;
+			u64 fout;
+
+			fout = div64_u64((u64)fpfd * m, 2);
+
+			if (fout < MHZ(2000) || fout > MHZ(4000))
+				continue;
+
+			fout = div64_u64(fout, setup_info->vclk_divider);
+
+			if (fout < setup_info->clkset->min_freq ||
+			    fout > setup_info->clkset->max_freq)
+				continue;
+
+			err = abs((long)(fout - fout_target) * 10000 /
+				  (long)fout_target);
+			if (err < best_err) {
+				setup_info->m = m;
+				setup_info->n = n;
+				setup_info->fout = (unsigned long)fout;
+				best_err = err;
+
+				if (err == 0)
+					return;
+			}
+		}
+	}
+}
 
 static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
 					  struct clk *clk, unsigned long target,
 					  struct dsi_setup_info *setup_info)
 {
 
-	const struct vco_cntrl_value *vco_cntrl;
+	const struct dsi_clk_config *clkset;
 	unsigned long fout_target;
-	unsigned long fin, fout;
-	unsigned long hsfreq;
-	unsigned int best_err = -1;
-	unsigned int divider;
-	unsigned int n;
 	unsigned int i;
 	unsigned int err;
 
@@ -198,70 +436,53 @@  static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
 	 */
 	fout_target = target * mipi_dsi_pixel_format_to_bpp(dsi->format)
 		    / (2 * dsi->lanes);
-	if (fout_target < 40000000 || fout_target > 1250000000)
+	if (fout_target < MHZ(40) || fout_target > MHZ(1250))
 		return;
 
 	/* Find vco_cntrl */
-	for (vco_cntrl = vco_cntrl_table; vco_cntrl->min_freq != 0; vco_cntrl++) {
-		if (fout_target > vco_cntrl->min_freq &&
-		    fout_target <= vco_cntrl->max_freq) {
-			setup_info->vco_cntrl = vco_cntrl->value;
-			if (fout_target >= 1150000000)
-				setup_info->prop_cntrl = 0x0c;
-			else
-				setup_info->prop_cntrl = 0x0b;
+	for (clkset = dsi->info->clk_cfg; clkset->min_freq != 0; clkset++) {
+		if (fout_target > clkset->min_freq &&
+		    fout_target <= clkset->max_freq) {
+			setup_info->clkset = clkset;
 			break;
 		}
 	}
 
-	/* Add divider */
-	setup_info->div = (setup_info->vco_cntrl & 0x30) >> 4;
+	switch (dsi->info->model) {
+	case RCAR_DSI_R8A779A0:
+		setup_info->vclk_divider = 1 << ((clkset->vco_cntrl >> 4) & 0x3);
+		rcar_mipi_dsi_pll_calc_r8a779a0(dsi, clk, fout_target, setup_info);
+		break;
+
+	case RCAR_DSI_R8A779G0:
+		setup_info->vclk_divider = 1 << (((clkset->vco_cntrl >> 3) & 0x7) + 1);
+		rcar_mipi_dsi_pll_calc_r8a779g0(dsi, clk, fout_target, setup_info);
+		break;
+
+	default:
+		return;
+	}
 
 	/* Find hsfreqrange */
-	hsfreq = fout_target * 2;
+	setup_info->hsfreq = setup_info->fout * 2;
 	for (i = 0; i < ARRAY_SIZE(hsfreqrange_table); i++) {
-		if (hsfreqrange_table[i][0] >= hsfreq) {
+		if (hsfreqrange_table[i][0] >= setup_info->hsfreq) {
 			setup_info->hsfreqrange = hsfreqrange_table[i][1];
 			break;
 		}
 	}
 
-	/*
-	 * Calculate n and m for PLL clock
-	 * Following the HW manual the ranges of n and m are
-	 * n = [3-8] and m = [64-625]
-	 */
-	fin = clk_get_rate(clk);
-	divider = 1 << setup_info->div;
-	for (n = 3; n < 9; n++) {
-		unsigned long fpfd;
-		unsigned int m;
-
-		fpfd = fin / n;
-
-		for (m = 64; m < 626; m++) {
-			fout = fpfd * m / divider;
-			err = abs((long)(fout - fout_target) * 10000 /
-				  (long)fout_target);
-			if (err < best_err) {
-				setup_info->m = m - 2;
-				setup_info->n = n - 1;
-				setup_info->fout = fout;
-				best_err = err;
-				if (err == 0)
-					goto done;
-			}
-		}
-	}
+	err = abs((long)(setup_info->fout - fout_target) * 10000 / (long)fout_target);
 
-done:
 	dev_dbg(dsi->dev,
-		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL M/N/DIV %u/%u/%u\n",
-		clk, fin, setup_info->fout, fout_target, best_err / 100,
-		best_err % 100, setup_info->m, setup_info->n, setup_info->div);
+		"Fout = %u * %lu / (2 * %u * %u) = %lu (target %lu Hz, error %d.%02u%%)\n",
+		setup_info->m, clk_get_rate(clk), setup_info->n, setup_info->vclk_divider,
+		setup_info->fout, fout_target,
+		err / 100, err % 100);
+
 	dev_dbg(dsi->dev,
 		"vco_cntrl = 0x%x\tprop_cntrl = 0x%x\thsfreqrange = 0x%x\n",
-		setup_info->vco_cntrl, setup_info->prop_cntrl,
+		clkset->vco_cntrl, clkset->prop_cntrl,
 		setup_info->hsfreqrange);
 }
 
@@ -324,7 +545,7 @@  static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 {
 	struct dsi_setup_info setup_info = {};
 	unsigned int timeout;
-	int ret, i;
+	int ret;
 	int dsi_format;
 	u32 phy_setup;
 	u32 clockset2, clockset3;
@@ -360,10 +581,21 @@  static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 	phy_setup |= PHYSETUP_HSFREQRANGE(setup_info.hsfreqrange);
 	rcar_mipi_dsi_write(dsi, PHYSETUP, phy_setup);
 
-	for (i = 0; i < ARRAY_SIZE(phtw); i++) {
-		ret = rcar_mipi_dsi_phtw_test(dsi, phtw[i]);
+	switch (dsi->info->model) {
+	case RCAR_DSI_R8A779A0:
+		ret = rcar_mipi_dsi_init_phtw_v3u(dsi);
+		if (ret < 0)
+			return ret;
+		break;
+
+	case RCAR_DSI_R8A779G0:
+		ret = rcar_mipi_dsi_init_phtw_v4h(dsi, &setup_info);
 		if (ret < 0)
 			return ret;
+		break;
+
+	default:
+		return -ENODEV;
 	}
 
 	/* PLL Clock Setting */
@@ -371,12 +603,13 @@  static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 	rcar_mipi_dsi_set(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
 	rcar_mipi_dsi_clr(dsi, CLOCKSET1, CLOCKSET1_SHADOW_CLEAR);
 
-	clockset2 = CLOCKSET2_M(setup_info.m) | CLOCKSET2_N(setup_info.n)
-		  | CLOCKSET2_VCO_CNTRL(setup_info.vco_cntrl);
-	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.prop_cntrl)
-		  | CLOCKSET3_INT_CNTRL(0)
-		  | CLOCKSET3_CPBIAS_CNTRL(0x10)
-		  | CLOCKSET3_GMP_CNTRL(1);
+	clockset2 = CLOCKSET2_M(setup_info.m - dsi->info->clockset2_m_offset)
+		  | CLOCKSET2_N(setup_info.n - dsi->info->clockset2_n_offset)
+		  | CLOCKSET2_VCO_CNTRL(setup_info.clkset->vco_cntrl);
+	clockset3 = CLOCKSET3_PROP_CNTRL(setup_info.clkset->prop_cntrl)
+		  | CLOCKSET3_INT_CNTRL(setup_info.clkset->int_cntrl)
+		  | CLOCKSET3_CPBIAS_CNTRL(setup_info.clkset->cpbias_cntrl)
+		  | CLOCKSET3_GMP_CNTRL(setup_info.clkset->gmp_cntrl);
 	rcar_mipi_dsi_write(dsi, CLOCKSET2, clockset2);
 	rcar_mipi_dsi_write(dsi, CLOCKSET3, clockset3);
 
@@ -407,10 +640,21 @@  static int rcar_mipi_dsi_startup(struct rcar_mipi_dsi *dsi,
 		return -ETIMEDOUT;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(phtw2); i++) {
-		ret = rcar_mipi_dsi_phtw_test(dsi, phtw2[i]);
+	switch (dsi->info->model) {
+	case RCAR_DSI_R8A779A0:
+		ret = rcar_mipi_dsi_post_init_phtw_v3u(dsi);
 		if (ret < 0)
 			return ret;
+		break;
+
+	case RCAR_DSI_R8A779G0:
+		ret = rcar_mipi_dsi_post_init_phtw_v4h(dsi, &setup_info);
+		if (ret < 0)
+			return ret;
+		break;
+
+	default:
+		return -ENODEV;
 	}
 
 	/* Enable DOT clock */
@@ -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);
+		break;
+
+	default:
+		return -ENODEV;
+	}
 
 	rcar_mipi_dsi_write(dsi, VCLKSET, vclkset);
 
@@ -841,8 +1098,25 @@  static int rcar_mipi_dsi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rcar_mipi_dsi_device_info r8a779a0_data = {
+	.model = RCAR_DSI_R8A779A0,
+	.clk_cfg = dsi_clk_cfg_r8a779a0,
+	.clockset2_m_offset = 2,
+	.clockset2_n_offset = 1,
+
+};
+
+static const struct rcar_mipi_dsi_device_info r8a779g0_data = {
+	.model = RCAR_DSI_R8A779G0,
+	.clk_cfg = dsi_clk_cfg_r8a779g0,
+	.clockset2_m_offset = 0,
+	.clockset2_n_offset = 1,
+
+};
+
 static const struct of_device_id rcar_mipi_dsi_of_table[] = {
-	{ .compatible = "renesas,r8a779a0-dsi-csi2-tx" },
+	{ .compatible = "renesas,r8a779a0-dsi-csi2-tx", .data = &r8a779a0_data },
+	{ .compatible = "renesas,r8a779g0-dsi-csi2-tx", .data = &r8a779g0_data },
 	{ }
 };
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
index 2eaca54636f3..608851340acf 100644
--- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi_regs.h
@@ -122,7 +122,8 @@ 
 #define VCLKSET_CKEN			(1 << 16)
 #define VCLKSET_COLOR_RGB		(0 << 8)
 #define VCLKSET_COLOR_YCC		(1 << 8)
-#define VCLKSET_DIV(x)			(((x) & 0x3) << 4)
+#define VCLKSET_DIV_R8A779A0(x)		(((x) & 0x3) << 4)
+#define VCLKSET_DIV_R8A779G0(x)		(((x) & 0x7) << 4)
 #define VCLKSET_BPP_16			(0 << 2)
 #define VCLKSET_BPP_18			(1 << 2)
 #define VCLKSET_BPP_18L			(2 << 2)
@@ -166,6 +167,9 @@ 
 #define PHTW_CWEN			(1 << 8)
 #define PHTW_TESTDIN_CODE(x)		(((x) & 0xff) << 0)
 
+#define PHTR				0x1038
+#define PHTR_TEST			(1 << 16)
+
 #define PHTC				0x103c
 #define PHTC_TESTCLR			(1 << 0)