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

[net-next v4 0/5] 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.

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 (5):
  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_FEC_MTIP_L2SW to support MTIP L2
    switch

 .../bindings/net/nxp,imx28-mtip-switch.yaml   |  126 ++
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts       |   54 +
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi          |    8 +-
 arch/arm/configs/mxs_defconfig                |   14 +-
 drivers/net/ethernet/freescale/Kconfig        |    1 +
 drivers/net/ethernet/freescale/Makefile       |    1 +
 drivers/net/ethernet/freescale/mtipsw/Kconfig |   13 +
 .../net/ethernet/freescale/mtipsw/Makefile    |    3 +
 .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 1970 +++++++++++++++++
 .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  782 +++++++
 .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  122 +
 .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  449 ++++
 13 files changed, 3537 insertions(+), 13 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 v4 1/5] 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>
---
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
---
 .../bindings/net/nxp,imx28-mtip-switch.yaml   | 126 ++++++++++++++++++
 1 file changed, 126 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..1afaf8029725
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
@@ -0,0 +1,126 @@
+# 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.
+
+$ref: ethernet-switch.yaml#/$defs/ethernet-ports
+
+patternProperties:
+  "^(ethernet-)?ports$":
+    type: object
+    additionalProperties: true
+
+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
+
+  pinctrl-names: true
+
+  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
+  - mdio
+  - ethernet-ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include<dt-bindings/interrupt-controller/irq.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>;
+        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 0>;
+            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 v4 2/5] 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>

---
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)
---
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 8 ++++++--
 1 file changed, 6 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..a0b565ffc83d 100644
--- a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
+++ b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
@@ -1321,8 +1321,12 @@ 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>;
+			clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
+			clock-names = "ipg", "ahb", "enet_out", "ptp";
 			status = "disabled";
 		};
 	};
-- 
2.39.5

[net-next v4 3/5] 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>
---
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]
---
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 54 +++++++++++++++++++++++++
 1 file changed, 54 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..280e5a79b787 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,59 @@ &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 v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Lukasz Majewski]

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v4:
- New patch
---
 arch/arm/configs/mxs_defconfig | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index d8a6e43c401e..4dc4306c035f 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -32,11 +32,10 @@ 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_BRIDGE=y
+CONFIG_NET_SWITCHDEV=y
 CONFIG_CAN=m
 # CONFIG_WIRELESS is not set
 CONFIG_DEVTMPFS=y
@@ -45,7 +44,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
@@ -56,11 +54,11 @@ 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
 CONFIG_REALTEK_PHY=y
-CONFIG_SMSC_PHY=y
 CONFIG_CAN_FLEXCAN=m
 CONFIG_USB_USBNET=y
 CONFIG_USB_NET_SMSC95XX=y
@@ -77,13 +75,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
@@ -138,10 +134,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
 CONFIG_TMPFS=y
 CONFIG_TMPFS_POSIX_ACL=y
-- 
2.39.5

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

[Simon Horman]

On Mon, Apr 07, 2025 at 04:51:56PM +0200, Lukasz Majewski wrote:
> This patch series provides support for More Than IP L2 switch embedded
> in the imx287 SoC.
> 
> This is a two port switch (placed between uDMA[01] and MAC-NET[01]),
> which can be used for offloading the network traffic.
> 
> It can be used interchangeably with current FEC driver - to be more
> specific: one can use either of it, depending on the requirements.
> 
> The biggest difference is the usage of DMA - when FEC is used, separate
> DMAs are available for each ENET-MAC block.
> However, with switch enabled - only the DMA0 is used to send/receive data
> to/form switch (and then switch sends them to respecitive ports).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Hi Lukasz,

This is not a complete review, but I did spend a bit of time
looking over this and have provided some feedback on
things I noticed below.

...

> diff --git a/drivers/net/ethernet/freescale/mtipsw/Kconfig b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> new file mode 100644
> index 000000000000..450ff734a321
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config FEC_MTIP_L2SW
> +	tristate "MoreThanIP L2 switch support to FEC driver"
> +	depends on OF
> +	depends on NET_SWITCHDEV
> +	depends on BRIDGE
> +	depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST
> +	help
> +	  This enables support for the MoreThan IP L2 switch on i.MX
> +	  SoCs (e.g. iMX28, vf610). It offloads bridging to this IP block's
> +	  hardware and allows switch management with standard Linux tools.
> +	  This switch driver can be used interchangeable with the already
> +	  available FEC driver, depending on the use case's requirments.

nit: requirements

Flagged by checkpatch.pl --codespell

...

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

...

> +static void mtip_enet_init(struct switch_enet_private *fep, int port)
> +{
> +	void __iomem *enet_addr = fep->enet_addr;
> +	u32 mii_speed, holdtime, tmp;

I think it would be best to avoid variable names like tmp which have little
meaning. Although still rather generic, perhaps reg would be more
appropriate. Or better still something relating to the name the register,
say rcr.

> +
> +	if (port == 2)
> +		enet_addr += MCF_ESW_ENET_PORT_OFFSET;
> +
> +	tmp = MCF_FEC_RCR_PROM | MCF_FEC_RCR_MII_MODE |
> +		MCF_FEC_RCR_MAX_FL(1522);
> +
> +	if (fep->phy_interface[port - 1]  == PHY_INTERFACE_MODE_RMII)
> +		tmp |= MCF_FEC_RCR_RMII_MODE;
> +
> +	writel(tmp, enet_addr + MCF_FEC_RCR);
> +
> +	/* TCR */
> +	writel(MCF_FEC_TCR_FDEN, enet_addr + MCF_FEC_TCR);
> +
> +	/* ECR */
> +	writel(MCF_FEC_ECR_ETHER_EN, enet_addr + MCF_FEC_ECR);
> +
> +	/* Set MII speed to 2.5 MHz
> +	 */
> +	mii_speed = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 5000000);
> +	mii_speed--;
> +
> +	/* The i.MX28 and i.MX6 types have another filed in the MSCR (aka
> +	 * MII_SPEED) register that defines the MDIO output hold time. Earlier
> +	 * versions are RAZ there, so just ignore the difference and write the
> +	 * register always.
> +	 * The minimal hold time according to IEE802.3 (clause 22) is 10 ns.
> +	 * HOLDTIME + 1 is the number of clk cycles the fec is holding the
> +	 * output.
> +	 * The HOLDTIME bitfield takes values between 0 and 7 (inclusive).
> +	 * Given that ceil(clkrate / 5000000) <= 64, the calculation for
> +	 * holdtime cannot result in a value greater than 3.
> +	 */
> +	holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg), 100000000) - 1;
> +
> +	fep->phy_speed = mii_speed << 1 | holdtime << 8;
> +
> +	writel(fep->phy_speed, enet_addr + MCF_FEC_MSCR);
> +}
> +
> +static int mtip_setup_mac(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	unsigned char *iap, tmpaddr[ETH_ALEN];

Maybe mac_addr instead of tmpaddr.

> +
> +	/* Use MAC address from DTS */
> +	iap = &fep->mac[priv->portnum - 1][0];
> +
> +	/* Use MAC address set by bootloader */
> +	if (!is_valid_ether_addr(iap)) {
> +		*((unsigned long *)&tmpaddr[0]) =
> +			be32_to_cpu(readl(fep->enet_addr + MCF_FEC_PALR));
> +		*((unsigned short *)&tmpaddr[4]) =
> +			be16_to_cpu(readl(fep->enet_addr +
> +					  MCF_FEC_PAUR) >> 16);

* Above, and elsewhere in this patch unsigned long seems to be
  used for 32 bit values. But unsigned long can be 64 bits wide.

  I would suggest using u32, u16, and friends throughout this
  patch where an integer has a specific number of bits.

* readl returns a 32-bit value in host byte order.
  But the above assumes it returns a big endian value.

  This does not seem correct.

* The point immediately above aside, the assignment of
  host byte order values to the byte-array tmpaddr
  seems to assume an endianness (little endian?).

  It should work on either endian.

> +		iap = &tmpaddr[0];
> +	}
> +
> +	/* Use random MAC address */
> +	if (!is_valid_ether_addr(iap)) {
> +		eth_hw_addr_random(dev);
> +		dev_info(&fep->pdev->dev, "Using random MAC address: %pM\n",
> +			 dev->dev_addr);
> +		iap = (unsigned char *)dev->dev_addr;
> +	}
> +
> +	/* Adjust MAC if using macaddr (and increment if needed) */
> +	eth_hw_addr_gen(dev, iap, priv->portnum - 1);
> +
> +	return 0;
> +}
> +
> +/**
> + * crc8_calc - calculate CRC for MAC storage
> + *
> + * @pmacaddress: A 6-byte array with the MAC address. The first byte is
> + *               the first byte transmitted.
> + *
> + * Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> + * (getting the remainder).
> + *
> + *  Examples (hexadecimal values):<br>
> + *   10-11-12-13-14-15  => CRC=0xc2
> + *   10-11-cc-dd-ee-00  => CRC=0xe6
> + *
> + * Return: The 8-bit CRC in bits 7:0
> + */
> +static int crc8_calc(unsigned char *pmacaddress)

Can lib/crc8.c:crc8() be used here?

> +{
> +	int byt; /* byte index */
> +	int bit; /* bit index */
> +	int crc = 0x12;
> +	int inval;
> +
> +	for (byt = 0; byt < 6; byt++) {
> +		inval = (((int)pmacaddress[byt]) & 0xff);
> +		/* shift bit 0 to bit 8 so all our bits
> +		 * travel through bit 8
> +		 * (simplifies below calc)
> +		 */
> +		inval <<= 8;
> +
> +		for (bit = 0; bit < 8; bit++) {
> +			/* next input bit comes into d7 after shift */
> +			crc |= inval & 0x100;
> +			if (crc & 0x01)
> +				/* before shift  */
> +				crc ^= 0x1c0;
> +
> +			crc >>= 1;
> +			inval >>= 1;
> +		}
> +	}
> +	/* upper bits are clean as we shifted in zeroes! */
> +	return crc;
> +}
> +
> +static void mtip_read_atable(struct switch_enet_private *fep, int index,
> +			     unsigned long *read_lo, unsigned long *read_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	*read_lo = readl((const void *)atable_base + (index << 3));
> +	*read_hi = readl((const void *)atable_base + (index << 3) + 4);
> +}

It is unclear why hwentry, which is a pointer, is being cast to an
integer and then back to a pointer. I see pointer arithmetic, but
that can operate on pointers just as well as integers, without making
assumptions about how wide pointers are with respect to longs.

And in any case, can't the types be used to directly access the
offsets needed like this?

	atable = fep->hwentry.mtip_table64b_entry;

	*read_lo = readl(&atable[index].lo);
	*read_hi = readl(&atable[index].hi);

Also, and perhaps more importantly, readl expects to be passed
a pointer to __iomem. But the appropriate annotations seem
to be missing (forcing them with a cast is not advisable here IMHO).

Please do run sparse over your patches to iron out __iomem
(and endian) issues.

> +
> +static void mtip_write_atable(struct switch_enet_private *fep, int index,
> +			      unsigned long write_lo, unsigned long write_hi)
> +{
> +	unsigned long atable_base = (unsigned long)fep->hwentry;
> +
> +	writel(write_lo, (void *)atable_base + (index << 3));
> +	writel(write_hi, (void *)atable_base + (index << 3) + 4);

Likewise here.

> +}

...

