[PATCH net-next] net: phy: Add support for asking the PHY its abilities

[PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Heiner Kallweit]

From: Andrew Lunn <andrew@lunn.ch>
Add support for runtime determination of what the PHY supports, by
adding a new function to the phy driver. The get_features call should
set the phydev->supported member with the features the PHY supports.
It is only called if phydrv->features is NULL.

This requires minor changes to pause. The PHY driver should not set
pause abilities, except for when it has odd cause capabilities, e.g.
pause cannot be disabled. With this change, phydev->supported already
contains the drivers abilities, including pause. So rather than
considering phydrv->features, look at the phydev->supported, and
enable pause if neither of the pause bits are already set.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
[hkallweit1@gmail.com: fixed small checkpatch complaint in one comment]
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 31 +++++++++++++++----------------
 include/linux/phy.h          |  6 ++++++
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 92b7a71df..8573d17ec 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2236,7 +2236,14 @@ static int phy_probe(struct device *dev)
 	 * a controller will attach, and may modify one
 	 * or both of these values
 	 */
-	linkmode_copy(phydev->supported, phydrv->features);
+	if (phydrv->features) {
+		linkmode_copy(phydev->supported, phydrv->features);
+	} else {
+		err = phydrv->get_features(phydev);
+		if (err)
+			goto out;
+	}
+
 	of_set_phy_supported(phydev);
 	linkmode_copy(phydev->advertising, phydev->supported);
 
@@ -2256,20 +2263,8 @@ static int phy_probe(struct device *dev)
 	 * (e.g. hardware erratum) where the driver wants to set only one
 	 * of these bits.
 	 */
-	if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features) ||
-	    test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydrv->features)) {
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-				   phydev->supported);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-				   phydev->supported);
-		if (test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydrv->features))
-			linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-					 phydev->supported);
-		if (test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-			     phydrv->features))
-			linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-					 phydev->supported);
-	} else {
+	if (!test_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported) &&
+	    !test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported)) {
 		linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 				 phydev->supported);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -2315,7 +2310,11 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 {
 	int retval;
 
-	if (WARN_ON(!new_driver->features)) {
+	/* Either the features are hard coded, or dynamically
+	 * determine. It cannot be both or neither
+	 */
+	if (WARN_ON((!new_driver->features && !new_driver->get_features) ||
+		    (new_driver->features && new_driver->get_features))) {
 		pr_err("%s: Driver features are missing\n", new_driver->name);
 		return -EINVAL;
 	}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f41bf651f..d2ffae992 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -502,6 +502,12 @@ struct phy_driver {
 	 */
 	int (*probe)(struct phy_device *phydev);
 
+	/*
+	 * Probe the hardware to determine what abilities it has.
+	 * Should only set phydev->supported.
+	 */
+	int (*get_features)(struct phy_device *phydev);
+
 	/* PHY Power Management */
 	int (*suspend)(struct phy_device *phydev);
 	int (*resume)(struct phy_device *phydev);
-- 
2.20.1

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Andrew Lunn]

On Sat, Feb 09, 2019 at 03:24:47PM +0100, Heiner Kallweit wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Add support for runtime determination of what the PHY supports, by
> adding a new function to the phy driver. The get_features call should
> set the phydev->supported member with the features the PHY supports.
> It is only called if phydrv->features is NULL.
> 
> This requires minor changes to pause. The PHY driver should not set
> pause abilities, except for when it has odd cause capabilities, e.g.
> pause cannot be disabled. With this change, phydev->supported already
> contains the drivers abilities, including pause. So rather than
> considering phydrv->features, look at the phydev->supported, and
> enable pause if neither of the pause bits are already set.

Hi Heiner

Ah, cool, these are the patches i was asking for, when you asked
about splitting Maxime's patches. There is one more in my tree which
converts the marvell10g to using this. I think that should be posted
as well. It makes it clear how this should be used, and it replaces
one of the patches in Maxime's set.

> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment]

Thanks.

	Andrew

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 9 Feb 2019 15:24:47 +0100

> From: Andrew Lunn <andrew@lunn.ch>
> Add support for runtime determination of what the PHY supports, by
> adding a new function to the phy driver. The get_features call should
> set the phydev->supported member with the features the PHY supports.
> It is only called if phydrv->features is NULL.
> 
> This requires minor changes to pause. The PHY driver should not set
> pause abilities, except for when it has odd cause capabilities, e.g.
> pause cannot be disabled. With this change, phydev->supported already
> contains the drivers abilities, including pause. So rather than
> considering phydrv->features, look at the phydev->supported, and
> enable pause if neither of the pause bits are already set.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment]
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied.

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Heiner Kallweit]

On 09.02.2019 17:35, Andrew Lunn wrote:
> On Sat, Feb 09, 2019 at 03:24:47PM +0100, Heiner Kallweit wrote:
>> From: Andrew Lunn <andrew@lunn.ch>
>> Add support for runtime determination of what the PHY supports, by
>> adding a new function to the phy driver. The get_features call should
>> set the phydev->supported member with the features the PHY supports.
>> It is only called if phydrv->features is NULL.
>>
>> This requires minor changes to pause. The PHY driver should not set
>> pause abilities, except for when it has odd cause capabilities, e.g.
>> pause cannot be disabled. With this change, phydev->supported already
>> contains the drivers abilities, including pause. So rather than
>> considering phydrv->features, look at the phydev->supported, and
>> enable pause if neither of the pause bits are already set.
> 
> Hi Heiner
> 
> Ah, cool, these are the patches i was asking for, when you asked
> about splitting Maxime's patches. There is one more in my tree which
> converts the marvell10g to using this. I think that should be posted
> as well. It makes it clear how this should be used, and it replaces
> one of the patches in Maxime's set.
> 
I know, it's patch 15 in your series ;) And I'm aware that usually new
core functionality is acceptable only if it comes together with a user,
to avoid having billions of orphaned good ideas in the code.
I focused on the core here to not get lost in all the new stuff, and to
provide Maxime with some direction for adjusting his series.

>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> [hkallweit1@gmail.com: fixed small checkpatch complaint in one comment]
> 
> Thanks.
> 
> 	Andrew
> 
Heiner

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Andrew Lunn]

> I know, it's patch 15 in your series ;) And I'm aware that usually new
> core functionality is acceptable only if it comes together with a user,
> to avoid having billions of orphaned good ideas in the code.
> I focused on the core here to not get lost in all the new stuff, and to
> provide Maxime with some direction for adjusting his series.

I'm just trying to avoid Maxime reimplementing something when we
already have a patch:

https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9

Maxime, feel free to cherry-pick this into your series.

	Andrew

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Heiner Kallweit]

On 09.02.2019 20:42, Andrew Lunn wrote:
>> I know, it's patch 15 in your series ;) And I'm aware that usually new
>> core functionality is acceptable only if it comes together with a user,
>> to avoid having billions of orphaned good ideas in the code.
>> I focused on the core here to not get lost in all the new stuff, and to
>> provide Maxime with some direction for adjusting his series.
> 
> I'm just trying to avoid Maxime reimplementing something when we
> already have a patch:
> 
Sure, I didn't mean Maxime should re-implement things we have in the pipe.
I meant it more in a way that he gets an idea in which direction we're moving.

> https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9
> 
> Maxime, feel free to cherry-pick this into your series.
> 
I'll submit this one too.

> 	Andrew
> 
Heiner

Oh, and I just saw: When we talk about/with somebody, we should add him to the
mail addressee list.

Re: [PATCH net-next] net: phy: Add support for asking the PHY its abilities

[Heiner Kallweit]

On 09.02.2019 20:50, Heiner Kallweit wrote:
> On 09.02.2019 20:42, Andrew Lunn wrote:
>>> I know, it's patch 15 in your series ;) And I'm aware that usually new
>>> core functionality is acceptable only if it comes together with a user,
>>> to avoid having billions of orphaned good ideas in the code.
>>> I focused on the core here to not get lost in all the new stuff, and to
>>> provide Maxime with some direction for adjusting his series.
>>
>> I'm just trying to avoid Maxime reimplementing something when we
>> already have a patch:
>>
> Sure, I didn't mean Maxime should re-implement things we have in the pipe.
> I meant it more in a way that he gets an idea in which direction we're moving.
> 
>> https://github.com/lunn/linux/commit/07e3fa8f183f05a09d969a9378534da35238eeb9
>>
>> Maxime, feel free to cherry-pick this into your series.
>>
> I'll submit this one too.
> 
Just saw that this patch depends on patch 5 of Maxime's series. And it needs a
small change because the generic function was renamed to
genphy_c45_pma_read_abilities(). So indeed it may be better if Maxime adds
this patch to his series.

One comment regarding the pause bits in Maxime's patch 5:
If no pause bit is set the core automatically sets both. So we have to do this
neither in the marvell10g driver nor in the generic read-abilities function.

>> 	Andrew
>>
Heiner

> Heiner
> 
> Oh, and I just saw: When we talk about/with somebody, we should add him to the
> mail addressee list.
>