Message-ID: <6848079b-eb8b-91c9-f64d-5f0c3fde36ec@bang-olufsen.dk>
Date:   Tue, 12 Oct 2021 13:22:39 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>,
        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>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...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 10/12/21 3:04 PM, Vladimir Oltean wrote:
> 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/>> 

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?

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

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?


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