diff mbox series

[v9,2/6] dt: bindings: add mt7621-clk device tree binding documentation

Message ID 20210218070709.11932-3-sergio.paracuellos@gmail.com (mailing list archive)
State Superseded
Headers show
Series MIPS: ralink: add CPU clock detection and clock driver for MT7621 | expand

Commit Message

Sergio Paracuellos Feb. 18, 2021, 7:07 a.m. UTC 
Adds device tree binding documentation for clocks in the
MT7621 SOC.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../bindings/clock/mediatek,mt7621-clk.yaml   | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml

Comments

Rob Herring March 5, 2021, 10:47 p.m. UTC | #1 
On Thu, Feb 18, 2021 at 08:07:05AM +0100, Sergio Paracuellos wrote:
> Adds device tree binding documentation for clocks in the
> MT7621 SOC.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  .../bindings/clock/mediatek,mt7621-clk.yaml   | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> new file mode 100644
> index 000000000000..842a0f2c9d40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/mediatek,mt7621-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MT7621 Clock Device Tree Bindings
> +
> +maintainers:
> +  - Sergio Paracuellos <sergio.paracuellos@gmail.com>
> +
> +description: |
> +  The MT7621 has a PLL controller from where the cpu clock is provided
> +  as well as derived clocks for the bus and the peripherals. It also
> +  can gate SoC device clocks.
> +
> +  Each clock is assigned an identifier and client nodes use this identifier
> +  to specify the clock which they consume.
> +
> +  All these identifiers could be found in:
> +  [1]: <include/dt-bindings/clock/mt7621-clk.h>.
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt7621-clk
> +
> +  "#clock-cells":
> +    description:
> +      The first cell indicates the clock number, see [1] for available
> +      clocks.
> +    const: 1
> +
> +  ralink,sysctl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle of syscon used to control system registers
> +
> +  ralink,memctl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle of syscon used to control memory registers

I assume one of these phandles are the main registers for the clocks? 
Make this a child node and drop that phandle.

> +
> +  clock-output-names:
> +    maxItems: 8
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +  - ralink,sysctl
> +  - ralink,memctl
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt7621-clk.h>
> +
> +    pll {
> +      compatible = "mediatek,mt7621-clk";
> +      #clock-cells = <1>;
> +      ralink,sysctl = <&sysc>;
> +      ralink,memctl = <&memc>;
> +      clock-output-names = "xtal", "cpu", "bus",
> +                           "50m", "125m", "150m",
> +                           "250m", "270m";
> +    };
> -- 
> 2.25.1
>
Chuanhong Guo March 6, 2021, 1:52 a.m. UTC | #2 
Hi Rob!

On Sat, Mar 6, 2021 at 6:48 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Feb 18, 2021 at 08:07:05AM +0100, Sergio Paracuellos wrote:
> > Adds device tree binding documentation for clocks in the
> > MT7621 SOC.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  .../bindings/clock/mediatek,mt7621-clk.yaml   | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> > new file mode 100644
> > index 000000000000..842a0f2c9d40
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/mediatek,mt7621-clk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MT7621 Clock Device Tree Bindings
> > +
> > +maintainers:
> > +  - Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > +
> > +description: |
> > +  The MT7621 has a PLL controller from where the cpu clock is provided
> > +  as well as derived clocks for the bus and the peripherals. It also
> > +  can gate SoC device clocks.
> > +
> > +  Each clock is assigned an identifier and client nodes use this identifier
> > +  to specify the clock which they consume.
> > +
> > +  All these identifiers could be found in:
> > +  [1]: <include/dt-bindings/clock/mt7621-clk.h>.
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt7621-clk
> > +
> > +  "#clock-cells":
> > +    description:
> > +      The first cell indicates the clock number, see [1] for available
> > +      clocks.
> > +    const: 1
> > +
> > +  ralink,sysctl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle of syscon used to control system registers
> > +
> > +  ralink,memctl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle of syscon used to control memory registers
>
> I assume one of these phandles are the main registers for the clocks?
> Make this a child node and drop that phandle.

On MT7621, CPU clock can be chosen from 3 sources: crystal clock,
a fixed 500MHz clock or a clock created by the memory controller.
sysctl contains a bootstrap register to determine crystal clock, a
clock mux for choosing between the 3 sources for CPU clock, and
a clock gate register for various peripherals. The ralink,memctl
phandle here is to read the cpu clock frequency from the memory
controller.
The original implementation hides this hardware detail to avoid
splitting the driver into three just for the CPU clock.
Is this approach okay and we can put it under sysctl node,
or this driver needs to be further splitted?
Sergio Paracuellos March 6, 2021, 7:12 a.m. UTC | #3 
Hi Rob,

On Fri, Mar 5, 2021 at 11:47 PM Rob Herring <robh@kernel.org> wrote:
[snip]
> > +
> > +  ralink,sysctl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle of syscon used to control system registers
> > +
> > +  ralink,memctl:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle of syscon used to control memory registers
>
> I assume one of these phandles are the main registers for the clocks?
> Make this a child node and drop that phandle.

