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

From: Biju Das

Date: Fri Apr 24 2026 - 03:52:31 EST

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 24 April 2026 08:09
> Subject: Re: [PATCH v5 4/4] clk: renesas: r9a08g046: Add clock and reset signals for the GBETH IPs
>
> Hi Biju,
>
> On Thu, 23 Apr 2026 at 13:14, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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.
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> i.e. will queue in renesas-clk for v7.2, with
> this fixed.

Thanks for taking care of this mistake.

>
> > > > + 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??
>
> No please. I'd rather keep it consistent with the other lines.

OK.

Cheers,
Biju