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

[PATCH 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):
  MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs:
    imx287)
  dt-bindings: net: Add MTIP L2 switch description
    (fec,mtip-switch.yaml)
  arm: dts: Adjust the 'reg' range for imx287 L2 switch description
  arm: dts: imx287: Provide description for MTIP L2 switch
  net: mtip: The L2 switch driver for imx287

 .../bindings/net/fec,mtip-switch.yaml         |  160 ++
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts       |   56 +
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi          |    4 +-
 drivers/net/ethernet/freescale/Kconfig        |    1 +
 drivers/net/ethernet/freescale/Makefile       |    1 +
 drivers/net/ethernet/freescale/mtipsw/Kconfig |   10 +
 .../net/ethernet/freescale/mtipsw/Makefile    |    6 +
 .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2108 +++++++++++++++++
 .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  784 ++++++
 .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
 .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  434 ++++
 12 files changed, 3682 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/fec,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

[PATCH 1/5] MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs: imx287)

[Lukasz Majewski]

Add myself as a maintainer for this particular network driver.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5959513a7359..255edd825fa1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9270,6 +9270,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/fec,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
-- 
2.39.5

[PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 .../bindings/net/fec,mtip-switch.yaml         | 160 ++++++++++++++++++
 1 file changed, 160 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/fec,mtip-switch.yaml

diff --git a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
new file mode 100644
index 000000000000..cd85385e0f79
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
@@ -0,0 +1,160 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale MTIP Level 2 (L2) switch
+
+maintainers:
+  - Lukasz Majewski <lukma@denx.de>
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+	  - imx287-mtip-switch
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 3
+
+  clocks:
+    maxItems: 4
+    description:
+      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for register accessing.
+      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
+      The "ptp"(option), for IEEE1588 timer clock that requires the clock.
+      The "enet_out"(option), output clock for external device, like supply clock
+      for PHY. The clock is required if PHY clock source from SOC.
+
+  clock-names:
+    minItems: 4
+    maxItems: 4
+    items:
+      enum:
+	- ipg
+	- ahb
+	- ptp
+	- enet_out
+
+  phy-supply:
+    description:
+      Regulator that powers the Ethernet PHY.
+
+  phy-reset-gpios:
+    deprecated: true
+    description:
+      Should specify the gpio for phy reset.
+
+  phy-reset-duration:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    deprecated: true
+    description:
+      Reset duration in milliseconds.  Should present only if property
+      "phy-reset-gpios" is available.  Missing the property will have the
+      duration be 1 millisecond.  Numbers greater than 1000 are invalid
+      and 1 millisecond will be used instead.
+
+  phy-reset-post-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    deprecated: true
+    description:
+      Post reset delay in milliseconds. If present then a delay of phy-reset-post-delay
+      milliseconds will be observed after the phy-reset-gpios has been toggled.
+      Can be omitted thus no delay is observed. Delay is in range of 1ms to 1000ms.
+      Other delays are invalid.
+
+  mdio:
+    $ref: mdio.yaml#
+    unevaluatedProperties: false
+    description:
+      Specifies the mdio bus in the FEC, used as a container for phy nodes.
+
+  ethernet-ports:
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      "^port@[0-9a-f]+$":
+	$ref: /schemas/net/ethernet-switch-controller.yaml#
+	unevaluatedProperties: false
+
+	properties:
+	  reg:
+	    description: Switch port number
+
+	required:
+	  - reg
+	  - phy-mode
+	  - phy-handle
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - mdio
+  - ethernet-ports
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    switch@800f0000 {
+	compatible = "fsl,imx287-mtip-switch";
+	pinctrl-names = "default";
+	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
+	phy-supply = <&reg_fec_3v3>;
+	phy-reset-duration = <25>;
+	phy-reset-post-delay = <10>;
+	interrupts = <100>, <101>, <102>;
+	clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
+	clock-names = "ipg", "ahb", "enet_out", "ptp";
+	status = "okay";
+
+	ethernet-ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mtip_port1: port@1 {
+			reg = <1>;
+			label = "lan0";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy0>;
+		};
+
+		mtip_port2: port@2 {
+			reg = <2>;
+			label = "lan1";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy1>;
+		};
+	};
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		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 (AND'ed) */
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+		};
+
+		ethphy1: ethernet-phy@1 {
+			reg = <1>;
+			smsc,disable-energy-detect;
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+		};
+	};
+    };
-- 
2.39.5

[PATCH 3/5] arm: dts: Adjust the 'reg' range for imx287 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.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 4 ++--
 1 file changed, 2 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..8f2aa32208cf 100644
--- a/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
+++ b/arch/arm/boot/dts/nxp/mxs/imx28.dtsi
@@ -1321,8 +1321,8 @@ mac1: ethernet@800f4000 {
 			status = "disabled";
 		};
 
-		eth_switch: switch@800f8000 {
-			reg = <0x800f8000 0x8000>;
+		eth_switch: switch@800f0000 {
+			reg = <0x800f0000 0x20000>;
 			status = "disabled";
 		};
 	};
-- 
2.39.5

[PATCH 4/5] arm: dts: imx287: Provide description for MTIP L2 switch

[Lukasz Majewski]

This description is similar to one supprted with the cpsw_new.c
driver.

It has separated ports and PHYs (connected to mdio bus).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56 +++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
index 6c5e6856648a..e645b086574d 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"
 
 / {
@@ -18,6 +19,61 @@ &can0 {
 	status = "okay";
 };
 
+&eth_switch {
+	compatible = "fsl,imx287-mtip-switch";
+	pinctrl-names = "default";
+	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
+	phy-supply = <&reg_fec_3v3>;
+	phy-reset-duration = <25>;
+	phy-reset-post-delay = <10>;
+	interrupts = <100>, <101>, <102>;
+	clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
+	clock-names = "ipg", "ahb", "enet_out", "ptp";
+	status = "okay";
+
+	ethernet-ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mtip_port1: port@1 {
+			reg = <1>;
+			label = "lan0";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy0>;
+		};
+
+		mtip_port2: port@2 {
+			reg = <2>;
+			label = "lan1";
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			phy-mode = "rmii";
+			phy-handle = <&ethphy1>;
+		};
+	};
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		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 (AND'ed) */
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+		};
+
+		ethphy1: ethernet-phy@1 {
+			reg = <1>;
+			smsc,disable-energy-detect;
+			interrupt-parent = <&gpio4>;
+			interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+		};
+	};
+};
+
 &i2c1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c1_pins_b>;
-- 
2.39.5

Re: [PATCH 1/5] MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs: imx287)

[Krzysztof Kozlowski]

