[PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt

[PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt

[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

[PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts

[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

Re: [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts

[Joel Stanley]

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)

Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt

[Joel Stanley]

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)

Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt

[Gavin Shan]

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

Re: [PATCH dev-4.7 2/2] net/faraday: Clear stale interrupts

[Gavin Shan]

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

Re: [PATCH dev-4.7 1/2] net/faraday: Avoid PHYSTS_CHG interrupt

[Joel Stanley]

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