[PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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 patch set is part 1 and it addresses the changes needed in phylib
and 7 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 (19):
  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()
  net: phy: mscc: use phy_trigger_machine() to notify link change
  net: phy: mscc: implement generic .handle_interrupt() callback
  net: phy: mscc: remove the use of .ack_interrupt()
  net: phy: aquantia: implement generic .handle_interrupt() callback
  net: phy: aquantia: remove the use of .ack_interrupt()
  net: phy: broadcom: implement generic .handle_interrupt() callback
  net: phy: broadcom: remove use of ack_interrupt()
  net: phy: cicada: implement the generic .handle_interrupt() callback
  net: phy: cicada: remove the use of .ack_interrupt()
  net: phy: davicom: implement generic .handle_interrupt() calback
  net: phy: davicom: remove the use of .ack_interrupt()
  net: phy: add genphy_handle_interrupt_no_ack()
  net: phy: realtek: implement generic .handle_interrupt() callback
  net: phy: realtek: remove the use of .ack_interrupt()

 drivers/net/phy/aquantia_main.c  |  57 ++++++++++----
 drivers/net/phy/at803x.c         |  42 ++++++++--
 drivers/net/phy/bcm-cygnus.c     |   2 +-
 drivers/net/phy/bcm-phy-lib.c    |  37 ++++++++-
 drivers/net/phy/bcm-phy-lib.h    |   1 +
 drivers/net/phy/bcm54140.c       |  39 +++++++---
 drivers/net/phy/bcm63xx.c        |  20 +++--
 drivers/net/phy/bcm87xx.c        |  50 ++++++------
 drivers/net/phy/broadcom.c       |  70 ++++++++++++-----
 drivers/net/phy/cicada.c         |  35 ++++++++-
 drivers/net/phy/davicom.c        |  59 ++++++++++----
 drivers/net/phy/mscc/mscc_main.c |  70 +++++++++--------
 drivers/net/phy/phy.c            |   6 +-
 drivers/net/phy/phy_device.c     |  23 +++++-
 drivers/net/phy/realtek.c        | 128 +++++++++++++++++++++++++++----
 include/linux/phy.h              |   3 +
 16 files changed, 484 insertions(+), 158 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>

-- 
2.28.0

[PATCH net-next 01/19] net: phy: export phy_error and phy_trigger_machine

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

These functions are currently used by phy_interrupt() to either signal
an error condition or to trigger the link state machine. In an attempt
to actually support shared PHY IRQs, export these two functions so that
the actual PHY drivers can use them.

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>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phy.c | 6 ++++--
 include/linux/phy.h   | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 35525a671400..477bdf2f94df 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -493,10 +493,11 @@ EXPORT_SYMBOL(phy_queue_state_machine);
  *
  * @phydev: the phy_device struct
  */
-static void phy_trigger_machine(struct phy_device *phydev)
+void phy_trigger_machine(struct phy_device *phydev)
 {
 	phy_queue_state_machine(phydev, 0);
 }
+EXPORT_SYMBOL(phy_trigger_machine);
 
 static void phy_abort_cable_test(struct phy_device *phydev)
 {
@@ -924,7 +925,7 @@ void phy_stop_machine(struct phy_device *phydev)
  * Must not be called from interrupt context, or while the
  * phydev->lock is held.
  */
-static void phy_error(struct phy_device *phydev)
+void phy_error(struct phy_device *phydev)
 {
 	WARN_ON(1);
 
@@ -934,6 +935,7 @@ static void phy_error(struct phy_device *phydev)
 
 	phy_trigger_machine(phydev);
 }
+EXPORT_SYMBOL(phy_error);
 
 /**
  * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
diff --git a/include/linux/phy.h b/include/linux/phy.h
index eb3cb1a98b45..566b39f6cd64 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1570,8 +1570,10 @@ void phy_drivers_unregister(struct phy_driver *drv, int n);
 int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
 int phy_drivers_register(struct phy_driver *new_driver, int n,
 			 struct module *owner);
+void phy_error(struct phy_device *phydev);
 void phy_state_machine(struct work_struct *work);
 void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies);
+void phy_trigger_machine(struct phy_device *phydev);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
-- 
2.28.0

[PATCH net-next 02/19] net: phy: add a shutdown procedure

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

In case of a board which uses a shared IRQ we can easily end up with an
IRQ storm after a forced reboot.

For example, a 'reboot -f' will trigger a call to the .shutdown()
callbacks of all devices. Because phylib does not implement that hook,
the PHY is not quiesced, thus it can very well leave its IRQ enabled.

At the next boot, if that IRQ line is found asserted by the first PHY
driver that uses it, but _before_ the driver that is _actually_ keeping
the shared IRQ asserted is probed, the IRQ is not going to be
acknowledged, thus it will keep being fired preventing the boot process
of the kernel to continue. This is even worse when the second PHY driver
is a module.

To fix this, implement the .shutdown() callback and disable the
interrupts if these are used.

Note that we are still susceptible to IRQ storms if the previous kernel
exited with a panic or if the bootloader left the shared IRQ active, but
there is absolutely nothing we can do about these cases.

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>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phy_device.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..413a0a2c5d51 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2947,6 +2947,13 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
+static void phy_shutdown(struct device *dev)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+
+	phy_disable_interrupts(phydev);
+}
+
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -2970,6 +2977,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
+	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
-- 
2.28.0

[PATCH net-next 03/19] net: phy: make .ack_interrupt() optional

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

As a first step into making phylib and all PHY drivers to actually
have support for shared IRQs, make the .ack_interrupt() callback
optional.

After all drivers have been moved to implement the generic
interrupt handle, the phy_drv_supports_irq() check will be
changed again to only require the .handle_interrupts() callback.

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>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phy_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 413a0a2c5d51..f54f483d7fd6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2815,7 +2815,7 @@ EXPORT_SYMBOL(phy_get_internal_delay);
 
 static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 {
-	return phydrv->config_intr && phydrv->ack_interrupt;
+	return phydrv->config_intr && (phydrv->ack_interrupt || phydrv->handle_interrupt);
 }
 
 /**
-- 
2.28.0

[PATCH net-next 04/19] net: phy: at803x: implement generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Michael Walle <michael@walle.cc>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/at803x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index ed601a7e46a0..106c6f53755f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -628,6 +628,24 @@ static int at803x_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static irqreturn_t at803x_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, AT803X_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;
+}
+
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
 	/*
@@ -1064,6 +1082,7 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
@@ -1084,6 +1103,7 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
 }, {
 	/* Qualcomm Atheros AR8031/AR8033 */
 	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
@@ -1102,6 +1122,7 @@ static struct phy_driver at803x_driver[] = {
 	.aneg_done		= at803x_aneg_done,
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
@@ -1122,6 +1143,7 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
 }, {
@@ -1134,6 +1156,7 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
+	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
 	.read_status		= at803x_read_status,
-- 
2.28.0

[PATCH net-next 05/19] net: phy: at803x: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: Michael Walle <michael@walle.cc>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/at803x.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 106c6f53755f..aba198adf62d 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -614,6 +614,11 @@ static int at803x_config_intr(struct phy_device *phydev)
 	value = phy_read(phydev, AT803X_INTR_ENABLE);
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		/* Clear any pending interrupts */
+		err = at803x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
 		value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
 		value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
@@ -621,9 +626,14 @@ static int at803x_config_intr(struct phy_device *phydev)
 		value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
 
 		err = phy_write(phydev, AT803X_INTR_ENABLE, value);
-	}
-	else
+	} else {
 		err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
+		if (err)
+			return err;
+
+		/* Clear any pending interrupts */
+		err = at803x_ack_interrupt(phydev);
+	}
 
 	return err;
 }
@@ -1080,7 +1090,6 @@ static struct phy_driver at803x_driver[] = {
 	.resume			= at803x_resume,
 	/* PHY_GBIT_FEATURES */
 	.read_status		= at803x_read_status,
-	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
@@ -1101,7 +1110,6 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_BASIC_FEATURES */
-	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 }, {
@@ -1120,7 +1128,6 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_GBIT_FEATURES */
 	.read_status		= at803x_read_status,
 	.aneg_done		= at803x_aneg_done,
-	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
@@ -1141,7 +1148,6 @@ static struct phy_driver at803x_driver[] = {
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_BASIC_FEATURES */
-	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
@@ -1154,7 +1160,6 @@ static struct phy_driver at803x_driver[] = {
 	.resume			= at803x_resume,
 	.flags			= PHY_POLL_CABLE_TEST,
 	/* PHY_BASIC_FEATURES */
-	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-- 
2.28.0

[PATCH net-next 06/19] net: phy: mscc: use phy_trigger_machine() to notify link change

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

According to the comment describing the phy_mac_interrupt() function, it
it intended to be used by MAC drivers which have noticed a link change
thus its use in the mscc PHY driver is improper and, most probably, was
added just because phy_trigger_machine() was not exported.
Now that we have acces to trigger the link state machine, use directly
the phy_trigger_machine() function to notify a link change detected by
the PHY driver.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/mscc/mscc_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 6bc7406a1ce7..b705121c9d26 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1498,7 +1498,7 @@ static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev)
 		vsc8584_handle_macsec_interrupt(phydev);
 
 	if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG)
-		phy_mac_interrupt(phydev);
+		phy_trigger_machine(phydev);
 
 	return IRQ_HANDLED;
 }
