Message-ID: <ZNd4AJlLLmszeOxg@shell.armlinux.org.uk>
Date: Sat, 12 Aug 2023 13:16:00 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...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] net: dsa: mark parsed interface mode for legacy
 switch drivers

On Thu, Aug 10, 2023 at 06:16:17PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Tue, Aug 08, 2023 at 03:19:20PM +0100, Russell King (Oracle) wrote:
> > On Tue, Aug 08, 2023 at 04:52:15PM +0300, Vladimir Oltean wrote:
> > > On Tue, Aug 08, 2023 at 01:57:43PM +0100, Russell King (Oracle) wrote:
> > > > Thanks for the r-b.
> > > > 
> > > > At risk of delaying this patch through further discussion... so I'll
> > > > say now that we're going off into discussions about future changes.
> > > > 
> > > > I believe all DSA drivers that provide .phylink_get_caps fill in the
> > > > .mac_capabilities member, which leaves just a few drivers that do not,
> > > > which are:
> > > > 
> > > > $ git grep -l dsa_switch_ops.*= drivers/net/dsa/ | xargs grep -L '\.phylink_get_caps'
> > > > drivers/net/dsa/dsa_loop.c
> > > > drivers/net/dsa/mv88e6060.c
> > > > drivers/net/dsa/realtek/rtl8366rb.c
> > > > drivers/net/dsa/vitesse-vsc73xx-core.c
> > > > 
> > > > I've floated the idea to Linus W and Arinc about setting
> > > > .mac_capabilities in the non-phylink_get_caps path as well, suggesting:
> > > 
> > > Not sure what you mean by "in the non-phylink_get_caps path" (what is
> > > that other path). Don't you mean that we should implement phylink_get_caps()
> > > for these drivers, to have a unified code flow for everyone?
> > 
> > I meant this:
> > 
> >                 /* For legacy drivers */
> >                 if (mode != PHY_INTERFACE_MODE_NA) {
> >                         __set_bit(mode, dp->pl_config.supported_interfaces);
> >                 } else {
> >                         __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >                                   dp->pl_config.supported_interfaces);
> >                         __set_bit(PHY_INTERFACE_MODE_GMII,
> >                                   dp->pl_config.supported_interfaces);
> >                 }
> 
> Ah, ok, you'd like a built-in assumption of the mac_capabilities in
> dsa_port_phylink_create().
> 
> > but ultimately yes, having the DSA phylink_get_caps method mandatory
> > would be excellent, but I don't think we have sufficient information
> > to do that.
> > 
> > For example, what interface modes does the Vitesse DSA switch support?
> > What speeds? Does it support pause? Does it vary depending on port?
> 
> I only have a VSC7395 datasheet which was shared with me by Linus (and
> that link is no longer functional).
> 
> This switch supports MII/REV-MII/GMII/RGMII on MAC 6, and MACs 0-4 are
> connected to internal PHYs (yes, there is no port 5, also see the
> comment in vsc73xx_probe()).
> 
> Based on vsc73xx_init_port() and vsc73xx_adjust_enable_port(), I guess
> all ports support flow control, and thus, PHYs should advertise it.
> 
> I don't have a datasheet for the other switches supported by the driver:
> 
>  * Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
>  * Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
>  * Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
>  * Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
> 
> but based on the common treatment in vsc73xx_init_port(), I'd say that
> on all models, port 6 (CPU_PORT) is the xMII port and all the others are
> internal PHY ports, and all support the same configuration. So a
> phylink_get_caps() implementation could probably also do one of two
> things, based on "if (port == CPU_PORT)".
...
> That could be an option, but I think the volume of switches is low
> enough that we could just consider converting them all.

It's actually better - the vitesse driver uses .adjust_link, which
means it's excluded from phylink for the DSA/CPU ports.

So, I think for Vitesse, we just need to set INTERNAL and GMII
for ports != CPU_PORT, speeds 10..1000Mbps at FD and HD, and also
sym and asym pause.

That leaves the RTL836x driver, for which I've found:

http://realtek.info/pdf/rtl8366_8369_datasheet_1-1.pdf

and that indicates that the user ports use RSGMII which is SGMII with
a clock in one direction. The only dts I can find is:

arch/arm/boot/dts/gemini-dlink-dir-685.dts

which doesn't specify phy-mode for these, so that'll be using the
phylib default of GMII.

Port 5 supports MII/GMII/RGMII by hardware strapping, which has three
modes of operation:

  MII/GMII (mac mode): 1G (GMII) when linked at 1G, otherwise 100M (MII)
  RGMII: only 1G
  MII (phy mode): only 100M FD supported. Flow control by hardware
  strapping but is readable via a register, but omits to say where.

There's also some suggestion that asym flow control is supported in 1G
mode - but it doesn't say whether it's supported in 100M (and since
IEEE 802.3 advertisements do not make this conditional on speed...
yea, sounds like a slightly broken design to me.)

So for realtek, I propose (completely untested):

8<====
From: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Subject: [PATCH net-next] net: dsa: realtek: add phylink_get_caps
 implementation

The user ports use RSGMII, but we don't have that, and DT doesn't
specify a phy interface mode, so phylib defaults to GMII. These support
1G, 100M and 10M with flow control. It is unknown whether asymetric
pause is supported at all speeds.

The CPU port uses MII/GMII/RGMII/REVMII by hardware pin strapping,
and support speeds specific to each, with full duplex only supported
in some modes. Flow control may be supported again by hardware pin
strapping, and theoretically is readable through a register but no
information is given in the datasheet for that.

So, we do a best efforts - and be lenient.

Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 25f88022b9e4..76b5c43e1430 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -1049,6 +1049,32 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
 	return DSA_TAG_PROTO_RTL4_A;
 }
 
+static void rtl8366rb_phylink_get_caps(struct dsa_switch *ds, int port,
+				       struct phylink_config *config)
+{
+	unsigned long *interfaces = config->supported_interfaces;
+	struct realtek_priv *priv = ds->priv;
+
+	if (port == priv->cpu_port) {
+		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+		/* Only supports 100M FD */
+		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
+		/* Only supports 1G FD */
+		__set_bit(PHY_INTERFACE_MODE_RGMII, interfaces);
+
+		config->mac_capabilities = MAC_1000 | MAC_100 |
+					   MAC_SYM_PAUSE;
+	}
+
+	/* RSGMII port, but we don't have that, and we don't
+	 * specify in DT, so phylib uses the default of GMII
+	 */
+	__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+	config->mac_capabilities = MAC_1000 | MAC_100 | MAC_10 |
+				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+}
+
 static void
 rtl8366rb_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
 		      phy_interface_t interface, struct phy_device *phydev,
@@ -1796,6 +1822,7 @@ static int rtl8366rb_detect(struct realtek_priv *priv)
 static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = {
 	.get_tag_protocol = rtl8366_get_tag_protocol,
 	.setup = rtl8366rb_setup,
+	.phylink_get_caps = rtl8366rb_phylink_get_caps,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
 	.phylink_mac_link_down = rtl8366rb_mac_link_down,
 	.get_strings = rtl8366_get_strings,
@@ -1821,6 +1848,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = {
 	.setup = rtl8366rb_setup,
 	.phy_read = rtl8366rb_dsa_phy_read,
 	.phy_write = rtl8366rb_dsa_phy_write,
+	.phylink_get_caps = rtl8366rb_phylink_get_caps,
 	.phylink_mac_link_up = rtl8366rb_mac_link_up,
 	.phylink_mac_link_down = rtl8366rb_mac_link_down,
 	.get_strings = rtl8366_get_strings,
-- 
2.30.2


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

Powered by blists - more mailing lists