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

[PATCH v3 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.
In general, panels or bridges can only be supported if MIPI-DSI lanes were 4.
If the number of lanes were 3,2,1, the math no longer works out.

A new API was created for the clock driver because the behaivior of the clock
driver depends on DPI vs MIPI, the screen resolution, and the number of MIPI
lanes.


Testing:
* RZ/G2L SMARC  (MIPI-DSI to HDMI bridge, lanes = 4)
* RZ/G2L-SBC    (MIPI-DSI to LCD panel, lanes = 2)
* RZ/G2UL SMARC (DPI to HDMI bridge)


Regression Testing:
There are 2 patches in this series.
If you just apply the first patch that only modifies the clock driver, the
operation of the RZ/G2L SMARC and RZ/G2UL SMARC remains the same.

However of course, the second patch is needed in the mipi-dsi driver in order
to make sure of the new API so that lanes 3,2,1 can be supported.

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               | 129 ++++++++++++++++--
 .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c    |  19 +++
 include/linux/clk/renesas.h                   |  11 ++
 3 files changed, 149 insertions(+), 10 deletions(-)

--
2.50.1

[PATCH v3 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>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

---
v1->v2:
- Remove unnecessary parentheses
- Add target argument to new API
- DPI mode has more restrictions on DIV_A and DIV_B

v2->v3:
- Removed Empty lines (Hugo)
- Add dummy for compile-testing CONFIG_CLK_RZG2L=n case (Geert)
- Renamed label found_dsi_div to calc_pll_clk (Hugo)
- Renamed label found_clk to clk_valid (Hugo)
- Removed 'found' var because not needed
- Move 'foutpostdiv_rate =' after if(foutvco_rate > 1500000000) (Hugo)
- Move PLL5_TARGET_* for new API to renesas.h (Hugo,Geert)
- Convert #define macros PLL5_TARGET_* to enum (Geert)
- static {unsigned} int dsi_div_ab; (Geert)
- {unsigned} int a, b;  (Geert)
- Change "((1 << a) * (b + 1))" to "(b + 1) << a"  (Geert)
- Change "foutvco_rate = rate * (1 << xxx ) * ..." to " = rate * ... * << xxx (Geert)
- Move (u64) outside of modulo operation to avoid helper on 32-bit compiles (Geert)
- Change DIV_ROUND_CLOSEST_ULL() to DIV_ROUND_CLOSEST() (Geert)
- void rzg2l_cpg_dsi_div_set_divider({unsinged} int divider, int target)
- Change "dsi_div_ab = (1 << AAA) * (BBB + 1)" to " = (BBB + 1) << AAA (Geert)
- Added Reviewed-by and Tested-by (Biju)
---
 drivers/clk/renesas/rzg2l-cpg.c | 129 +++++++++++++++++++++++++++++---
 include/linux/clk/renesas.h     |  11 +++
 2 files changed, 130 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 07909e80bae2..0e9362c2ca95 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -74,6 +74,19 @@
 #define MSTOP_OFF(conf)		FIELD_GET(GENMASK(31, 16), (conf))
 #define MSTOP_MASK(conf)	FIELD_GET(GENMASK(15, 0), (conf))
 
+#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
@@ -129,6 +142,12 @@ struct rzg2l_pll5_param {
 	u8 pl5_spread;
 };
 
+/* PLL5 output will be used for DPI or MIPI-DSI */
+static int dsi_div_target = PLL5_TARGET_DPI;
+
+/* Required division ratio for MIPI D-PHY clock changes depending on resolution and lanes. */
+static unsigned int dsi_div_ab;
+
 struct rzg2l_pll5_mux_dsi_div_param {
 	u8 clksrc;
 	u8 dsi_div_a;
@@ -557,23 +576,104 @@ 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;
+	unsigned 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++) {
+		if (dsi_div_target == PLL5_TARGET_DPI && a == 0)
+			continue;	/* 1/1 div not supported for DIV_DSI_A for DPI */
+
+		for (b = 0; b < 16; b++) {
+			if (dsi_div_target == PLL5_TARGET_DPI && b != 0)
+				continue;	/* Only 1/1 div supported for DIV_DSI_B in DPI */
 
-	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;
+			if ((b + 1) << a == dsi_div_ab) {
+				priv->mux_dsi_div_params.dsi_div_a = a;
+				priv->mux_dsi_div_params.dsi_div_b = b;
+
+				goto calc_pll_clk;
+			}
+		}
+	}
+
+calc_pll_clk:
+	/*
+	 * 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 * (priv->mux_dsi_div_params.dsi_div_b + 1) * div *
+				       params->pl5_postdiv1 * params->pl5_postdiv2 <<
+				       priv->mux_dsi_div_params.dsi_div_a;
+
+			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);
+
+				params->pl5_fracin = div_u64((u64)
+						     ((foutvco_rate * params->pl5_refdiv) %
+						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
+						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
+
+				goto clk_valid;
+			}
+		}
+	}
+
+	/* Set defaults since valid clock was not 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;
+
+clk_valid:
 	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;
-	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;
+		dsi_div_target = PLL5_TARGET_DSI;	/* Assume MIPI-DSI */
+		return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate);
+	}
+
+	foutpostdiv_rate = DIV_ROUND_CLOSEST(foutvco_rate,
+					     params->pl5_postdiv1 * params->pl5_postdiv2);
 
 	return foutpostdiv_rate;
 }
@@ -607,7 +707,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;
@@ -626,6 +726,13 @@ static int rzg2l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target)
+{
+	dsi_div_ab = divider;
+	dsi_div_target = target;
+}
+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)
@@ -858,7 +965,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);
@@ -948,6 +1055,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 = (priv->mux_dsi_div_params.dsi_div_b + 1) <<
+		     priv->mux_dsi_div_params.dsi_div_a;
 
 	return clk_hw->clk;
 }
diff --git a/include/linux/clk/renesas.h b/include/linux/clk/renesas.h
index 0ebbe2f0b45e..f2edd1f4a6d9 100644
--- a/include/linux/clk/renesas.h
+++ b/include/linux/clk/renesas.h
@@ -16,6 +16,11 @@ struct device;
 struct device_node;
 struct generic_pm_domain;
 
+enum {
+	PLL5_TARGET_DPI,
+	PLL5_TARGET_DSI
+};
+
 void cpg_mstp_add_clk_domain(struct device_node *np);
 #ifdef CONFIG_CLK_RENESAS_CPG_MSTP
 int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev);
@@ -32,4 +37,10 @@ void cpg_mssr_detach_dev(struct generic_pm_domain *unused, struct device *dev);
 #define cpg_mssr_attach_dev	NULL
 #define cpg_mssr_detach_dev	NULL
 #endif
+
+#ifdef CONFIG_CLK_RZG2L
+void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target);
+#else
+#define rzg2l_cpg_dsi_div_set_divider	NULL
+#endif
 #endif
-- 
2.50.1

[PATCH v3 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>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

---
v1->v2:
- Add spaces around '/' in comments
- Add target argument in new API

v2->v3:
- Add missing period in comment (Hugo)
- Changed '1' to 'PLL5_TARGET_DSI' (Hugo)
- Added Reviewed-by and Tested-by (Biju)
---
 .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c    | 19 +++++++++++++++++++
 1 file changed, 19 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 3b52dfc0ea1e..c9c5510bdd3e 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>
@@ -732,6 +733,24 @@ 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,
+				      PLL5_TARGET_DSI);
+
 	return 0;
 }
 
-- 
2.50.1

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

[Hugo Villeneuve]

Hi Chris,

On Wed, 22 Oct 2025 19:59:02 -0400
Chris Brandt <chris.brandt@renesas.com> wrote:

> 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>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> ---
> v1->v2:
> - Remove unnecessary parentheses
> - Add target argument to new API
> - DPI mode has more restrictions on DIV_A and DIV_B
> 
> v2->v3:
> - Removed Empty lines (Hugo)
> - Add dummy for compile-testing CONFIG_CLK_RZG2L=n case (Geert)
> - Renamed label found_dsi_div to calc_pll_clk (Hugo)
> - Renamed label found_clk to clk_valid (Hugo)
> - Removed 'found' var because not needed
> - Move 'foutpostdiv_rate =' after if(foutvco_rate > 1500000000) (Hugo)
> - Move PLL5_TARGET_* for new API to renesas.h (Hugo,Geert)
> - Convert #define macros PLL5_TARGET_* to enum (Geert)
> - static {unsigned} int dsi_div_ab; (Geert)
> - {unsigned} int a, b;  (Geert)
> - Change "((1 << a) * (b + 1))" to "(b + 1) << a"  (Geert)
> - Change "foutvco_rate = rate * (1 << xxx ) * ..." to " = rate * ... * << xxx (Geert)
> - Move (u64) outside of modulo operation to avoid helper on 32-bit compiles (Geert)
> - Change DIV_ROUND_CLOSEST_ULL() to DIV_ROUND_CLOSEST() (Geert)
> - void rzg2l_cpg_dsi_div_set_divider({unsinged} int divider, int target)
> - Change "dsi_div_ab = (1 << AAA) * (BBB + 1)" to " = (BBB + 1) << AAA (Geert)
> - Added Reviewed-by and Tested-by (Biju)
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 129 +++++++++++++++++++++++++++++---
>  include/linux/clk/renesas.h     |  11 +++
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
> index 07909e80bae2..0e9362c2ca95 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -74,6 +74,19 @@
>  #define MSTOP_OFF(conf)		FIELD_GET(GENMASK(31, 16), (conf))
>  #define MSTOP_MASK(conf)	FIELD_GET(GENMASK(15, 0), (conf))
>  
> +#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
> @@ -129,6 +142,12 @@ struct rzg2l_pll5_param {
>  	u8 pl5_spread;
>  };
>  
> +/* PLL5 output will be used for DPI or MIPI-DSI */
> +static int dsi_div_target = PLL5_TARGET_DPI;
> +
> +/* Required division ratio for MIPI D-PHY clock changes depending on resolution and lanes. */
> +static unsigned int dsi_div_ab;
> +
>  struct rzg2l_pll5_mux_dsi_div_param {
>  	u8 clksrc;
>  	u8 dsi_div_a;
> @@ -557,23 +576,104 @@ 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;
> +	unsigned 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++) {
> +		if (dsi_div_target == PLL5_TARGET_DPI && a == 0)
> +			continue;	/* 1/1 div not supported for DIV_DSI_A for DPI */
> +
> +		for (b = 0; b < 16; b++) {
> +			if (dsi_div_target == PLL5_TARGET_DPI && b != 0)
> +				continue;	/* Only 1/1 div supported for DIV_DSI_B in DPI */
>  
> -	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;
> +			if ((b + 1) << a == dsi_div_ab) {
> +				priv->mux_dsi_div_params.dsi_div_a = a;
> +				priv->mux_dsi_div_params.dsi_div_b = b;
> +
> +				goto calc_pll_clk;
> +			}
> +		}
> +	}
> +
> +calc_pll_clk:
> +	/*
> +	 * 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;

I think it would be easier to read/understand like this:
    params->pl5_postdiv1 <= PLL5_POSTDIV_MAX;

> +	     params->pl5_postdiv1++) {
> +		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
> +		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;

Ditto

> +		     params->pl5_postdiv2++) {
> +			foutvco_rate = rate * (priv->mux_dsi_div_params.dsi_div_b + 1) * div *
> +				       params->pl5_postdiv1 * params->pl5_postdiv2 <<
> +				       priv->mux_dsi_div_params.dsi_div_a;
> +
> +			if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 ||
> +			    foutvco_rate > PLL5_FOUTVCO_MAX - 1)

Similar:

    if (foutvco_rate <= PLL5_FOUTVCO_MIN ||
        foutvco_rate >= PLL5_FOUTVCO_MAX)

etc...

> +				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);
> +
> +				params->pl5_fracin = div_u64((u64)
> +						     ((foutvco_rate * params->pl5_refdiv) %
> +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);

Remove second identical block?

> +
> +				goto clk_valid;
> +			}
> +		}
> +	}
> +
> +	/* Set defaults since valid clock was not 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;
> +
> +clk_valid:
>  	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;
> -	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;
> +		dsi_div_target = PLL5_TARGET_DSI;	/* Assume MIPI-DSI */
> +		return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate);
> +	}
> +
> +	foutpostdiv_rate = DIV_ROUND_CLOSEST(foutvco_rate,
> +					     params->pl5_postdiv1 * params->pl5_postdiv2);
>  
>  	return foutpostdiv_rate;
>  }
> @@ -607,7 +707,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;
> @@ -626,6 +726,13 @@ static int rzg2l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
>  	return 0;
>  }
>  
> +void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target)
> +{
> +	dsi_div_ab = divider;
> +	dsi_div_target = target;
> +}
> +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)
> @@ -858,7 +965,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);
> @@ -948,6 +1055,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 = (priv->mux_dsi_div_params.dsi_div_b + 1) <<
> +		     priv->mux_dsi_div_params.dsi_div_a;
>  
>  	return clk_hw->clk;
>  }
> diff --git a/include/linux/clk/renesas.h b/include/linux/clk/renesas.h
> index 0ebbe2f0b45e..f2edd1f4a6d9 100644
> --- a/include/linux/clk/renesas.h
> +++ b/include/linux/clk/renesas.h
> @@ -16,6 +16,11 @@ struct device;
>  struct device_node;
>  struct generic_pm_domain;
>  
> +enum {
> +	PLL5_TARGET_DPI,
> +	PLL5_TARGET_DSI
> +};
> +
>  void cpg_mstp_add_clk_domain(struct device_node *np);
>  #ifdef CONFIG_CLK_RENESAS_CPG_MSTP
>  int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev);
> @@ -32,4 +37,10 @@ void cpg_mssr_detach_dev(struct generic_pm_domain *unused, struct device *dev);
>  #define cpg_mssr_attach_dev	NULL
>  #define cpg_mssr_detach_dev	NULL
>  #endif
> +
> +#ifdef CONFIG_CLK_RZG2L
> +void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target);
> +#else
> +#define rzg2l_cpg_dsi_div_set_divider	NULL
> +#endif
>  #endif
> -- 
> 2.50.1
> 
> 


