Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <20231120084606.4083194-1-claudiu.beznea.uj@bp.renesas.com> <20231120084606.4083194-9-claudiu.beznea.uj@bp.renesas.com>
In-Reply-To: <20231120084606.4083194-9-claudiu.beznea.uj@bp.renesas.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Thu, 23 Nov 2023 09:48:22 +0100
Message-ID: <CAMuHMdV=2_h2PW9K7zT3Hwqjdk6D2m_Dd09bqHtifAvVTM7Lrw@mail.gmail.com>
Subject: Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk
To:     Claudiu <claudiu.beznea@tuxon.dev>
Cc:     s.shtylyov@omp.ru, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, p.zabel@pengutronix.de,
        yoshihiro.shimoda.uh@renesas.com, wsa+renesas@sang-engineering.com,
        biju.das.jz@bp.renesas.com,
        prabhakar.mahadev-lad.rj@bp.renesas.com,
        sergei.shtylyov@cogentembedded.com,
        mitsuhiro.kimura.kc@renesas.com, masaru.nagai.vx@renesas.com,
        netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Claudiu,

Thanks for your patch (which seems to have been delayed by 3 days, ouch)!

On Thu, Nov 23, 2023 at 5:35 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> For RZ/G3S and RZ/G2L SoCs the Ethernet's reference clock is part of the
> Ethernet's power domain. It is controlled though CPG driver that is
> providing the support for power domain that Ethernet belongs. Thus,
> to be able to implement runtime PM (at least for RZ/G3S at the moment)

Why only for RZ/G3S?

> w/o the need to add clock enable/disable specific calls in runtime PM
> ops of ravb driver and interfere with other IP specific implementations,
> add a new variable to struct_hw_info and enable the reference clock
> based on the value of this variable (the variable states if reference
> clock is part of the Ethernet's power domain).
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1043,6 +1043,7 @@ struct ravb_hw_info {
>         unsigned nc_queues:1;           /* AVB-DMAC has RX and TX NC queues */
>         unsigned magic_pkt:1;           /* E-MAC supports magic packet detection */
>         unsigned half_duplex:1;         /* E-MAC supports half duplex mode */
> +       unsigned refclk_in_pd:1;        /* Reference clock is part of a power domain. */
>  };
>
>  struct ravb_private {
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 836fdb4b3bfd..ddd8cd2c0f89 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2502,6 +2502,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>         .tx_counters = 1,
>         .carrier_counters = 1,
>         .half_duplex = 1,
> +       .refclk_in_pd = 1,
>  };
>
>  static const struct of_device_id ravb_match_table[] = {
> @@ -2749,12 +2750,14 @@ static int ravb_probe(struct platform_device *pdev)
>                 goto out_release;
>         }
>
> -       priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
> -       if (IS_ERR(priv->refclk)) {
> -               error = PTR_ERR(priv->refclk);
> -               goto out_release;
> +       if (!info->refclk_in_pd) {
> +               priv->refclk = devm_clk_get_optional(&pdev->dev, "refclk");
> +               if (IS_ERR(priv->refclk)) {
> +                       error = PTR_ERR(priv->refclk);
> +                       goto out_release;
> +               }
> +               clk_prepare_enable(priv->refclk);
>         }
> -       clk_prepare_enable(priv->refclk);

Is this patch really needed? It doesn't hurt to manually enable a
clock that is also under Runtime PM control.  Clock prepare/enable
refcounting will take care of that.

>
>         if (info->gptp_ref_clk) {
>                 priv->gptp_clk = devm_clk_get(&pdev->dev, "gptp");
> @@ -2869,7 +2872,8 @@ static int ravb_probe(struct platform_device *pdev)
>         if (info->ccc_gac)
>                 ravb_ptp_stop(ndev);
>  out_disable_refclk:
> -       clk_disable_unprepare(priv->refclk);
> +       if (!info->refclk_in_pd)
> +               clk_disable_unprepare(priv->refclk);
>  out_release:
>         free_netdev(ndev);
>  pm_runtime_put:
> @@ -2890,7 +2894,8 @@ static void ravb_remove(struct platform_device *pdev)
>         if (info->ccc_gac)
>                 ravb_ptp_stop(ndev);
>
> -       clk_disable_unprepare(priv->refclk);
> +       if (!info->refclk_in_pd)
> +               clk_disable_unprepare(priv->refclk);
>
>         /* Set reset mode */
>         ravb_write(ndev, CCC_OPC_RESET, CCC);

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