[PATCH net-next 0/4] net: phy: Misc improvements for Generic 10G PHY

[PATCH net-next 0/4] net: phy: Misc improvements for Generic 10G PHY

[Jose Abreu]

Set of improvements for Generic 10G PHY. All of them tested using stmmac with
XGMAC2 IP working at 10G Link with a C45 PHY.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>

Jose Abreu (4):
  net: phy: Use C45 Helpers when forcing PHY
  net: phy-c45: Populate autoneg_done callback
  net: phy-c45: Implement reset/suspend/resume callbacks
  net: phy-c45: Populate missing features

 drivers/net/phy/phy-c45.c | 59 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/net/phy/phy.c     |  2 +-
 include/linux/phy.h       |  8 +++++++
 3 files changed, 66 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH net-next 1/4] net: phy: Use C45 Helpers when forcing PHY

[Jose Abreu]

If PHY is in force state and we have a C45 phy we need to use the
standard C45 helpers and not the C22 ones.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy.c | 2 +-
 include/linux/phy.h   | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1d73ac3309ce..0ff4946e208e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work)
 		}
 		break;
 	case PHY_FORCING:
-		err = genphy_update_link(phydev);
+		err = phy_update_link(phydev);
 		if (err)
 			break;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ea87f774a76..02c2ee8bc05b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)
 		return genphy_read_status(phydev);
 }
 
+static inline int phy_update_link(struct phy_device *phydev)
+{
+	if (phydev->is_c45)
+		return gen10g_read_status(phydev);
+	else
+		return genphy_update_link(phydev);
+}
+
 void phy_driver_unregister(struct phy_driver *drv);
 void phy_drivers_unregister(struct phy_driver *drv, int n);
 int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
-- 
2.7.4

Re: [PATCH net-next 1/4] net: phy: Use C45 Helpers when forcing PHY

[Florian Fainelli]

