| Message ID | 20220405195755.10817-5-nbd@nbd.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MediaTek SoC flow offload improvements + wireless support | expand |
On 05/04/2022 21:57, Felix Fietkau wrote: > From: Lorenzo Bianconi <lorenzo@kernel.org> > > Document the binding for the Wireless Ethernet Dispatch core on the MT7622 > SoC, which is used for Ethernet->WLAN offloading > Add related info in mediatek-net bindings. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > Signed-off-by: Felix Fietkau <nbd@nbd.name> Thank you for your patch. There is something to discuss/improve. > --- > .../arm/mediatek/mediatek,mt7622-wed.yaml | 50 +++++++++++++++++++ > .../devicetree/bindings/net/mediatek-net.txt | 2 + > 2 files changed, 52 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml Don't store drivers in arm directory. See: https://lore.kernel.org/linux-devicetree/YkJa1oLSEP8R4U6y@robh.at.kernel.org/ Isn't this a network offload engine? If yes, then probably it should be in "net/". > > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml > new file mode 100644 > index 000000000000..787d6673f952 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-wed.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: MediaTek Wireless Ethernet Dispatch Controller for MT7622 > + > +maintainers: > + - Lorenzo Bianconi <lorenzo@kernel.org> > + - Felix Fietkau <nbd@nbd.name> > + > +description: > + The mediatek wireless ethernet dispatch controller can be configured to > + intercept and handle access to the WLAN DMA queues and PCIe interrupts > + and implement hardware flow offloading from ethernet to WLAN. > + > +properties: > + compatible: > + items: > + - enum: > + - mediatek,mt7622-wed > + - const: syscon > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + wed0: wed@1020a000 { Generic node name, "wed" is specific. Maybe "network-offload"? Or "network-accelerator"? You probably know better what this device does, so maybe come with some generic name? The same in DTS patch. The bindings themself look ok. Best regards, Krzysztof
On 06.04.22 10:09, Krzysztof Kozlowski wrote: > On 05/04/2022 21:57, Felix Fietkau wrote: >> From: Lorenzo Bianconi <lorenzo@kernel.org> >> >> Document the binding for the Wireless Ethernet Dispatch core on the MT7622 >> SoC, which is used for Ethernet->WLAN offloading >> Add related info in mediatek-net bindings. >> >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> > > Thank you for your patch. There is something to discuss/improve. > >> --- >> .../arm/mediatek/mediatek,mt7622-wed.yaml | 50 +++++++++++++++++++ >> .../devicetree/bindings/net/mediatek-net.txt | 2 + >> 2 files changed, 52 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml > > Don't store drivers in arm directory. See: > https://lore.kernel.org/linux-devicetree/YkJa1oLSEP8R4U6y@robh.at.kernel.org/ > > Isn't this a network offload engine? If yes, then probably it should be > in "net/". It's not a network offload engine by itself. It's a SoC component that connects to the offload engine and controls a MTK PCIe WLAN device, intercepting interrupts and DMA rings in order to be able to inject packets coming in from the offload engine. Do you think it still belongs in net, or maybe in soc instead? >> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml >> new file mode 100644 >> index 000000000000..787d6673f952 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml >> @@ -0,0 +1,50 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-wed.yaml#" >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" >> + >> +title: MediaTek Wireless Ethernet Dispatch Controller for MT7622 >> + >> +maintainers: >> + - Lorenzo Bianconi <lorenzo@kernel.org> >> + - Felix Fietkau <nbd@nbd.name> >> + >> +description: >> + The mediatek wireless ethernet dispatch controller can be configured to >> + intercept and handle access to the WLAN DMA queues and PCIe interrupts >> + and implement hardware flow offloading from ethernet to WLAN. >> + >> +properties: >> + compatible: >> + items: >> + - enum: >> + - mediatek,mt7622-wed >> + - const: syscon >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + wed0: wed@1020a000 { > > Generic node name, "wed" is specific. Maybe "network-offload"? Or > "network-accelerator"? You probably know better what this device does, > so maybe come with some generic name? wed stands for "wireless ethernet dispatch". Both network-offload and network-accelerator don't really fit. Would it make sense to spell it out, or do you have any better naming ideas? Thanks, - Felix
On Wed, Apr 6, 2022 at 10:18 AM Felix Fietkau <nbd@nbd.name> wrote: > On 06.04.22 10:09, Krzysztof Kozlowski wrote: > > On 05/04/2022 21:57, Felix Fietkau wrote: > >> From: Lorenzo Bianconi <lorenzo@kernel.org> > >> > >> Document the binding for the Wireless Ethernet Dispatch core on the MT7622 > >> SoC, which is used for Ethernet->WLAN offloading > >> Add related info in mediatek-net bindings. > >> > >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >> Signed-off-by: Felix Fietkau <nbd@nbd.name> > > > > Thank you for your patch. There is something to discuss/improve. > > > >> --- > >> .../arm/mediatek/mediatek,mt7622-wed.yaml | 50 +++++++++++++++++++ > >> .../devicetree/bindings/net/mediatek-net.txt | 2 + > >> 2 files changed, 52 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml > > > > Don't store drivers in arm directory. See: > > https://lore.kernel.org/linux-devicetree/YkJa1oLSEP8R4U6y@robh.at.kernel.org/ > > > > Isn't this a network offload engine? If yes, then probably it should be > > in "net/". > It's not a network offload engine by itself. It's a SoC component that > connects to the offload engine and controls a MTK PCIe WLAN device, > intercepting interrupts and DMA rings in order to be able to inject > packets coming in from the offload engine. > Do you think it still belongs in net, or maybe in soc instead? I think it belongs into drivers/net/. Presumably this has some kind of user interface to configure which packets are forwarded? I would not want to maintain that in a SoC driver as this clearly needs to communicate with both of the normal network devices in some form. Arnd
On 06.04.22 10:29, Arnd Bergmann wrote: > On Wed, Apr 6, 2022 at 10:18 AM Felix Fietkau <nbd@nbd.name> wrote: >> On 06.04.22 10:09, Krzysztof Kozlowski wrote: >> > On 05/04/2022 21:57, Felix Fietkau wrote: >> >> From: Lorenzo Bianconi <lorenzo@kernel.org> >> >> >> >> Document the binding for the Wireless Ethernet Dispatch core on the MT7622 >> >> SoC, which is used for Ethernet->WLAN offloading >> >> Add related info in mediatek-net bindings. >> >> >> >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> > >> > Thank you for your patch. There is something to discuss/improve. >> > >> >> --- >> >> .../arm/mediatek/mediatek,mt7622-wed.yaml | 50 +++++++++++++++++++ >> >> .../devicetree/bindings/net/mediatek-net.txt | 2 + >> >> 2 files changed, 52 insertions(+) >> >> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml >> > >> > Don't store drivers in arm directory. See: >> > https://lore.kernel.org/linux-devicetree/YkJa1oLSEP8R4U6y@robh.at.kernel.org/ >> > >> > Isn't this a network offload engine? If yes, then probably it should be >> > in "net/". >> It's not a network offload engine by itself. It's a SoC component that >> connects to the offload engine and controls a MTK PCIe WLAN device, >> intercepting interrupts and DMA rings in order to be able to inject >> packets coming in from the offload engine. >> Do you think it still belongs in net, or maybe in soc instead? > > I think it belongs into drivers/net/. Presumably this has some kind of > user interface to configure which packets are forwarded? I would not > want to maintain that in a SoC driver as this clearly needs to communicate > with both of the normal network devices in some form. The WLAN driver attaches to WED in order to deal with the intercepted DMA rings, but other than that, WED itself has no user configuration. Offload is controlled by the PPE code in the ethernet driver (which is already upstream), and WED simply provides a destination port for PPE, which allows packets to flow to the wireless device. - Felix
On 06/04/2022 10:32, Felix Fietkau wrote: > On 06.04.22 10:29, Arnd Bergmann wrote: >> On Wed, Apr 6, 2022 at 10:18 AM Felix Fietkau <nbd@nbd.name> >> wrote: >>> On 06.04.22 10:09, Krzysztof Kozlowski wrote: >>>> On 05/04/2022 21:57, Felix Fietkau wrote: >>>>> From: Lorenzo Bianconi <lorenzo@kernel.org> >>>>> >>>>> Document the binding for the Wireless Ethernet Dispatch core >>>>> on the MT7622 SoC, which is used for Ethernet->WLAN >>>>> offloading Add related info in mediatek-net bindings. >>>>> >>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >>>> >>>> Thank you for your patch. There is something to >>>> discuss/improve. >>>> >>>>> --- .../arm/mediatek/mediatek,mt7622-wed.yaml | 50 >>>>> +++++++++++++++++++ >>>>> .../devicetree/bindings/net/mediatek-net.txt | 2 + 2 files >>>>> changed, 52 insertions(+) create mode 100644 >>>>> Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml >>>> >>>> >>>>> Don't store drivers in arm directory. See: >>>> https://lore.kernel.org/linux-devicetree/YkJa1oLSEP8R4U6y@robh.at.kernel.org/ >>>> >>>> >>>> Isn't this a network offload engine? If yes, then probably it should be >>>> in "net/". >>> It's not a network offload engine by itself. It's a SoC component >>> that connects to the offload engine and controls a MTK PCIe WLAN >>> device, intercepting interrupts and DMA rings in order to be able >>> to inject packets coming in from the offload engine. Do you think >>> it still belongs in net, or maybe in soc instead? >> >> I think it belongs into drivers/net/. Presumably this has some kind >> of user interface to configure which packets are forwarded? I would >> not want to maintain that in a SoC driver as this clearly needs to >> communicate with both of the normal network devices in some form. > The WLAN driver attaches to WED in order to deal with the intercepted > DMA rings, but other than that, WED itself has no user > configuration. Offload is controlled by the PPE code in the ethernet > driver (which is already upstream), and WED simply provides a > destination port for PPE, which allows packets to flow to the > wireless device. Thanks for clarification. I still wonder about the missing drivers as I responded to your second bindings: https://lore.kernel.org/all/20220405195755.10817-1-nbd@nbd.name/T/#m6d108c644f0c05cd12c05e56abe2ef75760c6cef Both of these compatibles - WED and PCIe - are not actually used. Now everything is done inside your Ethernet driver which pokes WED and PCIe-mirror address space via regmap/syscon. Separate bindings might have sense if WED/PCIe mirror were ever converted to real drivers. Best regards, Krzysztof
> > Isn't this a network offload engine? If yes, then probably it should be > > in "net/". > It's not a network offload engine by itself. It's a SoC component that > connects to the offload engine and controls a MTK PCIe WLAN device, > intercepting interrupts and DMA rings in order to be able to inject packets > coming in from the offload engine. Hi Felix Maybe turn the question around. Can it be used for something other than networking? If not, then somewhere under net seems reasonable. Andrew
On 07.04.22 17:50, Andrew Lunn wrote: >> > Isn't this a network offload engine? If yes, then probably it should be >> > in "net/". >> It's not a network offload engine by itself. It's a SoC component that >> connects to the offload engine and controls a MTK PCIe WLAN device, >> intercepting interrupts and DMA rings in order to be able to inject packets >> coming in from the offload engine. > > Hi Felix > > Maybe turn the question around. Can it be used for something other > than networking? If not, then somewhere under net seems reasonable. I'm fine with moving this to net. - Felix
On 06.04.22 10:57, Krzysztof Kozlowski wrote: > Thanks for clarification. I still wonder about the missing drivers as I > responded to your second bindings: > https://lore.kernel.org/all/20220405195755.10817-1-nbd@nbd.name/T/#m6d108c644f0c05cd12c05e56abe2ef75760c6cef > > Both of these compatibles - WED and PCIe - are not actually used. Now > everything is done inside your Ethernet driver which pokes WED and > PCIe-mirror address space via regmap/syscon. > > Separate bindings might have sense if WED/PCIe mirror were ever > converted to real drivers.I think in terms of hardware description it makes more sense to have separate nodes, even if the implementation uses them in one driver at the moment. - Felix
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml new file mode 100644 index 000000000000..787d6673f952 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-wed.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-wed.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: MediaTek Wireless Ethernet Dispatch Controller for MT7622 + +maintainers: + - Lorenzo Bianconi <lorenzo@kernel.org> + - Felix Fietkau <nbd@nbd.name> + +description: + The mediatek wireless ethernet dispatch controller can be configured to + intercept and handle access to the WLAN DMA queues and PCIe interrupts + and implement hardware flow offloading from ethernet to WLAN. + +properties: + compatible: + items: + - enum: + - mediatek,mt7622-wed + - const: syscon + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + soc { + #address-cells = <2>; + #size-cells = <2>; + wed0: wed@1020a000 { + compatible = "mediatek,mt7622-wed","syscon"; + reg = <0 0x1020a000 0 0x1000>; + interrupts = <GIC_SPI 214 IRQ_TYPE_LEVEL_LOW>; + }; + }; diff --git a/Documentation/devicetree/bindings/net/mediatek-net.txt b/Documentation/devicetree/bindings/net/mediatek-net.txt index 13cb12ee4ed6..1c8dc44bbb52 100644 --- a/Documentation/devicetree/bindings/net/mediatek-net.txt +++ b/Documentation/devicetree/bindings/net/mediatek-net.txt @@ -46,6 +46,8 @@ Optional properties: - mediatek,cci-control: phandle to the cache coherent interconnect node - mediatek,hifsys: phandle to the mediatek hifsys controller used to provide various clocks and reset to the system. +- mediatek,wed: a list of phandles to wireless ethernet dispatch nodes for + MT7622 SoC. * Ethernet MAC node