[PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

phylib defaults to GMII when no phy-mode or phy-connection-type property
is specified in a DSA port node.

Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
introduced implicit support for GMII mode on ports with internal PHY to
allow a PHY connection for device trees where the phy-mode is not
explicitly set to "internal".

Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
then broke this behaviour by discarding the usage of
rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
while switching to the new .phylink_get_caps API.

With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
Remove it altogether and add back the GMII capability - this time to
rtl8365mb_phylink_get_caps() - so that the above default behaviour works
for ports with internal PHY again.

Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---

Luiz, Russel:

Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
claims to have been fixing a regression in the net-next tree - is that
right? I seem to have missed both referenced commits when they were
posted and never hit this issue personally. I only found things now
during some other refactoring and the test for GMII looked weird to me
so I went and investigated.

Could you please help me identify that Fixes: tag? Just for my own
understanding of what caused this added requirement for GMII on ports
with internal PHY.

---
 drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 3bb42a9f236d..769f672e9128 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
 	return 0;
 }
 
-static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
-					 phy_interface_t interface)
-{
-	int ext_int;
-
-	ext_int = rtl8365mb_extint_port_map[port];
-
-	if (ext_int < 0 &&
-	    (interface == PHY_INTERFACE_MODE_NA ||
-	     interface == PHY_INTERFACE_MODE_INTERNAL ||
-	     interface == PHY_INTERFACE_MODE_GMII))
-		/* Internal PHY */
-		return true;
-	else if ((ext_int >= 1) &&
-		 phy_interface_mode_is_rgmii(interface))
-		/* Extension MAC */
-		return true;
-
-	return false;
-}
-
 static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
 				       struct phylink_config *config)
 {
-	if (dsa_is_user_port(ds, port))
+	if (dsa_is_user_port(ds, port)) {
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
-	else if (dsa_is_cpu_port(ds, port))
+
+		/* GMII is the default interface mode for phylib, so
+		 * we have to support it for ports with integrated PHY.
+		 */
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  config->supported_interfaces);
+	} else if (dsa_is_cpu_port(ds, port)) {
 		phy_interface_set_rgmii(config->supported_interfaces);
+	}
 
 	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
 				   MAC_10 | MAC_100 | MAC_1000FD;
@@ -996,12 +982,6 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
 	struct realtek_priv *priv = ds->priv;
 	int ret;
 
