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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z72fJSqng8od-5Z7@probook>
Date: Tue, 25 Feb 2025 10:44:53 +0000
From: J. Neuschäfer <j.ne@...teo.net>
To: Rob Herring <robh@...nel.org>
Cc: J. Neuschäfer <j.ne@...teo.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Claudiu Manoil <claudiu.manoil@....com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] dt-bindings: net: Convert fsl,gianfar to YAML

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@...teo.net>
> > ---
> >  .../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