Re: [PATCH 08/13] net: ravb: Rely on PM domain to enable refclk

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Geert,

On 23.11.2023 10:48, Geert Uytterhoeven wrote:
> 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?

(I'm copy pasting here what I already replied to Sergey)

The reasons I've limited only to RZ/G3S are:
1/ I don't have all the platforms to test it
2/ on G1H this doesn't work. I tried to debugged it but I don't have a
   platform at hand, only remotely, and is hardly to debug once the
   ethernet fails to work: probe is working(), open is executed, PHY is
   initialized and then TX/RX is not working... don't know why ATM.

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

I agree with that. I chose this path to not interfere w/ the comments
ravb_runtime_nop() which I didn't understand. Also I fail to understand why
the ravb_runtime_nop() is there...

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