Skip to contentSkip to conversation

Add preliminary NETC switch support for i.MX94

PATCH Superseded Wei Fang 4 participants 38 messages Mar 31 – Apr 9

Subsystems (8)

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Wei Fang

author maintainer
#0 lore raw

v4 cover Add preliminary NETC switch support for i.MX94

i.MX94 NETC (v4.3) integrates 802.1Q Ethernet switch functionality, the switch provides advanced QoS with 8 traffic classes and a full range of TSN standards capabilities. It has 3 user ports and 1 CPU port, and the CPU port is connected to an internal ENETC through the pseduo link, so instead of a back-to-back MAC, the lightweight "pseudo MAC" is used at both ends of the pseudo link to transfer Ethernet frames. The pseudo link provides a zero-copy interface (no serialization delay) and lower power (less logic and memory).

Like most Ethernet switches, the NETC switch also supports a proprietary switch tag, is used to carry in-band metadata information about frames. This in-band metadata information can include the source port from which the frame was received, what was the reason why this frame got forwarded to the entity, and for the entity to indicate the precise destination port of a frame. The NETC switch tag is added to frames after the source MAC address. There are three types of switch tags, and each type has 1 to 4 subtypes, more details are as follows.

Forward switch tag (Type = 0): Represents forwarded frames.
- SubType = 0 - Normal frame processing.

To_Port switch tag (Type = 1): Represents frames that are to be sent to a specific switch port.
- SubType = 0. No request to perform timestamping.
- SubType = 1. Request to perform one-step timestamping.
- SubType = 2. Request to perform two-step timestamping.
- SubType = 3. Request to perform both one-step timestamping and two-step timestamping.

To_Host switch tag (Type = 2): Represents frames redirected or copied to the switch management port.
- SubType = 0. Received frames redirected or copied to the switch management port.
- SubType = 1. Received frames redirected or copied to the switch management port with captured timestamp at the switch port where the frame was received.
- SubType = 2. Transmit timestamp response (two-step timestamping).

Currently, this patch set supports Forward tag, SubType 0 of To_Port tag and SubType 0 of To_Host tag. More tags will be supported in the future.

In addition, the switch supports NETC Table Management Protocol (NTMP), some switch functionality is controlled using control messages sent to the hardware using BD ring interface with 32B descriptors similar to the packet Transmit BD ring used on ENETC. This interface is referred to as the command BD ring. This is used to configure functionality where the underlying resources may be shared between different entities or being too large to configure using direct registers.

For this patch set, we have supported the following tables through the command BD ring interface.

FDB Table: It contains forwarding and/or filtering information about MAC addresses. The FDB table is used for MAC learning lookups and MAC forwarding lookups.

VLAN Filter Table: It contains configuration and control information for each VLAN configured on the switch.

Buffer Pool Table: It contains buffer pool configuration and operational information. Each entry corresponds to a buffer pool. Currently, we use this table to implement flow control feature on each port.

Ingress Port Filter Table: It contains a set of filters each capable of classifying incoming traffic using a mix of L2, L3, and L4 parsed and arbitrary field data. We use this table to implement host flood support to the switch port.

The switch also supports other tables, and we will add more advanced features through them in the future.

---
v4:
1. Fix some nits in patch 2.
2. Revert netc_ipv_to_buffer_pool_mapping() to v2, the new added code in v3 is actually a dead code and was only added to address false positives reported by AI review. Therefore, v4 optimizes the comments in netc_switch_bpt_default_config() to avoid the false positives.
v3 link: https://lore.kernel.org/imx/[email protected]/
v2 link: https://lore.kernel.org/imx/[email protected]/
v1 link: https://lore.kernel.org/imx/[email protected]/
---

Wei Fang (14):
dt-bindings: net: dsa: update the description of 'dsa,member' property
dt-bindings: net: dsa: add NETC switch
net: enetc: add pre-boot initialization for i.MX94 switch
net: enetc: add basic operations to the FDB table
net: enetc: add support for the "Add" operation to VLAN filter table
net: enetc: add support for the "Update" operation to buffer pool
table
net: enetc: add support for "Add" and "Delete" operations to IPFT
net: enetc: add multiple command BD rings support
net: dsa: add NETC switch tag support
net: dsa: netc: introduce NXP NETC switch driver for i.MX94
net: dsa: netc: add phylink MAC operations
net: dsa: netc: add more basic functions support
net: dsa: netc: initialize buffer bool table and implement
flow-control
net: dsa: netc: add support for the standardized counters

 .../devicetree/bindings/net/dsa/dsa.yaml      |    6 +-
 .../bindings/net/dsa/nxp,netc-switch.yaml     |  130 ++
 MAINTAINERS                                   |   11 +
 drivers/net/dsa/Kconfig                       |    3 +
 drivers/net/dsa/Makefile                      |    1 +
 drivers/net/dsa/netc/Kconfig                  |   14 +
 drivers/net/dsa/netc/Makefile                 |    3 +
 drivers/net/dsa/netc/netc_ethtool.c           |  192 +++
 drivers/net/dsa/netc/netc_main.c              | 1528 +++++++++++++++++
 drivers/net/dsa/netc/netc_platform.c          |   90 +
 drivers/net/dsa/netc/netc_switch.h            |  155 ++
 drivers/net/dsa/netc/netc_switch_hw.h         |  335 ++++
 .../ethernet/freescale/enetc/netc_blk_ctrl.c  |  188 +-
 drivers/net/ethernet/freescale/enetc/ntmp.c   |  383 ++++-
 .../ethernet/freescale/enetc/ntmp_private.h   |  122 +-
 include/linux/dsa/tag_netc.h                  |   14 +
 include/linux/fsl/netc_global.h               |    6 +
 include/linux/fsl/ntmp.h                      |  235 ++-
 include/net/dsa.h                             |    2 +
 include/uapi/linux/if_ether.h                 |    1 +
 net/dsa/Kconfig                               |   10 +
 net/dsa/Makefile                              |    1 +
 net/dsa/tag_netc.c                            |  185 ++
 23 files changed, 3585 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml
 create mode 100644 drivers/net/dsa/netc/Kconfig
 create mode 100644 drivers/net/dsa/netc/Makefile
 create mode 100644 drivers/net/dsa/netc/netc_ethtool.c
 create mode 100644 drivers/net/dsa/netc/netc_main.c
 create mode 100644 drivers/net/dsa/netc/netc_platform.c
 create mode 100644 drivers/net/dsa/netc/netc_switch.h
 create mode 100644 drivers/net/dsa/netc/netc_switch_hw.h
 create mode 100644 include/linux/dsa/tag_netc.h
 create mode 100644 net/dsa/tag_netc.c
Signature · 1 line
2.34.1
author maintainer
#1 lore raw

PATCH v4 1/14 dt-bindings: net: dsa: update the description of 'dsa,member' property

The current description indicates that the 'dsa,member' property cannot be set for a switch that is not part of any cluster. Vladimir thinks that this is a case where the actual technical limitation was poorly transposed into words when this restriction was first documented, in commit 8c5ad1d6179d ("net: dsa: Document new binding").

The true technical limitation is that many DSA tagging protocols are topology-unaware, and always call dsa_conduit_find_user() with a switch_id of 0. Specifying a custom "dsa,member" property with a non-zero switch_id would break them.

Therefore, for topology-aware switches, it is fine to specify this property for them, even if they are not part of any cluster. Our NETC switch is a good example which is topology-aware, the switch_id is carried in the switch tag, but the switch_id 0 is reserved for VEPA switch and cannot be used, so we need to use this property to assign a non-zero switch_id for it.

---
 Documentation/devicetree/bindings/net/dsa/dsa.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/net/dsa/dsa.yaml +5 -1
index 2abd036578d1..801e1411e5c2 100644
@@ -28,7 +28,11 @@ properties:
       A two element list indicates which DSA cluster, and position within the
       cluster a switch takes. <0 0> is cluster 0, switch 0. <0 1> is cluster 0,
       switch 1. <1 0> is cluster 1, switch 0. A switch not part of any cluster
-      (single device hanging off a CPU port) must not specify this property
+      (single device hanging off a CPU port) does not usually need to specify
+      this property, and then it becomes cluster 0, switch 0. For a topology
+      aware switch, its switch index can be specified through this property,
+      even if it is not part of any cluster. Also, topology-unaware switches
+      must always be defined as index 0 of their cluster.
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
 additionalProperties: true
Signature · 1 line
2.34.1
maintainer Acked-by on v4 1/14
#2 lore raw

Re: [PATCH v4 net-next 01/14] dt-bindings: net: dsa: update the description of 'dsa,member' property

Quoted 18 lines · Wei Fang

The current description indicates that the 'dsa,member' property cannot be set for a switch that is not part of any cluster. Vladimir thinks that this is a case where the actual technical limitation was poorly transposed into words when this restriction was first documented, in commit 8c5ad1d6179d ("net: dsa: Document new binding").

The true technical limitation is that many DSA tagging protocols are topology-unaware, and always call dsa_conduit_find_user() with a switch_id of 0. Specifying a custom "dsa,member" property with a non-zero switch_id would break them.

Therefore, for topology-aware switches, it is fine to specify this property for them, even if they are not part of any cluster. Our NETC switch is a good example which is topology-aware, the switch_id is carried in the switch tag, but the switch_id 0 is reserved for VEPA switch and cannot be used, so we need to use this property to assign a non-zero switch_id for it.

Quoted 6 lines
---
 Documentation/devicetree/bindings/net/dsa/dsa.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
author maintainer
#3 lore raw

PATCH v4 2/14 dt-bindings: net: dsa: add NETC switch

In reply to Wei Fang

Add bindings for NETC switch. This switch is a PCIe function of NETC IP, it supports advanced QoS with 8 traffic classes and 4 drop resilience levels, and a full range of TSN standards capabilities. The switch CPU port connects to an internal ENETC port, which is also a PCIe function of NETC IP. So these two ports use a light-weight "pseudo MAC" instead of a back-to-back MAC, because the "pseudo MAC" provides the delineation between switch and ENETC, this translates to lower power (less logic and memory) and lower delay (as there is no serialization delay across this link).

---
 .../bindings/net/dsa/nxp,netc-switch.yaml     | 130 ++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +130 -0
new file mode 100644
index 000000000000..5577f3ef987f
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/nxp,netc-switch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NETC Switch family
+
+description: >
+  The NETC presents itself as a multi-function PCIe Root Complex Integrated
+  Endpoint (RCiEP) and provides full 802.1Q Ethernet switch functionality,
+  advanced QoS with 8 traffic classes and 4 drop resilience levels, and a
+  full range of TSN standards capabilities.
+
+  The CPU port of the switch connects to an internal ENETC. The switch and
+  the internal ENETC are fully integrated into the NETC IP, a back-to-back
+  MAC is not required. Instead, a light-weight "pseudo MAC" provides the
+  delineation between the switch and ENETC. This translates to lower power
+  (less logic and memory) and lower delay (as there is no serialization
+  delay across this link).
+
+maintainers:
+  - Wei Fang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - pci1131,eef2
+
+  reg:
+    maxItems: 1
+
+  dsa,member:
+    description: >
+      The property indicates DSA cluster and switch index. For NETC switch,
+      the valid range of the switch index is 1 ~ 7, the index is reflected
+      in the switch tag as an indication of the switch ID where the frame
+      originated. The value 0 is reserved for ENETC VEPA switch, whose ID
+      is hardwired to zero.
+
+$ref: dsa.yaml#
+
+patternProperties:
+  "^(ethernet-)?ports$":
+    type: object
+    additionalProperties: true
+    patternProperties:
+      "^(ethernet-)?port@[0-9a-f]$":
+        type: object
+
+        $ref: dsa-port.yaml#
+
+        properties:
+          clocks:
+            items:
+              - description: MAC transmit/receive reference clock.
+
+          clock-names:
+            items:
+              - const: ref
+
+          mdio:
+            $ref: /schemas/net/mdio.yaml#
+            unevaluatedProperties: false
+            description:
+              Optional child node for switch port, otherwise use NETC EMDIO.
+
+        unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - dsa,member
+
+allOf:
+  - $ref: /schemas/pci/pci-device.yaml
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pcie {
+        #address-cells = <3>;
+        #size-cells = <2>;
+
+        ethernet-switch@0,2 {
+            compatible = "pci1131,eef2";
+            reg = <0x200 0 0 0 0>;
+            dsa,member = <0 1>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_switch>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    phy-handle = <&ethphy0>;
+                    phy-mode = "mii";
+                };
+
+                port@1 {
+                    reg = <1>;
+                    phy-handle = <&ethphy1>;
+                    phy-mode = "mii";
+                };
+
+                port@2 {
+                    reg = <2>;
+                    clocks = <&scmi_clk 103>;
+                    clock-names = "ref";
+                    phy-handle = <&ethphy2>;
+                    phy-mode = "rgmii-id";
+                };
+
+                port@3 {
+                    reg = <3>;
+                    ethernet = <&enetc3>;
+                    phy-mode = "internal";
+
+                    fixed-link {
+                        speed = <2500>;
+                        full-duplex;
+                        pause;
+                    };
+                };
+            };
+        };
+    };
Signature · 1 line
2.34.1
maintainer
#4 lore raw

Re: [PATCH v4 net-next 02/14] dt-bindings: net: dsa: add NETC switch

Quoted 54 lines · Wei Fang

Add bindings for NETC switch. This switch is a PCIe function of NETC IP, it supports advanced QoS with 8 traffic classes and 4 drop resilience levels, and a full range of TSN standards capabilities. The switch CPU port connects to an internal ENETC port, which is also a PCIe function of NETC IP. So these two ports use a light-weight "pseudo MAC" instead of a back-to-back MAC, because the "pseudo MAC" provides the delineation between switch and ENETC, this translates to lower power (less logic and memory) and lower delay (as there is no serialization delay across this link).

---
 .../bindings/net/dsa/nxp,netc-switch.yaml     | 130 ++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +31 -0
new file mode 100644
index 000000000000..5577f3ef987f
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/nxp,netc-switch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NETC Switch family
+
+description: >
+  The NETC presents itself as a multi-function PCIe Root Complex Integrated
+  Endpoint (RCiEP) and provides full 802.1Q Ethernet switch functionality,
+  advanced QoS with 8 traffic classes and 4 drop resilience levels, and a
+  full range of TSN standards capabilities.
+
+  The CPU port of the switch connects to an internal ENETC. The switch and
+  the internal ENETC are fully integrated into the NETC IP, a back-to-back
+  MAC is not required. Instead, a light-weight "pseudo MAC" provides the
+  delineation between the switch and ENETC. This translates to lower power
+  (less logic and memory) and lower delay (as there is no serialization
+  delay across this link).
+
+maintainers:
+  - Wei Fang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - pci1131,eef2
+
+  reg:
+    maxItems: 1

Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml line 41

Quoted 10 lines
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +10 -0
+
+  dsa,member:
+    description: >
+      The property indicates DSA cluster and switch index. For NETC switch,
+      the valid range of the switch index is 1 ~ 7, the index is reflected
+      in the switch tag as an indication of the switch ID where the frame
+      originated. The value 0 is reserved for ENETC VEPA switch, whose ID
+      is hardwired to zero.
+
+$ref: dsa.yaml#

Move this under the 'allOf' so all $ref's are together.

Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml line 44

Quoted 3 lines
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +3 -0
+
+patternProperties:
+  "^(ethernet-)?ports$":

'ethernet-ports' for new bindings.

Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml line 48

Quoted 4 lines
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +4 -0
+    type: object
+    additionalProperties: true
+    patternProperties:
+      "^(ethernet-)?port@[0-9a-f]$":

And 'ethernet-port'

Quoted 85 lines
Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml new file +82 -0
+        type: object
+
+        $ref: dsa-port.yaml#
+
+        properties:
+          clocks:
+            items:
+              - description: MAC transmit/receive reference clock.
+
+          clock-names:
+            items:
+              - const: ref
+
+          mdio:
+            $ref: /schemas/net/mdio.yaml#
+            unevaluatedProperties: false
+            description:
+              Optional child node for switch port, otherwise use NETC EMDIO.
+
+        unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - dsa,member
+
+allOf:
+  - $ref: /schemas/pci/pci-device.yaml
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    pcie {
+        #address-cells = <3>;
+        #size-cells = <2>;
+
+        ethernet-switch@0,2 {
+            compatible = "pci1131,eef2";
+            reg = <0x200 0 0 0 0>;
+            dsa,member = <0 1>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_switch>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    phy-handle = <&ethphy0>;
+                    phy-mode = "mii";
+                };
+
+                port@1 {
+                    reg = <1>;
+                    phy-handle = <&ethphy1>;
+                    phy-mode = "mii";
+                };
+
+                port@2 {
+                    reg = <2>;
+                    clocks = <&scmi_clk 103>;
+                    clock-names = "ref";
+                    phy-handle = <&ethphy2>;
+                    phy-mode = "rgmii-id";
+                };
+
+                port@3 {
+                    reg = <3>;
+                    ethernet = <&enetc3>;
+                    phy-mode = "internal";
+
+                    fixed-link {
+                        speed = <2500>;
+                        full-duplex;
+                        pause;
+                    };
+                };
+            };
+        };
+    };
Signature · 1 line
2.34.1
author maintainer
#5 lore raw

PATCH v4 3/14 net: enetc: add pre-boot initialization for i.MX94 switch

In reply to Wei Fang

Before probing the NETC switch driver, some pre-initialization needs to be set in NETCMIX and IERB to ensure that the switch can work properly. For example, i.MX94 NETC switch has three external ports and each port is bound to a link. And each link needs to be configured so that it can work properly, such as I/O variant and MII protocol.

In addition, the switch port 2 (MAC 2) and ENETC 0 (MAC 3) share the same parallel interface, they cannot be used at the same time due to the SoC constraint. And the MAC selection is controlled by the mac2_mac3_sel bit of EXT_PIN_CONTROL register. Currently, the interface is set for ENETC 0 by default unless the switch port 2 is enabled in the DT node.

Like ENETC, each external port of the NETC switch can manage its external PHY through its port MDIO registers. And the port can only access its own external PHY by setting the PHY address to the LaBCR[MDIO_PHYAD_PRTAD]. If the accessed PHY address is not equal to LaBCR[MDIO_PHYAD_PRTAD], then the MDIO access initiated by port MDIO will be invalid.

---
 .../ethernet/freescale/enetc/netc_blk_ctrl.c  | 188 ++++++++++++++++--
 1 file changed, 166 insertions(+), 22 deletions(-)
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +166 -22
index 92a0f824dae7..c21230f7a7fb 100644
@@ -261,40 +261,112 @@ static int imx94_link_config(struct netc_blk_ctrl *priv,
 }
 
 static int imx94_enetc_link_config(struct netc_blk_ctrl *priv,
-				   struct device_node *np)
+				   struct device_node *np,
+				   bool *enetc0_en)
 {
 	int link_id = imx94_enetc_get_link_id(np);
 
 	if (link_id < 0)
 		return link_id;
 
+	if (link_id == IMX94_ENETC0_LINK && of_device_is_available(np))
+		*enetc0_en = true;
+
 	return imx94_link_config(priv, np, link_id);
 }
 
+static struct device_node *netc_get_switch_ports(struct device_node *np)
+{
+	struct device_node *ports;
+
+	ports = of_get_child_by_name(np, "ports");
+	if (!ports)
+		ports = of_get_child_by_name(np, "ethernet-ports");
+
+	return ports;
+}
+
+static int imx94_switch_link_config(struct netc_blk_ctrl *priv,
+				    struct device_node *np,
+				    bool *swp2_en)
+{
+	struct device_node *ports;
+	int port_id, err = 0;
+
+	ports = netc_get_switch_ports(np);
+	if (!ports)
+		return -ENODEV;
+
+	for_each_available_child_of_node_scoped(ports, child) {
+		if (of_property_read_u32(child, "reg", &port_id) < 0) {
+			err = -ENODEV;
+			goto end;
+		}
+
+		switch (port_id) {
+		case 0 ... 2: /* External ports */
+			err = imx94_link_config(priv, child, port_id);
+			if (err)
+				goto end;
+
+			if (port_id == 2)
+				*swp2_en = true;
+
+			break;
+		case 3: /* CPU port */
+			break;
+		default:
+			err = -EINVAL;
+			goto end;
+		}
+	}
+
+end:
+	of_node_put(ports);
+
+	return err;
+}
+
 static int imx94_netcmix_init(struct platform_device *pdev)
 {
 	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
+	bool enetc0_en = false, swp2_en = false;
 	u32 val;
 	int err;
 
 	for_each_child_of_node_scoped(np, child) {
 		for_each_child_of_node_scoped(child, gchild) {
-			if (!of_device_is_compatible(gchild, "pci1131,e101"))
-				continue;
-
-			err = imx94_enetc_link_config(priv, gchild);
-			if (err)
-				return err;
+			if (of_device_is_compatible(gchild, "pci1131,e101")) {
+				err = imx94_enetc_link_config(priv, gchild,
+							      &enetc0_en);
+				if (err)
+					return err;
+			} else if (of_device_is_compatible(gchild,
+							   "pci1131,eef2")) {
+				err = imx94_switch_link_config(priv, gchild,
+							       &swp2_en);
+				if (err)
+					return err;
+			}
 		}
 	}
 
-	/* ENETC 0 and switch port 2 share the same parallel interface.
-	 * Currently, the switch is not supported, so this interface is
-	 * used by ENETC 0 by default.
+	if (enetc0_en && swp2_en) {
+		dev_err(&pdev->dev,
+			"Cannot enable swp2 and enetc0 at the same time\n");
+		return -EINVAL;
+	}
+
+	/* ENETC 0 and switch port 2 share the same parallel interface, they
+	 * cannot be enabled at the same time. The interface is set for the
+	 * ENETC 0 by default unless the switch port 2 is enabled in the DTS.
 	 */
 	val = netc_reg_read(priv->netcmix, IMX94_EXT_PIN_CONTROL);
-	val |= MAC2_MAC3_SEL;
+	if (!swp2_en)
+		val |= MAC2_MAC3_SEL;
+	else
+		val &= ~MAC2_MAC3_SEL;
 	netc_reg_write(priv->netcmix, IMX94_EXT_PIN_CONTROL, val);
 
 	return 0;
@@ -610,6 +682,77 @@ static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
 	return 0;
 }
 
