diff mbox series

[v2,05/14] dt-bindings: arm: mediatek: document the pcie mirror node on MT7622

Message ID 20220405195755.10817-6-nbd@nbd.name (mailing list archive)
State Accepted
Commit f14ac41b785f231573fe935a2eb5aaf737447f0c
Delegated to: Netdev Maintainers
Headers show
Series MediaTek SoC flow offload improvements + wireless support | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Felix Fietkau April 5, 2022, 7:57 p.m. UTC 
From: Lorenzo Bianconi <lorenzo@kernel.org>

This patch adds the pcie mirror document bindings for MT7622 SoC.
The feature is used for intercepting PCIe MMIO access for the WED core
Add related info in mediatek-net bindings.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
 .../devicetree/bindings/net/mediatek-net.txt  |  2 +
 2 files changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml

Comments

Krzysztof Kozlowski April 6, 2022, 8:20 a.m. UTC | #1 
On 05/04/2022 21:57, Felix Fietkau wrote:
> From: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> This patch adds the pcie mirror document bindings for MT7622 SoC.
> The feature is used for intercepting PCIe MMIO access for the WED core
> Add related info in mediatek-net bindings.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++

Eh, I wanted to ask to not put it inside arm/, but judging by your usage
- you did not create drivers for both of these (WED and PCIe mirror).

You only need them to expose address spaces via syscon.

This actually looks hacky. Either WED and PCIe mirror are part of
network driver, then add the address spaces via "reg". If they are not,
but instead they are separate blocks, why you don't have drivers for them?


Best regards,
Krzysztof
Felix Fietkau April 6, 2022, 11:01 a.m. UTC | #2 
On 06.04.22 10:20, Krzysztof Kozlowski wrote:
> On 05/04/2022 21:57, Felix Fietkau wrote:
>> From: Lorenzo Bianconi <lorenzo@kernel.org>
>> 
>> This patch adds the pcie mirror document bindings for MT7622 SoC.
>> The feature is used for intercepting PCIe MMIO access for the WED core
>> Add related info in mediatek-net bindings.
>> 
>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
> 
> Eh, I wanted to ask to not put it inside arm/, but judging by your usage
> - you did not create drivers for both of these (WED and PCIe mirror).
> 
> You only need them to expose address spaces via syscon.
> 
> This actually looks hacky. Either WED and PCIe mirror are part of
> network driver, then add the address spaces via "reg". If they are not,
> but instead they are separate blocks, why you don't have drivers for them?
The code that uses the WED block is built into the Ethernet driver, but 
not all SoCs that use this ethernet core have it. Also, there are two 
WED blocks, and I'm not sure if future SoCs might have a different 
number of them at some point.
The WED code also needs to access registers of the ethernet MAC.
One reason for having a separate device is this:
As long as WED is not in use, ethernet supports coherent DMA for 
increased performance. When the first wireless device attaches to WED, 
IO coherency gets disabled and the ethernet DMA rings are cleaned up and 
allocated again, this time with the struct device of WED (which doesn't 
have the dma-coherent property).

- Felix
Rob Herring (Arm) April 7, 2022, 5:16 p.m. UTC | #3 
On Wed, Apr 06, 2022 at 01:01:06PM +0200, Felix Fietkau wrote:
> 
> On 06.04.22 10:20, Krzysztof Kozlowski wrote:
> > On 05/04/2022 21:57, Felix Fietkau wrote:
> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > 
> > > This patch adds the pcie mirror document bindings for MT7622 SoC.
> > > The feature is used for intercepting PCIe MMIO access for the WED core
> > > Add related info in mediatek-net bindings.
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > Signed-off-by: Felix Fietkau <nbd@nbd.name>
> > > ---
> > >  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
> > 
> > Eh, I wanted to ask to not put it inside arm/, but judging by your usage
> > - you did not create drivers for both of these (WED and PCIe mirror).
> > 
> > You only need them to expose address spaces via syscon.
> > 
> > This actually looks hacky. Either WED and PCIe mirror are part of
> > network driver, then add the address spaces via "reg". If they are not,
> > but instead they are separate blocks, why you don't have drivers for them?
> The code that uses the WED block is built into the Ethernet driver, but not
> all SoCs that use this ethernet core have it. Also, there are two WED
> blocks, and I'm not sure if future SoCs might have a different number of
> them at some point.
> The WED code also needs to access registers of the ethernet MAC.
> One reason for having a separate device is this:
> As long as WED is not in use, ethernet supports coherent DMA for increased
> performance. When the first wireless device attaches to WED, IO coherency
> gets disabled and the ethernet DMA rings are cleaned up and allocated again,
> this time with the struct device of WED (which doesn't have the dma-coherent
> property).

I'm pretty sure there are assumptions in the driver core that coherency 
is not changing on the fly. In any case, if it is, using 'dma-coherent' 
is not appropriate. You obviously have another method to determine 
whether you are coherent or not.

Rob
Rob Herring (Arm) April 7, 2022, 5:19 p.m. UTC | #4 
On Tue, Apr 05, 2022 at 09:57:46PM +0200, Felix Fietkau wrote:
> From: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> This patch adds the pcie mirror document bindings for MT7622 SoC.
> The feature is used for intercepting PCIe MMIO access for the WED core
> Add related info in mediatek-net bindings.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
>  .../devicetree/bindings/net/mediatek-net.txt  |  2 +
>  2 files changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
> new file mode 100644
> index 000000000000..9fbeb626ab23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: MediaTek PCIE Mirror Controller for MT7622
> +
> +maintainers:
> +  - Lorenzo Bianconi <lorenzo@kernel.org>
> +  - Felix Fietkau <nbd@nbd.name>
> +
> +description:
> +  The mediatek PCIE mirror provides a configuration interface for PCIE
> +  controller on MT7622 soc.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - mediatek,mt7622-pcie-mirror
> +      - const: syscon

