From: Michael Krummsdorf <[email protected]>
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 <[email protected]>
Signed-off-by: Matthias Schiffer <[email protected]>
---
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),
.max_vid = 4095,
.port_base_addr = 0x8,
.phy_base_addr = 0x0,
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Hi Matthias,
> From: Michael Krummsdorf <[email protected]>
>
> 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
> <[email protected]> Signed-off-by: Matthias Schiffer
> <[email protected]> ---
>
> 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: [email protected]
On Tue, 2024-03-26 at 14:34 +0100, Lukasz Majewski wrote:
> Hi Matthias,
>
> > From: Michael Krummsdorf <[email protected]>
> >
> > 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
> > <[email protected]> Signed-off-by: Matthias Schiffer
> > <[email protected]> ---
> >
> > 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: [email protected]
> 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 <[email protected]>
> > >
> > > 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
> > > <[email protected]> Signed-off-by: Matthias Schiffer
> > > <[email protected]> ---
> > >
> > > 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:
> > [email protected] 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: [email protected]
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 <[email protected]>
> > > >
> > > > 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
> > > > <[email protected]> Signed-off-by: Matthias Schiffer
> > > > <[email protected]> ---
> > > >
> > > > 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:
> > > [email protected] 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: [email protected]
> 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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Matthias Schiffer <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Matthias Schiffer <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
On Tue, 26 Mar 2024 13:36:54 +0100 you wrote:
> From: Michael Krummsdorf <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Matthias Schiffer <[email protected]>
>
> [...]
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html