Hi Chris, > -----Original Message----- > From: Chris Brandt <[email protected]> > Sent: 11 November 2025 20:43 > Subject: RE: [PATCH v4 1/2] clk: renesas: rzg2l: Remove DSI clock rate > restrictions > > Hi Biju > > On Sat, Nov 8, 2025 3:30 AM, Biju Das wrote: > > > + if (dsi_div_target == PLL5_TARGET_DPI) { > > > + /* Fixed settings for DPI */ > > > + priv->mux_dsi_div_params.clksrc = 0; > > > + 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_desired = 8; /* (1 << a) * (b + 1) */ > > > > This block is duplicated may be add a helper function(), if you are > > planning to send another series. > > Actually, I just found another issue with the current driver when it was > assuming everything to be > hard-coded. > > The current code calls rzg2l_cpg_get_vclk_rate() BEFORE calling > rzg2l_cpg_get_foutpostdiv_rate(). > > vclk_rate = rzg2l_cpg_get_vclk_rate(hw, rate); > sipll5->foutpostdiv_rate = > rzg2l_cpg_get_foutpostdiv_rate(priv, ¶ms, vclk_rate); > > > The problem is that rzg2l_cpg_get_vclk_rate() is using the current values of > dsi_div_a, dsi_div_b and > clksrc to calculate a vclk. > But, the real values of dsi_div_a, dsi_div_b and clksrc are not set until > later in > rzg2l_cpg_get_foutpostdiv_rate(). > > So we have a chicken-and-egg scenario. > > We can get around this by using the new "dsi_div_ab_desired" variable because > we don't care what the > current settings are, we only care what we want them to be. > > static unsigned long rzg2l_cpg_get_vclk_rate(struct clk_hw *hw, > unsigned long rate) > { > - struct sipll5 *sipll5 = to_sipll5(hw); > - struct rzg2l_cpg_priv *priv = sipll5->priv; > - unsigned long vclk; > - > - vclk = rate / ((1 << priv->mux_dsi_div_params.dsi_div_a) * > - (priv->mux_dsi_div_params.dsi_div_b + 1)); > - > - if (priv->mux_dsi_div_params.clksrc) > - vclk /= 2; > - > - return vclk; > > + return rate / dsi_div_ab_desired; > } > > > Since this function is only called one place in the driver, I suggest we get > rid of it and just do: > > vclk_rate = rate / dsi_div_ab_desired; > sipll5->foutpostdiv_rate = > rzg2l_cpg_get_foutpostdiv_rate(priv, ¶ms, vclk_rate); > > > Finally, how this all relates to your comment is that instead of the same > code block in 2 places, we > can just set the default desired divider and target in > rzg2l_cpg_sipll5_register() which is always > called early. > > /* Default settings for DPI */ > - priv->mux_dsi_div_params.clksrc = 0; > - 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_desired = 8; /* (1 << a) * (b + 1) */ > + rzg2l_cpg_dsi_div_set_divider(8, PLL5_TARGET_DPI); > > > I just did some testing with DPI and DSI, and so far everything works the > same. > > > What do you think???
OK for me. Thanks, Biju
