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

[Chris Brandt]

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, &params, 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, &params, 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???


Chris