* [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 related [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 related [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
* 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
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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.