[1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver

Commit Message

Keguang Zhang Jan. 13, 2023, 11:07 a.m. UTC

Add devicetree binding document for the Loongson-1 clock driver.

Signed-off-by: Keguang Zhang <[e-mail]>
---
 .../bindings/clock/loongson,ls1x-clk.yaml     | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml

Comments

Krzysztof Kozlowski Jan. 13, 2023, 11:17 a.m. UTC | #1

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 <[e-mail]>
> ---
>  .../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 <[e-mail]>
> +
> +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

Rob Herring (Arm) Jan. 13, 2023, 3:26 p.m. UTC | #2

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 <[e-mail]>
> ---
>  .../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/[e-mail]

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.

Keguang Zhang Jan. 17, 2023, 10:31 a.m. UTC | #3

Krzysztof Kozlowski <[e-mail]> 于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 <[e-mail]>
> > ---
> >  .../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 <[e-mail]>
> > +
> > +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
>

Krzysztof Kozlowski Jan. 17, 2023, 10:47 a.m. UTC | #4

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

Keguang Zhang Jan. 17, 2023, 10:54 a.m. UTC | #5

Rob Herring <[e-mail]> 于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 <[e-mail]>
> > ---
> >  .../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/[e-mail]
>
> 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 Jan. 17, 2023, 11:08 a.m. UTC | #6

On 17/01/2023 11:54, Kelvin Cheung wrote:
> Rob Herring <[e-mail]> 于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 <[e-mail]>
>>> ---
>>>  .../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

Keguang Zhang Jan. 18, 2023, 11:16 a.m. UTC | #7

Hi Krzysztof,

Krzysztof Kozlowski <[e-mail]> 于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
>

Krzysztof Kozlowski Jan. 18, 2023, 11:28 a.m. UTC | #8

On 18/01/2023 12:16, Kelvin Cheung wrote:
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <[e-mail]> 于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

Patch

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 <[e-mail]>
+
+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>;
+        };
+    };
+...