Re: [PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Chunyan Zhang]

Hi Rob,

On 7 November 2017 at 01:15, Rob Herring  wrote:
> On Thu, Nov 02, 2017 at 02:56:17PM +0800, Chunyan Zhang wrote:
>> Introduce a new binding with its documentation for Spreadtrum clock
>> sub-framework.
>>
>> Signed-off-by: Chunyan Zhang 
>> ---
>>  Documentation/devicetree/bindings/clock/sprd.txt | 55 
>> 
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
>> b/Documentation/devicetree/bindings/clock/sprd.txt
>> new file mode 100644
>> index 000..5c09529
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/sprd.txt
>> @@ -0,0 +1,55 @@
>> +Spreadtrum Clock Binding
>> +
>> +
>> +Required properties:
>> +- compatible: should contain the following compatible strings:
>> + - "sprd,sc9860-pmu-gate"
>> + - "sprd,sc9860-pll"
>> + - "sprd,sc9860-ap-clk"
>> + - "sprd,sc9860-aon-prediv"
>> + - "sprd,sc9860-apahb-gate"
>> + - "sprd,sc9860-aon-gate"
>> + - "sprd,sc9860-aonsecure-clk"
>> + - "sprd,sc9860-agcp-gate"
>> + - "sprd,sc9860-gpu-clk"
>> + - "sprd,sc9860-vsp-clk"
>> + - "sprd,sc9860-vsp-gate"
>> + - "sprd,sc9860-cam-clk"
>> + - "sprd,sc9860-cam-gate"
>> + - "sprd,sc9860-disp-clk"
>> + - "sprd,sc9860-disp-gate"
>> + - "sprd,sc9860-apapb-gate"
>> +
>> +- #clock-cells: must be 1
>> +
>> +- clocks : shall be the input parent clock(s) phandle for the clock.
>
> You need to document how many clocks for each block.

It depends, "clocks" property here just simply shows which clock group
the clock's parents are in.
The detailed dependency relationship (i.e. how many parents and which
are the parents) are implemented in driver code.

Ok, I should address more, will do in the next version.

>
>> +
>> +Optional properties:
>> +
>> +- reg:   Contain the registers base address and length. It must be 
>> configured only if no 'sprd,syscon' under the node.
>> +
>> +- sprd,syscon: phandle to the syscon which is in the same address area with 
>> the clock.
>> +
>> +Example:
>> +
>> + pmu_gate: pmu-gate {
>> + compatible = "sprd,sc9860-pmu-gate";
>> + sprd,syscon = <_apb>;
>
> Ideally, the pmu-gate node would be a child of pmu_apb and use the reg
> property if clock registers are a contiguous range. Then you don't need
> this phandle.

The pmu-gate is actually a clock independent from the 'pmu_apb' syscon
device, using a reference to syscon node instead of a reg property is
just to avoid mapping the same address areas repeatedly.  Spreadtrum's
clock h/w design is a little complicated, after discussing with Arnd
and Stephen, I then chose to implement in this way.

I guess the name of 'pmu_apb' might be confused, it's actually not a
bus, but a global address area stored a lot of registers shared by a
few devices including some clocks.  I think I'd better use another
name instead of pmu_apb :)

Please let me know if I'm missing something here.

Thanks,
Chunyan

>
>> + clocks = <_26m>;
>> + #clock-cells = <1>;
>> + };
>> +
>> + pll: pll {
>> + compatible = "sprd,sc9860-pll";
>> + sprd,syscon = <_apb>;
>
> Same here.
>
>> + clocks = <_gate 0>;
>> + #clock-cells = <1>;
>> + };
>> +
>> + ap_clk: clock-controller@2000 {
>> + compatible = "sprd,sc9860-ap-clk";
>> + reg = <0 0x2000 0 0x400>;
>> + clocks = <_26m>, < 0>,
>> +  <_gate 0>;
>> + #clock-cells = <1>;
>> + };
>> --
>> 2.7.4
>>

Re: [PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Chunyan Zhang]

Hi Rob,

On 7 November 2017 at 01:15, Rob Herring  wrote:
> On Thu, Nov 02, 2017 at 02:56:17PM +0800, Chunyan Zhang wrote:
>> Introduce a new binding with its documentation for Spreadtrum clock
>> sub-framework.
>>
>> Signed-off-by: Chunyan Zhang 
>> ---
>>  Documentation/devicetree/bindings/clock/sprd.txt | 55 
>> 
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
>> b/Documentation/devicetree/bindings/clock/sprd.txt
>> new file mode 100644
>> index 000..5c09529
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/sprd.txt
>> @@ -0,0 +1,55 @@
>> +Spreadtrum Clock Binding
>> +
>> +
>> +Required properties:
>> +- compatible: should contain the following compatible strings:
>> + - "sprd,sc9860-pmu-gate"
>> + - "sprd,sc9860-pll"
>> + - "sprd,sc9860-ap-clk"
>> + - "sprd,sc9860-aon-prediv"
>> + - "sprd,sc9860-apahb-gate"
>> + - "sprd,sc9860-aon-gate"
>> + - "sprd,sc9860-aonsecure-clk"
>> + - "sprd,sc9860-agcp-gate"
>> + - "sprd,sc9860-gpu-clk"
>> + - "sprd,sc9860-vsp-clk"
>> + - "sprd,sc9860-vsp-gate"
>> + - "sprd,sc9860-cam-clk"
>> + - "sprd,sc9860-cam-gate"
>> + - "sprd,sc9860-disp-clk"
>> + - "sprd,sc9860-disp-gate"
>> + - "sprd,sc9860-apapb-gate"
>> +
>> +- #clock-cells: must be 1
>> +
>> +- clocks : shall be the input parent clock(s) phandle for the clock.
>
> You need to document how many clocks for each block.

It depends, "clocks" property here just simply shows which clock group
the clock's parents are in.
The detailed dependency relationship (i.e. how many parents and which
are the parents) are implemented in driver code.

Ok, I should address more, will do in the next version.

>
>> +
>> +Optional properties:
>> +
>> +- reg:   Contain the registers base address and length. It must be 
>> configured only if no 'sprd,syscon' under the node.
>> +
>> +- sprd,syscon: phandle to the syscon which is in the same address area with 
>> the clock.
>> +
>> +Example:
>> +
>> + pmu_gate: pmu-gate {
>> + compatible = "sprd,sc9860-pmu-gate";
>> + sprd,syscon = <_apb>;
>
> Ideally, the pmu-gate node would be a child of pmu_apb and use the reg
> property if clock registers are a contiguous range. Then you don't need
> this phandle.

The pmu-gate is actually a clock independent from the 'pmu_apb' syscon
device, using a reference to syscon node instead of a reg property is
just to avoid mapping the same address areas repeatedly.  Spreadtrum's
clock h/w design is a little complicated, after discussing with Arnd
and Stephen, I then chose to implement in this way.

I guess the name of 'pmu_apb' might be confused, it's actually not a
bus, but a global address area stored a lot of registers shared by a
few devices including some clocks.  I think I'd better use another
name instead of pmu_apb :)

Please let me know if I'm missing something here.

Thanks,
Chunyan

>
>> + clocks = <_26m>;
>> + #clock-cells = <1>;
>> + };
>> +
>> + pll: pll {
>> + compatible = "sprd,sc9860-pll";
>> + sprd,syscon = <_apb>;
>
> Same here.
>
>> + clocks = <_gate 0>;
>> + #clock-cells = <1>;
>> + };
>> +
>> + ap_clk: clock-controller@2000 {
>> + compatible = "sprd,sc9860-ap-clk";
>> + reg = <0 0x2000 0 0x400>;
>> + clocks = <_26m>, < 0>,
>> +  <_gate 0>;
>> + #clock-cells = <1>;
>> + };
>> --
>> 2.7.4
>>

Re: [PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Rob Herring]

On Thu, Nov 02, 2017 at 02:56:17PM +0800, Chunyan Zhang wrote:
> Introduce a new binding with its documentation for Spreadtrum clock
> sub-framework.
> 
> Signed-off-by: Chunyan Zhang 
> ---
>  Documentation/devicetree/bindings/clock/sprd.txt | 55 
> 
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
> b/Documentation/devicetree/bindings/clock/sprd.txt
> new file mode 100644
> index 000..5c09529
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sprd.txt
> @@ -0,0 +1,55 @@
> +Spreadtrum Clock Binding
> +
> +
> +Required properties:
> +- compatible: should contain the following compatible strings:
> + - "sprd,sc9860-pmu-gate"
> + - "sprd,sc9860-pll"
> + - "sprd,sc9860-ap-clk"
> + - "sprd,sc9860-aon-prediv"
> + - "sprd,sc9860-apahb-gate"
> + - "sprd,sc9860-aon-gate"
> + - "sprd,sc9860-aonsecure-clk"
> + - "sprd,sc9860-agcp-gate"
> + - "sprd,sc9860-gpu-clk"
> + - "sprd,sc9860-vsp-clk"
> + - "sprd,sc9860-vsp-gate"
> + - "sprd,sc9860-cam-clk"
> + - "sprd,sc9860-cam-gate"
> + - "sprd,sc9860-disp-clk"
> + - "sprd,sc9860-disp-gate"
> + - "sprd,sc9860-apapb-gate"
> +
> +- #clock-cells: must be 1
> +
> +- clocks : shall be the input parent clock(s) phandle for the clock.