-- 
2.28.0

[PATCH net-next 07/19] net: phy: mscc: implement generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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.

Also, remove the .did_interrupt() callback since it's not anymore used.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/mscc/mscc_main.c | 56 ++++++++++++++++----------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index b705121c9d26..ae790fd3734c 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1541,16 +1541,6 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int vsc8584_did_interrupt(struct phy_device *phydev)
-{
-	int rc = 0;
-
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
-		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
-
-	return (rc < 0) ? 0 : rc & MII_VSC85XX_INT_MASK_MASK;
-}
-
 static int vsc8514_config_pre_init(struct phy_device *phydev)
 {
 	/* These are the settings to override the silicon default
@@ -1948,6 +1938,24 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
 	return rc;
 }
 
+static irqreturn_t vsc85xx_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_VSC85XX_INT_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;
+}
+
 static int vsc85xx_config_aneg(struct phy_device *phydev)
 {
 	int rc;
@@ -2114,7 +2122,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init	= &vsc85xx_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt	= &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr	= &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2139,9 +2147,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_aneg    = &vsc85xx_config_aneg,
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8574_probe,
@@ -2163,7 +2170,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init    = &vsc8514_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2187,7 +2194,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init	= &vsc85xx_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt	= &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr	= &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2211,7 +2218,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init    = &vsc85xx_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2235,7 +2242,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init	= &vsc85xx_config_init,
 	.config_aneg	= &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt	= &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr	= &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2259,7 +2266,7 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init    = &vsc85xx_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2283,9 +2290,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init    = &vsc8584_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8574_probe,
@@ -2308,9 +2314,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_init    = &vsc8584_config_init,
 	.config_aneg    = &vsc85xx_config_aneg,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8584_probe,
@@ -2335,7 +2340,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.handle_interrupt = &vsc8584_handle_interrupt,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8574_probe,
@@ -2359,9 +2363,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.config_aneg    = &vsc85xx_config_aneg,
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.handle_interrupt = vsc85xx_handle_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8574_probe,
@@ -2388,7 +2391,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.handle_interrupt = &vsc8584_handle_interrupt,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8584_probe,
@@ -2413,7 +2415,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.handle_interrupt = &vsc8584_handle_interrupt,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8584_probe,
@@ -2438,7 +2439,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.handle_interrupt = &vsc8584_handle_interrupt,
 	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
-	.did_interrupt  = &vsc8584_did_interrupt,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
 	.probe		= &vsc8584_probe,
-- 
2.28.0

[PATCH net-next 08/19] net: phy: mscc: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Antoine Tenart <atenart@kernel.org>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/mscc/mscc_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index ae790fd3734c..266231b18bb4 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1923,6 +1923,10 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
 	int rc;
 
 	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		rc = vsc85xx_ack_interrupt(phydev);
+		if (rc)
+			return rc;
+
 		vsc8584_config_macsec_intr(phydev);
 		vsc8584_config_ts_intr(phydev);
 
@@ -1933,6 +1937,10 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
 		if (rc < 0)
 			return rc;
 		rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+		if (rc < 0)
+			return rc;
+
+		rc = vsc85xx_ack_interrupt(phydev);
 	}
 
 	return rc;
@@ -2338,7 +2346,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
 	.handle_interrupt = &vsc8584_handle_interrupt,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2389,7 +2396,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
 	.handle_interrupt = &vsc8584_handle_interrupt,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2413,7 +2419,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
 	.handle_interrupt = &vsc8584_handle_interrupt,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
@@ -2437,7 +2442,6 @@ static struct phy_driver vsc85xx_driver[] = {
 	.aneg_done	= &genphy_aneg_done,
 	.read_status	= &vsc85xx_read_status,
 	.handle_interrupt = &vsc8584_handle_interrupt,
-	.ack_interrupt  = &vsc85xx_ack_interrupt,
 	.config_intr    = &vsc85xx_config_intr,
 	.suspend	= &genphy_suspend,
 	.resume		= &genphy_resume,
-- 
2.28.0

[PATCH net-next 09/19] net: phy: aquantia: implement generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/aquantia_main.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 41e7c1432497..e7f315898efb 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -270,6 +270,24 @@ static int aqr_ack_interrupt(struct phy_device *phydev)
 	return (reg < 0) ? reg : 0;
 }
 
+static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
+	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 int aqr_read_status(struct phy_device *phydev)
 {
 	int val;
@@ -585,6 +603,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
 {
@@ -593,6 +612,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
 {
@@ -601,6 +621,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 	.suspend	= aqr107_suspend,
 	.resume		= aqr107_resume,
@@ -611,6 +632,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
 {
@@ -621,6 +643,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
 	.set_tunable    = aqr107_set_tunable,
@@ -639,6 +662,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
 	.set_tunable    = aqr107_set_tunable,
@@ -655,6 +679,7 @@ static struct phy_driver aqr_driver[] = {
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
 	.ack_interrupt	= aqr_ack_interrupt,
+	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
 };
-- 
2.28.0

[PATCH net-next 10/19] net: phy: aquantia: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/aquantia_main.c | 36 +++++++++++++++++----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index e7f315898efb..287178340625 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -246,6 +246,13 @@ static int aqr_config_intr(struct phy_device *phydev)
 	bool en = phydev->interrupts == PHY_INTERRUPT_ENABLED;
 	int err;
 
+	if (en) {
+		/* Clear any pending interrupts before enabling them */
+		err = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
+		if (err)
+			return err;
+	}
+
 	err = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_MASK2,
 			    en ? MDIO_AN_TX_VEND_INT_MASK2_LINK : 0);
 	if (err < 0)
@@ -256,18 +263,20 @@ static int aqr_config_intr(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	return phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK,
-			     en ? VEND1_GLOBAL_INT_VEND_MASK_GLOBAL3 |
-			     VEND1_GLOBAL_INT_VEND_MASK_AN : 0);
-}
+	err = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_INT_VEND_MASK,
+			    en ? VEND1_GLOBAL_INT_VEND_MASK_GLOBAL3 |
+			    VEND1_GLOBAL_INT_VEND_MASK_AN : 0);
+	if (err < 0)
+		return err;
 
-static int aqr_ack_interrupt(struct phy_device *phydev)
-{
-	int reg;
+	if (!en) {
+		/* Clear any pending interrupts after we have disabled them */
+		err = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
+		if (err)
+			return err;
+	}
 
-	reg = phy_read_mmd(phydev, MDIO_MMD_AN,
-			   MDIO_AN_TX_VEND_INT_STATUS2);
-	return (reg < 0) ? reg : 0;
+	return 0;
 }
 
 static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