+static int imx94_ierb_enetc_init(struct netc_blk_ctrl *priv,
+				 struct device_node *np,
+				 u32 phy_mask)
+{
+	int err;
+
+	err = imx94_enetc_update_tid(priv, np);
+	if (err)
+		return err;
+
+	return imx94_enetc_mdio_phyaddr_config(priv, np, phy_mask);
+}
+
+static int imx94_switch_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
+					    struct device_node *np,
+					    int port_id, u32 phy_mask)
+{
+	int addr;
+
+	/* The switch has 3 external ports at most */
+	if (port_id > 2)
+		return 0;
+
+	addr = netc_get_phy_addr(np);
+	if (addr < 0) {
+		if (addr == -ENODEV)
+			return 0;
+
+		return addr;
+	}
+
+	if (phy_mask & BIT(addr)) {
+		dev_err(&priv->pdev->dev,
+			"Found same PHY address in EMDIO and switch node\n");
+		return -EINVAL;
+	}
+
+	netc_reg_write(priv->ierb, IERB_LBCR(port_id),
+		       LBCR_MDIO_PHYAD_PRTAD(addr));
+
+	return 0;
+}
+
+static int imx94_ierb_switch_init(struct netc_blk_ctrl *priv,
+				  struct device_node *np,
+				  u32 phy_mask)
+{
+	struct device_node *ports;
+	int port_id, err = 0;
+
+	ports = netc_get_switch_ports(np);
+	if (!ports)
+		return -ENODEV;
+
+	for_each_available_child_of_node_scoped(ports, child) {
+		err = of_property_read_u32(child, "reg", &port_id);
+		if (err)
+			goto end;
+
+		err = imx94_switch_mdio_phyaddr_config(priv, child,
+						       port_id, phy_mask);
+		if (err)
+			goto end;
+	}
+
+end:
+	of_node_put(ports);
+
+	return err;
+}
+
 static int imx94_ierb_init(struct platform_device *pdev)
 {
 	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
@@ -625,17 +768,18 @@ static int imx94_ierb_init(struct platform_device *pdev)
 
 	for_each_child_of_node_scoped(np, child) {
 		for_each_child_of_node_scoped(child, gchild) {
-			if (!of_device_is_compatible(gchild, "pci1131,e101"))
-				continue;
-
-			err = imx94_enetc_update_tid(priv, gchild);
-			if (err)
-				return err;
-
-			err = imx94_enetc_mdio_phyaddr_config(priv, gchild,
-							      phy_mask);
-			if (err)
-				return err;
+			if (of_device_is_compatible(gchild, "pci1131,e101")) {
+				err = imx94_ierb_enetc_init(priv, gchild,
+							    phy_mask);
+				if (err)
+					return err;
+			} else if (of_device_is_compatible(gchild,
+							   "pci1131,eef2")) {
+				err = imx94_ierb_switch_init(priv, gchild,
+							     phy_mask);
+				if (err)
+					return err;
+			}
 		}
 	}
 
Signature · 1 line
2.34.1
maintainer
#6 lore raw

Re: [PATCH v4 net-next 03/14] net: enetc: add pre-boot initialization for i.MX94 switch

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: enetc: add pre-boot initialization for i.MX94 switch

This commit adds pre-boot initialization logic for the i.MX94 switch in the NETC block control driver. It configures external ports, MII protocols, and MDIO PHY addresses, and handles the MAC selection constraint between switch port 2 and ENETC 0.

Quoted 4 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +0 -0
index 92a0f824dae7..c21230f7a7fb 100644

[ ... ]

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 294

Quoted 6 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +6 -0
+static int imx94_switch_link_config(struct netc_blk_ctrl *priv,
+				    struct device_node *np,
+				    bool *swp2_en)
+{
+	struct device_node *ports;
+	int port_id, err = 0;

Since of_property_read_u32() expects a u32 pointer, does passing a signed int pointer here trigger an incompatible pointer types warning during compilation?

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 301

Quoted 7 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +7 -0
+
+	ports = netc_get_switch_ports(np);
+	if (!ports)
+		return -ENODEV;
+
+	for_each_available_child_of_node_scoped(ports, child) {
+		if (of_property_read_u32(child, "reg", &port_id) < 0) {

If the device tree provides a reg value with the MSB set, port_id would be evaluated as a negative number.

[ ... ]

Quoted 13 lines

static int imx94_netcmix_init(struct platform_device *pdev) {

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +1 -3
 	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
+	bool enetc0_en = false, swp2_en = false;
 	u32 val;
 	int err;
 
 	for_each_child_of_node_scoped(np, child) {
 		for_each_child_of_node_scoped(child, gchild) {
-			if (!of_device_is_compatible(gchild, "pci1131,e101"))
-				continue;
-

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 346

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +7 -3
-			err = imx94_enetc_link_config(priv, gchild);
-			if (err)
-				return err;
+			if (of_device_is_compatible(gchild, "pci1131,e101")) {
+				err = imx94_enetc_link_config(priv, gchild,
+							      &enetc0_en);
+				if (err)
+					return err;
+			} else if (of_device_is_compatible(gchild,
+							   "pci1131,eef2")) {

Is it possible for the switch node to be disabled in the device tree?

If so, should we check of_device_is_available() for gchild before proceeding, similar to how it is checked for enetc0_en?

Without checking availability, if the disabled switch lacks a ports subnode, imx94_switch_link_config() might return -ENODEV and abort the entire probe.

Alternatively, if the disabled switch node has a ports subnode with an available port 2, it could spuriously set swp2_en = true and cause a false conflict with ENETC 0.

Quoted 7 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +5 -0
+				err = imx94_switch_link_config(priv, gchild,
+							       &swp2_en);
+				if (err)
+					return err;
+			}
 		}
 	}

[ ... ]

Quoted 5 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +1 -0
@@ -610,6 +682,77 @@ static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
 	return 0;
 }
 
+static int imx94_ierb_enetc_init(struct netc_blk_ctrl *priv,

[ ... ]

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv, · line 706

Quoted 9 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +9 -0
+static int imx94_switch_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
+					    struct device_node *np,
+					    int port_id, u32 phy_mask)
+{
+	int addr;
+
+	/* The switch has 3 external ports at most */
+	if (port_id > 2)
+		return 0;

If port_id is negative due to the signed type, it will bypass this upper bound check.

Quoted 7 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +7 -0
+
+	addr = netc_get_phy_addr(np);
+	if (addr < 0) {
+		if (addr == -ENODEV)
+			return 0;
+
+		return addr;

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv, · line 723

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +10 -0
+	}
+
+	if (phy_mask & BIT(addr)) {
+		dev_err(&priv->pdev->dev,
+			"Found same PHY address in EMDIO and switch node\n");
+		return -EINVAL;
+	}
+
+	netc_reg_write(priv->ierb, IERB_LBCR(port_id),
+		       LBCR_MDIO_PHYAD_PRTAD(addr));

Could this lead to an out-of-bounds register write prior to the array base since port_id is negative?

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv, · line 733

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +10 -0
+
+	return 0;
+}
+
+static int imx94_ierb_switch_init(struct netc_blk_ctrl *priv,
+				  struct device_node *np,
+				  u32 phy_mask)
+{
+	struct device_node *ports;
+	int port_id, err = 0;

A similar signed int declaration is used here for port_id, which would run into the same pointer type warning and negative value issues.

Quoted 4 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +4 -0
+
+	ports = netc_get_switch_ports(np);
+	if (!ports)
+		return -ENODEV;

[ ... ]

Signature · 1 line
pw-bot: cr
author maintainer
#7 lore raw

RE: [PATCH v4 net-next 03/14] net: enetc: add pre-boot initialization for i.MX94 switch

Quoted 11 lines

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 294

Quoted 6 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +6 -0
+static int imx94_switch_link_config(struct netc_blk_ctrl *priv,
+				    struct device_node *np,
+				    bool *swp2_en)
+{
+	struct device_node *ports;
+	int port_id, err = 0;

Since of_property_read_u32() expects a u32 pointer, does passing a signed int pointer here trigger an incompatible pointer types warning during compilation?

Okay, I will change port_id to u32 type.

Quoted 38 lines

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 301

Quoted 7 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +7 -0
+
+	ports = netc_get_switch_ports(np);
+	if (!ports)
+		return -ENODEV;
+
+	for_each_available_child_of_node_scoped(ports, child) {
+		if (of_property_read_u32(child, "reg", &port_id) < 0) {

If the device tree provides a reg value with the MSB set, port_id would be evaluated as a negative number.

[ ... ]

Quoted 13 lines

static int imx94_netcmix_init(struct platform_device *pdev) {

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +1 -3
 	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
+	bool enetc0_en = false, swp2_en = false;
 	u32 val;
 	int err;
 
 	for_each_child_of_node_scoped(np, child) {
 		for_each_child_of_node_scoped(child, gchild) {
-			if (!of_device_is_compatible(gchild, "pci1131,e101"))
-				continue;
-

drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c static int imx94_link_config(struct netc_blk_ctrl *priv, · line 346

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +7 -3
-			err = imx94_enetc_link_config(priv, gchild);
-			if (err)
-				return err;
+			if (of_device_is_compatible(gchild, "pci1131,e101")) {
+				err = imx94_enetc_link_config(priv, gchild,
+							      &enetc0_en);
+				if (err)
+					return err;
+			} else if (of_device_is_compatible(gchild,
+							   "pci1131,eef2")) {

Is it possible for the switch node to be disabled in the device tree?

Quoted 2 lines

If so, should we check of_device_is_available() for gchild before proceeding, similar to how it is checked for enetc0_en?

The switch may be owned by a guest OS, in this case, the switch node of the host OS will be disabled, but we still hope that the host OS could do some configurations for the switch, as the netc_blk_ctrl is owned by host OS, so we cannot check of_device_is_available() for the switch node.

Quoted 3 lines

Without checking availability, if the disabled switch lacks a ports subnode, imx94_switch_link_config() might return -ENODEV and abort the entire probe.

This is the expected behavior, only one of swp2 and enetc0 can be enabled, as they share the same parallel interface. If all of them are enabled, the driver will detect this incorrect DTS configuration and return a failure.

Quoted 11 lines

Alternatively, if the disabled switch node has a ports subnode with an available port 2, it could spuriously set swp2_en = true and cause a false conflict with ENETC 0.

Quoted 7 lines
drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c +5 -0
+				err = imx94_switch_link_config(priv, gchild,
+							       &swp2_en);
+				if (err)
+					return err;
+			}
 		}
 	}
author maintainer
#8 lore raw

PATCH v4 4/14 net: enetc: add basic operations to the FDB table

In reply to Wei Fang

The FDB table is used for MAC learning lookups and MAC forwarding lookups. Each table entry includes information such as a FID and MAC address that may be unicast or multicast and a forwarding destination field containing a port bitmap identifying the associated port(s) with the MAC address. FDB table entries can be static or dynamic. Static entries are added from software whereby dynamic entries are added either by software or by the hardware as MAC addresses are learned in the datapath.

The FDB table can only be managed by the command BD ring using table management protocol version 2.0. Table management command operations Add, Delete, Update and Query are supported. And the FDB table supports three access methods: Entry ID, Exact Match Key Element and Search. This patch adds the following basic supports to the FDB table.

ntmp_fdbt_update_entry() - update the configuration element data of a specified FDB entry

ntmp_fdbt_delete_entry() - delete a specified FDB entry

ntmp_fdbt_add_entry() - add an entry into the FDB table

ntmp_fdbt_search_port_entry() - Search the FDB entry on the specified port based on RESUME_ENTRY_ID.

---
 drivers/net/ethernet/freescale/enetc/ntmp.c   | 201 +++++++++++++++++-
 .../ethernet/freescale/enetc/ntmp_private.h   |  61 +++++-
 include/linux/fsl/ntmp.h                      |  69 +++++-
 3 files changed, 328 insertions(+), 3 deletions(-)
drivers/net/ethernet/freescale/enetc/ntmp.c +200 -1
index 703752995e93..a4a99954baf2 100644
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
 /*
  * NETC NTMP (NETC Table Management Protocol) 2.0 Library
- * Copyright 2025 NXP
+ * Copyright 2025-2026 NXP
  */
 
 #include <linux/dma-mapping.h>
@@ -20,11 +20,15 @@
 /* Define NTMP Table ID */
 #define NTMP_MAFT_ID			1
 #define NTMP_RSST_ID			3
+#define NTMP_FDBT_ID			15
 
 /* Generic Update Actions for most tables */
 #define NTMP_GEN_UA_CFGEU		BIT(0)
 #define NTMP_GEN_UA_STSEU		BIT(1)
 
+/* Query Action: 0: Full query, 1: Only query entry ID */
+#define NTMP_QA_ENTRY_ID		1
+
 #define NTMP_ENTRY_ID_SIZE		4
 #define RSST_ENTRY_NUM			64
 #define RSST_STSE_DATA_SIZE(n)		((n) * 8)
@@ -225,6 +229,8 @@ static const char *ntmp_table_name(int tbl_id)
 		return "MAC Address Filter Table";
 	case NTMP_RSST_ID:
 		return "RSS Table";
+	case NTMP_FDBT_ID:
+		return "FDB Table";
 	default:
 		return "Unknown Table";
 	}
@@ -453,5 +459,198 @@ int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count)
 }
 EXPORT_SYMBOL_GPL(ntmp_rsst_query_entry);
 
+/**
+ * ntmp_fdbt_add_entry - add an entry into the FDB table
+ * @user: target ntmp_user struct
+ * @entry_id: returned value, the entry ID of the new added entry
+ * @keye: key element data
+ * @cfge: configuration element data
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_fdbt_add_entry(struct ntmp_user *user, u32 *entry_id,
+			const struct fdbt_keye_data *keye,
+			const struct fdbt_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct fdbt_req_ua),
+	};
+	struct fdbt_resp_query *resp;
+	struct fdbt_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.fdbt_ver, NTMP_QA_ENTRY_ID,
+		      NTMP_GEN_UA_CFGEU);
+	req->ak.exact.keye = *keye;
+	req->cfge = *cfge;
+
+	len = NTMP_LEN(data.size, sizeof(*resp));
+	/* The entry ID is allotted by hardware, so we need to perform
+	 * a query action after the add action to get the entry ID from
+	 * hardware.
+	 */
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_FDBT_ID,
+			      NTMP_CMD_AQ, NTMP_AM_EXACT_KEY);
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err) {
+		dev_err(user->dev, "Failed to add %s entry, err: %pe\n",
+			ntmp_table_name(NTMP_FDBT_ID), ERR_PTR(err));
+		goto end;
+	}
+
+	if (entry_id) {
+		resp = (struct fdbt_resp_query *)req;
+		*entry_id = le32_to_cpu(resp->entry_id);
+	}
+
+end:
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_fdbt_add_entry);
+
+/**
+ * ntmp_fdbt_update_entry - update the configuration element data of the
+ * specified FDB entry
+ * @user: target ntmp_user struct
+ * @entry_id: the specified entry ID of the FDB table
+ * @cfge: configuration element data
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_fdbt_update_entry(struct ntmp_user *user, u32 entry_id,
+			   const struct fdbt_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct fdbt_req_ua),
+	};
+	struct fdbt_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.fdbt_ver, 0, NTMP_GEN_UA_CFGEU);
+	req->ak.eid.entry_id = cpu_to_le32(entry_id);
+	req->cfge = *cfge;
+
+	/* Request header */
+	len = NTMP_LEN(data.size, NTMP_STATUS_RESP_LEN);
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_FDBT_ID,
+			      NTMP_CMD_UPDATE, NTMP_AM_ENTRY_ID);
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev, "Failed to update %s entry, err: %pe\n",
+			ntmp_table_name(NTMP_FDBT_ID), ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_fdbt_update_entry);
+
+/**
+ * ntmp_fdbt_delete_entry - delete the specified FDB entry
+ * @user: target ntmp_user struct
+ * @entry_id: the specified ID of the FDB entry
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_fdbt_delete_entry(struct ntmp_user *user, u32 entry_id)
+{
+	u32 req_len = sizeof(struct fdbt_req_qd);
+
+	return ntmp_delete_entry_by_id(user, NTMP_FDBT_ID,
+				       user->tbl.fdbt_ver,
+				       entry_id, req_len,
+				       NTMP_STATUS_RESP_LEN);
+}
+EXPORT_SYMBOL_GPL(ntmp_fdbt_delete_entry);
+
+/**
+ * ntmp_fdbt_search_port_entry - Search the FDB entry on the specified
+ * port based on RESUME_ENTRY_ID
+ * @user: target ntmp_user struct
+ * @port: the specified switch port ID
+ * @resume_entry_id: it is both an input and an output. As an input, it
+ * represents the FDB entry ID to be searched. If it is a NULL entry ID,
+ * it indicates that the first FDB entry for that port is being searched.
+ * As an output, it represents the next FDB entry ID to be searched.
+ * @entry: returned value, the response data of the searched FDB entry
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
+				u32 *resume_entry_id,
+				struct fdbt_entry_data *entry)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct fdbt_req_qd),
+	};
+	struct fdbt_resp_query *resp;
+	struct fdbt_req_qd *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.fdbt_ver, 0, 0);
+	req->ak.search.resume_eid = cpu_to_le32(*resume_entry_id);
+	req->ak.search.cfge.port_bitmap = cpu_to_le32(BIT(port));
+	/* Match CFGE_DATA[PORT_BITMAP] field */
+	req->ak.search.cfge_mc = FDBT_CFGE_MC_PORT_BITMAP;
+
+	/* Request header */
+	len = NTMP_LEN(data.size, sizeof(*resp));
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_FDBT_ID,
+			      NTMP_CMD_QUERY, NTMP_AM_SEARCH);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err) {
+		dev_err(user->dev,
+			"Failed to search %s entry on port %d, err: %pe\n",
+			ntmp_table_name(NTMP_FDBT_ID), port, ERR_PTR(err));
+		goto end;
+	}
+
+	if (!cbd.resp_hdr.num_matched) {
+		entry->entry_id = NTMP_NULL_ENTRY_ID;
+		*resume_entry_id = NTMP_NULL_ENTRY_ID;
+		goto end;
+	}
+
+	resp = (struct fdbt_resp_query *)req;
+	*resume_entry_id = le32_to_cpu(resp->status);
+	entry->entry_id = le32_to_cpu(resp->entry_id);
+	entry->keye = resp->keye;
+	entry->cfge = resp->cfge;
+	entry->acte = resp->acte;
+
+end:
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_fdbt_search_port_entry);
+
 MODULE_DESCRIPTION("NXP NETC Library");
 MODULE_LICENSE("Dual BSD/GPL");
drivers/net/ethernet/freescale/enetc/ntmp_private.h +60 -1

83 lines not on this page · raw message

include/linux/fsl/ntmp.h +68 -1

103 lines not on this page · raw message

Signature · 1 line
2.34.1
maintainer
#9 lore raw

Re: [PATCH v4 net-next 04/14] net: enetc: add basic operations to the FDB table

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: enetc: add basic operations to the FDB table

This commit adds FDB table management operations (Add, Delete, Update, Query/Search) to the enetc driver using NTMP version 2.0. FDB entries are managed via the command BD ring.

Quoted 4 lines
include/linux/fsl/ntmp.h +0 -0
index 916dc4fe7de3..a9f3e6cbf422 100644

[ ... ]

include/linux/fsl/ntmp.h static inline int ntmp_rsst_query_entry(struct ntmp_user *user, · line 166

Quoted 10 lines
include/linux/fsl/ntmp.h +6 -0
@@ -116,6 +158,31 @@ static inline int ntmp_rsst_query_entry(struct ntmp_user *user,
 	return 0;
 }
 
+static inline int ntmp_fdbt_add_entry(struct ntmp_user *user, u32 *entry_id,
+				      const struct fdbt_keye_data *keye,
+				      const struct fdbt_cfge_data *data)
+{
+	return 0;
+}

If CONFIG_NXP_NETC_LIB is disabled, will returning 0 here cause callers to falsely assume the operation succeeded and proceed to use an uninitialized entry_id pointer? Would it be safer to return an error code such as -EOPNOTSUPP for stubs that have output parameters?

[ ... ]

include/linux/fsl/ntmp.h static inline int ntmp_rsst_query_entry(struct ntmp_user *user, · line 184

Quoted 6 lines
include/linux/fsl/ntmp.h +6 -0
+static inline int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
+					      u32 *resume_entry_id,
+					      struct fdbt_entry_data *entry)
+{
+	return 0;
+}

Similar to the above, does returning 0 here leave both resume_entry_id and entry uninitialized while signaling success? Since resume_entry_id is typically used as an iterator, could this lead to an infinite loop or other unpredictable behavior in the caller if they interpret the 0 return as a successful read of valid hardware configuration data?

author maintainer
#10 lore raw

RE: [PATCH v4 net-next 04/14] net: enetc: add basic operations to the FDB table

Quoted 5 lines

Review static inline int ntmp_rsst_query_entry(struct

Quoted 1 line
(quoted diff) +0 -0
@@ -116,6 +158,31 @@ static inline int ntmp_rsst_query_entry(struct

ntmp_user *user,

Quoted 3 lines
include/linux/fsl/ntmp.h +0 -0
 	return 0;
 }
 
Quoted 12 lines

include/linux/fsl/ntmp.h static inline int ntmp_rsst_query_entry(struct ntmp_user *user, · line 166

Quoted 6 lines
include/linux/fsl/ntmp.h +6 -0
+static inline int ntmp_fdbt_add_entry(struct ntmp_user *user, u32 *entry_id,
+				      const struct fdbt_keye_data *keye,
+				      const struct fdbt_cfge_data *data)
+{
+	return 0;
+}

If CONFIG_NXP_NETC_LIB is disabled, will returning 0 here cause callers to falsely assume the operation succeeded and proceed to use an uninitialized entry_id pointer? Would it be safer to return an error code such as -EOPNOTSUPP for stubs that have output parameters?

The ntmp_fdbt_add_entry() is only called by the netc switch driver, and the driver will select CONFIG_NXP_NETC_LIB option, so actually this inline function is not used by any drivers. I suppose such inline functions could be removed. Thanks.

author maintainer
#11 lore raw

PATCH v4 5/14 net: enetc: add support for the "Add" operation to VLAN filter table

In reply to Wei Fang

The VLAN filter table contains configuration and control information for each VLAN configured on the switch. Each VLAN entry includes the VLAN port membership, which FID to use in the FDB lookup, which spanning tree group to use, the egress frame modification actions to apply to a frame exiting form this VLAN, and various configuration and control parameters for this VLAN.

The VLAN filter table can only be managed by the command BD ring using table management protocol version 2.0. The table supports Add, Delete, Update and Query operations. And the table supports 3 access methods:
Entry ID, Exact Match Key Element and Search. But currently we only add the ntmp_vft_add_entry() helper to support the upcoming switch driver to add an entry to the VLAN filter table. Other interfaces will be added in the future.

---
 drivers/net/ethernet/freescale/enetc/ntmp.c   | 50 +++++++++++++++++++
 .../ethernet/freescale/enetc/ntmp_private.h   | 19 +++++++
 include/linux/fsl/ntmp.h                      | 30 +++++++++++
 3 files changed, 99 insertions(+)
drivers/net/ethernet/freescale/enetc/ntmp.c +50 -0
index a4a99954baf2..d7d8a37078d1 100644
@@ -21,6 +21,7 @@
 #define NTMP_MAFT_ID			1
 #define NTMP_RSST_ID			3
 #define NTMP_FDBT_ID			15
+#define NTMP_VFT_ID			18
 
 /* Generic Update Actions for most tables */
 #define NTMP_GEN_UA_CFGEU		BIT(0)
@@ -231,6 +232,8 @@ static const char *ntmp_table_name(int tbl_id)
 		return "RSS Table";
 	case NTMP_FDBT_ID:
 		return "FDB Table";
+	case NTMP_VFT_ID:
+		return "VLAN Filter Table";
 	default:
 		return "Unknown Table";
 	}
@@ -652,5 +655,52 @@ int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
 }
 EXPORT_SYMBOL_GPL(ntmp_fdbt_search_port_entry);
 
+/**
+ * ntmp_vft_add_entry - add an entry into the VLAN filter table
+ * @user: target ntmp_user struct
+ * @vid: VLAN ID
+ * @cfge: configuration element data
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
+		       const struct vft_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct vft_req_ua),
+	};
+	struct vft_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.vft_ver, 0,
+		      NTMP_GEN_UA_CFGEU);
+	req->ak.exact.vid = cpu_to_le16(vid);
+	req->cfge = *cfge;
+
+	/* Request header */
+	len = NTMP_LEN(data.size, NTMP_STATUS_RESP_LEN);
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_VFT_ID,
+			      NTMP_CMD_ADD, NTMP_AM_EXACT_KEY);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev,
+			"Failed to add %s entry, vid: %u, err: %pe\n",
+			ntmp_table_name(NTMP_VFT_ID), vid, ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
+
 MODULE_DESCRIPTION("NXP NETC Library");
 MODULE_LICENSE("Dual BSD/GPL");
drivers/net/ethernet/freescale/enetc/ntmp_private.h +19 -0
index 8999eafe1920..d2a6399b0a36 100644
@@ -160,4 +160,23 @@ struct fdbt_resp_query {
 	u8 resv[3];
 };
 
+/* Access Key Format of VLAN Filter Table */
+struct vft_ak_exact {
+	__le16 vid; /* bit0~11: VLAN ID, other bits are reserved */
+	__le16 resv;
+};
+
+union vft_access_key {
+	__le32 entry_id; /* entry_id match */
+	struct vft_ak_exact exact;
+	__le32 resume_entry_id; /* search */
+};
+
+/* VLAN Filter Table Request Data Buffer Format of Update and Add actions */
+struct vft_req_ua {
+	struct ntmp_cmn_req_data crd;
+	union vft_access_key ak;
+	struct vft_cfge_data cfge;
+};
+
 #endif
include/linux/fsl/ntmp.h +30 -0
index a9f3e6cbf422..2aedea17307f 100644
@@ -32,6 +32,7 @@ struct netc_tbl_vers {
 	u8 maft_ver;
 	u8 rsst_ver;
 	u8 fdbt_ver;
+	u8 vft_ver;
 };
 
 struct netc_cbdr {
@@ -94,6 +95,27 @@ struct fdbt_entry_data {
 #define FDBT_ACT_FLAG		BIT(7)
 };
 
+struct vft_cfge_data {
+	__le32 bitmap_stg;
+#define VFT_PORT_MEMBERSHIP	GENMASK(23, 0)
+#define VFT_STG_ID_MASK		GENMASK(27, 24)
+#define VFT_STG_ID(g)		FIELD_PREP(VFT_STG_ID_MASK, (g))
+	__le16 fid;
+#define VFT_FID			GENMASK(11, 0)
+	__le16 cfg;
+#define VFT_MLO			GENMASK(2, 0)
+#define VFT_MFO			GENMASK(4, 3)
+#define VFT_IPMFE		BIT(6)
+#define VFT_IPMFLE		BIT(7)
+#define VFT_PGA			BIT(8)
+#define VFT_SFDA		BIT(10)
+#define VFT_OSFDA		BIT(11)
+#define VFT_FDBAFSS		BIT(12)
+	__le32 eta_port_bitmap;
+#define VFT_ETA_PORT_BITMAP	GENMASK(23, 0)
+	__le32 et_eid;
+};
+
 #if IS_ENABLED(CONFIG_NXP_NETC_LIB)
 int ntmp_init_cbdr(struct netc_cbdr *cbdr, struct device *dev,
 		   const struct netc_cbdr_regs *regs);
@@ -118,6 +140,8 @@ int ntmp_fdbt_delete_entry(struct ntmp_user *user, u32 entry_id);
 int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
 				u32 *resume_entry_id,
 				struct fdbt_entry_data *entry);
+int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
+		       const struct vft_cfge_data *cfge);
 #else
 static inline int ntmp_init_cbdr(struct netc_cbdr *cbdr, struct device *dev,
 				 const struct netc_cbdr_regs *regs)
@@ -183,6 +207,12 @@ static inline int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
 	return 0;
 }
 
+static inline int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
+				     const struct vft_cfge_data *cfge)
+{
+	return 0;
+}
+
 #endif
 
 #endif
Signature · 1 line
2.34.1
maintainer
#12 lore raw

Re: [PATCH v4 net-next 05/14] net: enetc: add support for the "Add" operation to VLAN filter table

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: enetc: add support for the "Add" operation to VLAN filter table

This patch adds the ntmp_vft_add_entry() helper function to support adding entries to the VLAN filter table. It allocates a DMA buffer, formats the request, and transmits it via the command BD ring using netc_xmit_ntmp_cmd().

Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +0 -0

[ ... ]

Quoted 25 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +24 -0
@@ -652,5 +655,52 @@ int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
+int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
+		       const struct vft_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct vft_req_ua),
+	};
+	struct vft_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.vft_ver, 0,
+		      NTMP_GEN_UA_CFGEU);
+	req->ak.exact.vid = cpu_to_le16(vid);
+	req->cfge = *cfge;
+
+	/* Request header */
+	len = NTMP_LEN(data.size, NTMP_STATUS_RESP_LEN);

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port, · line 688

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +10 -0
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_VFT_ID,
+			      NTMP_CMD_ADD, NTMP_AM_EXACT_KEY);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev,
+			"Failed to add %s entry, vid: %u, err: %pe\n",
+			ntmp_table_name(NTMP_VFT_ID), vid, ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);

Does this code safely handle hardware timeouts?

If netc_xmit_ntmp_cmd() times out and returns an error, the pending transaction is not explicitly aborted before ntmp_free_data_mem() unconditionally frees the DMA buffer.

Could the hardware eventually process the pending command and perform a DMA write of the response to the physical address of the freed buffer?

This might lead to silent memory corruption if that memory has already been reallocated to another subsystem. Would it be safer to explicitly abort the pending DMA transaction, reset the device on timeout, or intentionally delay freeing the buffer until it is proven safe?

Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +3 -0
+
+	return err;
+}
author maintainer
#13 lore raw

RE: [PATCH v4 net-next 05/14] net: enetc: add support for the "Add" operation to VLAN filter table

Quoted 50 lines

Review

Quoted 1 line
(quoted diff) +0 -0

b/drivers/net/ethernet/freescale/enetc/ntmp.c

Quoted 2 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +0 -0

[ ... ]

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_fdbt_search_port_entry(struct ntmp_user

Quoted 1 line
drivers/net/ethernet/freescale/enetc/ntmp.c +0 -0
@@ -652,5 +655,52 @@ int ntmp_fdbt_search_port_entry(struct ntmp_user

*user, int port,

Quoted 24 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +24 -0
+int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
+		       const struct vft_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct vft_req_ua),
+	};
+	struct vft_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	/* Request data */
+	ntmp_fill_crd(&req->crd, user->tbl.vft_ver, 0,
+		      NTMP_GEN_UA_CFGEU);
+	req->ak.exact.vid = cpu_to_le16(vid);
+	req->cfge = *cfge;
+
+	/* Request header */
+	len = NTMP_LEN(data.size, NTMP_STATUS_RESP_LEN);

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port, · line 699

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +10 -0
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_VFT_ID,
+			      NTMP_CMD_ADD, NTMP_AM_EXACT_KEY);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev,
+			"Failed to add %s entry, vid: %u, err: %pe\n",
+			ntmp_table_name(NTMP_VFT_ID), vid, ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);

Does this code safely handle hardware timeouts?

If netc_xmit_ntmp_cmd() times out and returns an error, the pending transaction is not explicitly aborted before ntmp_free_data_mem() unconditionally frees the DMA buffer.

Quoted 7 lines

Could the hardware eventually process the pending command and perform a DMA write of the response to the physical address of the freed buffer?

This might lead to silent memory corruption if that memory has already been reallocated to another subsystem. Would it be safer to explicitly abort the pending DMA transaction, reset the device on timeout, or intentionally delay freeing the buffer until it is proven safe?

This is indeed a potential issue, I will add a patch to fix it.

author maintainer
#14 lore raw

PATCH v4 6/14 net: enetc: add support for the "Update" operation to buffer pool table

In reply to Wei Fang

The buffer pool table contains buffer pool configuration and operational information. Each entry corresponds to a buffer pool. The Entry ID value represents the buffer pool ID to access.

The buffer pool table is a static bounded index table, buffer pools are always present and enabled. It only supports Update and Query operations, This patch only adds ntmp_bpt_update_entry() helper to support updating the specified entry of the buffer pool table. Query action to the table will be added in the future.

---
 drivers/net/ethernet/freescale/enetc/ntmp.c   | 39 +++++++++++++++++++
 .../ethernet/freescale/enetc/ntmp_private.h   |  6 +++
 include/linux/fsl/ntmp.h                      | 32 +++++++++++++++
 3 files changed, 77 insertions(+)
drivers/net/ethernet/freescale/enetc/ntmp.c +39 -0
index d7d8a37078d1..3c59b355c142 100644
@@ -22,11 +22,15 @@
 #define NTMP_RSST_ID			3
 #define NTMP_FDBT_ID			15
 #define NTMP_VFT_ID			18
+#define NTMP_BPT_ID			41
 
 /* Generic Update Actions for most tables */
 #define NTMP_GEN_UA_CFGEU		BIT(0)
 #define NTMP_GEN_UA_STSEU		BIT(1)
 
+/* Specific Update Actions for some tables */
+#define BPT_UA_BPSEU			BIT(1)
+
 /* Query Action: 0: Full query, 1: Only query entry ID */
 #define NTMP_QA_ENTRY_ID		1
 
@@ -234,6 +238,8 @@ static const char *ntmp_table_name(int tbl_id)
 		return "FDB Table";
 	case NTMP_VFT_ID:
 		return "VLAN Filter Table";
+	case NTMP_BPT_ID:
+		return "Buffer Pool Table";
 	default:
 		return "Unknown Table";
 	}
@@ -702,5 +708,38 @@ int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
 }
 EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
 
+int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
+			  const struct bpt_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct bpt_req_update),
+	};
+	struct bpt_req_update *req;
+	union netc_cbd cbd;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd_eid(&req->rbe, user->tbl.bpt_ver, 0,
+			  NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, entry_id);
+	req->cfge = *cfge;
+	ntmp_fill_request_hdr(&cbd, data.dma, NTMP_LEN(data.size, 0),
+			      NTMP_BPT_ID, NTMP_CMD_UPDATE, NTMP_AM_ENTRY_ID);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev,
+			"Failed to update %s entry 0x%x, err: %pe\n",
+			ntmp_table_name(NTMP_BPT_ID), entry_id, ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_bpt_update_entry);
+
 MODULE_DESCRIPTION("NXP NETC Library");
 MODULE_LICENSE("Dual BSD/GPL");
drivers/net/ethernet/freescale/enetc/ntmp_private.h +6 -0
index d2a6399b0a36..7d50af7745c7 100644
@@ -179,4 +179,10 @@ struct vft_req_ua {
 	struct vft_cfge_data cfge;
 };
 
+/* Buffer Pool Table Request Data Buffer Format of Update action */
+struct bpt_req_update {
+	struct ntmp_req_by_eid rbe;
+	struct bpt_cfge_data cfge;
+};
+
 #endif
include/linux/fsl/ntmp.h +32 -0
index 2aedea17307f..a54945dcdc61 100644
@@ -33,6 +33,7 @@ struct netc_tbl_vers {
 	u8 rsst_ver;
 	u8 fdbt_ver;
 	u8 vft_ver;
+	u8 bpt_ver;
 };
 
 struct netc_cbdr {
@@ -116,6 +117,29 @@ struct vft_cfge_data {
 	__le32 et_eid;
 };
 
+struct bpt_bpse_data {
+	__le32 amount_used;
+	__le32 amount_used_hwm;
+	u8 bpd_fc_state;
+#define BPT_FC_STATE		BIT(0)
+#define BPT_BPD			BIT(1)
+} __packed;
+
+struct bpt_cfge_data {
+	u8 fccfg_sbpen;
+#define BPT_SBP_EN		BIT(0)
+#define BPT_FC_CFG		GENMASK(2, 1)
+#define BPT_FC_CFG_EN_BPFC	1
+	u8 pfc_vector;
+	__le16 max_thresh;
+	__le16 fc_on_thresh;
+	__le16 fc_off_thresh;
+	__le16 sbp_thresh;
+	__le16 resv;
+	__le32 sbp_eid;
+	__le32 fc_ports;
+};
+
 #if IS_ENABLED(CONFIG_NXP_NETC_LIB)
 int ntmp_init_cbdr(struct netc_cbdr *cbdr, struct device *dev,
 		   const struct netc_cbdr_regs *regs);
@@ -142,6 +166,8 @@ int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
 				struct fdbt_entry_data *entry);
 int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
 		       const struct vft_cfge_data *cfge);
+int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
+			  const struct bpt_cfge_data *cfge);
 #else
 static inline int ntmp_init_cbdr(struct netc_cbdr *cbdr, struct device *dev,
 				 const struct netc_cbdr_regs *regs)
@@ -213,6 +239,12 @@ static inline int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
 	return 0;
 }
 
+static inline int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
+					const struct bpt_cfge_data *cfge)
+{
+	return 0;
+}
+
 #endif
 
 #endif
Signature · 1 line
2.34.1
maintainer
#15 lore raw

Re: [PATCH v4 net-next 06/14] net: enetc: add support for the "Update" operation to buffer pool table

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
Subject: net: enetc: add support for the "Update" operation to buffer pool table

This patch adds the ntmp_bpt_update_entry() helper function to support updating a specified entry in the buffer pool table. It provides the ability to update buffer pool configuration data.

Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp_private.h +0 -0

[ ... ]

Quoted 6 lines
drivers/net/ethernet/freescale/enetc/ntmp_private.h +6 -0
+/* Buffer Pool Table Request Data Buffer Format of Update action */
+struct bpt_req_update {
+	struct ntmp_req_by_eid rbe;
+	struct bpt_cfge_data cfge;
+};
+

[ ... ]

Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +0 -0

[ ... ]

Quoted 11 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +7 -0
@@ -702,5 +708,38 @@ int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
 }
 EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
 
+int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
+			  const struct bpt_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct bpt_req_update),
+	};

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid, · line 727

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +10 -0
+	struct bpt_req_update *req;
+	union netc_cbd cbd;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd_eid(&req->rbe, user->tbl.bpt_ver, 0,
+			  NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, entry_id);

Does this update action mask match the size of the request buffer?

The mask includes both NTMP_GEN_UA_CFGEU and BPT_UA_BPSEU, which instructs the hardware to update both the configuration entry and the status entry. However, struct bpt_req_update only contains the configuration data, omitting the status data.

If the hardware attempts to read the missing BPSE data, could it reject the command due to an undersized request length, or read beyond the intended payload into zero-initialized DMA padding? This might overwrite critical buffer pool tracking variables like amount_used_hwm.

If only the configuration is being updated, should the BPT_UA_BPSEU flag be removed?

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid, · line 738

Quoted 11 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +11 -0
+	req->cfge = *cfge;
+	ntmp_fill_request_hdr(&cbd, data.dma, NTMP_LEN(data.size, 0),
+			      NTMP_BPT_ID, NTMP_CMD_UPDATE, NTMP_AM_ENTRY_ID);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err)
+		dev_err(user->dev,
+			"Failed to update %s entry 0x%x, err: %pe\n",
+			ntmp_table_name(NTMP_BPT_ID), entry_id, ERR_PTR(err));
+
+	ntmp_free_data_mem(&data);

Can this result in a DMA use-after-free if the hardware command times out?

If netc_xmit_ntmp_cmd() times out waiting for the hardware to process the command, it returns -ETIMEDOUT without resetting the hardware ring or explicitly cancelling the command.

The descriptor remains active in the ring, and ntmp_free_data_mem() unconditionally frees the DMA-mapped buffer. If the hardware eventually processes the descriptor, it will read from the now-freed DMA memory.

Could this lead to an IOMMU fault or cause the hardware to read garbage data if the page has been reallocated?

Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +3 -0
+
+	return err;
+}
author maintainer
#16 lore raw

RE: [PATCH v4 net-next 06/14] net: enetc: add support for the "Update" operation to buffer pool table

Quoted 31 lines

Review int ntmp_vft_add_entry(struct ntmp_user *user,

Quoted 1 line
(quoted diff) +0 -0
@@ -702,5 +708,38 @@ int ntmp_vft_add_entry(struct ntmp_user *user,

u16 vid,

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +7 -0
 }
 EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
 
+int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
+			  const struct bpt_cfge_data *cfge)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct bpt_req_update),
+	};

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid, · line 727

Quoted 10 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +10 -0
+	struct bpt_req_update *req;
+	union netc_cbd cbd;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd_eid(&req->rbe, user->tbl.bpt_ver, 0,
+			  NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, entry_id);

Does this update action mask match the size of the request buffer?

The mask includes both NTMP_GEN_UA_CFGEU and BPT_UA_BPSEU, which instructs
the hardware to update both the configuration entry and the status entry. However, struct bpt_req_update only contains the configuration data, omitting the status data.

Quoted 7 lines

If the hardware attempts to read the missing BPSE data, could it reject the command due to an undersized request length, or read beyond the intended payload into zero-initialized DMA padding? This might overwrite critical buffer pool tracking variables like amount_used_hwm.

If only the configuration is being updated, should the BPT_UA_BPSEU flag be removed?

The BPSE_DATA is a set of statistics and status records kept by the hardware (internal buffer), and does not require the user to provide additional data to the hardware for updates. Therefore, for the 'update' operation, the request buffer does not need to provide BPSE_DATA data.

author maintainer
#17 lore raw

PATCH v4 7/14 net: enetc: add support for "Add" and "Delete" operations to IPFT

In reply to Wei Fang

The ingress port filter table (IPFT )contains a set of filters each capable of classifying incoming traffic using a mix of L2, L3, and L4 parsed and arbitrary field data. As a result of a filter match, several actions can be specified such as on whether to deny or allow a frame, overriding internal QoS attributes associated with the frame and setting parameters for the subsequent frame processing functions, such as stream identification, policing, ingress mirroring. Each entry corresponds to a filter. The ingress port filter entries are added using a precedence value. If a frame matches multiple entries, the entry with the higher precedence is used. Currently, this patch only adds "Add" and "Delete" operations to the ingress port filter table. These two interfaces will be used by both ENETC driver and NETC switch driver.

---
 drivers/net/ethernet/freescale/enetc/ntmp.c   |  76 +++++++++++++
 .../ethernet/freescale/enetc/ntmp_private.h   |  36 ++++++
 include/linux/fsl/ntmp.h                      | 104 ++++++++++++++++++
 3 files changed, 216 insertions(+)
drivers/net/ethernet/freescale/enetc/ntmp.c +76 -0
index 3c59b355c142..960d5be8ec42 100644
@@ -20,6 +20,7 @@
 /* Define NTMP Table ID */
 #define NTMP_MAFT_ID			1
 #define NTMP_RSST_ID			3
+#define NTMP_IPFT_ID			13
 #define NTMP_FDBT_ID			15
 #define NTMP_VFT_ID			18
 #define NTMP_BPT_ID			41
@@ -234,6 +235,8 @@ static const char *ntmp_table_name(int tbl_id)
 		return "MAC Address Filter Table";
 	case NTMP_RSST_ID:
 		return "RSS Table";
+	case NTMP_IPFT_ID:
+		return "Ingress Port Filter Table";
 	case NTMP_FDBT_ID:
 		return "FDB Table";
 	case NTMP_VFT_ID:
@@ -468,6 +471,79 @@ int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count)
 }
 EXPORT_SYMBOL_GPL(ntmp_rsst_query_entry);
 
+/**
+ * ntmp_ipft_add_entry - add an entry into the ingress port filter table
+ * @user: target ntmp_user struct
+ * @entry: the entry data, entry->cfge (configuration element data) and
+ * entry->keye (key element data) are used as input. Since the entry ID
+ * is assigned by the hardware, so entry->entry_id is a returned value
+ * for the driver to use, the driver can update/delete/query the entry
+ * based on the entry_id.
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_ipft_add_entry(struct ntmp_user *user,
+			struct ipft_entry_data *entry)
+{
+	struct ntmp_dma_buf data = {
+		.dev = user->dev,
+		.size = sizeof(struct ipft_resp_query),
+	};
+	struct ipft_resp_query *resp;
+	struct ipft_req_ua *req;
+	union netc_cbd cbd;
+	u32 len;
+	int err;
+
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd(&req->crd, user->tbl.ipft_ver, NTMP_QA_ENTRY_ID,
+		      NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU);
+	req->ak.keye = entry->keye;
+	req->cfge = entry->cfge;
+
+	len = NTMP_LEN(sizeof(*req), data.size);
+	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_IPFT_ID,
+			      NTMP_CMD_AQ, NTMP_AM_TERNARY_KEY);
+
+	err = netc_xmit_ntmp_cmd(user, &cbd);
+	if (err) {
+		dev_err(user->dev, "Failed to add %s entry, err: %pe\n",
+			ntmp_table_name(NTMP_IPFT_ID), ERR_PTR(err));
+
+		goto end;
+	}
+
+	resp = (struct ipft_resp_query *)req;
+	entry->entry_id = le32_to_cpu(resp->entry_id);
+
+end:
+	ntmp_free_data_mem(&data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(ntmp_ipft_add_entry);
+
+/**
+ * ntmp_ipft_delete_entry - delete a specified ingress port filter table entry
+ * @user: target ntmp_user struct
+ * @entry_id: the specified ID of the ingress port filter table entry
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_ipft_delete_entry(struct ntmp_user *user, u32 entry_id)
+{
+	u32 req_len = sizeof(struct ipft_req_qd);
+
+	return ntmp_delete_entry_by_id(user, NTMP_IPFT_ID,
+				       user->tbl.ipft_ver,
+				       entry_id, req_len,
+				       NTMP_STATUS_RESP_LEN);
+}
+EXPORT_SYMBOL_GPL(ntmp_ipft_delete_entry);
+
 /**
  * ntmp_fdbt_add_entry - add an entry into the FDB table
  * @user: target ntmp_user struct
drivers/net/ethernet/freescale/enetc/ntmp_private.h +36 -0
index 7d50af7745c7..6d519d59a433 100644
@@ -103,6 +103,42 @@ struct rsst_req_update {
 	u8 groups[];
 };
 
+/* Ingress Port Filter Table Response Data Buffer Format of Query action */
+struct ipft_resp_query {
+	__le32 status;
+	__le32 entry_id;
+	struct ipft_keye_data keye;
+	__le64 match_count; /* STSE_DATA */
+	struct ipft_cfge_data cfge;
+} __packed;
+
+struct ipft_ak_eid {
+	__le32 entry_id;
+	__le32 resv[52];
+};
+
+union ipft_access_key {
+	struct ipft_ak_eid eid;
+	struct ipft_keye_data keye;
+};
+
+/* Ingress Port Filter Table Request Data Buffer Format of Update and
+ * Add actions
+ */
+struct ipft_req_ua {
+	struct ntmp_cmn_req_data crd;
+	union ipft_access_key ak;
+	struct ipft_cfge_data cfge;
+};
+
+/* Ingress Port Filter Table Request Data Buffer Format of Query and
+ * Delete actions
+ */
+struct ipft_req_qd {
+	struct ntmp_req_by_eid rbe;
+	__le32 resv[52];
+};
+
 /* Access Key Format of FDB Table */
 struct fdbt_ak_eid {
 	__le32 entry_id;
include/linux/fsl/ntmp.h +104 -0

134 lines not on this page · raw message

Signature · 1 line
2.34.1
maintainer
#18 lore raw

Re: [PATCH v4 net-next 07/14] net: enetc: add support for "Add" and "Delete" operations to IPFT

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: enetc: add support for "Add" and "Delete" operations to IPFT

This commit adds support for the "Add" and "Delete" operations to the ingress port filter table (IPFT). These interfaces will be used by both the ENETC driver and the NETC switch driver.

Quoted 4 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +0 -0
index 3c59b355c142..960d5be8ec42 100644

[ ... ]

Quoted 18 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +14 -0
@@ -468,6 +471,79 @@ int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count)
 }
 EXPORT_SYMBOL_GPL(ntmp_rsst_query_entry);
 