> +/* Clear complete MAC Look Up Table */
> +void mtip_clear_atable(struct switch_enet_private *fep)
> +{
> +	int index;
> +
> +	for (index = 0; index < 2048; index++)
> +		mtip_write_atable(fep, index, 0, 0);
> +}
> +
> +/**
> + * mtip_update_atable_static - Update switch static address table
> + *
> + * @mac_addr: Pointer to the array containing MAC address to
> + *            be put as static entry
> + * @port:     Port bitmask numbers to be added in static entry,
> + *            valid values are 1-7
> + * @priority: The priority for the static entry in table

@fep should also be documented here.

Flagged by ./scripts/kernel-doc -none
and W=1 builds.

> + *
> + * Updates MAC address lookup table with a static entry.
> + *
> + * Searches if the MAC address is already there in the block and replaces
> + * the older entry with the new one. If MAC address is not there then puts
> + * a new entry in the first empty slot available in the block.
> + *
> + * Return: 0 for a successful update else -ENOSPC when no slot available
> + */
> +static int mtip_update_atable_static(unsigned char *mac_addr, unsigned int port,
> +				     unsigned int priority,
> +				     struct switch_enet_private *fep)

...

> +/* During a receive, the cur_rx points to the current incoming buffer.
> + * When we update through the ring, if the next incoming buffer has
> + * not been given to the system, we just set the empty indicator,
> + * effectively tossing the packet.
> + */
> +static int mtip_switch_rx(struct net_device *dev, int budget, int *port)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct switch_t *fecp = fep->hwp;
> +	unsigned short status, pkt_len;
> +	struct net_device *pndev;
> +	u8 *data, rx_port = 0xFF;
> +	struct ethhdr *eth_hdr;
> +	int pkt_received = 0;
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +	struct cbd_t *bdp;
> +
> +	spin_lock_irqsave(&fep->hw_lock, flags);
> +
> +	/* First, grab all of the stats for the incoming packet.
> +	 * These get messed up if we get called due to a busy condition.
> +	 */
> +	bdp = fep->cur_rx;
> +
> +	while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {

...

> +	} /* while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) */
> +
> +	writel(bdp, &fep->cur_rx);

I'm confused buy this.

At the top of this function, bdp is assigned using:

	bdp = fep->cur_rx;

But here writel() is used to assign bdp to &fep->cur_rx.
Which assumes that bdp is a 32-bit little endian value.
But it is a pointer in host byte order which may be wide than 32-bits.

On x86_64 int is 32-bits while pointers are 64 bits.
W=1 builds with gcc 14.2.0 flag this problem like this:


.../mtipl2sw.c:1108:9: error: incompatible pointer to integer conversion passing 'struct cbd_t *' to parameter of type 'unsigned int' [-Wint-conversion]
 1108 |         writel(bdp, &fep->cur_rx);
      |                ^~~


This also assumes that &fep->cur_rx is a pointer to __iomem,
but that does not seem to be the case.

> +	spin_unlock_irqrestore(&fep->hw_lock, flags);
> +
> +	return pkt_received;
> +}

...

> +static int __init mtip_switch_dma_init(struct switch_enet_private *fep)
> +{
> +	struct cbd_t *bdp, *cbd_base;
> +	int ret, i;
> +
> +	/* Check mask of the streaming and coherent API */
> +	ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_err(&fep->pdev->dev, "No suitable DMA available\n");
> +		return ret;
> +	}
> +
> +	/* Allocate memory for buffer descriptors */
> +	cbd_base = dma_alloc_coherent(&fep->pdev->dev, PAGE_SIZE, &fep->bd_dma,
> +				      GFP_KERNEL);
> +	if (!cbd_base)
> +		return -ENOMEM;
> +
> +	/* Set receive and transmit descriptor base */
> +	fep->rx_bd_base = cbd_base;
> +	fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> +
> +	/* Initialize the receive buffer descriptors */
> +	bdp = fep->rx_bd_base;
> +	for (i = 0; i < RX_RING_SIZE; i++) {
> +		bdp->cbd_sc = 0;
> +		bdp++;
> +	}
> +
> +	/* Set the last buffer to wrap */
> +	bdp--;
> +	bdp->cbd_sc |= BD_SC_WRAP;
> +
> +	/* ...and the same for transmmit */

nit: transmit

> +	bdp = fep->tx_bd_base;
> +	for (i = 0; i < TX_RING_SIZE; i++) {
> +		/* Initialize the BD for every fragment in the page */
> +		bdp->cbd_sc = 0;
> +		bdp->cbd_bufaddr = 0;
> +		bdp++;
> +	}
> +
> +	/* Set the last buffer to wrap */
> +	bdp--;
> +	bdp->cbd_sc |= BD_SC_WRAP;
> +
> +	return 0;
> +}
> +
> +static void mtip_ndev_cleanup(struct switch_enet_private *fep)
> +{
> +	int i;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		if (fep->ndev[i]) {
> +			unregister_netdev(fep->ndev[i]);
> +			free_netdev(fep->ndev[i]);
> +		}
> +	}
> +}

...

> +static const struct of_device_id mtipl2_of_match[] = {
> +	{ .compatible = "nxp,imx28-mtip-switch",
> +	  .data = &mtip_imx28_l2switch_info},
> +	{ /* sentinel */ }
> +}

There should be a trailing ';' on the line above.

...

> +struct  addr_table64b_entry {

One space after struct is enough.

> +	unsigned int lo;  /* lower 32 bits */
> +	unsigned int hi;  /* upper 32 bits */
> +};
> +
> +struct  mtip_addr_table_t {

I think you can drop the '_t' from the name of this struct.
We know it is a type :)

> +	struct addr_table64b_entry  mtip_table64b_entry[2048];

One space is enough after addr_table64b_entry.
And in general, unless the aim is to align field names
(not here because there is only one field :)

> +};
> +
> +#define MCF_ESW_LOOKUP_MEM_OFFSET      0x4000
> +#define MCF_ESW_ENET_PORT_OFFSET      0x4000
> +#define ENET_SWI_PHYS_ADDR_OFFSET	0x8000

Ditto.

...

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c

> new file mode 100644
> index 000000000000..0b76a60858a5
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  L2 switch Controller driver for MTIP block - bridge network interface
> + *
> + *  Copyright (C) 2025 DENX Software Engineering GmbH
> + *  Lukasz Majewski <lukma@denx.de>
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/platform_device.h>
> +
> +#include "mtipl2sw.h"

Blank line here please.

> +static int mtip_ndev_port_link(struct net_device *ndev,
> +			       struct net_device *br_ndev,
> +			       struct netlink_ext_ack *extack)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev), *other_priv;
> +	struct switch_enet_private *fep = priv->fep;
> +	struct net_device *other_ndev;
> +
> +	/* Check if one port of MTIP switch is already bridged */
> +	if (fep->br_members && !fep->br_offload) {
> +		/* Get the second bridge ndev */
> +		other_ndev = fep->ndev[fep->br_members - 1];
> +		other_priv = netdev_priv(other_ndev);
> +		if (other_priv->master_dev != br_ndev) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "L2 offloading only possible for the same bridge!");
> +			return notifier_from_errno(-EOPNOTSUPP);
> +		}
> +
> +		fep->br_offload = 1;
> +		mtip_switch_dis_port_separation(fep);
> +		mtip_clear_atable(fep);
> +	}
> +
> +	if (!priv->master_dev)
> +		priv->master_dev = br_ndev;
> +
> +	fep->br_members |= BIT(priv->portnum - 1);
> +
> +	dev_dbg(&ndev->dev,
> +		"%s: ndev: %s br: %s fep: 0x%x members: 0x%x offload: %d\n",
> +		__func__, ndev->name,  br_ndev->name, (unsigned int)fep,

Perhaps it would be best to use %p as the format specifier for fep
and not cast it.

On x86_64 int is 32-bits while pointers are 64 bits.
W=1 builds with gcc 14.2.0 flag this problem like this:

.../mtipl2sw_br.c:45:55: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   45 |                 __func__, ndev->name,  br_ndev->name, (unsigned int)fep,
      |                                                       ^

> +		fep->br_members, fep->br_offload);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n", __func__,
> +		ndev->name, fep->br_members);
> +
> +	fep->br_members &= ~BIT(priv->portnum - 1);
> +	priv->master_dev = NULL;
> +
> +	if (!fep->br_members) {
> +		fep->br_offload = 0;
> +		mtip_switch_en_port_separation(fep);
> +		mtip_clear_atable(fep);
> +	}
> +}

...

-- 
pw-bot: changes-requested

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

[Lukasz Majewski]

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

Hi Simon,

> On Mon, Apr 07, 2025 at 04:51:56PM +0200, Lukasz Majewski wrote:
> > This patch series provides support for More Than IP L2 switch
> > embedded in the imx287 SoC.
> > 
> > This is a two port switch (placed between uDMA[01] and MAC-NET[01]),
> > which can be used for offloading the network traffic.
> > 
> > It can be used interchangeably with current FEC driver - to be more
> > specific: one can use either of it, depending on the requirements.
> > 
> > The biggest difference is the usage of DMA - when FEC is used,
> > separate DMAs are available for each ENET-MAC block.
> > However, with switch enabled - only the DMA0 is used to
> > send/receive data to/form switch (and then switch sends them to
> > respecitive ports).
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> Hi Lukasz,
> 
> This is not a complete review, but I did spend a bit of time
> looking over this and have provided some feedback on
> things I noticed below.
> 

Thanks for your feedback.

> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > b/drivers/net/ethernet/freescale/mtipsw/Kconfig new file mode 100644
> > index 000000000000..450ff734a321
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +config FEC_MTIP_L2SW
> > +	tristate "MoreThanIP L2 switch support to FEC driver"
> > +	depends on OF
> > +	depends on NET_SWITCHDEV
> > +	depends on BRIDGE
> > +	depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST
> > +	help
> > +	  This enables support for the MoreThan IP L2 switch on
> > i.MX
> > +	  SoCs (e.g. iMX28, vf610). It offloads bridging to this
> > IP block's
> > +	  hardware and allows switch management with standard
> > Linux tools.
> > +	  This switch driver can be used interchangeable with the
> > already
> > +	  available FEC driver, depending on the use case's
> > requirments.  
> 
> nit: requirements
> 
> Flagged by checkpatch.pl --codespell
> 

Ok.

> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c  
> 
> ...
> 
> > +static void mtip_enet_init(struct switch_enet_private *fep, int
> > port) +{
> > +	void __iomem *enet_addr = fep->enet_addr;
> > +	u32 mii_speed, holdtime, tmp;  
> 
> I think it would be best to avoid variable names like tmp which have
> little meaning. Although still rather generic, perhaps reg would be
> more appropriate. Or better still something relating to the name the
> register, say rcr.

Ok, I will use reg/rcr instead of tmp.

> 
> > +
> > +	if (port == 2)
> > +		enet_addr += MCF_ESW_ENET_PORT_OFFSET;
> > +
> > +	tmp = MCF_FEC_RCR_PROM | MCF_FEC_RCR_MII_MODE |
> > +		MCF_FEC_RCR_MAX_FL(1522);
> > +
> > +	if (fep->phy_interface[port - 1]  ==
> > PHY_INTERFACE_MODE_RMII)
> > +		tmp |= MCF_FEC_RCR_RMII_MODE;
> > +
> > +	writel(tmp, enet_addr + MCF_FEC_RCR);
> > +
> > +	/* TCR */
> > +	writel(MCF_FEC_TCR_FDEN, enet_addr + MCF_FEC_TCR);
> > +
> > +	/* ECR */
> > +	writel(MCF_FEC_ECR_ETHER_EN, enet_addr + MCF_FEC_ECR);
> > +
> > +	/* Set MII speed to 2.5 MHz
> > +	 */
> > +	mii_speed = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg),
> > 5000000);
> > +	mii_speed--;
> > +
> > +	/* The i.MX28 and i.MX6 types have another filed in the
> > MSCR (aka
> > +	 * MII_SPEED) register that defines the MDIO output hold
> > time. Earlier
> > +	 * versions are RAZ there, so just ignore the difference
> > and write the
> > +	 * register always.
> > +	 * The minimal hold time according to IEE802.3 (clause 22)
> > is 10 ns.
> > +	 * HOLDTIME + 1 is the number of clk cycles the fec is
> > holding the
> > +	 * output.
> > +	 * The HOLDTIME bitfield takes values between 0 and 7
> > (inclusive).
> > +	 * Given that ceil(clkrate / 5000000) <= 64, the
> > calculation for
> > +	 * holdtime cannot result in a value greater than 3.
> > +	 */
> > +	holdtime = DIV_ROUND_UP(clk_get_rate(fep->clk_ipg),
> > 100000000) - 1; +
> > +	fep->phy_speed = mii_speed << 1 | holdtime << 8;
> > +
> > +	writel(fep->phy_speed, enet_addr + MCF_FEC_MSCR);
> > +}
> > +
> > +static int mtip_setup_mac(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	unsigned char *iap, tmpaddr[ETH_ALEN];  
> 
> Maybe mac_addr instead of tmpaddr.

Ok.

> 
> > +
> > +	/* Use MAC address from DTS */
> > +	iap = &fep->mac[priv->portnum - 1][0];
> > +
> > +	/* Use MAC address set by bootloader */
> > +	if (!is_valid_ether_addr(iap)) {
> > +		*((unsigned long *)&tmpaddr[0]) =
> > +			be32_to_cpu(readl(fep->enet_addr +
> > MCF_FEC_PALR));
> > +		*((unsigned short *)&tmpaddr[4]) =
> > +			be16_to_cpu(readl(fep->enet_addr +
> > +					  MCF_FEC_PAUR) >> 16);  
> 
> * Above, and elsewhere in this patch unsigned long seems to be
>   used for 32 bit values. But unsigned long can be 64 bits wide.
> 

As fair as I know - from the outset - this driver had some implicit
assumption to be used on 32 bit SoCs (imx28, vf610).

As a result the unsigned long would be 32 bits.

On the other hand - the documentation clearly says that registers in
this IP block implementation are 32 bit wide.

>   I would suggest using u32, u16, and friends throughout this
>   patch where an integer has a specific number of bits.
> 

I think that values, which directly readl()/writel() values from the
registers shall be u32. However, more generic code could use int/
unsigned int though.

> * readl returns a 32-bit value in host byte order.
>   But the above assumes it returns a big endian value.
> 
>   This does not seem correct.
> 

Please consult similar implementation from fec_main.c:
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/freescale/fec_main.c#L2044

I would use the same approach as in the above link.

> * The point immediately above aside, the assignment of
>   host byte order values to the byte-array tmpaddr
>   seems to assume an endianness (little endian?).
> 
>   It should work on either endian.
> 

I guess that implementation from above link is the correct one.

> > +		iap = &tmpaddr[0];
> > +	}
> > +
> > +	/* Use random MAC address */
> > +	if (!is_valid_ether_addr(iap)) {
> > +		eth_hw_addr_random(dev);
> > +		dev_info(&fep->pdev->dev, "Using random MAC
> > address: %pM\n",
> > +			 dev->dev_addr);
> > +		iap = (unsigned char *)dev->dev_addr;
> > +	}
> > +
> > +	/* Adjust MAC if using macaddr (and increment if needed) */
> > +	eth_hw_addr_gen(dev, iap, priv->portnum - 1);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * crc8_calc - calculate CRC for MAC storage
> > + *
> > + * @pmacaddress: A 6-byte array with the MAC address. The first
> > byte is
> > + *               the first byte transmitted.
> > + *
> > + * Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> > + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> > + * (getting the remainder).
> > + *
> > + *  Examples (hexadecimal values):<br>
> > + *   10-11-12-13-14-15  => CRC=0xc2
> > + *   10-11-cc-dd-ee-00  => CRC=0xe6
> > + *
> > + * Return: The 8-bit CRC in bits 7:0
> > + */
> > +static int crc8_calc(unsigned char *pmacaddress)  
> 
> Can lib/crc8.c:crc8() be used here?

This seems a bit problematic, as the above code is taken from the
vendor driver.

The documentation states - that the hash uses CRC-8 with following
polynomial:

x^8 + x^2 + x + 1 (0x07), which shall correspond to available in Linux:

static unsigned char mac1[] = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15};
crc8_populate_msb(mtipl2sw_crc8_table, 0x07);
crc8(mtipl2sw_crc8_table, mac1, sizeof(mac1), CRC8_INIT_VALUE);

However, those results don't match (either with 0xFF and 0x00 as the
initial value).

I've also searched the Internet to compare different CRC
implementations used in field:
https://crccalc.com/?crc=0x10 0x11 0x12 0x13 0x14
0x15&method=CRC-8&datatype=hex&outtype=hex

they also doesn't match results from this code.

Hence, the question - which implementation is right?

Vendor developer could have known a bit more than we do, so I would
prefer to keep this code as is - especially that switch internally may
use this crc8 calculation to find proper "slot/index" in the atable.

> 
> > +{
> > +	int byt; /* byte index */
> > +	int bit; /* bit index */
> > +	int crc = 0x12;
> > +	int inval;
> > +
> > +	for (byt = 0; byt < 6; byt++) {
> > +		inval = (((int)pmacaddress[byt]) & 0xff);
> > +		/* shift bit 0 to bit 8 so all our bits
> > +		 * travel through bit 8
> > +		 * (simplifies below calc)
> > +		 */
> > +		inval <<= 8;
> > +
> > +		for (bit = 0; bit < 8; bit++) {
> > +			/* next input bit comes into d7 after
> > shift */
> > +			crc |= inval & 0x100;
> > +			if (crc & 0x01)
> > +				/* before shift  */
> > +				crc ^= 0x1c0;
> > +
> > +			crc >>= 1;
> > +			inval >>= 1;
> > +		}
> > +	}
> > +	/* upper bits are clean as we shifted in zeroes! */
> > +	return crc;
> > +}
> > +
> > +static void mtip_read_atable(struct switch_enet_private *fep, int
> > index,
> > +			     unsigned long *read_lo, unsigned long
> > *read_hi) +{
> > +	unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > +	*read_lo = readl((const void *)atable_base + (index << 3));
> > +	*read_hi = readl((const void *)atable_base + (index << 3)
> > + 4); +}  
> 
> It is unclear why hwentry, which is a pointer, is being cast to an
> integer and then back to a pointer. I see pointer arithmetic, but
> that can operate on pointers just as well as integers, without making
> assumptions about how wide pointers are with respect to longs.
> 
> And in any case, can't the types be used to directly access the
> offsets needed like this?
> 
> 	atable = fep->hwentry.mtip_table64b_entry;
> 
> 	*read_lo = readl(&atable[index].lo);
> 	*read_hi = readl(&atable[index].hi);
> 

The code as is seems to be OK.

The (atable) memory structure is as follows:

1. You can store 2048 MAC addresses (2x32 bit each).

2. Memory from point 1 is addressed as follows:
	2.1 -> from MAC address the CRC8 is calculated (0x00 - 0xFF).
	This is the 'index' in the original code.
	2.2 -> as it may happen that for two different MAC address the
	same CRC8 is calculated (i.e. 'index' is the same), each
	'index' can store 8 entries for MAC addresses (and it is
	searched in a linear way if needed).

IMHO, the index above shall be multiplied by 8.

> Also, and perhaps more importantly, readl expects to be passed
> a pointer to __iomem. But the appropriate annotations seem
> to be missing (forcing them with a cast is not advisable here IMHO).
> 

I think that the code below:
unsigned long atable_base = (unsigned long)fep->hwentry;

could be replaced with
void __iomem *atable_base = fep->hwentry;

and the (index << 3) with (index * ATABLE_ENTRY_PER_SLOT)

> Please do run sparse over your patches to iron out __iomem
> (and endian) issues.
> 

Ok, I will run the make C=1 when compiling.

> > +
> > +static void mtip_write_atable(struct switch_enet_private *fep, int
> > index,
> > +			      unsigned long write_lo, unsigned
> > long write_hi) +{
> > +	unsigned long atable_base = (unsigned long)fep->hwentry;
> > +
> > +	writel(write_lo, (void *)atable_base + (index << 3));
> > +	writel(write_hi, (void *)atable_base + (index << 3) + 4);  
> 
> Likewise here.

Please see the above comment.

> 
> > +}  
> 
> ...
> 
> > +/* Clear complete MAC Look Up Table */
> > +void mtip_clear_atable(struct switch_enet_private *fep)
> > +{
> > +	int index;
> > +
> > +	for (index = 0; index < 2048; index++)
> > +		mtip_write_atable(fep, index, 0, 0);
> > +}
> > +
> > +/**
> > + * mtip_update_atable_static - Update switch static address table
> > + *
> > + * @mac_addr: Pointer to the array containing MAC address to
> > + *            be put as static entry
> > + * @port:     Port bitmask numbers to be added in static entry,
> > + *            valid values are 1-7
> > + * @priority: The priority for the static entry in table  
> 
> @fep should also be documented here.
> 

OK.

> Flagged by ./scripts/kernel-doc -none
> and W=1 builds.
> 
> > + *
> > + * Updates MAC address lookup table with a static entry.
> > + *
> > + * Searches if the MAC address is already there in the block and
> > replaces
> > + * the older entry with the new one. If MAC address is not there
> > then puts
> > + * a new entry in the first empty slot available in the block.
> > + *
> > + * Return: 0 for a successful update else -ENOSPC when no slot
> > available
> > + */
> > +static int mtip_update_atable_static(unsigned char *mac_addr,
> > unsigned int port,
> > +				     unsigned int priority,
> > +				     struct switch_enet_private
> > *fep)  
> 
> ...
> 
> > +/* During a receive, the cur_rx points to the current incoming
> > buffer.
> > + * When we update through the ring, if the next incoming buffer has
> > + * not been given to the system, we just set the empty indicator,
> > + * effectively tossing the packet.
> > + */
> > +static int mtip_switch_rx(struct net_device *dev, int budget, int
> > *port) +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct switch_t *fecp = fep->hwp;
> > +	unsigned short status, pkt_len;
> > +	struct net_device *pndev;
> > +	u8 *data, rx_port = 0xFF;
> > +	struct ethhdr *eth_hdr;
> > +	int pkt_received = 0;
> > +	struct sk_buff *skb;
> > +	unsigned long flags;
> > +	struct cbd_t *bdp;
> > +
> > +	spin_lock_irqsave(&fep->hw_lock, flags);
> > +
> > +	/* First, grab all of the stats for the incoming packet.
> > +	 * These get messed up if we get called due to a busy
> > condition.
> > +	 */
> > +	bdp = fep->cur_rx;
> > +
> > +	while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {  
> 
> ...
> 
> > +	} /* while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY))
> > */ +
> > +	writel(bdp, &fep->cur_rx);  
> 
> I'm confused buy this.
> 
> At the top of this function, bdp is assigned using:
> 
> 	bdp = fep->cur_rx;
> 
> But here writel() is used to assign bdp to &fep->cur_rx.
> Which assumes that bdp is a 32-bit little endian value.
> But it is a pointer in host byte order which may be wide than 32-bits.
> 
> On x86_64 int is 32-bits while pointers are 64 bits.

I will cross compile with make W=1 and C=1 to find all problematic
places.

> W=1 builds with gcc 14.2.0 flag this problem like this:
> 
> 
> .../mtipl2sw.c:1108:9: error: incompatible pointer to integer
> conversion passing 'struct cbd_t *' to parameter of type 'unsigned
> int' [-Wint-conversion] 1108 |         writel(bdp, &fep->cur_rx); |
>              ^~~
> 
> 
> This also assumes that &fep->cur_rx is a pointer to __iomem,
> but that does not seem to be the case.

The writel(bdp, &fep->cur_rx); shall be just replaced with fep->cur_rx
= bdp.

Thanks for spotting.

