[PATCH 0/9] net: dsa: Add support for mv88e6020 and mv88e6071

[PATCH 0/9] net: dsa: Add support for mv88e6020 and mv88e6071

[Lukasz Majewski]

This patch series provides support for Marvell's mv88e6020 and mv 88e6071
switches and can be divided to two parts;

1. Code necessary to support aforementioned switches

2. Driver modification to support proper size of frames for mv88e6071

Lukasz Majewski (5):
  net: dsa: mv88e6xxx: Add support for MV88E6071 switch
  net: dsa: marvell: Provide per device information about max frame size
  net: dsa: mv88e6071: Define max frame size (2048 bytes)
  net: dsa: mv88e6071: Provide struct mv88e6xxx_ops
  net: dsa: mv88e6071: Set .set_max_frame_size callback

Matthias Schiffer (4):
  net: dsa: allow switch drivers to override default slave PHY addresses
  net: dsa: mv88e6xxx: account for PHY base address offset in dual chip
    mode
  net: dsa: mv88e6xxx: implement get_phy_address
  net: dsa: mv88e6xxx: add support for MV88E6020 switch

 drivers/net/dsa/mv88e6xxx/chip.c    | 102 +++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/chip.h    |   4 ++
 drivers/net/dsa/mv88e6xxx/global2.c |   2 +-
 drivers/net/dsa/mv88e6xxx/port.h    |   2 +
 drivers/net/dsa/mv88e6xxx/smi.c     |   4 ++
 include/net/dsa.h                   |   1 +
 net/dsa/slave.c                     |   9 ++-
 7 files changed, 119 insertions(+), 5 deletions(-)

-- 
2.37.2

[PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Lukasz Majewski]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Avoid having to define a PHY for every physical port when PHY addresses
are fixed, but port index != PHY address.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Signed-off-by: Lukasz Majewski <lukma@denx.de>
[Adjustments for newest kernel upstreaming]
---
 include/net/dsa.h | 1 +
 net/dsa/slave.c   | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index ee369670e20e..210b0e215ac9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -858,6 +858,7 @@ struct dsa_switch_ops {
 	int	(*port_setup)(struct dsa_switch *ds, int port);
 	void	(*port_teardown)(struct dsa_switch *ds, int port);
 
+	int	(*get_phy_address)(struct dsa_switch *ds, int port);
 	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
 
 	/*
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a9fde48cffd4..8bb1e8770846 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2273,7 +2273,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	struct device_node *port_dn = dp->dn;
 	struct dsa_switch *ds = dp->ds;
 	u32 phy_flags = 0;
-	int ret;
+	int ret, addr;
 
 	dp->pl_config.dev = &slave_dev->dev;
 	dp->pl_config.type = PHYLINK_NETDEV;
@@ -2299,7 +2299,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 		/* We could not connect to a designated PHY or SFP, so try to
 		 * use the switch internal MDIO bus instead
 		 */
-		ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
+		if (ds->ops->get_phy_address)
+			addr = ds->ops->get_phy_address(ds, dp->index);
+		else
+			addr = dp->index;
+
+		ret = dsa_slave_phy_connect(slave_dev, addr, phy_flags);
 	}
 	if (ret) {
 		netdev_err(slave_dev, "failed to connect to PHY: %pe\n",
-- 
2.37.2

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Vladimir Oltean]

Hi Lukasz,

On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> Avoid having to define a PHY for every physical port when PHY addresses
> are fixed, but port index != PHY address.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> [Adjustments for newest kernel upstreaming]

(I got reminded by the message)
How are things going with the imx28 MTIP L2 switch? Upstreaming went
silent on that.

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Lukasz Majewski]

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

Hi Vladimir,

> Hi Lukasz,
> 
> On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > Avoid having to define a PHY for every physical port when PHY
> > addresses are fixed, but port index != PHY address.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > [Adjustments for newest kernel upstreaming]  
> 
> (I got reminded by the message)
> How are things going with the imx28 MTIP L2 switch? Upstreaming went
> silent on that.

Yes, this task has been postponed as:

- Customer decided to use Linux 4.19 CIP for which I've forward ported
  the original NXP patches [1][2]

- Considering the above - I would need to change the structure of the
  switch driver (which up till now is based on old - i.e. 2.6.3x - FEC
  driver) and modify the current FEC to add acceleration in similar way
  to TI's driver.

  This is IMHO quite time consuming task ...

  (The attempt to add it as DSA [2] was a bit less intrusive, but is
  conceptually wrong).

Links:
[1] - https://github.com/lmajewski/linux-imx28-l2switch/tree/master

[2] -
https://github.com/lmajewski/linux-imx28-l2switch/tree/imx28-v5.12-L2-upstream-RFC_v1

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Andrew Lunn]

On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> Avoid having to define a PHY for every physical port when PHY addresses
> are fixed, but port index != PHY address.

Please could you expand the commit message. What i think is going on
is that for the lower device, port 0 has phy address 0, port 1 phy
address 1. But the upper switch has port 0 phy address 16, port 1 phy
addr 17?

6141 and 6341 set phy_base_addr to 0x10. Oddly, this is only used for
the interrupt. So i assume these two devices also need device tree
phy-handle descriptions?

It would be nice to fix the 6141 and the 6341 as well.

What might help with understanding is have the patch for the mv88e6xxx
op first, and then wire it up in the core afterwards. Reviews tend to
happen first to last, so either your commit message needs to explain
what is coming, or you do things in the order which helps the reviewer
the most.

   Andrew

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > Avoid having to define a PHY for every physical port when PHY
> > addresses are fixed, but port index != PHY address.  
> 
> Please could you expand the commit message.

I've left the comment untouched from Matthias...

> What i think is going on
> is that for the lower device, port 0 has phy address 0, port 1 phy
> address 1. But the upper switch has port 0 phy address 16, port 1 phy
> addr 17?

