Re: [PATCH] net: phy: marvell: clear wol event before setting it

From: Bhadram Varka
Date: Thu Apr 26 2018 - 03:10:13 EST


Hi,

On 4/26/2018 11:56 AM, Bhadram Varka wrote:
Hi,
On 4/26/2018 11:45 AM, Jisheng Zhang wrote:
Hi,

On Thu, 26 Apr 2018 11:10:21 +0530 Bhadram Varka wrote:

Hi,

On 4/19/2018 5:48 PM, Andrew Lunn wrote:
On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:
From: Jingju Hou <Jingju.Hou@xxxxxxxxxxxxx>

If WOL event happened once, the LED[2] interrupt pin will not be
cleared unless reading the CSISR register. So clear the WOL event
before enabling it.

Signed-off-by: Jingju Hou <Jingju.Hou@xxxxxxxxxxxxx>
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
---
ÂÂ drivers/net/phy/marvell.c | 9 +++++++++
ÂÂ 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index c22e8e383247..b6abe1cbc84b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -115,6 +115,9 @@
ÂÂ /* WOL Event Interrupt Enable */
ÂÂ #define MII_88E1318S_PHY_CSIER_WOL_EIE BIT(7)
ÂÂ +/* Copper Specific Interrupt Status Register */
+#define MII_88E1318S_PHY_CSISRÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x13
+
ÂÂ /* LED Timer Control Register */
ÂÂ #define MII_88E1318S_PHY_LED_TCRÂÂÂÂÂÂÂÂÂÂÂ 0x12
ÂÂ #define MII_88E1318S_PHY_LED_TCR_FORCE_INT BIT(15)
@@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device *phydev,
ÂÂÂÂÂÂÂÂÂÂ if (err < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error;
ÂÂ +ÂÂÂÂÂÂÂ /* If WOL event happened once, the LED[2] interrupt pin
+ÂÂÂÂÂÂÂÂ * will not be cleared unless reading the CSISR register.
+ÂÂÂÂÂÂÂÂ * So clear the WOL event first before enabling it.
+ÂÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ phy_read(phydev, MII_88E1318S_PHY_CSISR);
+
Hi Jisheng

The problem with this is, you could be clearing a real interrupt, link
down/up etc. If interrupts are in use, i think the normal interrupt
handling will clear the WOL interrupt? So can you make this read
conditional on !phy_interrupt_is_valid()?
So this will clear WoL interrupt bit from Copper Interrupt status register.

How about clearing WoL status (Page 17, register 17) for every WOL event ?

This is already properly done by setting MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS
in m88e1318_set_wol()
This part of the code executes only when we enable WOL through ethtool (ethtool -s eth0 wol g)

Lets say once WOL enabled through magic packet - HW generates WOL interrupt once magic packet received.
The problem that I see here is that for the next immediate magic packet I don't see WOL interrupt generated by the HW.
I need to explicitly clear WOL status for HW to generate WOL interrupt.

With the below patch I see WOL event interrupt for every magic packet that HW receives...

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index ed8a67d..5d3d138 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -55,6 +55,7 @@

Â#define MII_M1011_IEVENTÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x13
Â#define MII_M1011_IEVENT_CLEARÂÂÂÂÂÂÂÂ 0x0000
+#define MII_M1011_IEVENT_WOL_EVENTÂÂÂÂ BIT(7)

Â#define MII_M1011_IMASKÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x12
- #define MII_M1011_IMASK_INITÂÂÂÂÂÂÂÂÂÂ 0x6400
+ #define MII_M1011_IMASK_INITÂÂÂÂÂÂÂÂÂÂ 0x6480

@@ -195,13 +196,40 @@ struct marvell_priv {
ÂÂÂÂÂÂÂ bool copper;
Â};

+static int marvell_clear_wol_status(struct phy_device *phydev)
+{
+ÂÂÂÂÂÂ int err, temp, oldpage;
+
+ÂÂÂÂÂÂ oldpage = phy_read(phydev, MII_MARVELL_PHY_PAGE);
+ÂÂÂÂÂÂ if (oldpage < 0)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return oldpage;
+
+ÂÂÂÂÂÂ err = phy_write(phydev, MII_MARVELL_PHY_PAGE,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MII_88E1318S_PHY_WOL_PAGE);
+ÂÂÂÂÂÂ if (err < 0)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return err;
+
+ÂÂÂÂÂÂ /*
+ÂÂÂÂÂÂÂ * Clear WOL status so that for next WOL event
+ÂÂÂÂÂÂÂ * interrupt will be generated by HW
+ÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂ temp = phy_read(phydev, MII_88E1318S_PHY_WOL_CTRL);
+ÂÂÂÂÂÂ temp |= MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS;
+ÂÂÂÂÂÂ err = phy_write(phydev, MII_88E1318S_PHY_WOL_CTRL, temp);
+ÂÂÂÂÂÂ if (err < 0)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return err;
+
+
+ÂÂÂÂÂÂ phy_write(phydev, MII_MARVELL_PHY_PAGE, oldpage);
+
+ÂÂÂÂÂÂ return 0;
+}
+
Âstatic int marvell_ack_interrupt(struct phy_device *phydev)
Â{
ÂÂÂÂÂÂÂ int err;

ÂÂÂÂÂÂÂ /* Clear the interrupts by reading the reg */
ÂÂÂÂÂÂÂ err = phy_read(phydev, MII_M1011_IEVENT);
-
ÂÂÂÂÂÂÂ if (err < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return err;

@@ -1454,12 +1482,18 @@ static int marvell_aneg_done(struct phy_device *phydev)

Âstatic int m88e1121_did_interrupt(struct phy_device *phydev)
Â{
-ÂÂÂÂÂÂ int imask;
+ÂÂÂÂÂÂ int imask, err;

ÂÂÂÂÂÂÂ imask = phy_read(phydev, MII_M1011_IEVENT);

-ÂÂÂÂÂÂ if (imask & MII_M1011_IMASK_INIT)
+ÂÂÂÂÂÂ if (imask & MII_M1011_IMASK_INIT) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (imask & MII_M1011_IEVENT_WOL_EVENT) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ err = marvell_clear_wol_status(phydev);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (err < 0)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 1;
+ÂÂÂÂÂÂ }

ÂÂÂÂÂÂÂ return 0;
Â}