| Message ID | 20221021171055.85888-1-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] dt-bindings: net: snps,dwmac: Document queue config subnodes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 = <ð_tx_setup>; snps,mtl-rx-config = <ð_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 = <ð_tx_setup>; > snps,mtl-rx-config = <ð_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
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 + additionalProperties: false 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: + 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: + 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 + snps,low_credit: + type: boolean + description: max read outstanding req. limit + 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
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(-)