Re: [PATCH 2/6] clk: renesas: rzg2l: Add PLL7 DSI 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 fractional PLL clock type (CLK_TYPE_G3L_PLLDSI) for the RZ/G3L
> SoC's PLL7, which drives the DSI interface and requires a dedicated
> parameter calculation and programming sequence distinct from other PLLs in
> the RZ/G2L family.
>
> PLL7 output frequency is determined by the formula:
>
>   Ffdco = (NIR + NFR / 4096) * (Fosc / MR)
>   Ffout = Ffdco / (1 << PR)
>
> where:
>   - Fosc = 24 MHz (oscillator input)
>   - PR in [0, 4]   (post divider, power-of-two)
>   - MR in [1, 12]  (input pre-divider)
>   - NIR in [56, 375], NFR in [0, 4095] (integer and fractional parts)
>
> The FDCO must fall within one of two valid ranges: 900–2000 MHz
> (rangesel=0) or 2000–3000 MHz (rangesel=1).
>
> The parameter search in rzg3l_dsi_get_pll_parameters_values() iterates
> over all valid (MR, PR) combinations, filtering by the required FPFD range
> of 8–16 MHz, then delegates to rzg3l_dsi_compute_pll_parameters() to find
> the NIR/NFR pair that best approximates the requested rate. An exact match
> returns immediately; otherwise the combination with the smallest absolute
> frequency error is used.
>
> Computed parameters are cached in pll7_dsi_params within pll_clk to
> avoid redundant recalculation in determine_rate() when the requested
> rate has not changed.
>
> 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
> @@ -67,6 +67,10 @@
>  #define CPG_PLL_MON_OFFSET(x)          (CPG_PLL_STBY_OFFSET(x) + 0xc)
>  #define CPG_PLL_MON_LOCK               BIT(4)
>  #define CPG_PLL_MON_RESETB             BIT(0)
> +#define CPG_PLL_CLK1_VAL(p, m, ni, nf, sel)    (FIELD_PREP(GENMASK(28, 26), p)  | \
> +                                                FIELD_PREP(GENMASK(25, 22), m)  | \
> +                                                FIELD_PREP(GENMASK(21, 13), ni) | \
> +                                                FIELD_PREP(GENMASK(12, 1), nf)  | (sel))

What about dropping CPG_PLL_CLK1_VAL(), defining

    #define CPG_PLL_CLK1_DIV_NF    GENMASK(12, 1)
    #define CPG_PLL_CLK1_DIV_NI    GENMASK(21, 13)
    ...

and using FIELD_PREP(CPG_PLL_CLK1_DIV_NF, ...) | ..." at all callsites?

>
>  #define RZG3L_SDIV_DIV_DSI_A_WEN       BIT(16)
>  #define RZG3L_SDIV_DIV_DSI_B_WEN       BIT(20)
> @@ -96,6 +100,26 @@
>  #define PLL5_HSCLK_MIN         10000000
>  #define PLL5_HSCLK_MAX         187500000
>
> +#define RZG3L_OSC_CLK                  (24 * MEGA)

This is the clock rate of the external crystal.
As this must be 24 MHz on RZ/G3L, I guess it is OK to use a define for that.

> +#define RZG3L_PLL7_FDCO_RANGE_0_MIN    (900 * MEGA)
> +#define RZG3L_PLL7_FDCO_RANGE_0_MAX    (2000 * MEGA)
> +#define RZG3L_PLL7_FDCO_RANGE_1_MIN    (2000 * MEGA)
> +#define RZG3L_PLL7_FDCO_RANGE_1_MAX    (3000ULL * MEGA)

I didn't check the ranges yet.
Do you need the 87 MHz upper limit of LVDS somewhere?

