[net-next v11 0/7] net: mtip: Add support for MTIP imx287 L2 switch driver

[net-next v11 0/7] net: mtip: Add support for MTIP imx287 L2 switch driver

[Lukasz Majewski]

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.

5. Clang build:
	make LLVM_SUFFIX=-19 LLVM=1 mrproper
	cp ./arch/arm/configs/mxs_defconfig .config
	make ARCH=arm LLVM_SUFFIX=-19 LLVM=1 W=1 menuconfig
	make ARCH=arm LLVM_SUFFIX=-19 LLVM=1 W=1 -j8 LOADADDR=0x40008000 uImage dtbs

6. Kernel compliance checks:
	make coccicheck MODE=report J=4 M=drivers/net/ethernet/freescale/mtipsw/
	~/work/src/smatch/smatch_scripts/kchecker drivers/net/ethernet/freescale/mtipsw/


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 (7):
  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
  ARM: mxs_defconfig: Enable CONFIG_NFS_FSCACHE
  ARM: mxs_defconfig: Update mxs_defconfig to 6.15-rc1
  ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2
    switch

 .../bindings/net/nxp,imx28-mtip-switch.yaml   |  149 ++
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts       |   56 +
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi          |    9 +-
 arch/arm/configs/mxs_defconfig                |   13 +-
 drivers/net/ethernet/freescale/Kconfig        |    1 +
 drivers/net/ethernet/freescale/Makefile       |    1 +
 drivers/net/ethernet/freescale/mtipsw/Kconfig |   13 +
 .../net/ethernet/freescale/mtipsw/Makefile    |    4 +
 .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 1968 +++++++++++++++++
 .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  771 +++++++
 .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  120 +
 .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  436 ++++
 13 files changed, 3537 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,imx28-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

-- 
2.39.5

[net-next v11 1/7] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

This patch provides description of the MTIP L2 switch available in some
NXP's SOCs - e.g. imx287.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

---
Changes for v2:
- Rename the file to match exactly the compatible
  (nxp,imx287-mtip-switch)

Changes for v3:
- Remove '-' from const:'nxp,imx287-mtip-switch'
- Use '^port@[12]+$' for port patternProperties
- Drop status = "okay";
- Provide proper indentation for 'example' binding (replace 8
  spaces with 4 spaces)
- Remove smsc,disable-energy-detect; property
- Remove interrupt-parent and interrupts properties as not required
- Remove #address-cells and #size-cells from required properties check
- remove description from reg:
- Add $ref: ethernet-switch.yaml#

Changes for v4:
- Use $ref: ethernet-switch.yaml#/$defs/ethernet-ports and remove already
  referenced properties
- Rename file to nxp,imx28-mtip-switch.yaml

Changes for v5:
- Provide proper description for 'ethernet-port' node

Changes for v6:
- Proper usage of
  $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
  when specifying the 'ethernet-ports' property
- Add description and check for interrupt-names property

Changes for v7:
- Change switch interrupt name from 'mtipl2sw' to 'enet_switch'

Changes for v8:
- None

Changes for v9:
- Add GPIO_ACTIVE_LOW to reset-gpios mdio phandle

Changes for v10:
- None

Changes for v11:
- None
---
 .../bindings/net/nxp,imx28-mtip-switch.yaml   | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml

diff --git a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
new file mode 100644
index 000000000000..35f1fe268de7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
+
+maintainers:
+  - Lukasz Majewski <lukma@denx.de>
+
+description:
+  The 2-port switch ethernet subsystem provides ethernet packet (L2)
+  communication and can be configured as an ethernet switch. It provides the
+  reduced media independent interface (RMII), the management data input
+  output (MDIO) for physical layer device (PHY) management.
+
+properties:
+  compatible:
+    const: nxp,imx28-mtip-switch
+
+  reg:
+    maxItems: 1
+
+  phy-supply:
+    description:
+      Regulator that powers Ethernet PHYs.
+
+  clocks:
+    items:
+      - description: Register accessing clock
+      - description: Bus access clock
+      - description: Output clock for external device - e.g. PHY source clock
+      - description: IEEE1588 timer clock
+
+  clock-names:
+    items:
+      - const: ipg
+      - const: ahb
+      - const: enet_out
+      - const: ptp
+
+  interrupts:
+    items:
+      - description: Switch interrupt
+      - description: ENET0 interrupt
+      - description: ENET1 interrupt
+
+  interrupt-names:
+    items:
+      - const: enet_switch
+      - const: enet0
+      - const: enet1
+
+  pinctrl-names: true
+
+  ethernet-ports:
+    type: object
+    $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
+    additionalProperties: true
+
+    patternProperties:
+      '^ethernet-port@[12]$':
+        type: object
+        additionalProperties: true
+        properties:
+          reg:
+            items:
+              - enum: [1, 2]
+            description: MTIP L2 switch port number
+
+        required:
+          - reg
+          - label
+          - phy-mode
+          - phy-handle
+
+  mdio:
+    type: object
+    $ref: mdio.yaml#
+    unevaluatedProperties: false
+    description:
+      Specifies the mdio bus in the switch, used as a container for phy nodes.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - mdio
+  - ethernet-ports
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include<dt-bindings/interrupt-controller/irq.h>
+    #include<dt-bindings/gpio/gpio.h>
+    switch@800f0000 {
+        compatible = "nxp,imx28-mtip-switch";
+        reg = <0x800f0000 0x20000>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
+        phy-supply = <&reg_fec_3v3>;
+        interrupts = <100>, <101>, <102>;
+        interrupt-names = "enet_switch", "enet0", "enet1";
+        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
+        clock-names = "ipg", "ahb", "enet_out", "ptp";
+
+        ethernet-ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            mtip_port1: ethernet-port@1 {
+                reg = <1>;
+                label = "lan0";
+                local-mac-address = [ 00 00 00 00 00 00 ];
+                phy-mode = "rmii";
+                phy-handle = <&ethphy0>;
+            };
+
+            mtip_port2: ethernet-port@2 {
+                reg = <2>;
+                label = "lan1";
+                local-mac-address = [ 00 00 00 00 00 00 ];
+                phy-mode = "rmii";
+                phy-handle = <&ethphy1>;
+            };
+        };
+
+        mdio_sw: mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            reset-gpios = <&gpio2 13 GPIO_ACTIVE_LOW>;
+            reset-delay-us = <25000>;
+            reset-post-delay-us = <10000>;
+
+            ethphy0: ethernet-phy@0 {
+                reg = <0>;
+            };
+
+            ethphy1: ethernet-phy@1 {
+                reg = <1>;
+            };
+        };
+    };
-- 
2.39.5

[net-next v11 2/7] ARM: dts: nxp: mxs: Adjust the imx28.dtsi L2 switch description

[Lukasz Majewski]

The current range of 'reg' property is too small to allow full control
of the L2 switch on imx287.

As this IP block also uses ENET-MAC blocks for its operation, the address
range for it must be included as well.

Moreover, some SoC common properties (like compatible, clocks, interrupts
numbers) have been moved to this node.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

---
Changes for v2:
- adding extra properties (like compatible, clocks, interupts)

Changes for v3:
- None

Changes for v4:
- Rename imx287 with imx28 (as the former is not used in kernel anymore)

Changes for v5:
- None

Changes for v6:
- Add interrupt-names property

Changes for v7:
- Change switch interrupt name from 'mtipl2sw' to 'enet_switch'

Changes for v8:
- None

Changes for v9:
- None

Changes for v10:
- None

Changes for v11:
- None
---
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
index bbea8b77386f..8aff2e87980e 100644
--- a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
+++ b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
@@ -1321,8 +1321,13 @@ mac1: ethernet@800f4000 {
 			status = "disabled";
 		};
 
-		eth_switch: switch@800f8000 {
-			reg = <0x800f8000 0x8000>;
+		eth_switch: switch@800f0000 {
+			compatible = "nxp,imx28-mtip-switch";
+			reg = <0x800f0000 0x20000>;
+			interrupts = <100>, <101>, <102>;
+			interrupt-names = "enet_switch", "enet0", "enet1";
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
+			clock-names = "ipg", "ahb", "enet_out", "ptp";
 			status = "disabled";
 		};
 	};
-- 
2.39.5

[net-next v11 3/7] ARM: dts: nxp: mxs: Adjust XEA board's DTS to support L2 switch

[Lukasz Majewski]

The description is similar to the one used with the new CPSW driver.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>

---
Changes for v2:
- Remove properties which are common for the imx28(7) SoC
- Use mdio properties to perform L2 switch reset (avoid using
  deprecated properties)

Changes for v3:
- Replace IRQ_TYPE_EDGE_FALLING with IRQ_TYPE_LEVEL_LOW
- Update comment regarding PHY interrupts s/AND/OR/g

Changes for v4:
- Use GPIO_ACTIVE_LOW instead of 0 in 'reset-gpios'
- Replace port@[12] with ethernet-port@[12]

