Date:   Mon, 24 Oct 2022 19:28:29 -0400
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sebastian Reichel <sebastian.reichel@...labora.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Jose Abreu <joabreu@...opsys.com>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH 1/1] dt-bindings: net: snps,dwmac: Document queue config
 subnodes

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@...labora.com>
>>> ---
>>> [...]
>>
>> Please update the DTS example with all this.
> 
> ok

BTW, I also found:

https://lore.kernel.org/linux-devicetree/20201214091616.13545-5-Sergey.Semin@baikalelectronics.ru/
> 
>>
>>>  
>>>    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.

You are changing the binding - adding new properties.

> 
> 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.

Yes, I understand. It also brings a way to add new bindings bypassing
the review process. It's super easy now to omit review, just do like this:
1. Submit DTS with anything, without bindings.
2. It might be applied. If not, keep resubmitting to different
maintainers or platforms.
3. Once it is in mainline, send whatever changes one wants saying "I am
just fixing bindings" or "documenting existing usage".

The properties were added in d976a525c371 ("net: stmmac: multiple queues
dt configuration") which was not reviewed/acked by Rob.

Awesome! My method of bypassing review works!

> 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.

I understand this as well and your way is good. I am not objecting to
the method itself.

But if you refactor this and introduce new properties to the binding,
please make them correct.

> 
Best regards,
Krzysztof

Powered by blists - more mailing lists