[PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[esben.haabendal]

From: Esben Haabendal <eha@deif.com>

Add a function for use in PHY driver probe functions, reading current
autoneg, speed and duplex configuration from BMCR register.

Useful for PHY that supports hardware strapped configuration, enabling
Linux to respect that configuration (i.e. strapped non-autoneg
configuration).

Signed-off-by: Esben Haabendal <eha@deif.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74664a6c0cdc..cc52ff2a2344 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_config_init);
 
+/**
+ * genphy_read_config - read configuration from PHY
+ * @phydev: target phy_device struct
+ *
+ * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
+ * accordingly.  For use in driver probe functions, to respect strapped
+ * configuration settings.
+ */
+int genphy_read_config(struct phy_device *phydev)
+{
+	int bmcr;
+
+	bmcr = phy_read(phydev, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
+
+	if (BMCR_ANENABLE & bmcr) {
+		phydev->autoneg = AUTONEG_ENABLE;
+
+		phydev->speed = 0;
+		phydev->duplex = -1;
+	} else {
+		phydev->autoneg = AUTONEG_DISABLE;
+
+		if (bmcr & BMCR_FULLDPLX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		if (bmcr & BMCR_SPEED1000)
+			phydev->speed = SPEED_1000;
+		else if (bmcr & BMCR_SPEED100)
+			phydev->speed = SPEED_100;
+		else
+			phydev->speed = SPEED_10;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_read_config);
+
 /* This is used for the phy device which doesn't support the MMD extended
  * register access, but it does have side effect when we are trying to access
  * the MMD register via indirect method.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c4c2379e010..5a8821c8105d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -970,6 +970,7 @@ void phy_attached_info(struct phy_device *phydev);
 
 /* Clause 22 PHY */
 int genphy_config_init(struct phy_device *phydev);
+int genphy_read_config(struct phy_device *phydev);
 int genphy_setup_forced(struct phy_device *phydev);
 int genphy_restart_aneg(struct phy_device *phydev);
 int genphy_config_aneg(struct phy_device *phydev);
-- 
2.16.3

[PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[esben.haabendal]

From: Esben Haabendal <eha@deif.com>

Read configration settings, to allow automatic forced speed/duplex setup
by hardware strapping.

Signed-off-by: Esben Haabendal <eha@deif.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/phy/dp83640.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..01e21b4998ad 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1134,6 +1134,10 @@ static int dp83640_probe(struct phy_device *phydev)
 	if (!dp83640)
 		goto no_memory;
 
+	err = genphy_read_config(phydev);
+	if (err)
+		goto no_config;
+
 	dp83640->phydev = phydev;
 	INIT_DELAYED_WORK(&dp83640->ts_work, rx_timestamp_work);
 
@@ -1166,6 +1170,7 @@ static int dp83640_probe(struct phy_device *phydev)
 
 no_register:
 	clock->chosen = NULL;
+no_config:
 	kfree(dp83640);
 no_memory:
 	dp83640_clock_put(clock);
-- 
2.16.3

Re: [PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[Florian Fainelli]

On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Add a function for use in PHY driver probe functions, reading current
> autoneg, speed and duplex configuration from BMCR register.
> 
> Useful for PHY that supports hardware strapped configuration, enabling
> Linux to respect that configuration (i.e. strapped non-autoneg
> configuration).
> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 74664a6c0cdc..cc52ff2a2344 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_config_init);
>  
> +/**
> + * genphy_read_config - read configuration from PHY
> + * @phydev: target phy_device struct
> + *
> + * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
> + * accordingly.  For use in driver probe functions, to respect strapped
> + * configuration settings.
> + */
> +int genphy_read_config(struct phy_device *phydev)

This duplicates what already exists, in part at least within
genphy_read_status() can you find a way to use that?

I really wonder how this is going to work though because an user can
decide to force the PHY to have auto-negotiation disabled just like a
MAC could actually attempt to do that while connecting to the PHY...
more comments in patch 2.
-- 
Florian

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Florian Fainelli]

On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
> From: Esben Haabendal <eha@deif.com>
> 
> Read configration settings, to allow automatic forced speed/duplex setup
> by hardware strapping.

OK but why? What problem is this solving for you? In general, we do not
really want to preserve too much of what the PHY has been previously
configured with, provided that the PHY driver can re-instate these
configuration values.

I just wonder how this can be robust when you connect this PHY with
auto-negotiation disabled to a peer that expects a set of link
parameters not covered by the default advertisement values? This really
looks like a recipe for disaster when you could just disable
auto-negotiation with ethtool.

> 
> Signed-off-by: Esben Haabendal <eha@deif.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/net/phy/dp83640.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 654f42d00092..01e21b4998ad 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1134,6 +1134,10 @@ static int dp83640_probe(struct phy_device *phydev)
>  	if (!dp83640)
>  		goto no_memory;
>  
> +	err = genphy_read_config(phydev);
> +	if (err)
> +		goto no_config;
> +
>  	dp83640->phydev = phydev;
>  	INIT_DELAYED_WORK(&dp83640->ts_work, rx_timestamp_work);
>  
> @@ -1166,6 +1170,7 @@ static int dp83640_probe(struct phy_device *phydev)
>  
>  no_register:
>  	clock->chosen = NULL;
> +no_config:
>  	kfree(dp83640);
>  no_memory:
>  	dp83640_clock_put(clock);
> 

-- 
Florian

Re: [PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[Esben Haabendal]

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Add a function for use in PHY driver probe functions, reading current
>> autoneg, speed and duplex configuration from BMCR register.
>> 
>> Useful for PHY that supports hardware strapped configuration, enabling
>> Linux to respect that configuration (i.e. strapped non-autoneg
>> configuration).
>> 
>> Signed-off-by: Esben Haabendal <eha@deif.com>
>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/phy.h          |  1 +
>>  2 files changed, 42 insertions(+)
>> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 74664a6c0cdc..cc52ff2a2344 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_config_init);
>>  
>> +/**
>> + * genphy_read_config - read configuration from PHY
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
>> + * accordingly.  For use in driver probe functions, to respect strapped
>> + * configuration settings.
>> + */
>> +int genphy_read_config(struct phy_device *phydev)
>
> This duplicates what already exists, in part at least within
> genphy_read_status() can you find a way to use that?

Make a small static function for updating duplex and speed fields from a
BMCR value.  It will not be big re-use, but it would make sense.  I will
do that in next patch version.

/Esben

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Read configration settings, to allow automatic forced speed/duplex setup
>> by hardware strapping.
>
> OK but why? What problem is this solving for you?

It is ensuring that the PHY is configured according to the HW design.
If the HW design has set the strap configuration to fx. fixed 100 Mbit
full-duplex, this avoids Linux configuring it for auto-negotiation.

> In general, we do not really want to preserve too much of what the PHY
> has been previously configured with,

Even when this "previous" configuration is the configuration selected by
the board configuration?

> provided that the PHY driver can re-instate these configuration
> values.

This is sort of what I try to do here.  The PHY driver needs to check
the BMCR register to figure out what the strapped configuration was.
Without that, it is necessary for software configuration to duplicate
the exact same configuration as is encoded in the strap configuration in
order for the PHY to be configured as it is supposed to.

> I just wonder how this can be robust when you connect this PHY with
> auto-negotiation disabled to a peer that expects a set of link
> parameters not covered by the default advertisement values?

I assume it will fail just as it will if you use ethtool to configure
the PHY wrongly.

> This really looks like a recipe for disaster when you could just
> disable auto-negotiation with ethtool.

The current PHY driver approach to always default to enable
auto-negotation, and then allow user-space to configure it differently
with ethtool.

With this patch, the default configuration is chosen based on the strap
configuration, but user-space can still change the configuration with
ethtool if needed / desired.

I don't think it is such a big change.

Bringing up the PHY with a default configuration according to HW
strapping instead of an in-kernel SW hard-coded value sounds like a good
idea to me.

/Esben

Re: [PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[Esben Haabendal]

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Add a function for use in PHY driver probe functions, reading current
>> autoneg, speed and duplex configuration from BMCR register.
>> 
>> Useful for PHY that supports hardware strapped configuration, enabling
>> Linux to respect that configuration (i.e. strapped non-autoneg
>> configuration).
>> 
>> Signed-off-by: Esben Haabendal <eha@deif.com>
>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>  drivers/net/phy/phy_device.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>  include/linux/phy.h          |  1 +
>>  2 files changed, 42 insertions(+)
>> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 74664a6c0cdc..cc52ff2a2344 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_config_init);
>>  
>> +/**
>> + * genphy_read_config - read configuration from PHY
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
>> + * accordingly.  For use in driver probe functions, to respect strapped
>> + * configuration settings.
>> + */
>> +int genphy_read_config(struct phy_device *phydev)
>
> This duplicates what already exists, in part at least within
> genphy_read_status() can you find a way to use that?

Make a small static function for updating duplex and speed fields from a
BMCR value.  It will not be big re-use, but it would make sense.  I will
do that in next patch version.

/Esben

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
>> From: Esben Haabendal <eha@deif.com>
>> 
>> Read configration settings, to allow automatic forced speed/duplex setup
>> by hardware strapping.
>
> OK but why? What problem is this solving for you?

It is ensuring that the PHY is configured according to the HW design.
If the HW design has set the strap configuration to fx. fixed 100 Mbit
full-duplex, this avoids Linux configuring it for auto-negotiation.

> In general, we do not really want to preserve too much of what the PHY
> has been previously configured with,

Even when this "previous" configuration is the configuration selected by
the board configuration?

> provided that the PHY driver can re-instate these configuration
> values.

This is sort of what I try to do here.  The PHY driver needs to check
the BMCR register to figure out what the strapped configuration was.
Without that, it is necessary for software configuration to duplicate
the exact same configuration as is encoded in the strap configuration in
order for the PHY to be configured as it is supposed to.

> I just wonder how this can be robust when you connect this PHY with
> auto-negotiation disabled to a peer that expects a set of link
> parameters not covered by the default advertisement values?

I assume it will fail just as it will if you use ethtool to configure
the PHY wrongly.

> This really looks like a recipe for disaster when you could just
> disable auto-negotiation with ethtool.

The current PHY driver approach to always default to enable
auto-negotation, and then allow user-space to configure it differently
with ethtool.

With this patch, the default configuration is chosen based on the strap
configuration, but user-space can still change the configuration with
ethtool if needed / desired.

I don't think it is such a big change.

Bringing up the PHY with a default configuration according to HW
strapping instead of an in-kernel SW hard-coded value sounds like a good
idea to me.

/Esben

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Andrew Lunn]

On Thu, Apr 05, 2018 at 10:30:45PM +0200, Esben Haabendal wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
> > On 04/05/2018 04:44 AM, esben.haabendal@gmail.com wrote:
> >> From: Esben Haabendal <eha@deif.com>
> >> 
> >> Read configration settings, to allow automatic forced speed/duplex setup
> >> by hardware strapping.
> >
> > OK but why? What problem is this solving for you?
> 
> It is ensuring that the PHY is configured according to the HW design.
> If the HW design has set the strap configuration to fx. fixed 100 Mbit
> full-duplex, this avoids Linux configuring it for auto-negotiation.

Hi Esben

Are you sure it contains the HW strapping? Is the driver hitting the
PHY with a hard reset to make it go back to the strapping defaults?
Or could it still contain whatever state the last boot of Linux, or
maybe the bootloader, left the PHY in?

      Andrew

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 5 Apr 2018 22:40:49 +0200

> Or could it still contain whatever state the last boot of Linux, or
> maybe the bootloader, left the PHY in?

Right, this is my concern as well.

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

David Miller <davem@davemloft.net> writes:

> From: Andrew Lunn <andrew@lunn.ch>
> Date: Thu, 5 Apr 2018 22:40:49 +0200
>
>> Or could it still contain whatever state the last boot of Linux, or
>> maybe the bootloader, left the PHY in?
>
> Right, this is my concern as well.

I don't think that should happen.
With config_init() being called (in phy_init_hw()) after soft_reset(),
any state set by software should be cleared.

>From DP83620 datasheet description of what happens when BMCR_RESET is
set:

    The software reset will reset the device such that all registers
    will be reset to default values and the hardware configuration
    values will be maintained.

But something else that could be a concern is the risk that there is
boards out there with wrong hardware configuration, which works with
current Linux (because it ignores hardware configuration).  Such designs
could break with this patch.

If we need to safeguard against that, maybe we could just keep the
genphy_read_config() function in the kernel, and let board specific code
use it as a phy_fixup where hardware configuration is to be respected.

Would that be a better approach?

/Esben