| Message ID | 20191024100914.16840-1-grygorii.strashko@ti.com |
|---|---|
| Headers | show |
| Series | net: ethernet: ti: introduce new cpsw switchdev based driver | expand |
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
Hi Florian, On 25/10/2019 20:47, Florian Fainelli wrote: > On 10/24/19 3:09 AM, Grygorii Strashko wrote: >> Add bindings for the new TI CPSW switch driver. Comparing to the legacy >> bindings (net/cpsw.txt): >> - ports definition follows DSA bindings (net/dsa/dsa.txt) and ports can be >> marked as "disabled" if not physically wired. >> - all deprecated properties dropped; >> - all legacy propertiies dropped which represent constant HW cpapbilities >> (cpdma_channels, ale_entries, bd_ram_size, mac_control, slaves, >> active_slave) >> - TI CPTS DT properties are reused as is, but grouped in "cpts" sub-node >> - TI Davinci MDIO DT bindings are reused as is, because Davinci MDIO is >> reused. >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- > > [snip] >> +- mdio : CPSW MDIO bus block description >> + - bus_freq : MDIO Bus frequency > > clock-frequency is a more typical property to describe the bus clock's > frequency, that is what i2c and spi do. The MDIO is re-used here unchanged (including bindings). i think, I could try to add standard optional property "bus-frequency" to MDIO bindings as separate series, and deprecate "bus_freq". > >> + See bindings/net/mdio.txt and davinci-mdio.txt >> + >> +- cpts : The Common Platform Time Sync (CPTS) module description >> + - clocks : should contain the CPTS reference clock >> + - clock-names : should be "cpts" >> + See bindings/clock/clock-bindings.txt >> + >> + Optional properties - all ports: >> + - cpts_clock_mult : Numerator to convert input clock ticks into ns >> + - cpts_clock_shift : Denominator to convert input clock ticks into ns >> + Mult and shift will be calculated basing on CPTS >> + rftclk frequency if both cpts_clock_shift and >> + cpts_clock_mult properties are not provided. > > Why would those two be needed that would be modeled in the Linux Common > Clock Framework? The CPTS is re-used here unchanged (including bindings). This is very specific tuning options for PHC clock (cyclecounter) which intended to be used in very rare cases with some ref frequencies when automatic calculation is not working properly. -- Best regards, grygorii
hi Andrew, On 29/10/2019 04:23, Andrew Lunn wrote: >> +TI SoC Ethernet Switch Controller Device Tree Bindings (new) >> +------------------------------------------------------ >> + >> +The 3-port switch gigabit ethernet subsystem provides ethernet packet >> +communication and can be configured as an ethernet switch. > > Hi Grygorii > > Maybe referring it to a 3-port switch will cause confusion, since in > this use case, it only has 2 ports, and you only list two ports in the > device tree. Yeah. This is how it's defined in TRM - Port 0 (CPU port) is the same as external Port from CPSW switch core point of view. > >> It provides the >> +gigabit media independent interface (GMII),reduced gigabit media >> +independent interface (RGMII), reduced media independent interface (RMII), [...] >> + >> +&mac_sw { >> + pinctrl-names = "default", "sleep"; >> + status = "okay"; >> +}; >> + >> +&cpsw_port1 { >> + phy-handle = <ðphy0_sw>; >> + phy-mode = "rgmii"; >> + ti,dual_emac_pvid = <1>; >> +}; >> + >> +&cpsw_port2 { >> + phy-handle = <ðphy1_sw>; >> + phy-mode = "rgmii"; >> + ti,dual_emac_pvid = <2>; >> +}; >> + >> +&davinci_mdio_sw { >> + ethphy0_sw: ethernet-phy@0 { >> + reg = <0>; >> + }; >> + >> + ethphy1_sw: ethernet-phy@1 { >> + reg = <1>; >> + }; >> +}; > > In an example, it is unusual to split things up like this. I > understand that parts of this will be in the dtsi file, and parts in > the .dts file, but examples generally keep it all as one. And when you > re-write this in YAML so it can be used to validated real DTs, you > will have to combine it. Thank you. I'll update. -- Best regards, grygorii
On 01/11/2019 19:36, Florian Fainelli wrote: > On 11/1/19 10:25 AM, Grygorii Strashko wrote: >> Hi Florian, >> >> On 25/10/2019 20:47, Florian Fainelli wrote: >>> On 10/24/19 3:09 AM, Grygorii Strashko wrote: >>>> Add bindings for the new TI CPSW switch driver. Comparing to the legacy >>>> bindings (net/cpsw.txt): >>>> - ports definition follows DSA bindings (net/dsa/dsa.txt) and ports >>>> can be >>>> marked as "disabled" if not physically wired. >>>> - all deprecated properties dropped; >>>> - all legacy propertiies dropped which represent constant HW >>>> cpapbilities >>>> (cpdma_channels, ale_entries, bd_ram_size, mac_control, slaves, >>>> active_slave) >>>> - TI CPTS DT properties are reused as is, but grouped in "cpts" sub-node >>>> - TI Davinci MDIO DT bindings are reused as is, because Davinci MDIO is >>>> reused. >>>> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>> >>> [snip] >>>> +- mdio : CPSW MDIO bus block description >>>> + - bus_freq : MDIO Bus frequency >>> >>> clock-frequency is a more typical property to describe the bus clock's >>> frequency, that is what i2c and spi do. >> >> The MDIO is re-used here unchanged (including bindings). >> i think, I could try to add standard optional property "bus-frequency" >> to MDIO bindings >> as separate series, and deprecate "bus_freq". > > What is wrong with 'clock-frequency'? > > Documentation/devicetree/bindings/i2c/i2c.txt: > > - clock-frequency > frequency of bus clock in Hz. > > Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt: > > - clock-frequency: the MDIO bus clock that must be output by the MDIO bus > hardware, if absent, the default hardware values are used > > Maybe this is a bit of a misnomer as it is usually considered a > replacement for the lack of a proper "clocks" property with a clock > provider, but we can flip the coin around any way we want, it looks > almost the same. > I can do clock-frequency, but I like more bus-frequency (personally) due to more understandable meaning, and because in "Devicetree Specification v0.2" clock-frequency is defined as more related to clocks. Any way I hope you agree that it should be part separate discussion? -- Best regards, grygorii
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. -- Best regards, grygorii
* 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
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 -- 2.17.1