[PATCH 0/2] Remove hard coded values for MIPI-DSI

[PATCH 0/2] Remove hard coded values for MIPI-DSI

[Chris Brandt]

When the initial drivers were submitted, some of the timing was hard coded and
did not allow for any MIPI-DSI panel to be attached.

This series assumes the follow patches have been applied.
Both patches exist in drm-misc-next.

5ce16c169a4c ("drm: renesas: rz-du: Add atomic_pre_enable")
6f392f371650 ("drm: renesas: rz-du: Implement MIPI DSI host transfers")



Chris Brandt (2):
  clk: renesas: rzg2l: Remove DSI clock rate restrictions
  drm: renesas: rz-du: Set DSI divider based on target MIPI device

 drivers/clk/renesas/rzg2l-cpg.c               | 113 ++++++++++++++++--
 .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c    |  17 +++
 include/linux/clk/renesas.h                   |   4 +
 3 files changed, 124 insertions(+), 10 deletions(-)

--
2.49.0

[PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Chris Brandt]

Convert the limited MIPI clock calculations to a full range of settings
based on math including H/W limitation validation.
Since the required DSI division setting must be specified from external
sources before calculations, expose a new API to set it.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Signed-off-by: hienhuynh <hien.huynh.px@renesas.com>
Signed-off-by: Nghia Vo <nghia.vo.zn@renesas.com>
---
 drivers/clk/renesas/rzg2l-cpg.c | 113 +++++++++++++++++++++++++++++---
 include/linux/clk/renesas.h     |   4 ++
 2 files changed, 107 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index a8628f64a03b..317e50f5b967 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -68,6 +68,19 @@
 
 #define MAX_VCLK_FREQ		(148500000)
 
+#define PLL5_FOUTVCO_MIN	800000000
+#define PLL5_FOUTVCO_MAX	3000000000
+#define PLL5_POSTDIV_MIN	1
+#define PLL5_POSTDIV_MAX	7
+#define PLL5_POSTDIV_DEF	1
+#define PLL5_REFDIV_MIN		1
+#define PLL5_REFDIV_MAX		2
+#define PLL5_REFDIV_DEF		1
+#define PLL5_INTIN_MIN		20
+#define PLL5_INTIN_MAX		320
+#define PLL5_INTIN_DEF		125
+#define PLL5_FRACIN_DEF		0
+
 /**
  * struct clk_hw_data - clock hardware data
  * @hw: clock hw
@@ -123,6 +136,9 @@ struct rzg2l_pll5_param {
 	u8 pl5_spread;
 };
 
+/* Required division ratio for the MIPI clock */
+int dsi_div_ab;
+
 struct rzg2l_pll5_mux_dsi_div_param {
 	u8 clksrc;
 	u8 dsi_div_a;
@@ -548,24 +564,93 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,
 }
 
 static unsigned long
-rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params,
+rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv,
+			       struct rzg2l_pll5_param *params,
 			       unsigned long rate)
 {
 	unsigned long foutpostdiv_rate, foutvco_rate;
+	u8 div = 1;
+	bool found = 0;
+	int a, b;
+
+	if (priv->mux_dsi_div_params.clksrc)
+		div = 2;
+
+	/* Calculate the DIV_DSI_A and DIV_DSI_B based on the final DIV DSI */
+	for (a = 0; a < 4; a++) {
+		for (b = 0; b < 16; b++) {
+			if (((1 << a) * (b + 1)) == dsi_div_ab) {
+				priv->mux_dsi_div_params.dsi_div_a = a;
+				priv->mux_dsi_div_params.dsi_div_b = b;
+
+				goto found_dsi_div;
+			}
+		}
+	}
+
+found_dsi_div:
+	/*
+	 * Below conditions must be set for PLL5 parameters:
+	 * - REFDIV must be between 1 and 2.
+	 * - POSTDIV1/2 must be between 1 and 7.
+	 * - INTIN must be between 20 and 320.
+	 * - FOUTVCO must be between 800MHz and 3000MHz.
+	 */
+	for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
+	     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
+	     params->pl5_postdiv1++) {
+		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
+		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
+		     params->pl5_postdiv2++) {
+			foutvco_rate = rate * ((1 << priv->mux_dsi_div_params.dsi_div_a) *
+					       (priv->mux_dsi_div_params.dsi_div_b + 1)) *
+					      div * params->pl5_postdiv1 * params->pl5_postdiv2;
+			if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 ||
+			    foutvco_rate > PLL5_FOUTVCO_MAX - 1)
+				continue;
+
+			for (params->pl5_refdiv = PLL5_REFDIV_MIN;
+			     params->pl5_refdiv < PLL5_REFDIV_MAX + 1;
+			     params->pl5_refdiv++) {
+				params->pl5_intin = (foutvco_rate * params->pl5_refdiv) /
+						    (EXTAL_FREQ_IN_MEGA_HZ * MEGA);
+				if (params->pl5_intin < PLL5_INTIN_MIN + 1 ||
+				    params->pl5_intin > PLL5_INTIN_MAX - 1)
+					continue;
+				params->pl5_fracin = div_u64(((u64)
+						     (foutvco_rate * params->pl5_refdiv) %
+						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
+						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
+				found = 1;
+				goto found_clk;
+			}
+		}
+	}
+
+found_clk:
+	if (!found) {
+		params->pl5_intin = PLL5_INTIN_DEF;
+		params->pl5_fracin = PLL5_FRACIN_DEF;
+		params->pl5_refdiv = PLL5_REFDIV_DEF;
+		params->pl5_postdiv1 = PLL5_POSTDIV_DEF;
+		params->pl5_postdiv2 = PLL5_POSTDIV_DEF;
+	}
 
