[PATCH net V2 0/2] net: lan743x: Fixes for multiple WOL related issues

[PATCH net V2 0/2] net: lan743x: Fixes for multiple WOL related issues

[Raju Lakkaraju]

This patch series implement the following fixes:
1. Disable WOL upon resume in order to restore full data path operation
2. Support WOL in MAC even when PHY does not

Change List:
------------
V1 -> V2:
  - Repost without patch V1 3/3 - net: lan743x: Address problems with wake
    option flags configuration sequences
V0 -> V1:
  - Include the missing maintainer's email ids in 'CC list
  - Remove the patch "Address problems with wake option flags configuration
    sequences" from this patch series. Will fix this in next patch series.
  - Variable "data" change from "int" to "unsigned int"
  - Change the "phy does not support WOL" print from netif_info() to
    netif_dbg()

Raju Lakkaraju (2):
  net: lan743x: disable WOL upon resume to restore full data path
    operation
  net: lan743x: support WOL in MAC even when PHY does not

 .../net/ethernet/microchip/lan743x_ethtool.c  | 14 +++++++++--
 drivers/net/ethernet/microchip/lan743x_main.c | 24 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h | 24 +++++++++++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH net V2 1/2] net: lan743x: disable WOL upon resume to restore full data path operation

[Raju Lakkaraju]

In order for datapath to be restored to normal functionality after resume
we disable all wakeup events. Additionally we clear all W1C status bits by
writing 1's to them.

Fixes: 4d94282afd95 ("lan743x: Add power management support")
Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
------------
V1 -> V2:
  - Repost - No change
V0 -> V1:
  - Variable "data" change from "int" to "unsigned int"

 drivers/net/ethernet/microchip/lan743x_main.c | 24 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h | 24 +++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index bd8aa83b47e5..385e9dcd8cd9 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -3550,7 +3550,7 @@ static void lan743x_pm_set_wol(struct lan743x_adapter *adapter)
 
 	/* clear wake settings */
 	pmtctl = lan743x_csr_read(adapter, PMT_CTL);
-	pmtctl |= PMT_CTL_WUPS_MASK_;
+	pmtctl |= PMT_CTL_WUPS_MASK_ | PMT_CTL_RES_CLR_WKP_MASK_;
 	pmtctl &= ~(PMT_CTL_GPIO_WAKEUP_EN_ | PMT_CTL_EEE_WAKEUP_EN_ |
 		PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_ |
 		PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_ | PMT_CTL_ETH_PHY_WAKE_EN_);
@@ -3685,6 +3685,7 @@ static int lan743x_pm_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	u32 data;
 	int ret;
 
 	pci_set_power_state(pdev, PCI_D0);
@@ -3715,6 +3716,27 @@ static int lan743x_pm_resume(struct device *dev)
 	netif_info(adapter, drv, adapter->netdev,
 		   "Wakeup source : 0x%08X\n", ret);
 
+	/* Clear the wol configuration and status bits when system
+	 * events occurs.
+	 * The status bits are "Write One to Clear (W1C)"
+	 */
+	data = MAC_WUCSR_EEE_TX_WAKE_ | MAC_WUCSR_EEE_RX_WAKE_ |
+	       MAC_WUCSR_RFE_WAKE_FR_ | MAC_WUCSR_PFDA_FR_ | MAC_WUCSR_WUFR_ |
+	       MAC_WUCSR_MPR_ | MAC_WUCSR_BCAST_FR_;
+	lan743x_csr_write(adapter, MAC_WUCSR, data);
+
+	data = MAC_WUCSR2_NS_RCD_ | MAC_WUCSR2_ARP_RCD_ |
+	       MAC_WUCSR2_IPV6_TCPSYN_RCD_ | MAC_WUCSR2_IPV4_TCPSYN_RCD_;
+	lan743x_csr_write(adapter, MAC_WUCSR2, data);
+
+	data = MAC_WK_SRC_ETH_PHY_WK_ | MAC_WK_SRC_IPV6_TCPSYN_RCD_WK_ |
+	       MAC_WK_SRC_IPV4_TCPSYN_RCD_WK_ | MAC_WK_SRC_EEE_TX_WK_ |
+	       MAC_WK_SRC_EEE_RX_WK_ | MAC_WK_SRC_RFE_FR_WK_ |
+	       MAC_WK_SRC_PFDA_FR_WK_ | MAC_WK_SRC_MP_FR_WK_ |
+	       MAC_WK_SRC_BCAST_FR_WK_ | MAC_WK_SRC_WU_FR_WK_ |
+	       MAC_WK_SRC_WK_FR_SAVED_;
+	lan743x_csr_write(adapter, MAC_WK_SRC, data);
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index be79cb0ae5af..77fc3abc1428 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -60,6 +60,7 @@
 #define PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_		BIT(18)
 #define PMT_CTL_GPIO_WAKEUP_EN_			BIT(15)
 #define PMT_CTL_EEE_WAKEUP_EN_			BIT(13)
+#define PMT_CTL_RES_CLR_WKP_MASK_		GENMASK(9, 8)
 #define PMT_CTL_READY_				BIT(7)
 #define PMT_CTL_ETH_PHY_RST_			BIT(4)
 #define PMT_CTL_WOL_EN_				BIT(3)
@@ -226,12 +227,31 @@
 #define MAC_WUCSR				(0x140)
 #define MAC_MP_SO_EN_				BIT(21)
 #define MAC_WUCSR_RFE_WAKE_EN_			BIT(14)
+#define MAC_WUCSR_EEE_TX_WAKE_			BIT(13)
+#define MAC_WUCSR_EEE_RX_WAKE_			BIT(11)
+#define MAC_WUCSR_RFE_WAKE_FR_			BIT(9)
+#define MAC_WUCSR_PFDA_FR_			BIT(7)
+#define MAC_WUCSR_WUFR_				BIT(6)
+#define MAC_WUCSR_MPR_				BIT(5)
+#define MAC_WUCSR_BCAST_FR_			BIT(4)
 #define MAC_WUCSR_PFDA_EN_			BIT(3)
 #define MAC_WUCSR_WAKE_EN_			BIT(2)
 #define MAC_WUCSR_MPEN_				BIT(1)
 #define MAC_WUCSR_BCST_EN_			BIT(0)
 
 #define MAC_WK_SRC				(0x144)