-- 
Hugo Villeneuve

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

[Chris Brandt]

Hi Hugo,

On Wed, Oct 22, 2025 9:49 PM, Hugo Villeneuve wrote:

> > +	for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
> > +	     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
>
> I think it would be easier to read/understand like this:
>     params->pl5_postdiv1 <= PLL5_POSTDIV_MAX;
>
> > +	     params->pl5_postdiv1++) {
> > +		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
> > +		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
>
> Ditto

OK. I can agree with that.


> > +				params->pl5_fracin = div_u64(((u64)
> > +						     (foutvco_rate * params->pl5_refdiv) %
> > +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> > +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> > +
> > +				params->pl5_fracin = div_u64((u64)
> > +						     ((foutvco_rate * params->pl5_refdiv) %
> > +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> > +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
>
> Remove second identical block?

Wow! How did that get in there????

Thanks !

I'll wait a little to see if there are any other comments, then I'll send V4


Chris

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

[Hugo Villeneuve]

On Thu, 23 Oct 2025 16:47:17 +0000
Chris Brandt <Chris.Brandt@renesas.com> wrote:

> Hi Hugo,
> 
> On Wed, Oct 22, 2025 9:49 PM, Hugo Villeneuve wrote:
> 
> > > +	for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
> > > +	     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
> >
> > I think it would be easier to read/understand like this:
> >     params->pl5_postdiv1 <= PLL5_POSTDIV_MAX;
> >
> > > +	     params->pl5_postdiv1++) {
> > > +		for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
> > > +		     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
> >
> > Ditto
> 
> OK. I can agree with that.

If you do that, you can also probably put this if() on as single line
to improve readability:

    if (foutvco_rate <= PLL5_FOUTVCO_MIN ||
 foutvco_rate >= PLL5_FOUTVCO_MAX)

> 
> 
> > > +				params->pl5_fracin = div_u64(((u64)
> > > +						     (foutvco_rate * params->pl5_refdiv) %
> > > +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> > > +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> > > +
> > > +				params->pl5_fracin = div_u64((u64)
> > > +						     ((foutvco_rate * params->pl5_refdiv) %
> > > +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> > > +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> >
> > Remove second identical block?
> 
> Wow! How did that get in there????
> 
> Thanks !
> 
> I'll wait a little to see if there are any other comments, then I'll send V4
> 
> 
> Chris
> 


-- 
Hugo Villeneuve

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

[kernel test robot]

Hi Chris,

kernel test robot noticed the following build warnings:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on geert-renesas-drivers/renesas-clk sunxi/sunxi/for-next linus/master v6.18-rc2 next-20251023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chris-Brandt/clk-renesas-rzg2l-Remove-DSI-clock-rate-restrictions/20251023-080220
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20251022235903.1091453-2-chris.brandt%40renesas.com
patch subject: [PATCH v3 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions
config: loongarch-randconfig-001-20251023 (https://download.01.org/0day-ci/archive/20251024/202510240100.fpturxgp-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project e1ae12640102fd2b05bc567243580f90acb1135f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251024/202510240100.fpturxgp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510240100.fpturxgp-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clk/renesas/rzg2l-cpg.c:636:27: warning: result of comparison of constant 319 with expression of type 'u8' (aka 'unsigned char') is always false [-Wtautological-constant-out-of-range-compare]
     636 |                                     params->pl5_intin > PLL5_INTIN_MAX - 1)
         |                                     ~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +636 drivers/clk/renesas/rzg2l-cpg.c

   577	
   578	static unsigned long
   579	rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv,
   580				       struct rzg2l_pll5_param *params,
   581				       unsigned long rate)
   582	{
   583		unsigned long foutpostdiv_rate, foutvco_rate;
   584		u8 div = 1;
   585		unsigned int a, b;
   586	
   587		if (priv->mux_dsi_div_params.clksrc)
   588			div = 2;
   589	
   590		/* Calculate the DIV_DSI_A and DIV_DSI_B based on the final DIV DSI */
   591		for (a = 0; a < 4; a++) {
   592			if (dsi_div_target == PLL5_TARGET_DPI && a == 0)
   593				continue;	/* 1/1 div not supported for DIV_DSI_A for DPI */
   594	
   595			for (b = 0; b < 16; b++) {
   596				if (dsi_div_target == PLL5_TARGET_DPI && b != 0)
   597					continue;	/* Only 1/1 div supported for DIV_DSI_B in DPI */
   598	
   599				if ((b + 1) << a == dsi_div_ab) {
   600					priv->mux_dsi_div_params.dsi_div_a = a;
   601					priv->mux_dsi_div_params.dsi_div_b = b;
   602	
   603					goto calc_pll_clk;
   604				}
   605			}
   606		}
   607	
   608	calc_pll_clk:
   609		/*
   610		 * Below conditions must be set for PLL5 parameters:
   611		 * - REFDIV must be between 1 and 2.
   612		 * - POSTDIV1/2 must be between 1 and 7.
   613		 * - INTIN must be between 20 and 320.
   614		 * - FOUTVCO must be between 800MHz and 3000MHz.
   615		 */
   616		for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN;
   617		     params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1;
   618		     params->pl5_postdiv1++) {
   619			for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN;
   620			     params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1;
   621			     params->pl5_postdiv2++) {
   622				foutvco_rate = rate * (priv->mux_dsi_div_params.dsi_div_b + 1) * div *
   623					       params->pl5_postdiv1 * params->pl5_postdiv2 <<
   624					       priv->mux_dsi_div_params.dsi_div_a;
   625	
   626				if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 ||
   627				    foutvco_rate > PLL5_FOUTVCO_MAX - 1)
   628					continue;
   629	
   630				for (params->pl5_refdiv = PLL5_REFDIV_MIN;
   631				     params->pl5_refdiv < PLL5_REFDIV_MAX + 1;
   632				     params->pl5_refdiv++) {
   633					params->pl5_intin = (foutvco_rate * params->pl5_refdiv) /
   634							    (EXTAL_FREQ_IN_MEGA_HZ * MEGA);
   635					if (params->pl5_intin < PLL5_INTIN_MIN + 1 ||
 > 636					    params->pl5_intin > PLL5_INTIN_MAX - 1)
   637						continue;
   638					params->pl5_fracin = div_u64(((u64)
   639							     (foutvco_rate * params->pl5_refdiv) %
   640							     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
   641							     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
   642	
   643					params->pl5_fracin = div_u64((u64)
   644							     ((foutvco_rate * params->pl5_refdiv) %
   645							     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
   646							     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
   647	
   648					goto clk_valid;
   649				}
   650			}
   651		}
   652	
   653		/* Set defaults since valid clock was not found */
   654		params->pl5_intin = PLL5_INTIN_DEF;
   655		params->pl5_fracin = PLL5_FRACIN_DEF;
   656		params->pl5_refdiv = PLL5_REFDIV_DEF;
   657		params->pl5_postdiv1 = PLL5_POSTDIV_DEF;
   658		params->pl5_postdiv2 = PLL5_POSTDIV_DEF;
   659	
   660	clk_valid:
   661		params->pl5_spread = 0x16;
   662	
   663		foutvco_rate = div_u64(mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA,
   664						   (params->pl5_intin << 24) + params->pl5_fracin),
   665				       params->pl5_refdiv) >> 24;
   666	
   667		/* If foutvco is above 1.5GHz, change parent and recalculate */
   668		if (priv->mux_dsi_div_params.clksrc && foutvco_rate > 1500000000) {
   669			priv->mux_dsi_div_params.clksrc = 0;
   670			dsi_div_ab *= 2;
   671			dsi_div_target = PLL5_TARGET_DSI;	/* Assume MIPI-DSI */
   672			return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate);
   673		}
   674	
   675		foutpostdiv_rate = DIV_ROUND_CLOSEST(foutvco_rate,
   676						     params->pl5_postdiv1 * params->pl5_postdiv2);
   677	
   678		return foutpostdiv_rate;
   679	}
   680	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Chris,

kernel test robot noticed the following build errors:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on geert-renesas-drivers/renesas-clk sunxi/sunxi/for-next linus/master v6.18-rc2 next-20251023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chris-Brandt/clk-renesas-rzg2l-Remove-DSI-clock-rate-restrictions/20251023-080220
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20251022235903.1091453-3-chris.brandt%40renesas.com
patch subject: [PATCH v3 2/2] drm: renesas: rz-du: Set DSI divider based on target MIPI device
config: powerpc-randconfig-002-20251023 (https://download.01.org/0day-ci/archive/20251024/202510240111.DvVTqUEp-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project e1ae12640102fd2b05bc567243580f90acb1135f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251024/202510240111.DvVTqUEp-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510240111.DvVTqUEp-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c:751:31: error: called object type 'void *' is not a function or function pointer
     751 |         rzg2l_cpg_dsi_div_set_divider(mipi_dsi_pixel_format_to_bpp(dsi->format) / dsi->lanes,
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
   1 error generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for HOTPLUG_CPU
   Depends on [n]: SMP [=y] && (PPC_PSERIES [=n] || PPC_PMAC [=n] || PPC_POWERNV [=n] || FSL_SOC_BOOKE [=n])
   Selected by [y]:
   - PM_SLEEP_SMP [=y] && SMP [=y] && (ARCH_SUSPEND_POSSIBLE [=y] || ARCH_HIBERNATION_POSSIBLE [=y]) && PM_SLEEP [=y]


vim +751 drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c

   687	
   688	/* -----------------------------------------------------------------------------
   689	 * Host setting
   690	 */
   691	
   692	static int rzg2l_mipi_dsi_host_attach(struct mipi_dsi_host *host,
   693					      struct mipi_dsi_device *device)
   694	{
   695		struct rzg2l_mipi_dsi *dsi = host_to_rzg2l_mipi_dsi(host);
   696		int ret;
   697	
   698		if (device->lanes > dsi->num_data_lanes) {
   699			dev_err(dsi->dev,
   700				"Number of lines of device (%u) exceeds host (%u)\n",
   701				device->lanes, dsi->num_data_lanes);
   702			return -EINVAL;
   703		}
   704	
   705		switch (mipi_dsi_pixel_format_to_bpp(device->format)) {
   706		case 24:
   707			break;
   708		case 18:
   709			break;
   710		case 16:
   711			if (!(dsi->info->features & RZ_MIPI_DSI_FEATURE_16BPP)) {
   712				dev_err(dsi->dev, "Unsupported format 0x%04x\n",
   713					device->format);
   714				return -EINVAL;
   715			}
   716			break;
   717		default:
   718			dev_err(dsi->dev, "Unsupported format 0x%04x\n", device->format);
   719			return -EINVAL;
   720		}
   721	
   722		dsi->lanes = device->lanes;
   723		dsi->format = device->format;
   724		dsi->mode_flags = device->mode_flags;
   725	
   726		dsi->next_bridge = devm_drm_of_get_bridge(dsi->dev, dsi->dev->of_node,
   727							  1, 0);
   728		if (IS_ERR(dsi->next_bridge)) {
   729			ret = PTR_ERR(dsi->next_bridge);
   730			dev_err(dsi->dev, "failed to get next bridge: %d\n", ret);
   731			return ret;
   732		}
   733	
   734		drm_bridge_add(&dsi->bridge);
   735	
   736		/*
   737		 * Report required division ratio setting for the MIPI clock dividers.
   738		 * Assume the default clock source is FOUTPOSTDIV (PLL/2) being fed to the DSI-PHY, but also
   739		 * the DSI-PHY must be 16x the MIPI-DSI HS clock.
   740		 *
   741		 * pllclk / 2 = vclk * DSI divider
   742		 * pllclk = vclk * DSI divider * 2
   743		 *
   744		 * hsclk = (vclk * DSI divider * 2) / 16
   745		 *
   746		 * vclk * bpp = hsclk * 8 * num_lanes
   747		 * vclk * bpp = ((vclk * DSI divider * 2) / 16) * 8 * num_lanes
   748		 *   which simplifies to...
   749		 * DSI divider = bpp / num_lanes
   750		 */
 > 751		rzg2l_cpg_dsi_div_set_divider(mipi_dsi_pixel_format_to_bpp(dsi->format) / dsi->lanes,
   752					      PLL5_TARGET_DSI);
   753	
   754		return 0;
   755	}
   756	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Chris Brandt]

Hi Hugo,

On Thu, Oct 23, 2025 2:03 PM, Hugo Villeneuve wrote:

> If you do that, you can also probably put this if() on as single line to improve readability:
>
>     if (foutvco_rate <= PLL5_FOUTVCO_MIN ||  foutvco_rate >= PLL5_FOUTVCO_MAX)

You're right!
With 3 characters left to spare on the line :)

Thanks,
Chris

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

[Hugo Villeneuve]

Hi Chris,

On Wed, 22 Oct 2025 19:59:02 -0400
Chris Brandt <chris.brandt@renesas.com> wrote:

> 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>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> ---
> v1->v2:
> - Remove unnecessary parentheses
> - Add target argument to new API
> - DPI mode has more restrictions on DIV_A and DIV_B
> 
> v2->v3:
> - Removed Empty lines (Hugo)
> - Add dummy for compile-testing CONFIG_CLK_RZG2L=n case (Geert)
> - Renamed label found_dsi_div to calc_pll_clk (Hugo)
> - Renamed label found_clk to clk_valid (Hugo)
> - Removed 'found' var because not needed
> - Move 'foutpostdiv_rate =' after if(foutvco_rate > 1500000000) (Hugo)
> - Move PLL5_TARGET_* for new API to renesas.h (Hugo,Geert)
> - Convert #define macros PLL5_TARGET_* to enum (Geert)
> - static {unsigned} int dsi_div_ab; (Geert)
> - {unsigned} int a, b;  (Geert)
> - Change "((1 << a) * (b + 1))" to "(b + 1) << a"  (Geert)
> - Change "foutvco_rate = rate * (1 << xxx ) * ..." to " = rate * ... * << xxx (Geert)
> - Move (u64) outside of modulo operation to avoid helper on 32-bit compiles (Geert)
> - Change DIV_ROUND_CLOSEST_ULL() to DIV_ROUND_CLOSEST() (Geert)
> - void rzg2l_cpg_dsi_div_set_divider({unsinged} int divider, int target)
> - Change "dsi_div_ab = (1 << AAA) * (BBB + 1)" to " = (BBB + 1) << AAA (Geert)
> - Added Reviewed-by and Tested-by (Biju)
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 129 +++++++++++++++++++++++++++++---
>  include/linux/clk/renesas.h     |  11 +++
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
> index 07909e80bae2..0e9362c2ca95 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -74,6 +74,19 @@
>  #define MSTOP_OFF(conf)		FIELD_GET(GENMASK(31, 16), (conf))
>  #define MSTOP_MASK(conf)	FIELD_GET(GENMASK(15, 0), (conf))
>  
> +#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
> @@ -129,6 +142,12 @@ struct rzg2l_pll5_param {
>  	u8 pl5_spread;
>  };
>  
> +/* PLL5 output will be used for DPI or MIPI-DSI */
> +static int dsi_div_target = PLL5_TARGET_DPI;
> +
> +/* Required division ratio for MIPI D-PHY clock changes depending on resolution and lanes. */
> +static unsigned int dsi_div_ab;
> +
>  struct rzg2l_pll5_mux_dsi_div_param {
>  	u8 clksrc;
>  	u8 dsi_div_a;
> @@ -557,23 +576,104 @@ 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;
> +	unsigned 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++) {
> +		if (dsi_div_target == PLL5_TARGET_DPI && a == 0)
> +			continue;	/* 1/1 div not supported for DIV_DSI_A for DPI */
> +
> +		for (b = 0; b < 16; b++) {
> +			if (dsi_div_target == PLL5_TARGET_DPI && b != 0)
> +				continue;	/* Only 1/1 div supported for DIV_DSI_B in DPI */
>  
> -	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;
> +			if ((b + 1) << a == dsi_div_ab) {
> +				priv->mux_dsi_div_params.dsi_div_a = a;
> +				priv->mux_dsi_div_params.dsi_div_b = b;
> +
> +				goto calc_pll_clk;
> +			}
> +		}
> +	}

If we arrive at this point, it seems that these values:
    priv->mux_dsi_div_params.dsi_div_a
    priv->mux_dsi_div_params.dsi_div_b

were not initialised by the previous loop. Is this expected? If yes,
maybe a comment would help?

> +
> +calc_pll_clk:
> +	/*
> +	 * Below conditions must be set for PLL5 parameters:
> +	 * - REFDIV must be between 1 and 2.

I am assuming this means PLL5_POSTDIV_MIN and PLL5_POSTDIV_MAX? If
these macros change, then that mean you would also need to change your
comment, not very practical and error-prone. I would suggest to remove
this comment altogether.

> +	 * - POSTDIV1/2 must be between 1 and 7.
> +	 * - INTIN must be between 20 and 320.
> +	 * - FOUTVCO must be between 800MHz and 3000MHz.

Same here.

> +	 */
> +	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 * (priv->mux_dsi_div_params.dsi_div_b + 1) * div *
> +				       params->pl5_postdiv1 * params->pl5_postdiv2 <<
> +				       priv->mux_dsi_div_params.dsi_div_a;
> +
> +			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);
> +
> +				params->pl5_fracin = div_u64((u64)
> +						     ((foutvco_rate * params->pl5_refdiv) %
> +						     (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24,
> +						     EXTAL_FREQ_IN_MEGA_HZ * MEGA);
> +
> +				goto clk_valid;
> +			}
> +		}
> +	}
> +
> +	/* Set defaults since valid clock was not 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;
> +
> +clk_valid:
>  	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;
> -	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> -						 params->pl5_postdiv1 * params->pl5_postdiv2);
> +
> +	/* If foutvco is above 1.5GHz, change parent and recalculate */

Similar suggestion for hardcoded values in comments, maybe replace
"above 1.5GHz" with "too high"...

> +	if (priv->mux_dsi_div_params.clksrc && foutvco_rate > 1500000000) {

Define a macro for 1500000000?

> +		priv->mux_dsi_div_params.clksrc = 0;
> +		dsi_div_ab *= 2;
> +		dsi_div_target = PLL5_TARGET_DSI;	/* Assume MIPI-DSI */
> +		return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate);
> +	}
> +
> +	foutpostdiv_rate = DIV_ROUND_CLOSEST(foutvco_rate,
> +					     params->pl5_postdiv1 * params->pl5_postdiv2);
>  
>  	return foutpostdiv_rate;
>  }
> @@ -607,7 +707,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;
> @@ -626,6 +726,13 @@ static int rzg2l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
>  	return 0;
>  }
>  
> +void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target)
> +{
> +	dsi_div_ab = divider;
> +	dsi_div_target = target;
> +}
> +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)
> @@ -858,7 +965,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);
> @@ -948,6 +1055,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 = (priv->mux_dsi_div_params.dsi_div_b + 1) <<
> +		     priv->mux_dsi_div_params.dsi_div_a;
>  
>  	return clk_hw->clk;
>  }
> diff --git a/include/linux/clk/renesas.h b/include/linux/clk/renesas.h
> index 0ebbe2f0b45e..f2edd1f4a6d9 100644
> --- a/include/linux/clk/renesas.h
> +++ b/include/linux/clk/renesas.h
> @@ -16,6 +16,11 @@ struct device;
>  struct device_node;
>  struct generic_pm_domain;
>  
> +enum {
> +	PLL5_TARGET_DPI,
> +	PLL5_TARGET_DSI
> +};
> +
>  void cpg_mstp_add_clk_domain(struct device_node *np);
>  #ifdef CONFIG_CLK_RENESAS_CPG_MSTP
>  int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev);
> @@ -32,4 +37,10 @@ void cpg_mssr_detach_dev(struct generic_pm_domain *unused, struct device *dev);
>  #define cpg_mssr_attach_dev	NULL
>  #define cpg_mssr_detach_dev	NULL
>  #endif
> +
> +#ifdef CONFIG_CLK_RZG2L
> +void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target);
> +#else
> +#define rzg2l_cpg_dsi_div_set_divider	NULL
> +#endif
>  #endif
> -- 
> 2.50.1
> 
> 


