Re: [PATCH net-next] rswitch: Add PM ops

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Shimoda-san,

On Thu, Oct 12, 2023 at 2:16 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Add PM ops for Suspend to Idle. When the system suspended,
> the Ethernet Serdes's clock will be stopped. So, this driver needs
> to re-initialize the Ethernet Serdes by phy_init() in
> renesas_eth_sw_resume(). Otherwise, timeout happened in phy_power_on().
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_net.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/slab.h>
> @@ -1315,6 +1316,7 @@ static int rswitch_phy_device_init(struct rswitch_device *rdev)
>         if (!phydev)
>                 goto out;
>         __set_bit(rdev->etha->phy_interface, phydev->host_interfaces);
> +       phydev->mac_managed_pm = true;
>
>         phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
>                                 rdev->etha->phy_interface);
> @@ -1991,11 +1993,52 @@ static void renesas_eth_sw_remove(struct platform_device *pdev)
>         platform_set_drvdata(pdev, NULL);
>  }
>
> +static int __maybe_unused renesas_eth_sw_suspend(struct device *dev)
> +{
> +       struct rswitch_private *priv = dev_get_drvdata(dev);
> +       struct net_device *ndev;
> +       int i;

unsigned int (also below)

> +
> +       rswitch_for_each_enabled_port(priv, i) {
> +               ndev = priv->rdev[i]->ndev;
> +               if (netif_running(ndev)) {
> +                       netif_device_detach(ndev);
> +                       rswitch_stop(ndev);
> +               }
> +               if (priv->rdev[i]->serdes->init_count)
> +                       phy_exit(priv->rdev[i]->serdes);

If !init_count, the PHY was not initialized before suspending? ...

> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused renesas_eth_sw_resume(struct device *dev)
> +{
> +       struct rswitch_private *priv = dev_get_drvdata(dev);
> +       struct net_device *ndev;
> +       int i;
> +
> +       rswitch_for_each_enabled_port(priv, i) {
> +               phy_init(priv->rdev[i]->serdes);

... while it is always initialized after resuming? Is that intentional,
or should the pre-suspend state be preserved?

> +               ndev = priv->rdev[i]->ndev;
> +               if (netif_running(ndev)) {
> +                       rswitch_open(ndev);
> +                       netif_device_attach(ndev);
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(renesas_eth_sw_pm_ops, renesas_eth_sw_suspend,
> +                        renesas_eth_sw_resume);

Please use DEFINE_SIMPLE_DEV_PM_OPS() instead, so you can drop the
__maybe_unused tags from the callbacks.

> +
>  static struct platform_driver renesas_eth_sw_driver_platform = {
>         .probe = renesas_eth_sw_probe,
>         .remove_new = renesas_eth_sw_remove,
>         .driver = {
>                 .name = "renesas_eth_sw",
> +               .pm = &renesas_eth_sw_pm_ops,

pm_sleep_ptr(...)

>                 .of_match_table = renesas_eth_sw_of_table,
>         }
>  };

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