Re: [PATCH net-next v5 01/14] dt-bindings: net: Add PPE for Qualcomm IPQ9574 SoC

[Luo Jie <quic_luoj@xxxxxxxxxxx>]

On 7/1/2025 3:11 PM, Krzysztof Kozlowski wrote:
> On Thu, Jun 26, 2025 at 10:31:00PM +0800, Luo Jie wrote:
> > +      resets:
> > +        maxItems: 1
> > +        description: EDMA reset from NSS clock controller
> > +
> > +      interrupts:
> > +        minItems: 65
> > +        maxItems: 65
> > +
> > +      interrupt-names:
> > +        minItems: 65
> > +        maxItems: 65
> > +        description:
> > +          Interrupts "txcmpl_[0-31]" are the Ethernet DMA TX completion ring interrupts.
> > +          Interrupts "rxfill_[0-7]" are the Ethernet DMA RX fill ring interrupts.
> > +          Interrupts "rxdesc_[0-23]" are the Ethernet DMA RX Descriptor ring interrupts.
> > +          Interrupt "misc" is the Ethernet DMA miscellaneous error interrupt.
> > +
> > +    required:
> > +      - clocks
> > +      - clock-names
> > +      - resets
> > +      - interrupts
> > +      - interrupt-names
> > +
> > +patternProperties:
> > +  "^(ethernet-)?port@[0-9a-f]+$":
>
> Only one port? What are you switching here?

Sorry that this was missed in the update. We wanted to
add the ethernet-port' property to ensure we document
the per-port clocks/resets for completeness, but missed
adding the 'ethernet-ports' property. I will add the
'ethernet-ports' node in the updated version of the patch
to accurately reflect the schema and hardware hierarchy.

There are six physical ports in the PPE that can be part of
the switch function.

> Anyway, ^ethernet-port..... is preferred over port.

OK, I will update to use '^ethernet-port' instead.

> But other problem is that it does not match referenced schema at all and
> nothing in commit msg explains why this appered. 1.5 years of
> development of this and some significant, unexpected and not correct
> changes.

I understand your concern. This change was described briefly in the V5
cover letter, but I will improve this description in cover letter and
update the commit message as well, to explicitly document this change.
The motivation for adding the ethernet-port node in bindings was to
document the required per-port clocks and resets as well, as these
are essential for enabling Ethernet functionality on this hardware.

Could you please review the following proposed changes and let me know
if this approach is acceptable?

patternProperties:
  "^(ethernet-)?ports$":
    additionalProperties: true
    patternProperties:
      "^ethernet-port@[1-6]+$":
        type: object
        unevaluatedProperties: false
        $ref: ethernet-controller.yaml#
		
        properties:
          reg:
            minimum: 1
            maximum: 6
            description: PPE Ethernet port ID
			
          clocks:
            items:
              - description: Port MAC clock from NSS clock controller
              - description: Port RX clock from NSS clock controller
              - description: Port TX clock from NSS clock controller
			
          clock-names:
            items:
              - const: mac
              - const: rx
              - const: tx
			
          resets:
            items:
              - description: Port MAC reset from NSS clock controller
              - description: Port RX reset from NSS clock controller
              - description: Port TX reset from NSS clock controller
			
          reset-names:
            items:
              - const: mac
              - const: rx
              - const: tx
			
        required:
          - reg
          - clocks
          - clock-names
          - resets
          - reset-names
		
...

allOf:
  - $ref: ethernet-switch.yaml
...

