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 3 (and final) of the entire change set and it
addresses the remaining PHY drivers that have not been migrated
previosly. Also, it finally removed the .did_interrupt() and
.ack_interrupt() callbacks since they are of no use anymore.
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 (15):
net: phy: intel-xway: implement generic .handle_interrupt() callback
net: phy: intel-xway: remove the use of .ack_interrupt()
net: phy: icplus: implement generic .handle_interrupt() callback
net: phy: icplus: remove the use .ack_interrupt()
net: phy: meson-gxl: implement generic .handle_interrupt() callback
net: phy: meson-gxl: remove the use of .ack_callback()
net: phy: micrel: implement generic .handle_interrupt() callback
net: phy: micrel: remove the use of .ack_interrupt()
net: phy: national: implement generic .handle_interrupt() callback
net: phy: national: remove the use of the .ack_interrupt()
net: phy: ti: implement generic .handle_interrupt() callback
net: phy: ti: remove the use of .ack_interrupt()
net: phy: qsemi: implement generic .handle_interrupt() callback
net: phy: qsemi: remove the use of .ack_interrupt()
net: phy: remove the .did_interrupt() and .ack_interrupt() callback
drivers/net/phy/dp83640.c | 38 ++++++++++++++++++-
drivers/net/phy/dp83822.c | 54 ++++++++++++++++++---------
drivers/net/phy/dp83848.c | 47 +++++++++++++++++++++++-
drivers/net/phy/dp83867.c | 44 +++++++++++++++++++---
drivers/net/phy/dp83869.c | 42 +++++++++++++++++++--
drivers/net/phy/dp83tc811.c | 53 ++++++++++++++++++++++++++-
drivers/net/phy/icplus.c | 58 +++++++++++++++++++----------
drivers/net/phy/intel-xway.c | 71 +++++++++++++++++++++---------------
drivers/net/phy/meson-gxl.c | 37 +++++++++++++++----
drivers/net/phy/micrel.c | 65 +++++++++++++++++++++++++--------
drivers/net/phy/national.c | 58 ++++++++++++++++++++++-------
drivers/net/phy/phy.c | 48 +-----------------------
drivers/net/phy/phy_device.c | 2 +-
drivers/net/phy/qsemi.c | 42 +++++++++++++++++++--
include/linux/phy.h | 19 ++--------
15 files changed, 497 insertions(+), 181 deletions(-)
Cc: Antoine Tenart <[email protected]>
Cc: Dan Murphy <[email protected]>
Cc: Divya Koppera <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Mathias Kresin <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: Philippe Schenker <[email protected]>
--
2.28.0
From: Ioana Ciornei <[email protected]>
In an attempt to actually support shared IRQs in phylib, we now move the
responsibility of triggering the phylib state machine or just returning
IRQ_NONE, based on the IRQ status register, to the PHY driver. Having
3 different IRQ handling callbacks (.handle_interrupt(),
.did_interrupt() and .ack_interrupt() ) is confusing so let the PHY
driver implement directly an IRQ handler like any other device driver.
Make this driver follow the new convention.
Cc: Martin Blumenstingl <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/icplus.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index d6e8516cd146..a74ff45fa99c 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -285,16 +285,24 @@ static int ip101a_g_config_intr(struct phy_device *phydev)
return phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
}
-static int ip101a_g_did_interrupt(struct phy_device *phydev)
+static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
{
- int val = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+ int irq_status;
- if (val < 0)
- return 0;
+ irq_status = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (!(irq_status & (IP101A_G_IRQ_SPEED_CHANGE |
+ IP101A_G_IRQ_DUPLEX_CHANGE |
+ IP101A_G_IRQ_LINK_CHANGE)))
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
- return val & (IP101A_G_IRQ_SPEED_CHANGE |
- IP101A_G_IRQ_DUPLEX_CHANGE |
- IP101A_G_IRQ_LINK_CHANGE);
+ return IRQ_HANDLED;
}
static int ip101a_g_ack_interrupt(struct phy_device *phydev)
@@ -332,8 +340,8 @@ static struct phy_driver icplus_driver[] = {
/* PHY_BASIC_FEATURES */
.probe = ip101a_g_probe,
.config_intr = ip101a_g_config_intr,
- .did_interrupt = ip101a_g_did_interrupt,
.ack_interrupt = ip101a_g_ack_interrupt,
+ .handle_interrupt = ip101a_g_handle_interrupt,
.config_init = &ip101a_g_config_init,
.suspend = genphy_suspend,
.resume = genphy_resume,
--
2.28.0
On Mon, 23 Nov 2020 23:13:11 +0100 Martin Blumenstingl wrote:
> > Ioana Ciornei (15):
> > net: phy: intel-xway: implement generic .handle_interrupt() callback
> > net: phy: intel-xway: remove the use of .ack_interrupt()
> > net: phy: icplus: implement generic .handle_interrupt() callback
> > net: phy: icplus: remove the use .ack_interrupt()
> > net: phy: meson-gxl: implement generic .handle_interrupt() callback
> > net: phy: meson-gxl: remove the use of .ack_callback()
> I will check the six patches above on Saturday (due to me being very
> busy with my daytime job)
> if that's too late for the netdev maintainers then I'm not worried
> about it. at first glance this looks fine to me. and we can always fix
> things afterwards (but still before -rc1).
That is a little long for patches to be hanging around. I was planning
to apply these on Wed. If either Ioana or you would prefer to get the
testing performed first, please split those patches out and repost once
they get validated.
On Mon, Nov 23, 2020 at 02:37:13PM -0800, Jakub Kicinski wrote:
> On Mon, 23 Nov 2020 23:13:11 +0100 Martin Blumenstingl wrote:
> > > Ioana Ciornei (15):
> > > net: phy: intel-xway: implement generic .handle_interrupt() callback
> > > net: phy: intel-xway: remove the use of .ack_interrupt()
> > > net: phy: icplus: implement generic .handle_interrupt() callback
> > > net: phy: icplus: remove the use .ack_interrupt()
> > > net: phy: meson-gxl: implement generic .handle_interrupt() callback
> > > net: phy: meson-gxl: remove the use of .ack_callback()
> > I will check the six patches above on Saturday (due to me being very
> > busy with my daytime job)
> > if that's too late for the netdev maintainers then I'm not worried
> > about it. at first glance this looks fine to me. and we can always fix
> > things afterwards (but still before -rc1).
>
> That is a little long for patches to be hanging around. I was planning
> to apply these on Wed. If either Ioana or you would prefer to get the
> testing performed first, please split those patches out and repost once
> they get validated.
If there is no issue reported in the meantime, I would say to apply the
series. I can always quickly fixup any problems that Martin might find.
Ioana
From: Ioana Ciornei <[email protected]>
In an attempt to actually support shared IRQs in phylib, we now move the
responsibility of triggering the phylib state machine or just returning
IRQ_NONE, based on the IRQ status register, to the PHY driver. Having
3 different IRQ handling callbacks (.handle_interrupt(),
.did_interrupt() and .ack_interrupt() ) is confusing so let the PHY
driver implement directly an IRQ handler like any other device driver.
Make this driver follow the new convention.
Cc: Jerome Brunet <[email protected]>
Cc: Neil Armstrong <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/meson-gxl.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index e8f2ca625837..b16b1cc89165 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -222,6 +222,24 @@ static int meson_gxl_config_intr(struct phy_device *phydev)
return phy_write(phydev, INTSRC_MASK, val);
}
+static irqreturn_t meson_gxl_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status;
+
+ irq_status = phy_read(phydev, INTSRC_FLAG);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (irq_status == 0)
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+
+ return IRQ_HANDLED;
+}
+
static struct phy_driver meson_gxl_phy[] = {
{
PHY_ID_MATCH_EXACT(0x01814400),
@@ -233,6 +251,7 @@ static struct phy_driver meson_gxl_phy[] = {
.read_status = meson_gxl_read_status,
.ack_interrupt = meson_gxl_ack_interrupt,
.config_intr = meson_gxl_config_intr,
+ .handle_interrupt = meson_gxl_handle_interrupt,
.suspend = genphy_suspend,
.resume = genphy_resume,
}, {
@@ -243,6 +262,7 @@ static struct phy_driver meson_gxl_phy[] = {
.soft_reset = genphy_soft_reset,
.ack_interrupt = meson_gxl_ack_interrupt,
.config_intr = meson_gxl_config_intr,
+ .handle_interrupt = meson_gxl_handle_interrupt,
.suspend = genphy_suspend,
.resume = genphy_resume,
},
--
2.28.0
Hello Ioana,
On Mon, Nov 23, 2020 at 4:38 PM Ioana Ciornei <[email protected]> wrote:
[...]
> Ioana Ciornei (15):
> net: phy: intel-xway: implement generic .handle_interrupt() callback
> net: phy: intel-xway: remove the use of .ack_interrupt()
> net: phy: icplus: implement generic .handle_interrupt() callback
> net: phy: icplus: remove the use .ack_interrupt()
> net: phy: meson-gxl: implement generic .handle_interrupt() callback
> net: phy: meson-gxl: remove the use of .ack_callback()
I will check the six patches above on Saturday (due to me being very
busy with my daytime job)
if that's too late for the netdev maintainers then I'm not worried
about it. at first glance this looks fine to me. and we can always fix
things afterwards (but still before -rc1).
Best regards,
Martin
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Mon, 23 Nov 2020 17:38:02 +0200 you 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.
>
> For a bit of context, at the moment, there are multiple phy_driver ops
> that deal with this subject:
>
> [...]
Here is the summary with links:
- [net-next,01/15] net: phy: intel-xway: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/1566db043952
- [net-next,02/15] net: phy: intel-xway: remove the use of .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/16c9709a7504
- [net-next,03/15] net: phy: icplus: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/25497b7f0bd9
- [net-next,04/15] net: phy: icplus: remove the use .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/12ae7ba3c15a
- [net-next,05/15] net: phy: meson-gxl: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/6719e2be0fcf
- [net-next,06/15] net: phy: meson-gxl: remove the use of .ack_callback()
https://git.kernel.org/netdev/net-next/c/84c8f773d2dc
- [net-next,07/15] net: phy: micrel: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/59ca4e58b917
- [net-next,08/15] net: phy: micrel: remove the use of .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/c0c99d0cd107
- [net-next,09/15] net: phy: national: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/6571b4555dc9
- [net-next,10/15] net: phy: national: remove the use of the .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/a4d7742149f6
- [net-next,11/15] net: phy: ti: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/1d1ae3c6ca3f
- [net-next,12/15] net: phy: ti: remove the use of .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/aa2d603ac8c0
- [net-next,13/15] net: phy: qsemi: implement generic .handle_interrupt() callback
https://git.kernel.org/netdev/net-next/c/efc3d9de7fa6
- [net-next,14/15] net: phy: qsemi: remove the use of .ack_interrupt()
https://git.kernel.org/netdev/net-next/c/a1a4417458cd
- [net-next,15/15] net: phy: remove the .did_interrupt() and .ack_interrupt() callback
https://git.kernel.org/netdev/net-next/c/6527b938426f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html