+#define MAC_WK_SRC_ETH_PHY_WK_			BIT(17)
+#define MAC_WK_SRC_IPV6_TCPSYN_RCD_WK_		BIT(16)
+#define MAC_WK_SRC_IPV4_TCPSYN_RCD_WK_		BIT(15)
+#define MAC_WK_SRC_EEE_TX_WK_			BIT(14)
+#define MAC_WK_SRC_EEE_RX_WK_			BIT(13)
+#define MAC_WK_SRC_RFE_FR_WK_			BIT(12)
+#define MAC_WK_SRC_PFDA_FR_WK_			BIT(11)
+#define MAC_WK_SRC_MP_FR_WK_			BIT(10)
+#define MAC_WK_SRC_BCAST_FR_WK_			BIT(9)
+#define MAC_WK_SRC_WU_FR_WK_			BIT(8)
+#define MAC_WK_SRC_WK_FR_SAVED_			BIT(7)
+
 #define MAC_MP_SO_HI				(0x148)
 #define MAC_MP_SO_LO				(0x14C)
 
@@ -294,6 +314,10 @@
 #define RFE_INDX(index)			(0x580 + (index << 2))
 
 #define MAC_WUCSR2			(0x600)
+#define MAC_WUCSR2_NS_RCD_		BIT(7)
+#define MAC_WUCSR2_ARP_RCD_		BIT(6)
+#define MAC_WUCSR2_IPV6_TCPSYN_RCD_	BIT(5)
+#define MAC_WUCSR2_IPV4_TCPSYN_RCD_	BIT(4)
 
 #define SGMII_ACC			(0x720)
 #define SGMII_ACC_SGMII_BZY_		BIT(31)
-- 
2.34.1

Re: [PATCH net V2 1/2] net: lan743x: disable WOL upon resume to restore full data path operation

[Jiri Pirko]

Wed, Mar 20, 2024 at 05:21:06AM CET, Raju.Lakkaraju@microchip.com wrote:
>In order for datapath to be restored to normal functionality after resume
>we disable all wakeup events. Additionally we clear all W1C status bits by
>writing 1's to them.

Not sure who's "we", but in the patch description, it is good to
describe the problem first and then to describe the fix but telling the
codebase what to do/change/fix in imperative mood:

https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes


>
>Fixes: 4d94282afd95 ("lan743x: Add power management support")
>Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
>---
>Change List:
>------------
>V1 -> V2:
>  - Repost - No change
>V0 -> V1:
>  - Variable "data" change from "int" to "unsigned int"
>
> drivers/net/ethernet/microchip/lan743x_main.c | 24 ++++++++++++++++++-
> drivers/net/ethernet/microchip/lan743x_main.h | 24 +++++++++++++++++++
> 2 files changed, 47 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
>index bd8aa83b47e5..385e9dcd8cd9 100644
>--- a/drivers/net/ethernet/microchip/lan743x_main.c
>+++ b/drivers/net/ethernet/microchip/lan743x_main.c
>@@ -3550,7 +3550,7 @@ static void lan743x_pm_set_wol(struct lan743x_adapter *adapter)
> 
> 	/* clear wake settings */
> 	pmtctl = lan743x_csr_read(adapter, PMT_CTL);
>-	pmtctl |= PMT_CTL_WUPS_MASK_;
>+	pmtctl |= PMT_CTL_WUPS_MASK_ | PMT_CTL_RES_CLR_WKP_MASK_;
> 	pmtctl &= ~(PMT_CTL_GPIO_WAKEUP_EN_ | PMT_CTL_EEE_WAKEUP_EN_ |
> 		PMT_CTL_WOL_EN_ | PMT_CTL_MAC_D3_RX_CLK_OVR_ |
> 		PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_ | PMT_CTL_ETH_PHY_WAKE_EN_);
>@@ -3685,6 +3685,7 @@ static int lan743x_pm_resume(struct device *dev)
> 	struct pci_dev *pdev = to_pci_dev(dev);
> 	struct net_device *netdev = pci_get_drvdata(pdev);
> 	struct lan743x_adapter *adapter = netdev_priv(netdev);
>+	u32 data;
> 	int ret;
> 
> 	pci_set_power_state(pdev, PCI_D0);
>@@ -3715,6 +3716,27 @@ static int lan743x_pm_resume(struct device *dev)
> 	netif_info(adapter, drv, adapter->netdev,
> 		   "Wakeup source : 0x%08X\n", ret);
> 
>+	/* Clear the wol configuration and status bits when system
>+	 * events occurs.
>+	 * The status bits are "Write One to Clear (W1C)"
>+	 */
>+	data = MAC_WUCSR_EEE_TX_WAKE_ | MAC_WUCSR_EEE_RX_WAKE_ |
>+	       MAC_WUCSR_RFE_WAKE_FR_ | MAC_WUCSR_PFDA_FR_ | MAC_WUCSR_WUFR_ |
>+	       MAC_WUCSR_MPR_ | MAC_WUCSR_BCAST_FR_;
>+	lan743x_csr_write(adapter, MAC_WUCSR, data);
>+
>+	data = MAC_WUCSR2_NS_RCD_ | MAC_WUCSR2_ARP_RCD_ |
>+	       MAC_WUCSR2_IPV6_TCPSYN_RCD_ | MAC_WUCSR2_IPV4_TCPSYN_RCD_;
>+	lan743x_csr_write(adapter, MAC_WUCSR2, data);
>+
>+	data = MAC_WK_SRC_ETH_PHY_WK_ | MAC_WK_SRC_IPV6_TCPSYN_RCD_WK_ |
>+	       MAC_WK_SRC_IPV4_TCPSYN_RCD_WK_ | MAC_WK_SRC_EEE_TX_WK_ |
>+	       MAC_WK_SRC_EEE_RX_WK_ | MAC_WK_SRC_RFE_FR_WK_ |
>+	       MAC_WK_SRC_PFDA_FR_WK_ | MAC_WK_SRC_MP_FR_WK_ |
>+	       MAC_WK_SRC_BCAST_FR_WK_ | MAC_WK_SRC_WU_FR_WK_ |
>+	       MAC_WK_SRC_WK_FR_SAVED_;
>+	lan743x_csr_write(adapter, MAC_WK_SRC, data);
>+
> 	return 0;
> }
> 
>diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
>index be79cb0ae5af..77fc3abc1428 100644
>--- a/drivers/net/ethernet/microchip/lan743x_main.h
>+++ b/drivers/net/ethernet/microchip/lan743x_main.h
>@@ -60,6 +60,7 @@
> #define PMT_CTL_RX_FCT_RFE_D3_CLK_OVR_		BIT(18)
> #define PMT_CTL_GPIO_WAKEUP_EN_			BIT(15)
> #define PMT_CTL_EEE_WAKEUP_EN_			BIT(13)
>+#define PMT_CTL_RES_CLR_WKP_MASK_		GENMASK(9, 8)
> #define PMT_CTL_READY_				BIT(7)
> #define PMT_CTL_ETH_PHY_RST_			BIT(4)
> #define PMT_CTL_WOL_EN_				BIT(3)
>@@ -226,12 +227,31 @@
> #define MAC_WUCSR				(0x140)
> #define MAC_MP_SO_EN_				BIT(21)
> #define MAC_WUCSR_RFE_WAKE_EN_			BIT(14)
>+#define MAC_WUCSR_EEE_TX_WAKE_			BIT(13)
>+#define MAC_WUCSR_EEE_RX_WAKE_			BIT(11)
>+#define MAC_WUCSR_RFE_WAKE_FR_			BIT(9)
>+#define MAC_WUCSR_PFDA_FR_			BIT(7)
>+#define MAC_WUCSR_WUFR_				BIT(6)
>+#define MAC_WUCSR_MPR_				BIT(5)
>+#define MAC_WUCSR_BCAST_FR_			BIT(4)
> #define MAC_WUCSR_PFDA_EN_			BIT(3)
> #define MAC_WUCSR_WAKE_EN_			BIT(2)
> #define MAC_WUCSR_MPEN_				BIT(1)
> #define MAC_WUCSR_BCST_EN_			BIT(0)
> 
> #define MAC_WK_SRC				(0x144)
>+#define MAC_WK_SRC_ETH_PHY_WK_			BIT(17)
>+#define MAC_WK_SRC_IPV6_TCPSYN_RCD_WK_		BIT(16)
>+#define MAC_WK_SRC_IPV4_TCPSYN_RCD_WK_		BIT(15)
>+#define MAC_WK_SRC_EEE_TX_WK_			BIT(14)
>+#define MAC_WK_SRC_EEE_RX_WK_			BIT(13)
>+#define MAC_WK_SRC_RFE_FR_WK_			BIT(12)
>+#define MAC_WK_SRC_PFDA_FR_WK_			BIT(11)
>+#define MAC_WK_SRC_MP_FR_WK_			BIT(10)
>+#define MAC_WK_SRC_BCAST_FR_WK_			BIT(9)
>+#define MAC_WK_SRC_WU_FR_WK_			BIT(8)
>+#define MAC_WK_SRC_WK_FR_SAVED_			BIT(7)
>+
> #define MAC_MP_SO_HI				(0x148)
> #define MAC_MP_SO_LO				(0x14C)
> 
>@@ -294,6 +314,10 @@
> #define RFE_INDX(index)			(0x580 + (index << 2))
> 
> #define MAC_WUCSR2			(0x600)
>+#define MAC_WUCSR2_NS_RCD_		BIT(7)
>+#define MAC_WUCSR2_ARP_RCD_		BIT(6)
>+#define MAC_WUCSR2_IPV6_TCPSYN_RCD_	BIT(5)
>+#define MAC_WUCSR2_IPV4_TCPSYN_RCD_	BIT(4)
> 
> #define SGMII_ACC			(0x720)
> #define SGMII_ACC_SGMII_BZY_		BIT(31)
>-- 
>2.34.1
>
>

[PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Raju Lakkaraju]

Allow WOL support if MAC supports it, even if the PHY does not support it

Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer dereference with bare card")
Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
------------
V1 -> V2:
  - Repost - No change
V0 -> V1:
  - Change the "phy does not support WOL" print from netif_info() to
    netif_dbg()

 drivers/net/ethernet/microchip/lan743x_ethtool.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 8a6ae171e375..7509a19269c3 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1163,6 +1163,17 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
 				   struct ethtool_wolinfo *wol)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	int ret;
+
+	if (netdev->phydev) {
+		ret = phy_ethtool_set_wol(netdev->phydev, wol);
+		if (ret != -EOPNOTSUPP && ret != 0)
+			return ret;
+
+		if (ret == -EOPNOTSUPP)
+			netif_dbg(adapter, drv, adapter->netdev,
+				  "phy does not support WOL\n");
+	}
 
 	adapter->wolopts = 0;
 	if (wol->wolopts & WAKE_UCAST)
@@ -1187,8 +1198,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
 
 	device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
 
-	return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
-			: -ENETDOWN;
+	return 0;
 }
 #endif /* CONFIG_PM */
 
-- 
2.34.1

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Jiri Pirko]

Wed, Mar 20, 2024 at 05:21:07AM CET, Raju.Lakkaraju@microchip.com wrote:
>Allow WOL support if MAC supports it, even if the PHY does not support it

Sentences like this one usually end with "."


