Messages in this thread | | | Date | Mon, 19 Sep 2022 15:56:06 +0200 | From | Alexander 'lynxis' Couzens <> | Subject | Re: [PATCH net-next v2 5/5] net: mediatek: sgmii: refactor power cycling into mtk_pcs_config() |
| |
> > - /* 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
| |