LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: phy: marvell: clear wol event before setting it
@ 2018-04-19  8:02 Jisheng Zhang
  2018-04-19  8:38 ` Bhadram Varka
  2018-04-19 12:18 ` Andrew Lunn
  0 siblings, 2 replies; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-19  8:02 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, David S. Miller
  Cc: netdev, linux-kernel, Jingju Hou

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
+
 /* 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);
+
 		/* Enable the WOL interrupt */
 		err = __phy_modify(phydev, MII_88E1318S_PHY_CSIER, 0,
 				   MII_88E1318S_PHY_CSIER_WOL_EIE);
-- 
2.17.0

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  8:02 [PATCH] net: phy: marvell: clear wol event before setting it Jisheng Zhang
@ 2018-04-19  8:38 ` Bhadram Varka
  2018-04-19  8:53   ` Jisheng Zhang
  2018-04-19 12:18 ` Andrew Lunn
  1 sibling, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-19  8:38 UTC (permalink / raw)
  To: Jisheng Zhang, Andrew Lunn, Florian Fainelli, David S. Miller
  Cc: netdev, linux-kernel, Jingju Hou

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 ?

>  /* 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 ?

>  		/* Enable the WOL interrupt */
>  		err = __phy_modify(phydev, MII_88E1318S_PHY_CSIER, 0,
>  				   MII_88E1318S_PHY_CSIER_WOL_EIE);
> --
> 2.17.0

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  8:38 ` Bhadram Varka
@ 2018-04-19  8:53   ` Jisheng Zhang
  2018-04-19  9:00     ` Bhadram Varka
  0 siblings, 1 reply; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-19  8:53 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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.


Thanks

^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  8:53   ` Jisheng Zhang
@ 2018-04-19  9:00     ` Bhadram Varka
  2018-04-19  9:09       ` Jisheng Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-19  9:00 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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 ?

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!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  9:00     ` Bhadram Varka
@ 2018-04-19  9:09       ` Jisheng Zhang
  2018-04-19 10:05         ` Bhadram Varka
  0 siblings, 1 reply; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-19  9:09 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  9:09       ` Jisheng Zhang
@ 2018-04-19 10:05         ` Bhadram Varka
  2018-04-19 11:33           ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-19 10:05 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19 10:05         ` Bhadram Varka
@ 2018-04-19 11:33           ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2018-04-19 11:33 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Jisheng Zhang, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

> >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.
> 
Hi Bhadram

I've seem implementations where the line from the PHY is connected to
the power supply. It simply turns the power on. No interrupt needed.

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19  8:02 [PATCH] net: phy: marvell: clear wol event before setting it Jisheng Zhang
  2018-04-19  8:38 ` Bhadram Varka
@ 2018-04-19 12:18 ` Andrew Lunn
  2018-04-26  5:40   ` Bhadram Varka
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2018-04-19 12:18 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Florian Fainelli, David S. Miller, netdev, linux-kernel, Jingju Hou