>
>Fixes: e9e13b6adc338 ("lan743x: fix for potential NULL pointer dereference with bare card")
>Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
>---
>Change List:
>------------
>V1 -> V2:
>  - Repost - No change
>V0 -> V1:
>  - Change the "phy does not support WOL" print from netif_info() to
>    netif_dbg()
>
> drivers/net/ethernet/microchip/lan743x_ethtool.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
>index 8a6ae171e375..7509a19269c3 100644
>--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
>+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
>@@ -1163,6 +1163,17 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
> 				   struct ethtool_wolinfo *wol)
> {
> 	struct lan743x_adapter *adapter = netdev_priv(netdev);
>+	int ret;

You use "ret" only in "if (netdev->phydev) {" scope, move it there.


>+
>+	if (netdev->phydev) {
>+		ret = phy_ethtool_set_wol(netdev->phydev, wol);
>+		if (ret != -EOPNOTSUPP && ret != 0)
>+			return ret;
>+
>+		if (ret == -EOPNOTSUPP)
>+			netif_dbg(adapter, drv, adapter->netdev,
>+				  "phy does not support WOL\n");

How about to chenge the flow to:

		ret = phy_ethtool_set_wol(netdev->phydev, wol);

		if (ret == -EOPNOTSUPP)
			netif_dbg(adapter, drv, adapter->netdev,
				  "phy does not support WOL\n");
		else if (ret)
			return ret;

to avoid double check of EOPNOTSUPP?


> 	}
> 
> 	adapter->wolopts = 0;
> 	if (wol->wolopts & WAKE_UCAST)
>@@ -1187,8 +1198,7 @@ static int lan743x_ethtool_set_wol(struct net_device *netdev,
> 
> 	device_set_wakeup_enable(&adapter->pdev->dev, (bool)wol->wolopts);
> 
>-	return netdev->phydev ? phy_ethtool_set_wol(netdev->phydev, wol)
>-			: -ENETDOWN;
>+	return 0;
> }
> #endif /* CONFIG_PM */
> 
>-- 
>2.34.1
>
>

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

> +	if (netdev->phydev) {
> +		ret = phy_ethtool_set_wol(netdev->phydev, wol);
> +		if (ret != -EOPNOTSUPP && ret != 0)
> +			return ret;

I'm not sure this condition is correct.

If there is an error, and the error is not EOPNOTSUPP, you want to
report that error. However, if the PHY can support the WoL
configuration, it will return 0, and this function should exit, WoL in
the MAC is not needed. And doing WoL in the PHY consumes less power
since you can suspend the MAC.

So i think it should simply be:

> +		if (ret != -EOPNOTSUPP)
> +			return ret;

Do you have a board with this MAC with a PHY which does have some WoL
support. Could you test PHY WoL is used when appropriate?

	Andrew

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Florian Fainelli]

On 3/20/24 15:53, Andrew Lunn wrote:
>> +	if (netdev->phydev) {
>> +		ret = phy_ethtool_set_wol(netdev->phydev, wol);
>> +		if (ret != -EOPNOTSUPP && ret != 0)
>> +			return ret;
> 
> I'm not sure this condition is correct.
> 
> If there is an error, and the error is not EOPNOTSUPP, you want to
> report that error. However, if the PHY can support the WoL
> configuration, it will return 0, and this function should exit, WoL in
> the MAC is not needed. And doing WoL in the PHY consumes less power
> since you can suspend the MAC.
> 
> So i think it should simply be:
> 
>> +		if (ret != -EOPNOTSUPP)
>> +			return ret;

Agreed, that's what I did for bcmgenet_wol.c.
-- 
Florian

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Raju.Lakkaraju]

Hi Andrew,

Sorry for delayed response.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, March 21, 2024 4:23 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
> PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +     if (netdev->phydev) {
> > +             ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > +             if (ret != -EOPNOTSUPP && ret != 0)
> > +                     return ret;
> 
> I'm not sure this condition is correct.
> 
> If there is an error, and the error is not EOPNOTSUPP, you want to report that
> error. However, if the PHY can support the WoL configuration, it will return 0,
> and this function should exit, WoL in the MAC is not needed. And doing WoL
> in the PHY consumes less power since you can suspend the MAC.
> 
> So i think it should simply be:
> 
> > +             if (ret != -EOPNOTSUPP)
> > +                     return ret;
> 
> Do you have a board with this MAC with a PHY which does have some WoL
> support. Could you test PHY WoL is used when appropriate?

Yes. 
We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x (PCIe Ethernet chip) 
If I don't register the Ethernet module in wakeup source, WOL is not working. Ethernet device power state shows as disable.
i.e.
/sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/power/wakeup   <--  disabled

PCI11x1x is PCIe bridge device between PCIe and Ethernet along with other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
0000:09:00.0 - Ethernet device
0000:05:00.0 - PCIe Bridge Up link 

When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C PHY), observe the following:
1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol p), GPY211 PHY configure the WOL. After resume from sleep, GPY211 WOL configuration vanish. Observed that gpy_config_init( ) function reset. Is it expected behaviour ? In other mail thread, we discussed that Ethtool configuration should retain after resume from sleep.

2. when WOL configure with ethtool, Either Link-down and Link-up on CLI, WOL configuration vanish. Is it expected behaviour ? Due to this, every time we have to configure WOL through Ethtool.

Based on above information, We need to check for return < 0 condition and return the error. Else enable the wakeup by calling "device_set_wakeup_enable( )" function.

> 
>         Andrew

Thanks,
Raju

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

On Fri, Apr 05, 2024 at 08:17:22AM +0000, Raju.Lakkaraju@microchip.com wrote:
> Hi Andrew,
> 
> Sorry for delayed response.
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Thursday, March 21, 2024 4:23 AM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> > Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> > UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> > Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
> > PHY does not
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > > +     if (netdev->phydev) {
> > > +             ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > > +             if (ret != -EOPNOTSUPP && ret != 0)
> > > +                     return ret;
> > 
> > I'm not sure this condition is correct.
> > 
> > If there is an error, and the error is not EOPNOTSUPP, you want to report that
> > error. However, if the PHY can support the WoL configuration, it will return 0,
> > and this function should exit, WoL in the MAC is not needed. And doing WoL
> > in the PHY consumes less power since you can suspend the MAC.
> > 
> > So i think it should simply be:
> > 
> > > +             if (ret != -EOPNOTSUPP)
> > > +                     return ret;
> > 
> > Do you have a board with this MAC with a PHY which does have some WoL
> > support. Could you test PHY WoL is used when appropriate?
> 
> Yes. 
> We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x (PCIe Ethernet chip) 
> If I don't register the Ethernet module in wakeup source, WOL is not working. Ethernet device power state shows as disable.

So i'm talking about the case where the PHY is doing the wakeup,
without help from the MAC. In that case, "Ethernet device power state
shows as disable." is sensible.

> i.e.
> /sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/power/wakeup   <--  disabled
> 
> PCI11x1x is PCIe bridge device between PCIe and Ethernet along with other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
> 0000:09:00.0 - Ethernet device
> 0000:05:00.0 - PCIe Bridge Up link 

How does the PHY indicate wake up? It could be you can power off the
MAC, but you need to keep parts of the PCIe bridge up, in order the
wake up gets delivered?

> When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C
> PHY), observe the following:

