Re: [PATCH v2 04/24] dt-bindings: net: dwmac: Refactor snps,*-config properties

[Rob Herring]

On Mon, Feb 08, 2021 at 04:55:48PM +0300, Serge Semin wrote:
> Currently the "snps,axi-config", "snps,mtl-rx-config" and
> "snps,mtl-tx-config" properties are declared as a single phandle reference
> to a node with corresponding parameters defined. That's not good for
> several reasons. First of all scattering around a device tree some
> particular device-specific configs with no visual relation to that device
> isn't suitable from maintainability point of view. That leads to a
> disturbed representation of the actual device tree mixing actual device
> nodes and some vendor-specific configs. Secondly using the same configs
> set for several device nodes doesn't represent well the devices structure,
> since the interfaces these configs describe in hardware belong to
> different devices and may actually differ. In the later case having the
> configs node separated from the corresponding device nodes gets to be
> even unjustified.
> 
> So instead of having a separate DW *MAC configs nodes we suggest to
> define them as sub-nodes of the device nodes, which interfaces they
> actually describe. By doing so we'll make the DW *MAC nodes visually
> correct describing all the aspects of the IP-core configuration. Thus
> we'll be able to describe the configs sub-nodes bindings right in the
> snps,dwmac.yaml file.
> 
> Note the former "snps,axi-config", "snps,mtl-rx-config" and
> "snps,mtl-tx-config" properties have been marked as deprecated in favor of
> the added by this commit "axi-config", "mtl-rx-config" and "mtl-tx-config"
> sub-nodes respectively.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> 
> ---
> 
> Note this change will work only if DT-schema tool is fixed like this:
> 
> --- a/meta-schemas/nodes.yaml	2021-02-08 14:20:56.732447780 +0300
> +++ b/meta-schemas/nodes.yaml	2021-02-08 14:21:00.736492245 +0300
> @@ -22,6 +22,7 @@
>      - unevaluatedProperties
>      - deprecated
>      - required
> +    - not
>      - allOf
>      - anyOf
>      - oneOf

Can you send me a patch or GH PR. There is another way to express. More 
below.

> 
> So a property with name "not" would be allowed and the "not-required"
> pattern would work.
> 
> Changelog v2:
> - Add the new sub-nodes "axi-config", "mtl-rx-config" and "mtl-tx-config"
>   describing the nodes now deprecated properties were supposed to
>   refer to.
> - Fix invalid identation in the "snps,route-*" property settings.
> - Use correct syntax of the JSON pointers, so the later would begin
>   with a '/' after the '#'.
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   | 389 +++++++++++++-----
>  1 file changed, 297 insertions(+), 92 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 03d58bf9965f..4dda9ffa822c 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -150,73 +150,264 @@ properties:
>        in a different mode than the PHY in order to function.
>  
>    snps,axi-config:
> +    deprecated: true
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
> -      AXI BUS Mode parameters. Phandle to a node that can contain the
> -      following properties
> -        * snps,lpi_en, enable Low Power Interface
> -        * snps,xit_frm, unlock on WoL
> -        * snps,wr_osr_lmt, max write outstanding req. limit
> -        * snps,rd_osr_lmt, max read outstanding req. limit
> -        * snps,kbbe, do not cross 1KiB boundary.
> -        * snps,blen, this is a vector of supported burst length.
> -        * snps,fb, fixed-burst
> -        * snps,mb, mixed-burst
> -        * snps,rb, rebuild INCRx Burst
> +      AXI BUS Mode parameters. Phandle to a node that contains the properties
> +      described in the 'axi-config' sub-node.
> +
> +  axi-config:
> +    type: object
> +    description: AXI BUS Mode parameters
> +
> +    properties:
> +      snps,lpi_en:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Enable Low Power Interface
> +
> +      snps,xit_frm:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Unlock on WoL
> +
> +      snps,wr_osr_lmt:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Max write outstanding req. limit
> +        default: 1
> +        minimum: 0
> +        maximum: 15
> +
> +      snps,rd_osr_lmt:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Max read outstanding req. limit
> +        default: 1
> +        minimum: 0
> +        maximum: 15
> +
> +      snps,kbbe:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Do not cross 1KiB boundary
> +
> +      snps,blen:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description: A vector of supported burst lengths
> +        minItems: 7
> +        maxItems: 7
> +        items:
> +          enum: [256, 128, 64, 32, 16, 8, 4, 0]
> +
> +      snps,fb:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Fixed-burst
> +
> +      snps,mb:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Mixed-burst
> +
> +      snps,rb:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Rebuild INCRx Burst
> +
> +    additionalProperties: false
>  
>    snps,mtl-rx-config:

