diff mbox series

[v2,tty-next,1/2] dt-bindings: serial: ni,ni16650: add bindings

Message ID 20230410211152.94332-2-brenda.streiff@ni.com
State New
Headers show
Series [v2,tty-next,1/2] dt-bindings: serial: ni,ni16650: add bindings | expand

Commit Message

Brenda Streiff April 10, 2023, 9:11 p.m. UTC
Add bindings for the NI 16550 UART.

Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
Cc: Gratian Crisan <gratian.crisan@ni.com>
Cc: Jason Smith <jason.smith@ni.com>
---
 .../bindings/serial/ni,ni16550.yaml           | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/ni,ni16550.yaml

Comments

Krzysztof Kozlowski April 11, 2023, 5:44 a.m. UTC | #1
On 10/04/2023 23:11, Brenda Streiff wrote:
> Add bindings for the NI 16550 UART.
> 
> Signed-off-by: Brenda Streiff <brenda.streiff@ni.com>
> Cc: Gratian Crisan <gratian.crisan@ni.com>
> Cc: Jason Smith <jason.smith@ni.com>

Thank you for your patch. There is something to discuss/improve.

> ---
>  .../bindings/serial/ni,ni16550.yaml           | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/ni,ni16550.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serial/ni,ni16550.yaml b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml
> new file mode 100644
> index 000000000000..13928e89f5aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/ni,ni16550.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NI 16550 asynchronous serial interface (UART)
> +
> +maintainers:
> +  - Brenda Streiff <brenda.streiff@ni.com>
> +
> +allOf:
> +  - $ref: serial.yaml#
> +
> +properties:
> +  compatible:
> +    const: ni,ni16550
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency: true

I missed it last time - why do you need this property? You do not have
any clock input, so which clock's frequency is it?

Best regards,
Krzysztof
Brenda Streiff April 12, 2023, 10:24 p.m. UTC | #2
On 4/11/23 00:44, Krzysztof Kozlowski wrote:
> On 10/04/2023 23:11, Brenda Streiff wrote:
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clock-frequency: true
> 
> I missed it last time - why do you need this property? You do not have
> any clock input, so which clock's frequency is it?
> 

This is the clock frequency of the UART; on our x86-based platforms this
comes from the LPC clock, on Zynq-7000 it's derived from a clock in the
FPGA. This is used to set struct uart_port::uartclk in the serial core,
as it is for other UARTs.

This clock frequency can vary based on board design (especially on the
x86 side, due to different LPC clocks) but for a given design is fixed-
frequency.

Would you prefer this be documented further? I was following 8250.yaml's
lead here with the simple `true`.
Krzysztof Kozlowski April 13, 2023, 7:39 a.m. UTC | #3
On 13/04/2023 00:24, Brenda Streiff wrote:
> On 4/11/23 00:44, Krzysztof Kozlowski wrote:
>> On 10/04/2023 23:11, Brenda Streiff wrote:
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency: true
>>
>> I missed it last time - why do you need this property? You do not have
>> any clock input, so which clock's frequency is it?
>>
> 
> This is the clock frequency of the UART; on our x86-based platforms this
> comes from the LPC clock, on Zynq-7000 it's derived from a clock in the
> FPGA. This is used to set struct uart_port::uartclk in the serial core,
> as it is for other UARTs.
> 
> This clock frequency can vary based on board design (especially on the
> x86 side, due to different LPC clocks) but for a given design is fixed-
> frequency.

So you must have clock input - clocks property. Once you add this, use
assigned-clocks to get the rate you want.

> 
> Would you prefer this be documented further? I was following 8250.yaml's
> lead here with the simple `true`.

I prefer to drop it. It is not correct and a legacy property. Without
clock inputs how can you even configure some clock?

Best regards,
Krzysztof
Brenda Streiff April 13, 2023, 8:44 p.m. UTC | #4
On 4/13/23 02:39, Krzysztof Kozlowski wrote:
> On 13/04/2023 00:24, Brenda Streiff wrote:
>> On 4/11/23 00:44, Krzysztof Kozlowski wrote:
>>> On 10/04/2023 23:11, Brenda Streiff wrote:
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  clock-frequency: true
>>>
>>> I missed it last time - why do you need this property? You do not have
>>> any clock input, so which clock's frequency is it?
>>>
>>
>> This is the clock frequency of the UART; on our x86-based platforms this
>> comes from the LPC clock, on Zynq-7000 it's derived from a clock in the
>> FPGA. This is used to set struct uart_port::uartclk in the serial core,
>> as it is for other UARTs.
>>
>> This clock frequency can vary based on board design (especially on the
>> x86 side, due to different LPC clocks) but for a given design is fixed-
>> frequency.
> 
> So you must have clock input - clocks property. Once you add this, use
> assigned-clocks to get the rate you want.
> 
>>
>> Would you prefer this be documented further? I was following 8250.yaml's
>> lead here with the simple `true`.
> 
> I prefer to drop it. It is not correct and a legacy property. Without
> clock inputs how can you even configure some clock?

