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

[Krzysztof Kozlowski]

On 24/10/2022 19:28, Krzysztof Kozlowski wrote:
> On 24/10/2022 18:28, Sebastian Reichel wrote:
>> 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
> 
> BTW, I also found:
> 
> https://lore.kernel.org/linux-devicetree/20201214091616.13545-5-Sergey.Semin@xxxxxxxxxxxxxxxxxxxx/
>>
>>>
>>>>  
>>>>    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 :)
> 
> I understand (and did not think it's your fault), but you are
> redesigning them. Existing DTS will have to be updated. If this is
> already implemented by some other DTS, then well... they did not follow
> bindings, so it's their fault. :)
> 
> What I want to say, why refactoring it if the new format is still poor?
>>
>>>> +            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:
> 
> Old binding did not document "tx-queues-config". Old binding had
> "snps,mtl-tx-config" which was a phandle, so this is an ABI break of
> bindings.

Bah, not ABI break, just change in bindings, of course :)

Best regards,
Krzysztof