Messages in this thread | | | Subject | Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY | From | Dan Murphy <> | Date | Mon, 30 Nov 2020 10:57:36 -0600 |
| |
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
| |