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

[Geert Uytterhoeven]

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))'?

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