Re: [PATCH] net: phy: marvell: clear wol event before setting it
From: Bhadram Varka
Date: Thu Apr 19 2018 - 06:05:52 EST
HiJisheng,
On 4/19/2018 2:39 PM, Jisheng Zhang wrote:
On Thu, 19 Apr 2018 09:00:40 +0000 Bhadram Varka wrote:
Hi,
-----Original Message-----
From: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
Sent: Thursday, April 19, 2018 2:24 PM
To: Bhadram Varka <vbhadram@xxxxxxxxxx>
Cc: Andrew Lunn <andrew@xxxxxxx>; Florian Fainelli <f.fainelli@xxxxxxxxx>;
David S. Miller <davem@xxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; Jingju Hou <Jingju.Hou@xxxxxxxxxxxxx>
Subject: Re: [PATCH] net: phy: marvell: clear wol event before setting it
Hi,
On Thu, 19 Apr 2018 08:38:45 +0000 Bhadram Varka wrote:
Hi,
-----Original Message-----
From: netdev-owner@xxxxxxxxxxxxxxx <netdev-owner@xxxxxxxxxxxxxxx> On
Behalf Of Jisheng Zhang
Sent: Thursday, April 19, 2018 1:33 PM
To: Andrew Lunn <andrew@xxxxxxx>; Florian Fainelli
<f.fainelli@xxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>
Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jingju Hou
<Jingju.Hou@xxxxxxxxxxxxx>
Subject: [PATCH] net: phy: marvell: clear wol event before setting
it
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
+
There is already macro to represent this register - MII_M1011_IEVENT. Do we
need this macro ?
Good point. Will use MII_M1011_IEVENT instead in v2.
/* 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);
This part of the operation already taken care by ack_interrupt and
did_interrupt [....] .ack_interrupt = &marvell_ack_interrupt,
.did_interrupt = &m88e1121_did_interrupt, [...]
If at all WOL event occurred marvell_ack_interrupt will take care of clearing the
interrupt status register.
Am I missing anything here ?
If there's no valid irq for phy, the ack_interrupt/did_interrupt won't be called.
Which means that the PHY is not having Interrupt pin ?
No valid irq doesn't mean "not having interrupt pin". they are different
Okay. If there is WoL event through magic packet then its valid irq for
the PHY right.
Then phy_interrupt will be called from there ack/did_interrupts will be
called. So it clears WoL interrupt.
Generally through PHY interrupt will wake up the system right. If there is no interrupt pin then how the system will wake up the from suspend for the magic packet.?
IIRC, the phy irq isn't necessary for WOL. The phy interrupt pin isn't
necessarily taken as "interrupt"
Please correct me if I am wrong. In this case how the system will wake
up from the SC7.There has to be wake capable irq/gpio pin to do this
operation.
Thanks,
Bhadram.