Re: [PATCH 4/4] clk: renesas: r9a09g077: Add LCDC and PLL3 clock support for RZ/T2H display pipeline

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Fri, Jun 5, 2026 at 2:39 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Mon, 11 May 2026 at 21:19, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add the clock definitions and PLL logic required to supply the LCDC
> > (VSPD/FCPVD/DU) blocks on the RZ/T2H (R9A09G077) SoC. The RZ/T2H display
> > subsystem depends on a dedicated PLL (PLL3) and a set of new derived
> > clocks.
> >
> > Introduce a new PLL clock type and implement rate recalculation,
> > programming and locking sequences for PLL3 using the RZ/T2H specific
> > divider and VCO limits. Add the corresponding muxes and divider entries,
> > expose the LCDC core clock, and register the LCDC module clock using the
> > correct PCLK parent.
> >
> > This enables the RZ/T2H clock driver to generate the display pipeline
> > clocking tree needed by the DU and VSP-based composition engines, allowing
> > upcoming display support to be integrated without duplicating CPG logic.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/Kconfig
> > +++ b/drivers/clk/renesas/Kconfig
> > @@ -218,10 +218,12 @@ config CLK_R9A09G057
> >  config CLK_R9A09G077
> >         bool "RZ/T2H clock support" if COMPILE_TEST
> >         select CLK_RENESAS_CPG_MSSR
> > +       select CLK_RZV2H
>
> That includes a lot. Perhaps spin off the required functionality in a
> separate file, like CLK_RCAR_CPG_LIB?
> That would impact the MODULE_IMPORT_NS("RZV2H_CPG") in
> drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c, too, though.
>
Ok I will create a rzv2h-cpg-lib.c and incremently switch the DSI
driver to use this new lib.

> >
> >  config CLK_R9A09G087
> >         bool "RZ/N2H clock support" if COMPILE_TEST
> >         select CLK_RENESAS_CPG_MSSR
> > +       select CLK_RZV2H
> >
> >  config CLK_SH73A0
> >         bool "SH-Mobile AG5 clock support" if COMPILE_TEST
> > diff --git a/drivers/clk/renesas/r9a09g077-cpg.c b/drivers/clk/renesas/r9a09g077-cpg.c
> > index f777601a23b9..48052e7b93fd 100644
> > --- a/drivers/clk/renesas/r9a09g077-cpg.c
> > +++ b/drivers/clk/renesas/r9a09g077-cpg.c
>
> > @@ -66,11 +73,26 @@
> >  #define DIVSCI2ASYNC   CONF_PACK(SCKCR3, 10, 2)
> >  #define DIVSCI3ASYNC   CONF_PACK(SCKCR3, 12, 2)
> >  #define DIVSCI4ASYNC   CONF_PACK(SCKCR3, 14, 2)
> > +#define LCDCDIVSEL     CONF_PACK(SCKCR3, 20, 4)
> > +
> > +#define PLL3EN         FIELD_PREP_CONST(OFFSET_MASK, (0xc0))
> > +
> > +#define CPG_PLLEN              BIT(0)
>
> CPG_PLL_EN_EN, for consistency with CPG_PLL_MON_LOCK below?
>
Ok.

> > +#define CPG_PLL3_VCO_CTR0(x)   ((x) + 0x4)
> > +#define CPG_PLL3_VCO_CTR0_PDIV GENMASK(21, 16)
> > +#define CPG_PLL3_VCO_CTR0_MDIV GENMASK(9, 0)
> > +#define CPG_PLL3_VCO_CTR1(x)   ((x) + 0x8)
> > +#define CPG_PLL3_VCO_CTR1_KDIV GENMASK(31, 16)
> > +#define CPG_PLL3_VCO_CTR1_SDIV GENMASK(2, 0)
> > +#define CPG_PLL_MON(x)         ((x) - 0x10)
> > +#define CPG_PLL_MON_LOCK       BIT(0)
>
> So all registers are calculated based on the CPG_PLL3EN register
> address...
>
Yes.

> >
> >  enum rzt2h_clk_types {
> >         CLK_TYPE_RZT2H_DIV = CLK_TYPE_CUSTOM,   /* Clock with divider */
> >         CLK_TYPE_RZT2H_MUX,                     /* Clock with clock source selector */
> >         CLK_TYPE_RZT2H_FSELXSPI,                /* Clock with FSELXSPIn source selector */
> > +       CLK_TYPE_RZT2H_PLL3,                    /* PLL3 Clock */
> > +       CLK_TYPE_RZT2H_LCDCDIV,                 /* LCDC divider clock */
> >  };
> >
> >  #define DEF_DIV(_name, _id, _parent, _conf, _dtable) \
> > @@ -83,10 +105,51 @@ enum rzt2h_clk_types {
> >  #define DEF_DIV_FSELXSPI(_name, _id, _parent, _conf, _dtable) \
> >         DEF_TYPE(_name, _id, CLK_TYPE_RZT2H_FSELXSPI, .conf = _conf, \
> >                  .parent = _parent, .dtable = _dtable, .flag = 0)
> > +#define DEF_PLL3(_name, _id, _parent, _conf) \
> > +       DEF_TYPE(_name, _id, CLK_TYPE_RZT2H_PLL3, .conf = _conf, \
> > +                .parent = _parent)
> > +#define DEF_DIV_LCDC(_name, _id, _parent, _conf, _dtable) \
> > +       DEF_TYPE(_name, _id, CLK_TYPE_RZT2H_LCDCDIV, .conf = _conf, \
> > +                .parent = _parent, .dtable = _dtable, .flag = CLK_SET_RATE_PARENT)
>
> I assume you can't reuse DEF_DIV() because you need the extra accuracy
> from interfacing with rzv2h_get_pll_divs_pars()?
>
Yep, thats correct.

> > +
> > +struct pll_clk {
> > +       void __iomem *reg;
>
> This is the PLLxEN register address, from which all other register
> addresses are calculated: pllen?
>
Yes.

> > +       const struct rzv2h_pll_limits *limits;
> > +       struct device *dev;
> > +       struct rzv2h_pll_pars pll_parameters;
> > +       struct clk_hw hw;
> > +       unsigned long cur_rate;
> > +};
> > +
> > +#define to_pll(_hw)    container_of(_hw, struct pll_clk, hw)
> > +
> > +struct r9a09g077_lcdc_div_clk {
> > +       const struct clk_div_table *dtable;
> > +       void __iomem *reg;
> > +       struct device *dev;
> > +       struct clk_hw hw;
> > +       u32 conf;
> > +       u8 divider;
> > +};
> > +
> > +#define to_lcdc_div_clk(_hw) \
> > +       container_of(_hw, struct r9a09g077_lcdc_div_clk, hw)
> > +
> > +#define RZT2H_MAX_LCDC_DIV_TABLES      16
> > +
> > +static const struct rzv2h_pll_limits r9a09g077_cpg_pll3_limits = {
> > +       .input_fref = 48 * MEGA,
> > +       .fout = { .min = 25 * MEGA, .max = 430 * MEGA },
> > +       .fvco = { .min = 1600 * MEGA, .max = 3200 * MEGA },
> > +       .m = { .min = 0x40, .max = 0x3ff },
> > +       .p = { .min = 0x2, .max = 0x8 },
> > +       .s = { .min = 0x0, .max = 0x6 },
> > +       .k = { .min = -32768, .max = 32767 },
> > +};
> >
> >  enum clk_ids {
> >         /* Core Clock Outputs exported to DT */
> > -       LAST_DT_CORE_CLK = R9A09G077_PCLKCAN,
> > +       LAST_DT_CORE_CLK = R9A09G077_LCDC_CLKD,
> >
> >         /* External Input Clocks */
> >         CLK_EXTAL,
>
> > @@ -242,6 +335,8 @@ static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
> >                          FSELXSPI1, dtable_6_8_16_32_64),
> >         DEF_MUX("PCLKCAN", R9A09G077_PCLKCAN, FSELCANFD,
> >                 sel_clk_pll4d3_div10_div20, ARRAY_SIZE(sel_clk_pll4d3_div10_div20), 0),
> > +       DEF_DIV_LCDC("LCDCDIV", R9A09G077_LCDC_CLKD, CLK_SEL_CLK_PLL3, LCDCDIVSEL,
>
> "LCDC_CLKD"
>
Ok.

>
> > +                    dtable_2_32),
> >  };
> >
> >  static const struct mssr_mod_clk r9a09g077_mod_clks[] __initconst = {
>
> > @@ -481,6 +577,272 @@ r9a09g077_cpg_fselxspi_div_clk_register(struct device *dev,
> >         return hw->clk;
> >  }
> >
> > +static unsigned long r9a09g077_cpg_pll3_clk_recalc_rate(struct clk_hw *hw,
> > +                                                       unsigned long parent_rate)
> > +{
> > +       struct pll_clk *pll_clk = to_pll(hw);
> > +       unsigned int ctr0, ctr1;
>
> u32
>
Ok.

Cheers,
Prabhakar

> > +       u8 pdiv, sdiv;
> > +       u64 rate;
> > +       u16 mdiv;
> > +       s16 kdiv;
> > +
> > +       ctr0 = readl(CPG_PLL3_VCO_CTR0(pll_clk->reg));
> > +       ctr1 = readl(CPG_PLL3_VCO_CTR1(pll_clk->reg));
> > +
> > +       pdiv = FIELD_GET(CPG_PLL3_VCO_CTR0_PDIV, ctr0);
> > +       mdiv = FIELD_GET(CPG_PLL3_VCO_CTR0_MDIV, ctr0);
> > +       kdiv = (s16)FIELD_GET(CPG_PLL3_VCO_CTR1_KDIV, ctr1);
> > +       sdiv = FIELD_GET(CPG_PLL3_VCO_CTR1_SDIV, ctr1);
> > +
> > +       rate = mul_u64_u32_shr(parent_rate, (mdiv << 16) + kdiv, 16 + sdiv);
> > +
> > +       return DIV_ROUND_CLOSEST_ULL(rate, pdiv);
> > +}
>
> The rest LGTM.
>
> 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