The 'ralink,sysctl' phandle is to read bootstrap register to be able
to derive xtal and a clk gate register for the peripherals.
The 'ralink,memctl' phandle is to read the cpu clock frequency from
the memory controller.

So there is not "main registers". I already put this as a child node
in v4 and I was told to get rid of child nodes. I need this as a
regmap to other DT node registers (sysctl, and memctl) to be able to
use the driver without specific architecture operations and properly
enable for COMPILE_TEST without dirty Makefile arch flags. Both sysctl
and memctl has no other child nodes, and I think that's why I was told
to avoid child nodes at the end. I explained here [0] current sysctl
and memctl in the mt7621 device tree and my view of the need for this
two syscons:

[0]: https://lkml.org/lkml/2021/1/2/9

So to avoid to send again "a previous version" on this patch, please
guide me in the correct thing to do. Stephen, Rob, I will be really
happy with your help :)

Best regards,
    Sergio Paracuellos
>
> > +
> > +  clock-output-names:
> > +    maxItems: 8
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +  - ralink,sysctl
> > +  - ralink,memctl
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/mt7621-clk.h>
> > +
> > +    pll {
> > +      compatible = "mediatek,mt7621-clk";
> > +      #clock-cells = <1>;
> > +      ralink,sysctl = <&sysc>;
> > +      ralink,memctl = <&memc>;
> > +      clock-output-names = "xtal", "cpu", "bus",
> > +                           "50m", "125m", "150m",
> > +                           "250m", "270m";
> > +    };
> > --
> > 2.25.1
> >
Sergio Paracuellos March 6, 2021, 9:54 a.m. UTC | #4 
Hi again,

On Sat, Mar 6, 2021 at 8:12 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Rob,
>
> On Fri, Mar 5, 2021 at 11:47 PM Rob Herring <robh@kernel.org> wrote:
> [snip]
> > > +
> > > +  ralink,sysctl:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      phandle of syscon used to control system registers
> > > +
> > > +  ralink,memctl:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      phandle of syscon used to control memory registers
> >
> > I assume one of these phandles are the main registers for the clocks?
> > Make this a child node and drop that phandle.
>
> The 'ralink,sysctl' phandle is to read bootstrap register to be able
> to derive xtal and a clk gate register for the peripherals.
> The 'ralink,memctl' phandle is to read the cpu clock frequency from
> the memory controller.
>
> So there is not "main registers". I already put this as a child node
> in v4 and I was told to get rid of child nodes. I need this as a
> regmap to other DT node registers (sysctl, and memctl) to be able to
> use the driver without specific architecture operations and properly
> enable for COMPILE_TEST without dirty Makefile arch flags. Both sysctl
> and memctl has no other child nodes, and I think that's why I was told
> to avoid child nodes at the end. I explained here [0] current sysctl
> and memctl in the mt7621 device tree and my view of the need for this
> two syscons:
>
> [0]: https://lkml.org/lkml/2021/1/2/9
>
> So to avoid to send again "a previous version" on this patch, please
> guide me in the correct thing to do. Stephen, Rob, I will be really
> happy with your help :)

Since there are no other child nodes for this sysc, should merge clock
properties
with this node in the following way a valid approach:

 sysc: sysc@0 {
     compatible = "mediatek,mt7621-sysc", "syscon";
     reg = <0x0 0x100>;
     #clock-cells = <1>;
     ralink,memctl = <&memc>;
     clock-output-names = "xtal", "cpu", "bus",
                                        "50m", "125m", "150m",
                                        "250m", "270m";
};

Consumer clock:

node: node@0 {
  ...
  clocks = <&sysc MT7621_CLK_WHATEVER>;
 ...
};

If that is the case... and since 'sysc' is used as system control
registers for all the rest of the world, where should be the yaml file
with bindings placed?

Thanks in advance again for your help.

Best regards,
    Sergio Paracuellos

