On 10/12/21 4:03 PM, Vladimir Oltean wrote:

On Tue, Oct 12, 2021 at 01:50:44PM +0000, Alvin Šipraga wrote:

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

quoted

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

quoted

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

quoted

quoted

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.