diff mbox series

[v4,3/6] dt-bindings: mvebu-uart: document DT bindings for marvell, armada-3700-uart-clock

Message ID 20210802144529.1520-4-pali@kernel.org (mailing list archive)
State New, archived
Headers show
Series serial: mvebu-uart: Support for higher baudrates | expand

Commit Message

Pali Rohár Aug. 2, 2021, 2:45 p.m. UTC 
This change adds DT bindings documentation for device nodes with compatible
string "marvell,armada-3700-uart-clock".

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 .../bindings/clock/armada3700-uart-clock.yaml | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml

Comments

Stephen Boyd Aug. 6, 2021, 12:30 a.m. UTC | #1 
> diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> new file mode 100644
> index 000000000000..5ef04f3affda
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +title: Marvell Armada 3720 UART clocks

Please add a newline here

> +properties:
> +  compatible:
> +    const: marvell,armada-3700-uart-clock

Please add a newline here

> +  reg:
> +    items:
> +      - description: UART Clock Control Register
> +      - description: UART 2 Baud Rate Divisor Register

Please add a newline here

> +  clocks:
> +    description: |
> +      List of parent clocks suitable for UART from following set:
> +        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
> +      UART clock can use one from this set and when more are provided
> +      then kernel would choose and configure the most suitable one.
> +      It is suggest to specify at least one TBG clock to achieve
> +      baudrates above 230400 and also to specify clock which bootloader
> +      used for UART (most probably xtal) for smooth boot log on UART.

Please use items and const like clock-names for the clocks property. The
description makes me feel like the DT is configuring the choices
available. Ideally, the clocks and clock-names properties are fixed in
length and never change unless the compatible changes.

Please add a newline here

> +  clock-names:
> +    items:
> +      - const: TBG-A-P
> +      - const: TBG-B-P
> +      - const: TBG-A-S
> +      - const: TBG-B-S
> +      - const: xtal
> +    minItems: 1
> +    maxItems: 5

Please add a newline here

> +  '#clock-cells':
> +    const: 1

Please add a newline here

> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'

Please add a newline here

> +additionalProperties: false

Please add a newline here

> +examples:
> +  - |
> +    uartclk: uartclk@12000 {
> +      compatible = "marvell,armada-3700-uart-clock";
> +      reg = <0x12010 0x4>, <0x12210 0x4>;
> +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
> +      #clock-cells = <1>;
> +    };
Pali Rohár Aug. 6, 2021, 8:28 a.m. UTC | #2 
On Thursday 05 August 2021 17:30:19 Stephen Boyd wrote:
> > diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > new file mode 100644
> > index 000000000000..5ef04f3affda
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +title: Marvell Armada 3720 UART clocks
> 
> Please add a newline here
> 
> > +properties:
> > +  compatible:
> > +    const: marvell,armada-3700-uart-clock
> 
> Please add a newline here
> 
> > +  reg:
> > +    items:
> > +      - description: UART Clock Control Register
> > +      - description: UART 2 Baud Rate Divisor Register
> 
> Please add a newline here
> 
> > +  clocks:
> > +    description: |
> > +      List of parent clocks suitable for UART from following set:
> > +        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
> > +      UART clock can use one from this set and when more are provided
> > +      then kernel would choose and configure the most suitable one.
> > +      It is suggest to specify at least one TBG clock to achieve
> > +      baudrates above 230400 and also to specify clock which bootloader
> > +      used for UART (most probably xtal) for smooth boot log on UART.
> 
> Please use items and const like clock-names for the clocks property.

It is already there, see below.

> The description makes me feel like the DT is configuring the choices
> available.

See description. It is kernel (driver) who is choosing one clock from
the set and then configure it a UART clock.