-	params->pl5_intin = rate / MEGA;
-	params->pl5_fracin = div_u64(((u64)rate % MEGA) << 24, MEGA);
-	params->pl5_refdiv = 2;
-	params->pl5_postdiv1 = 1;
-	params->pl5_postdiv2 = 1;
 	params->pl5_spread = 0x16;
 
 	foutvco_rate = div_u64(mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA,
-					   (params->pl5_intin << 24) + params->pl5_fracin),
-			       params->pl5_refdiv) >> 24;
+		       (params->pl5_intin << 24) + params->pl5_fracin),
+		       params->pl5_refdiv) >> 24;
 	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
 						 params->pl5_postdiv1 * params->pl5_postdiv2);
 
+	/* If foutvco is above 1.5GHz, change parent and recalculate */
+	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) {
+		priv->mux_dsi_div_params.clksrc = 0;
+		dsi_div_ab *= 2;
+		return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate);
+	}
+
 	return foutpostdiv_rate;
 }
 
@@ -598,7 +683,7 @@ static unsigned long rzg2l_cpg_get_vclk_parent_rate(struct clk_hw *hw,
 	struct rzg2l_pll5_param params;
 	unsigned long parent_rate;
 
-	parent_rate = rzg2l_cpg_get_foutpostdiv_rate(&params, rate);
+	parent_rate = rzg2l_cpg_get_foutpostdiv_rate(priv, &params, rate);
 
 	if (priv->mux_dsi_div_params.clksrc)
 		parent_rate /= 2;
@@ -617,6 +702,12 @@ static int rzg2l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+void rzg2l_cpg_dsi_div_set_divider(int divider)
+{
+	dsi_div_ab = divider;
+}
+EXPORT_SYMBOL_GPL(rzg2l_cpg_dsi_div_set_divider);
+
 static int rzg2l_cpg_dsi_div_set_rate(struct clk_hw *hw,
 				      unsigned long rate,
 				      unsigned long parent_rate)
@@ -850,7 +941,7 @@ static int rzg2l_cpg_sipll5_set_rate(struct clk_hw *hw,
 
 	vclk_rate = rzg2l_cpg_get_vclk_rate(hw, rate);
 	sipll5->foutpostdiv_rate =
-		rzg2l_cpg_get_foutpostdiv_rate(&params, vclk_rate);
+		rzg2l_cpg_get_foutpostdiv_rate(priv, &params, vclk_rate);
 
 	/* Put PLL5 into standby mode */
 	writel(CPG_SIPLL5_STBY_RESETB_WEN, priv->base + CPG_SIPLL5_STBY);
@@ -940,6 +1031,8 @@ rzg2l_cpg_sipll5_register(const struct cpg_core_clk *core,
 	priv->mux_dsi_div_params.clksrc = 1; /* Use clk src 1 for DSI */
 	priv->mux_dsi_div_params.dsi_div_a = 1; /* Divided by 2 */
 	priv->mux_dsi_div_params.dsi_div_b = 2; /* Divided by 3 */
+	dsi_div_ab = (1 << priv->mux_dsi_div_params.dsi_div_a) *
+		     (priv->mux_dsi_div_params.dsi_div_b + 1);
 
 	return clk_hw->clk;
 }
diff --git a/include/linux/clk/renesas.h b/include/linux/clk/renesas.h
index 0ebbe2f0b45e..da70afa02eaf 100644
--- a/include/linux/clk/renesas.h
+++ b/include/linux/clk/renesas.h
@@ -33,3 +33,7 @@ void cpg_mssr_detach_dev(struct generic_pm_domain *unused, struct device *dev);
 #define cpg_mssr_detach_dev	NULL
 #endif
 #endif
+
+#ifdef CONFIG_CLK_RZG2L
+void rzg2l_cpg_dsi_div_set_divider(int divider);
+#endif
-- 
2.49.0

[PATCH 2/2] drm: renesas: rz-du: Set DSI divider based on target MIPI device

[Chris Brandt]

Before the MIPI DSI clock source can be configured, the target divide
ratio needs to be known.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
index 4ad0be03a5b4..a8796df43f75 100644
--- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/clk/renesas.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
@@ -666,6 +667,22 @@ static int rzg2l_mipi_dsi_host_attach(struct mipi_dsi_host *host,
 
 	drm_bridge_add(&dsi->bridge);
 
+	/* Report required division ratio setting for the MIPI clock dividers
+	 * Assume the default clock source is FOUTPOSTDIV (PLL/2) being fed to the DSI-PHY, but also
+	 * the DSI-PHY must be 16x the MIPI-DSI HS clock.
+	 *
+	 * pllclk/2 = vclk * DSI divider
+	 * pllclk = vclk * DSI divider * 2
+	 *
+	 * hsclk = (vclk * DSI divider * 2) / 16
+	 *
+	 * vclk * bpp = hsclk * 8 * num_lanes
+	 * vclk * bpp = ((vclk * DSI divider * 2) / 16) * 8 * num_lanes
+	 *   which simplifies to...
+	 * DSI divider = bpp / num_lanes
+	 */
+	rzg2l_cpg_dsi_div_set_divider(mipi_dsi_pixel_format_to_bpp(dsi->format) / dsi->lanes);
+
 	return 0;
 }
 
-- 
2.49.0

Re: [PATCH 2/2] drm: renesas: rz-du: Set DSI divider based on target MIPI device

[Sergey Shtylyov]

On 7/9/25 11:55 PM, Chris Brandt wrote:

> Before the MIPI DSI clock source can be configured, the target divide
> ratio needs to be known.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> index 4ad0be03a5b4..a8796df43f75 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
[...]
> @@ -666,6 +667,22 @@ static int rzg2l_mipi_dsi_host_attach(struct mipi_dsi_host *host,
>  
>  	drm_bridge_add(&dsi->bridge);
>  
> +	/* Report required division ratio setting for the MIPI clock dividers

   /* should be on a line of its own (unless you do a networking patch)...

> +	 * Assume the default clock source is FOUTPOSTDIV (PLL/2) being fed to the DSI-PHY, but also
> +	 * the DSI-PHY must be 16x the MIPI-DSI HS clock.
> +	 *
> +	 * pllclk/2 = vclk * DSI divider

   Why no spaces around / while there are spaces around *?

[...]

MBR, Sergey

RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Biju Das]

Hi Chris Brandt,

Thanks for the patch.

> -----Original Message-----
> From: Chris Brandt <chris.brandt@renesas.com>
> Sent: 09 July 2025 21:56
> Subject: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions
> 
> Convert the limited MIPI clock calculations to a full range of settings based on math including H/W
> limitation validation.
> Since the required DSI division setting must be specified from external sources before calculations,
> expose a new API to set it.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Signed-off-by: hienhuynh <hien.huynh.px@renesas.com>
> Signed-off-by: Nghia Vo <nghia.vo.zn@renesas.com>
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 113 +++++++++++++++++++++++++++++---
>  include/linux/clk/renesas.h     |   4 ++
>  2 files changed, 107 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c index
> a8628f64a03b..317e50f5b967 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -68,6 +68,19 @@
> 
>  #define MAX_VCLK_FREQ		(148500000)
> 
> +#define PLL5_FOUTVCO_MIN	800000000
> +#define PLL5_FOUTVCO_MAX	3000000000
> +#define PLL5_POSTDIV_MIN	1
> +#define PLL5_POSTDIV_MAX	7
> +#define PLL5_POSTDIV_DEF	1
> +#define PLL5_REFDIV_MIN		1
> +#define PLL5_REFDIV_MAX		2
> +#define PLL5_REFDIV_DEF		1
> +#define PLL5_INTIN_MIN		20
> +#define PLL5_INTIN_MAX		320
> +#define PLL5_INTIN_DEF		125
> +#define PLL5_FRACIN_DEF		0
> +
>  /**
>   * struct clk_hw_data - clock hardware data
>   * @hw: clock hw
> @@ -123,6 +136,9 @@ struct rzg2l_pll5_param {
>  	u8 pl5_spread;
>  };
> 
> +/* Required division ratio for the MIPI clock */ int dsi_div_ab;
> +

static int dsi_div_ab;

>  struct rzg2l_pll5_mux_dsi_div_param {
>  	u8 clksrc;
>  	u8 dsi_div_a;
> @@ -548,24 +564,93 @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,  }
> 
>  static unsigned long
> -rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params,
> +rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv,
> +			       struct rzg2l_pll5_param *params,
>  			       unsigned long rate)
>  {
>  	unsigned long foutpostdiv_rate, foutvco_rate;
> +	u8 div = 1;
> +	bool found = 0;
> +	int a, b;
> +
> +	if (priv->mux_dsi_div_params.clksrc)
> +		div = 2;
> +

for the DPI, DIV_DSI_B = 1 and DIV_DSI_A ={2, 4, 8}

So, you need to adjust the below calculation for DPI as well??

Not sure do we need DSI driver registering a callback with CPG driver and 
CPG driver uses the callback to get DSI divider value and this callback
can be used to distinguish DPI from DSI??

Maybe Geert can provide more input on this?


> +	/* Calculate the DIV_DSI_A and DIV_DSI_B based on the final DIV DSI */
> +	for (a = 0; a < 4; a++) {
> +		for (b = 0; b < 16; b++) {
> +			if (((1 << a) * (b + 1)) == dsi_div_ab) {
> +				priv->mux_dsi_div_params.dsi_div_a = a;
> +				priv->mux_dsi_div_params.dsi_div_b = b;
> +
> +				goto found_dsi_div;
> +			}
> +		}
> +	}
> +
> +found_dsi_div:
> +	/*
> +	 * Below conditions must be set for PLL5 parameters:
> +	 * - REFDIV must be between 1 and 2.
> +	 * - POSTDIV1/2 must be between 1 and 7.
> +	 * - INTIN must be between 20 and 320.
> +	 * - FOUTVCO must be between 800MHz and 3000MHz.
> +	 */
> +	for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
> +	     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
> +	     params->pl5_postdiv1++) {
> +		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
> +		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
> +		     params->pl5_postdiv2++) {
> +			foutvco_rate = rate * ((1 << priv->mux_dsi_div_params.dsi_div_a) *
> +					       (priv->mux_dsi_div_params.dsi_div_b + 1)) *
> +					      div * params->pl5_postdiv1 * params->pl5_postdiv2;
> +			if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 ||
> +			    foutvco_rate > PLL5_FOUTVCO_MAX - 1)
> +				continue;
> +
> +			for (params->pl5_refdiv = PLL5_REFDIV_MIN;
> +			     params->pl5_refdiv < PLL5_REFDIV_MAX + 1;
> +			     params->pl5_refdiv++) {
> +				params->pl5_intin = (foutvco_rate * params->pl5_refdiv) /
> +						    (EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> +				if (params->pl5_intin < PLL5_INTIN_MIN + 1 ||
> +				    params->pl5_intin > PLL5_INTIN_MAX - 1)
> +					continue;
> +				params->pl5_fracin = div_u64(((u64)
> +						     (foutvco_rate * params->pl5_refdiv) %
> +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> +				found = 1;
> +				goto found_clk;
> +			}
> +		}
> +	}
> +
> +found_clk:
> +	if (!found) {

Can we add a dev_dbg statement here for !found clock?

> +		params->pl5_intin = PLL5_INTIN_DEF;
> +		params->pl5_fracin = PLL5_FRACIN_DEF;
> +		params->pl5_refdiv = PLL5_REFDIV_DEF;
> +		params->pl5_postdiv1 = PLL5_POSTDIV_DEF;
> +		params->pl5_postdiv2 = PLL5_POSTDIV_DEF;
> +	}
> 
> -	params->pl5_intin = rate / MEGA;
> -	params->pl5_fracin = div_u64(((u64)rate % MEGA) << 24, MEGA);
> -	params->pl5_refdiv = 2;
> -	params->pl5_postdiv1 = 1;
> -	params->pl5_postdiv2 = 1;
>  	params->pl5_spread = 0x16;
> 
>  	foutvco_rate = div_u64(mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA,
> -					   (params->pl5_intin << 24) + params->pl5_fracin),
> -			       params->pl5_refdiv) >> 24;
> +		       (params->pl5_intin << 24) + params->pl5_fracin),
> +		       params->pl5_refdiv) >> 24;
>  	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
>  						 params->pl5_postdiv1 * params->pl5_postdiv2);
> 
> +	/* If foutvco is above 1.5GHz, change parent and recalculate */
> +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) {

Check patch is complaining:

CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
#146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
+	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) {
total: 0 errors, 0 warnings, 1 checks, 172 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.
0002-clk-renesas-rzg2l-Remove-DSI-clock-rate-restrictions.patch has style problems, please review.


Cheers,
Biju

RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Chris Brandt]

Hi Biju,

Thank you for your review!


> > +/* Required division ratio for the MIPI clock */ 
> > +int dsi_div_ab;
>
> static int dsi_div_ab;

Good catch.


> for the DPI, DIV_DSI_B = 1 and DIV_DSI_A ={2, 4, 8}
>
> So, you need to adjust the below calculation for DPI as well??

You bring up a good point.

And looking at the hardware manual again, there are other restrictions when using FOUTPOSTDIV (straight PLL) compared to FOUT1PH0 (PLL/2).

From a chip design standpoint, they just expect to have 'one big driver that configures everything at once'.

> Not sure do we need DSI driver registering a callback with CPG driver and CPG driver uses the callback to get DSI divider value and this callback can be used to distinguish DPI from DSI??

Ya, you can't just tell the CPG driver to 'give me this rate'. There is so much other information that it needs to have before it can set up the registers.
Hmm....


> > +found_clk:
> > +	if (!found) {
>
> Can we add a dev_dbg statement here for !found clock?

Yes, good idea.
That was in the original driver before I started pulling out all the printk statements.


> > +	/* If foutvco is above 1.5GHz, change parent and recalculate */
> > +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) 
> > +{
> 
> Check patch is complaining:
> 
> CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
> #146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
> +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) {

I saw that...but I thought the ( ) makes it a little easier to read.

But, what's the general rule here? Make checkpatch come out perfect?
What's your thoughts?

Chris



-----Original Message-----
From: Biju Das <biju.das.jz@bp.renesas.com> 
Sent: Thursday, July 10, 2025 6:00 AM
To: Chris Brandt <Chris.Brandt@renesas.com>; Geert Uytterhoeven <geert+renesas@glider.be>; Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Hien Huynh <hien.huynh.px@renesas.com>; Nghia Vo <nghia.vo.zn@renesas.com>; Hugo Villeneuve <hugo@hugovil.com>
Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org; dri-devel@lists.freedesktop.org; Chris Brandt <Chris.Brandt@renesas.com>
Subject: RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

Hi Chris Brandt,

Thanks for the patch.

> -----Original Message-----
> From: Chris Brandt <chris.brandt@renesas.com>
> Sent: 09 July 2025 21:56
> Subject: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate 
> restrictions
> 
> Convert the limited MIPI clock calculations to a full range of 
> settings based on math including H/W limitation validation.
> Since the required DSI division setting must be specified from 
> external sources before calculations, expose a new API to set it.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Signed-off-by: hienhuynh <hien.huynh.px@renesas.com>
> Signed-off-by: Nghia Vo <nghia.vo.zn@renesas.com>
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 113 +++++++++++++++++++++++++++++---
>  include/linux/clk/renesas.h     |   4 ++
>  2 files changed, 107 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c 
> b/drivers/clk/renesas/rzg2l-cpg.c index
> a8628f64a03b..317e50f5b967 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -68,6 +68,19 @@
> 
>  #define MAX_VCLK_FREQ		(148500000)
> 
> +#define PLL5_FOUTVCO_MIN	800000000
> +#define PLL5_FOUTVCO_MAX	3000000000
> +#define PLL5_POSTDIV_MIN	1
> +#define PLL5_POSTDIV_MAX	7
> +#define PLL5_POSTDIV_DEF	1
> +#define PLL5_REFDIV_MIN		1
> +#define PLL5_REFDIV_MAX		2
> +#define PLL5_REFDIV_DEF		1
> +#define PLL5_INTIN_MIN		20
> +#define PLL5_INTIN_MAX		320
> +#define PLL5_INTIN_DEF		125
> +#define PLL5_FRACIN_DEF		0
> +
>  /**
>   * struct clk_hw_data - clock hardware data
>   * @hw: clock hw
> @@ -123,6 +136,9 @@ struct rzg2l_pll5_param {
>  	u8 pl5_spread;
>  };
> 
> +/* Required division ratio for the MIPI clock */ 
> +int dsi_div_ab;

static int dsi_div_ab;

>  struct rzg2l_pll5_mux_dsi_div_param {
>  	u8 clksrc;
>  	u8 dsi_div_a;
> @@ -548,24 +564,93 @@ rzg2l_cpg_sd_mux_clk_register(const struct 
> cpg_core_clk *core,  }
> 
>  static unsigned long
> -rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params,
> +rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv,
> +			       struct rzg2l_pll5_param *params,
>  			       unsigned long rate)
>  {
>  	unsigned long foutpostdiv_rate, foutvco_rate;
> +	u8 div = 1;
> +	bool found = 0;
> +	int a, b;
> +
> +	if (priv->mux_dsi_div_params.clksrc)
> +		div = 2;
> +

for the DPI, DIV_DSI_B = 1 and DIV_DSI_A ={2, 4, 8}

So, you need to adjust the below calculation for DPI as well??

Not sure do we need DSI driver registering a callback with CPG driver and CPG driver uses the callback to get DSI divider value and this callback can be used to distinguish DPI from DSI??

Maybe Geert can provide more input on this?


> +	/* Calculate the DIV_DSI_A and DIV_DSI_B based on the final DIV DSI */
> +	for (a = 0; a < 4; a++) {
> +		for (b = 0; b < 16; b++) {
> +			if (((1 << a) * (b + 1)) == dsi_div_ab) {
> +				priv->mux_dsi_div_params.dsi_div_a = a;
> +				priv->mux_dsi_div_params.dsi_div_b = b;
> +
> +				goto found_dsi_div;
> +			}
> +		}
> +	}
> +
> +found_dsi_div:
> +	/*
> +	 * Below conditions must be set for PLL5 parameters:
> +	 * - REFDIV must be between 1 and 2.
> +	 * - POSTDIV1/2 must be between 1 and 7.
> +	 * - INTIN must be between 20 and 320.
> +	 * - FOUTVCO must be between 800MHz and 3000MHz.
> +	 */
> +	for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
> +	     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
> +	     params->pl5_postdiv1++) {
> +		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
> +		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
> +		     params->pl5_postdiv2++) {
> +			foutvco_rate = rate * ((1 << priv->mux_dsi_div_params.dsi_div_a) *
> +					       (priv->mux_dsi_div_params.dsi_div_b + 1)) *
> +					      div * params->pl5_postdiv1 * params->pl5_postdiv2;
> +			if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 ||
> +			    foutvco_rate > PLL5_FOUTVCO_MAX - 1)
> +				continue;
> +
> +			for (params->pl5_refdiv = PLL5_REFDIV_MIN;
> +			     params->pl5_refdiv < PLL5_REFDIV_MAX + 1;
> +			     params->pl5_refdiv++) {
> +				params->pl5_intin = (foutvco_rate * params->pl5_refdiv) /
> +						    (EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> +				if (params->pl5_intin < PLL5_INTIN_MIN + 1 ||
> +				    params->pl5_intin > PLL5_INTIN_MAX - 1)
> +					continue;
> +				params->pl5_fracin = div_u64(((u64)
> +						     (foutvco_rate * params->pl5_refdiv) %
> +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> +				found = 1;
> +				goto found_clk;
> +			}
> +		}
> +	}
> +
> +found_clk:
> +	if (!found) {

Can we add a dev_dbg statement here for !found clock?

> +		params->pl5_intin = PLL5_INTIN_DEF;
> +		params->pl5_fracin = PLL5_FRACIN_DEF;
> +		params->pl5_refdiv = PLL5_REFDIV_DEF;
> +		params->pl5_postdiv1 = PLL5_POSTDIV_DEF;
> +		params->pl5_postdiv2 = PLL5_POSTDIV_DEF;
> +	}
> 
> -	params->pl5_intin = rate / MEGA;
> -	params->pl5_fracin = div_u64(((u64)rate % MEGA) << 24, MEGA);
> -	params->pl5_refdiv = 2;
> -	params->pl5_postdiv1 = 1;
> -	params->pl5_postdiv2 = 1;
>  	params->pl5_spread = 0x16;
> 
>  	foutvco_rate = div_u64(mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA,
> -					   (params->pl5_intin << 24) + params->pl5_fracin),
> -			       params->pl5_refdiv) >> 24;
> +		       (params->pl5_intin << 24) + params->pl5_fracin),
> +		       params->pl5_refdiv) >> 24;
>  	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
>  						 params->pl5_postdiv1 * params->pl5_postdiv2);
> 
> +	/* If foutvco is above 1.5GHz, change parent and recalculate */
> +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) 
> +{

Check patch is complaining:

CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
#146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
+	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000)) {
total: 0 errors, 0 warnings, 1 checks, 172 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.
0002-clk-renesas-rzg2l-Remove-DSI-clock-rate-restrictions.patch has style problems, please review.


Cheers,
Biju

RE: [PATCH 2/2] drm: renesas: rz-du: Set DSI divider based on target MIPI device

[Chris Brandt]

Hello Sergey,

> > +	/* Report required division ratio setting for the MIPI clock 
> > +dividers
>
>   /* should be on a line of its own (unless you do a networking patch)...

OK.


> > +	 * pllclk/2 = vclk * DSI divider
>
>    Why no spaces around / while there are spaces around *?

I missed that one.
Thank you.

Chris



-----Original Message-----
From: Sergey Shtylyov <sergei.shtylyov@gmail.com> 
Sent: Thursday, July 10, 2025 4:45 AM
To: Chris Brandt <Chris.Brandt@renesas.com>; Geert Uytterhoeven <geert+renesas@glider.be>; Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Biju Das <biju.das.jz@bp.renesas.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Hien Huynh <hien.huynh.px@renesas.com>; Nghia Vo <nghia.vo.zn@renesas.com>; Hugo Villeneuve <hugo@hugovil.com>
Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm: renesas: rz-du: Set DSI divider based on target MIPI device

On 7/9/25 11:55 PM, Chris Brandt wrote:

> Before the MIPI DSI clock source can be configured, the target divide 
> ratio needs to be known.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c 
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> index 4ad0be03a5b4..a8796df43f75 100644
> --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
[...]
> @@ -666,6 +667,22 @@ static int rzg2l_mipi_dsi_host_attach(struct 
> mipi_dsi_host *host,
>  
>  	drm_bridge_add(&dsi->bridge);
>  
> +	/* Report required division ratio setting for the MIPI clock 
> +dividers

   /* should be on a line of its own (unless you do a networking patch)...

> +	 * Assume the default clock source is FOUTPOSTDIV (PLL/2) being fed to the DSI-PHY, but also
> +	 * the DSI-PHY must be 16x the MIPI-DSI HS clock.
> +	 *
> +	 * pllclk/2 = vclk * DSI divider

   Why no spaces around / while there are spaces around *?

[...]

MBR, Sergey

RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Biju Das]

Hi Chris,

> -----Original Message-----
> From: Chris Brandt <Chris.Brandt@renesas.com>
> Sent: 10 July 2025 17:53
> Subject: RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions
> 
> Hi Biju,
> 
> Thank you for your review!
> 
> 
> > > +/* Required division ratio for the MIPI clock */ int dsi_div_ab;
> >
> > static int dsi_div_ab;
> 
> Good catch.
> 
> 
> > for the DPI, DIV_DSI_B = 1 and DIV_DSI_A ={2, 4, 8}
> >
> > So, you need to adjust the below calculation for DPI as well??
> 
> You bring up a good point.
> 
> And looking at the hardware manual again, there are other restrictions when using FOUTPOSTDIV (straight
> PLL) compared to FOUT1PH0 (PLL/2).
> 
> From a chip design standpoint, they just expect to have 'one big driver that configures everything at
> once'.
> 
> > Not sure do we need DSI driver registering a callback with CPG driver and CPG driver uses the
> callback to get DSI divider value and this callback can be used to distinguish DPI from DSI??
> 
> Ya, you can't just tell the CPG driver to 'give me this rate'. There is so much other information that
> it needs to have before it can set up the registers.
> Hmm....
> 
> 
> > > +found_clk:
> > > +	if (!found) {
> >
> > Can we add a dev_dbg statement here for !found clock?
> 
> Yes, good idea.
> That was in the original driver before I started pulling out all the printk statements.
> 
> 
> > > +	/* If foutvco is above 1.5GHz, change parent and recalculate */
> > > +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate >
> > > +1500000000)) {
> >
> > Check patch is complaining:
> >
> > CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
> > #146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
> > +	if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000))
> > +{
> 
> I saw that...but I thought the ( ) makes it a little easier to read.
> 
> But, what's the general rule here? Make checkpatch come out perfect?
> What's your thoughts?

I just ran check patch and it complained this. 
I am leaving Geert to comment on this.

Cheers,
Biju

Re: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Geert Uytterhoeven]

