| Message ID | 20250328133544.4149716-1-lukma@denx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: mtip: Add support for MTIP imx287 L2 switch driver | expand |
On Fri, 28 Mar 2025 14:35:40 +0100 Lukasz Majewski wrote: > This patch series adds support for More Than IP's L2 switch driver embedded > in some NXP's SoCs. ## Form letter - net-next-closed Linus already pulled net-next material v6.15 and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after Apr 7th. RFC patches sent for review only are obviously welcome at any time. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
Hi Jakub, > On Fri, 28 Mar 2025 14:35:40 +0100 Lukasz Majewski wrote: > > This patch series adds support for More Than IP's L2 switch driver > > embedded in some NXP's SoCs. > > ## Form letter - net-next-closed > > Linus already pulled net-next material v6.15 and therefore net-next > is closed for new drivers, features, code refactoring and > optimizations. We are currently accepting bug fixes only. > > Please repost when net-next reopens after Apr 7th. > :-/ > RFC patches sent for review only are obviously welcome at any time. > I hope, that I will receive some feedback regarding this driver, so I can repost (hopefully) final version at 07.04.2025. > See: > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 28/03/2025 14:35, Lukasz Majewski wrote: > + > +static void mtip_mii_unregister(struct switch_enet_private *fep) > +{ > + mdiobus_unregister(fep->mii_bus); > + mdiobus_free(fep->mii_bus); > +} > + > +static const struct fec_devinfo fec_imx28_l2switch_info = { > + .quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO, > +}; > + > +static struct platform_device_id pdev_id = { That's const. > + .name = "imx28-l2switch", > + .driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info, > +}; > + > +static int __init mtip_sw_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct switch_enet_private *fep; > + struct fec_devinfo *dev_info; > + struct switch_t *fecp; > + int ret; > + > + fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL); > + if (!fep) > + return -ENOMEM; > + > + pdev->id_entry = &pdev_id; Hm? This is some odd pattern. You are supposed to use OF table and get matched by it, not populate some custom/odd handling of platform tables. > + > + dev_info = (struct fec_devinfo *)pdev->id_entry->driver_data; I did not notice it before, but that's a no - you cannot drop the cast. Driver data is always const. > + if (dev_info) > + fep->quirks = dev_info->quirks; > + > + fep->pdev = pdev; > + platform_set_drvdata(pdev, fep); > + > + fep->enet_addr = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(fep->enet_addr)) > + return PTR_ERR(fep->enet_addr); > + > + fep->irq = platform_get_irq(pdev, 0); > + if (fep->irq < 0) > + return fep->irq; > + > + ret = mtip_parse_of(fep, np); > + if (ret < 0) { > + dev_err(&pdev->dev, "%s: OF parse error (%d)!\n", __func__, > + ret); > + return ret; > + } > + > + /* Create an Ethernet device instance. > + * The switch lookup address memory starts at 0x800FC000 > + */ > + fep->hwp_enet = fep->enet_addr; > + fecp = (struct switch_t *)(fep->enet_addr + ENET_SWI_PHYS_ADDR_OFFSET); > + > + fep->hwp = fecp; > + fep->hwentry = (struct mtip_addr_table_t *) > + ((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET); > + > + ret = devm_regulator_get_enable_optional(&pdev->dev, "phy"); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Unable to get and enable 'phy'\n"); > + > + fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg"); > + if (IS_ERR(fep->clk_ipg)) > + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ipg), > + "Unable to acquire 'ipg' clock\n"); > + > + fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb"); > + if (IS_ERR(fep->clk_ahb)) > + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ahb), > + "Unable to acquire 'ahb' clock\n"); > + > + fep->clk_enet_out = devm_clk_get_optional_enabled(&pdev->dev, > + "enet_out"); > + if (IS_ERR(fep->clk_enet_out)) > + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_enet_out), > + "Unable to acquire 'enet_out' clock\n"); > + > + spin_lock_init(&fep->learn_lock); > + spin_lock_init(&fep->hw_lock); > + spin_lock_init(&fep->mii_lock); > + > + ret = devm_request_irq(&pdev->dev, fep->irq, mtip_interrupt, 0, > + "mtip_l2sw", fep); > + if (ret) > + return dev_err_probe(&pdev->dev, fep->irq, > + "Could not alloc IRQ\n"); > + > + ret = mtip_register_notifiers(fep); > + if (ret) > + return ret; > + > + ret = mtip_ndev_init(fep); > + if (ret) { > + dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n", > + __func__, ret); > + goto ndev_init_err; > + } > + > + ret = mtip_switch_dma_init(fep); > + if (ret) { > + dev_err(&pdev->dev, "%s: ethernet switch init fail (%d)!\n", > + __func__, ret); > + goto dma_init_err; > + } > + > + ret = mtip_mii_init(fep, pdev); > + if (ret) { > + dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__, > + ret); > + goto mii_init_err; > + } > + /* setup timer for learning aging function */ > + timer_setup(&fep->timer_aging, mtip_aging_timer, 0); > + mod_timer(&fep->timer_aging, > + jiffies + msecs_to_jiffies(LEARNING_AGING_INTERVAL)); > + > + fep->task = kthread_run(mtip_sw_learning, fep, "mtip_l2sw_learning"); > + if (IS_ERR(fep->task)) { > + ret = PTR_ERR(fep->task); > + dev_err(&pdev->dev, "%s: learning kthread_run error (%d)!\n", > + __func__, ret); > + goto task_learning_err; > + } > + > + /* setup MII interface for external switch ports*/ > + mtip_enet_init(fep, 1); > + mtip_enet_init(fep, 2); > + > + return 0; > + > + task_learning_err: > + del_timer(&fep->timer_aging); > + mtip_mii_unregister(fep); > + mii_init_err: > + dma_init_err: > + mtip_ndev_cleanup(fep); > + ndev_init_err: > + mtip_unregister_notifiers(fep); > + > + return ret; > +} > + > +static void mtip_sw_remove(struct platform_device *pdev) > +{ > + struct switch_enet_private *fep = platform_get_drvdata(pdev); > + > + mtip_unregister_notifiers(fep); > + mtip_ndev_cleanup(fep); > + > + mtip_mii_remove(fep); > + > + kthread_stop(fep->task); > + del_timer(&fep->timer_aging); > + platform_set_drvdata(pdev, NULL); > + > + kfree(fep); > +} > + > +static const struct of_device_id mtipl2_of_match[] = { > + { .compatible = "nxp,imx287-mtip-switch", }, > + { /* sentinel */ } > +}; Missing module device table. > + > +static struct platform_driver mtipl2plat_driver = { > + .driver = { > + .name = "mtipl2sw", > + .of_match_table = mtipl2_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = mtip_sw_probe, > + .remove_new = mtip_sw_remove, > +}; > + > +module_platform_driver(mtipl2plat_driver); > +MODULE_AUTHOR("Lukasz Majewski <lukma@denx.de>"); > +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch"); > +MODULE_VERSION(VERSION); What is the point of paralell versioning with the kernel? Are you going to keep this updated or - just like in other cases - it will stay always theh same. Look for example at net/bridge/br.c or some other files - they are always the same even if driver changed significantly. BTW, this would be 1.0, not 1.4. Your out of tree versioning does not matter. > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:mtipl2sw"); You should not need MODULE_ALIAS() in normal cases. If you need it, usually it means your device ID table is wrong (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute for incomplete ID table. Best regards, Krzysztof
> +static bool bridge_offload; > +module_param(bridge_offload, bool, 0644); /* Allow setting by root on boot */ > +MODULE_PARM_DESC(bridge_offload, "L2 switch offload mode enable:1, disable:0"); Please drop. module parameters are not liked. In Linux, ports of a switch always starting in isolated mode, and userspace needs to add them to the same bridge. > + > +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb, > + struct net_device *dev); > +static void mtip_switch_tx(struct net_device *dev); > +static int mtip_switch_rx(struct net_device *dev, int budget, int *port); > +static void mtip_set_multicast_list(struct net_device *dev); > +static void mtip_switch_restart(struct net_device *dev, int duplex0, > + int duplex1); Forwards references are not like. Put the functions in the correct order so they are not needed. > +/* Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1. > + * It omits the final shift in of 8 zeroes a "normal" CRC would do > + * (getting the remainder). > + * > + * Examples (hexadecimal values):<br> > + * 10-11-12-13-14-15 => CRC=0xc2 > + * 10-11-cc-dd-ee-00 => CRC=0xe6 > + * > + * param: pmacaddress > + * A 6-byte array with the MAC address. > + * The first byte is the first byte transmitted > + * return The 8-bit CRC in bits 7:0 > + */ > +static int crc8_calc(unsigned char *pmacaddress) > +{ > + /* byte index */ > + int byt; > + /* bit index */ > + int bit; > + int inval; > + int crc; Reverse Christmas tree. Please look through the whole driver and fix it up. > +/* updates MAC address lookup table with a static entry > + * Searches if the MAC address is already there in the block and replaces > + * the older entry with new one. If MAC address is not there then puts a > + * new entry in the first empty slot available in the block > + * > + * mac_addr Pointer to the array containing MAC address to > + * be put as static entry > + * port Port bitmask numbers to be added in static entry, > + * valid values are 1-7 > + * priority The priority for the static entry in table > + * > + * return 0 for a successful update else -1 when no slot available It would be nice to turn this into proper kerneldoc. It is not too far away at the moment. Also, return a proper error code not -1. ENOSPC? > +static int mtip_update_atable_dynamic1(unsigned long write_lo, > + unsigned long write_hi, int block_index, > + unsigned int port, > + unsigned int curr_time, > + struct switch_enet_private *fep) It would be good to document the return value, because it is not the usual 0 success or negative error code. > +static const struct net_device_ops mtip_netdev_ops; more forward declarations. > +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev) > +{ > + if (ndev->netdev_ops == &mtip_netdev_ops) > + return netdev_priv(ndev); > + > + return NULL; > +} I _think_ the return value is not actually used. So maybe 0 or -ENODEV? > +static int esw_mac_addr_static(struct switch_enet_private *fep) > +{ > + int i; > + > + for (i = 0; i < SWITCH_EPORT_NUMBER; i++) { > + if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) { Is that possible? This is the interfaces own MAC address? If it is not valid, the probe should of failed. > + mtip_update_atable_static((unsigned char *) > + fep->ndev[i]->dev_addr, > + 7, 7, fep); > + } else { > + dev_err(&fep->pdev->dev, > + "Can not add mac address %pM to switch!\n", > + fep->ndev[i]->dev_addr); > + return -EFAULT; > + } > + } > + > + return 0; > +} > + > +static void mtip_print_link_status(struct phy_device *phydev) > +{ > + if (phydev->link) > + netdev_info(phydev->attached_dev, > + "Link is Up - %s/%s - flow control %s\n", > + phy_speed_to_str(phydev->speed), > + phy_duplex_to_str(phydev->duplex), > + phydev->pause ? "rx/tx" : "off"); > + else > + netdev_info(phydev->attached_dev, "Link is Down\n"); > +} phy_print_status() > +static void mtip_adjust_link(struct net_device *dev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(dev); > + struct switch_enet_private *fep = priv->fep; > + struct phy_device *phy_dev; > + int status_change = 0, idx; > + unsigned long flags; > + > + spin_lock_irqsave(&fep->hw_lock, flags); > + > + idx = priv->portnum - 1; > + phy_dev = fep->phy_dev[idx]; > + > + /* Prevent a state halted on mii error */ > + if (fep->mii_timeout && phy_dev->state == PHY_HALTED) { > + phy_dev->state = PHY_UP; > + goto spin_unlock; > + } A MAC driver should not be playing around with the internal state of phylib. > +static int mtip_mii_probe(struct net_device *dev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(dev); > + struct switch_enet_private *fep = priv->fep; > + int port_idx = priv->portnum - 1; > + struct phy_device *phy_dev = NULL; > + > + if (fep->phy_np[port_idx]) { > + phy_dev = of_phy_connect(dev, fep->phy_np[port_idx], > + &mtip_adjust_link, 0, > + fep->phy_interface[port_idx]); > + if (!phy_dev) { > + netdev_err(dev, "Unable to connect to phy\n"); > + return -ENODEV; > + } > + } > + > + phy_set_max_speed(phy_dev, 100); > + fep->phy_dev[port_idx] = phy_dev; > + fep->link[port_idx] = 0; > + fep->full_duplex[port_idx] = 0; > + > + dev_info(&dev->dev, > + "MTIP PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n", > + fep->phy_dev[port_idx]->drv->name, > + phydev_name(fep->phy_dev[port_idx]), > + fep->phy_dev[port_idx]->irq); phylib already prints something like that. > +static int mtip_mdiobus_reset(struct mii_bus *bus) > +{ > + if (!bus || !bus->reset_gpiod) { > + dev_err(&bus->dev, "Reset GPIO pin not provided!\n"); > + return -EINVAL; > + } > + > + gpiod_set_value_cansleep(bus->reset_gpiod, 1); > + > + /* Extra time to allow: > + * 1. GPIO RESET pin go high to prevent situation where its value is > + * "LOW" as it is NOT configured. > + * 2. The ENET CLK to stabilize before GPIO RESET is asserted > + */ > + usleep_range(200, 300); > + > + gpiod_set_value_cansleep(bus->reset_gpiod, 0); > + usleep_range(bus->reset_delay_us, bus->reset_delay_us + 1000); > + gpiod_set_value_cansleep(bus->reset_gpiod, 1); > + > + if (bus->reset_post_delay_us > 0) > + usleep_range(bus->reset_post_delay_us, > + bus->reset_post_delay_us + 1000); > + > + return 0; > +} What is wrong with the core code __mdiobus_register() which does the bus reset. > +static void mtip_get_drvinfo(struct net_device *dev, > + struct ethtool_drvinfo *info) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(dev); > + struct switch_enet_private *fep = priv->fep; > + > + strscpy(info->driver, fep->pdev->dev.driver->name, > + sizeof(info->driver)); > + strscpy(info->version, VERSION, sizeof(info->version)); Leave this empty, so you get the git hash of the kernel. > +static void mtip_ndev_setup(struct net_device *dev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(dev); > + > + ether_setup(dev); That is pretty unusual > + dev->ethtool_ops = &mtip_ethtool_ops; > + dev->netdev_ops = &mtip_netdev_ops; > + > + memset(priv, 0, sizeof(struct mtip_ndev_priv)); priv should already be zero.... > +static int mtip_ndev_init(struct switch_enet_private *fep) > +{ > + struct mtip_ndev_priv *priv; > + int i, ret = 0; > + > + for (i = 0; i < SWITCH_EPORT_NUMBER; i++) { > + fep->ndev[i] = alloc_netdev(sizeof(struct mtip_ndev_priv), > + fep->ndev_name[i], NET_NAME_USER, > + mtip_ndev_setup); This explains the ether_setup(). It would be more normal to pass ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here. > + if (!fep->ndev[i]) { > + ret = -1; -ENOMEM? > + break; > + } > + > + priv = netdev_priv(fep->ndev[i]); > + priv->fep = fep; > + priv->portnum = i + 1; > + fep->ndev[i]->irq = fep->irq; > + > + ret = mtip_setup_mac(fep->ndev[i]); > + if (ret) { > + dev_err(&fep->ndev[i]->dev, > + "%s: ndev %s MAC setup err: %d\n", > + __func__, fep->ndev[i]->name, ret); > + break; > + } > + > + ret = register_netdev(fep->ndev[i]); > + if (ret) { > + dev_err(&fep->ndev[i]->dev, > + "%s: ndev %s register err: %d\n", __func__, > + fep->ndev[i]->name, ret); > + break; > + } > + dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2 switch %pM\n", > + fep->ndev[i]->name, fep->ndev[i]->dev_addr); I would drop this. A driver is normally silent unless things go wrong. > + } > + > + if (ret) > + mtip_ndev_cleanup(fep); > + > + return 0; return ret? > +static int mtip_ndev_port_link(struct net_device *ndev, > + struct net_device *br_ndev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(ndev); > + struct switch_enet_private *fep = priv->fep; > + > + dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n", > + __func__, ndev->name, br_ndev->name, (unsigned int)fep); > + > + /* Check if MTIP switch is already enabled */ > + if (!fep->br_offload) { > + if (!priv->master_dev) > + priv->master_dev = br_ndev; It needs to be a little bit more complex than that, because the two ports could be assigned to two different bridges. You should only enable hardware bridging if they are a member of the same bridge. Andrew
Hi Andrew, > > +static bool bridge_offload; > > +module_param(bridge_offload, bool, 0644); /* Allow setting by root > > on boot */ +MODULE_PARM_DESC(bridge_offload, "L2 switch offload > > mode enable:1, disable:0"); > > Please drop. module parameters are not liked. > Ok. > In Linux, ports of a switch always starting in isolated mode, and > userspace needs to add them to the same bridge. Ok. > > > + > > +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb, > > + struct net_device *dev); > > +static void mtip_switch_tx(struct net_device *dev); > > +static int mtip_switch_rx(struct net_device *dev, int budget, int > > *port); +static void mtip_set_multicast_list(struct net_device > > *dev); +static void mtip_switch_restart(struct net_device *dev, int > > duplex0, > > + int duplex1); > > Forwards references are not like. Put the functions in the correct > order so they are not needed. Ok. > > > +/* Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1. > > + * It omits the final shift in of 8 zeroes a "normal" CRC would do > > + * (getting the remainder). > > + * > > + * Examples (hexadecimal values):<br> > > + * 10-11-12-13-14-15 => CRC=0xc2 > > + * 10-11-cc-dd-ee-00 => CRC=0xe6 > > + * > > + * param: pmacaddress > > + * A 6-byte array with the MAC address. > > + * The first byte is the first byte transmitted > > + * return The 8-bit CRC in bits 7:0 > > + */ > > +static int crc8_calc(unsigned char *pmacaddress) > > +{ > > + /* byte index */ > > + int byt; > > + /* bit index */ > > + int bit; > > + int inval; > > + int crc; > > Reverse Christmas tree. Please look through the whole driver and fix > it up. Ok. > > > +/* updates MAC address lookup table with a static entry > > + * Searches if the MAC address is already there in the block and > > replaces > > + * the older entry with new one. If MAC address is not there then > > puts a > > + * new entry in the first empty slot available in the block > > + * > > + * mac_addr Pointer to the array containing MAC address to > > + * be put as static entry > > + * port Port bitmask numbers to be added in static entry, > > + * valid values are 1-7 > > + * priority The priority for the static entry in table > > + * > > + * return 0 for a successful update else -1 when no slot > > available > > It would be nice to turn this into proper kerneldoc. It is not too far > away at the moment. > > Also, return a proper error code not -1. ENOSPC? Ok. > > > +static int mtip_update_atable_dynamic1(unsigned long write_lo, > > + unsigned long write_hi, int > > block_index, > > + unsigned int port, > > + unsigned int curr_time, > > + struct switch_enet_private > > *fep) > > It would be good to document the return value, because it is not the > usual 0 success or negative error code. Ok. > > > +static const struct net_device_ops mtip_netdev_ops; > > more forward declarations. Ok, fixed. > > > +struct switch_enet_private *mtip_netdev_get_priv(const struct > > net_device *ndev) +{ > > + if (ndev->netdev_ops == &mtip_netdev_ops) > > + return netdev_priv(ndev); > > + > > + return NULL; > > +} > > I _think_ the return value is not actually used. So maybe 0 or > -ENODEV? It is used at: drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c in mtip_port_dev_check() to assess if network interfaces eligible for bridging are using the same (i.e. mtipl2sw) driver. Only when they match - bridging is performed. > > > +static int esw_mac_addr_static(struct switch_enet_private *fep) > > +{ > > + int i; > > + > > + for (i = 0; i < SWITCH_EPORT_NUMBER; i++) { > > + if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) { > > > > Is that possible? This is the interfaces own MAC address? If it is not > valid, the probe should of failed. I've double checked it - this cannot happen (i.e. that is_valid_ether_addr(fep->ndev[i]->dev_addr) is NOT valid at this point of execution. I will remove this check > > > + mtip_update_atable_static((unsigned char *) > > + > > fep->ndev[i]->dev_addr, > > + 7, 7, fep); > > + } else { > > + dev_err(&fep->pdev->dev, > > + "Can not add mac address %pM to > > switch!\n", > > + fep->ndev[i]->dev_addr); > > + return -EFAULT; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static void mtip_print_link_status(struct phy_device *phydev) > > +{ > > + if (phydev->link) > > + netdev_info(phydev->attached_dev, > > + "Link is Up - %s/%s - flow control > > %s\n", > > + phy_speed_to_str(phydev->speed), > > + phy_duplex_to_str(phydev->duplex), > > + phydev->pause ? "rx/tx" : "off"); > > + else > > + netdev_info(phydev->attached_dev, "Link is > > Down\n"); +} > > phy_print_status() Yes, I will remove mtip_print_link_status() and replace it with phy_print_status() > > > +static void mtip_adjust_link(struct net_device *dev) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(dev); > > + struct switch_enet_private *fep = priv->fep; > > + struct phy_device *phy_dev; > > + int status_change = 0, idx; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&fep->hw_lock, flags); > > + > > + idx = priv->portnum - 1; > > + phy_dev = fep->phy_dev[idx]; > > + > > + /* Prevent a state halted on mii error */ > > + if (fep->mii_timeout && phy_dev->state == PHY_HALTED) { > > + phy_dev->state = PHY_UP; > > + goto spin_unlock; > > + } > > A MAC driver should not be playing around with the internal state of > phylib. Ok, I've replaced it with PHY API calls (phy_start() and phy_is_started()). > > > +static int mtip_mii_probe(struct net_device *dev) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(dev); > > + struct switch_enet_private *fep = priv->fep; > > + int port_idx = priv->portnum - 1; > > + struct phy_device *phy_dev = NULL; > > + > > + if (fep->phy_np[port_idx]) { > > + phy_dev = of_phy_connect(dev, > > fep->phy_np[port_idx], > > + &mtip_adjust_link, 0, > > + > > fep->phy_interface[port_idx]); > > + if (!phy_dev) { > > + netdev_err(dev, "Unable to connect to > > phy\n"); > > + return -ENODEV; > > + } > > + } > > + > > + phy_set_max_speed(phy_dev, 100); > > + fep->phy_dev[port_idx] = phy_dev; > > + fep->link[port_idx] = 0; > > + fep->full_duplex[port_idx] = 0; > > + > > + dev_info(&dev->dev, > > + "MTIP PHY driver [%s] (mii_bus:phy_addr=%s, > > irq=%d)\n", > > + fep->phy_dev[port_idx]->drv->name, > > + phydev_name(fep->phy_dev[port_idx]), > > + fep->phy_dev[port_idx]->irq); > > phylib already prints something like that. Yes, the "net lan0: lan0: MTIP eth L2 switch <mac addr>" is printed. For the original call - I've used dev_dbg(). > > > +static int mtip_mdiobus_reset(struct mii_bus *bus) > > +{ > > + if (!bus || !bus->reset_gpiod) { > > + dev_err(&bus->dev, "Reset GPIO pin not > > provided!\n"); > > + return -EINVAL; > > + } > > + > > + gpiod_set_value_cansleep(bus->reset_gpiod, 1); > > + > > + /* Extra time to allow: > > + * 1. GPIO RESET pin go high to prevent situation where > > its value is > > + * "LOW" as it is NOT configured. > > + * 2. The ENET CLK to stabilize before GPIO RESET is > > asserted > > + */ > > + usleep_range(200, 300); > > + > > + gpiod_set_value_cansleep(bus->reset_gpiod, 0); > > + usleep_range(bus->reset_delay_us, bus->reset_delay_us + > > 1000); > > + gpiod_set_value_cansleep(bus->reset_gpiod, 1); > > + > > + if (bus->reset_post_delay_us > 0) > > + usleep_range(bus->reset_post_delay_us, > > + bus->reset_post_delay_us + 1000); > > + > > + return 0; > > +} > > What is wrong with the core code __mdiobus_register() which does the > bus reset. The main problem is that the "default" mdio reset is just asserting and deasserting the reset line. It doesn't take into account the state of the reset gpio before assertion (if it was high for enough time) and if clocks already were stabilized. > > > +static void mtip_get_drvinfo(struct net_device *dev, > > + struct ethtool_drvinfo *info) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(dev); > > + struct switch_enet_private *fep = priv->fep; > > + > > + strscpy(info->driver, fep->pdev->dev.driver->name, > > + sizeof(info->driver)); > > + strscpy(info->version, VERSION, sizeof(info->version)); > > Leave this empty, so you get the git hash of the kernel. Ok. > > > +static void mtip_ndev_setup(struct net_device *dev) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(dev); > > + > > + ether_setup(dev); > > That is pretty unusual Yes - how it has been used is described below. > > > + dev->ethtool_ops = &mtip_ethtool_ops; > > + dev->netdev_ops = &mtip_netdev_ops; > > + > > + memset(priv, 0, sizeof(struct mtip_ndev_priv)); > > priv should already be zero.... Ok, I will remove > > > +static int mtip_ndev_init(struct switch_enet_private *fep) > > +{ > > + struct mtip_ndev_priv *priv; > > + int i, ret = 0; > > + > > + for (i = 0; i < SWITCH_EPORT_NUMBER; i++) { > > + fep->ndev[i] = alloc_netdev(sizeof(struct > > mtip_ndev_priv), > > + fep->ndev_name[i], > > NET_NAME_USER, > > + mtip_ndev_setup); > > This explains the ether_setup(). It would be more normal to pass > ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here. > Yes. I will do that. > > + if (!fep->ndev[i]) { > > + ret = -1; > > -ENOMEM? Ok. > > > + break; > > + } > > + > > + priv = netdev_priv(fep->ndev[i]); > > + priv->fep = fep; > > + priv->portnum = i + 1; > > + fep->ndev[i]->irq = fep->irq; > > + > > + ret = mtip_setup_mac(fep->ndev[i]); > > + if (ret) { > > + dev_err(&fep->ndev[i]->dev, > > + "%s: ndev %s MAC setup err: %d\n", > > + __func__, fep->ndev[i]->name, ret); > > + break; > > + } > > + > > + ret = register_netdev(fep->ndev[i]); > > + if (ret) { > > + dev_err(&fep->ndev[i]->dev, > > + "%s: ndev %s register err: %d\n", > > __func__, > > + fep->ndev[i]->name, ret); > > + break; > > + } > > + dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2 > > switch %pM\n", > > + fep->ndev[i]->name, > > fep->ndev[i]->dev_addr); > > I would drop this. A driver is normally silent unless things go wrong. I've replaced dev_info() with dev_dbg() as this information may be relevant during development. > > > + } > > + > > + if (ret) > > + mtip_ndev_cleanup(fep); > > + > > + return 0; > > return ret? Ok. > > > +static int mtip_ndev_port_link(struct net_device *ndev, > > + struct net_device *br_ndev) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(ndev); > > + struct switch_enet_private *fep = priv->fep; > > + > > + dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n", > > + __func__, ndev->name, br_ndev->name, (unsigned > > int)fep); + > > + /* Check if MTIP switch is already enabled */ > > + if (!fep->br_offload) { > > + if (!priv->master_dev) > > + priv->master_dev = br_ndev; > > It needs to be a little bit more complex than that, because the two > ports could be assigned to two different bridges. You should only > enable hardware bridging if they are a member of the same bridge. This has been explained earlier. The mtip_port_dev_check() checks in mtip_netdevice_event() if we use ports from the mtipl2sw driver. Only for them we start the bridge. > > Andrew Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> > > + /* Prevent a state halted on mii error */ > > > + if (fep->mii_timeout && phy_dev->state == PHY_HALTED) { > > > + phy_dev->state = PHY_UP; > > > + goto spin_unlock; > > > + } > > > > A MAC driver should not be playing around with the internal state of > > phylib. > > Ok, I've replaced it with PHY API calls (phy_start() and > phy_is_started()). phy_start() and phy_stop() should be used in pairs. It is not good to call start more often than stop. What exactly is going on here? Why would there be MII errors? Andrew
Hi Andrew. > > > > + /* Prevent a state halted on mii error */ > > > > + if (fep->mii_timeout && phy_dev->state == PHY_HALTED) { > > > > + phy_dev->state = PHY_UP; > > > > + goto spin_unlock; > > > > + } > > > > > > A MAC driver should not be playing around with the internal state > > > of phylib. > > > > Ok, I've replaced it with PHY API calls (phy_start() and > > phy_is_started()). > > phy_start() and phy_stop() should be used in pairs. It is not good to > call start more often than stop. > > What exactly is going on here? Why would there be MII errors? > Exactly. I've double check it - this can be safely dropped. > Andrew Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Krzysztof, > On 28/03/2025 14:35, Lukasz Majewski wrote: > > + > > +static void mtip_mii_unregister(struct switch_enet_private *fep) > > +{ > > + mdiobus_unregister(fep->mii_bus); > > + mdiobus_free(fep->mii_bus); > > +} > > + > > +static const struct fec_devinfo fec_imx28_l2switch_info = { > > + .quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO, > > +}; > > + > > +static struct platform_device_id pdev_id = { > > That's const. > > > + .name = "imx28-l2switch", > > + .driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info, > > +}; > > + > > +static int __init mtip_sw_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct switch_enet_private *fep; > > + struct fec_devinfo *dev_info; > > + struct switch_t *fecp; > > + int ret; > > + > > + fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL); > > + if (!fep) > > + return -ENOMEM; > > + > > + pdev->id_entry = &pdev_id; > > Hm? This is some odd pattern. You are supposed to use OF table and get > matched by it, not populate some custom/odd handling of platform > tables. I've removed it and fully utilized struct of_device_id. I will just use the OF approach without utilizing platform device. I think that it is better to just switch to OF. > > > + > > + dev_info = (struct fec_devinfo > > *)pdev->id_entry->driver_data; > > I did not notice it before, but that's a no - you cannot drop the > cast. Driver data is always const. The platform device ID approach has been dropped and completely replaced with OF. > > > + if (dev_info) > > + fep->quirks = dev_info->quirks; > > + > > + fep->pdev = pdev; > > + platform_set_drvdata(pdev, fep); > > + > > + fep->enet_addr = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(fep->enet_addr)) > > + return PTR_ERR(fep->enet_addr); > > + > > + fep->irq = platform_get_irq(pdev, 0); > > + if (fep->irq < 0) > > + return fep->irq; > > + > > + ret = mtip_parse_of(fep, np); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "%s: OF parse error (%d)!\n", > > __func__, > > + ret); > > + return ret; > > + } > > + > > + /* Create an Ethernet device instance. > > + * The switch lookup address memory starts at 0x800FC000 > > + */ > > + fep->hwp_enet = fep->enet_addr; > > + fecp = (struct switch_t *)(fep->enet_addr + > > ENET_SWI_PHYS_ADDR_OFFSET); + > > + fep->hwp = fecp; > > + fep->hwentry = (struct mtip_addr_table_t *) > > + ((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET); > > + > > + ret = devm_regulator_get_enable_optional(&pdev->dev, > > "phy"); > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "Unable to get and enable > > 'phy'\n"); + > > + fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg"); > > + if (IS_ERR(fep->clk_ipg)) > > + return dev_err_probe(&pdev->dev, > > PTR_ERR(fep->clk_ipg), > > + "Unable to acquire 'ipg' > > clock\n"); + > > + fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb"); > > + if (IS_ERR(fep->clk_ahb)) > > + return dev_err_probe(&pdev->dev, > > PTR_ERR(fep->clk_ahb), > > + "Unable to acquire 'ahb' > > clock\n"); + > > + fep->clk_enet_out = > > devm_clk_get_optional_enabled(&pdev->dev, > > + > > "enet_out"); > > + if (IS_ERR(fep->clk_enet_out)) > > + return dev_err_probe(&pdev->dev, > > PTR_ERR(fep->clk_enet_out), > > + "Unable to acquire 'enet_out' > > clock\n"); + > > + spin_lock_init(&fep->learn_lock); > > + spin_lock_init(&fep->hw_lock); > > + spin_lock_init(&fep->mii_lock); > > + > > + ret = devm_request_irq(&pdev->dev, fep->irq, > > mtip_interrupt, 0, > > + "mtip_l2sw", fep); > > + if (ret) > > + return dev_err_probe(&pdev->dev, fep->irq, > > + "Could not alloc IRQ\n"); > > + > > + ret = mtip_register_notifiers(fep); > > + if (ret) > > + return ret; > > + > > + ret = mtip_ndev_init(fep); > > + if (ret) { > > + dev_err(&pdev->dev, "%s: Failed to create virtual > > ndev (%d)\n", > > + __func__, ret); > > + goto ndev_init_err; > > + } > > + > > + ret = mtip_switch_dma_init(fep); > > + if (ret) { > > + dev_err(&pdev->dev, "%s: ethernet switch init fail > > (%d)!\n", > > + __func__, ret); > > + goto dma_init_err; > > + } > > + > > + ret = mtip_mii_init(fep, pdev); > > + if (ret) { > > + dev_err(&pdev->dev, "%s: Cannot init phy bus > > (%d)!\n", __func__, > > + ret); > > + goto mii_init_err; > > + } > > + /* setup timer for learning aging function */ > > + timer_setup(&fep->timer_aging, mtip_aging_timer, 0); > > + mod_timer(&fep->timer_aging, > > + jiffies + > > msecs_to_jiffies(LEARNING_AGING_INTERVAL)); + > > + fep->task = kthread_run(mtip_sw_learning, fep, > > "mtip_l2sw_learning"); > > + if (IS_ERR(fep->task)) { > > + ret = PTR_ERR(fep->task); > > + dev_err(&pdev->dev, "%s: learning kthread_run > > error (%d)!\n", > > + __func__, ret); > > + goto task_learning_err; > > + } > > + > > + /* setup MII interface for external switch ports*/ > > + mtip_enet_init(fep, 1); > > + mtip_enet_init(fep, 2); > > + > > + return 0; > > + > > + task_learning_err: > > + del_timer(&fep->timer_aging); > > + mtip_mii_unregister(fep); > > + mii_init_err: > > + dma_init_err: > > + mtip_ndev_cleanup(fep); > > + ndev_init_err: > > + mtip_unregister_notifiers(fep); > > + > > + return ret; > > +} > > + > > +static void mtip_sw_remove(struct platform_device *pdev) > > +{ > > + struct switch_enet_private *fep = > > platform_get_drvdata(pdev); + > > + mtip_unregister_notifiers(fep); > > + mtip_ndev_cleanup(fep); > > + > > + mtip_mii_remove(fep); > > + > > + kthread_stop(fep->task); > > + del_timer(&fep->timer_aging); > > + platform_set_drvdata(pdev, NULL); > > + > > + kfree(fep); > > +} > > + > > +static const struct of_device_id mtipl2_of_match[] = { > > + { .compatible = "nxp,imx287-mtip-switch", }, > > + { /* sentinel */ } > > +}; > > Missing module device table. Ok. I will add it. > > > + > > +static struct platform_driver mtipl2plat_driver = { > > + .driver = { > > + .name = "mtipl2sw", > > + .of_match_table = mtipl2_of_match, > > + .suppress_bind_attrs = true, > > + }, > > + .probe = mtip_sw_probe, > > + .remove_new = mtip_sw_remove, > > +}; > > + > > +module_platform_driver(mtipl2plat_driver); > > +MODULE_AUTHOR("Lukasz Majewski <lukma@denx.de>"); > > +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch"); > > +MODULE_VERSION(VERSION); > > What is the point of paralell versioning with the kernel? Are you > going to keep this updated or - just like in other cases - it will > stay always theh same. Look for example at net/bridge/br.c or some > other files - they are always the same even if driver changed > significantly. > > BTW, this would be 1.0, not 1.4. Your out of tree versioning does not > matter. I'm going to drop it totally. The "versioning" was only required when switching between major LTS kernels. I'd be more than happy to just use kernel SHA1, when this driver is pulled. > > > +MODULE_LICENSE("GPL"); > > +MODULE_ALIAS("platform:mtipl2sw"); > > You should not need MODULE_ALIAS() in normal cases. If you need it, > usually it means your device ID table is wrong (e.g. misses either > entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute > for incomplete ID table. > I will remove it. > > Best regards, > Krzysztof Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
This patch series adds support for More Than IP's L2 switch driver embedded in some NXP's SoCs. This one has been tested on imx287, but is also available in the vf610. In the past there has been performed some attempts to upstream this driver: 1. The 4.19-cip based one [1] 2. DSA based one for 5.12 [2] - i.e. the switch itself was treat as a DSA switch with NO tag appended. 3. The extension for FEC driver for 5.12 [3] - the trick here was to fully reuse FEC when the in-HW switching is disabled. When bridge offloading is enabled, the driver uses already configured MAC and PHY to also configure PHY. All three approaches were not accepted as eligible for upstreaming. The driver from this series has floowing features: 1. It is fully separated from fec_main - i.e. can be used interchangeable with it. To be more specific - one can build them as modules and if required switch between them when e.g. bridge offloading is required. To be more specific: - Use FEC_MAIN: When one needs support for two ETH ports with separate uDMAs used for both and bridging can be realized in SW. - Use MTIPL2SW: When it is enough to support two ports with only uDMA0 attached to switch and bridging shall be offloaded to HW. 2. This driver uses MTIP's L2 switch internal VLAN feature to provide port separation at boot time. Port separation is disabled when bridging is required. 3. Example usage: Configuration: ip link set lan0 up; sleep 1; ip link set lan1 up; sleep 1; ip link add name br0 type bridge; ip link set br0 up; sleep 1; ip link set lan0 master br0; ip link set lan1 master br0; bridge link; ip addr add 192.168.2.17/24 dev br0; ping -c 5 192.168.2.222 Removal: ip link set br0 down; ip link delete br0 type bridge; ip link set dev lan1 down ip link set dev lan0 down 4. Limitations: - Driver enables and disables switch operation with learning and ageing. - Missing is the advanced configuration (e.g. adding entries to FBD). This is on purpose, as up till now we didn't had consensus about how the driver shall be added to Linux. Links: [1] - https://github.com/lmajewski/linux-imx28-l2switch/commits/master [2] - https://github.com/lmajewski/linux-imx28-l2switch/tree/imx28-v5.12-L2-upstream-RFC_v1 [3] - https://source.denx.de/linux/linux-imx28-l2switch/-/tree/imx28-v5.12-L2-upstream-switchdev-RFC_v1?ref_type=heads Lukasz Majewski (4): dt-bindings: net: Add MTIP L2 switch description ARM: dts: nxp: mxs: Adjust the imx28.dtsi L2 switch description ARM: dts: nxp: mxs: Adjust XEA board's DTS to support L2 switch net: mtip: The L2 switch driver for imx287 .../bindings/net/nxp,imx287-mtip-switch.yaml | 165 ++ MAINTAINERS | 7 + arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 54 + arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 8 +- drivers/net/ethernet/freescale/Kconfig | 1 + drivers/net/ethernet/freescale/Makefile | 1 + drivers/net/ethernet/freescale/mtipsw/Kconfig | 13 + .../net/ethernet/freescale/mtipsw/Makefile | 3 + .../net/ethernet/freescale/mtipsw/mtipl2sw.c | 2035 +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h | 781 +++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c | 113 + .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c | 449 ++++ 12 files changed, 3628 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml create mode 100644 drivers/net/ethernet/freescale/mtipsw/Kconfig create mode 100644 drivers/net/ethernet/freescale/mtipsw/Makefile create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw.h create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c