Re: [PATCH net-next 04/18] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: Re: [PATCH net-next 04/18] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
Date: Mon, 26 Jul 2021 12:11:40 +0200	[thread overview]
Message-ID: <CAMuHMdWj6jjHPhP9M=W8uwj3x-eOeDCtnRLR8t8x6XY6-H7CWg@mail.gmail.com> (raw)
In-Reply-To: <20210722141351.13668-5-biju.das.jz@bp.renesas.com>

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

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