| Message ID | 20190219061800.31025-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | net: phy: at803x: Update delays for RGMII modes | expand |
Hi Vinod, On 19/02/2019 8.17, Vinod Koul wrote: > Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable > phy delay for RGMII mode") caused regression on am335x-evmsk board. > This board expects the Phy delay to be enabled but specified RGMII mode > which refers to delays being disabled. So fix this by disabling delay only > for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes. > > While at it, as pointed by Dave, don't inline the helpers. > > [1]: https://www.spinics.net/lists/netdev/msg550749.html Not sure what was changed since v2, but with https://patchwork.ozlabs.org/project/netdev/list/?series=92672 ethernet is working on am335x-evmsk. thank you: Tested-by: Peter Ujfalusi <peter.ujflausi@ti.com> > Vinod Koul (2): > net: phy: at803x: don't inline helpers > net: phy: at803x: disable delay only for RGMII mode > > drivers/net/phy/at803x.c | 51 ++++++++++++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 13 deletions(-) > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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 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.
Peter[1] reported that patch cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode") caused regression on am335x-evmsk board. This board expects the Phy delay to be enabled but specified RGMII mode which refers to delays being disabled. So fix this by disabling delay only for RGMII mode and enable for RGMII_ID and RGMII_TXID/RXID modes. While at it, as pointed by Dave, don't inline the helpers. [1]: https://www.spinics.net/lists/netdev/msg550749.html Vinod Koul (2): net: phy: at803x: don't inline helpers net: phy: at803x: disable delay only for RGMII mode drivers/net/phy/at803x.c | 51 ++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 13 deletions(-) -- 2.20.1