To be more specific -> for mv88e6071 and mv88e6020:

PHY ports have SMI addresses from 0 to 0x6 and
Switch PORTS have addresses from 0x8 to 0xE

Global 2 -> 0x7 
Global 1 -> 0xF

Access to PHYs is ONLY possible via indirect access from Global 2 (0x7
SMI addr) - offsets 0x18 and 0x19.

but :-)

there is also RO_LED/ADDR4 pin (bootstrap), which when set changes the
SMI address of PHYs (from 0x00 - 0x04 to 0x10 to 0x14). This allows
easy expansion of the number of ports for switch...

> 
> 6141 and 6341 set phy_base_addr to 0x10. 

It depends if the HW designer set this bootstrap pin low or high :-)
(often this pin is not concerned until mainline/BSP driver is not
working :-) )

As it costs $ to fix this - it is easier to add "quirk" to the code.

> Oddly, this is only used for
> the interrupt. So i assume these two devices also need device tree
> phy-handle descriptions?

I only have access to 6071 and 6020 switches.

> 
> It would be nice to fix the 6141 and the 6341 as well.

As Vladimir pointed out - many Marvell switches use the "old" DTS
description ...

> 
> What might help with understanding is have the patch for the mv88e6xxx
> op first, and then wire it up in the core afterwards. Reviews tend to
> happen first to last, so either your commit message needs to explain
> what is coming, or you do things in the order which helps the reviewer
> the most.

I must admit, that I've just used code (his patch sert) from Matthias as
a starting point (to keep his credits).

> 
>    Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Andrew Lunn]

> It depends if the HW designer set this bootstrap pin low or high :-)
> (often this pin is not concerned until mainline/BSP driver is not
> working :-) )
> 
> As it costs $ to fix this - it is easier to add "quirk" to the code.

Can you read the strapping configuration via the scratch registers?
Then you can detect it at probe time and do the right thing.

     Andrew

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Lukasz Majewski]

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

Hi Andrew,

> > It depends if the HW designer set this bootstrap pin low or high :-)
> > (often this pin is not concerned until mainline/BSP driver is not
> > working :-) )
> > 
> > As it costs $ to fix this - it is easier to add "quirk" to the
> > code.  
> 
> Can you read the strapping configuration via the scratch registers?
> Then you can detect it at probe time and do the right thing.
> 

This is how I do it, yes. My impression is that some DTS descriptions
have this hardcoded instead.

>      Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/9] net: dsa: allow switch drivers to override default slave PHY addresses

[Florian Fainelli]

On 11/8/2022 12:23 AM, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> Avoid having to define a PHY for every physical port when PHY addresses
> are fixed, but port index != PHY address.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> [Adjustments for newest kernel upstreaming]

Seems unnecessary when this can be specified through Device Tree entirely.

It is convenient to be able not to declare the switch internal MDIO bus 
and how the PHY address to port mapping is established, but Device Tree 
remains the most flexible way of mapping all possible types of 
configurations.
-- 
Florian

[PATCH 2/9] net: dsa: mv88e6xxx: account for PHY base address offset in dual chip mode

[Lukasz Majewski]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

