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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Wed, Mar 20, 2024 at 05:21:06AM CET, [email protected] 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 <[email protected]>
>---
>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
>
>
Wed, Mar 20, 2024 at 05:21:07AM CET, [email protected] 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 <[email protected]>
>---
>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
>
>
> + 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
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
Hi Andrew,
Sorry for delayed response.
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, March 21, 2024 4:23 AM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Bryan Whitehead - C21958 <[email protected]>;
> UNGLinuxDriver <[email protected]>
> 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
On Fri, Apr 05, 2024 at 08:17:22AM +0000, [email protected] wrote:
> Hi Andrew,
>
> Sorry for delayed response.
>
> > -----Original Message-----
> > From: Andrew Lunn <[email protected]>
> > Sent: Thursday, March 21, 2024 4:23 AM
> > To: Raju Lakkaraju - I30499 <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Bryan Whitehead - C21958 <[email protected]>;
> > UNGLinuxDriver <[email protected]>
> > 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
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Friday, April 5, 2024 10:42 PM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Bryan Whitehead - C21958 <[email protected]>;
> UNGLinuxDriver <[email protected]>
> 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, [email protected]
> wrote:
> > Hi Andrew,
> >
> > Sorry for delayed response.
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <[email protected]>
> > > Sent: Thursday, March 21, 2024 4:23 AM
> > > To: Raju Lakkaraju - I30499 <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; Bryan Whitehead - C21958
> > > <[email protected]>; UNGLinuxDriver
> > > <[email protected]>
> > > 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
> 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
> > 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
Hi Andrew,
Thank you for quick response and valuable suggestions.
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Wednesday, April 24, 2024 12:41 AM
> To: Raju Lakkaraju - I30499 <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Bryan Whitehead - C21958 <[email protected]>;
> UNGLinuxDriver <[email protected]>
> 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
> 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