Changes for v5:
- Add proper multiline comment for IRQs description

Changes for v6:
- None

Changes for v7:
- None

Changes for v8:
- None

Changes for v9:
- None

Changes for v10:
- None

Changes for v11:
- None
---
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56 +++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
index 6c5e6856648a..69032b29d767 100644
--- a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
+++ b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
@@ -5,6 +5,7 @@
  */
 
 /dts-v1/;
+#include<dt-bindings/interrupt-controller/irq.h>
 #include "imx28-lwe.dtsi"
 
 / {
@@ -90,6 +91,61 @@ &reg_usb_5v {
 	gpio = <&gpio0 2 0>;
 };
 
+&eth_switch {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
+	phy-supply = <&reg_fec_3v3>;
+	status = "okay";
+
+	ethernet-ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mtip_port1: ethernet-port@1 {
+			reg = <1>;
+			label = "lan0";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy0>;
+		};
+
+		mtip_port2: ethernet-port@2 {
+			reg = <2>;
+			label = "lan1";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy1>;
+		};
+	};
+
+	mdio_sw: mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
+		reset-delay-us = <25000>;
+		reset-post-delay-us = <10000>;
+
+		ethphy0: ethernet-phy@0 {
+			reg = <0>;
+			smsc,disable-energy-detect;
+			/*
+			 * Both PHYs (i.e. 0,1) have the same, single GPIO,
+			 * line to handle both, their interrupts (OR'ed)
+			 */
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+		};
+
+		ethphy1: ethernet-phy@1 {
+			reg = <1>;
+			smsc,disable-energy-detect;
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+		};
+	};
+};
+
 &spi2_pins_a {
 	fsl,pinmux-ids = <
 		MX28_PAD_SSP2_SCK__SSP2_SCK
-- 
2.39.5

[net-next v11 5/7] ARM: mxs_defconfig: Enable CONFIG_NFS_FSCACHE

[Lukasz Majewski]

It is not possible to enable by user the CONFIG_NETFS_SUPPORT anymore and
hence it depends on CONFIG_NFS_FSCACHE being enabled.

This patch fixes potential performance regression for NFS on the mxs
devices.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
Suggested-by: Stefan Wahren <wahrenst@gmx.net>

---
Changes for v6:
- New patch

Changes for v7:
- None

Changes for v8:
- None

Changes for v9:
- None

Changes for v10:
- None

Changes for v11:
- None
---
 arch/arm/configs/mxs_defconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index c76d66135abb..22f7639f61fe 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -138,8 +138,6 @@ CONFIG_PWM_MXS=y
 CONFIG_NVMEM_MXS_OCOTP=y
 CONFIG_EXT4_FS=y
 # CONFIG_DNOTIFY is not set
-CONFIG_NETFS_SUPPORT=m
-CONFIG_FSCACHE=y
 CONFIG_FSCACHE_STATS=y
 CONFIG_CACHEFILES=m
 CONFIG_VFAT_FS=y
@@ -155,6 +153,7 @@ CONFIG_NFS_FS=y
 CONFIG_NFS_V3_ACL=y
 CONFIG_NFS_V4=y
 CONFIG_ROOT_NFS=y
+CONFIG_NFS_FSCACHE=y
 CONFIG_NLS_CODEPAGE_437=y
 CONFIG_NLS_CODEPAGE_850=y
 CONFIG_NLS_ISO8859_1=y
-- 
2.39.5

[net-next v11 6/7] ARM: mxs_defconfig: Update mxs_defconfig to 6.15-rc1

[Lukasz Majewski]

This file is the updated version of mxs_defconfig for the v6.15-rc1
linux-next.

Detailed description of removed configuration entries:
-CONFIG_MTD_M25P80=y -> it has been replaced MTD_SPI_NOR (which is enabled)
-CONFIG_SMSC_PHY=y -> is enabled implicit by USB_NET_SMSC95XX
-CONFIG_GPIO_SYSFS=y -> it has been deprecated by moving to EXPERT and
                        its replacement GPIO_CDEV is enabled by default

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
Suggested-by: Stefan Wahren <wahrenst@gmx.net>

---
Changes for v5:
- New patch

Changes for v6:
- Add detailed description on the removed configuration options
  after update

Changes for v7:
- None

Changes for v8:
- None

Changes for v9:
- None

Changes for v10:
- None

Changes for v11:
- None
---
 arch/arm/configs/mxs_defconfig | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index 22f7639f61fe..b1a31cb914c8 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -32,9 +32,6 @@ CONFIG_INET=y
 CONFIG_IP_PNP=y
 CONFIG_IP_PNP_DHCP=y
 CONFIG_SYN_COOKIES=y
-# CONFIG_INET_XFRM_MODE_TRANSPORT is not set
-# CONFIG_INET_XFRM_MODE_TUNNEL is not set
-# CONFIG_INET_XFRM_MODE_BEET is not set
 # CONFIG_INET_DIAG is not set
 # CONFIG_IPV6 is not set
 CONFIG_CAN=m
@@ -45,7 +42,6 @@ CONFIG_MTD=y
 CONFIG_MTD_CMDLINE_PARTS=y
 CONFIG_MTD_BLOCK=y
 CONFIG_MTD_DATAFLASH=y
-CONFIG_MTD_M25P80=y
 CONFIG_MTD_SST25L=y
 CONFIG_MTD_RAW_NAND=y
 CONFIG_MTD_NAND_GPMI_NAND=y
@@ -60,7 +56,6 @@ CONFIG_ENC28J60=y
 CONFIG_ICPLUS_PHY=y
 CONFIG_MICREL_PHY=y
 CONFIG_REALTEK_PHY=y
-CONFIG_SMSC_PHY=y
 CONFIG_CAN_FLEXCAN=m
 CONFIG_USB_USBNET=y
 CONFIG_USB_NET_SMSC95XX=y
@@ -77,13 +72,11 @@ CONFIG_SERIAL_AMBA_PL011=y
 CONFIG_SERIAL_AMBA_PL011_CONSOLE=y
 CONFIG_SERIAL_MXS_AUART=y
 # CONFIG_HW_RANDOM is not set
-# CONFIG_I2C_COMPAT is not set
 CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_MXS=y
 CONFIG_SPI=y
 CONFIG_SPI_GPIO=m
 CONFIG_SPI_MXS=y
-CONFIG_GPIO_SYSFS=y
 # CONFIG_HWMON is not set
 CONFIG_WATCHDOG=y
 CONFIG_STMP3XXX_RTC_WATCHDOG=y
-- 
2.39.5

[net-next v11 7/7] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Lukasz Majewski]

This patch enables support for More Than IP L2 switch available on some
imx28[7] devices.

Moreover, it also enables CONFIG_SWITCHDEV and CONFIG_BRIDGE required
by this driver for correct operation.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
---
Changes for v4:
- New patch

Changes for v5:
- Apply this patch on top of patch, which updates mxs_defconfig to
  v6.15-rc1
- Add more verbose commit message with explanation why SWITCHDEV and
  BRIDGE must be enabled as well

Changes for v6:
- None

Changes for v7:
- None

Changes for v8:
- None

Changes for v9:
- None

Changes for v10:
- None

Changes for v11:
- None
---
 arch/arm/configs/mxs_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index b1a31cb914c8..ef4556222274 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -34,6 +34,8 @@ CONFIG_IP_PNP_DHCP=y
 CONFIG_SYN_COOKIES=y
 # CONFIG_INET_DIAG is not set
 # CONFIG_IPV6 is not set
+CONFIG_BRIDGE=y
+CONFIG_NET_SWITCHDEV=y
 CONFIG_CAN=m
 # CONFIG_WIRELESS is not set
 CONFIG_DEVTMPFS=y
@@ -52,6 +54,7 @@ CONFIG_EEPROM_AT24=y
 CONFIG_SCSI=y
 CONFIG_BLK_DEV_SD=y
 CONFIG_NETDEVICES=y
+CONFIG_FEC_MTIP_L2SW=y
 CONFIG_ENC28J60=y
 CONFIG_ICPLUS_PHY=y
 CONFIG_MICREL_PHY=y
-- 
2.39.5

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Paolo Abeni]