+/**
+ * ntmp_ipft_add_entry - add an entry into the ingress port filter table
+ * @user: target ntmp_user struct
+ * @entry: the entry data, entry->cfge (configuration element data) and
+ * entry->keye (key element data) are used as input. Since the entry ID
+ * is assigned by the hardware, so entry->entry_id is a returned value
+ * for the driver to use, the driver can update/delete/query the entry
+ * based on the entry_id.
+ *
+ * Return: 0 on success, otherwise a negative error code
+ */
+int ntmp_ipft_add_entry(struct ntmp_user *user,
+			struct ipft_entry_data *entry)
+{

[ ... ]

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count) · line 505

Quoted 8 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +8 -0
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd(&req->crd, user->tbl.ipft_ver, NTMP_QA_ENTRY_ID,
+		      NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU);
+	req->ak.keye = entry->keye;
+	req->cfge = entry->cfge;

The request sets NTMP_GEN_UA_STSEU to indicate statistics data is present, but struct ipft_req_ua does not appear to contain a match_count or STSE data field before cfge.

Since the hardware expects STSE data before CFGE data (as seen in struct ipft_resp_query), will this cause the hardware to misinterpret the first 8 bytes of cfge as the match_count, and then read the actual filter configuration from out-of-bounds or uninitialized memory?

Quoted 4 lines
drivers/net/ethernet/freescale/enetc/ntmp_private.h +0 -0
index 7d50af7745c7..6d519d59a433 100644

[ ... ]

