* [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt @ 2016-07-28 1:04 Gavin Shan 2016-07-28 1:04 ` [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts Gavin Shan 2016-08-01 8:27 ` [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Joel Stanley 0 siblings, 2 replies; 7+ messages in thread From: Gavin Shan @ 2016-07-28 1:04 UTC (permalink / raw) To: openbmc; +Cc: joel, Gavin Shan Bit#11 in MACCR (0x50) designates the signal level for PHY link status change. It's cleared, meaning high level enabled, by default. However, we can see continuous interrupt (bit#6) in ISR (0x0) for it and it's obviously a false alarm. The side effect is CPU cycles wasted to process the false alarm. This sets bit#11 in MACCR (0x50) to avoid the bogus interrupt. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 1 + drivers/net/ethernet/faraday/ftgmac100.h | 1 + 2 files changed, 2 insertions(+) diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index e805c4d..f00911a 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -224,6 +224,7 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv) FTGMAC100_MACCR_RXMAC_EN | \ FTGMAC100_MACCR_FULLDUP | \ FTGMAC100_MACCR_CRC_APD | \ + FTGMAC100_MACCR_PHY_LINK_LEVEL | \ FTGMAC100_MACCR_RX_RUNT | \ FTGMAC100_MACCR_RX_BROADPKT) diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h index c258586..d07b6ea 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.h +++ b/drivers/net/ethernet/faraday/ftgmac100.h @@ -152,6 +152,7 @@ #define FTGMAC100_MACCR_FULLDUP (1 << 8) #define FTGMAC100_MACCR_GIGA_MODE (1 << 9) #define FTGMAC100_MACCR_CRC_APD (1 << 10) +#define FTGMAC100_MACCR_PHY_LINK_LEVEL (1 << 11) #define FTGMAC100_MACCR_RX_RUNT (1 << 12) #define FTGMAC100_MACCR_JUMBO_LF (1 << 13) #define FTGMAC100_MACCR_RX_ALL (1 << 14) -- 2.1.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts 2016-07-28 1:04 [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Gavin Shan @ 2016-07-28 1:04 ` Gavin Shan 2016-08-01 8:26 ` Joel Stanley 2016-08-01 8:27 ` [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Joel Stanley 1 sibling, 1 reply; 7+ messages in thread From: Gavin Shan @ 2016-07-28 1:04 UTC (permalink / raw) To: openbmc; +Cc: joel, Gavin Shan There is stale interrupt (PHYSTS_CHG in ISR, bit#6 in 0x0) from the bootloader (uboot) when enabling the MAC. The stale interrupts aren't part of kernel and should be cleared. This clears the stale interrupts in ISR (0x0) when enabling the MAC. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index f00911a..fc2852f 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1119,6 +1119,7 @@ static int ftgmac100_open(struct net_device *netdev) { struct ftgmac100 *priv = netdev_priv(netdev); struct phy_device *phydev = netdev->phydev; + unsigned int status; int err; err = ftgmac100_alloc_buffers(priv); @@ -1145,6 +1146,10 @@ static int ftgmac100_open(struct net_device *netdev) ftgmac100_init_hw(priv); ftgmac100_start_hw(priv, priv->use_ncsi ? 100 : 10); + /* Clear stale interrupts */ + status = ioread32(priv->base + FTGMAC100_OFFSET_ISR); + iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR); + if (phydev) phy_start(phydev); else if (priv->use_ncsi) -- 2.1.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts 2016-07-28 1:04 ` [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts Gavin Shan @ 2016-08-01 8:26 ` Joel Stanley 2016-08-02 6:53 ` Gavin Shan 0 siblings, 1 reply; 7+ messages in thread From: Joel Stanley @ 2016-08-01 8:26 UTC (permalink / raw) To: Gavin Shan, openbmc On Thu, 2016-07-28 at 11:04 +1000, Gavin Shan wrote: > There is stale interrupt (PHYSTS_CHG in ISR, bit#6 in 0x0) from > the bootloader (uboot) when enabling the MAC. The stale interrupts > aren't part of kernel and should be cleared. > > This clears the stale interrupts in ISR (0x0) when enabling the MAC. Thanks Gavin. A question below. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index f00911a..fc2852f 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1119,6 +1119,7 @@ static int ftgmac100_open(struct net_device > *netdev) > { > struct ftgmac100 *priv = netdev_priv(netdev); > struct phy_device *phydev = netdev->phydev; > + unsigned int status; > int err; > > err = ftgmac100_alloc_buffers(priv); > @@ -1145,6 +1146,10 @@ static int ftgmac100_open(struct net_device > *netdev) > ftgmac100_init_hw(priv); > ftgmac100_start_hw(priv, priv->use_ncsi ? 100 : 10); > > + /* Clear stale interrupts */ > + status = ioread32(priv->base + FTGMAC100_OFFSET_ISR); > + iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR); By the time we get down here we've called ftgmac100_reset_hw. This has the following: /* NOTE: reset clears all registers */ iowrite32(FTGMAC100_MACCR_SW_RST, priv->base + FTGMAC100_OFFSET_MACCR); We are seeing that it does not actually clear all the registers? Secondly, if we're seeing stale interrupts is there a risk that they will fire again between now and the call below where we set FTGMAC100_OFFSET_IER? Should we be masking them all off, clearing the stale ones, and then re-enabling them? Cheers, Joel > + > if (phydev) > phy_start(phydev); > else if (priv->use_ncsi) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts 2016-08-01 8:26 ` Joel Stanley @ 2016-08-02 6:53 ` Gavin Shan 0 siblings, 0 replies; 7+ messages in thread From: Gavin Shan @ 2016-08-02 6:53 UTC (permalink / raw) To: Joel Stanley; +Cc: Gavin Shan, openbmc On Mon, Aug 01, 2016 at 05:56:01PM +0930, Joel Stanley wrote: >On Thu, 2016-07-28 at 11:04 +1000, Gavin Shan wrote: >> There is stale interrupt (PHYSTS_CHG in ISR, bit#6 in 0x0) from >> the bootloader (uboot) when enabling the MAC. The stale interrupts >> aren't part of kernel and should be cleared. >> >> This clears the stale interrupts in ISR (0x0) when enabling the MAC. > >Thanks Gavin. A question below. > >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c >> b/drivers/net/ethernet/faraday/ftgmac100.c >> index f00911a..fc2852f 100644 >> --- a/drivers/net/ethernet/faraday/ftgmac100.c >> +++ b/drivers/net/ethernet/faraday/ftgmac100.c >> @@ -1119,6 +1119,7 @@ static int ftgmac100_open(struct net_device >> *netdev) >> { >> struct ftgmac100 *priv = netdev_priv(netdev); >> struct phy_device *phydev = netdev->phydev; >> + unsigned int status; >> int err; >> >> err = ftgmac100_alloc_buffers(priv); >> @@ -1145,6 +1146,10 @@ static int ftgmac100_open(struct net_device >> *netdev) >> ftgmac100_init_hw(priv); >> ftgmac100_start_hw(priv, priv->use_ncsi ? 100 : 10); >> >> + /* Clear stale interrupts */ >> + status = ioread32(priv->base + FTGMAC100_OFFSET_ISR); >> + iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR); > >By the time we get down here we've called ftgmac100_reset_hw. This has >the following: > >/* NOTE: reset clears all registers */ >iowrite32(FTGMAC100_MACCR_SW_RST, priv->base + FTGMAC100_OFFSET_MACCR); > >We are seeing that it does not actually clear all the registers? > Yep, it clears all registers including ISR/IER/MACCR, meaning MACCR[11] is cleared. ISR[9] (PHYSTS_CHG) is set right after it even IER is cleared that time. So ISR[9] cannot be cleared if MACCR[11] isn't set. Note the MACCR[11] is set in PATCH[1/2] in ftgmac100_start_hw(). It also means ISR[9] cannot be cleared successfully until ftgmac100_start_hw() sets MACCR[11]. >Secondly, if we're seeing stale interrupts is there a risk that they >will fire again between now and the call below where we >set FTGMAC100_OFFSET_IER? > >Should we be masking them all off, clearing the stale ones, and then >re-enabling them? > IER is cleared by reset done in ftgmac100_reset_hw(), meaning all interrupts are disabled that time. We're clearing the stale interrupts (mainly ISR[9] - PHYSTS_CHG) before the interrupts are enabled. Thanks, Gavin >Cheers, > >Joel > > >> + >> if (phydev) >> phy_start(phydev); >> else if (priv->use_ncsi) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt 2016-07-28 1:04 [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Gavin Shan 2016-07-28 1:04 ` [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts Gavin Shan @ 2016-08-01 8:27 ` Joel Stanley 2016-08-02 4:45 ` Gavin Shan 1 sibling, 1 reply; 7+ messages in thread From: Joel Stanley @ 2016-08-01 8:27 UTC (permalink / raw) To: Gavin Shan, openbmc On Thu, 2016-07-28 at 11:04 +1000, Gavin Shan wrote: > Bit#11 in MACCR (0x50) designates the signal level for PHY link > status change. It's cleared, meaning high level enabled, by default. > However, we can see continuous interrupt (bit#6) in ISR (0x0) for it > and it's obviously a false alarm. The side effect is CPU cycles > wasted > to process the false alarm. > > This sets bit#11 in MACCR (0x50) to avoid the bogus interrupt. Thanks. I assume you're seeing this with the hardware in NCSI mode? Is the patch okay for other use cases? Cheers, Joel > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 1 + > drivers/net/ethernet/faraday/ftgmac100.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index e805c4d..f00911a 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -224,6 +224,7 @@ static void ftgmac100_init_hw(struct ftgmac100 > *priv) > FTGMAC100_MACCR_RXMAC_EN | \ > FTGMAC100_MACCR_FULLDUP | \ > FTGMAC100_MACCR_CRC_APD | \ > + FTGMAC100_MACCR_PHY_LINK_LEVEL | \ > FTGMAC100_MACCR_RX_RUNT | \ > FTGMAC100_MACCR_RX_BROADPKT) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.h > b/drivers/net/ethernet/faraday/ftgmac100.h > index c258586..d07b6ea 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.h > +++ b/drivers/net/ethernet/faraday/ftgmac100.h > @@ -152,6 +152,7 @@ > #define FTGMAC100_MACCR_FULLDUP (1 << 8) > #define FTGMAC100_MACCR_GIGA_MODE (1 << 9) > #define FTGMAC100_MACCR_CRC_APD (1 << 10) > +#define FTGMAC100_MACCR_PHY_LINK_LEVEL (1 << 11) > #define FTGMAC100_MACCR_RX_RUNT (1 << 12) > #define FTGMAC100_MACCR_JUMBO_LF (1 << 13) > #define FTGMAC100_MACCR_RX_ALL (1 << 14) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt 2016-08-01 8:27 ` [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Joel Stanley @ 2016-08-02 4:45 ` Gavin Shan 2016-08-08 8:27 ` Joel Stanley 0 siblings, 1 reply; 7+ messages in thread From: Gavin Shan @ 2016-08-02 4:45 UTC (permalink / raw) To: Joel Stanley; +Cc: Gavin Shan, openbmc On Mon, Aug 01, 2016 at 05:57:02PM +0930, Joel Stanley wrote: >On Thu, 2016-07-28 at 11:04 +1000, Gavin Shan wrote: >> Bit#11 in MACCR (0x50) designates the signal level for PHY link >> status change. It's cleared, meaning high level enabled, by default. >> However, we can see continuous interrupt (bit#6) in ISR (0x0) for it >> and it's obviously a false alarm. The side effect is CPU cycles >> wasted >> to process the false alarm. >> >> This sets bit#11 in MACCR (0x50) to avoid the bogus interrupt. > >Thanks. > >I assume you're seeing this with the hardware in NCSI mode? > >Is the patch okay for other use cases? > Yeah, the only available testing environment is NCSI. I didn't test it on PHY based AST2500. Joel, please give it a shoot on your AST2500 board before merging it. By the way, the interrupt (PHYSTS_CHG) shouldn't be used even we have a PHY connected as polling mechanism is specified to retrieve the link status in ftgmac100.c Thanks, Gavin >Cheers, > >Joel > >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> drivers/net/ethernet/faraday/ftgmac100.c | 1 + >> drivers/net/ethernet/faraday/ftgmac100.h | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c >> b/drivers/net/ethernet/faraday/ftgmac100.c >> index e805c4d..f00911a 100644 >> --- a/drivers/net/ethernet/faraday/ftgmac100.c >> +++ b/drivers/net/ethernet/faraday/ftgmac100.c >> @@ -224,6 +224,7 @@ static void ftgmac100_init_hw(struct ftgmac100 >> *priv) >> FTGMAC100_MACCR_RXMAC_EN | \ >> FTGMAC100_MACCR_FULLDUP | \ >> FTGMAC100_MACCR_CRC_APD | \ >> + FTGMAC100_MACCR_PHY_LINK_LEVEL | \ >> FTGMAC100_MACCR_RX_RUNT | \ >> FTGMAC100_MACCR_RX_BROADPKT) >> >> diff --git a/drivers/net/ethernet/faraday/ftgmac100.h >> b/drivers/net/ethernet/faraday/ftgmac100.h >> index c258586..d07b6ea 100644 >> --- a/drivers/net/ethernet/faraday/ftgmac100.h >> +++ b/drivers/net/ethernet/faraday/ftgmac100.h >> @@ -152,6 +152,7 @@ >> #define FTGMAC100_MACCR_FULLDUP (1 << 8) >> #define FTGMAC100_MACCR_GIGA_MODE (1 << 9) >> #define FTGMAC100_MACCR_CRC_APD (1 << 10) >> +#define FTGMAC100_MACCR_PHY_LINK_LEVEL (1 << 11) >> #define FTGMAC100_MACCR_RX_RUNT (1 << 12) >> #define FTGMAC100_MACCR_JUMBO_LF (1 << 13) >> #define FTGMAC100_MACCR_RX_ALL (1 << 14) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt 2016-08-02 4:45 ` Gavin Shan @ 2016-08-08 8:27 ` Joel Stanley 0 siblings, 0 replies; 7+ messages in thread From: Joel Stanley @ 2016-08-08 8:27 UTC (permalink / raw) To: Gavin Shan; +Cc: OpenBMC Maillist On Tue, Aug 2, 2016 at 2:15 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > On Mon, Aug 01, 2016 at 05:57:02PM +0930, Joel Stanley wrote: >>On Thu, 2016-07-28 at 11:04 +1000, Gavin Shan wrote: >>> Bit#11 in MACCR (0x50) designates the signal level for PHY link >>> status change. It's cleared, meaning high level enabled, by default. >>> However, we can see continuous interrupt (bit#6) in ISR (0x0) for it >>> and it's obviously a false alarm. The side effect is CPU cycles >>> wasted >>> to process the false alarm. >>> >>> This sets bit#11 in MACCR (0x50) to avoid the bogus interrupt. >> >>Thanks. >> >>I assume you're seeing this with the hardware in NCSI mode? >> >>Is the patch okay for other use cases? >> > > Yeah, the only available testing environment is NCSI. I didn't > test it on PHY based AST2500. Joel, please give it a shoot on > your AST2500 board before merging it. By the way, the interrupt > (PHYSTS_CHG) shouldn't be used even we have a PHY connected as > polling mechanism is specified to retrieve the link status in > ftgmac100.c I gave this a spin on the ast2500 evb. I did get these two messages when bringing the interface up: ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG But from then on the device worked as expected, with no flooding of the kernel buffer. I have applied the patches to my tree. Thanks! Cheers, Joel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-08 8:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-28 1:04 [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Gavin Shan 2016-07-28 1:04 ` [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts Gavin Shan 2016-08-01 8:26 ` Joel Stanley 2016-08-02 6:53 ` Gavin Shan 2016-08-01 8:27 ` [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt Joel Stanley 2016-08-02 4:45 ` Gavin Shan 2016-08-08 8:27 ` Joel Stanley
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.