mbox series

[v5,net-next,00/12] net: ethernet: ti: introduce new cpsw switchdev based driver

Message ID 20191024100914.16840-1-grygorii.strashko@ti.com
Headers show
Series net: ethernet: ti: introduce new cpsw switchdev based driver | expand

Message

Grygorii Strashko Oct. 24, 2019, 10:09 a.m. UTC
Hi All,

Huh, I was finally able to return to this work.

The major part of work done in this iteration is rebasing on top of net-next
with XDP series from Ivan Khoronzhuk [3], and enable XDP support in the new
CPSW switchdev driver (it was little bit painful ;(). There are mostly no
functional changes in new CPSW driver, just few fixes, sync with old driver
and cleanups/optimizations. So, I've kept rest of cover letter unchanged.

And thank you All for review of v4.

---
This series originally based on work [1][2] done by
Ilias Apalodimas <ilias.apalodimas@linaro.org>.

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
This can be done regardless of the state of Port's netdev devices - UP/DOWN, but
Port's netdev devices have to be in UP before joining the bridge to avoid
overwriting of bridge configuration as CPSW switch driver completely reloads its
configuration when first Port changes its state to UP.
When the both interfaces joined the bridge - CPSW switch driver will start
marking packets with offload_fwd_mark flag unless "ale_bypass=0".
All configuration is implemented via switchdev API. 

The previous solution of tracking both Ports joined the bridge
(from netdevice_notifier) proved to be not correct as changing CPSW switch
driver mode required cleanup of ALE table and CPSW settings which happens
while second Port is joined bridge and as result configuration loaded
by bridge for the first Port became corrupted.

The introduction of the new CPSW switchdev based driver (cpsw_new.c) is split
on two parts: Part 1 - basic dual-emac driver; Part 2 switchdev support.
Such approach has simplified code development and testing alot. And, I hope, 
it will help with better review.

patches #1 - 4: preparation patches which also moves common code to cpsw_priv.c
patches #5 - 8: Introduce TI CPSW switch driver based on switchdev and new
 DT bindings
patch #9: new CPSW switchdev driver documentation
patch #10: adds DT nodes for new CPSW switchdev driver added for DRA7 SoC
patch #11: adds DT nodes for new cpsw switchdev driver for am571x-idk board
patch #12: enables build of TI CPSW driver

Most of the contents of the previous cover-letter have been added in
new driver documentation, so please refer to that for configuration,
testing and future work.

These patches can be found at:
 https://github.com/grygoriyS/linux.git
 branch: lkml-5.4-switch-tbd-v5

changes in v5:
 - rebase on top of net-next with XDP series from Ivan Khoronzhuk [3],
   and enable XDP support in the new CPSW switchdev driver
   cpsw driver (tested XDP_DROP only)
 - sync with old cpsw driver
 - implement comments from  Ivan Khoronzhuk and Rob Herring
 - fixed "NETDEV WATCHDOG: .." warning after interface after interface UP/DOWN,
   missed TX wake in cpsw_adjust_link()

v4: https://patchwork.kernel.org/cover/11010523/
 - finished split of common CPSW code
 - added devlink support
 - changed CPSW mode configuration approach: from netdevice_notifier to devlink
   parameter
 - refactor and clean up ALE changes which allows to modify VLANs/MDBs entries
 - added missed support for port QDISC_CBS and QDISC_MQPRIO
 - the CPSW is split on two parts: basic dual_mac driver and switchdev support
 - added missed callback .ndo_get_port_parent_id()
 - reworked ingress frames marking in switch mode (offload_fwd_mark)
 - applied comments from Andrew Lunn

v3: https://lwn.net/Articles/786677/
Changes in v3:
- alot of work done to split properly common code between legacy and switchdev
  CPSW drivers and clean up code
- CPSW switchdev interface updated to the current LKML switchdev interface
- actually new CPSW switchdev based driver introduced
- optimized dual_mac mode in new driver. Main change is that in promiscuous
mode P0_UNI_FLOOD (both ports) is enabled in addition to ALLMULTI (current
port) instead of ALE_BYPASS.  So, port in non promiscuous mode will keep
possibility of mcast and vlan filtering.
- changed bridge join sequnce: now switch mode will be enabled only when
both ports joined the bridge. CPSW will be switched to dual_mac mode if any
port leave bridge. ALE table is completly cleared and then refiled while
switching to switch mode - this simplidies code a lot, but introduces some
limitation to bridge setup sequence:
 ip link add name br0 type bridge
 ip link set dev br0 type bridge ageing_time 1000
 ip link set dev br0 type bridge vlan_filtering 0 <- disable
 echo 0 > /sys/class/net/br0/bridge/default_vlan

 ip link set dev sw0p1 up <- add ports
 ip link set dev sw0p2 up
 ip link set dev sw0p1 master br0
 ip link set dev sw0p2 master br0

 echo 1 > /sys/class/net/br0/bridge/default_vlan <- enable
 ip link set dev br0 type bridge vlan_filtering 1
 bridge vlan add dev br0 vid 1 pvid untagged self
- STP tested with vlan_filtering 1/0. To make STP work I've had to set
  NO_SA_UPDATE for all slave ports (see comment in code). It also required to
  statically register STP mcast address {0x01, 0x80, 0xc2, 0x0, 0x0, 0x0};
- allowed build both TI_CPSW and TI_CPSW_SWITCHDEV drivers
- PTP can be enabled on both ports in dual_mac mode

[1] https://patchwork.ozlabs.org/cover/929367/
[2] https://patches.linaro.org/cover/136709/
[3] https://patchwork.kernel.org/cover/11035813/

Grygorii Strashko (8):
  net: ethernet: ti: cpsw: allow untagged traffic on host port
  net: ethernet: ti: cpsw: resolve build deps of cpsw drivers
  net: ethernet: ti: cpsw: move set of common functions in cpsw_priv
  dt-bindings: net: ti: add new cpsw switch driver bindings
  phy: ti: phy-gmii-sel: dependency from ti cpsw-switchdev driver
  ARM: dts: dra7: add dt nodes for new cpsw switch dev driver
  ARM: dts: am571x-idk: enable for new cpsw switch dev driver
  arm: omap2plus_defconfig: enable new cpsw switchdev driver

Ilias Apalodimas (4):
  net: ethernet: ti: cpsw: ale: modify vlan/mdb api for switchdev
  net: ethernet: ti: introduce cpsw  switchdev based driver part 1 -
    dual-emac
  net: ethernet: ti: introduce cpsw switchdev based driver part 2 -
    switch
  Documentation: networking: add cpsw switchdev based driver
    documentation

 .../bindings/net/ti,cpsw-switch.txt           |  145 ++
 .../device_drivers/ti/cpsw_switchdev.txt      |  207 ++
 arch/arm/boot/dts/am571x-idk.dts              |   27 +
 arch/arm/boot/dts/am572x-idk.dts              |    5 +
 arch/arm/boot/dts/am574x-idk.dts              |    5 +
 arch/arm/boot/dts/am57xx-idk-common.dtsi      |    5 -
 arch/arm/boot/dts/dra7-l4.dtsi                |   52 +
 arch/arm/configs/omap2plus_defconfig          |    1 +
 drivers/net/ethernet/ti/Kconfig               |   19 +-
 drivers/net/ethernet/ti/Makefile              |    2 +
 drivers/net/ethernet/ti/cpsw.c                | 1374 +-----------
 drivers/net/ethernet/ti/cpsw_ale.c            |  146 +-
 drivers/net/ethernet/ti/cpsw_ale.h            |   11 +
 drivers/net/ethernet/ti/cpsw_new.c            | 1995 +++++++++++++++++
 drivers/net/ethernet/ti/cpsw_priv.c           | 1245 +++++++++-
 drivers/net/ethernet/ti/cpsw_priv.h           |   79 +-
 drivers/net/ethernet/ti/cpsw_switchdev.c      |  589 +++++
 drivers/net/ethernet/ti/cpsw_switchdev.h      |   15 +
 drivers/phy/ti/Kconfig                        |    4 +-
 19 files changed, 4586 insertions(+), 1340 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,cpsw-switch.txt
 create mode 100644 Documentation/networking/device_drivers/ti/cpsw_switchdev.txt
 create mode 100644 drivers/net/ethernet/ti/cpsw_new.c
 create mode 100644 drivers/net/ethernet/ti/cpsw_switchdev.c
 create mode 100644 drivers/net/ethernet/ti/cpsw_switchdev.h

Comments

Tony Lindgren Oct. 24, 2019, 4:05 p.m. UTC | #1
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
Andrew Lunn Oct. 25, 2019, 1:01 p.m. UTC | #2
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
Andrew Lunn Oct. 29, 2019, 12:24 p.m. UTC | #3
>  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
Andrew Lunn Oct. 29, 2019, 12:32 p.m. UTC | #4
> +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
Grygorii Strashko Nov. 1, 2019, 4:55 p.m. UTC | #5
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.
Grygorii Strashko Nov. 1, 2019, 8:16 p.m. UTC | #6
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.
Grygorii Strashko Nov. 1, 2019, 8:34 p.m. UTC | #7
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.
Andrew Lunn Nov. 1, 2019, 8:39 p.m. UTC | #8
> > > +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
Grygorii Strashko Nov. 1, 2019, 8:46 p.m. UTC | #9
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?
Andrew Lunn Nov. 1, 2019, 8:57 p.m. UTC | #10
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
Andrew Lunn Nov. 1, 2019, 8:59 p.m. UTC | #11
> > 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
Grygorii Strashko Nov. 9, 2019, 3:15 p.m. UTC | #12
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.
Tony Lindgren Nov. 11, 2019, 5:08 p.m. UTC | #13
* 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
Grygorii Strashko Nov. 12, 2019, 9:48 a.m. UTC | #14
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.