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

Commit Message

Lukasz Majewski Nov. 8, 2022, 8:23 a.m. UTC

From: Matthias Schiffer <[e-mail]>

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 <[e-mail]>
Signed-off-by: Lukasz Majewski <[e-mail]>
[Adjustments for newest kernel upstreaming]
---
 include/net/dsa.h | 1 +
 net/dsa/slave.c   | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Vladimir Oltean Nov. 8, 2022, 9:12 a.m. UTC | #1

Hi Lukasz,

On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Lukasz Majewski <[e-mail]>
> [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.

Lukasz Majewski Nov. 8, 2022, 10:34 a.m. UTC | #2

Hi Vladimir,

> Hi Lukasz,
> 
> On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Lukasz Majewski <[e-mail]>
> > [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: [phone] Fax: [phone] Email: [e-mail]

Andrew Lunn Nov. 8, 2022, 1:21 p.m. UTC | #3

On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> From: Matthias Schiffer <[e-mail]>
> 
> 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

Florian Fainelli Nov. 8, 2022, 6:10 p.m. UTC | #4

On 11/8/2022 12:23 AM, Lukasz Majewski wrote:
> From: Matthias Schiffer <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Lukasz Majewski <[e-mail]>
> [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.

Lukasz Majewski Nov. 10, 2022, 3:34 p.m. UTC | #5

Hi Andrew,

> On Tue, Nov 08, 2022 at 09:23:22AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <[e-mail]>
> > 
> > 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: [phone] Fax: [phone] Email: [e-mail]

Andrew Lunn Nov. 10, 2022, 10:05 p.m. UTC | #6

> 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

Lukasz Majewski Nov. 14, 2022, 8:51 a.m. UTC | #7

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: [phone] Fax: [phone] Email: [e-mail]

Patch

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",