Re: [PATCH net-next 5/6] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On 10/12/21 3:22 PM, Alvin Šipraga wrote:
> 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@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 <mir@bang-olufsen.dk>
>>> Signed-off-by: Michael Rasmussen <mir@bang-olufsen.dk>
>>> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
>>> ---
>>
>> Reviewed-by: Vladimir Oltean <olteanv@gmail.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?

Another problem is that for the RX delay, I have no idea what the actual 
unit of measurement is. See the comment I left in 
rtl8365mb_ext_config_rgmii().

So I guess I could "reinterpret" rx-internal-delay-ps to mean these 
magic step values, or otherwise I don't know what might be the best 
practice.

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