RE: [PATCH net-next v2 3/7] net: pcs: xpcs: add 1000BASE-X AN interrupt support

On Tuesday, August 8, 2023 4:47 PM, Jiawen Wu wrote:

On Tuesday, August 8, 2023 4:21 PM, Russell King (Oracle) wrote:

quoted

On Tue, Aug 08, 2023 at 10:17:04AM +0800, Jiawen Wu wrote:

quoted

Enable CL37 AN complete interrupt for DW XPCS. It requires to clear the
bit(0) [CL37_ANCMPLT_INTR] of VR_MII_AN_INTR_STS after AN completed.

And there is a quirk for Wangxun devices to enable CL37 AN in backplane
configurations because of the special hardware design.

Where is the interrupt handler?

PCS interrupt is directly connected to the PCI interrupt on the board, so the
interrupt handler is txgbe_irq_handler() in the ethernet driver.

quoted

quoted

@@ -759,6 +762,8 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
 		return ret;

 	ret &= ~DW_VR_MII_PCS_MODE_MASK;
+	if (!xpcs->pcs.poll)
+		ret |= DW_VR_MII_AN_INTR_EN;

Does this interrupt only work in 1000baseX mode?

AN interrupt now only be implemented in 1000baseX mode.

quoted

quoted

 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, ret);
 	if (ret < 0)
 		return ret;

@@ -1012,6 +1017,17 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
 		if (bmsr < 0)
 			return bmsr;

+		/* Clear AN complete interrupt */
+		if (!xpcs->pcs.poll) {
+			int an_intr;
+
+			an_intr = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
+			if (an_intr & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
+				an_intr &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
+				xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, an_intr);
+			}
+		}
+

get_state isn't supposed to be used as a way to acknowledge interrupts,
because that will get called quite a bit later after the interrupt has
been received.

I think it's just to clear the AN complete bit here. Actually, ethernet driver
handle interrupt and call phylink_mac_change() to check PCS state. It does
get called later, though.

quoted

As an example of PCS that use interrupts, please see the converted
mv88e6xxx PCS, for example:

 drivers/net/dsa/mv88e6xxx/pcs-6352.c

If the interrupt handler for the PCS is threaded, then it can access
the DW_VR_MII_AN_INTR_STS register to acknowledge the interrupt and
call phylink_pcs_change() or phylink_mac_change().

I'll check the usage of this method, thanks.

If interrupt handler is to be implemented in PCS, the codes about interrupts in
txgbe driver needs to be all refactored. This will affect GPIO, SFP... 

Could I refactor these codes in a future patch, and keep the current IRQ structure
in this series?