> 1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol
> p), GPY211 PHY configure the WOL. After resume from sleep, GPY211
> WOL configuration vanish. Observed that gpy_config_init( ) function
> reset. Is it expected behaviour ? In other mail thread, we discussed
> that Ethtool configuration should retain after resume from sleep.

The whole point of suspend/resume is that the configuration is
retained. So i would expect from the users perspective WoL is still
enabled.

As you point out, we might have a logic issue here, that on resume we
hit the PHY with a hardware reset. That could be clearing out WoL?  We
might need to cache the PHY WoL configuration in phydev, and on resume
re-apply it. WoL is complex so we need to be careful who is actually
managing it. But this seems like something which could be done in the
phylib core.

> 2. when WOL configure with ethtool, Either Link-down and Link-up on
> CLI, WOL configuration vanish. Is it expected behaviour ? Due to
> this, every time we have to configure WOL through Ethtool.

This is unexpected. I would expect WoL to remain configured until the
configuration is changed.

> Based on above information, We need to check for return < 0
> condition and return the error. Else enable the wakeup by calling
> "device_set_wakeup_enable( )" function.

Once it is clear how the PHY does the wakeup, we can look at
this. However, if the PHY can support the WoL, you should not be
configuring the MAC as well to do the same WoL, you should be putting
the MAC into a low power mode.

    Andrew

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Raju.Lakkaraju]

