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

[Andrew Lunn <andrew@lunn.ch>]

> +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