You could keep these pointing to child nodes to avoid driver changes.

> +    deprecated: true
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
> -      Multiple RX Queues parameters. Phandle to a node that can
> -      contain the following properties
> -        * snps,rx-queues-to-use, number of RX queues to be used in the
> -          driver
> -        * Choose one of these RX scheduling algorithms
> -          * snps,rx-sched-sp, Strict priority
> -          * snps,rx-sched-wsp, Weighted Strict priority
> -        * For each RX queue
> -          * Choose one of these modes
> -            * snps,dcb-algorithm, Queue to be enabled as DCB
> -            * snps,avb-algorithm, Queue to be enabled as AVB
> -          * snps,map-to-dma-channel, Channel to map
> -          * Specifiy specific packet routing
> -            * snps,route-avcp, AV Untagged Control packets
> -            * snps,route-ptp, PTP Packets
> -            * snps,route-dcbcp, DCB Control Packets
> -            * snps,route-up, Untagged Packets
> -            * snps,route-multi-broad, Multicast & Broadcast Packets
> -          * snps,priority, bitmask of the tagged frames priorities assigned to
> -            the queue
> +      Multiple RX Queues parameters. Phandle to a node that contains the
> +      properties described in the 'mtl-rx-config' sub-node.
> +
> +  mtl-rx-config:
> +    type: object
> +    description: Multiple RX Queues parameters
> +
> +    properties:
> +      snps,rx-queues-to-use:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Number of RX queues to be used in the driver
> +        default: 1
> +        minimum: 1
> +
> +    patternProperties:
> +      "^snps,rx-sched-(sp|wsp)$":
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: Strict/Weighted Strict RX scheduling priority
> +
> +      "^queue[0-9]$":
> +        type: object
> +        description: Each RX Queue parameters
> +
> +        properties:
> +          snps,map-to-dma-channel:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: DMA channel to map
> +
> +          snps,priority:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: RX queue priority
> +            minimum: 0
> +            maximum: 15
> +
> +        patternProperties:
> +          "^snps,(dcb|avb)-algorithm$":
> +            $ref: /schemas/types.yaml#/definitions/flag
> +            description: Enable Queue as DCB/AVB
> +
> +          "^snps,route-(avcp|ptp|dcbcp|up|multi-broad)$":
> +            $ref: /schemas/types.yaml#/definitions/flag
> +            description:
> +              AV Untagged/PTP/DCB Control/Untagged/Multicast & Broadcast
> +              packets routing respectively.
> +
> +        additionalProperties: false
> +
> +        # Choose only one of the Queue modes and the packets routing
> +        allOf:
> +          - not:
> +              required:
> +                - snps,dcb-algorithm
> +                - snps,avb-algorithm
> +          - oneOf:
> +              - required:
> +                  - snps,route-avcp
> +              - required:
> +                  - snps,route-ptp
> +              - required:
> +                  - snps,route-dcbcp
> +              - required:
> +                  - snps,route-up
> +              - required:
> +                  - snps,route-multi-broad
> +              - not:
> +                  anyOf:
> +                    - required:
> +                        - snps,route-avcp
> +                    - required:
> +                        - snps,route-ptp
> +                    - required:
> +                        - snps,route-dcbcp
> +                    - required:
> +                        - snps,route-up
> +                    - required:
> +                        - snps,route-multi-broad

This 'not: ..." could be:

properties:
  snps,route-avcp: false
  snps,route-ptp: false
  snps,route-dcbcp: false
  snps,route-up: false
  snps,route-multi-broad: false

Not sure which one is better. Using required everywhere or more 
concise...

(Really, 'route' should have taken a value and the schema would be 
greatly simplified. Oh well.)

> +
> +    additionalProperties: false
> +
> +    # Choose one of the RX scheduling algorithms
> +    not:
> +      required:
> +        - snps,rx-sched-sp
> +        - snps,rx-sched-wsp

I guess this is the problematic one. The rest should be hidden behind 
conditionals (a common loophole in meta-schema checks). You could do 
that here:

allOf:
  - not:
      ...

But why not just make one of the 2 properties required? You're already 
changing things. 

Rob