> > +    unevaluatedProperties: false
> > +    $ref: ethernet-switch-port.yaml#
> > +
> > +    properties:
> > +      clocks:
> > +        items:
> > +          - description: Port MAC clock from NSS clock controller
> > +          - description: Port RX clock from NSS clock controller
> > +          - description: Port TX clock from NSS clock controller
> > +
> > +      clock-names:
> > +        items:
> > +          - const: mac
> > +          - const: rx
> > +          - const: tx
> > +
> > +      resets:
> > +        items:
> > +          - description: Port MAC reset from NSS clock controller
> > +          - description: Port RX reset from NSS clock controller
> > +          - description: Port TX reset from NSS clock controller
> > +
> > +      reset-names:
> > +        items:
> > +          - const: mac
> > +          - const: rx
> > +          - const: tx
> > +
> > +    required:
> > +      - reg
> > +      - clocks
> > +      - clock-names
> > +      - resets
> > +      - reset-names
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - interconnects
> > +  - interconnect-names
> > +  - ethernet-dma
> > +
> > +allOf:
> > +  - $ref: ethernet-switch.yaml
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
> > +    #include <dt-bindings/clock/qcom,ipq9574-nsscc.h>
> > +    #include <dt-bindings/interconnect/qcom,ipq9574.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/reset/qcom,ipq9574-nsscc.h>
> > +
> > +    ethernet-switch@3a000000 {
> > +        compatible = "qcom,ipq9574-ppe";
> > +        reg = <0x3a000000 0xbef800>;
> > +        clocks = <&nsscc NSS_CC_PPE_SWITCH_CLK>,
> > +                 <&nsscc NSS_CC_PPE_SWITCH_CFG_CLK>,
> > +                 <&nsscc NSS_CC_PPE_SWITCH_IPE_CLK>,
> > +                 <&nsscc NSS_CC_PPE_SWITCH_BTQ_CLK>;
> > +        clock-names = "ppe",
> > +                      "apb",
> > +                      "ipe",
> > +                      "btq";
> > +        resets = <&nsscc PPE_FULL_RESET>;
> > +        interrupts = <GIC_SPI 498 IRQ_TYPE_LEVEL_HIGH>;
> > +        interconnects = <&nsscc MASTER_NSSNOC_PPE &nsscc SLAVE_NSSNOC_PPE>,
> > +                        <&nsscc MASTER_NSSNOC_PPE_CFG &nsscc SLAVE_NSSNOC_PPE_CFG>,
> > +                        <&gcc MASTER_NSSNOC_QOSGEN_REF &gcc SLAVE_NSSNOC_QOSGEN_REF>,
> > +                        <&gcc MASTER_NSSNOC_TIMEOUT_REF &gcc SLAVE_NSSNOC_TIMEOUT_REF>,
> > +                        <&gcc MASTER_MEM_NOC_NSSNOC &gcc SLAVE_MEM_NOC_NSSNOC>,
> > +                        <&gcc MASTER_NSSNOC_MEMNOC &gcc SLAVE_NSSNOC_MEMNOC>,
> > +                        <&gcc MASTER_NSSNOC_MEM_NOC_1 &gcc SLAVE_NSSNOC_MEM_NOC_1>;
> > +        interconnect-names = "ppe",
> > +                             "ppe_cfg",
> > +                             "qos_gen",
> > +                             "timeout_ref",
> > +                             "nssnoc_memnoc",
> > +                             "memnoc_nssnoc",
> > +                             "memnoc_nssnoc_1";
> > +
> > +        ethernet-dma {
> > +            clocks = <&nsscc NSS_CC_PPE_EDMA_CLK>,
> > +                     <&nsscc NSS_CC_PPE_EDMA_CFG_CLK>;
> > +            clock-names = "sys",
> > +                          "apb";
> > +            resets = <&nsscc EDMA_HW_RESET>;
> > +            interrupts = <GIC_SPI 363 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 364 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 365 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 366 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 367 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 368 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 369 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 370 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 371 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 372 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 374 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 375 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 378 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 379 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 381 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 382 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 383 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 384 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 505 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 504 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 503 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 502 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 501 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 500 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 359 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 360 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 361 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 362 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 331 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 333 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 334 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 335 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 336 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 337 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 338 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 339 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 341 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 342 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 344 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 345 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 346 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 347 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 348 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 349 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 350 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 352 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 499 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "txcmpl_0",
> > +                              "txcmpl_1",
> > +                              "txcmpl_2",
> > +                              "txcmpl_3",
> > +                              "txcmpl_4",
> > +                              "txcmpl_5",
> > +                              "txcmpl_6",
> > +                              "txcmpl_7",
> > +                              "txcmpl_8",
> > +                              "txcmpl_9",
> > +                              "txcmpl_10",
> > +                              "txcmpl_11",
> > +                              "txcmpl_12",
> > +                              "txcmpl_13",
> > +                              "txcmpl_14",
> > +                              "txcmpl_15",
> > +                              "txcmpl_16",
> > +                              "txcmpl_17",
> > +                              "txcmpl_18",
> > +                              "txcmpl_19",
> > +                              "txcmpl_20",
> > +                              "txcmpl_21",
> > +                              "txcmpl_22",
> > +                              "txcmpl_23",
> > +                              "txcmpl_24",
> > +                              "txcmpl_25",
> > +                              "txcmpl_26",
> > +                              "txcmpl_27",
> > +                              "txcmpl_28",
> > +                              "txcmpl_29",
> > +                              "txcmpl_30",
> > +                              "txcmpl_31",
> > +                              "rxfill_0",
> > +                              "rxfill_1",
> > +                              "rxfill_2",
> > +                              "rxfill_3",
> > +                              "rxfill_4",
> > +                              "rxfill_5",
> > +                              "rxfill_6",
> > +                              "rxfill_7",
> > +                              "rxdesc_0",
> > +                              "rxdesc_1",
> > +                              "rxdesc_2",
> > +                              "rxdesc_3",
> > +                              "rxdesc_4",
> > +                              "rxdesc_5",
> > +                              "rxdesc_6",
> > +                              "rxdesc_7",
> > +                              "rxdesc_8",
> > +                              "rxdesc_9",
> > +                              "rxdesc_10",
> > +                              "rxdesc_11",
> > +                              "rxdesc_12",
> > +                              "rxdesc_13",
> > +                              "rxdesc_14",
> > +                              "rxdesc_15",
> > +                              "rxdesc_16",
> > +                              "rxdesc_17",
> > +                              "rxdesc_18",
> > +                              "rxdesc_19",
> > +                              "rxdesc_20",
> > +                              "rxdesc_21",
> > +                              "rxdesc_22",
> > +                              "rxdesc_23",
> > +                              "misc";
> > +        };
> > +
> > +        ethernet-ports {
>
> Look at your binding, not what it said...

I will fix this to add the ethernet-ports node in the next version.

> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            port@1 {
>
> Best regards,
> Krzysztof