Hi Biju,

On Thu, 14 Aug 2025 at 08:08, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Chris Brandt <Chris.Brandt@renesas.com>

> > > > + /* If foutvco is above 1.5GHz, change parent and recalculate */
> > > > + if (priv->mux_dsi_div_params.clksrc && (foutvco_rate >
> > > > +1500000000)) {
> > >
> > > Check patch is complaining:
> > >
> > > CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
> > > #146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
> > > +   if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 1500000000))
> > > +{
> >
> > I saw that...but I thought the ( ) makes it a little easier to read.
> >
> > But, what's the general rule here? Make checkpatch come out perfect?
> > What's your thoughts?
>
> I just ran check patch and it complained this.
> I am leaving Geert to comment on this.

/me chimes in ;-)

You are not required to fix checkpatch warnings or errors if you have a
good reason to do so.  In this case, I see no reason for the parentheses
(it is not a very complex expression), so please drop them.
Thanks!

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

RE: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Chris Brandt]

Hi Geert and Biju

> You are not required to fix checkpatch warnings or errors if you have a good reason to do so.  In this case, I see no reason for
> the parentheses (it is not a very complex expression), so please drop them.

OK. Good to know!

I was working on another project, but starting next week I'm going to back to working on fixing up this driver.

As Biju mentioned, the PLL settings have different restrictions depending on if MIPI or DPI is being used.
So, somehow that all has to be sorted out (the correct way).

As usual, the chip designers make a simple table in the hardware manual that becomes difficult to satisfy in software.


Chris


-----Original Message-----
From: Geert Uytterhoeven <geert@linux-m68k.org> 
Sent: Thursday, August 14, 2025 3:39 AM
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Chris Brandt <Chris.Brandt@renesas.com>; Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Hien Huynh <hien.huynh.px@renesas.com>; Nghia Vo <nghia.vo.zn@renesas.com>; Hugo Villeneuve <hugo@hugovil.com>; linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

Hi Biju,

On Thu, 14 Aug 2025 at 08:08, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Chris Brandt <Chris.Brandt@renesas.com>

> > > > + /* If foutvco is above 1.5GHz, change parent and recalculate 
> > > > +*/  if (priv->mux_dsi_div_params.clksrc && (foutvco_rate >
> > > > +1500000000)) {
> > >
> > > Check patch is complaining:
> > >
> > > CHECK: Unnecessary parentheses around 'foutvco_rate > 1500000000'
> > > #146: FILE: drivers/clk/renesas/rzg2l-cpg.c:648:
> > > +   if (priv->mux_dsi_div_params.clksrc && (foutvco_rate > 
> > > +1500000000)) {
> >
> > I saw that...but I thought the ( ) makes it a little easier to read.
> >
> > But, what's the general rule here? Make checkpatch come out perfect?
> > What's your thoughts?
>
> I just ran check patch and it complained this.
> I am leaving Geert to comment on this.

/me chimes in ;-)

You are not required to fix checkpatch warnings or errors if you have a good reason to do so.  In this case, I see no reason for the parentheses (it is not a very complex expression), so please drop them.
Thanks!

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