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

[Rob Herring]

On Mon, Dec 30, 2019 at 8:07 PM Chunyan Zhang <zhang.lyra@xxxxxxxxx> wrote:
>
> 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.

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