Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Benjamin Herrenschmidt]

On Wed, 2016-09-21 at 18:48 +0930, Joel Stanley wrote:
> > What line is it out of the PHY ? The PHY IRQ ? If yes then it's meant
> > to be telling you to go look at the PHY registers for a link status
> > change, but only works if the PHY has also been configured
> > appropriately...
> 
> Yep, PHY IRQ.
> 
> > Mostly we ignore those things in Linux and just poll the PHY.
> 
> That's simpler. It's what we're doing on Aspeed systems when using NSCI 
> already.
> 
> The driver is already polling the PHY, I propose we mask out this
> interrupt for all systems. I gave that a run on my ast2500evb and it
> behaved itself.

Right it's the easiest way.

If we want to use interrupts without polling in order to maybe
insignificantly reduce the load in the system, we should then make sure
we configure the IRQ with a matching polarity between the PHY and the
NIC *and* in a way that matches the pull up/down's on the board.

The above is possible but tricky as all the parts have to be right, and
the IRQ configuration of PHYs is basically PHY model specific.

So I'd start with just masking it out.

Cheers,
Ben.

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Joel Stanley]

On Wed, Sep 21, 2016 at 6:33 PM, Benjamin Herrenschmidt
 wrote:
> On Wed, 2016-09-21 at 11:32 +0930, Joel Stanley wrote:
>> I had a look at the eval board schematic and it appears that the line
>> has pull down resistors on it, explaining why the IRQ fires when it's
>> configured to active low. Other machines re-use the pin pin as a GPIO.
>> So yes, I will change this to a dt property in v2. That will mean
>> dropping 4/7 "net/faraday: Avoid PHYSTS_CHG interrupt" as well.
>
> What line is it out of the PHY ? The PHY IRQ ? If yes then it's meant
> to be telling you to go look at the PHY registers for a link status
> change, but only works if the PHY has also been configured
> appropriately...

Yep, PHY IRQ.

> Mostly we ignore those things in Linux and just poll the PHY.

That's simpler. It's what we're doing on Aspeed systems when using NSCI already.

The driver is already polling the PHY, I propose we mask out this
interrupt for all systems. I gave that a run on my ast2500evb and it
behaved itself.

Cheers,

Joe

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Benjamin Herrenschmidt]

On Wed, 2016-09-21 at 11:32 +0930, Joel Stanley wrote:
> I had a look at the eval board schematic and it appears that the line
> has pull down resistors on it, explaining why the IRQ fires when it's
> configured to active low. Other machines re-use the pin pin as a GPIO.
> So yes, I will change this to a dt property in v2. That will mean
> dropping 4/7 "net/faraday: Avoid PHYSTS_CHG interrupt" as well.

What line is it out of the PHY ? The PHY IRQ ? If yes then it's meant
to be telling you to go look at the PHY registers for a link status
change, but only works if the PHY has also been configured
appropriately...

Mostly we ignore those things in Linux and just poll the PHY.

Cheers,
Ben.

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Joel Stanley]

On Wed, Sep 21, 2016 at 12:59 AM, Andrew Lunn  wrote:
> On Tue, Sep 20, 2016 at 10:13:14PM +1000, Benjamin Herrenschmidt wrote:
>> On Tue, 2016-09-20 at 16:00 +0930, Joel Stanley wrote:
>> > On Aspeed SoC with a direct PHY connection (non-NSCI), we receive
>> > continual PHYSTS interrupts:
>> >
>> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.30] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >
>> > This is because the driver was enabling low-level sensitive interrupt
>> > generation where the systems are wired for high-level. All CPU cycles
>> > are spent servicing this interrupt.
>>
>> If this is a system wiring issue, should it be represented by a DT
>> property ?
>
> Is there a device tree binding document somewhere?
>
> Is it possible just to put ACTIVE_HIGH in the right place in the
> binding?

I wrote "wired for high level" wrt the SoC internals. To be honest I
wondered the same thing but it's hard with only one (non-NSCI) system
to test on.

I had a look at the eval board schematic and it appears that the line
has pull down resistors on it, explaining why the IRQ fires when it's
configured to active low. Other machines re-use the pin pin as a GPIO.
So yes, I will change this to a dt property in v2. That will mean
dropping 4/7 "net/faraday: Avoid PHYSTS_CHG interrupt" as well.

Cheers,

Joel

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Andrew Lunn]

On Tue, Sep 20, 2016 at 10:13:14PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2016-09-20 at 16:00 +0930, Joel Stanley wrote:
> > On Aspeed SoC with a direct PHY connection (non-NSCI), we receive
> > continual PHYSTS interrupts:
> > 
> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.30] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> > 
> > This is because the driver was enabling low-level sensitive interrupt
> > generation where the systems are wired for high-level. All CPU cycles
> > are spent servicing this interrupt.
> 
> If this is a system wiring issue, should it be represented by a DT
> property ?

Is there a device tree binding document somewhere?

Is it possible just to put ACTIVE_HIGH in the right place in the
binding?

Andrew

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Sergei Shtylyov]

Hello.

On 9/20/2016 9:30 AM, Joel Stanley wrote:


On Aspeed SoC with a direct PHY connection (non-NSCI), we receive
continual PHYSTS interrupts:

 [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.30] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG

This is because the driver was enabling low-level sensitive interrupt
generation where the systems are wired for high-level. All CPU cycles
are spent servicing this interrupt.

Signed-off-by: Joel Stanley 
---
 drivers/net/ethernet/faraday/ftgmac100.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 7ba0f2d58a8b..5466df028381 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -223,6 +223,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv, int 
speed)
 {
int maccr = MACCR_ENABLE_ALL;

+   if (of_machine_is_compatible("aspeed,ast2500")) {
+   maccr &= ~FTGMAC100_MACCR_PHY_LINK_LEVEL;
+   }


   {} not needed here.

[...]

MBR, Sergei

Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs

[Benjamin Herrenschmidt]

On Tue, 2016-09-20 at 16:00 +0930, Joel Stanley wrote:
> On Aspeed SoC with a direct PHY connection (non-NSCI), we receive
> continual PHYSTS interrupts:
> 
>  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.28] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.30] ftgmac100 1e66.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> 
> This is because the driver was enabling low-level sensitive interrupt
> generation where the systems are wired for high-level. All CPU cycles
> are spent servicing this interrupt.

If this is a system wiring issue, should it be represented by a DT
property ?

Cheers,
Ben.