On 5/4/25 4:55 PM, Lukasz Majewski wrote:
> +		/* This does 16 byte alignment, exactly what we need.
> +		 * The packet length includes FCS, but we don't want to
> +		 * include that when passing upstream as it messes up
> +		 * bridging applications.
> +		 */
> +		skb = netdev_alloc_skb(pndev, pkt_len + NET_IP_ALIGN);
> +		if (unlikely(!skb)) {
> +			dev_dbg(&fep->pdev->dev,
> +				"%s: Memory squeeze, dropping packet.\n",
> +				pndev->name);
> +			pndev->stats.rx_dropped++;
> +			goto err_mem;
> +		} else {
> +			skb_reserve(skb, NET_IP_ALIGN);
> +			skb_put(skb, pkt_len);      /* Make room */
> +			skb_copy_to_linear_data(skb, data, pkt_len);
> +			skb->protocol = eth_type_trans(skb, pndev);
> +			napi_gro_receive(&fep->napi, skb);
> +		}
> +
> +		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, data,
> +						  bdp->cbd_datlen,
> +						  DMA_FROM_DEVICE);
> +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> +					       bdp->cbd_bufaddr))) {
> +			dev_err(&fep->pdev->dev,
> +				"Failed to map descriptor rx buffer\n");
> +			pndev->stats.rx_errors++;
> +			pndev->stats.rx_dropped++;
> +			dev_kfree_skb_any(skb);
> +			goto err_mem;
> +		}

This is doing the mapping and ev. dropping the skb _after_ pushing the
skb up the stack, you must attempt the mapping first.

> +static void mtip_free_buffers(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct sk_buff *skb;
> +	struct cbd_t *bdp;
> +	int i;
> +
> +	bdp = fep->rx_bd_base;
> +	for (i = 0; i < RX_RING_SIZE; i++) {
> +		skb = fep->rx_skbuff[i];
> +
> +		if (bdp->cbd_bufaddr)
> +			dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
> +					 MTIP_SWITCH_RX_FRSIZE,
> +					 DMA_FROM_DEVICE);
> +		if (skb)
> +			dev_kfree_skb(skb);

I suspect that on error paths mtip_free_buffers() can be invoked
multiple consecutive times with any successful allocation in between:
skb will be freed twice. Likely you need to clear fep->rx_skbuff[i] here.

Side note: this patch is way too big for a proper review: you need to
break it in multiple smaller ones, introducing the basic features
separately.

Cheers,

Paolo

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

Hi Paolo,

> On 5/4/25 4:55 PM, Lukasz Majewski wrote:
> > +		/* This does 16 byte alignment, exactly what we
> > need.
> > +		 * The packet length includes FCS, but we don't
> > want to
> > +		 * include that when passing upstream as it messes
> > up
> > +		 * bridging applications.
> > +		 */
> > +		skb = netdev_alloc_skb(pndev, pkt_len +
> > NET_IP_ALIGN);
> > +		if (unlikely(!skb)) {
> > +			dev_dbg(&fep->pdev->dev,
> > +				"%s: Memory squeeze, dropping
> > packet.\n",
> > +				pndev->name);
> > +			pndev->stats.rx_dropped++;
> > +			goto err_mem;
> > +		} else {
> > +			skb_reserve(skb, NET_IP_ALIGN);
> > +			skb_put(skb, pkt_len);      /* Make room */
> > +			skb_copy_to_linear_data(skb, data,
> > pkt_len);
> > +			skb->protocol = eth_type_trans(skb, pndev);
> > +			napi_gro_receive(&fep->napi, skb);
> > +		}
> > +
> > +		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev,
> > data,
> > +						  bdp->cbd_datlen,
> > +						  DMA_FROM_DEVICE);
> > +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> > +					       bdp->cbd_bufaddr)))
> > {
> > +			dev_err(&fep->pdev->dev,
> > +				"Failed to map descriptor rx
> > buffer\n");
> > +			pndev->stats.rx_errors++;
> > +			pndev->stats.rx_dropped++;
> > +			dev_kfree_skb_any(skb);
> > +			goto err_mem;
> > +		}  
> 
> This is doing the mapping and ev. dropping the skb _after_ pushing the
> skb up the stack, you must attempt the mapping first.
> 
> > +static void mtip_free_buffers(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct sk_buff *skb;
> > +	struct cbd_t *bdp;
> > +	int i;
> > +
> > +	bdp = fep->rx_bd_base;
> > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > +		skb = fep->rx_skbuff[i];
> > +
> > +		if (bdp->cbd_bufaddr)
> > +			dma_unmap_single(&fep->pdev->dev,
> > bdp->cbd_bufaddr,
> > +					 MTIP_SWITCH_RX_FRSIZE,
> > +					 DMA_FROM_DEVICE);
> > +		if (skb)
> > +			dev_kfree_skb(skb);  
> 
> I suspect that on error paths mtip_free_buffers() can be invoked
> multiple consecutive times with any successful allocation in between:
> skb will be freed twice. Likely you need to clear fep->rx_skbuff[i]
> here.

I don't know what I shall say now.... really... 

> 
> Side note: this patch is way too big for a proper review: you need to
> break it in multiple smaller ones, introducing the basic features
> separately.
> 

This code is a basic version of the driver as discussed on March with
the community (Andrew).

It provides the basic functionality - like separate ports support and
then, if required, configures the IP block to perform L2 switching in
HW. 

> Cheers,
> 
> Paolo
> 




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Paolo Abeni]

On 5/6/25 1:04 PM, Lukasz Majewski wrote:
>> On 5/4/25 4:55 PM, Lukasz Majewski wrote:
>>> +		/* This does 16 byte alignment, exactly what we
>>> need.
>>> +		 * The packet length includes FCS, but we don't
>>> want to
>>> +		 * include that when passing upstream as it messes
>>> up
>>> +		 * bridging applications.
>>> +		 */
>>> +		skb = netdev_alloc_skb(pndev, pkt_len +
>>> NET_IP_ALIGN);
>>> +		if (unlikely(!skb)) {
>>> +			dev_dbg(&fep->pdev->dev,
>>> +				"%s: Memory squeeze, dropping
>>> packet.\n",
>>> +				pndev->name);
>>> +			pndev->stats.rx_dropped++;
>>> +			goto err_mem;
>>> +		} else {
>>> +			skb_reserve(skb, NET_IP_ALIGN);
>>> +			skb_put(skb, pkt_len);      /* Make room */
>>> +			skb_copy_to_linear_data(skb, data,
>>> pkt_len);
>>> +			skb->protocol = eth_type_trans(skb, pndev);
>>> +			napi_gro_receive(&fep->napi, skb);
>>> +		}
>>> +
>>> +		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev,
>>> data,
>>> +						  bdp->cbd_datlen,
>>> +						  DMA_FROM_DEVICE);
>>> +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
>>> +					       bdp->cbd_bufaddr)))
>>> {
>>> +			dev_err(&fep->pdev->dev,
>>> +				"Failed to map descriptor rx
>>> buffer\n");
>>> +			pndev->stats.rx_errors++;
>>> +			pndev->stats.rx_dropped++;
>>> +			dev_kfree_skb_any(skb);
>>> +			goto err_mem;
>>> +		}  
>>
>> This is doing the mapping and ev. dropping the skb _after_ pushing the
>> skb up the stack, you must attempt the mapping first.
>>
>>> +static void mtip_free_buffers(struct net_device *dev)
>>> +{
>>> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
>>> +	struct switch_enet_private *fep = priv->fep;
>>> +	struct sk_buff *skb;
>>> +	struct cbd_t *bdp;
>>> +	int i;
>>> +
>>> +	bdp = fep->rx_bd_base;
>>> +	for (i = 0; i < RX_RING_SIZE; i++) {
>>> +		skb = fep->rx_skbuff[i];
>>> +
>>> +		if (bdp->cbd_bufaddr)
>>> +			dma_unmap_single(&fep->pdev->dev,
>>> bdp->cbd_bufaddr,
>>> +					 MTIP_SWITCH_RX_FRSIZE,
>>> +					 DMA_FROM_DEVICE);
>>> +		if (skb)
>>> +			dev_kfree_skb(skb);  
>>
>> I suspect that on error paths mtip_free_buffers() can be invoked
>> multiple consecutive times with any successful allocation in between:
>> skb will be freed twice. Likely you need to clear fep->rx_skbuff[i]
>> here.
> 
> I don't know what I shall say now.... really... 

I suspect my email was not clear. AFAICS the current code contains at
least 2 serious issues, possibly more not yet discovered due to the
patch size. You need to submit (at least) a new revision coping with the
provided feedback.

Thanks,

Paolo

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3152 bytes --]

Hi Paolo,

> On 5/6/25 1:04 PM, Lukasz Majewski wrote:
> >> On 5/4/25 4:55 PM, Lukasz Majewski wrote:  
> >>> +		/* This does 16 byte alignment, exactly what we
> >>> need.
> >>> +		 * The packet length includes FCS, but we don't
> >>> want to
> >>> +		 * include that when passing upstream as it
> >>> messes up
> >>> +		 * bridging applications.
> >>> +		 */
> >>> +		skb = netdev_alloc_skb(pndev, pkt_len +
> >>> NET_IP_ALIGN);
> >>> +		if (unlikely(!skb)) {
> >>> +			dev_dbg(&fep->pdev->dev,
> >>> +				"%s: Memory squeeze, dropping
> >>> packet.\n",
> >>> +				pndev->name);
> >>> +			pndev->stats.rx_dropped++;
> >>> +			goto err_mem;
> >>> +		} else {
> >>> +			skb_reserve(skb, NET_IP_ALIGN);
> >>> +			skb_put(skb, pkt_len);      /* Make room
> >>> */
> >>> +			skb_copy_to_linear_data(skb, data,
> >>> pkt_len);
> >>> +			skb->protocol = eth_type_trans(skb,
> >>> pndev);
> >>> +			napi_gro_receive(&fep->napi, skb);
> >>> +		}
> >>> +
> >>> +		bdp->cbd_bufaddr =
> >>> dma_map_single(&fep->pdev->dev, data,
> >>> +
> >>> bdp->cbd_datlen,
> >>> +
> >>> DMA_FROM_DEVICE);
> >>> +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> >>> +
> >>> bdp->cbd_bufaddr))) {
> >>> +			dev_err(&fep->pdev->dev,
> >>> +				"Failed to map descriptor rx
> >>> buffer\n");
> >>> +			pndev->stats.rx_errors++;
> >>> +			pndev->stats.rx_dropped++;
> >>> +			dev_kfree_skb_any(skb);
> >>> +			goto err_mem;
> >>> +		}    
> >>
> >> This is doing the mapping and ev. dropping the skb _after_ pushing
> >> the skb up the stack, you must attempt the mapping first.
> >>  
> >>> +static void mtip_free_buffers(struct net_device *dev)
> >>> +{
> >>> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> >>> +	struct switch_enet_private *fep = priv->fep;
> >>> +	struct sk_buff *skb;
> >>> +	struct cbd_t *bdp;
> >>> +	int i;
> >>> +
> >>> +	bdp = fep->rx_bd_base;
> >>> +	for (i = 0; i < RX_RING_SIZE; i++) {
> >>> +		skb = fep->rx_skbuff[i];
> >>> +
> >>> +		if (bdp->cbd_bufaddr)
> >>> +			dma_unmap_single(&fep->pdev->dev,
> >>> bdp->cbd_bufaddr,
> >>> +					 MTIP_SWITCH_RX_FRSIZE,
> >>> +					 DMA_FROM_DEVICE);
> >>> +		if (skb)
> >>> +			dev_kfree_skb(skb);    
> >>
> >> I suspect that on error paths mtip_free_buffers() can be invoked
> >> multiple consecutive times with any successful allocation in
> >> between: skb will be freed twice. Likely you need to clear
> >> fep->rx_skbuff[i] here.  
> > 
> > I don't know what I shall say now.... really...   
> 
> I suspect my email was not clear. AFAICS the current code contains at
> least 2 serious issues,

Yes, I'm now aware of them - thanks for pointing them out.

> possibly more not yet discovered due to the
> patch size.
> You need to submit (at least) a new revision coping with
> the provided feedback.

+1


> 
> Thanks,
> 
> Paolo
> 




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 1/7] dt-bindings: net: Add MTIP L2 switch description

[Rob Herring]

On Sun, May 04, 2025 at 04:55:32PM +0200, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - e.g. imx287.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> 
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)
> 
> Changes for v3:
> - Remove '-' from const:'nxp,imx287-mtip-switch'
> - Use '^port@[12]+$' for port patternProperties
> - Drop status = "okay";
> - Provide proper indentation for 'example' binding (replace 8
>   spaces with 4 spaces)
> - Remove smsc,disable-energy-detect; property
> - Remove interrupt-parent and interrupts properties as not required
> - Remove #address-cells and #size-cells from required properties check
> - remove description from reg:
> - Add $ref: ethernet-switch.yaml#
> 
> Changes for v4:
> - Use $ref: ethernet-switch.yaml#/$defs/ethernet-ports and remove already
>   referenced properties
> - Rename file to nxp,imx28-mtip-switch.yaml
> 
> Changes for v5:
> - Provide proper description for 'ethernet-port' node
> 
> Changes for v6:
> - Proper usage of
>   $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
>   when specifying the 'ethernet-ports' property
> - Add description and check for interrupt-names property
> 
> Changes for v7:
> - Change switch interrupt name from 'mtipl2sw' to 'enet_switch'
> 
> Changes for v8:
> - None
> 
> Changes for v9:
> - Add GPIO_ACTIVE_LOW to reset-gpios mdio phandle
> 
> Changes for v10:
> - None
> 
> Changes for v11:
> - None
> ---
>  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> new file mode 100644
> index 000000000000..35f1fe268de7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +
> +description:
> +  The 2-port switch ethernet subsystem provides ethernet packet (L2)
> +  communication and can be configured as an ethernet switch. It provides the
> +  reduced media independent interface (RMII), the management data input
> +  output (MDIO) for physical layer device (PHY) management.
> +
> +properties:
> +  compatible:
> +    const: nxp,imx28-mtip-switch
> +
> +  reg:
> +    maxItems: 1
> +
> +  phy-supply:
> +    description:
> +      Regulator that powers Ethernet PHYs.
> +
> +  clocks:
> +    items:
> +      - description: Register accessing clock
> +      - description: Bus access clock
> +      - description: Output clock for external device - e.g. PHY source clock
> +      - description: IEEE1588 timer clock
> +
> +  clock-names:
> +    items:
> +      - const: ipg
> +      - const: ahb
> +      - const: enet_out
> +      - const: ptp
> +
> +  interrupts:
> +    items:
> +      - description: Switch interrupt
> +      - description: ENET0 interrupt
> +      - description: ENET1 interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: enet_switch
> +      - const: enet0
> +      - const: enet1
> +
> +  pinctrl-names: true
> +
> +  ethernet-ports:
> +    type: object
> +    $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties

'patternProperties' is wrong. Drop.

> +    additionalProperties: true

Drop.

> +
> +    patternProperties:
> +      '^ethernet-port@[12]$':
> +        type: object
> +        additionalProperties: true
> +        properties:
> +          reg:
> +            items:
> +              - enum: [1, 2]
> +            description: MTIP L2 switch port number
> +
> +        required:
> +          - reg
> +          - label

'label' shouldn't ever be required because s/w shouldn't care what the 
value is.

> +          - phy-mode
> +          - phy-handle
> +
> +  mdio:
> +    type: object
> +    $ref: mdio.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      Specifies the mdio bus in the switch, used as a container for phy nodes.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - mdio
> +  - ethernet-ports
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include<dt-bindings/interrupt-controller/irq.h>
> +    #include<dt-bindings/gpio/gpio.h>
> +    switch@800f0000 {
> +        compatible = "nxp,imx28-mtip-switch";
> +        reg = <0x800f0000 0x20000>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> +        phy-supply = <&reg_fec_3v3>;
> +        interrupts = <100>, <101>, <102>;
> +        interrupt-names = "enet_switch", "enet0", "enet1";
> +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> +
> +        ethernet-ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            mtip_port1: ethernet-port@1 {
> +                reg = <1>;
> +                label = "lan0";
> +                local-mac-address = [ 00 00 00 00 00 00 ];
> +                phy-mode = "rmii";
> +                phy-handle = <&ethphy0>;
> +            };
> +
> +            mtip_port2: ethernet-port@2 {
> +                reg = <2>;
> +                label = "lan1";
> +                local-mac-address = [ 00 00 00 00 00 00 ];
> +                phy-mode = "rmii";
> +                phy-handle = <&ethphy1>;
> +            };
> +        };
> +
> +        mdio_sw: mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            reset-gpios = <&gpio2 13 GPIO_ACTIVE_LOW>;
> +            reset-delay-us = <25000>;
> +            reset-post-delay-us = <10000>;
> +
> +            ethphy0: ethernet-phy@0 {
> +                reg = <0>;
> +            };
> +
> +            ethphy1: ethernet-phy@1 {
> +                reg = <1>;
> +            };
> +        };
> +    };
> -- 
> 2.39.5
> 

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

