RE: [PATCH 2/3] clk: renesas: rzg3s/rzg3l: Simplify PLL configuration macro

[Biju Das]

Hi Geert,

Thanks for the feedback

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 07 May 2026 14:06
> Subject: Re: [PATCH 2/3] clk: renesas: rzg3s/rzg3l: Simplify PLL configuration macro
> 
> Hi Biju,
> 
> On Mon, 4 May 2026 at 16:45, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Replace the per-SoC G3S_PLL146_CONF() and G3L_PLL1467_CONF() macros
> > with a unified CPG_PLL_CONF(stby, setting) macro defined in rzg2l-cpg.h.
> >
> > Drop the now-redundant GET_REG_SAMPLL_CLK1() and
> > GET_REG_SAMPLL_SETTING() macros, replacing the latter with
> > CPG_PLL1_SETTING_OFFSET() using
> > FIELD_GET() to extract the offset value. Update RZG3L_PLL_MON_OFFSET()
> > to use CPG_PLL_STBY_OFFSET() + 0xc directly.
> >
> > No functional changes.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a08g046-cpg.c
> > +++ b/drivers/clk/renesas/r9a08g046-cpg.c
> > @@ -56,9 +56,6 @@
> >  #define G3L_SEL_ETH1_CLK_TX_I  SEL_PLL_PACK(G3L_CPG_ETH_SSEL, 11, 1)
> > #define G3L_SEL_ETH1_CLK_RX_I  SEL_PLL_PACK(G3L_CPG_ETH_SSEL, 12, 1)
> >
> > -/* PLL 1/4/6/7 configuration registers macro. */ -#define
> > G3L_PLL1467_CONF(clk1, clk2, setting)  ((clk1) << 22 | (clk2) << 12 |
> > (setting))
> > -
> >  enum clk_ids {
> >         /* Core Clock Outputs exported to DT */
> >         LAST_DT_CORE_CLK = R9A08G046_USB_SCLK,
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -58,14 +58,15 @@
> >  #define RZG3S_DIV_NF           GENMASK(12, 1)
> >  #define RZG3S_SEL_PLL          BIT(0)
> >
> > +#define CPG_PLL1_SETTING_OFFSET(conf)  FIELD_GET(GENMASK(11, 0),
> > +(conf))
> >  #define CPG_PLL_STBY_OFFSET(conf)      FIELD_GET(GENMASK(23, 12), (conf))
> >  #define CPG_PLL_CLK1_OFFSET(x)         (CPG_PLL_STBY_OFFSET(x) + 0x4)
> >  #define CPG_PLL_CLK2_OFFSET(x)         (CPG_PLL_STBY_OFFSET(x) + 0x8)
> >
> > -#define RZG3L_PLL_STBY_OFFSET(x)       (GET_REG_SAMPLL_CLK1(x) - 0x4)
> > +#define RZG3L_PLL_STBY_OFFSET(x)       (CPG_PLL1_SETTING_OFFSET(x))
> 
> Shouldn't that be '(CPG_PLL_STBY_OFFSET(x))'?

Yes, it is a copy paste mistake.
Will fix this after my holidays(17/05).

Thanks,
Biju 

> 
> >  #define RZG3L_PLL_STBY_RESETB          BIT(0)
> >  #define RZG3L_PLL_STBY_RESETB_WEN      BIT(16)
> > -#define RZG3L_PLL_MON_OFFSET(x)                (GET_REG_SAMPLL_CLK1(x) + 0x8)
> > +#define RZG3L_PLL_MON_OFFSET(x)                (CPG_PLL_STBY_OFFSET(x) + 0xc)
> >  #define RZG3L_PLL_MON_RESETB           BIT(0)
> >  #define RZG3L_PLL_MON_LOCK             BIT(4)
> >
> > @@ -75,8 +76,6 @@
> >  #define CLK_MRST_R(reg)                (0x180 + (reg))
> >
> >  #define GET_REG_OFFSET(val)            ((val >> 20) & 0xfff)
> > -#define GET_REG_SAMPLL_CLK1(val)       ((val >> 22) & 0xfff)
> > -#define GET_REG_SAMPLL_SETTING(val)    ((val) & 0xfff)
> >
> >  #define CPG_WEN_BIT            BIT(16)
> >
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.h
> > +++ b/drivers/clk/renesas/rzg2l-cpg.h
> > @@ -59,6 +59,7 @@
> >  #define CPG_CLKSTATUS_SELSDHI1_STS     BIT(29)
> >
> >  #define CPG_SAM_PLL_CONF(stby)         ((stby) << 12)
> > +#define CPG_PLL_CONF(stby, setting)    ((stby) << 12 | (setting))
> >
> >  #define DDIV_PACK(offset, bitpos, size) \
> >                 (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
> 
> 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