| Message ID | 20201024121412.10070-1-ioana.ciornei@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: add support for shared interrupts | expand |
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
> - 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
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() ?
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
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
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
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
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. From my point of view, the interrupt handling in phylib has become somewhat confusing with all these callbacks that actually read the same PHY register - the interrupt status. A more streamlined approach would be to just move the responsibility to write an interrupt handler to the driver (as any other device driver does) and make .handle_interrupt() the only way to deal with interrupts. Another advantage with this approach would be that phylib would gain support for shared IRQs between different PHY (not just multi-PHY devices), something which at the moment would require extending every PHY driver anyway in order to implement their .did_interrupt() callback and duplicate the same logic as in .ack_interrupt(). The disadvantage of making .did_interrupt() mandatory would be that we are slightly changing the semantics of the phylib API and that would increase confusion instead of reducing it. What I am proposing is the following: - As a first step, make the .ack_interrupt() callback optional so that we do not break any PHY driver amid the transition. - 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; - Remove each PHY driver's implementation of the .ack_interrupt() by actually taking care of quiescing any pending interrupts before enabling/after disabling the interrupt line. - Finally, after all drivers have been ported, remove the .ack_interrupt() and .did_interrupt() callbacks from phy_driver. 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. I do not have access to most of these PHY's, therefore I Cc-ed the latest contributors to the individual PHY drivers in order to have access, hopefully, to more regression testing. Ioana Ciornei (5): net: phy: export phy_error and phy_trigger_machine net: phy: add a shutdown procedure net: phy: make .ack_interrupt() optional net: phy: at803x: implement generic .handle_interrupt() callback net: phy: at803x: remove the use of .ack_interrupt() drivers/net/phy/at803x.c | 42 ++++++++++++++++++++++++++++++------ drivers/net/phy/phy.c | 6 ++++-- drivers/net/phy/phy_device.c | 10 ++++++++- include/linux/phy.h | 2 ++ 4 files changed, 50 insertions(+), 10 deletions(-) Cc: Alexandru Ardelean <alexandru.ardelean@analog.com> Cc: Andre Edich <andre.edich@microchip.com> Cc: Antoine Tenart <atenart@kernel.org> Cc: Baruch Siach <baruch@tkos.co.il> Cc: Christophe Leroy <christophe.leroy@c-s.fr> Cc: Dan Murphy <dmurphy@ti.com> Cc: Divya Koppera <Divya.Koppera@microchip.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Hauke Mehrtens <hauke@hauke-m.de> Cc: Heiner Kallweit <hkallweit1@gmail.com> Cc: Jerome Brunet <jbrunet@baylibre.com> Cc: Kavya Sree Kotagiri <kavyasree.kotagiri@microchip.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Marco Felsch <m.felsch@pengutronix.de> Cc: Marek Vasut <marex@denx.de> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Cc: Mathias Kresin <dev@kresin.me> Cc: Maxim Kochetkov <fido_max@inbox.ru> Cc: Michael Walle <michael@walle.cc> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Nisar Sayed <Nisar.Sayed@microchip.com> Cc: Oleksij Rempel <o.rempel@pengutronix.de> Cc: Philippe Schenker <philippe.schenker@toradex.com> Cc: Willy Liu <willy.liu@realtek.com> Cc: Yuiko Oshino <yuiko.oshino@microchip.com>