@@ -602,7 +611,6 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQ1202",
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
@@ -611,7 +619,6 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQ2104",
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
@@ -620,7 +627,6 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQR105",
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 	.suspend	= aqr107_suspend,
@@ -631,7 +637,6 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQR106",
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
@@ -642,7 +647,6 @@ static struct phy_driver aqr_driver[] = {
 	.config_init	= aqr107_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
@@ -661,7 +665,6 @@ static struct phy_driver aqr_driver[] = {
 	.config_init	= aqcs109_config_init,
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr107_read_status,
 	.get_tunable    = aqr107_get_tunable,
@@ -678,7 +681,6 @@ static struct phy_driver aqr_driver[] = {
 	.name		= "Aquantia AQR405",
 	.config_aneg    = aqr_config_aneg,
 	.config_intr	= aqr_config_intr,
-	.ack_interrupt	= aqr_ack_interrupt,
 	.handle_interrupt = aqr_handle_interrupt,
 	.read_status	= aqr_read_status,
 },
-- 
2.28.0

[PATCH net-next 11/19] net: phy: broadcom: implement generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Michael Walle <michael@walle.cc>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/bcm-cygnus.c  |  1 +
 drivers/net/phy/bcm-phy-lib.c | 19 ++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  1 +
 drivers/net/phy/bcm54140.c    | 19 +++++++++++++-----
 drivers/net/phy/bcm63xx.c     |  2 ++
 drivers/net/phy/bcm87xx.c     | 32 +++++++++++++++++++++----------
 drivers/net/phy/broadcom.c    | 36 +++++++++++++++++++++++++++++++++++
 7 files changed, 95 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm-cygnus.c b/drivers/net/phy/bcm-cygnus.c
index 9ccf28b0a04d..a236e0b8d04d 100644
--- a/drivers/net/phy/bcm-cygnus.c
+++ b/drivers/net/phy/bcm-cygnus.c
@@ -258,6 +258,7 @@ static struct phy_driver bcm_cygnus_phy_driver[] = {
 	.config_init   = bcm_cygnus_config_init,
 	.ack_interrupt = bcm_phy_ack_intr,
 	.config_intr   = bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend       = genphy_suspend,
 	.resume        = bcm_cygnus_resume,
 }, {
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index ef6825b30323..a32da9ee98ab 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -196,6 +196,25 @@ int bcm_phy_config_intr(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_config_intr);
 
+irqreturn_t bcm_phy_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_BCM54XX_ISR);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status == 0)
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_handle_interrupt);
+
 int bcm_phy_read_shadow(struct phy_device *phydev, u16 shadow)
 {
 	phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_VAL(shadow));
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 237a8503c9b4..c3842f87c33b 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -63,6 +63,7 @@ int bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask,
 
 int bcm_phy_ack_intr(struct phy_device *phydev);
 int bcm_phy_config_intr(struct phy_device *phydev);
+irqreturn_t bcm_phy_handle_interrupt(struct phy_device *phydev);
 
 int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down);
 
diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 8998e68bb26b..f388cacd992a 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -637,13 +637,22 @@ static int bcm54140_config_init(struct phy_device *phydev)
 				  BCM54140_RDB_C_PWR_ISOLATE, 0);
 }
 
-static int bcm54140_did_interrupt(struct phy_device *phydev)
+static irqreturn_t bcm54140_handle_interrupt(struct phy_device *phydev)
 {
-	int ret;
+	int irq_status;
+
+	irq_status = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status == 0)
+		return IRQ_NONE;
 
-	ret = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+	phy_trigger_machine(phydev);
 
-	return (ret < 0) ? 0 : ret;
+	return IRQ_HANDLED;
 }
 
 static int bcm54140_ack_intr(struct phy_device *phydev)
@@ -834,8 +843,8 @@ static struct phy_driver bcm54140_drivers[] = {
 		.flags		= PHY_POLL_CABLE_TEST,
 		.features       = PHY_GBIT_FEATURES,
 		.config_init    = bcm54140_config_init,
-		.did_interrupt	= bcm54140_did_interrupt,
 		.ack_interrupt  = bcm54140_ack_intr,
+		.handle_interrupt = bcm54140_handle_interrupt,
 		.config_intr    = bcm54140_config_intr,
 		.probe		= bcm54140_probe,
 		.suspend	= genphy_suspend,
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index 459fb2069c7e..818c853b6638 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -69,6 +69,7 @@ static struct phy_driver bcm63xx_driver[] = {
 	.config_init	= bcm63xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm63xx_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	/* same phy as above, with just a different OUI */
 	.phy_id		= 0x002bdc00,
@@ -79,6 +80,7 @@ static struct phy_driver bcm63xx_driver[] = {
 	.config_init	= bcm63xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm63xx_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 } };
 
 module_phy_driver(bcm63xx_driver);
diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index df360e1c5069..f20cfb05ef04 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -153,10 +153,29 @@ static int bcm87xx_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-static int bcm87xx_did_interrupt(struct phy_device *phydev)
+static irqreturn_t bcm87xx_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, BCM87XX_LASI_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;
+}
+
+static int bcm87xx_ack_interrupt(struct phy_device *phydev)
 {
 	int reg;
 
+	/* Reading the LASI status clears it. */
 	reg = phy_read(phydev, BCM87XX_LASI_STATUS);
 
 	if (reg < 0) {
@@ -168,13 +187,6 @@ static int bcm87xx_did_interrupt(struct phy_device *phydev)
 	return (reg & 1) != 0;
 }
 
-static int bcm87xx_ack_interrupt(struct phy_device *phydev)
-{
-	/* Reading the LASI status clears it. */
-	bcm87xx_did_interrupt(phydev);
-	return 0;
-}
-
 static int bcm8706_match_phy_device(struct phy_device *phydev)
 {
 	return phydev->c45_ids.device_ids[4] == PHY_ID_BCM8706;
@@ -196,7 +208,7 @@ static struct phy_driver bcm87xx_driver[] = {
 	.read_status	= bcm87xx_read_status,
 	.ack_interrupt	= bcm87xx_ack_interrupt,
 	.config_intr	= bcm87xx_config_intr,
-	.did_interrupt	= bcm87xx_did_interrupt,
+	.handle_interrupt = bcm87xx_handle_interrupt,
 	.match_phy_device = bcm8706_match_phy_device,
 }, {
 	.phy_id		= PHY_ID_BCM8727,
@@ -208,7 +220,7 @@ static struct phy_driver bcm87xx_driver[] = {
 	.read_status	= bcm87xx_read_status,
 	.ack_interrupt	= bcm87xx_ack_interrupt,
 	.config_intr	= bcm87xx_config_intr,
-	.did_interrupt	= bcm87xx_did_interrupt,
+	.handle_interrupt = bcm87xx_handle_interrupt,
 	.match_phy_device = bcm8727_match_phy_device,
 } };
 
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index cd271de9609b..8bcdb94ef2fc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -643,6 +643,24 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static irqreturn_t brcm_fet_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_BRCM_FET_INTREG);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status == 0)
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
 struct bcm53xx_phy_priv {
 	u64	*stats;
 };
@@ -683,6 +701,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM5421,
 	.phy_id_mask	= 0xfffffff0,
@@ -691,6 +710,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM54210E,
 	.phy_id_mask	= 0xfffffff0,
@@ -699,6 +719,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
@@ -707,6 +728,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM54612E,
 	.phy_id_mask	= 0xfffffff0,
@@ -715,6 +737,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM54616S,
 	.phy_id_mask	= 0xfffffff0,
@@ -724,6 +747,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg	= bcm54616s_config_aneg,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 	.read_status	= bcm54616s_read_status,
 	.probe		= bcm54616s_probe,
 }, {
@@ -734,6 +758,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -745,6 +770,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg	= bcm5481_config_aneg,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id         = PHY_ID_BCM54810,
 	.phy_id_mask    = 0xfffffff0,
@@ -754,6 +780,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg    = bcm5481_config_aneg,
 	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
 	.resume		= bcm54xx_resume,
 }, {
@@ -765,6 +792,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_aneg    = bcm5481_config_aneg,
 	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
 	.resume		= bcm54xx_resume,
 }, {
@@ -776,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.read_status	= bcm5482_read_status,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM50610,
 	.phy_id_mask	= 0xfffffff0,
@@ -784,6 +813,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM50610M,
 	.phy_id_mask	= 0xfffffff0,
@@ -792,6 +822,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM57780,
 	.phy_id_mask	= 0xfffffff0,
@@ -800,6 +831,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCMAC131,
 	.phy_id_mask	= 0xfffffff0,
@@ -808,6 +840,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= brcm_fet_config_init,
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
+	.handle_interrupt = brcm_fet_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM5241,
 	.phy_id_mask	= 0xfffffff0,
@@ -816,6 +849,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= brcm_fet_config_init,
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
+	.handle_interrupt = brcm_fet_handle_interrupt,
 }, {
 	.phy_id		= PHY_ID_BCM5395,
 	.phy_id_mask	= 0xfffffff0,
@@ -839,6 +873,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init	= bcm54xx_config_init,
 	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
 	.phy_id         = PHY_ID_BCM89610,
 	.phy_id_mask    = 0xfffffff0,
@@ -847,6 +882,7 @@ static struct phy_driver broadcom_drivers[] = {
 	.config_init    = bcm54xx_config_init,
 	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
+	.handle_interrupt = bcm_phy_handle_interrupt,
 } };
 
 module_phy_driver(broadcom_drivers);
-- 
2.28.0

[PATCH net-next 12/19] net: phy: broadcom: remove use of ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Michael Walle <michael@walle.cc>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/bcm-cygnus.c  |  1 -
 drivers/net/phy/bcm-phy-lib.c | 18 ++++++++++++++----
 drivers/net/phy/bcm54140.c    | 20 +++++++++++++++-----
 drivers/net/phy/bcm63xx.c     | 18 +++++++++++++-----
 drivers/net/phy/bcm87xx.c     | 34 +++++++++++++---------------------
 drivers/net/phy/broadcom.c    | 34 +++++++++++++---------------------
 6 files changed, 68 insertions(+), 57 deletions(-)

