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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Tuesday, August 8, 2023 4:47 PM, Jiawen Wu wrote:
> On Tuesday, August 8, 2023 4:21 PM, Russell King (Oracle) wrote:
> > On Tue, Aug 08, 2023 at 10:17:04AM +0800, Jiawen Wu wrote:
> > > 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.
> 
> >
> > > @@ -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.
> 
> >
> > >  	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.
> 
> >
> > 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?