diff mbox series

[net-next,v3,1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

Message ID 20240202-for-netnext-mt7530-improvements-2-v3-1-63d5adae99ca@arinc9.com (mailing list archive)
State New
Headers show
Series MT7530 DSA Subdriver Improvements Act II | expand

Commit Message

Arınç ÜNAL via B4 Relay Feb. 2, 2024, 9:19 a.m. UTC 
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>
---
 drivers/net/dsa/mt7530.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Russell King (Oracle) Feb. 2, 2024, 11:40 a.m. UTC | #1 
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.
Arınç ÜNAL Feb. 2, 2024, 5:44 p.m. UTC | #2 
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ç
Russell King (Oracle) Feb. 2, 2024, 6:05 p.m. UTC | #3 
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.
Andrew Lunn Feb. 2, 2024, 8:25 p.m. UTC | #4 
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
Vladimir Oltean Feb. 5, 2024, 8:29 p.m. UTC | #5 
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 mbox series

Patch

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