This converts the marvell,pp2 bindings from text to proper schema.
Move 'marvell,system-controller' and 'dma-coherent' properties from
port up to the controller node, to match what is actually done in DT.
Signed-off-by: Michał Grzelak <[email protected]>
---
.../devicetree/bindings/net/marvell,pp2.yaml | 241 ++++++++++++++++++
.../devicetree/bindings/net/marvell-pp2.txt | 141 ----------
MAINTAINERS | 2 +-
3 files changed, 242 insertions(+), 142 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/marvell,pp2.yaml
delete mode 100644 Documentation/devicetree/bindings/net/marvell-pp2.txt
diff --git a/Documentation/devicetree/bindings/net/marvell,pp2.yaml b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
new file mode 100644
index 000000000000..6faa4c87dfc6
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
@@ -0,0 +1,241 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/marvell,pp2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell CN913X / Marvell Armada 375, 7K, 8K Ethernet Controller
+
+maintainers:
+ - Marcin Wojtas <[email protected]>
+ - Russell King <[email protected]>
+
+description: |
+ Marvell Armada 375 Ethernet Controller (PPv2.1)
+ Marvell Armada 7K/8K Ethernet Controller (PPv2.2)
+ Marvell CN913X Ethernet Controller (PPv2.3)
+
+properties:
+ compatible:
+ enum:
+ - marvell,armada-375-pp2
+ - marvell,armada-7k-pp22
+
+ reg:
+ minItems: 3
+ maxItems: 4
+ description: |
+ For "marvell,armada-375-pp2", must contain the following register sets:
+ - common controller registers
+ - LMS registers
+ - one register area per Ethernet port
+ For "marvell,armada-7k-pp22" used by 7K/8K and CN913X, must contain the following register sets:
+ - packet processor registers
+ - networking interfaces registers
+ - CM3 address space used for TX Flow Control
+
+ clocks:
+ minItems: 2
+ items:
+ - description: main controller clock
+ - description: GOP clock
+ - description: MG clock
+ - description: MG Core clock
+ - description: AXI clock
+
+ clock-names:
+ minItems: 2
+ items:
+ - const: pp_clk
+ - const: gop_clk
+ - const: mg_clk
+ - const: mg_core_clk
+ - const: axi_clk
+
+ dma-coherent: true
+ '#size-cells': true
+ '#address-cells': true
+
+ marvell,system-controller:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: a phandle to the system controller.
+
+patternProperties:
+ '^eth[0-9a-f]*(@.*)?$':
+ type: object
+ properties:
+ interrupts:
+ minItems: 1
+ maxItems: 10
+ description: interrupt(s) for the port
+
+ interrupt-names:
+ items:
+ - const: hif0
+ - const: hif1
+ - const: hif2
+ - const: hif3
+ - const: hif4
+ - const: hif5
+ - const: hif6
+ - const: hif7
+ - const: hif8
+ - const: link
+
+ description: >
+ if more than a single interrupt for is given, must be the
+ name associated to the interrupts listed. Valid names are:
+ "hifX", with X in [0..8], and "link". The names "tx-cpu0",
+ "tx-cpu1", "tx-cpu2", "tx-cpu3" and "rx-shared" are supported
+ for backward compatibility but shouldn't be used for new
+ additions.
+
+ port-id:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: ID of the port from the MAC point of view.
+
+ phy:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description: >
+ a phandle to a phy node defining the PHY address
+ (as the reg property, a single integer).
+
+ phy-mode:
+ $ref: "ethernet-controller.yaml#/properties/phy-mode"
+
+ marvell,loopback:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description: port is loopback mode.
+
+ gop-port-id:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: >
+ only for marvell,armada-7k-pp22, ID of the port from the
+ GOP (Group Of Ports) point of view. This ID is used to index the
+ per-port registers in the second register area.
+
+ required:
+ - interrupts
+ - port-id
+ - phy-mode
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+
+allOf:
+ - $ref: ethernet-controller.yaml#
+ - if:
+ properties:
+ compatible:
+ const: marvell,armada-7k-pp22
+ then:
+ patternProperties:
+ '^eth[0-9a-f]*(@.*)?$':
+ required:
+ - gop-port-id
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ // For Armada 375 variant
+ #include <dt-bindings/interrupt-controller/mvebu-icu.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ ethernet@f0000 {
+ compatible = "marvell,armada-375-pp2";
+ reg = <0xf0000 0xa000>,
+ <0xc0000 0x3060>,
+ <0xc4000 0x100>,
+ <0xc5000 0x100>;
+ clocks = <&gateclk 3>, <&gateclk 19>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clock-names = "pp_clk", "gop_clk";
+
+ eth0: eth0@c4000 {
+ reg = <0xc4000>;
+ interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+ port-id = <0>;
+ phy = <&phy0>;
+ phy-mode = "gmii";
+ };
+
+ eth1: eth1@c5000 {
+ reg = <0xc5000>;
+ interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+ port-id = <1>;
+ phy = <&phy3>;
+ phy-mode = "gmii";
+ };
+ };
+
+ - |
+ // For Armada 7k/8k and Cn913x variants
+ #include <dt-bindings/interrupt-controller/mvebu-icu.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ cpm_ethernet: ethernet@0 {
+ compatible = "marvell,armada-7k-pp22";
+ reg = <0x0 0x100000>, <0x129000 0xb000>, <0x220000 0x800>;
+ clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>,
+ <&cpm_syscon0 1 5>, <&cpm_syscon0 1 6>, <&cpm_syscon0 1 18>;
+ clock-names = "pp_clk", "gop_clk", "mg_clk", "mg_core_clk", "axi_clk";
+
+ eth00: eth0 {
+ interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 43 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 47 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 51 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 55 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 59 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 63 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 67 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 71 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 129 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
+ "hif5", "hif6", "hif7", "hif8", "link";
+ phy-mode = "10gbase-r";
+ port-id = <0>;
+ gop-port-id = <0>;
+ };
+
+ eth01: eth1 {
+ interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 44 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 48 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 52 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 56 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 60 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 64 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 68 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 72 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 128 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
+ "hif5", "hif6", "hif7", "hif8", "link";
+ phy-mode = "rgmii-id";
+ port-id = <1>;
+ gop-port-id = <2>;
+ };
+
+ eth02: eth2 {
+ interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 45 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 49 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 53 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 57 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 61 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 65 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 69 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 73 IRQ_TYPE_LEVEL_HIGH>,
+ <ICU_GRP_NSR 127 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
+ "hif5", "hif6", "hif7", "hif8", "link";
+ phy-mode = "gmii";
+ port-id = <2>;
+ gop-port-id = <3>;
+ };
+ };
diff --git a/Documentation/devicetree/bindings/net/marvell-pp2.txt b/Documentation/devicetree/bindings/net/marvell-pp2.txt
deleted file mode 100644
index ce15c173f43f..000000000000
--- a/Documentation/devicetree/bindings/net/marvell-pp2.txt
+++ /dev/null
@@ -1,141 +0,0 @@
-* Marvell Armada 375 Ethernet Controller (PPv2.1)
- Marvell Armada 7K/8K Ethernet Controller (PPv2.2)
- Marvell CN913X Ethernet Controller (PPv2.3)
-
-Required properties:
-
-- compatible: should be one of:
- "marvell,armada-375-pp2"
- "marvell,armada-7k-pp2"
-- reg: addresses and length of the register sets for the device.
- For "marvell,armada-375-pp2", must contain the following register
- sets:
- - common controller registers
- - LMS registers
- - one register area per Ethernet port
- For "marvell,armada-7k-pp2" used by 7K/8K and CN913X, must contain the following register
- sets:
- - packet processor registers
- - networking interfaces registers
- - CM3 address space used for TX Flow Control
-
-- clocks: pointers to the reference clocks for this device, consequently:
- - main controller clock (for both armada-375-pp2 and armada-7k-pp2)
- - GOP clock (for both armada-375-pp2 and armada-7k-pp2)
- - MG clock (only for armada-7k-pp2)
- - MG Core clock (only for armada-7k-pp2)
- - AXI clock (only for armada-7k-pp2)
-- clock-names: names of used clocks, must be "pp_clk", "gop_clk", "mg_clk",
- "mg_core_clk" and "axi_clk" (the 3 latter only for armada-7k-pp2).
-
-The ethernet ports are represented by subnodes. At least one port is
-required.
-
-Required properties (port):
-
-- interrupts: interrupt(s) for the port
-- port-id: ID of the port from the MAC point of view
-- gop-port-id: only for marvell,armada-7k-pp2, ID of the port from the
- GOP (Group Of Ports) point of view. This ID is used to index the
- per-port registers in the second register area.
-- phy-mode: See ethernet.txt file in the same directory
-
-Optional properties (port):
-
-- marvell,loopback: port is loopback mode
-- phy: a phandle to a phy node defining the PHY address (as the reg
- property, a single integer).
-- interrupt-names: if more than a single interrupt for is given, must be the
- name associated to the interrupts listed. Valid names are:
- "hifX", with X in [0..8], and "link". The names "tx-cpu0",
- "tx-cpu1", "tx-cpu2", "tx-cpu3" and "rx-shared" are supported
- for backward compatibility but shouldn't be used for new
- additions.
-- marvell,system-controller: a phandle to the system controller.
-
-Example for marvell,armada-375-pp2:
-
-ethernet@f0000 {
- compatible = "marvell,armada-375-pp2";
- reg = <0xf0000 0xa000>,
- <0xc0000 0x3060>,
- <0xc4000 0x100>,
- <0xc5000 0x100>;
- clocks = <&gateclk 3>, <&gateclk 19>;
- clock-names = "pp_clk", "gop_clk";
-
- eth0: eth0@c4000 {
- interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
- port-id = <0>;
- phy = <&phy0>;
- phy-mode = "gmii";
- };
-
- eth1: eth1@c5000 {
- interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
- port-id = <1>;
- phy = <&phy3>;
- phy-mode = "gmii";
- };
-};
-
-Example for marvell,armada-7k-pp2:
-
-cpm_ethernet: ethernet@0 {
- compatible = "marvell,armada-7k-pp22";
- reg = <0x0 0x100000>, <0x129000 0xb000>, <0x220000 0x800>;
- clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>,
- <&cpm_syscon0 1 5>, <&cpm_syscon0 1 6>, <&cpm_syscon0 1 18>;
- clock-names = "pp_clk", "gop_clk", "mg_clk", "mg_core_clk", "axi_clk";
-
- eth0: eth0 {
- interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 43 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 47 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 51 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 55 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 59 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 63 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 67 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 71 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 129 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
- "hif5", "hif6", "hif7", "hif8", "link";
- port-id = <0>;
- gop-port-id = <0>;
- };
-
- eth1: eth1 {
- interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 44 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 48 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 52 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 56 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 60 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 64 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 68 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 72 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 128 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
- "hif5", "hif6", "hif7", "hif8", "link";
- port-id = <1>;
- gop-port-id = <2>;
- };
-
- eth2: eth2 {
- interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 45 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 49 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 53 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 57 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 61 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 65 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 69 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 73 IRQ_TYPE_LEVEL_HIGH>,
- <ICU_GRP_NSR 127 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "hif0", "hif1", "hif2", "hif3", "hif4",
- "hif5", "hif6", "hif7", "hif8", "link";
- port-id = <2>;
- gop-port-id = <3>;
- };
-};
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ae989b32ebb..3d8e64bf7ae6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12191,7 +12191,7 @@ M: Marcin Wojtas <[email protected]>
M: Russell King <[email protected]>
L: [email protected]
S: Maintained
-F: Documentation/devicetree/bindings/net/marvell-pp2.txt
+F: Documentation/devicetree/bindings/net/marvell,pp2.yaml
F: drivers/net/ethernet/marvell/mvpp2/
MARVELL MWIFIEX WIRELESS DRIVER
--
Changelog:
v1->v2:
* move 'properties' to the front of the file
* remove blank line after 'properties'
* move 'compatible' to the front of 'properties'
* move 'clocks', 'clock-names' and 'reg' definitions to 'properties'
* substitute all occurences of 'marvell,armada-7k-pp2' with
'marvell,armada-7k-pp22'
* add properties:#size-cells and properties:#address-cells
* specify list in 'interrupt-names'
* remove blank lines after 'patternProperties'
* remove '^interrupt' and '^#.*-cells$' patterns
* remove blank line after 'allOf'
* remove first 'if-then-else' block from 'allOf'
* negate the condition in allOf:if schema
* delete 'interrupt-controller' from section 'examples'
* delete '#interrupt-cells' from section 'examples'
2.34.1
On 27/09/2022 01:21, Michał Grzelak wrote:
> This converts the marvell,pp2 bindings from text to proper schema.
>
> Move 'marvell,system-controller' and 'dma-coherent' properties from
> port up to the controller node, to match what is actually done in DT.
>
> Signed-off-by: Michał Grzelak <[email protected]>
> ---
> .../devicetree/bindings/net/marvell,pp2.yaml | 241 ++++++++++++++++++
> .../devicetree/bindings/net/marvell-pp2.txt | 141 ----------
Thank you for your patch. There is something to discuss/improve.
> +properties:
> + compatible:
> + enum:
> + - marvell,armada-375-pp2
> + - marvell,armada-7k-pp22
> +
> + reg:
> + minItems: 3
> + maxItems: 4
> + description: |
> + For "marvell,armada-375-pp2", must contain the following register sets:
> + - common controller registers
> + - LMS registers
> + - one register area per Ethernet port
> + For "marvell,armada-7k-pp22" used by 7K/8K and CN913X, must contain the following register sets:
> + - packet processor registers
> + - networking interfaces registers
> + - CM3 address space used for TX Flow Control
Instead of this description, in define them for each variant in
allOf:if:then (just like for clocks below)
> +
> + clocks:
> + minItems: 2
> + items:
> + - description: main controller clock
> + - description: GOP clock
> + - description: MG clock
> + - description: MG Core clock
> + - description: AXI clock
This needs to be restricted per variant - minItems and maxItems in
allOf:if:then.
> +
> + clock-names:
> + minItems: 2
> + items:
> + - const: pp_clk
> + - const: gop_clk
> + - const: mg_clk
> + - const: mg_core_clk
> + - const: axi_clk
The same.
> +
> + dma-coherent: true
> + '#size-cells': true
> + '#address-cells': true
You need const:X for both cells (unless they come from some other schema
but then you would not need to list them here).
> +
> + marvell,system-controller:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: a phandle to the system controller.
> +
> +patternProperties:
> + '^eth[0-9a-f]*(@.*)?$':
The name should be "(ethernet-)?port", unless anything depends on
particular naming?
> + type: object
You need description here.
> + properties:
> + interrupts:
> + minItems: 1
> + maxItems: 10
> + description: interrupt(s) for the port
> +
> + interrupt-names:
> + items:
minItems: 1
> + - const: hif0
> + - const: hif1
> + - const: hif2
> + - const: hif3
> + - const: hif4
> + - const: hif5
> + - const: hif6
> + - const: hif7
> + - const: hif8
> + - const: link
> +
> + description: >
> + if more than a single interrupt for is given, must be the
> + name associated to the interrupts listed. Valid names are:
> + "hifX", with X in [0..8], and "link". The names "tx-cpu0",
> + "tx-cpu1", "tx-cpu2", "tx-cpu3" and "rx-shared" are supported
> + for backward compatibility but shouldn't be used for new
> + additions.
> +
> + port-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: ID of the port from the MAC point of view.
> +
> + phy:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: >
> + a phandle to a phy node defining the PHY address
> + (as the reg property, a single integer).
> +
> + phy-mode:
> + $ref: "ethernet-controller.yaml#/properties/phy-mode"
You can skip quotes.
> +
> + marvell,loopback:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: port is loopback mode.
> +
> + gop-port-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: >
> + only for marvell,armada-7k-pp22, ID of the port from the
> + GOP (Group Of Ports) point of view. This ID is used to index the
> + per-port registers in the second register area.
> +
> + required:
> + - interrupts
> + - port-id
> + - phy-mode
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> +
> +allOf:
> + - $ref: ethernet-controller.yaml#
> + - if:
> + properties:
> + compatible:
> + const: marvell,armada-7k-pp22
> + then:
> + patternProperties:
> + '^eth[0-9a-f]*(@.*)?$':
> + required:
> + - gop-port-id
else:
patternProperties:
....
gop-port-id: false
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
Best regards,
Krzysztof
Hi Krzysztof,
Thanks for your comments and time spent on reviewing my patch.
All of those improvements will be included in next version.
Also, I would like to know your opinion about one.
>> +
>> + marvell,system-controller:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: a phandle to the system controller.
>> +
>> +patternProperties:
>> + '^eth[0-9a-f]*(@.*)?$':
>
> The name should be "(ethernet-)?port", unless anything depends on
> particular naming?
What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
It resembles pattern found in net/ethernet-phy.yaml like
properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
still passing `dt_binding_check' and `dtbs_check'. It should also
comply with your comment.
Best regards,
Michał
On 01/10/2022 17:53, Michał Grzelak wrote:
> Hi Krzysztof,
>
> Thanks for your comments and time spent on reviewing my patch.
> All of those improvements will be included in next version.
> Also, I would like to know your opinion about one.
>
>>> +
>>> + marvell,system-controller:
>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>> + description: a phandle to the system controller.
>>> +
>>> +patternProperties:
>>> + '^eth[0-9a-f]*(@.*)?$':
>>
>> The name should be "(ethernet-)?port", unless anything depends on
>> particular naming?
>
> What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
> It resembles pattern found in net/ethernet-phy.yaml like
> properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
> still passing `dt_binding_check' and `dtbs_check'. It should also
> comply with your comment.
Node names like ethernet-eth do not make much sense because they contain
redundant ethernet or eth. AFAIK, all other bindings like that call
these ethernet-ports (or sometimes shorter - ports). Unless this device
is different than all others?
Best regards,
Krzysztof
niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
<[email protected]> napisał(a):
>
> On 01/10/2022 17:53, Michał Grzelak wrote:
> > Hi Krzysztof,
> >
> > Thanks for your comments and time spent on reviewing my patch.
> > All of those improvements will be included in next version.
> > Also, I would like to know your opinion about one.
> >
> >>> +
> >>> + marvell,system-controller:
> >>> + $ref: /schemas/types.yaml#/definitions/phandle
> >>> + description: a phandle to the system controller.
> >>> +
> >>> +patternProperties:
> >>> + '^eth[0-9a-f]*(@.*)?$':
> >>
> >> The name should be "(ethernet-)?port", unless anything depends on
> >> particular naming?
> >
> > What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
> > It resembles pattern found in net/ethernet-phy.yaml like
> > properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
> > still passing `dt_binding_check' and `dtbs_check'. It should also
> > comply with your comment.
>
> Node names like ethernet-eth do not make much sense because they contain
> redundant ethernet or eth. AFAIK, all other bindings like that call
> these ethernet-ports (or sometimes shorter - ports). Unless this device
> is different than all others?
>
IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine
(as long as we don't have to modify the existing .dtsi files) - there
is no dependency in the driver code on that.
Best regards,
Marcin
On 02/10/2022 10:23, Marcin Wojtas wrote:
>niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
><[email protected]> napisał(a):
>>
>> On 01/10/2022 17:53, Michał Grzelak wrote:
>> > Hi Krzysztof,
>> >
>> > Thanks for your comments and time spent on reviewing my patch.
>> > All of those improvements will be included in next version.
>> > Also, I would like to know your opinion about one.
>> >
>> >>> +
>> >>> + marvell,system-controller:
>> >>> + $ref: /schemas/types.yaml#/definitions/phandle
>> >>> + description: a phandle to the system controller.
>> >>> +
>> >>> +patternProperties:
>> >>> + '^eth[0-9a-f]*(@.*)?$':
>> >>
>> >> The name should be "(ethernet-)?port", unless anything depends on
>> >> particular naming?
>> >
>> > What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
>> > It resembles pattern found in net/ethernet-phy.yaml like
>> > properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
>> > still passing `dt_binding_check' and `dtbs_check'. It should also
>> > comply with your comment.
>>
>> Node names like ethernet-eth do not make much sense because they contain
>> redundant ethernet or eth. AFAIK, all other bindings like that call
>> these ethernet-ports (or sometimes shorter - ports). Unless this device
>> is different than all others?
>>
>
>IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine
>(as long as we don't have to modify the existing .dtsi files) - there
>is no dependency in the driver code on that.
Indeed, driver's code isn't dependent; however, there is a dependency
on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.:
https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72
Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files
including the above would have to be modified to pass through `dtbs_check'.
Best regards,
Michał
pon., 3 paź 2022 o 19:06 Michał Grzelak <[email protected]> napisał(a):
>
> On 02/10/2022 10:23, Marcin Wojtas wrote:
> >niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
> ><[email protected]> napisał(a):
> >>
> >> On 01/10/2022 17:53, Michał Grzelak wrote:
> >> > Hi Krzysztof,
> >> >
> >> > Thanks for your comments and time spent on reviewing my patch.
> >> > All of those improvements will be included in next version.
> >> > Also, I would like to know your opinion about one.
> >> >
> >> >>> +
> >> >>> + marvell,system-controller:
> >> >>> + $ref: /schemas/types.yaml#/definitions/phandle
> >> >>> + description: a phandle to the system controller.
> >> >>> +
> >> >>> +patternProperties:
> >> >>> + '^eth[0-9a-f]*(@.*)?$':
> >> >>
> >> >> The name should be "(ethernet-)?port", unless anything depends on
> >> >> particular naming?
> >> >
> >> > What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
> >> > It resembles pattern found in net/ethernet-phy.yaml like
> >> > properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
> >> > still passing `dt_binding_check' and `dtbs_check'. It should also
> >> > comply with your comment.
> >>
> >> Node names like ethernet-eth do not make much sense because they contain
> >> redundant ethernet or eth. AFAIK, all other bindings like that call
> >> these ethernet-ports (or sometimes shorter - ports). Unless this device
> >> is different than all others?
> >>
> >
> >IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine
> >(as long as we don't have to modify the existing .dtsi files) - there
> >is no dependency in the driver code on that.
>
> Indeed, driver's code isn't dependent; however, there is a dependency
> on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.:
>
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72
>
> Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files
> including the above would have to be modified to pass through `dtbs_check'.
>
Can you please double check?
The .dts files use labels, the node name they relate to should be irrelevant:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/marvell/cn9130-db.dtsi?h=v6.0#n122
(BTW, for A7k8k/CN913x example please use updated names, i.e.
s/cpm_/cp0_/ and s/cps_/cp1_/)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-375-db.dts?h=v6.0#n167
Best regards,
Marcin
On 03/10/2022 19:06, Michał Grzelak wrote:
> On 02/10/2022 10:23, Marcin Wojtas wrote:
>> niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
>> <[email protected]> napisał(a):
>>>
>>> On 01/10/2022 17:53, Michał Grzelak wrote:
>>>> Hi Krzysztof,
>>>>
>>>> Thanks for your comments and time spent on reviewing my patch.
>>>> All of those improvements will be included in next version.
>>>> Also, I would like to know your opinion about one.
>>>>
>>>>>> +
>>>>>> + marvell,system-controller:
>>>>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> + description: a phandle to the system controller.
>>>>>> +
>>>>>> +patternProperties:
>>>>>> + '^eth[0-9a-f]*(@.*)?$':
>>>>>
>>>>> The name should be "(ethernet-)?port", unless anything depends on
>>>>> particular naming?
>>>>
>>>> What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
>>>> It resembles pattern found in net/ethernet-phy.yaml like
>>>> properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
>>>> still passing `dt_binding_check' and `dtbs_check'. It should also
>>>> comply with your comment.
>>>
>>> Node names like ethernet-eth do not make much sense because they contain
>>> redundant ethernet or eth. AFAIK, all other bindings like that call
>>> these ethernet-ports (or sometimes shorter - ports). Unless this device
>>> is different than all others?
>>>
>>
>> IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine
>> (as long as we don't have to modify the existing .dtsi files) - there
>> is no dependency in the driver code on that.
>
> Indeed, driver's code isn't dependent; however, there is a dependency
> on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.:
>
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72
>
> Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files
> including the above would have to be modified to pass through `dtbs_check'.
I didn't get it. What is the "dependency"? Usage of some names is not a
dependency... Old bindings were not precising any specific name of
subnodes, therefore I commented to change it. If the DTS already use
some other name, you can change them if none of upstream implementations
(BSD, bootloaders, firmware, Linux kernel) depend on it.
Best regards,
Krzysztof
pon., 3 paź 2022 o 19:29 Krzysztof Kozlowski
<[email protected]> napisał(a):
>
> On 03/10/2022 19:06, Michał Grzelak wrote:
> > On 02/10/2022 10:23, Marcin Wojtas wrote:
> >> niedz., 2 paź 2022 o 10:00 Krzysztof Kozlowski
> >> <[email protected]> napisał(a):
> >>>
> >>> On 01/10/2022 17:53, Michał Grzelak wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> Thanks for your comments and time spent on reviewing my patch.
> >>>> All of those improvements will be included in next version.
> >>>> Also, I would like to know your opinion about one.
> >>>>
> >>>>>> +
> >>>>>> + marvell,system-controller:
> >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>> + description: a phandle to the system controller.
> >>>>>> +
> >>>>>> +patternProperties:
> >>>>>> + '^eth[0-9a-f]*(@.*)?$':
> >>>>>
> >>>>> The name should be "(ethernet-)?port", unless anything depends on
> >>>>> particular naming?
> >>>>
> >>>> What do you think about pattern "^(ethernet-)?eth[0-9a-f]+(@.*)?$"?
> >>>> It resembles pattern found in net/ethernet-phy.yaml like
> >>>> properties:$nodename:pattern:"^ethernet-phy(@[a-f0-9]+)?$", while
> >>>> still passing `dt_binding_check' and `dtbs_check'. It should also
> >>>> comply with your comment.
> >>>
> >>> Node names like ethernet-eth do not make much sense because they contain
> >>> redundant ethernet or eth. AFAIK, all other bindings like that call
> >>> these ethernet-ports (or sometimes shorter - ports). Unless this device
> >>> is different than all others?
> >>>
> >>
> >> IMO "^(ethernet-)?port@[0-9]+$" for the subnodes' names could be fine
> >> (as long as we don't have to modify the existing .dtsi files) - there
> >> is no dependency in the driver code on that.
> >
> > Indeed, driver's code isn't dependent; however, there is a dependency
> > on 'eth[0-2]' name in all relevant .dts and .dtsi files, e.g.:
> >
> > https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/armada-375.dtsi#L190
> > https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi#L72
> >
> > Ports under 'ethernet' node are named eth[0-2], thus those and all .dts files
> > including the above would have to be modified to pass through `dtbs_check'.
>
> I didn't get it. What is the "dependency"? Usage of some names is not a
> dependency... Old bindings were not precising any specific name of
> subnodes, therefore I commented to change it. If the DTS already use
> some other name, you can change them if none of upstream implementations
> (BSD, bootloaders, firmware, Linux kernel) depend on it.
>
None of the PP2 drivers depends on nodes' names, so indeed we can
safely modify that and update the relevant .dtsi files. One comment
here, though - if we switch to e.g. ethernet-port@0 subnode, there is
a requirement of specifying a 'reg' property: See below warning:
Documentation/devicetree/bindings/net/marvell,pp2.example.dts:36.27-41.13:
Warning (unit_address_vs_reg):
/example-0/ethernet@f0000/ethernet-port@0: node has a unit name, but
no reg or ranges property
I think this convention is good and my idea is to use 'reg' property
as a port ID (like the DSA does) - it would become required. However,
we should retain 'port-id' to maintain backward compatibility. Once
this schema gets accepted, I'll prepare a driver update.
Best regards,
Marcin
Hi Michał,
Additional comments inline.
wt., 27 wrz 2022 o 01:22 Michał Grzelak <[email protected]> napisał(a):
>
> This converts the marvell,pp2 bindings from text to proper schema.
>
> Move 'marvell,system-controller' and 'dma-coherent' properties from
> port up to the controller node, to match what is actually done in DT.
>
> Signed-off-by: Michał Grzelak <[email protected]>
> ---
> .../devicetree/bindings/net/marvell,pp2.yaml | 241 ++++++++++++++++++
> .../devicetree/bindings/net/marvell-pp2.txt | 141 ----------
> MAINTAINERS | 2 +-
> 3 files changed, 242 insertions(+), 142 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/marvell,pp2.yaml
> delete mode 100644 Documentation/devicetree/bindings/net/marvell-pp2.txt
>
> diff --git a/Documentation/devicetree/bindings/net/marvell,pp2.yaml b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
> new file mode 100644
> index 000000000000..6faa4c87dfc6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,pp2.yaml
> @@ -0,0 +1,241 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,pp2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell CN913X / Marvell Armada 375, 7K, 8K Ethernet Controller
> +
> +maintainers:
> + - Marcin Wojtas <[email protected]>
> + - Russell King <[email protected]>
> +
> +description: |
> + Marvell Armada 375 Ethernet Controller (PPv2.1)
> + Marvell Armada 7K/8K Ethernet Controller (PPv2.2)
> + Marvell CN913X Ethernet Controller (PPv2.3)
> +
> +properties:
> + compatible:
> + enum:
> + - marvell,armada-375-pp2
> + - marvell,armada-7k-pp22
> +
> + reg:
> + minItems: 3
> + maxItems: 4
> + description: |
> + For "marvell,armada-375-pp2", must contain the following register sets:
> + - common controller registers
> + - LMS registers
> + - one register area per Ethernet port
> + For "marvell,armada-7k-pp22" used by 7K/8K and CN913X, must contain the following register sets:
> + - packet processor registers
> + - networking interfaces registers
> + - CM3 address space used for TX Flow Control
> +
> + clocks:
> + minItems: 2
> + items:
> + - description: main controller clock
> + - description: GOP clock
> + - description: MG clock
> + - description: MG Core clock
> + - description: AXI clock
> +
> + clock-names:
> + minItems: 2
> + items:
> + - const: pp_clk
> + - const: gop_clk
> + - const: mg_clk
> + - const: mg_core_clk
> + - const: axi_clk
> +
> + dma-coherent: true
> + '#size-cells': true
> + '#address-cells': true
> +
> + marvell,system-controller:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: a phandle to the system controller.
> +
> +patternProperties:
> + '^eth[0-9a-f]*(@.*)?$':
> + type: object
> + properties:
> + interrupts:
> + minItems: 1
> + maxItems: 10
> + description: interrupt(s) for the port
> +
> + interrupt-names:
> + items:
> + - const: hif0
> + - const: hif1
> + - const: hif2
> + - const: hif3
> + - const: hif4
> + - const: hif5
> + - const: hif6
> + - const: hif7
> + - const: hif8
> + - const: link
> +
> + description: >
> + if more than a single interrupt for is given, must be the
> + name associated to the interrupts listed. Valid names are:
> + "hifX", with X in [0..8], and "link". The names "tx-cpu0",
> + "tx-cpu1", "tx-cpu2", "tx-cpu3" and "rx-shared" are supported
> + for backward compatibility but shouldn't be used for new
> + additions.
> +
> + port-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: ID of the port from the MAC point of view.
> +
> + phy:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: >
> + a phandle to a phy node defining the PHY address
> + (as the reg property, a single integer).
> +
> + phy-mode:
> + $ref: "ethernet-controller.yaml#/properties/phy-mode"
> +
> + marvell,loopback:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: port is loopback mode.
> +
> + gop-port-id:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: >
> + only for marvell,armada-7k-pp22, ID of the port from the
> + GOP (Group Of Ports) point of view. This ID is used to index the
> + per-port registers in the second register area.
> +
> + required:
> + - interrupts
> + - port-id
> + - phy-mode
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> +
> +allOf:
> + - $ref: ethernet-controller.yaml#
> + - if:
> + properties:
> + compatible:
> + const: marvell,armada-7k-pp22
> + then:
> + patternProperties:
> + '^eth[0-9a-f]*(@.*)?$':
> + required:
> + - gop-port-id
For this variant, 'marvell,system-controller' should also be marked as required.
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + // For Armada 375 variant
> + #include <dt-bindings/interrupt-controller/mvebu-icu.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + ethernet@f0000 {
> + compatible = "marvell,armada-375-pp2";
> + reg = <0xf0000 0xa000>,
> + <0xc0000 0x3060>,
> + <0xc4000 0x100>,
> + <0xc5000 0x100>;
> + clocks = <&gateclk 3>, <&gateclk 19>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-names = "pp_clk", "gop_clk";
> +
> + eth0: eth0@c4000 {
> + reg = <0xc4000>;
> + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> + port-id = <0>;
> + phy = <&phy0>;
> + phy-mode = "gmii";
> + };
> +
> + eth1: eth1@c5000 {
> + reg = <0xc5000>;
> + interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> + port-id = <1>;
> + phy = <&phy3>;
> + phy-mode = "gmii";
> + };
> + };
> +
> + - |
> + // For Armada 7k/8k and Cn913x variants
> + #include <dt-bindings/interrupt-controller/mvebu-icu.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + cpm_ethernet: ethernet@0 {
> + compatible = "marvell,armada-7k-pp22";
> + reg = <0x0 0x100000>, <0x129000 0xb000>, <0x220000 0x800>;
> + clocks = <&cpm_syscon0 1 3>, <&cpm_syscon0 1 9>,
> + <&cpm_syscon0 1 5>, <&cpm_syscon0 1 6>, <&cpm_syscon0 1 18>;
s/syscon0/clk/
Also add missing marvell,system-controller.
Best regards,
Marcin