Re: [PATCH] net: phy: at803x: disable delay only for RGMII mode

[Niklas Cassel <niklas.cassel@linaro.org>]

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@ti.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  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