diff --git a/drivers/net/phy/bcm-cygnus.c b/drivers/net/phy/bcm-cygnus.c
index a236e0b8d04d..da8f7cb41b44 100644
--- a/drivers/net/phy/bcm-cygnus.c
+++ b/drivers/net/phy/bcm-cygnus.c
@@ -256,7 +256,6 @@ static struct phy_driver bcm_cygnus_phy_driver[] = {
 	.name          = "Broadcom Cygnus PHY",
 	/* PHY_GBIT_FEATURES */
 	.config_init   = bcm_cygnus_config_init,
-	.ack_interrupt = bcm_phy_ack_intr,
 	.config_intr   = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend       = genphy_suspend,
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index a32da9ee98ab..8739faf78560 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -181,18 +181,28 @@ EXPORT_SYMBOL_GPL(bcm_phy_ack_intr);
 
 int bcm_phy_config_intr(struct phy_device *phydev)
 {
-	int reg;
+	int reg, err;
 
 	reg = phy_read(phydev, MII_BCM54XX_ECR);
 	if (reg < 0)
 		return reg;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = bcm_phy_ack_intr(phydev);
+		if (err)
+			return err;
+
 		reg &= ~MII_BCM54XX_ECR_IM;
-	else
+		err = phy_write(phydev, MII_BCM54XX_ECR, reg);
+	} else {
 		reg |= MII_BCM54XX_ECR_IM;
+		err = phy_write(phydev, MII_BCM54XX_ECR, reg);
+		if (err)
+			return err;
 
-	return phy_write(phydev, MII_BCM54XX_ECR, reg);
+		err = bcm_phy_ack_intr(phydev);
+	}
+	return err;
 }
 EXPORT_SYMBOL_GPL(bcm_phy_config_intr);
 
diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index f388cacd992a..e71af3aa4878 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -674,7 +674,7 @@ static int bcm54140_config_intr(struct phy_device *phydev)
 		BCM54140_RDB_TOP_IMR_PORT0, BCM54140_RDB_TOP_IMR_PORT1,
 		BCM54140_RDB_TOP_IMR_PORT2, BCM54140_RDB_TOP_IMR_PORT3,
 	};
-	int reg;
+	int reg, err;
 
 	if (priv->port >= ARRAY_SIZE(port_to_imr_bit))
 		return -EINVAL;
@@ -683,12 +683,23 @@ static int bcm54140_config_intr(struct phy_device *phydev)
 	if (reg < 0)
 		return reg;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = bcm54140_ack_intr(phydev);
+		if (err)
+			return err;
+
 		reg &= ~port_to_imr_bit[priv->port];
-	else
+		err = bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+	} else {
 		reg |= port_to_imr_bit[priv->port];
+		err = bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+		if (err)
+			return err;
+
+		err = bcm54140_ack_intr(phydev);
+	}
 
-	return bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+	return err;
 }
 
 static int bcm54140_get_downshift(struct phy_device *phydev, u8 *data)
@@ -843,7 +854,6 @@ static struct phy_driver bcm54140_drivers[] = {
 		.flags		= PHY_POLL_CABLE_TEST,
 		.features       = PHY_GBIT_FEATURES,
 		.config_init    = bcm54140_config_init,
-		.ack_interrupt  = bcm54140_ack_intr,
 		.handle_interrupt = bcm54140_handle_interrupt,
 		.config_intr    = bcm54140_config_intr,
 		.probe		= bcm54140_probe,
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index 818c853b6638..0eb33be824f1 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -25,12 +25,22 @@ static int bcm63xx_config_intr(struct phy_device *phydev)
 	if (reg < 0)
 		return reg;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = bcm_phy_ack_intr(phydev);
+		if (err)
+			return err;
+
 		reg &= ~MII_BCM63XX_IR_GMASK;
-	else
+		err = phy_write(phydev, MII_BCM63XX_IR, reg);
+	} else {
 		reg |= MII_BCM63XX_IR_GMASK;
+		err = phy_write(phydev, MII_BCM63XX_IR, reg);
+		if (err)
+			return err;
+
+		err = bcm_phy_ack_intr(phydev);
+	}
 
-	err = phy_write(phydev, MII_BCM63XX_IR, reg);
 	return err;
 }
 
@@ -67,7 +77,6 @@ static struct phy_driver bcm63xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.flags		= PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm63xx_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -78,7 +87,6 @@ static struct phy_driver bcm63xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.flags		= PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm63xx_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 } };
diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index f20cfb05ef04..4ac8fd190e9d 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -144,12 +144,22 @@ static int bcm87xx_config_intr(struct phy_device *phydev)
 	if (reg < 0)
 		return reg;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = phy_read(phydev, BCM87XX_LASI_STATUS);
+		if (err)
+			return err;
+
 		reg |= 1;
-	else
+		err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
+	} else {
 		reg &= ~1;
+		err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
+		if (err)
+			return err;
+
+		err = phy_read(phydev, BCM87XX_LASI_STATUS);
+	}
 
-	err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
 	return err;
 }
 
