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 Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: lan969x: add RGMII support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-12-14--00-01 (tests: 794)

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);
 	}