| Message ID | 20230419082739.295180-1-jiawenwu@trustnetic.com |
|---|---|
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote: > Add basic support for XPCS using 10GBASE-R interface. This mode will > be extended to use interrupt, so set pcs.poll false. And avoid soft > reset so that the device using this mode is in the default configuration. I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the out-of-reset configuration the "default" one? > +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs, > + struct phylink_link_state *state) > +{ > + int ret; > + > + state->link = false; > + > + ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1); > + if (ret < 0) > + return ret; > + > + if (ret & MDIO_STAT1_LSTATUS) > + state->link = true; > + > + if (state->link) { It seems pointless to open a new "if" statement when this would have sufficed: if (ret & MDIO_STAT1_LSTATUS) { state->link = true; state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; ... } > + state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > + state->duplex = DUPLEX_FULL; > + state->speed = SPEED_10000; > + } > + > + return 0; > +}
On Wed, Apr 19, 2023 at 04:27:35PM +0800, Jiawen Wu wrote: > + ret = txgbe_sfp_register(txgbe); > + if (ret) { > + wx_err(txgbe->wx, "failed to register sfp\n"); > + goto err; > + } The usual error handling pattern is to jump to specific labels within the error unwind code path (which duplicates the normal teardown path except for the first operation), rather than calling the single cleanup function - here txgbe_remove_phy() - and filling that with "if" conditions. Normally (at least in the networking layer except for Qdiscs, that's all I know), one would expect that if txgbe_init_phy() fails, txgbe_remove_phy() is never called. So, given that expectation, txgbe->sfp_dev would never be NULL. > + > return 0; > > err: > @@ -131,6 +156,8 @@ int txgbe_init_phy(struct txgbe *txgbe) > > void txgbe_remove_phy(struct txgbe *txgbe) > { > + if (txgbe->sfp_dev) > + platform_device_unregister(txgbe->sfp_dev); > if (txgbe->i2c_dev) > platform_device_unregister(txgbe->i2c_dev); > > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h > index 771aefbc7c80..aa94c4fce311 100644 > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h > @@ -149,6 +149,7 @@ struct txgbe_nodes { > struct txgbe { > struct wx *wx; > struct txgbe_nodes nodes; > + struct platform_device *sfp_dev; > struct platform_device *i2c_dev; > }; > > -- > 2.27.0 >
On Wed, Apr 19, 2023 at 04:27:32PM +0800, Jiawen Wu wrote: > Register software nodes for GPIO, I2C, SFP and PHYLINK. Define the > device properties. > > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote: > On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote: > > Add basic support for XPCS using 10GBASE-R interface. This mode will > > be extended to use interrupt, so set pcs.poll false. And avoid soft > > reset so that the device using this mode is in the default configuration. > > I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the > out-of-reset configuration the "default" one? > Theoretically so, I need to configure 10GBASE-R mode after reset. But this configuration involves board info to configure PMA, etc., I'd like to implement it in the next patch. Now the "default" configuration refers to the mode in which the firmware is configured. > > +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs, > > + struct phylink_link_state *state) > > +{ > > + int ret; > > + > > + state->link = false; > > + > > + ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1); > > + if (ret < 0) > > + return ret; > > + > > + if (ret & MDIO_STAT1_LSTATUS) > > + state->link = true; > > + > > + if (state->link) { > > It seems pointless to open a new "if" statement when this would have > sufficed: > > if (ret & MDIO_STAT1_LSTATUS) { > state->link = true; > state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > ... > } > > > + state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX; > > + state->duplex = DUPLEX_FULL; > > + state->speed = SPEED_10000; > > + } > > + > > + return 0; > > +} >
On Thu, Apr 20, 2023 at 09:56:26AM +0800, Jiawen Wu wrote: > On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote: > > On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote: > > > Add basic support for XPCS using 10GBASE-R interface. This mode will > > > be extended to use interrupt, so set pcs.poll false. And avoid soft > > > reset so that the device using this mode is in the default configuration. > > > > I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the > > out-of-reset configuration the "default" one? > > Theoretically so, I need to configure 10GBASE-R mode after reset. But this > configuration involves board info to configure PMA, etc., I'd like to implement > it in the next patch. Now the "default" configuration refers to the mode in > which the firmware is configured. How much extra complexity are we talking about, to not depend on the configuration done by the bootloader?
On Thursday, April 20, 2023 4:03 PM, Vladimir Oltean wrote: > On Thu, Apr 20, 2023 at 09:56:26AM +0800, Jiawen Wu wrote: > > On Wednesday, April 19, 2023 9:20 PM, Vladimir Oltean wrote: > > > On Wed, Apr 19, 2023 at 04:27:37PM +0800, Jiawen Wu wrote: > > > > Add basic support for XPCS using 10GBASE-R interface. This mode will > > > > be extended to use interrupt, so set pcs.poll false. And avoid soft > > > > reset so that the device using this mode is in the default configuration. > > > > > > I'm not clear why the xpcs_soft_reset() call is avoided. Isn't the > > > out-of-reset configuration the "default" one? > > > > Theoretically so, I need to configure 10GBASE-R mode after reset. But this > > configuration involves board info to configure PMA, etc., I'd like to implement > > it in the next patch. Now the "default" configuration refers to the mode in > > which the firmware is configured. > > How much extra complexity are we talking about, to not depend on the > configuration done by the bootloader? > It needs to implement compat->pma_config, and add a flag in struct dw_xpcs to indicate board with specific pma configuration. For 10GBASE-R interface, it relatively simple, but a bit more complicate for 1000BASE-X since there are logic conflicts in xpcs_do_config(), I haven't resolved yet. In addition, reconfiguring XPCS will cause some known issues that I need to workaround in the ethernet driver. So I'd like to add configuration when I implement rate switching. There is a piece codes for my test: +static int xpcs_read_pma(struct dw_xpcs *xpcs, int reg) +{ + return xpcs_read(xpcs, MDIO_MMD_PMAPMD, DW_PMA_MMD + reg); +} + +static int xpcs_write_pma(struct dw_xpcs *xpcs, int reg, u16 val) +{ + return xpcs_write(xpcs, MDIO_MMD_PMAPMD, DW_PMA_MMD + reg, val); +} + +static int xpcs_poll_power_up(struct dw_xpcs *xpcs) +{ + int val, ret; + + /* Wait xpcs power-up good */ + ret = read_poll_timeout(xpcs_read_vpcs, val, + (val & DW_VR_XS_PCS_DIG_STS_PSEQ_ST) == + DW_VR_XS_PCS_DIG_STS_PSEQ_ST_GOOD, + 10000, 1000000, false, + xpcs, DW_VR_XS_PCS_DIG_STS); + if (ret < 0) + pr_err("%s: xpcs power-up timeout\n", __func__); + + return ret; +} + +static int xpcs_pma_init_done(struct dw_xpcs *xpcs) +{ + int val, ret; + + xpcs_write_vpcs(xpcs, DW_VR_XS_PCS_DIG_CTRL1, + DW_VR_XS_PCS_DIG_CTRL1_VR_RST | + DW_VR_XS_PCS_DIG_CTRL1_EN_VSMMD1); + + /* wait pma initialization done */ + ret = read_poll_timeout(xpcs_read_vpcs, val, + !(val & DW_VR_XS_PCS_DIG_CTRL1_VR_RST), + 100000, 10000000, false, + xpcs, DW_VR_XS_PCS_DIG_CTRL1); + if (ret < 0) + pr_err("%s: xpcs pma initialization timeout\n", __func__); + + return ret; +} + +static int xpcs_10gbaser_pma_config_wx(struct dw_xpcs *xpcs) +{ + int val, ret; + + ret = xpcs_poll_power_up(xpcs); + if (ret < 0) + return ret; + + xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2, MDIO_PCS_CTRL2_10GBR); + val = xpcs_read(xpcs, MDIO_MMD_PMAPMD, MDIO_CTRL1); + val |= MDIO_CTRL1_SPEED10G; + xpcs_write(xpcs, MDIO_MMD_PMAPMD, MDIO_CTRL1, val); + + xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL0, 0x21); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL3, 0); + val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_TX_GENCTL1); + val = u16_replace_bits(val, 0x5, DW_VR_XS_PMA_TX_GENCTL1_VBOOST_LVL); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_GENCTL1, val); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_MISC_CTL0, 0xCF00); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_VCO_CAL_LD0, 0x549); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_VCO_CAL_REF0, 0x29); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_RATE_CTL, 0); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_RATE_CTL, 0); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_TX_GEN_CTL2, 0x300); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_GEN_CTL2, 0x300); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_MPLLA_CTL2, 0x600); + + xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL0, 0x45); + val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_RX_EQ_ATTN_CTL); + val &= ~DW_VR_XS_PMA_RX_EQ_ATTN_LVL0; + xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_ATTN_CTL, val); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_DFE_TAP_CTL0, 0xBE); + val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_AFE_DFE_ENABLE); + val &= ~(DW_VR_XS_PMA_DFE_EN_0 | DW_VR_XS_PMA_AFE_EN_0); + xpcs_write_pma(xpcs, DW_VR_XS_PMA_AFE_DFE_ENABLE, val); + val = xpcs_read_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL4); + val &= ~DW_VR_XS_PMA_RX_EQ_CTL4_CONT_ADAPT0; + xpcs_write_pma(xpcs, DW_VR_XS_PMA_RX_EQ_CTL4, val); + + return xpcs_pma_init_done(xpcs); +} + +static int xpcs_10gbaser_pma_config(struct dw_xpcs *xpcs) +{ + if (xpcs->flags & DW_MODEL_WANGXUN_SP) + return xpcs_10gbaser_pma_config_wx(xpcs); + + return 0; +} +
On Thu, Apr 20, 2023 at 04:38:48PM +0800, Jiawen Wu wrote: > It needs to implement compat->pma_config, and add a flag in struct dw_xpcs > to indicate board with specific pma configuration. For 10GBASE-R interface, it > relatively simple, but a bit more complicate for 1000BASE-X since there are > logic conflicts in xpcs_do_config(), I haven't resolved yet. > > In addition, reconfiguring XPCS will cause some known issues that I need to > workaround in the ethernet driver. So I'd like to add configuration when I > implement rate switching. > > There is a piece codes for my test: The PMA initialization procedure looks pretty clean to me (although I'm not clear why it depends upon xpcs->flags & DW_MODEL_WANGXUN_SP when the registers seem to be present in the common databook), and having it in the XPCS driver seems much preferable to depending on an unknown previous initialization stage. Could you detail a bit the known issues and the 1000BASE-X conflicts in xpcs_do_config()?
On Thursday, April 20, 2023 4:52 PM, Vladimir Oltean wrote: > On Thu, Apr 20, 2023 at 04:38:48PM +0800, Jiawen Wu wrote: > > It needs to implement compat->pma_config, and add a flag in struct dw_xpcs > > to indicate board with specific pma configuration. For 10GBASE-R interface, it > > relatively simple, but a bit more complicate for 1000BASE-X since there are > > logic conflicts in xpcs_do_config(), I haven't resolved yet. > > > > In addition, reconfiguring XPCS will cause some known issues that I need to > > workaround in the ethernet driver. So I'd like to add configuration when I > > implement rate switching. > > > > There is a piece codes for my test: > > The PMA initialization procedure looks pretty clean to me (although I'm > not clear why it depends upon xpcs->flags & DW_MODEL_WANGXUN_SP when the > registers seem to be present in the common databook), and having it in > the XPCS driver seems much preferable to depending on an unknown previous > initialization stage. The values configured in PMA depend on the board signal quality, Synopsys once provided the values based on our board information, but we don't know the details of the computation. So I don't think it's universal. > > Could you detail a bit the known issues and the 1000BASE-X conflicts in > xpcs_do_config()? > Known issue is that traffic must be totally stopped while the PMA is being configured. And XPCS should add a judgment that PMA only need to be reconfigured when interface is changed. In 1000BASE-X interface, for my current testing, PMA configuration should precede AN configuration, and need to set PCS_DIG_CTRL1 reg? My test code for AN config: +static int xpcs_config_aneg_c37_1000basex_wx(struct dw_xpcs *xpcs, unsigned int mode, + const unsigned long *advertising) +{ + + xpcs_write(xpcs, MDIO_MMD_PCS, DW_VR_MII_DIG_CTRL1, 0x3002); + xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, 0x0109); + xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, 0x0200); + ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1); + ret |= BMCR_ANENABLE; + xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret); +}