@@ -171,22 +181,6 @@ static irqreturn_t bcm87xx_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
-static int bcm87xx_ack_interrupt(struct phy_device *phydev)
-{
-	int reg;
-
-	/* Reading the LASI status clears it. */
-	reg = phy_read(phydev, BCM87XX_LASI_STATUS);
-
-	if (reg < 0) {
-		phydev_err(phydev,
-			   "Error: Read of BCM87XX_LASI_STATUS failed: %d\n",
-			   reg);
-		return 0;
-	}
-	return (reg & 1) != 0;
-}
-
 static int bcm8706_match_phy_device(struct phy_device *phydev)
 {
 	return phydev->c45_ids.device_ids[4] == PHY_ID_BCM8706;
@@ -206,7 +200,6 @@ static struct phy_driver bcm87xx_driver[] = {
 	.config_init	= bcm87xx_config_init,
 	.config_aneg	= bcm87xx_config_aneg,
 	.read_status	= bcm87xx_read_status,
-	.ack_interrupt	= bcm87xx_ack_interrupt,
 	.config_intr	= bcm87xx_config_intr,
 	.handle_interrupt = bcm87xx_handle_interrupt,
 	.match_phy_device = bcm8706_match_phy_device,
@@ -218,7 +211,6 @@ static struct phy_driver bcm87xx_driver[] = {
 	.config_init	= bcm87xx_config_init,
 	.config_aneg	= bcm87xx_config_aneg,
 	.read_status	= bcm87xx_read_status,
-	.ack_interrupt	= bcm87xx_ack_interrupt,
 	.config_intr	= bcm87xx_config_intr,
 	.handle_interrupt = bcm87xx_handle_interrupt,
 	.match_phy_device = bcm8727_match_phy_device,
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 8bcdb94ef2fc..8a4ec3222168 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -634,12 +634,22 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	if (reg < 0)
 		return reg;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = brcm_fet_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		reg &= ~MII_BRCM_FET_IR_MASK;
-	else
+		err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
+	} else {
 		reg |= MII_BRCM_FET_IR_MASK;
+		err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
+		if (err)
+			return err;
+
+		err = brcm_fet_ack_interrupt(phydev);
+	}
 
-	err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
 	return err;
 }
 
@@ -699,7 +709,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM5411",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -708,7 +717,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM5421",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -717,7 +725,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM54210E",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -726,7 +733,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM5461",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -735,7 +741,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM54612E",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -745,7 +750,6 @@ static struct phy_driver broadcom_drivers[] = {
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= bcm54616s_config_aneg,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.read_status	= bcm54616s_read_status,
@@ -756,7 +760,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM5464",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
@@ -768,7 +771,6 @@ static struct phy_driver broadcom_drivers[] = {
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= bcm5481_config_aneg,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -778,7 +780,6 @@ static struct phy_driver broadcom_drivers[] = {
 	/* PHY_GBIT_FEATURES */
 	.config_init    = bcm54xx_config_init,
 	.config_aneg    = bcm5481_config_aneg,
-	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
@@ -790,7 +791,6 @@ static struct phy_driver broadcom_drivers[] = {
 	/* PHY_GBIT_FEATURES */
 	.config_init    = bcm54811_config_init,
 	.config_aneg    = bcm5481_config_aneg,
-	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 	.suspend	= genphy_suspend,
@@ -802,7 +802,6 @@ static struct phy_driver broadcom_drivers[] = {
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm5482_config_init,
 	.read_status	= bcm5482_read_status,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -811,7 +810,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM50610",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -820,7 +818,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM50610M",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -829,7 +826,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM57780",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -838,7 +834,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCMAC131",
 	/* PHY_BASIC_FEATURES */
 	.config_init	= brcm_fet_config_init,
-	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.handle_interrupt = brcm_fet_handle_interrupt,
 }, {
@@ -847,7 +842,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name		= "Broadcom BCM5241",
 	/* PHY_BASIC_FEATURES */
 	.config_init	= brcm_fet_config_init,
-	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.handle_interrupt = brcm_fet_handle_interrupt,
 }, {
@@ -871,7 +865,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.get_stats	= bcm53xx_phy_get_stats,
 	.probe		= bcm53xx_phy_probe,
 	.config_init	= bcm54xx_config_init,
-	.ack_interrupt	= bcm_phy_ack_intr,
 	.config_intr	= bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 }, {
@@ -880,7 +873,6 @@ static struct phy_driver broadcom_drivers[] = {
 	.name           = "Broadcom BCM89610",
 	/* PHY_GBIT_FEATURES */
 	.config_init    = bcm54xx_config_init,
-	.ack_interrupt  = bcm_phy_ack_intr,
 	.config_intr    = bcm_phy_config_intr,
 	.handle_interrupt = bcm_phy_handle_interrupt,
 } };
-- 
2.28.0

[PATCH net-next 13/19] net: phy: cicada: implement the generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/cicada.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/phy/cicada.c b/drivers/net/phy/cicada.c
index 9d1612a4d7e6..03b957483023 100644
--- a/drivers/net/phy/cicada.c
+++ b/drivers/net/phy/cicada.c
@@ -96,6 +96,24 @@ static int cis820x_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static irqreturn_t cis820x_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_CIS8201_ISTAT);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status == 0)
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
 /* Cicada 8201, a.k.a Vitesse VSC8201 */
 static struct phy_driver cis820x_driver[] = {
 {
@@ -106,6 +124,7 @@ static struct phy_driver cis820x_driver[] = {
 	.config_init	= &cis820x_config_init,
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
+	.handle_interrupt = &cis820x_handle_interrupt,
 }, {
 	.phy_id		= 0x000fc440,
 	.name		= "Cicada Cis8204",
@@ -114,6 +133,7 @@ static struct phy_driver cis820x_driver[] = {
 	.config_init	= &cis820x_config_init,
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
+	.handle_interrupt = &cis820x_handle_interrupt,
 } };
 
 module_phy_driver(cis820x_driver);
-- 
2.28.0

[PATCH net-next 14/19] net: phy: cicada: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/cicada.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/cicada.c b/drivers/net/phy/cicada.c
index 03b957483023..5252ad320254 100644
--- a/drivers/net/phy/cicada.c
+++ b/drivers/net/phy/cicada.c
@@ -87,11 +87,20 @@ static int cis820x_config_intr(struct phy_device *phydev)
 {
 	int err;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = cis820x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		err = phy_write(phydev, MII_CIS8201_IMASK,
 				MII_CIS8201_IMASK_MASK);
-	else
+	} else {
 		err = phy_write(phydev, MII_CIS8201_IMASK, 0);
+		if (err)
+			return err;
+
+		err = cis820x_ack_interrupt(phydev);
+	}
 
 	return err;
 }
@@ -122,7 +131,6 @@ static struct phy_driver cis820x_driver[] = {
 	.phy_id_mask	= 0x000ffff0,
 	/* PHY_GBIT_FEATURES */
 	.config_init	= &cis820x_config_init,
-	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.handle_interrupt = &cis820x_handle_interrupt,
 }, {
@@ -131,7 +139,6 @@ static struct phy_driver cis820x_driver[] = {
 	.phy_id_mask	= 0x000fffc0,
 	/* PHY_GBIT_FEATURES */
 	.config_init	= &cis820x_config_init,
-	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.handle_interrupt = &cis820x_handle_interrupt,
 } };
-- 
2.28.0

[PATCH net-next 15/19] net: phy: davicom: implement generic .handle_interrupt() calback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/davicom.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
index 942f277463a4..262ff6c79860 100644
--- a/drivers/net/phy/davicom.c
+++ b/drivers/net/phy/davicom.c
@@ -77,6 +77,24 @@ static int dm9161_config_intr(struct phy_device *phydev)
 	return temp;
 }
 
+static irqreturn_t dm9161_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_DM9161_INTR);
+	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 int dm9161_config_aneg(struct phy_device *phydev)
 {
 	int err;
@@ -149,6 +167,7 @@ static struct phy_driver dm91xx_driver[] = {
 	.config_aneg	= dm9161_config_aneg,
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
+	.handle_interrupt = dm9161_handle_interrupt,
 }, {
 	.phy_id		= 0x0181b8b0,
 	.name		= "Davicom DM9161B/C",
@@ -158,6 +177,7 @@ static struct phy_driver dm91xx_driver[] = {
 	.config_aneg	= dm9161_config_aneg,
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
+	.handle_interrupt = dm9161_handle_interrupt,
 }, {
 	.phy_id		= 0x0181b8a0,
 	.name		= "Davicom DM9161A",
@@ -167,6 +187,7 @@ static struct phy_driver dm91xx_driver[] = {
 	.config_aneg	= dm9161_config_aneg,
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
+	.handle_interrupt = dm9161_handle_interrupt,
 }, {
 	.phy_id		= 0x00181b80,
 	.name		= "Davicom DM9131",
@@ -174,6 +195,7 @@ static struct phy_driver dm91xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
+	.handle_interrupt = dm9161_handle_interrupt,
 } };
 
 module_phy_driver(dm91xx_driver);
-- 
2.28.0

[PATCH net-next 16/19] net: phy: davicom: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/davicom.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
index 262ff6c79860..a5a2468732c2 100644
--- a/drivers/net/phy/davicom.c
+++ b/drivers/net/phy/davicom.c
@@ -57,24 +57,40 @@ MODULE_AUTHOR("Andy Fleming");
 MODULE_LICENSE("GPL");
 
 
+static int dm9161_ack_interrupt(struct phy_device *phydev)
+{
+	int err = phy_read(phydev, MII_DM9161_INTR);
+
+	return (err < 0) ? err : 0;
+}
+
 #define DM9161_DELAY 1
 static int dm9161_config_intr(struct phy_device *phydev)
 {
-	int temp;
+	int temp, err;
 
 	temp = phy_read(phydev, MII_DM9161_INTR);
 
 	if (temp < 0)
 		return temp;
 
-	if (PHY_INTERRUPT_ENABLED == phydev->interrupts)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = dm9161_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		temp &= ~(MII_DM9161_INTR_STOP);
-	else
+		err = phy_write(phydev, MII_DM9161_INTR, temp);
+	} else {
 		temp |= MII_DM9161_INTR_STOP;
+		err = phy_write(phydev, MII_DM9161_INTR, temp);
+		if (err)
+			return err;
 
-	temp = phy_write(phydev, MII_DM9161_INTR, temp);
+		err = dm9161_ack_interrupt(phydev);
+	}
 
-	return temp;
+	return err;
 }
 
 static irqreturn_t dm9161_handle_interrupt(struct phy_device *phydev)
@@ -150,13 +166,6 @@ static int dm9161_config_init(struct phy_device *phydev)
 	return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
 }
 
-static int dm9161_ack_interrupt(struct phy_device *phydev)
-{
-	int err = phy_read(phydev, MII_DM9161_INTR);
-
-	return (err < 0) ? err : 0;
-}
-
 static struct phy_driver dm91xx_driver[] = {
 {
 	.phy_id		= 0x0181b880,
@@ -165,7 +174,6 @@ static struct phy_driver dm91xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
-	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.handle_interrupt = dm9161_handle_interrupt,
 }, {
@@ -175,7 +183,6 @@ static struct phy_driver dm91xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
-	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.handle_interrupt = dm9161_handle_interrupt,
 }, {
@@ -185,7 +192,6 @@ static struct phy_driver dm91xx_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
-	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.handle_interrupt = dm9161_handle_interrupt,
 }, {
@@ -193,7 +199,6 @@ static struct phy_driver dm91xx_driver[] = {
 	.name		= "Davicom DM9131",
 	.phy_id_mask	= 0x0ffffff0,
 	/* PHY_BASIC_FEATURES */
-	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.handle_interrupt = dm9161_handle_interrupt,
 } };
-- 
2.28.0

[PATCH net-next 17/19] net: phy: add genphy_handle_interrupt_no_ack()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

It seems there are cases where the interrupts are handled by another
entity (ie an IRQ controller embedded inside the PHY) and do not need
any other interraction from phylib. For this kind of PHYs, like the
RTL8366RB, add the genphy_handle_interrupt_no_ack() function which just
triggers the link state machine.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phy_device.c | 13 +++++++++++++
 include/linux/phy.h          |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f54f483d7fd6..e13a46c25437 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2463,6 +2463,19 @@ int genphy_soft_reset(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_soft_reset);
 
+irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev)
+{
+	/* It seems there are cases where the interrupts are handled by another
+	 * entity (ie an IRQ controller embedded inside the PHY) and do not
+	 * need any other interraction from phylib. In this case, just trigger
+	 * the state machine directly.
+	 */
+	phy_trigger_machine(phydev);
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_handle_interrupt_no_ack);
+
 /**
  * genphy_read_abilities - read PHY abilities from Clause 22 registers
  * @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 566b39f6cd64..4f158d6352ae 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1510,6 +1510,7 @@ int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
 int genphy_loopback(struct phy_device *phydev, bool enable);
 int genphy_soft_reset(struct phy_device *phydev);
+irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);
 
 static inline int genphy_config_aneg(struct phy_device *phydev)
 {
-- 
2.28.0

[PATCH net-next 18/19] net: phy: realtek: implement generic .handle_interrupt() callback

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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 <martin.blumenstingl@googlemail.com>
Cc: Willy Liu <willy.liu@realtek.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/realtek.c | 60 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index fb1db713b7fb..dd77703af1be 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -149,6 +149,60 @@ static int rtl8211f_config_intr(struct phy_device *phydev)
 	return phy_write_paged(phydev, 0xa42, RTL821x_INER, val);
 }
 
+static irqreturn_t rtl8201_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, RTL8201F_ISR);
+	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 irqreturn_t rtl821x_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, RTL821x_INSR);
+	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 irqreturn_t rtl8211f_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read_paged(phydev, 0xa43, RTL8211F_INSR);
+	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 int rtl8211_config_aneg(struct phy_device *phydev)
 {
 	int ret;
@@ -556,6 +610,7 @@ static struct phy_driver realtek_drvs[] = {
 		.name		= "RTL8201F Fast Ethernet",
 		.ack_interrupt	= &rtl8201_ack_interrupt,
 		.config_intr	= &rtl8201_config_intr,
+		.handle_interrupt = rtl8201_handle_interrupt,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
@@ -582,6 +637,7 @@ static struct phy_driver realtek_drvs[] = {
 		.name		= "RTL8211B Gigabit Ethernet",
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
+		.handle_interrupt = rtl821x_handle_interrupt,
 		.read_mmd	= &genphy_read_mmd_unsupported,
 		.write_mmd	= &genphy_write_mmd_unsupported,
 		.suspend	= rtl8211b_suspend,
@@ -601,6 +657,7 @@ static struct phy_driver realtek_drvs[] = {
 		.name		= "RTL8211DN Gigabit Ethernet",
 		.ack_interrupt	= rtl821x_ack_interrupt,
 		.config_intr	= rtl8211e_config_intr,
+		.handle_interrupt = rtl821x_handle_interrupt,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
@@ -611,6 +668,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_init	= &rtl8211e_config_init,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
+		.handle_interrupt = rtl821x_handle_interrupt,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
@@ -621,6 +679,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_init	= &rtl8211f_config_init,
 		.ack_interrupt	= &rtl8211f_ack_interrupt,
 		.config_intr	= &rtl8211f_config_intr,
+		.handle_interrupt = rtl8211f_handle_interrupt,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
@@ -670,6 +729,7 @@ static struct phy_driver realtek_drvs[] = {
 		 */
 		.ack_interrupt	= genphy_no_ack_interrupt,
 		.config_intr	= genphy_no_config_intr,
+		.handle_interrupt = genphy_handle_interrupt_no_ack,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 	},
-- 
2.28.0