Hi Paolo,

> On 5/4/25 4:55 PM, Lukasz Majewski wrote:
> > +		/* This does 16 byte alignment, exactly what we
> > need.
> > +		 * The packet length includes FCS, but we don't
> > want to
> > +		 * include that when passing upstream as it messes
> > up
> > +		 * bridging applications.
> > +		 */
> > +		skb = netdev_alloc_skb(pndev, pkt_len +
> > NET_IP_ALIGN);
> > +		if (unlikely(!skb)) {
> > +			dev_dbg(&fep->pdev->dev,
> > +				"%s: Memory squeeze, dropping
> > packet.\n",
> > +				pndev->name);
> > +			pndev->stats.rx_dropped++;
> > +			goto err_mem;
> > +		} else {
> > +			skb_reserve(skb, NET_IP_ALIGN);
> > +			skb_put(skb, pkt_len);      /* Make room */
> > +			skb_copy_to_linear_data(skb, data,
> > pkt_len);
> > +			skb->protocol = eth_type_trans(skb, pndev);
> > +			napi_gro_receive(&fep->napi, skb);
> > +		}
> > +
> > +		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev,
> > data,
> > +						  bdp->cbd_datlen,
> > +						  DMA_FROM_DEVICE);
> > +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> > +					       bdp->cbd_bufaddr)))
> > {
> > +			dev_err(&fep->pdev->dev,
> > +				"Failed to map descriptor rx
> > buffer\n");
> > +			pndev->stats.rx_errors++;
> > +			pndev->stats.rx_dropped++;
> > +			dev_kfree_skb_any(skb);
> > +			goto err_mem;
> > +		}  
> 
> This is doing the mapping and ev. dropping the skb _after_ pushing the
> skb up the stack, you must attempt the mapping first.

I've double check it - the code seems to be correct.

This code is a part of mtip_switch_rx() function, which handles
receiving data.

First, on probe, the initial dma memory is mapped for MTIP received
data.

When we receive data, it is processed and afterwards it is "pushed" up
to the network stack.

As a last step we do map memory for next, incoming data and leave the
function.

Hence, IMHO, the order is OK and this part shall be left as is.

> 
> > +static void mtip_free_buffers(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct sk_buff *skb;
> > +	struct cbd_t *bdp;
> > +	int i;
> > +
> > +	bdp = fep->rx_bd_base;
> > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > +		skb = fep->rx_skbuff[i];
> > +
> > +		if (bdp->cbd_bufaddr)
> > +			dma_unmap_single(&fep->pdev->dev,
> > bdp->cbd_bufaddr,
> > +					 MTIP_SWITCH_RX_FRSIZE,
> > +					 DMA_FROM_DEVICE);
> > +		if (skb)
> > +			dev_kfree_skb(skb);  
> 
> I suspect that on error paths mtip_free_buffers() can be invoked
> multiple consecutive times with any successful allocation in between:
> skb will be freed twice. Likely you need to clear fep->rx_skbuff[i]
> here.

+1 - I will add it with v12.

> 
> Side note: this patch is way too big for a proper review: you need to
> break it in multiple smaller ones, introducing the basic features
> separately.
> 
> Cheers,
> 
> Paolo
> 




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 1/7] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 8590 bytes --]

