[PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Nikita Yushchenko]

It was observed on spider board that with upstream kernel, PHY
auto-negotiation takes almost 1 second longer than with renesas BSP
kernel. This was tracked down to upstream kernel allowing more PHY modes
than renesas BSP kernel.

To avoid that effect when possible, always set max_speed to not more
than phy_interface allows.

While at this, also ensure that etha->speed always gets a supported
value, even if max_speed in device tree is set to something else.

Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 48 ++++++++++++++------------
 drivers/net/ethernet/renesas/rswitch.h |  1 +
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 84d09a8973b7..4b4e174e3abb 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1308,37 +1308,41 @@ static struct device_node *rswitch_get_port_node(struct rswitch_device *rdev)
 
 static int rswitch_etha_get_params(struct rswitch_device *rdev)
 {
-	u32 max_speed;
+	struct rswitch_etha *etha = rdev->etha;
 	int err;
 
 	if (!rdev->np_port)
 		return 0;	/* ignored */
 
-	err = of_get_phy_mode(rdev->np_port, &rdev->etha->phy_interface);
+	err = of_get_phy_mode(rdev->np_port, &etha->phy_interface);
 	if (err)
 		return err;
 
-	err = of_property_read_u32(rdev->np_port, "max-speed", &max_speed);
-	if (!err) {
-		rdev->etha->speed = max_speed;
-		return 0;
-	}
-
-	/* if no "max-speed" property, let's use default speed */
-	switch (rdev->etha->phy_interface) {
-	case PHY_INTERFACE_MODE_MII:
-		rdev->etha->speed = SPEED_100;
-		break;
+	switch (etha->phy_interface) {
 	case PHY_INTERFACE_MODE_SGMII:
-		rdev->etha->speed = SPEED_1000;
+		etha->max_speed = SPEED_1000;
 		break;
 	case PHY_INTERFACE_MODE_USXGMII:
-		rdev->etha->speed = SPEED_2500;
+	case PHY_INTERFACE_MODE_5GBASER:
+		etha->max_speed = SPEED_2500;
 		break;
 	default:
 		return -EINVAL;
 	}
 
+	/* Allow max_speed override */
+	of_property_read_u32(rdev->np_port, "max-speed", &etha->max_speed);
+
+	/* Set etha->speed to one of values expected by the driver */
+	if (etha->max_speed < SPEED_100)
+		return -EINVAL;
+	else if (etha->max_speed < SPEED_1000)
+		etha->speed = SPEED_100;
+	else if (etha->max_speed < SPEED_2500)
+		etha->speed = SPEED_1000;
+	else
+		etha->speed = SPEED_2500;
+
 	return 0;
 }
 
@@ -1412,6 +1416,13 @@ static void rswitch_adjust_link(struct net_device *ndev)
 static void rswitch_phy_remove_link_mode(struct rswitch_device *rdev,
 					 struct phy_device *phydev)
 {
+	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
+	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+
+	phy_set_max_speed(phydev, rdev->etha->max_speed);
+
 	if (!rdev->priv->etha_no_runtime_change)
 		return;
 
@@ -1431,8 +1442,6 @@ static void rswitch_phy_remove_link_mode(struct rswitch_device *rdev,
 	default:
 		break;
 	}
-
-	phy_set_max_speed(phydev, rdev->etha->speed);
 }
 
 static int rswitch_phy_device_init(struct rswitch_device *rdev)
@@ -1462,11 +1471,6 @@ static int rswitch_phy_device_init(struct rswitch_device *rdev)
 	if (!phydev)
 		goto out;
 
-	phy_set_max_speed(phydev, SPEED_2500);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
 	rswitch_phy_remove_link_mode(rdev, phydev);
 
 	phy_attached_info(phydev);
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 532192cbca4b..09f5d5e1933e 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -920,6 +920,7 @@ struct rswitch_etha {
 	bool external_phy;
 	struct mii_bus *mii;
 	phy_interface_t phy_interface;
+	u32 max_speed;
 	u32 psmcs;
 	u8 mac_addr[MAX_ADDR_LEN];
 	int link;
-- 
2.39.5

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Andrew Lunn]

On Mon, Feb 03, 2025 at 06:09:41PM +0100, Nikita Yushchenko wrote:
> It was observed on spider board that with upstream kernel, PHY
> auto-negotiation takes almost 1 second longer than with renesas BSP
> kernel. This was tracked down to upstream kernel allowing more PHY modes
> than renesas BSP kernel.
> 
> To avoid that effect when possible, always set max_speed to not more
> than phy_interface allows.

Please could you provide more information about the hardware. Is the
PHY more capable than the MAC?

When you have multi Gigi PHYs, the phy-mode is often taken to be the
starting point. But when the PHY has negotiated a link, it will tell
the MAC what PHY mode it needs to swap to, e.g from SGMII to
2500BaseX, etc.

You should only need max-speed when you have a PHY which can do more
than the MAC.

Also, phylink handles this a lot better than phylib. So you might want
to change rswitch to phylink, especially if you have link speeds > 1G.

	Andrew

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Nikita Yushchenko]

> You should only need max-speed when you have a PHY which can do more
> than the MAC.

This is exactly the case.

Unfortunately I don't have the spider schematics nearby, but AFAIU (one of flavours of) the board has 
PHYs capable of 5G but connected over SGMII.  When two such boards are connected to each other, on 
mainline kernel auto-negotiation takes noticeably longer than with the Renesas BSP kernel.

> Also, phylink handles this a lot better than phylib. So you might want
> to change rswitch to phylink, especially if you have link speeds > 1G.

The reverse switch happened in commit c16a5033f77b ("net: renesas: rswitch: Convert to phy_device").
I did not check the tech details of that, but decided not to touch it.

Nikita

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Andrew Lunn]

On Wed, Feb 05, 2025 at 05:26:09PM +0100, Nikita Yushchenko wrote:
> > You should only need max-speed when you have a PHY which can do more
> > than the MAC.
> 
> This is exactly the case.

O.K. Please expand the commit message to explain this.

> Unfortunately I don't have the spider schematics nearby, but AFAIU (one of
> flavours of) the board has PHYs capable of 5G but connected over SGMII.
> When two such boards are connected to each other, on mainline kernel
> auto-negotiation takes noticeably longer than with the Renesas BSP kernel.

I'm actually curious how it established a link at all. If both PHYs
are advertising 5G, they should be happy on the media side. They will
get link. But they will ask the MAC to swap to 5000BaseX or similar. I
assume the MAC cannot do that, but what does it do? How does the PHY
know it should try something slower?

> > Also, phylink handles this a lot better than phylib. So you might want
> > to change rswitch to phylink, especially if you have link speeds > 1G.
> 
> The reverse switch happened in commit c16a5033f77b ("net: renesas: rswitch: Convert to phy_device").
> I did not check the tech details of that, but decided not to touch it.

Might be worth taking another look, especially if anybody wants to use
SFPs.

	Andrew

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Andrew Lunn]

> +	err = of_get_phy_mode(rdev->np_port, &etha->phy_interface);
>  	if (err)
>  		return err;
>  
> -	err = of_property_read_u32(rdev->np_port, "max-speed", &max_speed);
> -	if (!err) {
> -		rdev->etha->speed = max_speed;
> -		return 0;
> -	}
> -
> -	/* if no "max-speed" property, let's use default speed */
> -	switch (rdev->etha->phy_interface) {
> -	case PHY_INTERFACE_MODE_MII:
> -		rdev->etha->speed = SPEED_100;
> -		break;
> +	switch (etha->phy_interface) {
>  	case PHY_INTERFACE_MODE_SGMII:
> -		rdev->etha->speed = SPEED_1000;
> +		etha->max_speed = SPEED_1000;
>  		break;
>  	case PHY_INTERFACE_MODE_USXGMII:
> -		rdev->etha->speed = SPEED_2500;
> +	case PHY_INTERFACE_MODE_5GBASER:
> +		etha->max_speed = SPEED_2500;

If the interface mode is 5GBASER why set the speed to SPEED_2500?
Also, USXGMII allows up to 10G. So this all looks a bit odd.

	Andrew

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Nikita Yushchenko]

> If the interface mode is 5GBASER why set the speed to SPEED_2500?
> Also, USXGMII allows up to 10G. So this all looks a bit odd.

2500 is hardware limit (or at least the datasheet states so).

Nikita

Re: [PATCH net-next] net: renesas: rswitch: cleanup max_speed setting

[Andrew Lunn]

On Wed, Feb 05, 2025 at 09:26:10PM +0100, Nikita Yushchenko wrote:
> > If the interface mode is 5GBASER why set the speed to SPEED_2500?
> > Also, USXGMII allows up to 10G. So this all looks a bit odd.
> 
> 2500 is hardware limit (or at least the datasheet states so).

Then it should return -EINVAL if the device tree has a phy-mode the
hardware does not support.

USXGMII is maybe a bit harder, since i don't know if the higher speeds
are optional, but my understanding is that USXGMII allows up to
10G. Is it really using USXGMII, or 2500BaseX?

	Andrew