-	if (!rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
-		dev_err(priv->dev, "phy mode %s is unsupported on port %d\n",
-			phy_modes(state->interface), port);
-		return;
-	}
-
 	if (mode != MLO_AN_PHY && mode != MLO_AN_FIXED) {
 		dev_err(priv->dev,
 			"port %d supports only conventional PHY or fixed-link\n",
-- 
2.36.0

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Russell King (Oracle)]

On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> phylib defaults to GMII when no phy-mode or phy-connection-type property
> is specified in a DSA port node.
> 
> Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
> introduced implicit support for GMII mode on ports with internal PHY to
> allow a PHY connection for device trees where the phy-mode is not
> explicitly set to "internal".
> 
> Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> then broke this behaviour by discarding the usage of
> rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
> while switching to the new .phylink_get_caps API.
> 
> With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
> Remove it altogether and add back the GMII capability - this time to
> rtl8365mb_phylink_get_caps() - so that the above default behaviour works
> for ports with internal PHY again.

Oops - I guess this has been caused by the delay between my patch being
initially prepared, it sitting around in my tree for many months while
other patches get merged, and it eventually seeing the light of day.

Sorry about that.

> Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
> 
> Luiz, Russel:
> 
> Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> claims to have been fixing a regression in the net-next tree - is that
> right? I seem to have missed both referenced commits when they were
> posted and never hit this issue personally. I only found things now
> during some other refactoring and the test for GMII looked weird to me
> so I went and investigated.
> 
> Could you please help me identify that Fixes: tag? Just for my own
> understanding of what caused this added requirement for GMII on ports
> with internal PHY.

I have absolutely no idea. I don't think any "requirement" has ever been
added - phylib has always defaulted to GMII, so as the driver stood when
it was first submitted on Oct 18 2021, I don't see how it could have
worked, unless the DT it was being tested with specified a phy-mode of
"internal". As you were the one who submitted it, you would have a
better idea.

The only suggestion I have is to bisect to find out exactly what caused
the GMII vs INTERNAL issue to crop up.

> 
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 3bb42a9f236d..769f672e9128 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  	return 0;
>  }
>  
> -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> -					 phy_interface_t interface)
> -{
> -	int ext_int;
> -
> -	ext_int = rtl8365mb_extint_port_map[port];
> -
> -	if (ext_int < 0 &&
> -	    (interface == PHY_INTERFACE_MODE_NA ||
> -	     interface == PHY_INTERFACE_MODE_INTERNAL ||
> -	     interface == PHY_INTERFACE_MODE_GMII))
> -		/* Internal PHY */
> -		return true;
> -	else if ((ext_int >= 1) &&
> -		 phy_interface_mode_is_rgmii(interface))
> -		/* Extension MAC */
> -		return true;
> -
> -	return false;
> -}
> -
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port))
> +	if (dsa_is_user_port(ds, port)) {

Given the updates to rtl8365mb_phy_mode_supported(), this misses out on
the check of rtl8365mb_extint_port_map[port] introduced in commit
6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint
ports").

>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> -	else if (dsa_is_cpu_port(ds, port))
> +
> +		/* GMII is the default interface mode for phylib, so
> +		 * we have to support it for ports with integrated PHY.
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +	} else if (dsa_is_cpu_port(ds, port)) {

This test also needs to be updated.

Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to
signify - maybe port unusable? Looks that way to me.

>  		phy_interface_set_rgmii(config->supported_interfaces);
> +	}
>  
>  	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>  				   MAC_10 | MAC_100 | MAC_1000FD;
> @@ -996,12 +982,6 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
>  	struct realtek_priv *priv = ds->priv;
>  	int ret;
>  
> -	if (!rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
> -		dev_err(priv->dev, "phy mode %s is unsupported on port %d\n",
> -			phy_modes(state->interface), port);
> -		return;
> -	}
> -
>  	if (mode != MLO_AN_PHY && mode != MLO_AN_FIXED) {
>  		dev_err(priv->dev,
>  			"port %d supports only conventional PHY or fixed-link\n",
> -- 
> 2.36.0
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Alvin Šipraga]

On Mon, Jun 06, 2022 at 02:31:16PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote:
> > From: Alvin Šipraga <alsi@bang-olufsen.dk>
> > 
> > phylib defaults to GMII when no phy-mode or phy-connection-type property
> > is specified in a DSA port node.
> > 
> > Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
> > introduced implicit support for GMII mode on ports with internal PHY to
> > allow a PHY connection for device trees where the phy-mode is not
> > explicitly set to "internal".
> > 
> > Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > then broke this behaviour by discarding the usage of
> > rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
> > while switching to the new .phylink_get_caps API.
> > 
> > With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
> > Remove it altogether and add back the GMII capability - this time to
> > rtl8365mb_phylink_get_caps() - so that the above default behaviour works
> > for ports with internal PHY again.
> 
> Oops - I guess this has been caused by the delay between my patch being
> initially prepared, it sitting around in my tree for many months while
> other patches get merged, and it eventually seeing the light of day.
> 
> Sorry about that.

No problem, it's actually my bad, but I am still getting to grips with the
responsibility of maintaining a Linux driver :)

> 
> > Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> > Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> > ---
> > 
> > Luiz, Russel:
> > 
> > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > claims to have been fixing a regression in the net-next tree - is that
> > right? I seem to have missed both referenced commits when they were
> > posted and never hit this issue personally. I only found things now
> > during some other refactoring and the test for GMII looked weird to me
> > so I went and investigated.
> > 
> > Could you please help me identify that Fixes: tag? Just for my own
> > understanding of what caused this added requirement for GMII on ports
> > with internal PHY.
> 
> I have absolutely no idea. I don't think any "requirement" has ever been
> added - phylib has always defaulted to GMII, so as the driver stood when
> it was first submitted on Oct 18 2021, I don't see how it could have
> worked, unless the DT it was being tested with specified a phy-mode of
> "internal". As you were the one who submitted it, you would have a
> better idea.
> 
> The only suggestion I have is to bisect to find out exactly what caused
> the GMII vs INTERNAL issue to crop up.

Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
I will try bisecting if I find the time.

For what it's worth, I believe the patch is correct and applicable in its
current form - it just lacks an explanation as to why this ever worked in the
first place.

> 
> > 
> > ---
> >  drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
> >  1 file changed, 9 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> > index 3bb42a9f236d..769f672e9128 100644
> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> > @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> >  	return 0;
> >  }
> >  
> > -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> > -					 phy_interface_t interface)
> > -{
> > -	int ext_int;
> > -
> > -	ext_int = rtl8365mb_extint_port_map[port];
> > -
> > -	if (ext_int < 0 &&
> > -	    (interface == PHY_INTERFACE_MODE_NA ||
> > -	     interface == PHY_INTERFACE_MODE_INTERNAL ||
> > -	     interface == PHY_INTERFACE_MODE_GMII))
> > -		/* Internal PHY */
> > -		return true;
> > -	else if ((ext_int >= 1) &&
> > -		 phy_interface_mode_is_rgmii(interface))
> > -		/* Extension MAC */
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> >  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
> >  				       struct phylink_config *config)
> >  {
> > -	if (dsa_is_user_port(ds, port))
> > +	if (dsa_is_user_port(ds, port)) {
> 
> Given the updates to rtl8365mb_phy_mode_supported(), this misses out on
> the check of rtl8365mb_extint_port_map[port] introduced in commit
> 6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint
> ports").

You are right, but since this is targetting net and not net-next, I figured I
would not introduce any additional changes. I have a separate series lined up
for net-next (just submitted) which fixes this in a much cleaner way.

My rationale here is: yes, this test is incorrect and is a shortcoming of the
driver, but it does not represent a real bug worthy of inclusion in the stable
trees.

> 
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> > -	else if (dsa_is_cpu_port(ds, port))
> > +
> > +		/* GMII is the default interface mode for phylib, so
> > +		 * we have to support it for ports with integrated PHY.
> > +		 */
> > +		__set_bit(PHY_INTERFACE_MODE_GMII,
> > +			  config->supported_interfaces);
> > +	} else if (dsa_is_cpu_port(ds, port)) {
> 
> This test also needs to be updated.
> 
> Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to
> signify - maybe port unusable? Looks that way to me.

Check the other series I just sent for a cleaner implementation of this.

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Luiz Angelo Daros de Luca]

> > > Luiz, Russel:
> > >
> > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > > claims to have been fixing a regression in the net-next tree - is that
> > > right? I seem to have missed both referenced commits when they were
> > > posted and never hit this issue personally. I only found things now
> > > during some other refactoring and the test for GMII looked weird to me
> > > so I went and investigated.
> > >
> > > Could you please help me identify that Fixes: tag? Just for my own
> > > understanding of what caused this added requirement for GMII on ports
> > > with internal PHY.
> >
> > I have absolutely no idea. I don't think any "requirement" has ever been
> > added - phylib has always defaulted to GMII, so as the driver stood when
> > it was first submitted on Oct 18 2021, I don't see how it could have
> > worked, unless the DT it was being tested with specified a phy-mode of
> > "internal". As you were the one who submitted it, you would have a
> > better idea.
> >
> > The only suggestion I have is to bisect to find out exactly what caused
> > the GMII vs INTERNAL issue to crop up.
>
> Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
> I will try bisecting if I find the time.

I don't know. I just got hit by the issue after a rebase (sorry, I
don't know exactly from which commit I was rebasing).
But I did test the net (!-next) and left a working commit note. You
can diff 3dd7d40b43..a5dba0f20.
If I'm to guess, I would blame:

21bd64bd717de: net: dsa: consolidate phylink creation

Regards,

Luiz

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Russell King (Oracle)]

On Tue, Jun 07, 2022 at 10:52:48AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > Luiz, Russel:
> > > >
> > > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > > > claims to have been fixing a regression in the net-next tree - is that
> > > > right? I seem to have missed both referenced commits when they were
> > > > posted and never hit this issue personally. I only found things now
> > > > during some other refactoring and the test for GMII looked weird to me
> > > > so I went and investigated.
> > > >
> > > > Could you please help me identify that Fixes: tag? Just for my own
> > > > understanding of what caused this added requirement for GMII on ports
> > > > with internal PHY.
> > >
> > > I have absolutely no idea. I don't think any "requirement" has ever been
> > > added - phylib has always defaulted to GMII, so as the driver stood when
> > > it was first submitted on Oct 18 2021, I don't see how it could have
> > > worked, unless the DT it was being tested with specified a phy-mode of
> > > "internal". As you were the one who submitted it, you would have a
> > > better idea.
> > >
> > > The only suggestion I have is to bisect to find out exactly what caused
> > > the GMII vs INTERNAL issue to crop up.
> >
> > Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
> > I will try bisecting if I find the time.
> 
> I don't know. I just got hit by the issue after a rebase (sorry, I
> don't know exactly from which commit I was rebasing).
> But I did test the net (!-next) and left a working commit note. You
> can diff 3dd7d40b43..a5dba0f20.
> If I'm to guess, I would blame:
> 
> 21bd64bd717de: net: dsa: consolidate phylink creation

