[PATCH net-next v3 0/2] net: phy: at803x: Update delays for RGMII modes

[PATCH net-next v3 0/2] net: phy: at803x: Update delays for RGMII modes

[Vinod Koul]

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

[PATCH net-next v3 1/2] net: phy: at803x: don't inline helpers

[Vinod Koul]

Some helpers were inlined, but makes more sense to allow compiler
to do the right optimizations instead, so remove inline for
at803x_disable_rx_delay() and at803x_disable_tx_delay()

Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/net/phy/at803x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 8ff12938ab47..c6e7d800fd7a 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -110,13 +110,13 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
 	return phy_write(phydev, AT803X_DEBUG_DATA, val);
 }
 
-static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+static int at803x_disable_rx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
 				     AT803X_DEBUG_RX_CLK_DLY_EN, 0);
 }
 
-static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+static int at803x_disable_tx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
 				     AT803X_DEBUG_TX_CLK_DLY_EN, 0);
-- 
2.20.1

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

[Vinod Koul]

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(-)

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)
-- 
2.20.1

Re: [PATCH net-next v3 0/2] net: phy: at803x: Update delays for RGMII modes

[Peter Ujfalusi]

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

Re: [PATCH net-next v3 1/2] net: phy: at803x: don't inline helpers

[Marc Gonzalez]

On 19/02/2019 07:17, Vinod Koul wrote:

> Some helpers were inlined, but makes more sense to allow compiler
> to do the right optimizations instead, so remove inline for
> at803x_disable_rx_delay() and at803x_disable_tx_delay()

I would word it slightly differently:

Some helpers were declared with the "inline" function specifier.
It is preferable to let the compiler pick the right optimizations,
so drop the specifier.

[ This is just a random suggestion, feel free to ignore ]


> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/net/phy/at803x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 8ff12938ab47..c6e7d800fd7a 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -110,13 +110,13 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
>  	return phy_write(phydev, AT803X_DEBUG_DATA, val);
>  }
>  
> -static inline int at803x_disable_rx_delay(struct phy_device *phydev)
> +static int at803x_disable_rx_delay(struct phy_device *phydev)
>  {
>  	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
>  				     AT803X_DEBUG_RX_CLK_DLY_EN, 0);
>  }
>  
> -static inline int at803x_disable_tx_delay(struct phy_device *phydev)
> +static int at803x_disable_tx_delay(struct phy_device *phydev)
>  {
>  	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
>  				     AT803X_DEBUG_TX_CLK_DLY_EN, 0);

Reviewed-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

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

[Marc Gonzalez]

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.

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

[Niklas Cassel]

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.

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

[Marc Gonzalez]

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.