Re: [PATCH net-next v5 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988

[SkyLake Huang (黃啟澤)]

On Thu, 2024-05-30 at 11:35 +0100, Russell King (Oracle) wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote:
> > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
> > +{
> > +bool changed = false;
> > +u32 adv;
> > +int ret;
> > +
> > +/* In fact, if we disable autoneg, we can't link up correctly:
> > + *  2.5G/1G: Need AN to exchange master/slave information.
> > + *  100M: Without AN, link starts at half duplex(According to IEEE
> 802.3-2018),
> > + *        which this phy doesn't support.
> > + *   10M: Deprecated in this ethernet phy.
> > + */
> > +if (phydev->autoneg == AUTONEG_DISABLE)
> > +return -EOPNOTSUPP;
> 
> We have another driver (stmmac) where a platform driver is wanting to
> put a hack in the ksettings_set() ethtool path to error out on
> disabling AN for 1G speeds. This sounds like something that is
> applicable to more than one hardware (and I've been wondering whether
> it is universally true that 1G copper links and faster all require
> AN to function.)
> 
> Thus, I'm wondering whether this is something that the core code
> should
> be doing.
> 
Yeah..As far as I know, 1G/2.5G/5G/10G speed require AN to decide
master/slave role. Actually I can use force mode by calling
genphy_c45_pma_set_forced, which will set correspoding C45 registers.
However, after that, this 2.5G PHY can't still link up with partners.

I'll leave EOPNOTSUPP here temporarily. Hope phylib can be patched
someday.

> > +/* This phy can't handle collision, and neither can (XFI)MAC it's
> connected to.
> > + * Although it can do HDX handshake, it doesn't support CSMA/CD
> that HDX requires.
> > + */
> 
> What the MAC can and can't do really has little bearing on what link
> modes the PHY driver should be providing. It is the responsibility of
> the MAC driver to appropriately change what is supported when
> attaching
> to the PHY. If using phylink, this is done by phylink via the MAC
> driver
> telling phylink what it is capable of via mac_capabilities.
> 
> > +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device
> *phydev,
> > +      phy_interface_t iface)
> > +{
> > +if (iface == PHY_INTERFACE_MODE_XGMII)
> > +return RATE_MATCH_PAUSE;
> 
> You mention above XFI...
> 
> XFI is 10GBASE-R protocol to XFP module electrical standards.
> SFI is 10GBASE-R protocol to SFP+ module electrical standards.
> 
> phy_interface_t is interested in the protocol. So, given that you
> mention XFI, why doesn't this test for PHY_INTERFACE_MODE_10GBASER?
> 
We have 2 XFI-MAC on mt7988 platform. One is connected to internal
2.5Gphy(SoC built-in), as we discussed here (We don't test this phy for
10G speed.) Another one is connected to external 10G phy.

> > +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> > +{
> > +struct mtk_i2p5ge_phy_priv *priv;
> > +
> > +priv = devm_kzalloc(&phydev->mdio.dev,
> > +    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
> > +if (!priv)
> > +return -ENOMEM;
> > +
> > +switch (phydev->drv->phy_id) {
> > +case MTK_2P5GPHY_ID_MT7988:
> > +/* The original hardware only sets MDIO_DEVS_PMAPMD */
> > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN |
> > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2);
> 
> No need for parens on the RHS. The RHS is an expression in its own
> right, and there's no point in putting parens around the expression
> to turn it into another expression!
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Do you mean these two line?
+phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN |
+ MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2);

What do you mean by "RHS is an expression in its own right"?
I put parens here to enhance readability so we don't need check
operator precedence again.

Sky