-- 
Hugo Villeneuve

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

[Geert Uytterhoeven]

Hi Chris,

On Thu, 23 Oct 2025 at 01:59, Chris Brandt <chris.brandt@renesas.com> wrote:
> 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>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

> v2->v3:
> - Removed Empty lines (Hugo)
> - Add dummy for compile-testing CONFIG_CLK_RZG2L=n case (Geert)
> - Renamed label found_dsi_div to calc_pll_clk (Hugo)
> - Renamed label found_clk to clk_valid (Hugo)
> - Removed 'found' var because not needed
> - Move 'foutpostdiv_rate =' after if(foutvco_rate > 1500000000) (Hugo)
> - Move PLL5_TARGET_* for new API to renesas.h (Hugo,Geert)
> - Convert #define macros PLL5_TARGET_* to enum (Geert)
> - static {unsigned} int dsi_div_ab; (Geert)
> - {unsigned} int a, b;  (Geert)
> - Change "((1 << a) * (b + 1))" to "(b + 1) << a"  (Geert)
> - Change "foutvco_rate = rate * (1 << xxx ) * ..." to " = rate * ... * << xxx (Geert)
> - Move (u64) outside of modulo operation to avoid helper on 32-bit compiles (Geert)
> - Change DIV_ROUND_CLOSEST_ULL() to DIV_ROUND_CLOSEST() (Geert)
> - void rzg2l_cpg_dsi_div_set_divider({unsinged} int divider, int target)
> - Change "dsi_div_ab = (1 << AAA) * (BBB + 1)" to " = (BBB + 1) << AAA (Geert)
> - Added Reviewed-by and Tested-by (Biju)'

