| Message ID | 20240202-for-netnext-mt7530-improvements-2-v3-1-63d5adae99ca@arinc9.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | MT7530 DSA Subdriver Improvements Act II | expand |
On Fri, Feb 02, 2024 at 12:19:07PM +0300, Arınç ÜNAL via B4 Relay wrote: > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > There're two code paths for setting up port 5: > > mt7530_setup() > -> mt7530_setup_port5() > > mt753x_phylink_mac_config() > -> mt753x_mac_config() > -> mt7530_mac_config() > -> mt7530_setup_port5() > > On the first code path, priv->p5_intf_sel is either set to > P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4 when mt7530_setup_port5() is run. > > On the second code path, priv->p5_intf_sel is set to P5_INTF_SEL_GMAC5 when > mt7530_setup_port5() is run. > > Empty the default case which will never run but is needed nonetheless to > handle all the remaining enumeration values. > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks! While reviewing this change, but not related to it, I notice that this function sets the TX delay based on the RGMII interface mode. This isn't correct. I've explained why this is this many times in the past, but essentially it comes down to the model: phy-mode in NIC node Network driver PCB PHY rgmii no delays delays no delays rgmii-id no delays no delays tx/rx delays rgmii-txid no delays no delays tx delays rgmii-rxid no delays no delays rx delays Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC node which define the RGMII delays at the local end and similar properties for the PHY node. So, if we take the view that, when a switch is connected to a NIC in RGMII mode, then the phy-mode specified delays still should not impact the local NIC. Now, for the switch, we specify the phy-mode in the port node as well. Consider the case of a switch port connected to a RGMII PHY. This has to operate in exactly the same way as a normal NIC - that is, the RGMII delays at the port should be ignored as it's the responsibility of a PHY. The final scenario to examine is the case of a RGMII switch port connected to a NIC. The NIC's phy-mode has no way to be communicated to DSA or vice versa, so neither phy-mode can impact the other side of the RGMII link, but should only place the link into RGMII mode. Given everything I've said above, the only way to configure RGMII delays is via the rx-internal-delay-ps and tx-internal-delay-ps properties. So, DSA drivers should _not_ be configuring their ports with RGMII delays based on the RGMII phy interface mode. The above is my purely logically reasoned point of view on this subject. Others may have other (to me completely illogical) interpretations that can only lead to interoperability issues.
On 2.02.2024 14:40, Russell King (Oracle) wrote: > While reviewing this change, but not related to it, I notice that this > function sets the TX delay based on the RGMII interface mode. This isn't > correct. I've explained why this is this many times in the past, but > essentially it comes down to the model: > > > phy-mode in NIC node Network driver PCB PHY > rgmii no delays delays no delays > rgmii-id no delays no delays tx/rx delays > rgmii-txid no delays no delays tx delays > rgmii-rxid no delays no delays rx delays > > Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC > node which define the RGMII delays at the local end and similar > properties for the PHY node. > > > So, if we take the view that, when a switch is connected to a NIC in > RGMII mode, then the phy-mode specified delays still should not impact > the local NIC. > > Now, for the switch, we specify the phy-mode in the port node as well. > Consider the case of a switch port connected to a RGMII PHY. This has > to operate in exactly the same way as a normal NIC - that is, the > RGMII delays at the port should be ignored as it's the responsibility > of a PHY. > > The final scenario to examine is the case of a RGMII switch port > connected to a NIC. The NIC's phy-mode has no way to be communicated > to DSA or vice versa, so neither phy-mode can impact the other side > of the RGMII link, but should only place the link into RGMII mode. > Given everything I've said above, the only way to configure RGMII > delays is via the rx-internal-delay-ps and tx-internal-delay-ps > properties. So, DSA drivers should _not_ be configuring their ports > with RGMII delays based on the RGMII phy interface mode. > > The above is my purely logically reasoned point of view on this > subject. Others may have other (to me completely illogical) > interpretations that can only lead to interoperability issues. I will address this with the next patch series. Thank you for explaining it in detail. Arınç
On Fri, Feb 02, 2024 at 08:44:39PM +0300, Arınç ÜNAL wrote: > On 2.02.2024 14:40, Russell King (Oracle) wrote: > > While reviewing this change, but not related to it, I notice that this > > function sets the TX delay based on the RGMII interface mode. This isn't > > correct. I've explained why this is this many times in the past, but > > essentially it comes down to the model: > > > > > > phy-mode in NIC node Network driver PCB PHY > > rgmii no delays delays no delays > > rgmii-id no delays no delays tx/rx delays > > rgmii-txid no delays no delays tx delays > > rgmii-rxid no delays no delays rx delays > > > > Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC > > node which define the RGMII delays at the local end and similar > > properties for the PHY node. > > > > > > So, if we take the view that, when a switch is connected to a NIC in > > RGMII mode, then the phy-mode specified delays still should not impact > > the local NIC. > > > > Now, for the switch, we specify the phy-mode in the port node as well. > > Consider the case of a switch port connected to a RGMII PHY. This has > > to operate in exactly the same way as a normal NIC - that is, the > > RGMII delays at the port should be ignored as it's the responsibility > > of a PHY. > > > > The final scenario to examine is the case of a RGMII switch port > > connected to a NIC. The NIC's phy-mode has no way to be communicated > > to DSA or vice versa, so neither phy-mode can impact the other side > > of the RGMII link, but should only place the link into RGMII mode. > > Given everything I've said above, the only way to configure RGMII > > delays is via the rx-internal-delay-ps and tx-internal-delay-ps > > properties. So, DSA drivers should _not_ be configuring their ports > > with RGMII delays based on the RGMII phy interface mode. > > > > The above is my purely logically reasoned point of view on this > > subject. Others may have other (to me completely illogical) > > interpretations that can only lead to interoperability issues. > > I will address this with the next patch series. Thank you for explaining it > in detail. This is a good time to point out not to rush with the next patch series, as my email will _likely_ provoke some additional discussion from Andrew and/or Vladimir. So please give it a few days (maybe around the middle of next week) to give them time to consider my email and respond.
On Fri, Feb 02, 2024 at 06:05:36PM +0000, Russell King (Oracle) wrote: > On Fri, Feb 02, 2024 at 08:44:39PM +0300, Arınç ÜNAL wrote: > > On 2.02.2024 14:40, Russell King (Oracle) wrote: > > > While reviewing this change, but not related to it, I notice that this > > > function sets the TX delay based on the RGMII interface mode. This isn't > > > correct. I've explained why this is this many times in the past, but > > > essentially it comes down to the model: > > > > > > > > > phy-mode in NIC node Network driver PCB PHY > > > rgmii no delays delays no delays > > > rgmii-id no delays no delays tx/rx delays > > > rgmii-txid no delays no delays tx delays > > > rgmii-rxid no delays no delays rx delays > > > > > > Then we have rx-internal-delay-ps and tx-internal-delay-ps in the NIC > > > node which define the RGMII delays at the local end and similar > > > properties for the PHY node. > > > > > > > > > So, if we take the view that, when a switch is connected to a NIC in > > > RGMII mode, then the phy-mode specified delays still should not impact > > > the local NIC. > > > > > > Now, for the switch, we specify the phy-mode in the port node as well. > > > Consider the case of a switch port connected to a RGMII PHY. This has > > > to operate in exactly the same way as a normal NIC - that is, the > > > RGMII delays at the port should be ignored as it's the responsibility > > > of a PHY. Not quite. It is unusual, but there are a few MAC drivers which do act on phy-mode, to configure MAC delays. However, if they do this, they then mask the value of phy-mode passed to the PHY in order to avoid double delays. Its not something i recommend, we prefer the PHYs do the delays. And it something i strongly suggest we avoid in DSA. > > > The final scenario to examine is the case of a RGMII switch port > > > connected to a NIC. This should also extend to a port connected to a port of another switch. For Marvell switches, these are so called DSA ports. > > > The NIC's phy-mode has no way to be communicated > > > to DSA or vice versa, so neither phy-mode can impact the other side > > > of the RGMII link, but should only place the link into RGMII mode. > > > Given everything I've said above, the only way to configure RGMII > > > delays is via the rx-internal-delay-ps and tx-internal-delay-ps > > > properties. So, DSA drivers should _not_ be configuring their ports > > > with RGMII delays based on the RGMII phy interface mode. Marvell goes against this. rx-internal-delay-ps and tx-internal-delay-ps are pretty new, when compared to the age of mv88e6xxx. I had the problem of a FEC connected to an mv88e6xxx using RGMII and i needed to somehow configure RGMII delays, or packets did not get through. So mv88e6xxx will apply phy-mode to ports being used in CPU or DSA mode. So in the case of the FEC->switch, rgmii-id is used by the switch. For DSA->DSA, there are DT blobs with rgmii-txid on both ends of the link, which results in the needed delays. > > > The above is my purely logically reasoned point of view on this > > > subject. Others may have other (to me completely illogical) > > > interpretations that can only lead to interoperability issues. Now that rx-internal-delay-ps and tx-internal-delay-ps actually exist, these are the best ways to handle such delays, for new drivers. But we cannot change old drivers without causing regressions. Andrew
On Fri, Feb 02, 2024 at 06:05:36PM +0000, Russell King (Oracle) wrote: > > > Given everything I've said above, the only way to configure RGMII > > > delays is via the rx-internal-delay-ps and tx-internal-delay-ps > > > properties. So, DSA drivers should _not_ be configuring their ports > > > with RGMII delays based on the RGMII phy interface mode. > > > > > > The above is my purely logically reasoned point of view on this > > > subject. Others may have other (to me completely illogical) > > > interpretations that can only lead to interoperability issues. > > > > I will address this with the next patch series. Thank you for explaining it > > in detail. > > This is a good time to point out not to rush with the next patch > series, as my email will _likely_ provoke some additional discussion > from Andrew and/or Vladimir. So please give it a few days (maybe > around the middle of next week) to give them time to consider my > email and respond. I agree with everything you've said. The only problem is that Documentation/devicetree/bindings/net/ethernet-controller.yaml is ambiguous on this topic (to put it mildly). @Arınç, there are ways to handle the "tx-internal-delay-ps" in mt7530_setup_port5() in a way that is backwards compatible. I don't know about RX delays - the function doesn't handle them, and I don't have the time to open datasheets now. You can take inspiration from ksz_parse_rgmii_delay() and sja1105_parse_rgmii_delays() on how to only fall back to the current logic to set the tx_delay if the more specific OF properties are not present.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 68be38ae66e0..330e22abc076 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -943,9 +943,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface) val &= ~MHWTRAP_P5_DIS; break; default: - dev_err(ds->dev, "Unsupported p5_intf_sel %d\n", - priv->p5_intf_sel); - goto unlock_exit; + break; } /* Setup RGMII settings */ @@ -975,7 +973,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface) dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n", val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface)); -unlock_exit: mutex_unlock(&priv->reg_mutex); }