On Wed, Mar 09, 2022 at 06:46:26PM +0000, Russell King (Oracle) wrote:

Hi Ioana,

Hi Russell,

quoted

--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c

@@ -2077,8 +2077,10 @@ static int dpaa2_eth_open(struct net_device *net_dev)
 		goto enable_err;
 	}
 
-	if (dpaa2_eth_is_type_phy(priv))
+	if (dpaa2_eth_is_type_phy(priv)) {
 		phylink_start(priv->mac->phylink);
+		dpaa2_mac_start(priv->mac);

Is this safe? Shouldn't dpaa2_mac_start() come before phylink_start()
in case phylink determines that the link is somehow already up? I'm
a big fan of teardown being in the reverse order of setup so having
the start and stop below in the same order just doesn't look right.

Agree that the teardown being done in the reverse order just looks
better. I can change it, of course.

I didn't really spot any actual problems with how are things now, but it
would be better to just bring up the SerDes lanes and then call
phylink_start().

quoted

+static enum dpmac_eth_if dpmac_eth_if_mode(phy_interface_t if_mode)
+{
+	switch (if_mode) {
+	case PHY_INTERFACE_MODE_RGMII:

Shouldn't this also include the other RGMII modes (which, from the MAC
point of view, are all synonymous?

Good point. Thanks for pointing it out.

quoted

+static int dpaa2_mac_prepare(struct phylink_config *config, unsigned int mode,
+			     phy_interface_t interface)
+{
+	dpaa2_mac_link_down(config, mode, interface);

You should never see a reconfiguration while the link is up. However,
if the link is in in-band mode, then obviously the link could come up
at any moment, and in that case, forcing it down in mac_prepare() is
a good idea - but that forcing needs to be removed in mac_finish()
to allow in-band to work again. Not sure that your firmware allows
that though, and I'm not convinced that calling the above function
achieves any of those guarantees.

Ok, I didn't know that I this was a guarantee from phylink's part.
In this case, I can just remove the prepare step, sure.

quoted

+	/* In case we have access to the SerDes phy/lane, then ask the SerDes
+	 * driver what interfaces are supported based on the current PLL
+	 * configuration.
+	 */
+	for (intf = 0; intf < PHY_INTERFACE_MODE_MAX; intf++) {

You probably want to avoid PHY_INTERFACE_MODE_NA here, even though your
driver may reject it anyway.

Yes, I'll just start from PHY_INTERFACE_MODE_INTERNAL.

quoted

-	if (dpaa2_switch_port_is_type_phy(port_priv))
+	if (dpaa2_switch_port_is_type_phy(port_priv)) {
 		phylink_start(port_priv->mac->phylink);
+		dpaa2_mac_start(port_priv->mac);

Same comments as for dpaa2-mac.

Sure. I'll change the order.

Thanks!

Thanks!
Ioana