[PATCH net v2] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[PATCH net v2] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[Rosen Penev]

marvell_config_intr() rewrote the whole register, so the config_intr
call from phy_init_hw() on resume silently cleared WOL_EIE on polled
PHYs: m88e1318_get_wol() still reported WAKE_MAGIC, but a matched magic
packet was no longer routed to INTn and the board did not wake.

Give these two PHYs a dedicated config_intr that sets WOL_EIE from the
driver's own WoL state rather than from the hardware, so a bit left
armed by the bootloader or a previous kernel is cleared at probe. On the
disable path the bit is only kept for polled PHYs; with a PHY interrupt
the handler is about to be freed or not yet requested, and a latched
WoL event would be left with nobody to clear it. The enable path re-arms
it. Add a handle_interrupt that also claims the WoL event mirrored in
the interrupt status register. m88e1318_set_wol() now also clears WOL_EIE
when WoL is fully disabled, so "wol d" disarms it immediately.

Fixes: 3871c3876f80 ("mv643xx_eth with 88E1318S: support Wake on LAN")
Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: resolved review warnings, including wol d.
 drivers/net/phy/marvell.c | 106 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..46527abab6af 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -354,6 +354,7 @@ struct marvell_priv {
 	u32 step;
 	s8 pair;
 	u8 vct_phase;
+	bool wol_armed;
 };
 
 static int marvell_read_page(struct phy_device *phydev)
@@ -425,6 +426,83 @@ static irqreturn_t marvell_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+/*
+ * On the 88E1318S/88E1510, copper page register 0x12 serves two
+ * masters: it is the MII_M1011_IMASK interrupt mask for the generic
+ * Marvell interrupt handling, and m88e1318_set_wol() sets the WoL
+ * interrupt enable bit (MII_88E1318S_PHY_CSIER_WOL_EIE) in it.
+ *
+ * config_intr runs from phy_probe(), from phy_init_hw() on attach and
+ * on resume, and from phy_request_interrupt()/phy_free_interrupt() on
+ * ifup/ifdown. Each of these rewrites the register, so the routines
+ * below re-arm WOL_EIE while WoL is set up; otherwise a resume would
+ * silently disarm Wake-on-LAN for the next suspend.
+ *
+ * WOL_EIE is derived from the driver's own WoL state rather than read
+ * back from the PHY, so a bit left armed by the bootloader or a
+ * previous kernel is cleared at probe/init instead of carried over.
+ */
+static int m88e1318_config_intr(struct phy_device *phydev)
+{
+	struct marvell_priv *priv = phydev->priv;
+	u16 wol_eie = 0;
+	int err;
+
+	if (priv->wol_armed)
+		wol_eie = MII_88E1318S_PHY_CSIER_WOL_EIE;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = marvell_ack_interrupt(phydev);
+		if (err < 0)
+			return err;
+
+		err = phy_write(phydev, MII_88E1318S_PHY_CSIER,
+				MII_M1011_IMASK_INIT | wol_eie);
+	} else {
+		/* On a polled PHY, keep WOL_EIE so an armed WoL event
+		 * still asserts INTn across phy_init_hw() on resume.
+		 *
+		 * With a PHY interrupt, being disabled means the handler
+		 * is not requested yet or is about to be freed, and
+		 * nobody would clear a latched WoL event; a shared line
+		 * would then be disabled as "nobody cared". Drop WOL_EIE
+		 * here, the enable path re-arms it.
+		 */
+		if (phy_interrupt_is_valid(phydev))
+			wol_eie = 0;
+
+		err = phy_write(phydev, MII_88E1318S_PHY_CSIER, wol_eie);
+		if (err < 0)
+			return err;
+
+		err = marvell_ack_interrupt(phydev);
+	}
+
+	return err;
+}
+
+static irqreturn_t m88e1318_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, MII_M1011_IEVENT);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	/* Claim events from the IMASK_INIT set as well as the WoL event
+	 * mirrored from WOL_EIE in the enable register.
+	 */
+	if (!(irq_status & (MII_M1011_IMASK_INIT |
+			    MII_88E1318S_PHY_CSIER_WOL_EIE)))
+		return IRQ_NONE;
+
+	phy_trigger_machine(phydev);
+
+	return IRQ_HANDLED;
+}
+
 static int marvell_set_polarity(struct phy_device *phydev, int polarity)
 {
 	u16 val;
@@ -1969,6 +2047,7 @@ static void m88e1318_get_wol(struct phy_device *phydev,
 static int m88e1318_set_wol(struct phy_device *phydev,
 			    struct ethtool_wolinfo *wol)
 {
+	struct marvell_priv *priv = phydev->priv;
 	int err = 0, oldpage;
 
 	oldpage = phy_save_page(phydev);
@@ -2074,6 +2153,25 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 			goto error;
 	}
 
+	if (!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY))) {
+		/* Fully disabled: drop the WoL interrupt enable now
+		 * instead of waiting for the next config_intr call.
+		 */
+		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
+		if (err < 0)
+			goto error;
+
+		err = __phy_clear_bits(phydev, MII_88E1318S_PHY_CSIER,
+				       MII_88E1318S_PHY_CSIER_WOL_EIE);
+		if (err < 0)
+			goto error;
+	}
+
+	/* Let m88e1318_config_intr() re-arm CSIER.WOL_EIE, or keep it
+	 * cleared once WoL is disabled.
+	 */
+	priv->wol_armed = !!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY));
+
 error:
 	return phy_restore_page(phydev, oldpage, err);
 }
