From: Jingju Hou <[email protected]>
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 <[email protected]>
Signed-off-by: Jisheng Zhang <[email protected]>
---
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
Hi,
> -----Original Message-----
> From: [email protected] <[email protected]> On
> Behalf Of Jisheng Zhang
> Sent: Thursday, April 19, 2018 1:33 PM
> To: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>;
> David S. Miller <[email protected]>
> Cc: [email protected]; [email protected]; Jingju Hou
> <[email protected]>
> Subject: [PATCH] net: phy: marvell: clear wol event before setting it
>
> From: Jingju Hou <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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
Hi,
On Thu, 19 Apr 2018 08:38:45 +0000 Bhadram Varka wrote:
> Hi,
>
> > -----Original Message-----
> > From: [email protected] <[email protected]> On
> > Behalf Of Jisheng Zhang
> > Sent: Thursday, April 19, 2018 1:33 PM
> > To: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>;
> > David S. Miller <[email protected]>
> > Cc: [email protected]; [email protected]; Jingju Hou
> > <[email protected]>
> > Subject: [PATCH] net: phy: marvell: clear wol event before setting it
> >
> > From: Jingju Hou <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > 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
Hi,
> -----Original Message-----
> From: Jisheng Zhang <[email protected]>
> Sent: Thursday, April 19, 2018 2:24 PM
> To: Bhadram Varka <[email protected]>
> Cc: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>;
> David S. Miller <[email protected]>; [email protected]; linux-
> [email protected]; Jingju Hou <[email protected]>
> 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: [email protected] <[email protected]> On
> > > Behalf Of Jisheng Zhang
> > > Sent: Thursday, April 19, 2018 1:33 PM
> > > To: Andrew Lunn <[email protected]>; Florian Fainelli
> > > <[email protected]>; David S. Miller <[email protected]>
> > > Cc: [email protected]; [email protected]; Jingju Hou
> > > <[email protected]>
> > > Subject: [PATCH] net: phy: marvell: clear wol event before setting
> > > it
> > >
> > > From: Jingju Hou <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Jisheng Zhang <[email protected]>
> > > ---
> > > 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!
On Thu, 19 Apr 2018 09:00:40 +0000 Bhadram Varka wrote:
> Hi,
>
> > -----Original Message-----
> > From: Jisheng Zhang <[email protected]>
> > Sent: Thursday, April 19, 2018 2:24 PM
> > To: Bhadram Varka <[email protected]>
> > Cc: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>;
> > David S. Miller <[email protected]>; [email protected]; linux-
> > [email protected]; Jingju Hou <[email protected]>
> > 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: [email protected] <[email protected]> On
> > > > Behalf Of Jisheng Zhang
> > > > Sent: Thursday, April 19, 2018 1:33 PM
> > > > To: Andrew Lunn <[email protected]>; Florian Fainelli
> > > > <[email protected]>; David S. Miller <[email protected]>
> > > > Cc: [email protected]; [email protected]; Jingju Hou
> > > > <[email protected]>
> > > > Subject: [PATCH] net: phy: marvell: clear wol event before setting
> > > > it
> > > >
> > > > From: Jingju Hou <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Jisheng Zhang <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
>>> Sent: Thursday, April 19, 2018 2:24 PM
>>> To: Bhadram Varka <[email protected]>
>>> Cc: Andrew Lunn <[email protected]>; Florian Fainelli <[email protected]>;
>>> David S. Miller <[email protected]>; [email protected]; linux-
>>> [email protected]; Jingju Hou <[email protected]>
>>> 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: [email protected] <[email protected]> On
>>>>> Behalf Of Jisheng Zhang
>>>>> Sent: Thursday, April 19, 2018 1:33 PM
>>>>> To: Andrew Lunn <[email protected]>; Florian Fainelli
>>>>> <[email protected]>; David S. Miller <[email protected]>
>>>>> Cc: [email protected]; [email protected]; Jingju Hou
>>>>> <[email protected]>
>>>>> Subject: [PATCH] net: phy: marvell: clear wol event before setting
>>>>> it
>>>>>
>>>>> From: Jingju Hou <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Jisheng Zhang <[email protected]>
>>>>> ---
>>>>> 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.
> >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
On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:
> From: Jingju Hou <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jisheng Zhang <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Jisheng Zhang <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Jisheng Zhang <[email protected]>
>>>> ---
>>>> 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.
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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Jisheng Zhang <[email protected]>
>>>>> ---
>>>>> 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;
}
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".
> 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
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
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
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;
> }
> > 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