diff mbox series

[1/4] dt-bindings: aspeed: Add UART controller

Message ID 20230210072643.2772-2-chiawei_wang@aspeedtech.com
State New
Headers
Series arm: aspeed: Add UART with DMA support |

Commit Message

ChiaWei Wang Feb. 10, 2023, 7:26 a.m. UTC 
  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

Comments

Krzysztof Kozlowski Feb. 10, 2023, 9:12 a.m. UTC | #1 
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
ChiaWei Wang Feb. 13, 2023, 1:57 a.m. UTC | #2 
> 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
Krzysztof Kozlowski Feb. 13, 2023, 8:26 a.m. UTC | #3 
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
Krzysztof Kozlowski Feb. 13, 2023, 8:33 a.m. UTC | #4 
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 mbox series

Patch

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