[PATCH net-next 19/19] net: phy: realtek: remove the use of .ack_interrupt()

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

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: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Willy Liu <willy.liu@realtek.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/realtek.c | 68 ++++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index dd77703af1be..d587f84c7380 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -102,24 +102,45 @@ static int rtl8211f_ack_interrupt(struct phy_device *phydev)
 static int rtl8201_config_intr(struct phy_device *phydev)
 {
 	u16 val;
+	int err;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = rtl8201_ack_interrupt(phydev);
+		if (err)
+			return err;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		val = BIT(13) | BIT(12) | BIT(11);
-	else
+		err = phy_write_paged(phydev, 0x7, RTL8201F_IER, val);
+	} else {
 		val = 0;
+		err = phy_write_paged(phydev, 0x7, RTL8201F_IER, val);
+		if (err)
+			return err;
+
+		err = rtl8201_ack_interrupt(phydev);
+	}
 
-	return phy_write_paged(phydev, 0x7, RTL8201F_IER, val);
+	return err;
 }
 
 static int rtl8211b_config_intr(struct phy_device *phydev)
 {
 	int err;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = rtl821x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		err = phy_write(phydev, RTL821x_INER,
 				RTL8211B_INER_INIT);
-	else
+	} else {
 		err = phy_write(phydev, RTL821x_INER, 0);
+		if (err)
+			return err;
+
+		err = rtl821x_ack_interrupt(phydev);
+	}
 
 	return err;
 }
@@ -128,11 +149,20 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 {
 	int err;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = rtl821x_ack_interrupt(phydev);
+		if (err)
+			return err;
+
 		err = phy_write(phydev, RTL821x_INER,
 				RTL8211E_INER_LINK_STATUS);
-	else
+	} else {
 		err = phy_write(phydev, RTL821x_INER, 0);
+		if (err)
+			return err;
+
+		err = rtl821x_ack_interrupt(phydev);
+	}
 
 	return err;
 }
@@ -140,13 +170,25 @@ static int rtl8211e_config_intr(struct phy_device *phydev)
 static int rtl8211f_config_intr(struct phy_device *phydev)
 {
 	u16 val;
+	int err;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = rtl8211f_ack_interrupt(phydev);
+		if (err)
+			return err;
 
-	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
 		val = RTL8211F_INER_LINK_STATUS;
-	else
+		err = phy_write_paged(phydev, 0xa42, RTL821x_INER, val);
+	} else {
 		val = 0;
+		err = phy_write_paged(phydev, 0xa42, RTL821x_INER, val);
+		if (err)
+			return err;
 
-	return phy_write_paged(phydev, 0xa42, RTL821x_INER, val);
+		err = rtl8211f_ack_interrupt(phydev);
+	}
+
+	return err;
 }
 
 static irqreturn_t rtl8201_handle_interrupt(struct phy_device *phydev)
@@ -608,7 +650,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc816),
 		.name		= "RTL8201F Fast Ethernet",
-		.ack_interrupt	= &rtl8201_ack_interrupt,
 		.config_intr	= &rtl8201_config_intr,
 		.handle_interrupt = rtl8201_handle_interrupt,
 		.suspend	= genphy_suspend,
@@ -635,7 +676,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc912),
 		.name		= "RTL8211B Gigabit Ethernet",
-		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
 		.handle_interrupt = rtl821x_handle_interrupt,
 		.read_mmd	= &genphy_read_mmd_unsupported,
@@ -655,7 +695,6 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc914),
 		.name		= "RTL8211DN Gigabit Ethernet",
-		.ack_interrupt	= rtl821x_ack_interrupt,
 		.config_intr	= rtl8211e_config_intr,
 		.handle_interrupt = rtl821x_handle_interrupt,
 		.suspend	= genphy_suspend,
@@ -666,7 +705,6 @@ static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc915),
 		.name		= "RTL8211E Gigabit Ethernet",
 		.config_init	= &rtl8211e_config_init,
-		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211e_config_intr,
 		.handle_interrupt = rtl821x_handle_interrupt,
 		.suspend	= genphy_suspend,
