On Thu, Feb 11, 2021 at 12:58:00AM +0300, Serge Semin wrote: > On Tue, Feb 09, 2021 at 04:26:08PM -0600, Rob Herring wrote: > > On Mon, Feb 08, 2021 at 04:55:48PM +0300, Serge Semin wrote: > > > Currently the "snps,axi-config", "snps,mtl-rx-config" and > > > "snps,mtl-tx-config" properties are declared as a single phandle reference > > > to a node with corresponding parameters defined. That's not good for > > > several reasons. First of all scattering around a device tree some > > > particular device-specific configs with no visual relation to that device > > > isn't suitable from maintainability point of view. That leads to a > > > disturbed representation of the actual device tree mixing actual device > > > nodes and some vendor-specific configs. Secondly using the same configs > > > set for several device nodes doesn't represent well the devices structure, > > > since the interfaces these configs describe in hardware belong to > > > different devices and may actually differ. In the later case having the > > > configs node separated from the corresponding device nodes gets to be > > > even unjustified. > > > > > > So instead of having a separate DW *MAC configs nodes we suggest to > > > define them as sub-nodes of the device nodes, which interfaces they > > > actually describe. By doing so we'll make the DW *MAC nodes visually > > > correct describing all the aspects of the IP-core configuration. Thus > > > we'll be able to describe the configs sub-nodes bindings right in the > > > snps,dwmac.yaml file. > > > > > > Note the former "snps,axi-config", "snps,mtl-rx-config" and > > > "snps,mtl-tx-config" properties have been marked as deprecated in favor of > > > the added by this commit "axi-config", "mtl-rx-config" and "mtl-tx-config" > > > sub-nodes respectively. > > > > > > Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx> > > > > > > --- > > > > > > Note this change will work only if DT-schema tool is fixed like this: > > > > > > --- a/meta-schemas/nodes.yaml 2021-02-08 14:20:56.732447780 +0300 > > > +++ b/meta-schemas/nodes.yaml 2021-02-08 14:21:00.736492245 +0300 > > > @@ -22,6 +22,7 @@ > > > - unevaluatedProperties > > > - deprecated > > > - required > > > + - not > > > - allOf > > > - anyOf > > > - oneOf > > > > > Can you send me a patch or GH PR. There is another way to express. More > > below. > > Ok. I'll send a patch. To what email and mailing lists shall I send it > to? Rob, any comments on my questions above and below? -Sergey > > > > > > > > > So a property with name "not" would be allowed and the "not-required" > > > pattern would work. > > > > > > Changelog v2: > > > - Add the new sub-nodes "axi-config", "mtl-rx-config" and "mtl-tx-config" > > > describing the nodes now deprecated properties were supposed to > > > refer to. > > > - Fix invalid identation in the "snps,route-*" property settings. > > > - Use correct syntax of the JSON pointers, so the later would begin > > > with a '/' after the '#'. > > > --- > > > .../devicetree/bindings/net/snps,dwmac.yaml | 389 +++++++++++++----- > > > 1 file changed, 297 insertions(+), 92 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > index 03d58bf9965f..4dda9ffa822c 100644 > > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > > @@ -150,73 +150,264 @@ properties: > > > in a different mode than the PHY in order to function. > > > > > > snps,axi-config: > > > + deprecated: true > > > $ref: /schemas/types.yaml#/definitions/phandle > > > description: > > > - AXI BUS Mode parameters. Phandle to a node that can contain the > > > - following properties > > > - * snps,lpi_en, enable Low Power Interface > > > - * snps,xit_frm, unlock on WoL > > > - * snps,wr_osr_lmt, max write outstanding req. limit > > > - * snps,rd_osr_lmt, max read outstanding req. limit > > > - * snps,kbbe, do not cross 1KiB boundary. > > > - * snps,blen, this is a vector of supported burst length. > > > - * snps,fb, fixed-burst > > > - * snps,mb, mixed-burst > > > - * snps,rb, rebuild INCRx Burst > > > + AXI BUS Mode parameters. Phandle to a node that contains the properties > > > + described in the 'axi-config' sub-node. > > > + > > > + axi-config: > > > + type: object > > > + description: AXI BUS Mode parameters > > > + > > > + properties: > > > + snps,lpi_en: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Enable Low Power Interface > > > + > > > + snps,xit_frm: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Unlock on WoL > > > + > > > + snps,wr_osr_lmt: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: Max write outstanding req. limit > > > + default: 1 > > > + minimum: 0 > > > + maximum: 15 > > > + > > > + snps,rd_osr_lmt: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: Max read outstanding req. limit > > > + default: 1 > > > + minimum: 0 > > > + maximum: 15 > > > + > > > + snps,kbbe: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Do not cross 1KiB boundary > > > + > > > + snps,blen: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: A vector of supported burst lengths > > > + minItems: 7 > > > + maxItems: 7 > > > + items: > > > + enum: [256, 128, 64, 32, 16, 8, 4, 0] > > > + > > > + snps,fb: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Fixed-burst > > > + > > > + snps,mb: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Mixed-burst > > > + > > > + snps,rb: > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Rebuild INCRx Burst > > > + > > > + additionalProperties: false > > > > > > snps,mtl-rx-config: > > > > > You could keep these pointing to child nodes to avoid driver changes. > > Right, but I'd prefer having the AXI/MTL-config nodes directly found > because they are supposed to be defined as sub-nodes anyway. Having > the snps-prefixed AXI/MTL-config properties with phandle reference are > going to be marked as deprecated. By doing so we'll discourage > defining the DW MAC-related configs scattered around the new dts-es. > > > > > > + deprecated: true > > > $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 contains the > > > + properties described in the 'mtl-rx-config' sub-node. > > > + > > > + mtl-rx-config: > > > + type: object > > > + description: Multiple RX Queues parameters > > > + > > > + properties: > > > + snps,rx-queues-to-use: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: Number of RX queues to be used in the driver > > > + default: 1 > > > + minimum: 1 > > > + > > > + patternProperties: > > > + "^snps,rx-sched-(sp|wsp)$": > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Strict/Weighted Strict RX scheduling priority > > > + > > > + "^queue[0-9]$": > > > + type: object > > > + description: Each RX Queue parameters > > > + > > > + properties: > > > + snps,map-to-dma-channel: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: DMA channel to map > > > + > > > + snps,priority: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: RX queue priority > > > + minimum: 0 > > > + maximum: 15 > > > + > > > + patternProperties: > > > + "^snps,(dcb|avb)-algorithm$": > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: Enable Queue as DCB/AVB > > > + > > > + "^snps,route-(avcp|ptp|dcbcp|up|multi-broad)$": > > > + $ref: /schemas/types.yaml#/definitions/flag > > > + description: > > > + AV Untagged/PTP/DCB Control/Untagged/Multicast & Broadcast > > > + packets routing respectively. > > > + > > > + additionalProperties: false > > > + > > > + # Choose only one of the Queue modes and the packets routing > > > + allOf: > > > + - not: > > > + required: > > > + - snps,dcb-algorithm > > > + - snps,avb-algorithm > > > + - oneOf: > > > + - required: > > > + - snps,route-avcp > > > + - required: > > > + - snps,route-ptp > > > + - required: > > > + - snps,route-dcbcp > > > + - required: > > > + - snps,route-up > > > + - required: > > > + - snps,route-multi-broad > > > + - not: > > > + anyOf: > > > + - required: > > > + - snps,route-avcp > > > + - required: > > > + - snps,route-ptp > > > + - required: > > > + - snps,route-dcbcp > > > + - required: > > > + - snps,route-up > > > + - required: > > > + - snps,route-multi-broad > > > > > This 'not: ..." could be: > > > > properties: > > snps,route-avcp: false > > snps,route-ptp: false > > snps,route-dcbcp: false > > snps,route-up: false > > snps,route-multi-broad: false > > > > Not sure which one is better. Using required everywhere or more > > concise... > > Thanks for suggesting an alternative. I didn't figure out such option > myself. Though in this case since we need to use the required property > anyway, I'd prefer to have it used in the 'not' sub-schema too. At > least for uniformity and to simplify the conditional statement > readability, even if it causes a bit of the concise loss. > > > > > (Really, 'route' should have taken a value and the schema would be > > greatly simplified. Oh well.) > > Yeah, I had the same thought in mind when first saw that > boolean-properties hell. There are few more properties in this > bindings file which should have been also defined as taking values > instead of being booleans... > > > > > > + > > > + additionalProperties: false > > > + > > > + # Choose one of the RX scheduling algorithms > > > + not: > > > + required: > > > + - snps,rx-sched-sp > > > + - snps,rx-sched-wsp > > > > > I guess this is the problematic one. The rest should be hidden behind > > conditionals (a common loophole in meta-schema checks). You could do > > that here: > > > > allOf: > > - not: > > ... > > Oh, thanks. I can't believe I've missed that option. Though fixing the > dt-schema tool would be better than using the combining schema > keywords as a workaround. > > > > > But why not just make one of the 2 properties required? You're already > > changing things. > > First of all the driver permits omitting all of them in the > corresponding nodes and implicitly using one of the properties by > default (the same thing is for the MTL Tx-queues). If we made some of > them being required we would have broken the driver dts-contract, > which is not good. Secondly I'd have to fix the > arch/arm/boot/dts/artpec6.dtsi dts file too, which would have been an > additional patch in the series, additional work, additional review > from the platform maintainer, additional merge path, etc. So to speak > that would cause more troubles, than just using the "not:" statement > here.) > > -Sergey > > > > > Rob
