2019-12-26 18:57:18

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH V2 3/6] dt-bindings: clk: sprd: add bindings for sc9863a clock controller

On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> From: Chunyan Zhang <[email protected]>
>
> add a new bindings to describe sc9863a clock compatible string.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> .../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 <[email protected]>
> + - Baolin Wang <[email protected]>
> + - Chunyan Zhang <[email protected]>
> +
> +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
>


2019-12-31 03:08:05

by Chunyan Zhang

[permalink] [raw]
Subject: Re: [PATCH V2 3/6] dt-bindings: clk: sprd: add bindings for sc9863a clock controller

On Fri, 27 Dec 2019 at 02:56, Rob Herring <[email protected]> wrote:
>
> On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> > From: Chunyan Zhang <[email protected]>
> >
> > add a new bindings to describe sc9863a clock compatible string.
> >
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > .../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 <[email protected]>
> > + - Baolin Wang <[email protected]>
> > + - Chunyan Zhang <[email protected]>
> > +
> > +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
> >

2020-01-02 22:45:13

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH V2 3/6] dt-bindings: clk: sprd: add bindings for sc9863a clock controller

On Mon, Dec 30, 2019 at 8:07 PM Chunyan Zhang <[email protected]> wrote:
>
> On Fri, 27 Dec 2019 at 02:56, Rob Herring <[email protected]> wrote:
> >
> > On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> > > From: Chunyan Zhang <[email protected]>
> > >
> > > add a new bindings to describe sc9863a clock compatible string.
> > >
> > > Signed-off-by: Chunyan Zhang <[email protected]>
> > > ---
> > > .../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 <[email protected]>
> > > + - Baolin Wang <[email protected]>
> > > + - Chunyan Zhang <[email protected]>
> > > +
> > > +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