This doesn't sound like a syscon to me. Are there multiple clients or 
functions in this block? A 'syscon' property is not the only way to 
create a regmap if that's what you need.

Rob
Felix Fietkau April 7, 2022, 5:29 p.m. UTC | #5 
On 07.04.22 19:16, Rob Herring wrote:
> On Wed, Apr 06, 2022 at 01:01:06PM +0200, Felix Fietkau wrote:
>> 
>> On 06.04.22 10:20, Krzysztof Kozlowski wrote:
>> > On 05/04/2022 21:57, Felix Fietkau wrote:
>> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
>> > > 
>> > > This patch adds the pcie mirror document bindings for MT7622 SoC.
>> > > The feature is used for intercepting PCIe MMIO access for the WED core
>> > > Add related info in mediatek-net bindings.
>> > > 
>> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>> > > Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> > > ---
>> > >  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
>> > 
>> > Eh, I wanted to ask to not put it inside arm/, but judging by your usage
>> > - you did not create drivers for both of these (WED and PCIe mirror).
>> > 
>> > You only need them to expose address spaces via syscon.
>> > 
>> > This actually looks hacky. Either WED and PCIe mirror are part of
>> > network driver, then add the address spaces via "reg". If they are not,
>> > but instead they are separate blocks, why you don't have drivers for them?
>> The code that uses the WED block is built into the Ethernet driver, but not
>> all SoCs that use this ethernet core have it. Also, there are two WED
>> blocks, and I'm not sure if future SoCs might have a different number of
>> them at some point.
>> The WED code also needs to access registers of the ethernet MAC.
>> One reason for having a separate device is this:
>> As long as WED is not in use, ethernet supports coherent DMA for increased
>> performance. When the first wireless device attaches to WED, IO coherency
>> gets disabled and the ethernet DMA rings are cleaned up and allocated again,
>> this time with the struct device of WED (which doesn't have the dma-coherent
>> property).
> 
> I'm pretty sure there are assumptions in the driver core that coherency
> is not changing on the fly. In any case, if it is, using 'dma-coherent'
> is not appropriate. You obviously have another method to determine
> whether you are coherent or not.
It's not really on the fly. Before changing coherency, all DMA memory is 
freed, and the subsequent reallocation uses the struct device of the WED 
core, which does not have the dma-coherent property.

- Felix
Felix Fietkau April 8, 2022, 9:03 a.m. UTC | #6 
On 07.04.22 19:19, Rob Herring wrote:
> On Tue, Apr 05, 2022 at 09:57:46PM +0200, Felix Fietkau wrote:
>> From: Lorenzo Bianconi <lorenzo@kernel.org>
>> 
>> This patch adds the pcie mirror document bindings for MT7622 SoC.
>> The feature is used for intercepting PCIe MMIO access for the WED core
>> Add related info in mediatek-net bindings.
>> 
>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>> ---
>>  .../mediatek/mediatek,mt7622-pcie-mirror.yaml | 42 +++++++++++++++++++
>>  .../devicetree/bindings/net/mediatek-net.txt  |  2 +
>>  2 files changed, 44 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
>> new file mode 100644
>> index 000000000000..9fbeb626ab23
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>> +
>> +title: MediaTek PCIE Mirror Controller for MT7622
>> +
>> +maintainers:
>> +  - Lorenzo Bianconi <lorenzo@kernel.org>
>> +  - Felix Fietkau <nbd@nbd.name>
>> +
>> +description:
>> +  The mediatek PCIE mirror provides a configuration interface for PCIE
>> +  controller on MT7622 soc.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - mediatek,mt7622-pcie-mirror
>> +      - const: syscon
> 
> This doesn't sound like a syscon to me. Are there multiple clients or
> functions in this block? A 'syscon' property is not the only way to
> create a regmap if that's what you need.
It's used only by the WED code in the ethernet driver, but there are 
multiple WED instances and only a single pcie-mirror block containing 
configuration for them.

- Felix
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
new file mode 100644
index 000000000000..9fbeb626ab23
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml
@@ -0,0 +1,42 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/arm/mediatek/mediatek,mt7622-pcie-mirror.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MediaTek PCIE Mirror Controller for MT7622
+
+maintainers:
+  - Lorenzo Bianconi <lorenzo@kernel.org>
+  - Felix Fietkau <nbd@nbd.name>
+
+description:
+  The mediatek PCIE mirror provides a configuration interface for PCIE
+  controller on MT7622 soc.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - mediatek,mt7622-pcie-mirror
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pcie_mirror: pcie-mirror@10000400 {
+        compatible = "mediatek,mt7622-pcie-mirror", "syscon";
+        reg = <0 0x10000400 0 0x10>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/net/mediatek-net.txt b/Documentation/devicetree/bindings/net/mediatek-net.txt
index 1c8dc44bbb52..f18d70189375 100644
--- a/Documentation/devicetree/bindings/net/mediatek-net.txt
+++ b/Documentation/devicetree/bindings/net/mediatek-net.txt
@@ -48,6 +48,8 @@  Optional properties:
 	various clocks and reset to the system.
 - mediatek,wed: a list of phandles to wireless ethernet dispatch nodes for
 	MT7622 SoC.
+- mediatek,pcie-mirror: phandle to the mediatek pcie-mirror controller for
+	MT7622 SoC.
 
 * Ethernet MAC node