Message ID | 20191024100914.16840-1-grygorii.strashko@ti.com |
---|---|
Headers | show |
Series | net: ethernet: ti: introduce new cpsw switchdev based driver | expand |
Hi, * Grygorii Strashko <grygorii.strashko@ti.com> [191024 10:10]: > This the RFC v5 which introduces new CPSW switchdev based driver which is > operating in dual-emac mode by default, thus working as 2 individual > network interfaces. The Switch mode can be enabled by configuring devlink driver > parameter "switch_mode" to 1/true: > devlink dev param set platform/48484000.ethernet_switch \ > name switch_mode value 1 cmode runtime Just wondering about the migration plan.. Is this a replacement driver or used in addition to the old driver? Regards, Tony
On Thu, Oct 24, 2019 at 01:09:06PM +0300, Grygorii Strashko wrote: > As a preparatory patch to add support for a switchdev based cpsw driver, > move common functions to cpsw-priv.c so that they can be used across both > drivers. Hi Grygorii Bike shedding, but it seems odd to move common code into a private file. How common is the current code in cpsw-common.c? Andrew
> config TI_CPTS > bool "TI Common Platform Time Sync (CPTS) Support" > - depends on TI_CPSW || TI_KEYSTONE_NETCP || COMPILE_TEST > + depends on TI_CPSW || TI_KEYSTONE_NETCP || COMPILE_TEST || TI_CPSW_SWITCHDEV nit picking, but COMPILE_TEST is generally last on the line. > +/** > + * cpsw_set_mc - adds multicast entry to the table if it's not added or deletes > + * if it's not deleted > + * @ndev: device to sync > + * @addr: address to be added or deleted > + * @vid: vlan id, if vid < 0 set/unset address for real device > + * @add: add address if the flag is set or remove otherwise > + */ > +static int cpsw_set_mc(struct net_device *ndev, const u8 *addr, > + int vid, int add) > +{ > + struct cpsw_priv *priv = netdev_priv(ndev); > + struct cpsw_common *cpsw = priv->cpsw; > + int slave_no = cpsw_slave_index(cpsw, priv); > + int mask, flags, ret; David will complain about reverse Christmas tree. You need to move some of the assignments into the body of the function. This problems happens a few times in the code. > +static int cpsw_set_pauseparam(struct net_device *ndev, > + struct ethtool_pauseparam *pause) > +{ > + struct cpsw_common *cpsw = ndev_to_cpsw(ndev); > + struct cpsw_priv *priv = netdev_priv(ndev); > + > + priv->rx_pause = pause->rx_pause ? true : false; > + priv->tx_pause = pause->tx_pause ? true : false; > + > + return phy_restart_aneg(cpsw->slaves[priv->emac_port - 1].phy); > +} You should look at the value of pause.autoneg. > +static const struct devlink_ops cpsw_devlink_ops; It would be nice to avoid this forward declaration. > +static const struct devlink_param cpsw_devlink_params[] = { > + DEVLINK_PARAM_DRIVER(CPSW_DL_PARAM_ALE_BYPASS, > + "ale_bypass", DEVLINK_PARAM_TYPE_BOOL, > + BIT(DEVLINK_PARAM_CMODE_RUNTIME), > + cpsw_dl_ale_ctrl_get, cpsw_dl_ale_ctrl_set, NULL), > +}; Is this documented? Andrew
> +static int cpsw_probe(struct platform_device *pdev) > +{ > + const struct soc_device_attribute *soc; > + struct device *dev = &pdev->dev; > + struct resource *ss_res; > + struct cpsw_common *cpsw; > + struct gpio_descs *mode; > + void __iomem *ss_regs; > + int ret = 0, ch; > + struct clk *clk; > + int irq; > + ... > + > + /* setup netdevs */ > + ret = cpsw_create_ports(cpsw); > + if (ret) > + goto clean_unregister_netdev; At this point, the slave ports go live. If the kernel is configured with NFS root etc, it will start using the interfaces. + > + /* Grab RX and TX IRQs. Note that we also have RX_THRESHOLD and > + * MISC IRQs which are always kept disabled with this driver so > + * we will not request them. > + * > + * If anyone wants to implement support for those, make sure to > + * first request and append them to irqs_table array. > + */ > + > + ret = devm_request_irq(dev, cpsw->irqs_table[0], cpsw_rx_interrupt, > + 0, dev_name(dev), cpsw); > + if (ret < 0) { > + dev_err(dev, "error attaching irq (%d)\n", ret); > + goto clean_unregister_netdev; > + } > + > + ret = devm_request_irq(dev, cpsw->irqs_table[1], cpsw_tx_interrupt, > + 0, dev_name(dev), cpsw); > + if (ret < 0) { > + dev_err(dev, "error attaching irq (%d)\n", ret); > + goto clean_unregister_netdev; > + } Are there any race conditions if the network starts using the devices before interrupts are requested? To be safe, maybe this should be done before the slaves are created? Andrew
On 25/10/2019 16:01, Andrew Lunn wrote: > On Thu, Oct 24, 2019 at 01:09:06PM +0300, Grygorii Strashko wrote: >> As a preparatory patch to add support for a switchdev based cpsw driver, >> move common functions to cpsw-priv.c so that they can be used across both >> drivers. > > Hi Grygorii > > Bike shedding, but it seems odd to move common code into a private > file. How common is the current code in cpsw-common.c? cpsw-common.c is used between cpsw and davinci_emac.c and, as of now, contains only code to retrieve MAC addr from eFuse regs. cpsw_priv.x were added intentionally as code moved to these files only used by ald and new CPSW drivers. This also allows to avoid build/link issues.
Hi Andrew, On 29/10/2019 14:24, Andrew Lunn wrote: >> config TI_CPTS >> bool "TI Common Platform Time Sync (CPTS) Support" >> - depends on TI_CPSW || TI_KEYSTONE_NETCP || COMPILE_TEST >> + depends on TI_CPSW || TI_KEYSTONE_NETCP || COMPILE_TEST || TI_CPSW_SWITCHDEV > > nit picking, but COMPILE_TEST is generally last on the line. > >> +/** >> + * cpsw_set_mc - adds multicast entry to the table if it's not added or deletes >> + * if it's not deleted >> + * @ndev: device to sync >> + * @addr: address to be added or deleted >> + * @vid: vlan id, if vid < 0 set/unset address for real device >> + * @add: add address if the flag is set or remove otherwise >> + */ >> +static int cpsw_set_mc(struct net_device *ndev, const u8 *addr, >> + int vid, int add) >> +{ >> + struct cpsw_priv *priv = netdev_priv(ndev); >> + struct cpsw_common *cpsw = priv->cpsw; >> + int slave_no = cpsw_slave_index(cpsw, priv); >> + int mask, flags, ret; > > David will complain about reverse Christmas tree. You need to move > some of the assignments into the body of the function. This problems > happens a few times in the code. > >> +static int cpsw_set_pauseparam(struct net_device *ndev, >> + struct ethtool_pauseparam *pause) >> +{ >> + struct cpsw_common *cpsw = ndev_to_cpsw(ndev); >> + struct cpsw_priv *priv = netdev_priv(ndev); >> + >> + priv->rx_pause = pause->rx_pause ? true : false; >> + priv->tx_pause = pause->tx_pause ? true : false; >> + >> + return phy_restart_aneg(cpsw->slaves[priv->emac_port - 1].phy); >> +} > > You should look at the value of pause.autoneg. I'll use phy_validate_pause() and phy_set_asym_pause() here, and fix other comments. > >> +static const struct devlink_ops cpsw_devlink_ops; > > It would be nice to avoid this forward declaration. It's not declaration, it's definition of devlink_ops without any standard callbacks implemented. > >> +static const struct devlink_param cpsw_devlink_params[] = { >> + DEVLINK_PARAM_DRIVER(CPSW_DL_PARAM_ALE_BYPASS, >> + "ale_bypass", DEVLINK_PARAM_TYPE_BOOL, >> + BIT(DEVLINK_PARAM_CMODE_RUNTIME), >> + cpsw_dl_ale_ctrl_get, cpsw_dl_ale_ctrl_set, NULL), >> +}; > > Is this documented? In patch 9. But I'll update it and add standard devlink parameter definition, like: ale_bypass [DEVICE, DRIVER-SPECIFIC] Allows to enable ALE_CONTROL(4).BYPASS mode for debug purposes Type: bool Configuration mode: runtime Thank you for review.
On 29/10/2019 14:32, Andrew Lunn wrote: >> +static int cpsw_probe(struct platform_device *pdev) >> +{ >> + const struct soc_device_attribute *soc; >> + struct device *dev = &pdev->dev; >> + struct resource *ss_res; >> + struct cpsw_common *cpsw; >> + struct gpio_descs *mode; >> + void __iomem *ss_regs; >> + int ret = 0, ch; >> + struct clk *clk; >> + int irq; >> + > > ... > >> + >> + /* setup netdevs */ >> + ret = cpsw_create_ports(cpsw); >> + if (ret) >> + goto clean_unregister_netdev; > > At this point, the slave ports go live. If the kernel is configured > with NFS root etc, it will start using the interfaces. > > + >> + /* Grab RX and TX IRQs. Note that we also have RX_THRESHOLD and >> + * MISC IRQs which are always kept disabled with this driver so >> + * we will not request them. >> + * >> + * If anyone wants to implement support for those, make sure to >> + * first request and append them to irqs_table array. >> + */ >> + >> + ret = devm_request_irq(dev, cpsw->irqs_table[0], cpsw_rx_interrupt, >> + 0, dev_name(dev), cpsw); >> + if (ret < 0) { >> + dev_err(dev, "error attaching irq (%d)\n", ret); >> + goto clean_unregister_netdev; >> + } >> + >> + ret = devm_request_irq(dev, cpsw->irqs_table[1], cpsw_tx_interrupt, >> + 0, dev_name(dev), cpsw); >> + if (ret < 0) { >> + dev_err(dev, "error attaching irq (%d)\n", ret); >> + goto clean_unregister_netdev; >> + } > > Are there any race conditions if the network starts using the devices > before interrupts are requested? To be safe, maybe this should be done > before the slaves are created? Usually during boot - there is no parallel probing (as opposite to modules loading by udev, for example). Also, there is barrier init call deferred_probe_initcall() to ensure all drivers probed before going to mount rootfs. So, i do not think this could cause any issue - max few packets will be delayed until kernel will switch back here, but the chances that ndo_open will be finished before probe ->0.
> > > +static const struct devlink_ops cpsw_devlink_ops; > > > > It would be nice to avoid this forward declaration. > > It's not declaration, it's definition of devlink_ops without any standard callbacks implemented. Ho Grygorii Ah, yes. How about = { }; to make it clearer? > > > +static const struct devlink_param cpsw_devlink_params[] = { > > > + DEVLINK_PARAM_DRIVER(CPSW_DL_PARAM_ALE_BYPASS, > > > + "ale_bypass", DEVLINK_PARAM_TYPE_BOOL, > > > + BIT(DEVLINK_PARAM_CMODE_RUNTIME), > > > + cpsw_dl_ale_ctrl_get, cpsw_dl_ale_ctrl_set, NULL), > > > +}; > > > > Is this documented? > > In patch 9. But I'll update it and add standard devlink parameter definition, like: > > ale_bypass [DEVICE, DRIVER-SPECIFIC] > Allows to enable ALE_CONTROL(4).BYPASS mode for debug purposes > Type: bool > Configuration mode: runtime And please you the standard file naming and location, Documentation/networking/devlink-params-foo.txt Thanks Andrew
On 01/11/2019 22:39, Andrew Lunn wrote: >>>> +static const struct devlink_ops cpsw_devlink_ops; >>> >>> It would be nice to avoid this forward declaration. >> >> It's not declaration, it's definition of devlink_ops without any standard callbacks implemented. > > Ho Grygorii > > Ah, yes. > > How about > > = { > }; > > to make it clearer? NP > >>>> +static const struct devlink_param cpsw_devlink_params[] = { >>>> + DEVLINK_PARAM_DRIVER(CPSW_DL_PARAM_ALE_BYPASS, >>>> + "ale_bypass", DEVLINK_PARAM_TYPE_BOOL, >>>> + BIT(DEVLINK_PARAM_CMODE_RUNTIME), >>>> + cpsw_dl_ale_ctrl_get, cpsw_dl_ale_ctrl_set, NULL), >>>> +}; >>> >>> Is this documented? >> >> In patch 9. But I'll update it and add standard devlink parameter definition, like: >> >> ale_bypass [DEVICE, DRIVER-SPECIFIC] >> Allows to enable ALE_CONTROL(4).BYPASS mode for debug purposes >> Type: bool >> Configuration mode: runtime > > And please you the standard file naming and location, > Documentation/networking/devlink-params-foo.txt Ok. I will. But I'd like to clarify: - drivers documentation placed in ./Documentation/networking/device_drivers/ti/ so could you confirm pls, that you want me to add devlink-params documentation in separate file and palace it in ./Documentation/networking/ folder directly?
On Fri, Nov 01, 2019 at 10:34:57PM +0200, Grygorii Strashko wrote: > > > On 29/10/2019 14:32, Andrew Lunn wrote: > > > +static int cpsw_probe(struct platform_device *pdev) > > > +{ > > > + const struct soc_device_attribute *soc; > > > + struct device *dev = &pdev->dev; > > > + struct resource *ss_res; > > > + struct cpsw_common *cpsw; > > > + struct gpio_descs *mode; > > > + void __iomem *ss_regs; > > > + int ret = 0, ch; > > > + struct clk *clk; > > > + int irq; > > > + > > > > ... > > > > > + > > > + /* setup netdevs */ > > > + ret = cpsw_create_ports(cpsw); > > > + if (ret) > > > + goto clean_unregister_netdev; > > > > At this point, the slave ports go live. If the kernel is configured > > with NFS root etc, it will start using the interfaces. > > > > + > > > + /* Grab RX and TX IRQs. Note that we also have RX_THRESHOLD and > > > + * MISC IRQs which are always kept disabled with this driver so > > > + * we will not request them. > > > + * > > > + * If anyone wants to implement support for those, make sure to > > > + * first request and append them to irqs_table array. > > > + */ > > > + > > > + ret = devm_request_irq(dev, cpsw->irqs_table[0], cpsw_rx_interrupt, > > > + 0, dev_name(dev), cpsw); > > > + if (ret < 0) { > > > + dev_err(dev, "error attaching irq (%d)\n", ret); > > > + goto clean_unregister_netdev; > > > + } > > > + > > > + ret = devm_request_irq(dev, cpsw->irqs_table[1], cpsw_tx_interrupt, > > > + 0, dev_name(dev), cpsw); > > > + if (ret < 0) { > > > + dev_err(dev, "error attaching irq (%d)\n", ret); > > > + goto clean_unregister_netdev; > > > + } > > > > Are there any race conditions if the network starts using the devices > > before interrupts are requested? To be safe, maybe this should be done > > before the slaves are created? > > Usually during boot - there is no parallel probing (as opposite to modules loading by > udev, for example). Also, there is barrier init call deferred_probe_initcall() to ensure all > drivers probed before going to mount rootfs. > > So, i do not think this could cause any issue - max few packets will be delayed > until kernel will switch back here, but the chances that ndo_open will be finished before probe ->0. I helped track down a crash recently, along these lines. ndo_open() was getting called before the probe function finished, when kernel ip address auto config was in action. This is not to do with parallel probing, i think there is something in register_netdev() which is triggered each time an interface is added to do the ip configuration. And the first thing that does is open the interface. Andrew
> > And please you the standard file naming and location, > > Documentation/networking/devlink-params-foo.txt > Ok. I will. > But I'd like to clarify: > - drivers documentation placed in ./Documentation/networking/device_drivers/ti/ > so could you confirm pls, that you want me to add devlink-params documentation in separate file > and palace it in ./Documentation/networking/ folder directly? Hi Grygorii That appears to be the expected place for devlink documentation. You can link to it from your document in the ti subdirectory. Andrew
Hi Tony, On 24/10/2019 19:05, Tony Lindgren wrote: > Hi, > > * Grygorii Strashko <grygorii.strashko@ti.com> [191024 10:10]: >> This the RFC v5 which introduces new CPSW switchdev based driver which is >> operating in dual-emac mode by default, thus working as 2 individual >> network interfaces. The Switch mode can be enabled by configuring devlink driver >> parameter "switch_mode" to 1/true: >> devlink dev param set platform/48484000.ethernet_switch \ >> name switch_mode value 1 cmode runtime > > Just wondering about the migration plan.. Is this a replacement > driver or used in addition to the old driver? > Sry, I've missed you mail. As it's pretty big change the idea is to keep both drivers at least for sometime. Step 1: add new driver and enable it on one platform. Do announcement. Step 2: switch all one-port and dual mac drivers to the new driver Step 3: switch all other platform to cpsw switchdev and deprecate old driver.
* Grygorii Strashko <grygorii.strashko@ti.com> [191109 15:16]: > Hi Tony, > > On 24/10/2019 19:05, Tony Lindgren wrote: > > Hi, > > > > * Grygorii Strashko <grygorii.strashko@ti.com> [191024 10:10]: > > > This the RFC v5 which introduces new CPSW switchdev based driver which is > > > operating in dual-emac mode by default, thus working as 2 individual > > > network interfaces. The Switch mode can be enabled by configuring devlink driver > > > parameter "switch_mode" to 1/true: > > > devlink dev param set platform/48484000.ethernet_switch \ > > > name switch_mode value 1 cmode runtime > > > > Just wondering about the migration plan.. Is this a replacement > > driver or used in addition to the old driver? > > > > Sry, I've missed you mail. > > As it's pretty big change the idea is to keep both drivers at least for sometime. > Step 1: add new driver and enable it on one platform. Do announcement. > Step 2: switch all one-port and dual mac drivers to the new driver > Step 3: switch all other platform to cpsw switchdev and deprecate old driver. OK sounds good to me. So for the dts changes, we keep the old binding and just add a new module there? Or do you also have to disable some parts of the old dts? Regards, Tony
On 11/11/2019 19:08, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [191109 15:16]: >> Hi Tony, >> >> On 24/10/2019 19:05, Tony Lindgren wrote: >>> Hi, >>> >>> * Grygorii Strashko <grygorii.strashko@ti.com> [191024 10:10]: >>>> This the RFC v5 which introduces new CPSW switchdev based driver which is >>>> operating in dual-emac mode by default, thus working as 2 individual >>>> network interfaces. The Switch mode can be enabled by configuring devlink driver >>>> parameter "switch_mode" to 1/true: >>>> devlink dev param set platform/48484000.ethernet_switch \ >>>> name switch_mode value 1 cmode runtime >>> >>> Just wondering about the migration plan.. Is this a replacement >>> driver or used in addition to the old driver? >>> >> >> Sry, I've missed you mail. >> >> As it's pretty big change the idea is to keep both drivers at least for sometime. >> Step 1: add new driver and enable it on one platform. Do announcement. >> Step 2: switch all one-port and dual mac drivers to the new driver >> Step 3: switch all other platform to cpsw switchdev and deprecate old driver. > > OK sounds good to me. So for the dts changes, we keep the old binding > and just add a new module there? yes, in general. As you can see Patch 11 I've rearranged cpsw nodes between boards so cpsw_new is enabled only on one board. But, Am5/dr7 board are good candidates to enable cpsw_new in batch as the have only dual_mac default cfg. > > Or do you also have to disable some parts of the old dts? it will be the case for am3/am5 most probably.