| Message ID | 20230210072643.2772-2-chiawei_wang@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: aspeed: Add UART with DMA support | expand |
On 10/02/2023 08:26, Chia-Wei Wang wrote: > Add dt-bindings for Aspeed UART controller. Describe the hardware. What's the difference against existing Aspeed UART used everywhere? > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > --- > .../bindings/serial/aspeed,uart.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/serial/aspeed,uart.yaml Filename: aspeed,ast2600-uart.yaml (unless you are adding here more compatibles, but your const suggests that it's not going to happen) > > diff --git a/Documentation/devicetree/bindings/serial/aspeed,uart.yaml b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml > new file mode 100644 > index 000000000000..10c457d6a72e > --- /dev/null > +++ b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml > @@ -0,0 +1,81 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/serial/aspeed,uart.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Aspeed Universal Asynchronous Receiver/Transmitter This title matches other Aspeed UARTs, so aren't you duplicating bindings? > + > +maintainers: > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > + > +allOf: > + - $ref: serial.yaml# > + > +description: | > + The Aspeed UART is based on the basic 8250 UART and compatible > + with 16550A, with support for DMA > + > +properties: > + compatible: > + const: aspeed,ast2600-uart > + > + reg: > + description: The base address of the UART register bank Drop description > + maxItems: 1 > + > + clocks: > + description: The clock the baudrate is derived from > + maxItems: 1 > + > + interrupts: > + description: The IRQ number of the device Drop description > + maxItems: 1 > + > + dma-mode: > + type: boolean > + description: Enable DMA Drop property. DMA is enabled on presence of dmas. > + > + dma-channel: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The channel number to be used in the DMA engine That's not a correct DMA property. dmas and dma-names git grep dma -- Documentation/devicetree/bindings/ > + > + virtual: > + type: boolean > + description: Indicate virtual UART Virtual means not existing in real world? We do not describe in DTS non-existing devices. Drop entire property. > + > + sirq: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The serial IRQ number on LPC bus interface Drop entire property. > + > + sirq-polarity: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: The serial IRQ polarity on LPC bus interface Drop entire property. > + > + pinctrl-0: true > + > + pinctrl_names: > + const: default Drop both, you do no not need them. > + > +required: > + - compatible > + - reg > + - clocks > + - interrupts > + > +unevaluatedProperties: false > + Best regards, Krzysztof
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, February 10, 2023 5:13 PM > > On 10/02/2023 08:26, Chia-Wei Wang wrote: > > Add dt-bindings for Aspeed UART controller. > > Describe the hardware. What's the difference against existing Aspeed UART > used everywhere? The description will be revised to explain more for UART and Virtual UART controllers. > > > > > Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > --- > > .../bindings/serial/aspeed,uart.yaml | 81 > +++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/serial/aspeed,uart.yaml > > Filename: aspeed,ast2600-uart.yaml > (unless you are adding here more compatibles, but your const suggests that it's > not going to happen) > > > > > diff --git a/Documentation/devicetree/bindings/serial/aspeed,uart.yaml > > b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml > > new file mode 100644 > > index 000000000000..10c457d6a72e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml > > @@ -0,0 +1,81 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/serial/aspeed,uart.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Aspeed Universal Asynchronous Receiver/Transmitter > > This title matches other Aspeed UARTs, so aren't you duplicating bindings? > > > + > > +maintainers: > > + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> > > + > > +allOf: > > + - $ref: serial.yaml# > > + > > +description: | > > + The Aspeed UART is based on the basic 8250 UART and compatible > > + with 16550A, with support for DMA > > + > > +properties: > > + compatible: > > + const: aspeed,ast2600-uart > > + > > + reg: > > + description: The base address of the UART register bank > > Drop description Will revise as suggested. > > > + maxItems: 1 > > + > > + clocks: > > + description: The clock the baudrate is derived from > > + maxItems: 1 > > + > > + interrupts: > > + description: The IRQ number of the device > > Drop description Will revise as suggested. > > > + maxItems: 1 > > + > > + dma-mode: > > + type: boolean > > + description: Enable DMA > > Drop property. DMA is enabled on presence of dmas. > > > + > > + dma-channel: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: The channel number to be used in the DMA engine > > That's not a correct DMA property. dmas and dma-names git grep dma -- > Documentation/devicetree/bindings/ > > > > + > > + virtual: > > + type: boolean > > + description: Indicate virtual UART > > Virtual means not existing in real world? We do not describe in DTS > non-existing devices. Drop entire property. The virtual property indicates it is a Virtual UART device. VUART of Aspeed SoC is actually a FIFO exposed in the 16550A UART interface. The one head of the FIFO is exposed to Host via eSPI/LPC and the other one is for BMC. There is no physical serial link between Host and BMC. And thus named as Virtual UART. > > > + > > + sirq: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: The serial IRQ number on LPC bus interface > > Drop entire property. Mandatory for Virtual UART > > > + > > + sirq-polarity: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: The serial IRQ polarity on LPC bus interface > > Drop entire property. Mandatory for Virtual UART > > > + > > + pinctrl-0: true > > + > > + pinctrl_names: > > + const: default > > > Drop both, you do no not need them. Will revise as suggested > > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - interrupts > > + > > +unevaluatedProperties: false > > + Regards, Chiawei
On 13/02/2023 02:57, ChiaWei Wang wrote: >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Friday, February 10, 2023 5:13 PM >> >> On 10/02/2023 08:26, Chia-Wei Wang wrote: >>> Add dt-bindings for Aspeed UART controller. >> >> Describe the hardware. What's the difference against existing Aspeed UART >> used everywhere? > > The description will be revised to explain more for UART and Virtual UART controllers. > >> >>> >>> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> >>> --- >>> .../bindings/serial/aspeed,uart.yaml | 81 >> +++++++++++++++++++ >>> 1 file changed, 81 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/serial/aspeed,uart.yaml >> >> Filename: aspeed,ast2600-uart.yaml >> (unless you are adding here more compatibles, but your const suggests that it's >> not going to happen) >> >>> >>> diff --git a/Documentation/devicetree/bindings/serial/aspeed,uart.yaml >>> b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml >>> new file mode 100644 >>> index 000000000000..10c457d6a72e >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml >>> @@ -0,0 +1,81 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/serial/aspeed,uart.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Aspeed Universal Asynchronous Receiver/Transmitter >> >> This title matches other Aspeed UARTs, so aren't you duplicating bindings? >> >>> + >>> +maintainers: >>> + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> >>> + >>> +allOf: >>> + - $ref: serial.yaml# >>> + >>> +description: | >>> + The Aspeed UART is based on the basic 8250 UART and compatible >>> + with 16550A, with support for DMA >>> + >>> +properties: >>> + compatible: >>> + const: aspeed,ast2600-uart >>> + >>> + reg: >>> + description: The base address of the UART register bank >> >> Drop description > > Will revise as suggested. > >> >>> + maxItems: 1 >>> + >>> + clocks: >>> + description: The clock the baudrate is derived from >>> + maxItems: 1 >>> + >>> + interrupts: >>> + description: The IRQ number of the device >> >> Drop description > > Will revise as suggested. > >> >>> + maxItems: 1 >>> + >>> + dma-mode: >>> + type: boolean >>> + description: Enable DMA >> >> Drop property. DMA is enabled on presence of dmas. >> >>> + >>> + dma-channel: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: The channel number to be used in the DMA engine >> >> That's not a correct DMA property. dmas and dma-names git grep dma -- >> Documentation/devicetree/bindings/ >> >> >>> + >>> + virtual: >>> + type: boolean >>> + description: Indicate virtual UART >> >> Virtual means not existing in real world? We do not describe in DTS >> non-existing devices. Drop entire property. > > The virtual property indicates it is a Virtual UART device. vuart already has its bindings, so you do not need this in such case. > VUART of Aspeed SoC is actually a FIFO exposed in the 16550A UART interface. > The one head of the FIFO is exposed to Host via eSPI/LPC and the other one is for BMC. > There is no physical serial link between Host and BMC. And thus named as Virtual UART. I don't understand what is the virtual in terms of hardware. How you route your serial lines does not change the type of the device. You need to describe the hardware in Devicetree, not some concepts for system. > >> >>> + >>> + sirq: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: The serial IRQ number on LPC bus interface >> >> Drop entire property. > > Mandatory for Virtual UART It's not a explanation why this is a property suitable for Devicetree. IRQ numbers are given via interrupts field. > >> >>> + >>> + sirq-polarity: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: The serial IRQ polarity on LPC bus interface >> >> Drop entire property. > > Mandatory for Virtual UART > Same here, this is a flag for interrupts field. Best regards, Krzysztof
On 10/02/2023 10:12, Krzysztof Kozlowski wrote: > On 10/02/2023 08:26, Chia-Wei Wang wrote: >> Add dt-bindings for Aspeed UART controller. > > Describe the hardware. What's the difference against existing Aspeed > UART used everywhere? I expect the answer here. You are adding duplicated driver and bindings instead of merging with existing ones. That's common issue and explanation "it is different" is not enough. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/serial/aspeed,uart.yaml b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml new file mode 100644 index 000000000000..10c457d6a72e --- /dev/null +++ b/Documentation/devicetree/bindings/serial/aspeed,uart.yaml @@ -0,0 +1,81 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/serial/aspeed,uart.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Aspeed Universal Asynchronous Receiver/Transmitter + +maintainers: + - Chia-Wei Wang <chiawei_wang@aspeedtech.com> + +allOf: + - $ref: serial.yaml# + +description: | + The Aspeed UART is based on the basic 8250 UART and compatible + with 16550A, with support for DMA + +properties: + compatible: + const: aspeed,ast2600-uart + + reg: + description: The base address of the UART register bank + maxItems: 1 + + clocks: + description: The clock the baudrate is derived from + maxItems: 1 + + interrupts: + description: The IRQ number of the device + maxItems: 1 + + dma-mode: + type: boolean + description: Enable DMA + + dma-channel: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The channel number to be used in the DMA engine + + virtual: + type: boolean + description: Indicate virtual UART + + sirq: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The serial IRQ number on LPC bus interface + + sirq-polarity: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The serial IRQ polarity on LPC bus interface + + pinctrl-0: true + + pinctrl_names: + const: default + +required: + - compatible + - reg + - clocks + - interrupts + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + + serial@1e783000 { + compatible = "aspeed,ast2600-uart"; + reg = <0x1e783000 0x20>; + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_LOW>; + clocks = <&syscon ASPEED_CLK_GATE_UART1CLK>; + pinctrl-0 = <&pinctrl_txd1_default &pinctrl_rxd1_default>; + dma-mode; + dma-channel = <0>; + };
Add dt-bindings for Aspeed UART controller. Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com> --- .../bindings/serial/aspeed,uart.yaml | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 Documentation/devicetree/bindings/serial/aspeed,uart.yaml