net/dsa/microchip: correct placement of dt property phy-mode?

net/dsa/microchip: correct placement of dt property phy-mode?

[Helmut Grohne]

Hi,

According to Documentation/devicetree/bindings/net/dsa/dsa.txt, the
phy-mode property should be specified on port nodes rather than the
enclosing switch node.

In drivers/net/dsa/microchip/ksz_common.c, ksz_switch_register parses
the phy-mode property from the switch node instead:
| int ksz_switch_register(struct ksz_device *dev,
|                         const struct ksz_dev_ops *ops)
| {
...
|         /* Host port interface will be self detected, or specifically set in
|          * device tree.
|          */
|         if (dev->dev->of_node) {
|                 ret = of_get_phy_mode(dev->dev->of_node, &interface);
|                 if (ret == 0)
|                         dev->interface = interface;
...

In drivers/net/dsa/microchip/ksz9477.c, this phy_interface_t is used to
configure the MAC ports:
| static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
| {
...
|                 switch (dev->interface) {
...
|                 }
|                 ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);

KSZ9477 has two MAC interfaces (GMAC 6 -> RGMII/MII/RMII and GMAC 7 ->
SGMII). Now we're trying to configure the same interface mode for both
MACs here even though these MACs only support distinct interface modes.
This may not be problematic in practice as GMAC 7 ignores most of the
settings on the XMII Port Control 1 Register, but it still sounds wrong.

If nothing else, it makes the device tree unintuitive to use.

Is this placement of the phy-mode on the switch intentional?

If yes: I think this should be prominently documented in
Documentation/devicetree/bindings/net/dsa/ksz.txt.

If no: The microchip driver should follow the documented dsa convention
and place the phy-mode on the relevant port nodes.

If no: Do we have to support old device trees that have the phy-mode
property on the switch?

Helmut

Re: net/dsa/microchip: correct placement of dt property phy-mode?

[Andrew Lunn]

> If nothing else, it makes the device tree unintuitive to use.
> 
> Is this placement of the phy-mode on the switch intentional?

That i cannot answer.
> 
> If yes: I think this should be prominently documented in
> Documentation/devicetree/bindings/net/dsa/ksz.txt.

Yes, it needs to be documented.

> If no: The microchip driver should follow the documented dsa convention
> and place the phy-mode on the relevant port nodes.
>
> If no: Do we have to support old device trees that have the phy-mode
> property on the switch?

We should not break existing DT blobs. So the driver should be
extended to first look in the port node. If it does not find it there,
look in the switch node. And maybe give a warning if it is found in
the switch node, saying the DT should be updated.

    Andrew

[PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
index 8d19925fc09e..6783cf16ff81 100644
--- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
@@ -116,7 +116,6 @@
 		switch0: ksz8563@0 {
 			compatible = "microchip,ksz8563";
 			reg = <0>;
-			phy-mode = "mii";
 			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
 
 			spi-max-frequency = <500000>;
@@ -140,6 +139,7 @@
 					reg = <2>;
 					label = "cpu";
 					ethernet = <&macb0>;
+					phy-mode = "mii";
 					fixed-link {
 						speed = <100>;
 						full-duplex;
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 7c17b0f705ec..01c03205db53 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -941,11 +941,18 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
 	if (cpu_port) {
+		if (!p->interface && dev->compat_interface) {
+			dev_warn(dev->dev,
+				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+				 port);
+			p->interface = dev->compat_interface;
+		}
+
 		/* Configure MII interface for proper network communication. */
 		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
 		data8 &= ~PORT_INTERFACE_TYPE;
 		data8 &= ~PORT_GMII_1GPS_MODE;
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			p->phydev.speed = SPEED_100;
 			break;
@@ -961,11 +968,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		default:
 			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
 			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IN_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_OUT_ENABLE;
 			data8 |= PORT_GMII_1GPS_MODE;
 			data8 |= PORT_INTERFACE_RGMII;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 8d15c3016024..06c76948814b 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1234,7 +1234,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 
 		/* configure MAC to 1G & RGMII mode */
 		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			ksz9477_set_xmii(dev, 0, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
@@ -1255,11 +1255,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 			ksz9477_set_gbit(dev, true, &data8);
 			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
 			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_EG_ENABLE;
 			p->phydev.speed = SPEED_1000;
 			break;
@@ -1303,23 +1303,31 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			dev->cpu_port = i;
 			dev->host_mask = (1 << dev->cpu_port);
 			dev->port_mask |= dev->host_mask;
+			p = &dev->ports[i];
 
 			/* Read from XMII register to determine host port
 			 * interface.  If set specifically in device tree
 			 * note the difference to help debugging.
 			 */
 			interface = ksz9477_get_interface(dev, i);
-			if (!dev->interface)
-				dev->interface = interface;
-			if (interface && interface != dev->interface)
+			if (!p->interface) {
+				if (dev->compat_interface) {
+					dev_warn(dev->dev,
+						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+						 i);
+					p->interface = dev->compat_interface;
+				} else {
+					p->interface = interface;
+				}
+			}
+			if (interface && interface != p->interface)
 				dev_info(dev->dev,
 					 "use %s instead of %s\n",
-					  phy_modes(dev->interface),
+					  phy_modes(p->interface),
 					  phy_modes(interface));
 
 			/* enable cpu port */
 			ksz9477_port_setup(dev, i, true);
-			p = &dev->ports[dev->cpu_port];
 			p->vid_member = dev->port_mask;
 			p->on = 1;
 		}
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index fd1d6676ae4f..9cb8e04109f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -416,6 +416,8 @@ int ksz_switch_register(struct ksz_device *dev,
 {
 	phy_interface_t interface;
 	int ret;
+	struct device_node *port;
+	unsigned int port_num;
 
 	if (dev->pdata)
 		dev->chip_id = dev->pdata->chip_id;
@@ -448,10 +450,19 @@ int ksz_switch_register(struct ksz_device *dev,
 	/* Host port interface will be self detected, or specifically set in
 	 * device tree.
 	 */
+	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
+		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
-			dev->interface = interface;
+			dev->compat_interface = interface;
+		for_each_available_child_of_node(dev->dev->of_node, port) {
+			if (of_property_read_u32(port, "reg", &port_num))
+				continue;
+			if (port_num >= dev->port_cnt)
+				return -EINVAL;
+			of_get_phy_mode(port, &dev->ports[port_num].interface);
+		}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
 	}
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index f2c9bb68fd33..c0c736aaef61 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -39,6 +39,7 @@ struct ksz_port {
 	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
+	phy_interface_t interface;
 };
 
 struct ksz_device {
@@ -72,7 +73,7 @@ struct ksz_device {
 	int mib_cnt;
 	int mib_port_cnt;
 	int last_port;			/* ports after that not used */
-	phy_interface_t interface;
+	phy_interface_t compat_interface;
 	u32 regs_size;
 	bool phy_errata_9477;
 	bool synclko_125;
-- 
2.20.1

Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Andrew Lunn]

On Tue, Jul 14, 2020 at 02:08:28PM +0200, Helmut Grohne wrote:
> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.

Hi Helmut

I think this change is more complex than it needs to be. Only the CPU
port supports different interface modes. So i don't see the need to
handle both dev->interface and p->interface. Just first search
ksz_switch_register() first look in the cpu port node, and if not
found go back to the old location. The rest of the code can stay the
same.

	Andrew

Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Hi Andrew,

Thank you for the quick reply.

On Wed, Jul 15, 2020 at 12:27:16AM +0200, Andrew Lunn wrote:
> I think this change is more complex than it needs to be. Only the CPU
> port supports different interface modes. So i don't see the need to
> handle both dev->interface and p->interface. Just first search
> ksz_switch_register() first look in the cpu port node, and if not
> found go back to the old location. The rest of the code can stay the
> same.

The driver supports (among others) the KSZ9897R, which comes with two
MAC ports supporting[1, page 8] RGMII/RMII/MII (ports 6 and 7). Both of
these can be connected to a CPU, so they can both operate as CPU ports
in principle.

However, one can only enable tail tagging for one of them at a time[1,
page 39]. As the current driver expects tail tagging to be enabled on
CPU ports, it doesn't work as is with the driver. It could still be used
to form a ring of switches such that a single failing switch would leave
two chains of switches attached to the CPU. This kind of failover seems
to be part of the DSA vision (but I fail to find a reference at the
moment).

For these reasons, I think that there can be multiple CPU ports in
future. Now there is a trade-off. Either we further encode the
assumption of there being only a single CPU port more deeply into the
driver (as it already does assume that) or we can take the opportunity
to already lift it here with the vision for runtime reconfiguration of
switch topologies.

You seem to be in favour of more deeply encoding the "there can be only
one CPU port" assumption. Based on that assumption, the rest of what you
write makes very much sense to me. Is that the direction to go?

Helmut

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9897R-Data-Sheet-DS00002330D.pdf

Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Andrew Lunn]

On Wed, Jul 15, 2020 at 09:31:12AM +0200, Helmut Grohne wrote:
> You seem to be in favour of more deeply encoding the "there can be only
> one CPU port" assumption. Based on that assumption, the rest of what you
> write makes very much sense to me. Is that the direction to go?

From what i understand, there is only one port which can do RGMII. It
does not really matter if that is the CPU port, or a user
port. Ideally, whatever port it is, should have the phy-mode property
in its port node.

How you store that information until you need it is up to the
driver. But KISS is generally best, reuse what you have, unless there
is a good reason to change it. If you see this code being reused when
more than one port supports RGMII, then adding a per port members
makes sense. But if that is unlikely, keep with the global.

      Andrew

Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

On Wed, Jul 15, 2020 at 03:00:46PM +0200, Andrew Lunn wrote:
> On Wed, Jul 15, 2020 at 09:31:12AM +0200, Helmut Grohne wrote:
> > You seem to be in favour of more deeply encoding the "there can be only
> > one CPU port" assumption. Based on that assumption, the rest of what you
> > write makes very much sense to me. Is that the direction to go?
> 
> From what i understand, there is only one port which can do RGMII. It

This is not universally true. It does hold for a number of smaller chips
including KSZ9893, but it does not hold for e.g. KSZ9897R as explained
in my previous mail on this matter.

I think that this is the sole point of disagreement here. If we assume
that there only ever is one CPU (or user) port, then I agree with the
rest of what you wrote. However, my understanding is that this premise
is violated by larger devices that are partially supported by this
driver.

> does not really matter if that is the CPU port, or a user
> port. Ideally, whatever port it is, should have the phy-mode property
> in its port node.
> 
> How you store that information until you need it is up to the
> driver. But KISS is generally best, reuse what you have, unless there
> is a good reason to change it. If you see this code being reused when
> more than one port supports RGMII, then adding a per port members
> makes sense. But if that is unlikely, keep with the global.

I've prepared a patch based one the one-CPU-port assumption. It really
becomes way simpler that way. I'd like to give it a little more testing
before sending it.

Given the point of discussion I think that the assumption is a
reasonable trade-off, because you can support larger devices with
multiple RGMII-capable ports with this driver as long as you only use
one of them as CPU port. If someone ever wants to use multiple CPU ports
(not me), more significant changes are needed anyway. Partially
supporting this runs afoul KISS as you say.

Helmut

Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

On Thu, Jul 16, 2020 at 09:00:00AM +0200, Helmut Grohne wrote:
> I've prepared a patch based one the one-CPU-port assumption. It really
> becomes way simpler that way. I'd like to give it a little more testing
> before sending it.

I'm sorry, but it is not that simple. Testing revealed a fatal flaw.

At the time ksz_switch_register is called, dev->cpu_port is not
necessarily initialized. For ksz8795, it is initialized during detect
and that is fine. For ksz9477, it is initialized in
ksz9477_config_cpu_port, which comes much too late. The ksz9477 driver
actually handles the case where we choose a CPU port and selects it
using dsa_is_cpu_port (which derives this information from the device
tree during dsa_register_switch).

So in ksz_switch_register, I have no good way of knowing which port will
end up being the CPU port. Options include:
 * Replicating the logic from ksz9477_config_cpu_port. I expect that
   doing this would make the driver harder to maintain.
 * Move the cpu_port computation from ksz9477_config_cpu_port to
   ksz9477_switch_detect. I don't think this would work, because we
   presently call detect before dsa_register_switch, which parses the
   device tree. During detect, dsa_is_cpu_port is not usable.
 * Add a function to ksz_common.c that looks up the phy mode for a port
   from the device tree on demand. That way, ksz9477_config_cpu_port
   could call into the common code instead of reading a ksz_device
   property. It would defer parsing the device tree though and it could
   issue the warning multiple times.
 * Stick with my previous patch that stores the phy mode per-port.

Given the above, the last option seems least bad to me. Do you agree?

Helmut

[RESEND PATCH] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 45 insertions(+), 18 deletions(-)

Why resend?

Andrew Lunn agreed to the semantic change performed in the patch, but
not to the implementation. He asked for a simpler implementation. I
attempted doing that, but it ultimately failed, because the knowledge of
which port is the cpu port is not available at the time of parsing the
device tree. I've documented my attempts and alternatives at
https://lore.kernel.org/netdev/20200716100743.GA3275@laureti-dev/ and
reached the conclusion that my initial implementation is the simplest
option. Please reconsider including it.

Helmut

diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
index 8d19925fc09e..6783cf16ff81 100644
--- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
@@ -116,7 +116,6 @@
 		switch0: ksz8563@0 {
 			compatible = "microchip,ksz8563";
 			reg = <0>;
-			phy-mode = "mii";
 			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
 
 			spi-max-frequency = <500000>;
@@ -140,6 +139,7 @@
 					reg = <2>;
 					label = "cpu";
 					ethernet = <&macb0>;
+					phy-mode = "mii";
 					fixed-link {
 						speed = <100>;
 						full-duplex;
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 8f1d15ea15d9..cae77eafd533 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -932,11 +932,18 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
 	if (cpu_port) {
+		if (!p->interface && dev->compat_interface) {
+			dev_warn(dev->dev,
+				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+				 port);
+			p->interface = dev->compat_interface;
+		}
+
 		/* Configure MII interface for proper network communication. */
 		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
 		data8 &= ~PORT_INTERFACE_TYPE;
 		data8 &= ~PORT_GMII_1GPS_MODE;
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			p->phydev.speed = SPEED_100;
 			break;
@@ -952,11 +959,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		default:
 			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
 			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IN_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_OUT_ENABLE;
 			data8 |= PORT_GMII_1GPS_MODE;
 			data8 |= PORT_INTERFACE_RGMII;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index dc999406ce86..32150c76d8f7 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1208,7 +1208,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 
 		/* configure MAC to 1G & RGMII mode */
 		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			ksz9477_set_xmii(dev, 0, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
@@ -1229,11 +1229,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 			ksz9477_set_gbit(dev, true, &data8);
 			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
 			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_EG_ENABLE;
 			p->phydev.speed = SPEED_1000;
 			break;
@@ -1269,23 +1269,31 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			dev->cpu_port = i;
 			dev->host_mask = (1 << dev->cpu_port);
 			dev->port_mask |= dev->host_mask;
+			p = &dev->ports[i];
 
 			/* Read from XMII register to determine host port
 			 * interface.  If set specifically in device tree
 			 * note the difference to help debugging.
 			 */
 			interface = ksz9477_get_interface(dev, i);
-			if (!dev->interface)
-				dev->interface = interface;
-			if (interface && interface != dev->interface)
+			if (!p->interface) {
+				if (dev->compat_interface) {
+					dev_warn(dev->dev,
+						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+						 i);
+					p->interface = dev->compat_interface;
+				} else {
+					p->interface = interface;
+				}
+			}
+			if (interface && interface != p->interface)
 				dev_info(dev->dev,
 					 "use %s instead of %s\n",
-					  phy_modes(dev->interface),
+					  phy_modes(p->interface),
 					  phy_modes(interface));
 
 			/* enable cpu port */
 			ksz9477_port_setup(dev, i, true);
-			p = &dev->ports[dev->cpu_port];
 			p->vid_member = dev->port_mask;
 			p->on = 1;
 		}
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8d53b12d40a8..d96b7ab6bb15 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -389,6 +389,8 @@ int ksz_switch_register(struct ksz_device *dev,
 {
 	phy_interface_t interface;
 	int ret;
+	struct device_node *port;
+	unsigned int port_num;
 
 	if (dev->pdata)
 		dev->chip_id = dev->pdata->chip_id;
@@ -421,10 +423,19 @@ int ksz_switch_register(struct ksz_device *dev,
 	/* Host port interface will be self detected, or specifically set in
 	 * device tree.
 	 */
+	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
+		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
-			dev->interface = interface;
+			dev->compat_interface = interface;
+		for_each_available_child_of_node(dev->dev->of_node, port) {
+			if (of_property_read_u32(port, "reg", &port_num))
+				continue;
+			if (port_num >= dev->port_cnt)
+				return -EINVAL;
+			of_get_phy_mode(port, &dev->ports[port_num].interface);
+		}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
 	}
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 206838160f49..cf866e48ff66 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -39,6 +39,7 @@ struct ksz_port {
 	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
+	phy_interface_t interface;
 };
 
 struct ksz_device {
@@ -72,7 +73,7 @@ struct ksz_device {
 	int mib_cnt;
 	int mib_port_cnt;
 	int last_port;			/* ports after that not used */
-	phy_interface_t interface;
+	phy_interface_t compat_interface;
 	u32 regs_size;
 	bool phy_errata_9477;
 	bool synclko_125;
-- 
2.20.1

Re: [RESEND PATCH] net: dsa: microchip: look for phy-mode in port nodes

[David Miller]

From: Helmut Grohne <helmut.grohne@intenta.de>
Date: Thu, 20 Aug 2020 08:03:33 +0200

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8d53b12d40a8..d96b7ab6bb15 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -389,6 +389,8 @@ int ksz_switch_register(struct ksz_device *dev,
>  {
>  	phy_interface_t interface;
>  	int ret;
> +	struct device_node *port;
> +	unsigned int port_num;
>  
>  	if (dev->pdata)
>  		dev->chip_id = dev->pdata->chip_id;

Please preserve the reverse christmas tree ordering of local variables here.

[PATCH v2] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 45 insertions(+), 18 deletions(-)

Changes since v1:
 * Preserve the reverse christmas tree ordering of local variables.
   Reported by David Miller.

Reason for resending v1:
 * While Andrew Lunn agreed to the semantic change, he found the
   implementation unnecessarily complex. He suggested going without a
   per-port interface attribute, but that happened to not work out. The
   information of which port will become the cpu port is only realized
   in a later initialization step.

There were no further replies, so here goes a v2 with minimal changes.

Helmut

diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
index 8d19925fc09e..6783cf16ff81 100644
--- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
@@ -116,7 +116,6 @@
 		switch0: ksz8563@0 {
 			compatible = "microchip,ksz8563";
 			reg = <0>;
-			phy-mode = "mii";
 			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
 
 			spi-max-frequency = <500000>;
@@ -140,6 +139,7 @@
 					reg = <2>;
 					label = "cpu";
 					ethernet = <&macb0>;
+					phy-mode = "mii";
 					fixed-link {
 						speed = <100>;
 						full-duplex;
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 8f1d15ea15d9..cae77eafd533 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -932,11 +932,18 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
 	if (cpu_port) {
+		if (!p->interface && dev->compat_interface) {
+			dev_warn(dev->dev,
+				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+				 port);
+			p->interface = dev->compat_interface;
+		}
+
 		/* Configure MII interface for proper network communication. */
 		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
 		data8 &= ~PORT_INTERFACE_TYPE;
 		data8 &= ~PORT_GMII_1GPS_MODE;
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			p->phydev.speed = SPEED_100;
 			break;
@@ -952,11 +959,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		default:
 			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
 			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IN_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_OUT_ENABLE;
 			data8 |= PORT_GMII_1GPS_MODE;
 			data8 |= PORT_INTERFACE_RGMII;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3cb22d149813..89e8934bc60b 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1208,7 +1208,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 
 		/* configure MAC to 1G & RGMII mode */
 		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			ksz9477_set_xmii(dev, 0, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
@@ -1229,11 +1229,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 			ksz9477_set_gbit(dev, true, &data8);
 			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
 			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_EG_ENABLE;
 			p->phydev.speed = SPEED_1000;
 			break;
@@ -1269,23 +1269,31 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			dev->cpu_port = i;
 			dev->host_mask = (1 << dev->cpu_port);
 			dev->port_mask |= dev->host_mask;
+			p = &dev->ports[i];
 
 			/* Read from XMII register to determine host port
 			 * interface.  If set specifically in device tree
 			 * note the difference to help debugging.
 			 */
 			interface = ksz9477_get_interface(dev, i);
-			if (!dev->interface)
-				dev->interface = interface;
-			if (interface && interface != dev->interface)
+			if (!p->interface) {
+				if (dev->compat_interface) {
+					dev_warn(dev->dev,
+						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
+						 i);
+					p->interface = dev->compat_interface;
+				} else {
+					p->interface = interface;
+				}
+			}
+			if (interface && interface != p->interface)
 				dev_info(dev->dev,
 					 "use %s instead of %s\n",
-					  phy_modes(dev->interface),
+					  phy_modes(p->interface),
 					  phy_modes(interface));
 
 			/* enable cpu port */
 			ksz9477_port_setup(dev, i, true);
-			p = &dev->ports[dev->cpu_port];
 			p->vid_member = dev->port_mask;
 			p->on = 1;
 		}
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8d53b12d40a8..8e755b50c9c1 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -388,6 +388,8 @@ int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops)
 {
 	phy_interface_t interface;
+	struct device_node *port;
+	unsigned int port_num;
 	int ret;
 
 	if (dev->pdata)
@@ -421,10 +423,19 @@ int ksz_switch_register(struct ksz_device *dev,
 	/* Host port interface will be self detected, or specifically set in
 	 * device tree.
 	 */
+	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
+		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
-			dev->interface = interface;
+			dev->compat_interface = interface;
+		for_each_available_child_of_node(dev->dev->of_node, port) {
+			if (of_property_read_u32(port, "reg", &port_num))
+				continue;
+			if (port_num >= dev->port_cnt)
+				return -EINVAL;
+			of_get_phy_mode(port, &dev->ports[port_num].interface);
+		}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
 	}
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 206838160f49..cf866e48ff66 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -39,6 +39,7 @@ struct ksz_port {
 	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
+	phy_interface_t interface;
 };
 
 struct ksz_device {
@@ -72,7 +73,7 @@ struct ksz_device {
 	int mib_cnt;
 	int mib_port_cnt;
 	int last_port;			/* ports after that not used */
-	phy_interface_t interface;
+	phy_interface_t compat_interface;
 	u32 regs_size;
 	bool phy_errata_9477;
 	bool synclko_125;
-- 
2.20.1

Re: [PATCH v2] net: dsa: microchip: look for phy-mode in port nodes

[Alexandre Belloni]

On 04/09/2020 10:14:42+0200, Helmut Grohne wrote:
> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.
> 
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
> ---
>  arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
>  drivers/net/dsa/microchip/ksz8795.c    | 17 +++++++++++-----
>  drivers/net/dsa/microchip/ksz9477.c    | 28 +++++++++++++++++---------
>  drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
>  drivers/net/dsa/microchip/ksz_common.h |  3 ++-
>  5 files changed, 45 insertions(+), 18 deletions(-)
> 
> Changes since v1:
>  * Preserve the reverse christmas tree ordering of local variables.
>    Reported by David Miller.
> 
> Reason for resending v1:
>  * While Andrew Lunn agreed to the semantic change, he found the
>    implementation unnecessarily complex. He suggested going without a
>    per-port interface attribute, but that happened to not work out. The
>    information of which port will become the cpu port is only realized
>    in a later initialization step.
> 
> There were no further replies, so here goes a v2 with minimal changes.
> 
> Helmut
> 
> diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
> index 8d19925fc09e..6783cf16ff81 100644
> --- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
> +++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
> @@ -116,7 +116,6 @@
>  		switch0: ksz8563@0 {
>  			compatible = "microchip,ksz8563";
>  			reg = <0>;
> -			phy-mode = "mii";
>  			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
>  
>  			spi-max-frequency = <500000>;
> @@ -140,6 +139,7 @@
>  					reg = <2>;
>  					label = "cpu";
>  					ethernet = <&macb0>;
> +					phy-mode = "mii";
>  					fixed-link {
>  						speed = <100>;
>  						full-duplex;
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 8f1d15ea15d9..cae77eafd533 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -932,11 +932,18 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
>  
>  	if (cpu_port) {
> +		if (!p->interface && dev->compat_interface) {
> +			dev_warn(dev->dev,
> +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +				 port);
> +			p->interface = dev->compat_interface;
> +		}
> +
>  		/* Configure MII interface for proper network communication. */
>  		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
>  		data8 &= ~PORT_INTERFACE_TYPE;
>  		data8 &= ~PORT_GMII_1GPS_MODE;
> -		switch (dev->interface) {
> +		switch (p->interface) {
>  		case PHY_INTERFACE_MODE_MII:
>  			p->phydev.speed = SPEED_100;
>  			break;
> @@ -952,11 +959,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		default:
>  			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
>  			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>  				data8 |= PORT_RGMII_ID_IN_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  				data8 |= PORT_RGMII_ID_OUT_ENABLE;
>  			data8 |= PORT_GMII_1GPS_MODE;
>  			data8 |= PORT_INTERFACE_RGMII;
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3cb22d149813..89e8934bc60b 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1208,7 +1208,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  
>  		/* configure MAC to 1G & RGMII mode */
>  		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
> -		switch (dev->interface) {
> +		switch (p->interface) {
>  		case PHY_INTERFACE_MODE_MII:
>  			ksz9477_set_xmii(dev, 0, &data8);
>  			ksz9477_set_gbit(dev, false, &data8);
> @@ -1229,11 +1229,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  			ksz9477_set_gbit(dev, true, &data8);
>  			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
>  			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>  				data8 |= PORT_RGMII_ID_IG_ENABLE;
> -			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> -			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>  				data8 |= PORT_RGMII_ID_EG_ENABLE;
>  			p->phydev.speed = SPEED_1000;
>  			break;
> @@ -1269,23 +1269,31 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  			dev->cpu_port = i;
>  			dev->host_mask = (1 << dev->cpu_port);
>  			dev->port_mask |= dev->host_mask;
> +			p = &dev->ports[i];
>  
>  			/* Read from XMII register to determine host port
>  			 * interface.  If set specifically in device tree
>  			 * note the difference to help debugging.
>  			 */
>  			interface = ksz9477_get_interface(dev, i);
> -			if (!dev->interface)
> -				dev->interface = interface;
> -			if (interface && interface != dev->interface)
> +			if (!p->interface) {
> +				if (dev->compat_interface) {
> +					dev_warn(dev->dev,
> +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +						 i);
> +					p->interface = dev->compat_interface;
> +				} else {
> +					p->interface = interface;
> +				}
> +			}
> +			if (interface && interface != p->interface)
>  				dev_info(dev->dev,
>  					 "use %s instead of %s\n",
> -					  phy_modes(dev->interface),
> +					  phy_modes(p->interface),
>  					  phy_modes(interface));
>  
>  			/* enable cpu port */
>  			ksz9477_port_setup(dev, i, true);
> -			p = &dev->ports[dev->cpu_port];
>  			p->vid_member = dev->port_mask;
>  			p->on = 1;
>  		}
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8d53b12d40a8..8e755b50c9c1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -388,6 +388,8 @@ int ksz_switch_register(struct ksz_device *dev,
>  			const struct ksz_dev_ops *ops)
>  {
>  	phy_interface_t interface;
> +	struct device_node *port;
> +	unsigned int port_num;
>  	int ret;
>  
>  	if (dev->pdata)
> @@ -421,10 +423,19 @@ int ksz_switch_register(struct ksz_device *dev,
>  	/* Host port interface will be self detected, or specifically set in
>  	 * device tree.
>  	 */
> +	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
> +		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
>  	if (dev->dev->of_node) {
>  		ret = of_get_phy_mode(dev->dev->of_node, &interface);
>  		if (ret == 0)
> -			dev->interface = interface;
> +			dev->compat_interface = interface;
> +		for_each_available_child_of_node(dev->dev->of_node, port) {
> +			if (of_property_read_u32(port, "reg", &port_num))
> +				continue;
> +			if (port_num >= dev->port_cnt)
> +				return -EINVAL;
> +			of_get_phy_mode(port, &dev->ports[port_num].interface);
> +		}
>  		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
>  							 "microchip,synclko-125");
>  	}
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 206838160f49..cf866e48ff66 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -39,6 +39,7 @@ struct ksz_port {
>  	u32 freeze:1;			/* MIB counter freeze is enabled */
>  
>  	struct ksz_port_mib mib;
> +	phy_interface_t interface;
>  };
>  
>  struct ksz_device {
> @@ -72,7 +73,7 @@ struct ksz_device {
>  	int mib_cnt;
>  	int mib_port_cnt;
>  	int last_port;			/* ports after that not used */
> -	phy_interface_t interface;
> +	phy_interface_t compat_interface;
>  	u32 regs_size;
>  	bool phy_errata_9477;
>  	bool synclko_125;
> -- 
> 2.20.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2] net: dsa: microchip: look for phy-mode in port nodes

[Andrew Lunn]

> +			dev_warn(dev->dev,
> +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",

That message seems mangled.

> +			if (!p->interface) {
> +				if (dev->compat_interface) {
> +					dev_warn(dev->dev,
> +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> +						 i);

Same warning again.

     Andrew

Re: [PATCH v2] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Hi Andrew,

On Fri, Sep 04, 2020 at 03:52:55PM +0200, Andrew Lunn wrote:
> > +			dev_warn(dev->dev,
> > +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",

This is inside ksz8795_port_setup.

> That message seems mangled.

I'm not sure that I understand what you are objecting to here.

> > +			if (!p->interface) {
> > +				if (dev->compat_interface) {
> > +					dev_warn(dev->dev,
> > +						 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> > +						 i);

This is inside ksz9477_config_cpu_port.

> Same warning again.

I guess that you believe the warning should only be issued in one place.
The locations affect different chips driven by the same driver. I
considered moving them to a common function, but figured that it was not
worth tearing the code apart. In case a third chip would be supported by
the driver, it would not need the compatibility code. It would start out
using only the correct phy-mode property.

Does that address your concern?

Helmut

Re: [PATCH v2] net: dsa: microchip: look for phy-mode in port nodes

[Andrew Lunn]

On Mon, Sep 07, 2020 at 08:15:33AM +0200, Helmut Grohne wrote:
> Hi Andrew,
> 
> On Fri, Sep 04, 2020 at 03:52:55PM +0200, Andrew Lunn wrote:
> > > +			dev_warn(dev->dev,
> > > +				 "Using legacy switch \"phy-mode\" missing on port %d node. Please update your device tree.\n",
> 
> This is inside ksz8795_port_setup.
> 
> > That message seems mangled.
> 
> I'm not sure that I understand what you are objecting to here.

Hi Helmut

The grammar seems wrong. 

"Using legacy switch \"phy-mode\" because \"phy-mode\" missing from port %d node. "
"Please update your device tree.\n"

	Andrew

[PATCH v3] net: dsa: microchip: look for phy-mode in port nodes

[Helmut Grohne]

Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
property should be specified on port nodes. However, the microchip
drivers read it from the switch node.

Let the driver use the per-port property and fall back to the old
location with a warning.

Fix in-tree users.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/arm/boot/dts/at91-sama5d2_icp.dts |  2 +-
 drivers/net/dsa/microchip/ksz8795.c    | 18 +++++++++++-----
 drivers/net/dsa/microchip/ksz9477.c    | 29 +++++++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.c | 13 +++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  3 ++-
 5 files changed, 47 insertions(+), 18 deletions(-)

Changes since v2:
 * Include Acked-by Alexandre Belloni.
 * Improve deprecation message. Thanks to Andrew Lunn.

Changes since v1:
 * Preserve the reverse christmas tree ordering of local variables.
   Reported by David Miller.

Reason for resending v1:
 * While Andrew Lunn agreed to the semantic change, he found the
   implementation unnecessarily complex. He suggested going without a
   per-port interface attribute, but that happened to not work out. The
   information of which port will become the cpu port is only realized
   in a later initialization step.

Helmut

diff --git a/arch/arm/boot/dts/at91-sama5d2_icp.dts b/arch/arm/boot/dts/at91-sama5d2_icp.dts
index 8d19925fc09e..6783cf16ff81 100644
--- a/arch/arm/boot/dts/at91-sama5d2_icp.dts
+++ b/arch/arm/boot/dts/at91-sama5d2_icp.dts
@@ -116,7 +116,6 @@
 		switch0: ksz8563@0 {
 			compatible = "microchip,ksz8563";
 			reg = <0>;
-			phy-mode = "mii";
 			reset-gpios = <&pioA PIN_PD4 GPIO_ACTIVE_LOW>;
 
 			spi-max-frequency = <500000>;
@@ -140,6 +139,7 @@
 					reg = <2>;
 					label = "cpu";
 					ethernet = <&macb0>;
+					phy-mode = "mii";
 					fixed-link {
 						speed = <100>;
 						full-duplex;
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 8f1d15ea15d9..f7d59d7b2cbe 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -932,11 +932,19 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
 	if (cpu_port) {
+		if (!p->interface && dev->compat_interface) {
+			dev_warn(dev->dev,
+				 "Using legacy switch \"phy-mode\" property, because it is missing on port %d node. "
+				 "Please update your device tree.\n",
+				 port);
+			p->interface = dev->compat_interface;
+		}
+
 		/* Configure MII interface for proper network communication. */
 		ksz_read8(dev, REG_PORT_5_CTRL_6, &data8);
 		data8 &= ~PORT_INTERFACE_TYPE;
 		data8 &= ~PORT_GMII_1GPS_MODE;
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			p->phydev.speed = SPEED_100;
 			break;
@@ -952,11 +960,11 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		default:
 			data8 &= ~PORT_RGMII_ID_IN_ENABLE;
 			data8 &= ~PORT_RGMII_ID_OUT_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IN_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_OUT_ENABLE;
 			data8 |= PORT_GMII_1GPS_MODE;
 			data8 |= PORT_INTERFACE_RGMII;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 3cb22d149813..2f5506ac7d19 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1208,7 +1208,7 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 
 		/* configure MAC to 1G & RGMII mode */
 		ksz_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
-		switch (dev->interface) {
+		switch (p->interface) {
 		case PHY_INTERFACE_MODE_MII:
 			ksz9477_set_xmii(dev, 0, &data8);
 			ksz9477_set_gbit(dev, false, &data8);
@@ -1229,11 +1229,11 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 			ksz9477_set_gbit(dev, true, &data8);
 			data8 &= ~PORT_RGMII_ID_IG_ENABLE;
 			data8 &= ~PORT_RGMII_ID_EG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_RXID)
 				data8 |= PORT_RGMII_ID_IG_ENABLE;
-			if (dev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-			    dev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			if (p->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+			    p->interface == PHY_INTERFACE_MODE_RGMII_TXID)
 				data8 |= PORT_RGMII_ID_EG_ENABLE;
 			p->phydev.speed = SPEED_1000;
 			break;
@@ -1269,23 +1269,32 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			dev->cpu_port = i;
 			dev->host_mask = (1 << dev->cpu_port);
 			dev->port_mask |= dev->host_mask;
+			p = &dev->ports[i];
 
 			/* Read from XMII register to determine host port
 			 * interface.  If set specifically in device tree
 			 * note the difference to help debugging.
 			 */
 			interface = ksz9477_get_interface(dev, i);
-			if (!dev->interface)
-				dev->interface = interface;
-			if (interface && interface != dev->interface)
+			if (!p->interface) {
+				if (dev->compat_interface) {
+					dev_warn(dev->dev,
+						 "Using legacy switch \"phy-mode\" property, because it is missing on port %d node. "
+						 "Please update your device tree.\n",
+						 i);
+					p->interface = dev->compat_interface;
+				} else {
+					p->interface = interface;
+				}
+			}
+			if (interface && interface != p->interface)
 				dev_info(dev->dev,
 					 "use %s instead of %s\n",
-					  phy_modes(dev->interface),
+					  phy_modes(p->interface),
 					  phy_modes(interface));
 
 			/* enable cpu port */
 			ksz9477_port_setup(dev, i, true);
-			p = &dev->ports[dev->cpu_port];
 			p->vid_member = dev->port_mask;
 			p->on = 1;
 		}
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8d53b12d40a8..8e755b50c9c1 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -388,6 +388,8 @@ int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops)
 {
 	phy_interface_t interface;
+	struct device_node *port;
+	unsigned int port_num;
 	int ret;
 
 	if (dev->pdata)
@@ -421,10 +423,19 @@ int ksz_switch_register(struct ksz_device *dev,
 	/* Host port interface will be self detected, or specifically set in
 	 * device tree.
 	 */
+	for (port_num = 0; port_num < dev->port_cnt; ++port_num)
+		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
-			dev->interface = interface;
+			dev->compat_interface = interface;
+		for_each_available_child_of_node(dev->dev->of_node, port) {
+			if (of_property_read_u32(port, "reg", &port_num))
+				continue;
+			if (port_num >= dev->port_cnt)
+				return -EINVAL;
+			of_get_phy_mode(port, &dev->ports[port_num].interface);
+		}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
 	}
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 206838160f49..cf866e48ff66 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -39,6 +39,7 @@ struct ksz_port {
 	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
+	phy_interface_t interface;
 };
 
 struct ksz_device {
@@ -72,7 +73,7 @@ struct ksz_device {
 	int mib_cnt;
 	int mib_port_cnt;
 	int last_port;			/* ports after that not used */
-	phy_interface_t interface;
+	phy_interface_t compat_interface;
 	u32 regs_size;
 	bool phy_errata_9477;
 	bool synclko_125;
-- 
2.20.1

Re: [PATCH v3] net: dsa: microchip: look for phy-mode in port nodes

[David Miller]

From: Helmut Grohne <helmut.grohne@intenta.de>
Date: Tue, 8 Sep 2020 10:01:38 +0200

> Documentation/devicetree/bindings/net/dsa/dsa.txt says that the phy-mode
> property should be specified on port nodes. However, the microchip
> drivers read it from the switch node.
> 
> Let the driver use the per-port property and fall back to the old
> location with a warning.
> 
> Fix in-tree users.
> 
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> Link: https://lore.kernel.org/netdev/20200617082235.GA1523@laureti-dev/
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Applied, thanks.

[PATCH] net: dsa: microchip: really look for phy-mode in port nodes

[Helmut Grohne]

The previous implementation failed to account for the "ports" node. The
actual port nodes are not child nodes of the switch node, but a "ports"
node sits in between.

Fixes: edecfa98f602 ("net: dsa: microchip: look for phy-mode in port nodes")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

I am very sorry that I need to send a fixup. It turned out that my
testing methodology was flawed. When I reintegrated Linus' master
branch, I noticed that it didn't work. It turned out that our hardware
now correctly implements hardware reset. As a consequence, the correct
setting of the phy-mode now became essential to operating the device.

I'm also looking forward to see "net: dsa: microchip: Improve phy mode
message" (from net-next) being merged. That would have helped me spot
this earlier.

Helmut

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8e755b50c9c1..c796d42730ba 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -387,8 +387,8 @@ EXPORT_SYMBOL(ksz_switch_alloc);
 int ksz_switch_register(struct ksz_device *dev,
 			const struct ksz_dev_ops *ops)
 {
+	struct device_node *port, *ports;
 	phy_interface_t interface;
-	struct device_node *port;
 	unsigned int port_num;
 	int ret;
 
@@ -429,13 +429,17 @@ int ksz_switch_register(struct ksz_device *dev,
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
-		for_each_available_child_of_node(dev->dev->of_node, port) {
-			if (of_property_read_u32(port, "reg", &port_num))
-				continue;
-			if (port_num >= dev->port_cnt)
-				return -EINVAL;
-			of_get_phy_mode(port, &dev->ports[port_num].interface);
-		}
+		ports = of_get_child_by_name(dev->dev->of_node, "ports");
+		if (ports)
+			for_each_available_child_of_node(ports, port) {
+				if (of_property_read_u32(port, "reg",
+							 &port_num))
+					continue;
+				if (port_num >= dev->port_cnt)
+					return -EINVAL;
+				of_get_phy_mode(port,
+						&dev->ports[port_num].interface);
+			}
 		dev->synclko_125 = of_property_read_bool(dev->dev->of_node,
 							 "microchip,synclko-125");
 	}
-- 
2.20.1

Re: [PATCH] net: dsa: microchip: really look for phy-mode in port nodes

[David Miller]

From: Helmut Grohne <helmut.grohne@intenta.de>
Date: Thu, 24 Sep 2020 10:37:47 +0200

> The previous implementation failed to account for the "ports" node. The
> actual port nodes are not child nodes of the switch node, but a "ports"
> node sits in between.
> 
> Fixes: edecfa98f602 ("net: dsa: microchip: look for phy-mode in port nodes")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>

Applied and queued up for -stable.

> I am very sorry that I need to send a fixup. It turned out that my
> testing methodology was flawed. When I reintegrated Linus' master
> branch, I noticed that it didn't work.

You should be testing against the 'net' GIT tree, not Linus's master
branch.