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

[PATCH v2 0/4] 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 (4):
  dt-bindings: net: Add MTIP L2 switch description
  ARM: dts: nxp: mxs: Adjust the imx28.dtsi L2 switch description
  ARM: dts: nxp: mxs: Adjust XEA board's DTS to support L2 switch
  net: mtip: The L2 switch driver for imx287

 .../bindings/net/nxp,imx287-mtip-switch.yaml  |  165 ++
 MAINTAINERS                                   |    7 +
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts       |   54 +
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi          |    8 +-
 drivers/net/ethernet/freescale/Kconfig        |    1 +
 drivers/net/ethernet/freescale/Makefile       |    1 +
 drivers/net/ethernet/freescale/mtipsw/Kconfig |   13 +
 .../net/ethernet/freescale/mtipsw/Makefile    |    3 +
 .../net/ethernet/freescale/mtipsw/mtipl2sw.c  | 2035 +++++++++++++++++
 .../net/ethernet/freescale/mtipsw/mtipl2sw.h  |  781 +++++++
 .../ethernet/freescale/mtipsw/mtipl2sw_br.c   |  113 +
 .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c |  449 ++++
 12 files changed, 3628 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Rename the file to match exactly the compatible
  (nxp,imx287-mtip-switch)
---
 .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165 ++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml

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

[Lukasz Majewski]

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

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

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---
Changes for v2:
- adding extra properties (like compatible, clocks, interupts)
---
 arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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

[PATCH v2 3/4] ARM: dts: nxp: mxs: Adjust XEA board's DTS to support L2 switch

[Lukasz Majewski]

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

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Remove properties which are common for the imx28(7) SoC
- Use mdio properties to perform L2 switch reset (avoid using
  deprecated properties)
---
 arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 54 +++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
index 6c5e6856648a..d5558e24844c 100644
--- a/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
+++ b/arch/arm/boot/dts/nxp/mxs/imx28-xea.dts
@@ -5,6 +5,7 @@
  */
 
 /dts-v1/;
+#include<dt-bindings/interrupt-controller/irq.h>
 #include "imx28-lwe.dtsi"
 
 / {
@@ -90,6 +91,59 @@ &reg_usb_5v {
 	gpio = <&gpio0 2 0>;
 };
 
+&eth_switch {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
+	phy-supply = <&reg_fec_3v3>;
+	status = "okay";
+
+	ethernet-ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mtip_port1: 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_sw: mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reset-gpios = <&gpio3 21 0>;
+		reset-delay-us = <25000>;
+		reset-post-delay-us = <10000>;
+
+		ethphy0: ethernet-phy@0 {
+			reg = <0>;
+			smsc,disable-energy-detect;
+			/* Both PHYs (i.e. 0,1) have the same, single GPIO, */
+			/* line to handle both, their interrupts (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>;
+		};
+	};
+};
+
 &spi2_pins_a {
 	fsl,pinmux-ids = <
 		MX28_PAD_SSP2_SCK__SSP2_SCK
-- 
2.39.5

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

[Jakub Kicinski]

On Fri, 28 Mar 2025 14:35:40 +0100 Lukasz Majewski wrote:
> This patch series adds support for More Than IP's L2 switch driver embedded
> in some NXP's SoCs.

## Form letter - net-next-closed

Linus already pulled net-next material v6.15 and therefore net-next is closed
for new drivers, features, code refactoring and optimizations. We are currently
accepting bug fixes only.

Please repost when net-next reopens after Apr 7th.

RFC patches sent for review only are obviously welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: defer
pv-bot: closed

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

[Lukasz Majewski]

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

Hi Jakub,

> On Fri, 28 Mar 2025 14:35:40 +0100 Lukasz Majewski wrote:
> > This patch series adds support for More Than IP's L2 switch driver
> > embedded in some NXP's SoCs.  
> 
> ## Form letter - net-next-closed
> 
> Linus already pulled net-next material v6.15 and therefore net-next
> is closed for new drivers, features, code refactoring and
> optimizations. We are currently accepting bug fixes only.
> 
> Please repost when net-next reopens after Apr 7th.
> 

:-/

> RFC patches sent for review only are obviously welcome at any time.
> 

I hope, that I will receive some feedback regarding this driver, so I
can repost (hopefully) final version at 07.04.2025.

> See:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle




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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Krzysztof Kozlowski]

On 28/03/2025 14:35, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - e.g. imx287.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)

Please implement all the changes, not only the rename. I gave several
comments, although quick glance suggests you did implement them, so then
changelog is just incomplete.

> ---
>  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> new file mode 100644
> index 000000000000..a3e0fe7783ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,imx287-mtip-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +
> +description:
> +  The 2-port switch ethernet subsystem provides ethernet packet (L2)
> +  communication and can be configured as an ethernet switch. It provides the
> +  reduced media independent interface (RMII), the management data input
> +  output (MDIO) for physical layer device (PHY) management.
> +

If this is ethernet switch, why it does not reference ethernet-switch
schema? or dsa.yaml or dsa/ethernet-ports? I am not sure which one
should go here, but surprising to see none.

> +properties:
> +  compatible:
> +    const: nxp,imx287-mtip--switch

Just one -.

> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      The physical base address and size of the MTIP L2 SW module IO range

Wasn't here, drop.

> +
> +  phy-supply:
> +    description:
> +      Regulator that powers Ethernet PHYs.
> +
> +  clocks:
> +    items:
> +      - description: Register accessing clock
> +      - description: Bus access clock
> +      - description: Output clock for external device - e.g. PHY source clock
> +      - description: IEEE1588 timer clock
> +
> +  clock-names:
> +    items:
> +      - const: ipg
> +      - const: ahb
> +      - const: enet_out
> +      - const: ptp
> +
> +  interrupts:
> +    items:
> +      - description: Switch interrupt
> +      - description: ENET0 interrupt
> +      - description: ENET1 interrupt
> +
> +  pinctrl-names: true

Drop

> +
> +  ethernet-ports:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      "^port@[0-9]+$":

Keep consistent quotes, either " or '. Also [01]


> +        type: object
> +        description: MTIP L2 switch external ports
> +
> +        $ref: ethernet-controller.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          reg:
> +            items:
> +              - enum: [1, 2]
> +            description: MTIP L2 switch port number
> +
> +          label:
> +            description: Label associated with this port
> +
> +        required:
> +          - reg
> +          - label
> +          - phy-mode
> +          - phy-handle
> +
> +  mdio:
> +    type: object
> +    $ref: mdio.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      Specifies the mdio bus in the switch, used as a container for phy nodes.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - mdio
> +  - ethernet-ports
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include<dt-bindings/interrupt-controller/irq.h>
> +    switch@800f0000 {
> +        compatible = "nxp,imx287-mtip-switch";
> +        reg = <0x800f0000 0x20000>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> +        phy-supply = <&reg_fec_3v3>;
> +        interrupts = <100>, <101>, <102>;
> +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> +        status = "okay";

Drop

> +
> +        ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;

Messed indentation. See example-schema or writing-schema.



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 28/03/2025 14:35, Lukasz Majewski wrote:
> +
> +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 = {

That's const.

> +	.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;
> +	int ret;
> +
> +	fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL);
> +	if (!fep)
> +		return -ENOMEM;
> +
> +	pdev->id_entry = &pdev_id;

Hm? This is some odd pattern. You are supposed to use OF table and get
matched by it, not populate some custom/odd handling of platform tables.

> +
> +	dev_info = (struct fec_devinfo *)pdev->id_entry->driver_data;

I did not notice it before, but that's a no - you cannot drop the cast.
Driver data is always const.

> +	if (dev_info)
> +		fep->quirks = dev_info->quirks;
> +
> +	fep->pdev = pdev;
> +	platform_set_drvdata(pdev, fep);
> +
> +	fep->enet_addr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(fep->enet_addr))
> +		return PTR_ERR(fep->enet_addr);
> +
> +	fep->irq = platform_get_irq(pdev, 0);
> +	if (fep->irq < 0)
> +		return fep->irq;
> +
> +	ret = mtip_parse_of(fep, np);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "%s: OF parse error (%d)!\n", __func__,
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Create an Ethernet device instance.
> +	 * The switch lookup address memory starts at 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);
> +
> +	ret = devm_regulator_get_enable_optional(&pdev->dev, "phy");
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Unable to get and enable 'phy'\n");
> +
> +	fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> +	if (IS_ERR(fep->clk_ipg))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ipg),
> +				     "Unable to acquire 'ipg' clock\n");
> +
> +	fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> +	if (IS_ERR(fep->clk_ahb))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ahb),
> +				     "Unable to acquire 'ahb' clock\n");
> +
> +	fep->clk_enet_out = devm_clk_get_optional_enabled(&pdev->dev,
> +							  "enet_out");
> +	if (IS_ERR(fep->clk_enet_out))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_enet_out),
> +				     "Unable to acquire 'enet_out' clock\n");
> +
> +	spin_lock_init(&fep->learn_lock);
> +	spin_lock_init(&fep->hw_lock);
> +	spin_lock_init(&fep->mii_lock);
> +
> +	ret = devm_request_irq(&pdev->dev, fep->irq, mtip_interrupt, 0,
> +			       "mtip_l2sw", fep);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, fep->irq,
> +				     "Could not alloc IRQ\n");
> +
> +	ret = mtip_register_notifiers(fep);
> +	if (ret)
> +		return ret;
> +
> +	ret = mtip_ndev_init(fep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n",
> +			__func__, ret);
> +		goto ndev_init_err;
> +	}
> +
> +	ret = mtip_switch_dma_init(fep);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ethernet switch init fail (%d)!\n",
> +			__func__, ret);
> +		goto dma_init_err;
> +	}
> +
> +	ret = mtip_mii_init(fep, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__,
> +			ret);
> +		goto mii_init_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);
> +		dev_err(&pdev->dev, "%s: learning kthread_run error (%d)!\n",
> +			__func__, ret);
> +		goto task_learning_err;
> +	}
> +
> +	/* setup MII interface for external switch ports*/
> +	mtip_enet_init(fep, 1);
> +	mtip_enet_init(fep, 2);
> +
> +	return 0;
> +
> + task_learning_err:
> +	del_timer(&fep->timer_aging);
> +	mtip_mii_unregister(fep);
> + mii_init_err:
> + dma_init_err:
> +	mtip_ndev_cleanup(fep);
> + ndev_init_err:
> +	mtip_unregister_notifiers(fep);
> +
> +	return ret;
> +}
> +
> +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 = "nxp,imx287-mtip-switch", },
> +	{ /* sentinel */ }
> +};

Missing module device table.

> +
> +static struct platform_driver mtipl2plat_driver = {
> +	.driver         = {
> +		.name   = "mtipl2sw",
> +		.of_match_table = mtipl2_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe          = mtip_sw_probe,
> +	.remove_new     = mtip_sw_remove,
> +};
> +
> +module_platform_driver(mtipl2plat_driver);
> +MODULE_AUTHOR("Lukasz Majewski <lukma@denx.de>");
> +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch");
> +MODULE_VERSION(VERSION);

What is the point of paralell versioning with the kernel? Are you going
to keep this updated or - just like in other cases - it will stay always
theh same. Look for example at net/bridge/br.c or some other files -
they are always the same even if driver changed significantly.

BTW, this would be 1.0, not 1.4. Your out of tree versioning does not
matter.

> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mtipl2sw");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Andrew Lunn]

> +                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>;

Shared interrupts cannot be edge. They are level, so that either can
hold the interrupt active until it is cleared.

Also, PHY interrupts in general are level, because there are multiple
interrupt sources within the PHY, and you need to clear them all
before the interrupt is released.

	Andrew

Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Andrew Lunn]

> +                        /* Both PHYs (i.e. 0,1) have the same, single GPIO, */
> +                        /* line to handle both, their interrupts (AND'ed) */

ORed, not ANDed.

Often, the interrupt line has a weak pullup resistor, so by default it
is high. Either PHY can then pull it low, using an open collector,
which is HI-Z when not driving.

	Andrew

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

[Andrew Lunn]

> +static bool bridge_offload;
> +module_param(bridge_offload, bool, 0644); /* Allow setting by root on boot */
> +MODULE_PARM_DESC(bridge_offload, "L2 switch offload mode enable:1, disable:0");

Please drop. module parameters are not liked.

In Linux, ports of a switch always starting in isolated mode, and
userspace needs to add them to the same bridge.

> +
> +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb,
> +				   struct net_device *dev);
> +static void mtip_switch_tx(struct net_device *dev);
> +static int mtip_switch_rx(struct net_device *dev, int budget, int *port);
> +static void mtip_set_multicast_list(struct net_device *dev);
> +static void mtip_switch_restart(struct net_device *dev, int duplex0,
> +				int duplex1);

Forwards references are not like. Put the functions in the correct
order so they are not needed.