Hi Rob,

> On Sun, May 04, 2025 at 04:55:32PM +0200, Lukasz Majewski wrote:
> > This patch provides description of the MTIP L2 switch available in
> > some NXP's SOCs - e.g. imx287.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> > 
> > ---
> > Changes for v2:
> > - Rename the file to match exactly the compatible
> >   (nxp,imx287-mtip-switch)
> > 
> > Changes for v3:
> > - Remove '-' from const:'nxp,imx287-mtip-switch'
> > - Use '^port@[12]+$' for port patternProperties
> > - Drop status = "okay";
> > - Provide proper indentation for 'example' binding (replace 8
> >   spaces with 4 spaces)
> > - Remove smsc,disable-energy-detect; property
> > - Remove interrupt-parent and interrupts properties as not required
> > - Remove #address-cells and #size-cells from required properties
> > check
> > - remove description from reg:
> > - Add $ref: ethernet-switch.yaml#
> > 
> > Changes for v4:
> > - Use $ref: ethernet-switch.yaml#/$defs/ethernet-ports and remove
> > already referenced properties
> > - Rename file to nxp,imx28-mtip-switch.yaml
> > 
> > Changes for v5:
> > - Provide proper description for 'ethernet-port' node
> > 
> > Changes for v6:
> > - Proper usage of
> >   $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
> >   when specifying the 'ethernet-ports' property
> > - Add description and check for interrupt-names property
> > 
> > Changes for v7:
> > - Change switch interrupt name from 'mtipl2sw' to 'enet_switch'
> > 
> > Changes for v8:
> > - None
> > 
> > Changes for v9:
> > - Add GPIO_ACTIVE_LOW to reset-gpios mdio phandle
> > 
> > Changes for v10:
> > - None
> > 
> > Changes for v11:
> > - None
> > ---
> >  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 149
> > ++++++++++++++++++ 1 file changed, 149 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > new file mode 100644 index 000000000000..35f1fe268de7 --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > @@ -0,0 +1,149 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > BSD-2-Clause) +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@denx.de>
> > +
> > +description:
> > +  The 2-port switch ethernet subsystem provides ethernet packet
> > (L2)
> > +  communication and can be configured as an ethernet switch. It
> > provides the
> > +  reduced media independent interface (RMII), the management data
> > input
> > +  output (MDIO) for physical layer device (PHY) management.
> > +
> > +properties:
> > +  compatible:
> > +    const: nxp,imx28-mtip-switch
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  phy-supply:
> > +    description:
> > +      Regulator that powers Ethernet PHYs.
> > +
> > +  clocks:
> > +    items:
> > +      - description: Register accessing clock
> > +      - description: Bus access clock
> > +      - description: Output clock for external device - e.g. PHY
> > source clock
> > +      - description: IEEE1588 timer clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ipg
> > +      - const: ahb
> > +      - const: enet_out
> > +      - const: ptp
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Switch interrupt
> > +      - description: ENET0 interrupt
> > +      - description: ENET1 interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: enet_switch
> > +      - const: enet0
> > +      - const: enet1
> > +
> > +  pinctrl-names: true
> > +
> > +  ethernet-ports:
> > +    type: object
> > +    $ref:
> > ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties  
> 
> 'patternProperties' is wrong. Drop.
> 

When I do drop it, then
make dt_binding_check DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml

shows:

nxp,imx28-mtip-switch.example.dtb: switch@800f0000: ethernet-ports:
'oneOf' conditional failed, one must be fixed:

'ports' is a required property 
'ethernet-ports' is a required property
        from schema $id:
        http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#

We do have ethernet-ports:
and we do "include" ($ref)
https://elixir.bootlin.com/linux/v6.14.6/source/Documentation/devicetree/bindings/net/ethernet-switch.yaml#L77

which is what we exactly need.

> > +    additionalProperties: true  
> 
> Drop.
> 

When removed we do have:
nxp,imx28-mtip-switch.example.dtb: switch@800f0000: Unevaluated
properties are not allowed ('ethernet-ports' was unexpected)

or 

nxp,imx28-mtip-switch.yaml: ethernet-ports: Missing
additionalProperties/unevaluatedProperties constraint

Depending if I do remove 'patternProperties' above.

To sum up - with the current code, the DT schema checks pass. It also
looks like the $ref for ethernet-switch is used in an optimal way.

I would opt for keeping the code as is for v12.

> > +
> > +    patternProperties:
> > +      '^ethernet-port@[12]$':
> > +        type: object
> > +        additionalProperties: true
> > +        properties:
> > +          reg:
> > +            items:
> > +              - enum: [1, 2]
> > +            description: MTIP L2 switch port number
> > +
> > +        required:
> > +          - reg
> > +          - label  
> 
> 'label' shouldn't ever be required because s/w shouldn't care what
> the value is.

Ok, I will remove it for v12.

> 
> > +          - phy-mode
> > +          - phy-handle
> > +
> > +  mdio:
> > +    type: object
> > +    $ref: mdio.yaml#
> > +    unevaluatedProperties: false
> > +    description:
> > +      Specifies the mdio bus in the switch, used as a container
> > for phy nodes. +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - mdio
> > +  - ethernet-ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/irq.h>
> > +    #include<dt-bindings/gpio/gpio.h>
> > +    switch@800f0000 {
> > +        compatible = "nxp,imx28-mtip-switch";
> > +        reg = <0x800f0000 0x20000>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> > +        phy-supply = <&reg_fec_3v3>;
> > +        interrupts = <100>, <101>, <102>;
> > +        interrupt-names = "enet_switch", "enet0", "enet1";
> > +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> > +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> > +
> > +        ethernet-ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            mtip_port1: ethernet-port@1 {
> > +                reg = <1>;
> > +                label = "lan0";
> > +                local-mac-address = [ 00 00 00 00 00 00 ];
> > +                phy-mode = "rmii";
> > +                phy-handle = <&ethphy0>;
> > +            };
> > +
> > +            mtip_port2: ethernet-port@2 {
> > +                reg = <2>;
> > +                label = "lan1";
> > +                local-mac-address = [ 00 00 00 00 00 00 ];
> > +                phy-mode = "rmii";
> > +                phy-handle = <&ethphy1>;
> > +            };
> > +        };
> > +
> > +        mdio_sw: mdio {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            reset-gpios = <&gpio2 13 GPIO_ACTIVE_LOW>;
> > +            reset-delay-us = <25000>;
> > +            reset-post-delay-us = <10000>;
> > +
> > +            ethphy0: ethernet-phy@0 {
> > +                reg = <0>;
> > +            };
> > +
> > +            ethphy1: ethernet-phy@1 {
> > +                reg = <1>;
> > +            };
> > +        };
> > +    };
> > -- 
> > 2.39.5
> >   




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 1/7] dt-bindings: net: Add MTIP L2 switch description

