Re: [RESEND PATCH v5 3/6] dt-bindings: mvebu-uart: document DT bindings for marvell,armada-3700-uart-clock

[Pali Rohár]

On Monday 27 September 2021 15:17:59 Rob Herring wrote:
> On Wed, Sep 22, 2021 at 5:56 AM Pali Rohár <pali@xxxxxxxxxx> wrote:
> >
> > This change adds DT bindings documentation for device nodes with compatible
> > string "marvell,armada-3700-uart-clock".
> 
> Please resend to the DT list so that checks run and this gets reviewed
> in a timely manner.

OK

> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > ---
> >  .../bindings/clock/armada3700-uart-clock.yaml | 57 +++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> >
> > 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..5bdb23e0ba3e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/armada3700-uart-clock.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license. checkpatch will tell you which ones.

OK

> > +%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
> 
> Don't need maxItems equal to length of 'items'.

OK

> > +
> > +  '#clock-cells':
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    uartclk: uartclk@12000 {
> 
> clock-controller@12010
> 
> > +      compatible = "marvell,armada-3700-uart-clock";
> > +      reg = <0x12010 0x4>, <0x12210 0x4>;
> 
> However, looks like this is part of some other block.

Yes, it is part of UART block.

Explanation is in commit message of patch 2/6.

And also discussed here:
https://lore.kernel.org/linux-serial/20210812200804.i4kbcs6ut27mapd3@pali/

> The whole block
> needs a binding (or at least the parent and whatever sub-functions you
> know about).

Whole UART block has already binding. Clock driver just needs access to
these clock bits of these two registers which are in UART block. HW
designers decided that clock which drives UART2 has configuration in
UART1 address space. As explained in commit message of patch 2/6 there
is no easy way how to deal with it in DTS backward compatible way. So
clock and UART driver shares mutex for accessing these two shared
registers, and these two registers are defined in all 3 DT nodes: UART1,
UART2 and UART-clock.

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