On Tue, Feb 12, 2019 at 07:49:22PM +0530, Vinod Koul wrote: > Per "Documentation/devicetree/bindings/net/ethernet.txt" RGMII mode > should not have delay in PHY whereas RGMII_ID and RGMII_RXID/RGMII_TXID > can have delay in phy. > > So disable the delay only for RGMII mode and disable for other modes > > Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") > Reported-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx> > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx> > --- > drivers/net/phy/at803x.c | 33 ++++++++++++++++++++++++++------- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 8ff12938ab47..7b54b54e3316 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -110,6 +110,18 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg, > return phy_write(phydev, AT803X_DEBUG_DATA, val); > } > > +static inline int at803x_enable_rx_delay(struct phy_device *phydev) > +{ > + return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0, > + AT803X_DEBUG_RX_CLK_DLY_EN); > +} > + > +static inline int at803x_enable_tx_delay(struct phy_device *phydev) > +{ > + return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0, > + AT803X_DEBUG_TX_CLK_DLY_EN); > +} > + > static inline int at803x_disable_rx_delay(struct phy_device *phydev) > { > return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, > @@ -255,18 +267,25 @@ static int at803x_config_init(struct phy_device *phydev) > if (ret < 0) > return ret; > > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID || > - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > - phydev->interface == PHY_INTERFACE_MODE_RGMII) { > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { > ret = at803x_disable_rx_delay(phydev); > if (ret < 0) > return ret; > + ret = at803x_disable_tx_delay(phydev); > + if (ret < 0) > + return ret; > + }; > + > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) { > + ret = at803x_enable_rx_delay(phydev); > + if (ret < 0) > + return ret; > } > > - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID || > - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > - phydev->interface == PHY_INTERFACE_MODE_RGMII) { > - ret = at803x_disable_tx_delay(phydev); > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + ret = at803x_enable_tx_delay(phydev); > if (ret < 0) > return ret; > } So we have these modes: PHY_INTERFACE_MODE_RGMII: TX and RX delays disabled PHY_INTERFACE_MODE_RGMII_ID: TX and RX delays enabled PHY_INTERFACE_MODE_RGMII_RXID: RX delay enabled, TX delay disabled PHY_INTERFACE_MODE_RGMII_TXID: TX delay enabled, RX delay disabled What I don't like with this patch, is that if we specify phy-mode PHY_INTERFACE_MODE_RGMII_TXID, this patch will enable TX delay, but RX delay will not be explicitly set. Thus it will use the default value of the PHY, and for this PHY both TX and RX delays are enabled by default. This means that someone specifying PHY_INTERFACE_MODE_RGMII_TXID will actually be getting PHY_INTERFACE_MODE_RGMII_ID. Marc's patch: https://www.spinics.net/lists/netdev/msg445053.html does explicitly either enable or disable each delay (so it does not depend on default values). However, Marc's patch was never merged, because someone reported a regression on am335x-evm. On Vinod's V1 submission Andrew replied that if this change breaks some existing DT (because that DT specifies a phy-mode that does not match with reality), then we should fix so that DT specifies the correct phy-mode: https://www.spinics.net/lists/netdev/msg542638.html IMHO, this makes most sense, since if a DT specifies an incorrect phy-mode, then that is what the user should get. Kind regards, Niklas