> 
> > +	spin_unlock_irqrestore(&fep->hw_lock, flags);
> > +
> > +	return pkt_received;
> > +}  
> 
> ...
> 
> > +static int __init mtip_switch_dma_init(struct switch_enet_private
> > *fep) +{
> > +	struct cbd_t *bdp, *cbd_base;
> > +	int ret, i;
> > +
> > +	/* Check mask of the streaming and coherent API */
> > +	ret = dma_set_mask_and_coherent(&fep->pdev->dev,
> > DMA_BIT_MASK(32));
> > +	if (ret < 0) {
> > +		dev_err(&fep->pdev->dev, "No suitable DMA
> > available\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Allocate memory for buffer descriptors */
> > +	cbd_base = dma_alloc_coherent(&fep->pdev->dev, PAGE_SIZE,
> > &fep->bd_dma,
> > +				      GFP_KERNEL);
> > +	if (!cbd_base)
> > +		return -ENOMEM;
> > +
> > +	/* Set receive and transmit descriptor base */
> > +	fep->rx_bd_base = cbd_base;
> > +	fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> > +
> > +	/* Initialize the receive buffer descriptors */
> > +	bdp = fep->rx_bd_base;
> > +	for (i = 0; i < RX_RING_SIZE; i++) {
> > +		bdp->cbd_sc = 0;
> > +		bdp++;
> > +	}
> > +
> > +	/* Set the last buffer to wrap */
> > +	bdp--;
> > +	bdp->cbd_sc |= BD_SC_WRAP;
> > +
> > +	/* ...and the same for transmmit */  
> 
> nit: transmit

Ok.

> 
> > +	bdp = fep->tx_bd_base;
> > +	for (i = 0; i < TX_RING_SIZE; i++) {
> > +		/* Initialize the BD for every fragment in the
> > page */
> > +		bdp->cbd_sc = 0;
> > +		bdp->cbd_bufaddr = 0;
> > +		bdp++;
> > +	}
> > +
> > +	/* Set the last buffer to wrap */
> > +	bdp--;
> > +	bdp->cbd_sc |= BD_SC_WRAP;
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtip_ndev_cleanup(struct switch_enet_private *fep)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > +		if (fep->ndev[i]) {
> > +			unregister_netdev(fep->ndev[i]);
> > +			free_netdev(fep->ndev[i]);
> > +		}
> > +	}
> > +}  
> 
> ...
> 
> > +static const struct of_device_id mtipl2_of_match[] = {
> > +	{ .compatible = "nxp,imx28-mtip-switch",
> > +	  .data = &mtip_imx28_l2switch_info},
> > +	{ /* sentinel */ }
> > +}  
> 
> There should be a trailing ';' on the line above.
> 

+1

> ...
> 
> > +struct  addr_table64b_entry {  
> 
> One space after struct is enough.
> 
> > +	unsigned int lo;  /* lower 32 bits */
> > +	unsigned int hi;  /* upper 32 bits */
> > +};
> > +
> > +struct  mtip_addr_table_t {  
> 
> I think you can drop the '_t' from the name of this struct.
> We know it is a type :)

Ok.

> 
> > +	struct addr_table64b_entry  mtip_table64b_entry[2048];  
> 
> One space is enough after addr_table64b_entry.
> And in general, unless the aim is to align field names
> (not here because there is only one field :)
> 
> > +};
> > +
> > +#define MCF_ESW_LOOKUP_MEM_OFFSET      0x4000
> > +#define MCF_ESW_ENET_PORT_OFFSET      0x4000
> > +#define ENET_SWI_PHYS_ADDR_OFFSET	0x8000  
> 
> Ditto.
> 

+1

> ...
> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
> > b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c  
> 
> > new file mode 100644
> > index 000000000000..0b76a60858a5
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c
> > @@ -0,0 +1,122 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + *  L2 switch Controller driver for MTIP block - bridge network
> > interface
> > + *
> > + *  Copyright (C) 2025 DENX Software Engineering GmbH
> > + *  Lukasz Majewski <lukma@denx.de>
> > + */
> > +
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "mtipl2sw.h"  
> 
> Blank line here please.
> 

Ok.

> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev,
> > +			       struct netlink_ext_ack *extack)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev),
> > *other_priv;
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct net_device *other_ndev;
> > +
> > +	/* Check if one port of MTIP switch is already bridged */
> > +	if (fep->br_members && !fep->br_offload) {
> > +		/* Get the second bridge ndev */
> > +		other_ndev = fep->ndev[fep->br_members - 1];
> > +		other_priv = netdev_priv(other_ndev);
> > +		if (other_priv->master_dev != br_ndev) {
> > +			NL_SET_ERR_MSG_MOD(extack,
> > +					   "L2 offloading only
> > possible for the same bridge!");
> > +			return notifier_from_errno(-EOPNOTSUPP);
> > +		}
> > +
> > +		fep->br_offload = 1;
> > +		mtip_switch_dis_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}
> > +
> > +	if (!priv->master_dev)
> > +		priv->master_dev = br_ndev;
> > +
> > +	fep->br_members |= BIT(priv->portnum - 1);
> > +
> > +	dev_dbg(&ndev->dev,
> > +		"%s: ndev: %s br: %s fep: 0x%x members: 0x%x
> > offload: %d\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep,  
> 
> Perhaps it would be best to use %p as the format specifier for fep
> and not cast it.
> 
> On x86_64 int is 32-bits while pointers are 64 bits.
> W=1 builds with gcc 14.2.0 flag this problem like this:
> 
> .../mtipl2sw_br.c:45:55: warning: cast from pointer to integer of
> different size [-Wpointer-to-int-cast] 45 |                 __func__,
> ndev->name,  br_ndev->name, (unsigned int)fep, |
>                                  ^

Ok.

> 
> > +		fep->br_members, fep->br_offload);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n",
> > __func__,
> > +		ndev->name, fep->br_members);
> > +
> > +	fep->br_members &= ~BIT(priv->portnum - 1);
> > +	priv->master_dev = NULL;
> > +
> > +	if (!fep->br_members) {
> > +		fep->br_offload = 0;
> > +		mtip_switch_en_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}
> > +}  
> 
> ...
> 




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 v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Stefan Wahren]

Hi Lukasz,

Am 07.04.25 um 16:51 schrieb Lukasz Majewski:
> This patch enables support for More Than IP switch available on some
> imx28[7] devices.
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
thanks adding the driver to mxs_defconfig. Unfortunately it's not
possible for reviewers to identify the relevant changes, also the commit
messages doesn't provide further information.

In general there are two approaches to solves this:
1) prepend an additional patch which synchronizes mxs_defconfig with
current mainline
2) manually create the relevant changes against mxs_defconfig

The decision about the approaches is up to the maintainer.

Btw driver review will follow ...

Regards
> ---
> Changes for v4:
> - New patch
> ---
>   arch/arm/configs/mxs_defconfig | 14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
> index d8a6e43c401e..4dc4306c035f 100644
> --- a/arch/arm/configs/mxs_defconfig
> +++ b/arch/arm/configs/mxs_defconfig
> @@ -32,11 +32,10 @@ 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_BRIDGE=y
> +CONFIG_NET_SWITCHDEV=y
>   CONFIG_CAN=m
>   # CONFIG_WIRELESS is not set
>   CONFIG_DEVTMPFS=y
> @@ -45,7 +44,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
> @@ -56,11 +54,11 @@ 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
>   CONFIG_REALTEK_PHY=y
> -CONFIG_SMSC_PHY=y
>   CONFIG_CAN_FLEXCAN=m
>   CONFIG_USB_USBNET=y
>   CONFIG_USB_NET_SMSC95XX=y
> @@ -77,13 +75,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
> @@ -138,10 +134,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
>   CONFIG_TMPFS=y
>   CONFIG_TMPFS_POSIX_ACL=y

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

[Andrew Lunn]

On Mon, Apr 07, 2025 at 04:51:55PM +0200, Lukasz Majewski wrote:
> 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>

    Andrew

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

[Andrew Lunn]

> +static int mtip_ndev_port_link(struct net_device *ndev,
> +			       struct net_device *br_ndev,
> +			       struct netlink_ext_ack *extack)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev), *other_priv;
> +	struct switch_enet_private *fep = priv->fep;
> +	struct net_device *other_ndev;
> +
> +	/* Check if one port of MTIP switch is already bridged */
> +	if (fep->br_members && !fep->br_offload) {
> +		/* Get the second bridge ndev */
> +		other_ndev = fep->ndev[fep->br_members - 1];
> +		other_priv = netdev_priv(other_ndev);
> +		if (other_priv->master_dev != br_ndev) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "L2 offloading only possible for the same bridge!");
> +			return notifier_from_errno(-EOPNOTSUPP);

This is not an error condition as such. The switch cannot do it, so
-EOPNOTSUPP is correct, but the Linux software bridge can, and it
will. So there is no need to use NL_SET_ERR_MSG_MOD().


> +		}
> +
> +		fep->br_offload = 1;
> +		mtip_switch_dis_port_separation(fep);
> +		mtip_clear_atable(fep);
> +	}
> +
> +	if (!priv->master_dev)
> +		priv->master_dev = br_ndev;
> +
> +	fep->br_members |= BIT(priv->portnum - 1);
> +
> +	dev_dbg(&ndev->dev,
> +		"%s: ndev: %s br: %s fep: 0x%x members: 0x%x offload: %d\n",
> +		__func__, ndev->name,  br_ndev->name, (unsigned int)fep,
> +		fep->br_members, fep->br_offload);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n", __func__,
> +		ndev->name, fep->br_members);
> +
> +	fep->br_members &= ~BIT(priv->portnum - 1);
> +	priv->master_dev = NULL;
> +
> +	if (!fep->br_members) {
> +		fep->br_offload = 0;
> +		mtip_switch_en_port_separation(fep);
> +		mtip_clear_atable(fep);
> +	}

This does not look quite correct. So you disable port separation once
both ports are a member of the same bridge. So you should enable port
separation as soon as one leaves. So if fep->br_members has 0 or 1
bits set, you need to enable port separation.

	Andrew

Re: [net-next v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Lukasz Majewski]

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

Hi Stefan,

> Hi Lukasz,
> 
> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:
> > This patch enables support for More Than IP switch available on some
> > imx28[7] devices.
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> thanks adding the driver to mxs_defconfig. Unfortunately it's not
> possible for reviewers to identify the relevant changes,

Could you be more specific here?
As fair as I see - there is only 14 LOCs changed for review.

Please also be aware that MTIP L2 switch driver has some dependencies -
on e.g. SWITCHDEV and BRIDGE, which had to be enabled to allow the
former one to be active.

> also the
> commit messages doesn't provide further information.
> 

What kind of extra information shall I provide? IMHO the patch is
self-explaining.

> In general there are two approaches to solves this:
> 1) prepend an additional patch which synchronizes mxs_defconfig with
> current mainline
> 2) manually create the relevant changes against mxs_defconfig
> 
> The decision about the approaches is up to the maintainer.

I took the linux-next's (or net-next) mxs defconfig (cp it to be
.config)

Then run CROSS_COMPILE= ... make ARCH=arm menuconfig
Enabled all the relevant Kconfig options and run

CROSS_COMPILE= ... make ARCH=arm savedefconfig
and copy defconfig to mxs_defconfig.
Then I used git to prepare the patch.

Isn't the above procedure correct?


> 
> Btw driver review will follow ...
> 
> Regards
> > ---
> > Changes for v4:
> > - New patch
> > ---
> >   arch/arm/configs/mxs_defconfig | 14 +++-----------
> >   1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm/configs/mxs_defconfig
> > b/arch/arm/configs/mxs_defconfig index d8a6e43c401e..4dc4306c035f
> > 100644 --- a/arch/arm/configs/mxs_defconfig
> > +++ b/arch/arm/configs/mxs_defconfig
> > @@ -32,11 +32,10 @@ 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_BRIDGE=y
> > +CONFIG_NET_SWITCHDEV=y
> >   CONFIG_CAN=m
> >   # CONFIG_WIRELESS is not set
> >   CONFIG_DEVTMPFS=y
> > @@ -45,7 +44,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
> > @@ -56,11 +54,11 @@ 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
> >   CONFIG_REALTEK_PHY=y
> > -CONFIG_SMSC_PHY=y
> >   CONFIG_CAN_FLEXCAN=m
> >   CONFIG_USB_USBNET=y
> >   CONFIG_USB_NET_SMSC95XX=y
> > @@ -77,13 +75,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
> > @@ -138,10 +134,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
> >   CONFIG_TMPFS=y
> >   CONFIG_TMPFS_POSIX_ACL=y  
> 




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 v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Krzysztof Kozlowski]