On 25/03/2025 12:57, Lukasz Majewski wrote:
> Add myself as a maintainer for this particular network driver.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5959513a7359..255edd825fa1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9270,6 +9270,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/fec,mtip-switch.yaml
> +F:	drivers/net/ethernet/freescale/mtipsw/*

You need to re-order the patches, there are no such files yet, so this
causes warnings.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski]

On 25/03/2025 12:57, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - imx287, vf610.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  .../bindings/net/fec,mtip-switch.yaml         | 160 ++++++++++++++++++

Use compatible as filename.

>  1 file changed, 160 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> new file mode 100644
> index 000000000000..cd85385e0f79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale MTIP Level 2 (L2) switch
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +

description?

> +allOf:
> +  - $ref: ethernet-controller.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:

Drop, you have only one variant.

> +      - enum:
> +	  - imx287-mtip-switch

This wasn't tested. Except whitespace errors, above compatible does not
have format of compatible. Please look at other NXP bindings.

Missing blank line.

> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 3

Need to list items instead.

> +
> +  clocks:
> +    maxItems: 4
> +    description:
> +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for register accessing.
> +      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
> +      The "ptp"(option), for IEEE1588 timer clock that requires the clock.
> +      The "enet_out"(option), output clock for external device, like supply clock
> +      for PHY. The clock is required if PHY clock source from SOC.

Same problems. This binding does not look at all as any other binding. I
finish review here, but the code has similar trivial issues all the way,
including incorrect indentation. Start from well reviewed existing
binding or example-schema.

Best regards,
Krzysztof

Re: [PATCH 3/5] arm: dts: Adjust the 'reg' range for imx287 L2 switch description

[Krzysztof Kozlowski]

On 25/03/2025 12:57, Lukasz Majewski wrote:
> 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.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Missing nxp or mxs.

Best regards,
Krzysztof

Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

[Krzysztof Kozlowski]

On 25/03/2025 12:57, 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 interchangeable 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.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  drivers/net/ethernet/freescale/Kconfig        |    1 +
>  drivers/net/ethernet/freescale/Makefile       |    1 +
>  drivers/net/ethernet/freescale/mtipsw/Kconfig |   10 +
>  .../net/ethernet/freescale/mtipsw/Makefile    |    6 +
>  .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2108 +++++++++++++++++
>  .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  784 ++++++
>  .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
>  .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  434 ++++
>  8 files changed, 3457 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/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..5cc9b758bad4
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> @@ -0,0 +1,10 @@
> +# 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

Missing compile test

> +	help
> +		This enables support for the MoreThan IP L2 switch on i.MX
> +		SoCs (e.g. iMX28, vf610).

Wrong indentation. Look at other Kconfig files.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/Makefile b/drivers/net/ethernet/freescale/mtipsw/Makefile
> new file mode 100644
> index 000000000000..e87e06d6870a
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for the L2 switch from MTIP embedded in NXP SoCs

Drop, obvious.


...

> +
> +static int mtip_parse_of(struct switch_enet_private *fep,
> +			 struct device_node *np)
> +{
> +	struct device_node *port, *p;
> +	unsigned int port_num;
> +	int ret;
> +
> +	p = of_find_node_by_name(np, "ethernet-ports");
> +
> +	for_each_available_child_of_node(p, port) {
> +		if (of_property_read_u32(port, "reg", &port_num))
> +			continue;
> +
> +		fep->n_ports = port_num;
> +		of_get_mac_address(port, &fep->mac[port_num - 1][0]);
> +
> +		ret = of_property_read_string(port, "label",
> +					      &fep->ndev_name[port_num - 1]);
> +		if (ret < 0) {
> +			pr_err("%s: Cannot get ethernet port name (%d)!\n",
> +			       __func__, ret);
> +			goto of_get_err;

Just use scoped loop.

> +		}
> +
> +		ret = of_get_phy_mode(port, &fep->phy_interface[port_num - 1]);
> +		if (ret < 0) {
> +			pr_err("%s: Cannot get PHY mode (%d)!\n", __func__,
> +			       ret);

No, drivers do not use pr_xxx. From where did you get this code?

> +			goto of_get_err;
> +		}
> +
> +		fep->phy_np[port_num - 1] = of_parse_phandle(port,
> +							     "phy-handle", 0);
> +	}
> +
> + of_get_err:
> +	of_node_put(p);
> +
> +	return 0;
> +}
> +
> +static int mtip_sw_learning(void *arg)
> +{
> +	struct switch_enet_private *fep = arg;
> +
> +	while (!kthread_should_stop()) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		/* check learning record valid */
> +		mtip_atable_dynamicms_learn_migration(fep, fep->curr_time,
> +						      NULL, NULL);
> +		schedule_timeout(HZ / 100);
> +	}
> +
> +	return 0;
> +}
> +
> +static void mtip_mii_unregister(struct switch_enet_private *fep)
> +{
> +	mdiobus_unregister(fep->mii_bus);
> +	mdiobus_free(fep->mii_bus);
> +}
> +
> +static const struct fec_devinfo fec_imx28_l2switch_info = {
> +	.quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO,
> +};
> +
> +static struct platform_device_id pdev_id = {
> +	.name = "imx28-l2switch",
> +	.driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info,
> +};
> +
> +static int __init mtip_sw_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct switch_enet_private *fep;
> +	struct fec_devinfo *dev_info;
> +	struct switch_t *fecp;
> +	struct resource *r;
> +	int err, ret;
> +	u32 rev;
> +
> +	pr_info("Ethernet Switch Version %s\n", VERSION);

Drivers use dev, not pr. Anyway drop. Drivers do not have versions and
should be silent on success.

> +
> +	fep = kzalloc(sizeof(*fep), GFP_KERNEL);

Why not devm? See last comment here (at the end).

> +	if (!fep)
> +		return -ENOMEM;
> +
> +	pdev->id_entry = &pdev_id;
> +
> +	dev_info = (struct fec_devinfo *)pdev->id_entry->driver_data;
> +	if (dev_info)
> +		fep->quirks = dev_info->quirks;
> +
> +	fep->pdev = pdev;
> +	platform_set_drvdata(pdev, fep);
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	fep->enet_addr = devm_ioremap_resource(&pdev->dev, r);

Use proper wrapper.

> +	if (IS_ERR(fep->enet_addr)) {
> +		ret = PTR_ERR(fep->enet_addr);
> +		goto failed_ioremap;
> +	}
> +
> +	fep->irq = platform_get_irq(pdev, 0);
> +
> +	ret = mtip_parse_of(fep, np);
> +	if (ret < 0) {
> +		pr_err("%s: OF parse error (%d)!\n", __func__, ret);
> +		goto failed_parse_of;
> +	}
> +
> +	/* Create an Ethernet device instance.
> +	 * The switch lookup address memory starts 0x800FC000
> +	 */
> +	fep->hwp_enet = fep->enet_addr;
> +	fecp = (struct switch_t *)(fep->enet_addr + ENET_SWI_PHYS_ADDR_OFFSET);
> +
> +	fep->hwp = fecp;
> +	fep->hwentry = (struct mtip_addr_table_t *)
> +		((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET);
> +
> +	rev = readl(&fecp->ESW_REVISION);
> +	pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> +		MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> +		MCF_MTIP_REVISION_CORE_REVISION(rev));

Drop

> +
> +	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> +	if (!IS_ERR(fep->reg_phy)) {
> +		ret = regulator_enable(fep->reg_phy);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"Failed to enable phy regulator: %d\n", ret);
> +			goto failed_regulator;
> +		}
> +	} else {
> +		if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto failed_regulator;
> +		}
> +		fep->reg_phy = NULL;

I don't understand this code. Do you want to re-implement get_optional?
But why?

> +	}
> +
> +	fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	if (IS_ERR(fep->clk_ipg))
> +		fep->clk_ipg = NULL;

Why?

