| 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 |
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
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`.
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
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.)
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 --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"; + }; + +...
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