Re: [PATCH 1/1] dt-bindings: net: snps,dwmac: Document queue config subnodes

[Sebastian Reichel]

Hi,

On Sat, Oct 22, 2022 at 12:05:15PM -0400, Krzysztof Kozlowski wrote:
> On 21/10/2022 13:10, Sebastian Reichel wrote:
> > The queue configuration is referenced by snps,mtl-rx-config and
> > snps,mtl-tx-config. Most in-tree DTs put the referenced object
> > as child node of the dwmac node.
> > 
> > This adds proper description for this setup, which has the
> > advantage of properly making sure only known properties are
> > used.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > ---
>> [...]
> 
> Please update the DTS example with all this.

ok

> 
> >  
> >    snps,mtl-tx-config:
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >      description:
> > -      Multiple TX Queues parameters. Phandle to a node that can
> > -      contain the following properties
> > -        * snps,tx-queues-to-use, number of TX queues to be used in the
> > -          driver
> > -        * Choose one of these TX scheduling algorithms
> > -          * snps,tx-sched-wrr, Weighted Round Robin
> > -          * snps,tx-sched-wfq, Weighted Fair Queuing
> > -          * snps,tx-sched-dwrr, Deficit Weighted Round Robin
> > -          * snps,tx-sched-sp, Strict priority
> > -        * For each TX queue
> > -          * snps,weight, TX queue weight (if using a DCB weight
> > -            algorithm)
> > -          * Choose one of these modes
> > -            * snps,dcb-algorithm, TX queue will be working in DCB
> > -            * snps,avb-algorithm, TX queue will be working in AVB
> > -              [Attention] Queue 0 is reserved for legacy traffic
> > -                          and so no AVB is available in this queue.
> > -          * Configure Credit Base Shaper (if AVB Mode selected)
> > -            * snps,send_slope, enable Low Power Interface
> > -            * snps,idle_slope, unlock on WoL
> > -            * snps,high_credit, max write outstanding req. limit
> > -            * snps,low_credit, max read outstanding req. limit
> > -          * snps,priority, bitmask of the priorities assigned to the queue.
> > -            When a PFC frame is received with priorities matching the bitmask,
> > -            the queue is blocked from transmitting for the pause time specified
> > -            in the PFC frame.
> > +      Multiple TX Queues parameters. Phandle to a node that
> > +      implements the 'tx-queues-config' object described in
> > +      this binding.
> > +
> > +  tx-queues-config:
> > +    type: object
> > +    properties:
> > +      snps,tx-queues-to-use:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: number of TX queues to be used in the driver
> > +      snps,tx-sched-wrr:
> > +        type: boolean
> > +        description: Weighted Round Robin
> > +      snps,tx-sched-wfq:
> > +        type: boolean
> > +        description: Weighted Fair Queuing
> > +      snps,tx-sched-dwrr:
> > +        type: boolean
> > +        description: Deficit Weighted Round Robin
> > +      snps,tx-sched-sp:
> > +        type: boolean
> > +        description: Strict priority
> > +    patternProperties:
> > +      "^queue[0-9]$":
> > +        description: Each subnode represents a queue.
> > +        type: object
> > +        properties:
> > +          snps,weight:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            description: TX queue weight (if using a DCB weight algorithm)
> > +          snps,dcb-algorithm:
> > +            type: boolean
> > +            description: TX queue will be working in DCB
> > +          snps,avb-algorithm:
> 
> Is DCB and AVB compatible with each other? If not, then this should be
> rather enum (with a string for algorithm name).
> 
> This applies also to other fields which are mutually exclusive.

Yes and I agree it is ugly. But this is not a new binding, but just
properly describing the existing binding. It's not my fault :)

> > +            type: boolean
> > +            description:
> > +              TX queue will be working in AVB.
> > +              Queue 0 is reserved for legacy traffic and so no AVB is
> > +              available in this queue.
> > +          snps,send_slope:
> 
> Use hyphens, no underscores.
> (This is already an incompatible change in bindings, so we can fix up
> the naming)

No, this is not an incompatible change in the bindings. It's 100%
compatible. What this patch does is removing the text description
for 'snps,mtl-tx-config' and instead documenting the node in YAML
syntax. 'snps,mtl-tx-config' does not specify where this node should
be, so many DTS files do this:

ethernet {
    compatible = "blabla";
    snps,mtl-tx-config = <&eth_tx_setup>;
    snps,mtl-rx-config = <&eth_rx_setup>;

    eth_tx_setup: tx-queues-config {
        properties;
    };

    eth_rx_setup: rx-queues-config {
        properties;
    };
};

This right now triggers a dt-validate warning, because the binding
does not expect 'tx-queues-config' and 'rx-queues-config'. This
patch fixes the binding to allow that common setup. Also it improves
the validation for this common case. Having the queue config stored
somewhere else is still supported, but in that case the node is not
validated.

> > +            type: boolean
> > +            description: enable Low Power Interface
> > +          snps,idle_slope:
> > +            type: boolean
> > +            description: unlock on WoL
> > +          snps,high_credit:
> > +            type: boolean
> > +            description: max write outstanding req. limit
> 
> Is it really a boolean?
>
> > +          snps,low_credit:
> > +            type: boolean
> > +            description: max read outstanding req. limit
> 
> Same question

No, they are mistakes on my side. I will fix this in v2.

> > +          snps,priority:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            description:
> > +              Bitmask of the tagged frames priorities assigned to the queue.
> > +              When a PFC frame is received with priorities matching the bitmask,
> > +              the queue is blocked from transmitting for the pause time specified
> > +              in the PFC frame.
> > +    additionalProperties: false
> >  
> >    snps,reset-gpio:
> >      deprecated: true

Thanks,

-- Sebastian

Attachment: signature.asc
Description: PGP signature