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

[Jisheng Zhang <Jisheng.Zhang@synaptics.com>]

On Thu, 19 Apr 2018 09:00:40 +0000 Bhadram Varka wrote:

> Hi,
> 
> > -----Original Message-----
> > From: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > Sent: Thursday, April 19, 2018 2:24 PM
> > To: Bhadram Varka <vbhadram@nvidia.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>;
> > David S. Miller <davem@davemloft.net>; netdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Jingju Hou <Jingju.Hou@synaptics.com>
> > 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@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > > > Behalf Of Jisheng Zhang
> > > > Sent: Thursday, April 19, 2018 1:33 PM
> > > > To: Andrew Lunn <andrew@lunn.ch>; Florian Fainelli
> > > > <f.fainelli@gmail.com>; David S. Miller <davem@davemloft.net>
> > > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Jingju Hou
> > > > <Jingju.Hou@synaptics.com>
> > > > Subject: [PATCH] net: phy: marvell: clear wol event before setting
> > > > it
> > > >
> > > > From: Jingju Hou <Jingju.Hou@synaptics.com>
> > > >
> > > > 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@synaptics.com>
> > > > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > > > ---
> > > >  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

> 
> 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"

PS: Did you use outlook as your email client? it's not suitable
for kernel mail list.

Thanks