Thanks for the update!

> --- a/include/linux/clk/renesas.h
> +++ b/include/linux/clk/renesas.h
> @@ -16,6 +16,11 @@ struct device;
>  struct device_node;
>  struct generic_pm_domain;
>
> +enum {
> +       PLL5_TARGET_DPI,
> +       PLL5_TARGET_DSI
> +};
> +
>  void cpg_mstp_add_clk_domain(struct device_node *np);
>  #ifdef CONFIG_CLK_RENESAS_CPG_MSTP
>  int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev);
> @@ -32,4 +37,10 @@ void cpg_mssr_detach_dev(struct generic_pm_domain *unused, struct device *dev);
>  #define cpg_mssr_attach_dev    NULL
>  #define cpg_mssr_detach_dev    NULL
>  #endif
> +
> +#ifdef CONFIG_CLK_RZG2L
> +void rzg2l_cpg_dsi_div_set_divider(unsigned int divider, int target);
> +#else
> +#define rzg2l_cpg_dsi_div_set_divider  NULL

static inline void rzg2l_cpg_dsi_div_set_divider(int divider, int target) { }

For cpg_mssr_attach_dev and friends, NULL is suitable because these
are only used to populate function pointers.

> +#endif

Blank line please.

>  #endif

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 v3 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions

[Chris Brandt]

Hi Hugo,

Thank you again for the review.
:)


On Thu, Oct 23, 2025 2:32 PM, Hugo Villeneuve wrote:
> > +			if ((b + 1) << a == dsi_div_ab) {
> > +				priv->mux_dsi_div_params.dsi_div_a = a;
> > +				priv->mux_dsi_div_params.dsi_div_b = b;
> > +
> > +				goto calc_pll_clk;
> > +			}
> > +		}
> > +	}
>
> If we arrive at this point, it seems that these values:
>     priv->mux_dsi_div_params.dsi_div_a
>     priv->mux_dsi_div_params.dsi_div_b
>
> were not initialised by the previous loop. Is this expected? If yes, maybe a comment would help?

Actually, I think I want to print out a warning.

dev_warn(priv->dev, "Failed to calculate DIV_DSI A,B\n");

I could not find a monitor that would cause this, but it's possible.
So to be kind to the users, I will print out a message so if something does not work, at least
they will have an idea what is wrong.


> > +calc_pll_clk:
> > +	/*
> > +	 * Below conditions must be set for PLL5 parameters:
> > +	 * - REFDIV must be between 1 and 2.
>
> I am assuming this means PLL5_POSTDIV_MIN and PLL5_POSTDIV_MAX? If these macros change, then that mean you would also need to change your comment, not very practical and error-prone. I would suggest to remove this comment altogether.

"REFDIV" is the term used in the hardware manual to reference the signal.

> > +	 * - POSTDIV1/2 must be between 1 and 7.
> > +	 * - INTIN must be between 20 and 320.
> > +	 * - FOUTVCO must be between 800MHz and 3000MHz.
>
> Same here.

While I doubt the hardware design will change, your point is valid that I'm not giving the reader
any more info than they could get from the code.

Kind of like the classic source code comment:

    value = 5;   /* Set value to 5 */

