[RFC V2:PATCH 0/2] add reset-hi3660

[RFC V2:PATCH 0/2] add reset-hi3660

[Zhangfei Gao]

Considering Arnd and Philipp suggestions, 
move reset register to dts as table instead of dts header in case of ABI issue

Zhangfei Gao (2):
  dt-bindings: Document the hi3660 reset bindings
  reset: hisilicon: add reset-hi3660

 .../bindings/reset/hisilicon,hi3660-reset.txt      |  51 ++++++++
 drivers/reset/hisilicon/Kconfig                    |   7 +
 drivers/reset/hisilicon/Makefile                   |   1 +
 drivers/reset/hisilicon/reset-hi3660.c             | 144 +++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
 create mode 100644 drivers/reset/hisilicon/reset-hi3660.c

-- 
2.7.4

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Zhangfei Gao]

Add DT bindings documentation for hi3660 SoC reset controller.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt

diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
new file mode 100644
index 0000000..250daf2
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
@@ -0,0 +1,51 @@
+Hisilicon System Reset Controller
+======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+The reset controller registers are part of the system-ctl block on
+hi3660 SoC.
+
+Required properties:
+- compatible: should be
+		 "hisilicon,hi3660-reset"
+- #reset-cells: 1, see below
+- hisi,rst-syscon: phandle of the reset's syscon.
+- hisi,reset-bits: Contains the reset control register information
+		  Should contain 2 cells for each reset exposed to
+		  consumers, defined as:
+			Cell #1 : offset from the syscon register base
+			Cell #2 : bits position of the control register
+
+Example:
+	iomcu: iomcu at ffd7e000 {
+		compatible = "hisilicon,hi3660-iomcu", "syscon";
+		reg = <0x0 0xffd7e000 0x0 0x1000>;
+	};
+
+	iomcu_rst: iomcu_rst_controller {
+		compatible = "hisilicon,hi3660-reset";
+		#reset-cells = <1>;
+		hisi,rst-syscon = <&iomcu>;
+		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
+				   0x20 0x10		/* 1: i2c1 */
+				   0x20 0x20		/* 2: i2c2 */
+				   0x20 0x8000000>;	/* 3: i2c6 */
+	};
+
+Specifying reset lines connected to IP modules
+==============================================
+example:
+
+        i2c0: i2c at ..... {
+                ...
+		resets = <&iomcu_rst 0>;
+                ...
+        };
+
+	i2c1: i2c at ..... {
+                ...
+		resets = <&iomcu_rst 1>;
+                ...
+        };
-- 
2.7.4

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> Add DT bindings documentation for hi3660 SoC reset controller.
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> new file mode 100644
> index 0000000..250daf2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> @@ -0,0 +1,51 @@
> +Hisilicon System Reset Controller
> +======================================
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +The reset controller registers are part of the system-ctl block on
> +hi3660 SoC.
> +
> +Required properties:
> +- compatible: should be
> +		 "hisilicon,hi3660-reset"
> +- #reset-cells: 1, see below
> +- hisi,rst-syscon: phandle of the reset's syscon.
> +- hisi,reset-bits: Contains the reset control register information
> +		  Should contain 2 cells for each reset exposed to
> +		  consumers, defined as:
> +			Cell #1 : offset from the syscon register base
> +			Cell #2 : bits position of the control register
> +
> +Example:
> +	iomcu: iomcu at ffd7e000 {
> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> +	};
> +
> +	iomcu_rst: iomcu_rst_controller {

This should be
	iomcu_rst: reset-controller {

> +		compatible = "hisilicon,hi3660-reset";
> +		#reset-cells = <1>;
> +		hisi,rst-syscon = <&iomcu>;
> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> +				   0x20 0x10		/* 1: i2c1 */
> +				   0x20 0x20		/* 2: i2c2 */
> +				   0x20 0x8000000>;	/* 3: i2c6 */
> +	};

The reset lines are controlled through iomcu bits, is there a reason not
to put the iomcu_rst node inside the iomcu node? That way the
hisi,rst-syscon property could be removed and the syscon could be
retrieved via the reset-controller parent node.

> +Specifying reset lines connected to IP modules
> +==============================================
> +example:
> +
> +        i2c0: i2c at ..... {
> +                ...
> +		resets = <&iomcu_rst 0>;
> +                ...
> +        };
> +
> +	i2c1: i2c at ..... {
> +                ...
> +		resets = <&iomcu_rst 1>;
> +                ...
> +        };

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

On 2016?11?24? 17:26, Philipp Zabel wrote:
> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>> Add DT bindings documentation for hi3660 SoC reset controller.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>> new file mode 100644
>> index 0000000..250daf2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>> @@ -0,0 +1,51 @@
>> +Hisilicon System Reset Controller
>> +======================================
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +The reset controller registers are part of the system-ctl block on
>> +hi3660 SoC.
>> +
>> +Required properties:
>> +- compatible: should be
>> +		 "hisilicon,hi3660-reset"
>> +- #reset-cells: 1, see below
>> +- hisi,rst-syscon: phandle of the reset's syscon.
>> +- hisi,reset-bits: Contains the reset control register information
>> +		  Should contain 2 cells for each reset exposed to
>> +		  consumers, defined as:
>> +			Cell #1 : offset from the syscon register base
>> +			Cell #2 : bits position of the control register
>> +
>> +Example:
>> +	iomcu: iomcu at ffd7e000 {
>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
>> +	};
>> +
>> +	iomcu_rst: iomcu_rst_controller {
> This should be
> 	iomcu_rst: reset-controller {
>
>> +		compatible = "hisilicon,hi3660-reset";
>> +		#reset-cells = <1>;
>> +		hisi,rst-syscon = <&iomcu>;
>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
>> +				   0x20 0x10		/* 1: i2c1 */
>> +				   0x20 0x20		/* 2: i2c2 */
>> +				   0x20 0x8000000>;	/* 3: i2c6 */
>> +	};
> The reset lines are controlled through iomcu bits, is there a reason not
> to put the iomcu_rst node inside the iomcu node? That way the
> hisi,rst-syscon property could be removed and the syscon could be
> retrieved via the reset-controller parent node.
iomcu is common registers, controls clock and reset, etc.
So we use syscon, without mapping the registers everywhere.
It is common case in hisilicon, same in hi6220.

Also the #clock-cells and #reset-cells can not be put in the same node,
if they are both using probe, since reset_probe will not be called.

So we use hisi,rst-syscon as a general solution.

Thanks

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> 
> On 2016?11?24? 17:26, Philipp Zabel wrote:
> > Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >> Add DT bindings documentation for hi3660 SoC reset controller.
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> ---
> >>   .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> new file mode 100644
> >> index 0000000..250daf2
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> @@ -0,0 +1,51 @@
> >> +Hisilicon System Reset Controller
> >> +======================================
> >> +
> >> +Please also refer to reset.txt in this directory for common reset
> >> +controller binding usage.
> >> +
> >> +The reset controller registers are part of the system-ctl block on
> >> +hi3660 SoC.
> >> +
> >> +Required properties:
> >> +- compatible: should be
> >> +		 "hisilicon,hi3660-reset"
> >> +- #reset-cells: 1, see below
> >> +- hisi,rst-syscon: phandle of the reset's syscon.
> >> +- hisi,reset-bits: Contains the reset control register information
> >> +		  Should contain 2 cells for each reset exposed to
> >> +		  consumers, defined as:
> >> +			Cell #1 : offset from the syscon register base
> >> +			Cell #2 : bits position of the control register
> >> +
> >> +Example:
> >> +	iomcu: iomcu at ffd7e000 {
> >> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> >> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >> +	};
> >> +
> >> +	iomcu_rst: iomcu_rst_controller {
> > This should be
> > 	iomcu_rst: reset-controller {
> >
> >> +		compatible = "hisilicon,hi3660-reset";
> >> +		#reset-cells = <1>;
> >> +		hisi,rst-syscon = <&iomcu>;
> >> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> >> +				   0x20 0x10		/* 1: i2c1 */
> >> +				   0x20 0x20		/* 2: i2c2 */
> >> +				   0x20 0x8000000>;	/* 3: i2c6 */
> >> +	};
> > The reset lines are controlled through iomcu bits, is there a reason not
> > to put the iomcu_rst node inside the iomcu node? That way the
> > hisi,rst-syscon property could be removed and the syscon could be
> > retrieved via the reset-controller parent node.
> iomcu is common registers, controls clock and reset, etc.
> So we use syscon, without mapping the registers everywhere.
> It is common case in hisilicon, same in hi6220.
>
> Also the #clock-cells and #reset-cells can not be put in the same node,
> if they are both using probe, since reset_probe will not be called.
> 
> So we use hisi,rst-syscon as a general solution.

What I meant is this:

	iomcu: iomcu at ffd7e000 {
		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
		reg = <0x0 0xffd7e000 0x0 0x1000>;

		iomcu_rst: reset-controller {
			compatible = "hisilicon,hi3660-reset";
			#reset-cells = <1>;
			hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
					   0x20 0x10		/* 1: i2c1 */
					   0x20 0x20		/* 2: i2c2 */
					   0x20 0x8000000>;	/* 3: i2c6 */
		};
	};

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

On 2016?11?24? 17:50, Philipp Zabel wrote:
> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
>> On 2016?11?24? 17:26, Philipp Zabel wrote:
>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>>>> Add DT bindings documentation for hi3660 SoC reset controller.
>>>>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>> ---
>>>>    .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>>>>    1 file changed, 51 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>> new file mode 100644
>>>> index 0000000..250daf2
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>> @@ -0,0 +1,51 @@
>>>> +Hisilicon System Reset Controller
>>>> +======================================
>>>> +
>>>> +Please also refer to reset.txt in this directory for common reset
>>>> +controller binding usage.
>>>> +
>>>> +The reset controller registers are part of the system-ctl block on
>>>> +hi3660 SoC.
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be
>>>> +		 "hisilicon,hi3660-reset"
>>>> +- #reset-cells: 1, see below
>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
>>>> +- hisi,reset-bits: Contains the reset control register information
>>>> +		  Should contain 2 cells for each reset exposed to
>>>> +		  consumers, defined as:
>>>> +			Cell #1 : offset from the syscon register base
>>>> +			Cell #2 : bits position of the control register
>>>> +
>>>> +Example:
>>>> +	iomcu: iomcu at ffd7e000 {
>>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
>>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
>>>> +	};
>>>> +
>>>> +	iomcu_rst: iomcu_rst_controller {
>>> This should be
>>> 	iomcu_rst: reset-controller {
>>>
>>>> +		compatible = "hisilicon,hi3660-reset";
>>>> +		#reset-cells = <1>;
>>>> +		hisi,rst-syscon = <&iomcu>;
>>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
>>>> +				   0x20 0x10		/* 1: i2c1 */
>>>> +				   0x20 0x20		/* 2: i2c2 */
>>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
>>>> +	};
>>> The reset lines are controlled through iomcu bits, is there a reason not
>>> to put the iomcu_rst node inside the iomcu node? That way the
>>> hisi,rst-syscon property could be removed and the syscon could be
>>> retrieved via the reset-controller parent node.
>> iomcu is common registers, controls clock and reset, etc.
>> So we use syscon, without mapping the registers everywhere.
>> It is common case in hisilicon, same in hi6220.
>>
>> Also the #clock-cells and #reset-cells can not be put in the same node,
>> if they are both using probe, since reset_probe will not be called.
>>
>> So we use hisi,rst-syscon as a general solution.
> What I meant is this:
>
> 	iomcu: iomcu at ffd7e000 {
> 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> 		reg = <0x0 0xffd7e000 0x0 0x1000>;
#clock-cells = <1>;
In my test, if there add #clock-cells = <1>, reset_probe will not be 
called any more.
Since clk_probe is called first.
No matter iomcu_rst is child node or not.

Thanks
>
> 		iomcu_rst: reset-controller {
> 			compatible = "hisilicon,hi3660-reset";
> 			#reset-cells = <1>;
> 			hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> 					   0x20 0x10		/* 1: i2c1 */
> 					   0x20 0x20		/* 2: i2c2 */
> 					   0x20 0x8000000>;	/* 3: i2c6 */
> 		};
> 	};
>
> regards
> Philipp
>

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
> 
> On 2016?11?24? 17:50, Philipp Zabel wrote:
> > Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> >> On 2016?11?24? 17:26, Philipp Zabel wrote:
> >>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >>>> Add DT bindings documentation for hi3660 SoC reset controller.
> >>>>
> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>>> ---
> >>>>    .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
> >>>>    1 file changed, 51 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>> new file mode 100644
> >>>> index 0000000..250daf2
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>> @@ -0,0 +1,51 @@
> >>>> +Hisilicon System Reset Controller
> >>>> +======================================
> >>>> +
> >>>> +Please also refer to reset.txt in this directory for common reset
> >>>> +controller binding usage.
> >>>> +
> >>>> +The reset controller registers are part of the system-ctl block on
> >>>> +hi3660 SoC.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: should be
> >>>> +		 "hisilicon,hi3660-reset"
> >>>> +- #reset-cells: 1, see below
> >>>> +- hisi,rst-syscon: phandle of the reset's syscon.
> >>>> +- hisi,reset-bits: Contains the reset control register information
> >>>> +		  Should contain 2 cells for each reset exposed to
> >>>> +		  consumers, defined as:
> >>>> +			Cell #1 : offset from the syscon register base
> >>>> +			Cell #2 : bits position of the control register
> >>>> +
> >>>> +Example:
> >>>> +	iomcu: iomcu at ffd7e000 {
> >>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> >>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>> +	};
> >>>> +
> >>>> +	iomcu_rst: iomcu_rst_controller {
> >>> This should be
> >>> 	iomcu_rst: reset-controller {
> >>>
> >>>> +		compatible = "hisilicon,hi3660-reset";
> >>>> +		#reset-cells = <1>;
> >>>> +		hisi,rst-syscon = <&iomcu>;
> >>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> >>>> +				   0x20 0x10		/* 1: i2c1 */
> >>>> +				   0x20 0x20		/* 2: i2c2 */
> >>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
> >>>> +	};
> >>> The reset lines are controlled through iomcu bits, is there a reason not
> >>> to put the iomcu_rst node inside the iomcu node? That way the
> >>> hisi,rst-syscon property could be removed and the syscon could be
> >>> retrieved via the reset-controller parent node.
> >> iomcu is common registers, controls clock and reset, etc.
> >> So we use syscon, without mapping the registers everywhere.
> >> It is common case in hisilicon, same in hi6220.
> >>
> >> Also the #clock-cells and #reset-cells can not be put in the same node,
> >> if they are both using probe, since reset_probe will not be called.
> >>
> >> So we use hisi,rst-syscon as a general solution.
> > What I meant is this:
> >
> > 	iomcu: iomcu at ffd7e000 {
> > 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> > 		reg = <0x0 0xffd7e000 0x0 0x1000>;
> #clock-cells = <1>;
>
> In my test, if there add #clock-cells = <1>, reset_probe will not be 
> called any more.
> Since clk_probe is called first.
> No matter iomcu_rst is child node or not.

I don't understand this, does the clock driver bind to the iomcu node
using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?

My comment was based only on this reset binding documentation and the
example DT snippet. Could you point me to the clock driver probe code
and show me a more complete part of the hi3660 device tree?

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

On 2016?11?25? 18:25, Philipp Zabel wrote:
> Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
>> On 2016?11?24? 17:50, Philipp Zabel wrote:
>>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
>>>> On 2016?11?24? 17:26, Philipp Zabel wrote:
>>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
>>>>>>
>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>>> ---
>>>>>>     .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>>>>>>     1 file changed, 51 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..250daf2
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>> @@ -0,0 +1,51 @@
>>>>>> +Hisilicon System Reset Controller
>>>>>> +======================================
>>>>>> +
>>>>>> +Please also refer to reset.txt in this directory for common reset
>>>>>> +controller binding usage.
>>>>>> +
>>>>>> +The reset controller registers are part of the system-ctl block on
>>>>>> +hi3660 SoC.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should be
>>>>>> +		 "hisilicon,hi3660-reset"
>>>>>> +- #reset-cells: 1, see below
>>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
>>>>>> +- hisi,reset-bits: Contains the reset control register information
>>>>>> +		  Should contain 2 cells for each reset exposed to
>>>>>> +		  consumers, defined as:
>>>>>> +			Cell #1 : offset from the syscon register base
>>>>>> +			Cell #2 : bits position of the control register
>>>>>> +
>>>>>> +Example:
>>>>>> +	iomcu: iomcu at ffd7e000 {
>>>>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
>>>>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
>>>>>> +	};
>>>>>> +
>>>>>> +	iomcu_rst: iomcu_rst_controller {
>>>>> This should be
>>>>> 	iomcu_rst: reset-controller {
>>>>>
>>>>>> +		compatible = "hisilicon,hi3660-reset";
>>>>>> +		#reset-cells = <1>;
>>>>>> +		hisi,rst-syscon = <&iomcu>;
>>>>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
>>>>>> +				   0x20 0x10		/* 1: i2c1 */
>>>>>> +				   0x20 0x20		/* 2: i2c2 */
>>>>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
>>>>>> +	};
>>>>> The reset lines are controlled through iomcu bits, is there a reason not
>>>>> to put the iomcu_rst node inside the iomcu node? That way the
>>>>> hisi,rst-syscon property could be removed and the syscon could be
>>>>> retrieved via the reset-controller parent node.
>>>> iomcu is common registers, controls clock and reset, etc.
>>>> So we use syscon, without mapping the registers everywhere.
>>>> It is common case in hisilicon, same in hi6220.
>>>>
>>>> Also the #clock-cells and #reset-cells can not be put in the same node,
>>>> if they are both using probe, since reset_probe will not be called.
>>>>
>>>> So we use hisi,rst-syscon as a general solution.
>>> What I meant is this:
>>>
>>> 	iomcu: iomcu at ffd7e000 {
>>> 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
>>> 		reg = <0x0 0xffd7e000 0x0 0x1000>;
>> #clock-cells = <1>;
>>
>> In my test, if there add #clock-cells = <1>, reset_probe will not be
>> called any more.
>> Since clk_probe is called first.
>> No matter iomcu_rst is child node or not.
> I don't understand this, does the clock driver bind to the iomcu node
> using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?

This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
and we have to use probe instead, to make all driver build as modules as 
possible.

For example hi3660.
static struct platform_driver hi3660_clk_driver = {
         .probe          = hi3660_clk_probe,
         .driver         = {
                 .name   = "hi3660-clk",
                 .of_match_table = hi3660_clk_match_table,
         },
};

static int __init hi3660_clk_init(void)
{
         return platform_driver_register(&hi3660_clk_driver);
}
core_initcall(hi3660_clk_init);

And many examples in drivers/clock, just
#grep -rn probe drivers/clk/
drivers/clk/clk-axm5516.c:587:    .probe        = axmclk_probe,

If the parent node happen to be clock, and set

#clock-cells = <1>;
Then clock_probe/hi3660_clk_probe will be called first.
But reset_probe will not be called any more.

Thanks

>
> My comment was based only on this reset binding documentation and the
> example DT snippet. Could you point me to the clock driver probe code
> and show me a more complete part of the hi3660 device tree?
>
> regards
> Philipp
>

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Freitag, den 25.11.2016, 18:42 +0800 schrieb zhangfei:
> 
> On 2016?11?25? 18:25, Philipp Zabel wrote:
> > Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
> >> On 2016?11?24? 17:50, Philipp Zabel wrote:
> >>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> >>>> On 2016?11?24? 17:26, Philipp Zabel wrote:
> >>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
> >>>>>>
> >>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>>>>> ---
> >>>>>>     .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
> >>>>>>     1 file changed, 51 insertions(+)
> >>>>>>     create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..250daf2
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>> @@ -0,0 +1,51 @@
> >>>>>> +Hisilicon System Reset Controller
> >>>>>> +======================================
> >>>>>> +
> >>>>>> +Please also refer to reset.txt in this directory for common reset
> >>>>>> +controller binding usage.
> >>>>>> +
> >>>>>> +The reset controller registers are part of the system-ctl block on
> >>>>>> +hi3660 SoC.
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible: should be
> >>>>>> +		 "hisilicon,hi3660-reset"
> >>>>>> +- #reset-cells: 1, see below
> >>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
> >>>>>> +- hisi,reset-bits: Contains the reset control register information
> >>>>>> +		  Should contain 2 cells for each reset exposed to
> >>>>>> +		  consumers, defined as:
> >>>>>> +			Cell #1 : offset from the syscon register base
> >>>>>> +			Cell #2 : bits position of the control register
> >>>>>> +
> >>>>>> +Example:
> >>>>>> +	iomcu: iomcu at ffd7e000 {
> >>>>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> >>>>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>>>> +	};
> >>>>>> +
> >>>>>> +	iomcu_rst: iomcu_rst_controller {
> >>>>> This should be
> >>>>> 	iomcu_rst: reset-controller {
> >>>>>
> >>>>>> +		compatible = "hisilicon,hi3660-reset";
> >>>>>> +		#reset-cells = <1>;
> >>>>>> +		hisi,rst-syscon = <&iomcu>;
> >>>>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> >>>>>> +				   0x20 0x10		/* 1: i2c1 */
> >>>>>> +				   0x20 0x20		/* 2: i2c2 */
> >>>>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
> >>>>>> +	};
> >>>>> The reset lines are controlled through iomcu bits, is there a reason not
> >>>>> to put the iomcu_rst node inside the iomcu node? That way the
> >>>>> hisi,rst-syscon property could be removed and the syscon could be
> >>>>> retrieved via the reset-controller parent node.
> >>>> iomcu is common registers, controls clock and reset, etc.
> >>>> So we use syscon, without mapping the registers everywhere.
> >>>> It is common case in hisilicon, same in hi6220.
> >>>>
> >>>> Also the #clock-cells and #reset-cells can not be put in the same node,
> >>>> if they are both using probe, since reset_probe will not be called.
> >>>>
> >>>> So we use hisi,rst-syscon as a general solution.
> >>> What I meant is this:
> >>>
> >>> 	iomcu: iomcu at ffd7e000 {
> >>> 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> >>> 		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >> #clock-cells = <1>;
> >>
> >> In my test, if there add #clock-cells = <1>, reset_probe will not be
> >> called any more.
> >> Since clk_probe is called first.
> >> No matter iomcu_rst is child node or not.
> > I don't understand this, does the clock driver bind to the iomcu node
> > using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
> 
> This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
> and we have to use probe instead, to make all driver build as modules as 
> possible.
> 
> For example hi3660.
> static struct platform_driver hi3660_clk_driver = {
>          .probe          = hi3660_clk_probe,
>          .driver         = {
>                  .name   = "hi3660-clk",
>                  .of_match_table = hi3660_clk_match_table,
>          },
> };

hi3660_clk_match_table contains the "hisilicon,hi3660-iomcu" compatible?
If so, you could call
	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
from hi3660_clk_probe instead of using "simple-mfd" to probe the iomcu
node's children.

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

On 2016?11?25? 18:54, Philipp Zabel wrote:
> Am Freitag, den 25.11.2016, 18:42 +0800 schrieb zhangfei:
>> On 2016?11?25? 18:25, Philipp Zabel wrote:
>>> Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
>>>> On 2016?11?24? 17:50, Philipp Zabel wrote:
>>>>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
>>>>>> On 2016?11?24? 17:26, Philipp Zabel wrote:
>>>>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>>>>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
>>>>>>>>
>>>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>>>>> ---
>>>>>>>>      .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>>>>>>>>      1 file changed, 51 insertions(+)
>>>>>>>>      create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..250daf2
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>>>>>>> @@ -0,0 +1,51 @@
>>>>>>>> +Hisilicon System Reset Controller
>>>>>>>> +======================================
>>>>>>>> +
>>>>>>>> +Please also refer to reset.txt in this directory for common reset
>>>>>>>> +controller binding usage.
>>>>>>>> +
>>>>>>>> +The reset controller registers are part of the system-ctl block on
>>>>>>>> +hi3660 SoC.
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible: should be
>>>>>>>> +		 "hisilicon,hi3660-reset"
>>>>>>>> +- #reset-cells: 1, see below
>>>>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
>>>>>>>> +- hisi,reset-bits: Contains the reset control register information
>>>>>>>> +		  Should contain 2 cells for each reset exposed to
>>>>>>>> +		  consumers, defined as:
>>>>>>>> +			Cell #1 : offset from the syscon register base
>>>>>>>> +			Cell #2 : bits position of the control register
>>>>>>>> +
>>>>>>>> +Example:
>>>>>>>> +	iomcu: iomcu at ffd7e000 {
>>>>>>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
>>>>>>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
>>>>>>>> +	};
>>>>>>>> +
>>>>>>>> +	iomcu_rst: iomcu_rst_controller {
>>>>>>> This should be
>>>>>>> 	iomcu_rst: reset-controller {
>>>>>>>
>>>>>>>> +		compatible = "hisilicon,hi3660-reset";
>>>>>>>> +		#reset-cells = <1>;
>>>>>>>> +		hisi,rst-syscon = <&iomcu>;
>>>>>>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
>>>>>>>> +				   0x20 0x10		/* 1: i2c1 */
>>>>>>>> +				   0x20 0x20		/* 2: i2c2 */
>>>>>>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
>>>>>>>> +	};
>>>>>>> The reset lines are controlled through iomcu bits, is there a reason not
>>>>>>> to put the iomcu_rst node inside the iomcu node? That way the
>>>>>>> hisi,rst-syscon property could be removed and the syscon could be
>>>>>>> retrieved via the reset-controller parent node.
>>>>>> iomcu is common registers, controls clock and reset, etc.
>>>>>> So we use syscon, without mapping the registers everywhere.
>>>>>> It is common case in hisilicon, same in hi6220.
>>>>>>
>>>>>> Also the #clock-cells and #reset-cells can not be put in the same node,
>>>>>> if they are both using probe, since reset_probe will not be called.
>>>>>>
>>>>>> So we use hisi,rst-syscon as a general solution.
>>>>> What I meant is this:
>>>>>
>>>>> 	iomcu: iomcu at ffd7e000 {
>>>>> 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
>>>>> 		reg = <0x0 0xffd7e000 0x0 0x1000>;
>>>> #clock-cells = <1>;
>>>>
>>>> In my test, if there add #clock-cells = <1>, reset_probe will not be
>>>> called any more.
>>>> Since clk_probe is called first.
>>>> No matter iomcu_rst is child node or not.
>>> I don't understand this, does the clock driver bind to the iomcu node
>>> using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
>> This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
>> and we have to use probe instead, to make all driver build as modules as
>> possible.
>>
>> For example hi3660.
>> static struct platform_driver hi3660_clk_driver = {
>>           .probe          = hi3660_clk_probe,
>>           .driver         = {
>>                   .name   = "hi3660-clk",
>>                   .of_match_table = hi3660_clk_match_table,
>>           },
>> };
> hi3660_clk_match_table contains the "hisilicon,hi3660-iomcu" compatible?
> If so, you could call
> 	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> from hi3660_clk_probe instead of using "simple-mfd" to probe the iomcu
> node's children.

Not using simple-mfd:

Like
static const struct of_device_id hi3660_clk_match_table[] = {
         { .compatible = "hisilicon,hi3660-iomcu", },
         { }
};
MODULE_DEVICE_TABLE(of, hi3660_clk_match_table);

static int hi3660_clk_probe(struct platform_device *pdev)
{
         struct device *dev = &pdev->dev;
         struct device_node *np = pdev->dev.of_node;
         const struct of_device_id *of_id;
         enum hi3660_clk_type type;

         of_id = of_match_device(hi3660_clk_match_table, dev);
         if (!of_id)
                 return -EINVAL;
~
}

If put iomcu_rst as child node, and set #clock-cells = <1> to iomcu,
then hi3660_clk_probe is called, hi3660_reset_probe will not be called.
So using "hisi,rst-syscon" as pointer does not have the issue.

Thanks

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Freitag, den 25.11.2016, 20:08 +0800 schrieb zhangfei:
> 
> On 2016?11?25? 18:54, Philipp Zabel wrote:
> > Am Freitag, den 25.11.2016, 18:42 +0800 schrieb zhangfei:
> >> On 2016?11?25? 18:25, Philipp Zabel wrote:
> >>> Am Donnerstag, den 24.11.2016, 18:20 +0800 schrieb zhangfei:
> >>>> On 2016?11?24? 17:50, Philipp Zabel wrote:
> >>>>> Am Donnerstag, den 24.11.2016, 17:40 +0800 schrieb zhangfei:
> >>>>>> On 2016?11?24? 17:26, Philipp Zabel wrote:
> >>>>>>> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >>>>>>>> Add DT bindings documentation for hi3660 SoC reset controller.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>>>>>>> ---
> >>>>>>>>      .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
> >>>>>>>>      1 file changed, 51 insertions(+)
> >>>>>>>>      create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>>>> new file mode 100644
> >>>>>>>> index 0000000..250daf2
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>>>>>>> @@ -0,0 +1,51 @@
> >>>>>>>> +Hisilicon System Reset Controller
> >>>>>>>> +======================================
> >>>>>>>> +
> >>>>>>>> +Please also refer to reset.txt in this directory for common reset
> >>>>>>>> +controller binding usage.
> >>>>>>>> +
> >>>>>>>> +The reset controller registers are part of the system-ctl block on
> >>>>>>>> +hi3660 SoC.
> >>>>>>>> +
> >>>>>>>> +Required properties:
> >>>>>>>> +- compatible: should be
> >>>>>>>> +		 "hisilicon,hi3660-reset"
> >>>>>>>> +- #reset-cells: 1, see below
> >>>>>>>> +- hisi,rst-syscon: phandle of the reset's syscon.
> >>>>>>>> +- hisi,reset-bits: Contains the reset control register information
> >>>>>>>> +		  Should contain 2 cells for each reset exposed to
> >>>>>>>> +		  consumers, defined as:
> >>>>>>>> +			Cell #1 : offset from the syscon register base
> >>>>>>>> +			Cell #2 : bits position of the control register
> >>>>>>>> +
> >>>>>>>> +Example:
> >>>>>>>> +	iomcu: iomcu at ffd7e000 {
> >>>>>>>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> >>>>>>>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>>>>>> +	};
> >>>>>>>> +
> >>>>>>>> +	iomcu_rst: iomcu_rst_controller {
> >>>>>>> This should be
> >>>>>>> 	iomcu_rst: reset-controller {
> >>>>>>>
> >>>>>>>> +		compatible = "hisilicon,hi3660-reset";
> >>>>>>>> +		#reset-cells = <1>;
> >>>>>>>> +		hisi,rst-syscon = <&iomcu>;
> >>>>>>>> +		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
> >>>>>>>> +				   0x20 0x10		/* 1: i2c1 */
> >>>>>>>> +				   0x20 0x20		/* 2: i2c2 */
> >>>>>>>> +				   0x20 0x8000000>;	/* 3: i2c6 */
> >>>>>>>> +	};
> >>>>>>> The reset lines are controlled through iomcu bits, is there a reason not
> >>>>>>> to put the iomcu_rst node inside the iomcu node? That way the
> >>>>>>> hisi,rst-syscon property could be removed and the syscon could be
> >>>>>>> retrieved via the reset-controller parent node.
> >>>>>> iomcu is common registers, controls clock and reset, etc.
> >>>>>> So we use syscon, without mapping the registers everywhere.
> >>>>>> It is common case in hisilicon, same in hi6220.
> >>>>>>
> >>>>>> Also the #clock-cells and #reset-cells can not be put in the same node,
> >>>>>> if they are both using probe, since reset_probe will not be called.
> >>>>>>
> >>>>>> So we use hisi,rst-syscon as a general solution.
> >>>>> What I meant is this:
> >>>>>
> >>>>> 	iomcu: iomcu at ffd7e000 {
> >>>>> 		compatible = "hisilicon,hi3660-iomcu", "syscon", "simple-mfd";
> >>>>> 		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >>>> #clock-cells = <1>;
> >>>>
> >>>> In my test, if there add #clock-cells = <1>, reset_probe will not be
> >>>> called any more.
> >>>> Since clk_probe is called first.
> >>>> No matter iomcu_rst is child node or not.
> >>> I don't understand this, does the clock driver bind to the iomcu node
> >>> using CLK_OF_DECLARE_DRIVER(..., "hisilicon,hi3660-iomcu", ...)?
> >> This method:CLK_OF_DECLARE_DRIVER is not prefered in clock,
> >> and we have to use probe instead, to make all driver build as modules as
> >> possible.
> >>
> >> For example hi3660.
> >> static struct platform_driver hi3660_clk_driver = {
> >>           .probe          = hi3660_clk_probe,
> >>           .driver         = {
> >>                   .name   = "hi3660-clk",
> >>                   .of_match_table = hi3660_clk_match_table,
> >>           },
> >> };
> > hi3660_clk_match_table contains the "hisilicon,hi3660-iomcu" compatible?
> > If so, you could call
> > 	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> > from hi3660_clk_probe instead of using "simple-mfd" to probe the iomcu
> > node's children.
> 
> Not using simple-mfd:
> 
> Like
> static const struct of_device_id hi3660_clk_match_table[] = {
>          { .compatible = "hisilicon,hi3660-iomcu", },
>          { }
> };
> MODULE_DEVICE_TABLE(of, hi3660_clk_match_table);
> 
> static int hi3660_clk_probe(struct platform_device *pdev)
> {
>          struct device *dev = &pdev->dev;
>          struct device_node *np = pdev->dev.of_node;
>          const struct of_device_id *of_id;
>          enum hi3660_clk_type type;
> 
>          of_id = of_match_device(hi3660_clk_match_table, dev);
>          if (!of_id)
>                  return -EINVAL;
> ~
> }
> 
> If put iomcu_rst as child node, and set #clock-cells = <1> to iomcu,
> then hi3660_clk_probe is called, hi3660_reset_probe will not be called.

For hi3660_reset_probe to be called, you'll have to call
of_platform_populate to probe the hi3660-iomcu children in this case.

> So using "hisi,rst-syscon" as pointer does not have the issue.

I understand that, it still sounds to me like you are organizing the
device tree around limitations of the current code. Instead the device
tree should be organized to best describe the hardware, and the code
should be adapted to support that.

Of course, if you use the flat DT layout everywhere else, I won't try to
block the reset driver because of this issue. I'm just saying nested
nodes in the DT would better describe the real control flow.

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

On 2016?11?24? 17:26, Philipp Zabel wrote:
> Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
>> Add DT bindings documentation for hi3660 SoC reset controller.
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>> new file mode 100644
>> index 0000000..250daf2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>> @@ -0,0 +1,51 @@
>> +Hisilicon System Reset Controller
>> +======================================
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +The reset controller registers are part of the system-ctl block on
>> +hi3660 SoC.
>> +
>> +Required properties:
>> +- compatible: should be
>> +		 "hisilicon,hi3660-reset"
>> +- #reset-cells: 1, see below
>> +- hisi,rst-syscon: phandle of the reset's syscon.
>> +- hisi,reset-bits: Contains the reset control register information
>> +		  Should contain 2 cells for each reset exposed to
>> +		  consumers, defined as:
>> +			Cell #1 : offset from the syscon register base
>> +			Cell #2 : bits position of the control register
>> +
>> +Example:
>> +	iomcu: iomcu at ffd7e000 {
>> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
>> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
>> +	};
>> +
>> +	iomcu_rst: iomcu_rst_controller {
> This should be
> 	iomcu_rst: reset-controller {
By the way, could I keep the original name?
Since there will be build error if several nodes use the same name.
like:
-               iomcu_rst: iomcu_rst_controller {
+               iomcu_rst: reset-controller {

-               crg_rst: crg_rst_controller {
+               crg_rst: reset-controller {

Thanks

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Freitag, den 25.11.2016, 11:04 +0800 schrieb zhangfei:
> 
> On 2016?11?24? 17:26, Philipp Zabel wrote:
> > Am Mittwoch, den 23.11.2016, 16:07 +0800 schrieb Zhangfei Gao:
> >> Add DT bindings documentation for hi3660 SoC reset controller.
> >>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >> ---
> >>   .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> new file mode 100644
> >> index 0000000..250daf2
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> >> @@ -0,0 +1,51 @@
> >> +Hisilicon System Reset Controller
> >> +======================================
> >> +
> >> +Please also refer to reset.txt in this directory for common reset
> >> +controller binding usage.
> >> +
> >> +The reset controller registers are part of the system-ctl block on
> >> +hi3660 SoC.
> >> +
> >> +Required properties:
> >> +- compatible: should be
> >> +		 "hisilicon,hi3660-reset"
> >> +- #reset-cells: 1, see below
> >> +- hisi,rst-syscon: phandle of the reset's syscon.
> >> +- hisi,reset-bits: Contains the reset control register information
> >> +		  Should contain 2 cells for each reset exposed to
> >> +		  consumers, defined as:
> >> +			Cell #1 : offset from the syscon register base
> >> +			Cell #2 : bits position of the control register
> >> +
> >> +Example:
> >> +	iomcu: iomcu at ffd7e000 {
> >> +		compatible = "hisilicon,hi3660-iomcu", "syscon";
> >> +		reg = <0x0 0xffd7e000 0x0 0x1000>;
> >> +	};
> >> +
> >> +	iomcu_rst: iomcu_rst_controller {
> > This should be
> > 	iomcu_rst: reset-controller {
> By the way, could I keep the original name?
> Since there will be build error if several nodes use the same name.
> like:
> -               iomcu_rst: iomcu_rst_controller {
> +               iomcu_rst: reset-controller {
> 
> -               crg_rst: crg_rst_controller {
> +               crg_rst: reset-controller {

That should not be a problem if they are moved inside the controlling
node:

	iomcu {
		iomcu_rst: reset-controller {
		};
	};

	crg {
		crg_rst: reset-controller {
		};
	};

regards
Philipp

[RFC v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Zhangfei Gao]

On Wed, Nov 23, 2016 at 4:07 PM, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
> Add DT bindings documentation for hi3660 SoC reset controller.
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
>
> diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> new file mode 100644
> index 0000000..250daf2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
> @@ -0,0 +1,51 @@
> +Hisilicon System Reset Controller
> +======================================
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +The reset controller registers are part of the system-ctl block on
> +hi3660 SoC.
> +
> +Required properties:
> +- compatible: should be
> +                "hisilicon,hi3660-reset"
> +- #reset-cells: 1, see below
> +- hisi,rst-syscon: phandle of the reset's syscon.
> +- hisi,reset-bits: Contains the reset control register information
> +                 Should contain 2 cells for each reset exposed to
> +                 consumers, defined as:
> +                       Cell #1 : offset from the syscon register base
> +                       Cell #2 : bits position of the control register
> +
> +Example:
> +       iomcu: iomcu at ffd7e000 {
> +               compatible = "hisilicon,hi3660-iomcu", "syscon";
> +               reg = <0x0 0xffd7e000 0x0 0x1000>;
> +       };
> +
> +       iomcu_rst: iomcu_rst_controller {
> +               compatible = "hisilicon,hi3660-reset";
> +               #reset-cells = <1>;
> +               hisi,rst-syscon = <&iomcu>;
> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> +                                  0x20 0x10            /* 1: i2c1 */
> +                                  0x20 0x20            /* 2: i2c2 */
> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> +       };
> +
> +Specifying reset lines connected to IP modules
> +==============================================
> +example:
> +
> +        i2c0: i2c at ..... {
> +                ...
> +               resets = <&iomcu_rst 0>;
> +                ...
> +        };
> +
> +       i2c1: i2c at ..... {
> +                ...
> +               resets = <&iomcu_rst 1>;
> +                ...
> +        };
> --
> 2.7.4


Sorry, missing cc when send-email.
Help take a look.

Thanks

[RFC V2: PATCH 2/2] reset: hisilicon: add reset-hi3660

[Zhangfei Gao]

Add hi3660 reset driver
Reset offset & bits should be listed in dts
Like:
	iomcu_rst: iomcu_rst_controller {
		compatible = "hisilicon,hi3660-reset";
		#reset-cells = <1>;
		hisi,rst-syscon = <&iomcu>;
		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
				   0x20 0x10		/* 1: i2c1 */
				   0x20 0x20		/* 2: i2c2 */
				   0x20 0x8000000>;	/* 3: i2c6 */
	};

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/reset/hisilicon/Kconfig        |   7 ++
 drivers/reset/hisilicon/Makefile       |   1 +
 drivers/reset/hisilicon/reset-hi3660.c | 144 +++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 drivers/reset/hisilicon/reset-hi3660.c

diff --git a/drivers/reset/hisilicon/Kconfig b/drivers/reset/hisilicon/Kconfig
index 1ff8b0c..10134dc 100644
--- a/drivers/reset/hisilicon/Kconfig
+++ b/drivers/reset/hisilicon/Kconfig
@@ -1,3 +1,10 @@
+config COMMON_RESET_HI3660
+	tristate "Hi3660 Reset Driver"
+	depends on ARCH_HISI || COMPILE_TEST
+	default ARCH_HISI
+	help
+	  Build the Hisilicon Hi3660 reset driver.
+
 config COMMON_RESET_HI6220
 	tristate "Hi6220 Reset Driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/reset/hisilicon/Makefile b/drivers/reset/hisilicon/Makefile
index c932f86..ab8a7bf 100644
--- a/drivers/reset/hisilicon/Makefile
+++ b/drivers/reset/hisilicon/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_COMMON_RESET_HI6220) += hi6220_reset.o
+obj-$(CONFIG_COMMON_RESET_HI3660) += reset-hi3660.o
diff --git a/drivers/reset/hisilicon/reset-hi3660.c b/drivers/reset/hisilicon/reset-hi3660.c
new file mode 100644
index 0000000..3307252
--- /dev/null
+++ b/drivers/reset/hisilicon/reset-hi3660.c
@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) 2016-2017 Linaro Ltd.
+ * Copyright (c) 2016-2017 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+struct hi3660_reset_data {
+	unsigned int off;
+	unsigned int bits;
+};
+
+struct hi3660_reset_controller {
+	struct reset_controller_dev rst;
+	struct regmap *map;
+	const struct hi3660_reset_data *datas;
+};
+
+#define to_hi3660_reset_controller(_rst) \
+	container_of(_rst, struct hi3660_reset_controller, rst)
+
+static int hi3660_reset_program_hw(struct reset_controller_dev *rcdev,
+				   unsigned long idx, bool assert)
+{
+	struct hi3660_reset_controller *rc = to_hi3660_reset_controller(rcdev);
+	const struct hi3660_reset_data *d;
+
+	if (idx >= rcdev->nr_resets)
+		return -EINVAL;
+
+	d = &rc->datas[idx];
+
+	if (assert)
+		return regmap_write(rc->map, d->off, d->bits);
+	else
+		return regmap_write(rc->map, d->off + 4, d->bits);
+}
+
+static int hi3660_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long idx)
+{
+	return hi3660_reset_program_hw(rcdev, idx, true);
+}
+
+static int hi3660_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long idx)
+{
+	return hi3660_reset_program_hw(rcdev, idx, false);
+}
+
+static int hi3660_reset_dev(struct reset_controller_dev *rcdev,
+			    unsigned long idx)
+{
+	int err;
+
+	err = hi3660_reset_assert(rcdev, idx);
+	if (err)
+		return err;
+
+	return hi3660_reset_deassert(rcdev, idx);
+}
+
+static struct reset_control_ops hi3660_reset_ops = {
+	.reset    = hi3660_reset_dev,
+	.assert   = hi3660_reset_assert,
+	.deassert = hi3660_reset_deassert,
+};
+
+static int hi3660_reset_probe(struct platform_device *pdev)
+{
+	struct hi3660_reset_controller *rc;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct hi3660_reset_data *datas;
+	const __be32 *list;
+	int size, nr, i;
+
+	rc = devm_kzalloc(dev, sizeof(*rc), GFP_KERNEL);
+	if (!rc)
+		return -ENOMEM;
+
+	rc->map = syscon_regmap_lookup_by_phandle(np, "hisi,rst-syscon");
+	if (IS_ERR(rc->map)) {
+		dev_err(dev, "failed to get hi3660,rst-syscon\n");
+		return PTR_ERR(rc->map);
+	}
+
+	list = of_get_property(np, "hisi,reset-bits", &size);
+	if (!list || (size / sizeof(*list)) % 2 != 0) {
+		dev_err(dev, "invalid DT reset description\n");
+		return -EINVAL;
+	}
+
+	nr = (size / sizeof(*list)) / 2;
+	datas = devm_kzalloc(dev, nr * sizeof(*datas), GFP_KERNEL);
+	if (!datas)
+		return -ENOMEM;
+
+	for (i = 0; i < nr; i++) {
+		datas[i].off = be32_to_cpup(list++);
+		datas[i].bits = be32_to_cpup(list++);
+	}
+
+	rc->rst.ops = &hi3660_reset_ops,
+	rc->rst.of_node = np;
+	rc->rst.nr_resets = nr;
+	rc->datas = datas;
+
+	return reset_controller_register(&rc->rst);
+}
+
+static const struct of_device_id hi3660_reset_match[] = {
+	{ .compatible = "hisilicon,hi3660-reset", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, hi3660_reset_match);
+
+static struct platform_driver hi3660_reset_driver = {
+	.probe = hi3660_reset_probe,
+	.driver = {
+		.name = "hi3660-reset",
+		.of_match_table = hi3660_reset_match,
+	},
+};
+
+static int __init hi3660_reset_init(void)
+{
+	return platform_driver_register(&hi3660_reset_driver);
+}
+arch_initcall(hi3660_reset_init);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:hi3660-reset");
+MODULE_DESCRIPTION("HiSilicon Hi3660 Reset Driver");
-- 
2.7.4