[Rob Herring]

On Tue, May 13, 2025 at 08:09:20AM +0200, Lukasz Majewski wrote:
> Hi Rob,
> 
> > On Sun, May 04, 2025 at 04:55:32PM +0200, Lukasz Majewski wrote:
> > > This patch provides description of the MTIP L2 switch available in
> > > some NXP's SOCs - e.g. imx287.
> > > 
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> > > 
> > > ---
> > > Changes for v2:
> > > - Rename the file to match exactly the compatible
> > >   (nxp,imx287-mtip-switch)
> > > 
> > > Changes for v3:
> > > - Remove '-' from const:'nxp,imx287-mtip-switch'
> > > - Use '^port@[12]+$' for port patternProperties
> > > - Drop status = "okay";
> > > - Provide proper indentation for 'example' binding (replace 8
> > >   spaces with 4 spaces)
> > > - Remove smsc,disable-energy-detect; property
> > > - Remove interrupt-parent and interrupts properties as not required
> > > - Remove #address-cells and #size-cells from required properties
> > > check
> > > - remove description from reg:
> > > - Add $ref: ethernet-switch.yaml#
> > > 
> > > Changes for v4:
> > > - Use $ref: ethernet-switch.yaml#/$defs/ethernet-ports and remove
> > > already referenced properties
> > > - Rename file to nxp,imx28-mtip-switch.yaml
> > > 
> > > Changes for v5:
> > > - Provide proper description for 'ethernet-port' node
> > > 
> > > Changes for v6:
> > > - Proper usage of
> > >   $ref: ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
> > >   when specifying the 'ethernet-ports' property
> > > - Add description and check for interrupt-names property
> > > 
> > > Changes for v7:
> > > - Change switch interrupt name from 'mtipl2sw' to 'enet_switch'
> > > 
> > > Changes for v8:
> > > - None
> > > 
> > > Changes for v9:
> > > - Add GPIO_ACTIVE_LOW to reset-gpios mdio phandle
> > > 
> > > Changes for v10:
> > > - None
> > > 
> > > Changes for v11:
> > > - None
> > > ---
> > >  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 149
> > > ++++++++++++++++++ 1 file changed, 149 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > new file mode 100644 index 000000000000..35f1fe268de7 --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > @@ -0,0 +1,149 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > > BSD-2-Clause) +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> > > +
> > > +maintainers:
> > > +  - Lukasz Majewski <lukma@denx.de>
> > > +
> > > +description:
> > > +  The 2-port switch ethernet subsystem provides ethernet packet
> > > (L2)
> > > +  communication and can be configured as an ethernet switch. It
> > > provides the
> > > +  reduced media independent interface (RMII), the management data
> > > input
> > > +  output (MDIO) for physical layer device (PHY) management.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: nxp,imx28-mtip-switch
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  phy-supply:
> > > +    description:
> > > +      Regulator that powers Ethernet PHYs.
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: Register accessing clock
> > > +      - description: Bus access clock
> > > +      - description: Output clock for external device - e.g. PHY
> > > source clock
> > > +      - description: IEEE1588 timer clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: ipg
> > > +      - const: ahb
> > > +      - const: enet_out
> > > +      - const: ptp
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: Switch interrupt
> > > +      - description: ENET0 interrupt
> > > +      - description: ENET1 interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: enet_switch
> > > +      - const: enet0
> > > +      - const: enet1
> > > +
> > > +  pinctrl-names: true
> > > +
> > > +  ethernet-ports:
> > > +    type: object
> > > +    $ref:
> > > ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties  
> > 
> > 'patternProperties' is wrong. Drop.
> > 
> 
> When I do drop it, then
> make dt_binding_check DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml
> 
> shows:
> 
> nxp,imx28-mtip-switch.example.dtb: switch@800f0000: ethernet-ports:
> 'oneOf' conditional failed, one must be fixed:
> 
> 'ports' is a required property 
> 'ethernet-ports' is a required property
>         from schema $id:
>         http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#

Actually, it needs to be at the top-level as well:

allOf:
  - $ref: ethernet-switch.yaml#/$defs/ethernet-ports


> 
> We do have ethernet-ports:
> and we do "include" ($ref)
> https://elixir.bootlin.com/linux/v6.14.6/source/Documentation/devicetree/bindings/net/ethernet-switch.yaml#L77
> 
> which is what we exactly need.

The $ref is effectively pasting in what you reference, so the result 
would be:

ethernet-ports:
  type: object
  "^(ethernet-)?ports$":
    patternProperties:
      "^(ethernet-)?port@[0-9a-f]+$":
        description: Ethernet switch ports
        $ref: ethernet-switch-port.yaml#
        unevaluatedProperties: false

A DT node/property name and json-schema keyword at the same level is 
never correct. json-schema behavior is to ignore (silently) any unknown 
keyword. So the validator sees the keyword "^(ethernet-)?ports$" and 
ignores everything below it.


> 
> > > +    additionalProperties: true  
> > 
> > Drop.
> > 
> 
> When removed we do have:
> nxp,imx28-mtip-switch.example.dtb: switch@800f0000: Unevaluated
> properties are not allowed ('ethernet-ports' was unexpected)
> 
> or 
> 
> nxp,imx28-mtip-switch.yaml: ethernet-ports: Missing
> additionalProperties/unevaluatedProperties constraint

'additionalProperties: true' should suppress that.

> 
> Depending if I do remove 'patternProperties' above.
> 
> To sum up - with the current code, the DT schema checks pass. It also
> looks like the $ref for ethernet-switch is used in an optimal way.
> 
> I would opt for keeping the code as is for v12.

Re: [net-next v11 1/7] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 8197 bytes --]

Hi Rob,

Thanks for your detailed explanation.

> On Tue, May 13, 2025 at 08:09:20AM +0200, Lukasz Majewski wrote:
> > Hi Rob,
> >   
> > > On Sun, May 04, 2025 at 04:55:32PM +0200, Lukasz Majewski wrote:  
> > > > This patch provides description of the MTIP L2 switch available
> > > > in some NXP's SOCs - e.g. imx287.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > > > Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> > > > 
> > > > ---
> > > > Changes for v2:
> > > > - Rename the file to match exactly the compatible
> > > >   (nxp,imx287-mtip-switch)
> > > > 
> > > > Changes for v3:
> > > > - Remove '-' from const:'nxp,imx287-mtip-switch'
> > > > - Use '^port@[12]+$' for port patternProperties
> > > > - Drop status = "okay";
> > > > - Provide proper indentation for 'example' binding (replace 8
> > > >   spaces with 4 spaces)
> > > > - Remove smsc,disable-energy-detect; property
> > > > - Remove interrupt-parent and interrupts properties as not
> > > > required
> > > > - Remove #address-cells and #size-cells from required properties
> > > > check
> > > > - remove description from reg:
> > > > - Add $ref: ethernet-switch.yaml#
> > > > 
> > > > Changes for v4:
> > > > - Use $ref: ethernet-switch.yaml#/$defs/ethernet-ports and
> > > > remove already referenced properties
> > > > - Rename file to nxp,imx28-mtip-switch.yaml
> > > > 
> > > > Changes for v5:
> > > > - Provide proper description for 'ethernet-port' node
> > > > 
> > > > Changes for v6:
> > > > - Proper usage of
> > > >   $ref:
> > > > ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties
> > > > when specifying the 'ethernet-ports' property
> > > > - Add description and check for interrupt-names property
> > > > 
> > > > Changes for v7:
> > > > - Change switch interrupt name from 'mtipl2sw' to 'enet_switch'
> > > > 
> > > > Changes for v8:
> > > > - None
> > > > 
> > > > Changes for v9:
> > > > - Add GPIO_ACTIVE_LOW to reset-gpios mdio phandle
> > > > 
> > > > Changes for v10:
> > > > - None
> > > > 
> > > > Changes for v11:
> > > > - None
> > > > ---
> > > >  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 149
> > > > ++++++++++++++++++ 1 file changed, 149 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > > new file mode 100644 index 000000000000..35f1fe268de7 ---
> > > > /dev/null +++
> > > > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > > > @@ -0,0 +1,149 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > > > BSD-2-Clause) +%YAML 1.2
> > > > +---
> > > > +$id:
> > > > http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# +
> > > > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP
> > > > switch) +
> > > > +maintainers:
> > > > +  - Lukasz Majewski <lukma@denx.de>
> > > > +
> > > > +description:
> > > > +  The 2-port switch ethernet subsystem provides ethernet packet
> > > > (L2)
> > > > +  communication and can be configured as an ethernet switch. It
> > > > provides the
> > > > +  reduced media independent interface (RMII), the management
> > > > data input
> > > > +  output (MDIO) for physical layer device (PHY) management.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: nxp,imx28-mtip-switch
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  phy-supply:
> > > > +    description:
> > > > +      Regulator that powers Ethernet PHYs.
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: Register accessing clock
> > > > +      - description: Bus access clock
> > > > +      - description: Output clock for external device - e.g.
> > > > PHY source clock
> > > > +      - description: IEEE1588 timer clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: ipg
> > > > +      - const: ahb
> > > > +      - const: enet_out
> > > > +      - const: ptp
> > > > +
> > > > +  interrupts:
> > > > +    items:
> > > > +      - description: Switch interrupt
> > > > +      - description: ENET0 interrupt
> > > > +      - description: ENET1 interrupt
> > > > +
> > > > +  interrupt-names:
> > > > +    items:
> > > > +      - const: enet_switch
> > > > +      - const: enet0
> > > > +      - const: enet1
> > > > +
> > > > +  pinctrl-names: true
> > > > +
> > > > +  ethernet-ports:
> > > > +    type: object
> > > > +    $ref:
> > > > ethernet-switch.yaml#/$defs/ethernet-ports/patternProperties    
> > > 
> > > 'patternProperties' is wrong. Drop.
> > >   
> > 
> > When I do drop it, then
> > make dt_binding_check DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml
> > 
> > shows:
> > 
> > nxp,imx28-mtip-switch.example.dtb: switch@800f0000: ethernet-ports:
> > 'oneOf' conditional failed, one must be fixed:
> > 
> > 'ports' is a required property 
> > 'ethernet-ports' is a required property
> >         from schema $id:
> >         http://devicetree.org/schemas/net/nxp,imx28-mtip-switch.yaml#
> >  
> 
> Actually, it needs to be at the top-level as well:
> 
> allOf:
>   - $ref: ethernet-switch.yaml#/$defs/ethernet-ports
> 

