On Tue, Oct 12, 2021 at 02:35:54PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Michael Rasmussen <mir@xxxxxxxxxxxxxxx>
> Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> ---
Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>
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@xxxxxxxxxxxxx/
> +
> + 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;
> + }
> +}
