From: Ioana Ciornei <[email protected]>
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 & irq_mask))
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 patch set is part 2 of the entire change set and it addresses the
changes needed in 9 PHY drivers. The rest can be found on my Github
branch here:
https://github.com/IoanaCiornei/linux/commits/phylib-shared-irq
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 (18):
net: phy: vitesse: implement generic .handle_interrupt() callback
net: phy: vitesse: remove the use of .ack_interrupt()
net: phy: microchip: implement generic .handle_interrupt() callback
net: phy: microchip: remove the use of .ack_interrupt()
net: phy: marvell: implement generic .handle_interrupt() callback
net: phy: marvell: remove the use of .ack_interrupt()
net: phy: lxt: implement generic .handle_interrupt() callback
net: phy: lxt: remove the use of .ack_interrupt()
net: phy: nxp-tja11xx: implement generic .handle_interrupt() callback
net: phy: nxp-tja11xx: remove the use of .ack_interrupt()
net: phy: amd: implement generic .handle_interrupt() callback
net: phy: amd: remove the use of .ack_interrupt()
net: phy: smsc: implement generic .handle_interrupt() callback
net: phy: smsc: remove the use of .ack_interrupt()
net: phy: ste10Xp: implement generic .handle_interrupt() callback
net: phy: ste10Xp: remove the use of .ack_interrupt()
net: phy: adin: implement generic .handle_interrupt() callback
net: phy: adin: remove the use of the .ack_interrupt()
drivers/net/phy/adin.c | 45 +++++++++++++---
drivers/net/phy/amd.c | 37 +++++++++++--
drivers/net/phy/lxt.c | 94 ++++++++++++++++++++++++++++++----
drivers/net/phy/marvell.c | 88 ++++++++++++++++---------------
drivers/net/phy/microchip.c | 24 +++++++--
drivers/net/phy/microchip_t1.c | 28 +++++++---
drivers/net/phy/nxp-tja11xx.c | 42 +++++++++++++--
drivers/net/phy/smsc.c | 55 ++++++++++++++++----
drivers/net/phy/ste10Xp.c | 53 +++++++++++++------
drivers/net/phy/vitesse.c | 61 ++++++++++++++--------
10 files changed, 405 insertions(+), 122 deletions(-)
Cc: Alexandru Ardelean <[email protected]>
Cc: Andre Edich <[email protected]>
Cc: Baruch Siach <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Kavya Sree Kotagiri <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Marco Felsch <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Maxim Kochetkov <[email protected]>
Cc: Nisar Sayed <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: Robert Hancock <[email protected]>
Cc: Yuiko Oshino <[email protected]>
--
2.28.0
From: Ioana Ciornei <[email protected]>
In preparation of removing the .ack_interrupt() callback, we must replace
its occurrences (aka phy_clear_interrupt), from the 2 places where it is
called from (phy_enable_interrupts and phy_disable_interrupts), with
equivalent functionality.
This means that clearing interrupts now becomes something that the PHY
driver is responsible of doing, before enabling interrupts and after
clearing them. Make this driver follow the new contract.
Cc: Nisar Sayed <[email protected]>
Cc: Yuiko Oshino <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/microchip.c | 13 +++++--------
drivers/net/phy/microchip_t1.c | 17 +++++++----------
2 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
index b472a2149f08..9f1f2b6c97d4 100644
--- a/drivers/net/phy/microchip.c
+++ b/drivers/net/phy/microchip.c
@@ -44,14 +44,12 @@ static int lan88xx_phy_config_intr(struct phy_device *phydev)
LAN88XX_INT_MASK_LINK_CHANGE_);
} else {
rc = phy_write(phydev, LAN88XX_INT_MASK, 0);
- }
-
- return rc < 0 ? rc : 0;
-}
+ if (rc)
+ return rc;
-static int lan88xx_phy_ack_interrupt(struct phy_device *phydev)
-{
- int rc = phy_read(phydev, LAN88XX_INT_STS);
+ /* Ack interrupts after they have been disabled */
+ rc = phy_read(phydev, LAN88XX_INT_STS);
+ }
return rc < 0 ? rc : 0;
}
@@ -358,7 +356,6 @@ static struct phy_driver microchip_phy_driver[] = {
.config_init = lan88xx_config_init,
.config_aneg = lan88xx_config_aneg,
- .ack_interrupt = lan88xx_phy_ack_interrupt,
.config_intr = lan88xx_phy_config_intr,
.handle_interrupt = lan88xx_handle_interrupt,
diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index 04cda8865deb..4dc00bd5a8d2 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -189,16 +189,14 @@ static int lan87xx_phy_config_intr(struct phy_device *phydev)
rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0x7FFF);
rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
val = LAN87XX_MASK_LINK_UP | LAN87XX_MASK_LINK_DOWN;
- }
-
- rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
-
- return rc < 0 ? rc : 0;
-}
+ rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
+ } else {
+ rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
+ if (rc)
+ return rc;
-static int lan87xx_phy_ack_interrupt(struct phy_device *phydev)
-{
- int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
+ rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
+ }
return rc < 0 ? rc : 0;
}
@@ -238,7 +236,6 @@ static struct phy_driver microchip_t1_phy_driver[] = {
.config_init = lan87xx_config_init,
- .ack_interrupt = lan87xx_phy_ack_interrupt,
.config_intr = lan87xx_phy_config_intr,
.handle_interrupt = lan87xx_handle_interrupt,
--
2.28.0
From: Ioana Ciornei <[email protected]>
In preparation of removing the .ack_interrupt() callback, we must replace
its occurrences (aka phy_clear_interrupt), from the 2 places where it is
called from (phy_enable_interrupts and phy_disable_interrupts), with
equivalent functionality.
This means that clearing interrupts now becomes something that the PHY
driver is responsible of doing, before enabling interrupts and after
clearing them. Make this driver follow the new contract.
Cc: Maxim Kochetkov <[email protected]>
Cc: Baruch Siach <[email protected]>
Cc: Robert Hancock <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/marvell.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb843b960436..587930a7f48b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -317,12 +317,21 @@ static int marvell_config_intr(struct phy_device *phydev)
{
int err;
- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = marvell_ack_interrupt(phydev);
+ if (err)
+ return err;
+
err = phy_write(phydev, MII_M1011_IMASK,
MII_M1011_IMASK_INIT);
- else
+ } else {
err = phy_write(phydev, MII_M1011_IMASK,
MII_M1011_IMASK_CLEAR);
+ if (err)
+ return err;
+
+ err = marvell_ack_interrupt(phydev);
+ }
return err;
}
@@ -2703,7 +2712,6 @@ static struct phy_driver marvell_drivers[] = {
.probe = marvell_probe,
.config_init = marvell_config_init,
.config_aneg = m88e1101_config_aneg,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2722,7 +2730,6 @@ static struct phy_driver marvell_drivers[] = {
.probe = marvell_probe,
.config_init = m88e1111_config_init,
.config_aneg = marvell_config_aneg,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2744,7 +2751,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e1111_config_init,
.config_aneg = m88e1111_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2766,7 +2772,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e1111_config_init,
.config_aneg = m88e1111_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2787,7 +2792,6 @@ static struct phy_driver marvell_drivers[] = {
.probe = marvell_probe,
.config_init = m88e1118_config_init,
.config_aneg = m88e1118_config_aneg,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2807,7 +2811,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e1121_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2829,7 +2832,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e1318_config_init,
.config_aneg = m88e1318_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.get_wol = m88e1318_get_wol,
@@ -2851,7 +2853,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e1145_config_init,
.config_aneg = m88e1101_config_aneg,
.read_status = genphy_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2872,7 +2873,6 @@ static struct phy_driver marvell_drivers[] = {
.probe = marvell_probe,
.config_init = m88e1149_config_init,
.config_aneg = m88e1118_config_aneg,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2891,7 +2891,6 @@ static struct phy_driver marvell_drivers[] = {
.probe = marvell_probe,
.config_init = m88e1111_config_init,
.config_aneg = marvell_config_aneg,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2909,7 +2908,6 @@ static struct phy_driver marvell_drivers[] = {
/* PHY_GBIT_FEATURES */
.probe = marvell_probe,
.config_init = m88e1116r_config_init,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2932,7 +2930,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e1510_config_init,
.config_aneg = m88e1510_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.get_wol = m88e1318_get_wol,
@@ -2961,7 +2958,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e1510_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -2987,7 +2983,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e1510_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -3012,7 +3007,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = m88e3016_config_init,
.aneg_done = marvell_aneg_done,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -3033,7 +3027,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e6390_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -3058,7 +3051,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e1510_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
@@ -3080,7 +3072,6 @@ static struct phy_driver marvell_drivers[] = {
.config_init = marvell_config_init,
.config_aneg = m88e1510_config_aneg,
.read_status = marvell_read_status,
- .ack_interrupt = marvell_ack_interrupt,
.config_intr = marvell_config_intr,
.handle_interrupt = marvell_handle_interrupt,
.resume = genphy_resume,
--
2.28.0
From: Ioana Ciornei <[email protected]>
In preparation of removing the .ack_interrupt() callback, we must replace
its occurrences (aka phy_clear_interrupt), from the 2 places where it is
called from (phy_enable_interrupts and phy_disable_interrupts), with
equivalent functionality.
This means that clearing interrupts now becomes something that the PHY
driver is responsible of doing, before enabling interrupts and after
clearing them. Make this driver follow the new contract.
Cc: Kavya Sree Kotagiri <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/vitesse.c | 24 +++---------------------
1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 9f6cd6ec9747..16704e243162 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -275,25 +275,14 @@ static int vsc8601_config_init(struct phy_device *phydev)
return 0;
}
-static int vsc824x_ack_interrupt(struct phy_device *phydev)
-{
- int err = 0;
-
- /* Don't bother to ACK the interrupts if interrupts
- * are disabled. The 824x cannot clear the interrupts
- * if they are disabled.
- */
- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
- err = phy_read(phydev, MII_VSC8244_ISTAT);
-
- return (err < 0) ? err : 0;
-}
-
static int vsc82xx_config_intr(struct phy_device *phydev)
{
int err;
if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ /* Don't bother to ACK the interrupts since the 824x cannot
+ * clear the interrupts if they are disabled.
+ */
err = phy_write(phydev, MII_VSC8244_IMASK,
(phydev->drv->phy_id == PHY_ID_VSC8234 ||
phydev->drv->phy_id == PHY_ID_VSC8244 ||
@@ -420,7 +409,6 @@ static struct phy_driver vsc82xx_driver[] = {
/* PHY_GBIT_FEATURES */
.config_init = &vsc824x_config_init,
.config_aneg = &vsc82x4_config_aneg,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -430,7 +418,6 @@ static struct phy_driver vsc82xx_driver[] = {
/* PHY_GBIT_FEATURES */
.config_init = &vsc824x_config_init,
.config_aneg = &vsc82x4_config_aneg,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -440,7 +427,6 @@ static struct phy_driver vsc82xx_driver[] = {
/* PHY_GBIT_FEATURES */
.config_init = &vsc824x_config_init,
.config_aneg = &vsc82x4_config_aneg,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -449,7 +435,6 @@ static struct phy_driver vsc82xx_driver[] = {
.phy_id_mask = 0x000ffff0,
/* PHY_GBIT_FEATURES */
.config_init = &vsc8601_config_init,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -495,7 +480,6 @@ static struct phy_driver vsc82xx_driver[] = {
/* PHY_GBIT_FEATURES */
.config_init = &vsc824x_config_init,
.config_aneg = &vsc82x4_config_aneg,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -505,7 +489,6 @@ static struct phy_driver vsc82xx_driver[] = {
.name = "Vitesse VSC8221",
/* PHY_GBIT_FEATURES */
.config_init = &vsc8221_config_init,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
}, {
@@ -515,7 +498,6 @@ static struct phy_driver vsc82xx_driver[] = {
.name = "Vitesse VSC8211",
/* PHY_GBIT_FEATURES */
.config_init = &vsc8221_config_init,
- .ack_interrupt = &vsc824x_ack_interrupt,
.config_intr = &vsc82xx_config_intr,
.handle_interrupt = &vsc82xx_handle_interrupt,
} };
--
2.28.0
On Thu, 12 Nov 2020 17:54:55 +0200 Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> 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.
Ioana, would you mind resending? Looks like the kernel.org patchwork
instance is way less reliable at piecing series together :(
On Thu, Nov 12, 2020 at 06:19:10PM -0800, Jakub Kicinski wrote:
> On Thu, 12 Nov 2020 17:54:55 +0200 Ioana Ciornei wrote:
> > From: Ioana Ciornei <[email protected]>
> >
> > 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.
>
> Ioana, would you mind resending? Looks like the kernel.org patchwork
> instance is way less reliable at piecing series together :(
No problem, I'll resend.
So from now on we'll be using the kernel.org patchwork only, right? I
am asking this because from what I can tell the ozlabs patchwork was not
updated in the last days.
Ioana
On Fri, 13 Nov 2020 08:17:00 +0000 Ioana Ciornei wrote:
> On Thu, Nov 12, 2020 at 06:19:10PM -0800, Jakub Kicinski wrote:
> > On Thu, 12 Nov 2020 17:54:55 +0200 Ioana Ciornei wrote:
> > > From: Ioana Ciornei <[email protected]>
> > >
> > > 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.
> >
> > Ioana, would you mind resending? Looks like the kernel.org patchwork
> > instance is way less reliable at piecing series together :(
>
> No problem, I'll resend.
>
> So from now on we'll be using the kernel.org patchwork only, right? I
> am asking this because from what I can tell the ozlabs patchwork was not
> updated in the last days.
Yup, that's the plan. We're keeping ozlabs going for a bit just in case
kernel.org is acting up (like if the need to repost stuff persists),
but the plan is to stop the patchwork entry for netdev on ozlabs once
things settle.
From where I live ozlabs takes 1s to respond to a request while
kernel.org instance takes 0.2s.