diff mbox series

[V2,3/6] dt-bindings: clk: sprd: add bindings for sc9863a clock controller

Message ID 20191216121932.22967-4-zhang.lyra@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Add clocks for Unisoc's SC9863A | expand

Commit Message

Chunyan Zhang Dec. 16, 2019, 12:19 p.m. UTC
From: Chunyan Zhang <chunyan.zhang@unisoc.com>

add a new bindings to describe sc9863a clock compatible string.

Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
---
 .../bindings/clock/sprd,sc9863a-clk.yaml      | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml

Comments

Rob Herring (Arm) Dec. 26, 2019, 6:56 p.m. UTC | #1
On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> 
> add a new bindings to describe sc9863a clock compatible string.
> 
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
>  .../bindings/clock/sprd,sc9863a-clk.yaml      | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> new file mode 100644
> index 000000000000..881f0a0287e5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2019 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/sprd,sc9863a-clk.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: SC9863A Clock Control Unit Device Tree Bindings
> +
> +maintainers:
> +  - Orson Zhai <orsonzhai@gmail.com>
> +  - Baolin Wang <baolin.wang7@gmail.com>
> +  - Chunyan Zhang <zhang.lyra@gmail.com>
> +
> +properties:
> +  "#clock-cells":
> +    const: 1
> +
> +  compatible :
> +    enum:
> +      - sprd,sc9863a-ap-clk
> +      - sprd,sc9863a-pmu-gate
> +      - sprd,sc9863a-pll
> +      - sprd,sc9863a-mpll
> +      - sprd,sc9863a-rpll
> +      - sprd,sc9863a-dpll
> +      - sprd,sc9863a-aon-clk
> +      - sprd,sc9863a-apahb-gate
> +      - sprd,sc9863a-aonapb-gate
> +      - sprd,sc9863a-mm-gate
> +      - sprd,sc9863a-mm-clk
> +      - sprd,sc9863a-vspahb-gate
> +      - sprd,sc9863a-apapb-gate

These will probably need to be split to separate schemas for the reasons 
below...

> +
> +  clocks:
> +    description: |
> +      The input parent clock(s) phandle for this clock, only list fixed
> +      clocks which are decleared in devicetree.

typo.

You need to define how many clocks.

> +
> +  clock-names:
> +    description: |
> +      Clock name strings used for driver to reference.

You need to list out the names.

> +
> +  reg:
> +    description: |
> +      Contain the registers base address and length. It must be configured
> +      only if no 'sprd,syscon' under the node.
> +
> +  sprd,syscon:
> +    $ref: '/schemas/types.yaml#/definitions/phandle'
> +    description: |
> +      The phandle to the syscon which is in the same address area with
> +      the clock, and so we can get regmap for the clocks from the
> +      syscon device.

It is preferred to make the clock node a child of the syscon and then 
you don't need this property.

> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +
> +examples:
> +  - |
> +    ap_clk: clock-controller@21500000 {
> +      compatible = "sprd,sc9863a-ap-clk";
> +      reg = <0 0x21500000 0 0x1000>;
> +      clocks = <&ext_32k>, <&ext_26m>;
> +      clock-names = "ext-32k", "ext-26m";
> +      #clock-cells = <1>;
> +    };
> +
> +  - |
> +    apahb_gate: apahb-gate {
> +      compatible = "sprd,sc9863a-apahb-gate";
> +      sprd,syscon = <&ap_ahb_regs>;
> +      #clock-cells = <1>;
> +    };
> +
> +...
> -- 
> 2.20.1
>
Chunyan Zhang Dec. 31, 2019, 3:06 a.m. UTC | #2
On Fri, 27 Dec 2019 at 02:56, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> >
> > add a new bindings to describe sc9863a clock compatible string.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > ---
> >  .../bindings/clock/sprd,sc9863a-clk.yaml      | 77 +++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> > new file mode 100644
> > index 000000000000..881f0a0287e5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2019 Unisoc Inc.
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/clock/sprd,sc9863a-clk.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: SC9863A Clock Control Unit Device Tree Bindings
> > +
> > +maintainers:
> > +  - Orson Zhai <orsonzhai@gmail.com>
> > +  - Baolin Wang <baolin.wang7@gmail.com>
> > +  - Chunyan Zhang <zhang.lyra@gmail.com>
> > +
> > +properties:
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  compatible :
> > +    enum:
> > +      - sprd,sc9863a-ap-clk
> > +      - sprd,sc9863a-pmu-gate
> > +      - sprd,sc9863a-pll
> > +      - sprd,sc9863a-mpll
> > +      - sprd,sc9863a-rpll
> > +      - sprd,sc9863a-dpll
> > +      - sprd,sc9863a-aon-clk
> > +      - sprd,sc9863a-apahb-gate
> > +      - sprd,sc9863a-aonapb-gate
> > +      - sprd,sc9863a-mm-gate
> > +      - sprd,sc9863a-mm-clk
> > +      - sprd,sc9863a-vspahb-gate
> > +      - sprd,sc9863a-apapb-gate
>
> These will probably need to be split to separate schemas for the reasons
> below...
>
> > +
> > +  clocks:
> > +    description: |
> > +      The input parent clock(s) phandle for this clock, only list fixed
> > +      clocks which are decleared in devicetree.
>
> typo.
>
> You need to define how many clocks.

