| Message ID | 20230113110738.1505973-2-keguang.zhang@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Devicetree support for Loongson-1 clock | expand |
On 13/01/2023 12:07, Keguang Zhang wrote: > Add devicetree binding document for the Loongson-1 clock driver. Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. Subject: Drop driver, not related to hardware. > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > --- > .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > diff --git a/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > new file mode 100644 > index 000000000000..4709c6757f1e > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > @@ -0,0 +1,81 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/loongson,ls1x-clk.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Loongson-1 Clock Controller Wasn't this already sent? https://lore.kernel.org/all/20190130194731.GA25716@bogus/ Then this is a v4? Aren't you duplicating efforts (and reviewers efforts)? > + > +maintainers: > + - Keguang Zhang <keguang.zhang@gmail.com> > + > +properties: compatible is a first property. > + "#clock-cells": > + const: 0 > + > + compatible: > + enum: > + - loongson,ls1b-clk-pll > + - loongson,ls1b-clk-cpu > + - loongson,ls1b-clk-ahb > + - loongson,ls1c-clk-pll > + - loongson,ls1c-clk-cpu > + - loongson,ls1c-clk-ahb Are you registering single clocks? It looks like. No, make a proper clock controller. > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > +required: > + - "#clock-cells" > + - compatible > + - clocks > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + clocks { No, not really related to the binding. > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + xtal: xtal { Incorrect in this context. Missing unit address. > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <33000000>; > + }; > + > + pll: pll@1fe78030 { Node names should be generic, so "clock-controller" https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "loongson,ls1b-clk-pll"; > + #clock-cells = <0>; > + clocks = <&xtal>; > + reg = <0x1fe78030 0x4>; compatible is first property, then reg, then the rest. > + }; > + > + cpu_clk: cpu_clk@1fe78034 { No underscores in node names. Anyway this should be gone - make a proper clock controller. Best regards, Krzysztof
On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote: > Add devicetree binding document for the Loongson-1 clock driver. > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > --- > .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034) doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230113110738.1505973-2-keguang.zhang@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月13日周五 19:17写道： > > On 13/01/2023 12:07, Keguang Zhang wrote: > > Add devicetree binding document for the Loongson-1 clock driver. > > Subject: drop second/last, redundant "bindings". The "dt-bindings" > prefix is already stating that these are bindings. > > Subject: Drop driver, not related to hardware. Wil do. > > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > --- > > .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > > > diff --git a/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > new file mode 100644 > > index 000000000000..4709c6757f1e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > @@ -0,0 +1,81 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/clock/loongson,ls1x-clk.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Loongson-1 Clock Controller > > Wasn't this already sent? > https://lore.kernel.org/all/20190130194731.GA25716@bogus/ > Then this is a v4? Aren't you duplicating efforts (and reviewers efforts)? Sorry. I didn't notice that patch. This binding is totally different from that, which goes with the following driver re-implementation. > > > + > > +maintainers: > > + - Keguang Zhang <keguang.zhang@gmail.com> > > + > > +properties: > > compatible is a first property. Will do. > > > + "#clock-cells": > > + const: 0 > > + > > + compatible: > > + enum: > > + - loongson,ls1b-clk-pll > > + - loongson,ls1b-clk-cpu > > + - loongson,ls1b-clk-ahb > > + - loongson,ls1c-clk-pll > > + - loongson,ls1c-clk-cpu > > + - loongson,ls1c-clk-ahb > > Are you registering single clocks? It looks like. No, make a proper > clock controller. This binding contains two types of clock, pll-clk and div-clk. Should I split the binding to two bindings files? > > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > +required: > > + - "#clock-cells" > > + - compatible > > + - clocks > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + clocks { > > No, not really related to the binding. Should I remove the "clocks" section? > > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + xtal: xtal { > > Incorrect in this context. Missing unit address. XTAL doesn't have reg property. > > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <33000000>; > > + }; > > + > > + pll: pll@1fe78030 { > > Node names should be generic, so "clock-controller" > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Will change the node name. > > > + compatible = "loongson,ls1b-clk-pll"; > > + #clock-cells = <0>; > > + clocks = <&xtal>; > > + reg = <0x1fe78030 0x4>; > > compatible is first property, then reg, then the rest. Will do. > > > + }; > > + > > + cpu_clk: cpu_clk@1fe78034 { > > No underscores in node names. Anyway this should be gone - make a proper > clock controller. Will change the node name. > > > Best regards, > Krzysztof >
On 17/01/2023 11:31, Kelvin Cheung wrote: >>> + "#clock-cells": >>> + const: 0 >>> + >>> + compatible: >>> + enum: >>> + - loongson,ls1b-clk-pll >>> + - loongson,ls1b-clk-cpu >>> + - loongson,ls1b-clk-ahb >>> + - loongson,ls1c-clk-pll >>> + - loongson,ls1c-clk-cpu >>> + - loongson,ls1c-clk-ahb >> >> Are you registering single clocks? It looks like. No, make a proper >> clock controller. > > This binding contains two types of clock, pll-clk and div-clk. > Should I split the binding to two bindings files? No, you should register rather one clock controller. Why this have to be 3 separate clock controllers? >> >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> +required: >>> + - "#clock-cells" >>> + - compatible >>> + - clocks >>> + - reg >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + clocks { >> >> No, not really related to the binding. > > Should I remove the "clocks" section? Yes. >> >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges; >>> + >>> + xtal: xtal { >> >> Incorrect in this context. Missing unit address. > > XTAL doesn't have reg property. Yeah, but DTS is not correct now, is it? If you doubt, build your DTB with W=1. >> >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <33000000>; >>> + }; >>> + Best regards, Krzysztof
Rob Herring <robh@kernel.org> 于2023年1月13日周五 23:26写道： > > > On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote: > > Add devicetree binding document for the Loongson-1 clock driver. > > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > > --- > > .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034) I did notice this warning. But my situation is the cpu_clk and ahb_clk share the same registers. May I have your suggestion? Thanks! > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230113110738.1505973-2-keguang.zhang@gmail.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. >
On 17/01/2023 11:54, Kelvin Cheung wrote: > Rob Herring <robh@kernel.org> 于2023年1月13日周五 23:26写道： >> >> >> On Fri, 13 Jan 2023 19:07:37 +0800, Keguang Zhang wrote: >>> Add devicetree binding document for the Loongson-1 clock driver. >>> >>> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> >>> --- >>> .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ >>> 1 file changed, 81 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml >>> >> >> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' >> on your patch (DT_CHECKER_FLAGS is new in v5.13): >> >> yamllint warnings/errors: >> >> dtschema/dtc warnings/errors: >> Documentation/devicetree/bindings/clock/loongson,ls1x-clk.example.dts:36.39-41.15: Warning (unique_unit_address_if_enabled): /example-0/clocks/cpu_clk@1fe78034: duplicate unit-address (also used in node /example-0/clocks/ahb_clk@1fe78034) > > I did notice this warning. > But my situation is the cpu_clk and ahb_clk share the same registers. > May I have your suggestion? Don't introduce warnings and errors no matter what. If your code is not correct, don't submit it, but instead try to fix it. You got proper solution - use one clock controller, not devices per each clock. Why Loongson is special here from all other devices in the world? Best regards, Krzysztof
Hi Krzysztof, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月17日周二 18:47写道： > > On 17/01/2023 11:31, Kelvin Cheung wrote: > >>> + "#clock-cells": > >>> + const: 0 > >>> + > >>> + compatible: > >>> + enum: > >>> + - loongson,ls1b-clk-pll > >>> + - loongson,ls1b-clk-cpu > >>> + - loongson,ls1b-clk-ahb > >>> + - loongson,ls1c-clk-pll > >>> + - loongson,ls1c-clk-cpu > >>> + - loongson,ls1c-clk-ahb > >> > >> Are you registering single clocks? It looks like. No, make a proper > >> clock controller. > > > > This binding contains two types of clock, pll-clk and div-clk. > > Should I split the binding to two bindings files? > > No, you should register rather one clock controller. Why this have to be > 3 separate clock controllers? > This sounds like a big change for the driver. Could you please show me a good example of one clock controller? Thanks very much! > >> > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + clocks: > >>> + maxItems: 1 > >>> + > >>> +required: > >>> + - "#clock-cells" > >>> + - compatible > >>> + - clocks > >>> + - reg > >>> + > >>> +additionalProperties: false > >>> + > >>> +examples: > >>> + - | > >>> + clocks { > >> > >> No, not really related to the binding. > > > > Should I remove the "clocks" section? > > Yes. > > >> > >>> + #address-cells = <1>; > >>> + #size-cells = <1>; > >>> + ranges; > >>> + > >>> + xtal: xtal { > >> > >> Incorrect in this context. Missing unit address. > > > > XTAL doesn't have reg property. > > Yeah, but DTS is not correct now, is it? If you doubt, build your DTB > with W=1. > No doubt. I just want to know the right way to declare XTAL. Could you please show me an example? Thanks again! > >> > >>> + compatible = "fixed-clock"; > >>> + #clock-cells = <0>; > >>> + clock-frequency = <33000000>; > >>> + }; > >>> + > > Best regards, > Krzysztof >
On 18/01/2023 12:16, Kelvin Cheung wrote: > Hi Krzysztof, > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 于2023年1月17日周二 18:47写道： >> >> On 17/01/2023 11:31, Kelvin Cheung wrote: >>>>> + "#clock-cells": >>>>> + const: 0 >>>>> + >>>>> + compatible: >>>>> + enum: >>>>> + - loongson,ls1b-clk-pll >>>>> + - loongson,ls1b-clk-cpu >>>>> + - loongson,ls1b-clk-ahb >>>>> + - loongson,ls1c-clk-pll >>>>> + - loongson,ls1c-clk-cpu >>>>> + - loongson,ls1c-clk-ahb >>>> >>>> Are you registering single clocks? It looks like. No, make a proper >>>> clock controller. >>> >>> This binding contains two types of clock, pll-clk and div-clk. >>> Should I split the binding to two bindings files? >> >> No, you should register rather one clock controller. Why this have to be >> 3 separate clock controllers? >> > This sounds like a big change for the driver. > Could you please show me a good example of one clock controller? All or almost all the drivers? > Thanks very much! >>>> >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + clocks: >>>>> + maxItems: 1 >>>>> + >>>>> +required: >>>>> + - "#clock-cells" >>>>> + - compatible >>>>> + - clocks >>>>> + - reg >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + clocks { >>>> >>>> No, not really related to the binding. >>> >>> Should I remove the "clocks" section? >> >> Yes. >> >>>> >>>>> + #address-cells = <1>; >>>>> + #size-cells = <1>; >>>>> + ranges; >>>>> + >>>>> + xtal: xtal { >>>> >>>> Incorrect in this context. Missing unit address. >>> >>> XTAL doesn't have reg property. >> >> Yeah, but DTS is not correct now, is it? If you doubt, build your DTB >> with W=1. >> > No doubt. > I just want to know the right way to declare XTAL. > Could you please show me an example? Almost all DTSes? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml new file mode 100644 index 000000000000..4709c6757f1e --- /dev/null +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml @@ -0,0 +1,81 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/loongson,ls1x-clk.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Loongson-1 Clock Controller + +maintainers: + - Keguang Zhang <keguang.zhang@gmail.com> + +properties: + "#clock-cells": + const: 0 + + compatible: + enum: + - loongson,ls1b-clk-pll + - loongson,ls1b-clk-cpu + - loongson,ls1b-clk-ahb + - loongson,ls1c-clk-pll + - loongson,ls1c-clk-cpu + - loongson,ls1c-clk-ahb + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + +required: + - "#clock-cells" + - compatible + - clocks + - reg + +additionalProperties: false + +examples: + - | + clocks { + #address-cells = <1>; + #size-cells = <1>; + ranges; + + xtal: xtal { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <33000000>; + }; + + pll: pll@1fe78030 { + compatible = "loongson,ls1b-clk-pll"; + #clock-cells = <0>; + clocks = <&xtal>; + reg = <0x1fe78030 0x4>; + }; + + cpu_clk: cpu_clk@1fe78034 { + compatible = "loongson,ls1b-clk-cpu"; + #clock-cells = <0>; + clocks = <&pll>; + reg = <0x1fe78034 0x4>; + }; + + ahb_clk: ahb_clk@1fe78034 { + compatible = "loongson,ls1b-clk-ahb"; + #clock-cells = <0>; + clocks = <&pll>; + reg = <0x1fe78034 0x4>; + }; + + apb_clk: apb_clk { + compatible = "fixed-factor-clock"; + #clock-cells = <0>; + clocks = <&ahb_clk>; + clock-div = <2>; + clock-mult = <1>; + }; + }; +...
Add devicetree binding document for the Loongson-1 clock driver. Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> --- .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml