Date:   Tue, 12 Oct 2021 16:04:29 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Alvin Šipraga <alvin@...s.dk>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Michael Rasmussen <mir@...g-olufsen.dk>,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 5/6] net: dsa: realtek-smi: add rtl8365mb
 subdriver for RTL8365MB-VC

On Tue, Oct 12, 2021 at 02:35:54PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@...g-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 <mir@...g-olufsen.dk>
> Signed-off-by: Michael Rasmussen <mir@...g-olufsen.dk>
> Signed-off-by: Alvin Šipraga <alsi@...g-olufsen.dk>
> ---

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

Just one comment below

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

> +
> +	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;
> +}

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

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

> +		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;
> +	}
> +}

Powered by blists - more mailing lists