You need to document how many clocks for each block.

> +
> +Optional properties:
> +
> +- reg:   Contain the registers base address and length. It must be 
> configured only if no 'sprd,syscon' under the node.
> +
> +- sprd,syscon: phandle to the syscon which is in the same address area with 
> the clock.
> +
> +Example:
> +
> + pmu_gate: pmu-gate {
> + compatible = "sprd,sc9860-pmu-gate";
> + sprd,syscon = <_apb>;

Ideally, the pmu-gate node would be a child of pmu_apb and use the reg 
property if clock registers are a contiguous range. Then you don't need 
this phandle.

> + clocks = <_26m>;
> + #clock-cells = <1>;
> + };
> +
> + pll: pll {
> + compatible = "sprd,sc9860-pll";
> + sprd,syscon = <_apb>;

Same here.

> + clocks = <_gate 0>;
> + #clock-cells = <1>;
> + };
> +
> + ap_clk: clock-controller@2000 {
> + compatible = "sprd,sc9860-ap-clk";
> + reg = <0 0x2000 0 0x400>;
> + clocks = <_26m>, < 0>,
> +  <_gate 0>;
> + #clock-cells = <1>;
> + };
> -- 
> 2.7.4
> 

Re: [PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Rob Herring]

On Thu, Nov 02, 2017 at 02:56:17PM +0800, Chunyan Zhang wrote:
> Introduce a new binding with its documentation for Spreadtrum clock
> sub-framework.
> 
> Signed-off-by: Chunyan Zhang 
> ---
>  Documentation/devicetree/bindings/clock/sprd.txt | 55 
> 
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
> b/Documentation/devicetree/bindings/clock/sprd.txt
> new file mode 100644
> index 000..5c09529
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sprd.txt
> @@ -0,0 +1,55 @@
> +Spreadtrum Clock Binding
> +
> +
> +Required properties:
> +- compatible: should contain the following compatible strings:
> + - "sprd,sc9860-pmu-gate"
> + - "sprd,sc9860-pll"
> + - "sprd,sc9860-ap-clk"
> + - "sprd,sc9860-aon-prediv"
> + - "sprd,sc9860-apahb-gate"
> + - "sprd,sc9860-aon-gate"
> + - "sprd,sc9860-aonsecure-clk"
> + - "sprd,sc9860-agcp-gate"
> + - "sprd,sc9860-gpu-clk"
> + - "sprd,sc9860-vsp-clk"
> + - "sprd,sc9860-vsp-gate"
> + - "sprd,sc9860-cam-clk"
> + - "sprd,sc9860-cam-gate"
> + - "sprd,sc9860-disp-clk"
> + - "sprd,sc9860-disp-gate"
> + - "sprd,sc9860-apapb-gate"
> +
> +- #clock-cells: must be 1
> +
> +- clocks : shall be the input parent clock(s) phandle for the clock.

You need to document how many clocks for each block.

> +
> +Optional properties:
> +
> +- reg:   Contain the registers base address and length. It must be 
> configured only if no 'sprd,syscon' under the node.
> +
> +- sprd,syscon: phandle to the syscon which is in the same address area with 
> the clock.
> +
> +Example:
> +
> + pmu_gate: pmu-gate {
> + compatible = "sprd,sc9860-pmu-gate";
> + sprd,syscon = <_apb>;

Ideally, the pmu-gate node would be a child of pmu_apb and use the reg 
property if clock registers are a contiguous range. Then you don't need 
this phandle.

> + clocks = <_26m>;
> + #clock-cells = <1>;
> + };
> +
> + pll: pll {
> + compatible = "sprd,sc9860-pll";
> + sprd,syscon = <_apb>;

Same here.

> + clocks = <_gate 0>;
> + #clock-cells = <1>;
> + };
> +
> + ap_clk: clock-controller@2000 {
> + compatible = "sprd,sc9860-ap-clk";
> + reg = <0 0x2000 0 0x400>;
> + clocks = <_26m>, < 0>,
> +  <_gate 0>;
> + #clock-cells = <1>;
> + };
> -- 
> 2.7.4
> 

[PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Chunyan Zhang]

Introduce a new binding with its documentation for Spreadtrum clock
sub-framework.

Signed-off-by: Chunyan Zhang 
---
 Documentation/devicetree/bindings/clock/sprd.txt | 55 
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt

diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
b/Documentation/devicetree/bindings/clock/sprd.txt
new file mode 100644
index 000..5c09529
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/sprd.txt
@@ -0,0 +1,55 @@
+Spreadtrum Clock Binding
+
+
+Required properties:
+- compatible: should contain the following compatible strings:
+   - "sprd,sc9860-pmu-gate"
+   - "sprd,sc9860-pll"
+   - "sprd,sc9860-ap-clk"
+   - "sprd,sc9860-aon-prediv"
+   - "sprd,sc9860-apahb-gate"
+   - "sprd,sc9860-aon-gate"
+   - "sprd,sc9860-aonsecure-clk"
+   - "sprd,sc9860-agcp-gate"
+   - "sprd,sc9860-gpu-clk"
+   - "sprd,sc9860-vsp-clk"
+   - "sprd,sc9860-vsp-gate"
+   - "sprd,sc9860-cam-clk"
+   - "sprd,sc9860-cam-gate"
+   - "sprd,sc9860-disp-clk"
+   - "sprd,sc9860-disp-gate"
+   - "sprd,sc9860-apapb-gate"
+
+- #clock-cells: must be 1
+
+- clocks : shall be the input parent clock(s) phandle for the clock.
+
+Optional properties:
+
+- reg: Contain the registers base address and length. It must be configured 
only if no 'sprd,syscon' under the node.
+
+- sprd,syscon: phandle to the syscon which is in the same address area with 
the clock.
+
+Example:
+
+   pmu_gate: pmu-gate {
+   compatible = "sprd,sc9860-pmu-gate";
+   sprd,syscon = <_apb>;
+   clocks = <_26m>;
+   #clock-cells = <1>;
+   };
+
+   pll: pll {
+   compatible = "sprd,sc9860-pll";
+   sprd,syscon = <_apb>;
+   clocks = <_gate 0>;
+   #clock-cells = <1>;
+   };
+
+   ap_clk: clock-controller@2000 {
+   compatible = "sprd,sc9860-ap-clk";
+   reg = <0 0x2000 0 0x400>;
+   clocks = <_26m>, < 0>,
+<_gate 0>;
+   #clock-cells = <1>;
+   };
-- 
2.7.4

[PATCH V3 02/11] dt-bindings: Add Spreadtrum clock binding documentation

[Chunyan Zhang]

Introduce a new binding with its documentation for Spreadtrum clock
sub-framework.

Signed-off-by: Chunyan Zhang 
---
 Documentation/devicetree/bindings/clock/sprd.txt | 55 
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sprd.txt

diff --git a/Documentation/devicetree/bindings/clock/sprd.txt 
b/Documentation/devicetree/bindings/clock/sprd.txt
new file mode 100644
index 000..5c09529
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/sprd.txt
@@ -0,0 +1,55 @@
+Spreadtrum Clock Binding
+
+
+Required properties:
+- compatible: should contain the following compatible strings:
+   - "sprd,sc9860-pmu-gate"
+   - "sprd,sc9860-pll"
+   - "sprd,sc9860-ap-clk"
+   - "sprd,sc9860-aon-prediv"
+   - "sprd,sc9860-apahb-gate"
+   - "sprd,sc9860-aon-gate"
+   - "sprd,sc9860-aonsecure-clk"
+   - "sprd,sc9860-agcp-gate"
+   - "sprd,sc9860-gpu-clk"
+   - "sprd,sc9860-vsp-clk"
+   - "sprd,sc9860-vsp-gate"
+   - "sprd,sc9860-cam-clk"
+   - "sprd,sc9860-cam-gate"
+   - "sprd,sc9860-disp-clk"
+   - "sprd,sc9860-disp-gate"
+   - "sprd,sc9860-apapb-gate"
+
+- #clock-cells: must be 1
+
+- clocks : shall be the input parent clock(s) phandle for the clock.
+
+Optional properties:
+
+- reg: Contain the registers base address and length. It must be configured 
only if no 'sprd,syscon' under the node.
+
+- sprd,syscon: phandle to the syscon which is in the same address area with 
the clock.
+
+Example:
+
+   pmu_gate: pmu-gate {
+   compatible = "sprd,sc9860-pmu-gate";
+   sprd,syscon = <_apb>;
+   clocks = <_26m>;
+   #clock-cells = <1>;
+   };
+
+   pll: pll {
+   compatible = "sprd,sc9860-pll";
+   sprd,syscon = <_apb>;
+   clocks = <_gate 0>;
+   #clock-cells = <1>;
+   };
+
+   ap_clk: clock-controller@2000 {
+   compatible = "sprd,sc9860-ap-clk";
+   reg = <0 0x2000 0 0x400>;
+   clocks = <_26m>, < 0>,
+<_gate 0>;
+   #clock-cells = <1>;
+   };
-- 
2.7.4