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

[Bhadram Varka]

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 ?

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.?

Thanks!