Message-ID: <CAMhs-H8OQ9gJLsifLuHD2GN8rYwnY=Zmdb0kMEfX4UUHhjMUyQ@mail.gmail.com>
Date:   Mon, 20 Mar 2023 18:24:56 +0100
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     linux-clk@...r.kernel.org, linux-mips@...r.kernel.org,
        tsbogend@...ha.franken.de, john@...ozen.org,
        linux-kernel@...r.kernel.org, p.zabel@...gutronix.de,
        mturquette@...libre.com, sboyd@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        devicetree@...r.kernel.org, arinc.unal@...nc9.com
Subject: Re: [PATCH 01/10] dt: bindings: clock: add mtmips SoCs clock device
 tree binding documentation

Hi Krzysztof,

On Mon, Mar 20, 2023 at 5:36 PM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 20/03/2023 17:18, Sergio Paracuellos wrote:
> > Adds device tree binding documentation for clocks and resets in the
> > Mediatek MIPS and Ralink SOCs. This covers RT2880, RT3050, RT3052, RT3350,
> > RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs.
>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>
> Subject: drop second/last, redundant "device tree binding
> documentation". The "dt-bindings" prefix is already stating that these
> are bindings.

Sure, will do. Sorry for the inconvenience.

> (BTW, that's the longest redundant component I ever saw)

I thought it was better to just list compatible strings inside one
single file than adding the same binding in multiple files.

>
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
> > ---
> >  .../bindings/clock/mtmips-clock.yaml          | 68 +++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/mtmips-clock.yaml b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml
> > new file mode 100644
> > index 000000000000..c92969ce231d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml
>
> Filename matching compatible, so vendor prefix and device name (or
> family of names).

I used mtmips here but list compatibles starting with ralink. As I
have said in the cover letter I am inspired by the last merged pinctrl
series for these SoCs.
See:
- https://lore.kernel.org/linux-gpio/e9e6ad87-2db5-9767-ff39-64a302b06185@arinc9.com/T/#t

Not all of compatible currently exist. All of these are at the end the
way we can properly match compatible-data to write a proper driver.
The current ralink dtsi files which are in tree now
are totally incomplete and not documented so we are planning to align
all of this with openWRT used files and others soon. That's the reason
we are not touching
'arch/mips/boot/dts' at all now. I don't think anybody is using any of
this but mt7621 which is properly completed and documented.

>
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/mtmips-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MTMIPS SoCs Clock
>
> One clock? Are you sure these describe exactly one clock?

I will change this to 'Clocks'.

>
> > +
> > +maintainers:
> > +  - Sergio Paracuellos <sergio.paracuellos@...il.com>
> > +
> > +description: |
> > +  MediaTek MIPS and Ralink SoCs have an XTAL from where the cpu clock is
> > +  provided as well as derived clocks for the bus and the peripherals.
> > +
> > +  Each clock is assigned an identifier and client nodes use this identifier
> > +  to specify the clock which they consume.
>
> Drop useless or obvious pieces of description. Describe the hardware.
>
> > +
> > +  The clocks are provided inside a system controller node.

>
> ???

I meant, this node is a syscon from where both clock and reset related
registers are used. I think writing in this way was enough since it
has a pretty similar description like the one in
'mediatek,mt7621-sysc.yaml'.

>
> > +
> > +  This node is also a reset provider for all the peripherals.
>
> ??? Does it mean it is not only "Clock" but also reset controller?

Yes, this node is a clock and reset controller for all the SoC.

>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - ralink,rt2880-sysc
> > +          - ralink,rt3050-sysc
> > +          - ralink,rt3052-sysc
> > +          - ralink,rt3352-sysc
> > +          - ralink,rt3883-sysc
> > +          - ralink,rt5350-sysc
> > +          - ralink,mt7620-sysc
> > +          - ralink,mt7620a-sysc
> > +          - ralink,mt7628-sysc'
> > +          - ralink,mt7688-sysc
> > +          - ralink,rt2880-reset
>
> That's odd. rt2880 is sysc and reset? One device with two compatibles?

This 'ralink,rt2880-reset' is for compatibility reasons. Reset related
code was inside 'arch/mips/ralink' folder reset.c file but it is moved
to this new driver, so we have maintained this reset stuff for the
reset compatibility. All of the rest are the new possible stuff for
both reset and clocks. Clock driver is instantiated in two phases. The
earlier one set up the clocks via CLK_OF_DECLARE macro. Resets are set
up as a platform driver. Is only inside this where
'ralink,rt2880-reset' is used. See patch 2 of the series for details.

>
> Also, order these by name.

All are ordered but I maintained the  'ralink,rt2880-reset' at the end.

>
>
> > +      - const: syscon
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#clock-cells':
> > +    description:
> > +      The first cell indicates the clock number.
> > +    const: 1
> > +
> > +  '#reset-cells':
> > +    description:
> > +      The first cell indicates the reset bit within the register.
> > +    const: 1
>
> Wait, only rt2880-reset is reset controller? This is confusing.

No, that is the reset compatibility one. All the rest are both clock
and reset controllers from now on.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#clock-cells'
> > +  - '#reset-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    sysc: sysc@0 {
>
> Drop label.

Sure, thanks.

>
> Node names should be generic, clock-controller or reset-controller or
> system-controller sometimes.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> > +      compatible = "ralink,rt5350-sysc", "syscon";
> > +      reg = <0x0 0x100>;
> > +      #clock-cells = <1>;
> > +      #reset-cells = <1>;
> > +    };

Ok, so I will set this as 'syscon@' if you are ok with it.

>
> Best regards,
> Krzysztof
>

Thanks to you for the review.

Best regards,
    Sergio Paracuellos

Powered by blists - more mailing lists