[PATCH net-next 0/2] net: dsa: mv88e6xxx: add 88E6361 support

[PATCH net-next 0/2] net: dsa: mv88e6xxx: add 88E6361 support

[alexis.lothore]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

This series brings initial support for Marvell 88E6361 switch.

MV88E6361 is a 8 ports switch with 5 integrated Gigabit PHYs and 3
2.5Gigabit SerDes interfaces. It is in fact a new variant in the
88E639X/88E6193X/88E6191X family with a subset of existing features (e.g.
reduced ports count, lower maximal speed for MII interfaces).
Since said family is already well supported in mv88e6xxx driver, adding
initial support for this new switch mostly consists in finding the ID
exposed in its identification register, and then add a proper description
in switch description tables in mv88e6xxx driver.

This initial support has been tested with two samples of a custom board
with the following hardware configuration:
- a main CPU connected to MV88E6361 using port 0 as CPU port
- port 9 wired to a SFP cage
- port 10 wired to a G.Hn transceiver

The following setup was used:
PC <-ethernet-> (copper SFP) - Board 1 - (G.hn) <-phone line(RJ11)-> (G.hn) Board 2

The unit 1 has been configured to bridge SFP port and G.hn port together,
which allowed to successfully ping Board 2 from PC.

Alexis Lothoré (2):
  dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility
    list
  net: dsa: mv88e6xxx: enable support for 88E6361 switch

 .../devicetree/bindings/net/dsa/marvell.txt   |  2 +-
 drivers/net/dsa/mv88e6xxx/chip.c              | 25 +++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h              |  3 ++-
 drivers/net/dsa/mv88e6xxx/port.h              |  1 +
 4 files changed, 29 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[alexis.lothore]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

Marvell MV88E6361 is an 8-port switch derived from the
88E6393X/88E9193X/88E6191X switches family. Since its functional behavior
is very close to switches from this family, it can benefit from existing
drivers for this family, so add it to the list of compatible switches

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 Documentation/devicetree/bindings/net/dsa/marvell.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt
index 2363b412410c..33726134f5c9 100644
--- a/Documentation/devicetree/bindings/net/dsa/marvell.txt
+++ b/Documentation/devicetree/bindings/net/dsa/marvell.txt
@@ -20,7 +20,7 @@ which is at a different MDIO base address in different switch families.
 			  6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321,
 			  6341, 6350, 6351, 6352
 - "marvell,mv88e6190"	: Switch has base address 0x00. Use with models:
-			  6190, 6190X, 6191, 6290, 6390, 6390X
+			  6163, 6190, 6190X, 6191, 6290, 6390, 6390X
 - "marvell,mv88e6250"	: Switch has base address 0x08 or 0x18. Use with model:
 			  6220, 6250
 
-- 
2.40.1

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[Andrew Lunn]

On Wed, May 17, 2023 at 10:34:29PM +0200, alexis.lothore@bootlin.com wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Marvell MV88E6361 is an 8-port switch derived from the
> 88E6393X/88E9193X/88E6191X switches family. Since its functional behavior
> is very close to switches from this family, it can benefit from existing
> drivers for this family, so add it to the list of compatible switches
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 558 bytes --]

On Wed, May 17, 2023 at 10:34:29PM +0200, alexis.lothore@bootlin.com wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Marvell MV88E6361 is an 8-port switch derived from the
> 88E6393X/88E9193X/88E6191X switches family. Since its functional behavior
> is very close to switches from this family, it can benefit from existing
> drivers for this family, so add it to the list of compatible switches
> 
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[airat.gl]

Is there an error? The new string include 6163 instead of 6361

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[Andrew Lunn]

On Thu, Aug 24, 2023 at 08:49:17PM +0200, airat.gl@gmail.com wrote:
> Is there an error? The new string include 6163 instead of 6361

You need to provide some context here. Maybe the git hash of the
patch? Or a chunk of code.

	Andrew

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: marvell: add MV88E6361 switch to compatibility list

[Alexis Lothoré]

Hello,

On 8/24/23 20:49, airat.gl@gmail.com wrote:
> Is there an error? The new string include 6163 instead of 6361

I am afraid you are right, I made a dumb typo in the binding docs while
submitting 88E6361 support.
I have sent the corresponding fix to net, thanks for spotting this.

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[alexis.lothore]

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

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 25 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  3 ++-
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64a2f2f83735..0be7135fa39d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6309,6 +6309,31 @@ 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,
+		.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 da6e1339f809..c88e52e355a5 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.h b/drivers/net/dsa/mv88e6xxx/port.h
index aec9d4fd20e3..22e2147c29a7 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
-- 
2.40.1

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Andrew Lunn]

On Wed, May 17, 2023 at 10:34:30PM +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

So what exactly is supported for link modes?

The way you reuse the 6393 ops, are these differences actually
enforced? It looks like mv88e6393x_phylink_get_caps() will allow
2500BaseX, 5GBaseX and 10GBaseR for port 10.

> +	[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,

Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
mv88e6xxx_phy_is_internal() return for these ports, and
mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
need to list 8 here?

     Andrew

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Alexis Lothoré]

Hello Andrew, thanks for the prompt review !

On 5/17/23 22:51, Andrew Lunn wrote:
> On Wed, May 17, 2023 at 10:34:30PM +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
> 
> So what exactly is supported for link modes?
> 
> The way you reuse the 6393 ops, are these differences actually
> enforced? It looks like mv88e6393x_phylink_get_caps() will allow
> 2500BaseX, 5GBaseX and 10GBaseR for port 10.

You are right, mv88e6393x_phylink_get_caps is currently too "generous" with
capabilities for 88E6361. With this chip, supported links modes are the following:
- port 0: MII, RMII, RGMII, 1000BaseX, 2500BaseX
- port 3 to 7: triple speed internal phys
- port 9 and 10: 1000BaseX, 25000BaseX
I'll add those specifications in cover letter for next revision for this series.
So indeed reported capabilities are wrong, I will update it. Taking a quick look
at other ops, I guess I'll have to fix some others too like
mv88e6393x_port_max_speed_mode
> 
>> +	[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,
> 
> Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
> mv88e6xxx_phy_is_internal() return for these ports, and
> mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
> need to list 8 here?

Indeed there is something wrong here too. I need to tune
mv88e6393x_phylink_get_caps to reflect 88E6361 differences.

As stated above, port 3 to 7 are the ones with internal PHY.
For mv88e6xxx_phy_is_internal, I see that it is merely comparing the port index
to the number of internal phys, so in this case it would advertise (wrongly)
that ports 0 to 4 have internal phys. I also see that your suggestion (setting
num_interal_phys to max internal phy index + 1) is already in use for this
family (6393X has 8 internal phys but defines num_internal_phys to 9), so if
it's acceptable I will do as you suggest and set it to 8.

Thanks,
Alexis

> 
>      Andrew

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Andrew Lunn]

> >> +	[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,
> > 
> > Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
> > mv88e6xxx_phy_is_internal() return for these ports, and
> > mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
> > need to list 8 here?
> 
> Indeed there is something wrong here too. I need to tune
> mv88e6393x_phylink_get_caps to reflect 88E6361 differences.
> 
> As stated above, port 3 to 7 are the ones with internal PHY.
> For mv88e6xxx_phy_is_internal, I see that it is merely comparing the port index
> to the number of internal phys, so in this case it would advertise (wrongly)
> that ports 0 to 4 have internal phys.

Ports 1 and 2 should hopefully be protected by the
invalid_port_mask. It should not even be possible to create those
ports. port 0 is interesting, and possibly currently broken on
6393. Please take a look at that.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Marek Behún]

On Thu, 18 May 2023 14:58:00 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > >> +	[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,  
> > > 
> > > Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
> > > mv88e6xxx_phy_is_internal() return for these ports, and
> > > mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
> > > need to list 8 here?  
> > 
> > Indeed there is something wrong here too. I need to tune
> > mv88e6393x_phylink_get_caps to reflect 88E6361 differences.
> > 
> > As stated above, port 3 to 7 are the ones with internal PHY.
> > For mv88e6xxx_phy_is_internal, I see that it is merely comparing the port index
> > to the number of internal phys, so in this case it would advertise (wrongly)
> > that ports 0 to 4 have internal phys.  
> 
> Ports 1 and 2 should hopefully be protected by the
> invalid_port_mask. It should not even be possible to create those
> ports. port 0 is interesting, and possibly currently broken on
> 6393. Please take a look at that.

Why would port 0 be broken on 6393x ?

Marek

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Alexis Lothoré]

On 5/19/23 14:38, Marek Behún wrote:
> On Thu, 18 May 2023 14:58:00 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>>>> +	[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,  
>>>>
>>>> Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
>>>> mv88e6xxx_phy_is_internal() return for these ports, and
>>>> mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
>>>> need to list 8 here?  
>>>
>>> Indeed there is something wrong here too. I need to tune
>>> mv88e6393x_phylink_get_caps to reflect 88E6361 differences.
>>>
>>> As stated above, port 3 to 7 are the ones with internal PHY.
>>> For mv88e6xxx_phy_is_internal, I see that it is merely comparing the port index
>>> to the number of internal phys, so in this case it would advertise (wrongly)
>>> that ports 0 to 4 have internal phys.  
>>
>> Ports 1 and 2 should hopefully be protected by the
>> invalid_port_mask. It should not even be possible to create those
>> ports. port 0 is interesting, and possibly currently broken on
>> 6393. Please take a look at that.
> 
> Why would port 0 be broken on 6393x ?
By "broken", I guess Andrew means that if we feed port 0 to
mv88e6xxx_phy_is_internal, it will return true, which is wrong since there is no
internal phy for port 0 on 6393X ?
> 
> Marek

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Andrew Lunn]

> >> Ports 1 and 2 should hopefully be protected by the
> >> invalid_port_mask. It should not even be possible to create those
> >> ports. port 0 is interesting, and possibly currently broken on
> >> 6393. Please take a look at that.
> > 
> > Why would port 0 be broken on 6393x ?
> By "broken", I guess Andrew means that if we feed port 0 to
> mv88e6xxx_phy_is_internal, it will return true, which is wrong since there is no
> internal phy for port 0 on 6393X ?

Yes, that is what i was thinking. But i did not spend the time to look
at the code see if this is actually true. There might be a special
case somewhere in the code.

But in general, we try to avoid special cases, and add device specific
ops.

	Andrew

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: enable support for 88E6361 switch

[Marek Behún]

On Fri, 19 May 2023 15:16:57 +0200
Alexis Lothoré <alexis.lothore@bootlin.com> wrote:

> On 5/19/23 14:38, Marek Behún wrote:
> > On Thu, 18 May 2023 14:58:00 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> >>>>> +	[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,    
> >>>>
> >>>> Which ports have internal PHYs? 2, 3, 4, 5, 6, 7 ?  What does
> >>>> mv88e6xxx_phy_is_internal() return for these ports, and
> >>>> mv88e6xxx_get_capsmv88e6xxx_get_caps()? I'm wondering if you actually
> >>>> need to list 8 here?    
> >>>
> >>> Indeed there is something wrong here too. I need to tune
> >>> mv88e6393x_phylink_get_caps to reflect 88E6361 differences.
> >>>
> >>> As stated above, port 3 to 7 are the ones with internal PHY.
> >>> For mv88e6xxx_phy_is_internal, I see that it is merely comparing the port index
> >>> to the number of internal phys, so in this case it would advertise (wrongly)
> >>> that ports 0 to 4 have internal phys.    
> >>
> >> Ports 1 and 2 should hopefully be protected by the
> >> invalid_port_mask. It should not even be possible to create those
> >> ports. port 0 is interesting, and possibly currently broken on
> >> 6393. Please take a look at that.  
> > 
> > Why would port 0 be broken on 6393x ?  
> By "broken", I guess Andrew means that if we feed port 0 to
> mv88e6xxx_phy_is_internal, it will return true, which is wrong since there is no
> internal phy for port 0 on 6393X ?

OK that's true :)