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

[Luo Jie <quic_luoj@quicinc.com>]

On 5/19/2025 4:16 PM, Krzysztof Kozlowski wrote:
> On Tue, May 13, 2025 at 05:58:21PM GMT, Luo Jie wrote:
>> The PPE (packet process engine) hardware block is available in Qualcomm
>> IPQ chipsets that support PPE architecture, such as IPQ9574. The PPE in
>> the IPQ9574 SoC includes six ethernet ports (6 GMAC and 6 XGMAC), which
>> are used to connect with external PHY devices by PCS. It includes an L2
>> switch function for bridging packets among the 6 ethernet ports and the
>> CPU port. The CPU port enables packet transfer between the ethernet
>> ports and the ARM cores in the SoC, using the ethernet DMA.
>>
>> The PPE also includes packet processing offload capabilities for various
>> networking functions such as route and bridge flows, VLANs, different
>> tunnel protocols and VPN.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   .../devicetree/bindings/net/qcom,ipq9574-ppe.yaml  | 406 +++++++++++++++++++++
>>   1 file changed, 406 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
>> new file mode 100644
>> index 000000000000..f36f4d180674
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq9574-ppe.yaml
>> @@ -0,0 +1,406 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/qcom,ipq9574-ppe.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm IPQ packet process engine (PPE)
>> +
>> +maintainers:
>> +  - Luo Jie <quic_luoj@quicinc.com>
>> +  - Lei Wei <quic_leiwei@quicinc.com>
>> +  - Suruchi Agarwal <quic_suruchia@quicinc.com>
>> +  - Pavithra R <quic_pavir@quicinc.com>>
> 
> Double >>

Thanks, will fix it.

> 
>> +
>> +description:
> 
> You got here comment didn't you?
> 

Yes. We initially believed the '|' marker may not be required
since the format of hardware diagram in the description is
preserved with a specific '|' already. However we relooked
at this again, and will go ahead will add the marker based
on the below documentation reference.

https://docs.kernel.org/devicetree/bindings/writing-schema.html#example-schema

>> +  The Ethernet functionality in the PPE (Packet Process Engine) is comprised
>> +  of three components, the switch core, port wrapper and Ethernet DMA.
>> +
>> +  The Switch core in the IPQ9574 PPE has maximum of 6 front panel ports and
>> +  two FIFO interfaces. One of the two FIFO interfaces is used for Ethernet
>> +  port to host CPU communication using Ethernet DMA. The other is used
>> +  communicating to the EIP engine which is used for IPsec offload. On the
>> +  IPQ9574, the PPE includes 6 GMAC/XGMACs that can be connected with external
>> +  Ethernet PHY. Switch core also includes BM (Buffer Management), QM (Queue
>> +  Management) and SCH (Scheduler) modules for supporting the packet processing.
> 
> ...
> 
>> +  clock-names:
>> +    items:
>> +      - const: ppe
>> +      - const: apb
>> +      - const: ipe
>> +      - const: btq
>> +
>> +  resets:
>> +    maxItems: 1
>> +    description: PPE reset, which is necessary before configuring PPE hardware
>> +
>> +  interconnects:
>> +    items:
>> +      - description: Clock path leading to PPE switch core function
>> +      - description: Clock path leading to PPE register access
>> +      - description: Clock path leading to QoS generation
>> +      - description: Clock path leading to timeout reference
>> +      - description: Clock path leading to NSS NOC from memory NOC
>> +      - description: Clock path leading to memory NOC from NSS NOC
>> +      - description: Clock path leading to enhanced memory NOC from NSS NOC
>> +
>> +  interconnect-names:
>> +    items:
>> +      - const: ppe
>> +      - const: ppe_cfg
>> +      - const: qos_gen
>> +      - const: timeout_ref
>> +      - const: nssnoc_memnoc
>> +      - const: memnoc_nssnoc
>> +      - const: memnoc_nssnoc_1
>> +
>> +  ethernet-dma:
> 
> I don't get why this is a separate node.
> 

We used a separate node because the EDMA (Ethernet DMA)
is a separate block within the PPE block, with specific
functions like ports-to-host-CPU packet transfer and
hardware packet steering. We felt that a separate node
would depict the hierarchy more clearly. Could you please
suggest if a single node is recommended instead?

>> +    type: object
>> +    additionalProperties: false
>> +    description:
>> +      EDMA (Ethernet DMA) is used to transmit packets between PPE and ARM
>> +      host CPU. There are 32 TX descriptor rings, 32 TX completion rings,
>> +      24 RX descriptor rings and 8 RX fill rings supported.
>> +
>> +    properties:
>> +      clocks:
>> +        items:
>> +          - description: EDMA system clock from NSS Clock Controller
>> +          - description: EDMA APB (Advanced Peripheral Bus) clock from
>> +              NSS Clock Controller
>> +
>> +      clock-names:
>> +        items:
>> +          - const: sys
>> +          - const: apb
>> +
>> +      resets:
>> +        maxItems: 1
>> +        description: EDMA reset from NSS clock controller
>> +
>> +      interrupts:
>> +        minItems: 29
>> +        maxItems: 57
> 
> Why is this flexible on the same SoC?

Thanks for pointing to this. I reviewed this again and agree
that this need not be a flexible setting. I will fix this in
the next update by setting 'minItems' and 'maxItems' here to
the count of all available EDMA interrupts.

> 
> Best regards,
> Krzysztof
>