> Ideally, the clocks and clock-names properties are fixed in
> length and never change unless the compatible changes.
> 
> Please add a newline here
> 
> > +  clock-names:
> > +    items:
> > +      - const: TBG-A-P
> > +      - const: TBG-B-P
> > +      - const: TBG-A-S
> > +      - const: TBG-B-S
> > +      - const: xtal
> > +    minItems: 1
> > +    maxItems: 5
> 
> Please add a newline here
> 
> > +  '#clock-cells':
> > +    const: 1
> 
> Please add a newline here
> 
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> 
> Please add a newline here
> 
> > +additionalProperties: false
> 
> Please add a newline here
> 
> > +examples:
> > +  - |
> > +    uartclk: uartclk@12000 {
> > +      compatible = "marvell,armada-3700-uart-clock";
> > +      reg = <0x12010 0x4>, <0x12210 0x4>;
> > +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> > +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
> > +      #clock-cells = <1>;
> > +    };
Stephen Boyd Aug. 12, 2021, 7:33 p.m. UTC | #3 
Quoting Pali (2021-08-06 01:28:18)
> On Thursday 05 August 2021 17:30:19 Stephen Boyd wrote:
> > > diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > > new file mode 100644
> > > index 000000000000..5ef04f3affda
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > > @@ -0,0 +1,49 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +title: Marvell Armada 3720 UART clocks
> > 
> > Please add a newline here
> > 
> > > +properties:
> > > +  compatible:
> > > +    const: marvell,armada-3700-uart-clock
> > 
> > Please add a newline here
> > 
> > > +  reg:
> > > +    items:
> > > +      - description: UART Clock Control Register
> > > +      - description: UART 2 Baud Rate Divisor Register
> > 
> > Please add a newline here
> > 
> > > +  clocks:
> > > +    description: |
> > > +      List of parent clocks suitable for UART from following set:
> > > +        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
> > > +      UART clock can use one from this set and when more are provided
> > > +      then kernel would choose and configure the most suitable one.
> > > +      It is suggest to specify at least one TBG clock to achieve
> > > +      baudrates above 230400 and also to specify clock which bootloader
> > > +      used for UART (most probably xtal) for smooth boot log on UART.
> > 
> > Please use items and const like clock-names for the clocks property.
> 
> It is already there, see below.
> 
> > The description makes me feel like the DT is configuring the choices
> > available.
> 
> See description. It is kernel (driver) who is choosing one clock from
> the set and then configure it a UART clock.

Can that be done with assigned-clock-parents?

