Messages in this thread | | | Date | Mon, 19 Sep 2022 12:23:57 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v2 5/5] net: mediatek: sgmii: refactor power cycling into mtk_pcs_config() |
| |
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.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |