Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Thursday, October 12, 2023 9:35 PM
>
> Hi Shimoda-san,
>
> On Thu, Oct 12, 2023 at 2:16 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> Thanks for your patch!
Thank you for your review!
> > --- 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)
I don't know why unsigned int is needed. Other functions use
rswitch_for_each_enabled_port{_continue_reverse}() with int.
Especially, rswitch_for_each_enabled_port_continue_reverse()
has the following code, unsigned int will not work correctly:
---
#define rswitch_for_each_enabled_port_continue_reverse(priv, i) \
for (i--; i >= 0; i--) \
if (priv->rdev[i]->disabled) \
continue; \
else
---
So, I would like to keep this for consistency with other functions'
implementation. But, what do you think?
> > +
> > + 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? ...
Yes, it was possible if renesas_eth_sw_resume() called phy_init()
and it failed.
> > + }
> > +
> > + 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?
Yes, that is intentional. After this driver was probed, the phy was always
initialized.
> > + 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.
I got it. I'll fix them.
> > +
> > 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(...)
I'll use it on v2.
Best regards,
Yoshihiro Shimoda
> > .of_match_table = renesas_eth_sw_of_table,
> > }
> > };
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> 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
