Message-ID: <3db3be5f-7b74-02a2-82b8-705fe498cc1b@bang-olufsen.dk>
Date:   Tue, 12 Oct 2021 14:30:50 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     Alvin Šipraga <alvin@...s.dk>,
        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 4:03 PM, Vladimir Oltean wrote:
> On Tue, Oct 12, 2021 at 01:50:44PM +0000, Alvin Šipraga wrote:
>>>>> +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?
> 
> Yes, I think so.
> 
>> 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.
> 
> I think what could work is you could accept only the 0 or 2000 ps values.
> For the TX delay you say it is clear that you should program "1" to hardware.
> For the RX delay I guess that the value of "4" is simply your best guess
> of what would correspond to 2 ns. So you could just transform the 2000 ps
> value into a "4" for the RX delay and make no other guesses otherwise.

OK, this is also the most obvious way to deal with it. Will address in v2.

> 
>>>>> +    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?
> 
> I was going to say with rtl8365mb_phylink_mac_config() where you do have
> a specific check for phy_interface_mode_is_rgmii(), but now I notice
> that it is further guarded by a "dsa_is_user_port()" check. So, with nothing.
> 
>>> 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?
> 
> Understood that the code is functionally correct, but you're not forcing
> the link because it's a CPU port, you're forcing the link because it's
> an RGMII port. Semantically, a CPU port means something entirely
> different: pass DSA-tagged frames to a host. Nothing at the physical link level.
> On your switch it is basically a coincidence that all user ports have
> internal PHYs, and the CPU port is RGMII. All I'm saying is to remove
> the assumptions based on port roles from your MAC configuration logic.

I see your point. However I would still like to keep the 
dsa_is_{user,cpu}_port() checks in rtl8365mb_phy_mode_supported(), just 
so that somebody doesn't unwittingly misconfigure the chip via device 
tree. But I'll remove the port type checks in 
.phylink_mac_{config,link_down,link_up}.

> 
> For somebody searching the git tree for .phylink_mac_link_up implementations
> and sleepwalking into your code, it will be deeply confusing to see such
> logic, even if there is a drawing at the top of the file.
> 
> Why do you need these checks anyway and cannot simply distinguish based
> on PHY_INTERFACE_MODE_INTERNAL vs PHY_INTERFACE_MODE_RGMII*?


Even this might not be necessary, but I'll check it out for v2.

Powered by blists - more mailing lists