[-- Attachment #1: Type: text/plain, Size: 7330 bytes --]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, April 5, 2024 10:42 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
> PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, Apr 05, 2024 at 08:17:22AM +0000, Raju.Lakkaraju@microchip.com
> wrote:
> > Hi Andrew,
> >
> > Sorry for delayed response.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Thursday, March 21, 2024 4:23 AM
> > > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > > Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> > > pabeni@redhat.com; edumazet@google.com;
> > > linux-kernel@vger.kernel.org; Bryan Whitehead - C21958
> > > <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> > > <UNGLinuxDriver@microchip.com>
> > > Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC
> > > even when PHY does not
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > > +     if (netdev->phydev) {
> > > > +             ret = phy_ethtool_set_wol(netdev->phydev, wol);
> > > > +             if (ret != -EOPNOTSUPP && ret != 0)
> > > > +                     return ret;
> > >
> > > I'm not sure this condition is correct.
> > >
> > > If there is an error, and the error is not EOPNOTSUPP, you want to
> > > report that error. However, if the PHY can support the WoL
> > > configuration, it will return 0, and this function should exit, WoL
> > > in the MAC is not needed. And doing WoL in the PHY consumes less
> power since you can suspend the MAC.
> > >
> > > So i think it should simply be:
> > >
> > > > +             if (ret != -EOPNOTSUPP)
> > > > +                     return ret;
> > >
> > > Do you have a board with this MAC with a PHY which does have some
> > > WoL support. Could you test PHY WoL is used when appropriate?
> >
> > Yes.
> > We have external PHY (Max Linear GPY211C) attach to MAC of PCI11x1x
> > (PCIe Ethernet chip) If I don't register the Ethernet module in wakeup
> source, WOL is not working. Ethernet device power state shows as disable.
> 
> So i'm talking about the case where the PHY is doing the wakeup, without
> help from the MAC. In that case, "Ethernet device power state shows as
> disable." is sensible.
> 

If we don't enable/register the PCI11x1x's ethernet device in wake list by calling " device_set_wakeup_enable( )" function, device power state shows as disable.
When I refer the other vendor pcie's ethernet drivers, " device_set_wakeup_enable( )" function called in ethernet driver Power Management's suspend and resume functions.
But LAN743x driver call this function when Ethtool configure the WOL.

> > i.e.
> >
> /sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.
> 0/power/wakeup   <--  disabled

When I call the device_set_wakeup_enable( ) with enable = 1, wakeup (/sys/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:03.0/0000:09:00.0/power/wakeup) shows enable.
Also observe that device (PCI11x1x's ethernet device : 0000:09:00.0) add in wake list.
i.e.
device: 'wakeup34': device_add

> >
> > PCI11x1x is PCIe bridge device between PCIe and Ethernet along with
> > other peripherals (i.e. UART, SPI, I2C, USB and PCIe devices)
> > 0000:09:00.0 - Ethernet device
> > 0000:05:00.0 - PCIe Bridge Up link
> 
> How does the PHY indicate wake up? It could be you can power off the MAC,
> but you need to keep parts of the PCIe bridge up, in order the wake up gets
> delivered?
> 

PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device.
When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY, Interrupt generation observed when magic packet or link activity (link down or link up).
If wakeup enable in PCI11x1x's ethernet device, System resumes from sleep.

> > When I test the WOL_PHY option on setup (PCI11x1x MAC + GPY211C PHY),
> > observe the following:
> 
> > 1. When enable WOL_PHY by using ethtool (i.e. ethtool -s enp9s0 wol
> > p), GPY211 PHY configure the WOL. After resume from sleep, GPY211 WOL
> > configuration vanish. Observed that gpy_config_init( ) function reset.
> > Is it expected behaviour ? In other mail thread, we discussed that
> > Ethtool configuration should retain after resume from sleep.
> 
> The whole point of suspend/resume is that the configuration is retained. So i
> would expect from the users perspective WoL is still enabled.
> 
> As you point out, we might have a logic issue here, that on resume we hit the
> PHY with a hardware reset. That could be clearing out WoL?  We might need
> to cache the PHY WoL configuration in phydev, and on resume re-apply it.
> WoL is complex so we need to be careful who is actually managing it. But this
> seems like something which could be done in the phylib core.
> 

Yes. You are correct.  On resume we hit the PHY with a hardware reset. That could be clearing out WoL.

On resume, phy_init_hw( ) calls the PHY's configuration and interrupt functions to resets.
As you suggested, I add the wolopts flag in phydev and update this when set_wol function execute. This change fix the issue.
i.e.
Re-apply WOL configuration on PHY in phy_suspend( ) function.

Please find the attached prototype code change (Temporary patch)for reference. 
I will submit this patch separately.

> > 2. when WOL configure with ethtool, Either Link-down and Link-up on
> > CLI, WOL configuration vanish. Is it expected behaviour ? Due to this,
> > every time we have to configure WOL through Ethtool.
> 
> This is unexpected. I would expect WoL to remain configured until the
> configuration is changed.
> 

Observe that magic packet configuration(ethtool's wol g option) did not vanish.
But when Link flap occur for WAKE_PHY case, this issue observes. This might be recent "netlink" changes issue. 
I will debug and confirm.

> > Based on above information, We need to check for return < 0 condition
> > and return the error. Else enable the wakeup by calling
> > "device_set_wakeup_enable( )" function.
> 
> Once it is clear how the PHY does the wakeup, we can look at this. However, if
> the PHY can support the WoL, you should not be configuring the MAC as well
> to do the same WoL, you should be putting the MAC into a low power mode.
> 

When PHY support's the WOL (WAKE_PHY, WAKE_MAGIC), it's generated only MDINT interrupt which can handle by MAC.
But we must configure the wake phy interrupt bit in MAC's Power Management register.

PCI11x1x's ethernet device MAC supports "Secure-ON" features, but GPY211 does not support "Secure-ON".
So, we have to use MAC WOL for secure-on feature.

I tested this part. 
When enable the MAC's WAKE_PHY_INT bit, when PHY generate the interrupt, System resumes from sleep.

>     Andrew

Thanks,
Raju

[-- Attachment #2: 0001-Wake-ON-LAN-PHY-reconfiguration.patch --]
[-- Type: application/octet-stream, Size: 2015 bytes --]

From 6b5ab531a2a2a13bef6a54a0e2a78ffe4008bdb4 Mon Sep 17 00:00:00 2001
From: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
Date: Tue, 23 Apr 2024 15:47:28 +0530
Subject: [PATCH] Wake-ON-LAN PHY reconfiguration

---
 drivers/net/phy/mxl-gpy.c    | 4 ++++
 drivers/net/phy/phy_device.c | 5 +++++
 include/linux/phy.h          | 1 +
 3 files changed, 10 insertions(+)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index b2d36a3a96f1..9b97facc9b03 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
 	struct net_device *attach_dev = phydev->attached_dev;
 	int ret;
 
+	phydev->wolopts = 0;
 	if (wol->wolopts & WAKE_MAGIC) {
 		/* MAC address - Byte0:Byte1:Byte2:Byte3:Byte4:Byte5
 		 * VPSPEC2_WOL_AD45 = Byte0:Byte1
@@ -748,9 +749,12 @@ static int gpy_set_wol(struct phy_device *phydev,
 		if (ret & (PHY_IMASK_MASK & ~PHY_IMASK_LSTC))
 			phy_trigger_machine(phydev);
 
+		phydev->wolopts = wol->wolopts;
 		return 0;
 	}
 
+	phydev->wolopts = wol->wolopts;
+
 	/* Disable the link state change interrupt */
 	return phy_clear_bits(phydev, PHY_IMASK, PHY_IMASK_LSTC);
 }
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 616bd7ba46cb..9740f08ad98e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2038,6 +2038,11 @@ int phy_suspend(struct phy_device *phydev)
 	if (phydev->suspended)
 		return 0;
 
+	if (phydev->wolopts) {
+		wol.wolopts = phydev->wolopts;
+		phy_ethtool_set_wol(phydev, &wol);
+	}
+
 	phy_ethtool_get_wol(phydev, &wol);
 	phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled);
 	/* If the device has WOL enabled, we cannot suspend the PHY */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ddfe7fe781a..c88ffa7ac89b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -766,6 +766,7 @@ struct phy_device {
 
 	u8 mdix;
 	u8 mdix_ctrl;
+	u32 wolopts;
 
 	int pma_extable;
 
-- 
2.34.1

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

> If we don't enable/register the PCI11x1x's ethernet device in wake
> list by calling " device_set_wakeup_enable( )" function, device
> power state shows as disable.

> PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device.

> When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY,
> Interrupt generation observed when magic packet or link activity
> (link down or link up).  If wakeup enable in PCI11x1x's ethernet
> device, System resumes from sleep.

This is the bit that is missing from your commit message, and maybe it
should be in a comment. The interrupt controller is part of the
MAC. So you need to leave MAC burning power, maybe even processing
packets, because you cannot disable the MAC but leave the interrupt
controller functioning, so that it can trigger a wake up via PCIe.

There are a few things you should consider:

Call phy_speed_down().  This will renegotiate the link, dropping it to
the slowest speed both link partners support. So hopefully down to
10Mbps. Your MAC will then only need to pointlessly process 10Mbps of
packets, rather than 1Gbps.

See if you can disable parts of the MAC, in particularly the receive
engine, in order to save power.

Talk to your hardware engineer and see if the next generation of the
hardware can separate the interrupt controller from the MAC, so you
can disable the MAC and leave the interrupt controller functioning.

> Please find the attached prototype code change (Temporary patch)for reference. 
> I will submit this patch separately.

Please just submit it in the normal way for review.

       Andrew

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Ronnie.Kunin]

Thanks very much for your feedback and suggestions Andrew, some comments and clarifications below.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, April 23, 2024 3:11 PM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; linux-kernel@vger.kernel.org; Bryan Whitehead - C21958
> <Bryan.Whitehead@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > If we don't enable/register the PCI11x1x's ethernet device in wake
> > list by calling " device_set_wakeup_enable( )" function, device power
> > state shows as disable.
> 
> > PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device.
> 
> > When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY, Interrupt
> > generation observed when magic packet or link activity (link down or
> > link up).  If wakeup enable in PCI11x1x's ethernet device, System
> > resumes from sleep.
> 
> This is the bit that is missing from your commit message, and maybe it should be in a comment. The
> interrupt controller is part of the MAC. So you need to leave MAC burning power, maybe even
> processing packets, because you cannot disable the MAC but leave the interrupt controller functioning,
> so that it can trigger a wake up via PCIe.

I think there is a terminology problem here. Within MCHP we sometimes call "the MAC" to the whole ethernet controller chip that has everything (i.e. actual MAC, FIFOs, filtering engines, offloads, interrupt controller, bus interface, etc) except the PHY.
That is what Raju probably means when he says that the interrupt is handled by "the MAC". While the registers that control enabling/disabling processing of the phy interrupt do reside within the MAC block's register set in the ethernet controller, it neither means that the extensive parts of the actual MAC block need to be kept enabled nor that processing packets has to occur in the MAC in order for the PCI11x1x chip to detect an event coming from the PHY that should be processed as a wake event over PCIe  

> 
> There are a few things you should consider:
> 
> Call phy_speed_down().  This will renegotiate the link, dropping it to the slowest speed both link
> partners support. So hopefully down to 10Mbps. Your MAC will then only need to pointlessly process
> 10Mbps of packets, rather than 1Gbps.p

I am Windows driver expert, not Linux so I may be wrong for Linux, but with the advent of dynamic PM in modern OSs (connected and then modern standby in Windows, I remember also autosuspend - at least in USB, maybe not applicable to PCIe  - in Linux ) we have stayed away from renegotiating the link to down speed during suspend - resume events since those interfere with / delay connectivity significantly.

> 
> See if you can disable parts of the MAC, in particularly the receive engine, in order to save power.

This is already supported by the hardware and I had told Raju to do it as soon as he successfully got the host to wake over PCIe from events signaled from the PHY 

> 
> Talk to your hardware engineer and see if the next generation of the hardware can separate the
> interrupt controller from the MAC, so you can disable the MAC and leave the interrupt controller
> functioning.

As mentioned above, for all intents and purposes, that is already the case.

> 
> > Please find the attached prototype code change (Temporary patch)for reference.
> > I will submit this patch separately.
> 
> Please just submit it in the normal way for review.
> 
>        Andrew

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Ronnie.Kunin]

> While the registers that control enabling/disabling processing of the phy interrupt do reside within the MAC
> block's register set in the ethernet controller
Correction, the bits that control processing of the PHY interrupt are NOT within the actual MAC block register set either. 

> -----Original Message-----
> From: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
> Sent: Wednesday, April 24, 2024 1:59 AM
> To: Andrew Lunn <andrew@lunn.ch>; Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; linux-kernel@vger.kernel.org; Bryan Whitehead - C21958
> <Bryan.Whitehead@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not
> 
> Thanks very much for your feedback and suggestions Andrew, some comments and clarifications below.
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Tuesday, April 23, 2024 3:11 PM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> > Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> > UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> > Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even
> > when PHY does not
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > > If we don't enable/register the PCI11x1x's ethernet device in wake
> > > list by calling " device_set_wakeup_enable( )" function, device
> > > power state shows as disable.
> >
> > > PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet device.
> >
> > > When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY,
> > > Interrupt generation observed when magic packet or link activity
> > > (link down or link up).  If wakeup enable in PCI11x1x's ethernet
> > > device, System resumes from sleep.
> >
> > This is the bit that is missing from your commit message, and maybe it
> > should be in a comment. The interrupt controller is part of the MAC.
> > So you need to leave MAC burning power, maybe even processing packets,
> > because you cannot disable the MAC but leave the interrupt controller functioning, so that it can
> trigger a wake up via PCIe.
> 
> I think there is a terminology problem here. Within MCHP we sometimes call "the MAC" to the whole
> ethernet controller chip that has everything (i.e. actual MAC, FIFOs, filtering engines, offloads, interrupt
> controller, bus interface, etc) except the PHY.
> That is what Raju probably means when he says that the interrupt is handled by "the MAC". While the
> registers that control enabling/disabling processing of the phy interrupt do reside within the MAC
> block's register set in the ethernet controller, it neither means that the extensive parts of the actual MAC
> block need to be kept enabled nor that processing packets has to occur in the MAC in order for the
> PCI11x1x chip to detect an event coming from the PHY that should be processed as a wake event over
> PCIe
> 
> >
> > There are a few things you should consider:
> >
> > Call phy_speed_down().  This will renegotiate the link, dropping it to
> > the slowest speed both link partners support. So hopefully down to
> > 10Mbps. Your MAC will then only need to pointlessly process 10Mbps of
> > packets, rather than 1Gbps.p
> 
> I am Windows driver expert, not Linux so I may be wrong for Linux, but with the advent of dynamic PM
> in modern OSs (connected and then modern standby in Windows, I remember also autosuspend - at
> least in USB, maybe not applicable to PCIe  - in Linux ) we have stayed away from renegotiating the link
> to down speed during suspend - resume events since those interfere with / delay connectivity
> significantly.
> 
> >
> > See if you can disable parts of the MAC, in particularly the receive engine, in order to save power.
> 
> This is already supported by the hardware and I had told Raju to do it as soon as he successfully got the
> host to wake over PCIe from events signaled from the PHY
> 
> >
> > Talk to your hardware engineer and see if the next generation of the
> > hardware can separate the interrupt controller from the MAC, so you
> > can disable the MAC and leave the interrupt controller functioning.
> 
> As mentioned above, for all intents and purposes, that is already the case.
> 
> >
> > > Please find the attached prototype code change (Temporary patch)for reference.
> > > I will submit this patch separately.
> >
> > Please just submit it in the normal way for review.
> >
> >        Andrew

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

> > This is the bit that is missing from your commit message, and maybe it should be in a comment. The
> > interrupt controller is part of the MAC. So you need to leave MAC burning power, maybe even
> > processing packets, because you cannot disable the MAC but leave the interrupt controller functioning,
> > so that it can trigger a wake up via PCIe.
> 

> I think there is a terminology problem here. Within MCHP we
> sometimes call "the MAC" to the whole ethernet controller chip that
> has everything (i.e. actual MAC, FIFOs, filtering engines, offloads,
> interrupt controller, bus interface, etc) except the PHY.

> That is what Raju probably means when he says that the interrupt is
> handled by "the MAC". While the registers that control
> enabling/disabling processing of the phy interrupt do reside within
> the MAC block's register set in the ethernet controller, it neither
> means that the extensive parts of the actual MAC block need to be
> kept enabled nor that processing packets has to occur in the MAC in
> order for the PCI11x1x chip to detect an event coming from the PHY
> that should be processed as a wake event over PCIe

I was expecting that, which is why i suggested looking at shutting
down what is not needed.  The current lan743x_ethtool_set_wol() does
not do any of that.

> I am Windows driver expert, not Linux so I may be wrong for Linux,
> but with the advent of dynamic PM in modern OSs (connected and then
> modern standby in Windows, I remember also autosuspend - at least in
> USB, maybe not applicable to PCIe - in Linux ) we have stayed away
> from renegotiating the link to down speed during suspend - resume
> events since those interfere with / delay connectivity
> significantly.

Renegotiation does take a little over 1s. It maybe not worth it for
suspend to RAM. But for suspend to disk, a resume is probably going to
take awhile, so maybe 1 second is less noticeable.

Also you maybe need to think about is EU norms about standby and power
off consumption. Going from 1G to 10M can save you 1W.

	Andrew

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Raju.Lakkaraju]

Hi Andrew,

Thank you for quick response and valuable suggestions.

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, April 24, 2024 12:41 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; linux-kernel@vger.kernel.org;
> Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when
> PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > If we don't enable/register the PCI11x1x's ethernet device in wake
> > list by calling " device_set_wakeup_enable( )" function, device power
> > state shows as disable.
> 
> > PHY (GPY211C)'s interrupt pin (MDINT) connect to PCI11x1x's ethernet
> device.
> 
> > When I configure the WAKE_PHY or WAKE_MAGIC on GPY211C PHY,
> Interrupt
> > generation observed when magic packet or link activity (link down or
> > link up).  If wakeup enable in PCI11x1x's ethernet device, System
> > resumes from sleep.
> 
> This is the bit that is missing from your commit message, and maybe it should
> be in a comment. The interrupt controller is part of the MAC. So you need to
> leave MAC burning power, maybe even processing packets, because you
> cannot disable the MAC but leave the interrupt controller functioning, so that
> it can trigger a wake up via PCIe.
> 
> There are a few things you should consider:
> 
> Call phy_speed_down().  This will renegotiate the link, dropping it to the
> slowest speed both link partners support. So hopefully down to 10Mbps.
> Your MAC will then only need to pointlessly process 10Mbps of packets,
> rather than 1Gbps.
> 

If PHY handles the magic packet or phy activity (i.e. WAKE_MAGIC or WAKE_PHY), our PCI11x1x's MAC will handle only interrupt (MDINT from PHY). Not MAC's magic packet.
In this case do we really call phy_speed_down( ) ?

In case of WAKE_PHY enable and put system in suspend mode,  I'm seeing issues with calling phy_speed_down(). 
When speed change occurs, Link interrupt occurs by Link partner which internally system resume from sleep.

Do we need to call phy_speed_down( ) for only magic packet (WAKE_MAGIC) case only ?

> See if you can disable parts of the MAC, in particularly the receive engine, in
> order to save power.
> 
> Talk to your hardware engineer and see if the next generation of the
> hardware can separate the interrupt controller from the MAC, so you can
> disable the MAC and leave the interrupt controller functioning.
> 
> > Please find the attached prototype code change (Temporary patch)for
> reference.
> > I will submit this patch separately.
> 

Sure. I will do.

> Please just submit it in the normal way for review.
> 
>        Andrew

Thanks,
Raju

Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Andrew Lunn]

> If PHY handles the magic packet or phy activity (i.e. WAKE_MAGIC or WAKE_PHY), our PCI11x1x's MAC will handle only interrupt (MDINT from PHY). Not MAC's magic packet.
> In this case do we really call phy_speed_down( ) ?

phy_speed_down() is orthogonal to who does the wake. Packets are
packets. phy_speed_down() does not change that. All it does it drop
the link to a slower speed. And slower speed means less power
consumption. A PHY operating at 10Mbps uses about 1W less power than a
PHY operating at 1G. The numbers will depend on the PHY, but you get
the idea. Plus the link peer will also save a similar amount out
power....

If the MAC is needed for WoL, because the PHY does not support the
needed modes, you probably also save power with the MAC running at
10Mbps. Its clocks probably tick slower, etc.

But there is a trade off. When resuming, you want to go back to the
full speed link. And that takes time, a little over 1 second. So you
need to decide, do you want to prioritise minimum power consumption
when suspended, or fast resume?

     Andrew

RE: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not

[Ronnie.Kunin]

Agreed about the tradeoff. 

From your previous entry in this thread:
>Renegotiation does take a little over 1s. It maybe not worth it for suspend to RAM. But for suspend to disk, a resume is probably going to take awhile, so maybe 1 second is less noticeable.
With some of the partner PHYs we've used, specially when supporting higher speeds such as 2.5G we had seen even much longer renegotiation times, reaching as much as 6-7 seconds in some cases.  

Raju,
Due to the above and some of the other issues you mentioned you are facing (i.e. undesired wakes on intermediate link changes that you would need to "filter out" ) , I'd  rather not try to further optimize phy power consumption by down-speeding the link at this time, and keep fast resume time and overall simplicity the priority. If/when customers request it or power regulatory norms (EU as mentioned by Andrew) make it a must have we can always revisit it. That being said, after verifying wake functionality is correct as the first priority, please do look into turning off anything in the LAN743x  and PCI11x1x devices that is not needed while asleep before completing the suspend.

Thanks,
Ronnie 

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, April 25, 2024 10:13 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; linux-kernel@vger.kernel.org; Bryan Whitehead - C21958
> <Bryan.Whitehead@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH net V2 2/2] net: lan743x: support WOL in MAC even when PHY does not
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > If PHY handles the magic packet or phy activity (i.e. WAKE_MAGIC or WAKE_PHY), our PCI11x1x's
> MAC will handle only interrupt (MDINT from PHY). Not MAC's magic packet.
> > In this case do we really call phy_speed_down( ) ?
> 
> phy_speed_down() is orthogonal to who does the wake. Packets are packets. phy_speed_down() does
> not change that. All it does it drop the link to a slower speed. And slower speed means less power
> consumption. A PHY operating at 10Mbps uses about 1W less power than a PHY operating at 1G. The
> numbers will depend on the PHY, but you get the idea. Plus the link peer will also save a similar amount
> out power....
> 
> If the MAC is needed for WoL, because the PHY does not support the needed modes, you probably also
> save power with the MAC running at 10Mbps. Its clocks probably tick slower, etc.
> 
> But there is a trade off. When resuming, you want to go back to the full speed link. And that takes time,
> a little over 1 second. So you need to decide, do you want to prioritise minimum power consumption
> when suspended, or fast resume?
> 
>      Andrew