Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

From: Vladimir Oltean <olteanv@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Ansuel Smith <ansuelsmth@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config
Date: Sun, 10 Oct 2021 18:53:28 +0300	[thread overview]
Message-ID: <20211010155328.zwhs2vpdpddazcmq@skbuf> (raw)
In-Reply-To: <YWMEOLueatMCTS2Z@lunn.ch>

On Sun, Oct 10, 2021 at 05:18:16PM +0200, Andrew Lunn wrote:
> > > -{
> > > -	struct device_node *port_dn;
> > > -	phy_interface_t mode;
> > > -	struct dsa_port *dp;
> > > -	u32 val;
> > > -
> > > -	/* CPU port is already checked */
> > > -	dp = dsa_to_port(priv->ds, 0);
> > > -
> > > -	port_dn = dp->dn;
> > > -
> > > -	/* Check if port 0 is set to the correct type */
> > > -	of_get_phy_mode(port_dn, &mode);
> > > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > > -		return 0;
> > > -	}
> > > -
> > > -	switch (mode) {
> > > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> > 
> > Also, since you touch this area.
> > There have been tons of discussions on this topic, but I believe that
> > your interpretation of the RGMII delays is wrong.
> > Basically a MAC should not apply delays based on the phy-mode string (so
> > it should treat "rgmii" same as "rgmii-id"), but based on the value of
> > "rx-internal-delay-ps" and "tx-internal-delay-ps".
> > The phy-mode is for a PHY to use.
> 
> There is one exception to this, when the MAC is taking the place of a
> PHY, i.e. CPU port. You need delays added somewhere, and the mv88e6xxx
> driver will look at the phy-mode in this case.

Yes, and I don't think it's an actual exception, it's still back-to-back MACs.
It is true on the other hand that some drivers don't seem to behave this way.
Russell has been suggesting that the phy-mode should be treated the same
way regardless of whether a PHY is attached or not. It would be nice to
reach a common agreement so that we know what to suggest people to do.

> And i think in general, a DSA driver needs this for the CPU port.

Not saying it doesn't need it, just saying that the need for the
{rx,tx}-internal-delay-ps MAC property was not recognized at the time
and that's why the vast majority of drivers don't act upon it.
The qca8k driver is somewhat special in that it does parse these new
properties, but at the same time also parse the phy-mode, that is kind
of strange.