<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN">
<HTML>
<HEAD>
  <META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8">
  <META NAME="GENERATOR" CONTENT="GtkHTML/4.6.6">
</HEAD>
<BODY>
Appreciate your comments!<BR>
<BR>
On Thu, 2019-10-10 at 16:01 -0700, Jakub Kicinski wrote:
<BLOCKQUOTE TYPE=CITE>
<PRE>
On Wed, 9 Oct 2019 16:56:49 +0800, Biao Huang wrote:
<FONT COLOR="#737373">> disable ptp_ref_clk in suspend flow, and enable it in resume flow.</FONT>
<FONT COLOR="#737373">> </FONT>
<FONT COLOR="#737373">> Signed-off-by: Biao Huang <<A HREF="mailto:biao.huang@mediatek.com">biao.huang@mediatek.com</A>></FONT>
<FONT COLOR="#737373">> ---</FONT>
<FONT COLOR="#737373">>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++</FONT>
<FONT COLOR="#737373">>  1 file changed, 4 insertions(+)</FONT>
<FONT COLOR="#737373">> </FONT>
<FONT COLOR="#737373">> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c</FONT>
<FONT COLOR="#737373">> index c7c9e5f162e6..b592aeecc3dd 100644</FONT>
<FONT COLOR="#737373">> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c</FONT>
<FONT COLOR="#737373">> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c</FONT>
<FONT COLOR="#737373">> @@ -4469,6 +4469,8 @@ int stmmac_suspend(struct device *dev)</FONT>
<FONT COLOR="#737373">>              stmmac_mac_set(priv, priv->ioaddr, false);</FONT>
<FONT COLOR="#737373">>              pinctrl_pm_select_sleep_state(priv->device);</FONT>
<FONT COLOR="#737373">>              /* Disable clock in case of PWM is off */</FONT>
<FONT COLOR="#737373">> +            if (priv->plat->clk_ptp_ref)</FONT>
<FONT COLOR="#737373">> +                    clk_disable_unprepare(priv->plat->clk_ptp_ref);</FONT>

I don't know much embedded, but it seems like this should perhaps just
be clk_disable() without the unprepare? stmmac_hw_teardown() is called
when driver is removed so it needs to unprepare as well.

Please feel free to explain to me why this needs to be
clk_disable_unprepare(), as I said - not an expert.

</PRE>
</BLOCKQUOTE>
As our clock owner's advice, there are prepare/unprepare clk_ops for pll, no enable/disable clk_ops for it, and pll will be off only when the prepare reference count decrease to 0.<BR>
<BR>
so for the sake of power saving, we'd better call clk_disable_unprepare to turn the parent pll off.<BR>
<BR>
<BR>
<BLOCKQUOTE TYPE=CITE>
<PRE>
Also - if this is a bug fix and you'd like to have it backported to
older releases you need to add a Fixes tag.
</PRE>
</BLOCKQUOTE>
Thanks for the reminder, I'll add the fixes tag in the next version.
<BLOCKQUOTE TYPE=CITE>
<PRE>

Thanks!

<FONT COLOR="#737373">>                 clk_disable(priv->plat->pclk)</FONT>;
</PRE>
</BLOCKQUOTE>
maybe it should be clk_disable_unprepare(priv->plat->pclk); here.
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#737373">>              clk_disable(priv->plat->stmmac_clk);</FONT>
<FONT COLOR="#737373">>      }</FONT>
<FONT COLOR="#737373">> @@ -4535,6 +4537,8 @@ int stmmac_resume(struct device *dev)</FONT>
<FONT COLOR="#737373">>              /* enable the clk previously disabled */</FONT>
<FONT COLOR="#737373">>              clk_enable(priv->plat->stmmac_clk);</FONT>
<FONT COLOR="#737373">>              clk_enable(priv->plat->pclk);</FONT>
<FONT COLOR="#737373">> +            if (priv->plat->clk_ptp_ref)</FONT>
<FONT COLOR="#737373">> +                    clk_prepare_enable(priv->plat->clk_ptp_ref);</FONT>
<FONT COLOR="#737373">>              /* reset the phy so that it's ready */</FONT>
<FONT COLOR="#737373">>              if (priv->mii)</FONT>
<FONT COLOR="#737373">>                      stmmac_mdio_reset(priv->mii);</FONT>

</PRE>
</BLOCKQUOTE>
<BR>
</BODY>
</HTML>