Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 于2022年4月19日周二 00:28写道： > > On 18/04/2022 14:56, Cixi Geng wrote: > > From: Cixi Geng <cixi.geng1@xxxxxxxxxx> > > > > Add a new bindings to describe ums512 clock compatible string. > > > > Signed-off-by: Cixi Geng <cixi.geng1@xxxxxxxxxx> > > --- > > .../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@xxxxxxxxx> > > + - Baolin Wang <baolin.wang7@xxxxxxxxx> > > + - Chunyan Zhang <zhang.lyra@xxxxxxxxx> > > + > > +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