Why do you suspect that commit? I fail to see any functional change in
that commit that would cause the problem.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Alvin Šipraga]

On Tue, Jun 07, 2022 at 03:05:40PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 07, 2022 at 10:52:48AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > > Luiz, Russel:
> > > > >
> > > > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > > > > claims to have been fixing a regression in the net-next tree - is that
> > > > > right? I seem to have missed both referenced commits when they were
> > > > > posted and never hit this issue personally. I only found things now
> > > > > during some other refactoring and the test for GMII looked weird to me
> > > > > so I went and investigated.
> > > > >
> > > > > Could you please help me identify that Fixes: tag? Just for my own
> > > > > understanding of what caused this added requirement for GMII on ports
> > > > > with internal PHY.
> > > >
> > > > I have absolutely no idea. I don't think any "requirement" has ever been
> > > > added - phylib has always defaulted to GMII, so as the driver stood when
> > > > it was first submitted on Oct 18 2021, I don't see how it could have
> > > > worked, unless the DT it was being tested with specified a phy-mode of
> > > > "internal". As you were the one who submitted it, you would have a
> > > > better idea.
> > > >
> > > > The only suggestion I have is to bisect to find out exactly what caused
> > > > the GMII vs INTERNAL issue to crop up.
> > >
> > > Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
> > > I will try bisecting if I find the time.
> > 
> > I don't know. I just got hit by the issue after a rebase (sorry, I
> > don't know exactly from which commit I was rebasing).
> > But I did test the net (!-next) and left a working commit note. You
> > can diff 3dd7d40b43..a5dba0f20.
> > If I'm to guess, I would blame:
> > 
> > 21bd64bd717de: net: dsa: consolidate phylink creation
> 
> Why do you suspect that commit? I fail to see any functional change in
> that commit that would cause the problem.

Agree, seems like the referenced commit makes no functional change.

But thanks for the range of commits Luiz, I found one that looks like the
culprit. It's small so I will reproduce the whole thing below. Will test later.

--------------------8<-------------------

commit a18e6521a7d95dae8c65b5b0ef6bbe624fbe808c
Author: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Date:   Fri Nov 19 16:28:06 2021 +0000

    net: phylink: handle NA interface mode in phylink_fwnode_phy_connect()
    
    Commit 4904b6ea1f9db ("net: phy: phylink: Use PHY device interface if
    N/A") introduced handling for the phy interface mode where this is not
    known at phylink creation time. This was never added to the OF/fwnode
    paths, but is necessary when the phy is present in DT, but the phy-mode
    is not specified.
    
    Add this handling.
    
    Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
    Acked-by: Florian Fainelli <f.fainelli@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 2d201a795775..3603c024109a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1325,7 +1325,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
        mutex_unlock(&phy->lock);
 
        phylink_dbg(pl,
-                   "phy: setting supported %*pb advertising %*pb\n",
+                   "phy: %s setting supported %*pb advertising %*pb\n",
+                   phy_modes(interface),
                    __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
                    __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising);
 
@@ -1443,6 +1444,12 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
        if (!phy_dev)
                return -ENODEV;
 
+       /* Use PHY device/driver interface */
+       if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
+               pl->link_interface = phy_dev->interface;
+               pl->link_config.interface = pl->link_interface;
+       }
+
        ret = phy_attach_direct(pl->netdev, phy_dev, flags,
                                pl->link_interface);
        if (ret) {

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Russell King (Oracle)]

On Tue, Jun 07, 2022 at 02:17:44PM +0000, Alvin Šipraga wrote:
> On Tue, Jun 07, 2022 at 03:05:40PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 07, 2022 at 10:52:48AM -0300, Luiz Angelo Daros de Luca wrote:
> > > > > > Luiz, Russel:
> > > > > >
> > > > > > Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> > > > > > claims to have been fixing a regression in the net-next tree - is that
> > > > > > right? I seem to have missed both referenced commits when they were
> > > > > > posted and never hit this issue personally. I only found things now
> > > > > > during some other refactoring and the test for GMII looked weird to me
> > > > > > so I went and investigated.
> > > > > >
> > > > > > Could you please help me identify that Fixes: tag? Just for my own
> > > > > > understanding of what caused this added requirement for GMII on ports
> > > > > > with internal PHY.
> > > > >
> > > > > I have absolutely no idea. I don't think any "requirement" has ever been
> > > > > added - phylib has always defaulted to GMII, so as the driver stood when
> > > > > it was first submitted on Oct 18 2021, I don't see how it could have
> > > > > worked, unless the DT it was being tested with specified a phy-mode of
> > > > > "internal". As you were the one who submitted it, you would have a
> > > > > better idea.
> > > > >
> > > > > The only suggestion I have is to bisect to find out exactly what caused
> > > > > the GMII vs INTERNAL issue to crop up.
> > > >
> > > > Alright, thanks for the quick response. Maybe Luiz has a better idea, otherwise
> > > > I will try bisecting if I find the time.
> > > 
> > > I don't know. I just got hit by the issue after a rebase (sorry, I
> > > don't know exactly from which commit I was rebasing).
> > > But I did test the net (!-next) and left a working commit note. You
> > > can diff 3dd7d40b43..a5dba0f20.
> > > If I'm to guess, I would blame:
> > > 
> > > 21bd64bd717de: net: dsa: consolidate phylink creation
> > 
> > Why do you suspect that commit? I fail to see any functional change in
> > that commit that would cause the problem.
> 
> Agree, seems like the referenced commit makes no functional change.
> 
> But thanks for the range of commits Luiz, I found one that looks like the
> culprit. It's small so I will reproduce the whole thing below. Will test later.

This one I agree could well be the culpret, but it means that the
original premise that PHY_INTERFACE_MODE_INTERNAL was being used is
incorrect - it's actually been relying on using PHY_INTERFACE_MODE_NA.

It instead means that PHY_INTERFACE_MODE_NA was being used, which
really isn't good, because PHY_INTERFACE_MODE_NA internally inside
phylink has always had a special meaning - that being with the
validate step which has been used to get _all_ possible modes from
the MAC. This was never intended to be used for anything except
phylink's internal use to retrieve that information from the MAC
driver to make decisions about what mode(s) a SFP should use.

So yes, this is most likely the culpret, and if proven, please use
it for the Fixes: tag for any fixes to drivers that incorrectly
relied upon that behaviour.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Alvin Šipraga]

On Tue, Jun 07, 2022 at 03:45:32PM +0100, Russell King (Oracle) wrote:
> 
> This one I agree could well be the culpret, but it means that the
> original premise that PHY_INTERFACE_MODE_INTERNAL was being used is
> incorrect - it's actually been relying on using PHY_INTERFACE_MODE_NA.
> 
> It instead means that PHY_INTERFACE_MODE_NA was being used, which
> really isn't good, because PHY_INTERFACE_MODE_NA internally inside
> phylink has always had a special meaning - that being with the
> validate step which has been used to get _all_ possible modes from
> the MAC. This was never intended to be used for anything except
> phylink's internal use to retrieve that information from the MAC
> driver to make decisions about what mode(s) a SFP should use.

Right, and there is even a "do not touch" warning in the enum definition of
PHY_INTERFACE_MODE_NA ;-)

Thanks for the explanation.

> 
> So yes, this is most likely the culpret, and if proven, please use
> it for the Fixes: tag for any fixes to drivers that incorrectly
> relied upon that behaviour.

If I take net and revert the aforementioned commit, then I am able to connect a
PHY and indeed the mode is _NA, as evidenced by log lines like this:

[   10.702205] realtek-smi ethernet-switch swp0: configuring for phy/ link mode

So a18e6521a7d9 is indeed the reason it broke for Luiz to begin with.

So, in summary:

- initial driver version

  rtl8365mb with no phy-mode specified on the port would connect with _NA
  because it supported it erroneously; at this point in time, when no phy-mode
  is specified in the DT, _NA gets used, which is also technically wrong

- a18e6521a7d9 ("net: phylink: handle NA interface mode in phylink_fwnode_phy_connect()")

  rtl8365mb now doesn't work without phy-mode specified because you fixed the
  _NA behaviour and now _GMII is used (which is right), but rtl8365mb doesn't
  support _GMII
  
    
- a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")

  rtl8365mb now supports _GMII so everything works again
  
- 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")

  rtl8365mb breaks again because _GMII is no longer supported due to some
  refactoring

I will re-send the patch this evening with an updated description.

Kind regards,
Alvin