So after your suggestions - I've add:

allOf:
  - $ref: ethernet-switch.yaml#/$defs/ethernet-ports

In the top level of the file - above of
properties:
  ...
  ethernet-ports:
    type: object
    additionalProperties: true

    patternProperties:
	'^ethernet-port@[12]$':
	  type:·object
          additionalProperties:·true
          properties:
	    reg:
              items:
                - enum: [1, 2]
              description: MTIP L2 switch port number

And after calling:
make dt_binding_check DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml

there are no errors.

However, when I make a mistake nad in the "examples" section I change
'lablel' -> 'labelx' I see

switch@800f0000: ethernet-ports:ethernet-port@2: Unevaluated properties
are not allowed ('labelx' was unexpected)

which I suppose is expected behaviour.

> 
> > 
> > We do have ethernet-ports:
> > and we do "include" ($ref)
> > https://elixir.bootlin.com/linux/v6.14.6/source/Documentation/devicetree/bindings/net/ethernet-switch.yaml#L77
> > 
> > which is what we exactly need.  
> 
> The $ref is effectively pasting in what you reference, so the result 
> would be:
> 
> ethernet-ports:
>   type: object
>   "^(ethernet-)?ports$":
>     patternProperties:
>       "^(ethernet-)?port@[0-9a-f]+$":
>         description: Ethernet switch ports
>         $ref: ethernet-switch-port.yaml#
>         unevaluatedProperties: false
> 
> A DT node/property name and json-schema keyword at the same level is 
> never correct. json-schema behavior is to ignore (silently) any
> unknown keyword. So the validator sees the keyword
> "^(ethernet-)?ports$" and ignores everything below it.
> 
> 
> >   
> > > > +    additionalProperties: true    
> > > 
> > > Drop.
> > >   
> > 
> > When removed we do have:
> > nxp,imx28-mtip-switch.example.dtb: switch@800f0000: Unevaluated
> > properties are not allowed ('ethernet-ports' was unexpected)
> > 
> > or 
> > 
> > nxp,imx28-mtip-switch.yaml: ethernet-ports: Missing
> > additionalProperties/unevaluatedProperties constraint  
> 
> 'additionalProperties: true' should suppress that.
> 
> > 
> > Depending if I do remove 'patternProperties' above.
> > 
> > To sum up - with the current code, the DT schema checks pass. It
> > also looks like the $ref for ethernet-switch is used in an optimal
> > way.
> > 
> > I would opt for keeping the code as is for v12.  




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

Hi Paolo,

