diff mbox series

[net-next,v4,6/9] net: sparx5: verify RGMII speeds

Message ID 20241213-sparx5-lan969x-switch-driver-4-v4-6-d1a72c9c4714@microchip.com (mailing list archive)
State New, archived
Headers show
Series net: lan969x: add RGMII support | expand

Commit Message

Daniel Machon Dec. 13, 2024, 1:41 p.m. UTC 
When doing a port config, we verify the port speed against the PHY mode
and supported speeds of that PHY mode. Add checks for the four RGMII phy
modes: RGMII, RGMII_ID, RGMII_TXID and RGMII_RXID.

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_port.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Russell King (Oracle) Dec. 13, 2024, 7:48 p.m. UTC | #1 
On Fri, Dec 13, 2024 at 02:41:05PM +0100, Daniel Machon wrote:
> When doing a port config, we verify the port speed against the PHY mode
> and supported speeds of that PHY mode. Add checks for the four RGMII phy
> modes: RGMII, RGMII_ID, RGMII_TXID and RGMII_RXID.
> 
> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>

You do realise that phylink knows what speeds each interface supports
(see phylink_get_capabilities()) and restricts the media advertisement
to ensure that ethtool link modes that can't be supported by the MAC
capabilities and set of interfaces that would be used are not
advertised.

This should mean none of your verification ever triggers. If it does,
then I'd like to know about it.
Daniel Machon Dec. 16, 2024, 12:10 p.m. UTC | #2 
> On Fri, Dec 13, 2024 at 02:41:05PM +0100, Daniel Machon wrote:
> > When doing a port config, we verify the port speed against the PHY mode
> > and supported speeds of that PHY mode. Add checks for the four RGMII phy
> > modes: RGMII, RGMII_ID, RGMII_TXID and RGMII_RXID.
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> 
> You do realise that phylink knows what speeds each interface supports
> (see phylink_get_capabilities()) and restricts the media advertisement
> to ensure that ethtool link modes that can't be supported by the MAC
> capabilities and set of interfaces that would be used are not
> advertised.
> 
> This should mean none of your verification ever triggers. If it does,
> then I'd like to know about it.

Yes, I agree. Having an extra look at phylink, these checks should not trigger
at all.

As it is, the default switch case is to throw an error, so without this
addition, the sparx5_port_verify_speed() function will fail when the PHY mode
is any of RGMII{_ID,_TXID,_RXID}. This patch just follows the established
pattern of adding the new PHY mode and checking the speed. TBH, I think that
all the checks in this function can be removed entirely, but that is something
I would like to verify and follow up on in a separate series, if that is OK? 

Thanks.

/Daniel
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
index 0a1374422ccb..86d6c9e9ec7c 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_port.c
@@ -257,6 +257,15 @@  static int sparx5_port_verify_speed(struct sparx5 *sparx5,
 		     conf->speed != SPEED_25000))
 			return sparx5_port_error(port, conf, SPX5_PERR_SPEED);
 		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		if (conf->speed != SPEED_1000 &&
+		    conf->speed != SPEED_100 &&
+		    conf->speed != SPEED_10)
+			return sparx5_port_error(port, conf, SPX5_PERR_SPEED);
+		break;
 	default:
 		return sparx5_port_error(port, conf, SPX5_PERR_IFTYPE);
 	}