> +/* Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> + * (getting the remainder).
> + *
> + *  Examples (hexadecimal values):<br>
> + *   10-11-12-13-14-15  => CRC=0xc2
> + *   10-11-cc-dd-ee-00  => CRC=0xe6
> + *
> + *   param: pmacaddress
> + *          A 6-byte array with the MAC address.
> + *          The first byte is the first byte transmitted
> + *   return The 8-bit CRC in bits 7:0
> + */
> +static int crc8_calc(unsigned char *pmacaddress)
> +{
> +	/* byte index */
> +	int byt;
> +	/* bit index */
> +	int bit;
> +	int inval;
> +	int crc;

Reverse Christmas tree. Please look through the whole driver and fix
it up.

> +/* updates MAC address lookup table with a static entry
> + * Searches if the MAC address is already there in the block and replaces
> + * the older entry with new one. If MAC address is not there then puts a
> + * new entry in the first empty slot available in the block
> + *
> + * mac_addr Pointer to the array containing MAC address to
> + *          be put as static entry
> + * port     Port bitmask numbers to be added in static entry,
> + *          valid values are 1-7
> + * priority The priority for the static entry in table
> + *
> + * return 0 for a successful update else -1  when no slot available

It would be nice to turn this into proper kerneldoc. It is not too far
away at the moment.

Also, return a proper error code not -1. ENOSPC?

> +static int mtip_update_atable_dynamic1(unsigned long write_lo,
> +				       unsigned long write_hi, int block_index,
> +				       unsigned int port,
> +				       unsigned int curr_time,
> +				       struct switch_enet_private *fep)

It would be good to document the return value, because it is not the
usual 0 success or negative error code.

> +static const struct net_device_ops mtip_netdev_ops;

more forward declarations.

> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev)
> +{
> +	if (ndev->netdev_ops == &mtip_netdev_ops)
> +		return netdev_priv(ndev);
> +
> +	return NULL;
> +}

I _think_ the return value is not actually used. So maybe 0 or
-ENODEV?

> +static int esw_mac_addr_static(struct switch_enet_private *fep)
> +{
> +	int i;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) {

Is that possible? This is the interfaces own MAC address? If it is not
valid, the probe should of failed.

> +			mtip_update_atable_static((unsigned char *)
> +						  fep->ndev[i]->dev_addr,
> +						  7, 7, fep);
> +		} else {
> +			dev_err(&fep->pdev->dev,
> +				"Can not add mac address %pM to switch!\n",
> +				fep->ndev[i]->dev_addr);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void mtip_print_link_status(struct phy_device *phydev)
> +{
> +	if (phydev->link)
> +		netdev_info(phydev->attached_dev,
> +			    "Link is Up - %s/%s - flow control %s\n",
> +			    phy_speed_to_str(phydev->speed),
> +			    phy_duplex_to_str(phydev->duplex),
> +			    phydev->pause ? "rx/tx" : "off");
> +	else
> +		netdev_info(phydev->attached_dev, "Link is Down\n");
> +}

phy_print_status()

> +static void mtip_adjust_link(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct phy_device *phy_dev;
> +	int status_change = 0, idx;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fep->hw_lock, flags);
> +
> +	idx = priv->portnum - 1;
> +	phy_dev = fep->phy_dev[idx];
> +
> +	/* Prevent a state halted on mii error */
> +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> +		phy_dev->state = PHY_UP;
> +		goto spin_unlock;
> +	}

A MAC driver should not be playing around with the internal state of
phylib.

