From: Cixi Geng <[email protected]>
Add a new bindings to describe ums512 clock compatible string.
Signed-off-by: Cixi Geng <[email protected]>
---
.../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
+
+maintainers:
+ - Orson Zhai <[email protected]>
+ - Baolin Wang <[email protected]>
+ - Chunyan Zhang <[email protected]>
+
+properties:
+ "#clock-cells":
+ const: 1
+
+ compatible:
+ oneOf:
+ - items:
+ - const: sprd,ums512-glbregs
+ - const: syscon
+ - const: 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
+ description: |
+ The input parent clock(s) phandle for this clock, only list fixed
+ clocks which are declared in devicetree.
+
+ clock-names:
+ minItems: 1
+ items:
+ - const: ext-26m
+ - const: ext-32k
+ - const: ext-4m
+ - const: rco-100m
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - '#clock-cells'
+
+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
+
+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";
+ 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>;
+ };
+ };
+
+...
--
2.25.1
Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
>
> On 18/04/2022 14:56, Cixi Geng wrote:
> > From: Cixi Geng <[email protected]>
> >
> > Add a new bindings to describe ums512 clock compatible string.
> >
> > Signed-off-by: Cixi Geng <[email protected]>
> > ---
> > .../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 <[email protected]>
> > + - Baolin Wang <[email protected]>
> > + - Chunyan Zhang <[email protected]>
> > +
> > +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
On 19/04/2022 03:53, Cixi Geng wrote:
> Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
>>
>> On 18/04/2022 14:56, Cixi Geng wrote:
>>> From: Cixi Geng <[email protected]>
>>>
>>> Add a new bindings to describe ums512 clock compatible string.
>>>
>>> Signed-off-by: Cixi Geng <[email protected]>
>>> ---
>>> .../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 <[email protected]>
>>> + - Baolin Wang <[email protected]>
>>> + - Chunyan Zhang <[email protected]>
>>> +
>>> +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']
Neither here nor later you did not answer the question - why do you need
such complex construction, instead of adding syscon to the clock controller?
Let me paste again my concerns:
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.
IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
Because of this you need to create complex ways to get the regmap for
the clock controller... Why not making it simple? Clock controller with
syscon?
>>
>>> + - 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
Warning is about clock-names, not about clocks. Fix the bindings.
>
>>
>>> + 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]
Which needs fixing. The "false" is a strict requirement for such end
(non-extendable) bindings.
>
>>
>>> +
>>> +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"
No, it should not. Fix the drivers, fix the DTS and the bindings. Then
the "should" disappears.
>
>>
>> This also pollutes the actual bindings because you did not add
>> properties required for clock controllers, because of describing here
>> the syscon part.
Best regards,
Krzysztof
On 18/04/2022 14:56, Cixi Geng wrote:
> From: Cixi Geng <[email protected]>
>
> Add a new bindings to describe ums512 clock compatible string.
>
> Signed-off-by: Cixi Geng <[email protected]>
> ---
> .../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 <[email protected]>
> + - Baolin Wang <[email protected]>
> + - Chunyan Zhang <[email protected]>
> +
> +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?
> + - 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
> + 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
> +
> +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.
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
Hi Krzysztof:
I find the history discuss about the sp9863 clock[1] and last
ums512-clk dt-bindings patch[2] which from chunyan.
please refer to the reasons below.
These clocks are at the same register range with global registers.
the registers shared with more than one devices
which basically are multimedia devices. You may noticed that these
are all gate clocks which are in the global registers
ranges and are used to controll the enable status of some devices or
some part of devices.
[1].https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/
[2].https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/
Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 14:38写道：
>
> On 19/04/2022 03:53, Cixi Geng wrote:
> > Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
> >>
> >> On 18/04/2022 14:56, Cixi Geng wrote:
> >>> From: Cixi Geng <[email protected]>
> >>>
> >>> Add a new bindings to describe ums512 clock compatible string.
> >>>
> >>> Signed-off-by: Cixi Geng <[email protected]>
> >>> ---
> >>> .../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 <[email protected]>
> >>> + - Baolin Wang <[email protected]>
> >>> + - Chunyan Zhang <[email protected]>
> >>> +
> >>> +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']
>
> Neither here nor later you did not answer the question - why do you need
> such complex construction, instead of adding syscon to the clock controller?
>
> Let me paste again my concerns:
>
> 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.
>
> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> Because of this you need to create complex ways to get the regmap for
> the clock controller... Why not making it simple? Clock controller with
> syscon?
>
> >>
> >>> + - 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
>
> Warning is about clock-names, not about clocks. Fix the bindings.
>
> >
> >>
> >>> + 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]
>
> Which needs fixing. The "false" is a strict requirement for such end
> (non-extendable) bindings.
>
> >
> >>
> >>> +
> >>> +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"
>
> No, it should not. Fix the drivers, fix the DTS and the bindings. Then
> the "should" disappears.
> >
> >>
> >> This also pollutes the actual bindings because you did not add
> >> properties required for clock controllers, because of describing here
> >> the syscon part.
>
>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 14:38写道：
>
> On 19/04/2022 03:53, Cixi Geng wrote:
> > Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
> >>
> >> On 18/04/2022 14:56, Cixi Geng wrote:
> >>> From: Cixi Geng <[email protected]>
> >>>
> >>> Add a new bindings to describe ums512 clock compatible string.
> >>>
> >>> Signed-off-by: Cixi Geng <[email protected]>
> >>> ---
> >>> .../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 <[email protected]>
> >>> + - Baolin Wang <[email protected]>
> >>> + - Chunyan Zhang <[email protected]>
> >>> +
> >>> +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']
>
> Neither here nor later you did not answer the question - why do you need
> such complex construction, instead of adding syscon to the clock controller?
>
> Let me paste again my concerns:
>
> 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.
>
> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> Because of this you need to create complex ways to get the regmap for
> the clock controller... Why not making it simple? Clock controller with
> syscon?
I find the history discuss about the sp9863 clock[1] and last
ums512-clk dt-bindings patch[2] which from chunyan.
please refer to the reasons below.
These clocks are at the same register range with global registers.
the registers shared with more than one devices which basically
are multimedia devices. You may noticed that these are all gate
clocks which are in the global registers ranges and are used to
controll the enable status of some devices or some part of devices.
[1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
[2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
>
> >>
> >>> + - 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
>
> Warning is about clock-names, not about clocks. Fix the bindings.
>
> >
> >>
> >>> + 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]
>
> Which needs fixing. The "false" is a strict requirement for such end
> (non-extendable) bindings.
>
> >
> >>
> >>> +
> >>> +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"
>
> No, it should not. Fix the drivers, fix the DTS and the bindings. Then
> the "should" disappears.
> >
> >>
> >> This also pollutes the actual bindings because you did not add
> >> properties required for clock controllers, because of describing here
> >> the syscon part.
>
>
>
> Best regards,
> Krzysztof
On 24/04/2022 12:47, Cixi Geng wrote:
> Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 14:38写道：
>>
>> On 19/04/2022 03:53, Cixi Geng wrote:
>>> Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
>>>>
>>>> On 18/04/2022 14:56, Cixi Geng wrote:
>>>>> From: Cixi Geng <[email protected]>
>>>>>
>>>>> Add a new bindings to describe ums512 clock compatible string.
>>>>>
>>>>> Signed-off-by: Cixi Geng <[email protected]>
>>>>> ---
>>>>> .../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 <[email protected]>
>>>>> + - Baolin Wang <[email protected]>
>>>>> + - Chunyan Zhang <[email protected]>
>>>>> +
>>>>> +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']
>>
>> Neither here nor later you did not answer the question - why do you need
>> such complex construction, instead of adding syscon to the clock controller?
>>
>> Let me paste again my concerns:
>>
>> 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.
>>
>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
>> Because of this you need to create complex ways to get the regmap for
>> the clock controller... Why not making it simple? Clock controller with
>> syscon?
>
> I find the history discuss about the sp9863 clock[1] and last
> ums512-clk dt-bindings patch[2] which from chunyan.
> please refer to the reasons below.
>
> These clocks are at the same register range with global registers.
> the registers shared with more than one devices which basically
> are multimedia devices. You may noticed that these are all gate
> clocks which are in the global registers ranges and are used to
> controll the enable status of some devices or some part of devices.
>
> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
Which looks like discussion about different bindings. You had there a
clock controller and additional clock device using "sprd,syscon". Why
the rpll is a subdevice and not a part of clock controller. The same as
all other clocks coming from that clock-controller, right? What is so
special about rpll that is is a separate device, not part of the clock
controller? It's the same address space, isn't it?
Best regards,
Krzysztof
On 24/04/2022 16:22, Cixi Geng wrote:
>>>>
>>>> Neither here nor later you did not answer the question - why do you need
>>>> such complex construction, instead of adding syscon to the clock controller?
>>>>
>>>> Let me paste again my concerns:
>>>>
>>>> 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.
>>>>
>>>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
>>>> Because of this you need to create complex ways to get the regmap for
>>>> the clock controller... Why not making it simple? Clock controller with
>>>> syscon?
>>>
>>> I find the history discuss about the sp9863 clock[1] and last
>>> ums512-clk dt-bindings patch[2] which from chunyan.
>>> please refer to the reasons below.
>>>
>>> These clocks are at the same register range with global registers.
>>> the registers shared with more than one devices which basically
>>> are multimedia devices. You may noticed that these are all gate
>>> clocks which are in the global registers ranges and are used to
>>> controll the enable status of some devices or some part of devices.
>>>
>>> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
>>> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
>>
>> Which looks like discussion about different bindings. You had there a
>> clock controller and additional clock device using "sprd,syscon". Why
>> the rpll is a subdevice and not a part of clock controller. The same as
>> all other clocks coming from that clock-controller, right? What is so
>> special about rpll that is is a separate device, not part of the clock
>> controller? It's the same address space, isn't it?
> The hardware spec design these clocks are not belonged to the syscon,
> the phandle is only used to get virtual map address for clocks which
> have the same phsical address base with one syscon.(I don't know the
> historical reason for this design) It also can wroten a clock sperated from
> syscon by add the reg which same as syscon. but will lead to a duplicate
> mapping--one is from the clock,and one is from syscon. which make difficulty
> in analyzing some panic problems.
I don't understand still. You said that they do not belong to same
address space, right? But the sprd,ums512-apahb-gate in this patch or
mentioned rpll
(https://elixir.bootlin.com/linux/v5.18-rc3/source/arch/arm64/boot/dts/sprd/sharkl3.dtsi#L106)
does not reference any other address space. It's entire address space is
the same as address space of glbregs.
So if it does not belong to the same address space, where is this space
defined?
Best regards,
Krzysztof
On 24/04/2022 05:14, Cixi Geng wrote:
> Hi Krzysztof:
> I find the history discuss about the sp9863 clock[1] and last
> ums512-clk dt-bindings patch[2] which from chunyan.
> please refer to the reasons below.
>
> These clocks are at the same register range with global registers.
> the registers shared with more than one devices
> which basically are multimedia devices. You may noticed that these
> are all gate clocks which are in the global registers
> ranges and are used to controll the enable status of some devices or
> some part of devices.
You replied top-post, I don't know what is it about.
>
> [1].https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/
> [2].https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/
Could you keep URLs without any additional characters (so surrounded by
whitespace characters) because it makes impossible to open them.
Best regards,
Krzysztof
Krzysztof Kozlowski <[email protected]> 于2022年4月24日周日 19:21写道：
>
> On 24/04/2022 12:47, Cixi Geng wrote:
> > Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 14:38写道：
> >>
> >> On 19/04/2022 03:53, Cixi Geng wrote:
> >>> Krzysztof Kozlowski <[email protected]> 于2022年4月19日周二 00:28写道：
> >>>>
> >>>> On 18/04/2022 14:56, Cixi Geng wrote:
> >>>>> From: Cixi Geng <[email protected]>
> >>>>>
> >>>>> Add a new bindings to describe ums512 clock compatible string.
> >>>>>
> >>>>> Signed-off-by: Cixi Geng <[email protected]>
> >>>>> ---
> >>>>> .../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 <[email protected]>
> >>>>> + - Baolin Wang <[email protected]>
> >>>>> + - Chunyan Zhang <[email protected]>
> >>>>> +
> >>>>> +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']
> >>
> >> Neither here nor later you did not answer the question - why do you need
> >> such complex construction, instead of adding syscon to the clock controller?
> >>
> >> Let me paste again my concerns:
> >>
> >> 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.
> >>
> >> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> >> Because of this you need to create complex ways to get the regmap for
> >> the clock controller... Why not making it simple? Clock controller with
> >> syscon?
> >
> > I find the history discuss about the sp9863 clock[1] and last
> > ums512-clk dt-bindings patch[2] which from chunyan.
> > please refer to the reasons below.
> >
> > These clocks are at the same register range with global registers.
> > the registers shared with more than one devices which basically
> > are multimedia devices. You may noticed that these are all gate
> > clocks which are in the global registers ranges and are used to
> > controll the enable status of some devices or some part of devices.
> >
> > [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
> > [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
>
> Which looks like discussion about different bindings. You had there a
> clock controller and additional clock device using "sprd,syscon". Why
> the rpll is a subdevice and not a part of clock controller. The same as
> all other clocks coming from that clock-controller, right? What is so
> special about rpll that is is a separate device, not part of the clock
> controller? It's the same address space, isn't it?
The hardware spec design these clocks are not belonged to the syscon,
the phandle is only used to get virtual map address for clocks which
have the same phsical address base with one syscon.(I don't know the
historical reason for this design) It also can wroten a clock sperated from
syscon by add the reg which same as syscon. but will lead to a duplicate
mapping--one is from the clock,and one is from syscon. which make difficulty
in analyzing some panic problems.
>
> Best regards,
> Krzysztof
On 24/04/2022 17:12, Cixi Geng wrote:
> Krzysztof Kozlowski <[email protected]> 于2022年4月24日周日 22:30写道：
>>
>> On 24/04/2022 16:22, Cixi Geng wrote:
>>>>>>
>>>>>> Neither here nor later you did not answer the question - why do you need
>>>>>> such complex construction, instead of adding syscon to the clock controller?
>>>>>>
>>>>>> Let me paste again my concerns:
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
>>>>>> Because of this you need to create complex ways to get the regmap for
>>>>>> the clock controller... Why not making it simple? Clock controller with
>>>>>> syscon?
>>>>>
>>>>> I find the history discuss about the sp9863 clock[1] and last
>>>>> ums512-clk dt-bindings patch[2] which from chunyan.
>>>>> please refer to the reasons below.
>>>>>
>>>>> These clocks are at the same register range with global registers.
>>>>> the registers shared with more than one devices which basically
>>>>> are multimedia devices. You may noticed that these are all gate
>>>>> clocks which are in the global registers ranges and are used to
>>>>> controll the enable status of some devices or some part of devices.
>>>>>
>>>>> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
>>>>> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
>>>>
>>>> Which looks like discussion about different bindings. You had there a
>>>> clock controller and additional clock device using "sprd,syscon". Why
>>>> the rpll is a subdevice and not a part of clock controller. The same as
>>>> all other clocks coming from that clock-controller, right? What is so
>>>> special about rpll that is is a separate device, not part of the clock
>>>> controller? It's the same address space, isn't it?
>>> The hardware spec design these clocks are not belonged to the syscon,
>>> the phandle is only used to get virtual map address for clocks which
>>> have the same phsical address base with one syscon.(I don't know the
>>> historical reason for this design) It also can wroten a clock sperated from
>>> syscon by add the reg which same as syscon. but will lead to a duplicate
>>> mapping--one is from the clock,and one is from syscon. which make difficulty
>>> in analyzing some panic problems.
>>
>> I don't understand still. You said that they do not belong to same
>> address space, right? But the sprd,ums512-apahb-gate in this patch or
>> mentioned rpll
>> (https://elixir.bootlin.com/linux/v5.18-rc3/source/arch/arm64/boot/dts/sprd/sharkl3.dtsi#L106)
>> does not reference any other address space. It's entire address space is
>> the same as address space of glbregs.
> Maybe I didn't describe clearly, what I said is these clocks isn't the
> syscom sub-clock.
> from chunyan's explain:
> they are at the same register range with global registers. in
> originally we put them
> directly onto the bus indeed when submitting the patches for SC9863A
> clocks last year,
> and it had a private property named 'sprd,syscon' which could provide
> regmap for these clocks.
> after follow Rob's suggetion we make them a child of the syscon. these
> are all gate clocks which
> are in the global registers ranges and are used to controll the enable
> status of some devices
> or some part of devices.
You need to help me here with the naming. What is "global registers"
range? Let's focus on sharkl3.dtsi and syscon@4035c000 with "rpll".
You have a clock controller @4035c000, which provides several clocks,
right? Then you have a rpll also @4035c000, so the register range is the
same. The register range is the same, isn't it?
I still don't see the answer to my question - why do you need a child
device for one clock if this looks like part of the clock-controller?
Best regards,
Krzysztof
Krzysztof Kozlowski <[email protected]> 于2022年4月24日周日 22:30写道：
>
> On 24/04/2022 16:22, Cixi Geng wrote:
> >>>>
> >>>> Neither here nor later you did not answer the question - why do you need
> >>>> such complex construction, instead of adding syscon to the clock controller?
> >>>>
> >>>> Let me paste again my concerns:
> >>>>
> >>>> 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.
> >>>>
> >>>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> >>>> Because of this you need to create complex ways to get the regmap for
> >>>> the clock controller... Why not making it simple? Clock controller with
> >>>> syscon?
> >>>
> >>> I find the history discuss about the sp9863 clock[1] and last
> >>> ums512-clk dt-bindings patch[2] which from chunyan.
> >>> please refer to the reasons below.
> >>>
> >>> These clocks are at the same register range with global registers.
> >>> the registers shared with more than one devices which basically
> >>> are multimedia devices. You may noticed that these are all gate
> >>> clocks which are in the global registers ranges and are used to
> >>> controll the enable status of some devices or some part of devices.
> >>>
> >>> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
> >>> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
> >>
> >> Which looks like discussion about different bindings. You had there a
> >> clock controller and additional clock device using "sprd,syscon". Why
> >> the rpll is a subdevice and not a part of clock controller. The same as
> >> all other clocks coming from that clock-controller, right? What is so
> >> special about rpll that is is a separate device, not part of the clock
> >> controller? It's the same address space, isn't it?
> > The hardware spec design these clocks are not belonged to the syscon,
> > the phandle is only used to get virtual map address for clocks which
> > have the same phsical address base with one syscon.(I don't know the
> > historical reason for this design) It also can wroten a clock sperated from
> > syscon by add the reg which same as syscon. but will lead to a duplicate
> > mapping--one is from the clock,and one is from syscon. which make difficulty
> > in analyzing some panic problems.
>
> I don't understand still. You said that they do not belong to same
> address space, right? But the sprd,ums512-apahb-gate in this patch or
> mentioned rpll
> (https://elixir.bootlin.com/linux/v5.18-rc3/source/arch/arm64/boot/dts/sprd/sharkl3.dtsi#L106)
> does not reference any other address space. It's entire address space is
> the same as address space of glbregs.
Maybe I didn't describe clearly, what I said is these clocks isn't the
syscom sub-clock.
from chunyan's explain:
they are at the same register range with global registers. in
originally we put them
directly onto the bus indeed when submitting the patches for SC9863A
clocks last year,
and it had a private property named 'sprd,syscon' which could provide
regmap for these clocks.
after follow Rob's suggetion we make them a child of the syscon. these
are all gate clocks which
are in the global registers ranges and are used to controll the enable
status of some devices
or some part of devices.
>
> So if it does not belong to the same address space, where is this space
> defined?
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski <[email protected]> 于2022年4月25日周一 17:00写道：
>
> On 24/04/2022 17:12, Cixi Geng wrote:
> > Krzysztof Kozlowski <[email protected]> 于2022年4月24日周日 22:30写道：
> >>
> >> On 24/04/2022 16:22, Cixi Geng wrote:
> >>>>>>
> >>>>>> Neither here nor later you did not answer the question - why do you need
> >>>>>> such complex construction, instead of adding syscon to the clock controller?
> >>>>>>
> >>>>>> Let me paste again my concerns:
> >>>>>>
> >>>>>> 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.
> >>>>>>
> >>>>>> IOW, sc9863a uses similar pattern as here and the DTS is made wrong.
> >>>>>> Because of this you need to create complex ways to get the regmap for
> >>>>>> the clock controller... Why not making it simple? Clock controller with
> >>>>>> syscon?
> >>>>>
> >>>>> I find the history discuss about the sp9863 clock[1] and last
> >>>>> ums512-clk dt-bindings patch[2] which from chunyan.
> >>>>> please refer to the reasons below.
> >>>>>
> >>>>> These clocks are at the same register range with global registers.
> >>>>> the registers shared with more than one devices which basically
> >>>>> are multimedia devices. You may noticed that these are all gate
> >>>>> clocks which are in the global registers ranges and are used to
> >>>>> controll the enable status of some devices or some part of devices.
> >>>>>
> >>>>> [1] https://lore.kernel.org/all/CAAfSe-s0gcehu0ZDj=FTe5S7CzAHC5mahXBH2fJm7mXS7Xys1Q@mail.gmail.com/#r
> >>>>> [2] https://lore.kernel.org/all/163425295208.1688384.11023187625793114662@swboyd.mtv.corp.google.com/#r
> >>>>
> >>>> Which looks like discussion about different bindings. You had there a
> >>>> clock controller and additional clock device using "sprd,syscon". Why
> >>>> the rpll is a subdevice and not a part of clock controller. The same as
> >>>> all other clocks coming from that clock-controller, right? What is so
> >>>> special about rpll that is is a separate device, not part of the clock
> >>>> controller? It's the same address space, isn't it?
> >>> The hardware spec design these clocks are not belonged to the syscon,
> >>> the phandle is only used to get virtual map address for clocks which
> >>> have the same phsical address base with one syscon.(I don't know the
> >>> historical reason for this design) It also can wroten a clock sperated from
> >>> syscon by add the reg which same as syscon. but will lead to a duplicate
> >>> mapping--one is from the clock,and one is from syscon. which make difficulty
> >>> in analyzing some panic problems.
> >>
> >> I don't understand still. You said that they do not belong to same
> >> address space, right? But the sprd,ums512-apahb-gate in this patch or
> >> mentioned rpll
> >> (https://elixir.bootlin.com/linux/v5.18-rc3/source/arch/arm64/boot/dts/sprd/sharkl3.dtsi#L106)
> >> does not reference any other address space. It's entire address space is
> >> the same as address space of glbregs.
> > Maybe I didn't describe clearly, what I said is these clocks isn't the
> > syscom sub-clock.
> > from chunyan's explain:
> > they are at the same register range with global registers. in
> > originally we put them
> > directly onto the bus indeed when submitting the patches for SC9863A
> > clocks last year,
> > and it had a private property named 'sprd,syscon' which could provide
> > regmap for these clocks.
> > after follow Rob's suggetion we make them a child of the syscon. these
> > are all gate clocks which
> > are in the global registers ranges and are used to controll the enable
> > status of some devices
> > or some part of devices.
>
> You need to help me here with the naming. What is "global registers"
> range? Let's focus on sharkl3.dtsi and syscon@4035c000 with "rpll".
>
> You have a clock controller @4035c000, which provides several clocks,
> right? Then you have a rpll also @4035c000, so the register range is the
> same. The register range is the same, isn't it?
the anlg_phy_g5_regs is not a clock controller.
In fact, this is just to provide an address for other modules to call regmap.
not provide a clk interface or device.
The clk configuration of rpll is based on the anlg_phy_g5_regs register.
The analog_g5 asic document is not only used to configure rpll, but also other
functions can be configured, but currently our driver is only used to provide
configuration rpll, so the range of the device node of rpll can be less than or
equal to the range of anlg_phy_g5_regs.
Hope this could explains your question
Best regards,
Cixi
>
> I still don't see the answer to my question - why do you need a child
> device for one clock if this looks like part of the clock-controller?
>
> Best regards,
> Krzysztof
On 26/04/2022 07:40, Cixi Geng wrote:
>> You need to help me here with the naming. What is "global registers"
>> range? Let's focus on sharkl3.dtsi and syscon@4035c000 with "rpll".
>>
>> You have a clock controller @4035c000, which provides several clocks,
>> right? Then you have a rpll also @4035c000, so the register range is the
>> same. The register range is the same, isn't it?
>
> the anlg_phy_g5_regs is not a clock controller.
> In fact, this is just to provide an address for other modules to call regmap.
> not provide a clk interface or device.
> The clk configuration of rpll is based on the anlg_phy_g5_regs register.
> The analog_g5 asic document is not only used to configure rpll, but also other
> functions can be configured, but currently our driver is only used to provide
> configuration rpll, so the range of the device node of rpll can be less than or
> equal to the range of anlg_phy_g5_regs.
> Hope this could explains your question
I see, thanks for explanation. Indeed making entire @4035c000
(anlg_phy_g5_regs) a clock controller would not match actual hardware,
since rpll clock is a small part of it. I am afraid though, that you
will duplicate such pattern even for the cases where that
design/register range would be suitable to be a clock controller and a
syscon. In one device.
Please fix the other comments in my review - except this discussed here,
the last one from email:
https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof