| Message ID | 20240326123655.40666-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 625aefac340f45a4fc60908da763f437599a0d6f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: fix usable ports on 88e6020 | expand |
Hi Matthias, > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > The switch has 4 ports with 2 internal PHYs, but ports are numbered up > to 6, with ports 0, 1, 5 and 6 being usable. > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for MV88E6020 > switch") Signed-off-by: Michael Krummsdorf > <michael.krummsdorf@tq-group.com> Signed-off-by: Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> --- > > I was unfortunately too busy to notice the issue when the patch this > Fixes was resubmitted in my name. It would have been better to change > my From into a Based-on-patch-by or similar when modifying it The "discussion" about this work was lasting at least a few months with several iterations and changing the design decisions ... > - and > the final version obviously wasn't even tested on an 88E6020... Can you share on which kernel version have you tested the patch that you claim that testing was omitted? > > Best regards, > Matthias > > > drivers/net/dsa/mv88e6xxx/chip.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > b/drivers/net/dsa/mv88e6xxx/chip.c index 9ed1821184ece..c95787cb90867 > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -5503,8 +5503,12 @@ static const struct mv88e6xxx_info > mv88e6xxx_table[] = { .family = MV88E6XXX_FAMILY_6250, > .name = "Marvell 88E6020", > .num_databases = 64, > - .num_ports = 4, > + /* Ports 2-4 are not routed to pins > + * => usable ports 0, 1, 5, 6 > + */ > + .num_ports = 7, > .num_internal_phys = 2, > + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), > .max_vid = 4095, > .port_base_addr = 0x8, > .phy_base_addr = 0x0, Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, 2024-03-26 at 14:34 +0100, Lukasz Majewski wrote: > Hi Matthias, > > > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > > > The switch has 4 ports with 2 internal PHYs, but ports are numbered up > > to 6, with ports 0, 1, 5 and 6 being usable. > > > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for MV88E6020 > > switch") Signed-off-by: Michael Krummsdorf > > <michael.krummsdorf@tq-group.com> Signed-off-by: Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> --- > > > > I was unfortunately too busy to notice the issue when the patch this > > Fixes was resubmitted in my name. It would have been better to change > > my From into a Based-on-patch-by or similar when modifying it > > The "discussion" about this work was lasting at least a few months with > several iterations and changing the design decisions ... > > > - and > > the final version obviously wasn't even tested on an 88E6020... > > > Can you share on which kernel version have you tested the patch that > you claim that testing was omitted? Hi Lukasz, we are currently testing with commit 71d94a432a15 backported onto a recent Linux 6.1.y. At least on this kernel version, the driver will reject a Device Tree configuration that uses the ports 5 and 6 (rightfully so, as num_ports is set to 4), leaving only the internal-PHY ports 0 and 1, and none of the *MII ports that are likely to be used as CPU ports. So if the accepted version worked fine for you, your configuration possibly only used the first two ports, or newer kernels somehow ignore num_ports when determining if a port number is valid. We should be able to repeat our test on a newer kernel next week if needed. Best regards, Matthias > > > > > > > drivers/net/dsa/mv88e6xxx/chip.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > b/drivers/net/dsa/mv88e6xxx/chip.c index 9ed1821184ece..c95787cb90867 > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > @@ -5503,8 +5503,12 @@ static const struct mv88e6xxx_info > > mv88e6xxx_table[] = { .family = MV88E6XXX_FAMILY_6250, > > .name = "Marvell 88E6020", > > .num_databases = 64, > > - .num_ports = 4, > > + /* Ports 2-4 are not routed to pins > > + * => usable ports 0, 1, 5, 6 > > + */ > > + .num_ports = 7, > > .num_internal_phys = 2, > > + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), > > .max_vid = 4095, > > .port_base_addr = 0x8, > > .phy_base_addr = 0x0, > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de > Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter. > Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it. > >
Hi Matthias, > On Tue, 2024-03-26 at 14:34 +0100, Lukasz Majewski wrote: > > Hi Matthias, > > > > > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > > > > > The switch has 4 ports with 2 internal PHYs, but ports are > > > numbered up to 6, with ports 0, 1, 5 and 6 being usable. > > > > > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for > > > MV88E6020 switch") Signed-off-by: Michael Krummsdorf > > > <michael.krummsdorf@tq-group.com> Signed-off-by: Matthias Schiffer > > > <matthias.schiffer@ew.tq-group.com> --- > > > > > > I was unfortunately too busy to notice the issue when the patch > > > this Fixes was resubmitted in my name. It would have been better > > > to change my From into a Based-on-patch-by or similar when > > > modifying it > > > > The "discussion" about this work was lasting at least a few months > > with several iterations and changing the design decisions ... > > > > > - and > > > the final version obviously wasn't even tested on an 88E6020... > > > > > > Can you share on which kernel version have you tested the patch that > > you claim that testing was omitted? > > Hi Lukasz, > > we are currently testing with commit 71d94a432a15 backported onto a > recent Linux 6.1.y. At least on this kernel version, the driver will > reject a Device Tree configuration that uses the ports 5 and 6 > (rightfully so, as num_ports is set to 4), leaving only the > internal-PHY ports 0 and 1, and none of the *MII ports that are > likely to be used as CPU ports. > > So if the accepted version worked fine for you, your configuration > possibly only used the first two ports, And then the penny has dropped.... switch@10 { compatible = "marvell,mv88e6250"; reg = <0x10>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan1"; }; port@1 { reg = <1>; label = "lan2"; }; port@6 { reg = <6>; label = "cpu"; phy-mode = "rmii"; ethernet = <&fec2>; fixed-link { speed = <100>; full-duplex; }; }; }; }; > or newer kernels somehow > ignore num_ports when determining if a port number is valid. > > We should be able to repeat our test on a newer kernel next week if > needed. I think that considering the above snippet - customer was using only port 0 and 1. > > Best regards, > Matthias > > > > > > > > > > > > > drivers/net/dsa/mv88e6xxx/chip.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > b/drivers/net/dsa/mv88e6xxx/chip.c index > > > 9ed1821184ece..c95787cb90867 100644 --- > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++ > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -5503,8 +5503,12 @@ static > > > const struct mv88e6xxx_info mv88e6xxx_table[] = { .family = > > > MV88E6XXX_FAMILY_6250, .name = "Marvell 88E6020", > > > .num_databases = 64, > > > - .num_ports = 4, > > > + /* Ports 2-4 are not routed to pins > > > + * => usable ports 0, 1, 5, 6 > > > + */ > > > + .num_ports = 7, > > > .num_internal_phys = 2, > > > + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), > > > .max_vid = 4095, > > > .port_base_addr = 0x8, > > > .phy_base_addr = 0x0, > > > > > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Erika Unter > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de Achtung externe E-Mail: Öffnen Sie Anhänge und Links > > nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen > > und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung > > an den IT-Helpdesk weiter. Attention external email: Open > > attachments and links only if you know that they are from a secure > > source and are safe. In doubt forward the email to the IT-Helpdesk > > to check it. > > > > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, 2024-03-26 at 16:52 +0100, Lukasz Majewski wrote: > Hi Matthias, > > > On Tue, 2024-03-26 at 14:34 +0100, Lukasz Majewski wrote: > > > Hi Matthias, > > > > > > > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > > > > > > > The switch has 4 ports with 2 internal PHYs, but ports are > > > > numbered up to 6, with ports 0, 1, 5 and 6 being usable. > > > > > > > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for > > > > MV88E6020 switch") Signed-off-by: Michael Krummsdorf > > > > <michael.krummsdorf@tq-group.com> Signed-off-by: Matthias Schiffer > > > > <matthias.schiffer@ew.tq-group.com> --- > > > > > > > > I was unfortunately too busy to notice the issue when the patch > > > > this Fixes was resubmitted in my name. It would have been better > > > > to change my From into a Based-on-patch-by or similar when > > > > modifying it > > > > > > The "discussion" about this work was lasting at least a few months > > > with several iterations and changing the design decisions ... > > > > > > > - and > > > > the final version obviously wasn't even tested on an 88E6020... > > > > > > > > > Can you share on which kernel version have you tested the patch that > > > you claim that testing was omitted? > > > > Hi Lukasz, > > > > we are currently testing with commit 71d94a432a15 backported onto a > > recent Linux 6.1.y. At least on this kernel version, the driver will > > reject a Device Tree configuration that uses the ports 5 and 6 > > (rightfully so, as num_ports is set to 4), leaving only the > > internal-PHY ports 0 and 1, and none of the *MII ports that are > > likely to be used as CPU ports. > > > > So if the accepted version worked fine for you, your configuration > > possibly only used the first two ports, > > And then the penny has dropped.... > > > switch@10 { > compatible = "marvell,mv88e6250"; > reg = <0x10>; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@0 { > reg = <0>; > label = "lan1"; > }; > > port@1 { > reg = <1>; > label = "lan2"; > }; > > port@6 { > reg = <6>; > label = "cpu"; > phy-mode = "rmii"; > ethernet = <&fec2>; > > fixed-link { > speed = <100>; > full-duplex; > }; > }; > }; > }; > > > > > or newer kernels somehow > > ignore num_ports when determining if a port number is valid. > > > > We should be able to repeat our test on a newer kernel next week if > > needed. > > I think that considering the above snippet - customer was using only > port 0 and 1. Hmm, I don't think that's right - the DTS has port 6 as CPU port, which should make the whole probe of the switch fail with num_ports == 4 (`reg >= ds->num_ports` check in dsa_switch_parse_ports_of()) - that's what we saw on kernel 6.1.y with our board for CPU on port 5. Best regards, Matthias > > > > > > > > > > > > > > > > > drivers/net/dsa/mv88e6xxx/chip.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > > b/drivers/net/dsa/mv88e6xxx/chip.c index > > > > 9ed1821184ece..c95787cb90867 100644 --- > > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++ > > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -5503,8 +5503,12 @@ static > > > > const struct mv88e6xxx_info mv88e6xxx_table[] = { .family = > > > > MV88E6XXX_FAMILY_6250, .name = "Marvell 88E6020", > > > > .num_databases = 64, > > > > - .num_ports = 4, > > > > + /* Ports 2-4 are not routed to pins > > > > + * => usable ports 0, 1, 5, 6 > > > > + */ > > > > + .num_ports = 7, > > > > .num_internal_phys = 2, > > > > + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), > > > > .max_vid = 4095, > > > > .port_base_addr = 0x8, > > > > .phy_base_addr = 0x0, > > > > > > > > > > > > > > > Best regards, > > > > > > Lukasz Majewski > > > > > > -- > > > > > > DENX Software Engineering GmbH, Managing Director: Erika Unter > > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > > lukma@denx.de Achtung externe E-Mail: Öffnen Sie Anhänge und Links > > > nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen > > > und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung > > > an den IT-Helpdesk weiter. Attention external email: Open > > > attachments and links only if you know that they are from a secure > > > source and are safe. In doubt forward the email to the IT-Helpdesk > > > to check it. > > > > > > > > > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de > Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter. > Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it. > >
On Tue, Mar 26, 2024 at 01:36:54PM +0100, Matthias Schiffer wrote: > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > The switch has 4 ports with 2 internal PHYs, but ports are numbered up > to 6, with ports 0, 1, 5 and 6 being usable. > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for MV88E6020 switch") > Signed-off-by: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > > I was unfortunately too busy to notice the issue when the patch this > Fixes was resubmitted in my name. It would have been better to change > my From into a Based-on-patch-by or similar when modifying it - and the > final version obviously wasn't even tested on an 88E6020... > > Best regards, > Matthias > > > drivers/net/dsa/mv88e6xxx/chip.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 9ed1821184ece..c95787cb90867 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -5503,8 +5503,12 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { > .family = MV88E6XXX_FAMILY_6250, > .name = "Marvell 88E6020", > .num_databases = 64, > - .num_ports = 4, > + /* Ports 2-4 are not routed to pins > + * => usable ports 0, 1, 5, 6 > + */ > + .num_ports = 7, > .num_internal_phys = 2, > + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), This patch does what the commit message suggests. So Reviewed-by: Andrew Lunn <andrew@lunn.ch> It would however be nice to fully understand how it currently works for the deployed use case, just to ensure we are not missing something. Andrew
On Tue, Mar 26, 2024 at 01:36:54PM +0100, Matthias Schiffer wrote: > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > The switch has 4 ports with 2 internal PHYs, but ports are numbered up > to 6, with ports 0, 1, 5 and 6 being usable. > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for MV88E6020 switch") > Signed-off-by: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 26 Mar 2024 13:36:54 +0100 you wrote: > From: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > > The switch has 4 ports with 2 internal PHYs, but ports are numbered up > to 6, with ports 0, 1, 5 and 6 being usable. > > Fixes: 71d94a432a15 ("net: dsa: mv88e6xxx: add support for MV88E6020 switch") > Signed-off-by: Michael Krummsdorf <michael.krummsdorf@tq-group.com> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > [...] Here is the summary with links: - net: dsa: mv88e6xxx: fix usable ports on 88e6020 https://git.kernel.org/netdev/net/c/625aefac340f You are awesome, thank you!
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 9ed1821184ece..c95787cb90867 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -5503,8 +5503,12 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .family = MV88E6XXX_FAMILY_6250, .name = "Marvell 88E6020", .num_databases = 64, - .num_ports = 4, + /* Ports 2-4 are not routed to pins + * => usable ports 0, 1, 5, 6 + */ + .num_ports = 7, .num_internal_phys = 2, + .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), .max_vid = 4095, .port_base_addr = 0x8, .phy_base_addr = 0x0,