[PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

[PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

[Baruch Siach]

The MV_V2_PORT_CTRL_SWRST bit in MV_V2_PORT_CTRL is reserved on 88E2110.
Setting SWRST on 88E2110 breaks packets transfer after interface down/up
cycle.

Fixes: 8f48c2ac85ed ("net: marvell10g: soft-reset the PHY when coming out of low power")
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/phy/marvell10g.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index d3cb88651ad2..601686f64341 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -263,7 +263,8 @@ static int mv3310_power_up(struct phy_device *phydev)
 	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
 				 MV_V2_PORT_CTRL_PWRDOWN);
 
-	if (priv->firmware_ver < 0x00030000)
+	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 ||
+	    priv->firmware_ver < 0x00030000)
 		return ret;
 
 	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-- 
2.26.1

Re: [PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

[Russell King - ARM Linux admin]

On Tue, Apr 21, 2020 at 12:04:46PM +0300, Baruch Siach wrote:
> The MV_V2_PORT_CTRL_SWRST bit in MV_V2_PORT_CTRL is reserved on 88E2110.
> Setting SWRST on 88E2110 breaks packets transfer after interface down/up
> cycle.
> 
> Fixes: 8f48c2ac85ed ("net: marvell10g: soft-reset the PHY when coming out of low power")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Okay, the presence of 88E2110 combined with 88X3310 support is going to
be a constant source of pain in terms of maintanence, since I know
nothing about this PHY, nor do I have any way to test my changes there.

I think we need to think about how to deal with that - do we split the
code, so that 88X3310 can be maintained separately from 88E2110 (even
though most of the code may be the same), or can someone send me a board
that has the 88E2110 on (I can't purchase as I have no funds to do so.)

So, I guess splitting the code is likely to be the only solution.

> ---
>  drivers/net/phy/marvell10g.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index d3cb88651ad2..601686f64341 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -263,7 +263,8 @@ static int mv3310_power_up(struct phy_device *phydev)
>  	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
>  				 MV_V2_PORT_CTRL_PWRDOWN);
>  
> -	if (priv->firmware_ver < 0x00030000)
> +	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 ||
> +	    priv->firmware_ver < 0x00030000)
>  		return ret;
>  
>  	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
> -- 
> 2.26.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

[Baruch Siach]

Hi Russell,

On Tue, Apr 21 2020, Russell King - ARM Linux admin wrote:
> On Tue, Apr 21, 2020 at 12:04:46PM +0300, Baruch Siach wrote:
>> The MV_V2_PORT_CTRL_SWRST bit in MV_V2_PORT_CTRL is reserved on 88E2110.
>> Setting SWRST on 88E2110 breaks packets transfer after interface down/up
>> cycle.
>>
>> Fixes: 8f48c2ac85ed ("net: marvell10g: soft-reset the PHY when coming out of low power")
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>
> Okay, the presence of 88E2110 combined with 88X3310 support is going to
> be a constant source of pain in terms of maintanence, since I know
> nothing about this PHY, nor do I have any way to test my changes there.
>
> I think we need to think about how to deal with that - do we split the
> code, so that 88X3310 can be maintained separately from 88E2110 (even
> though most of the code may be the same), or can someone send me a board
> that has the 88E2110 on (I can't purchase as I have no funds to do so.)

I'll contact you in private about hardware availability.

> So, I guess splitting the code is likely to be the only solution.

This situation is no different than other drivers that support many
variants of the same basic hardware. FEC and mvneta driver come to mind
as examples. Hardware availability limitation is always a challenge.

I don't think this justifies splitting the code. But that's your call.

Thanks for reviewing,
baruch

>> ---
>>  drivers/net/phy/marvell10g.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>> index d3cb88651ad2..601686f64341 100644
>> --- a/drivers/net/phy/marvell10g.c
>> +++ b/drivers/net/phy/marvell10g.c
>> @@ -263,7 +263,8 @@ static int mv3310_power_up(struct phy_device *phydev)
>>  	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
>>  				 MV_V2_PORT_CTRL_PWRDOWN);
>>
>> -	if (priv->firmware_ver < 0x00030000)
>> +	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310 ||
>> +	    priv->firmware_ver < 0x00030000)
>>  		return ret;
>>
>>  	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH net] net: phy: marvell10g: limit soft reset to 88x3310

[David Miller]

From: Baruch Siach <baruch@tkos.co.il>
Date: Tue, 21 Apr 2020 12:04:46 +0300

> The MV_V2_PORT_CTRL_SWRST bit in MV_V2_PORT_CTRL is reserved on 88E2110.
> Setting SWRST on 88E2110 breaks packets transfer after interface down/up
> cycle.
> 
> Fixes: 8f48c2ac85ed ("net: marvell10g: soft-reset the PHY when coming out of low power")
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Applied.