In dual chip mode (6250 family), not only global and port registers are
shifted by sw_addr, but also the PHY addresses. Account for this in the
IRQ mapping.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/dsa/mv88e6xxx/chip.h    | 1 +
 drivers/net/dsa/mv88e6xxx/global2.c | 2 +-
 drivers/net/dsa/mv88e6xxx/smi.c     | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..b03f279a673d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -328,6 +328,7 @@ struct mv88e6xxx_chip {
 	const struct mv88e6xxx_bus_ops *smi_ops;
 	struct mii_bus *bus;
 	int sw_addr;
+	unsigned int phy_base_addr;
 
 	/* Handles automatic disabling and re-enabling of the PHY
 	 * polling unit.
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index fa65ecd9cb85..fd6ba1fc6bef 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -1172,7 +1172,7 @@ int mv88e6xxx_g2_irq_mdio_setup(struct mv88e6xxx_chip *chip,
 			err = irq;
 			goto out;
 		}
-		bus->irq[chip->info->phy_base_addr + phy] = irq;
+		bus->irq[chip->phy_base_addr + phy] = irq;
 	}
 	return 0;
 out:
diff --git a/drivers/net/dsa/mv88e6xxx/smi.c b/drivers/net/dsa/mv88e6xxx/smi.c
index a990271b7482..520e47b375b2 100644
--- a/drivers/net/dsa/mv88e6xxx/smi.c
+++ b/drivers/net/dsa/mv88e6xxx/smi.c
@@ -186,5 +186,9 @@ int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
 	if (chip->smi_ops->init)
 		return chip->smi_ops->init(chip);
 
+	chip->phy_base_addr = chip->info->phy_base_addr;
+	if (chip->info->dual_chip)
+		chip->phy_base_addr += sw_addr;
+
 	return 0;
 }
-- 
2.37.2

Re: [PATCH 2/9] net: dsa: mv88e6xxx: account for PHY base address offset in dual chip mode

[Andrew Lunn]

On Tue, Nov 08, 2022 at 09:23:23AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> In dual chip mode (6250 family), not only global and port registers are
> shifted by sw_addr, but also the PHY addresses. Account for this in the
> IRQ mapping.

> +++ b/drivers/net/dsa/mv88e6xxx/smi.c
> @@ -186,5 +186,9 @@ int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
>  	if (chip->smi_ops->init)
>  		return chip->smi_ops->init(chip);
>  
> +	chip->phy_base_addr = chip->info->phy_base_addr;
> +	if (chip->info->dual_chip)
> +		chip->phy_base_addr += sw_addr;
> +
>  	return 0;


Again, reviewing first to last, i assume the will be a patch soon
implementing get_phy_address(), and it will have the same logic. Why
not call it here, a default implementation which returns
info->phy_base_addr, and a version for 6250 which returns sw_addr.

	Andrew

Re: [PATCH 2/9] net: dsa: mv88e6xxx: account for PHY base address offset in dual chip mode

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:23AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > In dual chip mode (6250 family), not only global and port registers
> > are shifted by sw_addr, but also the PHY addresses. Account for
> > this in the IRQ mapping.  
> 
> > +++ b/drivers/net/dsa/mv88e6xxx/smi.c
> > @@ -186,5 +186,9 @@ int mv88e6xxx_smi_init(struct mv88e6xxx_chip
> > *chip, if (chip->smi_ops->init)
> >  		return chip->smi_ops->init(chip);
> >  
> > +	chip->phy_base_addr = chip->info->phy_base_addr;
> > +	if (chip->info->dual_chip)
> > +		chip->phy_base_addr += sw_addr;
> > +
> >  	return 0;  
> 
> 
> Again, reviewing first to last, i assume the will be a patch soon
> implementing get_phy_address(), and it will have the same logic. Why
> not call it here, a default implementation which returns
> info->phy_base_addr, and a version for 6250 which returns sw_addr.
> 

I will squash Matthias patches and prepare new set of them with proper
operation's ordering.

> 	Andrew


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Lukasz Majewski]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Avoid the need to specify a PHY for each physical port in the device tree
when phy_base_addr is not 0 (6250 and 6341 families).

This change should be backwards-compatible with existing device trees,
as it only adds sensible defaults where explicit definitions were
required before.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2479be3a1e35..d51fd1966be9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6362,6 +6362,13 @@ static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds,
 	return chip->tag_protocol;
 }
 
+static int mv88e6xxx_get_phy_address(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	return chip->phy_base_addr + port;
+}
+
 static int mv88e6xxx_change_tag_protocol(struct dsa_switch *ds,
 					 enum dsa_tag_protocol proto)
 {
@@ -6887,6 +6894,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.phylink_mac_link_up	= mv88e6xxx_mac_link_up,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
+	.get_phy_address        = mv88e6xxx_get_phy_address,
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 	.port_enable		= mv88e6xxx_port_enable,
 	.port_disable		= mv88e6xxx_port_disable,
-- 
2.37.2

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Vladimir Oltean]

On Tue, Nov 08, 2022 at 09:23:24AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> Avoid the need to specify a PHY for each physical port in the device tree
> when phy_base_addr is not 0 (6250 and 6341 families).
> 
> This change should be backwards-compatible with existing device trees,
> as it only adds sensible defaults where explicit definitions were
> required before.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Needs your Signed-off-by tag as well.

> ---

Would it be possible to do like armada-3720-turris-mox.dts does, and put
the phy-handle in the device tree, avoiding the need for so many PHY
address translation quirks?

If you're going to have U-Boot support for this switch as well, the
phy-handle mechanism is the only thing that U-Boot supports, so device
trees written in this way will work for both (and can be passed by
U-Boot to Linux):

	switch1@11 {
		compatible = "marvell,mv88e6190";
		reg = <0x11>;
		dsa,member = <0 1>;
		interrupt-parent = <&moxtet>;
		interrupts = <MOXTET_IRQ_PERIDOT(1)>;
		status = "disabled";

		mdio {
			#address-cells = <1>;
			#size-cells = <0>;

			switch1phy1: switch1phy1@1 {
				reg = <0x1>;
			};

			switch1phy2: switch1phy2@2 {
				reg = <0x2>;
			};

			switch1phy3: switch1phy3@3 {
				reg = <0x3>;
			};

			switch1phy4: switch1phy4@4 {
				reg = <0x4>;
			};

			switch1phy5: switch1phy5@5 {
				reg = <0x5>;
			};

			switch1phy6: switch1phy6@6 {
				reg = <0x6>;
			};

			switch1phy7: switch1phy7@7 {
				reg = <0x7>;
			};

			switch1phy8: switch1phy8@8 {
				reg = <0x8>;
			};
		};

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@1 {
				reg = <0x1>;
				label = "lan9";
				phy-handle = <&switch1phy1>;
			};

			port@2 {
				reg = <0x2>;
				label = "lan10";
				phy-handle = <&switch1phy2>;
			};

			port@3 {
				reg = <0x3>;
				label = "lan11";
				phy-handle = <&switch1phy3>;
			};

			port@4 {
				reg = <0x4>;
				label = "lan12";
				phy-handle = <&switch1phy4>;
			};

			port@5 {
				reg = <0x5>;
				label = "lan13";
				phy-handle = <&switch1phy5>;
			};

			port@6 {
				reg = <0x6>;
				label = "lan14";
				phy-handle = <&switch1phy6>;
			};

			port@7 {
				reg = <0x7>;
				label = "lan15";
				phy-handle = <&switch1phy7>;
			};

			port@8 {
				reg = <0x8>;
				label = "lan16";
				phy-handle = <&switch1phy8>;
			};

			switch1port9: port@9 {
				reg = <0x9>;
				label = "dsa";
				phy-mode = "2500base-x";
				managed = "in-band-status";
				link = <&switch0port10>;
			};

			switch1port10: port@a {
				reg = <0xa>;
				label = "dsa";
				phy-mode = "2500base-x";
				managed = "in-band-status";
				link = <&switch2port9>;
				status = "disabled";
			};

			port-sfp@a {
				reg = <0xa>;
				label = "sfp";
				sfp = <&sfp>;
				phy-mode = "sgmii";
				managed = "in-band-status";
				status = "disabled";
			};
		};
	};

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Andrew Lunn]

> Would it be possible to do like armada-3720-turris-mox.dts does, and put
> the phy-handle in the device tree, avoiding the need for so many PHY
> address translation quirks?
> 
> If you're going to have U-Boot support for this switch as well, the
> phy-handle mechanism is the only thing that U-Boot supports, so device
> trees written in this way will work for both (and can be passed by
> U-Boot to Linux):

This is how i expect any board using the MV88E6141 and MV88E6341 work.
It has the same issue that it is not a 1:1 mapping.

Portability with U-boot is an interesting argument. Maybe there are
patches to u-boot to add the same sort of quirks?

	Andrew

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Lukasz Majewski]

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

Hi Andrew,

> > Would it be possible to do like armada-3720-turris-mox.dts does,
> > and put the phy-handle in the device tree, avoiding the need for so
> > many PHY address translation quirks?
> > 
> > If you're going to have U-Boot support for this switch as well, the
> > phy-handle mechanism is the only thing that U-Boot supports,

Maybe in the generic case of PHY, yes (via Driver Model).

However, when you delve into the mv88e6xxx driver [1] - you would find
that this is not supporting it yet ...

> > so
> > device trees written in this way will work for both (and can be
> > passed by U-Boot to Linux):  
> 
> This is how i expect any board using the MV88E6141 and MV88E6341 work.
> It has the same issue that it is not a 1:1 mapping.

Please be aware that there is also majority of DTS entries, which use
the "old" switch description ...

> 
> Portability with U-boot is an interesting argument. Maybe there are
> patches to u-boot to add the same sort of quirks?

As fair as I know - for the driver [1] - there was no ongoing effort
recently.

> 
> 	Andrew

Links:
[1] -
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Vladimir Oltean]

On Thu, Nov 10, 2022 at 06:00:53PM +0100, Lukasz Majewski wrote:
> Maybe in the generic case of PHY, yes (via Driver Model).
> 
> However, when you delve into the mv88e6xxx driver [1] - you would find
> that this is not supporting it yet ...
> 
> As fair as I know - for the driver [1] - there was no ongoing effort
> recently.
> 
> Links:
> [1] -
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c

U-Boot mailing list is moving a bit slower than netdev (and the patch
set is not yet applied despite being ready), but I was talking about
this:
https://patchwork.ozlabs.org/project/uboot/list/?series=324983

If you study DM_DSA, you'll see that only supporting PHY connection via
phy-handle (even if the PHY is internal) (or fixed-link, in lack of a
PHY) was an absolutely deliberate decision.

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Lukasz Majewski]

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

Hi Vladimir,

> On Thu, Nov 10, 2022 at 06:00:53PM +0100, Lukasz Majewski wrote:
> > Maybe in the generic case of PHY, yes (via Driver Model).
> > 
> > However, when you delve into the mv88e6xxx driver [1] - you would
> > find that this is not supporting it yet ...
> > 
> > As fair as I know - for the driver [1] - there was no ongoing effort
> > recently.
> > 
> > Links:
> > [1] -
> > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mv88e61xx.c
> >  
> 
> U-Boot mailing list is moving a bit slower than netdev (and the patch
> set is not yet applied despite being ready), but I was talking about
> this:
> https://patchwork.ozlabs.org/project/uboot/list/?series=324983
> 

Thanks for sharing this link. It looks like this driver also supports
switchnig addresses for Marvell ICs.

(This was the goal for mine previous patches).


> If you study DM_DSA, you'll see that only supporting PHY connection
> via phy-handle (even if the PHY is internal) (or fixed-link, in lack
> of a PHY) was an absolutely deliberate decision.

Ok. I do agree now - will adjust the code accordingly.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Lukasz Majewski]

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

Hi Vladimir,

> On Tue, Nov 08, 2022 at 09:23:24AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > Avoid the need to specify a PHY for each physical port in the
> > device tree when phy_base_addr is not 0 (6250 and 6341 families).
> > 
> > This change should be backwards-compatible with existing device
> > trees, as it only adds sensible defaults where explicit definitions
> > were required before.
> > 
> > Signed-off-by: Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com>  
> 
> Needs your Signed-off-by tag as well.

Ok.

> 
> > ---  
> 
> Would it be possible to do like armada-3720-turris-mox.dts does, and
> put the phy-handle in the device tree, avoiding the need for so many
> PHY address translation quirks?

As far as I can tell - the mv88e6xxx driver
(./drivers/net/dsa/mv88e6xxx) now uses hardcoded values for each member
of mv88e6xxx_info struct.

Those values are "port_base_addr" and "phy_base_addr". Those values
could be read from DTS description as pasted below.

> 
> If you're going to have U-Boot support for this switch as well, the
> phy-handle mechanism is the only thing that U-Boot supports, so device
> trees written in this way will work for both (and can be passed by
> U-Boot to Linux):
> 
> 	switch1@11 {
> 		compatible = "marvell,mv88e6190";
> 		reg = <0x11>;
> 		dsa,member = <0 1>;
> 		interrupt-parent = <&moxtet>;
> 		interrupts = <MOXTET_IRQ_PERIDOT(1)>;
> 		status = "disabled";
> 
> 		mdio {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			switch1phy1: switch1phy1@1 {
> 				reg = <0x1>;
> 			};
> 
> 			switch1phy2: switch1phy2@2 {
> 				reg = <0x2>;
> 			};
> 
> 			switch1phy3: switch1phy3@3 {
> 				reg = <0x3>;
> 			};
> 
> 			switch1phy4: switch1phy4@4 {
> 				reg = <0x4>;
> 			};
> 
> 			switch1phy5: switch1phy5@5 {
> 				reg = <0x5>;
> 			};
> 
> 			switch1phy6: switch1phy6@6 {
> 				reg = <0x6>;
> 			};
> 
> 			switch1phy7: switch1phy7@7 {
> 				reg = <0x7>;
> 			};
> 
> 			switch1phy8: switch1phy8@8 {
> 				reg = <0x8>;
> 			};
> 		};
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@1 {
> 				reg = <0x1>;
> 				label = "lan9";
> 				phy-handle = <&switch1phy1>;
> 			};
> 
> 			port@2 {
> 				reg = <0x2>;
> 				label = "lan10";
> 				phy-handle = <&switch1phy2>;
> 			};
> 
> 			port@3 {
> 				reg = <0x3>;
> 				label = "lan11";
> 				phy-handle = <&switch1phy3>;
> 			};
> 
> 			port@4 {
> 				reg = <0x4>;
> 				label = "lan12";
> 				phy-handle = <&switch1phy4>;
> 			};
> 
> 			port@5 {
> 				reg = <0x5>;
> 				label = "lan13";
> 				phy-handle = <&switch1phy5>;
> 			};
> 
> 			port@6 {
> 				reg = <0x6>;
> 				label = "lan14";
> 				phy-handle = <&switch1phy6>;
> 			};
> 
> 			port@7 {
> 				reg = <0x7>;
> 				label = "lan15";
> 				phy-handle = <&switch1phy7>;
> 			};
> 
> 			port@8 {
> 				reg = <0x8>;
> 				label = "lan16";
> 				phy-handle = <&switch1phy8>;
> 			};
> 
> 			switch1port9: port@9 {
> 				reg = <0x9>;
> 				label = "dsa";
> 				phy-mode = "2500base-x";
> 				managed = "in-band-status";
> 				link = <&switch0port10>;
> 			};
> 
> 			switch1port10: port@a {
> 				reg = <0xa>;
> 				label = "dsa";
> 				phy-mode = "2500base-x";
> 				managed = "in-band-status";
> 				link = <&switch2port9>;
> 				status = "disabled";
> 			};
> 
> 			port-sfp@a {
> 				reg = <0xa>;
> 				label = "sfp";
> 				sfp = <&sfp>;
> 				phy-mode = "sgmii";
> 				managed = "in-band-status";
> 				status = "disabled";
> 			};
> 		};
> 	};

The u-boot mailine has basic support for mv88e6071 and mv88e6020 (and
also some 'extension' patches which are floating around [1]).

For the current code - I'm using:

 mdio {
        #address-cells = <1>;
        #size-cells = <0>;

        switch@0 {
            compatible = "marvell,mv88e6250";
            reg = <0x00>;

            interrupt-parent = <&gpio2>;
            interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
            interrupt-controller;
            #interrupt-cells = <2>;

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    label = "lan1";
                };

                port@1 {
                    reg = <1>;
                    label = "lan2";
                };

                port@2 {
                    reg = <2>;
                    label = "lan3";
                };

                port@5 {
                    reg = <5>;
                    label = "cpu";
                    phy-mode = "rgmii-id";
                    ethernet = <&fec1>;

                    fixed-link {
                           speed = <100>;
                           full-duplex;
                    };
                };
	};


The only "hack" which I see from time to time is the replacement of
'switch@0' with 'switch@8' to take into account the R0_LED/ADDRES4
bootstrap pin value (to shift up ports addresses).


Links:

[1] - https://lists.denx.de/pipermail/u-boot/2021-March/444827.html



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/9] net: dsa: mv88e6xxx: add support for MV88E6020 switch

[Lukasz Majewski]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

A 6250 family switch with 5 internal PHYs and no PTP support.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Signed-off-by: Lukasz Majewski <lukma@denx.de>
[Adjustments for newest kernel upstreaming]
---
 drivers/net/dsa/mv88e6xxx/chip.c | 20 ++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 3 files changed, 22 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d51fd1966be9..cfb6df516e27 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5527,6 +5527,26 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 };
 
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
+	[MV88E6020] = {
+		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6020,
+		.family = MV88E6XXX_FAMILY_6250,
+		.name = "Marvell 88E6020",
+		.num_databases = 64,
+		.num_ports = 7,
+		.num_internal_phys = 5,
+		.max_vid = 4095,
+		.port_base_addr = 0x8,
+		.phy_base_addr = 0x0,
+		.global1_addr = 0xf,
+		.global2_addr = 0x7,
+		.age_time_coeff = 15000,
+		.g1_irqs = 9,
+		.g2_irqs = 5,
+		.atu_move_port_mask = 0xf,
+		.dual_chip = true,
+		.ops = &mv88e6250_ops,
+	},
+
 	[MV88E6085] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6085,
 		.family = MV88E6XXX_FAMILY_6097,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index b03f279a673d..c7cbbecd7fe1 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -54,6 +54,7 @@ enum mv88e6xxx_frame_mode {
 
 /* List of supported models */
 enum mv88e6xxx_model {
+	MV88E6020,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index cb04243f37c1..60a36a8bc131 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -111,6 +111,7 @@
 /* Offset 0x03: Switch Identifier Register */
 #define MV88E6XXX_PORT_SWITCH_ID		0x03
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_MASK	0xfff0
+#define MV88E6XXX_PORT_SWITCH_ID_PROD_6020	0x0200
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6085	0x04a0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6095	0x0950
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6097	0x0990
-- 
2.37.2

[PATCH 5/9] net: dsa: mv88e6xxx: Add support for MV88E6071 switch

[Lukasz Majewski]

This patch provides support for MV88E6071 Marvell switch.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 21 +++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 drivers/net/dsa/mv88e6xxx/port.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index cfb6df516e27..09877a464665 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5547,6 +5547,27 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.ops = &mv88e6250_ops,
 	},
 
+	[MV88E6071] = {
+		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6071,
+		.family = MV88E6XXX_FAMILY_6250,
+		.name = "Marvell 88E6071",
+		.num_databases = 64,
+		.num_ports = 7,
+		.num_internal_phys = 5,
+		.max_vid = 4095,
+		.port_base_addr = 0x08,
+		.phy_base_addr = 0x00,
+		.global1_addr = 0x0f,
+		.global2_addr = 0x07,
+		.age_time_coeff = 15000,
+		.g1_irqs = 9,
+		.g2_irqs = 5,
+		.atu_move_port_mask = 0xf,
+		.dual_chip = true,
+		.ptp_support = true,
+		.ops = &mv88e6250_ops,
+	},
+
 	[MV88E6085] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6085,
 		.family = MV88E6XXX_FAMILY_6097,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index c7cbbecd7fe1..2fcab41e03b7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -55,6 +55,7 @@ enum mv88e6xxx_frame_mode {
 /* List of supported models */
 enum mv88e6xxx_model {
 	MV88E6020,
+	MV88E6071,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 60a36a8bc131..04e814a45597 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -112,6 +112,7 @@
 #define MV88E6XXX_PORT_SWITCH_ID		0x03
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_MASK	0xfff0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6020	0x0200
+#define MV88E6XXX_PORT_SWITCH_ID_PROD_6071	0x0710
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6085	0x04a0
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6095	0x0950
 #define MV88E6XXX_PORT_SWITCH_ID_PROD_6097	0x0990
-- 
2.37.2

[PATCH 6/9] net: dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski]

Different Marvell DSA switches support different size of max frame
bytes to be sent.

For example mv88e6185 supports max 1632B, which is now in-driver
standard value. On the other hand - mv88e6071 supports 2048 bytes.

As this value is internal and may be different for each switch IC
new entry in struct mv88e6xxx_info has been added to store it.

When the 'max_frame_size' is not defined (and hence zeroed by
the kvzalloc()) the default of 1632 bytes is used.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 12 ++++++++++--
 drivers/net/dsa/mv88e6xxx/chip.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 09877a464665..d90835b4c606 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3542,11 +3542,19 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	int max_frame_size;
 
 	if (chip->info->ops->port_set_jumbo_size)
 		return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
-	else if (chip->info->ops->set_max_frame_size)
-		return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
+	else if (chip->info->ops->set_max_frame_size) {
+		if (chip->info->max_frame_size)
+			max_frame_size = chip->info->max_frame_size;
+		else
+			max_frame_size = 1632;
+
+		return max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
+	}
+
 	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 2fcab41e03b7..6ec4010fd634 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -132,6 +132,7 @@ struct mv88e6xxx_info {
 	unsigned int num_ports;
 	unsigned int num_internal_phys;
 	unsigned int num_gpio;
+	unsigned int max_frame_size;
 	unsigned int max_vid;
 	unsigned int max_sid;
 	unsigned int port_base_addr;
-- 
2.37.2

Re: [PATCH 6/9] net: dsa: marvell: Provide per device information about max frame size

[Andrew Lunn]

On Tue, Nov 08, 2022 at 09:23:27AM +0100, Lukasz Majewski wrote:
> Different Marvell DSA switches support different size of max frame
> bytes to be sent.
> 
> For example mv88e6185 supports max 1632B, which is now in-driver
> standard value. On the other hand - mv88e6071 supports 2048 bytes.
> 
> As this value is internal and may be different for each switch IC
> new entry in struct mv88e6xxx_info has been added to store it.
> 
> When the 'max_frame_size' is not defined (and hence zeroed by
> the kvzalloc()) the default of 1632 bytes is used.

I would prefer every entry states the value. That both simplifies the
code, and probably reduces the likelihood of somebody forgetting to
set it when adding a new chip.

    Andrew

Re: [PATCH 6/9] net: dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:27AM +0100, Lukasz Majewski wrote:
> > Different Marvell DSA switches support different size of max frame
> > bytes to be sent.
> > 
> > For example mv88e6185 supports max 1632B, which is now in-driver
> > standard value. On the other hand - mv88e6071 supports 2048 bytes.
> > 
> > As this value is internal and may be different for each switch IC
> > new entry in struct mv88e6xxx_info has been added to store it.
> > 
> > When the 'max_frame_size' is not defined (and hence zeroed by
> > the kvzalloc()) the default of 1632 bytes is used.  
> 
> I would prefer every entry states the value.

You mean to add it explicitly (i.e. for each supported switch version)
to the struct mv88e6xxx_ops ?

> That both simplifies the
> code, and probably reduces the likelihood of somebody forgetting to
> set it when adding a new chip.

Ok.

> 
>     Andrew


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 6/9] net: dsa: marvell: Provide per device information about max frame size

[Andrew Lunn]

On Thu, Nov 10, 2022 at 04:36:37PM +0100, Lukasz Majewski wrote:
> Hi Andrew,
> 
> > On Tue, Nov 08, 2022 at 09:23:27AM +0100, Lukasz Majewski wrote:
> > > Different Marvell DSA switches support different size of max frame
> > > bytes to be sent.
> > > 
> > > For example mv88e6185 supports max 1632B, which is now in-driver
> > > standard value. On the other hand - mv88e6071 supports 2048 bytes.
> > > 
> > > As this value is internal and may be different for each switch IC
> > > new entry in struct mv88e6xxx_info has been added to store it.
> > > 
> > > When the 'max_frame_size' is not defined (and hence zeroed by
> > > the kvzalloc()) the default of 1632 bytes is used.  
> > 
> > I would prefer every entry states the value.
> 
> You mean to add it explicitly (i.e. for each supported switch version)
> to the struct mv88e6xxx_ops ?

To the info structure. You added it for just your devices and left it
to 0 for the rest. Rather than special case 0, always set the value
and remove the special case. Maybe even -EINVAL if you find a 0 there.

   Andrew

Re: [PATCH 6/9] net: dsa: marvell: Provide per device information about max frame size

[Lukasz Majewski]

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

Hi Andrew,

> On Thu, Nov 10, 2022 at 04:36:37PM +0100, Lukasz Majewski wrote:
> > Hi Andrew,
> >   
> > > On Tue, Nov 08, 2022 at 09:23:27AM +0100, Lukasz Majewski wrote:  
> > > > Different Marvell DSA switches support different size of max
> > > > frame bytes to be sent.
> > > > 
> > > > For example mv88e6185 supports max 1632B, which is now in-driver
> > > > standard value. On the other hand - mv88e6071 supports 2048
> > > > bytes.
> > > > 
> > > > As this value is internal and may be different for each switch
> > > > IC new entry in struct mv88e6xxx_info has been added to store
> > > > it.
> > > > 
> > > > When the 'max_frame_size' is not defined (and hence zeroed by
> > > > the kvzalloc()) the default of 1632 bytes is used.    
> > > 
> > > I would prefer every entry states the value.  
> > 
> > You mean to add it explicitly (i.e. for each supported switch
> > version) to the struct mv88e6xxx_ops ?  
> 
> To the info structure. You added it for just your devices and left it
> to 0 for the rest. Rather than special case 0, always set the value
> and remove the special case. Maybe even -EINVAL if you find a 0 there.
> 

Ok.

>    Andrew


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 7/9] net: dsa: mv88e6071: Define max frame size (2048 bytes)

[Lukasz Majewski]

Accroding to the documentation - the mv88e6071 can support
frame size up to 2048 bytes.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d90835b4c606..e0224fc92ddf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5563,6 +5563,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.max_vid = 4095,
+		.max_frame_size = 2048,
 		.port_base_addr = 0x08,
 		.phy_base_addr = 0x00,
 		.global1_addr = 0x0f,
-- 
2.37.2

Re: [PATCH 7/9] net: dsa: mv88e6071: Define max frame size (2048 bytes)

[Andrew Lunn]

On Tue, Nov 08, 2022 at 09:23:28AM +0100, Lukasz Majewski wrote:
> Accroding to the documentation - the mv88e6071 can support
> frame size up to 2048 bytes.

Since the mv88e6020 is in the same family, it probably is the same?
And what about the mv88e66220?

    Andrew

Re: [PATCH 7/9] net: dsa: mv88e6071: Define max frame size (2048 bytes)

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:28AM +0100, Lukasz Majewski wrote:
> > Accroding to the documentation - the mv88e6071 can support
> > frame size up to 2048 bytes.  
> 
> Since the mv88e6020 is in the same family, it probably is the same?

Yes it is 2048 B

> And what about the mv88e66220?

You mean mv88e6220 ?

IIRC they are from the same family of ICs, so my guess :-) is that they
also have the same value.