@@ -3817,8 +3915,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_init = m88e1318_config_init,
 		.config_aneg = m88e1318_config_aneg,
 		.read_status = marvell_read_status,
-		.config_intr = marvell_config_intr,
-		.handle_interrupt = marvell_handle_interrupt,
+		.config_intr = m88e1318_config_intr,
+		.handle_interrupt = m88e1318_handle_interrupt,
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
 		.resume = genphy_resume,
@@ -3925,8 +4023,8 @@ static struct phy_driver marvell_drivers[] = {
 		.config_init = m88e1510_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
-		.config_intr = marvell_config_intr,
-		.handle_interrupt = marvell_handle_interrupt,
+		.config_intr = m88e1318_config_intr,
+		.handle_interrupt = m88e1318_handle_interrupt,
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
 		.resume = m88e1510_resume,
-- 
2.56.0

Re: [PATCH net v2] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[Andrew Lunn]

> +static irqreturn_t m88e1318_handle_interrupt(struct phy_device *phydev)
> +{
> +	int irq_status;
> +
> +	irq_status = phy_read(phydev, MII_M1011_IEVENT);
> +	if (irq_status < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	/* Claim events from the IMASK_INIT set as well as the WoL event
> +	 * mirrored from WOL_EIE in the enable register.
> +	 */
> +	if (!(irq_status & (MII_M1011_IMASK_INIT |
> +			    MII_88E1318S_PHY_CSIER_WOL_EIE)))

The existing naming in this driver is not nice. We have:

#define MII_M1011_IMASK			0x12

/* Copper Specific Interrupt Enable Register */
#define MII_88E1318S_PHY_CSIER				0x12

So the same register has two macros/names.

And then

#define MII_88E1318S_PHY_CSIER_WOL_EIE			BIT(7)

So this is one bit in that register. Combing MII_M1011_IMASK with
MII_88E1318S_PHY_CSIER_WOL_EIE is technically correct, but looks wrong
because they have different prefixes.

Marvell has an SDK called DSDT. It is GPL v2 licenses, so if you can
find a copy anywhere on the Internet, it is fine to use as
reference. It does not treat bit 7 special between different devices,
those with and without WoL. So i don't think we need a special
interrupt handler, the existing one can be extended for WoL.

Can we sort out the naming for the interrupt registers and the bits.

DSDT has:

#define MAD_COPPER_AUTONEGO_ERROR        MAD_BIT_15
#define MAD_COPPER_SPEED_CHANGED         MAD_BIT_14
#define MAD_COPPER_DUPLEX_CHANGED        MAD_BIT_13
#define MAD_COPPER_PAGE_RECEIVED         MAD_BIT_12
#define MAD_COPPER_AUTO_NEG_COMPLETED    MAD_BIT_11
#define MAD_COPPER_LINK_STATUS_CHANGED   MAD_BIT_10
#define MAD_COPPER_SYMBOL_ERROR          MAD_BIT_9
#define MAD_COPPER_FALSE_CARRIER         MAD_BIT_8
#define MAD_COPPER_WOL_EVENT             MAD_BIT_7
#define MAD_COPPER_CROSSOVER_CHANGED     MAD_BIT_6
#define MAD_COPPER_DOWNSHIFT_DETECT      MAD_BIT_5
#define MAD_COPPER_ENERGY_DETECT         MAD_BIT_4
#define MAD_COPPER_FLP_EXCH_COMP_NO_LNK  MAD_BIT_3
#define MAD_COPPER_DTE_DETECT_CHANGED    MAD_BIT_2
#define MAD_COPPER_POLARITY_CHANGED      MAD_BIT_1
#define MAD_COPPER_JABBER                MAD_BIT_0

Which would make:

#define MII_M1011_IMASK_INIT		0x6400

link status change, duplex change and speed change.

Probably MII_88E1318S_PHY_CSIER should go away, keep with IEVENT,
IMASK, and name the bits MII_M1011_I_JABBER, MII_M1011_I_SPEED_CHANGE?

       Thanks
	Andrew