On 10/04/2025 09:01, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> Hi Lukasz,
>>
>> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:
>>> This patch enables support for More Than IP switch available on some
>>> imx28[7] devices.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>  
>> thanks adding the driver to mxs_defconfig. Unfortunately it's not
>> possible for reviewers to identify the relevant changes,
> 
> Could you be more specific here?
> As fair as I see - there is only 14 LOCs changed for review.

Really, the comment was very specific. You make multiple independent,
looking irrelevant changes to the file.

> 
> Please also be aware that MTIP L2 switch driver has some dependencies -
> on e.g. SWITCHDEV and BRIDGE, which had to be enabled to allow the
> former one to be active.
> 
>> also the
>> commit messages doesn't provide further information.
>>
> 
> What kind of extra information shall I provide? IMHO the patch is
> self-explaining.

For example explain why do you think GPIO_SYSFS should be dropped.

> 
>> In general there are two approaches to solves this:
>> 1) prepend an additional patch which synchronizes mxs_defconfig with
>> current mainline
>> 2) manually create the relevant changes against mxs_defconfig
>>
>> The decision about the approaches is up to the maintainer.
> 
> I took the linux-next's (or net-next) mxs defconfig (cp it to be
> .config)
> 
> Then run CROSS_COMPILE= ... make ARCH=arm menuconfig
> Enabled all the relevant Kconfig options and run
> 
> CROSS_COMPILE= ... make ARCH=arm savedefconfig
> and copy defconfig to mxs_defconfig.
> Then I used git to prepare the patch.
> 
> Isn't the above procedure correct?

No, it is not correct. Do not make any changes in the "Enabled all the
relevant Kconfig options and run" step and check the result. Do you see
difference in result file? If yes, then why such difference should be
part of this commit?

Best regards,
Krzysztof

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

[Lukasz Majewski]

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

Hi Andrew,

> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev,
> > +			       struct netlink_ext_ack *extack)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev),
> > *other_priv;
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct net_device *other_ndev;
> > +
> > +	/* Check if one port of MTIP switch is already bridged */
> > +	if (fep->br_members && !fep->br_offload) {
> > +		/* Get the second bridge ndev */
> > +		other_ndev = fep->ndev[fep->br_members - 1];
> > +		other_priv = netdev_priv(other_ndev);
> > +		if (other_priv->master_dev != br_ndev) {
> > +			NL_SET_ERR_MSG_MOD(extack,
> > +					   "L2 offloading only
> > possible for the same bridge!");
> > +			return notifier_from_errno(-EOPNOTSUPP);  
> 
> This is not an error condition as such. The switch cannot do it, so
> -EOPNOTSUPP is correct,

Ok, so the:

return notifier_from_errno(-EOPNOTSUPP);

is correct.


> but the Linux software bridge can, and it
> will. So there is no need to use NL_SET_ERR_MSG_MOD().
> 

This NL_SET_ERR_MSG_MOD() only is relevant for two ports managed by
MTIP L2 switch driver.

As a result - other bridges created by other drivers will not follow
this execution path.

Considering the above - I would keep this message - it is informative
for the potential user (similar approach has been taken when HSR driver
was added for KSZ9477 switch).

> 
> > +		}
> > +
> > +		fep->br_offload = 1;
> > +		mtip_switch_dis_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}
> > +
> > +	if (!priv->master_dev)
> > +		priv->master_dev = br_ndev;
> > +
> > +	fep->br_members |= BIT(priv->portnum - 1);
> > +
> > +	dev_dbg(&ndev->dev,
> > +		"%s: ndev: %s br: %s fep: 0x%x members: 0x%x
> > offload: %d\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep,
> > +		fep->br_members, fep->br_offload);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n",
> > __func__,
> > +		ndev->name, fep->br_members);
> > +
> > +	fep->br_members &= ~BIT(priv->portnum - 1);
> > +	priv->master_dev = NULL;
> > +
> > +	if (!fep->br_members) {
> > +		fep->br_offload = 0;
> > +		mtip_switch_en_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}  
> 
> This does not look quite correct. So you disable port separation once
> both ports are a member of the same bridge. So you should enable port
> separation as soon as one leaves. So if fep->br_members has 0 or 1
> bits set, you need to enable port separation.

Yes, the if (!fep->br_members)

shall be replaced with:

if (fep->br_members && fep->br_offload)

to avoid situation when disabling second port would re-enable
separation and clear atable.

> 
> 	Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

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

Re: [net-next v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Stefan Wahren]

Am 10.04.25 um 09:01 schrieb Lukasz Majewski:
> Hi Stefan,
>
>> Hi Lukasz,
>>
>> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:
>>> This patch enables support for More Than IP switch available on some
>>> imx28[7] devices.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>> thanks adding the driver to mxs_defconfig. Unfortunately it's not
>> possible for reviewers to identify the relevant changes,
> Could you be more specific here?
> As fair as I see - there is only 14 LOCs changed for review.
Yes, this is exactly the problem. The subject suggests enable one
driver, but the changes are not self-explaining.
> Please also be aware that MTIP L2 switch driver has some dependencies -
> on e.g. SWITCHDEV and BRIDGE, which had to be enabled to allow the
> former one to be active.
This should be part of the commit message.
>
>> also the
>> commit messages doesn't provide further information.
>>
> What kind of extra information shall I provide? IMHO the patch is
> self-explaining.
No, it is not. How should a reviewer know that

CONFIG_MTD_M25P80=y

should be dropped? This is completely unrelated to the change.
>
>> In general there are two approaches to solves this:
>> 1) prepend an additional patch which synchronizes mxs_defconfig with
>> current mainline
>> 2) manually create the relevant changes against mxs_defconfig
>>
>> The decision about the approaches is up to the maintainer.
> I took the linux-next's (or net-next) mxs defconfig (cp it to be
> .config)
>
> Then run CROSS_COMPILE= ... make ARCH=arm menuconfig
> Enabled all the relevant Kconfig options and run
>
> CROSS_COMPILE= ... make ARCH=arm savedefconfig
> and copy defconfig to mxs_defconfig.
> Then I used git to prepare the patch.
>
> Isn't the above procedure correct?
The problem here is that you send a patch for two steps. First is to
synchronize mxs_defconfig against current tree and the second is to
enable the driver.

Regards
>
>
>> Btw driver review will follow ...
>>
>> Regards
>>> ---
>>> Changes for v4:
>>> - New patch
>>> ---
>>>    arch/arm/configs/mxs_defconfig | 14 +++-----------
>>>    1 file changed, 3 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/arm/configs/mxs_defconfig
>>> b/arch/arm/configs/mxs_defconfig index d8a6e43c401e..4dc4306c035f
>>> 100644 --- a/arch/arm/configs/mxs_defconfig
>>> +++ b/arch/arm/configs/mxs_defconfig
>>> @@ -32,11 +32,10 @@ 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_BRIDGE=y
>>> +CONFIG_NET_SWITCHDEV=y
>>>    CONFIG_CAN=m
>>>    # CONFIG_WIRELESS is not set
>>>    CONFIG_DEVTMPFS=y
>>> @@ -45,7 +44,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
>>> @@ -56,11 +54,11 @@ 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
>>>    CONFIG_REALTEK_PHY=y
>>> -CONFIG_SMSC_PHY=y
>>>    CONFIG_CAN_FLEXCAN=m
>>>    CONFIG_USB_USBNET=y
>>>    CONFIG_USB_NET_SMSC95XX=y
>>> @@ -77,13 +75,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
>>> @@ -138,10 +134,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
>>>    CONFIG_TMPFS=y
>>>    CONFIG_TMPFS_POSIX_ACL=y
>
>
>
> 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

Re: [net-next v4 5/5] ARM: mxs_defconfig: Enable CONFIG_FEC_MTIP_L2SW to support MTIP L2 switch

[Lukasz Majewski]

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

Hi Krzysztof,

> On 10/04/2025 09:01, Lukasz Majewski wrote:
> > Hi Stefan,
> >   
> >> Hi Lukasz,
> >>
> >> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:  
> >>> This patch enables support for More Than IP switch available on
> >>> some imx28[7] devices.
> >>>
> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>    
> >> thanks adding the driver to mxs_defconfig. Unfortunately it's not
> >> possible for reviewers to identify the relevant changes,  
> > 
> > Could you be more specific here?
> > As fair as I see - there is only 14 LOCs changed for review.  
> 
> Really, the comment was very specific. You make multiple independent,
> looking irrelevant changes to the file.
> 
> > 
> > Please also be aware that MTIP L2 switch driver has some
> > dependencies - on e.g. SWITCHDEV and BRIDGE, which had to be
> > enabled to allow the former one to be active.
> >   
> >> also the
> >> commit messages doesn't provide further information.
> >>  
> > 
> > What kind of extra information shall I provide? IMHO the patch is
> > self-explaining.  
> 
> For example explain why do you think GPIO_SYSFS should be dropped.
> 
> >   
> >> In general there are two approaches to solves this:
> >> 1) prepend an additional patch which synchronizes mxs_defconfig
> >> with current mainline
> >> 2) manually create the relevant changes against mxs_defconfig
> >>
> >> The decision about the approaches is up to the maintainer.  
> > 
> > I took the linux-next's (or net-next) mxs defconfig (cp it to be
> > .config)
> > 
> > Then run CROSS_COMPILE= ... make ARCH=arm menuconfig
> > Enabled all the relevant Kconfig options and run
> > 
> > CROSS_COMPILE= ... make ARCH=arm savedefconfig
> > and copy defconfig to mxs_defconfig.
> > Then I used git to prepare the patch.
> > 
> > Isn't the above procedure correct?  
> 
> No, it is not correct. Do not make any changes in the "Enabled all the
> relevant Kconfig options and run" step and check the result. Do you
> see difference in result file? If yes, then why such difference
> should be part of this commit?

Ok, I get your point. Then I shall prepare a separate, pre-patch.
Thanks for info.

> 
> Best regards,
> Krzysztof


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

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

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

[Rob Herring]

On Mon, Apr 07, 2025 at 04:51:53PM +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>
> ---
> 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
> ---
>  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 126 ++++++++++++++++++
>  1 file changed, 126 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..1afaf8029725
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> @@ -0,0 +1,126 @@
> +# 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.
> +
> +$ref: ethernet-switch.yaml#/$defs/ethernet-ports
> +

> +patternProperties:
> +  "^(ethernet-)?ports$":

New bindings should only use 'ethernet-ports'.

> +    type: object
> +    additionalProperties: true

But what's this for? I thought you had some constrants for phy-mode and 
phy-handle?

> +
> +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
> +
> +  pinctrl-names: true
> +
> +  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
> +  - mdio
> +  - ethernet-ports
> +
> +additionalProperties: false

unevaluatedProperties: false

> +
> +examples:
> +  - |
> +    #include<dt-bindings/interrupt-controller/irq.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>;
> +        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 0>;
> +            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 v4 1/5] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Rob,

> On Mon, Apr 07, 2025 at 04:51:53PM +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>
> > ---
> > 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
> > ---
> >  .../bindings/net/nxp,imx28-mtip-switch.yaml   | 126
> > ++++++++++++++++++ 1 file changed, 126 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..1afaf8029725 --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> > @@ -0,0 +1,126 @@ +# 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.
> > +
> > +$ref: ethernet-switch.yaml#/$defs/ethernet-ports
> > +  
> 
> > +patternProperties:
> > +  "^(ethernet-)?ports$":  
> 
> New bindings should only use 'ethernet-ports'.

