On Fri, 2021-04-23 at 01:28 +0200, Andrew Lunn wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe

quoted

quoted

+
+           lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
+
+           /* clear MII selection & set it based on interface later */
+           data8 &= ~PORT_MII_SEL_M;
+
+           /* configure MAC based on p->interface */
+           switch (p->interface) {
+           case PHY_INTERFACE_MODE_MII:
+                   lan937x_set_gbit(dev, false, &data8);
+                   data8 |= PORT_MII_SEL;
+                   break;
+           case PHY_INTERFACE_MODE_RMII:
+                   lan937x_set_gbit(dev, false, &data8);
+                   data8 |= PORT_RMII_SEL;
+                   break;
+           default:
+                   lan937x_set_gbit(dev, true, &data8);
+                   data8 |= PORT_RGMII_SEL;
+
+                   data8 &= ~PORT_RGMII_ID_IG_ENABLE;
+                   data8 &= ~PORT_RGMII_ID_EG_ENABLE;
+
+                   if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+                       p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+                           data8 |= PORT_RGMII_ID_IG_ENABLE;
+
+                   if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+                       p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+                           data8 |= PORT_RGMII_ID_EG_ENABLE;

This is interesting. If you have an RGMII port connected to an external
PHY, how do you ensure that either the lan937x driver, or the PHY driver,
but not both, enable RGMII delays?

What generally happens is the MAC adds no delays, and the PHY acts
upon the interface mode, inserting delays as requested.

There are a very small number of exceptions to this, for boards which
have a PHY which cannot do delays, and the MAC can. If i remember
correctly, this pretty much limited to one MAC vendor. In that case,
the MAC adds delays, if the interface mode requests it, and it always
passes PHY_INTERFACE_MODE_RGMII to the PHY so it does not add delays.

So what needs to be looked at here is what is passed to the phy
connect call? passing p->interface is definitely wrong if the MAC is
acting on it.

If even if the connect is correct, i would still prefer the MAC not do
the delays, let the PHY do it, like nearly every other setup.

        Andrew

It comes here only if the port is not internal phy which means for MII
interface. As Andrew said, let the phy driver handles the delay if it has the
associated phy vendor driver, otherwise can still be added by MAC if required
(like for cpu port)?

What do you think on the following code?

	struct dsa_port *dp = dsa_to_port(dev->ds, port);
	struct phy_device *phy_dev = dp->slave->phydev;
	.
	.
	.

    	if (!phydev || phy_driver_is_genphy(phydev)) {
		/*Add RGMII internal delay*/
    	}