[PATCH net-next V1 0/4] Add option to enable PHY WOL with PMT enabled

[PATCH net-next V1 0/4] Add option to enable PHY WOL with PMT enabled

[Ling Pei Lee]

This patchset main objective is to provide an option to enable PHY WoL even the
PMT is enabled by default in the HW features.

The current stmmac driver WOL implementation will enable MAC WOL if MAC HW PMT
feature is on. Else, the driver will check for PHY WOL support.
Intel EHL mgbe are designed to wake up through PHY WOL
although the HW PMT is enabled.Hence, introduced use_phy_wol platform
data to provide this PHY WOL option. Set use_phy_wol will disable the plat->pmt
which currently used to determine the system to wake up by MAC WOL or PHY WOL.

During testing, it is discovered that PHY did not reconfigured the PHY WOL
after waking up from S3/S4 through magic packet. During the driver resume flow,
the driver will reconfigure the PHY WOL depending on the ethool WOL settings.

This WOL patchset includes of setting the device power state to D3hot.
This is because the EHL PSE will need to PSE mgbe to be in D3 state in order
for the PSE to goes into suspend mode.

Ling Pei Lee (2):
  net: stmmac: option to enable PHY WOL with PMT enabled
  stmmac: intel: Enable PHY WOL option in EHL

Muhammad Husaini Zulkifli (1):
  net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

Voon Weifeng (1):
  stmmac: intel: set PCI_D3hot in suspend

 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c |  2 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 ++++++++++++-
 include/linux/stmmac.h                            |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH net-next V1 1/4] net: stmmac: option to enable PHY WOL with PMT enabled

[Ling Pei Lee]

The current stmmac driver WOL implementation will enable MAC WOL
if MAC HW PMT feature is on. Else, the driver will check for
PHY WOL support. There is another case where MAC HW PMT is
enabled but the platform still goes for the PHY WOL option.
E.g, Intel platform are designed for PHY WOL but not MAC WOL
although HW MAC PMT features are enabled.

Introduce use_phy_wol platform data to select PHY WOL
instead of depending on HW PMT features. Set use_phy_wol
will disable the plat->pmt which currently used to
determine the system to wake up by MAC WOL or PHY WOL.

Signed-off-by: Ling Pei Lee <pei.lee.ling@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 include/linux/stmmac.h                            | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0a266fa0af7e..a3b79ddcf08e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6533,7 +6533,8 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 		 * register (if supported).
 		 */
 		priv->plat->enh_desc = priv->dma_cap.enh_desc;
-		priv->plat->pmt = priv->dma_cap.pmt_remote_wake_up;
+		priv->plat->pmt = priv->dma_cap.pmt_remote_wake_up &&
+				!priv->plat->use_phy_wol;
 		priv->hw->pmt = priv->plat->pmt;
 		if (priv->dma_cap.hash_tb_sz) {
 			priv->hw->multicast_filter_bins =
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index e55a4807e3ea..9496e6c9ee82 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -263,5 +263,6 @@ struct plat_stmmacenet_data {
 	int msi_sfty_ue_vec;
 	int msi_rx_base_vec;
 	int msi_tx_base_vec;
+	bool use_phy_wol;
 };
 #endif
-- 
2.25.1

[PATCH net-next V1 2/4] stmmac: intel: Enable PHY WOL option in EHL

[Ling Pei Lee]

Enable PHY Wake On LAN in Intel EHL Intel platform.
PHY Wake on LAN option is enabled due to
Intel EHL Intel platform is designed for
PHY Wake On LAN but not MAC Wake On LAN.

Signed-off-by: Ling Pei Lee <pei.lee.ling@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 2ecf93c84b9d..73be34a10a4c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -567,6 +567,7 @@ static int ehl_common_data(struct pci_dev *pdev,
 	plat->rx_queues_to_use = 8;
 	plat->tx_queues_to_use = 8;
 	plat->clk_ptp_rate = 200000000;
+	plat->use_phy_wol = 1;
 
 	plat->safety_feat_cfg->tsoee = 1;
 	plat->safety_feat_cfg->mrxpee = 1;
-- 
2.25.1

[PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Ling Pei Lee]

From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

After PHY received a magic packet, the PHY WOL event will be
triggered then PHY WOL event interrupt will be disarmed.
Ethtool settings will remain with WOL enabled after a S3/S4
suspend resume cycle as expected. Hence,the driver should
reconfigure the PHY settings to reenable/disable WOL
depending on the ethtool WOL settings in the resume flow.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Co-developed-by: Ling Pei Lee <pei.lee.ling@intel.com>
Signed-off-by: Ling Pei Lee <pei.lee.ling@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a3b79ddcf08e..cd96e4d7a22e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7246,6 +7246,16 @@ int stmmac_resume(struct device *dev)
 		phylink_start(priv->phylink);
 		/* We may have called phylink_speed_down before */
 		phylink_speed_up(priv->phylink);
+		/* Reconfigure PHY WOL if the WOL is enabled in ethtool,
+		 * so that subsequent WOL still can be triggered.
+		 */
+		if (!priv->plat->pmt) {
+			struct ethtool_wolinfo phy_wol = { .cmd = ETHTOOL_GWOL };
+
+			phylink_ethtool_get_wol(priv->phylink, &phy_wol);
+			if (phy_wol.wolopts)
+				phylink_ethtool_set_wol(priv->phylink, &phy_wol);
+		}
 		rtnl_unlock();
 	}
 
-- 
2.25.1

Re: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Andrew Lunn]