> +
> +	fep->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(fep->clk_ahb))
> +		fep->clk_ahb = NULL;
> +
> +	fep->clk_enet_out = devm_clk_get(&pdev->dev, "enet_out");
> +	if (IS_ERR(fep->clk_enet_out))
> +		fep->clk_enet_out = NULL;
> +
> +	ret = clk_prepare_enable(fep->clk_ipg);
> +	if (ret) {
> +		pr_err("%s: clock ipg cannot be enabled\n", __func__);
> +		goto failed_clk_ipg;
> +	}
> +
> +	ret = clk_prepare_enable(fep->clk_ahb);
> +	if (ret) {
> +		pr_err("%s: clock ahb cannot be enabled\n", __func__);
> +		goto failed_clk_ahb;
> +	}
> +
> +	ret = clk_prepare_enable(fep->clk_enet_out);
> +	if (ret)
> +		pr_err("%s: clock clk_enet_out cannot be enabled\n", __func__);
> +
> +	spin_lock_init(&fep->learn_lock);
> +
> +	ret = mtip_reset_phy(pdev);
> +	if (ret < 0) {
> +		pr_err("%s: GPIO PHY RST error (%d)!\n", __func__, ret);
> +		goto get_phy_mode_err;
> +	}
> +
> +	ret = request_irq(fep->irq, mtip_interrupt, 0, "mtip_l2sw", fep);
> +	if (ret) {
> +		pr_err("MTIP_L2: Could not alloc IRQ(%d)!\n", fep->irq);
> +		goto request_irq_err;
> +	}
> +
> +	spin_lock_init(&fep->hw_lock);
> +	spin_lock_init(&fep->mii_lock);
> +
> +	ret = mtip_register_notifiers(fep);
> +	if (ret)
> +		goto clean_unregister_netdev;
> +
> +	ret = mtip_ndev_init(fep);
> +	if (ret) {
> +		pr_err("%s: Failed to create virtual ndev (%d)\n",
> +		       __func__, ret);
> +		goto mtip_ndev_init_err;
> +	}
> +
> +	err = mtip_switch_dma_init(fep);
> +	if (err) {
> +		pr_err("%s: ethernet switch init fail (%d)!\n", __func__, err);
> +		goto mtip_switch_dma_init_err;
> +	}
> +
> +	err = mtip_mii_init(fep, pdev);
> +	if (err)
> +		pr_err("%s: Cannot init phy bus (%d)!\n", __func__, err);
> +
> +	/* setup timer for learning aging function */
> +	timer_setup(&fep->timer_aging, mtip_aging_timer, 0);
> +	mod_timer(&fep->timer_aging,
> +		  jiffies + msecs_to_jiffies(LEARNING_AGING_INTERVAL));
> +
> +	fep->task = kthread_run(mtip_sw_learning, fep, "mtip_l2sw_learning");
> +	if (IS_ERR(fep->task)) {
> +		ret = PTR_ERR(fep->task);
> +		pr_err("%s: learning kthread_run error (%d)!\n", __func__, ret);
> +		goto fail_task_learning;
> +	}
> +
> +	/* setup MII interface for external switch ports*/
> +	mtip_enet_init(fep, 1);
> +	mtip_enet_init(fep, 2);
> +
> +	return 0;
> +
> + fail_task_learning:
> +	mtip_mii_unregister(fep);
> +	del_timer(&fep->timer_aging);
> + mtip_switch_dma_init_err:
> +	mtip_ndev_cleanup(fep);
> + mtip_ndev_init_err:
> +	mtip_unregister_notifiers(fep);
> + clean_unregister_netdev:
> +	free_irq(fep->irq, NULL);
> + request_irq_err:
> +	platform_set_drvdata(pdev, NULL);
> + get_phy_mode_err:
> +	if (fep->clk_enet_out)
> +		clk_disable_unprepare(fep->clk_enet_out);
> +	clk_disable_unprepare(fep->clk_ahb);
> + failed_clk_ahb:
> +	clk_disable_unprepare(fep->clk_ipg);
> + failed_clk_ipg:
> +	if (fep->reg_phy) {
> +		regulator_disable(fep->reg_phy);
> +		devm_regulator_put(fep->reg_phy);
> +	}
> + failed_regulator:
> + failed_parse_of:
> +	devm_ioremap_release(&pdev->dev, r);
> + failed_ioremap:
> +	kfree(fep);
> +	return err;
> +}
> +
> +static void mtip_sw_remove(struct platform_device *pdev)
> +{
> +	struct switch_enet_private *fep = platform_get_drvdata(pdev);
> +
> +	mtip_unregister_notifiers(fep);
> +	mtip_ndev_cleanup(fep);
> +
> +	mtip_mii_remove(fep);
> +
> +	kthread_stop(fep->task);
> +	del_timer(&fep->timer_aging);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	kfree(fep);
> +}
> +
> +static const struct of_device_id mtipl2_of_match[] = {
> +	{ .compatible = "fsl,imx287-mtip-switch", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver mtipl2plat_driver = {
> +	.probe          = mtip_sw_probe,
> +	.remove         = mtip_sw_remove,
> +	.driver         = {
> +		.name   = "mtipl2sw",
> +		.owner  = THIS_MODULE,

Oh no, please do not send us 10-12 year old code. This is long, looong
time gone. If you copied this pattern, then for sure you copied all
other issues we fixed during last 10 years, so basically you ask us to
re-review the same code we already fixed.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 12:57, Lukasz Majewski wrote:
> > This patch provides description of the MTIP L2 switch available in
> > some NXP's SOCs - imx287, vf610.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  .../bindings/net/fec,mtip-switch.yaml         | 160
> > ++++++++++++++++++  
> 
> Use compatible as filename.

I've followed the fsl,fec.yaml as an example. This file has description
for all the device tree sources from fec_main.c

I've considered adding the full name - e.g. fec,imx287-mtip-switch.yaml
but this driver could (and probably will) be extended to vf610.

So what is the advised way to go?

> 
> >  1 file changed, 160 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> > b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml new
> > file mode 100644 index 000000000000..cd85385e0f79 --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> > @@ -0,0 +1,160 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale MTIP Level 2 (L2) switch
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@denx.de>
> > +  
> 
> description?

Ok.

> 
> > +allOf:
> > +  - $ref: ethernet-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:  
> 
> Drop, you have only one variant.

Ok, for imx287 this can be dropped, and then extended with vf610.

> 
> > +      - enum:
> > +	  - imx287-mtip-switch  
> 
> This wasn't tested. Except whitespace errors, above compatible does
> not have format of compatible. Please look at other NXP bindings.
> 
> Missing blank line.
> 
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 3  
> 
> Need to list items instead.
> 
> > +
> > +  clocks:
> > +    maxItems: 4
> > +    description:
> > +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for
> > register accessing.
> > +      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
> > +      The "ptp"(option), for IEEE1588 timer clock that requires
> > the clock.
> > +      The "enet_out"(option), output clock for external device,
> > like supply clock
> > +      for PHY. The clock is required if PHY clock source from SOC.
> >  
> 
> Same problems. This binding does not look at all as any other
> binding. I finish review here, but the code has similar trivial
> issues all the way, including incorrect indentation. Start from well
> reviewed existing binding or example-schema.

As I've stated above - this code is reduced copy of fsl,fec.yaml...

> 
> 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: [PATCH 1/5] MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs: imx287)

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 12:57, Lukasz Majewski wrote:
> > Add myself as a maintainer for this particular network driver.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  MAINTAINERS | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5959513a7359..255edd825fa1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9270,6 +9270,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/fec,mtip-switch.yaml
> > +F:	drivers/net/ethernet/freescale/mtipsw/*  
> 
> You need to re-order the patches, there are no such files yet, so this
> causes warnings.

This shall be probably squashed with the patch adding mtipl2sw* files.

> 
> 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: [PATCH 3/5] arm: dts: Adjust the 'reg' range for imx287 L2 switch description

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 12:57, Lukasz Majewski wrote:
> > 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.
> >   
> 
> Please use subject prefixes matching the subsystem. You can get them
> for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching. For bindings, the preferred
> subjects are explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> Missing nxp or mxs.

Ok. I will add it.

> 
> 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: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski]

On 25/03/2025 13:15, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 25/03/2025 12:57, Lukasz Majewski wrote:
>>> This patch provides description of the MTIP L2 switch available in
>>> some NXP's SOCs - imx287, vf610.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>  .../bindings/net/fec,mtip-switch.yaml         | 160
>>> ++++++++++++++++++  
>>
>> Use compatible as filename.
> 
> I've followed the fsl,fec.yaml as an example. This file has description
> for all the device tree sources from fec_main.c


That's a 14 year old binding, so clear antipattern.

> 
> I've considered adding the full name - e.g. fec,imx287-mtip-switch.yaml
> but this driver could (and probably will) be extended to vf610.

Unless you add vf610 now, this should follow the compatible name.

> 
> So what is the advised way to go?
> 
>>
>>>  1 file changed, 160 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>> b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml new
>>> file mode 100644 index 000000000000..cd85385e0f79 --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
>>> @@ -0,0 +1,160 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Freescale MTIP Level 2 (L2) switch
>>> +
>>> +maintainers:
>>> +  - Lukasz Majewski <lukma@denx.de>
>>> +  
>>
>> description?
> 
> Ok.
> 
>>
>>> +allOf:
>>> +  - $ref: ethernet-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:  
>>
>> Drop, you have only one variant.
> 
> Ok, for imx287 this can be dropped, and then extended with vf610.
> 
>>
>>> +      - enum:
>>> +	  - imx287-mtip-switch  
>>
>> This wasn't tested. Except whitespace errors, above compatible does
>> not have format of compatible. Please look at other NXP bindings.
>>
>> Missing blank line.
>>
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 3  
>>
>> Need to list items instead.
>>
>>> +
>>> +  clocks:
>>> +    maxItems: 4
>>> +    description:
>>> +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for
>>> register accessing.
>>> +      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
>>> +      The "ptp"(option), for IEEE1588 timer clock that requires
>>> the clock.
>>> +      The "enet_out"(option), output clock for external device,
>>> like supply clock
>>> +      for PHY. The clock is required if PHY clock source from SOC.
>>>  
>>
>> Same problems. This binding does not look at all as any other
>> binding. I finish review here, but the code has similar trivial
>> issues all the way, including incorrect indentation. Start from well
>> reviewed existing binding or example-schema.
> 
> As I've stated above - this code is reduced copy of fsl,fec.yaml...

Don't take the worst, old code with all the anti-patterns we point out
on each review, as an example.

Take the most recent, well reviewed binding as an example. Or
example-schema.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 13:15, Lukasz Majewski wrote:
> > Hi Krzysztof,
> >   
> >> On 25/03/2025 12:57, Lukasz Majewski wrote:  
> >>> This patch provides description of the MTIP L2 switch available in
> >>> some NXP's SOCs - imx287, vf610.
> >>>
> >>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>> ---
> >>>  .../bindings/net/fec,mtip-switch.yaml         | 160
> >>> ++++++++++++++++++    
> >>
> >> Use compatible as filename.  
> > 
> > I've followed the fsl,fec.yaml as an example. This file has
> > description for all the device tree sources from fec_main.c  
> 
> 
> That's a 14 year old binding, so clear antipattern.

For some reason it is still there...

> 
> > 
> > I've considered adding the full name - e.g.
> > fec,imx287-mtip-switch.yaml but this driver could (and probably
> > will) be extended to vf610.  
> 
> Unless you add vf610 now, this should follow the compatible name.

Ok.

> 
> > 
> > So what is the advised way to go?
> >   
> >>  
> >>>  1 file changed, 160 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> >>> b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml new
> >>> file mode 100644 index 000000000000..cd85385e0f79 --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> >>> @@ -0,0 +1,160 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/net/fsl,mtip-switch.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Freescale MTIP Level 2 (L2) switch
> >>> +
> >>> +maintainers:
> >>> +  - Lukasz Majewski <lukma@denx.de>
> >>> +    
> >>
> >> description?  
> > 
> > Ok.
> >   
> >>  
> >>> +allOf:
> >>> +  - $ref: ethernet-controller.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    oneOf:    
> >>
> >> Drop, you have only one variant.  
> > 
> > Ok, for imx287 this can be dropped, and then extended with vf610.
> >   
> >>  
> >>> +      - enum:
> >>> +	  - imx287-mtip-switch    
> >>
> >> This wasn't tested. Except whitespace errors, above compatible does
> >> not have format of compatible. Please look at other NXP bindings.
> >>
> >> Missing blank line.
> >>  
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 3    
> >>
> >> Need to list items instead.
> >>  
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 4
> >>> +    description:
> >>> +      The "ipg", for MAC ipg_clk_s, ipg_clk_mac_s that are for
> >>> register accessing.
> >>> +      The "ahb", for MAC ipg_clk, ipg_clk_mac that are bus clock.
> >>> +      The "ptp"(option), for IEEE1588 timer clock that requires
> >>> the clock.
> >>> +      The "enet_out"(option), output clock for external device,
> >>> like supply clock
> >>> +      for PHY. The clock is required if PHY clock source from
> >>> SOC. 
> >>
> >> Same problems. This binding does not look at all as any other
> >> binding. I finish review here, but the code has similar trivial
> >> issues all the way, including incorrect indentation. Start from
> >> well reviewed existing binding or example-schema.  
> > 
> > As I've stated above - this code is reduced copy of fsl,fec.yaml...
> >  
> 
> Don't take the worst, old code with all the anti-patterns we point out
> on each review, as an example.
> 
> Take the most recent, well reviewed binding as an example. Or
> example-schema.

Ok.

> 
> 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: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski]

On 25/03/2025 13:39, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 25/03/2025 13:15, Lukasz Majewski wrote:
>>> Hi Krzysztof,
>>>   
>>>> On 25/03/2025 12:57, Lukasz Majewski wrote:  
>>>>> This patch provides description of the MTIP L2 switch available in
>>>>> some NXP's SOCs - imx287, vf610.
>>>>>
>>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>>>> ---
>>>>>  .../bindings/net/fec,mtip-switch.yaml         | 160
>>>>> ++++++++++++++++++    
>>>>
>>>> Use compatible as filename.  
>>>
>>> I've followed the fsl,fec.yaml as an example. This file has
>>> description for all the device tree sources from fec_main.c  
>>
>>
>> That's a 14 year old binding, so clear antipattern.
> 
> For some reason it is still there...

And it will be there for very long time. Bindings are not removed just
because they are old, because they are an ABI. That's still not a reason
to use something old as starting point.

It's the same with drivers, although driver can be easier changed and
old pattern can be dropped. You cannot easily drop old, anti-patterns
from the binding.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Rob Herring (Arm)]

On Tue, 25 Mar 2025 12:57:33 +0100, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - imx287, vf610.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  .../bindings/net/fec,mtip-switch.yaml         | 160 ++++++++++++++++++
>  1 file changed, 160 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/fec,mtip-switch.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/fec,mtip-switch.yaml:19:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/fec,mtip-switch.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/net/fec,mtip-switch.yaml:19:1: found character '\t' that cannot start any token
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/fec,mtip-switch.example.dts'
Documentation/devicetree/bindings/net/fec,mtip-switch.yaml:19:1: found character '\t' that cannot start any token
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/fec,mtip-switch.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250325115736.1732721-3-lukma@denx.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 12:57, 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 interchangeable 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.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  drivers/net/ethernet/freescale/Kconfig        |    1 +
> >  drivers/net/ethernet/freescale/Makefile       |    1 +
> >  drivers/net/ethernet/freescale/mtipsw/Kconfig |   10 +
> >  .../net/ethernet/freescale/mtipsw/Makefile    |    6 +
> >  .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2108
> > +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |
> > 784 ++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
> >  .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  434 ++++
> >  8 files changed, 3457 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/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..5cc9b758bad4
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > @@ -0,0 +1,10 @@
> > +# 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  
> 
> Missing compile test

Could you be more specific?

I'm compiling and testing this driver for the last week... (6.6 LTS +
net-next).

> 
> > +	help
> > +		This enables support for the MoreThan IP L2 switch
> > on i.MX
> > +		SoCs (e.g. iMX28, vf610).  
> 
> Wrong indentation. Look at other Kconfig files.

The indentation from Kconfig from FEC:

<tab>help
<tab><2 spaces>FOO BAR....

also causes the checkpatch on net-next to generated WARNING.

> 
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/Makefile
> > b/drivers/net/ethernet/freescale/mtipsw/Makefile new file mode
> > 100644 index 000000000000..e87e06d6870a
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Makefile for the L2 switch from MTIP embedded in NXP SoCs  
> 
> Drop, obvious.

Ok.

> 
> 
> ...
> 
> > +
> > +static int mtip_parse_of(struct switch_enet_private *fep,
> > +			 struct device_node *np)
> > +{
> > +	struct device_node *port, *p;
> > +	unsigned int port_num;
> > +	int ret;
> > +
> > +	p = of_find_node_by_name(np, "ethernet-ports");
> > +
> > +	for_each_available_child_of_node(p, port) {
> > +		if (of_property_read_u32(port, "reg", &port_num))
> > +			continue;
> > +
> > +		fep->n_ports = port_num;
> > +		of_get_mac_address(port, &fep->mac[port_num -
> > 1][0]); +
> > +		ret = of_property_read_string(port, "label",
> > +
> > &fep->ndev_name[port_num - 1]);
> > +		if (ret < 0) {
> > +			pr_err("%s: Cannot get ethernet port name
> > (%d)!\n",
> > +			       __func__, ret);
> > +			goto of_get_err;  
> 
> Just use scoped loop.

I've used for_each_available_child_of_node(p, port) {} to iterate
through children.

Is it wrong?

> 
> > +		}
> > +
> > +		ret = of_get_phy_mode(port,
> > &fep->phy_interface[port_num - 1]);
> > +		if (ret < 0) {
> > +			pr_err("%s: Cannot get PHY mode (%d)!\n",
> > __func__,
> > +			       ret);  
> 
> No, drivers do not use pr_xxx. From where did you get this code?

There have been attempts to upstream this driver since 2020...
The original code is from - v4.4 for vf610 and 2.6.35 for imx287.

It has been then subsequently updated/rewritten for:

- 4.19-cip
- 5.12 (two versions for switchdev and DSA)
- 6.6 LTS
- net-next.

The pr_err() were probably added by me as replacement for
"printk(KERN_ERR". I can change them to dev_* or netdev_*. This shall
not be a problem.

> 
> > +			goto of_get_err;
> > +		}
> > +
> > +		fep->phy_np[port_num - 1] = of_parse_phandle(port,
> > +
> > "phy-handle", 0);
> > +	}
> > +
> > + of_get_err:
> > +	of_node_put(p);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtip_sw_learning(void *arg)
> > +{
> > +	struct switch_enet_private *fep = arg;
> > +
> > +	while (!kthread_should_stop()) {
> > +		set_current_state(TASK_INTERRUPTIBLE);
> > +		/* check learning record valid */
> > +		mtip_atable_dynamicms_learn_migration(fep,
> > fep->curr_time,
> > +						      NULL, NULL);
> > +		schedule_timeout(HZ / 100);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtip_mii_unregister(struct switch_enet_private *fep)
> > +{
> > +	mdiobus_unregister(fep->mii_bus);
> > +	mdiobus_free(fep->mii_bus);
> > +}
> > +
> > +static const struct fec_devinfo fec_imx28_l2switch_info = {
> > +	.quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO,
> > +};
> > +
> > +static struct platform_device_id pdev_id = {
> > +	.name = "imx28-l2switch",
> > +	.driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info,
> > +};
> > +
> > +static int __init mtip_sw_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct switch_enet_private *fep;
> > +	struct fec_devinfo *dev_info;
> > +	struct switch_t *fecp;
> > +	struct resource *r;
> > +	int err, ret;
> > +	u32 rev;
> > +
> > +	pr_info("Ethernet Switch Version %s\n", VERSION);  
> 
> Drivers use dev, not pr. Anyway drop. Drivers do not have versions and
> should be silent on success.

As I've written above - there are several "versions" on this particular
driver. Hence the information.

I would opt for dev_info() for backwards compatibility.

> 
> > +
> > +	fep = kzalloc(sizeof(*fep), GFP_KERNEL);  
> 
> Why not devm? See last comment here (at the end).

As I've written above - no problem to change it.

> 
> > +	if (!fep)
> > +		return -ENOMEM;
> > +
> > +	pdev->id_entry = &pdev_id;
> > +
> > +	dev_info = (struct fec_devinfo
> > *)pdev->id_entry->driver_data;
> > +	if (dev_info)
> > +		fep->quirks = dev_info->quirks;
> > +
> > +	fep->pdev = pdev;
> > +	platform_set_drvdata(pdev, fep);
> > +
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	fep->enet_addr = devm_ioremap_resource(&pdev->dev, r);  
> 
> Use proper wrapper.

I've followed following pattern:
https://elixir.bootlin.com/linux/v6.13.7/source/lib/devres.c#L180

> 
> > +	if (IS_ERR(fep->enet_addr)) {
> > +		ret = PTR_ERR(fep->enet_addr);
> > +		goto failed_ioremap;
> > +	}
> > +
> > +	fep->irq = platform_get_irq(pdev, 0);
> > +
> > +	ret = mtip_parse_of(fep, np);
> > +	if (ret < 0) {
> > +		pr_err("%s: OF parse error (%d)!\n", __func__,
> > ret);
> > +		goto failed_parse_of;
> > +	}
> > +
> > +	/* Create an Ethernet device instance.
> > +	 * The switch lookup address memory starts 0x800FC000
> > +	 */
> > +	fep->hwp_enet = fep->enet_addr;
> > +	fecp = (struct switch_t *)(fep->enet_addr +
> > ENET_SWI_PHYS_ADDR_OFFSET); +
> > +	fep->hwp = fecp;
> > +	fep->hwentry = (struct mtip_addr_table_t *)
> > +		((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET);
> > +
> > +	rev = readl(&fecp->ESW_REVISION);
> > +	pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> > +		MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> > +		MCF_MTIP_REVISION_CORE_REVISION(rev));  
> 
> Drop

Ok.

> 
> > +
> > +	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> > +	if (!IS_ERR(fep->reg_phy)) {
> > +		ret = regulator_enable(fep->reg_phy);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"Failed to enable phy regulator:
> > %d\n", ret);
> > +			goto failed_regulator;
> > +		}
> > +	} else {
> > +		if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> > +			ret = -EPROBE_DEFER;
> > +			goto failed_regulator;
> > +		}
> > +		fep->reg_phy = NULL;  
> 
> I don't understand this code. Do you want to re-implement
> get_optional? But why?

Here the get_optional() shall be used.

> 
> > +	}
> > +
> > +	fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	if (IS_ERR(fep->clk_ipg))
> > +		fep->clk_ipg = NULL;  
> 
> Why?

I will update the code.

> 
> > +
> > +	fep->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> > +	if (IS_ERR(fep->clk_ahb))
> > +		fep->clk_ahb = NULL;
> > +
> > +	fep->clk_enet_out = devm_clk_get(&pdev->dev, "enet_out");
> > +	if (IS_ERR(fep->clk_enet_out))
> > +		fep->clk_enet_out = NULL;
> > +

devm_clk_get_optional() shall be used for 'enet_out'.

> > +	ret = clk_prepare_enable(fep->clk_ipg);
> > +	if (ret) {
> > +		pr_err("%s: clock ipg cannot be enabled\n",
> > __func__);
> > +		goto failed_clk_ipg;
> > +	}
> > +
> > +	ret = clk_prepare_enable(fep->clk_ahb);
> > +	if (ret) {
> > +		pr_err("%s: clock ahb cannot be enabled\n",
> > __func__);
> > +		goto failed_clk_ahb;
> > +	}
> > +
> > +	ret = clk_prepare_enable(fep->clk_enet_out);
> > +	if (ret)
> > +		pr_err("%s: clock clk_enet_out cannot be
> > enabled\n", __func__); +
> > +	spin_lock_init(&fep->learn_lock);
> > +
> > +	ret = mtip_reset_phy(pdev);
> > +	if (ret < 0) {
> > +		pr_err("%s: GPIO PHY RST error (%d)!\n", __func__,
> > ret);
> > +		goto get_phy_mode_err;
> > +	}
> > +
> > +	ret = request_irq(fep->irq, mtip_interrupt, 0,
> > "mtip_l2sw", fep);
> > +	if (ret) {
> > +		pr_err("MTIP_L2: Could not alloc IRQ(%d)!\n",
> > fep->irq);
> > +		goto request_irq_err;
> > +	}
> > +
> > +	spin_lock_init(&fep->hw_lock);
> > +	spin_lock_init(&fep->mii_lock);
> > +
> > +	ret = mtip_register_notifiers(fep);
> > +	if (ret)
> > +		goto clean_unregister_netdev;
> > +
> > +	ret = mtip_ndev_init(fep);
> > +	if (ret) {
> > +		pr_err("%s: Failed to create virtual ndev (%d)\n",
> > +		       __func__, ret);
> > +		goto mtip_ndev_init_err;
> > +	}
> > +
> > +	err = mtip_switch_dma_init(fep);
> > +	if (err) {
> > +		pr_err("%s: ethernet switch init fail (%d)!\n",
> > __func__, err);
> > +		goto mtip_switch_dma_init_err;
> > +	}
> > +
> > +	err = mtip_mii_init(fep, pdev);
> > +	if (err)
> > +		pr_err("%s: Cannot init phy bus (%d)!\n",
> > __func__, err); +
> > +	/* setup timer for learning aging function */
> > +	timer_setup(&fep->timer_aging, mtip_aging_timer, 0);
> > +	mod_timer(&fep->timer_aging,
> > +		  jiffies +
> > msecs_to_jiffies(LEARNING_AGING_INTERVAL)); +
> > +	fep->task = kthread_run(mtip_sw_learning, fep,
> > "mtip_l2sw_learning");
> > +	if (IS_ERR(fep->task)) {
> > +		ret = PTR_ERR(fep->task);
> > +		pr_err("%s: learning kthread_run error (%d)!\n",
> > __func__, ret);
> > +		goto fail_task_learning;
> > +	}
> > +
> > +	/* setup MII interface for external switch ports*/
> > +	mtip_enet_init(fep, 1);
> > +	mtip_enet_init(fep, 2);
> > +
> > +	return 0;
> > +
> > + fail_task_learning:
> > +	mtip_mii_unregister(fep);
> > +	del_timer(&fep->timer_aging);
> > + mtip_switch_dma_init_err:
> > +	mtip_ndev_cleanup(fep);
> > + mtip_ndev_init_err:
> > +	mtip_unregister_notifiers(fep);
> > + clean_unregister_netdev:
> > +	free_irq(fep->irq, NULL);
> > + request_irq_err:
> > +	platform_set_drvdata(pdev, NULL);
> > + get_phy_mode_err:
> > +	if (fep->clk_enet_out)
> > +		clk_disable_unprepare(fep->clk_enet_out);
> > +	clk_disable_unprepare(fep->clk_ahb);
> > + failed_clk_ahb:
> > +	clk_disable_unprepare(fep->clk_ipg);
> > + failed_clk_ipg:
> > +	if (fep->reg_phy) {
> > +		regulator_disable(fep->reg_phy);
> > +		devm_regulator_put(fep->reg_phy);
> > +	}
> > + failed_regulator:
> > + failed_parse_of:
> > +	devm_ioremap_release(&pdev->dev, r);
> > + failed_ioremap:
> > +	kfree(fep);
> > +	return err;
> > +}
> > +
> > +static void mtip_sw_remove(struct platform_device *pdev)
> > +{
> > +	struct switch_enet_private *fep =
> > platform_get_drvdata(pdev); +
> > +	mtip_unregister_notifiers(fep);
> > +	mtip_ndev_cleanup(fep);
> > +
> > +	mtip_mii_remove(fep);
> > +
> > +	kthread_stop(fep->task);
> > +	del_timer(&fep->timer_aging);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	kfree(fep);
> > +}
> > +
> > +static const struct of_device_id mtipl2_of_match[] = {
> > +	{ .compatible = "fsl,imx287-mtip-switch", },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver mtipl2plat_driver = {
> > +	.probe          = mtip_sw_probe,
> > +	.remove         = mtip_sw_remove,
> > +	.driver         = {
> > +		.name   = "mtipl2sw",
> > +		.owner  = THIS_MODULE,  
> 
> Oh no, please do not send us 10-12 year old code. This is long, looong
> time gone. If you copied this pattern,

I've stated the chronology of this particular driver. It is working
with recent kernels.

> then for sure you copied all
> other issues we fixed during last 10 years, so basically you ask us to
> re-review the same code we already fixed.

I cannot agree with this statement.

Even better, the code has passed net-next's checkpatch.pl without
complaining about the "THIS_MODULE" statement.

I will remove it.

> 
> 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: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 13:39, Lukasz Majewski wrote:
> > Hi Krzysztof,
> >   
> >> On 25/03/2025 13:15, Lukasz Majewski wrote:  
> >>> Hi Krzysztof,
> >>>     
> >>>> On 25/03/2025 12:57, Lukasz Majewski wrote:    
> >>>>> This patch provides description of the MTIP L2 switch available
> >>>>> in some NXP's SOCs - imx287, vf610.
> >>>>>
> >>>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >>>>> ---
> >>>>>  .../bindings/net/fec,mtip-switch.yaml         | 160
> >>>>> ++++++++++++++++++      
> >>>>
> >>>> Use compatible as filename.    
> >>>
> >>> I've followed the fsl,fec.yaml as an example. This file has
> >>> description for all the device tree sources from fec_main.c    
> >>
> >>
> >> That's a 14 year old binding, so clear antipattern.  
> > 
> > For some reason it is still there...  
> 
> And it will be there for very long time. Bindings are not removed just
> because they are old, because they are an ABI. That's still not a
> reason to use something old as starting point.

It was unintentional - if I would know that fsl,fec.yaml is so old, I
would use another one as a starting point.

I will use more recent one to provide proper bindings for this driver.

> 
> It's the same with drivers, although driver can be easier changed and
> old pattern can be dropped. You cannot easily drop old, anti-patterns
> from the binding.

Yes, I'm fully aware of the problem.

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

[Krzysztof Kozlowski]

On 25/03/2025 14:28, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 25/03/2025 12:57, 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 interchangeable 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.
>>>
>>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>>> ---
>>>  drivers/net/ethernet/freescale/Kconfig        |    1 +
>>>  drivers/net/ethernet/freescale/Makefile       |    1 +
>>>  drivers/net/ethernet/freescale/mtipsw/Kconfig |   10 +
>>>  .../net/ethernet/freescale/mtipsw/Makefile    |    6 +
>>>  .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2108
>>> +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |
>>> 784 ++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
>>>  .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  434 ++++
>>>  8 files changed, 3457 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/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..5cc9b758bad4
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
>>> @@ -0,0 +1,10 @@
>>> +# 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  
>>
>> Missing compile test
> 
> Could you be more specific?
> 
> I'm compiling and testing this driver for the last week... (6.6 LTS +
> net-next).

You miss dependency on compile test.

> 
>>
>>> +	help
>>> +		This enables support for the MoreThan IP L2 switch
>>> on i.MX
>>> +		SoCs (e.g. iMX28, vf610).  
>>
>> Wrong indentation. Look at other Kconfig files.
> 
> The indentation from Kconfig from FEC:
> 
> <tab>help
> <tab><2 spaces>FOO BAR....

That's correct, but you do not use it :/

> 
> also causes the checkpatch on net-next to generated WARNING.
> 
>>
>>> diff --git a/drivers/net/ethernet/freescale/mtipsw/Makefile
>>> b/drivers/net/ethernet/freescale/mtipsw/Makefile new file mode
>>> 100644 index 000000000000..e87e06d6870a
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/freescale/mtipsw/Makefile
>>> @@ -0,0 +1,6 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Makefile for the L2 switch from MTIP embedded in NXP SoCs  
>>
>> Drop, obvious.
> 
> Ok.
> 
>>
>>
>> ...
>>
>>> +
>>> +static int mtip_parse_of(struct switch_enet_private *fep,
>>> +			 struct device_node *np)
>>> +{
>>> +	struct device_node *port, *p;
>>> +	unsigned int port_num;
>>> +	int ret;
>>> +
>>> +	p = of_find_node_by_name(np, "ethernet-ports");
>>> +
>>> +	for_each_available_child_of_node(p, port) {
>>> +		if (of_property_read_u32(port, "reg", &port_num))
>>> +			continue;
>>> +
>>> +		fep->n_ports = port_num;
>>> +		of_get_mac_address(port, &fep->mac[port_num -
>>> 1][0]); +
>>> +		ret = of_property_read_string(port, "label",
>>> +
>>> &fep->ndev_name[port_num - 1]);
>>> +		if (ret < 0) {
>>> +			pr_err("%s: Cannot get ethernet port name
>>> (%d)!\n",
>>> +			       __func__, ret);
>>> +			goto of_get_err;  
>>
>> Just use scoped loop.
> 
> I've used for_each_available_child_of_node(p, port) {} to iterate
> through children.
> 
> Is it wrong?

No, it is not, just can be simpler with scoped variant.

...

>>
>>> +	if (!fep)
>>> +		return -ENOMEM;
>>> +
>>> +	pdev->id_entry = &pdev_id;
>>> +
>>> +	dev_info = (struct fec_devinfo
>>> *)pdev->id_entry->driver_data;
>>> +	if (dev_info)
>>> +		fep->quirks = dev_info->quirks;
>>> +
>>> +	fep->pdev = pdev;
>>> +	platform_set_drvdata(pdev, fep);
>>> +
>>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	fep->enet_addr = devm_ioremap_resource(&pdev->dev, r);  
>>
>> Use proper wrapper.
> 
> I've followed following pattern:
> https://elixir.bootlin.com/linux/v6.13.7/source/lib/devres.c#L180

which is correct example of that usage, but most drivers were converted
to different usage. Probably cocci also has rule for that.

>>> +
>>> +static const struct of_device_id mtipl2_of_match[] = {
>>> +	{ .compatible = "fsl,imx287-mtip-switch", },
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static struct platform_driver mtipl2plat_driver = {
>>> +	.probe          = mtip_sw_probe,
>>> +	.remove         = mtip_sw_remove,
>>> +	.driver         = {
>>> +		.name   = "mtipl2sw",
>>> +		.owner  = THIS_MODULE,  
>>
>> Oh no, please do not send us 10-12 year old code. This is long, looong
>> time gone. If you copied this pattern,
> 
> I've stated the chronology of this particular driver. It is working
> with recent kernels.

That's not how upstreaming should work with explanation below.
> 
>> then for sure you copied all
>> other issues we fixed during last 10 years, so basically you ask us to
>> re-review the same code we already fixed.

^^^ this one. It does not matter if it works. You can send us working
code with hungarian notation and the argument "it works" is not correct.
It still will be hungarian notation.

> 
> I cannot agree with this statement.
> 
> Even better, the code has passed net-next's checkpatch.pl without
> complaining about the "THIS_MODULE" statement.
> 
It's not part of checkpatch. It's part of coccinelle, which - just like
smatch and sparse - are supposed to report zero or almost zero issues on
new drivers.

Best regards,
Krzysztof

Re: [PATCH 0/5] net: mtip: Add support for MTIP imx287 L2 switch driver

[Rob Herring (Arm)]

On Tue, 25 Mar 2025 12:57:31 +0100, Lukasz Majewski wrote:
> 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):
>   MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs:
>     imx287)
>   dt-bindings: net: Add MTIP L2 switch description
>     (fec,mtip-switch.yaml)
>   arm: dts: Adjust the 'reg' range for imx287 L2 switch description
>   arm: dts: imx287: Provide description for MTIP L2 switch
>   net: mtip: The L2 switch driver for imx287
> 
>  .../bindings/net/fec,mtip-switch.yaml         |  160 ++
>  MAINTAINERS                                   |    7 +
>  arch/arm/boot/dts/nxp/mxs/imx28-xea.dts       |   56 +
>  arch/arm/boot/dts/nxp/mxs/imx28.dtsi          |    4 +-
>  drivers/net/ethernet/freescale/Kconfig        |    1 +
>  drivers/net/ethernet/freescale/Makefile       |    1 +
>  drivers/net/ethernet/freescale/mtipsw/Kconfig |   10 +
>  .../net/ethernet/freescale/mtipsw/Makefile    |    6 +
>  .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2108 +++++++++++++++++
>  .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  784 ++++++
>  .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
>  .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  434 ++++
>  12 files changed, 3682 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/fec,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
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y for arch/arm/boot/dts/nxp/' for 20250325115736.1732721-1-lukma@denx.de:

arch/arm/boot/dts/nxp/mxs/imx28-xea.dtb: /ahb@80080000/switch@800f0000: failed to match any schema with compatible: ['fsl,imx287-mtip-switch']
arch/arm/boot/dts/nxp/imx/imx6q-tx6q-1110.dtb: /lvds1-panel: failed to match any schema with compatible: ['nlt,nl12880bc20-spwg-24']

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Andrew Lunn]

> +  phy-reset-gpios:
> +    deprecated: true
> +    description:
> +      Should specify the gpio for phy reset.

It seem odd that a new binding has deprecated properties. Maybe add a
comment in the commit message as to why they are there. I assume this
is because you are re-using part of the FEC code as is, and it
implements them?

	   Andrew

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski]

On 25/03/2025 16:05, Andrew Lunn wrote:
>> +  phy-reset-gpios:
>> +    deprecated: true
>> +    description:
>> +      Should specify the gpio for phy reset.
> 
> It seem odd that a new binding has deprecated properties. Maybe add a
> comment in the commit message as to why they are there. I assume this
> is because you are re-using part of the FEC code as is, and it
> implements them?

Re-using existing driver code should not be a reason to use deprecated
properties, because you can change that common driver parts to support
both: old FEC style and new MTIP-L2-FEC switch.

Best regards,
Krzysztof

Re: [PATCH 4/5] arm: dts: imx287: Provide description for MTIP L2 switch

[Andrew Lunn]

On Tue, Mar 25, 2025 at 12:57:35PM +0100, Lukasz Majewski wrote:
> This description is similar to one supprted with the cpsw_new.c
> driver.
> 
> It has separated ports and PHYs (connected to mdio bus).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56 +++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
> index 6c5e6856648a..e645b086574d 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"
>  
>  / {
> @@ -18,6 +19,61 @@ &can0 {
>  	status = "okay";
>  };
>  
> +&eth_switch {
> +	compatible = "fsl,imx287-mtip-switch";

The switch is part of the SoC. So i would expect the compatible to be
in the .dtsi file for the SoC.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> +	phy-supply = <&reg_fec_3v3>;
> +	phy-reset-duration = <25>;
> +	phy-reset-post-delay = <10>;
> +	interrupts = <100>, <101>, <102>;
> +	clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> +	clock-names = "ipg", "ahb", "enet_out", "ptp";

Which of these properties are SoC properties? I _guess_ interrupts,
clocks and clock-names. So they should be in the SoC .dtsi file. You
should only add board properties here.

       Andrew

Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

[Andrew Lunn]

> > > +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  
> > 
> > Missing compile test
> 
> Could you be more specific?

config FEC
        tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
        depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \
                   ARCH_MXC || ARCH_S32 || SOC_IMX28 || COMPILE_TEST)

|| COMPILE_TEST

So that it also gets build on amd64, s390, powerpc etc. The code
should cleanly build on all architectures. It if does not, it probably
means the code is using the kernel abstractions wrong. Also, most
developers build test on amd64, not arm, so if they are making tree
wide changes, you want this driver to build on amd64 so such tree wide
changes get build tested.

> There have been attempts to upstream this driver since 2020...
> The original code is from - v4.4 for vf610 and 2.6.35 for imx287.
> 
> It has been then subsequently updated/rewritten for:
> 
> - 4.19-cip
> - 5.12 (two versions for switchdev and DSA)
> - 6.6 LTS
> - net-next.
> 
> The pr_err() were probably added by me as replacement for
> "printk(KERN_ERR". I can change them to dev_* or netdev_*. This shall
> not be a problem.

With Ethernet switches, you need to look at the context. Is it
something specific to one interface? The netdev_err(). If it is global
to the whole switch, then dev_err().

> > > +	pr_info("Ethernet Switch Version %s\n", VERSION);  
> > 
> > Drivers use dev, not pr. Anyway drop. Drivers do not have versions and
> > should be silent on success.
> 
> As I've written above - there are several "versions" on this particular
> driver. Hence the information.

There is only one version in mainline, this version (maybe). Mainline
does not care about other versions. Such version information is also
useless, because once it is merged, it never changes. What you
actually want to know is the kernel git hash, so you can find the
exact sources. ethtool -I will return that, assuming your ethtool code
is correct.

> > > +	pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> > > +		MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> > > +		MCF_MTIP_REVISION_CORE_REVISION(rev));  
> > 
> > Drop
> 
> Ok.

You can report this via ethtool -I. But i suggest you leave that for
later patches.

> > > +	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> > > +	if (!IS_ERR(fep->reg_phy)) {
> > > +		ret = regulator_enable(fep->reg_phy);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev,
> > > +				"Failed to enable phy regulator:
> > > %d\n", ret);
> > > +			goto failed_regulator;
> > > +		}
> > > +	} else {
> > > +		if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> > > +			ret = -EPROBE_DEFER;
> > > +			goto failed_regulator;
> > > +		}
> > > +		fep->reg_phy = NULL;  
> > 
> > I don't understand this code. Do you want to re-implement
> > get_optional? But why?
> 
> Here the get_optional() shall be used.

This is the problem with trying to use old code. It needs more work
than just making it compile. It needs to be brought up to HEAD of
mainline standard, which often nearly ends in a re-write.

> > > +	fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > +	if (IS_ERR(fep->clk_ipg))
> > > +		fep->clk_ipg = NULL;  
> > 
> > Why?
> 
> I will update the code.

FYI: NULL is a valid clock. But do you actually want _optional()?

This is the sort of thing a 10 year old codebase will be missing, and
you need to re-write. You might also be able to use the clk _bulk_
API?

	Andrew

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Hi Andrew,

> > +  phy-reset-gpios:
> > +    deprecated: true
> > +    description:
> > +      Should specify the gpio for phy reset.  
> 
> It seem odd that a new binding has deprecated properties. Maybe add a
> comment in the commit message as to why they are there. I assume this
> is because you are re-using part of the FEC code as is, and it
> implements them?

+1

> 
> 	   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: [PATCH 4/5] arm: dts: imx287: Provide description for MTIP L2 switch

[Lukasz Majewski]

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

Hi Andrew,

> On Tue, Mar 25, 2025 at 12:57:35PM +0100, Lukasz Majewski wrote:
> > This description is similar to one supprted with the cpsw_new.c
> > driver.
> > 
> > It has separated ports and PHYs (connected to mdio bus).
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56
> > +++++++++++++++++++++++++ 1 file changed, 56 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
> > b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts index
> > 6c5e6856648a..e645b086574d 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"
> >  
> >  / {
> > @@ -18,6 +19,61 @@ &can0 {
> >  	status = "okay";
> >  };
> >  
> > +&eth_switch {
> > +	compatible = "fsl,imx287-mtip-switch";  
> 
> The switch is part of the SoC. So i would expect the compatible to be
> in the .dtsi file for the SoC.

Ok.

I'm also wondering if I shall use "fsl," or "nxp," prefix. The former
one is the same as in fec_main.c, but as I do add new driver, the
prefix could be updated.

> 
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> > +	phy-supply = <&reg_fec_3v3>;
> > +	phy-reset-duration = <25>;
> > +	phy-reset-post-delay = <10>;
> > +	interrupts = <100>, <101>, <102>;
> > +	clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> > +	clock-names = "ipg", "ahb", "enet_out", "ptp";  
> 
> Which of these properties are SoC properties? I _guess_ interrupts,
> clocks and clock-names. So they should be in the SoC .dtsi file. You
> should only add board properties here.

Ok. I will add them.

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

[Lukasz Majewski]

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

Hi Andrew,

> > > > +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    
> > > 
> > > Missing compile test  
> > 
> > Could you be more specific?  
> 
> config FEC
>         tristate "FEC ethernet controller (of ColdFire and some i.MX
> CPUs)" depends on (M523x || M527x || M5272 || M528x || M520x || M532x
> || \ ARCH_MXC || ARCH_S32 || SOC_IMX28 || COMPILE_TEST)
> 
> || COMPILE_TEST

^^^^^^^^^^^^^^^^^^ - ach ... this "compile test" :-)

(Not that I've posted the code not even compiling ... :-D)

> 
> So that it also gets build on amd64, s390, powerpc etc. The code
> should cleanly build on all architectures. It if does not, it probably
> means the code is using the kernel abstractions wrong. Also, most
> developers build test on amd64, not arm, so if they are making tree
> wide changes, you want this driver to build on amd64 so such tree wide
> changes get build tested.
> 
> > There have been attempts to upstream this driver since 2020...
> > The original code is from - v4.4 for vf610 and 2.6.35 for imx287.
> > 
> > It has been then subsequently updated/rewritten for:
> > 
> > - 4.19-cip
> > - 5.12 (two versions for switchdev and DSA)
> > - 6.6 LTS
> > - net-next.
> > 
> > The pr_err() were probably added by me as replacement for
> > "printk(KERN_ERR". I can change them to dev_* or netdev_*. This
> > shall not be a problem.  
> 
> With Ethernet switches, you need to look at the context. Is it
> something specific to one interface? The netdev_err(). If it is global
> to the whole switch, then dev_err().

Ok.

> 
> > > > +	pr_info("Ethernet Switch Version %s\n", VERSION);    
> > > 
> > > Drivers use dev, not pr. Anyway drop. Drivers do not have
> > > versions and should be silent on success.  
> > 
> > As I've written above - there are several "versions" on this
> > particular driver. Hence the information.  
> 
> There is only one version in mainline, this version (maybe). Mainline
> does not care about other versions. Such version information is also
> useless, because once it is merged, it never changes. What you
> actually want to know is the kernel git hash, so you can find the
> exact sources. ethtool -I will return that, assuming your ethtool code
> is correct.

Ok. I will.

> 
> > > > +	pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> > > > +		MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> > > > +		MCF_MTIP_REVISION_CORE_REVISION(rev));    
> > > 
> > > Drop  
> > 
> > Ok.  
> 
> You can report this via ethtool -I. But i suggest you leave that for
> later patches.

I will remove it.

> 
> > > > +	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> > > > +	if (!IS_ERR(fep->reg_phy)) {
> > > > +		ret = regulator_enable(fep->reg_phy);
> > > > +		if (ret) {
> > > > +			dev_err(&pdev->dev,
> > > > +				"Failed to enable phy
> > > > regulator: %d\n", ret);
> > > > +			goto failed_regulator;
> > > > +		}
> > > > +	} else {
> > > > +		if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> > > > +			ret = -EPROBE_DEFER;
> > > > +			goto failed_regulator;
> > > > +		}
> > > > +		fep->reg_phy = NULL;    
> > > 
> > > I don't understand this code. Do you want to re-implement
> > > get_optional? But why?  
> > 
> > Here the get_optional() shall be used.  
> 
> This is the problem with trying to use old code. It needs more work
> than just making it compile. It needs to be brought up to HEAD of
> mainline standard, which often nearly ends in a re-write.

But you cannot rewrite this code from scratch, as the IP block is not
so well documented, and there maybe are some issues that you are not
aware of.

Moreover, this code is already in production use, and you don't want to
be in situation when regression tests cannot be run.

> 
> > > > +	fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > > +	if (IS_ERR(fep->clk_ipg))
> > > > +		fep->clk_ipg = NULL;    
> > > 
> > > Why?  
> > 
> > I will update the code.  
> 
> FYI: NULL is a valid clock. But do you actually want _optional()?

Yes, I will use _optional() and _bulk_ if applicable.

> 
> This is the sort of thing a 10 year old codebase will be missing, and
> you need to re-write. You might also be able to use the clk _bulk_
> API?

The goal is to have this driver upstreamed (finally), so the starting
point of further development would be in kernel.

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

[Krzysztof Kozlowski]

On 25/03/2025 17:38, Lukasz Majewski wrote:
>>>>
>>>> I don't understand this code. Do you want to re-implement
>>>> get_optional? But why?  
>>>
>>> Here the get_optional() shall be used.  
>>
>> This is the problem with trying to use old code. It needs more work
>> than just making it compile. It needs to be brought up to HEAD of
>> mainline standard, which often nearly ends in a re-write.
> 
> But you cannot rewrite this code from scratch, as the IP block is not
> so well documented, and there maybe are some issues that you are not
> aware of.
> 
> Moreover, this code is already in production use, and you don't want to
> be in situation when regression tests cannot be run.

This is a good reason to add it to staging, but not to mainline. Just
because someone has somewhere products with poor code is not the reason
to accept that poor code. Otherwise all the people and companies who
upstream BEFORE would be quite disappointed. Why anyone would care to
work on upstreaming BEFORE hardware release, if you can ship whatever to
production and then ask mainline to pick up "because it is in production
use".

Best regards,
Krzysztof

Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

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

Hi Krzysztof,

> On 25/03/2025 17:38, Lukasz Majewski wrote:
> >>>>
> >>>> I don't understand this code. Do you want to re-implement
> >>>> get_optional? But why?    
> >>>
> >>> Here the get_optional() shall be used.    
> >>
> >> This is the problem with trying to use old code. It needs more work
> >> than just making it compile. It needs to be brought up to HEAD of
> >> mainline standard, which often nearly ends in a re-write.  
> > 
> > But you cannot rewrite this code from scratch, as the IP block is
> > not so well documented, and there maybe are some issues that you
> > are not aware of.
> > 
> > Moreover, this code is already in production use, and you don't
> > want to be in situation when regression tests cannot be run.  
> 
> This is a good reason to add it to staging, but not to mainline. Just
> because someone has somewhere products with poor code is not the
> reason to accept that poor code.

I've tried to upstream this driver several times. Attempts were
made for 4.19 and 5.12. The reason the code was not accepted was that
conceptually the code had to be written in a different way (exact
discussion is available [1]).

What I've tried to say above - was that I need to have working device
at any point of development.

And, yes "upstream first" is a great policy, but imx287 based HW was in
the kernel long time ago.

> Otherwise all the people and
> companies who upstream BEFORE would be quite disappointed. Why anyone
> would care to work on upstreaming BEFORE hardware release, 

Yes, this shall be appreciated.

> if you can
> ship whatever to production and then ask mainline to pick up "because
> it is in production use".

Where I've stated this?

My point is that for regression testing I prefer to gradually update
the code and not start from scratch.

I do appreciate your and Andrew's feedback and try to make the driver
eligible for upstreaming.

To sum up:
##########

- Yes, I'm aware that this code needs some more adjustments/update

- Yes, fsl,fec.yaml was the wrong file to use as a starting point

- Yes, bindings are ABI and shall be done right (that was one of the
  reason the driver from 5.12 was not accepted).


Links:
[1] - https://lore.kernel.org/netdev/20210629140104.70a3da1a@ktm/T/

> 
> 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: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Lukasz Majewski]

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

Hi Andrew,

> > +  phy-reset-gpios:
> > +    deprecated: true
> > +    description:
> > +      Should specify the gpio for phy reset.  
> 
> It seem odd that a new binding has deprecated properties. Maybe add a
> comment in the commit message as to why they are there. I assume this
> is because you are re-using part of the FEC code as is, and it
> implements them?
> 

In the case of MTIP L2 switch, the reset gpio line (in my case, but
also on e.g. imx28-evk, and vf610) is single for both PHYs.

I could move the reset to mdio child nodes, but this would be
problematic, as asserting reset on one PHY would reset the second one.

That is why there is a single 'phy-reset-gpios' property for the switch
driver.

I do believe that for FEC it may be deprecated, but for the HW
configurations I'm aware of it fits best. 

> 	   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: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Krzysztof Kozlowski]

On 26/03/2025 14:43, Lukasz Majewski wrote:
> Hi Andrew,
> 
>>> +  phy-reset-gpios:
>>> +    deprecated: true
>>> +    description:
>>> +      Should specify the gpio for phy reset.  
>>
>> It seem odd that a new binding has deprecated properties. Maybe add a
>> comment in the commit message as to why they are there. I assume this
>> is because you are re-using part of the FEC code as is, and it
>> implements them?
>>
> 
> In the case of MTIP L2 switch, the reset gpio line (in my case, but
> also on e.g. imx28-evk, and vf610) is single for both PHYs.

That's kind of proof that property was not placed in correct place.

> 
> I could move the reset to mdio child nodes, but this would be
> problematic, as asserting reset on one PHY would reset the second one.

It wouldn't if you used reset controller framework for that GPIO. Please
move the GPIOs to the device actually having the line, so the GPIOs.

Since a year such workarounds are not allowed in kernel anymore.

Best regards,
Krzysztof

Re: [PATCH 2/5] dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml)

[Andrew Lunn]

On Wed, Mar 26, 2025 at 02:43:16PM +0100, Lukasz Majewski wrote:
> Hi Andrew,
> 
> > > +  phy-reset-gpios:
> > > +    deprecated: true
> > > +    description:
> > > +      Should specify the gpio for phy reset.  
> > 
> > It seem odd that a new binding has deprecated properties. Maybe add a
> > comment in the commit message as to why they are there. I assume this
> > is because you are re-using part of the FEC code as is, and it
> > implements them?
> > 
> 
> In the case of MTIP L2 switch, the reset gpio line (in my case, but
> also on e.g. imx28-evk, and vf610) is single for both PHYs.

So this is known as an MDIO bus reset, not a PHY reset.

Documentation/devicetree/bindings/net/mdio.yaml

  reset-gpios:
    maxItems: 1
    description:
      The phandle and specifier for the GPIO that controls the RESET
      lines of all devices on that MDIO bus.

We have moved the handling of such reset lines into core code, which
is why per driver properties are deprecated, and you should use the
properties defined in the core.

	Andrew