lkml.org 
[lkml]   [2022]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH V3 1/3] dt-bindings: clk: sprd: Add bindings for ums512 clock controller
    Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2022年4月19日周二 00:28写道:
    >
    > On 18/04/2022 14:56, Cixi Geng wrote:
    > > From: Cixi Geng <cixi.geng1@unisoc.com>
    > >
    > > Add a new bindings to describe ums512 clock compatible string.
    > >
    > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
    > > ---
    > > .../bindings/clock/sprd,ums512-clk.yaml | 112 ++++++++++++++++++
    > > 1 file changed, 112 insertions(+)
    > > create mode 100644 Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
    > >
    > > diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
    > > new file mode 100644
    > > index 000000000000..89824d7c6be4
    > > --- /dev/null
    > > +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml
    > > @@ -0,0 +1,112 @@
    > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
    > > +# Copyright 2022 Unisoc Inc.
    > > +%YAML 1.2
    > > +---
    > > +$id: "http://devicetree.org/schemas/clock/sprd,ums512-clk.yaml#"
    > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
    > > +
    > > +title: UMS512 Clock Control Unit Device Tree Bindings
    >
    > Remove "Device Tree Bindings". You could do the same also in the
    > subject, because you repeat the prefix ("dt-bindings: clk: sprd: Add
    > ums512 clock controller").
    >
    > > +
    > > +maintainers:
    > > + - Orson Zhai <orsonzhai@gmail.com>
    > > + - Baolin Wang <baolin.wang7@gmail.com>
    > > + - Chunyan Zhang <zhang.lyra@gmail.com>
    > > +
    > > +properties:
    > > + "#clock-cells":
    > > + const: 1
    > > +
    > > + compatible:
    >
    > Put the compatible first, by convention and makes finding matching
    > bindings easier.
    >
    > > + oneOf:
    > > + - items:
    > > + - const: sprd,ums512-glbregs
    > > + - const: syscon
    > > + - const: simple-mfd
    >
    > Why do you need simple-mfd for these? This looks like a regular syscon,
    > so usually does not come with children. What is more, why this "usual
    > syscon" is a separate clock controller in these bindings?
    there is a warning log before add these const. and the reason we need
    the simply-mfd
    is some clock is a child of syscon node,which should set these compatible.
    failed to match any schema with compatible: ['sprd,ums512-glbregs',
    'syscon', 'simple-mfd']
    >
    > > + - items:
    > > + - enum:
    > > + - sprd,ums512-apahb-gate
    > > + - sprd,ums512-ap-clk
    > > + - sprd,ums512-aonapb-clk
    > > + - sprd,ums512-pmu-gate
    > > + - sprd,ums512-g0-pll
    > > + - sprd,ums512-g2-pll
    > > + - sprd,ums512-g3-pll
    > > + - sprd,ums512-gc-pll
    > > + - sprd,ums512-aon-gate
    > > + - sprd,ums512-audcpapb-gate
    > > + - sprd,ums512-audcpahb-gate
    > > + - sprd,ums512-gpu-clk
    > > + - sprd,ums512-mm-clk
    > > + - sprd,ums512-mm-gate-clk
    > > + - sprd,ums512-apapb-gate
    > > +
    > > + clocks:
    > > + minItems: 1
    >
    > maxItems needed
    the previous version did has the maxitems, but makes error when run
    'make DT_CHECKER_FLAGS=-m dt_binding_check O=./dt-out \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml'
    CHKDT Documentation/devicetree/bindings/processed-schema.json
    /path-to-linux/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml:
    properties:clock-names: {'minItems': 1, 'maxItems': 4, 'items':
    [{'const': 'ext-26m'}, {'const': 'ext-32k'}, {'const': 'ext-4m'},
    {'const': 'rco-100m'}]} should not be valid under {'required':
    ['maxItems']}
    hint: "maxItems" is not needed with an "items" list

    >
    > > + description: |
    > > + The input parent clock(s) phandle for this clock, only list fixed
    > > + clocks which are declared in devicetree.
    >
    > The description does not make sense. These are bindings for a clock
    > controller, but you say here "for this clock", so what does "this" mean
    > here?
    >
    > > +
    > > + clock-names:
    > > + minItems: 1
    > > + items:
    > > + - const: ext-26m
    > > + - const: ext-32k
    > > + - const: ext-4m
    > > + - const: rco-100m
    > > +
    > > + reg:
    > > + maxItems: 1
    > > +
    > > +required:
    > > + - compatible
    > > + - '#clock-cells'
    >
    > Isn't reg also required? Always? Do you have examples where it is not
    > required? How do you configure the clocks without "reg"? I see you
    > copied a lot from sprd,sc9863a-clk.yaml but that file does not look correct.
    >
    > You have nodes with reg but without unit address ("rpll"). These nodes
    > are modeled as children but they are not children - it's a workaround
    > for exposing syscon, isn't it? The sc9863a looks like broken design, so
    > please do not duplicate it here.
    >
    > > +
    > > +if:
    > > + properties:
    > > + compatible:
    > > + enum:
    > > + - sprd,ums512-ap-clk
    > > + - sprd,ums512-aonapb-clk
    > > + - sprd,ums512-mm-clk
    > > +then:
    > > + required:
    > > + - reg
    > > +
    > > +else:
    > > + description: |
    > > + Other UMS512 clock nodes should be the child of a syscon node in
    > > + which compatible string should be:
    > > + "sprd,ums512-glbregs", "syscon", "simple-mfd"
    > > +
    > > + The 'reg' property for the clock node is also required if there is a sub
    > > + range of registers for the clocks.
    > > +
    > > +additionalProperties: true
    >
    > false
    the "false" makes error log:
    /path-to-linux/Documentation/devicetree/bindings/clock/sprd,ums512-clk.example.dtb:
    syscon@71000000: '#address-cells', '#size-cells',
    'clock-controller@0', 'ranges' do not match any of the regexes:
    'pinctrl-[0-9]+'
    and I reference the patch
    [https://www.spinics.net/lists/linux-leds/msg17032.html]

    >
    > > +
    > > +examples:
    > > + - |
    > > + ap_clk: clock-controller@20200000 {
    > > + compatible = "sprd,ums512-ap-clk";
    > > + reg = <0x20200000 0x1000>;
    > > + clocks = <&ext_26m>;
    > > + clock-names = "ext-26m";
    > > + #clock-cells = <1>;
    > > + };
    > > +
    > > + - |
    > > + ap_apb_regs: syscon@71000000 {
    > > + compatible = "sprd,ums512-glbregs", "syscon", "simple-mfd";
    >
    > So here is the answer why you added MFD, but I still don't get why do
    > you need it for one child? It is quite a dance here and in your drivers,
    > instead of adding "syscon" to your clock controller.

    [1]in the if/else describtion, th other UMS512 clock nodes should be
    the child of a syscon node in
    which compatible string should be: "sprd,ums512-glbregs", "syscon",
    "simple-mfd"

    >
    > This also pollutes the actual bindings because you did not add
    > properties required for clock controllers, because of describing here
    > the syscon part.
    >
    > > + reg = <0x71000000 0x3000>;
    > > + #address-cells = <1>;
    > > + #size-cells = <1>;
    > > + #clock-cells = <1>;
    > > + ranges = <0 0x71000000 0x3000>;
    > > +
    > > + apahb_gate: clock-controller@0 {
    > > + compatible = "sprd,ums512-apahb-gate";
    > > + reg = <0x0 0x2000>;
    > > + #clock-cells = <1>;
    > > + };
    > > + };
    > > +
    > > +...
    >
    >
    > Best regards,
    > Krzysztof

    \
     
     \ /
      Last update: 2022-04-19 03:55    [W:5.388 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site