Date: Mon, 3 Jun 2024 07:19:35 +0000
From: SkyLake Huang (黃啟澤)
	<SkyLake.Huang@...iatek.com>
To: "linux@...linux.org.uk" <linux@...linux.org.uk>
CC: "andrew@...n.ch" <andrew@...n.ch>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "dqfext@...il.com" <dqfext@...il.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
	Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "hkallweit1@...il.com"
	<hkallweit1@...il.com>, "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "daniel@...rotopia.org"
	<daniel@...rotopia.org>
Subject: Re: [PATCH net-next v5 5/5] net: phy: add driver for built-in 2.5G
 ethernet PHY on MT7988

On Thu, 2024-05-30 at 17:55 +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 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.
> 
I don't intend to throw  problems to other people. And actually this
isn't a "problem" currently(at least in this driver). IMHO, disabling
AN isn't a normal operation for current ethernet environment. However,
now, ethtool supports this kind of configuration. Maybe we should
prohibit "AN disable" config for certain speed? Or maybe for all
speed? This will take lots of time for discussion. No matter what,
these discussions have little relevance to this driver.

For your reference, there's the same design in en8811h_config_aneg() of
drivers/net/phy/air_en8811h.c.

> > > > +/* 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.
> 
I guess I got your point.
On mt7988 
1st XFI-MAC (XGMAC2): For built-in 2.5Gphy or external 10Gphy
2nd XFI-MAC (XGMAC3): Only for external 10Gphy

Basically, if we use this driver for built-in 2.5Gphy. We'll only pass
phy-mode = "xgmii" or phy-mode = "internal" in dts, i.e,
PHY_INTERFACE_MODE_XGMII or PHY_INTERFACE_MODE_INTERNAL.
Also, we pass phy-mode = "usxgmii" (PHY_INTERFACE_MODE_10GBASER) in dts
once we use external 10Gphy with XGMAC2.

So we don't test PHY_INTERFACE_MODE_10GBASER or
PHY_INTERFACE_MODE_10GKR in driver's rate_matching().

But I think I should change it this way: 

if (iface == PHY_INTERFACE_MODE_XGMII ||
    iface == PHY_INTERFACE_MODE_INTERNAL)
		return RATE_MATCH_PAUSE;
return RATE_MATCH_NONE;

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

I'll fix this in v6.

Sky

Powered by blists - more mailing lists