> 
>     Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/9] net: dsa: mv88e6071: Define max frame size (2048 bytes)

[Andrew Lunn]

On Thu, Nov 10, 2022 at 04:42:36PM +0100, Lukasz Majewski wrote:
> Hi Andrew,
> 
> > On Tue, Nov 08, 2022 at 09:23:28AM +0100, Lukasz Majewski wrote:
> > > Accroding to the documentation - the mv88e6071 can support
> > > frame size up to 2048 bytes.  
> > 
> > Since the mv88e6020 is in the same family, it probably is the same?
> 
> Yes it is 2048 B
> 
> > And what about the mv88e66220?
> 
> You mean mv88e6220 ?

Upps, sorry, yes.

> 
> IIRC they are from the same family of ICs, so my guess :-) is that they
> also have the same value.

My point being, you created a new _ops structure when i don't think it
is needed.

   Andrew

Re: [PATCH 7/9] net: dsa: mv88e6071: Define max frame size (2048 bytes)

[Lukasz Majewski]

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

Hi Andrew,

> On Thu, Nov 10, 2022 at 04:42:36PM +0100, Lukasz Majewski wrote:
> > Hi Andrew,
> >   
> > > On Tue, Nov 08, 2022 at 09:23:28AM +0100, Lukasz Majewski wrote:  
> > > > Accroding to the documentation - the mv88e6071 can support
> > > > frame size up to 2048 bytes.    
> > > 
> > > Since the mv88e6020 is in the same family, it probably is the
> > > same?  
> > 
> > Yes it is 2048 B
> >   
> > > And what about the mv88e66220?  
> > 
> > You mean mv88e6220 ?  
> 
> Upps, sorry, yes.
> 
> > 
> > IIRC they are from the same family of ICs, so my guess :-) is that
> > they also have the same value.  
> 
> My point being, you created a new _ops structure when i don't think it
> is needed.

