mbox series

[RFC,net-next,0/5] net: phy: add support for shared interrupts

Message ID 20201024121412.10070-1-ioana.ciornei@nxp.com
Headers show
Series net: phy: add support for shared interrupts | expand

Message

Ioana Ciornei Oct. 24, 2020, 12:14 p.m. UTC 
This patch set aims to actually add support for shared interrupts in
phylib and not only for multi-PHY devices. While we are at it,
streamline the interrupt handling in phylib.

For a bit of context, at the moment, there are multiple phy_driver ops
that deal with this subject:

- .config_intr() - Enable/disable the interrupt line.

- .ack_interrupt() - Should quiesce any interrupts that may have been
  fired.  It's also used by phylib in conjunction with .config_intr() to
  clear any pending interrupts after the line was disabled, and before
  it is going to be enabled.

- .did_interrupt() - Intended for multi-PHY devices with a shared IRQ
  line and used by phylib to discern which PHY from the package was the
  one that actually fired the interrupt.

- .handle_interrupt() - Completely overrides the default interrupt
  handling logic from phylib. The PHY driver is responsible for checking
  if any interrupt was fired by the respective PHY and choose
  accordingly if it's the one that should trigger the link state machine.

Comments

Ioana Ciornei Oct. 24, 2020, 2:09 p.m. UTC | #1 
On Sat, Oct 24, 2020 at 03:14:07PM +0300, Ioana Ciornei wrote:

> This RFC just contains the patches for phylib and a single driver -
> Atheros. The rest can be found on my Github branch here: TODO
> They will be submitted as a multi-part series once the merge window
> closes.
> 

It seems that I forgot to add a link to the Github branch. Here it is:

https://github.com/IoanaCiornei/linux/commits/phylib-shared-irq
Andrew Lunn Oct. 24, 2020, 5:17 p.m. UTC | #2 
> - Every PHY driver gains a .handle_interrupt() implementation that, for
>   the most part, would look like below:
> 
> 	irq_status = phy_read(phydev, INTR_STATUS);
> 	if (irq_status < 0) {
> 		phy_error(phydev);
> 		return IRQ_NONE;
> 	}
> 
> 	if (irq_status == 0)
> 		return IRQ_NONE;
> 
> 	phy_trigger_machine(phydev);
> 
> 	return IRQ_HANDLED;

Hi Ioana

It looks like phy_trigger_machine(phydev) could be left in the core,
phy_interrupt(). It just needs to look at the return code, IRQ_HANDLED
means trigger the state machine.

      Andrew
Russell King (Oracle) Oct. 24, 2020, 6:09 p.m. UTC | #3 
On Sat, Oct 24, 2020 at 07:17:05PM +0200, Andrew Lunn wrote:
> > - Every PHY driver gains a .handle_interrupt() implementation that, for

> >   the most part, would look like below:

> > 

> > 	irq_status = phy_read(phydev, INTR_STATUS);

> > 	if (irq_status < 0) {

> > 		phy_error(phydev);

> > 		return IRQ_NONE;

> > 	}

> > 

> > 	if (irq_status == 0)

> > 		return IRQ_NONE;

> > 

> > 	phy_trigger_machine(phydev);

> > 

> > 	return IRQ_HANDLED;

> 

> Hi Ioana

> 

> It looks like phy_trigger_machine(phydev) could be left in the core,

> phy_interrupt(). It just needs to look at the return code, IRQ_HANDLED

> means trigger the state machine.


Is this appropriate for things such as the existing user of
handle_interrupt - vsc8584_handle_interrupt() ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Ioana Ciornei Oct. 24, 2020, 6:19 p.m. UTC | #4 
On Sat, Oct 24, 2020 at 07:17:05PM +0200, Andrew Lunn wrote:
> > - Every PHY driver gains a .handle_interrupt() implementation that, for

> >   the most part, would look like below:

> > 

> > 	irq_status = phy_read(phydev, INTR_STATUS);

> > 	if (irq_status < 0) {

> > 		phy_error(phydev);

> > 		return IRQ_NONE;

> > 	}

> > 

> > 	if (irq_status == 0)

> > 		return IRQ_NONE;

> > 

> > 	phy_trigger_machine(phydev);

> > 

> > 	return IRQ_HANDLED;

> 

> Hi Ioana

> 

> It looks like phy_trigger_machine(phydev) could be left in the core,

