RE: [PATCH v5 4/4] clk: renesas: r9a08g046: Add clock and reset signals for the GBETH IPs

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 23 April 2026 10:39
> Subject: Re: [PATCH v5 4/4] clk: renesas: r9a08g046: Add clock and reset signals for the GBETH IPs
> 
> Hi Biju,
> 
> On Thu, 26 Mar 2026 at 12:06, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Add clock and reset entries for the Gigabit Ethernet Interfaces (GBETH
> > 0-1) IPs found on the RZ/G3L SoC. This includes various dividers and
> > mux clocks needed by these two GBETH IPs. Also add tx, tx-180, rx,
> > rx-180, rmii, rmii-tx and rmii-rx clocks to r9a08g046_no_pm_mod_clk
> > table to avoid enabling both normal and rmii clocks by the PM framework.
> >
> > 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
> 
> > @@ -86,6 +140,17 @@ static const struct cpg_core_clk r9a08g046_core_clks[] __initconst = {
> >                     500000000UL),
> >         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> >         DEF_FIXED(".pll3_div2", CLK_PLL3_DIV2, CLK_PLL3, 1, 2),
> > +       DEF_FIXED(".pll6_div10", CLK_PLL6_DIV10, CLK_PLL6, 1, 10),
> > +       DEF_MUX(".sel_eth0_tx", CLK_SEL_ETH0_TX, G3L_SEL_ETH0_TX, sel_eth0_tx),
> > +       DEF_MUX(".sel_eth0_rx", CLK_SEL_ETH0_RX, G3L_SEL_ETH0_RX, sel_eth0_rx),
> > +       DEF_MUX(".sel_eth0_rm", CLK_SEL_ETH0_RM, G3L_SEL_ETH0_RM, sel_eth0_rm),
> > +       DEF_MUX(".sel_eth1_tx", CLK_SEL_ETH1_TX, G3L_SEL_ETH1_TX, sel_eth1_tx),
> > +       DEF_MUX(".sel_eth1_rx", CLK_SEL_ETH1_RX, G3L_SEL_ETH1_RX, sel_eth1_rx),
> > +       DEF_MUX(".sel_eth1_rm", CLK_SEL_ETH1_RM, G3L_SEL_ETH1_RM, sel_eth1_rm),
> > +       DEF_DIV(".div_eth0_tr", CLK_ETH0_TR, CLK_PLL6, G3L_SDIV_ETH_A, dtable_4_200),
> > +       DEF_DIV(".div_eth1_tr", CLK_ETH1_TR, CLK_PLL6, G3L_SDIV_ETH_C, dtable_4_200),
> > +       DEF_DIV(".div_eth0_rm", CLK_ETH0_RM, CLK_SEL_ETH0_RM, G3L_SDIV_ETH_B, dtable_2_20),
> > +       DEF_DIV(".div_eth1_rm", CLK_ETH1_RM, CLK_SEL_ETH1_RM,
> > + G3L_SDIV_ETH_D, dtable_2_20),
> >
> >         /* Core output clk */
> >         DEF_G3S_DIV("P0", R9A08G046_CLK_P0, CLK_PLL2_DIV2,
> > G3L_DIVPL2B, G3L_DIVPL2B_STS, @@ -94,6 +159,21 @@ static const struct cpg_core_clk
> r9a08g046_core_clks[] __initconst = {
> >                     dtable_4_128, 0, 0, 0, NULL),
> >         DEF_G3S_DIV("P3", R9A08G046_CLK_P3, CLK_PLL2_DIV2, G3L_DIVPL2A, G3L_DIVPL2A_STS,
> >                     dtable_4_128, 0, 0, 0, NULL),
> > +       DEF_FIXED("HP", R9A08G046_CLK_HP, CLK_PLL6_DIV10, 1, 1),
> > +       DEF_MUX_FLAGS("ETHTX01", R9A08G046_CLK_ETHTX01, G3L_SEL_ETH0_CLK_TX_I, sel_eth0_clk_tx_i,
> > +                     CLK_SET_RATE_PARENT),
> > +       DEF_MUX_FLAGS("ETHRX01", R9A08G046_CLK_ETHRX01, G3L_SEL_ETH0_CLK_RX_I, sel_eth0_clk_rx_i,
> > +                     CLK_SET_RATE_PARENT),
> > +       DEF_MUX_FLAGS("ETHTX11", R9A08G046_CLK_ETHTX11, G3L_SEL_ETH1_CLK_TX_I, sel_eth1_clk_tx_i,
> > +                     CLK_SET_RATE_PARENT),
> > +       DEF_MUX_FLAGS("ETHRX11", R9A08G046_CLK_ETHRX11, G3L_SEL_ETH1_CLK_RX_I, sel_eth1_clk_rx_i,
> > +                     CLK_SET_RATE_PARENT),
> > +       DEF_FIXED("ETHRM0", R9A08G046_CLK_ETHRM0, CLK_ETH0_RM, 1, 1),
> 
> Shouldn't the parent be CLK_SEL_ETH0_RM (i.e. before the 1/2 or 1/20 divider)?

Oops, I missed this. You are correct.

> 
> > +       DEF_FIXED("ETHTX02", R9A08G046_CLK_ETHTX02, CLK_SEL_ETH0_TX, 1, 1),
> > +       DEF_FIXED("ETHRX02", R9A08G046_CLK_ETHRX02, CLK_SEL_ETH0_RX, 1, 1),
> > +       DEF_FIXED("ETHRM1", R9A08G046_CLK_ETHRM1, CLK_ETH1_RM, 1, 1),
> 
> Likewise, CLK_SEL_ETH1_RM?
> 
> If you agree, I can fix this up while applying.

I Agree.

> 
> > +       DEF_FIXED("ETHTX12", R9A08G046_CLK_ETHTX12, CLK_SEL_ETH1_TX, 1, 1),
> > +       DEF_FIXED("ETHRX12", R9A08G046_CLK_ETHRX12, CLK_SEL_ETH1_RX,
> > + 1, 1),
> >  };
> >
> >  static const struct rzg2l_mod_clk r9a08g046_mod_clks[] = { @@ -107,6
> > +187,50 @@ static const struct rzg2l_mod_clk r9a08g046_mod_clks[] = {
> >                                         MSTOP(BUS_REG1, BIT(2))),
> >         DEF_MOD("dmac_pclk",            R9A08G046_DMAC_PCLK, R9A08G046_CLK_P3, 0x52c, 1,
> >                                         MSTOP(BUS_REG1, BIT(3))),
> > +       DEF_MOD("eth0_clk_axi",         R9A08G046_ETH0_CLK_AXI, R9A08G046_CLK_P1, 0x57c, 0,
> > +                                       MSTOP(BUS_PERI_COM, BIT(2))),
> > +       DEF_MOD("eth1_clk_axi",         R9A08G046_ETH1_CLK_AXI, R9A08G046_CLK_P1, 0x57c, 1,
> > +                                       MSTOP(BUS_PERI_COM, BIT(3))),
> > +       DEF_MOD("eth0_clk_chi",         R9A08G046_ETH0_CLK_CHI, R9A08G046_CLK_P1, 0x57c, 2,
> > +                                       MSTOP(BUS_PERI_COM, BIT(2))),
> > +       DEF_MOD("eth1_clk_chi",         R9A08G046_ETH1_CLK_CHI, R9A08G046_CLK_P1, 0x57c, 3,
> > +                                       MSTOP(BUS_PERI_COM, BIT(3))),
> > +       DEF_COUPLED("eth0_tx_i",        R9A08G046_ETH0_CLK_TX_I, R9A08G046_CLK_ETHTX01, 0x57c, 4,
> > +                                       MSTOP(BUS_PERI_COM, BIT(2))),
> > +       DEF_COUPLED("eth0_tx_180_i", R9A08G046_ETH0_CLK_TX_180_I, R9A08G046_CLK_ETHTX02, 0x57c, 4,
> > +                                       MSTOP(BUS_PERI_COM, BIT(2))),
> > +       DEF_COUPLED("eth1_tx_i",        R9A08G046_ETH1_CLK_TX_I, R9A08G046_CLK_ETHTX11, 0x57c, 5,
> > +                                       MSTOP(BUS_PERI_COM, BIT(3))),
> > +       DEF_COUPLED("eth1_tx_180_i", R9A08G046_ETH1_CLK_TX_180_I,
> > + R9A08G046_CLK_ETHTX12, 0x57c, 5,
> 
> Inconsistent alignment (more below).

I tried to squeeze it into 100 columns. Maybe I should have split this into 3 lines??

Cheers,
Biju