This was mostly my precaution to not introduce regression for other
supported ICs.

However, this shall not be the problem as long as the max supported
frame size is set for each of it. Then calling set_max_frame_size()
callback will always provide correct value.

> 
>    Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 8/9] net: dsa: mv88e6071: Provide struct mv88e6xxx_ops

[Lukasz Majewski]

This structure is the same as the one for mv88e6250 and is supposed
to ease further development of mv88e6071 (to put functions not present
on the mv88e6250 device).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 43 ++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e0224fc92ddf..1aba9d15a5e0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4961,6 +4961,45 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
 	.phylink_get_caps = mv88e6250_phylink_get_caps,
 };
 
+static const struct mv88e6xxx_ops mv88e6071_ops = {
+	/* MV88E6XXX_FAMILY_6071 */
+	.ieee_pri_map = mv88e6250_g1_ieee_pri_map,
+	.ip_pri_map = mv88e6085_g1_ip_pri_map,
+	.irl_init_all = mv88e6352_g2_irl_init_all,
+	.get_eeprom = mv88e6xxx_g2_get_eeprom16,
+	.set_eeprom = mv88e6xxx_g2_set_eeprom16,
+	.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
+	.phy_read = mv88e6xxx_g2_smi_phy_read,
+	.phy_write = mv88e6xxx_g2_smi_phy_write,
+	.port_set_link = mv88e6xxx_port_set_link,
+	.port_sync_link = mv88e6xxx_port_sync_link,
+	.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
+	.port_set_speed_duplex = mv88e6250_port_set_speed_duplex,
+	.port_tag_remap = mv88e6095_port_tag_remap,
+	.port_set_frame_mode = mv88e6351_port_set_frame_mode,
+	.port_set_ucast_flood = mv88e6352_port_set_ucast_flood,
+	.port_set_mcast_flood = mv88e6352_port_set_mcast_flood,
+	.port_set_ether_type = mv88e6351_port_set_ether_type,
+	.port_egress_rate_limiting = mv88e6097_port_egress_rate_limiting,
+	.port_pause_limit = mv88e6097_port_pause_limit,
+	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
+	.stats_snapshot = mv88e6320_g1_stats_snapshot,
+	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
+	.stats_get_sset_count = mv88e6250_stats_get_sset_count,
+	.stats_get_strings = mv88e6250_stats_get_strings,
+	.stats_get_stats = mv88e6250_stats_get_stats,
+	.set_cpu_port = mv88e6095_g1_set_cpu_port,
+	.set_egress_port = mv88e6095_g1_set_egress_port,
+	.watchdog_ops = &mv88e6250_watchdog_ops,
+	.mgmt_rsvd2cpu = mv88e6352_g2_mgmt_rsvd2cpu,
+	.pot_clear = mv88e6xxx_g2_pot_clear,
+	.reset = mv88e6250_g1_reset,
+	.vtu_getnext = mv88e6185_g1_vtu_getnext,
+	.vtu_loadpurge = mv88e6185_g1_vtu_loadpurge,
+	.avb_ops = &mv88e6352_avb_ops,
+	.ptp_ops = &mv88e6250_ptp_ops,
+};
+
 static const struct mv88e6xxx_ops mv88e6290_ops = {
 	/* MV88E6XXX_FAMILY_6390 */
 	.setup_errata = mv88e6390_setup_errata,
@@ -5552,7 +5591,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.g2_irqs = 5,
 		.atu_move_port_mask = 0xf,
 		.dual_chip = true,
-		.ops = &mv88e6250_ops,
+		.ops = &mv88e6071_ops,
 	},
 
 	[MV88E6071] = {
@@ -5574,7 +5613,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.atu_move_port_mask = 0xf,
 		.dual_chip = true,
 		.ptp_support = true,
-		.ops = &mv88e6250_ops,
+		.ops = &mv88e6071_ops,
 	},
 
 	[MV88E6085] = {
-- 
2.37.2

[PATCH 9/9] net: dsa: mv88e6071: Set .set_max_frame_size callback

[Lukasz Majewski]

The .set_max_frame_size is now set to the
mv88e6185_g1_set_max_frame_size() function.

The global switch control register (0x4 offset) used
as well as the bit (10) are the same.

The only difference is the misleading suffix (1632)
as the mv88e6071/mv88e6020 supports 2048 bytes
as a maximal size of the frame.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 1aba9d15a5e0..ce7723d1ffbe 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4998,6 +4998,7 @@ static const struct mv88e6xxx_ops mv88e6071_ops = {
 	.vtu_loadpurge = mv88e6185_g1_vtu_loadpurge,
 	.avb_ops = &mv88e6352_avb_ops,
 	.ptp_ops = &mv88e6250_ptp_ops,
+	.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
 };
 
 static const struct mv88e6xxx_ops mv88e6290_ops = {
-- 
2.37.2

Re: [PATCH 9/9] net: dsa: mv88e6071: Set .set_max_frame_size callback

[Andrew Lunn]

On Tue, Nov 08, 2022 at 09:23:30AM +0100, Lukasz Majewski wrote:
> The .set_max_frame_size is now set to the
> mv88e6185_g1_set_max_frame_size() function.
> 
> The global switch control register (0x4 offset) used
> as well as the bit (10) are the same.
> 
> The only difference is the misleading suffix (1632)
> as the mv88e6071/mv88e6020 supports 2048 bytes
> as a maximal size of the frame.

Are you really sure that different members of the 6250 family have
different maximum frame sizes?

Marvells GPL DSDT SDK has:

#define G1_DEV_88ESPANNAK_FAMILY  (DEV_88E3020 | DEV_88E6020 | DEV_88E6070 | DEV_88E6071 | DEV_88E6220  | DEV_88E6250 )

The differences within a family tend to be the number of ports, if PTP
is provided, if AVB is provided etc.

   Andrew

Re: [PATCH 9/9] net: dsa: mv88e6071: Set .set_max_frame_size callback

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:30AM +0100, Lukasz Majewski wrote:
> > The .set_max_frame_size is now set to the
> > mv88e6185_g1_set_max_frame_size() function.
> > 
> > The global switch control register (0x4 offset) used
> > as well as the bit (10) are the same.
> > 
> > The only difference is the misleading suffix (1632)
> > as the mv88e6071/mv88e6020 supports 2048 bytes
> > as a maximal size of the frame.  
> 
> Are you really sure that different members of the 6250 family have
> different maximum frame sizes?
> 
> Marvells GPL DSDT SDK has:
> 
> #define G1_DEV_88ESPANNAK_FAMILY  (DEV_88E3020 | DEV_88E6020 |
> DEV_88E6070 | DEV_88E6071 | DEV_88E6220  | DEV_88E6250 )

From the above list - all but 88E3020 are from the same IC "family"
(they are listed in the same documentation pdf). They all have the 2048
B max packet size.

I've re-used the mv88e6185_g1_set_max_frame_size, as it does the job

> 
> The differences within a family tend to be the number of ports, if PTP
> is provided, if AVB is provided etc.
> 
>    Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]