> phy_interrupt(). It just needs to look at the return code, IRQ_HANDLED

> means trigger the state machine.


I tend to disagree that this would bring us any benefit.

Keeping the phy_trigger_machine() inside the phy_interrupt() would mean
that we are changing the convention of what the implementation of
.handle_interrupt() should do.

At the moment, there are drivers which use it to handle multiple
interrupt sources within the same PHY device (e.g. MACSEC, 1588, link
state). With your suggestion, when a MACSEC interrupt is received, the
PHY driver would be forced to return IRQ_NONE just so phylib does not
trigger the link state machine. I think this would eventually lead to
some "irq X: nobody cared".

Also, the vsc8584_handle_interrupt() already calls a wrapper over
phy_trigger_machine() called phy_mac_interrupt() which was intended for
MAC driver use only.

Ioana
Andrew Lunn Oct. 24, 2020, 8:07 p.m. UTC | #5 
On Sat, Oct 24, 2020 at 07:09:53PM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Oct 24, 2020 at 07:17:05PM +0200, Andrew Lunn wrote:
> > > - Every PHY driver gains a .handle_interrupt() implementation that, for
> > >   the most part, would look like below:
> > > 
> > > 	irq_status = phy_read(phydev, INTR_STATUS);
> > > 	if (irq_status < 0) {
> > > 		phy_error(phydev);
> > > 		return IRQ_NONE;
> > > 	}
> > > 
> > > 	if (irq_status == 0)
> > > 		return IRQ_NONE;
> > > 
> > > 	phy_trigger_machine(phydev);
> > > 
> > > 	return IRQ_HANDLED;
> > 
> > Hi Ioana
> > 
> > It looks like phy_trigger_machine(phydev) could be left in the core,
> > phy_interrupt(). It just needs to look at the return code, IRQ_HANDLED
> > means trigger the state machine.
> 
> Is this appropriate for things such as the existing user of
> handle_interrupt - vsc8584_handle_interrupt() ?

Ah, yes, you are likely to get a lot more ptp interrupts than link
up/down interrupts, and there is no point running the phy state
machine after each ptp interrupt. So Ioana's structure is better.

And now that phy_trigger_machine is exported, that driver can swap
from phy_mac_interrupt() to phy_trigger_machine().

	Andrew
Michael Walle Oct. 25, 2020, 8:17 a.m. UTC | #6 
Am 2020-10-24 14:14, schrieb Ioana Ciornei:
> - Every PHY driver gains a .handle_interrupt() implementation that, for

>   the most part, would look like below:

> 

> 	irq_status = phy_read(phydev, INTR_STATUS);

> 	if (irq_status < 0) {

> 		phy_error(phydev);

> 		return IRQ_NONE;

> 	}

> 

> 	if (irq_status == 0)

> 		return IRQ_NONE;

> 

> 	phy_trigger_machine(phydev);

> 

> 	return IRQ_HANDLED;


Would it make sense to provide this (default) version inside the core?
Simple PHY drivers then just could set the callback to this function.
(There must be some property for the INTR_STATUS, which is likely to
be different between different PHYs, though).

-michael
Ioana Ciornei Oct. 25, 2020, 10:18 a.m. UTC | #7 
On Sun, Oct 25, 2020 at 09:17:58AM +0100, Michael Walle wrote:
> Am 2020-10-24 14:14, schrieb Ioana Ciornei:
> > - Every PHY driver gains a .handle_interrupt() implementation that, for
> >   the most part, would look like below:
> > 
> > 	irq_status = phy_read(phydev, INTR_STATUS);
> > 	if (irq_status < 0) {
> > 		phy_error(phydev);
> > 		return IRQ_NONE;
> > 	}
> > 
> > 	if (irq_status == 0)
> > 		return IRQ_NONE;
> > 
> > 	phy_trigger_machine(phydev);
> > 
> > 	return IRQ_HANDLED;
> 
> Would it make sense to provide this (default) version inside the core?
> Simple PHY drivers then just could set the callback to this function.
> (There must be some property for the INTR_STATUS, which is likely to
> be different between different PHYs, though).

Yes, the interrupt status register's address differs even between PHYs
from the same vendor so making this somehow into a default handler would
mean to add even another callback that actually reads the register.

For simple PHY drivers, the .handle_interrupt() implementation would
mean 15-20 lines of code for a much more straightforward implementation.
I think that's fair.

Ioana