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