[PATCH RFC 0/4] net: phy: improve fixed_phy / swphy handling

[PATCH RFC 0/4] net: phy: improve fixed_phy / swphy handling

[Heiner Kallweit]

Based on discussion [0] I prepared a patch set for improving few
aspects of swphy and fixed_phy handling. So far it's compile-tested
only. I'd appreciate testing on different devices.

[0] https://marc.info/?t=156553610700001&r=1&w=2

Heiner Kallweit (4):
  net: phy: swphy: emulate register MII_ESTATUS
  net: phy: allow to bind genphy driver at probe time
  net: phy: swphy: bind swphy to genphy driver at probe time
  net: phy: fixed_phy: let genphy driver set supported and advertised
    modes

 drivers/net/phy/fixed_phy.c  | 23 -----------------------
 drivers/net/phy/phy_device.c |  3 +--
 drivers/net/phy/swphy.c      | 11 ++++++++++-
 include/linux/phy.h          |  4 ++++
 4 files changed, 15 insertions(+), 26 deletions(-)

-- 
2.22.0

[PATCH RFC 1/4] net: phy: swphy: emulate register MII_ESTATUS

[Heiner Kallweit]

When the genphy driver binds to a swphy it will call
genphy_read_abilites that will try to read MII_ESTATUS if BMSR_ESTATEN
is set in MII_BMSR. So far this would read the default value 0xffff
and 1000FD and 1000HD are reported as supported just by chance.
Better add explicit support for emulating MII_ESTATUS.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/swphy.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/swphy.c b/drivers/net/phy/swphy.c
index dad22481d..53c214a22 100644
--- a/drivers/net/phy/swphy.c
+++ b/drivers/net/phy/swphy.c
@@ -22,6 +22,7 @@ struct swmii_regs {
 	u16 bmsr;
 	u16 lpa;
 	u16 lpagb;
+	u16 estat;
 };
 
 enum {
@@ -48,6 +49,7 @@ static const struct swmii_regs speed[] = {
 	[SWMII_SPEED_1000] = {
 		.bmsr  = BMSR_ESTATEN,
 		.lpagb = LPA_1000FULL | LPA_1000HALF,
+		.estat = ESTATUS_1000_TFULL | ESTATUS_1000_THALF,
 	},
 };
 
@@ -56,11 +58,13 @@ static const struct swmii_regs duplex[] = {
 		.bmsr  = BMSR_ESTATEN | BMSR_100HALF,
 		.lpa   = LPA_10HALF | LPA_100HALF,
 		.lpagb = LPA_1000HALF,
+		.estat = ESTATUS_1000_THALF,
 	},
 	[SWMII_DUPLEX_FULL] = {
 		.bmsr  = BMSR_ESTATEN | BMSR_100FULL,
 		.lpa   = LPA_10FULL | LPA_100FULL,
 		.lpagb = LPA_1000FULL,
+		.estat = ESTATUS_1000_TFULL,
 	},
 };
 
@@ -112,6 +116,7 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
 {
 	int speed_index, duplex_index;
 	u16 bmsr = BMSR_ANEGCAPABLE;
+	u16 estat = 0;
 	u16 lpagb = 0;
 	u16 lpa = 0;
 
@@ -125,6 +130,7 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
 	duplex_index = state->duplex ? SWMII_DUPLEX_FULL : SWMII_DUPLEX_HALF;
 
 	bmsr |= speed[speed_index].bmsr & duplex[duplex_index].bmsr;
+	estat |= speed[speed_index].estat & duplex[duplex_index].estat;
 
 	if (state->link) {
 		bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
@@ -151,6 +157,8 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
 		return lpa;
 	case MII_STAT1000:
 		return lpagb;
+	case MII_ESTATUS:
+		return estat;
 
 	/*
 	 * We do not support emulating Clause 45 over Clause 22 register
-- 
2.22.0

Re: [PATCH RFC 1/4] net: phy: swphy: emulate register MII_ESTATUS

[Andrew Lunn]

On Tue, Aug 13, 2019 at 11:24:56PM +0200, Heiner Kallweit wrote:
> When the genphy driver binds to a swphy it will call
> genphy_read_abilites that will try to read MII_ESTATUS if BMSR_ESTATEN
> is set in MII_BMSR. So far this would read the default value 0xffff
> and 1000FD and 1000HD are reported as supported just by chance.
> Better add explicit support for emulating MII_ESTATUS.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe time

[Heiner Kallweit]

In cases like a fixed phy that is never attached to a net_device we
may want to bind the genphy driver at probe time. Setting a PHY ID of
0xffffffff to bind the genphy driver would fail due to a check in
get_phy_device(). Therefore let's change the PHY ID the genphy driver
binds to to 0xfffffffe. This still shouldn't match any real PHY,
and it will pass the check in get_phy_devcie().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 3 +--
 include/linux/phy.h          | 4 ++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 163295dbc..54f80af31 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n)
 EXPORT_SYMBOL(phy_drivers_unregister);
 
 static struct phy_driver genphy_driver = {
-	.phy_id		= 0xffffffff,
-	.phy_id_mask	= 0xffffffff,
+	PHY_ID_MATCH_EXACT(GENPHY_ID),
 	.name		= "Generic PHY",
 	.soft_reset	= genphy_no_soft_reset,
 	.get_features	= genphy_read_abilities,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5ac7d2137..3b07bce78 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -37,6 +37,10 @@
 #define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
 				 SUPPORTED_1000baseT_Full)
 
+#define GENPHY_ID_HIGH		0xffffU
+#define GENPHY_ID_LOW		0xfffeU
+#define GENPHY_ID		((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW)
+
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_basic_t1_features) __ro_after_init;
 extern __ETHTOOL_DECLARE_LINK_MODE_MASK(phy_gbit_features) __ro_after_init;
-- 
2.22.0

Re: [PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe time

[Florian Fainelli]

On 8/13/19 2:25 PM, Heiner Kallweit wrote:
> In cases like a fixed phy that is never attached to a net_device we
> may want to bind the genphy driver at probe time. Setting a PHY ID of
> 0xffffffff to bind the genphy driver would fail due to a check in
> get_phy_device(). Therefore let's change the PHY ID the genphy driver
> binds to to 0xfffffffe. This still shouldn't match any real PHY,
> and it will pass the check in get_phy_devcie().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 3 +--
>  include/linux/phy.h          | 4 ++++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 163295dbc..54f80af31 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n)
>  EXPORT_SYMBOL(phy_drivers_unregister);
>  
>  static struct phy_driver genphy_driver = {
> -	.phy_id		= 0xffffffff,
> -	.phy_id_mask	= 0xffffffff,
> +	PHY_ID_MATCH_EXACT(GENPHY_ID),
>  	.name		= "Generic PHY",
>  	.soft_reset	= genphy_no_soft_reset,
>  	.get_features	= genphy_read_abilities,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5ac7d2137..3b07bce78 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -37,6 +37,10 @@
>  #define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
>  				 SUPPORTED_1000baseT_Full)
>  
> +#define GENPHY_ID_HIGH		0xffffU
> +#define GENPHY_ID_LOW		0xfffeU
> +#define GENPHY_ID		((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW)

This is a possible user ABI change here, if there is anything that
relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking
it. We might as well try to assign ourselves a specific PHY OUI, very
much like the Linux USB hubs show up with a Linux Foundation vendor ID.
-- 
Florian

Re: [PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe time

[Heiner Kallweit]

On 14.08.2019 00:53, Florian Fainelli wrote:
> On 8/13/19 2:25 PM, Heiner Kallweit wrote:
>> In cases like a fixed phy that is never attached to a net_device we
>> may want to bind the genphy driver at probe time. Setting a PHY ID of
>> 0xffffffff to bind the genphy driver would fail due to a check in
>> get_phy_device(). Therefore let's change the PHY ID the genphy driver
>> binds to to 0xfffffffe. This still shouldn't match any real PHY,
>> and it will pass the check in get_phy_devcie().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 3 +--
>>  include/linux/phy.h          | 4 ++++
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 163295dbc..54f80af31 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n)
>>  EXPORT_SYMBOL(phy_drivers_unregister);
>>  
>>  static struct phy_driver genphy_driver = {
>> -	.phy_id		= 0xffffffff,
>> -	.phy_id_mask	= 0xffffffff,
>> +	PHY_ID_MATCH_EXACT(GENPHY_ID),
>>  	.name		= "Generic PHY",
>>  	.soft_reset	= genphy_no_soft_reset,
>>  	.get_features	= genphy_read_abilities,
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5ac7d2137..3b07bce78 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -37,6 +37,10 @@
>>  #define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
>>  				 SUPPORTED_1000baseT_Full)
>>  
>> +#define GENPHY_ID_HIGH		0xffffU
>> +#define GENPHY_ID_LOW		0xfffeU
>> +#define GENPHY_ID		((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW)
> 
> This is a possible user ABI change here, if there is anything that
> relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking
> it. We might as well try to assign ourselves a specific PHY OUI, very
> much like the Linux USB hubs show up with a Linux Foundation vendor ID.
> 

I see the point. However in get_phy_device() we have the following check
that should cause a PHY with ID 0xffff_ffff to be ignored. Therefore
I doubt there's any such PHY ID in use.

	/* If the phy_id is mostly Fs, there is no device there */
	if ((phy_id & 0x1fffffff) == 0x1fffffff)
		return ERR_PTR(-ENODEV);

Heiner

Re: [PATCH RFC 2/4] net: phy: allow to bind genphy driver at probe time

[Florian Fainelli]

On 8/13/19 4:02 PM, Heiner Kallweit wrote:
> On 14.08.2019 00:53, Florian Fainelli wrote:
>> On 8/13/19 2:25 PM, Heiner Kallweit wrote:
>>> In cases like a fixed phy that is never attached to a net_device we
>>> may want to bind the genphy driver at probe time. Setting a PHY ID of
>>> 0xffffffff to bind the genphy driver would fail due to a check in
>>> get_phy_device(). Therefore let's change the PHY ID the genphy driver
>>> binds to to 0xfffffffe. This still shouldn't match any real PHY,
>>> and it will pass the check in get_phy_devcie().
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/net/phy/phy_device.c | 3 +--
>>>  include/linux/phy.h          | 4 ++++
>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index 163295dbc..54f80af31 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -2388,8 +2388,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n)
>>>  EXPORT_SYMBOL(phy_drivers_unregister);
>>>  
>>>  static struct phy_driver genphy_driver = {
>>> -	.phy_id		= 0xffffffff,
>>> -	.phy_id_mask	= 0xffffffff,
>>> +	PHY_ID_MATCH_EXACT(GENPHY_ID),
>>>  	.name		= "Generic PHY",
>>>  	.soft_reset	= genphy_no_soft_reset,
>>>  	.get_features	= genphy_read_abilities,
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 5ac7d2137..3b07bce78 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -37,6 +37,10 @@
>>>  #define PHY_1000BT_FEATURES	(SUPPORTED_1000baseT_Half | \
>>>  				 SUPPORTED_1000baseT_Full)
>>>  
>>> +#define GENPHY_ID_HIGH		0xffffU
>>> +#define GENPHY_ID_LOW		0xfffeU
>>> +#define GENPHY_ID		((GENPHY_ID_HIGH << 16) | GENPHY_ID_LOW)
>>
>> This is a possible user ABI change here, if there is anything that
>> relies on reading 0xffff_ffff as a valid PHY OUI, you would be breaking
>> it. We might as well try to assign ourselves a specific PHY OUI, very
>> much like the Linux USB hubs show up with a Linux Foundation vendor ID.
>>
> 
> I see the point. However in get_phy_device() we have the following check
> that should cause a PHY with ID 0xffff_ffff to be ignored. Therefore
> I doubt there's any such PHY ID in use.
> 
> 	/* If the phy_id is mostly Fs, there is no device there */
> 	if ((phy_id & 0x1fffffff) == 0x1fffffff)
> 		return ERR_PTR(-ENODEV);

Indeed, it looks like the phy_id reported through sysfs for fixed PHY is
actually 0, so your change should be fine then, thanks!
-- 
Florian

[PATCH RFC 3/4] net: phy: swphy: bind swphy to genphy driver at probe time

[Heiner Kallweit]

Let a swphy bind to the genphy driver at probe time. This provides
automatic feature detection even if the swphy never gets attached to a
net_device. So far the genphy driver binds to a PHY as fallback only
once the PHY is attached to a net_device.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/swphy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/swphy.c b/drivers/net/phy/swphy.c
index 53c214a22..7ac5054fa 100644
--- a/drivers/net/phy/swphy.c
+++ b/drivers/net/phy/swphy.c
@@ -151,8 +151,9 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
 	case MII_BMSR:
 		return bmsr;
 	case MII_PHYSID1:
+		return GENPHY_ID_HIGH;
 	case MII_PHYSID2:
-		return 0;
+		return GENPHY_ID_LOW;
 	case MII_LPA:
 		return lpa;
 	case MII_STAT1000:
-- 
2.22.0

[PATCH RFC 4/4] net: phy: fixed_phy: let genphy driver set supported and advertised modes

[Heiner Kallweit]

A fixed phy as special swphy binds to the genphy driver that calls
genphy_read_abilities(). This function populates the supported and
advertised modes, so we don't have to do it manually.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/fixed_phy.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 7c5265fd2..db4d96f2f 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -282,29 +282,6 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
 	phy->mdio.dev.of_node = np;
 	phy->is_pseudo_fixed_link = true;
 
-	switch (status->speed) {
-	case SPEED_1000:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
-				 phy->supported);
-		/* fall through */
-	case SPEED_100:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
-				 phy->supported);
-		/* fall through */
-	case SPEED_10:
-	default:
-		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
-				 phy->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
-				 phy->supported);
-	}
-
-	phy_advertise_supported(phy);
-
 	ret = phy_device_register(phy);
 	if (ret) {
 		phy_device_free(phy);
-- 
2.22.0