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

[Chunyan Zhang]

On Fri, 27 Dec 2019 at 02:56, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Mon, Dec 16, 2019 at 08:19:29PM +0800, Chunyan Zhang wrote:
> > From: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> >
> > add a new bindings to describe sc9863a clock compatible string.
> >
> > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxx>
> > +  - Baolin Wang <baolin.wang7@xxxxxxxxx>
> > +  - Chunyan Zhang <zhang.lyra@xxxxxxxxx>
> > +
> > +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
> >