drivers/net/ethernet/freescale/enetc/ntmp_private.h struct rsst_req_update { · line 132

Quoted 8 lines
drivers/net/ethernet/freescale/enetc/ntmp_private.h +8 -0
+/* Ingress Port Filter Table Request Data Buffer Format of Update and
+ * Add actions
+ */
+struct ipft_req_ua {
+	struct ntmp_cmn_req_data crd;
+	union ipft_access_key ak;
+	struct ipft_cfge_data cfge;
+};

Does this struct need an STSE data field before cfge to match the request header flags?

Quoted 4 lines
include/linux/fsl/ntmp.h +0 -0
index a54945dcdc61..efda35e5e22c 100644

[ ... ]

Quoted 16 lines
include/linux/fsl/ntmp.h +12 -0
@@ -66,6 +68,94 @@ struct maft_entry_data {
 	struct maft_cfge_data cfge;
 };
 
+struct ipft_pld_byte {
+	u8 data;
+	u8 mask;
+};
+
+struct ipft_keye_data {
+	__le16 precedence;
+	__le16 resv0[3];
+	__le16 frm_attr_flags;
+#define IPFT_FAF_OVLAN		BIT(2)
+#define IPFT_FAF_IVLAN		BIT(3)
+#define IPFT_FAF_IP_HDR		BIT(7)

include/linux/fsl/ntmp.h struct maft_entry_data { · line 92

Quoted 10 lines
include/linux/fsl/ntmp.h +10 -0
+#define IPFT_FAF_IP_VER6	BIT(8)
+#define IPFT_FAF_L4_CODE	GENMASK(11, 10)
+#define  IPFT_FAF_TCP_HDR	1
+#define  IPFT_FAF_UDP_HDR	2
+#define  IPFT_FAF_SCTP_HDR	3
+#define IPFT_FAF_WOL_MAGIC	BIT(12)
+	__le16 frm_attr_flags_mask;
+	__le16 dscp;
+#define IPFT_DSCP		GENMASK(5, 0)
+#define IPFT_DSCP_MASK		GENMASK(11, 0)

Does IPFT_DSCP_MASK completely overlap with IPFT_DSCP?

If it is defined as GENMASK(11, 0) instead of GENMASK(11, 6), using FIELD_PREP with this mask will not shift the value into the upper bits, which might overwrite the actual DSCP value in bits 0-5.

author maintainer
#19 lore raw

RE: [PATCH v4 net-next 07/14] net: enetc: add support for "Add" and "Delete" operations to IPFT

Quoted 17 lines
Quoted 3 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +3 -0
+int ntmp_ipft_add_entry(struct ntmp_user *user,
+			struct ipft_entry_data *entry)
+{

[ ... ]

drivers/net/ethernet/freescale/enetc/ntmp.c int ntmp_rsst_query_entry(struct ntmp_user *user, u32 *table, int count) · line 505

Quoted 8 lines
drivers/net/ethernet/freescale/enetc/ntmp.c +8 -0
+	err = ntmp_alloc_data_mem(&data, (void **)&req);
+	if (err)
+		return err;
+
+	ntmp_fill_crd(&req->crd, user->tbl.ipft_ver, NTMP_QA_ENTRY_ID,
+		      NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU);
+	req->ak.keye = entry->keye;
+	req->cfge = entry->cfge;

The request sets NTMP_GEN_UA_STSEU to indicate statistics data is present, but struct ipft_req_ua does not appear to contain a match_count or STSE data field before cfge.

Quoted 4 lines

Since the hardware expects STSE data before CFGE data (as seen in struct ipft_resp_query), will this cause the hardware to misinterpret the first 8 bytes of cfge as the match_count, and then read the actual filter configuration from out-of-bounds or uninitialized memory?

Based on NETC manual, the STSE_DATA is present only for the commands which perform a query. So for the 'add' command, we do not need to provide the STSE_DATA buffer. And STSE_DATA is the statistics of the entry, which is stored in the internal buffer of IPF table. Performing NTMP_GEN_UA_STSEU is to reset the statistic counter when adding an entry.

[...]

Quoted 25 lines
Quoted 12 lines
include/linux/fsl/ntmp.h +12 -0
+struct ipft_pld_byte {
+	u8 data;
+	u8 mask;
+};
+
+struct ipft_keye_data {
+	__le16 precedence;
+	__le16 resv0[3];
+	__le16 frm_attr_flags;
+#define IPFT_FAF_OVLAN		BIT(2)
+#define IPFT_FAF_IVLAN		BIT(3)
+#define IPFT_FAF_IP_HDR		BIT(7)

include/linux/fsl/ntmp.h struct maft_entry_data { · line 92

Quoted 10 lines
include/linux/fsl/ntmp.h +10 -0
+#define IPFT_FAF_IP_VER6	BIT(8)
+#define IPFT_FAF_L4_CODE	GENMASK(11, 10)
+#define  IPFT_FAF_TCP_HDR	1
+#define  IPFT_FAF_UDP_HDR	2
+#define  IPFT_FAF_SCTP_HDR	3
+#define IPFT_FAF_WOL_MAGIC	BIT(12)
+	__le16 frm_attr_flags_mask;
+	__le16 dscp;
+#define IPFT_DSCP		GENMASK(5, 0)
+#define IPFT_DSCP_MASK		GENMASK(11, 0)

Does IPFT_DSCP_MASK completely overlap with IPFT_DSCP?

Quoted 3 lines

If it is defined as GENMASK(11, 0) instead of GENMASK(11, 6), using FIELD_PREP with this mask will not shift the value into the upper bits, which might overwrite the actual DSCP value in bits 0-5.

Thanks, this is a bug, I will fix it.

author maintainer
#20 lore raw

PATCH v4 8/14 net: enetc: add multiple command BD rings support

In reply to Wei Fang

All the tables of NETC switch are managed through the command BD ring, but unlike ENETC, the switch has two command BD rings, if the current ring is busy, the switch driver can switch to another ring to manage the table. Currently, the NTMP driver does not support multiple rings. Therefore, netc_select_cbdr() is added to select a appropriate ring to execute the command for the switch.

---
 drivers/net/ethernet/freescale/enetc/ntmp.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/net/ethernet/freescale/enetc/ntmp.c +13 -4
index 960d5be8ec42..5679b866af48 100644
@@ -125,12 +125,21 @@ static int netc_xmit_ntmp_cmd(struct ntmp_user *user, union netc_cbd *cbd)
 	u16 status;
 	u32 val;
 
-	/* Currently only i.MX95 ENETC is supported, and it only has one
-	 * command BD ring
+	for (i = 0; i < user->cbdr_num; i++) {
+		cbdr = &user->ring[i];
+		if (spin_trylock_bh(&cbdr->ring_lock))
+			break;
+	}
+
+	/* If all command BD rings are locked, we need to select
+	 * one of them and wait for it.
 	 */
-	cbdr = &user->ring[0];
+	if (i == user->cbdr_num) {
+		int cpu = raw_smp_processor_id();
 
-	spin_lock_bh(&cbdr->ring_lock);
+		cbdr = &user->ring[cpu % user->cbdr_num];
+		spin_lock_bh(&cbdr->ring_lock);
+	}
 
 	if (unlikely(!ntmp_get_free_cbd_num(cbdr)))
 		ntmp_clean_cbdr(cbdr);
Signature · 1 line
2.34.1
author maintainer
#21 lore raw

PATCH v4 9/14 net: dsa: add NETC switch tag support

In reply to Wei Fang

The NXP NETC switch tag is a proprietary header added to frames after the source MAC address. The switch tag has 3 types, and each type has 1 ~ 4 subtypes, the details are as follows.

Forward NXP switch tag (Type=0): Represents forwarded frames.
- SubType = 0 - Normal frame processing.

To_Port NXP switch tag (Type=1): Represents frames that are to be sent to a specific switch port.
- SubType = 0. No request to perform timestamping.
- SubType = 1. Request to perform one-step timestamping.
- SubType = 2. Request to perform two-step timestamping.
- SubType = 3. Request to perform both one-step timestamping and two-step timestamping.

To_Host NXP switch tag (Type=2): Represents frames redirected or copied to the switch management port.
- SubType = 0. Received frames redirected or copied to the switch management port.
- SubType = 1. Received frames redirected or copied to the switch management port with captured timestamp at the switch port where the frame was received.
- SubType = 2. Transmit timestamp response (two-step timestamping).

In addition, the length of different type switch tag is different, the minimum length is 6 bytes, the maximum length is 14 bytes. Currently, Forward tag, SubType 0 of To_Port tag and Subtype 0 of To_Host tag are supported. More tags will be supported in the future.

---
 include/linux/dsa/tag_netc.h  |  14 +++
 include/net/dsa.h             |   2 +
 include/uapi/linux/if_ether.h |   1 +
 net/dsa/Kconfig               |  10 ++
 net/dsa/Makefile              |   1 +
 net/dsa/tag_netc.c            | 185 ++++++++++++++++++++++++++++++++++
 6 files changed, 213 insertions(+)
 create mode 100644 include/linux/dsa/tag_netc.h
 create mode 100644 net/dsa/tag_netc.c
include/linux/dsa/tag_netc.h new file +14 -0
new file mode 100644
index 000000000000..fe964722e5b0
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright 2025-2026 NXP
+ */
+
+#ifndef __NET_DSA_TAG_NETC_H
+#define __NET_DSA_TAG_NETC_H
+
+#include <linux/skbuff.h>
+#include <net/dsa.h>
+
+#define NETC_TAG_MAX_LEN			14
+
+#endif
include/net/dsa.h +2 -0
index 6c17446f3dcc..6bed824d1f07 100644
@@ -58,6 +58,7 @@ struct tc_action;
 #define DSA_TAG_PROTO_YT921X_VALUE		30
 #define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
 #define DSA_TAG_PROTO_MXL862_VALUE		32
+#define DSA_TAG_PROTO_NETC_VALUE		33
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -93,6 +94,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_YT921X		= DSA_TAG_PROTO_YT921X_VALUE,
 	DSA_TAG_PROTO_MXL_GSW1XX	= DSA_TAG_PROTO_MXL_GSW1XX_VALUE,
 	DSA_TAG_PROTO_MXL862		= DSA_TAG_PROTO_MXL862_VALUE,
+	DSA_TAG_PROTO_NETC		= DSA_TAG_PROTO_NETC_VALUE,
 };
 
 struct dsa_switch;
include/uapi/linux/if_ether.h +1 -0
index df9d44a11540..fb5efc8e06cc 100644
@@ -123,6 +123,7 @@
 #define ETH_P_DSA_A5PSW	0xE001		/* A5PSW Tag Value [ NOT AN OFFICIALLY REGISTERED ID ] */
 #define ETH_P_IFE	0xED3E		/* ForCES inter-FE LFB type */
 #define ETH_P_AF_IUCV   0xFBFB		/* IBM af_iucv [ NOT AN OFFICIALLY REGISTERED ID ] */
+#define ETH_P_NXP_NETC  0xFD3A		/* NXP NETC DSA [ NOT AN OFFICIALLY REGISTERED ID ] */
 
 #define ETH_P_802_3_MIN	0x0600		/* If the value in the ethernet type is more than this value
 					 * then the frame is Ethernet II. Else it is 802.3 */
net/dsa/Kconfig +10 -0
index 5ed8c704636d..d5e725b90d78 100644
@@ -125,6 +125,16 @@ config NET_DSA_TAG_KSZ
 	  Say Y if you want to enable support for tagging frames for the
 	  Microchip 8795/937x/9477/9893 families of switches.
 
+config NET_DSA_TAG_NETC
+	tristate "Tag driver for NXP NETC switches"
+	help
+	  Say Y or M if you want to enable support for the NXP Switch Tag (NST),
+	  as implemented by NXP NETC switches having version 4.3 or later. The
+	  switch tag is a proprietary header added to frames after the source
+	  MAC address, it has 3 types and each type has different subtypes, so
+	  its length depends on the type and subtype of the tag, the maximum
+	  length is 14 bytes.
+
 config NET_DSA_TAG_OCELOT
 	tristate "Tag driver for Ocelot family of switches, using NPI port"
 	select PACKING
net/dsa/Makefile +1 -0
index bf7247759a64..b8c2667cd14a 100644
@@ -30,6 +30,7 @@ obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
 obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
 obj-$(CONFIG_NET_DSA_TAG_MXL_862XX) += tag_mxl862xx.o
 obj-$(CONFIG_NET_DSA_TAG_MXL_GSW1XX) += tag_mxl-gsw1xx.o
+obj-$(CONFIG_NET_DSA_TAG_NETC) += tag_netc.o
 obj-$(CONFIG_NET_DSA_TAG_NONE) += tag_none.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
net/dsa/tag_netc.c new file +185 -0

185 lines not on this page · raw message

Signature · 1 line
2.34.1
maintainer
#22 lore raw

Re: [PATCH v4 net-next 09/14] net: dsa: add NETC switch tag support

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: dsa: add NETC switch tag support

Adds support for the proprietary NXP NETC switch tag used in NXP NETC switches version 4.3 or later. The tag has 3 types (Forward, To_Port, To_Host) and supports a varying length between 6 and 14 bytes.

Quoted 5 lines
net/dsa/tag_netc.c new file +0 -0
new file mode 100644
index 000000000000..addd41f7f1b6

[ ... ]

net/dsa/tag_netc.c line 68

Quoted 7 lines
net/dsa/tag_netc.c new file +7 -0
+static void *netc_fill_common_tp_tag(struct sk_buff *skb,
+				     struct net_device *ndev,
+				     u8 subtype, int tag_len)
+{
+	struct dsa_port *dp = dsa_user_to_port(ndev);
+	u16 queue = skb_get_queue_mapping(skb);
+	u8 ipv = netdev_txq_to_tc(ndev, queue);

What happens here if the interface doesn't have traffic classes configured?

By default, netdev_txq_to_tc() returns -1. Because ipv is an unsigned 8-bit integer, this casts to 255.

When this is later passed to FIELD_PREP(NETC_TAG_IPV, ipv) in netc_fill_common_tag(), the macro blindly masks the variable, silently forcing the IPV field to 7 (the maximum priority) for all default, unconfigured traffic. Valid TC values greater than 7 would also be silently truncated.

Does this code bypass the intended QoS mechanisms?

net/dsa/tag_netc.c line 77

Quoted 9 lines
net/dsa/tag_netc.c new file +9 -0
+	void *tag;
+
+	skb_push(skb, tag_len);
+	dsa_alloc_etype_header(skb, tag_len);
+
+	tag = dsa_etype_header_pos_tx(skb);
+	memset(tag + NETC_TAG_CMN_LEN, 0, tag_len - NETC_TAG_CMN_LEN);
+	netc_fill_common_tag(tag, NETC_TAG_TO_PORT, subtype,
+			     dp->ds->index, dp->index, ipv);

Will this break communication for standard single-switch setups?

The driver passes dp->ds->index as the hardware switch ID here, which defaults to 0 for the first switch in a DSA tree. However, on the receive path, frames stamped with switch ID 0 are rejected.

If the physical switch hardware uses an ID greater than 0, frames will fail the lookup in dsa_conduit_find_user() on receive because the software index is 0.

Quoted 3 lines
net/dsa/tag_netc.c new file +3 -0
+
+	return tag;
+}

[ ... ]

Quoted 5 lines
net/dsa/tag_netc.c new file +5 -0
+static struct sk_buff *netc_rcv(struct sk_buff *skb,
+				struct net_device *ndev)
+{
+	struct netc_tag_cmn *tag_cmn;
+	int tag_len, sw_id, port;

[ ... ]

net/dsa/tag_netc.c line 138

Quoted 4 lines
net/dsa/tag_netc.c new file +4 -0
+	if (tag_cmn->qos & NETC_TAG_QV)
+		skb->priority = FIELD_GET(NETC_TAG_IPV, tag_cmn->qos);
+
+	sw_id = NETC_TAG_SWITCH & tag_cmn->switch_port;

This isn't a bug, but is there a reason to use a raw bitwise AND for sw_id instead of using FIELD_GET(NETC_TAG_SWITCH, ...) to match how the port field is extracted just below this?

Quoted 10 lines
net/dsa/tag_netc.c new file +10 -0
+	/* ENETC VEPA switch ID (0) is not supported yet */
+	if (!sw_id) {
+		dev_warn_ratelimited(&ndev->dev,
+				     "VEPA switch ID is not supported yet\n");
+
+		return NULL;
+	}
+
+	port = FIELD_GET(NETC_TAG_PORT, tag_cmn->switch_port);
+	skb->dev = dsa_conduit_find_user(ndev, sw_id, port);

[ ... ]

author maintainer
#23 lore raw

RE: [PATCH v4 net-next 09/14] net: dsa: add NETC switch tag support

Quoted 6 lines
Quoted 5 lines
net/dsa/tag_netc.c new file +0 -0
new file mode 100644
index 000000000000..addd41f7f1b6

[ ... ]

Quoted 9 lines

net/dsa/tag_netc.c line 68

Quoted 7 lines
net/dsa/tag_netc.c new file +7 -0
+static void *netc_fill_common_tp_tag(struct sk_buff *skb,
+				     struct net_device *ndev,
+				     u8 subtype, int tag_len)
+{
+	struct dsa_port *dp = dsa_user_to_port(ndev);
+	u16 queue = skb_get_queue_mapping(skb);
+	u8 ipv = netdev_txq_to_tc(ndev, queue);

What happens here if the interface doesn't have traffic classes configured?

netdev_txq_to_tc() will return 0 if not TC configured, I mean ndev->num_tc is 0.

Quoted 3 lines

By default, netdev_txq_to_tc() returns -1. Because ipv is an unsigned 8-bit integer, this casts to 255.

Okay, this is a potential bug when we add TC_SETUP_QDISC_MQPRIO support. I will fix it.

Quoted 8 lines

When this is later passed to FIELD_PREP(NETC_TAG_IPV, ipv) in netc_fill_common_tag(), the macro blindly masks the variable, silently forcing the IPV field to 7 (the maximum priority) for all default, unconfigured traffic. Valid TC values greater than 7 would also be silently truncated.

Does this code bypass the intended QoS mechanisms?

No, configure IPV to send packets according to the expected TC.

Quoted 13 lines

net/dsa/tag_netc.c line 77

Quoted 10 lines
net/dsa/tag_netc.c new file +9 -0
+	void *tag;
+
+	skb_push(skb, tag_len);
+	dsa_alloc_etype_header(skb, tag_len);
+
+	tag = dsa_etype_header_pos_tx(skb);
+	memset(tag + NETC_TAG_CMN_LEN, 0, tag_len - NETC_TAG_CMN_LEN);
+	netc_fill_common_tag(tag, NETC_TAG_TO_PORT, subtype,
+			     dp->ds->index, dp->index, ipv);

Will this break communication for standard single-switch setups?

Quoted 7 lines

The driver passes dp->ds->index as the hardware switch ID here, which defaults to 0 for the first switch in a DSA tree. However, on the receive path, frames stamped with switch ID 0 are rejected.

If the physical switch hardware uses an ID greater than 0, frames will fail the lookup in dsa_conduit_find_user() on receive because the software index is 0.

We added the 'dsa,member' property to the netc switch DT-binding doc, specifying that the 'member' (switch index) value cannot be 0. Furthermore, the netc switch driver also checks the switch index to ensure it is not 0. Therefore, the software index and hardware index are equal.

Quoted 11 lines
Quoted 4 lines
net/dsa/tag_netc.c new file +3 -0
+
+	return tag;
+}

[ ... ]

Quoted 5 lines
net/dsa/tag_netc.c new file +5 -0
+static struct sk_buff *netc_rcv(struct sk_buff *skb,
+				struct net_device *ndev)
+{
+	struct netc_tag_cmn *tag_cmn;
+	int tag_len, sw_id, port;

[ ... ]

Quoted 8 lines

net/dsa/tag_netc.c line 138

Quoted 4 lines
net/dsa/tag_netc.c new file +4 -0
+	if (tag_cmn->qos & NETC_TAG_QV)
+		skb->priority = FIELD_GET(NETC_TAG_IPV, tag_cmn->qos);
+
+	sw_id = NETC_TAG_SWITCH & tag_cmn->switch_port;

This isn't a bug, but is there a reason to use a raw bitwise AND for sw_id instead of using FIELD_GET(NETC_TAG_SWITCH, ...) to match how the port field is extracted just below this?

I think either is fine, the final result is the same. I just wrote it this way for convenience during implementation.

author maintainer
#24 lore raw

PATCH v4 10/14 net: dsa: netc: introduce NXP NETC switch driver for i.MX94

In reply to Wei Fang

For i.MX94 series, the NETC IP provides full 802.1Q Ethernet switch functionality, advanced QoS with 8 traffic classes, and a full range of TSN standards capabilities. The switch has 3 user ports and 1 CPU port, the CPU port is connected to an internal ENETC. Since the switch and the internal ENETC are fully integrated within the NETC IP, no back-to-back MAC connection is required. Instead, a light-weight "pseudo MAC" is used between the switch and the ENETC. This translates to lower power (less logic and memory) and lower delay (as there is no serialization delay across this link).

This patch introduces the initial NETC switch driver. At this stage, only basic probe and remove functionality is supported. More features will be supported in the subsequent patches.

---
 MAINTAINERS                           |  11 +
 drivers/net/dsa/Kconfig               |   3 +
 drivers/net/dsa/Makefile              |   1 +
 drivers/net/dsa/netc/Kconfig          |  14 +
 drivers/net/dsa/netc/Makefile         |   3 +
 drivers/net/dsa/netc/netc_main.c      | 672 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_platform.c  |  49 ++
 drivers/net/dsa/netc/netc_switch.h    |  92 ++++
 drivers/net/dsa/netc/netc_switch_hw.h | 137 ++++++
 9 files changed, 982 insertions(+)
 create mode 100644 drivers/net/dsa/netc/Kconfig
 create mode 100644 drivers/net/dsa/netc/Makefile
 create mode 100644 drivers/net/dsa/netc/netc_main.c
 create mode 100644 drivers/net/dsa/netc/netc_platform.c
 create mode 100644 drivers/net/dsa/netc/netc_switch.h
 create mode 100644 drivers/net/dsa/netc/netc_switch_hw.h
MAINTAINERS +11 -0
index a09bf30a057d..fe744320b1e5 100644
@@ -19083,6 +19083,17 @@ F:	Documentation/devicetree/bindings/clock/*imx*
 F:	drivers/clk/imx/
 F:	include/dt-bindings/clock/*imx*
 
+NXP NETC ETHERNET SWITCH DRIVER
+M:	Wei Fang <[email protected]>
+R:	Clark Wang <[email protected]>
+L:	[email protected]
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/dsa/nxp,netc-switch.yaml
+F:	drivers/net/dsa/netc/
+F:	include/linux/dsa/tag_netc.h
+F:	net/dsa/tag_netc.c
+
 NXP NETC TIMER PTP CLOCK DRIVER
 M:	Wei Fang <[email protected]>
 M:	Clark Wang <[email protected]>
drivers/net/dsa/Kconfig +3 -0
index 39fb8ead16b5..e9c7a6874791 100644
@@ -74,8 +74,11 @@ source "drivers/net/dsa/microchip/Kconfig"
 
 source "drivers/net/dsa/mv88e6xxx/Kconfig"
 
+
 source "drivers/net/dsa/mxl862xx/Kconfig"
 
+source "drivers/net/dsa/netc/Kconfig"
+
 source "drivers/net/dsa/ocelot/Kconfig"
 
 source "drivers/net/dsa/qca/Kconfig"
drivers/net/dsa/Makefile +1 -0
index f5a463b87ec2..d2975badffc0 100644
@@ -21,6 +21,7 @@ obj-y				+= lantiq/
 obj-y				+= microchip/
 obj-y				+= mv88e6xxx/
 obj-y				+= mxl862xx/
+obj-y				+= netc/
 obj-y				+= ocelot/
 obj-y				+= qca/
 obj-y				+= realtek/
drivers/net/dsa/netc/Kconfig new file +14 -0
new file mode 100644
index 000000000000..8824d30ed3ea
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config NET_DSA_NETC_SWITCH
+	tristate "NXP NETC Ethernet switch support"
+	depends on NET_DSA && PCI
+	select NET_DSA_TAG_NETC
+	select FSL_ENETC_MDIO
+	select NXP_NTMP
+	select NXP_NETC_LIB
+	help
+	  This driver supports the NXP NETC Ethernet switch, which is embedded
+	  as a PCIe function of the NXP NETC IP. But note that this driver does
+	  only support switch versions greater than or equal to NETC v4.3.
+
+	  If compiled as module (M), the module name is nxp-netc-switch.
drivers/net/dsa/netc/Makefile new file +3 -0
new file mode 100644
index 000000000000..4a5767562574
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_NET_DSA_NETC_SWITCH) += nxp-netc-switch.o
+nxp-netc-switch-objs := netc_main.o netc_platform.o
drivers/net/dsa/netc/netc_main.c new file +672 -0

672 lines not on this page · raw message

drivers/net/dsa/netc/netc_platform.c new file +49 -0

49 lines not on this page · raw message

drivers/net/dsa/netc/netc_switch.h new file +92 -0

92 lines not on this page · raw message

drivers/net/dsa/netc/netc_switch_hw.h new file +137 -0

137 lines not on this page · raw message

Signature · 1 line
2.34.1
maintainer
#25 lore raw

Re: [PATCH v4 net-next 10/14] net: dsa: netc: introduce NXP NETC switch driver for i.MX94

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: dsa: netc: introduce NXP NETC switch driver for i.MX94

This commit introduces the initial NXP NETC switch driver for the i.MX94 series. It supports basic probe and remove functionality for the integrated 802.1Q Ethernet switch. The switch includes 3 user ports and 1 CPU port connected to an internal ENETC.

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c new file +0 -0
new file mode 100644
index 000000000000..5828fd3e342e

[ ... ]

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c new file +3 -0
+static int netc_port_create_emdio_bus(struct netc_port *np,
+				      struct device_node *node)
+{

[ ... ]

Quoted 9 lines
drivers/net/dsa/netc/netc_main.c new file +9 -0
+	err = devm_of_mdiobus_register(dev, bus, node);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Cannot register EMDIO bus\n");
+
+	np->emdio = bus;
+
+	return 0;
+}

[ ... ]

drivers/net/dsa/netc/netc_main.c line 169

Quoted 13 lines
drivers/net/dsa/netc/netc_main.c new file +13 -0
+static int netc_init_switch_id(struct netc_switch *priv)
+{
+	struct netc_switch_regs *regs = &priv->regs;
+	struct dsa_switch *ds = priv->ds;
+
+	/* The value of 0 is reserved for the VEPA switch and cannot
+	 * be used.
+	 */
+	if (ds->index > SWCR_SWID || !ds->index) {
+		dev_err(priv->dev, "Switch index %d out of range\n",
+			ds->index);
+		return -ERANGE;
+	}

Does this check cause the probe to fail unconditionally for standard single-switch topologies?

The DSA core typically assigns ds->index = 0 by default for the first switch. Because !ds->index evaluates to true for index 0, this setup function will always fail unless the dsa,member property is explicitly overridden in the device tree.

Could the driver translate the hardware ID internally, for example by writing ds->index + 1 to NETC_SWCR, rather than forcing this hardware-specific restriction onto the software DSA index?

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c new file +5 -0
+
+	netc_base_wr(regs, NETC_SWCR, ds->index);
+
+	return 0;
+}

[ ... ]

Quoted 27 lines
drivers/net/dsa/netc/netc_main.c new file +27 -0
+static bool netc_port_is_emdio_consumer(struct device_node *node)
+{
+	struct device_node *mdio_node;
+
+	/* If the port node has phy-handle property and it does
+	 * not contain a mdio child node, then the port is the
+	 * EMDIO consumer.
+	 */
+	mdio_node = of_get_child_by_name(node, "mdio");
+	if (!mdio_node)
+		return true;
+
+	of_node_put(mdio_node);
+
+	return false;
+}
+
+/* Currently, phylink_of_phy_connect() is called by dsa_user_create(),
+ * so if the switch uses the external MDIO controller (like the EMDIO
+ * function) to manage the external PHYs. The MDIO bus may not be
+ * created when phylink_of_phy_connect() is called, so it will return
+ * an error and cause the switch driver to fail to probe.
+ * This workaround can be removed when DSA phylink_of_phy_connect()
+ * calls are moved from probe() to ndo_open().
+ */
+static int netc_switch_check_emdio_is_ready(struct device *dev)
+{

[ ... ]

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c new file +10 -0
+	for_each_available_child_of_node_scoped(ports, child) {
+		/* If the node does not have phy-handle property, then
+		 * the port does not connect to a PHY, so the port is
+		 * not the EMDIO consumer.
+		 */
+		phy_node = of_parse_phandle(child, "phy-handle", 0);
+		if (!phy_node)
+			continue;
+
+		if (!netc_port_is_emdio_consumer(child)) {

drivers/net/dsa/netc/netc_main.c line 488

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c new file +10 -0
+			of_node_put(phy_node);
+			continue;
+		}
+
+		phydev = of_phy_find_device(phy_node);
+		of_node_put(phy_node);
+		if (!phydev) {
+			err = -EPROBE_DEFER;
+			goto out;
+		}

Could this logic cause an infinite probe deferral loop for ports sharing an internal MDIO bus?

The netc_port_is_emdio_consumer() function assumes any port node without an mdio child is an EMDIO consumer waiting for an external controller. However, if multiple sibling ports on the same switch share a single internal MDIO bus defined under one port, the other ports will lack an mdio child node.

This would cause those ports to be mistakenly treated as external EMDIO consumers, calling of_phy_find_device(). Because the internal MDIO bus is not registered until later in netc_setup(), the PHY device cannot be found, and the driver returns -EPROBE_DEFER.

Since netc_setup() is never reached, the MDIO bus is never registered, causing an infinite deferral loop. Could the driver more accurately identify whether the PHY belongs to its own switch before deferring?

Quoted 12 lines
drivers/net/dsa/netc/netc_main.c new file +12 -0
+
+		put_device(&phydev->mdio.dev);
+	}
+
+out:
+	of_node_put(ports);
+
+	return err;
+}
+
+static int netc_switch_pci_init(struct pci_dev *pdev)
+{

[ ... ]

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c new file +6 -0
+	regs->base = pci_ioremap_bar(pdev, NETC_REGS_BAR);
+	if (!regs->base) {
+		err = -ENXIO;
+		dev_err(dev, "pci_ioremap_bar() failed\n");
+		goto release_mem_regions;
+	}

[ ... ]

drivers/net/dsa/netc/netc_main.c line 562

Quoted 8 lines
drivers/net/dsa/netc/netc_main.c new file +8 -0
+static void netc_switch_pci_destroy(struct pci_dev *pdev)
+{
+	struct netc_switch *priv = pci_get_drvdata(pdev);
+
+	iounmap(priv->regs.base);
+	pci_release_mem_regions(pdev);
+	pci_disable_device(pdev);
+}

Does unmapping the memory here introduce a use-after-unmap issue when the MDIO bus is unregistered?

The driver registers the MDIO bus using devm_of_mdiobus_register(), meaning devres cleans it up asynchronously after the remove function returns or when probe fails.

However, in netc_switch_pci_destroy(), the driver explicitly unmaps the PCI I/O memory via iounmap(). Since netc_switch_pci_destroy() is called synchronously inside the remove function and probe error paths, the memory is unmapped before the MDIO bus is unregistered by devres.

When devres eventually unregisters the MDIO bus, it detaches PHY devices, which often perform MDIO transactions during removal. Because the underlying memory priv->regs.base is already unmapped, these MDIO read/write callbacks could trigger a synchronous external abort or kernel panic.

Would it be better to use pcim_iomap() to tie the memory mapping lifecycle to devres, ensuring it outlives the MDIO bus?

author maintainer
#26 lore raw

RE: [PATCH v4 net-next 10/14] net: dsa: netc: introduce NXP NETC switch driver for i.MX94

Quoted 17 lines

drivers/net/dsa/netc/netc_main.c line 169

Quoted 13 lines
drivers/net/dsa/netc/netc_main.c new file +13 -0
+static int netc_init_switch_id(struct netc_switch *priv)
+{
+	struct netc_switch_regs *regs = &priv->regs;
+	struct dsa_switch *ds = priv->ds;
+
+	/* The value of 0 is reserved for the VEPA switch and cannot
+	 * be used.
+	 */
+	if (ds->index > SWCR_SWID || !ds->index) {
+		dev_err(priv->dev, "Switch index %d out of range\n",
+			ds->index);
+		return -ERANGE;
+	}

Does this check cause the probe to fail unconditionally for standard single-switch topologies?

Quoted 4 lines

The DSA core typically assigns ds->index = 0 by default for the first switch. Because !ds->index evaluates to true for index 0, this setup function will always fail unless the dsa,member property is explicitly overridden in the device tree.

As I mentioned in another mail, we added the 'dsa,member' property to the netc switch DT-binding doc, specifying that the 'member' (switch index) value cannot be 0. And 'dsa,member' is a required property for netc switch.

Quoted 4 lines

Could the driver translate the hardware ID internally, for example by writing ds->index + 1 to NETC_SWCR, rather than forcing this hardware-specific restriction onto the software DSA index?

The current approach is based on Vladimir's suggestion. I need to confirm with Vladimir which approach is better.

Hi Vladimir,

What are your thoughts on this suggestion? Is this approach better?

Quoted 32 lines

[ ... ]

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c new file +10 -0
+	for_each_available_child_of_node_scoped(ports, child) {
+		/* If the node does not have phy-handle property, then
+		 * the port does not connect to a PHY, so the port is
+		 * not the EMDIO consumer.
+		 */
+		phy_node = of_parse_phandle(child, "phy-handle", 0);
+		if (!phy_node)
+			continue;
+
+		if (!netc_port_is_emdio_consumer(child)) {

drivers/net/dsa/netc/netc_main.c line 488

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c new file +10 -0
+			of_node_put(phy_node);
+			continue;
+		}
+
+		phydev = of_phy_find_device(phy_node);
+		of_node_put(phy_node);
+		if (!phydev) {
+			err = -EPROBE_DEFER;
+			goto out;
+		}

Could this logic cause an infinite probe deferral loop for ports sharing an internal MDIO bus?

The netc_port_is_emdio_consumer() function assumes any port node without an
mdio child is an EMDIO consumer waiting for an external controller. However, if multiple sibling ports on the same switch share a single internal MDIO bus defined under one port, the other ports will lack an mdio child node.

Quoted 8 lines

This would cause those ports to be mistakenly treated as external EMDIO consumers, calling of_phy_find_device(). Because the internal MDIO bus is not registered until later in netc_setup(), the PHY device cannot be found, and the driver returns -EPROBE_DEFER.

Since netc_setup() is never reached, the MDIO bus is never registered, causing an infinite deferral loop. Could the driver more accurately identify whether the PHY belongs to its own switch before deferring?

This case will not be true, from the hardware perspective, the switch ports do not support share a single internal MDIO bus defined under one port. The internal MDIO bus of a port can only access its own external PHY. So a switch port must either use EMDIO to access its PHY or its internal MDIO bus to access its PHY.

Quoted 42 lines
Quoted 13 lines
drivers/net/dsa/netc/netc_main.c new file +12 -0
+
+		put_device(&phydev->mdio.dev);
+	}
+
+out:
+	of_node_put(ports);
+
+	return err;
+}
+
+static int netc_switch_pci_init(struct pci_dev *pdev)
+{

[ ... ]

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c new file +6 -0
+	regs->base = pci_ioremap_bar(pdev, NETC_REGS_BAR);
+	if (!regs->base) {
+		err = -ENXIO;
+		dev_err(dev, "pci_ioremap_bar() failed\n");
+		goto release_mem_regions;
+	}

[ ... ]

drivers/net/dsa/netc/netc_main.c line 562

Quoted 8 lines
drivers/net/dsa/netc/netc_main.c new file +8 -0
+static void netc_switch_pci_destroy(struct pci_dev *pdev)
+{
+	struct netc_switch *priv = pci_get_drvdata(pdev);
+
+	iounmap(priv->regs.base);
+	pci_release_mem_regions(pdev);
+	pci_disable_device(pdev);
+}

Does unmapping the memory here introduce a use-after-unmap issue when the MDIO bus is unregistered?

The driver registers the MDIO bus using devm_of_mdiobus_register(), meaning devres cleans it up asynchronously after the remove function returns or when probe fails.

However, in netc_switch_pci_destroy(), the driver explicitly unmaps the PCI I/O memory via iounmap(). Since netc_switch_pci_destroy() is called synchronously inside the remove function and probe error paths, the memory is unmapped before the MDIO bus is unregistered by devres.

Quoted 8 lines

When devres eventually unregisters the MDIO bus, it detaches PHY devices, which often perform MDIO transactions during removal. Because the underlying
memory priv->regs.base is already unmapped, these MDIO read/write callbacks could trigger a synchronous external abort or kernel panic.

Would it be better to use pcim_iomap() to tie the memory mapping lifecycle to devres, ensuring it outlives the MDIO bus?

Great, I will pcim_iomap().

maintainer
#27 lore raw

Re: [PATCH v4 net-next 10/14] net: dsa: netc: introduce NXP NETC switch driver for i.MX94

Quoted 31 lines · Wei Fang
Quoted 17 lines

drivers/net/dsa/netc/netc_main.c line 169

Quoted 13 lines
drivers/net/dsa/netc/netc_main.c new file +13 -0
+static int netc_init_switch_id(struct netc_switch *priv)
+{
+	struct netc_switch_regs *regs = &priv->regs;
+	struct dsa_switch *ds = priv->ds;
+
+	/* The value of 0 is reserved for the VEPA switch and cannot
+	 * be used.
+	 */
+	if (ds->index > SWCR_SWID || !ds->index) {
+		dev_err(priv->dev, "Switch index %d out of range\n",
+			ds->index);
+		return -ERANGE;
+	}

Does this check cause the probe to fail unconditionally for standard single-switch topologies?

Quoted 4 lines

The DSA core typically assigns ds->index = 0 by default for the first switch. Because !ds->index evaluates to true for index 0, this setup function will always fail unless the dsa,member property is explicitly overridden in the device tree.

As I mentioned in another mail, we added the 'dsa,member' property to the netc switch DT-binding doc, specifying that the 'member' (switch index) value cannot be 0. And 'dsa,member' is a required property for netc switch.

Quoted 4 lines

Could the driver translate the hardware ID internally, for example by writing ds->index + 1 to NETC_SWCR, rather than forcing this hardware-specific restriction onto the software DSA index?

Quoted 6 lines

The current approach is based on Vladimir's suggestion. I need to confirm with Vladimir which approach is better.

Hi Vladimir,

What are your thoughts on this suggestion? Is this approach better?

I maintain it is preferable/simpler the way you are doing it, with a 1:1 mapping. The LLM's concern would be more valid if the switch were discrete and every board DTS author would have to remember to place the dsa,member property. But the switch OF node will live in an SoC dtsi.

Maybe you could put something along these lines in the commit message, hopefully the LLM will pick it up and stop complaining.

author maintainer
#28 lore raw

PATCH v4 11/14 net: dsa: netc: add phylink MAC operations

In reply to Wei Fang

Different versions of NETC switches have different numbers of ports and MAC capabilities, so add .phylink_get_caps() to struct netc_switch_info, so that each version of the NETC switch can implement its own callback to obtain MAC capabilities. In addition, related interfaces of struct phylink_mac_ops are added, such as .mac_config(), .mac_link_up(), and .mac_link_down().

---
 drivers/net/dsa/netc/netc_main.c      | 179 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_platform.c  |  41 ++++++
 drivers/net/dsa/netc/netc_switch.h    |   4 +
 drivers/net/dsa/netc/netc_switch_hw.h |  21 +++
 4 files changed, 245 insertions(+)
drivers/net/dsa/netc/netc_main.c +179 -0
index 5828fd3e342e..f11f5d0f6a6d 100644
@@ -569,10 +569,188 @@ static void netc_switch_get_ip_revision(struct netc_switch *priv)
 	priv->revision = val & IPBRR0_IP_REV;
 }
 
+static void netc_phylink_get_caps(struct dsa_switch *ds, int port,
+				  struct phylink_config *config)
+{
+	struct netc_switch *priv = ds->priv;
+
+	priv->info->phylink_get_caps(port, config);
+}
+
+static void netc_port_set_mac_mode(struct netc_port *np,
+				   unsigned int mode,
+				   phy_interface_t phy_mode)
+{
+	u32 mask = PM_IF_MODE_IFMODE | PM_IF_MODE_REVMII;
+	u32 val = 0;
+
+	switch (phy_mode) {
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val |= IFMODE_RGMII;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		val |= IFMODE_RMII;
+		break;
+	case PHY_INTERFACE_MODE_REVMII:
+		val |= PM_IF_MODE_REVMII;
+		fallthrough;
+	case PHY_INTERFACE_MODE_MII:
+		val |= IFMODE_MII;
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		val |= IFMODE_SGMII;
+		break;
+	default:
+		break;
+	}
+
+	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
+}
+
+static void netc_mac_config(struct phylink_config *config, unsigned int mode,
+			    const struct phylink_link_state *state)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+
+	netc_port_set_mac_mode(NETC_PORT(dp->ds, dp->index), mode,
+			       state->interface);
+}
+
+static void netc_port_set_speed(struct netc_port *np, int speed)
+{
+	netc_port_rmw(np, NETC_PCR, PCR_PSPEED, PSPEED_SET_VAL(speed));
+}
+
+static void netc_port_set_rgmii_mac(struct netc_port *np,
+				    int speed, int duplex)
+{
+	u32 mask, val;
+
+	mask = PM_IF_MODE_SSP | PM_IF_MODE_HD | PM_IF_MODE_M10;
+
+	switch (speed) {
+	default:
+	case SPEED_1000:
+		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_1G);
+		break;
+	case SPEED_100:
+		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_100M);
+		break;
+	case SPEED_10:
+		val = FIELD_PREP(PM_IF_MODE_SSP, SSP_10M);
+		break;
+	}
+
+	if (duplex != DUPLEX_FULL)
+		val |= PM_IF_MODE_HD;
+
+	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
+}
+
+static void netc_port_set_rmii_mii_mac(struct netc_port *np,
+				       int speed, int duplex)
+{
+	u32 mask, val = 0;
+
+	mask = PM_IF_MODE_SSP | PM_IF_MODE_HD | PM_IF_MODE_M10;
+
+	if (speed == SPEED_10)
+		val |= PM_IF_MODE_M10;
+
+	if (duplex != DUPLEX_FULL)
+		val |= PM_IF_MODE_HD;
+
+	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
+}
+
+static void netc_port_mac_rx_enable(struct netc_port *np)
+{
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, 0);
+	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN,
+			  PM_CMD_CFG_RX_EN);
+}
+
+static void netc_port_wait_rx_empty(struct netc_port *np, int mac)
+{
+	u32 val;
+
+	if (read_poll_timeout(netc_port_rd, val, val & PM_IEVENT_RX_EMPTY,
+			      100, 10000, false, np, NETC_PM_IEVENT(mac)))
+		dev_warn(np->switch_priv->dev,
+			 "MAC %d of swp%d RX is not empty\n", mac,
+			 np->dp->index);
+}
+
+static void netc_port_mac_rx_graceful_stop(struct netc_port *np)
+{
+	u32 val;
+
+	if (is_netc_pseudo_port(np))
+		goto check_rx_busy;
+
+	if (np->caps.pmac) {
+		netc_port_rmw(np, NETC_PM_CMD_CFG(1), PM_CMD_CFG_RX_EN, 0);
+		netc_port_wait_rx_empty(np, 1);
+	}
+
+	netc_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN, 0);
+	netc_port_wait_rx_empty(np, 0);
+
+check_rx_busy:
+	if (read_poll_timeout(netc_port_rd, val, !(val & PSR_RX_BUSY),
+			      100, 10000, false, np, NETC_PSR))
+		dev_warn(np->switch_priv->dev, "swp%d RX is busy\n",
+			 np->dp->index);
+
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, PCR_RXDIS);
+}
+
+static void netc_mac_link_up(struct phylink_config *config,
+			     struct phy_device *phy, unsigned int mode,
+			     phy_interface_t interface, int speed,
+			     int duplex, bool tx_pause, bool rx_pause)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct netc_port *np;
+
+	np = NETC_PORT(dp->ds, dp->index);
+	netc_port_set_speed(np, speed);
+
+	if (phy_interface_mode_is_rgmii(interface))
+		netc_port_set_rgmii_mac(np, speed, duplex);
+
+	if (interface == PHY_INTERFACE_MODE_RMII ||
+	    interface == PHY_INTERFACE_MODE_REVMII ||
+	    interface == PHY_INTERFACE_MODE_MII)
+		netc_port_set_rmii_mii_mac(np, speed, duplex);
+
+	netc_port_mac_rx_enable(np);
+}
+
+static void netc_mac_link_down(struct phylink_config *config,
+			       unsigned int mode,
+			       phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+
+	netc_port_mac_rx_graceful_stop(NETC_PORT(dp->ds, dp->index));
+}
+
+static const struct phylink_mac_ops netc_phylink_mac_ops = {
+	.mac_config		= netc_mac_config,
+	.mac_link_up		= netc_mac_link_up,
+	.mac_link_down		= netc_mac_link_down,
+};
+
 static const struct dsa_switch_ops netc_switch_ops = {
 	.get_tag_protocol		= netc_get_tag_protocol,
 	.setup				= netc_setup,
 	.teardown			= netc_teardown,
+	.phylink_get_caps		= netc_phylink_get_caps,
 };
 
 static int netc_switch_probe(struct pci_dev *pdev,
@@ -613,6 +791,7 @@ static int netc_switch_probe(struct pci_dev *pdev,
 	ds->num_ports = priv->info->num_ports;
 	ds->num_tx_queues = NETC_TC_NUM;
 	ds->ops = &netc_switch_ops;
+	ds->phylink_mac_ops = &netc_phylink_mac_ops;
 	ds->priv = priv;
 
 	priv->ds = ds;
drivers/net/dsa/netc/netc_platform.c +41 -0

49 lines not on this page · raw message

drivers/net/dsa/netc/netc_switch.h +4 -0
index dac19bfba02b..eb65c36ecead 100644
@@ -34,6 +34,7 @@ struct netc_switch;
 
 struct netc_switch_info {
 	u32 num_ports;
+	void (*phylink_get_caps)(int port, struct phylink_config *config);
 };
 
 struct netc_port_caps {
@@ -70,6 +71,9 @@ struct netc_switch {
 	struct ntmp_user ntmp;
 };
 
+#define NETC_PRIV(ds)			((struct netc_switch *)((ds)->priv))
+#define NETC_PORT(ds, port_id)		(NETC_PRIV(ds)->ports[(port_id)])
+
 /* Write/Read Switch base registers */
 #define netc_base_rd(r, o)		netc_read((r)->base + (o))
 #define netc_base_wr(r, o, v)		netc_write((r)->base + (o), v)
drivers/net/dsa/netc/netc_switch_hw.h +21 -0
index 11cb124ce4bf..881122004644 100644
@@ -71,6 +71,10 @@
 #define  PCR_TXDIS			BIT(0)
 #define  PCR_RXDIS			BIT(1)
 
+#define NETC_PSR			0x104
+#define  PSR_TX_BUSY			BIT(0)
+#define  PSR_RX_BUSY			BIT(1)
+
 #define NETC_PTCTMSDUR(a)		(0x208 + (a) * 0x20)
 #define  PTCTMSDUR_MAXSDU		GENMASK(15, 0)
 #define  PTCTMSDUR_SDU_TYPE		GENMASK(17, 16)
@@ -127,6 +131,23 @@ enum netc_mfo {
 #define NETC_PM_MAXFRM(a)		(0x1014 + (a) * 0x400)
 #define  PM_MAXFRAM			GENMASK(15, 0)
 
+#define NETC_PM_IEVENT(a)		(0x1040 + (a) * 0x400)
+#define  PM_IEVENT_RX_EMPTY		BIT(6)
+
+#define NETC_PM_IF_MODE(a)		(0x1300 + (a) * 0x400)
+#define  PM_IF_MODE_IFMODE		GENMASK(2, 0)
+#define   IFMODE_MII			1
+#define   IFMODE_RMII			3
+#define   IFMODE_RGMII			4
+#define   IFMODE_SGMII			5
+#define  PM_IF_MODE_REVMII		BIT(3)
+#define  PM_IF_MODE_M10			BIT(4)
+#define  PM_IF_MODE_HD			BIT(6)
+#define  PM_IF_MODE_SSP			GENMASK(14, 13)
+#define   SSP_100M			0
+#define   SSP_10M			1
+#define   SSP_1G			2
+
 #define NETC_PEMDIOCR			0x1c00
 #define NETC_EMDIO_BASE			NETC_PEMDIOCR
 
Signature · 1 line
2.34.1
maintainer
#29 lore raw

Re: [PATCH v4 net-next 11/14] net: dsa: netc: add phylink MAC operations

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: dsa: netc: add phylink MAC operations

Different versions of NETC switches have different numbers of ports and MAC capabilities, so add .phylink_get_caps() to struct netc_switch_info. In addition, related interfaces of struct phylink_mac_ops are added.

Quoted 4 lines
drivers/net/dsa/netc/netc_main.c +0 -0
index 5828fd3e342e..f11f5d0f6a6d 100644

[ ... ]

Quoted 9 lines
drivers/net/dsa/netc/netc_main.c +9 -0
+static void netc_port_set_mac_mode(struct netc_port *np,
+				   unsigned int mode,
+				   phy_interface_t phy_mode)
+{
+	u32 mask = PM_IF_MODE_IFMODE | PM_IF_MODE_REVMII;
+	u32 val = 0;
+
+	switch (phy_mode) {
+	case PHY_INTERFACE_MODE_RGMII:

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 612

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		val |= IFMODE_SGMII;
+		break;
+	default:
+		break;
+	}
+
+	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
+}

In imx94_switch_phylink_get_caps(), PHY_INTERFACE_MODE_1000BASEX is advertised as supported for ports 0 and 1. Does it need to be handled in this switch statement? As written, it appears it will fall through to the default case and clear the IFMODE bits, leaving the MAC in an undefined mode.

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 675

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c +6 -0
+static void netc_port_mac_rx_enable(struct netc_port *np)
+{
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, 0);
+	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN,
+			  PM_CMD_CFG_RX_EN);
+}

Pseudo ports (like the CPU port) bypass PMAC register accesses during graceful stop via is_netc_pseudo_port(). Should there be a similar check here, and in netc_port_set_mac_mode(), to prevent accessing physical MAC registers on pseudo ports?

Additionally, when np->caps.pmac is true, netc_port_mac_rx_graceful_stop() disables the preemption MAC via NETC_PM_CMD_CFG(1). Should this function re-enable it when bringing the link back up?

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 686

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+static void netc_port_wait_rx_empty(struct netc_port *np, int mac)
+{
+	u32 val;
+
+	if (read_poll_timeout(netc_port_rd, val, val & PM_IEVENT_RX_EMPTY,
+			      100, 10000, false, np, NETC_PM_IEVENT(mac)))
+		dev_warn(np->switch_priv->dev,
+			 "MAC %d of swp%d RX is not empty\n", mac,
+			 np->dp->index);
+}

Is NETC_PM_IEVENT a sticky write-1-to-clear register? If the RX FIFO had previously emptied during normal operation, could this bit already be set, causing read_poll_timeout() to return immediately without waiting?

Quoted 13 lines
drivers/net/dsa/netc/netc_main.c +13 -0
+static void netc_port_mac_rx_graceful_stop(struct netc_port *np)
+{
+	u32 val;
+
+	if (is_netc_pseudo_port(np))
+		goto check_rx_busy;
+
+	if (np->caps.pmac) {
+		netc_port_rmw(np, NETC_PM_CMD_CFG(1), PM_CMD_CFG_RX_EN, 0);
+		netc_port_wait_rx_empty(np, 1);
+	}
+
+	netc_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN, 0);

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 710

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	netc_port_wait_rx_empty(np, 0);
+
+check_rx_busy:
+	if (read_poll_timeout(netc_port_rd, val, !(val & PSR_RX_BUSY),
+			      100, 10000, false, np, NETC_PSR))
+		dev_warn(np->switch_priv->dev, "swp%d RX is busy\n",
+			 np->dp->index);
+
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, PCR_RXDIS);
+}