I'll remove it.

> +	/* Set defaults since valid clock was not 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;

I'm going to print out a warning here too.

dev_warn(priv->dev, "Failed to calculate exact PLL5 settings\n");


> > -	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> > -						 params->pl5_postdiv1 * params->pl5_postdiv2);
> > +
> > +	/* If foutvco is above 1.5GHz, change parent and recalculate */
>
> Similar suggestion for hardcoded values in comments, maybe replace "above 1.5GHz" with "too high"...

This one I'm OK with because that's the design specification of the hardware IP that's used in all the devices.
If for some reason they re-design the hardware in future devices, something going to have to change and
the driver will need to be updated. So we'll deal with it at that point.

Cheers

Chris

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

[Hugo Villeneuve]

Hi Chris,

On Tue, 28 Oct 2025 16:17:51 +0000
Chris Brandt <Chris.Brandt@renesas.com> wrote:

> Hi Hugo,
> 
> Thank you again for the review.
> :)
> 
> 
> On Thu, Oct 23, 2025 2:32 PM, Hugo Villeneuve wrote:
> > > +			if ((b + 1) << a == dsi_div_ab) {
> > > +				priv->mux_dsi_div_params.dsi_div_a = a;
> > > +				priv->mux_dsi_div_params.dsi_div_b = b;
> > > +
> > > +				goto calc_pll_clk;
> > > +			}
> > > +		}
> > > +	}
> >
> > If we arrive at this point, it seems that these values:
> >     priv->mux_dsi_div_params.dsi_div_a
> >     priv->mux_dsi_div_params.dsi_div_b
> >
> > were not initialised by the previous loop. Is this expected? If yes, maybe a comment would help?

So are the uninitialised values valid at all?

> 
> Actually, I think I want to print out a warning.
> 
> dev_warn(priv->dev, "Failed to calculate DIV_DSI A,B\n");
> 
> I could not find a monitor that would cause this, but it's possible.
> So to be kind to the users, I will print out a message so if something does not work, at least
> they will have an idea what is wrong.

So is it ok to continue in this case?

If yes, then maybe your message could specify: "Failed to calculate
DIV_DSI A,B, using defaults\n" ?


> 
> > > +calc_pll_clk:
> > > +	/*
> > > +	 * Below conditions must be set for PLL5 parameters:
> > > +	 * - REFDIV must be between 1 and 2.
> >
> > I am assuming this means PLL5_POSTDIV_MIN and PLL5_POSTDIV_MAX? If these macros change, then that mean you would also need to change your comment, not very practical and error-prone. I would suggest to remove this comment altogether.
> 
> "REFDIV" is the term used in the hardware manual to reference the signal.
> 
> > > +	 * - POSTDIV1/2 must be between 1 and 7.
> > > +	 * - INTIN must be between 20 and 320.
> > > +	 * - FOUTVCO must be between 800MHz and 3000MHz.
> >
> > Same here.
> 
> While I doubt the hardware design will change, your point is valid that I'm not giving the reader
> any more info than they could get from the code.
> 
> Kind of like the classic source code comment:
> 
>     value = 5;   /* Set value to 5 */
> 
> I'll remove it.

Ok :)


> 
> > +	/* Set defaults since valid clock was not 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;
> 
> I'm going to print out a warning here too.
> 
> dev_warn(priv->dev, "Failed to calculate exact PLL5 settings\n");

Similar to my comment above, would it be a good idea to add something
like "Failed to calculate exact PLL5 settings, using defaults\n" ?

> 
> 
> > > -	foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate,
> > > -						 params->pl5_postdiv1 * params->pl5_postdiv2);
> > > +
> > > +	/* If foutvco is above 1.5GHz, change parent and recalculate */
> >
> > Similar suggestion for hardcoded values in comments, maybe replace "above 1.5GHz" with "too high"...
> 
> This one I'm OK with because that's the design specification of the hardware IP that's used in all the devices.
> If for some reason they re-design the hardware in future devices, something going to have to change and
> the driver will need to be updated. So we'll deal with it at that point.
> 
> Cheers
> 
> Chris
> 


-- 
Hugo Villeneuve

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

[Chris Brandt]

Hi Hugo,

On Tue, Oct 28, 2025 12:39 PM, Hugo Villeneuve wrote:

> > > If we arrive at this point, it seems that these values:
> > >     priv->mux_dsi_div_params.dsi_div_a
> > >     priv->mux_dsi_div_params.dsi_div_b
> > >
> > > were not initialised by the previous loop. Is this expected? If yes, maybe a comment would help?
>
> So are the uninitialised values valid at all?

Actually, after some more testing......I remember what I did.

The PLL5 needs to support both the MIPI-DSI and DPI (Parallel) use cases.

But, since the execution paths are different in the kernel for MIPI vs DPI, I needed to make the default settings for DPI
knowing that if MIPI-DSI was used, they would get overwritten (hence the new API was introduced)

However, current defaults today in the driver are illegal for DPI, even though they clearly work in real life on all the RZ/G2UL boards.
	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 */

Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI"

So what we have today works fine, but technically does not match the hardware manual for the DPI case.
I need to change the code around for the DPI case and test again to make sure nothing breaks.

Ugh!

> > dev_warn(priv->dev, "Failed to calculate exact PLL5 settings\n");
>
> Similar to my comment above, would it be a good idea to add something like "Failed to calculate exact PLL5 settings, using defaults\n" ?

I can agree to that.
I'll change the message.

Chris

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

[Biju Das]

Hi Chris,