On Mon, Jun 21, 2021 at 05:45:35PM +0800, Ling Pei Lee wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> After PHY received a magic packet, the PHY WOL event will be
> triggered then PHY WOL event interrupt will be disarmed.
> Ethtool settings will remain with WOL enabled after a S3/S4
> suspend resume cycle as expected. Hence,the driver should
> reconfigure the PHY settings to reenable/disable WOL
> depending on the ethtool WOL settings in the resume flow.

Please could you explain this a bit more? I'm wondering if you have a
PHY driver bug. PHY WOL should remain enabled until it is explicitly
disabled.

	Andrew

RE: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Voon, Weifeng]

> > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> >
> > After PHY received a magic packet, the PHY WOL event will be triggered
> > then PHY WOL event interrupt will be disarmed.
> > Ethtool settings will remain with WOL enabled after a S3/S4 suspend
> > resume cycle as expected. Hence,the driver should reconfigure the PHY
> > settings to reenable/disable WOL depending on the ethtool WOL settings
> > in the resume flow.
> 
> Please could you explain this a bit more? I'm wondering if you have a
> PHY driver bug. PHY WOL should remain enabled until it is explicitly
> disabled.
> 
> 	Andrew

Let's take Marvell 1510 as example. 

As explained in driver/net/phy/marvell.c
1773 >------->-------/* If WOL event happened once, the LED[2] interrupt pin
1774 >------->------- * will not be cleared unless we reading the interrupt status
1775 >------->------- * register. 

The WOL event will not able trigger again if the driver does not clear
the interrupt status.
Are we expecting PHY driver will automatically clears the interrupt
status rather than trigger from the MAC driver?

After scanning through all the PHY drivers, the drivers only touches 
the WOL settings in the get|set_wol() callbacks. Hence, I think that 
currently there are no PHY drivers that clear the WOL status.
Unless the PHY able to self-clear the WOL event status, the PHY WOL
would not able to remain enabled after resume from S3/S4.
Therefore, we implemented it in the MAC driver to reconfigure the PHY
WOL during the MAC resume() flow.      

Weifeng
 

Re: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Andrew Lunn]

On Wed, Jun 23, 2021 at 10:06:44AM +0000, Voon, Weifeng wrote:
> > > From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> > >
> > > After PHY received a magic packet, the PHY WOL event will be triggered
> > > then PHY WOL event interrupt will be disarmed.
> > > Ethtool settings will remain with WOL enabled after a S3/S4 suspend
> > > resume cycle as expected. Hence,the driver should reconfigure the PHY
> > > settings to reenable/disable WOL depending on the ethtool WOL settings
> > > in the resume flow.
> > 
> > Please could you explain this a bit more? I'm wondering if you have a
> > PHY driver bug. PHY WOL should remain enabled until it is explicitly
> > disabled.
> > 
> > 	Andrew
> 
> Let's take Marvell 1510 as example. 
> 
> As explained in driver/net/phy/marvell.c
> 1773 >------->-------/* If WOL event happened once, the LED[2] interrupt pin
> 1774 >------->------- * will not be cleared unless we reading the interrupt status
> 1775 >------->------- * register. 
> 
> The WOL event will not able trigger again if the driver does not clear
> the interrupt status.
> Are we expecting PHY driver will automatically clears the interrupt
> status rather than trigger from the MAC driver?

So you are saying the interrupt it getting discarded? I would of
though it is this interrupt which brings to system out of suspend, and
it should trigger the usual action, i.e. call the interrupt
handler. That should then clear the interrupt.

	 Andrew

RE: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Voon, Weifeng]

> > > > After PHY received a magic packet, the PHY WOL event will be
> > > > triggered then PHY WOL event interrupt will be disarmed.
> > > > Ethtool settings will remain with WOL enabled after a S3/S4
> > > > suspend resume cycle as expected. Hence,the driver should
> > > > reconfigure the PHY settings to reenable/disable WOL depending on
> > > > the ethtool WOL settings in the resume flow.
> > >
> > > Please could you explain this a bit more? I'm wondering if you have
> > > a PHY driver bug. PHY WOL should remain enabled until it is
> > > explicitly disabled.
> > >
> > > 	Andrew
> >
> > Let's take Marvell 1510 as example.
> >
> > As explained in driver/net/phy/marvell.c
> > 1773 >------->-------/* If WOL event happened once, the LED[2]
> > interrupt pin
> > 1774 >------->------- * will not be cleared unless we reading the
> > interrupt status
> > 1775 >------->------- * register.
> >
> > The WOL event will not able trigger again if the driver does not clear
> > the interrupt status.
> > Are we expecting PHY driver will automatically clears the interrupt
> > status rather than trigger from the MAC driver?
> 
> So you are saying the interrupt it getting discarded? I would of though it
> is this interrupt which brings to system out of suspend, and it should
> trigger the usual action, i.e. call the interrupt handler. That should then
> clear the interrupt.
> 
> 	 Andrew

No, the interrupt will not be discarded. If the PHY is in interrupt mode, the
interrupt handler will triggers and ISR will clear the WOL status bit. 
The condition here is when the PHY is in polling mode, the PHY driver does not
have any other mechanism to clear the WOL interrupt status bit.
Hence, we need to go through the PHY set_wol() again. 

Weifeng

Re: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Andrew Lunn]

> No, the interrupt will not be discarded. If the PHY is in interrupt mode, the
> interrupt handler will triggers and ISR will clear the WOL status bit. 
> The condition here is when the PHY is in polling mode, the PHY driver does not
> have any other mechanism to clear the WOL interrupt status bit.
> Hence, we need to go through the PHY set_wol() again. 

I would say you have a broken setup. If you are explicitly using the
interrupt as a wakeup source, you need to be servicing the
interrupt. You cannot use polled mode.

	   Andrew

RE: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Voon, Weifeng]

> > No, the interrupt will not be discarded. If the PHY is in interrupt
> > mode, the interrupt handler will triggers and ISR will clear the WOL
> status bit.
> > The condition here is when the PHY is in polling mode, the PHY driver
> > does not have any other mechanism to clear the WOL interrupt status bit.
> > Hence, we need to go through the PHY set_wol() again.
> 
> I would say you have a broken setup. If you are explicitly using the
> interrupt as a wakeup source, you need to be servicing the interrupt. You
> cannot use polled mode.
 
Sorry for the confusion. But I would like to clarify the I should use the
term of "WOL event status" rather than "WOL interrupt status". 
For interrupt mode, clearing the "WOL interrupt status" register will auto
clear the "WOL event status".
For polling mode, the phy driver can manually clear the "WOL event status" by
setting 1 to "Clear WOL Status" bit.  


I would like to rephase the commit message to make things clear:

After PHY received a magic packet, the PHY WOL event will be
triggered. At the same time, the "Magic Packet Match Detected" bit
is set. In order for the PHY WOL event to be triggered again, the 
WOL event status of "Magic Packet Match Detected" bit needs to be 
cleared. When the PHY is in polling mode, the WOL event status needs
to be manually cleared.

Ethtool settings will remain with WOL enabled after a S3/S4
suspend resume cycle as expected. Hence, the driver should
reconfigure the PHY settings to reenable/disable WOL
depending on the ethtool WOL settings in the MAC resume flow.
The PHY set_wol flow would clear the WOL event status.

Weifeng

Re: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Andrew Lunn]

> I would like to rephase the commit message to make things clear:
> 
> After PHY received a magic packet, the PHY WOL event will be
> triggered. At the same time, the "Magic Packet Match Detected" bit
> is set. In order for the PHY WOL event to be triggered again, the 
> WOL event status of "Magic Packet Match Detected" bit needs to be 
> cleared. When the PHY is in polling mode, the WOL event status needs
> to be manually cleared.
> 
> Ethtool settings will remain with WOL enabled after a S3/S4
> suspend resume cycle as expected. Hence, the driver should
> reconfigure the PHY settings to reenable/disable WOL
> depending on the ethtool WOL settings in the MAC resume flow.
> The PHY set_wol flow would clear the WOL event status.

I would still argue that making use of a WoL interrupts and PHY
polling is just wrong. But i assume you cannot fix this? You have a
hardware design error?

The problem with this solution is you need to modify every MAC driver
using the Marvell PHY. It does not scale.

Please try to find a solution within phylib or the marvell
driver. Something which will work for any broken setup which is using
WoL interrupts combined with polling.

    Andrew

RE: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Voon, Weifeng]

> > I would like to rephase the commit message to make things clear:
> >
> > After PHY received a magic packet, the PHY WOL event will be
> > triggered. At the same time, the "Magic Packet Match Detected" bit is
> > set. In order for the PHY WOL event to be triggered again, the WOL
> > event status of "Magic Packet Match Detected" bit needs to be cleared.
> > When the PHY is in polling mode, the WOL event status needs to be
> > manually cleared.
> >
> > Ethtool settings will remain with WOL enabled after a S3/S4 suspend
> > resume cycle as expected. Hence, the driver should reconfigure the PHY
> > settings to reenable/disable WOL depending on the ethtool WOL settings
> > in the MAC resume flow.
> > The PHY set_wol flow would clear the WOL event status.
> 
> I would still argue that making use of a WoL interrupts and PHY polling is
> just wrong. But i assume you cannot fix this? You have a hardware design
> error?
> 
> The problem with this solution is you need to modify every MAC driver using
> the Marvell PHY. It does not scale.
> 
> Please try to find a solution within phylib or the marvell driver.
> Something which will work for any broken setup which is using WoL
> interrupts combined with polling.

Yes, I would not able to fix this as the PHY WOL event signal pin is connected
directly to the PMC. And, I do not have the info why the HW is designed in
this way. 

But, I totally agreed that this solution is not scalable. We will drop this
patch from this patchset for v2. We will find another solution and most
probably in phylib as this behavior most likely will be similar across all
other PHYs.

Weifeng

Re: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Russell King (Oracle)]

On Fri, Jun 25, 2021 at 03:58:17PM +0000, Voon, Weifeng wrote:
> > > No, the interrupt will not be discarded. If the PHY is in interrupt
> > > mode, the interrupt handler will triggers and ISR will clear the WOL
> > status bit.
> > > The condition here is when the PHY is in polling mode, the PHY driver
> > > does not have any other mechanism to clear the WOL interrupt status bit.
> > > Hence, we need to go through the PHY set_wol() again.
> > 
> > I would say you have a broken setup. If you are explicitly using the
> > interrupt as a wakeup source, you need to be servicing the interrupt. You
> > cannot use polled mode.
>  
> Sorry for the confusion. But I would like to clarify the I should use the
> term of "WOL event status" rather than "WOL interrupt status". 
> For interrupt mode, clearing the "WOL interrupt status" register will auto
> clear the "WOL event status".
> For polling mode, the phy driver can manually clear the "WOL event status" by
> setting 1 to "Clear WOL Status" bit.  

If WOL raises an interrupt signal from the PHY, but the PHY interrupt
signal is not wired, how does the wakeup happen? What is the PHY
interrupt wired to?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next V1 3/4] net: stmmac: Reconfigure the PHY WOL settings in stmmac_resume()

[Voon, Weifeng]

> > > > No, the interrupt will not be discarded. If the PHY is in
> > > > interrupt mode, the interrupt handler will triggers and ISR will
> > > > clear the WOL
> > > status bit.
> > > > The condition here is when the PHY is in polling mode, the PHY
> > > > driver does not have any other mechanism to clear the WOL interrupt
> status bit.
> > > > Hence, we need to go through the PHY set_wol() again.
> > >
> > > I would say you have a broken setup. If you are explicitly using the
> > > interrupt as a wakeup source, you need to be servicing the
> > > interrupt. You cannot use polled mode.
> >
> > Sorry for the confusion. But I would like to clarify the I should use
> > the term of "WOL event status" rather than "WOL interrupt status".
> > For interrupt mode, clearing the "WOL interrupt status" register will
> > auto clear the "WOL event status".
> > For polling mode, the phy driver can manually clear the "WOL event
> > status" by setting 1 to "Clear WOL Status" bit.
> 
> If WOL raises an interrupt signal from the PHY, but the PHY interrupt
> signal is not wired, how does the wakeup happen? What is the PHY interrupt
> wired to?

The PHY WOL event signal is wired directly to the PMC. The PMC will detect 
the triggered WOL event signal and wakeup the system.

Weifeng

[PATCH net-next V1 4/4] stmmac: intel: set PCI_D3hot in suspend

[Ling Pei Lee]

From: Voon Weifeng <weifeng.voon@intel.com>

During suspend, set the Intel mgbe to D3hot state
to save power consumption.

Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>
Signed-off-by: Ling Pei Lee <pei.lee.ling@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 73be34a10a4c..69a725b661c2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -1088,6 +1088,7 @@ static int __maybe_unused intel_eth_pci_suspend(struct device *dev)
 		return ret;
 
 	pci_wake_from_d3(pdev, true);
+	pci_set_power_state(pdev, PCI_D3hot);
 	return 0;
 }
 
-- 
2.25.1