diff mbox series

[net-next,v2,7/7] net: dsa: mv88e6xxx: enable support for 88E6361 switch

Message ID 20230519141303.245235-8-alexis.lothore@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: mv88e6xxx: add 88E6361 support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
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: 8 this patch: 8
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff fail author Signed-off-by missing
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: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 82 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alexis Lothoré (eBPF Foundation) May 19, 2023, 2:13 p.m. UTC 
From: Alexis Lothoré <alexis.lothore@bootlin.com>

Marvell 88E6361 is an 8-port switch derived from the
88E6393X/88E9193X/88E6191X switches family. It can benefit from the
existing mv88e6xxx driver by simply adding the proper switch description in
the driver. Main differences with other switches from this
family are:
- 8 ports exposed (instead of 11): ports 1, 2 and 8 not available
- No 5GBase-x nor SFI/USXGMII support

---
Changes since v1:
- define internal phys offset
- enforce 88e6361 features in mv88e6393x_phylink_get_caps
- enforce 88e6361 features in mv88e6393x_port_set_speed_duplex
- enforce 88e6361 features in mv88e6393x_port_max_speed_mode

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 42 ++++++++++++++++++++++++++++----
 drivers/net/dsa/mv88e6xxx/chip.h |  3 ++-
 drivers/net/dsa/mv88e6xxx/port.c | 11 ++++++++-
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 4 files changed, 50 insertions(+), 7 deletions(-)

Comments

Russell King (Oracle) May 19, 2023, 2:43 p.m. UTC | #1 
On Fri, May 19, 2023 at 04:13:03PM +0200, alexis.lothore@bootlin.com wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Marvell 88E6361 is an 8-port switch derived from the
> 88E6393X/88E9193X/88E6191X switches family. It can benefit from the
> existing mv88e6xxx driver by simply adding the proper switch description in
> the driver. Main differences with other switches from this
> family are:
> - 8 ports exposed (instead of 11): ports 1, 2 and 8 not available
> - No 5GBase-x nor SFI/USXGMII support
> 
> ---
> Changes since v1:
> - define internal phys offset
> - enforce 88e6361 features in mv88e6393x_phylink_get_caps
> - enforce 88e6361 features in mv88e6393x_port_set_speed_duplex
> - enforce 88e6361 features in mv88e6393x_port_max_speed_mode

Not exactly related to this patch, but please do not rely on this "max
speed mode" - please always ensure that you specify the phy-mode and
fixed-link settings for CPU and DSA ports in firmware. Thanks.
Andrew Lunn May 19, 2023, 4:21 p.m. UTC | #2 
> @@ -421,9 +421,14 @@ phy_interface_t mv88e6390x_port_max_speed_mode(struct mv88e6xxx_chip *chip,
>  int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
>  				     int speed, int duplex)
>  {
> +	bool is_6361 =
> +		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6361;
>  	u16 reg, ctrl;
>  	int err;
>  
> +	if (is_6361 && speed > 2500)
> +		return -EOPNOTSUPP;

I would move the comparison inside the if, so removing the ugly looking split is_6361 line.

> +
>  	if (speed == 200 && port != 0)
>  		return -EOPNOTSUPP;
>  
> @@ -506,8 +511,12 @@ int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
>  phy_interface_t mv88e6393x_port_max_speed_mode(struct mv88e6xxx_chip *chip,
>  					       int port)
>  {
> +	bool is_6361 =
> +		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6361;
> +
>  	if (port == 0 || port == 9 || port == 10)
> -		return PHY_INTERFACE_MODE_10GBASER;
> +		return is_6361 ? PHY_INTERFACE_MODE_2500BASEX :
> +			PHY_INTERFACE_MODE_10GBASER;

Please see if you can rearrange this code as well.

Thanks
	Andrew
Alexis Lothoré (eBPF Foundation) May 22, 2023, 8:15 a.m. UTC | #3 
Hi Russell, thanks for review

On 5/19/23 16:43, Russell King (Oracle) wrote:
> On Fri, May 19, 2023 at 04:13:03PM +0200, alexis.lothore@bootlin.com wrote:
>> From: Alexis Lothoré <alexis.lothore@bootlin.com>
>>
>> Marvell 88E6361 is an 8-port switch derived from the
>> 88E6393X/88E9193X/88E6191X switches family. It can benefit from the
>> existing mv88e6xxx driver by simply adding the proper switch description in
>> the driver. Main differences with other switches from this
>> family are:
>> - 8 ports exposed (instead of 11): ports 1, 2 and 8 not available
>> - No 5GBase-x nor SFI/USXGMII support
>>
>> ---
>> Changes since v1:
>> - define internal phys offset
>> - enforce 88e6361 features in mv88e6393x_phylink_get_caps
>> - enforce 88e6361 features in mv88e6393x_port_set_speed_duplex
>> - enforce 88e6361 features in mv88e6393x_port_max_speed_mode
> 
> Not exactly related to this patch, but please do not rely on this "max
> speed mode" - please always ensure that you specify the phy-mode and
> fixed-link settings for CPU and DSA ports in firmware. Thanks.

I would like to make sure to fully understand your point:
- when telling so specify phy-mode and fixed-link in firmware, you mean
device-tree, right ?
- when checking for code and execution flow, I observe that port_max_speed is
always called and its output is always used to configure shared ports mode in
mv88e6xxx driver. Are you telling that eventually, the whole mv88e6xxx driver
should stop relying on port_max_speed_mode for shared ports ?

Kind regards,
Andrew Lunn May 22, 2023, 12:19 p.m. UTC | #4 
> > Not exactly related to this patch, but please do not rely on this "max
> > speed mode" - please always ensure that you specify the phy-mode and
> > fixed-link settings for CPU and DSA ports in firmware. Thanks.
> 
> I would like to make sure to fully understand your point:
> - when telling so specify phy-mode and fixed-link in firmware, you mean
> device-tree, right ?
> - when checking for code and execution flow, I observe that port_max_speed is
> always called and its output is always used to configure shared ports mode in
> mv88e6xxx driver. Are you telling that eventually, the whole mv88e6xxx driver
> should stop relying on port_max_speed_mode for shared ports ?

Yes, the concept of port_max_speed_mode causes problems for PHYLINK,
and we want to remove it. Russell and i have been updating DT
descriptions adding fixed-link and phy-mode properties to all
mv88e6xxx systems so that it is not needed. Either at the end of this
cycle, or the beginning of the next we will change the code to
actually enforce this.

	 Andrew
Alexis Lothoré (eBPF Foundation) May 22, 2023, 12:26 p.m. UTC | #5 
Hello Andrew,
On 5/22/23 14:19, Andrew Lunn wrote:
>>> Not exactly related to this patch, but please do not rely on this "max
>>> speed mode" - please always ensure that you specify the phy-mode and
>>> fixed-link settings for CPU and DSA ports in firmware. Thanks.
>>
>> I would like to make sure to fully understand your point:
>> - when telling so specify phy-mode and fixed-link in firmware, you mean
>> device-tree, right ?
>> - when checking for code and execution flow, I observe that port_max_speed is
>> always called and its output is always used to configure shared ports mode in
>> mv88e6xxx driver. Are you telling that eventually, the whole mv88e6xxx driver
>> should stop relying on port_max_speed_mode for shared ports ?
> 
> Yes, the concept of port_max_speed_mode causes problems for PHYLINK,
> and we want to remove it. Russell and i have been updating DT
> descriptions adding fixed-link and phy-mode properties to all
> mv88e6xxx systems so that it is not needed. Either at the end of this
> cycle, or the beginning of the next we will change the code to
> actually enforce this.

Understood, thanks for clarification

> 
> 	 Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0e6267193ac1..7c77b4b634f9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -790,6 +790,8 @@  static void mv88e6393x_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
 	unsigned long *supported = config->supported_interfaces;
 	bool is_6191x =
 		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6191X;
+	bool is_6361 =
+		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6361;
 
 	mv88e6xxx_translate_cmode(chip->ports[port].cmode, supported);
 
@@ -804,13 +806,17 @@  static void mv88e6393x_phylink_get_caps(struct mv88e6xxx_chip *chip, int port,
 		/* 6191X supports >1G modes only on port 10 */
 		if (!is_6191x || port == 10) {
 			__set_bit(PHY_INTERFACE_MODE_2500BASEX, supported);
-			__set_bit(PHY_INTERFACE_MODE_5GBASER, supported);
-			__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
+			config->mac_capabilities |= MAC_2500FD;
+
+			/* 6361 only supports up to 2500BaseX */
+			if (!is_6361) {
+				__set_bit(PHY_INTERFACE_MODE_5GBASER, supported);
+				__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
+				config->mac_capabilities |= MAC_5000FD |
+					MAC_10000FD;
+			}
 			/* FIXME: USXGMII is not supported yet */
 			/* __set_bit(PHY_INTERFACE_MODE_USXGMII, supported); */
-
-			config->mac_capabilities |= MAC_2500FD | MAC_5000FD |
-				MAC_10000FD;
 		}
 	}
 
@@ -6311,6 +6317,32 @@  static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.ptp_support = true,
 		.ops = &mv88e6352_ops,
 	},
+	[MV88E6361] = {
+		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6361,
+		.family = MV88E6XXX_FAMILY_6393,
+		.name = "Marvell 88E6361",
+		.num_databases = 4096,
+		.num_macs = 16384,
+		.num_ports = 11,
+		/* Ports 1, 2 and 8 are not routed */
+		.invalid_port_mask = BIT(1) | BIT(2) | BIT(8),
+		.num_internal_phys = 5,
+		.internal_phys_offset = 3,
+		.max_vid = 4095,
+		.max_sid = 63,
+		.port_base_addr = 0x0,
+		.phy_base_addr = 0x0,
+		.global1_addr = 0x1b,
+		.global2_addr = 0x1c,
+		.age_time_coeff = 3750,
+		.g1_irqs = 10,
+		.g2_irqs = 14,
+		.atu_move_port_mask = 0x1f,
+		.pvt = true,
+		.multi_chip = true,
+		.ptp_support = true,
+		.ops = &mv88e6393x_ops,
+	},
 	[MV88E6390] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6390,
 		.family = MV88E6XXX_FAMILY_6390,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index dd7c8880e987..79c06ba42c54 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -82,6 +82,7 @@  enum mv88e6xxx_model {
 	MV88E6350,
 	MV88E6351,
 	MV88E6352,
+	MV88E6361,
 	MV88E6390,
 	MV88E6390X,
 	MV88E6393X,
@@ -100,7 +101,7 @@  enum mv88e6xxx_family {
 	MV88E6XXX_FAMILY_6351,	/* 6171 6175 6350 6351 */
 	MV88E6XXX_FAMILY_6352,	/* 6172 6176 6240 6352 */
 	MV88E6XXX_FAMILY_6390,  /* 6190 6190X 6191 6290 6390 6390X */
-	MV88E6XXX_FAMILY_6393,	/* 6191X 6193X 6393X */
+	MV88E6XXX_FAMILY_6393,	/* 6191X 6193X 6361 6393X */
 };
 
 /**
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 66f1b40b4e96..e72ea3c8092f 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -421,9 +421,14 @@  phy_interface_t mv88e6390x_port_max_speed_mode(struct mv88e6xxx_chip *chip,
 int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
 				     int speed, int duplex)
 {
+	bool is_6361 =
+		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6361;
 	u16 reg, ctrl;
 	int err;
 
+	if (is_6361 && speed > 2500)
+		return -EOPNOTSUPP;
+
 	if (speed == 200 && port != 0)
 		return -EOPNOTSUPP;
 
@@ -506,8 +511,12 @@  int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
 phy_interface_t mv88e6393x_port_max_speed_mode(struct mv88e6xxx_chip *chip,
 					       int port)
 {
+	bool is_6361 =
+		chip->info->prod_num == MV88E6XXX_PORT_SWITCH_ID_PROD_6361;
+
 	if (port == 0 || port == 9 || port == 10)
-		return PHY_INTERFACE_MODE_10GBASER;
+		return is_6361 ? PHY_INTERFACE_MODE_2500BASEX :
+			PHY_INTERFACE_MODE_10GBASER;
 
 	return PHY_INTERFACE_MODE_NA;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 3c9fc17abdd2..56dfa9d3d4e0 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -138,6 +138,7 @@ 
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6141	0x3400
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6341	0x3410
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6352	0x3520
+#define MV88E6XXX_PORT_SWITCH_ID_PROD_6361	0x2610
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6350	0x3710
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6351	0x3750
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6390	0x3900