On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:
> 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
> +
>  /* 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()?

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-19 12:18 ` Andrew Lunn
@ 2018-04-26  5:40   ` Bhadram Varka
  2018-04-26  6:15     ` Jisheng Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-26  5:40 UTC (permalink / raw)
  To: Andrew Lunn, Jisheng Zhang
  Cc: Florian Fainelli, David S. Miller, netdev, linux-kernel, Jingju Hou

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@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
>> +
>>   /* 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 ?

Observed that once WoL event occurred for magic packet then for next 
magic packet WoL event is not asserted.
Need to explicitly clear WOL status so that WOL interrupt will be 
generated by the HW.

Thanks,
Bhadram.



Thanks,
Bhadram

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  5:40   ` Bhadram Varka
@ 2018-04-26  6:15     ` Jisheng Zhang
  2018-04-26  6:26       ` Bhadram Varka
  0 siblings, 1 reply; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-26  6:15 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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@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
> >> +
> >>   /* 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()

Thanks

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  6:15     ` Jisheng Zhang
@ 2018-04-26  6:26       ` Bhadram Varka
  2018-04-26  7:09         ` Bhadram Varka
  2018-04-26  7:56         ` Jisheng Zhang
  0 siblings, 2 replies; 18+ messages in thread
From: Bhadram Varka @ 2018-04-26  6:26 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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@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
>>>> +
>>>>    /* 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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  6:26       ` Bhadram Varka
@ 2018-04-26  7:09         ` Bhadram Varka
  2018-04-27  7:25           ` Jisheng Zhang
  2018-04-26  7:56         ` Jisheng Zhang
  1 sibling, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-26  7:09 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

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@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
>>>>> +
>>>>>    /* 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;
  }

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  6:26       ` Bhadram Varka
  2018-04-26  7:09         ` Bhadram Varka
@ 2018-04-26  7:56         ` Jisheng Zhang
  2018-04-26 12:40           ` Andrew Lunn
  1 sibling, 1 reply; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-26  7:56 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou


On Thu, 26 Apr 2018 11:56:33 +0530 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:  

<snip>

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

hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
requires WOL should be "stick".

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  7:56         ` Jisheng Zhang
@ 2018-04-26 12:40           ` Andrew Lunn
  2018-04-27  3:52             ` Bhadram Varka
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2018-04-26 12:40 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Bhadram Varka, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

> hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
> requires WOL should be "stick".

I see two different cases:

Suspend/resume: The WoL state in the kernel is probably kept across
such a cycle. If so, you would expect another suspend/resume to also
work, without needs to reconfigure it.

Boot from powered off: If the interrupt just enables the power supply,
it is possible to wake up after a shutdown. There is no state kept, so
WoL will be disabled in the kernel. So WoL should also be disabled in
the hardware.

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26 12:40           ` Andrew Lunn
@ 2018-04-27  3:52             ` Bhadram Varka
  2018-04-27  7:23               ` Jisheng Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Bhadram Varka @ 2018-04-27  3:52 UTC (permalink / raw)
  To: Andrew Lunn, Jisheng Zhang
  Cc: Florian Fainelli, David S. Miller, netdev, linux-kernel, Jingju Hou

Hi Andrew/Jisheng,

On 4/26/2018 6:10 PM, Andrew Lunn wrote:
>> hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
>> requires WOL should be "stick".
> I see two different cases:
>
> Suspend/resume: The WoL state in the kernel is probably kept across
> such a cycle. If so, you would expect another suspend/resume to also
> work, without needs to reconfigure it.
Trying this scenario (suspend/resume) from my side. In this case WoL 
should be enabled in the HW. For Marvell PHY to generate WoL interrupt 
we need to clear WoL status.
Above mentioned change required to make this happen. Please share your 
thoughts on this.
>
> Boot from powered off: If the interrupt just enables the power supply,
> it is possible to wake up after a shutdown. There is no state kept, so
> WoL will be disabled in the kernel. So WoL should also be disabled in
> the hardware.
>
>      Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-27  3:52             ` Bhadram Varka
@ 2018-04-27  7:23               ` Jisheng Zhang
  0 siblings, 0 replies; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-27  7:23 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

On Fri, 27 Apr 2018 09:22:34 +0530 Bhadram Varka wrote:

> Hi Andrew/Jisheng,
> 
> On 4/26/2018 6:10 PM, Andrew Lunn wrote:
> >> hmm, so you want a "stick" WOL feature, I dunno whether Linux kernel
> >> requires WOL should be "stick".  
> > I see two different cases:
> >
> > Suspend/resume: The WoL state in the kernel is probably kept across
> > such a cycle. If so, you would expect another suspend/resume to also
> > work, without needs to reconfigure it.  
> Trying this scenario (suspend/resume) from my side. In this case WoL 
> should be enabled in the HW. For Marvell PHY to generate WoL interrupt 
> we need to clear WoL status.
> Above mentioned change required to make this happen. Please share your 
> thoughts on this.

I'm fine with that patch. Maybe you could send out a patch?

Thanks

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-26  7:09         ` Bhadram Varka
@ 2018-04-27  7:25           ` Jisheng Zhang
  2018-04-30 13:17             ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Jisheng Zhang @ 2018-04-27  7:25 UTC (permalink / raw)
  To: Bhadram Varka
  Cc: Andrew Lunn, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

On Thu, 26 Apr 2018 12:39:59 +0530 Bhadram Varka wrote:

> >>>>>
> >>>>> 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);

is it better to reuse __phy_write()?

> +       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;
>   }

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: phy: marvell: clear wol event before setting it
  2018-04-27  7:25           ` Jisheng Zhang
@ 2018-04-30 13:17             ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2018-04-30 13:17 UTC (permalink / raw)
  To: Jisheng Zhang
  Cc: Bhadram Varka, Florian Fainelli, David S. Miller, netdev,
	linux-kernel, Jingju Hou

> > 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);
> 
> is it better to reuse __phy_write()?

phy_modify_paged() would be the best function to use.

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-04-30 13:17 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19  8:02 [PATCH] net: phy: marvell: clear wol event before setting it Jisheng Zhang
2018-04-19  8:38 ` Bhadram Varka
2018-04-19  8:53   ` Jisheng Zhang
2018-04-19  9:00     ` Bhadram Varka
2018-04-19  9:09       ` Jisheng Zhang
2018-04-19 10:05         ` Bhadram Varka
2018-04-19 11:33           ` Andrew Lunn
2018-04-19 12:18 ` Andrew Lunn
2018-04-26  5:40   ` Bhadram Varka
2018-04-26  6:15     ` Jisheng Zhang
2018-04-26  6:26       ` Bhadram Varka
2018-04-26  7:09         ` Bhadram Varka
2018-04-27  7:25           ` Jisheng Zhang
2018-04-30 13:17             ` Andrew Lunn
2018-04-26  7:56         ` Jisheng Zhang
2018-04-26 12:40           ` Andrew Lunn
2018-04-27  3:52             ` Bhadram Varka
2018-04-27  7:23               ` Jisheng Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).