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

  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@collabora.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   | 154 ++++++++++++------
 1 file changed, 108 insertions(+), 46 deletions(-)
  

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@collabora.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   | 154 ++++++++++++------
>  1 file changed, 108 insertions(+), 46 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 13b984076af5..0bf6112cec2f 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -167,56 +167,118 @@ properties:
>    snps,mtl-rx-config:
>      $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
> +      implements the 'rx-queues-config' object described in
> +      this binding.
> +
> +  rx-queues-config:

If this field is specific to this device, then you need vendor prefix:
snps,rq-queues-config

> +    type: object
> +    properties:
> +      snps,rx-queues-to-use:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: number of RX queues to be used in the driver
> +      snps,rx-sched-sp:
> +        type: boolean
> +        description: Strict priority
> +      snps,rx-sched-wsp:
> +        type: boolean
> +        description: Weighted Strict priority
> +    patternProperties:
> +      "^queue[0-9]$":
> +        description: Each subnode represents a queue.
> +        type: object
> +        properties:
> +          snps,dcb-algorithm:
> +            type: boolean
> +            description: Queue to be enabled as DCB
> +          snps,avb-algorithm:
> +            type: boolean
> +            description: Queue to be enabled as AVB
> +          snps,map-to-dma-channel:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: DMA channel id to map
> +          snps,route-avcp:
> +            type: boolean
> +            description: AV Untagged Control packets
> +          snps,route-ptp:
> +            type: boolean
> +            description: PTP Packets
> +          snps,route-dcbcp:
> +            type: boolean
> +            description: DCB Control Packets
> +          snps,route-up:
> +            type: boolean
> +            description: Untagged Packets
> +          snps,route-multi-broad:
> +            type: boolean
> +            description: Multicast & Broadcast Packets
> +          snps,priority:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: Bitmask of the tagged frames priorities assigned to the queue
> +    additionalProperties: false

Please update the DTS example with all this.

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

> +            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)

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

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

Best regards,
Krzysztof
  

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@collabora.com>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml   | 154 ++++++++++++------
> >  1 file changed, 108 insertions(+), 46 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index 13b984076af5..0bf6112cec2f 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -167,56 +167,118 @@ properties:
> >    snps,mtl-rx-config:
> >      $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
> > +      implements the 'rx-queues-config' object described in
> > +      this binding.
> > +
> > +  rx-queues-config:
> 
> If this field is specific to this device, then you need vendor prefix:
> snps,rq-queues-config

Not for a node name...

Rob
  

On 24/10/2022 14:53, Rob Herring wrote:
> 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@collabora.com>
>>> ---
>>>  .../devicetree/bindings/net/snps,dwmac.yaml   | 154 ++++++++++++------
>>>  1 file changed, 108 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index 13b984076af5..0bf6112cec2f 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -167,56 +167,118 @@ properties:
>>>    snps,mtl-rx-config:
>>>      $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
>>> +      implements the 'rx-queues-config' object described in
>>> +      this binding.
>>> +
>>> +  rx-queues-config:
>>
>> If this field is specific to this device, then you need vendor prefix:
>> snps,rq-queues-config
> 
> Not for a node name...

Right.

Best regards,
Krzysztof
  

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@collabora.com>
> > ---
>> [...]
> 
> 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
  

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

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

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

Best regards,
Krzysztof
  

Hi,

On Mon, Oct 24, 2022 at 07:28:29PM -0400, Krzysztof Kozlowski wrote:
> 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.

The new binding still has the phandle. The only thing I changed is
explicitly allowing the referenced node to be a subnode of the dwmac
node. This is 100% compatible, since the binding does not specify
where the referenced node should be. Only the example suggested it
could be next to the ethernet node. But changing any properties in
the config node means a ABI break requiring code changes.

Note, that right now 4/7 devicetrees with snps,mtl-tx-config already
follow the scheme I documented. The other 3 have the queue config
below the root node like the current example:

has the queues config in /:
 * arch/arm/boot/dts/artpec6.dtsi
 * arch/arm64/boot/dts/mediatek/mt2712e.dtsi
 * arch/arm64/boot/dts/qcom/sa8155p-adp.dts