> +#define RZG3L_PLL7_PR_MIN              (0)
> +#define RZG3L_PLL7_PR_MAX              (4)
> +#define RZG3L_PLL7_MR_MIN              (0)
> +#define RZG3L_PLL7_MR_MAX              (11)
> +#define RZG3L_PLL7_NIR_MIN             (55)
> +#define RZG3L_PLL7_NIR_MAX             (374)
> +#define RZG3L_PLL7_NFR_MIN             (0)
> +#define RZG3L_PLL7_NFR_MAX             (4095)
> +#define RZG3L_PLL7_NR_MIN              (56250)  /* Multiplied value 56.25 * 1000 */
> +#define RZG3L_PLL7_NR_MAX              (375000) /* Multiplied value 375 * 1000 */
> +#define RZG3L_PLL7_MULT_MIN            (293)    /* Multiplied value 0.293 * 1000 */
> +#define RZG3L_PLL7_MULT_MAX            (375000) /* Multiplied value 375 * 1000 */
> +#define RZG3L_PLL7_FSTD_DIV_MR_MIN     (8 * MEGA)
> +#define RZG3L_PLL7_FSTD_DIV_MR_MAX     (16 * MEGA)
> +
>  /**
>   * struct clk_hw_data - clock hardware data
>   * @hw: clock hw

> @@ -1368,6 +1402,196 @@ static const struct clk_ops rzg3l_cpg_pll_ops = {
>         .recalc_rate = rzg3s_cpg_pll_clk_recalc_rate,
>  };
>
> +static inline bool
> +rzg3l_dsi_compute_pll_parameters(struct rzg3l_plldsi_parameters *pars,
> +                                struct rzg3l_plldsi_parameters *p,
> +                                struct rzg3l_plldsi_parameters *best,
> +                                u64 freq_millihz, u32 fpfd, u32 pr)
> +{
> +       for (p->nir = RZG3L_PLL7_NIR_MIN; p->nir <= RZG3L_PLL7_NIR_MAX; p->nir++) {
> +               u64 output_nir, output_nfr_range;
> +               s64 nfr, output_nfr;
> +               u64 fdco, output;
> +               u64 nr_div_mr_pr;
> +
> +               /*
> +                * The frequency generated by the PLL is calculated as follows:
> +                *
> +                * With:
> +                * Freq = Ffout = Ffdco / pr
> +                * input frequency(fstd) = 24MHz
> +                * fpfd = fstd / mr
> +                * nr = nir + nfr / 4096
> +                * Ffdco = nr * fpfd
> +                * Ffdco = (nir + (nfr / 4096)) * fpfd
> +                *
> +                * Freq can also be rewritten as:
> +                * Freq = Ffdco / pr
> +                *      = (nir * fpfd) / pr + ((nfr / 4096) * fpfd) / pr
> +                *      = output_nir + output_nfr
> +                *
> +                * Every parameter has been determined at this point, but nfr.
> +                * Considering that:
> +                * 0 <= nfr <= 4095
> +                * Then:
> +                * 0 <= (nfr / 4096) < 1
> +                * Therefore:
> +                * 0 <= output_nfr < fpfd / pr
> +                */
> +
> +               /* Compute output nir component (in mHz) */
> +               output_nir = DIV_ROUND_CLOSEST_ULL((p->nir + 1) * 1ULL * fpfd * MILLI, pr);

The multiplication with 1ULL to force 64-bit looks a bit odd.
As p->nir is small, perhaps mul_u32_u32((p->nir + 1) * MILLI, fpfd)?

> +               /* Compute range for output nfr (in mHz) */
> +               output_nfr_range = DIV_ROUND_CLOSEST_ULL(fpfd * 1ULL * MILLI, pr);

mul_u32_u32(fpfd, MILLI)

> +               /* No point in continuing if we can't achieve the desired frequency */
> +               if (freq_millihz < output_nir  || freq_millihz >= (output_nir + output_nfr_range))
> +                       continue;
> +
> +               /*
> +                * Compute the nfr component
> +                *
> +                * Since:
> +                * Freq = output_nir + output_nfr
> +                * Then:
> +                * output_nfr = Freq - output_nir
> +                *            = ((nfr / 4096) * fpfd) / pr
> +                * Therefore:
> +                * nfr = (output_nfr * 4096 * pr) / fpfd
> +                */
> +               output_nfr = freq_millihz - output_nir;
> +               nfr = div64_s64(output_nfr * 4096ULL * pr, fpfd);