Ok.

> 
> > +    type: object
> > +    additionalProperties: true  
> 
> But what's this for? I thought you had some constrants for phy-mode
> and phy-handle?
> 

I've prepared following changes:

--- a/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
+++ b/Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
@@ -17,11 +17,6 @@ description:
 
 $ref: ethernet-switch.yaml#/$defs/ethernet-ports
 
-patternProperties:
-  "^(ethernet-)?ports$":
-    type: object
-    additionalProperties: true
-
 properties:
   compatible:
     const: nxp,imx28-mtip-switch
@@ -55,6 +50,26 @@ properties:
 
   pinctrl-names: true
 
+  ethernet-ports:
+    type: object
+    additionalProperties: true
+    properties:
+      ethernet-port:
+        type: object
+        unevaluatedProperties: false
+
+        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#
@@ -71,7 +86,7 @@ required:
   - mdio
   - ethernet-ports
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:

To be applied on top of this patch. It takes into account the
"required:" for 'ethernet-port' and also reuses the 
$ref: ethernet-switch.yaml#/$defs/ethernet-ports

Last, but not least:
make dt_binding_check DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml
make CHECK_DTBS=y DT_SCHEMA_FILES=nxp,imx28-mtip-switch.yaml
nxp/mxs/imx28-xea.dtb

show no errors.


> > +
> > +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
> > +
> > +  pinctrl-names: true
> > +
> > +  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
> > +  - mdio
> > +  - ethernet-ports
> > +
> > +additionalProperties: false  
> 
> unevaluatedProperties: false

Ok.

> 
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/irq.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>;
> > +        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 0>;
> > +            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: 484 bytes --]

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

[Lukasz Majewski]

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

Hi Simon,

> > It is unclear why hwentry, which is a pointer, is being cast to an
> > integer and then back to a pointer. I see pointer arithmetic, but
> > that can operate on pointers just as well as integers, without
> > making assumptions about how wide pointers are with respect to
> > longs.
> > 
> > And in any case, can't the types be used to directly access the
> > offsets needed like this?
> > 
> > 	atable = fep->hwentry.mtip_table64b_entry;
> > 
> > 	*read_lo = readl(&atable[index].lo);
> > 	*read_hi = readl(&atable[index].hi);
> >   
> 
> The code as is seems to be OK.
> 
> The (atable) memory structure is as follows:
> 
> 1. You can store 2048 MAC addresses (2x32 bit each).
> 
> 2. Memory from point 1 is addressed as follows:
> 	2.1 -> from MAC address the CRC8 is calculated (0x00 - 0xFF).
> 	This is the 'index' in the original code.
> 	2.2 -> as it may happen that for two different MAC address the
> 	same CRC8 is calculated (i.e. 'index' is the same), each
> 	'index' can store 8 entries for MAC addresses (and it is
> 	searched in a linear way if needed).
> 
> IMHO, the index above shall be multiplied by 8.

I've double check it and it turned out that you were right :-)

The following code:

struct addr_table64b_entry *atable_base =
fep->hwentry->mtip_table64b_entry;

*read_lo = readl(&atable_base[index].lo);
*read_hi = readl(&atable_base[index].hi);

Is more readable than the current code.

The same would be used for atable_write()

I will change it for v5.

> 
> > Also, and perhaps more importantly, readl expects to be passed
> > a pointer to __iomem. But the appropriate annotations seem
> > to be missing (forcing them with a cast is not advisable here IMHO).
> >   
> 
> I think that the code below:
> unsigned long atable_base = (unsigned long)fep->hwentry;
> 
> could be replaced with
> void __iomem *atable_base = fep->hwentry;
> 
> and the (index << 3) with (index * ATABLE_ENTRY_PER_SLOT)




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 v4 4/5] net: mtip: The L2 switch driver for imx287

[Stefan Wahren]

Am 10.04.25 um 15:37 schrieb Lukasz Majewski:
> Hi Stefan,
>
>> Hi Lukasz,
>>
>> thanks for sending this to linux-arm-kernel
>>
>> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:
>>> This patch series provides support for More Than IP L2 switch
>>> embedded in the imx287 SoC.
>>>
>>> This is a two port switch (placed between uDMA[01] and MAC-NET[01]),
>>> which can be used for offloading the network traffic.
>>>
>>> It can be used interchangeably with current FEC driver - to be more
>>> specific: one can use either of it, depending on the requirements.
>>>
>>> The biggest difference is the usage of DMA - when FEC is used,
>>> separate DMAs are available for each ENET-MAC block.
>>> However, with switch enabled - only the DMA0 is used to
>>> send/receive data to/form switch (and then switch sends them to
>>> respecitive ports).
>>>
>>> Signed-off-by: Lukasz Majewski<lukma@denx.de>
>>> ---
>>> Changes for v2:
>>>
>>> - Remove not needed comments
>>> - Restore udelay(10) for switch reset (such delay is explicitly
>>> specifed in the documentation
>>> - Add COMPILE_TEST
>>> - replace pr_* with dev_*
>>> - Use for_each_available_child_of_node_scoped()
>>> - Use devm_* function for memory allocation
>>> - Remove printing information about the HW and SW revision of the
>>> driver
>>> - Use devm_regulator_get_optional()
>>> - Change compatible prefix from 'fsl' to more up to date 'nxp'
>>> - Remove .owner = THIS_MODULE
>>> - Use devm_platform_ioremap_resource(pdev, 0);
>>> - Use devm_request_irq()
>>> - Use devm_regulator_get_enable_optional()
>>> - Replace clk_prepare_enable() and devm_clk_get() with single
>>>     call to devm_clk_get_optional_enabled()
>>> - Cleanup error patch when function calls in probe fail
>>> - Refactor the mtip_reset_phy() to serve as mdio bus reset callback
>>> - Add myself as the MTIP L2 switch maintainer (squashed the
>>> separated commit)
>>> - More descriptive help paragraphs (> 4 lines)
>>>
>>> Changes for v3:
>>> - Remove 'bridge_offloading' module parameter (to bridge ports just
>>> after probe)
>>> - Remove forward references
>>> - Fix reverse christmas tree formatting in functions
>>> - Convert eligible comments to kernel doc format
>>> - Remove extra MAC address validation check at esw_mac_addr_static()
>>> - Remove mtip_print_link_status() and replace it with
>>> phy_print_status()
>>> - Avoid changing phy device state in the driver (instead use
>>> functions exported by the phy API)
>>> - Do not print extra information regarding PHY (which is printed by
>>> phylib) - e.g. net lan0: lan0: MTIP eth L2 switch 1e:ce:a5:0b:4c:12
>>> - Remove VERSION from the driver - now we rely on the SHA1 in Linux
>>>     mainline tree
>>> - Remove zeroing of the net device private area (shall be already
>>> done during allocation)
>>> - Refactor the code to remove mtip_ndev_setup()
>>> - Use -ENOMEM instead of -1 return code when allocation fails
>>> - Replace dev_info() with dev_dbg() to reduce number of information
>>> print on normal operation
>>> - Return ret instead of 0 from mtip_ndev_init()
>>> - Remove fep->mii_timeout flag from the driver
>>> - Remove not used stop_gpr_* fields in mtip_devinfo struct
>>> - Remove platform_device_id description for mtipl2sw driver
>>> - Add MODULE_DEVICE_TABLE() for mtip_of_match
>>> - Remove MODULE_ALIAS()
>>>
>>> Changes for v4:
>>> - Rename imx287 with imx28 (as the former is not used in kernel
>>> anymore)
>>> - Reorder the place where ENET interface is initialized - without
>>> this change the enet_out clock has default (25 MHz) value, which
>>> causes issues during reset (RMII's 50 MHz is required for proper
>>> PHY reset).
>>> - Use PAUR instead of PAUR register to program MAC address
>>> - Replace eth_mac_addr() with eth_hw_addr_set()
>>> - Write to HW the randomly generated MAC address (if required)
>>> - Adjust the reset code
>>> - s/read_atable/mtip_read_atable/g and
>>> s/write_atable/mtip_write_atable/g
>>> - Add clk_disable() and netif_napi_del() when errors occur during
>>>     mtip_open() - refactor the error handling path.
>>> - Refactor the mtip_set_multicast_list() to write (now) correct
>>> values to ENET-FEC registers.
>>> - Replace dev_warn() with dev_err()
>>> - Use GPIO_ACTIVE_LOW to indicate polarity in DTS
>>> - Refactor code to check if network device is the switch device
>>> - Remove mtip_port_dev_check()
>>> - Refactor mtip_ndev_port_link() avoid starting HW offloading for
>>> bridge when MTIP ports are parts of two distinct bridges
>>> - Replace del_timer() with timer_delete_sync()
>>> ---
>>>    MAINTAINERS                                   |    7 +
>>>    drivers/net/ethernet/freescale/Kconfig        |    1 +
>>>    drivers/net/ethernet/freescale/Makefile       |    1 +
>>>    drivers/net/ethernet/freescale/mtipsw/Kconfig |   13 +
>>>    .../net/ethernet/freescale/mtipsw/Makefile    |    3 +
>>>    .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 1970
>>> +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |
>>> 782 +++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  122 +
>>>    .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  449 ++++
>>>    9 files changed, 3348 insertions(+)
>>>    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
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 4c5c2e2c1278..9c5626c2b3b7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -9455,6 +9455,13 @@ S:	Maintained
>>>    F:	Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
>>>    F:	drivers/i2c/busses/i2c-mpc.c
>>>
>>> +FREESCALE MTIP ETHERNET SWITCH DRIVER
>>> +M:	Lukasz Majewski<lukma@denx.de>
>>> +L:	netdev@vger.kernel.org
>>> +S:	Maintained
>>> +F:
>>> Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
>>> +F:	drivers/net/ethernet/freescale/mtipsw/* +
>>>    FREESCALE QORIQ DPAA ETHERNET DRIVER
>>>    M:	Madalin Bucur<madalin.bucur@nxp.com>
>>>    L:	netdev@vger.kernel.org
>>> diff --git a/drivers/net/ethernet/freescale/Kconfig
>>> b/drivers/net/ethernet/freescale/Kconfig index
>>> a2d7300925a8..056a11c3a74e 100644 ---
>>> a/drivers/net/ethernet/freescale/Kconfig +++
>>> b/drivers/net/ethernet/freescale/Kconfig @@ -60,6 +60,7 @@ config
>>> FEC_MPC52xx_MDIO
>>>
>>>    source "drivers/net/ethernet/freescale/fs_enet/Kconfig"
>>>    source "drivers/net/ethernet/freescale/fman/Kconfig"
>>> +source "drivers/net/ethernet/freescale/mtipsw/Kconfig"
>>>
>>>    config FSL_PQ_MDIO
>>>    	tristate "Freescale PQ MDIO" diff --git
>>> a/drivers/net/ethernet/freescale/Makefile
>>> b/drivers/net/ethernet/freescale/Makefile index
>>> de7b31842233..0e6cacb0948a 100644 ---
>>> a/drivers/net/ethernet/freescale/Makefile +++
>>> b/drivers/net/ethernet/freescale/Makefile @@ -25,3 +25,4 @@
>>> obj-$(CONFIG_FSL_DPAA_ETH) += dpaa/ obj-$(CONFIG_FSL_DPAA2_ETH) +=
>>> dpaa2/ obj-y += enetc/ +obj-y += mtipsw/ diff --git
>>> a/drivers/net/ethernet/freescale/mtipsw/Kconfig
>>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig new file mode
>>> 100644 index 000000000000..450ff734a321 --- /dev/null +++
>>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig @@ -0,0 +1,13 @@ +#
>>> SPDX-License-Identifier: GPL-2.0-only +config FEC_MTIP_L2SW +
>>> tristate "MoreThanIP L2 switch support to FEC driver"
>>> +	depends on OF
>>> +	depends on NET_SWITCHDEV
>>> +	depends on BRIDGE
>>> +	depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST
>>> +	help
>>> +	  This enables support for the MoreThan IP L2 switch on
>>> i.MX
>>> +	  SoCs (e.g. iMX28, vf610). It offloads bridging to this
>>> IP block's
>> This is confusing. The Kconfig and most of the code looks prepared for
>> other platforms than i.MX28, but there is only a i.MX28 OF
>> compatible.
> I've took the approach to upstream the driver in several steps.
> The first step is this patch set - add the code for a single platform
> (imx28).
>
> And Yes, I also have on my desk another board with soc having this IP
> block (vf610).
>
> However, I will not start any other upstream work until patches from
> this "step" are not pulled.
>
> (To follow "one things at a time" principle)
>
>> I don't like that Kconfig pretent something, which is not
>> true.
> If you prefer I can remove 'depends on ARCH_MXC' and the vf610 SoC...
> (only to add it afterwards).
In case you want to do "one thing at a time", please remove this.
>>> +
>>> +static void mtip_config_switch(struct switch_enet_private *fep)
>>> +{
>>> +	struct switch_t *fecp = fep->hwp;
>>> +
>>> +	esw_mac_addr_static(fep);
>>> +
>>> +	writel(0, &fecp->ESW_BKLR);
>>> +
>>> +	/* Do NOT disable learning */
>>> +	mtip_port_learning_config(fep, 0, 0, 0);
>>> +	mtip_port_learning_config(fep, 1, 0, 0);
>>> +	mtip_port_learning_config(fep, 2, 0, 0);
>> Looks like the last 2 parameter are always 0?
> Those functions are defined in mtipl2sw_mgnt.c file.
>
> I've followed the way legacy (i.e. vendor) driver has defined them. In
> this particular case the last '0' is to not enable interrupt for
> learning.
This wasn't my concern. The question was "why do we need a parameter if
it's always the same?". But you answered this further below, so i'm fine.
>>> +
>>> +static irqreturn_t mtip_interrupt(int irq, void *ptr_fep)
>>> +{
>>> +	struct switch_enet_private *fep = ptr_fep;
>>> +	struct switch_t *fecp = fep->hwp;
>>> +	irqreturn_t ret = IRQ_NONE;
>>> +	u32 int_events, int_imask;
>>> +
>>> +	/* Get the interrupt events that caused us to be here */
>>> +	do {
>>> +		int_events = readl(&fecp->ESW_ISR);
>>> +		writel(int_events, &fecp->ESW_ISR);
>>> +
>>> +		if (int_events & (MCF_ESW_ISR_RXF |
>>> MCF_ESW_ISR_TXF)) {
>>> +			ret = IRQ_HANDLED;
>>> +			/* Disable the RX interrupt */
>>> +			if (napi_schedule_prep(&fep->napi)) {
>>> +				int_imask = readl(&fecp->ESW_IMR);
>>> +				int_imask &= ~MCF_ESW_IMR_RXF;
>>> +				writel(int_imask, &fecp->ESW_IMR);
>>> +				__napi_schedule(&fep->napi);
>>> +			}
>>> +		}
>>> +	} while (int_events);
>> This looks bad, in case of bad hardware / timing behavior this
>> interrupt handler will loop forever.
> The 'writel(int_events, &fecp->ESW_ISR);'
>
> clears the interrupts, so after reading them and clearing (by writing
> the same value), the int_events shall be 0.
>
> Also, during probe the IRQ mask for switch IRQ is written, so only
> expected (and served) interrupts are delivered.
This was not my point. A possible endless loop especially in a interrupt
handler should be avoided. The kernel shouldn't trust the hardware and
the driver should be fair to all the other interrupts which might have
occurred.
> +
> +static int mtip_rx_napi(struct napi_struct *napi, int budget)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(napi->dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct switch_t *fecp = fep->hwp;
> +	int pkts, port;
> +
> +	pkts = mtip_switch_rx(napi->dev, budget, &port);
> +	if (!fep->br_offload &&
> +	    (port == 1 || port == 2) && fep->ndev[port - 1])
>> (port > 0) && fep->ndev[port - 1])
> This needs to be kept as is - only when port is set to 1 or 2 (after
> reading the switch internal register) this shall be executed.
Oops, missed that
> (port also can be 0xFF or 0x3 -> then we shall send frame to both
> egress ports).
Maybe we should use defines for such special values
> This code is responsible for port separation when bridge HW offloading
> is disabled.
>
>
>>> +		of_get_mac_address(port, &fep->mac[port_num -
>>> 1][0]);
>> This can fail
> I will add:
>
> ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]);
> if (ret)
> 	dev_warn(dev, "of_get_mac_address(%pOF) failed\n", port);
>
> as it is also valid to not have the mac address defined in DTS (then
> some random based value is generated).
AFAIK this is a little bit more complex. It's possible that the MAC is
stored within a NVMEM and the driver isn't ready (EPROBE_DEFER).

