Message ID | 20200714120827.GA7939@laureti-dev |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: dsa: microchip: look for phy-mode in port nodes | expand |
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
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
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
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
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
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;
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(-)