| Message ID | 20220906135511.144725-6-sergiu.moga@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make atmel serial driver aware of GCLK | expand |
On 06/09/2022 15:55, Sergiu Moga wrote: > Convert at91 USART DT Binding for Atmel/Microchip SoCs to > json-schema format. Furthermore, move this binding to the > serial directory, since binding directories match hardware, > unlike the driver subsystems which match Linux convention. > > Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com> > --- > > > > v1 -> v2: > - only do what the commit says, split the addition of other compatibles and > properties in other patches > - remove unnecessary "|"'s > - mention header in `atmel,usart-mode`'s description > - place `if:` under `allOf:` > - respect order of spi0's DT properties: compatible, then reg then the reset of properties > > > > > > .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------- > .../bindings/serial/atmel,at91-usart.yaml | 183 ++++++++++++++++++ > 2 files changed, 183 insertions(+), 98 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt > create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt > deleted file mode 100644 > index a09133066aff..000000000000 > --- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt > +++ /dev/null > @@ -1,98 +0,0 @@ > -* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) > - > -Required properties for USART: > -- compatible: Should be one of the following: > - - "atmel,at91rm9200-usart" > - - "atmel,at91sam9260-usart" > - - "microchip,sam9x60-usart" > - - "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart" > - - "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart" > - - "microchip,sam9x60-dbgu", "microchip,sam9x60-usart" > -- reg: Should contain registers location and length > -- interrupts: Should contain interrupt > -- clock-names: tuple listing input clock names. > - Required elements: "usart" > -- clocks: phandles to input clocks. > - > -Required properties for USART in SPI mode: > -- #size-cells : Must be <0> > -- #address-cells : Must be <1> > -- cs-gpios: chipselects (internal cs not supported) > -- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h) > - > -Optional properties in serial and SPI mode: > -- dma bindings for dma transfer: > - - dmas: DMA specifier, consisting of a phandle to DMA controller node, > - memory peripheral interface and USART DMA channel ID, FIFO configuration. > - The order of DMA channels is fixed. The first DMA channel must be TX > - associated channel and the second one must be RX associated channel. > - Refer to dma.txt and atmel-dma.txt for details. > - - dma-names: "tx" for TX channel. > - "rx" for RX channel. > - The order of dma-names is also fixed. The first name must be "tx" > - and the second one must be "rx" as in the examples below. > - > -Optional properties in serial mode: > -- atmel,use-dma-rx: use of PDC or DMA for receiving data > -- atmel,use-dma-tx: use of PDC or DMA for transmitting data > -- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively. > - It will use specified PIO instead of the peripheral function pin for the USART feature. > - If unsure, don't specify this property. > -- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO > - capable USARTs. > -- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt > - > -<chip> compatible description: > -- at91rm9200: legacy USART support > -- at91sam9260: generic USART implementation for SAM9 SoCs > - > -Example: > -- use PDC: > - usart0: serial@fff8c000 { > - compatible = "atmel,at91sam9260-usart"; > - reg = <0xfff8c000 0x4000>; > - interrupts = <7>; > - clocks = <&usart0_clk>; > - clock-names = "usart"; > - atmel,use-dma-rx; > - atmel,use-dma-tx; > - rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>; > - cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>; > - dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>; > - dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>; > - dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>; > - rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>; > - }; > - > -- use DMA: > - usart0: serial@f001c000 { > - compatible = "atmel,at91sam9260-usart"; > - reg = <0xf001c000 0x100>; > - interrupts = <12 4 5>; > - clocks = <&usart0_clk>; > - clock-names = "usart"; > - atmel,use-dma-rx; > - atmel,use-dma-tx; > - dmas = <&dma0 2 0x3>, > - <&dma0 2 0x204>; > - dma-names = "tx", "rx"; > - atmel,fifo-size = <32>; > - }; > - > -- SPI mode: > - #include <dt-bindings/mfd/at91-usart.h> > - > - spi0: spi@f001c000 { > - #address-cells = <1>; > - #size-cells = <0>; > - compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart"; > - atmel,usart-mode = <AT91_USART_MODE_SPI>; > - reg = <0xf001c000 0x100>; > - interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>; > - clocks = <&usart0_clk>; > - clock-names = "usart"; > - dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>, > - <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>; > - dma-names = "tx", "rx"; > - cs-gpios = <&pioB 3 0>; > - }; > diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml > new file mode 100644 > index 000000000000..b25535b7a4d2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml > @@ -0,0 +1,183 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) > + > +maintainers: > + - Richard Genoud <richard.genoud@gmail.com> > + > +properties: > + compatible: > + oneOf: > + - enum: > + - atmel,at91rm9200-usart > + - atmel,at91sam9260-usart > + - microchip,sam9x60-usart > + - items: > + - const: atmel,at91rm9200-dbgu > + - const: atmel,at91rm9200-usart > + - items: > + - const: atmel,at91sam9260-dbgu > + - const: atmel,at91sam9260-usart > + - items: > + - const: microchip,sam9x60-dbgu > + - const: microchip,sam9x60-usart > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clock-names: > + const: usart > + > + clocks: > + maxItems: 1 > + > + dmas: > + items: > + - description: TX DMA Channel > + - description: RX DMA Channel > + > + dma-names: > + items: > + - const: tx > + - const: rx > + > + atmel,usart-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Must be either <AT91_USART_MODE_SPI> for SPI or > + <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h). > + enum: [ 0, 1 ] > + > +required: > + - compatible > + - reg > + - interrupts > + - clock-names > + - clocks > + > +allOf: > + - if: > + properties: > + $nodename: > + pattern: "^serial@[0-9a-f]+$" You should rather check value of atmel,usart-mode, because now you won't properly match device nodes called "foobar". Since usart-mode has only two possible values, this will nicely simplify you if-else. > + then: > + allOf: > + - $ref: /schemas/serial/serial.yaml# > + - $ref: /schemas/serial/rs485.yaml# > + > + properties: > + atmel,use-dma-rx: > + type: boolean > + description: use of PDC or DMA for receiving data > + > + atmel,use-dma-tx: > + type: boolean > + description: use of PDC or DMA for transmitting data > + > + atmel,fifo-size: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Maximum number of data the RX and TX FIFOs can store for FIFO > + capable USARTS. > + enum: [ 16, 32 ] I did not mention it last time, but I think it should follow generic practice, so define all properties top-level and disallow them for other type. This allows you to simply use additionalProperties:false at the end. > + > + else: > + if: > + properties: > + $nodename: > + pattern: "^spi@[0-9a-f]+$" > + then: > + allOf: > + - $ref: /schemas/spi/spi-controller.yaml# > + > + properties: > + atmel,usart-mode: > + const: 1 > + > + "#size-cells": > + const: 0 > + > + "#address-cells": > + const: 1 The same - top level and disallow them for uart. > + > + required: > + - atmel,usart-mode > + - "#size-cells" > + - "#address-cells" End else in this branch is what? > + > +unevaluatedProperties: false > + Best regards, Krzysztof
On 08.09.2022 15:29, Krzysztof Kozlowski wrote: > On 06/09/2022 15:55, Sergiu Moga wrote: >> Convert at91 USART DT Binding for Atmel/Microchip SoCs to >> json-schema format. Furthermore, move this binding to the >> serial directory, since binding directories match hardware, >> unlike the driver subsystems which match Linux convention. >> >> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com> >> --- >> >> >> >> v1 -> v2: >> - only do what the commit says, split the addition of other compatibles and >> properties in other patches >> - remove unnecessary "|"'s >> - mention header in `atmel,usart-mode`'s description >> - place `if:` under `allOf:` >> - respect order of spi0's DT properties: compatible, then reg then the reset of properties >> >> >> >> >> >> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------- >> .../bindings/serial/atmel,at91-usart.yaml | 183 ++++++++++++++++++ >> 2 files changed, 183 insertions(+), 98 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt >> create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml >> >> diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt >> deleted file mode 100644 >> index a09133066aff..000000000000 >> --- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt >> +++ /dev/null >> @@ -1,98 +0,0 @@ >> -* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) >> - >> -Required properties for USART: >> -- compatible: Should be one of the following: >> - - "atmel,at91rm9200-usart" >> - - "atmel,at91sam9260-usart" >> - - "microchip,sam9x60-usart" >> - - "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart" >> - - "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart" >> - - "microchip,sam9x60-dbgu", "microchip,sam9x60-usart" >> -- reg: Should contain registers location and length >> -- interrupts: Should contain interrupt >> -- clock-names: tuple listing input clock names. >> - Required elements: "usart" >> -- clocks: phandles to input clocks. >> - >> -Required properties for USART in SPI mode: >> -- #size-cells : Must be <0> >> -- #address-cells : Must be <1> >> -- cs-gpios: chipselects (internal cs not supported) >> -- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h) >> - >> -Optional properties in serial and SPI mode: >> -- dma bindings for dma transfer: >> - - dmas: DMA specifier, consisting of a phandle to DMA controller node, >> - memory peripheral interface and USART DMA channel ID, FIFO configuration. >> - The order of DMA channels is fixed. The first DMA channel must be TX >> - associated channel and the second one must be RX associated channel. >> - Refer to dma.txt and atmel-dma.txt for details. >> - - dma-names: "tx" for TX channel. >> - "rx" for RX channel. >> - The order of dma-names is also fixed. The first name must be "tx" >> - and the second one must be "rx" as in the examples below. >> - >> -Optional properties in serial mode: >> -- atmel,use-dma-rx: use of PDC or DMA for receiving data >> -- atmel,use-dma-tx: use of PDC or DMA for transmitting data >> -- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively. >> - It will use specified PIO instead of the peripheral function pin for the USART feature. >> - If unsure, don't specify this property. >> -- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO >> - capable USARTs. >> -- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt >> - >> -<chip> compatible description: >> -- at91rm9200: legacy USART support >> -- at91sam9260: generic USART implementation for SAM9 SoCs >> - >> -Example: >> -- use PDC: >> - usart0: serial@fff8c000 { >> - compatible = "atmel,at91sam9260-usart"; >> - reg = <0xfff8c000 0x4000>; >> - interrupts = <7>; >> - clocks = <&usart0_clk>; >> - clock-names = "usart"; >> - atmel,use-dma-rx; >> - atmel,use-dma-tx; >> - rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>; >> - cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>; >> - dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>; >> - dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>; >> - dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>; >> - rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>; >> - }; >> - >> -- use DMA: >> - usart0: serial@f001c000 { >> - compatible = "atmel,at91sam9260-usart"; >> - reg = <0xf001c000 0x100>; >> - interrupts = <12 4 5>; >> - clocks = <&usart0_clk>; >> - clock-names = "usart"; >> - atmel,use-dma-rx; >> - atmel,use-dma-tx; >> - dmas = <&dma0 2 0x3>, >> - <&dma0 2 0x204>; >> - dma-names = "tx", "rx"; >> - atmel,fifo-size = <32>; >> - }; >> - >> -- SPI mode: >> - #include <dt-bindings/mfd/at91-usart.h> >> - >> - spi0: spi@f001c000 { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart"; >> - atmel,usart-mode = <AT91_USART_MODE_SPI>; >> - reg = <0xf001c000 0x100>; >> - interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>; >> - clocks = <&usart0_clk>; >> - clock-names = "usart"; >> - dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>, >> - <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>; >> - dma-names = "tx", "rx"; >> - cs-gpios = <&pioB 3 0>; >> - }; >> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml >> new file mode 100644 >> index 000000000000..b25535b7a4d2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml >> @@ -0,0 +1,183 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) >> + >> +maintainers: >> + - Richard Genoud <richard.genoud@gmail.com> >> + >> +properties: >> + compatible: >> + oneOf: >> + - enum: >> + - atmel,at91rm9200-usart >> + - atmel,at91sam9260-usart >> + - microchip,sam9x60-usart >> + - items: >> + - const: atmel,at91rm9200-dbgu >> + - const: atmel,at91rm9200-usart >> + - items: >> + - const: atmel,at91sam9260-dbgu >> + - const: atmel,at91sam9260-usart >> + - items: >> + - const: microchip,sam9x60-dbgu >> + - const: microchip,sam9x60-usart >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clock-names: >> + const: usart >> + >> + clocks: >> + maxItems: 1 >> + >> + dmas: >> + items: >> + - description: TX DMA Channel >> + - description: RX DMA Channel >> + >> + dma-names: >> + items: >> + - const: tx >> + - const: rx >> + >> + atmel,usart-mode: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Must be either <AT91_USART_MODE_SPI> for SPI or >> + <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h). >> + enum: [ 0, 1 ] >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clock-names >> + - clocks >> + >> +allOf: >> + - if: >> + properties: >> + $nodename: >> + pattern: "^serial@[0-9a-f]+$" > > You should rather check value of atmel,usart-mode, because now you won't > properly match device nodes called "foobar". Since usart-mode has only > two possible values, this will nicely simplify you if-else. > > I did think of that but the previous binding specifies that atmel,usart-mode is required only for the SPI mode and it is optional for the USART mode. That is why I went for the node's regex since I thought it is something that both nodes would have. >> + then: >> + allOf: >> + - $ref: /schemas/serial/serial.yaml# >> + - $ref: /schemas/serial/rs485.yaml# >> + >> + properties: >> + atmel,use-dma-rx: >> + type: boolean >> + description: use of PDC or DMA for receiving data >> + >> + atmel,use-dma-tx: >> + type: boolean >> + description: use of PDC or DMA for transmitting data >> + >> + atmel,fifo-size: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Maximum number of data the RX and TX FIFOs can store for FIFO >> + capable USARTS. >> + enum: [ 16, 32 ] > > I did not mention it last time, but I think it should follow generic > practice, so define all properties top-level and disallow them for other > type. This allows you to simply use additionalProperties:false at the end. > What would be a good example binding in this case? >> + >> + else: >> + if: >> + properties: >> + $nodename: >> + pattern: "^spi@[0-9a-f]+$" >> + then: >> + allOf: >> + - $ref: /schemas/spi/spi-controller.yaml# >> + >> + properties: >> + atmel,usart-mode: >> + const: 1 >> + >> + "#size-cells": >> + const: 0 >> + >> + "#address-cells": >> + const: 1 > > The same - top level and disallow them for uart. > These values of #size-cells and #address-cells are only meant for the SPI so I guess I would still have to specify their mandatory const values here. >> + >> + required: >> + - atmel,usart-mode >> + - "#size-cells" >> + - "#address-cells" > > End else in this branch is what? > You are right, I will remove the useless if: after else: >> + >> +unevaluatedProperties: false >> + > > Best regards, > Krzysztof Regards, Sergiu
On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote: > On 08.09.2022 15:29, Krzysztof Kozlowski wrote: >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clock-names >>> + - clocks >>> + >>> +allOf: >>> + - if: >>> + properties: >>> + $nodename: >>> + pattern: "^serial@[0-9a-f]+$" >> >> You should rather check value of atmel,usart-mode, because now you won't >> properly match device nodes called "foobar". Since usart-mode has only >> two possible values, this will nicely simplify you if-else. >> >> > > > I did think of that but the previous binding specifies that > atmel,usart-mode is required only for the SPI mode and it is optional > for the USART mode. That is why I went for the node's regex since I > thought it is something that both nodes would have. I think it should be explicit - you configure node either to this or that, so the property should be always present. The node name should not be responsible for it, even though we want node names to match certain patterns. > > >>> + then: >>> + allOf: >>> + - $ref: /schemas/serial/serial.yaml# >>> + - $ref: /schemas/serial/rs485.yaml# >>> + >>> + properties: >>> + atmel,use-dma-rx: >>> + type: boolean >>> + description: use of PDC or DMA for receiving data >>> + >>> + atmel,use-dma-tx: >>> + type: boolean >>> + description: use of PDC or DMA for transmitting data >>> + >>> + atmel,fifo-size: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Maximum number of data the RX and TX FIFOs can store for FIFO >>> + capable USARTS. >>> + enum: [ 16, 32 ] >> >> I did not mention it last time, but I think it should follow generic >> practice, so define all properties top-level and disallow them for other >> type. This allows you to simply use additionalProperties:false at the end. >> > > > What would be a good example binding in this case? The example binding. https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212 > > >>> + >>> + else: >>> + if: >>> + properties: >>> + $nodename: >>> + pattern: "^spi@[0-9a-f]+$" >>> + then: >>> + allOf: >>> + - $ref: /schemas/spi/spi-controller.yaml# >>> + >>> + properties: >>> + atmel,usart-mode: >>> + const: 1 >>> + >>> + "#size-cells": >>> + const: 0 >>> + >>> + "#address-cells": >>> + const: 1 >> >> The same - top level and disallow them for uart. >> > > > These values of #size-cells and #address-cells are only meant for the > SPI so I guess I would still have to specify their mandatory const > values here. Sure, ok. > > >>> + >>> + required: >>> + - atmel,usart-mode >>> + - "#size-cells" >>> + - "#address-cells" >> >> End else in this branch is what? >> > > > You are right, I will remove the useless if: after else: Best regards, Krzysztof
On 08.09.2022 18:10, Krzysztof Kozlowski wrote: > On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote: >> On 08.09.2022 15:29, Krzysztof Kozlowski wrote: > >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - interrupts >>>> + - clock-names >>>> + - clocks >>>> + >>>> +allOf: >>>> + - if: >>>> + properties: >>>> + $nodename: >>>> + pattern: "^serial@[0-9a-f]+$" >>> >>> You should rather check value of atmel,usart-mode, because now you won't >>> properly match device nodes called "foobar". Since usart-mode has only >>> two possible values, this will nicely simplify you if-else. >>> >>> >> >> >> I did think of that but the previous binding specifies that >> atmel,usart-mode is required only for the SPI mode and it is optional >> for the USART mode. That is why I went for the node's regex since I >> thought it is something that both nodes would have. > > I think it should be explicit - you configure node either to this or > that, so the property should be always present. No DT of ours has that property atm, since they are all on USART mode by default. If I were to make it required. all nodes would fail so I would have to add it to each of them. > The node name should not > be responsible for it, even though we want node names to match certain > patterns. > Does checkig for the node's pattern not make it better then? Since it imposes an additional check? If it would not have a conventional pattern, it would fail through unevaluatedProperies:false at the end, since it would have properties that were contained inside a branch that the validation of the node would not have gone through since it contains a pattern that does not match the conditions of that branch. >> >> >>>> + then: >>>> + allOf: >>>> + - $ref: /schemas/serial/serial.yaml# >>>> + - $ref: /schemas/serial/rs485.yaml# >>>> + >>>> + properties: >>>> + atmel,use-dma-rx: >>>> + type: boolean >>>> + description: use of PDC or DMA for receiving data >>>> + >>>> + atmel,use-dma-tx: >>>> + type: boolean >>>> + description: use of PDC or DMA for transmitting data >>>> + >>>> + atmel,fifo-size: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: >>>> + Maximum number of data the RX and TX FIFOs can store for FIFO >>>> + capable USARTS. >>>> + enum: [ 16, 32 ] >>> >>> I did not mention it last time, but I think it should follow generic >>> practice, so define all properties top-level and disallow them for other >>> type. This allows you to simply use additionalProperties:false at the end. >>> >> >> >> What would be a good example binding in this case? > > The example binding. > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212 > Ah, I understand now. I did not get what you meant by "disallow", I guess it's just a "property-name: false". Thanks! >> >> >>>> + >>>> + else: >>>> + if: >>>> + properties: >>>> + $nodename: >>>> + pattern: "^spi@[0-9a-f]+$" >>>> + then: >>>> + allOf: >>>> + - $ref: /schemas/spi/spi-controller.yaml# >>>> + >>>> + properties: >>>> + atmel,usart-mode: >>>> + const: 1 >>>> + >>>> + "#size-cells": >>>> + const: 0 >>>> + >>>> + "#address-cells": >>>> + const: 1 >>> >>> The same - top level and disallow them for uart. >>> >> >> >> These values of #size-cells and #address-cells are only meant for the >> SPI so I guess I would still have to specify their mandatory const >> values here. > > Sure, ok. > >> >> >>>> + >>>> + required: >>>> + - atmel,usart-mode >>>> + - "#size-cells" >>>> + - "#address-cells" >>> >>> End else in this branch is what? >>> >> >> >> You are right, I will remove the useless if: after else: > > Best regards, > Krzysztof Regards, Sergiu
On 08/09/2022 17:27, Sergiu.Moga@microchip.com wrote: > On 08.09.2022 18:10, Krzysztof Kozlowski wrote: >> On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote: >>> On 08.09.2022 15:29, Krzysztof Kozlowski wrote: >> >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - interrupts >>>>> + - clock-names >>>>> + - clocks >>>>> + >>>>> +allOf: >>>>> + - if: >>>>> + properties: >>>>> + $nodename: >>>>> + pattern: "^serial@[0-9a-f]+$" >>>> >>>> You should rather check value of atmel,usart-mode, because now you won't >>>> properly match device nodes called "foobar". Since usart-mode has only >>>> two possible values, this will nicely simplify you if-else. >>>> >>>> >>> >>> >>> I did think of that but the previous binding specifies that >>> atmel,usart-mode is required only for the SPI mode and it is optional >>> for the USART mode. That is why I went for the node's regex since I >>> thought it is something that both nodes would have. >> >> I think it should be explicit - you configure node either to this or >> that, so the property should be always present. > > > > No DT of ours has that property atm, since they are all on USART mode by > default. If I were to make it required. all nodes would fail so I would > have to add it to each of them. Which is a problem because...? Have in mind that bindings can be changed. ABI here won't be broken. > > > > >> The node name should not >> be responsible for it, even though we want node names to match certain >> patterns. >> > > > Does checkig for the node's pattern not make it better then? Since it > imposes an additional check? Not really, because if it is "foobar" your schema would not be applied correctly. > If it would not have a conventional > pattern, it would fail through unevaluatedProperies:false at the end, > since it would have properties that were contained inside a branch that > the validation of the node would not have gone through since it contains > a pattern that does not match the conditions of that branch. Not for properties which are for example missing... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt deleted file mode 100644 index a09133066aff..000000000000 --- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt +++ /dev/null @@ -1,98 +0,0 @@ -* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) - -Required properties for USART: -- compatible: Should be one of the following: - - "atmel,at91rm9200-usart" - - "atmel,at91sam9260-usart" - - "microchip,sam9x60-usart" - - "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart" - - "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart" - - "microchip,sam9x60-dbgu", "microchip,sam9x60-usart" -- reg: Should contain registers location and length -- interrupts: Should contain interrupt -- clock-names: tuple listing input clock names. - Required elements: "usart" -- clocks: phandles to input clocks. - -Required properties for USART in SPI mode: -- #size-cells : Must be <0> -- #address-cells : Must be <1> -- cs-gpios: chipselects (internal cs not supported) -- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h) - -Optional properties in serial and SPI mode: -- dma bindings for dma transfer: - - dmas: DMA specifier, consisting of a phandle to DMA controller node, - memory peripheral interface and USART DMA channel ID, FIFO configuration. - The order of DMA channels is fixed. The first DMA channel must be TX - associated channel and the second one must be RX associated channel. - Refer to dma.txt and atmel-dma.txt for details. - - dma-names: "tx" for TX channel. - "rx" for RX channel. - The order of dma-names is also fixed. The first name must be "tx" - and the second one must be "rx" as in the examples below. - -Optional properties in serial mode: -- atmel,use-dma-rx: use of PDC or DMA for receiving data -- atmel,use-dma-tx: use of PDC or DMA for transmitting data -- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively. - It will use specified PIO instead of the peripheral function pin for the USART feature. - If unsure, don't specify this property. -- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO - capable USARTs. -- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt - -<chip> compatible description: -- at91rm9200: legacy USART support -- at91sam9260: generic USART implementation for SAM9 SoCs - -Example: -- use PDC: - usart0: serial@fff8c000 { - compatible = "atmel,at91sam9260-usart"; - reg = <0xfff8c000 0x4000>; - interrupts = <7>; - clocks = <&usart0_clk>; - clock-names = "usart"; - atmel,use-dma-rx; - atmel,use-dma-tx; - rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>; - cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>; - dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>; - dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>; - dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>; - rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>; - }; - -- use DMA: - usart0: serial@f001c000 { - compatible = "atmel,at91sam9260-usart"; - reg = <0xf001c000 0x100>; - interrupts = <12 4 5>; - clocks = <&usart0_clk>; - clock-names = "usart"; - atmel,use-dma-rx; - atmel,use-dma-tx; - dmas = <&dma0 2 0x3>, - <&dma0 2 0x204>; - dma-names = "tx", "rx"; - atmel,fifo-size = <32>; - }; - -- SPI mode: - #include <dt-bindings/mfd/at91-usart.h> - - spi0: spi@f001c000 { - #address-cells = <1>; - #size-cells = <0>; - compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart"; - atmel,usart-mode = <AT91_USART_MODE_SPI>; - reg = <0xf001c000 0x100>; - interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>; - clocks = <&usart0_clk>; - clock-names = "usart"; - dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>, - <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>; - dma-names = "tx", "rx"; - cs-gpios = <&pioB 3 0>; - }; diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml new file mode 100644 index 000000000000..b25535b7a4d2 --- /dev/null +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml @@ -0,0 +1,183 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART) + +maintainers: + - Richard Genoud <richard.genoud@gmail.com> + +properties: + compatible: + oneOf: + - enum: + - atmel,at91rm9200-usart + - atmel,at91sam9260-usart + - microchip,sam9x60-usart + - items: + - const: atmel,at91rm9200-dbgu + - const: atmel,at91rm9200-usart + - items: + - const: atmel,at91sam9260-dbgu + - const: atmel,at91sam9260-usart + - items: + - const: microchip,sam9x60-dbgu + - const: microchip,sam9x60-usart + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clock-names: + const: usart + + clocks: + maxItems: 1 + + dmas: + items: + - description: TX DMA Channel + - description: RX DMA Channel + + dma-names: + items: + - const: tx + - const: rx + + atmel,usart-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Must be either <AT91_USART_MODE_SPI> for SPI or + <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h). + enum: [ 0, 1 ] + +required: + - compatible + - reg + - interrupts + - clock-names + - clocks + +allOf: + - if: + properties: + $nodename: + pattern: "^serial@[0-9a-f]+$" + then: + allOf: + - $ref: /schemas/serial/serial.yaml# + - $ref: /schemas/serial/rs485.yaml# + + properties: + atmel,use-dma-rx: + type: boolean + description: use of PDC or DMA for receiving data + + atmel,use-dma-tx: + type: boolean + description: use of PDC or DMA for transmitting data + + atmel,fifo-size: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Maximum number of data the RX and TX FIFOs can store for FIFO + capable USARTS. + enum: [ 16, 32 ] + + else: + if: + properties: + $nodename: + pattern: "^spi@[0-9a-f]+$" + then: + allOf: + - $ref: /schemas/spi/spi-controller.yaml# + + properties: + atmel,usart-mode: + const: 1 + + "#size-cells": + const: 0 + + "#address-cells": + const: 1 + + required: + - atmel,usart-mode + - "#size-cells" + - "#address-cells" + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/mfd/at91-usart.h> + #include <dt-bindings/dma/at91.h> + + /* use PDC */ + usart0: serial@fff8c000 { + compatible = "atmel,at91sam9260-usart"; + reg = <0xfff8c000 0x4000>; + interrupts = <7>; + clocks = <&usart0_clk>; + clock-names = "usart"; + atmel,use-dma-rx; + atmel,use-dma-tx; + rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>; + cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>; + dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>; + dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>; + dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>; + rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/mfd/at91-usart.h> + #include <dt-bindings/dma/at91.h> + + /* use DMA */ + usart1: serial@f001c000 { + compatible = "atmel,at91sam9260-usart"; + reg = <0xf001c000 0x100>; + interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>; + clocks = <&usart0_clk>; + clock-names = "usart"; + atmel,use-dma-rx; + atmel,use-dma-tx; + dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>, + <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>; + dma-names = "tx", "rx"; + atmel,fifo-size = <32>; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/mfd/at91-usart.h> + #include <dt-bindings/dma/at91.h> + + /* SPI mode */ + spi0: spi@f001c000 { + compatible = "atmel,at91sam9260-usart"; + reg = <0xf001c000 0x100>; + #address-cells = <1>; + #size-cells = <0>; + atmel,usart-mode = <AT91_USART_MODE_SPI>; + interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>; + clocks = <&usart0_clk>; + clock-names = "usart"; + dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>, + <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>; + dma-names = "tx", "rx"; + cs-gpios = <&pioB 3 GPIO_ACTIVE_HIGH>; + };
Convert at91 USART DT Binding for Atmel/Microchip SoCs to json-schema format. Furthermore, move this binding to the serial directory, since binding directories match hardware, unlike the driver subsystems which match Linux convention. Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com> --- v1 -> v2: - only do what the commit says, split the addition of other compatibles and properties in other patches - remove unnecessary "|"'s - mention header in `atmel,usart-mode`'s description - place `if:` under `allOf:` - respect order of spi0's DT properties: compatible, then reg then the reset of properties .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------- .../bindings/serial/atmel,at91-usart.yaml | 183 ++++++++++++++++++ 2 files changed, 183 insertions(+), 98 deletions(-) delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml