[net-next,v2,01/10] net: phy: Update PHY linkmodes after config_init

Commit Message

Maxime Chevallier Feb. 7, 2019, 9:49 a.m. UTC

We want to be able to update a PHY's supported list in the config_init
callback, so move the Pause parameters settings from phydrv->features
after calling config_init to make sure these  parameters aren't
overwritten.

Signed-off-by: Maxime Chevallier <[e-mail]>
---
 drivers/net/phy/phy_device.c | 89 +++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 31 deletions(-)

Comments

Maxime Chevallier Feb. 7, 2019, 10:31 a.m. UTC | #1

Hello everyone,

On Thu,  7 Feb 2019 10:49:30 +0100
Maxime Chevallier <[e-mail]> wrote:

>We want to be able to update a PHY's supported list in the config_init
>callback, so move the Pause parameters settings from phydrv->features
>after calling config_init to make sure these  parameters aren't
>overwritten.

I realised that I did not include updating the modes according to the
limitations imposed by of_set_phy_supported in this series. If there's
a speed limitation set in the DT, it could be overwritten by
config_init.

I'll respin the series with this addition, sorry about that.

Maxime

Andrew Lunn Feb. 7, 2019, 1:48 p.m. UTC | #2

On Thu, Feb 07, 2019 at 10:49:30AM +0100, Maxime Chevallier wrote:
> We want to be able to update a PHY's supported list in the config_init
> callback, so move the Pause parameters settings from phydrv->features
> after calling config_init to make sure these  parameters aren't
> overwritten.

Hi Maxime

I have a patch which makes some core changes to support PHY doing
runtime feature detection. I would prefer to use them, than this.

Either I or Heiner will post them soon.

       Andrew

Maxime Chevallier Feb. 7, 2019, 1:55 p.m. UTC | #3

Hello Andrew,

On Thu, 7 Feb 2019 14:48:07 +0100
Andrew Lunn <[e-mail]> wrote:

>On Thu, Feb 07, 2019 at 10:49:30AM +0100, Maxime Chevallier wrote:
>> We want to be able to update a PHY's supported list in the config_init
>> callback, so move the Pause parameters settings from phydrv->features
>> after calling config_init to make sure these  parameters aren't
>> overwritten.  
>
>Hi Maxime
>
>I have a patch which makes some core changes to support PHY doing
>runtime feature detection. I would prefer to use them, than this.
>
>Either I or Heiner will post them soon.

Sure, no problem, thanks for doing this. I'll be happy to review and
test that on my side.

As I said, I lack the big picture view on that part so my approach was
pretty naive, I'm glad you can take care of this :)

Thanks,

Maxime

Heiner Kallweit Feb. 7, 2019, 6:21 p.m. UTC | #4

On 07.02.2019 14:55, Maxime Chevallier wrote:
> Hello Andrew,
> 
> On Thu, 7 Feb 2019 14:48:07 +0100
> Andrew Lunn <[e-mail]> wrote:
> 
>> On Thu, Feb 07, 2019 at 10:49:30AM +0100, Maxime Chevallier wrote:
>>> We want to be able to update a PHY's supported list in the config_init
>>> callback, so move the Pause parameters settings from phydrv->features
>>> after calling config_init to make sure these  parameters aren't
>>> overwritten.  
>>
>> Hi Maxime
>>
>> I have a patch which makes some core changes to support PHY doing
>> runtime feature detection. I would prefer to use them, than this.
>>
>> Either I or Heiner will post them soon.
> 
> Sure, no problem, thanks for doing this. I'll be happy to review and
> test that on my side.
> 
The second version of the patch set comes in time because I was just
about to submit parts of it on Maxime's behalf. We may have some
dependencies between adding the get_features callback in phy_driver
and this patch set. Maybe the patch set needs to be splitted.
I think tomorrow or the day after I can have a closer look at this.

> As I said, I lack the big picture view on that part so my approach was
> pretty naive, I'm glad you can take care of this :)
> 
Don't be so self-effacing, it's good work :)

> Thanks,
> 
> Maxime
> 
Heiner

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 891e0178b97f..18a10565efd4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1059,6 +1059,59 @@  static int phy_poll_reset(struct phy_device *phydev)
 	return 0;
 }
 
+/**
+ * phy_update_linkmodes - Update and sanitize linkmodes and pause parameters
+ * @phydev: The PHY device whose parameters we want to update.
+ *
+ * Description: The list of supported and advertised linkmodes is not
+ * straightforward to maintain, since PHYs and MACs are subject to quirks and
+ * erratas. This function re-builds the list of the supported pause parameters
+ * by taking into account the parameters expressed in the driver's features
+ * list.
+ */
+static void phy_update_linkmodes(struct phy_device *phydev)
+{
+	struct device_driver *drv = phydev->mdio.dev.driver;
+	struct phy_driver *phydrv = to_phy_driver(drv);
+
+	mutex_lock(&phydev->lock);
+
+	/* The Pause Frame bits indicate that the PHY can support passing
+	 * pause frames. During autonegotiation, the PHYs will determine if
+	 * they should allow pause frames to pass.  The MAC driver should then
+	 * use that result to determine whether to enable flow control via
+	 * pause frames.
+	 *
+	 * Normally, PHY drivers should not set the Pause bits, and instead
+	 * allow phylib to do that.  However, there may be some situations
+	 * (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 {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+				 phydev->supported);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 phydev->supported);
+	}
+
+	linkmode_copy(phydev->advertising, phydev->supported);
+
+	mutex_unlock(&phydev->lock);
+}
+
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
@@ -1082,6 +1135,11 @@  int phy_init_hw(struct phy_device *phydev)
 	if (phydev->drv->config_init)
 		ret = phydev->drv->config_init(phydev);
 
+	/* Update and sanitize the supported and advertised linkmodes, since
+	 * they might have been changed in config_init
+	 */
+	phy_update_linkmodes(phydev);
+
 	return ret;
 }
 EXPORT_SYMBOL(phy_init_hw);
@@ -2221,37 +2279,6 @@  static int phy_probe(struct device *dev)
 	 */
 	of_set_phy_eee_broken(phydev);
 
-	/* The Pause Frame bits indicate that the PHY can support passing
-	 * pause frames. During autonegotiation, the PHYs will determine if
-	 * they should allow pause frames to pass.  The MAC driver should then
-	 * use that result to determine whether to enable flow control via
-	 * pause frames.
-	 *
-	 * Normally, PHY drivers should not set the Pause bits, and instead
-	 * allow phylib to do that.  However, there may be some situations
-	 * (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 {
-		linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-				 phydev->supported);
-		linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-				 phydev->supported);
-	}
-
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;