RE: [PATCH 1/6] clk: renesas: rzg2l: Add DSI divider clock support for RZ/G3L
From: Biju Das
Date: Mon Jul 27 2026 - 09:13:05 EST
Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> 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 <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > 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 <biju.das.jz@xxxxxxxxxxxxxx>
>
> 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