has the queues config in the ethernet node:
 * arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
 * arch/arm64/boot/dts/freescale/imx8mp-evk.dts
 * arch/arm64/boot/dts/rockchip/rk3568.dtsi
 * arch/arm64/boot/dts/rockchip/rk356x.dtsi

After my change both are considered valid. Anyways I'm doing this
for rk3588 and planned to follow the subnode style. But if I have
to fully fix this mess I will just put the queue config to the
root node instead and let somebody else figure this out.

-- Sebastian
  

On 25/10/2022 10:17, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Oct 24, 2022 at 07:28:29PM -0400, Krzysztof Kozlowski wrote:
>> 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.
> 
> The new binding still has the phandle. The only thing I changed is
> explicitly allowing the referenced node to be a subnode of the dwmac
> node. This is 100% compatible, since the binding does not specify
> where the referenced node should be. Only the example suggested it
> could be next to the ethernet node. But changing any properties in
> the config node means a ABI break requiring code changes.
> 
> Note, that right now 4/7 devicetrees with snps,mtl-tx-config already
> follow the scheme I documented. The other 3 have the queue config
> below the root node like the current example:
> 
> has the queues config in /:
>  * arch/arm/boot/dts/artpec6.dtsi
>  * arch/arm64/boot/dts/mediatek/mt2712e.dtsi
>  * arch/arm64/boot/dts/qcom/sa8155p-adp.dts
> 
> has the queues config in the ethernet node:
>  * arch/arm64/boot/dts/freescale/imx8mp-verdin.dtsi
>  * arch/arm64/boot/dts/freescale/imx8mp-evk.dts
>  * arch/arm64/boot/dts/rockchip/rk3568.dtsi
>  * arch/arm64/boot/dts/rockchip/rk356x.dtsi
> 
> After my change both are considered valid. Anyways I'm doing this
> for rk3588 and planned to follow the subnode style. But if I have
> to fully fix this mess I will just put the queue config to the
> root node instead and let somebody else figure this out.

Yeah, let it be.

Best regards,
Krzysztof
  

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@collabora.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   | 154 ++++++++++++------
>  1 file changed, 108 insertions(+), 46 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 13b984076af5..0bf6112cec2f 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -167,56 +167,118 @@ properties:
>    snps,mtl-rx-config:
>      $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
> +      implements the 'rx-queues-config' object described in
> +      this binding.
> +
> +  rx-queues-config:
> +    type: object
> +    properties:
> +      snps,rx-queues-to-use:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: number of RX queues to be used in the driver
> +      snps,rx-sched-sp:
> +        type: boolean
> +        description: Strict priority
> +      snps,rx-sched-wsp:
> +        type: boolean
> +        description: Weighted Strict priority
> +    patternProperties:
> +      "^queue[0-9]$":
> +        description: Each subnode represents a queue.
> +        type: object
> +        properties:
> +          snps,dcb-algorithm:
> +            type: boolean
> +            description: Queue to be enabled as DCB
> +          snps,avb-algorithm:
> +            type: boolean
> +            description: Queue to be enabled as AVB
> +          snps,map-to-dma-channel:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: DMA channel id to map
> +          snps,route-avcp:
> +            type: boolean
> +            description: AV Untagged Control packets
> +          snps,route-ptp:
> +            type: boolean
> +            description: PTP Packets
> +          snps,route-dcbcp:
> +            type: boolean
> +            description: DCB Control Packets
> +          snps,route-up:
> +            type: boolean
> +            description: Untagged Packets
> +          snps,route-multi-broad:
> +            type: boolean
> +            description: Multicast & Broadcast Packets
> +          snps,priority:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description: Bitmask of the tagged frames priorities assigned to the queue

If we are not going to fix it, at least let's improve the constraints,
so add allOf:if:then here (with proper indentation) which disallows
mixing mutually exclusive properties.
Here's example:
https://elixir.bootlin.com/linux/v5.17-rc2/source/Documentation/devicetree/bindings/mfd/samsung,s5m8767.yaml#L155

Best regards,
Krzysztof