Ok, will add a define of maxItems.

>
> > +
> > +  clock-names:
> > +    description: |
> > +      Clock name strings used for driver to reference.
>
> You need to list out the names.
>
> > +
> > +  reg:
> > +    description: |
> > +      Contain the registers base address and length. It must be configured
> > +      only if no 'sprd,syscon' under the node.
> > +
> > +  sprd,syscon:
> > +    $ref: '/schemas/types.yaml#/definitions/phandle'
> > +    description: |
> > +      The phandle to the syscon which is in the same address area with
> > +      the clock, and so we can get regmap for the clocks from the
> > +      syscon device.
>
> It is preferred to make the clock node a child of the syscon and then
> you don't need this property.

According to the hardware topology, any clocks are not belonged to
syscon, like described here, this phandle is only used to get virtual
map address for clocks which have the same phsical address base with
one syscon.

In the past, clocks were defined like below:
    apahb_gate: apahb-gate {
      compatible = "sprd,sc9863a-apahb-gate";
      reg = <0 0x20e00000 0 0x1000>;
      #clock-cells = <1>;
    };

And there was also a syscon which had the same base address like below:
ap_ahb_regs: syscon@20e00000 {
compatible = "sprd,sc9863a-glbregs", "syscon";
reg = <0 0x20e00000 0 0x4000>;
};

To avoid one phsical address was remapped more than one time, I think
using the mapped address by syscon directly would be better.
Any other suggestions are very appreciated.

Thanks,
Chunyan

>
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +
> > +examples:
> > +  - |
> > +    ap_clk: clock-controller@21500000 {
> > +      compatible = "sprd,sc9863a-ap-clk";
> > +      reg = <0 0x21500000 0 0x1000>;
> > +      clocks = <&ext_32k>, <&ext_26m>;
> > +      clock-names = "ext-32k", "ext-26m";
> > +      #clock-cells = <1>;
> > +    };
> > +
> > +  - |
> > +    apahb_gate: apahb-gate {
> > +      compatible = "sprd,sc9863a-apahb-gate";
> > +      sprd,syscon = <&ap_ahb_regs>;
> > +      #clock-cells = <1>;
> > +    };
> > +
> > +...
> > --
> > 2.20.1
> >
Rob Herring (Arm) Jan. 2, 2020, 10:42 p.m. UTC | #3
On Mon, Dec 30, 2019 at 8:07 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>
> On Fri, 27 Dec 2019 at 02:56, Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> > > From: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > >
> > > add a new bindings to describe sc9863a clock compatible string.
> > >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> > > ---
> > >  .../bindings/clock/sprd,sc9863a-clk.yaml      | 77 +++++++++++++++++++
> > >  1 file changed, 77 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> > > new file mode 100644
> > > index 000000000000..881f0a0287e5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
> > > @@ -0,0 +1,77 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright 2019 Unisoc Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/clock/sprd,sc9863a-clk.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: SC9863A Clock Control Unit Device Tree Bindings
> > > +
> > > +maintainers:
> > > +  - Orson Zhai <orsonzhai@gmail.com>
> > > +  - Baolin Wang <baolin.wang7@gmail.com>
> > > +  - Chunyan Zhang <zhang.lyra@gmail.com>
> > > +
> > > +properties:
> > > +  "#clock-cells":
> > > +    const: 1
> > > +
> > > +  compatible :
> > > +    enum:
> > > +      - sprd,sc9863a-ap-clk
> > > +      - sprd,sc9863a-pmu-gate
> > > +      - sprd,sc9863a-pll
> > > +      - sprd,sc9863a-mpll
> > > +      - sprd,sc9863a-rpll
> > > +      - sprd,sc9863a-dpll
> > > +      - sprd,sc9863a-aon-clk
> > > +      - sprd,sc9863a-apahb-gate
> > > +      - sprd,sc9863a-aonapb-gate
> > > +      - sprd,sc9863a-mm-gate
> > > +      - sprd,sc9863a-mm-clk
> > > +      - sprd,sc9863a-vspahb-gate
> > > +      - sprd,sc9863a-apapb-gate
> >
> > These will probably need to be split to separate schemas for the reasons
> > below...
> >
> > > +
> > > +  clocks:
> > > +    description: |
> > > +      The input parent clock(s) phandle for this clock, only list fixed
> > > +      clocks which are decleared in devicetree.
> >
> > typo.
> >
> > You need to define how many clocks.
>
> Ok, will add a define of maxItems.
>
> >
> > > +
> > > +  clock-names:
> > > +    description: |
> > > +      Clock name strings used for driver to reference.
> >
> > You need to list out the names.
> >
> > > +
> > > +  reg:
> > > +    description: |
> > > +      Contain the registers base address and length. It must be configured
> > > +      only if no 'sprd,syscon' under the node.
> > > +
> > > +  sprd,syscon:
> > > +    $ref: '/schemas/types.yaml#/definitions/phandle'
> > > +    description: |
> > > +      The phandle to the syscon which is in the same address area with
> > > +      the clock, and so we can get regmap for the clocks from the
> > > +      syscon device.
> >
> > It is preferred to make the clock node a child of the syscon and then
> > you don't need this property.
>
> According to the hardware topology, any clocks are not belonged to
> syscon, like described here, this phandle is only used to get virtual
> map address for clocks which have the same phsical address base with
> one syscon.
>
> In the past, clocks were defined like below:
>     apahb_gate: apahb-gate {
>       compatible = "sprd,sc9863a-apahb-gate";
>       reg = <0 0x20e00000 0 0x1000>;
>       #clock-cells = <1>;
>     };
>
> And there was also a syscon which had the same base address like below:
> ap_ahb_regs: syscon@20e00000 {
> compatible = "sprd,sc9863a-glbregs", "syscon";
> reg = <0 0x20e00000 0 0x4000>;
> };
>
> To avoid one phsical address was remapped more than one time, I think
> using the mapped address by syscon directly would be better.
> Any other suggestions are very appreciated.