> 
> > Ideally, the clocks and clock-names properties are fixed in
> > length and never change unless the compatible changes.
> > 
> > Please add a newline here
> > 
> > > +  clock-names:
> > > +    items:
> > > +      - const: TBG-A-P
> > > +      - const: TBG-B-P
> > > +      - const: TBG-A-S
> > > +      - const: TBG-B-S
> > > +      - const: xtal
> > > +    minItems: 1
> > > +    maxItems: 5
> > 
> > Please add a newline here
> > 
> > > +  '#clock-cells':
> > > +    const: 1
> > 
> > Please add a newline here
> > 
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - '#clock-cells'
> > 
> > Please add a newline here
> > 
> > > +additionalProperties: false
> > 
> > Please add a newline here
> > 
> > > +examples:
> > > +  - |
> > > +    uartclk: uartclk@12000 {
> > > +      compatible = "marvell,armada-3700-uart-clock";
> > > +      reg = <0x12010 0x4>, <0x12210 0x4>;

The reg property looks like this is poking a mux that is part of a
larger device. What device is this actually part of? The uart hardware?

> > > +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> > > +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
Pali Rohár Aug. 12, 2021, 8:08 p.m. UTC | #4 
On Thursday 12 August 2021 12:33:19 Stephen Boyd wrote:
> Quoting Pali (2021-08-06 01:28:18)
> > On Thursday 05 August 2021 17:30:19 Stephen Boyd wrote:
> > > > diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > > > new file mode 100644
> > > > index 000000000000..5ef04f3affda
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > > > @@ -0,0 +1,49 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +title: Marvell Armada 3720 UART clocks
> > > 
> > > Please add a newline here
> > > 
> > > > +properties:
> > > > +  compatible:
> > > > +    const: marvell,armada-3700-uart-clock
> > > 
> > > Please add a newline here
> > > 
> > > > +  reg:
> > > > +    items:
> > > > +      - description: UART Clock Control Register
> > > > +      - description: UART 2 Baud Rate Divisor Register
> > > 
> > > Please add a newline here
> > > 
> > > > +  clocks:
> > > > +    description: |
> > > > +      List of parent clocks suitable for UART from following set:
> > > > +        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
> > > > +      UART clock can use one from this set and when more are provided
> > > > +      then kernel would choose and configure the most suitable one.
> > > > +      It is suggest to specify at least one TBG clock to achieve
> > > > +      baudrates above 230400 and also to specify clock which bootloader
> > > > +      used for UART (most probably xtal) for smooth boot log on UART.
> > > 
> > > Please use items and const like clock-names for the clocks property.
> > 
> > It is already there, see below.
> > 
> > > The description makes me feel like the DT is configuring the choices
> > > available.
> > 
> > See description. It is kernel (driver) who is choosing one clock from
> > the set and then configure it a UART clock.
> 
> Can that be done with assigned-clock-parents?

I'm not sure if it is possible. Frequencies of "TBG-A-P", "TBG-B-P",
"TBG-A-S" and "TBG-B-S" clocks are set by firmware and frequency of
"xtal" by strapping pin. So they are not static constants. And also
kernel cannot change frequency of these clocks.

Driver reads frequencies of these clocks and choose the one which is the
most suitable for UART at runtime.

> > > Ideally, the clocks and clock-names properties are fixed in
> > > length and never change unless the compatible changes.
> > > 
> > > Please add a newline here
> > > 
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: TBG-A-P
> > > > +      - const: TBG-B-P
> > > > +      - const: TBG-A-S
> > > > +      - const: TBG-B-S
> > > > +      - const: xtal
> > > > +    minItems: 1
> > > > +    maxItems: 5
> > > 
> > > Please add a newline here
> > > 
> > > > +  '#clock-cells':
> > > > +    const: 1
> > > 
> > > Please add a newline here
> > > 
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - '#clock-cells'
> > > 
> > > Please add a newline here
> > > 
> > > > +additionalProperties: false
> > > 
> > > Please add a newline here
> > > 
> > > > +examples:
> > > > +  - |
> > > > +    uartclk: uartclk@12000 {
> > > > +      compatible = "marvell,armada-3700-uart-clock";
> > > > +      reg = <0x12010 0x4>, <0x12210 0x4>;
> 
> The reg property looks like this is poking a mux that is part of a
> larger device. What device is this actually part of? The uart hardware?

Yes, UART hardware.

> > > > +      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
> > > > +      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
new file mode 100644
index 000000000000..5ef04f3affda
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
@@ -0,0 +1,49 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/marvell,armada-3700-uart-clock#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Marvell Armada 3720 UART clocks
+properties:
+  compatible:
+    const: marvell,armada-3700-uart-clock
+  reg:
+    items:
+      - description: UART Clock Control Register
+      - description: UART 2 Baud Rate Divisor Register
+  clocks:
+    description: |
+      List of parent clocks suitable for UART from following set:
+        "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal"
+      UART clock can use one from this set and when more are provided
+      then kernel would choose and configure the most suitable one.
+      It is suggest to specify at least one TBG clock to achieve
+      baudrates above 230400 and also to specify clock which bootloader
+      used for UART (most probably xtal) for smooth boot log on UART.
+  clock-names:
+    items:
+      - const: TBG-A-P
+      - const: TBG-B-P
+      - const: TBG-A-S
+      - const: TBG-B-S
+      - const: xtal
+    minItems: 1
+    maxItems: 5
+  '#clock-cells':
+    const: 1
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+additionalProperties: false
+examples:
+  - |
+    uartclk: uartclk@12000 {
+      compatible = "marvell,armada-3700-uart-clock";
+      reg = <0x12010 0x4>, <0x12210 0x4>;
+      clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>, <&tbg 3>, <&xtalclk>;
+      clock-names = "TBG-A-P", "TBG-B-P", "TBG-A-S", "TBG-B-S", "xtal";
+      #clock-cells = <1>;
+    };