On 10/22/18 3:32 AM, Jose Abreu wrote:
> If PHY is in force state and we have a C45 phy we need to use the
> standard C45 helpers and not the C22 ones.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Joao Pinto <joao.pinto@synopsys.com>
> ---
>  drivers/net/phy/phy.c | 2 +-
>  include/linux/phy.h   | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1d73ac3309ce..0ff4946e208e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work)
>  		}
>  		break;
>  	case PHY_FORCING:
> -		err = genphy_update_link(phydev);
> +		err = phy_update_link(phydev);
>  		if (err)
>  			break;
>  
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3ea87f774a76..02c2ee8bc05b 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)
>  		return genphy_read_status(phydev);
>  }
>  
> +static inline int phy_update_link(struct phy_device *phydev)
> +{
> +	if (phydev->is_c45)
> +		return gen10g_read_status(phydev);

Should not this be genphy_c45_read_link() for symmetry with
genphy_update_link() which only updates phydev->link?
-- 
Florian

Re: [PATCH net-next 1/4] net: phy: Use C45 Helpers when forcing PHY

[Jose Abreu]

On 22-10-2018 18:11, Florian Fainelli wrote:
> On 10/22/18 3:32 AM, Jose Abreu wrote:
>> If PHY is in force state and we have a C45 phy we need to use the
>> standard C45 helpers and not the C22 ones.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Joao Pinto <joao.pinto@synopsys.com>
>> ---
>>  drivers/net/phy/phy.c | 2 +-
>>  include/linux/phy.h   | 8 ++++++++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 1d73ac3309ce..0ff4946e208e 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -995,7 +995,7 @@ void phy_state_machine(struct work_struct *work)
>>  		}
>>  		break;
>>  	case PHY_FORCING:
>> -		err = genphy_update_link(phydev);
>> +		err = phy_update_link(phydev);
>>  		if (err)
>>  			break;
>>  
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 3ea87f774a76..02c2ee8bc05b 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -1044,6 +1044,14 @@ static inline int phy_read_status(struct phy_device *phydev)
>>  		return genphy_read_status(phydev);
>>  }
>>  
>> +static inline int phy_update_link(struct phy_device *phydev)
>> +{
>> +	if (phydev->is_c45)
>> +		return gen10g_read_status(phydev);
> Should not this be genphy_c45_read_link() for symmetry with
> genphy_update_link() which only updates phydev->link?

Hmmm, genphy_c45_read_link() does not update phydev->link ... I
can create a new gen10g_update_link() that wraps around
genphy_c45_read_link() and updates link ...

Thanks and Best Regards,
Jose Miguel Abreu

[PATCH net-next 2/4] net: phy-c45: Populate autoneg_done callback

[Jose Abreu]

We already have this callback implemented. Use it in driver structure.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy-c45.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index e1225545362d..c0135217b81f 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -330,6 +330,7 @@ struct phy_driver genphy_10g_driver = {
 	.soft_reset	= gen10g_no_soft_reset,
 	.config_init    = gen10g_config_init,
 	.features       = 0,
+	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = gen10g_config_aneg,
 	.read_status    = gen10g_read_status,
 	.suspend        = gen10g_suspend,
-- 
2.7.4

Re: [PATCH net-next 2/4] net: phy-c45: Populate autoneg_done callback

[Andrew Lunn]

On Mon, Oct 22, 2018 at 11:32:47AM +0100, Jose Abreu wrote:
> We already have this callback implemented. Use it in driver structure.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Joao Pinto <joao.pinto@synopsys.com>
> ---
>  drivers/net/phy/phy-c45.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index e1225545362d..c0135217b81f 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -330,6 +330,7 @@ struct phy_driver genphy_10g_driver = {
>  	.soft_reset	= gen10g_no_soft_reset,
>  	.config_init    = gen10g_config_init,
>  	.features       = 0,
> +	.aneg_done	= genphy_c45_aneg_done,
>  	.config_aneg    = gen10g_config_aneg,
>  	.read_status    = gen10g_read_status,
>  	.suspend        = gen10g_suspend,

We should probably do something about the naming. I need to look at
the history to understand why we have gen10g_ and genphy_c45_.

    Andrew

[PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Jose Abreu]

Implement the missing callbacks for Generic 10G PHY. Tested using XGMAC
with a C45 PHY working at 10G Link.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy-c45.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index c0135217b81f..7e62bd7795a3 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1,6 +1,7 @@
 /*
  * Clause 45 PHY support
  */
+#include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/export.h>
 #include <linux/mdio.h>
@@ -294,6 +295,35 @@ int gen10g_read_status(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(gen10g_read_status);
 
+static int gen10g_poll_reset(struct phy_device *phydev)
+{
+	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
+	unsigned int retries = 12;
+	int ret;
+
+	do {
+		msleep(50);
+		ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+		if (ret < 0)
+			return ret;
+	} while (ret & MDIO_CTRL1_RESET && --retries);
+	if (ret & MDIO_CTRL1_RESET)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int gen10g_soft_reset(struct phy_device *phydev)
+{
+	int val;
+
+	val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, MDIO_CTRL1_RESET);
+	if (val < 0)
+		return val;
+
+	return gen10g_poll_reset(phydev);
+}
+
 int gen10g_no_soft_reset(struct phy_device *phydev)
 {
 	/* Do nothing for now */
@@ -313,12 +343,36 @@ EXPORT_SYMBOL_GPL(gen10g_config_init);
 
 int gen10g_suspend(struct phy_device *phydev)
 {
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+	if (val < 0)
+		return val;
+
+	val |= MDIO_CTRL1_LPOWER;
+
+	val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val);
+	if (val < 0)
+		return val;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(gen10g_suspend);
 
 int gen10g_resume(struct phy_device *phydev)
 {
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+	if (val < 0)
+		return val;
+
+	val &= ~MDIO_CTRL1_LPOWER;
+
+	val = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, val);
+	if (val < 0)
+		return val;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(gen10g_resume);
@@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
 	.phy_id         = 0xffffffff,
 	.phy_id_mask    = 0xffffffff,
 	.name           = "Generic 10G PHY",
-	.soft_reset	= gen10g_no_soft_reset,
+	.soft_reset	= gen10g_soft_reset,
 	.config_init    = gen10g_config_init,
 	.features       = 0,
 	.aneg_done	= genphy_c45_aneg_done,
-- 
2.7.4

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Andrew Lunn]

>  EXPORT_SYMBOL_GPL(gen10g_resume);
> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>  	.phy_id         = 0xffffffff,
>  	.phy_id_mask    = 0xffffffff,
>  	.name           = "Generic 10G PHY",
> -	.soft_reset	= gen10g_no_soft_reset,
> +	.soft_reset	= gen10g_soft_reset,
>  	.config_init    = gen10g_config_init,
>  	.features       = 0,
>  	.aneg_done	= genphy_c45_aneg_done,

Hi Jose

You need to be careful here. There is a reason this is called
gen10g_no_soft_reset, rather than having an empty
gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
gen10g_soft_reset is fine, but don't change this here, without first
understanding the history, and talking to Russell King.

	      Andrew

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Jose Abreu]

Hello,

On 22-10-2018 13:28, Andrew Lunn wrote:
>>  EXPORT_SYMBOL_GPL(gen10g_resume);
>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>>  	.phy_id         = 0xffffffff,
>>  	.phy_id_mask    = 0xffffffff,
>>  	.name           = "Generic 10G PHY",
>> -	.soft_reset	= gen10g_no_soft_reset,
>> +	.soft_reset	= gen10g_soft_reset,
>>  	.config_init    = gen10g_config_init,
>>  	.features       = 0,
>>  	.aneg_done	= genphy_c45_aneg_done,
> Hi Jose
>
> You need to be careful here. There is a reason this is called
> gen10g_no_soft_reset, rather than having an empty
> gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
> gen10g_soft_reset is fine, but don't change this here, without first
> understanding the history, and talking to Russell King.

Hmm, the reset function only interacts with standard PCS
registers, which should always be available ...

>From my tests I need to do at least 1 reset during power-up so in
ultimate case I can add a feature quirk or similar.

Russell, can you please comment ?

Thanks and Best Regards,
Jose Miguel Abreu

>
> 	      Andrew

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Russell King - ARM Linux]

On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> Hello,
> 
> On 22-10-2018 13:28, Andrew Lunn wrote:
> >>  EXPORT_SYMBOL_GPL(gen10g_resume);
> >> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> >>  	.phy_id         = 0xffffffff,
> >>  	.phy_id_mask    = 0xffffffff,
> >>  	.name           = "Generic 10G PHY",
> >> -	.soft_reset	= gen10g_no_soft_reset,
> >> +	.soft_reset	= gen10g_soft_reset,
> >>  	.config_init    = gen10g_config_init,
> >>  	.features       = 0,
> >>  	.aneg_done	= genphy_c45_aneg_done,
> > Hi Jose
> >
> > You need to be careful here. There is a reason this is called
> > gen10g_no_soft_reset, rather than having an empty
> > gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
> > gen10g_soft_reset is fine, but don't change this here, without first
> > understanding the history, and talking to Russell King.
> 
> Hmm, the reset function only interacts with standard PCS
> registers, which should always be available ...
> 
> >From my tests I need to do at least 1 reset during power-up so in
> ultimate case I can add a feature quirk or similar.
> 
> Russell, can you please comment ?

Setting the reset bit on 88x3310 causes the entire device to become
completely inaccessible until hardware reset.  Therefore, this bit
must _never_ be set for these devices.  That said, we have a separate
driver for these PHYs, but that will only be used for them if it's
present in the kernel.  If we accidentally fall back to the generic
driver, then we'll screw the 88x3310 until a full hardware reset.

We also have a bunch of net devices that make use of this crippled
"generic" 10G support - we don't know whether resetting the PHY
for those systems will cause a regression - maybe board firmware
already configured the PHY?  I can't say either way on that, except
that we've had crippled 10G support in PHYLIB for a number of years
now _with_ users, and adding reset support drastically changes the
subsystem's behaviour for these users.

I would recommend not touching the generic 10G driver, but instead
implement your own driver for your PHY to avoid causing regressions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Florian Fainelli]

On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
>> Hello,
>>
>> On 22-10-2018 13:28, Andrew Lunn wrote:
>>>>  EXPORT_SYMBOL_GPL(gen10g_resume);
>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>>>>  	.phy_id         = 0xffffffff,
>>>>  	.phy_id_mask    = 0xffffffff,
>>>>  	.name           = "Generic 10G PHY",
>>>> -	.soft_reset	= gen10g_no_soft_reset,
>>>> +	.soft_reset	= gen10g_soft_reset,
>>>>  	.config_init    = gen10g_config_init,
>>>>  	.features       = 0,
>>>>  	.aneg_done	= genphy_c45_aneg_done,
>>> Hi Jose
>>>
>>> You need to be careful here. There is a reason this is called
>>> gen10g_no_soft_reset, rather than having an empty
>>> gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
>>> gen10g_soft_reset is fine, but don't change this here, without first
>>> understanding the history, and talking to Russell King.
>>
>> Hmm, the reset function only interacts with standard PCS
>> registers, which should always be available ...
>>
>> >From my tests I need to do at least 1 reset during power-up so in
>> ultimate case I can add a feature quirk or similar.
>>
>> Russell, can you please comment ?
> 
> Setting the reset bit on 88x3310 causes the entire device to become
> completely inaccessible until hardware reset.  Therefore, this bit
> must _never_ be set for these devices.  That said, we have a separate
> driver for these PHYs, but that will only be used for them if it's
> present in the kernel.  If we accidentally fall back to the generic
> driver, then we'll screw the 88x3310 until a full hardware reset.
> 
> We also have a bunch of net devices that make use of this crippled
> "generic" 10G support - we don't know whether resetting the PHY
> for those systems will cause a regression - maybe board firmware
> already configured the PHY?  I can't say either way on that, except
> that we've had crippled 10G support in PHYLIB for a number of years
> now _with_ users, and adding reset support drastically changes the
> subsystem's behaviour for these users.
> 
> I would recommend not touching the generic 10G driver, but instead
> implement your own driver for your PHY to avoid causing regressions.
> 

Agreed.
-- 
Florian

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Jose Abreu]