Configure in what respect? Software can't change this clock; it's
effectively a fixed oscillator.

In practice, this would always be pointing at a compatible="fixed-clock"
which declares a clock-frequency; this seems like "clock-frequency but
more steps". I can add that, but I'm not clear on what value that adds.

We also have shipping devices with ACPI tables using "clock-frequency",
so independent of support for "clocks" and "assigned-clocks" for
devicetree-using systems, I would still have to keep support in the
driver for a "clock-frequency" device property for ACPI-using systems.

(Is there documentation on a standalone clock-property being a legacy
property that I've missed? I don't see anything of the sort in
writing-bindings.rst or in dt-schema and I want to make sure that I
haven't missed the proper guidance here.)
Krzysztof Kozlowski April 14, 2023, 7:42 a.m. UTC | #5
On 13/04/2023 22:44, Brenda Streiff wrote:
> 
> 
> On 4/13/23 02:39, Krzysztof Kozlowski wrote:
>> On 13/04/2023 00:24, Brenda Streiff wrote:
>>> On 4/11/23 00:44, Krzysztof Kozlowski wrote:
>>>> On 10/04/2023 23:11, Brenda Streiff wrote:
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clock-frequency: true
>>>>
>>>> I missed it last time - why do you need this property? You do not have
>>>> any clock input, so which clock's frequency is it?
>>>>
>>>
>>> This is the clock frequency of the UART; on our x86-based platforms this
>>> comes from the LPC clock, on Zynq-7000 it's derived from a clock in the
>>> FPGA. This is used to set struct uart_port::uartclk in the serial core,
>>> as it is for other UARTs.
>>>
>>> This clock frequency can vary based on board design (especially on the
>>> x86 side, due to different LPC clocks) but for a given design is fixed-
>>> frequency.
>>
>> So you must have clock input - clocks property. Once you add this, use
>> assigned-clocks to get the rate you want.
>>
>>>
>>> Would you prefer this be documented further? I was following 8250.yaml's
>>> lead here with the simple `true`.
>>
>> I prefer to drop it. It is not correct and a legacy property. Without
>> clock inputs how can you even configure some clock?
> 
> Configure in what respect? Software can't change this clock; it's
> effectively a fixed oscillator.

So where is the clock located? Physically.

> 
> In practice, this would always be pointing at a compatible="fixed-clock"
> which declares a clock-frequency; this seems like "clock-frequency but
> more steps". I can add that, but I'm not clear on what value that adds.

Aren't we talking about two different things? Based on limited
informamtion, I claimed you have clock input. If you have clock input,
then you miss clocks property.

What value would that add? I don't know... the rules that bindings
describe hardware?

> 
> We also have shipping devices with ACPI tables using "clock-frequency",
> so independent of support for "clocks" and "assigned-clocks" for
> devicetree-using systems, I would still have to keep support in the
> driver for a "clock-frequency" device property for ACPI-using systems.

I don't care about ACPI and ACPI has nothing to do with Bindings. We do
not write bindings for ACPI.

What your driver has or has not to do, it's also separate question. I2C
camera sensors solved it long time ago. I don't see why this must be
special.

> 
> (Is there documentation on a standalone clock-property being a legacy
> property that I've missed? 
> I don't see anything of the sort in
> writing-bindings.rst or in dt-schema and I want to make sure that I

dtschema:
  # Legacy clock properties
  clock-frequency:
    description: Legacy property ...

> haven't missed the proper guidance here.)

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/ni,ni16550.yaml b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml
new file mode 100644
index 000000000000..13928e89f5aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/ni,ni16550.yaml
@@ -0,0 +1,51 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/ni,ni16550.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NI 16550 asynchronous serial interface (UART)
+
+maintainers:
+  - Brenda Streiff <brenda.streiff@ni.com>
+
+allOf:
+  - $ref: serial.yaml#
+
+properties:
+  compatible:
+    const: ni,ni16550
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clock-frequency: true
+
+  ni,serial-port-mode:
+    description: Indicates whether this is an RS-232 or RS-485 serial port.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ RS-232, RS-485 ]
+    default: RS-485
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clock-frequency
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    serial@80000000 {
+      compatible = "ni,ni16550";
+      reg = <0x80000000 0x8>;
+      interrupts = <0 30 4>;
+      clock-frequency = <58824000>;
+      ni,serial-port-mode = "RS-232";
+    };
+
+...