Re: [PATCH 3/3] dt-bindings: net: Convert fsl,gianfar to YAML

[J. Neuschäfer <j.ne@xxxxxxxxxx>]

On Fri, Feb 21, 2025 at 05:35:23PM -0600, Rob Herring wrote:
> On Thu, Feb 20, 2025 at 06:29:23PM +0100, J. Neuschäfer wrote:
> > Add a binding for the "Gianfar" ethernet controller, also known as
> > TSEC/eTSEC.
> > 
> > Signed-off-by: J. Neuschäfer <j.ne@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/net/fsl,gianfar.yaml       | 242 +++++++++++++++++++++
> >  .../devicetree/bindings/net/fsl-tsec-phy.txt       |  39 +---
> >  2 files changed, 243 insertions(+), 38 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/fsl,gianfar.yaml b/Documentation/devicetree/bindings/net/fsl,gianfar.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..dc75ceb5dc6fdee8765bb17273f394d01cce0710
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/fsl,gianfar.yaml
> > @@ -0,0 +1,242 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/fsl,gianfar.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale Three-Speed Ethernet Controller (TSEC), "Gianfar"
[...]
> > +  "#address-cells": true
> 
> enum: [ 1, 2 ]
> 
> because 3 is not valid here.
> 
> > +
> > +  "#size-cells": true
> 
> enum: [ 1, 2 ]
> 
> because 0 is not valid here.

Good point.

> 
> 
> > +
> > +  cell-index:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  interrupts:
> > +    maxItems: 3
> 
> Based on the if/then schema, you need 'minItems' here if the min is not 3.
> 
> Really, move the descriptions here and make them work for the combined 
> interrupt case (just a guess).

The difference here (as previously documented in prose) is by device
variant:

 for FEC:

   - one combined interrupt

 for TSEC, eTSEC:

   - transmit interrupt
   - receive interrupt
   - error interrupt

Combining these cases might look like this, not sure if it's good:

  interrupts:
    minItems: 1
    description:
      items:
        - Transmit interrupt or combined interrupt
        - Receive interrupt
        - Error interrupt

> 
> > +
> > +  dma-coherent:
> > +    type: boolean
> 
> dma-coherent: true

Will do.


> > +
> > +  fsl,num_rx_queues:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Number of receive queues
> 
> Constraints? I assume there's at least more than 0.
> 
> > +
> > +  fsl,num_tx_queues:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Number of transmit queues
> 
> Constraints?

Good point, for both of these the only value I can find in use is 8,
which corresponds to the number of queues documented in at least one
hardware manual (MPC8548E).


> > +  # eTSEC2 controller nodes have "queue group" subnodes and don't need a "reg"
> > +  # property.
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,etsec2
> > +    then:
> > +      patternProperties:
> > +        "^queue-group@[0-9a-f]+$":
> > +          type: object
> > +
> > +          properties:
> > +            "#address-cells": true
> > +
> > +            "#size-cells": true
> 
> These have no effect if there are not child nodes or a 'ranges' 
> property.

Ah, good point, these properties are used in existing DTs, but I see no
reason to keep them. I'll remove them.

> 
> > +
> > +            reg:
> > +              maxItems: 1
> > +
> > +            interrupts:
> > +              maxItems: 3
> 
> Need to define what each one is.

Will do.


> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    soc1 {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> 
> You don't need the soc1 node.

Ah, true.

> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    soc2 {
> 
> bus {

Will rename.


Thanks,
J. Neuschäfer