On 10/12/21 3:04 PM, Vladimir Oltean wrote:

On Tue, Oct 12, 2021 at 02:35:54PM +0200, Alvin Šipraga wrote:

quoted

From: Alvin Šipraga <alsi@bang-olufsen.dk>

This patch adds a realtek-smi subdriver for the RTL8365MB-VC 4+1 port
10/100/1000M switch controller. The driver has been developed based on a
GPL-licensed OS-agnostic Realtek vendor driver known as rtl8367c found
in the OpenWrt source tree.

Despite the name, the RTL8365MB-VC has an entirely different register
layout to the already-supported RTL8366RB ASIC. Notwithstanding this,
the structure of the rtl8365mb subdriver is based on the rtl8366rb
subdriver and makes use of the rtl8366 helper library for setup of the
SMI interface and handling of MIB counters. Like the 'rb, it establishes
its own irqchip to handle cascaded PHY link status interrupts.

The RTL8365MB-VC switch is capable of offloading a large number of
features from the software, but this patch introduces only the most
basic DSA driver functionality. The ports always function as standalone
ports, with bridging handled in software.

One more thing. Realtek's nomenclature for switches makes it hard to
know exactly what other ASICs might be supported by this driver. The
vendor driver goes by the name rtl8367c, but as far as I can tell, no
chip actually exists under this name. As such, the subdriver is named
rtl8365mb to emphasize the potentially limited support. But it is clear
from the vendor sources that a number of other more advanced switches
share a similar register layout, and further support should not be too
hard to add given access to the relevant hardware. With this in mind,
the subdriver has been written with as few assumptions about the
particular chip as is reasonable. But the RTL8365MB-VC is the only
hardware I have available, so some further work is surely needed.

Co-developed-by: Michael Rasmussen <redacted>
Signed-off-by: Michael Rasmussen <redacted>
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Just one comment below

quoted

+static int rtl8365mb_ext_config_rgmii(struct realtek_smi *smi, int port,
+				      phy_interface_t interface)
+{
+	int tx_delay = 0;
+	int rx_delay = 0;
+	int ext_port;
+	int ret;
+
+	if (port == smi->cpu_port) {
+		ext_port = PORT_NUM_L2E(port);
+	} else {
+		dev_err(smi->dev, "only one EXT port is currently supported\n");
+		return -EINVAL;
+	}
+
+	/* Set the RGMII TX/RX delay
+	 *
+	 * The Realtek vendor driver indicates the following possible
+	 * configuration settings:
+	 *
+	 *   TX delay:
+	 *     0 = no delay, 1 = 2 ns delay
+	 *   RX delay:
+	 *     0 = no delay, 7 = maximum delay
+	 *     No units are specified, but there are a total of 8 steps.
+	 *
+	 * The vendor driver also states that this must be configured *before*
+	 * forcing the external interface into a particular mode, which is done
+	 * in the rtl8365mb_phylink_mac_link_{up,down} functions.
+	 *
+	 * NOTE: For now this is hardcoded to tx_delay = 1, rx_delay = 4.
+	 */
+	if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_TXID)
+		tx_delay = 1; /* 2 ns */
+
+	if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    interface == PHY_INTERFACE_MODE_RGMII_RXID)
+		rx_delay = 4;

There is this ongoing discussion that we have been interpreting the
meaning of "phy-mode" incorrectly for RGMII all along. The conclusion
seems to be that for a PHY driver, it is okay to configure its internal
delay lines based on the value of the phy-mode string, but for a MAC
driver it is not. The only viable option for a MAC driver to configure
its internal delays is based on parsing some new device tree properties
called rx-internal-delay-ps and tx-internal-delay-ps.
Since you do not seem to have any baggage to support here (new driver),
could you please just accept any PHY_INTERFACE_MODE_RGMII* value and
apply delays (or not) based on those other OF properties?
https://patchwork.kernel.org/project/netdevbpf/patch/20210723173108.459770-6-prasanna.vengateshan@microchip.com/>> 

Ugh, I remember my head spinning when I first looked into this. But OK, 
I can do as you suggest.

Just to clarify: if the *-internal-delay-ps property is missing, you are 
saying that I should set the delay to 0 rather than my defaults (tx=1, 
rx=4), right?

quoted

+
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_EXT_RGMXF_REG(ext_port),
+		RTL8365MB_EXT_RGMXF_TXDELAY_MASK |
+			RTL8365MB_EXT_RGMXF_RXDELAY_MASK,
+		FIELD_PREP(RTL8365MB_EXT_RGMXF_TXDELAY_MASK, tx_delay) |
+			FIELD_PREP(RTL8365MB_EXT_RGMXF_RXDELAY_MASK, rx_delay));
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(
+		smi->map, RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_port),
+		RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_port),
+		RTL8365MB_EXT_PORT_MODE_RGMII
+			<< RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(
+				   ext_port));
+	if (ret)
+		return ret;
+
+	return 0;
+}

quoted

+static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
+					 unsigned int mode,
+					 const struct phylink_link_state *state)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (!rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
+		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
+			phy_modes(state->interface), port);
+		return;
+	}
+
+	/* If port MAC is connected to an internal PHY, we have nothing to do */
+	if (dsa_is_user_port(ds, port))
+		return;
+
+	if (mode != MLO_AN_PHY && mode != MLO_AN_FIXED) {
+		dev_err(smi->dev,
+			"port %d supports only conventional PHY or fixed-link\n",
+			port);
+		return;
+	}
+
+	if (phy_interface_mode_is_rgmii(state->interface)) {
+		ret = rtl8365mb_ext_config_rgmii(smi, port, state->interface);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to configure RGMII mode on port %d: %d\n",
+				port, ret);
+		return;
+	}
+
+	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
+	 * supports
+	 */
+}
+
+static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					    unsigned int mode,
+					    phy_interface_t interface)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (dsa_is_cpu_port(ds, port)) {

I assume the "dsa_is_cpu_port()" check here can also be replaced with
"phy_interface_mode_is_rgmii(interface)"? Can you please do that for
consistency?

Consistency with what exactly? All I'm saying with this code is that for 
CPU ports, we have to force some mode on it in response to mac_link_up. 
This doesn't apply to user ports because the PHY is always internal to 
the switch (this appears to be the case for all switches in the 
rtl8365mb-like family). Or are you wondering about a scenario where the 
port is treated as a DSA port?

quoted

+		ret = rtl8365mb_ext_config_forcemode(smi, port, false, 0, 0,
+						     false, false);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to reset forced mode on port %d: %d\n",
+				port, ret);
+
+		return;
+	}
+}
+
+static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
+					  unsigned int mode,
+					  phy_interface_t interface,
+					  struct phy_device *phydev, int speed,
+					  int duplex, bool tx_pause,
+					  bool rx_pause)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	if (dsa_is_cpu_port(ds, port)) {

and here

quoted

+		ret = rtl8365mb_ext_config_forcemode(smi, port, true, speed,
+						     duplex, tx_pause,
+						     rx_pause);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to force mode on port %d: %d\n", port,
+				ret);
+
+		return;
+	}
+}