| Message ID | 20210722141351.13668-5-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
Hi Biju, On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Add ETH{0,1} clock/reset entries to CPG driver. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/r9a07g044-cpg.c > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > @@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = { > 0x578, 2), > DEF_MOD("usb_pclk", R9A07G044_USB_PCLK, R9A07G044_CLK_P1, > 0x578, 3), > + DEF_MOD("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0, > + 0x57c, 0), > + DEF_MOD("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT, > + 0x57c, 0), > + DEF_MOD("eth1_axi", R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0, > + 0x57c, 1), > + DEF_MOD("eth1_chi", R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT, > + 0x57c, 1), The AXI and CHI clocks use the same register bits, so this won't work as expected. E.g. when disabling one clock, the other clock will be disabled, too. The correct way to handle this is to create a new clock type for coupled clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and clears the bit only when both clocks are disabled. > DEF_MOD("i2c0", R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0, > 0x580, 0), > DEF_MOD("i2c1", R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0, > @@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = { > DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1), > DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2), > DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3), > + DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0), > + DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1), > DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0), > DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1), > DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2), This part is OK. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH net-next 04/18] drivers: clk: renesas: r9a07g044-cpg: > Add GbEthernet clock/reset > > Hi Biju, > > On Thu, Jul 22, 2021 at 4:14 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > Add ETH{0,1} clock/reset entries to CPG driver. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c > > @@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = > { > > 0x578, 2), > > DEF_MOD("usb_pclk", R9A07G044_USB_PCLK, R9A07G044_CLK_P1, > > 0x578, 3), > > + DEF_MOD("eth0_axi", R9A07G044_ETH0_CLK_AXI, > R9A07G044_CLK_M0, > > + 0x57c, 0), > > + DEF_MOD("eth0_chi", R9A07G044_ETH0_CLK_CHI, > R9A07G044_CLK_ZT, > > + 0x57c, 0), > > + DEF_MOD("eth1_axi", R9A07G044_ETH1_CLK_AXI, > R9A07G044_CLK_M0, > > + 0x57c, 1), > > + DEF_MOD("eth1_chi", R9A07G044_ETH1_CLK_CHI, > R9A07G044_CLK_ZT, > > + 0x57c, 1), > > The AXI and CHI clocks use the same register bits, so this won't work as > expected. E.g. when disabling one clock, the other clock will be disabled, > too. The correct way to handle this is to create a new clock type for > coupled clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit when at least > one clock is enabled, and clears the bit only when both clocks are > disabled. OK. Will create new clk type and add the logic to handle the same. Regards, Biju > > > DEF_MOD("i2c0", R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0, > > 0x580, 0), > > DEF_MOD("i2c1", R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0, > > @@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = { > > DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1), > > DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2), > > DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3), > > + DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0), > > + DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1), > > DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0), > > DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1), > > DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2), > > This part is OK. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > 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
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c index c78bea2f6ea8..8be09bcdf0f6 100644 --- a/drivers/clk/renesas/r9a07g044-cpg.c +++ b/drivers/clk/renesas/r9a07g044-cpg.c @@ -137,6 +137,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = { 0x578, 2), DEF_MOD("usb_pclk", R9A07G044_USB_PCLK, R9A07G044_CLK_P1, 0x578, 3), + DEF_MOD("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0, + 0x57c, 0), + DEF_MOD("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT, + 0x57c, 0), + DEF_MOD("eth1_axi", R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0, + 0x57c, 1), + DEF_MOD("eth1_chi", R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT, + 0x57c, 1), DEF_MOD("i2c0", R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0, 0x580, 0), DEF_MOD("i2c1", R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0, @@ -181,6 +189,8 @@ static struct rzg2l_reset r9a07g044_resets[] = { DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1), DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2), DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3), + DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0), + DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1), DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0), DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1), DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),
Add ETH{0,1} clock/reset entries to CPG driver. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/clk/renesas/r9a07g044-cpg.c | 10 ++++++++++ 1 file changed, 10 insertions(+)