Message-ID: <20211012140356.kqd5g6h2lhvugpxz@skbuf>
Date:   Tue, 12 Oct 2021 17:03:56 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
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 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.

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

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

Powered by blists - more mailing lists