>
> Best regards,
>     Sergio Paracuellos
> >
> > > +
> > > +  clock-output-names:
> > > +    maxItems: 8
> > > +
> > > +required:
> > > +  - compatible
> > > +  - '#clock-cells'
> > > +  - ralink,sysctl
> > > +  - ralink,memctl
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/mt7621-clk.h>
> > > +
> > > +    pll {
> > > +      compatible = "mediatek,mt7621-clk";
> > > +      #clock-cells = <1>;
> > > +      ralink,sysctl = <&sysc>;
> > > +      ralink,memctl = <&memc>;
> > > +      clock-output-names = "xtal", "cpu", "bus",
> > > +                           "50m", "125m", "150m",
> > > +                           "250m", "270m";
> > > +    };
> > > --
> > > 2.25.1
> > >
Sergio Paracuellos March 7, 2021, 6:27 a.m. UTC | #5 
Hi,
On Sat, Mar 6, 2021 at 10:54 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi again,
>
> On Sat, Mar 6, 2021 at 8:12 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Hi Rob,
> >
> > On Fri, Mar 5, 2021 at 11:47 PM Rob Herring <robh@kernel.org> wrote:
> > [snip]
> > > > +
> > > > +  ralink,sysctl:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      phandle of syscon used to control system registers
> > > > +
> > > > +  ralink,memctl:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      phandle of syscon used to control memory registers
> > >
> > > I assume one of these phandles are the main registers for the clocks?
> > > Make this a child node and drop that phandle.
> >
> > The 'ralink,sysctl' phandle is to read bootstrap register to be able
> > to derive xtal and a clk gate register for the peripherals.
> > The 'ralink,memctl' phandle is to read the cpu clock frequency from
> > the memory controller.
> >
> > So there is not "main registers". I already put this as a child node
> > in v4 and I was told to get rid of child nodes. I need this as a
> > regmap to other DT node registers (sysctl, and memctl) to be able to
> > use the driver without specific architecture operations and properly
> > enable for COMPILE_TEST without dirty Makefile arch flags. Both sysctl
> > and memctl has no other child nodes, and I think that's why I was told
> > to avoid child nodes at the end. I explained here [0] current sysctl
> > and memctl in the mt7621 device tree and my view of the need for this
> > two syscons:
> >
> > [0]: https://lkml.org/lkml/2021/1/2/9
> >
> > So to avoid to send again "a previous version" on this patch, please
> > guide me in the correct thing to do. Stephen, Rob, I will be really
> > happy with your help :)
>
> Since there are no other child nodes for this sysc, should merge clock
> properties
> with this node in the following way a valid approach:
>
>  sysc: sysc@0 {
>      compatible = "mediatek,mt7621-sysc", "syscon";
>      reg = <0x0 0x100>;
>      #clock-cells = <1>;
>      ralink,memctl = <&memc>;
>      clock-output-names = "xtal", "cpu", "bus",
>                                         "50m", "125m", "150m",
>                                         "250m", "270m";
> };
>
> Consumer clock:
>
> node: node@0 {
>   ...
>   clocks = <&sysc MT7621_CLK_WHATEVER>;
>  ...
> };

I have been reviewing bindings review comments along the time and I
was already suggested to do this I am saying here (see [0]) but my
mind seems that filtered it for any reason I don't really understand.
Maybe I should sleep a bit more :).

I will send v10 with these changes that hopefully will be the correct ones.

Thanks and sorry for bothering you with already suggested things.

Best regards,
    Sergio Paracuellos

[0]: https://lkml.org/lkml/2020/12/31/206

>
> If that is the case... and since 'sysc' is used as system control
> registers for all the rest of the world, where should be the yaml file
> with bindings placed?
>
> Thanks in advance again for your help.
>
> Best regards,
>     Sergio Paracuellos
>
> >
> > Best regards,
> >     Sergio Paracuellos
> > >
> > > > +
> > > > +  clock-output-names:
> > > > +    maxItems: 8
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - '#clock-cells'
> > > > +  - ralink,sysctl
> > > > +  - ralink,memctl
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/mt7621-clk.h>
> > > > +
> > > > +    pll {
> > > > +      compatible = "mediatek,mt7621-clk";
> > > > +      #clock-cells = <1>;
> > > > +      ralink,sysctl = <&sysc>;
> > > > +      ralink,memctl = <&memc>;
> > > > +      clock-output-names = "xtal", "cpu", "bus",
> > > > +                           "50m", "125m", "150m",
> > > > +                           "250m", "270m";
> > > > +    };
> > > > --
> > > > 2.25.1
> > > >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
new file mode 100644
index 000000000000..842a0f2c9d40
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-clk.yaml
@@ -0,0 +1,66 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/mediatek,mt7621-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MT7621 Clock Device Tree Bindings
+
+maintainers:
+  - Sergio Paracuellos <sergio.paracuellos@gmail.com>
+
+description: |
+  The MT7621 has a PLL controller from where the cpu clock is provided
+  as well as derived clocks for the bus and the peripherals. It also
+  can gate SoC device clocks.
+
+  Each clock is assigned an identifier and client nodes use this identifier
+  to specify the clock which they consume.
+
+  All these identifiers could be found in:
+  [1]: <include/dt-bindings/clock/mt7621-clk.h>.
+
+properties:
+  compatible:
+    const: mediatek,mt7621-clk
+
+  "#clock-cells":
+    description:
+      The first cell indicates the clock number, see [1] for available
+      clocks.
+    const: 1
+
+  ralink,sysctl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle of syscon used to control system registers
+
+  ralink,memctl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle of syscon used to control memory registers
+
+  clock-output-names:
+    maxItems: 8
+
+required:
+  - compatible
+  - '#clock-cells'
+  - ralink,sysctl
+  - ralink,memctl
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt7621-clk.h>
+
+    pll {
+      compatible = "mediatek,mt7621-clk";
+      #clock-cells = <1>;
+      ralink,sysctl = <&sysc>;
+      ralink,memctl = <&memc>;
+      clock-output-names = "xtal", "cpu", "bus",
+                           "50m", "125m", "150m",
+                           "250m", "270m";
+    };