No need for the ULL
fpfd is u32, so div_s64 is overkill, and div_s64 is sufficient.

> +               nfr = DIV_S64_ROUND_CLOSEST(nfr, 1000);
> +
> +               /* Validate nfr value within allowed limits */
> +               if (nfr < RZG3L_PLL7_NFR_MIN || nfr > RZG3L_PLL7_NFR_MAX)
> +                       continue;
> +
> +               p->nfr = nfr;
> +
> +               /* Compute (Ffdco * 4096) */
> +               fdco = (((p->nir + 1) * 4096ULL) + p->nfr) * fpfd;

mul_u32_u32(p->nir + 1, 4096)

> +               if (fdco < (RZG3L_PLL7_FDCO_RANGE_0_MIN * 4096ULL) ||
> +                   fdco > (RZG3L_PLL7_FDCO_RANGE_1_MAX * 4096ULL))
> +                       continue;
> +
> +               if (fdco <= (RZG3L_PLL7_FDCO_RANGE_0_MAX * 4096ULL))
> +                       p->rangesel = 0;
> +               else
> +                       p->rangesel = 1;
> +
> +               /* compute the nr and magnify by 1000 */
> +               output = mul_u32_u32((p->nir + 1), 4096);

Please move this up, and use it in the calculation of fdco above.

> +               output += p->nfr;
> +               output *= 1000;
> +               nr_div_mr_pr = output / 4096;

Open-coded 64-by-32 division, please use div_u64() instead.
Or >> 12.

mul_u64_add_u64_div_u64 (multiply, add, and divide) and
mul_u64_u32_shr (multiply and shift) might also be handy, somewhere...

> +               if (nr_div_mr_pr < RZG3L_PLL7_NR_MIN || nr_div_mr_pr > RZG3L_PLL7_NR_MAX)
> +                       continue;
> +
> +               /* compute the magnified multipier = nr(magnified)/(mr *pr)  */
> +               nr_div_mr_pr /= (p->mr + 1) * pr;

Open-coded 64-by-32 division, please use div_u64() instead.

> +               if (nr_div_mr_pr < RZG3L_PLL7_MULT_MIN || nr_div_mr_pr > RZG3L_PLL7_MULT_MAX)
> +                       continue;
> +
> +               output *= RZG3L_OSC_CLK;
> +               output /= (p->mr + 1) * pr * 4096;

Open-coded 64-by-32 division. Or perhaps even 64-by-64?

> +
> +               p->error_millihz = freq_millihz - output;
> +               p->freq_millihz = output;
> +
> +               /* If an exact match is found, return immediately */
> +               if (p->error_millihz == 0) {
> +                       *pars = *p;
> +                       return true;
> +               }
> +
> +               /* Update best match if error is smaller */
> +               if (abs(p->error_millihz) < abs(best->error_millihz))
> +                       *best = *p;
> +       }
> +
> +       return false;
> +}
> +

> +static const struct clk_ops rzg3l_cpg_plldsi_ops = {
> +       .recalc_rate = rzg3s_cpg_pll_clk_recalc_rate,
> +       .determine_rate = rzg3l_cpg_plldsi_determine_rate,
> +       .set_rate = rzg3l_cpg_plldsi_set_rate,
> +       .is_enabled = rzg3l_cpg_pll_clk_is_enabled,
> +       .enable = rzg3l_cpg_pll_clk_enable,
> +};

Basically this is rzg3l_cpg_pll_ops with .determine_rate() and
.set_rate() added.  Why can't you just add them to the existing
rzg3l_cpg_pll_ops, and use that for all PLLs? PLL7 doesn't seem that
different to me.

> +
>  static struct clk
>  *rzg2l_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
>                                void *data)

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