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

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

[Matthias Schiffer]

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>
---
 include/net/dsa.h |  1 +
 net/dsa/slave.c   | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index aeb411e77b9a..8216f3687799 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -391,6 +391,7 @@ struct dsa_switch_ops {
 
 	int	(*setup)(struct dsa_switch *ds);
 	void	(*teardown)(struct dsa_switch *ds);
+	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 8ced165a7908..1c78f8cae9e9 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1546,7 +1546,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	struct dsa_switch *ds = dp->ds;
 	phy_interface_t mode;
 	u32 phy_flags = 0;
-	int ret;
+	int addr, ret;
 
 	ret = of_get_phy_mode(port_dn, &mode);
 	if (ret)
@@ -1578,7 +1578,13 @@ 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);
+
+		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);
 		if (ret) {
 			netdev_err(slave_dev,
 				   "failed to connect to port %d: %d\n",
-- 
2.17.1

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

[Matthias Schiffer]

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 e5430cf2ad71..88c148a62366 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -257,6 +257,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 8fd483020c5b..3c3dfaf16882 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -1156,7 +1156,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 282fe08db050..a62d7b8702d5 100644
--- a/drivers/net/dsa/mv88e6xxx/smi.c
+++ b/drivers/net/dsa/mv88e6xxx/smi.c
@@ -175,5 +175,9 @@ int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
 	chip->bus = bus;
 	chip->sw_addr = sw_addr;
 
+	chip->phy_base_addr = chip->info->phy_base_addr;
+	if (chip->info->dual_chip)
+		chip->phy_base_addr += sw_addr;
+
 	return 0;
 }
-- 
2.17.1

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

[David Miller]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Date: Mon, 30 Mar 2020 15:53:43 +0200

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index e5430cf2ad71..88c148a62366 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -257,6 +257,7 @@ struct mv88e6xxx_chip {
>  	const struct mv88e6xxx_bus_ops *smi_ops;
>  	struct mii_bus *bus;
>  	int sw_addr;
> +	unsigned int phy_base_addr;

Please preserve the reverse christmas tree ordering here.

And this submission was quite late and net-next is closing now so
please resubmit this after the merge window and net-next opens again.

Thanks.

[PATCH net-next 3/4] net: dsa: mv88e6xxx: implement get_phy_address

[Matthias Schiffer]

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 221593261e8f..228c1b085b66 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5379,6 +5379,13 @@ static enum dsa_tag_protocol mv88e6xxx_get_tag_protocol(struct dsa_switch *ds,
 	return chip->info->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_port_mdb_prepare(struct dsa_switch *ds, int port,
 				      const struct switchdev_obj_port_mdb *mdb)
 {
@@ -5509,6 +5516,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
 	.setup			= mv88e6xxx_setup,
 	.teardown		= mv88e6xxx_teardown,
+	.get_phy_address	= mv88e6xxx_get_phy_address,
 	.phylink_validate	= mv88e6xxx_validate,
 	.phylink_mac_link_state	= mv88e6xxx_serdes_pcs_get_state,
 	.phylink_mac_config	= mv88e6xxx_mac_config,
-- 
2.17.1

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

[Matthias Schiffer]

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

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 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 228c1b085b66..a72b81c9d417 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4647,6 +4647,27 @@ static const struct mv88e6xxx_ops mv88e6390x_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,
+		.tag_protocol = DSA_TAG_PROTO_DSA,
+		.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 88c148a62366..f75d48427c26 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -47,6 +47,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 44d76ac973f6..190d7d5568e9 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -101,6 +101,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.17.1

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

[Florian Fainelli]

On 3/30/2020 6:53 AM, Matthias Schiffer wrote:
> 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>

You could do this much more elegantly by doing this with Device Tree and
specifying the built-in PHYs to be hanging off the switch's internal
MDIO bus and specifying the port to PHY address mapping, you would only
patch #4 then.
-- 
Florian

Re: (EXT) Re: [PATCH net-next 1/4] net: dsa: allow switch drivers to override default slave PHY addresses

[Matthias Schiffer]

On Mon, 2020-03-30 at 20:04 -0700, Florian Fainelli wrote:
> 
> On 3/30/2020 6:53 AM, Matthias Schiffer wrote:
> > 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
> > >
> 
> You could do this much more elegantly by doing this with Device Tree
> and
> specifying the built-in PHYs to be hanging off the switch's internal
> MDIO bus and specifying the port to PHY address mapping, you would
> only
> patch #4 then.

This does work indeed, but it seems we have different ideas on
elegance.

I'm not happy about the fact that an implementor needs to study the
switch manual in great detail to find out about things like the PHY
address offsets when the driver could just to the right thing by
default. Requiring this only for some switch configurations, while
others work fine with the defaults, doesn't make this any less
confusing (I'd even argue that it would be better if there weren't any
default PHY and IRQ mappings for the switch ports, but I also
understand that this can't easily be removed at this point...)

In particular when PHY IRQ support is desired (not implemented on the
PHY driver side for this switch yet; not sure if my current project
will require it), indices are easy to get wrong - which might not be
noticed as long as there is no PHY driver with IRQ support for the port
PHYs, potentially breaking existing Device Trees with future kernel
updates. For this reason, I think at least patch #2 should be
considered even if #1 and #3 are rejected.

Kind regards,
Matthias

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

[Matthias Schiffer]

On Tue, 2020-03-31 at 11:09 +0200, Matthias Schiffer wrote:
> On Mon, 2020-03-30 at 20:04 -0700, Florian Fainelli wrote:
> > 
> > On 3/30/2020 6:53 AM, Matthias Schiffer wrote:
> > > 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
> > > > 
> > 
> > You could do this much more elegantly by doing this with Device
> > Tree
> > and
> > specifying the built-in PHYs to be hanging off the switch's
> > internal
> > MDIO bus and specifying the port to PHY address mapping, you would
> > only
> > patch #4 then.
> 
> This does work indeed, but it seems we have different ideas on
> elegance.
> 
> I'm not happy about the fact that an implementor needs to study the
> switch manual in great detail to find out about things like the PHY
> address offsets when the driver could just to the right thing by
> default. Requiring this only for some switch configurations, while
> others work fine with the defaults, doesn't make this any less
> confusing (I'd even argue that it would be better if there weren't
> any
> default PHY and IRQ mappings for the switch ports, but I also
> understand that this can't easily be removed at this point...)
> 
> In particular when PHY IRQ support is desired (not implemented on the
> PHY driver side for this switch yet; not sure if my current project
> will require it), indices are easy to get wrong - which might not be
> noticed as long as there is no PHY driver with IRQ support for the
> port
> PHYs, potentially breaking existing Device Trees with future kernel
> updates. For this reason, I think at least patch #2 should be
> considered even if #1 and #3 are rejected.
> 
> Kind regards,
> Matthias

net-next is open again, and I'd like to come to a conclusion regarding
this patch series before I resend it (or parts of it).

It is my impression that a detailed configuration in the Device Tree is
most useful when the driver that it configures is very generic, so
different values are useful in practice.

The mv88e6xxx driver is not generic in this sense: It already lists
every single piece of supported hardware, often using completely
different code for different chips - which is already not configurable
in the Device Tree (and being able to wouldn't make much sense IMO).

Having the additional pieces of sane defaults in the driver that are
introduced by the patches 1..3 of this series avoids mistakes in the DT
configuration, for settings that never need to be modified for a given
switch model supported by mv88e6xxx.

Kind regards,
Matthias