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

[Russell King (Oracle)]

On Thu, May 30, 2024 at 04:25:56PM +0000, SkyLake Huang (黃啟澤) wrote:
> 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.

Please no. "someday" tends to never happen, and you're basically
throwing the problem over the wall to other people to solve who
then have to spot your hack and eventually remove it.

We need this solved properly, not by people hacking drivers. This
is open source, you can propose a patch to phylib to fix this for
everyone.

> > > +/* 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.

I can't parse your response in a meaningful way, to me it doesn't
address my point.

> 
> > > +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.

|= one of the assignment operators, all of which have one of the
lowest precedence. Only the , operator has a lower precedence.
Therefore, everything except , has higher precedence. Therefore,
the parens on the right hand side of |= make no difference.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!