@@ -677,7 +715,6 @@ static struct phy_driver realtek_drvs[] = {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
 		.config_init	= &rtl8211f_config_init,
-		.ack_interrupt	= &rtl8211f_ack_interrupt,
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
 		.suspend	= genphy_suspend,
@@ -727,7 +764,6 @@ static struct phy_driver realtek_drvs[] = {
 		 * irq is requested and ACKed by reading the status register,
 		 * which is done by the irqchip code.
 		 */
-		.ack_interrupt	= genphy_no_ack_interrupt,
 		.config_intr	= genphy_no_config_intr,
 		.handle_interrupt = genphy_handle_interrupt_no_ack,
 		.suspend	= genphy_suspend,
-- 
2.28.0

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Heiner Kallweit]

On 29.10.2020 11:07, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> 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.
> 

Looks good to me. The current interrupt support in phylib basically
just covers the link change interrupt and we need more flexibility.

And even in the current limited use case we face smaller issues.
One reason is that INTR_STATUS typically is self-clearing on read.
phylib has to deal with the case that did_interrupt may or may not
have read INTR_STATUS already.

I'd just like to avoid the term "shared interrupt", because it has
a well-defined meaning. Our major concern isn't shared interrupts
but support for multiple interrupt sources (in addition to
link change) in a PHY.

WRT implementing a shutdown hook another use case was mentioned
recently: https://lkml.org/lkml/2020/9/30/451
But that's not really relevant here and just fyi.


> This patch set is part 1 and it addresses the changes needed in phylib
> and 7 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 (19):
>   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()
>   net: phy: mscc: use phy_trigger_machine() to notify link change
>   net: phy: mscc: implement generic .handle_interrupt() callback
>   net: phy: mscc: remove the use of .ack_interrupt()
>   net: phy: aquantia: implement generic .handle_interrupt() callback
>   net: phy: aquantia: remove the use of .ack_interrupt()
>   net: phy: broadcom: implement generic .handle_interrupt() callback
>   net: phy: broadcom: remove use of ack_interrupt()
>   net: phy: cicada: implement the generic .handle_interrupt() callback
>   net: phy: cicada: remove the use of .ack_interrupt()
>   net: phy: davicom: implement generic .handle_interrupt() calback
>   net: phy: davicom: remove the use of .ack_interrupt()
>   net: phy: add genphy_handle_interrupt_no_ack()
>   net: phy: realtek: implement generic .handle_interrupt() callback
>   net: phy: realtek: remove the use of .ack_interrupt()
> 
>  drivers/net/phy/aquantia_main.c  |  57 ++++++++++----
>  drivers/net/phy/at803x.c         |  42 ++++++++--
>  drivers/net/phy/bcm-cygnus.c     |   2 +-
>  drivers/net/phy/bcm-phy-lib.c    |  37 ++++++++-
>  drivers/net/phy/bcm-phy-lib.h    |   1 +
>  drivers/net/phy/bcm54140.c       |  39 +++++++---
>  drivers/net/phy/bcm63xx.c        |  20 +++--
>  drivers/net/phy/bcm87xx.c        |  50 ++++++------
>  drivers/net/phy/broadcom.c       |  70 ++++++++++++-----
>  drivers/net/phy/cicada.c         |  35 ++++++++-
>  drivers/net/phy/davicom.c        |  59 ++++++++++----
>  drivers/net/phy/mscc/mscc_main.c |  70 +++++++++--------
>  drivers/net/phy/phy.c            |   6 +-
>  drivers/net/phy/phy_device.c     |  23 +++++-
>  drivers/net/phy/realtek.c        | 128 +++++++++++++++++++++++++++----
>  include/linux/phy.h              |   3 +
>  16 files changed, 484 insertions(+), 158 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>
> 

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Vladimir Oltean]

On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> I'd just like to avoid the term "shared interrupt", because it has
> a well-defined meaning. Our major concern isn't shared interrupts
> but support for multiple interrupt sources (in addition to
> link change) in a PHY.

You may be a little bit confused Heiner.
This series adds support for exactly _that_ meaning of shared interrupts.
Shared interrupts (aka wired-OR on the PCB) don't work today with the
PHY library. I have a board that won't even boot to prompt when the
interrupt lines of its 2 PHYs are enabled, that this series fixes.
You might need to take another look through the commit messages I'm afraid.

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Heiner Kallweit]

On 30.10.2020 23:06, Vladimir Oltean wrote:
> On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
>> I'd just like to avoid the term "shared interrupt", because it has
>> a well-defined meaning. Our major concern isn't shared interrupts
>> but support for multiple interrupt sources (in addition to
>> link change) in a PHY.
> 
> You may be a little bit confused Heiner.
> This series adds support for exactly _that_ meaning of shared interrupts.
> Shared interrupts (aka wired-OR on the PCB) don't work today with the
> PHY library. I have a board that won't even boot to prompt when the
> interrupt lines of its 2 PHYs are enabled, that this series fixes.
> You might need to take another look through the commit messages I'm afraid.
> 
Right, I was focusing on the PTP irq use case.

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Heiner Kallweit]

On 29.10.2020 11:07, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> 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)

Here I have a concern, bits may be set even if the respective interrupt
source isn't enabled. Therefore we may falsely blame a device to have
triggered the interrupt. irq_status should be masked with the actually
enabled irq source bits.

> 		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 1 and it addresses the changes needed in phylib
> and 7 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 (19):
>   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()
>   net: phy: mscc: use phy_trigger_machine() to notify link change
>   net: phy: mscc: implement generic .handle_interrupt() callback
>   net: phy: mscc: remove the use of .ack_interrupt()
>   net: phy: aquantia: implement generic .handle_interrupt() callback
>   net: phy: aquantia: remove the use of .ack_interrupt()
>   net: phy: broadcom: implement generic .handle_interrupt() callback
>   net: phy: broadcom: remove use of ack_interrupt()
>   net: phy: cicada: implement the generic .handle_interrupt() callback
>   net: phy: cicada: remove the use of .ack_interrupt()
>   net: phy: davicom: implement generic .handle_interrupt() calback
>   net: phy: davicom: remove the use of .ack_interrupt()
>   net: phy: add genphy_handle_interrupt_no_ack()
>   net: phy: realtek: implement generic .handle_interrupt() callback
>   net: phy: realtek: remove the use of .ack_interrupt()
> 
>  drivers/net/phy/aquantia_main.c  |  57 ++++++++++----
>  drivers/net/phy/at803x.c         |  42 ++++++++--
>  drivers/net/phy/bcm-cygnus.c     |   2 +-
>  drivers/net/phy/bcm-phy-lib.c    |  37 ++++++++-
>  drivers/net/phy/bcm-phy-lib.h    |   1 +
>  drivers/net/phy/bcm54140.c       |  39 +++++++---
>  drivers/net/phy/bcm63xx.c        |  20 +++--
>  drivers/net/phy/bcm87xx.c        |  50 ++++++------
>  drivers/net/phy/broadcom.c       |  70 ++++++++++++-----
>  drivers/net/phy/cicada.c         |  35 ++++++++-
>  drivers/net/phy/davicom.c        |  59 ++++++++++----
>  drivers/net/phy/mscc/mscc_main.c |  70 +++++++++--------
>  drivers/net/phy/phy.c            |   6 +-
>  drivers/net/phy/phy_device.c     |  23 +++++-
>  drivers/net/phy/realtek.c        | 128 +++++++++++++++++++++++++++----
>  include/linux/phy.h              |   3 +
>  16 files changed, 484 insertions(+), 158 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>
> 

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Vladimir Oltean]

On Sat, Oct 31, 2020 at 12:06:42AM +0200, Vladimir Oltean wrote:
> On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> > I'd just like to avoid the term "shared interrupt", because it has
> > a well-defined meaning. Our major concern isn't shared interrupts
> > but support for multiple interrupt sources (in addition to
> > link change) in a PHY.
> 
> You may be a little bit confused Heiner.
> This series adds support for exactly _that_ meaning of shared interrupts.
> Shared interrupts (aka wired-OR on the PCB) don't work today with the
> PHY library. I have a board that won't even boot to prompt when the
> interrupt lines of its 2 PHYs are enabled, that this series fixes.
> You might need to take another look through the commit messages I'm afraid.

Maybe this diagram will help you visualize better.

time
 |
 |       PHY 1                  PHY 2 has pending IRQ
 |         |                      (e.g. link up)
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_HANDLED via               |
 |  phy_clear_interrupt()                |
 |         |                             |
 |         |                             |
 |         v                             |
 | handling of shared IRQ                |
 |     ends here                         |
 |         |                             |
 |         |                             v
 |         |                PHY 2 still has pending IRQ
 |         |            because, you know, it wasn't actually
 |         |                          serviced
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_HANDLED via               |
 |  phy_clear_interrupt()                |
 |         |                             |
 |         |                             |
 |         v                             |
 | handling of shared IRQ                |
 |     ends here                         |
 |         |                PHY 2: Hey! It's me! Over here!
 |         |                             |
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_HANDLED via               |
 |  phy_clear_interrupt()                |
 |         |                             |
 |         |                             |
 |         v                             |
 | handling of shared IRQ                |
 |     ends here                         |
 |         |                       PHY 2: Srsly?
 |         |                             |
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 |        ...                           ...
 |
 |               21 seconds later
 |
 |                  RCU stall
 v

This happens because today, the way phy_interrupt() is written, you can
only return IRQ_NONE and give the other driver a chance _if_ your driver
implements .did_interrupt(). But the kernel documentation of
.did_interrupt() only recommends to implement that function if you are a
multi-PHY package driver (otherwise stated, the hardware chip has an
embedded shared IRQ). But as things stand, _everybody_ should implement
.did_interrupt() in order for any combination of PHY drivers to support
shared IRQs.

What Ioana is proposing, and this is something that I fully agree with,
is that we just get rid of the layering where the PHY library tries to
be helpful but instead invites everybody to write systematically bad
code. Anyone knows how to write an IRQ handler with eyes closed, but the
fact that .did_interrupt() is mandatory for proper shared IRQ support is
not obvious to everybody, it seems. So let's just have a dedicated IRQ
handling function per each PHY driver, so that we don't get confused in
this sloppy mess of return values, and the code can actually be
followed.

Even _with_ Ioana's changes, there is one more textbook case of shared
interrupts causing trouble, and that is actually the reason why nobody
likes them except hardware engineers who don't get to deal with this.

time
 |
 |   PHY 1 probed
 | (module or built-in)
 |         |                   PHY 2 has pending IRQ
 |         |               (it had link up from previous
 |         v               boot, or from bootloader, etc)
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_NONE as                   |
 |     it should                         v
 |         |                PHY 2 still has pending IRQ
 |         |               but its handler wasn't called
 |         |              because its driver has not been
 |         |                        yet loaded
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_NONE as                   |
 |      it should                        v
 |         |                   PHY 2: Not again :(
 |         |                             |
 |         v                             |
 |   phy_interrupt()                     |
 |  called for PHY 1                     |
 |         |                             |
 |         v                             |
 | returns IRQ_NONE as                   |
 |      it should                        |
 |         |                             |
 |        ...                           ...
 |         |                             |
 |         |                PHY 2 driver never gets probed
 |         |               either because it's a module or
 |         |                because the system is too busy
 |         |                checking PHY 1 over and over
 |         |                again for an interrupt that
 |         |                     it did not trigger
 |         |                             |
 |        ...                           ...
 |
 |               21 seconds later
 |
 |                  RCU stall
 v

The way that it's solved is that it's never 100% solved.
This one you can just avoid, but never recover from it.
To avoid it, you must ensure from your previous boot environments
(bootloader, kexec) that the IRQ line is not pending. Because if you
leave the shared IRQ line pending, the system is kaput if it happens to
probe the drivers in the wrong order (aka don't probe first the driver
that will clear that shared IRQ). It's like Minesweeper, only worse.

That's why the shutdown hook is there, as a best-effort attempt for
Linux to clean up after itself. But we're always at the mercy of the
bootloader, or even at the mercy of chance. If the previous kernel
panicked, there's no orderly cleanup to speak of.

Hope it's clearer now.

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Andrew Lunn]

> > - 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)
> 
> Here I have a concern, bits may be set even if the respective interrupt
> source isn't enabled. Therefore we may falsely blame a device to have
> triggered the interrupt. irq_status should be masked with the actually
> enabled irq source bits.

Hi Heiner

I would say that is a driver implementation detail, for each driver to
handle how it needs to handle it. I've seen some hardware where the
interrupt status is already masked with the interrupt enabled
bits. I've soon other hardware where it is not.

For example code, what is listed above is O.K. The real implementation
in a driver need knowledge of the hardware.

      Andrew

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

On Sat, Oct 31, 2020 at 12:36:27AM +0100, 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)
> > 
> > Here I have a concern, bits may be set even if the respective interrupt
> > source isn't enabled. Therefore we may falsely blame a device to have
> > triggered the interrupt. irq_status should be masked with the actually
> > enabled irq source bits.
> 
> Hi Heiner
> 
> I would say that is a driver implementation detail, for each driver to
> handle how it needs to handle it. I've seen some hardware where the
> interrupt status is already masked with the interrupt enabled
> bits. I've soon other hardware where it is not.
> 
> For example code, what is listed above is O.K. The real implementation
> in a driver need knowledge of the hardware.
> 

Hi,

As Andrew said, that is just an example code that could work for some
devices but should be extended depending on how the actual PHY is
working.

For example, the VSC8584 will still be trigerring the link state machine
just on the link change interrupt, I am not changing this:

static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev)
{
	irqreturn_t ret;
	int irq_status;

	irq_status = phy_read(phydev, MII_VSC85XX_INT_STATUS);
	if (irq_status < 0)
		return IRQ_NONE;

	/* Timestamping IRQ does not set a bit in the global INT_STATUS, so
	 * irq_status would be 0.
	 */
	ret = vsc8584_handle_ts_interrupt(phydev);
	if (!(irq_status & MII_VSC85XX_INT_MASK_MASK))
		return ret;

	if (irq_status & MII_VSC85XX_INT_MASK_EXT)
		vsc8584_handle_macsec_interrupt(phydev);

	if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG)
		phy_trigger_machine(phydev);

	return IRQ_HANDLED;
}

Ioana

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> On 29.10.2020 11:07, Ioana Ciornei wrote:
> > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > 
> > 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.
> > 
> 
> Looks good to me. The current interrupt support in phylib basically
> just covers the link change interrupt and we need more flexibility.
> 
> And even in the current limited use case we face smaller issues.
> One reason is that INTR_STATUS typically is self-clearing on read.
> phylib has to deal with the case that did_interrupt may or may not
> have read INTR_STATUS already.
> 
> I'd just like to avoid the term "shared interrupt", because it has
> a well-defined meaning. Our major concern isn't shared interrupts
> but support for multiple interrupt sources (in addition to
> link change) in a PHY.
> 

I am not going to address this part, Vladimir did a good job in the
following emails describing exactly the problem that I am trying to fix
- shared interrupts even between PHYs which are not in the same package
or even the same type of device.

> WRT implementing a shutdown hook another use case was mentioned
> recently: https://lkml.org/lkml/2020/9/30/451
> But that's not really relevant here and just fyi.
> 

I missed this thread. Thanks for the link!

Ioana

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Heiner Kallweit]

On 31.10.2020 00:36, 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)
>>
>> Here I have a concern, bits may be set even if the respective interrupt
>> source isn't enabled. Therefore we may falsely blame a device to have
>> triggered the interrupt. irq_status should be masked with the actually
>> enabled irq source bits.
> 
> Hi Heiner
> 
Hi Andrew,

> I would say that is a driver implementation detail, for each driver to
> handle how it needs to handle it. I've seen some hardware where the
> interrupt status is already masked with the interrupt enabled
> bits. I've soon other hardware where it is not.
> 
Sure, I just wanted to add the comment before others simply copy and
paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
it is used as is. And IIRC at least the Aquantia PHY doesn't mask
the interrupt status.

> For example code, what is listed above is O.K. The real implementation
> in a driver need knowledge of the hardware.
> 
>       Andrew
> .
> 
Heiner

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

On Sat, Oct 31, 2020 at 11:18:18AM +0100, Heiner Kallweit wrote:
> On 31.10.2020 00:36, 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)
> >>
> >> Here I have a concern, bits may be set even if the respective interrupt
> >> source isn't enabled. Therefore we may falsely blame a device to have
> >> triggered the interrupt. irq_status should be masked with the actually
> >> enabled irq source bits.
> > 
> > Hi Heiner
> > 
> Hi Andrew,
> 
> > I would say that is a driver implementation detail, for each driver to
> > handle how it needs to handle it. I've seen some hardware where the
> > interrupt status is already masked with the interrupt enabled
> > bits. I've soon other hardware where it is not.
> > 
> Sure, I just wanted to add the comment before others simply copy and
> paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> the interrupt status.
> 

Hi Heiner,

If I understand correctly what you are suggesting, the
.handle_interrupt() for the aquantia would look like this:

static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
{
	int irq_status;

	irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
	if (irq_status < 0) {
		phy_error(phydev);
		return IRQ_NONE;
	}

	if (!(irq_status & MDIO_AN_TX_VEND_INT_STATUS2_MASK))
		return IRQ_NONE;

	phy_trigger_machine(phydev);

	return IRQ_HANDLED;
}

So only return IRQ_HANDLED when one of the bits from INT_STATUS
corresponding with the enabled interrupts are set, not if any other link
status bit is set.

Ok, I'll send a v2 with these kind of changes.

Ioana

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Andrew Lunn]

> Sure, I just wanted to add the comment before others simply copy and
> paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> the interrupt status.

And that is were we are going to have issues with this patch set, and
need review by individual PHY driver maintainers, or a good look at
the datasheet.

    Andrew

Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

On Sat, Oct 31, 2020 at 03:32:15PM +0100, Andrew Lunn wrote:
> > Sure, I just wanted to add the comment before others simply copy and
> > paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> > it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> > the interrupt status.
> 
> And that is were we are going to have issues with this patch set, and
> need review by individual PHY driver maintainers, or a good look at
> the datasheet.
> 

Yep, I already started to comb through all the drivers and their
datasheets so that I can only check for the interrupts that the driver
enables.

Ioana