| Message ID | 20190219061800.31025-3-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: phy: at803x: Update delays for RGMII modes | expand |
On 19/02/2019 07:18, 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. PHY or phy? :-) > So disable the delay only for RGMII mode and enable for other modes. > Also treat the default case as disabled delays. > > Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org> > Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com> > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++---------- > 1 file changed, 36 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index c6e7d800fd7a..dc1b13f7fc12 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 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 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 int at803x_disable_rx_delay(struct phy_device *phydev) > { > return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, > @@ -255,23 +267,36 @@ 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) { > - ret = at803x_disable_rx_delay(phydev); > + /* The hardware register default is RX and TX delay enabled, so lets > + * first disable the RX and TX delays in phy and enable them based > + * on the mode selected > + */ "let's" (let us) For the record, AFAIR, the default is not *quite* RX and TX enabled: https://www.spinics.net/lists/netdev/msg444527.html RX: enabled at HW reset TX: disabled at HW reset, but retains value after SW reset > + 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) { > + /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > + * otherwise keep it disabled > + */ > + 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 (ret < 0) > - return ret; > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > + * otherwise keep it disabled > + */ > + ret = at803x_enable_tx_delay(phydev); > } > > - return 0; > + return ret; > } IMO, the asymmetry in error handling for RX and TX is unfortunate. Didn't you like the way it was done in my old patch? :-) https://www.spinics.net/lists/netdev/msg445053.html Regards.
On Tue, Feb 19, 2019 at 12:57:18PM +0100, Marc Gonzalez wrote: > On 19/02/2019 07:18, 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. > > PHY or phy? :-) > > > So disable the delay only for RGMII mode and enable for other modes. > > Also treat the default case as disabled delays. > > > > Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") > > Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org> > > Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com> > > Signed-off-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 36 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > index c6e7d800fd7a..dc1b13f7fc12 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 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 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 int at803x_disable_rx_delay(struct phy_device *phydev) > > { > > return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, > > @@ -255,23 +267,36 @@ 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) { > > - ret = at803x_disable_rx_delay(phydev); > > + /* The hardware register default is RX and TX delay enabled, so lets > > + * first disable the RX and TX delays in phy and enable them based > > + * on the mode selected > > + */ > > "let's" (let us) > > For the record, AFAIR, the default is not *quite* RX and TX enabled: > > https://www.spinics.net/lists/netdev/msg444527.html Hello Marc, > > RX: enabled at HW reset > TX: disabled at HW reset, but retains value after SW reset You are correct of course. However, since the bootloader might have enabled delay on TX, I still think that this patch does the right thing by starting out with disabling delays for both RX and TX. But we should probably make the comment more elaborate: The value after HW reset is RX delay enabled and TX delay disabled. The value after SW reset is RX delay enabled, while TX delay retains the value before reset. In order to not depend on reset values, start off by disabling both delays. Kind regards, Niklas > > > + 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) { > > + /* If RGMII_ID or RGMII_RXID are specified enable RX delay, > > + * otherwise keep it disabled > > + */ > > + 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 (ret < 0) > > - return ret; > > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || > > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { > > + /* If RGMII_ID or RGMII_TXID are specified enable TX delay, > > + * otherwise keep it disabled > > + */ > > + ret = at803x_enable_tx_delay(phydev); > > } > > > > - return 0; > > + return ret; > > } > > IMO, the asymmetry in error handling for RX and TX is unfortunate. > > Didn't you like the way it was done in my old patch? :-) > > https://www.spinics.net/lists/netdev/msg445053.html > > Regards.
On 19/02/2019 13:33, Niklas Cassel wrote: > However, since the bootloader might have enabled delay on > TX, I still think that this patch does the right thing by > starting out with disabling delays for both RX and TX. > > But we should probably make the comment more elaborate: > > The value after HW reset is RX delay enabled and TX delay disabled. > The value after SW reset is RX delay enabled, while TX delay retains > the value before reset. > In order to not depend on reset values, start off by disabling both > delays. Ultimately, the two patches do the same thing, AFAICT ;-) I was just arguing that my way was better because... errr... because it was my way! :-) Regards.
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index c6e7d800fd7a..dc1b13f7fc12 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 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 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 int at803x_disable_rx_delay(struct phy_device *phydev) { return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, @@ -255,23 +267,36 @@ 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) { - ret = at803x_disable_rx_delay(phydev); + /* The hardware register default is RX and TX delay enabled, so lets + * first disable the RX and TX delays in phy and enable them based + * on the mode selected + */ + 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) { + /* If RGMII_ID or RGMII_RXID are specified enable RX delay, + * otherwise keep it disabled + */ + 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 (ret < 0) - return ret; + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID || + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + /* If RGMII_ID or RGMII_TXID are specified enable TX delay, + * otherwise keep it disabled + */ + ret = at803x_enable_tx_delay(phydev); } - return 0; + return ret; } static int at803x_ack_interrupt(struct phy_device *phydev)
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 enable for other modes. Also treat the default case as disabled delays. Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org> Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com> Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 11 deletions(-)