Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1487086 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs Joel Stanley <joel@jms.id.au> - 2016-09-20 08:40 +0200
    Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5  SoCs Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-09-20 14:20 +0200
      Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5  SoCs Andrew Lunn <andrew@lunn.ch> - 2016-09-20 17:30 +0200
        Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs Joel Stanley <joel@jms.id.au> - 2016-09-21 04:10 +0200
          Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5  SoCs Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-09-21 11:10 +0200
            Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5 SoCs Joel Stanley <joel@jms.id.au> - 2016-09-21 11:20 +0200
              Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5  SoCs Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-09-21 12:50 +0200
    Re: [PATCH net-next 6/7] net/faraday: Fix phy link irq on Aspeed G5  SoCs Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-09-20 14:20 +0200

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

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

 [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
 [   20.300000] ftgmac100 1e660000.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 <joel@jms.id.au>
---
 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;
+	}
+
 	switch (speed) {
 	default:
 	case 10:
-- 
2.9.3

[toc] | [next] | [standalone]

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

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.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.300000] ftgmac100 1e660000.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.

[toc] | [prev] | [next] | [standalone]

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

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.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
> >  [   20.300000] ftgmac100 1e660000.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

[toc] | [prev] | [next] | [standalone]

#1487764

On Wed, Sep 21, 2016 at 12:59 AM, Andrew Lunn <andrew@lunn.ch> 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.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>> >  [   20.300000] ftgmac100 1e660000.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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1487940

On Wed, Sep 21, 2016 at 6:33 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.280000] ftgmac100 1e660000.ethernet eth0: [ISR] = 0x200: PHYSTS_CHG
>  [   20.300000] ftgmac100 1e660000.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 <joel@jms.id.au>
> ---
>  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web