On 22-10-2018 18:13, Florian Fainelli wrote:
> On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
>> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
>>> Hello,
>>>
>>> On 22-10-2018 13:28, Andrew Lunn wrote:
>>>>>  EXPORT_SYMBOL_GPL(gen10g_resume);
>>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>>>>>  	.phy_id         = 0xffffffff,
>>>>>  	.phy_id_mask    = 0xffffffff,
>>>>>  	.name           = "Generic 10G PHY",
>>>>> -	.soft_reset	= gen10g_no_soft_reset,
>>>>> +	.soft_reset	= gen10g_soft_reset,
>>>>>  	.config_init    = gen10g_config_init,
>>>>>  	.features       = 0,
>>>>>  	.aneg_done	= genphy_c45_aneg_done,
>>>> Hi Jose
>>>>
>>>> You need to be careful here. There is a reason this is called
>>>> gen10g_no_soft_reset, rather than having an empty
>>>> gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
>>>> gen10g_soft_reset is fine, but don't change this here, without first
>>>> understanding the history, and talking to Russell King.
>>> Hmm, the reset function only interacts with standard PCS
>>> registers, which should always be available ...
>>>
>>> >From my tests I need to do at least 1 reset during power-up so in
>>> ultimate case I can add a feature quirk or similar.
>>>
>>> Russell, can you please comment ?
>> Setting the reset bit on 88x3310 causes the entire device to become
>> completely inaccessible until hardware reset.  Therefore, this bit
>> must _never_ be set for these devices.  That said, we have a separate
>> driver for these PHYs, but that will only be used for them if it's
>> present in the kernel.  If we accidentally fall back to the generic
>> driver, then we'll screw the 88x3310 until a full hardware reset.
>>
>> We also have a bunch of net devices that make use of this crippled
>> "generic" 10G support - we don't know whether resetting the PHY
>> for those systems will cause a regression - maybe board firmware
>> already configured the PHY?  I can't say either way on that, except
>> that we've had crippled 10G support in PHYLIB for a number of years
>> now _with_ users, and adding reset support drastically changes the
>> subsystem's behaviour for these users.
>>
>> I would recommend not touching the generic 10G driver, but instead
>> implement your own driver for your PHY to avoid causing regressions.
>>
> Agreed.

What about .suspend / .resume ?

Thanks and Best Regards,
Jose Miguel Abreu

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Russell King - ARM Linux]

On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote:
> On 22-10-2018 18:13, Florian Fainelli wrote:
> > On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
> >> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
> >>> Hello,
> >>>
> >>> On 22-10-2018 13:28, Andrew Lunn wrote:
> >>>>>  EXPORT_SYMBOL_GPL(gen10g_resume);
> >>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
> >>>>>  	.phy_id         = 0xffffffff,
> >>>>>  	.phy_id_mask    = 0xffffffff,
> >>>>>  	.name           = "Generic 10G PHY",
> >>>>> -	.soft_reset	= gen10g_no_soft_reset,
> >>>>> +	.soft_reset	= gen10g_soft_reset,
> >>>>>  	.config_init    = gen10g_config_init,
> >>>>>  	.features       = 0,
> >>>>>  	.aneg_done	= genphy_c45_aneg_done,
> >>>> Hi Jose
> >>>>
> >>>> You need to be careful here. There is a reason this is called
> >>>> gen10g_no_soft_reset, rather than having an empty
> >>>> gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
> >>>> gen10g_soft_reset is fine, but don't change this here, without first
> >>>> understanding the history, and talking to Russell King.
> >>> Hmm, the reset function only interacts with standard PCS
> >>> registers, which should always be available ...
> >>>
> >>> >From my tests I need to do at least 1 reset during power-up so in
> >>> ultimate case I can add a feature quirk or similar.
> >>>
> >>> Russell, can you please comment ?
> >> Setting the reset bit on 88x3310 causes the entire device to become
> >> completely inaccessible until hardware reset.  Therefore, this bit
> >> must _never_ be set for these devices.  That said, we have a separate
> >> driver for these PHYs, but that will only be used for them if it's
> >> present in the kernel.  If we accidentally fall back to the generic
> >> driver, then we'll screw the 88x3310 until a full hardware reset.
> >>
> >> We also have a bunch of net devices that make use of this crippled
> >> "generic" 10G support - we don't know whether resetting the PHY
> >> for those systems will cause a regression - maybe board firmware
> >> already configured the PHY?  I can't say either way on that, except
> >> that we've had crippled 10G support in PHYLIB for a number of years
> >> now _with_ users, and adding reset support drastically changes the
> >> subsystem's behaviour for these users.
> >>
> >> I would recommend not touching the generic 10G driver, but instead
> >> implement your own driver for your PHY to avoid causing regressions.
> >>
> > Agreed.
> 
> What about .suspend / .resume ?

I have no idea what you're proposing there - your patches weren't copied
to me.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Jose Abreu]

On 23-10-2018 11:20, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2018 at 11:17:50AM +0100, Jose Abreu wrote:
>> On 22-10-2018 18:13, Florian Fainelli wrote:
>>> On 10/22/18 8:48 AM, Russell King - ARM Linux wrote:
>>>> On Mon, Oct 22, 2018 at 01:47:48PM +0100, Jose Abreu wrote:
>>>>> Hello,
>>>>>
>>>>> On 22-10-2018 13:28, Andrew Lunn wrote:
>>>>>>>  EXPORT_SYMBOL_GPL(gen10g_resume);
>>>>>>> @@ -327,7 +381,7 @@ struct phy_driver genphy_10g_driver = {
>>>>>>>  	.phy_id         = 0xffffffff,
>>>>>>>  	.phy_id_mask    = 0xffffffff,
>>>>>>>  	.name           = "Generic 10G PHY",
>>>>>>> -	.soft_reset	= gen10g_no_soft_reset,
>>>>>>> +	.soft_reset	= gen10g_soft_reset,
>>>>>>>  	.config_init    = gen10g_config_init,
>>>>>>>  	.features       = 0,
>>>>>>>  	.aneg_done	= genphy_c45_aneg_done,
>>>>>> Hi Jose
>>>>>>
>>>>>> You need to be careful here. There is a reason this is called
>>>>>> gen10g_no_soft_reset, rather than having an empty
>>>>>> gen10g_soft_reset. Some PHYs break when you do a reset.  So adding a
>>>>>> gen10g_soft_reset is fine, but don't change this here, without first
>>>>>> understanding the history, and talking to Russell King.
>>>>> Hmm, the reset function only interacts with standard PCS
>>>>> registers, which should always be available ...
>>>>>
>>>>> >From my tests I need to do at least 1 reset during power-up so in
>>>>> ultimate case I can add a feature quirk or similar.
>>>>>
>>>>> Russell, can you please comment ?
>>>> Setting the reset bit on 88x3310 causes the entire device to become
>>>> completely inaccessible until hardware reset.  Therefore, this bit
>>>> must _never_ be set for these devices.  That said, we have a separate
>>>> driver for these PHYs, but that will only be used for them if it's
>>>> present in the kernel.  If we accidentally fall back to the generic
>>>> driver, then we'll screw the 88x3310 until a full hardware reset.
>>>>
>>>> We also have a bunch of net devices that make use of this crippled
>>>> "generic" 10G support - we don't know whether resetting the PHY
>>>> for those systems will cause a regression - maybe board firmware
>>>> already configured the PHY?  I can't say either way on that, except
>>>> that we've had crippled 10G support in PHYLIB for a number of years
>>>> now _with_ users, and adding reset support drastically changes the
>>>> subsystem's behaviour for these users.
>>>>
>>>> I would recommend not touching the generic 10G driver, but instead
>>>> implement your own driver for your PHY to avoid causing regressions.
>>>>
>>> Agreed.
>> What about .suspend / .resume ?
> I have no idea what you're proposing there - your patches weren't copied
> to me.
>

They just set / unset  MDIO_CTRL1_LPOWER bit in PCS. I find that
without this remote end doesn't detect link is down ...

If it's okay for Generic 10G driver I can submit only this and
manually reset PHY in stmmac driver so that I don't need to
implement custom PHY driver ...

BTW, I just found out currently Generic 10G Driver is broken
without patch 4/4 of this series [1]

[1] https://patchwork.ozlabs.org/patch/987570/

Thanks and Best Regards,
Jose Miguel Abreu

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Russell King - ARM Linux]

On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote:
> On 23-10-2018 11:20, Russell King - ARM Linux wrote:
> > I have no idea what you're proposing there - your patches weren't copied
> > to me.
> 
> They just set / unset  MDIO_CTRL1_LPOWER bit in PCS. I find that
> without this remote end doesn't detect link is down ...
> 
> If it's okay for Generic 10G driver I can submit only this and
> manually reset PHY in stmmac driver so that I don't need to
> implement custom PHY driver ...



> BTW, I just found out currently Generic 10G Driver is broken
> without patch 4/4 of this series [1]
> 
> [1] https://patchwork.ozlabs.org/patch/987570/

How is it broken - what are the symptoms?

The generic 10G driver is bound not via the normal bus matching and
phy_bus_match(), but via a manual bind in phy_attach_direct().  This
calls the probe function, which is phy_probe(), which initialises
the supported/advertising to the driver's features (which as you note
are zero.)

However, phy_attach_direct() goes on to call phy_init_hw(), which
calls the config_init() method.  The config_init() method initialises
the supported/advertising masks to 10GbaseT.  This is (partly) what
I refer to when I say that the generic 10G support is crippled - it
only supports this single speed and media.

So the supported/advertising masks should be forced to only 10GbaseT
at the completion of phy_attach_direct().

The "generic 10G" support doesn't do autonegotiation, configuration
or link mode forcing.  It only assumes 10GbaseT is supported, and
only checks for the "link up" bits.

It isn't like the non-10G generic PHY support due to history - it
was added in 2014 by Andy Fleming (see 124059fd53af).

BTW, your patch 1 is wrong as well (introducing phy_update_link()).
You don't take account that a 10G phy may have alternative ways of
reading the link (like 88x3310 does, because it has multiple
instances of AN/PCS/PHYXS at 1k offsets.)  All the gen10g_*
functions are legacy functions for the crippled "generic" 10G
support.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Jose Abreu]

On 23-10-2018 11:58, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote:
>> On 23-10-2018 11:20, Russell King - ARM Linux wrote:
>>> I have no idea what you're proposing there - your patches weren't copied
>>> to me.
>> They just set / unset  MDIO_CTRL1_LPOWER bit in PCS. I find that
>> without this remote end doesn't detect link is down ...
>>
>> If it's okay for Generic 10G driver I can submit only this and
>> manually reset PHY in stmmac driver so that I don't need to
>> implement custom PHY driver ...
>
>
>> BTW, I just found out currently Generic 10G Driver is broken
>> without patch 4/4 of this series [1]
>>
>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_987570_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zYEDSMZPTCHiPc_3B8buyu0kXnlIEYawnHWAxPrsoSU&s=MlF6I2cBSYkGxgEwNV-hXpXJIvXv_gRYXP-CazjkUSw&e=
> How is it broken - what are the symptoms?
>
> The generic 10G driver is bound not via the normal bus matching and
> phy_bus_match(), but via a manual bind in phy_attach_direct().  This
> calls the probe function, which is phy_probe(), which initialises
> the supported/advertising to the driver's features (which as you note
> are zero.)

Since 719655a14971 ("net: phy: Replace phy driver features u32
with link_mode bitmap"), phy_probe() calls
ethtool_convert_link_mode_to_legacy_u32() with phydrv->features
as argument. Since features are NULL, we will get NULL pointer
dereference.

I guess Generic 10G driver was forgotten in the conversion.

Thanks and Best Regards,
Jose Miguel Abreu

>
> However, phy_attach_direct() goes on to call phy_init_hw(), which
> calls the config_init() method.  The config_init() method initialises
> the supported/advertising masks to 10GbaseT.  This is (partly) what
> I refer to when I say that the generic 10G support is crippled - it
> only supports this single speed and media.
>
> So the supported/advertising masks should be forced to only 10GbaseT
> at the completion of phy_attach_direct().
>
> The "generic 10G" support doesn't do autonegotiation, configuration
> or link mode forcing.  It only assumes 10GbaseT is supported, and
> only checks for the "link up" bits.
>
> It isn't like the non-10G generic PHY support due to history - it
> was added in 2014 by Andy Fleming (see 124059fd53af).
>
> BTW, your patch 1 is wrong as well (introducing phy_update_link()).
> You don't take account that a 10G phy may have alternative ways of
> reading the link (like 88x3310 does, because it has multiple
> instances of AN/PCS/PHYXS at 1k offsets.)  All the gen10g_*
> functions are legacy functions for the crippled "generic" 10G
> support.
>

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Andrew Lunn]

> Since 719655a14971 ("net: phy: Replace phy driver features u32
> with link_mode bitmap"), phy_probe() calls
> ethtool_convert_link_mode_to_legacy_u32() with phydrv->features
> as argument. Since features are NULL, we will get NULL pointer
> dereference.

Hi Jose

Thanks for pointing that out. I will fix it ASAP.

       Andrew

Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks

[Andrew Lunn]

> If it's okay for Generic 10G driver I can submit only this and
> manually reset PHY in stmmac driver so that I don't need to
> implement custom PHY driver ...

Hi Jose

That is a bad idea. What happens when somebody uses a different PHY
which uses a different reset sequence? Please keep with the
abstraction. Anything touching the PHY needs to be in a PHY driver, or
phylib.

   Andrew

[PATCH net-next 4/4] net: phy-c45: Populate missing features

[Jose Abreu]

Populate the missing features field of Generic 10G PHY Driver. This will
be overwritten in .config_init callback so we can just set basic 10G
funcionalities in the field.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joao Pinto <joao.pinto@synopsys.com>
---
 drivers/net/phy/phy-c45.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 7e62bd7795a3..99c66b452af9 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -383,7 +383,7 @@ struct phy_driver genphy_10g_driver = {
 	.name           = "Generic 10G PHY",
 	.soft_reset	= gen10g_soft_reset,
 	.config_init    = gen10g_config_init,
-	.features       = 0,
+	.features       = PHY_10GBIT_FEATURES,
 	.aneg_done	= genphy_c45_aneg_done,
 	.config_aneg    = gen10g_config_aneg,
 	.read_status    = gen10g_read_status,
-- 
2.7.4