| Message ID | 20230808021708.196160-4-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | support more link mode for TXGBE | expand |
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? > @@ -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? > 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. 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(). Thanks.
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?
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 82bbc94f4ef1..914d119f62bb 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -738,6 +738,9 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, int ret, mdio_ctrl, adv; bool changed = 0; + if (xpcs->dev_flag == DW_DEV_TXGBE) + xpcs_write_vpcs(xpcs, DW_VR_XS_PCS_DIG_CTRL1, DW_CL37_BP); + /* According to Chap 7.12, to set 1000BASE-X C37 AN, AN must * be disabled first:- * 1) VR_MII_MMD_CTRL Bit(12)[AN_ENABLE] = 0b @@ -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; 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); + } + } + phylink_mii_c22_pcs_decode_state(state, bmsr, lpa); } diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h index 2657138391af..08a8881614de 100644 --- a/drivers/net/pcs/pcs-xpcs.h +++ b/drivers/net/pcs/pcs-xpcs.h @@ -17,6 +17,7 @@ #define DW_USXGMII_EN BIT(9) #define DW_VR_XS_PCS_DIG_CTRL1 0x0000 #define DW_VR_RST BIT(15) +#define DW_CL37_BP BIT(12) #define DW_VR_XS_PCS_DIG_STS 0x0010 #define DW_RXFIFO_ERR GENMASK(6, 5) #define DW_PSEQ_ST GENMASK(4, 2) @@ -79,8 +80,10 @@ #define DW_VR_MII_PCS_MODE_MASK GENMASK(2, 1) #define DW_VR_MII_PCS_MODE_C37_1000BASEX 0x0 #define DW_VR_MII_PCS_MODE_C37_SGMII 0x2 +#define DW_VR_MII_AN_INTR_EN BIT(0) /* VR_MII_AN_INTR_STS */ +#define DW_VR_MII_AN_STS_C37_ANCMPLT_INTR BIT(0) #define DW_VR_MII_AN_STS_C37_ANSGM_FD BIT(1) #define DW_VR_MII_AN_STS_C37_ANSGM_SP_SHIFT 2 #define DW_VR_MII_AN_STS_C37_ANSGM_SP GENMASK(3, 2)
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. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/pcs/pcs-xpcs.c | 16 ++++++++++++++++ drivers/net/pcs/pcs-xpcs.h | 3 +++ 2 files changed, 19 insertions(+)