Are you sure about the randomize fallback behavior of of_get_mac_address() ?

I tought you still need to call eth_random_addr().


>>> +
>>> +int mtip_set_vlan_verification(struct switch_enet_private *fep,
>>> int port,
>>> +			       int vlan_domain_verify_en,
>>> +			       int vlan_discard_unknown_en)
>>> +{
>>> +	struct switch_t *fecp = fep->hwp;
>>> +
>>> +	if (port < 0 || port > 2) {
>>> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not
>>> supported!\n",
>>> +			__func__, port);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (vlan_domain_verify_en == 1) {
>>> +		if (port == 0)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_VV0,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 1)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_VV1,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 2)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_VV2,
>>> +			       &fecp->ESW_VLANV);
>>> +	} else if (vlan_domain_verify_en == 0) {
>>> +		if (port == 0)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_VV0,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 1)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_VV1,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 2)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_VV2,
>>> +			       &fecp->ESW_VLANV);
>>> +	}
>>> +
>>> +	if (vlan_discard_unknown_en == 1) {
>>> +		if (port == 0)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_DU0,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 1)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_DU1,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 2)
>>> +			writel(readl(&fecp->ESW_VLANV) |
>>> MCF_ESW_VLANV_DU2,
>>> +			       &fecp->ESW_VLANV);
>>> +	} else if (vlan_discard_unknown_en == 0) {
>>> +		if (port == 0)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_DU0,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 1)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_DU1,
>>> +			       &fecp->ESW_VLANV);
>>> +		else if (port == 2)
>>> +			writel(readl(&fecp->ESW_VLANV) &
>>> ~MCF_ESW_VLANV_DU2,
>>> +			       &fecp->ESW_VLANV);
>>> +	}
>> This looks like a lot of copy & paste
> IMHO, the readability of the code is OK.
Actually the concern was about maintenance.

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

[Fabio Estevam]

Hi Lukasz,

On Mon, Apr 7, 2025 at 11:52 AM Lukasz Majewski <lukma@denx.de> wrote:

> +               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) */

Please fix the multi-line comment style.

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

[Lukasz Majewski]

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

Hi Fabio,

> Hi Lukasz,
> 
> On Mon, Apr 7, 2025 at 11:52 AM Lukasz Majewski <lukma@denx.de> wrote:
> 
> > +               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) */  
> 
> Please fix the multi-line comment style.

Ok, I will fix it.


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 v4 4/5] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

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

Hi Stefan,

