| Message ID | 20220919083713.730512-6-lynxis@fe80.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mediatek: sgmii stability | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Mon, Sep 19, 2022 at 10:37:12AM +0200, Alexander Couzens wrote: > Both code paths (autonegotiated and force mode) are power cycling > the phy. Move power cycling code to the caller to remove code > duplicity. I think we can do more consolidation here - and it probably makes sense to do in another patch. > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > index 4c8e8c7b1d32..50f605208295 100644 > --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > @@ -25,9 +25,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface > { > unsigned int val; > > - /* PHYA power down */ > - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); > - > /* Set SGMII phy speed */ > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > val &= ~RG_PHY_SPEED_MASK; > @@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, > { > unsigned int val; > > - /* PHYA power down */ > - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); > - > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > val &= ~RG_PHY_SPEED_MASK; > if (interface == PHY_INTERFACE_MODE_2500BASEX) After powering the PHY down, the next thing that is done is to configure the speed. Even with my comments on patch 4, this can still be consolidated. > @@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, > struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs); unsigned int val; > int err = 0; > > + /* PHYA power down */ > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); > + regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); val &= ~RG_PHY_SPEED_MASK; if (interface == PHY_INTERFACE_MODE_2500BASEX) val |= RG_PHY_SPEED_3_125G; regmap_write(mpcs->regmap, mpcs->ana_rgc3, val); which would make logical sense to do here, so we always configure the speed for the PCS correctly. That then leaves the configuration of SGMSYS_PCS_CONTROL_1 and SGMSYS_SGMII_MODE, which I think could also be consolidated, but I'll leave that to those with the hardware to make that decision. Reading between the lines of the code in this driver, it looks to me like this hardware supports only SGMII, but doesn't actually support 1000base-X and 2500base-X with negotiation. Is that correct? If so, it would be good to add a mtk_pcs_validate() function that clears ETHTOOL_LINK_MODE_Autoneg_BIT for these modes. Thanks.
> > - /* PHYA power down */ > > - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > SGMII_PHYA_PWD); - > > /* Set SGMII phy speed */ > > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > > val &= ~RG_PHY_SPEED_MASK; > > @@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct > > mtk_pcs *mpcs, { > > unsigned int val; > > > > - /* PHYA power down */ > > - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > SGMII_PHYA_PWD); - > > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > > val &= ~RG_PHY_SPEED_MASK; > > if (interface == PHY_INTERFACE_MODE_2500BASEX) > > After powering the PHY down, the next thing that is done is to > configure the speed. Even with my comments on patch 4, this can still > be consolidated. I'll move more code out of the functions. > > > @@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs > > *pcs, unsigned int mode, struct mtk_pcs *mpcs = > > pcs_to_mtk_pcs(pcs); > > unsigned int val; > > > int err = 0; > > > > + /* PHYA power down */ > > + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, > > SGMII_PHYA_PWD); > > + > > regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > val &= ~RG_PHY_SPEED_MASK; > if (interface == PHY_INTERFACE_MODE_2500BASEX) > val |= RG_PHY_SPEED_3_125G; > regmap_write(mpcs->regmap, mpcs->ana_rgc3, val); > > which would make logical sense to do here, so we always configure the > speed for the PCS correctly. > > That then leaves the configuration of SGMSYS_PCS_CONTROL_1 and > SGMSYS_SGMII_MODE, which I think could also be consolidated, but I'll > leave that to those with the hardware to make that decision. > > Reading between the lines of the code in this driver, it looks to me > like this hardware supports only SGMII, but doesn't actually support > 1000base-X and 2500base-X with negotiation. Is that correct? If so, > it would be good to add a mtk_pcs_validate() function that clears > ETHTOOL_LINK_MODE_Autoneg_BIT for these modes. I don't know. I don't have hardware to debug the serdes interface further. I only have a test board with mt7622 soc connect via SGMII/2500basex to a realtek phy (rtl8221). Maybe the maintainers from mediatek could share some knowledge if the SGMII block supports 1000/2500basex autoneg? At least with the public available datasheets (mt7531, mt7622) doesn't explain it further. I could also imagine we need to modify the page register (PCS_SPEED_ABILITY) and link timer to get autoneg for 1000basex/2500basex working. Best, lynxis
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 4c8e8c7b1d32..50f605208295 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -25,9 +25,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface { unsigned int val; - /* PHYA power down */ - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); - /* Set SGMII phy speed */ regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); val &= ~RG_PHY_SPEED_MASK; @@ -48,18 +45,6 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs, phy_interface_t interface val |= SGMII_AN_RESTART | SGMII_AN_ENABLE; regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val); - /* Release PHYA power down state - * Only removing bit SGMII_PHYA_PWD isn't enough. - * There are cases when the SGMII_PHYA_PWD register contains 0x9 which - * prevents SGMII from working. The SGMII still shows link but no traffic - * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was - * taken from a good working state of the SGMII interface. - * Unknown how much the QPHY needs but it is racy without a sleep. - * Tested on mt7622 & mt7986. - */ - usleep_range(50, 100); - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); - return 0; } @@ -72,9 +57,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, { unsigned int val; - /* PHYA power down */ - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); - regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); val &= ~RG_PHY_SPEED_MASK; if (interface == PHY_INTERFACE_MODE_2500BASEX) @@ -92,18 +74,6 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs, val |= SGMII_SPEED_1000; regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); - /* Release PHYA power down state - * Only removing bit SGMII_PHYA_PWD isn't enough. - * There are cases when the SGMII_PHYA_PWD register contains 0x9 which - * prevents SGMII from working. The SGMII still shows link but no traffic - * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was - * taken from a good working state of the SGMII interface. - * Unknown how much the QPHY needs but it is racy without a sleep. - * Tested on mt7622 & mt7986. - */ - usleep_range(50, 100); - regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); - return 0; } @@ -115,12 +85,27 @@ static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode, struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs); int err = 0; + /* PHYA power down */ + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD); + /* Setup SGMIISYS with the determined property */ if (interface != PHY_INTERFACE_MODE_SGMII) err = mtk_pcs_setup_mode_force(mpcs, interface); else if (phylink_autoneg_inband(mode)) err = mtk_pcs_setup_mode_an(mpcs, interface); + /* Release PHYA power down state + * Only removing bit SGMII_PHYA_PWD isn't enough. + * There are cases when the SGMII_PHYA_PWD register contains 0x9 which + * prevents SGMII from working. The SGMII still shows link but no traffic + * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was + * taken from a good working state of the SGMII interface. + * Unknown how much the QPHY needs but it is racy without a sleep. + * Tested on mt7622 & mt7986. + */ + usleep_range(50, 100); + regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0); + return err; }
Both code paths (autonegotiated and force mode) are power cycling the phy. Move power cycling code to the caller to remove code duplicity. Signed-off-by: Alexander Couzens <lynxis@fe80.eu> --- drivers/net/ethernet/mediatek/mtk_sgmii.c | 45 ++++++++--------------- 1 file changed, 15 insertions(+), 30 deletions(-)