Re: [PATCH v6 1/4] clk: renesas: rzv2h-cpg: Add support for DSI clocks

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Biju,

On Thu, Jun 19, 2025 at 6:05 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Biju Das
> > Sent: 18 June 2025 14:26
> > Subject: RE: [PATCH v6 1/4] clk: renesas: rzv2h-cpg: Add support for DSI clocks
> >
> > Hi Prabhakar,
> >
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > > Lad, Prabhakar
> > > Sent: 16 June 2025 11:45
> > > Subject: Re: [PATCH v6 1/4] clk: renesas: rzv2h-cpg: Add support for
> > > DSI clocks
> > >
> > > Hi Biju,
> > >
> > > Thank you for the review.
> > >
> > > On Fri, Jun 13, 2025 at 6:57 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > > -----Original Message-----
> > > > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > > > Sent: 30 May 2025 18:19
> > > > .castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> > > > > lad.rj@bp.renesas.com>
> > > > > Subject: [PATCH v6 1/4] clk: renesas: rzv2h-cpg: Add support for
> > > > > DSI clocks
> > > > >
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Add support for PLLDSI and PLLDSI divider clocks.
> > > > >
> > > > > Introduce the `renesas-rzv2h-dsi.h` header to centralize and share
> > > > > PLLDSI-related data structures, limits, and algorithms between the RZ/V2H CPG and DSI drivers.
> > > > >
> > > > > The DSI PLL is functionally similar to the CPG's PLLDSI, but has
> > > > > slightly different parameter limits and omits the programmable
> > > > > divider present in CPG. To ensure precise frequency
> > > > > calculations-especially for milliHz-level accuracy needed by the
> > > > > DSI driver-the shared algorithm
> > > allows both drivers to compute PLL parameters consistently using the same logic and input clock.
> > > > >
> > > > > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > Signed-off-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > > v5->v6:
> > > > > - Renamed CPG_PLL_STBY_SSCGEN_WEN to CPG_PLL_STBY_SSC_EN_WEN
> > > > > - Updated CPG_PLL_CLK1_DIV_K, CPG_PLL_CLK1_DIV_M, and
> > > > >   CPG_PLL_CLK1_DIV_P macros to use GENMASK
> > > > > - Updated req->rate in rzv2h_cpg_plldsi_div_determine_rate()
> > > > > - Dropped the cast in rzv2h_cpg_plldsi_div_set_rate()
> > > > > - Dropped rzv2h_cpg_plldsi_round_rate() and implemented
> > > > >   rzv2h_cpg_plldsi_determine_rate() instead
> > > > > - Made use of FIELD_PREP()
> > > > > - Moved CPG_CSDIV1 macro in patch 2/4
> > > > > - Dropped two_pow_s in rzv2h_dsi_get_pll_parameters_values()
> > > > > - Used mul_u32_u32() while calculating output_m and output_k_range
> > > > > - Used div_s64() instead of div64_s64() while calculating
> > > > >   pll_k
> > > > > - Used mul_u32_u32() while calculating fvco and fvco checks
> > > > > - Rounded the final output using DIV_U64_ROUND_CLOSEST()
> > > > >
> > > > > v4->v5:
> > > > > - No changes
> > > > >
> > > > > v3->v4:
> > > > > - Corrected parameter name in rzv2h_dsi_get_pll_parameters_values()
> > > > >   description freq_millihz
> > > > >
> > > > > v2->v3:
> > > > > - Update the commit message to clarify the purpose of `renesas-rzv2h-dsi.h`
> > > > >   header
> > > > > - Used mul_u32_u32() in rzv2h_cpg_plldsi_div_determine_rate()
> > > > > - Replaced *_mhz to *_millihz for clarity
> > > > > - Updated u64->u32 for fvco limits
> > > > > - Initialized the members in declaration order for
> > > > >   RZV2H_CPG_PLL_DSI_LIMITS() macro
> > > > > - Used clk_div_mask() in rzv2h_cpg_plldsi_div_recalc_rate()
> > > > > - Replaced `unsigned long long` with u64
> > > > > - Dropped rzv2h_cpg_plldsi_clk_recalc_rate() and reused
> > > > >   rzv2h_cpg_pll_clk_recalc_rate() instead
> > > > > - In rzv2h_cpg_plldsi_div_set_rate() followed the same style
> > > > >   of RMW-operation as done in the other functions
> > > > > - Renamed rzv2h_cpg_plldsi_set_rate() to rzv2h_cpg_pll_set_rate()
> > > > > - Dropped rzv2h_cpg_plldsi_clk_register() and reused
> > > > >   rzv2h_cpg_pll_clk_register() instead
> > > > > - Added a gaurd in renesas-rzv2h-dsi.h header
> > > > >
> > > > > v1->v2:
> > > > > - No changes
> > > > > ---
> > > > >  drivers/clk/renesas/rzv2h-cpg.c       | 278 +++++++++++++++++++++++++-
> > > > >  drivers/clk/renesas/rzv2h-cpg.h       |  13 ++
> > > > >  include/linux/clk/renesas-rzv2h-dsi.h | 210 +++++++++++++++++++
> > > > >  3 files changed, 492 insertions(+), 9 deletions(-)  create mode
> > > > > 100644 include/linux/clk/renesas- rzv2h-dsi.h
> > > > >
> > > > > diff --git a/drivers/clk/renesas/rzv2h-cpg.c
> > > > > b/drivers/clk/renesas/rzv2h-cpg.c index
> > > > > 761da3bf77ce..d590f9f47371 100644
> > > > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > > > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > > > > @@ -14,9 +14,13 @@
> > > > >  #include <linux/bitfield.h>
> > > > >  #include <linux/clk.h>
> > > > >  #include <linux/clk-provider.h>
> > > > > +#include <linux/clk/renesas-rzv2h-dsi.h>
> > > > >  #include <linux/delay.h>
> > > > >  #include <linux/init.h>
> > > > >  #include <linux/iopoll.h>
> > > > > +#include <linux/math.h>
> > > >
> > > >
> > > >
> > > > > +     req->rate =
> > > > > + DIV_ROUND_CLOSEST_ULL(dsi_dividers->freq_millihz,
> > > > > + MILLI);
> > > > > +
> > > > > +     return 0;
> > > > > +};
> > > > > +
> > > > > +static int rzv2h_cpg_plldsi_div_set_rate(struct clk_hw *hw,
> > > > > +                                      unsigned long rate,
> > > > > +                                      unsigned long parent_rate) {
> > > > > +     struct rzv2h_plldsi_div_clk *dsi_div = to_plldsi_div_clk(hw);
> > > > > +     struct rzv2h_cpg_priv *priv = dsi_div->priv;
> > > > > +     struct rzv2h_plldsi_parameters *dsi_dividers = &priv->plldsi_div_parameters;
> > > > > +     struct ddiv ddiv = dsi_div->ddiv;
> > > > > +     const struct clk_div_table *clkt;
> > > > > +     bool div_found = false;
> > > > > +     u32 val, shift, div;
> > > > > +
> > > > > +     div = dsi_dividers->csdiv;
> > > > > +     for (clkt = dsi_div->dtable; clkt->div; clkt++) {
> > > > > +             if (clkt->div == div) {
> > > > > +                     div_found = true;
> > > > > +                     break;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (!div_found)
> > > > > +             return -EINVAL;
> > > >
> > > > This check can be done in determine rate and cache the divider??
> > > >
> > > Ok, I'll drop this check as the divider is already cached. The for
> > > loop above is to determine the val which is used below to program the registers.
> >
> > If you are caching actual divider value, then the check is not required here.
> > Otherwise the above code is fine.
> >
> > Assume the csdiv you found, have no corresponding match in the table.
>
>
> 1) By looking at RZ/G3E, can we make this code more scalable?
>
> RZ/G3E has 2 PLL-DSI's
> PLL-DSI1 supports DUAL LVDS, Single LVDS and DSI
> PLL-DSI2 supports single LVDS,  DPI and DSI
>
Sure, I'll make it more scalable.

> In total there will be 4 limit tables (DSI, single LVDS, Dual LVDS and DPI)
>
> Based on the display output, each PLL needs to pick the appropriate limit table
> to compute PLL parameters.
>
> 2) Can we drop DSI divider limits from the limit table and use the values from dsi divider table
>    itself which is passed in DEF_PLLDSI_DIV?
>
Sure, I will do that.

Cheers,
Prabhakar