netdev - Re: [PATCH net-next v2] net: dsa: mv88e6060: add phylink_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZNUV2VzY01TWVSgk@shell.armlinux.org.uk>
Date: Thu, 10 Aug 2023 17:52:41 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Sergei Antonov <saproj@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: dsa: mv88e6060: add phylink_get_caps
 implementation

On Thu, Aug 10, 2023 at 07:44:41PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Wed, Aug 09, 2023 at 03:46:03PM +0100, Russell King (Oracle) wrote:
> > Add a phylink_get_caps implementation for Marvell 88e6060 DSA switch.
> > This is a fast ethernet switch, with internal PHYs for ports 0 through
> > 4. Port 4 also supports MII, REVMII, REVRMII and SNI. Port 5 supports
> > MII, REVMII, REVRMII and SNI without an internal PHY.
> > 
> > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> > Sergei,
> > 
> > Would it be possible for you to check that this patch works with your
> > setup please?
> > 
> > Thanks!
> > 
> >  drivers/net/dsa/mv88e6060.c | 46 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> > index fdda62d6eb16..0e776be5e941 100644
> > --- a/drivers/net/dsa/mv88e6060.c
> > +++ b/drivers/net/dsa/mv88e6060.c
> > @@ -247,11 +247,57 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
> >  	return reg_write(priv, addr, regnum, val);
> >  }
> >  
> > +static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port,
> > +				       struct phylink_config *config)
> > +{
> > +	unsigned long *interfaces = config->supported_interfaces;
> > +	struct mv88e6060_priv *priv = ds->priv;
> > +	int addr = REG_PORT(port);
> > +	int ret;
> > +
> > +	ret = reg_read(priv, addr, PORT_STATUS);
> > +	if (ret < 0) {
> > +		dev_err(ds->dev,
> > +			"port %d: unable to read status register: %pe\n",
> > +			port, ERR_PTR(ret));
> > +		return;
> > +	}
> > +
> > +	if (!(ret & PORT_STATUS_PORTMODE)) {
> > +		/* Port configured in SNI mode (acts as a 10Mbps PHY) */
> > +		config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE;
> > +		/* I don't think SNI is SMII - SMII has a sync signal, and
> > +		 * SNI doesn't.
> > +		 */
> > +		__set_bit(PHY_INTERFACE_MODE_SMII, interfaces);
> 
> I don't think that SNI == SMII either.
> 
> From what I could gather (datasheets of implementations in the wild,
> rather than any official spec):
> KSZ8895: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8895MQX-RQX-FQX-MLX-Integrated-5-Port-10-100-Managed-Ethernet-Switch-with-MII-RMII-Interface-DS00002246C.pdf
> DP83910A: https://www.ti.com/lit/ds/symlink/dp83910a.pdf
> RTL8201BL: https://file.elecfans.com/web1/M00/9A/9F/pIYBAF0fDAuANJTlAAqoYDastvw919.pdf
> 
> SNI (7-wire Serial Network Interface) has the same structural pin layout
> as the parallel MII/Rev-MII, except for the fact that RXD[3:0] and
> TXD[3:0] became RXD0 and TXD0, resulting in an effectively "serial"
> interface and reducing the baud rate of the 100 Mbps MII to a quarter,
> and the TX_CLK and RX_CLK signals also operate at a reduced 10 MHz
> rather than MII's 25 MHz, to provide a further 2.5x baud rate reduction
> down to 10 Mbps. It was a once popular (in the 1990s) interface mode
> between a MAC and a 10Mbps-only PHY.
> 
> If we compare that to SMII (Serial MII), I could only find this document here:
> https://opencores.org/ocsvn/smii/smii/trunk/doc/SMII.pdf
> 
> but it appears to be quite different. SMII seems to be a gasket/encapsulation
> module which serializes both the data and control signals of up to 4 10/100
> Mbps MACs, which can be connected to a quad SMII PHY. The resulting
> (cumulated, or individual) bandwidth is much larger than that of SNI, too.
> 
> The pinout of SMII is:
> - one RX and one TX signal for each MAC. Data transfer consists of
>   segments (10 serial bits on these lines). The bits in each segment are:
>   RX direction: CRS, RX_DV, RXD0, RXD1, RXD2, RXD3, RXD4, RXD5, RXD6, RXD7
>   TX direction: TX_ER, TX_EN, TXD0, TXD1, TXD2, TXD3, TXD4, TXD5, TXD6, TXD7
> - SYNC: denotes the beginning of a new segment
> - CLK: denotes the beginning of a new bit
> 
> So, I guess we have to introduce PHY_INTERFACE_MODE_SNI rather than
> pretend it is the same as SMII.

I wonder whether we have any implementation using SNI mode. I couldn't
find anything in the in-kernel dts files for this driver, the only
dts we have is one that was posted on-list recently, and that was using
MII at 100Mbps:

https://lore.kernel.org/r/CABikg9zfGVEJsWf7eq=K5oKQozt86LLn-rzMaVmycekXkQEa8Q@mail.gmail.com

No one would be able to specify "sni" in their dts, so maybe for the
sake of simplicity, we shouldn't detect whether it's in SNI mode, and
just use MII, and limit the speed to just 10Mbps?

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