| Message ID | ZDgOLHw1IkmWVU79@lenoch (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | staging: octeon: Convert to use phylink | expand |
> num_interfaces = cvmx_helper_get_number_of_interfaces(); > for (interface = 0; interface < num_interfaces; interface++) { > + int num_ports, port_index; > + const struct net_device_ops *ops; > + const char *name; > + phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA; > cvmx_helper_interface_mode_t imode = > - cvmx_helper_interface_get_mode(interface); > - int num_ports = cvmx_helper_ports_on_interface(interface); > - int port_index; > + cvmx_helper_interface_get_mode(interface); > + > + switch (imode) { > + case CVMX_HELPER_INTERFACE_MODE_NPI: > + ops = &cvm_oct_npi_netdev_ops; > + name = "npi%d"; In general, the kernel does not give the interface names other than ethX. userspace can rename them, e.g. systemd with its persistent names. So as part of getting this driver out of staging, i would throw this naming code away. > + num_ports = cvmx_helper_ports_on_interface(interface); > for (port_index = 0, > port = cvmx_helper_get_ipd_port(interface, 0); > port < cvmx_helper_get_ipd_port(interface, num_ports); > port_index++, port++) { > struct octeon_ethernet *priv; > struct net_device *dev = > - alloc_etherdev(sizeof(struct octeon_ethernet)); > + alloc_etherdev(sizeof(struct octeon_ethernet)); Please try to avoid white space changed. Put such white space changes into a patch of their own, with a commit message saying it just contains whitespace cleanup. > if (!dev) { > pr_err("Failed to allocate ethernet device for port %d\n", > port); > @@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev) > priv->port = port; > priv->queue = cvmx_pko_get_base_queue(priv->port); > priv->fau = fau - cvmx_pko_get_num_queues(port) * 4; > - priv->phy_mode = PHY_INTERFACE_MODE_NA; > + priv->phy_mode = phy_mode; You should be getting phy_mode from DT. Ideally, you want lots of small patches which are obviously correct. So i would try to break this up into smaller changes. I also wounder if you are addresses issues in the correct order. This driver is in staging for a reason. It needs a lot of work. You might be better off first cleaning it up. And then consider moving it to phylink. Andrew
Hi Andrew, On Thu, Apr 13, 2023 at 06:12:28PM +0200, Andrew Lunn wrote: > > num_interfaces = cvmx_helper_get_number_of_interfaces(); > > for (interface = 0; interface < num_interfaces; interface++) { > > + int num_ports, port_index; > > + const struct net_device_ops *ops; > > + const char *name; > > + phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA; > > cvmx_helper_interface_mode_t imode = > > - cvmx_helper_interface_get_mode(interface); > > - int num_ports = cvmx_helper_ports_on_interface(interface); > > - int port_index; > > + cvmx_helper_interface_get_mode(interface); > > + > > + switch (imode) { > > + case CVMX_HELPER_INTERFACE_MODE_NPI: > > + ops = &cvm_oct_npi_netdev_ops; > > + name = "npi%d"; > > In general, the kernel does not give the interface names other than > ethX. userspace can rename them, e.g. systemd with its persistent > names. So as part of getting this driver out of staging, i would throw > this naming code away. That would break all userspace (which is often running vendor's kernel). But since driver is in staging and noone cares about vendor's kernel I guess it is okay... > > + num_ports = cvmx_helper_ports_on_interface(interface); > > for (port_index = 0, > > port = cvmx_helper_get_ipd_port(interface, 0); > > port < cvmx_helper_get_ipd_port(interface, num_ports); > > port_index++, port++) { > > struct octeon_ethernet *priv; > > struct net_device *dev = > > - alloc_etherdev(sizeof(struct octeon_ethernet)); > > + alloc_etherdev(sizeof(struct octeon_ethernet)); > > Please try to avoid white space changed. Put such white space changes > into a patch of their own, with a commit message saying it just > contains whitespace cleanup. Sorry, I overlooked this. > > if (!dev) { > > pr_err("Failed to allocate ethernet device for port %d\n", > > port); > > @@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev) > > priv->port = port; > > priv->queue = cvmx_pko_get_base_queue(priv->port); > > priv->fau = fau - cvmx_pko_get_num_queues(port) * 4; > > - priv->phy_mode = PHY_INTERFACE_MODE_NA; > > + priv->phy_mode = phy_mode; > > You should be getting phy_mode from DT. > > Ideally, you want lots of small patches which are obviously > correct. So i would try to break this up into smaller changes. > > I also wounder if you are addresses issues in the correct order. This > driver is in staging for a reason. It needs a lot of work. You might > be better off first cleaning it up. And then consider moving it to > phylink. I was asking this question myself and then came to this: Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you? > Andrew Thank you, ladis
> I was asking this question myself and then came to this: > Converting driver to phylink makes separating different macs easier as > this driver is splitted between staging and arch/mips/cavium-octeon/executive/ > However I'll provide changes spotted previously as separate preparational > patches. Would that work for you? Is you end goal to get this out of staging? phylib vs phylink is not a reason to keep it in staging. It just seems odd to be adding new features to a staging driver. As a bit of a "carrot and stick" maybe we should say you cannot add new features until it is ready to move out of staging? But staging is not my usual domain. Andrew
On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote: > > I was asking this question myself and then came to this: > > Converting driver to phylink makes separating different macs easier as > > this driver is splitted between staging and arch/mips/cavium-octeon/executive/ > > However I'll provide changes spotted previously as separate preparational > > patches. Would that work for you? > > Is you end goal to get this out of staging? phylib vs phylink is not a > reason to keep it in staging. I agree. However it is a way to move it out as once phylink_mac_ops for each mac gets implemented, most code from arch/mips/cavium-octeon/executive could then be moved into respective phylink_mac_op, so driver become self contained. > It just seems odd to be adding new features to a staging driver. As a > bit of a "carrot and stick" maybe we should say you cannot add new > features until it is ready to move out of staging? Ok. I will continue to add cleanup patches before phylink support and we'll see how far we can get. That oddity has pretty simple reasoning: mainline kernel should be useable instead of vendor's solution (which does dirty SFP tricks from userpace and also supports AGL interface which is missing in staging driver). Without this, it will end as a spare time activity with a low priority. See this thread for context: https://lore.kernel.org/linux-mips/Y6rsbaT0l5cNBGbu@lenoch/ > But staging is not my usual domain. Network drivers are not my usual domain, but I'll try to deal with that :) > Andrew Thanks for the patience, ladis
+David Daney, Steven J. Hill On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote: > > I was asking this question myself and then came to this: > > Converting driver to phylink makes separating different macs easier as > > this driver is splitted between staging and arch/mips/cavium-octeon/executive/ > > However I'll provide changes spotted previously as separate preparational > > patches. Would that work for you? > > Is you end goal to get this out of staging? phylib vs phylink is not a > reason to keep it in staging. A side question here: Once upon the time there was an "Cavium OCTEON-III network driver" patchset floating around, reached v9, at least that's the last one I was able to find [1]. Prerequisites were intended to go via linux-mips tree [2]. I was unable to find any further traces and this driver is not even in Cavium's 5.4 vendor tree. What happened with that? It is for different hardware, but some design decisions might be interesting here as well. > It just seems odd to be adding new features to a staging driver. As a > bit of a "carrot and stick" maybe we should say you cannot add new > features until it is ready to move out of staging? > > But staging is not my usual domain. > > Andrew [1] https://www.spinics.net/lists/netdev/msg498700.html [2] https://www.spinics.net/lists/netdev/msg498696.html
On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote: > > I was asking this question myself and then came to this: > > Converting driver to phylink makes separating different macs easier as > > this driver is splitted between staging and arch/mips/cavium-octeon/executive/ > > However I'll provide changes spotted previously as separate preparational > > patches. Would that work for you? > > Is you end goal to get this out of staging? phylib vs phylink is not a > reason to keep it in staging. > > It just seems odd to be adding new features to a staging driver. As a > bit of a "carrot and stick" maybe we should say you cannot add new > features until it is ready to move out of staging? We already have that rule. But I don't know anything about phy vs phylink... regards, dan carpenter
On Mon, Apr 17, 2023 at 11:37:30AM +0300, Dan Carpenter wrote: > On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote: > > > I was asking this question myself and then came to this: > > > Converting driver to phylink makes separating different macs easier as > > > this driver is splitted between staging and arch/mips/cavium-octeon/executive/ > > > However I'll provide changes spotted previously as separate preparational > > > patches. Would that work for you? > > > > Is you end goal to get this out of staging? phylib vs phylink is not a > > reason to keep it in staging. > > > > It just seems odd to be adding new features to a staging driver. As a > > bit of a "carrot and stick" maybe we should say you cannot add new > > features until it is ready to move out of staging? > > We already have that rule. But I don't know anything about phy vs > phylink... Let me elaborate here a bit then. Current Octeon driver comes from Cavium's vendor kernel tree. Cavium started this about two decades ago based on their own ideas and tries to bend kernel interfaces around them. Driver is based aroud Packet Input Processing/Input Packet Data (PIP/IPD) units which can connect their data streams to various interfaces. SGMII/1000BASE-X/QSGMII and RGMII are just two of them. Currently driver iterates over all interfaces and all ports to bind interfaces to PIP/IPD. There is a lot of code deciding which interfaces/ports exits on given Octeon SoC, see arch/mips/cavium-octeon/executive/ Driver code then calls those helpers with interface/port aguments and they do the magic using switches deciding what to do based on interface type. I'm proposing to leave all that trickery behind and just follow what's written in device tree, so each I/O interface ends up as a driver with its own mac ops. While it is possible to implement that as private mac ops as some other drivers do, I think it is more convenient to use phylink_mac_ops. In case I'm missing something or I'm wrong with analysis, please let me know. Thanks, ladis
> I'm proposing to leave all that trickery behind and just follow what's > written in device tree, so each I/O interface ends up as a driver > with its own mac ops. While it is possible to implement that as > private mac ops as some other drivers do, I think it is more > convenient to use phylink_mac_ops. > > In case I'm missing something or I'm wrong with analysis, please let > me know. It is a reasonable solution, and does help clean up some of the mess keeping it in staging. But as Maintainers, we also have to consider, are you just going to add the cleanup you need in order that phylink supports SFPs, which is what you seem most interesting in. And then stop the cleanup. Despite the code being in staging, it is good enough for you. So converting to phylink, with the existing feature set, is fine. Maybe adding SFP support is a new feature, and we might consider not accepting such patches until the driver has made it out of staging? Andrew
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c index 949ef51bf896..466d43a71d34 100644 --- a/drivers/staging/octeon/ethernet.c +++ b/drivers/staging/octeon/ethernet.c @@ -799,18 +799,63 @@ static int cvm_oct_probe(struct platform_device *pdev) num_interfaces = cvmx_helper_get_number_of_interfaces(); for (interface = 0; interface < num_interfaces; interface++) { + int num_ports, port_index; + const struct net_device_ops *ops; + const char *name; + phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA; cvmx_helper_interface_mode_t imode = - cvmx_helper_interface_get_mode(interface); - int num_ports = cvmx_helper_ports_on_interface(interface); - int port_index; + cvmx_helper_interface_get_mode(interface); + + switch (imode) { + case CVMX_HELPER_INTERFACE_MODE_NPI: + ops = &cvm_oct_npi_netdev_ops; + name = "npi%d"; + break; + + case CVMX_HELPER_INTERFACE_MODE_XAUI: + ops = &cvm_oct_xaui_netdev_ops; + name = "xaui%d"; + break; + + case CVMX_HELPER_INTERFACE_MODE_LOOP: + ops = &cvm_oct_npi_netdev_ops; + name = "loop%d"; + break; + + case CVMX_HELPER_INTERFACE_MODE_SGMII: + ops = &cvm_oct_sgmii_netdev_ops; + name = "eth%d"; + phy_mode = PHY_INTERFACE_MODE_SGMII; + break; + + case CVMX_HELPER_INTERFACE_MODE_SPI: + ops = &cvm_oct_spi_netdev_ops; + name = "spi%d"; + break; + + case CVMX_HELPER_INTERFACE_MODE_GMII: + ops = &cvm_oct_rgmii_netdev_ops; + name = "eth%d"; + phy_mode = PHY_INTERFACE_MODE_GMII; + break; + case CVMX_HELPER_INTERFACE_MODE_RGMII: + ops = &cvm_oct_rgmii_netdev_ops; + name = "eth%d"; + break; + + default: + continue; + } + + num_ports = cvmx_helper_ports_on_interface(interface); for (port_index = 0, port = cvmx_helper_get_ipd_port(interface, 0); port < cvmx_helper_get_ipd_port(interface, num_ports); port_index++, port++) { struct octeon_ethernet *priv; struct net_device *dev = - alloc_etherdev(sizeof(struct octeon_ethernet)); + alloc_etherdev(sizeof(struct octeon_ethernet)); if (!dev) { pr_err("Failed to allocate ethernet device for port %d\n", port); @@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev) priv->port = port; priv->queue = cvmx_pko_get_base_queue(priv->port); priv->fau = fau - cvmx_pko_get_num_queues(port) * 4; - priv->phy_mode = PHY_INTERFACE_MODE_NA; + priv->phy_mode = phy_mode; + if (imode == CVMX_HELPER_INTERFACE_MODE_RGMII) + cvm_set_rgmii_delay(priv, interface, + port_index); + dev->netdev_ops = ops; + strscpy(dev->name, name, sizeof(dev->name)); for (qos = 0; qos < 16; qos++) skb_queue_head_init(&priv->tx_free_list[qos]); for (qos = 0; qos < cvmx_pko_get_num_queues(port); @@ -839,64 +889,16 @@ static int cvm_oct_probe(struct platform_device *pdev) dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead; dev->max_mtu = OCTEON_MAX_MTU - mtu_overhead; - switch (priv->imode) { - /* These types don't support ports to IPD/PKO */ - case CVMX_HELPER_INTERFACE_MODE_DISABLED: - case CVMX_HELPER_INTERFACE_MODE_PCIE: - case CVMX_HELPER_INTERFACE_MODE_PICMG: - break; - - case CVMX_HELPER_INTERFACE_MODE_NPI: - dev->netdev_ops = &cvm_oct_npi_netdev_ops; - strscpy(dev->name, "npi%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_XAUI: - dev->netdev_ops = &cvm_oct_xaui_netdev_ops; - strscpy(dev->name, "xaui%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_LOOP: - dev->netdev_ops = &cvm_oct_npi_netdev_ops; - strscpy(dev->name, "loop%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_SGMII: - priv->phy_mode = PHY_INTERFACE_MODE_SGMII; - dev->netdev_ops = &cvm_oct_sgmii_netdev_ops; - strscpy(dev->name, "eth%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_SPI: - dev->netdev_ops = &cvm_oct_spi_netdev_ops; - strscpy(dev->name, "spi%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_GMII: - priv->phy_mode = PHY_INTERFACE_MODE_GMII; - dev->netdev_ops = &cvm_oct_rgmii_netdev_ops; - strscpy(dev->name, "eth%d", sizeof(dev->name)); - break; - - case CVMX_HELPER_INTERFACE_MODE_RGMII: - dev->netdev_ops = &cvm_oct_rgmii_netdev_ops; - strscpy(dev->name, "eth%d", sizeof(dev->name)); - cvm_set_rgmii_delay(priv, interface, - port_index); - break; - } - if (priv->of_node && of_phy_is_fixed_link(priv->of_node)) { if (of_phy_register_fixed_link(priv->of_node)) { netdev_err(dev, "Failed to register fixed link for interface %d, port %d\n", interface, priv->port); - dev->netdev_ops = NULL; + free_netdev(dev); + continue; } } - if (!dev->netdev_ops) { - free_netdev(dev); - } else if (register_netdev(dev) < 0) { + if (register_netdev(dev) < 0) { pr_err("Failed to register ethernet device for interface %d, port %d\n", interface, priv->port); free_netdev(dev);