For pseudo ports, the code jumps directly to check_rx_busy and polls for PSR_RX_BUSY to clear. Since PCR_RXDIS is only set after the polling completes, will the RX pipeline continue accepting traffic and cause this poll to always timeout on an active link?

Quoted 11 lines
drivers/net/dsa/netc/netc_main.c +11 -0
+static void netc_mac_link_up(struct phylink_config *config,
+			     struct phy_device *phy, unsigned int mode,
+			     phy_interface_t interface, int speed,
+			     int duplex, bool tx_pause, bool rx_pause)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct netc_port *np;
+
+	np = NETC_PORT(dp->ds, dp->index);
+	netc_port_set_speed(np, speed);
+

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 732

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	if (phy_interface_mode_is_rgmii(interface))
+		netc_port_set_rgmii_mac(np, speed, duplex);
+
+	if (interface == PHY_INTERFACE_MODE_RMII ||
+	    interface == PHY_INTERFACE_MODE_REVMII ||
+	    interface == PHY_INTERFACE_MODE_MII)
+		netc_port_set_rmii_mii_mac(np, speed, duplex);
+
+	netc_port_mac_rx_enable(np);
+}

The callback imx94_switch_phylink_get_caps() advertises MAC_ASYM_PAUSE and MAC_SYM_PAUSE capabilities. Is it expected that tx_pause and rx_pause are ignored here? If flow control is implemented in a subsequent patch, should the advertisement be deferred until then?

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 741

Quoted 8 lines
drivers/net/dsa/netc/netc_main.c +8 -0
+static void netc_mac_link_down(struct phylink_config *config,
+			       unsigned int mode,
+			       phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+
+	netc_port_mac_rx_graceful_stop(NETC_PORT(dp->ds, dp->index));
+}

This correctly stops the RX pipeline, but should the TX path (PM_CMD_CFG_TX_EN or PCR_TXDIS) also be disabled when the link goes down?

author maintainer
#30 lore raw

RE: [PATCH v4 net-next 11/14] net: dsa: netc: add phylink MAC operations

Quoted 11 lines

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 675

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c +6 -0
+static void netc_port_mac_rx_enable(struct netc_port *np)
+{
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, 0);
+	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN,
+			  PM_CMD_CFG_RX_EN);
+}

Pseudo ports (like the CPU port) bypass PMAC register accesses during graceful stop via is_netc_pseudo_port(). Should there be a similar check here, and in netc_port_set_mac_mode(), to prevent accessing physical MAC registers on pseudo ports?

netc_mac_port_rmw() has checked whether the port is pseudo MAC. See below.

static void netc_mac_port_rmw(struct netc_port *np, u32 reg,

			      u32 mask, u32 val)
{
	u32 old, new;

	if (is_netc_pseudo_port(np))
		return;

	[...]
}
Quoted 4 lines

Additionally, when np->caps.pmac is true, netc_port_mac_rx_graceful_stop() disables the preemption MAC via NETC_PM_CMD_CFG(1). Should this function re-enable it when bringing the link back up?

netc_mac_port_rmw() also set preemption MAC if the port supports preemtion.

static void netc_mac_port_rmw(struct netc_port *np, u32 reg,

			      u32 mask, u32 val)
{
	[...]
	if (np->caps.pmac)
		netc_port_wr(np, reg + NETC_PMAC_OFFSET, new);
}
Quoted 15 lines

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 686

Quoted 11 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+static void netc_port_wait_rx_empty(struct netc_port *np, int mac)
+{
+	u32 val;
+
+	if (read_poll_timeout(netc_port_rd, val, val & PM_IEVENT_RX_EMPTY,
+			      100, 10000, false, np, NETC_PM_IEVENT(mac)))
+		dev_warn(np->switch_priv->dev,
+			 "MAC %d of swp%d RX is not empty\n", mac,
+			 np->dp->index);
+}

Is NETC_PM_IEVENT a sticky write-1-to-clear register? If the RX FIFO had previously emptied during normal operation, could this bit already be set, causing read_poll_timeout() to return immediately without waiting?

NETC_PM_IEVENT[RX_EMPTY] is a read only bit. It will be cleared if Rx FIFO not empty or Rx packet receive in process. And it will be set if Rx FIFO is empty and no Rx packet receive in process.

Quoted 22 lines

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv)

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +9 -0
+static void netc_port_mac_rx_graceful_stop(struct netc_port *np)
+{
+	u32 val;
+
+	if (is_netc_pseudo_port(np))
+		goto check_rx_busy;
+
+	if (np->caps.pmac) {
+		netc_port_rmw(np, NETC_PM_CMD_CFG(1), PM_CMD_CFG_RX_EN,

0);

Quoted 11 lines
drivers/net/dsa/netc/netc_main.c +11 -0
+		netc_port_wait_rx_empty(np, 1);
+	}
+
+	netc_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_RX_EN, 0);
+	netc_port_wait_rx_empty(np, 0);
+
+check_rx_busy:
+	if (read_poll_timeout(netc_port_rd, val, !(val & PSR_RX_BUSY),
+			      100, 10000, false, np, NETC_PSR))
+		dev_warn(np->switch_priv->dev, "swp%d RX is busy\n",
+			 np->dp->index);
Quoted 8 lines

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 710

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +3 -0
+
+	netc_port_rmw(np, NETC_POR, PCR_RXDIS, PCR_RXDIS);
+}

For pseudo ports, the code jumps directly to check_rx_busy and polls for PSR_RX_BUSY to clear. Since PCR_RXDIS is only set after the polling completes, will the RX pipeline continue accepting traffic and cause this poll to always timeout on an active link?

For the pseudo ports, no need to check the PSR_RX_BUSY bit, only need to set PCR_RXDIS bit to discard RX packets. I will improve it.

Quoted 20 lines
Quoted 20 lines
drivers/net/dsa/netc/netc_main.c +19 -0
+static void netc_mac_link_up(struct phylink_config *config,
+			     struct phy_device *phy, unsigned int mode,
+			     phy_interface_t interface, int speed,
+			     int duplex, bool tx_pause, bool rx_pause)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct netc_port *np;
+
+	np = NETC_PORT(dp->ds, dp->index);
+	netc_port_set_speed(np, speed);
+
+	if (phy_interface_mode_is_rgmii(interface))
+		netc_port_set_rgmii_mac(np, speed, duplex);
+
+	if (interface == PHY_INTERFACE_MODE_RMII ||
+	    interface == PHY_INTERFACE_MODE_REVMII ||
+	    interface == PHY_INTERFACE_MODE_MII)
+		netc_port_set_rmii_mii_mac(np, speed, duplex);
+
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	netc_port_mac_rx_enable(np);
+}

The callback imx94_switch_phylink_get_caps() advertises MAC_ASYM_PAUSE and
MAC_SYM_PAUSE capabilities. Is it expected that tx_pause and rx_pause are ignored here? If flow control is implemented in a subsequent patch, should the advertisement be deferred until then?

Okay, I will move the advertisement to the subsequent patch.

author maintainer
#31 lore raw

PATCH v4 12/14 net: dsa: netc: add more basic functions support

In reply to Wei Fang

This patch expands the NETC switch driver with several foundational features, including FDB and MDB management, STP state handling, MTU configuration, port setup/teardown, and host flooding support.

At this stage, the driver operates only in standalone port mode. Each port uses VLAN 0 as its PVID, meaning ingress frames are internally assigned VID 0 regardless of whether they arrive tagged or untagged. Note that this does not inject a VLAN 0 header into the frame, the VID is used purely for subsequent VLAN processing within the switch.

---
 drivers/net/dsa/netc/netc_main.c      | 540 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_switch.h    |  33 ++
 drivers/net/dsa/netc/netc_switch_hw.h |  11 +
 3 files changed, 584 insertions(+)
drivers/net/dsa/netc/netc_main.c +540 -0
index f11f5d0f6a6d..3609d83ac363 100644
@@ -7,11 +7,36 @@
 #include <linux/clk.h>
 #include <linux/etherdevice.h>
 #include <linux/fsl/enetc_mdio.h>
+#include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
 #include <linux/of_mdio.h>
 
 #include "netc_switch.h"
 
+static struct netc_fdb_entry *
+netc_lookup_fdb_entry(struct netc_switch *priv,
+		      const unsigned char *addr,
+		      u16 vid)
+{
+	struct netc_fdb_entry *entry;
+
+	hlist_for_each_entry(entry, &priv->fdb_list, node)
+		if (ether_addr_equal(entry->keye.mac_addr, addr) &&
+		    le16_to_cpu(entry->keye.fid) == vid)
+			return entry;
+
+	return NULL;
+}
+
+static void netc_destroy_fdb_list(struct netc_switch *priv)
+{
+	struct netc_fdb_entry *entry;
+	struct hlist_node *tmp;
+
+	hlist_for_each_entry_safe(entry, tmp, &priv->fdb_list, node)
+		netc_del_fdb_entry(entry);
+}
+
 static enum dsa_tag_protocol
 netc_get_tag_protocol(struct dsa_switch *ds, int port,
 		      enum dsa_tag_protocol mprot)
@@ -386,6 +411,212 @@ static void netc_port_default_config(struct netc_port *np)
 	netc_port_rmw(np, NETC_POR, PCR_TXDIS, 0);
 }
 
+static u32 netc_available_port_bitmap(struct netc_switch *priv)
+{
+	struct dsa_port *dp;
+	u32 bitmap = 0;
+
+	dsa_switch_for_each_available_port(dp, priv->ds)
+		bitmap |= BIT(dp->index);
+
+	return bitmap;
+}
+
+static int netc_add_standalone_vlan_entry(struct netc_switch *priv)
+{
+	u32 bitmap_stg = VFT_STG_ID(0) | netc_available_port_bitmap(priv);
+	struct vft_cfge_data *cfge;
+	u16 cfg;
+	int err;
+
+	cfge = kzalloc_obj(*cfge);
+	if (!cfge)
+		return -ENOMEM;
+
+	cfge->bitmap_stg = cpu_to_le32(bitmap_stg);
+	cfge->et_eid = cpu_to_le32(NTMP_NULL_ENTRY_ID);
+	cfge->fid = cpu_to_le16(NETC_STANDALONE_PVID);
+
+	/* For standalone ports, MAC learning needs to be disabled, so frames
+	 * from other user ports will not be forwarded to the standalone ports,
+	 * because there are no FDB entries on the standalone ports. Also, the
+	 * frames received by the standalone ports cannot be flooded to other
+	 * ports, so MAC forwarding option needs to be set to
+	 * MFO_NO_MATCH_DISCARD, so the frames will discarded rather than
+	 * flooding to other ports.
+	 */
+	cfg = FIELD_PREP(VFT_MLO, MLO_DISABLE) |
+	      FIELD_PREP(VFT_MFO, MFO_NO_MATCH_DISCARD);
+	cfge->cfg = cpu_to_le16(cfg);
+
+	err = ntmp_vft_add_entry(&priv->ntmp, NETC_STANDALONE_PVID, cfge);
+	if (err)
+		dev_err(priv->dev,
+			"Failed to add standalone VLAN entry\n");
+
+	kfree(cfge);
+
+	return err;
+}
+
+static int netc_port_add_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct netc_fdb_entry *entry;
+	struct fdbt_keye_data *keye;
+	struct fdbt_cfge_data *cfge;
+	int port = np->dp->index;
+	u32 cfg = 0;
+	int err;
+
+	entry = kzalloc_obj(*entry);
+	if (!entry)
+		return -ENOMEM;
+
+	keye = &entry->keye;
+	cfge = &entry->cfge;
+	ether_addr_copy(keye->mac_addr, addr);
+	keye->fid = cpu_to_le16(vid);
+
+	cfge->port_bitmap = cpu_to_le32(BIT(port));
+	cfge->cfg = cpu_to_le32(cfg);
+	cfge->et_eid = cpu_to_le32(NTMP_NULL_ENTRY_ID);
+
+	err = ntmp_fdbt_add_entry(&priv->ntmp, &entry->entry_id, keye, cfge);
+	if (err) {
+		kfree(entry);
+
+		return err;
+	}
+
+	netc_add_fdb_entry(priv, entry);
+
+	return 0;
+}
+
+static int netc_port_set_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct netc_fdb_entry *entry;
+	int port = np->dp->index;
+	u32 port_bitmap;
+	int err = 0;
+
+	mutex_lock(&priv->fdbt_lock);
+
+	entry = netc_lookup_fdb_entry(priv, addr, vid);
+	if (!entry) {
+		err = netc_port_add_fdb_entry(np, addr, vid);
+		if (err)
+			dev_err(priv->dev,
+				"Failed to add FDB entry on port %d\n",
+				port);
+
+		goto unlock_fdbt;
+	}
+
+	port_bitmap = le32_to_cpu(entry->cfge.port_bitmap);
+	/* If the entry already exists on the port, return 0 directly */
+	if (unlikely(port_bitmap & BIT(port)))
+		goto unlock_fdbt;
+
+	/* If the entry already exists, but not on this port, we need to
+	 * update the port bitmap. In general, it should only be valid
+	 * for multicast or broadcast address.
+	 */
+	port_bitmap ^= BIT(port);
+	entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);
+	err = ntmp_fdbt_update_entry(&priv->ntmp, entry->entry_id,
+				     &entry->cfge);
+	if (err) {
+		port_bitmap ^= BIT(port);
+		entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);
+		dev_err(priv->dev, "Failed to set FDB entry on port %d\n",
+			port);
+	}
+
+unlock_fdbt:
+	mutex_unlock(&priv->fdbt_lock);
+
+	return err;
+}
+
+static int netc_port_del_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct ntmp_user *ntmp = &priv->ntmp;
+	struct netc_fdb_entry *entry;
+	int port = np->dp->index;
+	u32 port_bitmap;
+	int err = 0;
+
+	mutex_lock(&priv->fdbt_lock);
+
+	entry = netc_lookup_fdb_entry(priv, addr, vid);
+	if (unlikely(!entry))
+		goto unlock_fdbt;
+
+	port_bitmap = le32_to_cpu(entry->cfge.port_bitmap);
+	if (unlikely(!(port_bitmap & BIT(port))))
+		goto unlock_fdbt;
+
+	if (port_bitmap != BIT(port)) {
+		/* If the entry also exists on other ports, we need to
+		 * update the entry in the FDB table.
+		 */
+		port_bitmap ^= BIT(port);
+		entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);
+		err = ntmp_fdbt_update_entry(ntmp, entry->entry_id,
+					     &entry->cfge);
+		if (err) {
+			port_bitmap ^= BIT(port);
+			entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);
+			goto unlock_fdbt;
+		}
+	} else {
+		/* If the entry only exists on this port, just delete
+		 * it from the FDB table.
+		 */
+		err = ntmp_fdbt_delete_entry(ntmp, entry->entry_id);
+		if (err)
+			goto unlock_fdbt;
+
+		netc_del_fdb_entry(entry);
+	}
+
+unlock_fdbt:
+	mutex_unlock(&priv->fdbt_lock);
+
+	return err;
+}
+
+static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv)
+{
+	const u8 bcast[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+	struct dsa_port *dp, *cpu_dp = NULL;
+
+	dsa_switch_for_each_cpu_port(dp, priv->ds) {
+		cpu_dp = dp;
+		break;
+	}
+
+	if (!cpu_dp)
+		return -ENODEV;
+
+	/* If the user port acts as a standalone port, then its PVID is 0,
+	 * MLO is set to "disable MAC learning" and MFO is set to "discard
+	 * frames if no matching entry found in FDB table". Therefore, we
+	 * need to add a broadcast FDB entry on the CPU port so that the
+	 * broadcast frames received on the user port can be forwarded to
+	 * the CPU port.
+	 */
+	return netc_port_set_fdb_entry(NETC_PORT(priv->ds, cpu_dp->index),
+				       bcast, NETC_STANDALONE_PVID);
+}
+
 static int netc_setup(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
@@ -404,19 +635,61 @@ static int netc_setup(struct dsa_switch *ds)
 	if (err)
 		return err;
 
+	INIT_HLIST_HEAD(&priv->fdb_list);
+	mutex_init(&priv->fdbt_lock);
+
 	netc_switch_fixed_config(priv);
 
 	/* default setting for ports */
 	dsa_switch_for_each_available_port(dp, ds)
 		netc_port_default_config(priv->ports[dp->index]);
 
+	err = netc_add_standalone_vlan_entry(priv);
+	if (err)
+		goto free_lock_and_ntmp_user;
+
+	err = netc_add_standalone_fdb_bcast_entry(priv);
+	if (err)
+		goto free_lock_and_ntmp_user;
+
 	return 0;
+
+free_lock_and_ntmp_user:
+	mutex_destroy(&priv->fdbt_lock);
+	netc_free_ntmp_user(priv);
+
+	return err;
+}
+
+static void netc_destroy_all_lists(struct netc_switch *priv)
+{
+	netc_destroy_fdb_list(priv);
+	mutex_destroy(&priv->fdbt_lock);
+}
+
+static void netc_free_host_flood_rules(struct netc_switch *priv)
+{
+	struct dsa_port *dp;
+
+	dsa_switch_for_each_user_port(dp, priv->ds) {
+		struct netc_port *np = priv->ports[dp->index];
+
+		/* No need to clear the hardware IPFT entry. Because PCIe
+		 * FLR will be performed when the switch is re-registered,
+		 * it will reset hardware state. So only need to free the
+		 * memory to avoid memory leak.
+		 */
+		kfree(np->host_flood);
+		np->host_flood = NULL;
+	}
 }
 
 static void netc_teardown(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
 
+	netc_destroy_all_lists(priv);
+	netc_free_host_flood_rules(priv);
 	netc_free_ntmp_user(priv);
 }
 
@@ -569,6 +842,261 @@ static void netc_switch_get_ip_revision(struct netc_switch *priv)
 	priv->revision = val & IPBRR0_IP_REV;
 }
 