> Hi Paolo,
> 
> > On 5/4/25 4:55 PM, Lukasz Majewski wrote:  
> > > +		/* This does 16 byte alignment, exactly what we
> > > need.
> > > +		 * The packet length includes FCS, but we don't
> > > want to
> > > +		 * include that when passing upstream as it
> > > messes up
> > > +		 * bridging applications.
> > > +		 */
> > > +		skb = netdev_alloc_skb(pndev, pkt_len +
> > > NET_IP_ALIGN);
> > > +		if (unlikely(!skb)) {
> > > +			dev_dbg(&fep->pdev->dev,
> > > +				"%s: Memory squeeze, dropping
> > > packet.\n",
> > > +				pndev->name);
> > > +			pndev->stats.rx_dropped++;
> > > +			goto err_mem;
> > > +		} else {
> > > +			skb_reserve(skb, NET_IP_ALIGN);
> > > +			skb_put(skb, pkt_len);      /* Make room
> > > */
> > > +			skb_copy_to_linear_data(skb, data,
> > > pkt_len);
> > > +			skb->protocol = eth_type_trans(skb,
> > > pndev);
> > > +			napi_gro_receive(&fep->napi, skb);
> > > +		}
> > > +
> > > +		bdp->cbd_bufaddr =
> > > dma_map_single(&fep->pdev->dev, data,
> > > +
> > > bdp->cbd_datlen,
> > > +
> > > DMA_FROM_DEVICE);
> > > +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> > > +
> > > bdp->cbd_bufaddr))) {
> > > +			dev_err(&fep->pdev->dev,
> > > +				"Failed to map descriptor rx
> > > buffer\n");
> > > +			pndev->stats.rx_errors++;
> > > +			pndev->stats.rx_dropped++;
> > > +			dev_kfree_skb_any(skb);
> > > +			goto err_mem;
> > > +		}    
> > 
> > This is doing the mapping and ev. dropping the skb _after_ pushing
> > the skb up the stack, you must attempt the mapping first.  
> 
> I've double check it - the code seems to be correct.
> 
> This code is a part of mtip_switch_rx() function, which handles
> receiving data.
> 
> First, on probe, the initial dma memory is mapped for MTIP received
> data.
> 
> When we receive data, it is processed and afterwards it is "pushed" up
> to the network stack.
> 
> As a last step we do map memory for next, incoming data and leave the
> function.
> 
> Hence, IMHO, the order is OK and this part shall be left as is.

Is the explanation sufficient?

> 
> >   
> > > +static void mtip_free_buffers(struct net_device *dev)
> > > +{
> > > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > > +	struct switch_enet_private *fep = priv->fep;
> > > +	struct sk_buff *skb;
> > > +	struct cbd_t *bdp;
> > > +	int i;
> > > +
> > > +	bdp = fep->rx_bd_base;
> > > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > > +		skb = fep->rx_skbuff[i];
> > > +
> > > +		if (bdp->cbd_bufaddr)
> > > +			dma_unmap_single(&fep->pdev->dev,
> > > bdp->cbd_bufaddr,
> > > +					 MTIP_SWITCH_RX_FRSIZE,
> > > +					 DMA_FROM_DEVICE);
> > > +		if (skb)
> > > +			dev_kfree_skb(skb);    
> > 
> > I suspect that on error paths mtip_free_buffers() can be invoked
> > multiple consecutive times with any successful allocation in
> > between: skb will be freed twice. Likely you need to clear
> > fep->rx_skbuff[i] here.  
> 
> +1 - I will add it with v12.
> 
> > 
> > Side note: this patch is way too big for a proper review: you need
> > to break it in multiple smaller ones, introducing the basic features
> > separately.
> > 
> > Cheers,
> > 
> > Paolo
> >   
> 
> 
> 
> 
> 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




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4575 bytes --]

Hi Paolo,

> Hi Paolo,
> 
> > Hi Paolo,
> >   
> > > On 5/4/25 4:55 PM, Lukasz Majewski wrote:    
> > > > +		/* This does 16 byte alignment, exactly what we
> > > > need.
> > > > +		 * The packet length includes FCS, but we don't
> > > > want to
> > > > +		 * include that when passing upstream as it
> > > > messes up
> > > > +		 * bridging applications.
> > > > +		 */
> > > > +		skb = netdev_alloc_skb(pndev, pkt_len +
> > > > NET_IP_ALIGN);
> > > > +		if (unlikely(!skb)) {
> > > > +			dev_dbg(&fep->pdev->dev,
> > > > +				"%s: Memory squeeze, dropping
> > > > packet.\n",
> > > > +				pndev->name);
> > > > +			pndev->stats.rx_dropped++;
> > > > +			goto err_mem;
> > > > +		} else {
> > > > +			skb_reserve(skb, NET_IP_ALIGN);
> > > > +			skb_put(skb, pkt_len);      /* Make
> > > > room */
> > > > +			skb_copy_to_linear_data(skb, data,
> > > > pkt_len);
> > > > +			skb->protocol = eth_type_trans(skb,
> > > > pndev);
> > > > +			napi_gro_receive(&fep->napi, skb);
> > > > +		}
> > > > +
> > > > +		bdp->cbd_bufaddr =
> > > > dma_map_single(&fep->pdev->dev, data,
> > > > +
> > > > bdp->cbd_datlen,
> > > > +
> > > > DMA_FROM_DEVICE);
> > > > +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> > > > +
> > > > bdp->cbd_bufaddr))) {
> > > > +			dev_err(&fep->pdev->dev,
> > > > +				"Failed to map descriptor rx
> > > > buffer\n");
> > > > +			pndev->stats.rx_errors++;
> > > > +			pndev->stats.rx_dropped++;
> > > > +			dev_kfree_skb_any(skb);
> > > > +			goto err_mem;
> > > > +		}      
> > > 
> > > This is doing the mapping and ev. dropping the skb _after_ pushing
> > > the skb up the stack, you must attempt the mapping first.    
> > 
> > I've double check it - the code seems to be correct.
> > 
> > This code is a part of mtip_switch_rx() function, which handles
> > receiving data.
> > 
> > First, on probe, the initial dma memory is mapped for MTIP received
> > data.
> > 
> > When we receive data, it is processed and afterwards it is "pushed"
> > up to the network stack.
> > 
> > As a last step we do map memory for next, incoming data and leave
> > the function.
> > 
> > Hence, IMHO, the order is OK and this part shall be left as is.  
> 
> Is the explanation sufficient?

I would do appreciate your feedback as your OK is required to prepare
v12, so I would had the opportunity to have this patch set accepted to
net-next before cut-off date.

Thanks in advance for your help.

> 
> >   
> > >     
> > > > +static void mtip_free_buffers(struct net_device *dev)
> > > > +{
> > > > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > > > +	struct switch_enet_private *fep = priv->fep;
> > > > +	struct sk_buff *skb;
> > > > +	struct cbd_t *bdp;
> > > > +	int i;
> > > > +
> > > > +	bdp = fep->rx_bd_base;
> > > > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > > > +		skb = fep->rx_skbuff[i];
> > > > +
> > > > +		if (bdp->cbd_bufaddr)
> > > > +			dma_unmap_single(&fep->pdev->dev,
> > > > bdp->cbd_bufaddr,
> > > > +					 MTIP_SWITCH_RX_FRSIZE,
> > > > +					 DMA_FROM_DEVICE);
> > > > +		if (skb)
> > > > +			dev_kfree_skb(skb);      
> > > 
> > > I suspect that on error paths mtip_free_buffers() can be invoked
> > > multiple consecutive times with any successful allocation in
> > > between: skb will be freed twice. Likely you need to clear
> > > fep->rx_skbuff[i] here.    
> > 
> > +1 - I will add it with v12.
> >   
> > > 
> > > Side note: this patch is way too big for a proper review: you need
> > > to break it in multiple smaller ones, introducing the basic
> > > features separately.
> > > 
> > > Cheers,
> > > 
> > > Paolo
> > >     
> > 
> > 
> > 
> > 
> > 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  
> 
> 
> 
> 
> 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




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Paolo Abeni]

On 5/13/25 7:31 AM, Lukasz Majewski wrote:
>> On 5/4/25 4:55 PM, Lukasz Majewski wrote:
>>> +		/* This does 16 byte alignment, exactly what we
>>> need.
>>> +		 * The packet length includes FCS, but we don't
>>> want to
>>> +		 * include that when passing upstream as it messes
>>> up
>>> +		 * bridging applications.
>>> +		 */
>>> +		skb = netdev_alloc_skb(pndev, pkt_len +
>>> NET_IP_ALIGN);
>>> +		if (unlikely(!skb)) {
>>> +			dev_dbg(&fep->pdev->dev,
>>> +				"%s: Memory squeeze, dropping
>>> packet.\n",
>>> +				pndev->name);
>>> +			pndev->stats.rx_dropped++;
>>> +			goto err_mem;
>>> +		} else {
>>> +			skb_reserve(skb, NET_IP_ALIGN);
>>> +			skb_put(skb, pkt_len);      /* Make room */
>>> +			skb_copy_to_linear_data(skb, data,
>>> pkt_len);
>>> +			skb->protocol = eth_type_trans(skb, pndev);
>>> +			napi_gro_receive(&fep->napi, skb);
>>> +		}
>>> +
>>> +		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev,
>>> data,
>>> +						  bdp->cbd_datlen,
>>> +						  DMA_FROM_DEVICE);
>>> +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
>>> +					       bdp->cbd_bufaddr)))
>>> {
>>> +			dev_err(&fep->pdev->dev,
>>> +				"Failed to map descriptor rx
>>> buffer\n");
>>> +			pndev->stats.rx_errors++;
>>> +			pndev->stats.rx_dropped++;
>>> +			dev_kfree_skb_any(skb);
>>> +			goto err_mem;
>>> +		}  
>>
>> This is doing the mapping and ev. dropping the skb _after_ pushing the
>> skb up the stack, you must attempt the mapping first.
> 
> I've double check it - the code seems to be correct.
> 
> This code is a part of mtip_switch_rx() function, which handles
> receiving data.
> 
> First, on probe, the initial dma memory is mapped for MTIP received
> data.
> 
> When we receive data, it is processed and afterwards it is "pushed" up
> to the network stack.
> 
> As a last step we do map memory for next, incoming data and leave the
> function.
> 
> Hence, IMHO, the order is OK and this part shall be left as is.

First thing first, I'm sorry for lagging behind. This fell outside my
radar. Let's keep the conversation on the new patch version, it should
help me to avoid repeating this mistake.

/P

Re: [net-next v11 4/7] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

Hi Paolo,

> On 5/13/25 7:31 AM, Lukasz Majewski wrote:
> >> On 5/4/25 4:55 PM, Lukasz Majewski wrote:  
> >>> +		/* This does 16 byte alignment, exactly what we
> >>> need.
> >>> +		 * The packet length includes FCS, but we don't
> >>> want to
> >>> +		 * include that when passing upstream as it
> >>> messes up
> >>> +		 * bridging applications.
> >>> +		 */
> >>> +		skb = netdev_alloc_skb(pndev, pkt_len +
> >>> NET_IP_ALIGN);
> >>> +		if (unlikely(!skb)) {
> >>> +			dev_dbg(&fep->pdev->dev,
> >>> +				"%s: Memory squeeze, dropping
> >>> packet.\n",
> >>> +				pndev->name);
> >>> +			pndev->stats.rx_dropped++;
> >>> +			goto err_mem;
> >>> +		} else {
> >>> +			skb_reserve(skb, NET_IP_ALIGN);
> >>> +			skb_put(skb, pkt_len);      /* Make room
> >>> */
> >>> +			skb_copy_to_linear_data(skb, data,
> >>> pkt_len);
> >>> +			skb->protocol = eth_type_trans(skb,
> >>> pndev);
> >>> +			napi_gro_receive(&fep->napi, skb);
> >>> +		}
> >>> +
> >>> +		bdp->cbd_bufaddr =
> >>> dma_map_single(&fep->pdev->dev, data,
> >>> +
> >>> bdp->cbd_datlen,
> >>> +
> >>> DMA_FROM_DEVICE);
> >>> +		if (unlikely(dma_mapping_error(&fep->pdev->dev,
> >>> +
> >>> bdp->cbd_bufaddr))) {
> >>> +			dev_err(&fep->pdev->dev,
> >>> +				"Failed to map descriptor rx
> >>> buffer\n");
> >>> +			pndev->stats.rx_errors++;
> >>> +			pndev->stats.rx_dropped++;
> >>> +			dev_kfree_skb_any(skb);
> >>> +			goto err_mem;
> >>> +		}    
> >>
> >> This is doing the mapping and ev. dropping the skb _after_ pushing
> >> the skb up the stack, you must attempt the mapping first.  
> > 
> > I've double check it - the code seems to be correct.
> > 
> > This code is a part of mtip_switch_rx() function, which handles
> > receiving data.
> > 
> > First, on probe, the initial dma memory is mapped for MTIP received
> > data.
> > 
> > When we receive data, it is processed and afterwards it is "pushed"
> > up to the network stack.
> > 
> > As a last step we do map memory for next, incoming data and leave
> > the function.
> > 
> > Hence, IMHO, the order is OK and this part shall be left as is.  
> 
> First thing first, I'm sorry for lagging behind. This fell outside my
> radar. Let's keep the conversation on the new patch version, it should
> help me to avoid repeating this mistake.

+1

> 
> /P
> 




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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]