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@xxxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Michael Rasmussen <mir@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
>>> ---
>>
>> Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>
>>
>> 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@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?
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;
>>> + }
>>> +}
>
>