> Am 10.04.25 um 15:37 schrieb Lukasz Majewski:
> > Hi Stefan,
> >  
> >> Hi Lukasz,
> >>
> >> thanks for sending this to linux-arm-kernel
> >>
> >> Am 07.04.25 um 16:51 schrieb Lukasz Majewski:  
> >>> This patch series provides support for More Than IP L2 switch
> >>> embedded in the imx287 SoC.
> >>>
> >>> This is a two port switch (placed between uDMA[01] and
> >>> MAC-NET[01]), which can be used for offloading the network
> >>> traffic.
> >>>
> >>> It can be used interchangeably with current FEC driver - to be
> >>> more specific: one can use either of it, depending on the
> >>> requirements.
> >>>
> >>> The biggest difference is the usage of DMA - when FEC is used,
> >>> separate DMAs are available for each ENET-MAC block.
> >>> However, with switch enabled - only the DMA0 is used to
> >>> send/receive data to/form switch (and then switch sends them to
> >>> respecitive ports).
> >>>
> >>> Signed-off-by: Lukasz Majewski<lukma@denx.de>
> >>> ---
> >>> Changes for v2:
> >>>
> >>> - Remove not needed comments
> >>> - Restore udelay(10) for switch reset (such delay is explicitly
> >>> specifed in the documentation
> >>> - Add COMPILE_TEST
> >>> - replace pr_* with dev_*
> >>> - Use for_each_available_child_of_node_scoped()
> >>> - Use devm_* function for memory allocation
> >>> - Remove printing information about the HW and SW revision of the
> >>> driver
> >>> - Use devm_regulator_get_optional()
> >>> - Change compatible prefix from 'fsl' to more up to date 'nxp'
> >>> - Remove .owner = THIS_MODULE
> >>> - Use devm_platform_ioremap_resource(pdev, 0);
> >>> - Use devm_request_irq()
> >>> - Use devm_regulator_get_enable_optional()
> >>> - Replace clk_prepare_enable() and devm_clk_get() with single
> >>>     call to devm_clk_get_optional_enabled()
> >>> - Cleanup error patch when function calls in probe fail
> >>> - Refactor the mtip_reset_phy() to serve as mdio bus reset
> >>> callback
> >>> - Add myself as the MTIP L2 switch maintainer (squashed the
> >>> separated commit)
> >>> - More descriptive help paragraphs (> 4 lines)
> >>>
> >>> Changes for v3:
> >>> - Remove 'bridge_offloading' module parameter (to bridge ports
> >>> just after probe)
> >>> - Remove forward references
> >>> - Fix reverse christmas tree formatting in functions
> >>> - Convert eligible comments to kernel doc format
> >>> - Remove extra MAC address validation check at
> >>> esw_mac_addr_static()
> >>> - Remove mtip_print_link_status() and replace it with
> >>> phy_print_status()
> >>> - Avoid changing phy device state in the driver (instead use
> >>> functions exported by the phy API)
> >>> - Do not print extra information regarding PHY (which is printed
> >>> by phylib) - e.g. net lan0: lan0: MTIP eth L2 switch
> >>> 1e:ce:a5:0b:4c:12
> >>> - Remove VERSION from the driver - now we rely on the SHA1 in
> >>> Linux mainline tree
> >>> - Remove zeroing of the net device private area (shall be already
> >>> done during allocation)
> >>> - Refactor the code to remove mtip_ndev_setup()
> >>> - Use -ENOMEM instead of -1 return code when allocation fails
> >>> - Replace dev_info() with dev_dbg() to reduce number of
> >>> information print on normal operation
> >>> - Return ret instead of 0 from mtip_ndev_init()
> >>> - Remove fep->mii_timeout flag from the driver
> >>> - Remove not used stop_gpr_* fields in mtip_devinfo struct
> >>> - Remove platform_device_id description for mtipl2sw driver
> >>> - Add MODULE_DEVICE_TABLE() for mtip_of_match
> >>> - Remove MODULE_ALIAS()
> >>>
> >>> Changes for v4:
> >>> - Rename imx287 with imx28 (as the former is not used in kernel
> >>> anymore)
> >>> - Reorder the place where ENET interface is initialized - without
> >>> this change the enet_out clock has default (25 MHz) value, which
> >>> causes issues during reset (RMII's 50 MHz is required for proper
> >>> PHY reset).
> >>> - Use PAUR instead of PAUR register to program MAC address
> >>> - Replace eth_mac_addr() with eth_hw_addr_set()
> >>> - Write to HW the randomly generated MAC address (if required)
> >>> - Adjust the reset code
> >>> - s/read_atable/mtip_read_atable/g and
> >>> s/write_atable/mtip_write_atable/g
> >>> - Add clk_disable() and netif_napi_del() when errors occur during
> >>>     mtip_open() - refactor the error handling path.
> >>> - Refactor the mtip_set_multicast_list() to write (now) correct
> >>> values to ENET-FEC registers.
> >>> - Replace dev_warn() with dev_err()
> >>> - Use GPIO_ACTIVE_LOW to indicate polarity in DTS
> >>> - Refactor code to check if network device is the switch device
> >>> - Remove mtip_port_dev_check()
> >>> - Refactor mtip_ndev_port_link() avoid starting HW offloading for
> >>> bridge when MTIP ports are parts of two distinct bridges
> >>> - Replace del_timer() with timer_delete_sync()
> >>> ---
> >>>    MAINTAINERS                                   |    7 +
> >>>    drivers/net/ethernet/freescale/Kconfig        |    1 +
> >>>    drivers/net/ethernet/freescale/Makefile       |    1 +
> >>>    drivers/net/ethernet/freescale/mtipsw/Kconfig |   13 +
> >>>    .../net/ethernet/freescale/mtipsw/Makefile    |    3 +
> >>>    .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 1970
> >>> +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |
> >>> 782 +++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  122 +
> >>>    .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  449 ++++
> >>>    9 files changed, 3348 insertions(+)
> >>>    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
> >>>
> >>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>> index 4c5c2e2c1278..9c5626c2b3b7 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -9455,6 +9455,13 @@ S:	Maintained
> >>>    F:	Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
> >>>    F:	drivers/i2c/busses/i2c-mpc.c
> >>>
> >>> +FREESCALE MTIP ETHERNET SWITCH DRIVER
> >>> +M:	Lukasz Majewski<lukma@denx.de>
> >>> +L:	netdev@vger.kernel.org
> >>> +S:	Maintained
> >>> +F:
> >>> Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml
> >>> +F:	drivers/net/ethernet/freescale/mtipsw/* +
> >>>    FREESCALE QORIQ DPAA ETHERNET DRIVER
> >>>    M:	Madalin Bucur<madalin.bucur@nxp.com>
> >>>    L:	netdev@vger.kernel.org
> >>> diff --git a/drivers/net/ethernet/freescale/Kconfig
> >>> b/drivers/net/ethernet/freescale/Kconfig index
> >>> a2d7300925a8..056a11c3a74e 100644 ---
> >>> a/drivers/net/ethernet/freescale/Kconfig +++
> >>> b/drivers/net/ethernet/freescale/Kconfig @@ -60,6 +60,7 @@ config
> >>> FEC_MPC52xx_MDIO
> >>>
> >>>    source "drivers/net/ethernet/freescale/fs_enet/Kconfig"
> >>>    source "drivers/net/ethernet/freescale/fman/Kconfig"
> >>> +source "drivers/net/ethernet/freescale/mtipsw/Kconfig"
> >>>
> >>>    config FSL_PQ_MDIO
> >>>    	tristate "Freescale PQ MDIO" diff --git
> >>> a/drivers/net/ethernet/freescale/Makefile
> >>> b/drivers/net/ethernet/freescale/Makefile index
> >>> de7b31842233..0e6cacb0948a 100644 ---
> >>> a/drivers/net/ethernet/freescale/Makefile +++
> >>> b/drivers/net/ethernet/freescale/Makefile @@ -25,3 +25,4 @@
> >>> obj-$(CONFIG_FSL_DPAA_ETH) += dpaa/ obj-$(CONFIG_FSL_DPAA2_ETH) +=
> >>> dpaa2/ obj-y += enetc/ +obj-y += mtipsw/ diff --git
> >>> a/drivers/net/ethernet/freescale/mtipsw/Kconfig
> >>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig new file mode
> >>> 100644 index 000000000000..450ff734a321 --- /dev/null +++
> >>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig @@ -0,0 +1,13 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only +config FEC_MTIP_L2SW +
> >>> tristate "MoreThanIP L2 switch support to FEC driver"
> >>> +	depends on OF
> >>> +	depends on NET_SWITCHDEV
> >>> +	depends on BRIDGE
> >>> +	depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST
> >>> +	help
> >>> +	  This enables support for the MoreThan IP L2 switch on
> >>> i.MX
> >>> +	  SoCs (e.g. iMX28, vf610). It offloads bridging to this
> >>> IP block's  
> >> This is confusing. The Kconfig and most of the code looks prepared
> >> for other platforms than i.MX28, but there is only a i.MX28 OF
> >> compatible.  
> > I've took the approach to upstream the driver in several steps.
> > The first step is this patch set - add the code for a single
> > platform (imx28).
> >
> > And Yes, I also have on my desk another board with soc having this
> > IP block (vf610).
> >
> > However, I will not start any other upstream work until patches from
> > this "step" are not pulled.
> >
> > (To follow "one things at a time" principle)
> >  
> >> I don't like that Kconfig pretent something, which is not
> >> true.  
> > If you prefer I can remove 'depends on ARCH_MXC' and the vf610
> > SoC... (only to add it afterwards).  
> In case you want to do "one thing at a time", please remove this.

+1

> >>> +
> >>> +static void mtip_config_switch(struct switch_enet_private *fep)
> >>> +{
> >>> +	struct switch_t *fecp = fep->hwp;
> >>> +
> >>> +	esw_mac_addr_static(fep);
> >>> +
> >>> +	writel(0, &fecp->ESW_BKLR);
> >>> +
> >>> +	/* Do NOT disable learning */
> >>> +	mtip_port_learning_config(fep, 0, 0, 0);
> >>> +	mtip_port_learning_config(fep, 1, 0, 0);
> >>> +	mtip_port_learning_config(fep, 2, 0, 0);  
> >> Looks like the last 2 parameter are always 0?  
> > Those functions are defined in mtipl2sw_mgnt.c file.
> >
> > I've followed the way legacy (i.e. vendor) driver has defined them.
> > In this particular case the last '0' is to not enable interrupt for
> > learning.  
> This wasn't my concern. The question was "why do we need a parameter
> if it's always the same?". But you answered this further below, so
> i'm fine.

Ok.

> >>> +
> >>> +static irqreturn_t mtip_interrupt(int irq, void *ptr_fep)
> >>> +{
> >>> +	struct switch_enet_private *fep = ptr_fep;
> >>> +	struct switch_t *fecp = fep->hwp;
> >>> +	irqreturn_t ret = IRQ_NONE;
> >>> +	u32 int_events, int_imask;
> >>> +
> >>> +	/* Get the interrupt events that caused us to be here */
> >>> +	do {
> >>> +		int_events = readl(&fecp->ESW_ISR);
> >>> +		writel(int_events, &fecp->ESW_ISR);
> >>> +
> >>> +		if (int_events & (MCF_ESW_ISR_RXF |
> >>> MCF_ESW_ISR_TXF)) {
> >>> +			ret = IRQ_HANDLED;
> >>> +			/* Disable the RX interrupt */
> >>> +			if (napi_schedule_prep(&fep->napi)) {
> >>> +				int_imask =
> >>> readl(&fecp->ESW_IMR);
> >>> +				int_imask &= ~MCF_ESW_IMR_RXF;
> >>> +				writel(int_imask,
> >>> &fecp->ESW_IMR);
> >>> +				__napi_schedule(&fep->napi);
> >>> +			}
> >>> +		}
> >>> +	} while (int_events);  
> >> This looks bad, in case of bad hardware / timing behavior this
> >> interrupt handler will loop forever.  
> > The 'writel(int_events, &fecp->ESW_ISR);'
> >
> > clears the interrupts, so after reading them and clearing (by
> > writing the same value), the int_events shall be 0.
> >
> > Also, during probe the IRQ mask for switch IRQ is written, so only
> > expected (and served) interrupts are delivered.  
> This was not my point. A possible endless loop especially in a
> interrupt handler should be avoided. The kernel shouldn't trust the
> hardware and the driver should be fair to all the other interrupts
> which might have occurred.

Ok.

> > +
> > +static int mtip_rx_napi(struct napi_struct *napi, int budget)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(napi->dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct switch_t *fecp = fep->hwp;
> > +	int pkts, port;
> > +
> > +	pkts = mtip_switch_rx(napi->dev, budget, &port);
> > +	if (!fep->br_offload &&
> > +	    (port == 1 || port == 2) && fep->ndev[port - 1])  
> >> (port > 0) && fep->ndev[port - 1])  
> > This needs to be kept as is - only when port is set to 1 or 2 (after
> > reading the switch internal register) this shall be executed.  
> Oops, missed that
> > (port also can be 0xFF or 0x3 -> then we shall send frame to both
> > egress ports).  
> Maybe we should use defines for such special values

Port = 1 or port = 2 seems to be self explanatory.

Nonetheless, I've added MTIP_PORT_FORWARDING_INIT for 0xFF initial
forwarding value.

> > This code is responsible for port separation when bridge HW
> > offloading is disabled.
> >
> >  
> >>> +		of_get_mac_address(port, &fep->mac[port_num -
> >>> 1][0]);  
> >> This can fail  
> > I will add:
> >
> > ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]);
> > if (ret)
> > 	dev_warn(dev, "of_get_mac_address(%pOF) failed\n", port);
> >
> > as it is also valid to not have the mac address defined in DTS (then
> > some random based value is generated).  
> AFAIK this is a little bit more complex. It's possible that the MAC is
> stored within a NVMEM and the driver isn't ready (EPROBE_DEFER).
> 

In my use case - always the local mac address is provided during
production (to ethernet-port nodes).

The other possibility is no MAC address provided, that is why there is
dev_warn().

> Are you sure about the randomize fallback behavior of
> of_get_mac_address() ?
> 
> I tought you still need to call eth_random_addr().
> 

The "random" MAC address is generated in another place - namely
mtip_setup_mac();

> 
> >>> +
> >>> +int mtip_set_vlan_verification(struct switch_enet_private *fep,
> >>> int port,
> >>> +			       int vlan_domain_verify_en,
> >>> +			       int vlan_discard_unknown_en)
> >>> +{
> >>> +	struct switch_t *fecp = fep->hwp;
> >>> +
> >>> +	if (port < 0 || port > 2) {
> >>> +		dev_err(&fep->pdev->dev, "%s: Port (%d) not
> >>> supported!\n",
> >>> +			__func__, port);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	if (vlan_domain_verify_en == 1) {
> >>> +		if (port == 0)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_VV0,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 1)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_VV1,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 2)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_VV2,
> >>> +			       &fecp->ESW_VLANV);
> >>> +	} else if (vlan_domain_verify_en == 0) {
> >>> +		if (port == 0)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_VV0,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 1)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_VV1,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 2)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_VV2,
> >>> +			       &fecp->ESW_VLANV);
> >>> +	}
> >>> +
> >>> +	if (vlan_discard_unknown_en == 1) {
> >>> +		if (port == 0)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_DU0,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 1)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_DU1,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 2)
> >>> +			writel(readl(&fecp->ESW_VLANV) |
> >>> MCF_ESW_VLANV_DU2,
> >>> +			       &fecp->ESW_VLANV);
> >>> +	} else if (vlan_discard_unknown_en == 0) {
> >>> +		if (port == 0)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_DU0,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 1)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_DU1,
> >>> +			       &fecp->ESW_VLANV);
> >>> +		else if (port == 2)
> >>> +			writel(readl(&fecp->ESW_VLANV) &
> >>> ~MCF_ESW_VLANV_DU2,
> >>> +			       &fecp->ESW_VLANV);
> >>> +	}  
> >> This looks like a lot of copy & paste  
> > IMHO, the readability of the code is OK.  
> Actually the concern was about maintenance.

IMHO, (for me) the maintenance cost of this code is acceptable.


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 --]