> -----Original Message-----
> From: Chris Brandt <Chris.Brandt@renesas.com>
> Sent: 28 October 2025 17:46
> Subject: RE: [PATCH v3 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions
> 
> Hi Hugo,
> 
> On Tue, Oct 28, 2025 12:39 PM, Hugo Villeneuve wrote:
> 
> > > > If we arrive at this point, it seems that these values:
> > > >     priv->mux_dsi_div_params.dsi_div_a
> > > >     priv->mux_dsi_div_params.dsi_div_b
> > > >
> > > > were not initialised by the previous loop. Is this expected? If yes, maybe a comment would help?
> >
> > So are the uninitialised values valid at all?
> 
> Actually, after some more testing......I remember what I did.
> 
> The PLL5 needs to support both the MIPI-DSI and DPI (Parallel) use cases.
> 
> But, since the execution paths are different in the kernel for MIPI vs DPI, I needed to make the
> default settings for DPI knowing that if MIPI-DSI was used, they would get overwritten (hence the new
> API was introduced)
> 
> However, current defaults today in the driver are illegal for DPI, even though they clearly work in
> real life on all the RZ/G2UL boards.
> 	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 */
> 
> Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI"

Why? 

This is correct as per hardware manual page 283 and [1] and [2]

[1] https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/clk/renesas/r9a07g044-cpg.c#L142 -->RZ/G2L.

[2] https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/clk/renesas/r9a07g043-cpg.c#L159 -->RZ/G2UL

Cheers,
Biju

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

[Chris Brandt]

Hi Biju,

On Tue, Oct 28, 2025 3:07 PM, Biju Das wrote:

> > However, current defaults today in the driver are illegal for DPI, 
> > even though they clearly work in real life on all the RZ/G2UL boards.
> > 	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 */
> > 
> > Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI"
>
> Why? 
>
> This is correct as per hardware manual page 283 and [1] and [2]

For "DPI", both RZ/G2L and RZ/G2UL have the same table and same restrictions.

Valid values for register bits for DIVDSIA_SET =  1,2,3
Valid values for register bits for DIVDSIB_SET = 0

Remember, this is for DPI (not MIPI-DSI)

You were the one that pointed this out to me.

The driver needs to default to DPI mode (PLL5_TARGET_DPI ) because for RZ/G2UL, no one is going to call the new API to change it.

> > 	priv->mux_dsi_div_params.dsi_div_a = 1; /* Divided by 2 */
> > 	priv->mux_dsi_div_params.dsi_div_b = 2; /* Divided by 3 */

So the current defaults of DIV_B = 2 is no good.
DIV_B must always be '0'


I just changed the code to this,

	/* Default settings for DPI */
	priv->mux_dsi_div_params.clksrc = 1;
	priv->mux_dsi_div_params.dsi_div_a = 3; /* Divided by 8 */
	priv->mux_dsi_div_params.dsi_div_b = 0; /* Divided by 1 */
	dsi_div_ab = (priv->mux_dsi_div_params.dsi_div_b + 1) <<
		     priv->mux_dsi_div_params.dsi_div_a;

And on a RZ/G2UL (DPI) EVK, resolutions between 640x48 to 1920x1080 all work.

Do you agree? Or am I missing something?


Chris

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

[Biju Das]

Hi Chris,

> -----Original Message-----
> From: Chris Brandt <Chris.Brandt@renesas.com>
> Sent: 28 October 2025 19:43
> Subject: RE: [PATCH v3 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions
> 
> Hi Biju,
> 
> On Tue, Oct 28, 2025 3:07 PM, Biju Das wrote:
> 
> > > However, current defaults today in the driver are illegal for DPI,
> > > even though they clearly work in real life on all the RZ/G2UL boards.
> > > 	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 */
> > >
> > > Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI"
> >
> > Why?
> >
> > This is correct as per hardware manual page 283 and [1] and [2]
> 
> For "DPI", both RZ/G2L and RZ/G2UL have the same table and same restrictions.
> 
> Valid values for register bits for DIVDSIA_SET =  1,2,3 Valid values for register bits for DIVDSIB_SET
> = 0
> 
> Remember, this is for DPI (not MIPI-DSI)
> 
> You were the one that pointed this out to me.
> 
> The driver needs to default to DPI mode (PLL5_TARGET_DPI ) because for RZ/G2UL, no one is going to
> call the new API to change it.
> 
> > > 	priv->mux_dsi_div_params.dsi_div_a = 1; /* Divided by 2 */
> > > 	priv->mux_dsi_div_params.dsi_div_b = 2; /* Divided by 3 */
> 
> So the current defaults of DIV_B = 2 is no good.
> DIV_B must always be '0'
> 
> 
> I just changed the code to this,
> 
> 	/* Default settings for DPI */
> 	priv->mux_dsi_div_params.clksrc = 1;
> 	priv->mux_dsi_div_params.dsi_div_a = 3; /* Divided by 8 */
> 	priv->mux_dsi_div_params.dsi_div_b = 0; /* Divided by 1 */
> 	dsi_div_ab = (priv->mux_dsi_div_params.dsi_div_b + 1) <<
> 		     priv->mux_dsi_div_params.dsi_div_a;
> 
> And on a RZ/G2UL (DPI) EVK, resolutions between 640x48 to 1920x1080 all work.
> 
> Do you agree? Or am I missing something?

I was referring to your comment
" Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI""

DPI always use FOUTPOSTDIV. So, clksrc=0 always for RZ/G2UL.
See [1] and [2]

It is illegal clksrc=1(FOUT1PH0) for RZ/G2UL. You can cross check mux register value
with RZ/G2UL.

DSI uses mux and it can take clksrc=0(FOUTPOSTDIV) or clksrc=1(FOUT1PH0)

[1] https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/clk/renesas/r9a07g043-cpg.c#L159
[2] https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/clk/renesas/rzg2l-cpg.c#L738

Cheers,
Biju

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

[Chris Brandt]

Hi Biju,

On Tue, Oct 28, 2025 4:12 PM, Biju Das wrote:
> Do you agree? Or am I missing something?
>
> I was referring to your comment
> " Side note, that code comment "Use clk src 1 for DSI " is wrong ...it should say "DPI""
>
> DPI always use FOUTPOSTDIV. So, clksrc=0 always for RZ/G2UL.

Hmm...

I have code that was tested on all the boards, but you're right that it does not
match the documentation.

So now I have to change the code to match the documentation, but keep the functionality the same.

Then, retest it all.

:(

Chris