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@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?
>
> 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.
