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

[Geert Uytterhoeven]

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)

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

> +
> +       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".

> +}
> +
> +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

> +       bool divider_found = false;
> +       unsigned int div_a, div_b;

Perhaps move the declarations of the loop counters inside the
for()-statements?

> +
> +       if (dsi_div_target) {
> +               /* Calculate the DIV_DSI_A and DIV_DSI_B */

Please drop "the".

> +               for (div_a = 5; div_a >= 0 && !divider_found; div_a--) {

div_a is unsigned, so >= is always true

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

(div_b + 1) << div_a

> +                               if (divider == dsi_div_ab_desired) {

divider is 1..512, while dsi_div_ab_desired is only u8.

> +                                       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"

> +               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)


> +              priv->base + dsi_div->off);
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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