I'm only suggesting you change the location of the node and get the
syscon by getting the parent node. IOW, do this:

ap_ahb_regs: syscon@20e00000 {
    compatible = "sprd,sc9863a-glbregs", "syscon";
    reg = <0 0x20e00000 0 0x4000>;

    apahb_gate: apahb-gate {
        compatible = "sprd,sc9863a-apahb-gate";
        #clock-cells = <1>;
    };
};

Now, if there is a sub range of registers for the clocks that are
contiguous, then apahb-gate should have a 'reg' property too (and
ranges in the parent). Linux will not use that, but it's a more
complete description of the h/w.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
new file mode 100644
index 000000000000..881f0a0287e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/sprd,sc9863a-clk.yaml
@@ -0,0 +1,77 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2019 Unisoc Inc.
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/clock/sprd,sc9863a-clk.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: SC9863A Clock Control Unit Device Tree Bindings
+
+maintainers:
+  - Orson Zhai <orsonzhai@gmail.com>
+  - Baolin Wang <baolin.wang7@gmail.com>
+  - Chunyan Zhang <zhang.lyra@gmail.com>
+
+properties:
+  "#clock-cells":
+    const: 1
+
+  compatible :
+    enum:
+      - sprd,sc9863a-ap-clk
+      - sprd,sc9863a-pmu-gate
+      - sprd,sc9863a-pll
+      - sprd,sc9863a-mpll
+      - sprd,sc9863a-rpll
+      - sprd,sc9863a-dpll
+      - sprd,sc9863a-aon-clk
+      - sprd,sc9863a-apahb-gate
+      - sprd,sc9863a-aonapb-gate
+      - sprd,sc9863a-mm-gate
+      - sprd,sc9863a-mm-clk
+      - sprd,sc9863a-vspahb-gate
+      - sprd,sc9863a-apapb-gate
+
+  clocks:
+    description: |
+      The input parent clock(s) phandle for this clock, only list fixed
+      clocks which are decleared in devicetree.
+
+  clock-names:
+    description: |
+      Clock name strings used for driver to reference.
+
+  reg:
+    description: |
+      Contain the registers base address and length. It must be configured
+      only if no 'sprd,syscon' under the node.
+
+  sprd,syscon:
+    $ref: '/schemas/types.yaml#/definitions/phandle'
+    description: |
+      The phandle to the syscon which is in the same address area with
+      the clock, and so we can get regmap for the clocks from the
+      syscon device.
+
+required:
+  - compatible
+  - '#clock-cells'
+
+examples:
+  - |
+    ap_clk: clock-controller@21500000 {
+      compatible = "sprd,sc9863a-ap-clk";
+      reg = <0 0x21500000 0 0x1000>;
+      clocks = <&ext_32k>, <&ext_26m>;
+      clock-names = "ext-32k", "ext-26m";
+      #clock-cells = <1>;
+    };
+
+  - |
+    apahb_gate: apahb-gate {
+      compatible = "sprd,sc9863a-apahb-gate";
+      sprd,syscon = <&ap_ahb_regs>;
+      #clock-cells = <1>;
+    };
+
+...