> +static int mtip_mii_probe(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int port_idx = priv->portnum - 1;
> +	struct phy_device *phy_dev = NULL;
> +
> +	if (fep->phy_np[port_idx]) {
> +		phy_dev = of_phy_connect(dev, fep->phy_np[port_idx],
> +					 &mtip_adjust_link, 0,
> +					 fep->phy_interface[port_idx]);
> +		if (!phy_dev) {
> +			netdev_err(dev, "Unable to connect to phy\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	phy_set_max_speed(phy_dev, 100);
> +	fep->phy_dev[port_idx] = phy_dev;
> +	fep->link[port_idx] = 0;
> +	fep->full_duplex[port_idx] = 0;
> +
> +	dev_info(&dev->dev,
> +		 "MTIP PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n",
> +		 fep->phy_dev[port_idx]->drv->name,
> +		 phydev_name(fep->phy_dev[port_idx]),
> +		 fep->phy_dev[port_idx]->irq);

phylib already prints something like that.

> +static int mtip_mdiobus_reset(struct mii_bus *bus)
> +{
> +	if (!bus || !bus->reset_gpiod) {
> +		dev_err(&bus->dev, "Reset GPIO pin not provided!\n");
> +		return -EINVAL;
> +	}
> +
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +
> +	/* Extra time to allow:
> +	 * 1. GPIO RESET pin go high to prevent situation where its value is
> +	 *    "LOW" as it is NOT configured.
> +	 * 2. The ENET CLK to stabilize before GPIO RESET is asserted
> +	 */
> +	usleep_range(200, 300);
> +
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 0);
> +	usleep_range(bus->reset_delay_us, bus->reset_delay_us + 1000);
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +
> +	if (bus->reset_post_delay_us > 0)
> +		usleep_range(bus->reset_post_delay_us,
> +			     bus->reset_post_delay_us + 1000);
> +
> +	return 0;
> +}

What is wrong with the core code __mdiobus_register() which does the
bus reset.

> +static void mtip_get_drvinfo(struct net_device *dev,
> +			     struct ethtool_drvinfo *info)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	strscpy(info->driver, fep->pdev->dev.driver->name,
> +		sizeof(info->driver));
> +	strscpy(info->version, VERSION, sizeof(info->version));

Leave this empty, so you get the git hash of the kernel.

> +static void mtip_ndev_setup(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +
> +	ether_setup(dev);

That is pretty unusual

> +	dev->ethtool_ops = &mtip_ethtool_ops;
> +	dev->netdev_ops = &mtip_netdev_ops;
> +
> +	memset(priv, 0, sizeof(struct mtip_ndev_priv));

priv should already be zero....

> +static int mtip_ndev_init(struct switch_enet_private *fep)
> +{
> +	struct mtip_ndev_priv *priv;
> +	int i, ret = 0;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		fep->ndev[i] = alloc_netdev(sizeof(struct mtip_ndev_priv),
> +					    fep->ndev_name[i], NET_NAME_USER,
> +					    mtip_ndev_setup);

This explains the ether_setup(). It would be more normal to pass
ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here.

> +		if (!fep->ndev[i]) {
> +			ret = -1;

-ENOMEM?

> +			break;
> +		}
> +
> +		priv = netdev_priv(fep->ndev[i]);
> +		priv->fep = fep;
> +		priv->portnum = i + 1;
> +		fep->ndev[i]->irq = fep->irq;
> +
> +		ret = mtip_setup_mac(fep->ndev[i]);
> +		if (ret) {
> +			dev_err(&fep->ndev[i]->dev,
> +				"%s: ndev %s MAC setup err: %d\n",
> +				__func__, fep->ndev[i]->name, ret);
> +			break;
> +		}
> +
> +		ret = register_netdev(fep->ndev[i]);
> +		if (ret) {
> +			dev_err(&fep->ndev[i]->dev,
> +				"%s: ndev %s register err: %d\n", __func__,
> +				fep->ndev[i]->name, ret);
> +			break;
> +		}
> +		dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2 switch %pM\n",
> +			 fep->ndev[i]->name, fep->ndev[i]->dev_addr);

I would drop this. A driver is normally silent unless things go wrong.

> +	}
> +
> +	if (ret)
> +		mtip_ndev_cleanup(fep);
> +
> +	return 0;

return ret?

> +static int mtip_ndev_port_link(struct net_device *ndev,
> +			       struct net_device *br_ndev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n",
> +		__func__, ndev->name,  br_ndev->name, (unsigned int)fep);
> +
> +	/* Check if MTIP switch is already enabled */
> +	if (!fep->br_offload) {
> +		if (!priv->master_dev)
> +			priv->master_dev = br_ndev;

It needs to be a little bit more complex than that, because the two
ports could be assigned to two different bridges. You should only
enable hardware bridging if they are a member of the same bridge.

	Andrew

Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Rob Herring (Arm)]

On Fri, 28 Mar 2025 14:35:41 +0100, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - e.g. imx287.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)
> ---
>  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: /example-0/switch@800f0000: failed to match any schema with compatible: ['nxp,imx287-mtip-switch']
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: ethernet-phy@0: interrupts: [[13], [2]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: ethernet-phy@1: interrupts: [[13], [2]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250328133544.4149716-2-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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Rob Herring]

On Fri, Mar 28, 2025 at 02:35:41PM +0100, Lukasz Majewski wrote:
> This patch provides description of the MTIP L2 switch available in some
> NXP's SOCs - e.g. imx287.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)
> ---
>  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> new file mode 100644
> index 000000000000..a3e0fe7783ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,imx287-mtip-switch.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> +
> +maintainers:
> +  - Lukasz Majewski <lukma@denx.de>
> +
> +description:
> +  The 2-port switch ethernet subsystem provides ethernet packet (L2)
> +  communication and can be configured as an ethernet switch. It provides the
> +  reduced media independent interface (RMII), the management data input
> +  output (MDIO) for physical layer device (PHY) management.
> +
> +properties:
> +  compatible:
> +    const: nxp,imx287-mtip--switch
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      The physical base address and size of the MTIP L2 SW module IO range
> +
> +  phy-supply:
> +    description:
> +      Regulator that powers Ethernet PHYs.
> +
> +  clocks:
> +    items:
> +      - description: Register accessing clock
> +      - description: Bus access clock
> +      - description: Output clock for external device - e.g. PHY source clock
> +      - description: IEEE1588 timer clock
> +
> +  clock-names:
> +    items:
> +      - const: ipg
> +      - const: ahb
> +      - const: enet_out
> +      - const: ptp
> +
> +  interrupts:
> +    items:
> +      - description: Switch interrupt
> +      - description: ENET0 interrupt
> +      - description: ENET1 interrupt
> +
> +  pinctrl-names: true
> +
> +  ethernet-ports:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      "^port@[0-9]+$":
> +        type: object
> +        description: MTIP L2 switch external ports
> +
> +        $ref: ethernet-controller.yaml#
> +        unevaluatedProperties: false
> +
> +        properties:
> +          reg:
> +            items:
> +              - enum: [1, 2]
> +            description: MTIP L2 switch port number
> +
> +          label:
> +            description: Label associated with this port
> +
> +        required:
> +          - reg
> +          - label
> +          - phy-mode
> +          - phy-handle
> +
> +  mdio:
> +    type: object
> +    $ref: mdio.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      Specifies the mdio bus in the switch, used as a container for phy nodes.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - mdio
> +  - ethernet-ports
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include<dt-bindings/interrupt-controller/irq.h>
> +    switch@800f0000 {
> +        compatible = "nxp,imx287-mtip-switch";
> +        reg = <0x800f0000 0x20000>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> +        phy-supply = <&reg_fec_3v3>;
> +        interrupts = <100>, <101>, <102>;
> +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> +        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_sw: mdio {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                reset-gpios = <&gpio2 13 0>;
> +                reset-delay-us = <25000>;
> +                reset-post-delay-us = <10000>;
> +
> +                ethphy0: ethernet-phy@0 {
> +                        reg = <0>;
> +                        smsc,disable-energy-detect;

With a custom property, you should have a specific compatible.

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

The error report is because the examples have to guess the number of 
provider interrupt cells and only 1 guess is supported. It guessed 1 
from above.

In any case, unless the phys are built-in and fixed, they are out of 
scope of this binding. So perhaps drop the interrupts and smsc property.

Rob

Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Rob,

> On Fri, Mar 28, 2025 at 02:35:41PM +0100, Lukasz Majewski wrote:
> > This patch provides description of the MTIP L2 switch available in
> > some NXP's SOCs - e.g. imx287.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > Changes for v2:
> > - Rename the file to match exactly the compatible
> >   (nxp,imx287-mtip-switch)
> > ---
> >  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165
> > ++++++++++++++++++ 1 file changed, 165 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > new file mode 100644 index 000000000000..a3e0fe7783ec --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > @@ -0,0 +1,165 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > BSD-2-Clause) +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,imx287-mtip-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@denx.de>
> > +
> > +description:
> > +  The 2-port switch ethernet subsystem provides ethernet packet
> > (L2)
> > +  communication and can be configured as an ethernet switch. It
> > provides the
> > +  reduced media independent interface (RMII), the management data
> > input
> > +  output (MDIO) for physical layer device (PHY) management.
> > +
> > +properties:
> > +  compatible:
> > +    const: nxp,imx287-mtip--switch
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      The physical base address and size of the MTIP L2 SW module
> > IO range +
> > +  phy-supply:
> > +    description:
> > +      Regulator that powers Ethernet PHYs.
> > +
> > +  clocks:
> > +    items:
> > +      - description: Register accessing clock
> > +      - description: Bus access clock
> > +      - description: Output clock for external device - e.g. PHY
> > source clock
> > +      - description: IEEE1588 timer clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ipg
> > +      - const: ahb
> > +      - const: enet_out
> > +      - const: ptp
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Switch interrupt
> > +      - description: ENET0 interrupt
> > +      - description: ENET1 interrupt
> > +
> > +  pinctrl-names: true
> > +
> > +  ethernet-ports:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^port@[0-9]+$":
> > +        type: object
> > +        description: MTIP L2 switch external ports
> > +
> > +        $ref: ethernet-controller.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          reg:
> > +            items:
> > +              - enum: [1, 2]
> > +            description: MTIP L2 switch port number
> > +
> > +          label:
> > +            description: Label associated with this port
> > +
> > +        required:
> > +          - reg
> > +          - label
> > +          - phy-mode
> > +          - phy-handle
> > +
> > +  mdio:
> > +    type: object
> > +    $ref: mdio.yaml#
> > +    unevaluatedProperties: false
> > +    description:
> > +      Specifies the mdio bus in the switch, used as a container
> > for phy nodes. +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - mdio
> > +  - ethernet-ports
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/irq.h>
> > +    switch@800f0000 {
> > +        compatible = "nxp,imx287-mtip-switch";
> > +        reg = <0x800f0000 0x20000>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> > +        phy-supply = <&reg_fec_3v3>;
> > +        interrupts = <100>, <101>, <102>;
> > +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> > +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> > +        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_sw: mdio {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                reset-gpios = <&gpio2 13 0>;
> > +                reset-delay-us = <25000>;
> > +                reset-post-delay-us = <10000>;
> > +
> > +                ethphy0: ethernet-phy@0 {
> > +                        reg = <0>;
> > +                        smsc,disable-energy-detect;  
> 
> With a custom property, you should have a specific compatible.

I could add:
compatible = "ethernet-phy-id0007.c0f0","ethernet-phy-ieee802.3-c22";

but it would not make things either clearer nor simpler.

I will just remove this property.

> 
> > +                        /* 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>;  
> 
> The error report is because the examples have to guess the number of 
> provider interrupt cells and only 1 guess is supported. It guessed 1 
> from above.
> 
> In any case, unless the phys are built-in and fixed, they are out of 
> scope of this binding. So perhaps drop the interrupts and smsc
> property.

Ok, I will remove them.

> 
> Rob




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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Krzysztof,

> On 28/03/2025 14:35, Lukasz Majewski wrote:
> > This patch provides description of the MTIP L2 switch available in
> > some NXP's SOCs - e.g. imx287.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > Changes for v2:
> > - Rename the file to match exactly the compatible
> >   (nxp,imx287-mtip-switch)  
> 
> Please implement all the changes, not only the rename. I gave several
> comments, although quick glance suggests you did implement them, so
> then changelog is just incomplete.

Those comments were IMHO addressed automatically, as this time I took
(I suppose :-) ) better file as a starting point.

To be more specific it was:
./Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml

as I've been advised to use for the MTIP driver the same DTS
description as the above one has (i.e. they are conceptually similar,
so DTS description ABI can be used for both).

I've also checked the:
make CHECK_DTBS=y DT_SCHEMA_FILES=nxp,imx287-mtip-switch.yaml
nxp/mxs/imx28-xea.dtb

on Linux next and it gave no errors.

> 
> > ---
> >  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165
> > ++++++++++++++++++ 1 file changed, 165 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > new file mode 100644 index 000000000000..a3e0fe7783ec --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > @@ -0,0 +1,165 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > BSD-2-Clause) +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,imx287-mtip-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@denx.de>
> > +
> > +description:
> > +  The 2-port switch ethernet subsystem provides ethernet packet
> > (L2)
> > +  communication and can be configured as an ethernet switch. It
> > provides the
> > +  reduced media independent interface (RMII), the management data
> > input
> > +  output (MDIO) for physical layer device (PHY) management.
> > +  
> 
> If this is ethernet switch, why it does not reference ethernet-switch
> schema? or dsa.yaml or dsa/ethernet-ports? I am not sure which one
> should go here, but surprising to see none.

It uses:
$ref:·ethernet-controller.yaml#

for "ports".

Other crucial node is "mdio", which references $ref: mdio.yaml#

> 
> > +properties:
> > +  compatible:
> > +    const: nxp,imx287-mtip--switch  
> 
> Just one -.
> 

Ok.

> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      The physical base address and size of the MTIP L2 SW module
> > IO range  
> 
> Wasn't here, drop.
> 

The 'reg' property (reg = <0x800f0000 0x20000>;) is defined in
imx28.dtsi, where the SoC generic properties (as suggested by Andrew -
like clocks, interrupts, clock-names) are moved.

> > +
> > +  phy-supply:
> > +    description:
> > +      Regulator that powers Ethernet PHYs.
> > +
> > +  clocks:
> > +    items:
> > +      - description: Register accessing clock
> > +      - description: Bus access clock
> > +      - description: Output clock for external device - e.g. PHY
> > source clock
> > +      - description: IEEE1588 timer clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ipg
> > +      - const: ahb
> > +      - const: enet_out
> > +      - const: ptp
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Switch interrupt
> > +      - description: ENET0 interrupt
> > +      - description: ENET1 interrupt
> > +
> > +  pinctrl-names: true  
> 
> Drop

The 'pinctrl-names = "default";' are specified.

Shouldn't it be kept?

> 
> > +
> > +  ethernet-ports:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^port@[0-9]+$":  
> 
> Keep consistent quotes, either " or '. Also [01]
> 

[12] - ports are numbered starting from 1.

> 
> > +        type: object
> > +        description: MTIP L2 switch external ports
> > +
> > +        $ref: ethernet-controller.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          reg:
> > +            items:
> > +              - enum: [1, 2]
> > +            description: MTIP L2 switch port number
> > +
> > +          label:
> > +            description: Label associated with this port
> > +
> > +        required:
> > +          - reg
> > +          - label
> > +          - phy-mode
> > +          - phy-handle
> > +
> > +  mdio:
> > +    type: object
> > +    $ref: mdio.yaml#
> > +    unevaluatedProperties: false
> > +    description:
> > +      Specifies the mdio bus in the switch, used as a container
> > for phy nodes. +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - mdio
> > +  - ethernet-ports
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/irq.h>
> > +    switch@800f0000 {
> > +        compatible = "nxp,imx287-mtip-switch";
> > +        reg = <0x800f0000 0x20000>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> > +        phy-supply = <&reg_fec_3v3>;
> > +        interrupts = <100>, <101>, <102>;
> > +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> > +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> > +        status = "okay";  
> 
> Drop

Ok.

> 
> > +
> > +        ethernet-ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;  
> 
> Messed indentation. See example-schema or writing-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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Andrew,

> > +                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>;  
> 
> Shared interrupts cannot be edge. They are level, so that either can
> hold the interrupt active until it is cleared.
> 
> Also, PHY interrupts in general are level, because there are multiple
> interrupt sources within the PHY, and you need to clear them all
> before the interrupt is released.

Good point, I will update it accordingly. Thanks.

> 
> 	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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Andrew,

> > +                        /* Both PHYs (i.e. 0,1) have the same,
> > single GPIO, */
> > +                        /* line to handle both, their interrupts
> > (AND'ed) */  
> 
> ORed, not ANDed.
> 

Yes, correct.

> Often, the interrupt line has a weak pullup resistor, so by default it
> is high. Either PHY can then pull it low, using an open collector,
> which is HI-Z when not driving.

Yes, correct - this is how it works.

> 
> 	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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Krzysztof Kozlowski]

On 29/03/2025 23:10, Lukasz Majewski wrote:
>>> +  
>>
>> If this is ethernet switch, why it does not reference ethernet-switch
>> schema? or dsa.yaml or dsa/ethernet-ports? I am not sure which one
>> should go here, but surprising to see none.
> 
> It uses:
> $ref:·ethernet-controller.yaml#
> 
> for "ports".
> 
> Other crucial node is "mdio", which references $ref: mdio.yaml#

These are children, I am speaking about this device node.

> 
>>
>>> +properties:
>>> +  compatible:
>>> +    const: nxp,imx287-mtip--switch  
>>
>> Just one -.
>>
> 
> Ok.
> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +    description:
>>> +      The physical base address and size of the MTIP L2 SW module
>>> IO range  
>>
>> Wasn't here, drop.
>>
> 
> The 'reg' property (reg = <0x800f0000 0x20000>;) is defined in
> imx28.dtsi, where the SoC generic properties (as suggested by Andrew -
> like clocks, interrupts, clock-names) are moved.

Drop description, not the reg. Reg was in the previous version. You
added random changes here, not coming from the previous review.

Best regards,
Krzysztof

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

[Lukasz Majewski]

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

Hi Andrew,

> > +static bool bridge_offload;
> > +module_param(bridge_offload, bool, 0644); /* Allow setting by root
> > on boot */ +MODULE_PARM_DESC(bridge_offload, "L2 switch offload
> > mode enable:1, disable:0");  
> 
> Please drop. module parameters are not liked.
> 

Ok.

> In Linux, ports of a switch always starting in isolated mode, and
> userspace needs to add them to the same bridge.

Ok.

> 
> > +
> > +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb,
> > +				   struct net_device *dev);
> > +static void mtip_switch_tx(struct net_device *dev);
> > +static int mtip_switch_rx(struct net_device *dev, int budget, int
> > *port); +static void mtip_set_multicast_list(struct net_device
> > *dev); +static void mtip_switch_restart(struct net_device *dev, int
> > duplex0,
> > +				int duplex1);  
> 
> Forwards references are not like. Put the functions in the correct
> order so they are not needed.

Ok.

> 
> > +/* Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> > + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> > + * (getting the remainder).
> > + *
> > + *  Examples (hexadecimal values):<br>
> > + *   10-11-12-13-14-15  => CRC=0xc2
> > + *   10-11-cc-dd-ee-00  => CRC=0xe6
> > + *
> > + *   param: pmacaddress
> > + *          A 6-byte array with the MAC address.
> > + *          The first byte is the first byte transmitted
> > + *   return The 8-bit CRC in bits 7:0
> > + */
> > +static int crc8_calc(unsigned char *pmacaddress)
> > +{
> > +	/* byte index */
> > +	int byt;
> > +	/* bit index */
> > +	int bit;
> > +	int inval;
> > +	int crc;  
> 
> Reverse Christmas tree. Please look through the whole driver and fix
> it up.

Ok.

> 
> > +/* updates MAC address lookup table with a static entry
> > + * Searches if the MAC address is already there in the block and
> > replaces
> > + * the older entry with new one. If MAC address is not there then
> > puts a
> > + * new entry in the first empty slot available in the block
> > + *
> > + * mac_addr Pointer to the array containing MAC address to
> > + *          be put as static entry
> > + * port     Port bitmask numbers to be added in static entry,
> > + *          valid values are 1-7
> > + * priority The priority for the static entry in table
> > + *
> > + * return 0 for a successful update else -1  when no slot
> > available  
> 
> It would be nice to turn this into proper kerneldoc. It is not too far
> away at the moment.
> 
> Also, return a proper error code not -1. ENOSPC?

Ok.

> 
> > +static int mtip_update_atable_dynamic1(unsigned long write_lo,
> > +				       unsigned long write_hi, int
> > block_index,
> > +				       unsigned int port,
> > +				       unsigned int curr_time,
> > +				       struct switch_enet_private
> > *fep)  
> 
> It would be good to document the return value, because it is not the
> usual 0 success or negative error code.

Ok.

> 
> > +static const struct net_device_ops mtip_netdev_ops;  
> 
> more forward declarations.

Ok, fixed.

> 
> > +struct switch_enet_private *mtip_netdev_get_priv(const struct
> > net_device *ndev) +{
> > +	if (ndev->netdev_ops == &mtip_netdev_ops)
> > +		return netdev_priv(ndev);
> > +
> > +	return NULL;
> > +}  
> 
> I _think_ the return value is not actually used. So maybe 0 or
> -ENODEV?

It is used at:
drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c in
mtip_port_dev_check()

to assess if network interfaces eligible for bridging are using the
same (i.e. mtipl2sw) driver.

Only when they match - bridging is performed.

> 
> > +static int esw_mac_addr_static(struct switch_enet_private *fep)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > +		if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) {
> >  
> 
> Is that possible? This is the interfaces own MAC address? If it is not
> valid, the probe should of failed.

I've double checked it - this cannot happen (i.e. that
is_valid_ether_addr(fep->ndev[i]->dev_addr) is NOT valid at this point
of execution.

I will remove this check

> 
> > +			mtip_update_atable_static((unsigned char *)
> > +
> > fep->ndev[i]->dev_addr,
> > +						  7, 7, fep);
> > +		} else {
> > +			dev_err(&fep->pdev->dev,
> > +				"Can not add mac address %pM to
> > switch!\n",
> > +				fep->ndev[i]->dev_addr);
> > +			return -EFAULT;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtip_print_link_status(struct phy_device *phydev)
> > +{
> > +	if (phydev->link)
> > +		netdev_info(phydev->attached_dev,
> > +			    "Link is Up - %s/%s - flow control
> > %s\n",
> > +			    phy_speed_to_str(phydev->speed),
> > +			    phy_duplex_to_str(phydev->duplex),
> > +			    phydev->pause ? "rx/tx" : "off");
> > +	else
> > +		netdev_info(phydev->attached_dev, "Link is
> > Down\n"); +}  
> 
> phy_print_status()

Yes, I will remove mtip_print_link_status() and replace it with
phy_print_status()

> 
> > +static void mtip_adjust_link(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct phy_device *phy_dev;
> > +	int status_change = 0, idx;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fep->hw_lock, flags);
> > +
> > +	idx = priv->portnum - 1;
> > +	phy_dev = fep->phy_dev[idx];
> > +
> > +	/* Prevent a state halted on mii error */
> > +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> > +		phy_dev->state = PHY_UP;
> > +		goto spin_unlock;
> > +	}  
> 
> A MAC driver should not be playing around with the internal state of
> phylib.

Ok, I've replaced it with PHY API calls (phy_start() and
phy_is_started()).

> 
> > +static int mtip_mii_probe(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +	int port_idx = priv->portnum - 1;
> > +	struct phy_device *phy_dev = NULL;
> > +
> > +	if (fep->phy_np[port_idx]) {
> > +		phy_dev = of_phy_connect(dev,
> > fep->phy_np[port_idx],
> > +					 &mtip_adjust_link, 0,
> > +
> > fep->phy_interface[port_idx]);
> > +		if (!phy_dev) {
> > +			netdev_err(dev, "Unable to connect to
> > phy\n");
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	phy_set_max_speed(phy_dev, 100);
> > +	fep->phy_dev[port_idx] = phy_dev;
> > +	fep->link[port_idx] = 0;
> > +	fep->full_duplex[port_idx] = 0;
> > +
> > +	dev_info(&dev->dev,
> > +		 "MTIP PHY driver [%s] (mii_bus:phy_addr=%s,
> > irq=%d)\n",
> > +		 fep->phy_dev[port_idx]->drv->name,
> > +		 phydev_name(fep->phy_dev[port_idx]),
> > +		 fep->phy_dev[port_idx]->irq);  
> 
> phylib already prints something like that.

Yes, the 
"net lan0: lan0: MTIP eth L2 switch <mac addr>" 

is printed.

For the original call - I've used dev_dbg().

> 
> > +static int mtip_mdiobus_reset(struct mii_bus *bus)
> > +{
> > +	if (!bus || !bus->reset_gpiod) {
> > +		dev_err(&bus->dev, "Reset GPIO pin not
> > provided!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> > +
> > +	/* Extra time to allow:
> > +	 * 1. GPIO RESET pin go high to prevent situation where
> > its value is
> > +	 *    "LOW" as it is NOT configured.
> > +	 * 2. The ENET CLK to stabilize before GPIO RESET is
> > asserted
> > +	 */
> > +	usleep_range(200, 300);
> > +
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 0);
> > +	usleep_range(bus->reset_delay_us, bus->reset_delay_us +
> > 1000);
> > +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> > +
> > +	if (bus->reset_post_delay_us > 0)
> > +		usleep_range(bus->reset_post_delay_us,
> > +			     bus->reset_post_delay_us + 1000);
> > +
> > +	return 0;
> > +}  
> 
> What is wrong with the core code __mdiobus_register() which does the
> bus reset.

The main problem is that the "default" mdio reset is just asserting and
deasserting the reset line.

It doesn't take into account the state of the reset gpio before
assertion (if it was high for enough time) and if clocks already
were stabilized.

> 
> > +static void mtip_get_drvinfo(struct net_device *dev,
> > +			     struct ethtool_drvinfo *info)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	strscpy(info->driver, fep->pdev->dev.driver->name,
> > +		sizeof(info->driver));
> > +	strscpy(info->version, VERSION, sizeof(info->version));  
> 
> Leave this empty, so you get the git hash of the kernel.

Ok.

> 
> > +static void mtip_ndev_setup(struct net_device *dev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> > +
> > +	ether_setup(dev);  
> 
> That is pretty unusual

Yes - how it has been used is described below.

> 
> > +	dev->ethtool_ops = &mtip_ethtool_ops;
> > +	dev->netdev_ops = &mtip_netdev_ops;
> > +
> > +	memset(priv, 0, sizeof(struct mtip_ndev_priv));  
> 
> priv should already be zero....

Ok, I will remove

> 
> > +static int mtip_ndev_init(struct switch_enet_private *fep)
> > +{
> > +	struct mtip_ndev_priv *priv;
> > +	int i, ret = 0;
> > +
> > +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> > +		fep->ndev[i] = alloc_netdev(sizeof(struct
> > mtip_ndev_priv),
> > +					    fep->ndev_name[i],
> > NET_NAME_USER,
> > +					    mtip_ndev_setup);  
> 
> This explains the ether_setup(). It would be more normal to pass
> ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here.
> 

Yes. I will do that.

> > +		if (!fep->ndev[i]) {
> > +			ret = -1;  
> 
> -ENOMEM?

Ok.

> 
> > +			break;
> > +		}
> > +
> > +		priv = netdev_priv(fep->ndev[i]);
> > +		priv->fep = fep;
> > +		priv->portnum = i + 1;
> > +		fep->ndev[i]->irq = fep->irq;
> > +
> > +		ret = mtip_setup_mac(fep->ndev[i]);
> > +		if (ret) {
> > +			dev_err(&fep->ndev[i]->dev,
> > +				"%s: ndev %s MAC setup err: %d\n",
> > +				__func__, fep->ndev[i]->name, ret);
> > +			break;
> > +		}
> > +
> > +		ret = register_netdev(fep->ndev[i]);
> > +		if (ret) {
> > +			dev_err(&fep->ndev[i]->dev,
> > +				"%s: ndev %s register err: %d\n",
> > __func__,
> > +				fep->ndev[i]->name, ret);
> > +			break;
> > +		}
> > +		dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2
> > switch %pM\n",
> > +			 fep->ndev[i]->name,
> > fep->ndev[i]->dev_addr);  
> 
> I would drop this. A driver is normally silent unless things go wrong.

I've replaced dev_info() with dev_dbg() as this information may be
relevant during development.

> 
> > +	}
> > +
> > +	if (ret)
> > +		mtip_ndev_cleanup(fep);
> > +
> > +	return 0;  
> 
> return ret?

Ok.

> 
> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep); +
> > +	/* Check if MTIP switch is already enabled */
> > +	if (!fep->br_offload) {
> > +		if (!priv->master_dev)
> > +			priv->master_dev = br_ndev;  
> 
> It needs to be a little bit more complex than that, because the two
> ports could be assigned to two different bridges. You should only
> enable hardware bridging if they are a member of the same bridge.

This has been explained earlier.
The mtip_port_dev_check() checks in mtip_netdevice_event() if we use
ports from the mtipl2sw driver.

Only for them we start the bridge.

> 
> 	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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Krzysztof,

> On 29/03/2025 23:10, Lukasz Majewski wrote:
> >>> +    
> >>
> >> If this is ethernet switch, why it does not reference
> >> ethernet-switch schema? or dsa.yaml or dsa/ethernet-ports? I am
> >> not sure which one should go here, but surprising to see none.  
> > 
> > It uses:
> > $ref:·ethernet-controller.yaml#
> > 
> > for "ports".
> > 
> > Other crucial node is "mdio", which references $ref: mdio.yaml#  
> 
> These are children, I am speaking about this device node.

It looks like there is no such reference.

I've checked the aforementioned ti,cpsw-switch.yaml,
microchip,lan966x-switch.yaml and renesas,r8a779f0-ether-switch.yaml.

Those only have $ref: for ethernet-port children node.

The "outer" one doesn't have it.


Or am I missing something?

> 
> >   
> >>  
> >>> +properties:
> >>> +  compatible:
> >>> +    const: nxp,imx287-mtip--switch    
> >>
> >> Just one -.
> >>  
> > 
> > Ok.
> >   
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      The physical base address and size of the MTIP L2 SW module
> >>> IO range    
> >>
> >> Wasn't here, drop.
> >>  
> > 
> > The 'reg' property (reg = <0x800f0000 0x20000>;) is defined in
> > imx28.dtsi, where the SoC generic properties (as suggested by
> > Andrew - like clocks, interrupts, clock-names) are moved.  
> 
> Drop description, not the reg. Reg was in the previous version. You
> added random changes here, not coming from the previous review.
> 

Ach... You mean the "description" in the:

	reg:
	  maxItems: 1
	  description:
	    XX YY

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

[Andrew Lunn]

> > > +	/* Prevent a state halted on mii error */
> > > +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> > > +		phy_dev->state = PHY_UP;
> > > +		goto spin_unlock;
> > > +	}  
> > 
> > A MAC driver should not be playing around with the internal state of
> > phylib.
> 
> Ok, I've replaced it with PHY API calls (phy_start() and
> phy_is_started()).

phy_start() and phy_stop() should be used in pairs. It is not good to
call start more often than stop.

What exactly is going on here? Why would there be MII errors?

	Andrew

Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Krzysztof Kozlowski]

On 30/03/2025 23:04, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 29/03/2025 23:10, Lukasz Majewski wrote:
>>>>> +    
>>>>
>>>> If this is ethernet switch, why it does not reference
>>>> ethernet-switch schema? or dsa.yaml or dsa/ethernet-ports? I am
>>>> not sure which one should go here, but surprising to see none.  
>>>
>>> It uses:
>>> $ref:·ethernet-controller.yaml#
>>>
>>> for "ports".
>>>
>>> Other crucial node is "mdio", which references $ref: mdio.yaml#  
>>
>> These are children, I am speaking about this device node.
> 
> It looks like there is no such reference.
> 
> I've checked the aforementioned ti,cpsw-switch.yaml,
> microchip,lan966x-switch.yaml and renesas,r8a779f0-ether-switch.yaml.
> 
> Those only have $ref: for ethernet-port children node.
> 
> The "outer" one doesn't have it.
> 
> 
> Or am I missing something?

There is ethernet-switch.yaml for non-DSA switches and there is DSA
(using ethernet switch, btw). I don't know why these devices do not use
it, I guess no one converted them after we split ethernet-switch out of DSA.

Best regards,
Krzysztof

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

[Lukasz Majewski]

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

Hi Andrew.

> > > > +	/* Prevent a state halted on mii error */
> > > > +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> > > > +		phy_dev->state = PHY_UP;
> > > > +		goto spin_unlock;
> > > > +	}    
> > > 
> > > A MAC driver should not be playing around with the internal state
> > > of phylib.  
> > 
> > Ok, I've replaced it with PHY API calls (phy_start() and
> > phy_is_started()).  
> 
> phy_start() and phy_stop() should be used in pairs. It is not good to
> call start more often than stop.
> 
> What exactly is going on here? Why would there be MII errors?
> 

Exactly.

I've double check it - this can be safely dropped.

> 	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 v2 1/4] dt-bindings: net: Add MTIP L2 switch description

[Lukasz Majewski]

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

Hi Krzysztof,

> On 30/03/2025 23:04, Lukasz Majewski wrote:
> > Hi Krzysztof,
> >   
> >> On 29/03/2025 23:10, Lukasz Majewski wrote:  
> >>>>> +      
> >>>>
> >>>> If this is ethernet switch, why it does not reference
> >>>> ethernet-switch schema? or dsa.yaml or dsa/ethernet-ports? I am
> >>>> not sure which one should go here, but surprising to see none.
> >>>>  
> >>>
> >>> It uses:
> >>> $ref:·ethernet-controller.yaml#
> >>>
> >>> for "ports".
> >>>
> >>> Other crucial node is "mdio", which references $ref: mdio.yaml#
> >>>  
> >>
> >> These are children, I am speaking about this device node.  
> > 
> > It looks like there is no such reference.
> > 
> > I've checked the aforementioned ti,cpsw-switch.yaml,
> > microchip,lan966x-switch.yaml and
> > renesas,r8a779f0-ether-switch.yaml.
> > 
> > Those only have $ref: for ethernet-port children node.
> > 
> > The "outer" one doesn't have it.
> > 
> > 
> > Or am I missing something?  
> 
> There is ethernet-switch.yaml for non-DSA switches and there is DSA
> (using ethernet switch, btw). I don't know why these devices do not
> use it, I guess no one converted them after we split ethernet-switch
> out of DSA.

In net next there is:
Documentation/devicetree/bindings/net/dsa/dsa.yaml
Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml

which uses
$ref: ethernet-switch.yaml#

I will add it in a similar way as it is in dsa.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 v2 4/4] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

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

Hi Krzysztof,

> On 28/03/2025 14:35, Lukasz Majewski wrote:
> > +
> > +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 = {  
> 
> That's const.
> 
> > +	.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;
> > +	int ret;
> > +
> > +	fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL);
> > +	if (!fep)
> > +		return -ENOMEM;
> > +
> > +	pdev->id_entry = &pdev_id;  
> 
> Hm? This is some odd pattern. You are supposed to use OF table and get
> matched by it, not populate some custom/odd handling of platform
> tables.

I've removed it and fully utilized struct of_device_id. I will just use
the OF approach without utilizing platform device.

I think that it is better to just switch to OF.

> 
> > +
> > +	dev_info = (struct fec_devinfo
> > *)pdev->id_entry->driver_data;  
> 
> I did not notice it before, but that's a no - you cannot drop the
> cast. Driver data is always const.

The platform device ID approach has been dropped and completely
replaced with OF.

> 
> > +	if (dev_info)
> > +		fep->quirks = dev_info->quirks;
> > +
> > +	fep->pdev = pdev;
> > +	platform_set_drvdata(pdev, fep);
> > +
> > +	fep->enet_addr = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(fep->enet_addr))
> > +		return PTR_ERR(fep->enet_addr);
> > +
> > +	fep->irq = platform_get_irq(pdev, 0);
> > +	if (fep->irq < 0)
> > +		return fep->irq;
> > +
> > +	ret = mtip_parse_of(fep, np);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "%s: OF parse error (%d)!\n",
> > __func__,
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Create an Ethernet device instance.
> > +	 * The switch lookup address memory starts at 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);
> > +
> > +	ret = devm_regulator_get_enable_optional(&pdev->dev,
> > "phy");
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Unable to get and enable
> > 'phy'\n"); +
> > +	fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> > +	if (IS_ERR(fep->clk_ipg))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ipg),
> > +				     "Unable to acquire 'ipg'
> > clock\n"); +
> > +	fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> > +	if (IS_ERR(fep->clk_ahb))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ahb),
> > +				     "Unable to acquire 'ahb'
> > clock\n"); +
> > +	fep->clk_enet_out =
> > devm_clk_get_optional_enabled(&pdev->dev,
> > +
> > "enet_out");
> > +	if (IS_ERR(fep->clk_enet_out))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_enet_out),
> > +				     "Unable to acquire 'enet_out'
> > clock\n"); +
> > +	spin_lock_init(&fep->learn_lock);
> > +	spin_lock_init(&fep->hw_lock);
> > +	spin_lock_init(&fep->mii_lock);
> > +
> > +	ret = devm_request_irq(&pdev->dev, fep->irq,
> > mtip_interrupt, 0,
> > +			       "mtip_l2sw", fep);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, fep->irq,
> > +				     "Could not alloc IRQ\n");
> > +
> > +	ret = mtip_register_notifiers(fep);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtip_ndev_init(fep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Failed to create virtual
> > ndev (%d)\n",
> > +			__func__, ret);
> > +		goto ndev_init_err;
> > +	}
> > +
> > +	ret = mtip_switch_dma_init(fep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: ethernet switch init fail
> > (%d)!\n",
> > +			__func__, ret);
> > +		goto dma_init_err;
> > +	}
> > +
> > +	ret = mtip_mii_init(fep, pdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Cannot init phy bus
> > (%d)!\n", __func__,
> > +			ret);
> > +		goto mii_init_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);
> > +		dev_err(&pdev->dev, "%s: learning kthread_run
> > error (%d)!\n",
> > +			__func__, ret);
> > +		goto task_learning_err;
> > +	}
> > +
> > +	/* setup MII interface for external switch ports*/
> > +	mtip_enet_init(fep, 1);
> > +	mtip_enet_init(fep, 2);
> > +
> > +	return 0;
> > +
> > + task_learning_err:
> > +	del_timer(&fep->timer_aging);
> > +	mtip_mii_unregister(fep);
> > + mii_init_err:
> > + dma_init_err:
> > +	mtip_ndev_cleanup(fep);
> > + ndev_init_err:
> > +	mtip_unregister_notifiers(fep);
> > +
> > +	return ret;
> > +}
> > +
> > +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 = "nxp,imx287-mtip-switch", },
> > +	{ /* sentinel */ }
> > +};  
> 
> Missing module device table.

Ok. I will add it.

> 
> > +
> > +static struct platform_driver mtipl2plat_driver = {
> > +	.driver         = {
> > +		.name   = "mtipl2sw",
> > +		.of_match_table = mtipl2_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe          = mtip_sw_probe,
> > +	.remove_new     = mtip_sw_remove,
> > +};
> > +
> > +module_platform_driver(mtipl2plat_driver);
> > +MODULE_AUTHOR("Lukasz Majewski <lukma@denx.de>");
> > +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch");
> > +MODULE_VERSION(VERSION);  
> 
> What is the point of paralell versioning with the kernel? Are you
> going to keep this updated or - just like in other cases - it will
> stay always theh same. Look for example at net/bridge/br.c or some
> other files - they are always the same even if driver changed
> significantly.
> 
> BTW, this would be 1.0, not 1.4. Your out of tree versioning does not
> matter.

I'm going to drop it totally. The "versioning" was only required when
switching between major LTS kernels.

I'd be more than happy to just use kernel SHA1, when this driver is
pulled.


> 
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:mtipl2sw");  
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
> 

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