Re: [PATCH net-next v3 2/2] net: phy: at803x: disable delay only for RGMII mode

From: Niklas Cassel <niklas.cassel@linaro.org>
To: Marc Gonzalez <marc.w.gonzalez@free.fr>
Cc: Vinod Koul <vkoul@kernel.org>,
	David S Miller <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>,
	MSM <linux-arm-msm@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Nori Sekhar <nsekhar@ti.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>
Subject: Re: [PATCH net-next v3 2/2] net: phy: at803x: disable delay only for RGMII mode
Date: Tue, 19 Feb 2019 13:33:39 +0100	[thread overview]
Message-ID: <20190219123339.GA13172@centauri.lan> (raw)
In-Reply-To: <1dddc455-309d-5e7d-ca26-479fe800a013@free.fr>

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.