RE: [PATCH 1/6] clk: renesas: rzg2l: Add DSI divider clock support for RZ/G3L

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <[email protected]>
> Sent: 10 July 2026 16:33
> Subject: Re: [PATCH 1/6] clk: renesas: rzg2l: Add DSI divider clock support for RZ/G3L
> 
> Hi Biju,
> 
> On Fri, 19 Jun 2026 at 18:40, Biju <[email protected]> wrote:
> > From: Biju Das <[email protected]>
> >
> > Add a new DSI divider clock type (CLK_TYPE_G3L_PLLDSI_DIV) for the
> > RZ/G3L SoC, which requires a different divider implementation than the
> > existing RZ/G2L DSI divider clock.
> >
> > The RZ/G3L DSI divider uses two cascaded dividers, DIV_DSI_A and
> > DIV_DSI_B, where the effective divider is:
> >
> >   rate = parent_rate / ((1 << div_a) * (div_b + 1))
> >
> > DIV_DSI_A is a power-of-two divider with values in the range [0, 5],
> > and DIV_DSI_B is a linear divider with values in the range [1, 16].
> >
> > Introduce the g3l_dsi_div_hw_data structure, rzg3l_cpg_dsi_div_ops,
> > and
> > rzg3l_cpg_dsi_div_clk_register() to implement the new clock type, and
> > add the DEF_G3L_PLLDSI_DIV() macro for use in clock table definitions.
> >
> > Signed-off-by: Biju Das <[email protected]>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
> > @@ -834,6 +837,134 @@ rzg2l_cpg_dsi_div_clk_register(const struct cpg_core_clk *core,
> >         return clk_hw->clk;
> >  }
> >
> > +struct g3l_dsi_div_hw_data {
> > +       struct clk_hw hw;
> > +       struct rzg2l_cpg_priv *priv;
> > +       unsigned long rate;
> > +       u32 off;
> > +       u8 div_a;
> > +       u8 div_b;
> > +};
> > +
> > +#define to_g3l_dsi_div_hw_data(_hw)    container_of(_hw, struct g3l_dsi_div_hw_data, hw)
> > +
> > +static unsigned long rzg3l_cpg_dsi_div_recalc_rate(struct clk_hw *hw,
> > +                                                  unsigned long
> > +parent_rate) {
> > +       struct g3l_dsi_div_hw_data *dsi_div = to_g3l_dsi_div_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +       int div_a, div_b, val;
> > +
> > +       val = readl(priv->base + dsi_div->off);
> > +       div_a = FIELD_GET(GENMASK(2, 0), val);
> > +       div_b = FIELD_GET(GENMASK(7, 4), val);
> 
> Please add and use
> 
>     #define DIV_DSI_A_SET    GENMASK(2, 0)
>     #define DIV_DSI_B_SET    GENMASK(7, 4)

OK.

> 
> (after having seen G3L_SDIV_DSI_C_SET, I kept on looking for  similar  A_SET and B_SET use).

The DSI divider values is proportional to (BPP * 2/ Number of lanes)

VCLK = PLLCLK / DSI_DIV -- (1)
VCLK * bpp <= HSCLK * 8 * num_lanes
VCLK = HSCLK * 8 * num_lanes / bpp -- (2)
PLLCLK / DSI_DIV = HSCLK * 8 * num_lanes / bpp

PLLCLK = HSCLK * 16 -- (3)

DSI_DIV = bpp * 16 / (8 * num_lanes) = bpp * 2 / num_lanes --(4)

Maximum DSI_DIV = 48 (bpp=24, num_lanes=1)

We need a custom divider clk to calculate the divider values

That is the reason for not having G3L_SDIV_DSI_{A,B}_SET

> 
> > +
> > +       return DIV_ROUND_CLOSEST_ULL((u64)parent_rate, (1 << div_a) *
> > + (div_b + 1));
> 
> You can simplify the divider to "(div_b + 1) << div_a".

OK.

> 
> > +}
> > +
> > +static int rzg3l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
> > +                                           struct clk_rate_request
> > +*req) {
> > +       struct g3l_dsi_div_hw_data *dsi_div = to_g3l_dsi_div_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +       u32 divider = dsi_div_ab_desired;
> 
> unsigned int

OK.

> 
> > +       bool divider_found = false;
> > +       unsigned int div_a, div_b;
> 
> Perhaps move the declarations of the loop counters inside the for()-statements?

OK.

> 
> > +
> > +       if (dsi_div_target) {
> > +               /* Calculate the DIV_DSI_A and DIV_DSI_B */
> 
> Please drop "the".

Agreed.

> 
> > +               for (div_a = 5; div_a >= 0 && !divider_found; div_a--)
> > + {
> 
> div_a is unsigned, so >= is always true

OK, will change the logic and will use div_a_tmp; 

for (div_a = 0; div_a <= 5 && !divider_found; div_a++)
	uisigned int div_a_tmp = 5 - div_a;

> 
> > +                       for (div_b = 0; div_b < 16; div_b++) {
> > +                               divider = (1 << div_a) * (div_b + 1);
> 
> (div_b + 1) << div_a

Agreed.

> 
> > +                               if (divider == dsi_div_ab_desired) {
> 
> divider is 1..512, while dsi_div_ab_desired is only u8.

The maximum value of dsi_div_ab_desired = 48, so u8 is sufficient.

> 
> > +                                       dsi_div->div_a = div_a;
> > +                                       dsi_div->div_b = div_b;
> > +                                       divider_found = true;
> > +                                       break;
> > +                               }
> > +                       }
> > +               }
> > +       } else {
> > +               dsi_div->div_b = 0;
> > +               /* Calculate the DIV_DSI_A */
> 
> Please drop "the"

OK.

> 
> > +               for (div_a = 5; div_a >= 0 && !divider_found; div_a--) {
> > +                       divider = (1 << div_a);
> > +                       if (divider == dsi_div_ab_desired) {
> > +                               dsi_div->div_a = div_a;
> > +                               divider_found = true;
> > +                               break;
> > +                       }
> > +               }
> > +       }
> > +
> > +       if (!divider_found) {
> > +               dev_err(priv->dev, "failed dsi div for: %u\n", divider);
> > +               return -EINVAL;
> > +       }
> > +
> > +       req->best_parent_rate = req->rate * divider;
> > +
> > +       return 0;
> > +}
> > +
> > +static int rzg3l_cpg_dsi_div_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                                     unsigned long parent_rate) {
> > +       struct g3l_dsi_div_hw_data *dsi_div = to_g3l_dsi_div_hw_data(hw);
> > +       struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +
> > +       writel(RZG3L_SDIV_DIV_DSI_A_WEN | RZG3L_SDIV_DIV_DSI_B_WEN |
> > +              (dsi_div->div_a << 0) | (dsi_div->div_b << 4),
> 
> FIELD_PREP(DIV_DSI_A_SET, dsi_div->div_a) | FIELD_PREP(DIV_DSI_B_SET, dsi_div->div_b)

Agreed.

Will fix the above in next version.

Cheers,
Biju