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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 10 July 2026 16:39
> Subject: Re: [PATCH 2/6] clk: renesas: rzg2l: Add PLL7 DSI 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 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?

OK.

> 
> >
> >  #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?

Not here. I believe 87MHz upper limit is to be taken care in LVDS bridge driver(mode_valid),
same case for DSI and DPI. 

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

Agreed.

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

Agreed.

> 
> > +               /* 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.

Agreed will use div_s64.

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

Ok.

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

Agreed.

> 
> > +               output += p->nfr;
> > +               output *= 1000;
> > +               nr_div_mr_pr = output / 4096;
> 
> Open-coded 64-by-32 division, please use div_u64() instead.
> Or >> 12.

Ok will use div_u64().
> 
> mul_u64_add_u64_div_u64 (multiply, add, and divide) and mul_u64_u32_shr (multiply and shift) might also
> be handy, somewhere...

If you agree, will do this optimization later.

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

OK. Will use div_u64().
> 
> > +               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?

OK. Will use div_u64().
> 
> > +
> > +               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.

As per Figure 4.4-6 Block Diagram of the Deformed Clock System (5),
Only PLL7 is variable one.

PLL registers (1, 4, 6, 7), PLL1-CA55, PLL4-DDR, PLL6- GBETH are fixed frequency PLL's and
I believe we need to use dividers for changing the rate for this PLL's. where as for PLL7, 
as the divider is fixed (eg: DSI: for a given bpp and num_lanes fixed. LVDS: 1/7 divider,
same case for DPI). So, we need to generate variable frequencies at run time using PLL7
registers.

Cheers,
Biju