+static int netc_port_enable(struct dsa_switch *ds, int port,
+			    struct phy_device *phy)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+	int err;
+
+	if (np->enable)
+		return 0;
+
+	err = clk_prepare_enable(np->ref_clk);
+	if (err) {
+		dev_err(ds->dev,
+			"Failed to enable enet_ref_clk of port %d\n", port);
+		return err;
+	}
+
+	np->enable = true;
+
+	return 0;
+}
+
+static void netc_port_disable(struct dsa_switch *ds, int port)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	/* When .port_disable() is called, .port_enable() may not have been
+	 * called. In this case, both the prepare_count and enable_count of
+	 * clock are 0. Calling clk_disable_unprepare() at this time will
+	 * cause warnings.
+	 */
+	if (!np->enable)
+		return;
+
+	clk_disable_unprepare(np->ref_clk);
+	np->enable = false;
+}
+
+static void netc_port_stp_state_set(struct dsa_switch *ds,
+				    int port, u8 state)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+	u32 val;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+	case BR_STATE_LISTENING:
+	case BR_STATE_BLOCKING:
+		val = NETC_STG_STATE_DISABLED;
+		break;
+	case BR_STATE_LEARNING:
+		val = NETC_STG_STATE_LEARNING;
+		break;
+	case BR_STATE_FORWARDING:
+		val = NETC_STG_STATE_FORWARDING;
+		break;
+	default:
+		return;
+	}
+
+	netc_port_wr(np, NETC_BPSTGSR, val);
+}
+
+static int netc_port_change_mtu(struct dsa_switch *ds,
+				int port, int mtu)
+{
+	u32 max_frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (dsa_is_cpu_port(ds, port))
+		max_frame_size += NETC_TAG_MAX_LEN;
+
+	netc_port_set_max_frame_size(np, max_frame_size);
+
+	return 0;
+}
+
+static int netc_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	return NETC_MAX_FRAME_LEN - VLAN_ETH_HLEN - ETH_FCS_LEN;
+}
+
+static int netc_port_fdb_add(struct dsa_switch *ds, int port,
+			     const unsigned char *addr, u16 vid,
+			     struct dsa_db db)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	/* Currently, we only support standalone port mode, so all VLANs
+	 * should be converted to NETC_STANDALONE_PVID.
+	 */
+	return netc_port_set_fdb_entry(np, addr, NETC_STANDALONE_PVID);
+}
+
+static int netc_port_fdb_del(struct dsa_switch *ds, int port,
+			     const unsigned char *addr, u16 vid,
+			     struct dsa_db db)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	return netc_port_del_fdb_entry(np, addr, NETC_STANDALONE_PVID);
+}
+
+static int netc_port_fdb_dump(struct dsa_switch *ds, int port,
+			      dsa_fdb_dump_cb_t *cb, void *data)
+{
+	struct netc_switch *priv = ds->priv;
+	u32 resume_eid = NTMP_NULL_ENTRY_ID;
+	struct fdbt_entry_data *entry;
+	struct fdbt_keye_data *keye;
+	struct fdbt_cfge_data *cfge;
+	bool is_static;
+	u32 cfg;
+	int err;
+	u16 vid;
+
+	entry = kmalloc_obj(*entry);
+	if (!entry)
+		return -ENOMEM;
+
+	keye = &entry->keye;
+	cfge = &entry->cfge;
+	mutex_lock(&priv->fdbt_lock);
+
+	do {
+		memset(entry, 0, sizeof(*entry));
+		err = ntmp_fdbt_search_port_entry(&priv->ntmp, port,
+						  &resume_eid, entry);
+		if (err || entry->entry_id == NTMP_NULL_ENTRY_ID)
+			break;
+
+		cfg = le32_to_cpu(cfge->cfg);
+		is_static = (cfg & FDBT_DYNAMIC) ? false : true;
+		vid = le16_to_cpu(keye->fid);
+
+		err = cb(keye->mac_addr, vid, is_static, data);
+		if (err)
+			break;
+	} while (resume_eid != NTMP_NULL_ENTRY_ID);
+
+	mutex_unlock(&priv->fdbt_lock);
+	kfree(entry);
+
+	return err;
+}
+
+static int netc_port_mdb_add(struct dsa_switch *ds, int port,
+			     const struct switchdev_obj_port_mdb *mdb,
+			     struct dsa_db db)
+{
+	return netc_port_fdb_add(ds, port, mdb->addr, mdb->vid, db);
+}
+
+static int netc_port_mdb_del(struct dsa_switch *ds, int port,
+			     const struct switchdev_obj_port_mdb *mdb,
+			     struct dsa_db db)
+{
+	return netc_port_fdb_del(ds, port, mdb->addr, mdb->vid, db);
+}
+
+static int netc_port_add_host_flood_rule(struct netc_port *np,
+					 bool uc, bool mc)
+{
+	const u8 dmac_mask[ETH_ALEN] = {0x1, 0, 0, 0, 0, 0};
+	struct netc_switch *priv = np->switch_priv;
+	struct ipft_entry_data *host_flood;
+	struct ipft_keye_data *keye;
+	struct ipft_cfge_data *cfge;
+	u16 src_port;
+	u32 cfg;
+	int err;
+
+	if (!uc && !mc)
+		return 0;
+
+	host_flood = kzalloc_obj(*host_flood);
+	if (!host_flood)
+		return -ENOMEM;
+
+	keye = &host_flood->keye;
+	cfge = &host_flood->cfge;
+
+	src_port = FIELD_PREP(IPFT_SRC_PORT, np->dp->index);
+	src_port |= IPFT_SRC_PORT_MASK;
+	keye->src_port = cpu_to_le16(src_port);
+
+	/* If either only unicast or only multicast need to be flooded
+	 * to the host, we always set the mask that tests the first MAC
+	 * DA octet. The value should be 0 for the first bit (if unicast
+	 * has to be flooded) or 1 (if multicast). If both unicast and
+	 * multicast have to be flooded, we leave the key mask empty, so
+	 * it matches everything.
+	 */
+	if (uc && !mc)
+		ether_addr_copy(keye->dmac_mask, dmac_mask);
+
+	if (!uc && mc) {
+		ether_addr_copy(keye->dmac, dmac_mask);
+		ether_addr_copy(keye->dmac_mask, dmac_mask);
+	}
+
+	cfg = FIELD_PREP(IPFT_FLTFA, IPFT_FLTFA_REDIRECT);
+	cfg |= FIELD_PREP(IPFT_HR, NETC_HR_HOST_FLOOD);
+	cfge->cfg = cpu_to_le32(cfg);
+
+	err = ntmp_ipft_add_entry(&priv->ntmp, host_flood);
+	if (err) {
+		kfree(host_flood);
+		return err;
+	}
+
+	np->uc = uc;
+	np->mc = mc;
+	np->host_flood = host_flood;
+	/* Enable ingress port filter table lookup */
+	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
+
+	return 0;
+}
+
+static void netc_port_remove_host_flood(struct netc_port *np)
+{
+	struct netc_switch *priv = np->switch_priv;
+
+	if (!np->host_flood)
+		return;
+
+	ntmp_ipft_delete_entry(&priv->ntmp, np->host_flood->entry_id);
+	kfree(np->host_flood);
+	np->host_flood = NULL;
+	np->uc = false;
+	np->mc = false;
+	/* Disable ingress port filter table lookup */
+	netc_port_wr(np, NETC_PIPFCR, 0);
+}
+
+static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
+				     bool uc, bool mc)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (np->uc == uc && np->mc == mc)
+		return;
+
+	/* IPFT does not support in-place updates to the KEYE element,
+	 * so we need to delete the old IPFT entry and then add a new
+	 * one.
+	 */
+	if (np->host_flood)
+		netc_port_remove_host_flood(np);
+
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
+			port);
+}
+
 static void netc_phylink_get_caps(struct dsa_switch *ds, int port,
 				  struct phylink_config *config)
 {
@@ -751,6 +1279,17 @@ static const struct dsa_switch_ops netc_switch_ops = {
 	.setup				= netc_setup,
 	.teardown			= netc_teardown,
 	.phylink_get_caps		= netc_phylink_get_caps,
+	.port_enable			= netc_port_enable,
+	.port_disable			= netc_port_disable,
+	.port_stp_state_set		= netc_port_stp_state_set,
+	.port_change_mtu		= netc_port_change_mtu,
+	.port_max_mtu			= netc_port_max_mtu,
+	.port_fdb_add			= netc_port_fdb_add,
+	.port_fdb_del			= netc_port_fdb_del,
+	.port_fdb_dump			= netc_port_fdb_dump,
+	.port_mdb_add			= netc_port_mdb_add,
+	.port_mdb_del			= netc_port_mdb_del,
+	.port_set_host_flood		= netc_port_set_host_flood,
 };
 
 static int netc_switch_probe(struct pci_dev *pdev,
@@ -792,6 +1331,7 @@ static int netc_switch_probe(struct pci_dev *pdev,
 	ds->num_tx_queues = NETC_TC_NUM;
 	ds->ops = &netc_switch_ops;
 	ds->phylink_mac_ops = &netc_phylink_mac_ops;
+	ds->fdb_isolation = true;
 	ds->priv = priv;
 
 	priv->ds = ds;
drivers/net/dsa/netc/netc_switch.h +33 -0
index eb65c36ecead..4b229a71578e 100644
@@ -30,6 +30,8 @@
 
 #define NETC_MAX_FRAME_LEN		9600
 
+#define NETC_STANDALONE_PVID		0
+
 struct netc_switch;
 
 struct netc_switch_info {
@@ -43,6 +45,11 @@ struct netc_port_caps {
 	u32 pseudo_link:1;
 };
 
+enum netc_host_reason {
+	/* Software defined host reasons */
+	NETC_HR_HOST_FLOOD = 8,
+};
+
 struct netc_port {
 	void __iomem *iobase;
 	struct netc_switch *switch_priv;
@@ -50,6 +57,11 @@ struct netc_port {
 	struct dsa_port *dp;
 	struct clk *ref_clk; /* RGMII/RMII reference clock */
 	struct mii_bus *emdio;
+
+	u16 enable:1;
+	u16 uc:1;
+	u16 mc:1;
+	struct ipft_entry_data *host_flood;
 };
 
 struct netc_switch_regs {
@@ -58,6 +70,13 @@ struct netc_switch_regs {
 	void __iomem *global;
 };
 
+struct netc_fdb_entry {
+	u32 entry_id;
+	struct fdbt_cfge_data cfge;
+	struct fdbt_keye_data keye;
+	struct hlist_node node;
+};
+
 struct netc_switch {
 	struct pci_dev *pdev;
 	struct device *dev;
@@ -69,6 +88,8 @@ struct netc_switch {
 	struct netc_port **ports;
 
 	struct ntmp_user ntmp;
+	struct hlist_head fdb_list;
+	struct mutex fdbt_lock; /* FDB table lock */
 };
 
 #define NETC_PRIV(ds)			((struct netc_switch *)((ds)->priv))
@@ -91,6 +112,18 @@ static inline bool is_netc_pseudo_port(struct netc_port *np)
 	return np->caps.pseudo_link;
 }
 
+static inline void netc_add_fdb_entry(struct netc_switch *priv,
+				      struct netc_fdb_entry *entry)
+{
+	hlist_add_head(&entry->node, &priv->fdb_list);
+}
+
+static inline void netc_del_fdb_entry(struct netc_fdb_entry *entry)
+{
+	hlist_del(&entry->node);
+	kfree(entry);
+}
+
 int netc_switch_platform_probe(struct netc_switch *priv);
 
 #endif
drivers/net/dsa/netc/netc_switch_hw.h +11 -0
index 881122004644..c6a0c0a8ff8a 100644
@@ -67,6 +67,9 @@
 #define  PQOSMR_VQMP			GENMASK(19, 16)
 #define  PQOSMR_QVMP			GENMASK(23, 20)
 
+#define NETC_PIPFCR			0x0084
+#define  PIPFCR_EN			BIT(0)
+
 #define NETC_POR			0x100
 #define  PCR_TXDIS			BIT(0)
 #define  PCR_RXDIS			BIT(1)
@@ -122,6 +125,14 @@ enum netc_mfo {
 #define  BPDVR_RXVAM			BIT(24)
 #define  BPDVR_TXTAGA			GENMASK(26, 25)
 
+#define NETC_BPSTGSR			0x520
+
+enum netc_stg_stage {
+	NETC_STG_STATE_DISABLED = 0,
+	NETC_STG_STATE_LEARNING,
+	NETC_STG_STATE_FORWARDING,
+};
+
 /* Definition of Switch ethernet MAC port registers */
 #define NETC_PMAC_OFFSET		0x400
 #define NETC_PM_CMD_CFG(a)		(0x1008 + (a) * 0x400)
Signature · 1 line
2.34.1
maintainer
#32 lore raw

Re: [PATCH v4 net-next 12/14] net: dsa: netc: add more basic functions support

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: dsa: netc: add more basic functions support

This patch expands the NETC switch driver with several foundational features, including FDB and MDB management, STP state handling, MTU configuration, port setup/teardown, and host flooding support.

At this stage, the driver operates only in standalone port mode. Each port uses VLAN 0 as its PVID, meaning ingress frames are internally assigned VID 0 regardless of whether they arrive tagged or untagged. Note that this does not inject a VLAN 0 header into the frame, the VID is used purely for subsequent VLAN processing within the switch.

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +0 -0

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 427

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +3 -0
+static int netc_add_standalone_vlan_entry(struct netc_switch *priv)
+{
+	u32 bitmap_stg = VFT_STG_ID(0) | netc_available_port_bitmap(priv);

Does putting all standalone ports into a single shared VLAN 0 break the DSA isolation requirements?

If a user manually adds a static FDB entry to a standalone port, it is programmed into VLAN 0. Because all standalone ports reside in this VLAN, a frame arriving on one standalone port with a matching destination MAC might be hardware-switched directly to another standalone port, bypassing the CPU.

[ ... ]

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +3 -0
+static int netc_port_set_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 530

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c +6 -0
+	/* If the entry already exists, but not on this port, we need to
+	 * update the port bitmap. In general, it should only be valid
+	 * for multicast or broadcast address.
+	 */
+	port_bitmap ^= BIT(port);
+	entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);

Can this XOR operation cause multi-port floods during unicast MAC migrations?

When a unicast MAC address moves to a new port, the bridging layer issues an ADD for the new port. The XOR operation leaves the old port bit set alongside the new port bit. Does the driver need to differentiate between unicast and multicast addresses and explicitly overwrite the bitmap for unicast entries?

[ ... ]

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c +5 -0
+static int netc_port_del_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct ntmp_user *ntmp = &priv->ntmp;

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 560

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	struct netc_fdb_entry *entry;
+	int port = np->dp->index;
+	u32 port_bitmap;
+	int err = 0;
+
+	mutex_lock(&priv->fdbt_lock);
+
+	entry = netc_lookup_fdb_entry(priv, addr, vid);
+	if (unlikely(!entry))
+		goto unlock_fdbt;

Will this prevent dynamically learned FDB entries from being deleted?

Since the CPU port is configured with MLO_HW, the switch hardware dynamically learns MAC addresses, but they are not added to the software priv->fdb_list. If a user attempts to delete a dynamic entry, the software lookup will fail here and return 0 (success), leaving the stale dynamically learned entry active in hardware.

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 914

Quoted 8 lines
drivers/net/dsa/netc/netc_main.c +8 -0
+static int netc_port_change_mtu(struct dsa_switch *ds,
+				int port, int mtu)
+{
+	u32 max_frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (dsa_is_cpu_port(ds, port))
+		max_frame_size += NETC_TAG_MAX_LEN;

Does this double-count the DSA tag overhead for the CPU port?

The DSA core computes the CPU port's MTU by adding the tagger overhead before calling the switch's .port_change_mtu() function. If the maximum MTU of 9582 bytes is requested, the CPU port receives an MTU of 9596. Adding NETC_TAG_MAX_LEN again here results in 9628 bytes, which exceeds the hardware limit of 9600 (NETC_MAX_FRAME_LEN) and might lead to truncation.

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 936

Quoted 11 lines
drivers/net/dsa/netc/netc_main.c +11 -0
+static int netc_port_fdb_add(struct dsa_switch *ds, int port,
+			     const unsigned char *addr, u16 vid,
+			     struct dsa_db db)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	/* Currently, we only support standalone port mode, so all VLANs
+	 * should be converted to NETC_STANDALONE_PVID.
+	 */
+	return netc_port_set_fdb_entry(np, addr, NETC_STANDALONE_PVID);
+}

Is it safe to silently override user-requested VLAN IDs to VLAN 0?

If a user explicitly attempts to configure a MAC address for a specific VLAN, the hardware is programmed for VLAN 0 and returns success. Should this instead check if the requested vid is not 0 and not NETC_STANDALONE_PVID, and return -EOPNOTSUPP to inform the networking stack?

[ ... ]

Quoted 9 lines
drivers/net/dsa/netc/netc_main.c +9 -0
+static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
+				     bool uc, bool mc)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (np->uc == uc && np->mc == mc)
+		return;
+
+	/* IPFT does not support in-place updates to the KEYE element,

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 1098

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	 * so we need to delete the old IPFT entry and then add a new
+	 * one.
+	 */
+	if (np->host_flood)
+		netc_port_remove_host_flood(np);
+
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
+			port);
+}

Could this permanently lose the host flood configuration if the new rule addition fails?

If the call to netc_port_add_host_flood_rule() fails (for example, due to -ENOMEM), the function logs an error and returns, leaving the port entirely without a host flood rule. Should the driver revert to the previous configuration on failure to avoid blackholing traffic meant for the CPU?

author maintainer
#33 lore raw

RE: [PATCH v4 net-next 12/14] net: dsa: netc: add more basic functions support

Quoted 11 lines

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 427

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +3 -0
+static int netc_add_standalone_vlan_entry(struct netc_switch *priv)
+{
+	u32 bitmap_stg = VFT_STG_ID(0) | netc_available_port_bitmap(priv);

Does putting all standalone ports into a single shared VLAN 0 break the DSA isolation requirements?

If a user manually adds a static FDB entry to a standalone port, it is programmed into VLAN 0. Because all standalone ports reside in this VLAN, a frame arriving on one standalone port with a matching destination MAC might be hardware-switched directly to another standalone port, bypassing the CPU.

I think the FDB entry can only be added to the CPU port if the port is a standalone port not a bridge port. So this case will not be true.

Quoted 18 lines

[ ... ]

Quoted 3 lines
drivers/net/dsa/netc/netc_main.c +3 -0
+static int netc_port_set_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 530

Quoted 6 lines
drivers/net/dsa/netc/netc_main.c +6 -0
+	/* If the entry already exists, but not on this port, we need to
+	 * update the port bitmap. In general, it should only be valid
+	 * for multicast or broadcast address.
+	 */
+	port_bitmap ^= BIT(port);
+	entry->cfge.port_bitmap = cpu_to_le32(port_bitmap);

Can this XOR operation cause multi-port floods during unicast MAC migrations?

Quoted 4 lines

When a unicast MAC address moves to a new port, the bridging layer issues an ADD for the new port. The XOR operation leaves the old port bit set alongside the new port bit. Does the driver need to differentiate between unicast and multicast addresses and explicitly overwrite the bitmap for unicast entries?

I think the old FDB entry should be removed first when a unicast MAC address moves to a new port rather than only adding a new entry. If AI refers to adding before deleting, then this problem will indeed occur, and it is unavoidable.

Quoted 21 lines

[ ... ]

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c +5 -0
+static int netc_port_del_fdb_entry(struct netc_port *np,
+				   const unsigned char *addr, u16 vid)
+{
+	struct netc_switch *priv = np->switch_priv;
+	struct ntmp_user *ntmp = &priv->ntmp;

drivers/net/dsa/netc/netc_main.c static void netc_port_default_config(struct netc_port *np) · line 560

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	struct netc_fdb_entry *entry;
+	int port = np->dp->index;
+	u32 port_bitmap;
+	int err = 0;
+
+	mutex_lock(&priv->fdbt_lock);
+
+	entry = netc_lookup_fdb_entry(priv, addr, vid);
+	if (unlikely(!entry))
+		goto unlock_fdbt;

Will this prevent dynamically learned FDB entries from being deleted?

Quoted 1 line

Since the CPU port is configured with MLO_HW, the switch hardware

Currently we only support single port mode, so only support static FDB entry. The MLO_HW should be removed from the code, I will improve the logic.

Quoted 19 lines

dynamically
learns MAC addresses, but they are not added to the software priv->fdb_list. If a user attempts to delete a dynamic entry, the software lookup will fail here and return 0 (success), leaving the stale dynamically learned entry active in hardware.

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 914

Quoted 8 lines
drivers/net/dsa/netc/netc_main.c +8 -0
+static int netc_port_change_mtu(struct dsa_switch *ds,
+				int port, int mtu)
+{
+	u32 max_frame_size = mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (dsa_is_cpu_port(ds, port))
+		max_frame_size += NETC_TAG_MAX_LEN;

Does this double-count the DSA tag overhead for the CPU port?

Quoted 6 lines

The DSA core computes the CPU port's MTU by adding the tagger overhead before
calling the switch's .port_change_mtu() function. If the maximum MTU of 9582 bytes is requested, the CPU port receives an MTU of 9596. Adding NETC_TAG_MAX_LEN again here results in 9628 bytes, which exceeds the hardware limit of 9600 (NETC_MAX_FRAME_LEN) and might lead to truncation.

It seems a bug, I will fix it.

Quoted 17 lines

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 936

Quoted 11 lines
drivers/net/dsa/netc/netc_main.c +11 -0
+static int netc_port_fdb_add(struct dsa_switch *ds, int port,
+			     const unsigned char *addr, u16 vid,
+			     struct dsa_db db)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	/* Currently, we only support standalone port mode, so all VLANs
+	 * should be converted to NETC_STANDALONE_PVID.
+	 */
+	return netc_port_set_fdb_entry(np, addr, NETC_STANDALONE_PVID);
+}

Is it safe to silently override user-requested VLAN IDs to VLAN 0?

Quoted 4 lines

If a user explicitly attempts to configure a MAC address for a specific VLAN, the hardware is programmed for VLAN 0 and returns success. Should this instead check if the requested vid is not 0 and not NETC_STANDALONE_PVID, and return -EOPNOTSUPP to inform the networking stack?

Okay, I will add a check.

Quoted 26 lines

[ ... ]

Quoted 9 lines
drivers/net/dsa/netc/netc_main.c +9 -0
+static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
+				     bool uc, bool mc)
+{
+	struct netc_port *np = NETC_PORT(ds, port);
+
+	if (np->uc == uc && np->mc == mc)
+		return;
+
+	/* IPFT does not support in-place updates to the KEYE element,

drivers/net/dsa/netc/netc_main.c static void netc_switch_get_ip_revision(struct netc_switch *priv) · line 1098

Quoted 10 lines
drivers/net/dsa/netc/netc_main.c +10 -0
+	 * so we need to delete the old IPFT entry and then add a new
+	 * one.
+	 */
+	if (np->host_flood)
+		netc_port_remove_host_flood(np);
+
+	if (netc_port_add_host_flood_rule(np, uc, mc))
+		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
+			port);
+}

Could this permanently lose the host flood configuration if the new rule addition fails?

Quoted 4 lines

If the call to netc_port_add_host_flood_rule() fails (for example, due to -ENOMEM), the function logs an error and returns, leaving the port entirely without a host flood rule. Should the driver revert to the previous configuration on failure to avoid blackholing traffic meant for the CPU?

Okay, I will improve it.

author maintainer
#34 lore raw

PATCH v4 13/14 net: dsa: netc: initialize buffer bool table and implement flow-control

In reply to Wei Fang

The buffer pool is a quantity of memory available for buffering a group of flows (e.g. frames having the same priority, frames received from the same port), while waiting to be transmitted on a port. The buffer pool tracks internal memory consumption with upper bound limits and optionally a non-shared portion when associated with a shared buffer pool. Currently the shared buffer pool is not supported, it will be added in the future.

For i.MX94, the switch has 4 ports and 8 buffer pools, so each port is allocated two buffer pools. For frames with priorities of 0 to 3, they will be mapped to the first buffer pool; For frames with priorities of 4 to 7, they will be mapped to the second buffer pool. Each buffer pool has a flow control on threshold and a flow control off threshold. By setting these threshold, add the flow control support to each port.

---
 drivers/net/dsa/netc/netc_main.c      | 133 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_switch.h    |   9 ++
 drivers/net/dsa/netc/netc_switch_hw.h |  13 +++
 3 files changed, 155 insertions(+)
drivers/net/dsa/netc/netc_main.c +133 -0
index 3609d83ac363..688dceb486c2 100644
@@ -379,6 +379,8 @@ static void netc_port_set_mlo(struct netc_port *np, enum netc_mlo mlo)
 
 static void netc_port_fixed_config(struct netc_port *np)
 {
+	u32 pqnt = 0xffff, qth = 0xff00;
+
 	/* Default IPV and DR setting */
 	netc_port_rmw(np, NETC_PQOSMR, PQOSMR_VS | PQOSMR_VE,
 		      PQOSMR_VS | PQOSMR_VE);
@@ -386,6 +388,15 @@ static void netc_port_fixed_config(struct netc_port *np)
 	/* Enable L2 and L3 DOS */
 	netc_port_rmw(np, NETC_PCR, PCR_L2DOSE | PCR_L3DOSE,
 		      PCR_L2DOSE | PCR_L3DOSE);
+
+	/* Set the quanta value of TX PAUSE frame */
+	netc_mac_port_wr(np, NETC_PM_PAUSE_QUANTA(0), pqnt);
+
+	/* When a quanta timer counts down and reaches this value,
+	 * the MAC sends a refresh PAUSE frame with the programmed
+	 * full quanta value if a pause condition still exists.
+	 */
+	netc_mac_port_wr(np, NETC_PM_PAUSE_TRHESH(0), qth);
 }
 
 static void netc_port_default_config(struct netc_port *np)
@@ -617,6 +628,87 @@ static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv)
 				       bcast, NETC_STANDALONE_PVID);
 }
 
+static u32 netc_get_buffer_pool_num(struct netc_switch *priv)
+{
+	/* The BPCAPR register is a read only register, the hardware
+	 * guarantees that the BPCAPR_NUM_BP field will not be 0.
+	 */
+	return netc_base_rd(&priv->regs, NETC_BPCAPR) & BPCAPR_NUM_BP;
+}
+
+static void netc_port_set_pbpmcr(struct netc_port *np, u64 mapping)
+{
+	u32 pbpmcr0 = lower_32_bits(mapping);
+	u32 pbpmcr1 = upper_32_bits(mapping);
+
+	netc_port_wr(np, NETC_PBPMCR0, pbpmcr0);
+	netc_port_wr(np, NETC_PBPMCR1, pbpmcr1);
+}
+
+static void netc_ipv_to_buffer_pool_mapping(struct netc_switch *priv)
+{
+	int bp_per_port = priv->num_bp / priv->info->num_ports;
+	int q = NETC_IPV_NUM / bp_per_port;
+	int r = NETC_IPV_NUM % bp_per_port;
+	int num = q + r;
+
+	/* IPV-to–buffer-pool mapping per port:
+	 * Each port is allocated 'bp_per_port' buffer pools and supports 8
+	 * IPVs, where a higher IPV indicates a higher frame priority. Each
+	 * IPV can be mapped to only one buffer pool.
+	 *
+	 * The mapping rule is as follows:
+	 * - The first 'num' IPVs share the port's first buffer pool (index
+	 * 'base_id').
+	 * - After that, every 'q' IPVs share one buffer pool, with pool
+	 * indices increasing sequentially.
+	 */
+	for (int i = 0; i < priv->info->num_ports; i++) {
+		u32 base_id = i * bp_per_port;
+		u32 bp_id = base_id;
+		u64 mapping = 0;
+
+		for (int ipv = 0; ipv < NETC_IPV_NUM; ipv++) {
+			/* Update the buffer pool index */
+			if (ipv >= num)
+				bp_id = base_id + ((ipv - num) / q) + 1;
+
+			mapping |= (u64)bp_id << (ipv * 8);
+		}
+
+		netc_port_set_pbpmcr(priv->ports[i], mapping);
+	}
+}
+
+static int netc_switch_bpt_default_config(struct netc_switch *priv)
+{
+	/* priv->num_bp is read from register, based on the NETC block
+	 * guide, its value is hardcoded to a non-zero value. And it
+	 * is greater than the number of ports (priv->info->num_ports).
+	 */
+	priv->num_bp = netc_get_buffer_pool_num(priv);
+	priv->bpt_list = devm_kcalloc(priv->dev, priv->num_bp,
+				      sizeof(struct bpt_cfge_data),
+				      GFP_KERNEL);
+	if (!priv->bpt_list)
+		return -ENOMEM;
+
+	/* Initialize the maximum threshold of each buffer pool entry */
+	for (int i = 0; i < priv->num_bp; i++) {
+		struct bpt_cfge_data *cfge = &priv->bpt_list[i];
+		int err;
+
+		cfge->max_thresh = cpu_to_le16(NETC_BP_THRESH);
+		err = ntmp_bpt_update_entry(&priv->ntmp, i, cfge);
+		if (err)
+			return err;
+	}
+
+	netc_ipv_to_buffer_pool_mapping(priv);
+
+	return 0;
+}
+
 static int netc_setup(struct dsa_switch *ds)
 {
 	struct netc_switch *priv = ds->priv;
@@ -644,6 +736,10 @@ static int netc_setup(struct dsa_switch *ds)
 	dsa_switch_for_each_available_port(dp, ds)
 		netc_port_default_config(priv->ports[dp->index]);
 
+	err = netc_switch_bpt_default_config(priv);
+	if (err)
+		goto free_lock_and_ntmp_user;
+
 	err = netc_add_standalone_vlan_entry(priv);
 	if (err)
 		goto free_lock_and_ntmp_user;
@@ -1195,6 +1291,41 @@ static void netc_port_set_rmii_mii_mac(struct netc_port *np,
 	netc_mac_port_rmw(np, NETC_PM_IF_MODE(0), mask, val);
 }
 
+static void netc_port_set_tx_pause(struct netc_port *np, bool tx_pause)
+{
+	struct netc_switch *priv = np->switch_priv;
+	int port = np->dp->index;
+	int i, j, num_bp;
+
+	num_bp = priv->num_bp / priv->info->num_ports;
+	for (i = 0, j = port * num_bp; i < num_bp; i++, j++) {
+		struct bpt_cfge_data *cfge = &priv->bpt_list[j];
+		struct bpt_cfge_data old_cfge = *cfge;
+
+		if (tx_pause) {
+			cfge->fc_on_thresh = cpu_to_le16(NETC_FC_THRESH_ON);
+			cfge->fc_off_thresh = cpu_to_le16(NETC_FC_THRESH_OFF);
+			cfge->fccfg_sbpen = FIELD_PREP(BPT_FC_CFG,
+						       BPT_FC_CFG_EN_BPFC);
+			cfge->fc_ports = cpu_to_le32(BIT(port));
+		} else {
+			cfge->fc_on_thresh = cpu_to_le16(0);
+			cfge->fc_off_thresh = cpu_to_le16(0);
+			cfge->fccfg_sbpen = 0;
+			cfge->fc_ports = cpu_to_le32(0);
+		}
+
+		if (ntmp_bpt_update_entry(&priv->ntmp, j, cfge))
+			*cfge = old_cfge;
+	}
+}
+
+static void netc_port_set_rx_pause(struct netc_port *np, bool rx_pause)
+{
+	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_PAUSE_IGN,
+			  rx_pause ? 0 : PM_CMD_CFG_PAUSE_IGN);
+}
+
 static void netc_port_mac_rx_enable(struct netc_port *np)
 {
 	netc_port_rmw(np, NETC_POR, PCR_RXDIS, 0);
@@ -1256,6 +1387,8 @@ static void netc_mac_link_up(struct phylink_config *config,
 	    interface == PHY_INTERFACE_MODE_MII)
 		netc_port_set_rmii_mii_mac(np, speed, duplex);
 
+	netc_port_set_tx_pause(np, tx_pause);
+	netc_port_set_rx_pause(np, rx_pause);
 	netc_port_mac_rx_enable(np);
 }
 
drivers/net/dsa/netc/netc_switch.h +9 -0
index 4b229a71578e..7ebffb136b2f 100644
@@ -32,6 +32,12 @@
 
 #define NETC_STANDALONE_PVID		0
 
+#define NETC_IPV_NUM			8
+/* MANT = bits 11:4, EXP = bits 3:0, threshold = MANT * 2 ^ EXP */
+#define NETC_BP_THRESH			0x334
+#define NETC_FC_THRESH_ON		0x533
+#define NETC_FC_THRESH_OFF		0x3c3
+
 struct netc_switch;
 
 struct netc_switch_info {
@@ -90,6 +96,9 @@ struct netc_switch {
 	struct ntmp_user ntmp;
 	struct hlist_head fdb_list;
 	struct mutex fdbt_lock; /* FDB table lock */
+
+	u32 num_bp;
+	struct bpt_cfge_data *bpt_list;
 };
 
 #define NETC_PRIV(ds)			((struct netc_switch *)((ds)->priv))
drivers/net/dsa/netc/netc_switch_hw.h +13 -0
index c6a0c0a8ff8a..1e1c0d279a21 100644
@@ -12,6 +12,12 @@
 #define NETC_SWITCH_DEVICE_ID		0xeef2
 
 /* Definition of Switch base registers */
+#define NETC_BPCAPR			0x0008
+#define  BPCAPR_NUM_BP			GENMASK(7, 0)
+
+#define NETC_PBPMCR0			0x0400
+#define NETC_PBPMCR1			0x0404
+
 #define NETC_CBDRMR(a)			(0x0800 + (a) * 0x30)
 #define NETC_CBDRBAR0(a)		(0x0810 + (a) * 0x30)
 #define NETC_CBDRBAR1(a)		(0x0814 + (a) * 0x30)
@@ -138,6 +144,7 @@ enum netc_stg_stage {
 #define NETC_PM_CMD_CFG(a)		(0x1008 + (a) * 0x400)
 #define  PM_CMD_CFG_TX_EN		BIT(0)
 #define  PM_CMD_CFG_RX_EN		BIT(1)
+#define  PM_CMD_CFG_PAUSE_IGN		BIT(8)
 
 #define NETC_PM_MAXFRM(a)		(0x1014 + (a) * 0x400)
 #define  PM_MAXFRAM			GENMASK(15, 0)
@@ -145,6 +152,12 @@ enum netc_stg_stage {
 #define NETC_PM_IEVENT(a)		(0x1040 + (a) * 0x400)
 #define  PM_IEVENT_RX_EMPTY		BIT(6)
 
+#define NETC_PM_PAUSE_QUANTA(a)		(0x1054 + (a) * 0x400)
+#define  PAUSE_QUANTA_PQNT		GENMASK(15, 0)
+
+#define NETC_PM_PAUSE_TRHESH(a)		(0x1064 + (a) * 0x400)
+#define  PAUSE_TRHESH_QTH		GENMASK(15, 0)
+
 #define NETC_PM_IF_MODE(a)		(0x1300 + (a) * 0x400)
 #define  PM_IF_MODE_IFMODE		GENMASK(2, 0)
 #define   IFMODE_MII			1
Signature · 1 line
2.34.1
maintainer
#35 lore raw

Re: [PATCH v4 net-next 13/14] net: dsa: netc: initialize buffer bool table and implement flow-control

This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible.
---
net: dsa: netc: initialize buffer bool table and implement flow-control

This commit adds buffer pool initialization and flow control support for the NXP netc switch. It maps frame priorities to specific buffer pools and configures transmission pause thresholds.

Quoted 4 lines
drivers/net/dsa/netc/netc_main.c +0 -0
index 3609d83ac363..688dceb486c2 100644

[ ... ]

drivers/net/dsa/netc/netc_main.c static void netc_port_fixed_config(struct netc_port *np) · line 399

Quoted 13 lines
drivers/net/dsa/netc/netc_main.c +9 -0
@@ -386,6 +388,15 @@ static void netc_port_fixed_config(struct netc_port *np)
 	/* Enable L2 and L3 DOS */
 	netc_port_rmw(np, NETC_PCR, PCR_L2DOSE | PCR_L3DOSE,
 		      PCR_L2DOSE | PCR_L3DOSE);
+
+	/* Set the quanta value of TX PAUSE frame */
+	netc_mac_port_wr(np, NETC_PM_PAUSE_QUANTA(0), pqnt);
+
+	/* When a quanta timer counts down and reaches this value,
+	 * the MAC sends a refresh PAUSE frame with the programmed
+	 * full quanta value if a pause condition still exists.
+	 */
+	netc_mac_port_wr(np, NETC_PM_PAUSE_TRHESH(0), qth);

This isn't a bug, but there's a typo in the macro name (TRHESH instead of THRESH).

[ ... ]

Quoted 7 lines
drivers/net/dsa/netc/netc_main.c +7 -0
+static u32 netc_get_buffer_pool_num(struct netc_switch *priv)
+{
+	/* The BPCAPR register is a read only register, the hardware
+	 * guarantees that the BPCAPR_NUM_BP field will not be 0.
+	 */
+	return netc_base_rd(&priv->regs, NETC_BPCAPR) & BPCAPR_NUM_BP;
+}

[ ... ]

drivers/net/dsa/netc/netc_main.c static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv) · line 652

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c +5 -0
+static void netc_ipv_to_buffer_pool_mapping(struct netc_switch *priv)
+{
+	int bp_per_port = priv->num_bp / priv->info->num_ports;
+	int q = NETC_IPV_NUM / bp_per_port;
+	int r = NETC_IPV_NUM % bp_per_port;

If the hardware register read in netc_get_buffer_pool_num() returns a value smaller than priv->info->num_ports (for instance, during a bus error or reset state), bp_per_port will evaluate to 0.

Can this lead to a division by zero panic here?

Should priv->num_bp be validated against the number of ports before performing this calculation to ensure the driver fails gracefully if the hardware returns an unexpected value?

[ ... ]

Quoted 4 lines
drivers/net/dsa/netc/netc_switch_hw.h +0 -0
index c6a0c0a8ff8a..1e1c0d279a21 100644

[ ... ]

drivers/net/dsa/netc/netc_switch_hw.h enum netc_stg_stage { · line 159

Quoted 9 lines
drivers/net/dsa/netc/netc_switch_hw.h +5 -0
@@ -145,6 +152,12 @@ enum netc_stg_stage {
 #define NETC_PM_IEVENT(a)		(0x1040 + (a) * 0x400)
 #define  PM_IEVENT_RX_EMPTY		BIT(6)
 
+#define NETC_PM_PAUSE_QUANTA(a)		(0x1054 + (a) * 0x400)
+#define  PAUSE_QUANTA_PQNT		GENMASK(15, 0)
+
+#define NETC_PM_PAUSE_TRHESH(a)		(0x1064 + (a) * 0x400)
+#define  PAUSE_TRHESH_QTH		GENMASK(15, 0)

This isn't a bug, but there's a typo in the macro names here as well (TRHESH instead of THRESH).

author maintainer
#36 lore raw

RE: [PATCH v4 net-next 13/14] net: dsa: netc: initialize buffer bool table and implement flow-control

Quoted 18 lines
Quoted 7 lines
drivers/net/dsa/netc/netc_main.c +7 -0
+static u32 netc_get_buffer_pool_num(struct netc_switch *priv)
+{
+	/* The BPCAPR register is a read only register, the hardware
+	 * guarantees that the BPCAPR_NUM_BP field will not be 0.
+	 */
+	return netc_base_rd(&priv->regs, NETC_BPCAPR) & BPCAPR_NUM_BP;
+}

[ ... ]

drivers/net/dsa/netc/netc_main.c static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv) · line 652

Quoted 5 lines
drivers/net/dsa/netc/netc_main.c +5 -0
+static void netc_ipv_to_buffer_pool_mapping(struct netc_switch *priv)
+{
+	int bp_per_port = priv->num_bp / priv->info->num_ports;
+	int q = NETC_IPV_NUM / bp_per_port;
+	int r = NETC_IPV_NUM % bp_per_port;

If the hardware register read in netc_get_buffer_pool_num() returns a value smaller than priv->info->num_ports (for instance, during a bus error or reset state), bp_per_port will evaluate to 0.

Quoted 5 lines

Can this lead to a division by zero panic here?

Should priv->num_bp be validated against the number of ports before performing this calculation to ensure the driver fails gracefully if the hardware returns an unexpected value?

If the hardware is so unstable and prone to such malfunctions, that would be terrible, but I believe it won't. The AI ​​review is being a bit too nitpicky. Anyway, I will ll add this check to avoid AI review warnings.

author maintainer
#37 lore raw

PATCH v4 14/14 net: dsa: netc: add support for the standardized counters

In reply to Wei Fang

Each user port of the NETC switch supports 802.3 basic and mandatory managed objects statistic counters and IETF Management Information Database (MIB) package (RFC2665) and Remote Network Monitoring (RMON) counters. And all of these counters are 64-bit registers. In addition, some user ports support preemption, so these ports have two MACs, MAC 0 is the express MAC (eMAC), MAC 1 is the preemptible MAC (pMAC). So for ports that support preemption, the statistics are the sum of the pMAC and eMAC statistics.

Note that the current switch driver does not support preemption, all frames are sent and received via the eMAC by default. The statistics read from the pMAC should be zero.

---
 drivers/net/dsa/netc/Makefile         |   2 +-
 drivers/net/dsa/netc/netc_ethtool.c   | 192 ++++++++++++++++++++++++++
 drivers/net/dsa/netc/netc_main.c      |   4 +
 drivers/net/dsa/netc/netc_switch.h    |  17 +++
 drivers/net/dsa/netc/netc_switch_hw.h | 153 ++++++++++++++++++++
 include/linux/fsl/netc_global.h       |   6 +
 6 files changed, 373 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/dsa/netc/netc_ethtool.c
drivers/net/dsa/netc/Makefile +1 -1
index 4a5767562574..f40b13c702e0 100644
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_NETC_SWITCH) += nxp-netc-switch.o
-nxp-netc-switch-objs := netc_main.o netc_platform.o
+nxp-netc-switch-objs := netc_main.o netc_platform.o netc_ethtool.o
drivers/net/dsa/netc/netc_ethtool.c new file +192 -0

192 lines not on this page · raw message

drivers/net/dsa/netc/netc_main.c +4 -0
index 688dceb486c2..8e58884139e8 100644
@@ -1423,6 +1423,10 @@ static const struct dsa_switch_ops netc_switch_ops = {
 	.port_mdb_add			= netc_port_mdb_add,
 	.port_mdb_del			= netc_port_mdb_del,
 	.port_set_host_flood		= netc_port_set_host_flood,
+	.get_pause_stats		= netc_port_get_pause_stats,
+	.get_rmon_stats			= netc_port_get_rmon_stats,
+	.get_eth_ctrl_stats		= netc_port_get_eth_ctrl_stats,
+	.get_eth_mac_stats		= netc_port_get_eth_mac_stats,
 };
 
 static int netc_switch_probe(struct pci_dev *pdev,
drivers/net/dsa/netc/netc_switch.h +17 -0
index 7ebffb136b2f..825601847c89 100644
@@ -70,6 +70,11 @@ struct netc_port {
 	struct ipft_entry_data *host_flood;
 };
 
+enum netc_port_mac {
+	NETC_PORT_EMAC = 0,
+	NETC_PORT_PMAC,
+};
+
 struct netc_switch_regs {
 	void __iomem *base;
 	void __iomem *port;
@@ -110,6 +115,7 @@ struct netc_switch {
 
 /* Write/Read registers of Switch Port (including pseudo MAC port) */
 #define netc_port_rd(p, o)		netc_read((p)->iobase + (o))
+#define netc_port_rd64(p, o)		netc_read64((p)->iobase + (o))
 #define netc_port_wr(p, o, v)		netc_write((p)->iobase + (o), v)
 
 /* Write/Read Switch global registers */
@@ -135,4 +141,15 @@ static inline void netc_del_fdb_entry(struct netc_fdb_entry *entry)
 
 int netc_switch_platform_probe(struct netc_switch *priv);
 
+/* ethtool APIs */
+void netc_port_get_pause_stats(struct dsa_switch *ds, int port,
+			       struct ethtool_pause_stats *pause_stats);
+void netc_port_get_rmon_stats(struct dsa_switch *ds, int port,
+			      struct ethtool_rmon_stats *rmon_stats,
+			      const struct ethtool_rmon_hist_range **ranges);
+void netc_port_get_eth_ctrl_stats(struct dsa_switch *ds, int port,
+				  struct ethtool_eth_ctrl_stats *ctrl_stats);
+void netc_port_get_eth_mac_stats(struct dsa_switch *ds, int port,
+				 struct ethtool_eth_mac_stats *mac_stats);
+
 #endif
drivers/net/dsa/netc/netc_switch_hw.h +153 -0

159 lines not on this page · raw message

include/linux/fsl/netc_global.h +6 -0
index fdecca8c90f0..5b8ff528d369 100644
@@ -5,6 +5,7 @@
 #define __NETC_GLOBAL_H
 
 #include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 
 static inline u32 netc_read(void __iomem *reg)
 {
@@ -16,4 +17,9 @@ static inline void netc_write(void __iomem *reg, u32 val)
 	iowrite32(val, reg);
 }
 
+static inline u64 netc_read64(void __iomem *reg)
+{
+	return ioread64(reg);
+}
+
 #endif
Signature · 1 line
2.34.1