Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Dan Murphy]

Andrew

On 11/19/20 7:49 PM, Andrew Lunn wrote:
+static int dp83td510_config_init(struct phy_device *phydev)
+{
+       struct dp83td510_private *dp83td510 = phydev->priv;
+       int ret = 0;
+
+       if (phy_interface_is_rgmii(phydev)) {
+               if (dp83td510->rgmii_delay) {
+                       ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
+                                              DP83TD510_MAC_CFG_1,
+                                              dp83td510->rgmii_delay);
Just to be safe, you should always write rgmii_delay, even if it is
zero. We have had too many bugs with RGMII delays which cause bad
backwards compatibility problems, so i would prefer to do a write
which might be unneeded, that find a bug here in a few years time.

OK.



+                       if (ret)
+                               return ret;
+               }
+       }
+
+       if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
+               ret = phy_modify(phydev, DP83TD510_GEN_CFG,
+                                DP83TD510_FIFO_DEPTH_MASK,
+                                dp83td510->tx_fifo_depth);
So there is no need to set the FIFO depth for the other three RGMII
modes? Or should this also be phy_interface_is_rgmii(phydev)?

According to the data sheet the FIFO depth is for RMII.

"Fifo depth for RMII Tx fifo"

But I will ask the HW team for clarification.



+#if IS_ENABLED(CONFIG_OF_MDIO)
+static int dp83td510_of_init(struct phy_device *phydev)
+{
+       struct dp83td510_private *dp83td510 = phydev->priv;
+       struct device *dev = &phydev->mdio.dev;
+       struct device_node *of_node = dev->of_node;
You need to move this assignment to later in order to keep with
reverse christmas tree.
Well this is only used once so I will just remove the of_node declaration

+#else
+static int dp83869_of_init(struct phy_device *phydev)
+{
+       dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
+       dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
You don't have DT, so there is no fine control, but you still need to
do the basic 2ns delay as indicated by the phydev->interface value. So
i think you still need to set dp83td510->rgmii_delay depending on
which RGMII mode is requested.

The RGMII delay is fixed in the PHY.  The user can either turn it on or 
off. The default is 'off' which is 0.

I can explicitly set the rgmii_delay to 0 in non-OF cases.

Dan