Introduce EN7581 ethernet support
Subsystems (5)
Lorenzo Bianconi
maintainerv2 cover Introduce EN7581 ethernet support
Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board. EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.
Changes since v1:
- drop patch 2/3
- remove queue lock for rx queues
- add bql support
- add ethtool stats support
- fix possible infinite loop in airoha_qdma_rx_process routine
- always destroy page_pool in case of error during initialization
- cosmetics
Lorenzo Bianconi (2):
dt-bindings: net: airoha: Add EN7581 ethernet controller
net: airoha: Introduce ethernet support for EN7581 SoC
.../bindings/net/airoha,en7581.yaml | 106 + MAINTAINERS | 10 + drivers/net/ethernet/mediatek/Kconfig | 11 +- drivers/net/ethernet/mediatek/Makefile | 1 + drivers/net/ethernet/mediatek/airoha_eth.c | 1804 +++++++++++++++++ drivers/net/ethernet/mediatek/airoha_eth.h | 793 ++++++++ 6 files changed, 2724 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yaml create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.c create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.h
Signature · 1 line
2.45.1
Lorenzo Bianconi
maintainerPATCH v2 1/2 dt-bindings: net: airoha: Add EN7581 ethernet controller
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/
--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yaml
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +106 -0
@@ -0,0 +1,106 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/airoha,en7581.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 Frame Engine Ethernet controller + +allOf: + - $ref: ethernet-controller.yaml# + +maintainers: + - Lorenzo Bianconi <[email protected]> + +description: + The frame engine ethernet controller can be found on Airoha SoCs. + These SoCs have dual GMAC ports. + +properties: + compatible: + enum: + - airoha,en7581-eth + + reg: + items: + - description: Frame engine base address + - description: QDMA0 base address + - description: QDMA1 base address + + reg-names: + items: + - const: fe + - const: qdma0 + - const: qdma1 + + interrupts: + maxItems: 10 + + resets: + maxItems: 7 + + reset-names: + items: + - const: fe + - const: pdma + - const: qdma + - const: xsi-mac + - const: hsi0-mac + - const: hsi1-mac + - const: hsi-mac + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +required: + - compatible + - reg + - interrupts + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/en7523-clk.h> + #include <dt-bindings/reset/airoha,en7581-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + eth0: ethernet@1fb50000 { + compatible = "airoha,en7581-eth"; + reg = <0 0x1fb50000 0 0x2600>, + <0 0x1fb54000 0 0x2000>, + <0 0x1fb56000 0 0x2000>; + reg-names = "fe", "qdma0", "qdma1"; + + resets = <&scuclk EN7581_FE_RST>, + <&scuclk EN7581_FE_PDMA_RST>, + <&scuclk EN7581_FE_QDMA_RST>, + <&scuclk EN7581_XSI_MAC_RST>, + <&scuclk EN7581_DUAL_HSI0_MAC_RST>, + <&scuclk EN7581_DUAL_HSI1_MAC_RST>, + <&scuclk EN7581_HSI_MAC_RST>; + reset-names = "fe", "pdma", "qdma", "xsi-mac", + "hsi0-mac", "hsi1-mac", "hsi-mac"; + + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; + }; + };
Signature · 1 line
2.45.1
Rob Herring (Arm)
maintainerRe: [PATCH v2 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller
Quoted 4 lines · Lorenzo Bianconi
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
Signed-off-by Lorenzo Bianconi <[email protected]>
Quoted 9 lines
---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yaml
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:Documentation/devicetree/bindings/net/airoha,en7581.example.dts:27:18: fatal error: dt-bindings/reset/airoha,en7581-reset.h: No such file or directory
27 | #include <dt-bindings/reset/airoha,en7581-reset.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/net/airoha,en7581.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Re: [PATCH v2 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller
In reply to Lorenzo BianconiQuoted 8 lines · Lorenzo Bianconi
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
Signed-off-by Lorenzo Bianconi <[email protected]>---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/
Why introduce that dep? What's here is just an example, you can use the "raw" numbers (or any made up numbers really) and not have to depend on that.
Thanks,
Conor.
Quoted 79 lines
--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yamlDocumentation/devicetree/bindings/net/airoha,en7581.yaml new file +68
-0@@ -0,0 +1,106 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/airoha,en7581.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 Frame Engine Ethernet controller + +allOf: + - $ref: ethernet-controller.yaml# + +maintainers: + - Lorenzo Bianconi <[email protected]> + +description: + The frame engine ethernet controller can be found on Airoha SoCs. + These SoCs have dual GMAC ports. + +properties: + compatible: + enum: + - airoha,en7581-eth + + reg: + items: + - description: Frame engine base address + - description: QDMA0 base address + - description: QDMA1 base address + + reg-names: + items: + - const: fe + - const: qdma0 + - const: qdma1 + + interrupts: + maxItems: 10 + + resets: + maxItems: 7 + + reset-names: + items: + - const: fe + - const: pdma + - const: qdma + - const: xsi-mac + - const: hsi0-mac + - const: hsi1-mac + - const: hsi-mac + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +required: + - compatible + - reg + - interrupts + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - |
Documentation/devicetree/bindings/net/airoha,en7581.yaml line 78
Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +10
-0+ #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/en7523-clk.h> + #include <dt-bindings/reset/airoha,en7581-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + eth0: ethernet@1fb50000 {
The label here is unused and should be removed.
Documentation/devicetree/bindings/net/airoha,en7581.yaml line 84
Quoted 6 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +6
-0+ compatible = "airoha,en7581-eth"; + reg = <0 0x1fb50000 0 0x2600>, + <0 0x1fb54000 0 0x2000>, + <0 0x1fb56000 0 0x2000>; + reg-names = "fe", "qdma0", "qdma1"; +
Unusual whitespace.
Those are nits:
Quoted 25 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +22
-0+ resets = <&scuclk EN7581_FE_RST>, + <&scuclk EN7581_FE_PDMA_RST>, + <&scuclk EN7581_FE_QDMA_RST>, + <&scuclk EN7581_XSI_MAC_RST>, + <&scuclk EN7581_DUAL_HSI0_MAC_RST>, + <&scuclk EN7581_DUAL_HSI1_MAC_RST>, + <&scuclk EN7581_HSI_MAC_RST>; + reset-names = "fe", "pdma", "qdma", "xsi-mac", + "hsi0-mac", "hsi1-mac", "hsi-mac"; + + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; + }; + };Signature · 1 line
2.45.1
Lorenzo Bianconi
maintainerRe: [PATCH v2 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller
Quoted 10 lines · Lorenzo Bianconi
Quoted 8 lines
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
Signed-off-by Lorenzo Bianconi <[email protected]>---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/
Quoted 3 lines
Why introduce that dep? What's here is just an example, you can use the "raw" numbers (or any made up numbers really) and not have to depend on that.
ack, fine. I will fix it in v3.
Regards,
Lorenzo
Quoted 134 lines
Thanks,
Conor.Quoted 79 lines
--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yamlDocumentation/devicetree/bindings/net/airoha,en7581.yaml new file +68
-0@@ -0,0 +1,106 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/airoha,en7581.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 Frame Engine Ethernet controller + +allOf: + - $ref: ethernet-controller.yaml# + +maintainers: + - Lorenzo Bianconi <[email protected]> + +description: + The frame engine ethernet controller can be found on Airoha SoCs. + These SoCs have dual GMAC ports. + +properties: + compatible: + enum: + - airoha,en7581-eth + + reg: + items: + - description: Frame engine base address + - description: QDMA0 base address + - description: QDMA1 base address + + reg-names: + items: + - const: fe + - const: qdma0 + - const: qdma1 + + interrupts: + maxItems: 10 + + resets: + maxItems: 7 + + reset-names: + items: + - const: fe + - const: pdma + - const: qdma + - const: xsi-mac + - const: hsi0-mac + - const: hsi1-mac + - const: hsi-mac + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +required: + - compatible + - reg + - interrupts + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - |Documentation/devicetree/bindings/net/airoha,en7581.yaml line 78
Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +10
-0+ #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/en7523-clk.h> + #include <dt-bindings/reset/airoha,en7581-reset.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + eth0: ethernet@1fb50000 {The label here is unused and should be removed.
Documentation/devicetree/bindings/net/airoha,en7581.yaml line 84
Quoted 6 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +6
-0+ compatible = "airoha,en7581-eth"; + reg = <0 0x1fb50000 0 0x2600>, + <0 0x1fb54000 0 0x2000>, + <0 0x1fb56000 0 0x2000>; + reg-names = "fe", "qdma0", "qdma1"; +Unusual whitespace.
Those are nits:
Reviewed-by Conor Dooley <[email protected]>Quoted 25 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +22
-0+ resets = <&scuclk EN7581_FE_RST>, + <&scuclk EN7581_FE_PDMA_RST>, + <&scuclk EN7581_FE_QDMA_RST>, + <&scuclk EN7581_XSI_MAC_RST>, + <&scuclk EN7581_DUAL_HSI0_MAC_RST>, + <&scuclk EN7581_DUAL_HSI1_MAC_RST>, + <&scuclk EN7581_HSI_MAC_RST>; + reset-names = "fe", "pdma", "qdma", "xsi-mac", + "hsi0-mac", "hsi1-mac", "hsi-mac"; + + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; + }; + };Signature · 1 line
2.45.1
Re: [PATCH v2 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller
In reply to Lorenzo BianconiQuoted 18 lines · Lorenzo Bianconi
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
Signed-off-by Lorenzo Bianconi <[email protected]>---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yamlDocumentation/devicetree/bindings/net/airoha,en7581.yaml new file +0
-0
Documentation/devicetree/bindings/net/airoha,en7581.yaml line 22
Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +4
-0+properties: + compatible: + enum: + - airoha,en7581-eth
Actually, one other thing - filename matching compatible please.
Lorenzo Bianconi
maintainerRe: [PATCH v2 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller
Quoted 20 lines · Lorenzo Bianconi
Quoted 18 lines
Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.
Signed-off-by Lorenzo Bianconi <[email protected]>---
This patch is based on the following one not applied yet on clk tree:
dt-bindings: clock: airoha: Add reset support to EN7581 clock binding
https://patchwork.kernel.org/project/linux-clk/patch/ac557b6f4029cb3428d4c0ed1582d0c602481fb6.1718282056.git.lorenzo@kernel.org/--- .../bindings/net/airoha,en7581.yaml | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581.yamlDocumentation/devicetree/bindings/net/airoha,en7581.yaml new file +0
-0
Quoted 7 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml line 22
Quoted 4 lines
Documentation/devicetree/bindings/net/airoha,en7581.yaml new file +4
-0+properties: + compatible: + enum: + - airoha,en7581-ethActually, one other thing - filename matching compatible please.
ack, I will fix in v3.
Rregards,
Lorenzo
Lorenzo Bianconi
maintainerPATCH v2 2/2 net: airoha: Introduce ethernet support for EN7581 SoC
In reply to Lorenzo BianconiAdd airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.
--- MAINTAINERS | 10 + drivers/net/ethernet/mediatek/Kconfig | 11 +- drivers/net/ethernet/mediatek/Makefile | 1 + drivers/net/ethernet/mediatek/airoha_eth.c | 1804 ++++++++++++++++++++ drivers/net/ethernet/mediatek/airoha_eth.h | 793 +++++++++ 5 files changed, 2618 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.c create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.h
MAINTAINERS +10 -0
@@ -682,6 +682,16 @@ S: Supported F: fs/aio.c F: include/linux/*aio*.h +AIROHA ETHERNET DRIVER +M: Lorenzo Bianconi <[email protected]> +L: [email protected] (moderated for non-subscribers) +L: [email protected] (moderated for non-subscribers) +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/net/airoha,en7581.yaml +F: drivers/net/ethernet/mediatek/airoha_eth.c +F: drivers/net/ethernet/mediatek/airoha_eth.h + AIROHA SPI SNFI DRIVER M: Lorenzo Bianconi <[email protected]> M: Ray Liu <[email protected]>
drivers/net/ethernet/mediatek/Kconfig +10 -1
@@ -1,12 +1,21 @@ # SPDX-License-Identifier: GPL-2.0-only config NET_VENDOR_MEDIATEK bool "MediaTek devices" - depends on ARCH_MEDIATEK || SOC_MT7621 || SOC_MT7620 || COMPILE_TEST + depends on ARCH_MEDIATEK || ARCH_AIROHA || SOC_MT7621 || SOC_MT7620 || COMPILE_TEST help If you have a Mediatek SoC with ethernet, say Y. if NET_VENDOR_MEDIATEK +config NET_AIROHA + tristate "Airoha SoC Gigabit Ethernet support" + depends on NET_DSA || !NET_DSA + select PAGE_POOL + select PAGE_POOL_STATS + help + This driver supports the gigabit ethernet MACs in the + Airoha SoC family. + config NET_MEDIATEK_SOC_WED depends on ARCH_MEDIATEK || COMPILE_TEST def_bool NET_MEDIATEK_SOC != n
drivers/net/ethernet/mediatek/Makefile +1 -0
@@ -11,3 +11,4 @@ mtk_eth-$(CONFIG_NET_MEDIATEK_SOC_WED) += mtk_wed_debugfs.o endif obj-$(CONFIG_NET_MEDIATEK_SOC_WED) += mtk_wed_ops.o obj-$(CONFIG_NET_MEDIATEK_STAR_EMAC) += mtk_star_emac.o +obj-$(CONFIG_NET_AIROHA) += airoha_eth.o
drivers/net/ethernet/mediatek/airoha_eth.c new file +1804 -0
1,804 lines not on this page · raw message
drivers/net/ethernet/mediatek/airoha_eth.h new file +793 -0
793 lines not on this page · raw message
Signature · 1 line
2.45.1
Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
Quoted 2 lines
(quoted diff) +2
-0+static void airoha_fe_maccr_init(struct airoha_eth *eth) +{
...
drivers/net/ethernet/mediatek/airoha_eth.c line 218
Quoted 1 line
drivers/net/ethernet/mediatek/airoha_eth.c new file +1
-0+airoha_fe_wr (eth,REG_FE_VIP_PATN (11 ),0xc057 );/* PPP->IPv6CP (0xc057) */
include/uapi/linux/ppp_defs.h
#define PPP_IPV6CP 0x8057 /* IPv6 Control Protocol */
Are these the same thing? Why is there one bit difference?
drivers/net/ethernet/mediatek/airoha_eth.c line 251
Quoted 11 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+airoha_fe_wr (eth,REG_FE_VIP_PATN (17 ),0x1ae0 ); +airoha_fe_wr (eth,REG_FE_VIP_EN (17 ), +PATN_FCPU_EN_MASK |PATN_SP_EN_MASK | +FIELD_PREP (PATN_TYPE_MASK ,3 ) |PATN_EN_MASK ); + +airoha_fe_wr (eth,REG_FE_VIP_PATN (18 ),0x1ae00000 ); +airoha_fe_wr (eth,REG_FE_VIP_EN (18 ), +PATN_FCPU_EN_MASK |PATN_DP_EN_MASK | +FIELD_PREP (PATN_TYPE_MASK ,3 ) |PATN_EN_MASK ); +airoha_fe_wr (eth,REG_FE_VIP_PATN (22 ),0xaaaa );
Please add a comment what these match.
drivers/net/ethernet/mediatek/airoha_eth.c line 1423
Quoted 7 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +7
-0+static int airoha_dev_change_mtu (struct net_device *dev,int new_mtu) +{ + dev->mtu = new_mtu; + +return 0 ; +} +
I don't think this is needed. Look at __dev_set_mtu().
Andrew
---
pw-bot: cr
Lorenzo Bianconi
maintainerRe: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
Quoted 11 lines
Quoted 2 lines
(quoted diff) +2
-0+static void airoha_fe_maccr_init(struct airoha_eth *eth) +{...
drivers/net/ethernet/mediatek/airoha_eth.c line 218
Quoted 1 line
drivers/net/ethernet/mediatek/airoha_eth.c new file +1
-0+airoha_fe_wr (eth,REG_FE_VIP_PATN (11 ),0xc057 );/* PPP->IPv6CP (0xc057) */
include/uapi/linux/ppp_defs.h
#definePPP_IPV6CP0x8057/* IPv6 Control Protocol */Are these the same thing? Why is there one bit difference?
ack, I guess there is a typo here.
Quoted 12 lines
Quoted 11 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +9
-0+airoha_fe_wr (eth,REG_FE_VIP_PATN (17 ),0x1ae0 ); +airoha_fe_wr (eth,REG_FE_VIP_EN (17 ), +PATN_FCPU_EN_MASK |PATN_SP_EN_MASK | +FIELD_PREP (PATN_TYPE_MASK ,3 ) |PATN_EN_MASK ); + +airoha_fe_wr (eth,REG_FE_VIP_PATN (18 ),0x1ae00000 ); +airoha_fe_wr (eth,REG_FE_VIP_EN (18 ), +PATN_FCPU_EN_MASK |PATN_DP_EN_MASK | +FIELD_PREP (PATN_TYPE_MASK ,3 ) |PATN_EN_MASK );
Quoted 3 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 251
Quoted 1 line
drivers/net/ethernet/mediatek/airoha_eth.c new file +1
-0+airoha_fe_wr (eth,REG_FE_VIP_PATN (22 ),0xaaaa );Please add a comment what these match.
ack, I will look into them.
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1423
Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +7
-0+static int airoha_dev_change_mtu (struct net_device *dev,int new_mtu) +{ + dev->mtu = new_mtu; + +return 0 ; +} +I don't think this is needed. Look at
__dev_set_mtu().
ack, I will get rid of it in v3.
Regards,
Lorenzo
Quoted 5 lines
Andrew
---
pw-bot: cr
kernel test robot
build botRe: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
In reply to Lorenzo Bianconikernel test robot noticed the following build warnings:
Hi Lorenzo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Bianconi/dt-bindings-net-airoha-Add-EN7581-ethernet-controller/20240618-182217 base: net-next/main patch link: https://lore.kernel.org/r/f146a6f58492394a77f7d159f3c650a268fbe489.1718696209.git.lorenzo%40kernel.org patch subject: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC config: arm64-randconfig-r121-20240623 (https://download.01.org/0day-ci/archive/20240624/[email protected]/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project ad79a14c9e5ec4a369eed4adf567c22cc029863f) reproduce: (https://download.01.org/0day-ci/archive/20240624/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/mediatek/airoha_eth.c:1328:45: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __sum16 [usertype] check @@ got restricted __be16 [usertype] @@ drivers/net/ethernet/mediatek/airoha_eth.c:1328:45: sparse: expected restricted __sum16 [usertype] check drivers/net/ethernet/mediatek/airoha_eth.c:1328:45: sparse: got restricted __be16 [usertype] >> drivers/net/ethernet/mediatek/airoha_eth.c:1787:27: sparse: sparse: symbol 'of_airoha_match' was not declared. Should it be static?
vim +1328 drivers/net/ethernet/mediatek/airoha_eth.c
1303
1304 static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
1305 struct net_device *dev)
1306 {
1307 struct skb_shared_info *sinfo = skb_shinfo(skb);
1308 struct airoha_eth *eth = netdev_priv(dev);
1309 int i, qid = skb_get_queue_mapping(skb);
1310 u32 nr_frags, msg0 = 0, msg1;
1311 u32 len = skb_headlen(skb);
1312 struct netdev_queue *txq;
1313 struct airoha_queue *q;
1314 void *data = skb->data;
1315 u16 index;
1316
1317 if (skb->ip_summed == CHECKSUM_PARTIAL)
1318 msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TCO_MASK, 1) |
1319 FIELD_PREP(QDMA_ETH_TXMSG_UCO_MASK, 1) |
1320 FIELD_PREP(QDMA_ETH_TXMSG_ICO_MASK, 1);
1321
1322 /* TSO: fill MSS info in tcp checksum field */
1323 if (skb_is_gso(skb)) {
1324 if (skb_cow_head(skb, 0))
1325 goto error;
1326
1327 if (sinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
> 1328 tcp_hdr(skb)->check = cpu_to_be16(sinfo->gso_size);
1329 msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TSO_MASK, 1);
1330 }
1331 }
1332
1333 msg1 = FIELD_PREP(QDMA_ETH_TXMSG_FPORT_MASK, DPORT_GDM1) |
1334 FIELD_PREP(QDMA_ETH_TXMSG_METER_MASK, 0x7f);
1335
1336 if (WARN_ON_ONCE(qid >= ARRAY_SIZE(eth->q_tx)))
1337 qid = 0;
1338
1339 q = ð->q_tx[qid];
1340 if (WARN_ON_ONCE(!q->ndesc))
1341 goto error;
1342
1343 spin_lock_bh(&q->lock);
1344
1345 nr_frags = 1 + sinfo->nr_frags;
1346 if (q->queued + nr_frags > q->ndesc) {
1347 /* not enough space in the queue */
1348 spin_unlock_bh(&q->lock);
1349 return NETDEV_TX_BUSY;
1350 }
1351
1352 index = q->head;
1353 for (i = 0; i < nr_frags; i++) {
1354 struct airoha_qdma_desc *desc = &q->desc[index];
1355 struct airoha_queue_entry *e = &q->entry[index];
1356 skb_frag_t *frag = &sinfo->frags[i];
1357 dma_addr_t addr;
1358 u32 val;
1359
1360 addr = dma_map_single(dev->dev.parent, data, len,
1361 DMA_TO_DEVICE);
1362 if (unlikely(dma_mapping_error(dev->dev.parent, addr)))
1363 goto error_unmap;
1364
1365 index = (index + 1) % q->ndesc;
1366
1367 val = FIELD_PREP(QDMA_DESC_LEN_MASK, len);
1368 if (i < nr_frags - 1)
1369 val |= FIELD_PREP(QDMA_DESC_MORE_MASK, 1);
1370 WRITE_ONCE(desc->ctrl, cpu_to_le32(val));
1371 WRITE_ONCE(desc->addr, cpu_to_le32(addr));
1372 val = FIELD_PREP(QDMA_DESC_NEXT_ID_MASK, index);
1373 WRITE_ONCE(desc->data, cpu_to_le32(val));
1374 WRITE_ONCE(desc->msg0, cpu_to_le32(msg0));
1375 WRITE_ONCE(desc->msg1, cpu_to_le32(msg1));
1376 WRITE_ONCE(desc->msg2, cpu_to_le32(0xffff));
1377
1378 e->skb = i ? NULL : skb;
1379 e->dma_addr = addr;
1380 e->dma_len = len;
1381
1382 wmb();
1383 airoha_qdma_rmw(eth, REG_TX_CPU_IDX(qid), TX_RING_CPU_IDX_MASK,
1384 FIELD_PREP(TX_RING_CPU_IDX_MASK, index));
1385
1386 data = skb_frag_address(frag);
1387 len = skb_frag_size(frag);
1388 }
1389
1390 q->head = index;
1391 q->queued += i;
1392
1393 txq = netdev_get_tx_queue(dev, qid);
1394 netdev_tx_sent_queue(txq, skb->len);
1395 skb_tx_timestamp(skb);
1396
1397 if (q->ndesc - q->queued < q->free_thr)
1398 netif_tx_stop_queue(txq);
1399
1400 spin_unlock_bh(&q->lock);
1401
1402 return NETDEV_TX_OK;
1403
1404 error_unmap:
1405 for (i--; i >= 0; i++)
1406 dma_unmap_single(dev->dev.parent, q->entry[i].dma_addr,
1407 q->entry[i].dma_len, DMA_TO_DEVICE);
1408
1409 spin_unlock_bh(&q->lock);
1410 error:
1411 dev_kfree_skb_any(skb);
1412 dev->stats.tx_dropped++;
1413
1414 return NETDEV_TX_OK;
1415 }
1416
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
In reply to Lorenzo BianconiQuoted 12 lines · Lorenzo Bianconi
Add
airoha_ethdriver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed byairoha_ethas mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.
Quoted 2 lines
Tested-by Benjamin Larsson <[email protected]>Signed-off-by Lorenzo Bianconi <[email protected]>
I noticed a few small things that you may want to improve:
Quoted 6 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+static void airoha_qdma_set_irqmask (struct airoha_eth *eth,int index, +u32 clear,u32 set) +{ +unsigned long flags; + +if (WARN_ON_ONCE (index >=ARRAY_SIZE (eth->irqmask)))
drivers/net/ethernet/mediatek/airoha_eth.c line 66
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+return ; + +spin_lock_irqsave (ð->irq_lock, flags); + + eth->irqmask[index] &= ~clear; + eth->irqmask[index] |= set; +airoha_qdma_wr (eth,REG_INT_ENABLE (index), eth->irqmask[index]); + +spin_unlock_irqrestore (ð->irq_lock, flags); +}
spin_lock_irqsave() is fairly expensive here, and it doesn't actually protect the register write since that is posted and can leak out of the spinlock.
You can probably just remove the lock and instead do the mask with atomic_cmpxchg() here.
Quoted 6 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+ +dma_sync_single_for_device (dev, e->dma_addr, e->dma_len, dir); + + val =FIELD_PREP (QDMA_DESC_LEN_MASK , e->dma_len); +WRITE_ONCE (desc->ctrl,cpu_to_le32 (val)); +WRITE_ONCE (desc->addr,cpu_to_le32 (e->dma_addr));
drivers/net/ethernet/mediatek/airoha_eth.c line 599
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+ val =FIELD_PREP (QDMA_DESC_NEXT_ID_MASK , q->head); +WRITE_ONCE (desc->data,cpu_to_le32 (val)); +WRITE_ONCE (desc->msg0,0 ); +WRITE_ONCE (desc->msg1,0 ); +WRITE_ONCE (desc->msg2,0 ); +WRITE_ONCE (desc->msg3,0 ); + +wmb (); +airoha_qdma_rmw (eth,REG_RX_CPU_IDX (qid),RX_RING_CPU_IDX_MASK , +FIELD_PREP (RX_RING_CPU_IDX_MASK , q->head));
The wmb() between the descriptor write and the MMIO does nothing and can probably just be removed here, a writel() already contains all the barriers you need to make DMA memory visible before the MMIO write.
If there is a chance that the device might read the descriptor while it is being updated by you have not written to the register, there should be a barrier before the last store to the descriptor that sets a 'valid' bit. That one can be a cheaper dma_wmb() then.
drivers/net/ethernet/mediatek/airoha_eth.c line 1126
Quoted 11 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +11
-0+static irqreturn_t airoha_irq_handler (int irq,void *dev_instance) +{ +struct airoha_eth *eth = dev_instance; +u32 intr[ARRAY_SIZE (eth->irqmask)]; +int i; + +for (i =0 ; i <ARRAY_SIZE (eth->irqmask); i++) { + intr[i] =airoha_qdma_rr (eth,REG_INT_STATUS (i)); + intr[i] &= eth->irqmask[i]; +airoha_qdma_wr (eth,REG_INT_STATUS (i), intr[i]); + }
This looks like you send an interrupt Ack to each interrupt in order to re-arm it, but then you disable it again. Would it be possible to leave the interrupt enabled but defer the Ack until the napi poll function is completed?
drivers/net/ethernet/mediatek/airoha_eth.c line 1138
Quoted 11 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +11
-0+if (!test_bit (DEV_STATE_INITIALIZED , ð->state)) +return IRQ_NONE ; + +if (intr[1 ] &RX_DONE_INT_MASK ) { +airoha_qdma_irq_disable (eth,QDMA_INT_REG_IDX1 , +RX_DONE_INT_MASK ); +airoha_qdma_for_each_q_rx (eth, i) { +if (intr[1 ] &BIT (i)) +napi_schedule (ð->q_rx[i].napi); + } + }
Something seems wrong here, but that's probably just me misunderstanding the design: if all queues are signaled through the same interrupt handler, and you then do napi_schedule() for each queue, I would expect them to just all get run on the same CPU.
If you have separate queues, doesn't that mean you also need separate irq numbers here so they can be distributed to the available CPUs?
Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +8
-0+ val =FIELD_PREP (QDMA_DESC_LEN_MASK , len); +if (i < nr_frags -1 ) + val |=FIELD_PREP (QDMA_DESC_MORE_MASK ,1 ); +WRITE_ONCE (desc->ctrl,cpu_to_le32 (val)); +WRITE_ONCE (desc->addr,cpu_to_le32 (addr)); + val =FIELD_PREP (QDMA_DESC_NEXT_ID_MASK , index); +WRITE_ONCE (desc->data,cpu_to_le32 (val)); +WRITE_ONCE (desc->msg0,cpu_to_le32 (msg0));
drivers/net/ethernet/mediatek/airoha_eth.c line 1384
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+WRITE_ONCE (desc->msg1,cpu_to_le32 (msg1)); +WRITE_ONCE (desc->msg2,cpu_to_le32 (0xffff )); + + e->skb = i ?NULL : skb; + e->dma_addr = addr; + e->dma_len = len; + +wmb (); +airoha_qdma_rmw (eth,REG_TX_CPU_IDX (qid),TX_RING_CPU_IDX_MASK , +FIELD_PREP (TX_RING_CPU_IDX_MASK , index));
Same as above with the wmb().
Quoted 5 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +5
-0+static int airoha_rx_queues_show (struct seq_file *s,void *data) +{ +struct airoha_eth *eth = s->private; +int i; +
...
drivers/net/ethernet/mediatek/airoha_eth.c line 1638
Quoted 5 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +5
-0+static int airoha_xmit_queues_show (struct seq_file *s,void *data) +{ +struct airoha_eth *eth = s->private; +int i; +
Isn't this information available through ethtool already?
drivers/net/ethernet/mediatek/airoha_eth.h line 7
Quoted 13 lines
b/drivers/net/ethernet/mediatek/airoha_eth.h
new file mode 100644
index 000000000000..fcd684e1418adrivers/net/ethernet/mediatek/airoha_eth.h new file +7
-0@@ -0,0 +1,793 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (C) 2024 Lorenzo Bianconi <[email protected]> +*/ + +#define AIROHA_MAX_NUM_RSTS 3 +#define AIROHA_MAX_NUM_XSI_RSTS 4
If your driver only has a single .c file, I would suggest moving all the contents of the .h file into that as well for better readability.
Arnd
Lorenzo Bianconi
maintainerRe: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
Quoted 13 lines · Lorenzo Bianconi
Quoted 12 lines
Add
airoha_ethdriver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed byairoha_ethas mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.
Quoted 4 lines
Quoted 2 lines
Tested-by Benjamin Larsson <[email protected]>Signed-off-by Lorenzo Bianconi <[email protected]>I noticed a few small things that you may want to improve:
Hi Arnd,
thx for the review.
Quoted 22 lines
Quoted 7 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+static void airoha_qdma_set_irqmask (struct airoha_eth *eth,int index, +u32 clear,u32 set) +{ +unsigned long flags; + +if (WARN_ON_ONCE (index >=ARRAY_SIZE (eth->irqmask)))drivers/net/ethernet/mediatek/airoha_eth.c line 66
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+return ; + +spin_lock_irqsave (ð->irq_lock, flags); + + eth->irqmask[index] &= ~clear; + eth->irqmask[index] |= set; +airoha_qdma_wr (eth,REG_INT_ENABLE (index), eth->irqmask[index]); + +spin_unlock_irqrestore (ð->irq_lock, flags); +}
spin_lock_irqsave()is fairly expensive here, and it doesn't actually protect the register write since that is posted and can leak out of the spinlock.
Quoted 2 lines
You can probably just remove the lock and instead do the mask with
atomic_cmpxchg()here.
I did not get what you mean here. I guess the spin_lock is used to avoid concurrent irq registers updates from user/bh context or irq handler. Am I missing something?
Quoted 23 lines
Quoted 7 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+ +dma_sync_single_for_device (dev, e->dma_addr, e->dma_len, dir); + + val =FIELD_PREP (QDMA_DESC_LEN_MASK , e->dma_len); +WRITE_ONCE (desc->ctrl,cpu_to_le32 (val)); +WRITE_ONCE (desc->addr,cpu_to_le32 (e->dma_addr));drivers/net/ethernet/mediatek/airoha_eth.c line 599
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+ val =FIELD_PREP (QDMA_DESC_NEXT_ID_MASK , q->head); +WRITE_ONCE (desc->data,cpu_to_le32 (val)); +WRITE_ONCE (desc->msg0,0 ); +WRITE_ONCE (desc->msg1,0 ); +WRITE_ONCE (desc->msg2,0 ); +WRITE_ONCE (desc->msg3,0 ); + +wmb (); +airoha_qdma_rmw (eth,REG_RX_CPU_IDX (qid),RX_RING_CPU_IDX_MASK , +FIELD_PREP (RX_RING_CPU_IDX_MASK , q->head));The
wmb()between the descriptor write and the MMIO does nothing and can probably just be removed here, awritel()already contains all the barriers you need to make DMA memory visible before the MMIO write.
Quoted 5 lines
If there is a chance that the device might read the descriptor while it is being updated by you have not written to the register, there should be a barrier before the last store to the descriptor that sets a 'valid' bit. That one can be a cheaper
dma_wmb()then.
ack, I will fix it in v4.
Quoted 17 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1126
Quoted 12 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +11
-0+static irqreturn_t airoha_irq_handler (int irq,void *dev_instance) +{ +struct airoha_eth *eth = dev_instance; +u32 intr[ARRAY_SIZE (eth->irqmask)]; +int i; + +for (i =0 ; i <ARRAY_SIZE (eth->irqmask); i++) { + intr[i] =airoha_qdma_rr (eth,REG_INT_STATUS (i)); + intr[i] &= eth->irqmask[i]; +airoha_qdma_wr (eth,REG_INT_STATUS (i), intr[i]); + }This looks like you send an interrupt Ack to each interrupt in order to re-arm it, but then you disable it again. Would it be possible to leave the interrupt enabled but defer the Ack until the napi poll function is completed?
I guess doing so we are not using NAPIs as expected since they are supposed to run with interrupt disabled. Agree?
Quoted 19 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1138
Quoted 12 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +11
-0+if (!test_bit (DEV_STATE_INITIALIZED , ð->state)) +return IRQ_NONE ; + +if (intr[1 ] &RX_DONE_INT_MASK ) { +airoha_qdma_irq_disable (eth,QDMA_INT_REG_IDX1 , +RX_DONE_INT_MASK ); +airoha_qdma_for_each_q_rx (eth, i) { +if (intr[1 ] &BIT (i)) +napi_schedule (ð->q_rx[i].napi); + } + }Something seems wrong here, but that's probably just me misunderstanding the design: if all queues are signaled through the same interrupt handler, and you then do
napi_schedule()for each queue, I would expect them to just all get run on the same CPU.
Quoted 3 lines
If you have separate queues, doesn't that mean you also need separate irq numbers here so they can be distributed to the available CPUs?
Actually I missed to mark the NAPI as threaded. Doing so, even if we have a single irq line shared by all Rx queues, the scheduler can run the NAPIs in parallel on different CPUs. I will fix it in v4.
Quoted 13 lines
Quoted 13 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +12
-0+ val =FIELD_PREP (QDMA_DESC_LEN_MASK , len); +if (i < nr_frags -1 ) + val |=FIELD_PREP (QDMA_DESC_MORE_MASK ,1 ); +WRITE_ONCE (desc->ctrl,cpu_to_le32 (val)); +WRITE_ONCE (desc->addr,cpu_to_le32 (addr)); + val =FIELD_PREP (QDMA_DESC_NEXT_ID_MASK , index); +WRITE_ONCE (desc->data,cpu_to_le32 (val)); +WRITE_ONCE (desc->msg0,cpu_to_le32 (msg0)); +WRITE_ONCE (desc->msg1,cpu_to_le32 (msg1)); +WRITE_ONCE (desc->msg2,cpu_to_le32 (0xffff )); + + e->skb = i ?NULL : skb;
Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1384
Quoted 6 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+ e->dma_addr = addr; + e->dma_len = len; + +wmb (); +airoha_qdma_rmw (eth,REG_TX_CPU_IDX (qid),TX_RING_CPU_IDX_MASK , +FIELD_PREP (TX_RING_CPU_IDX_MASK , index));Same as above with the
wmb().
ack, I will fix it in v4.
Quoted 7 lines
Quoted 6 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +5
-0+static int airoha_rx_queues_show (struct seq_file *s,void *data) +{ +struct airoha_eth *eth = s->private; +int i; +...
Quoted 7 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1638
Quoted 5 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +5
-0+static int airoha_xmit_queues_show (struct seq_file *s,void *data) +{ +struct airoha_eth *eth = s->private; +int i; +Isn't this information available through ethtool already?
I guess we can get rid of this debugfs since it was just for debugging.
Quoted 17 lines
Quoted 7 lines
b/drivers/net/ethernet/mediatek/airoha_eth.h
new file mode 100644
index 000000000000..fcd684e1418adrivers/net/ethernet/mediatek/airoha_eth.h line 7
Quoted 7 lines
drivers/net/ethernet/mediatek/airoha_eth.h new file +7
-0@@ -0,0 +1,793 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (C) 2024 Lorenzo Bianconi <[email protected]> +*/ + +#define AIROHA_MAX_NUM_RSTS 3 +#define AIROHA_MAX_NUM_XSI_RSTS 4If your driver only has a single .c file, I would suggest moving all the contents of the .h file into that as well for better readability.
I do not have a strong opinion about it but since we will extend the driver in the future, keeping .c and .h in different files, seems a bit more tidy. What do you think?
Regards,
Lorenzo
Quoted 2 lines
Arnd
Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
Quoted 21 lines · Lorenzo Bianconi
Quoted 21 lines
Quoted 6 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +6
-0+static void airoha_qdma_set_irqmask (struct airoha_eth *eth,int index, +u32 clear,u32 set) +{ +unsigned long flags; + +if (WARN_ON_ONCE (index >=ARRAY_SIZE (eth->irqmask)))drivers/net/ethernet/mediatek/airoha_eth.c line 66
Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10
-0+return ; + +spin_lock_irqsave (ð->irq_lock, flags); + + eth->irqmask[index] &= ~clear; + eth->irqmask[index] |= set; +airoha_qdma_wr (eth,REG_INT_ENABLE (index), eth->irqmask[index]); + +spin_unlock_irqrestore (ð->irq_lock, flags); +}
spin_lock_irqsave()is fairly expensive here, and it doesn't actually protect the register write since that is posted and can leak out of the spinlock.
Quoted 6 lines
Quoted 2 lines
You can probably just remove the lock and instead do the mask with
atomic_cmpxchg()here.I did not get what you mean here. I guess the
spin_lockis used to avoid concurrent irq registers updates from user/bh context or irq handler. Am I missing something?
What I meant is that the airoha_qdma_wr() doesn't complete until after the unlock because this is a normal 'posted' ioremap() mapping.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +8
-0+static irqreturn_t airoha_irq_handler (int irq,void *dev_instance) +{ +struct airoha_eth *eth = dev_instance; +u32 intr[ARRAY_SIZE (eth->irqmask)]; +int i; + +for (i =0 ; i <ARRAY_SIZE (eth->irqmask); i++) { + intr[i] =airoha_qdma_rr (eth,REG_INT_STATUS (i));Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1126
Quoted 3 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +3
-0+ intr[i] &= eth->irqmask[i]; +airoha_qdma_wr (eth,REG_INT_STATUS (i), intr[i]); + }This looks like you send an interrupt Ack to each interrupt in order to re-arm it, but then you disable it again. Would it be possible to leave the interrupt enabled but defer the Ack until the napi poll function is completed?
Quoted 2 lines
I guess doing so we are not using NAPIs as expected since they are supposed to run with interrupt disabled. Agree?
The idea of NAPI is that you don't get the same interrupt again until all remaining events have been processed.
How this is achieved is device dependent, and it can either be done by masking the interrupt as you do here, or by not rearming the interrupt if it gets turned off automatically by the hardware. My guess is that writing to REG_INT_STATUS(i) is the rearming here, but the device documentation should clarify that. It's also possible that this is an Ack that is required so you don't immediately get another interrupt.
Quoted 18 lines
Quoted 18 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1138
Quoted 11 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +11
-0+if (!test_bit (DEV_STATE_INITIALIZED , ð->state)) +return IRQ_NONE ; + +if (intr[1 ] &RX_DONE_INT_MASK ) { +airoha_qdma_irq_disable (eth,QDMA_INT_REG_IDX1 , +RX_DONE_INT_MASK ); +airoha_qdma_for_each_q_rx (eth, i) { +if (intr[1 ] &BIT (i)) +napi_schedule (ð->q_rx[i].napi); + } + }Something seems wrong here, but that's probably just me misunderstanding the design: if all queues are signaled through the same interrupt handler, and you then do
napi_schedule()for each queue, I would expect them to just all get run on the same CPU.
Quoted 7 lines
Quoted 3 lines
If you have separate queues, doesn't that mean you also need separate irq numbers here so they can be distributed to the available CPUs?
Actually I missed to mark the NAPI as threaded. Doing so, even if we have a single irq line shared by all Rx queues, the scheduler can run the NAPIs in parallel on different CPUs. I will fix it in v4.
Ok. It's a bit disappointing that the hardware integration messed this up by not having multiple IRQ lines because that adds a lot of latency, but I guess there is not much else you can do about it.
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
b/drivers/net/ethernet/mediatek/airoha_eth.h
new file mode 100644
index 000000000000..fcd684e1418adrivers/net/ethernet/mediatek/airoha_eth.h new file +2
-0@@ -0,0 +1,793 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.h line 7
Quoted 5 lines
drivers/net/ethernet/mediatek/airoha_eth.h new file +5
-0+* Copyright (C) 2024 Lorenzo Bianconi <[email protected]> +*/ + +#define AIROHA_MAX_NUM_RSTS 3 +#define AIROHA_MAX_NUM_XSI_RSTS 4If your driver only has a single .c file, I would suggest moving all the contents of the .h file into that as well for better readability.
Quoted 3 lines
I do not have a strong opinion about it but since we will extend the driver in the future, keeping .c and .h in different files, seems a bit more tidy. What do you think?
I would still keep it all in one file. If you extend it to multiple
files later, it's easy to move those parts that you actually need to
share into an interface header. Most likely the majority of the current contents won't be needed by other files.
Arnd
Felix Fietkau
maintainerRe: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC
Quoted 23 lines · Arnd Bergmann
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +8
-0+static irqreturn_t airoha_irq_handler (int irq,void *dev_instance) +{ +struct airoha_eth *eth = dev_instance; +u32 intr[ARRAY_SIZE (eth->irqmask)]; +int i; + +for (i =0 ; i <ARRAY_SIZE (eth->irqmask); i++) { + intr[i] =airoha_qdma_rr (eth,REG_INT_STATUS (i));Quoted 8 lines
drivers/net/ethernet/mediatek/airoha_eth.c line 1126
Quoted 3 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +3
-0+ intr[i] &= eth->irqmask[i]; +airoha_qdma_wr (eth,REG_INT_STATUS (i), intr[i]); + }This looks like you send an interrupt Ack to each interrupt in order to re-arm it, but then you disable it again. Would it be possible to leave the interrupt enabled but defer the Ack until the napi poll function is completed?
Quoted 2 lines
I guess doing so we are not using NAPIs as expected since they are supposed to run with interrupt disabled. Agree?
The idea of NAPI is that you don't get the same interrupt again until all remaining events have been processed.
Quoted 7 lines
How this is achieved is device dependent, and it can either be done by masking the interrupt as you do here, or by not rearming the interrupt if it gets turned off automatically by the hardware. My guess is that writing to
REG_INT_STATUS(i)is the rearming here, but the device documentation should clarify that. It's also possible that this is an Ack that is required so you don't immediately get another interrupt.
The interrupt handling of this hardware is pretty much the same as what many other devices do: the interrupt status is not automatically cleared by the hardware, so the write at the beginning of the interrupt handler does that explicitly. Within the same handler, the interrupt is then masked to ensure that it does not fire again until the NAPI poll has completed.
Performing the status write after the poll has completed would be wrong, since that leaves open a small race window where events might be missed.
- Felix