[resend,v2:,1/2] dt-bindings: Document the hi3660 reset bindings

Message ID 1480553321-17400-2-git-send-email-zhangfei.gao@linaro.org
State New
Headers show

Commit Message

Zhangfei Gao Dec. 1, 2016, 12:48 a.m.
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

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann Dec. 1, 2016, 12:05 p.m. | #1
On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> +               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@..... {

> +                ...

> +               resets = <&iomcu_rst 0>;

> +                ...

> +        };


I don't really like this approach, since now the information is
in two places. Why not put the data into the reset specifier
directly when it is used?

Also the format seems a little too close to the actual register
layout and could be a little more abstract, using bit numbers instead
of a bitmask and register numbers instead of offsets.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao Dec. 2, 2016, 12:21 a.m. | #2
Hi, Arnd

On 2016年12月01日 20:05, Arnd Bergmann wrote:
> On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

>> +               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@..... {

>> +                ...

>> +               resets = <&iomcu_rst 0>;

>> +                ...

>> +        };

> I don't really like this approach, since now the information is

> in two places. Why not put the data into the reset specifier

> directly when it is used?

Any example, still not understand.
They are consumer and provider.
>

> Also the format seems a little too close to the actual register

> layout and could be a little more abstract, using bit numbers instead

> of a bitmask and register numbers instead of offsets.

We use bit numbers first.
But in the developing process, we found several bits may be required for 
one driver.
And they may not be continuous as the bits may already be occupied.
Directly using offset, we can set several bits together for simple, to 
give more flexibility.
So after discussion, we directly use offset.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Dec. 2, 2016, 12:32 p.m. | #3
On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> Hi, Arnd

> 

> On 2016年12月01日 20:05, Arnd Bergmann wrote:

> > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

> >> +               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@..... {

> >> +                ...

> >> +               resets = <&iomcu_rst 0>;

> >> +                ...

> >> +        };

> > I don't really like this approach, since now the information is

> > in two places. Why not put the data into the reset specifier

> > directly when it is used?

> Any example, still not understand.

> They are consumer and provider.


I mean in the i2c node, have

	i2c0: i2c@..... {
		...
		resets = <&iomcu_rst 0x20 0x8>;
		...
	}

> > Also the format seems a little too close to the actual register

> > layout and could be a little more abstract, using bit numbers instead

> > of a bitmask and register numbers instead of offsets.

> We use bit numbers first.

> But in the developing process, we found several bits may be required for 

> one driver.

> And they may not be continuous as the bits may already be occupied.

> Directly using offset, we can set several bits together for simple, to 

> give more flexibility.

> So after discussion, we directly use offset.


Can you give an example for why this is needed? Is this different
from a device that has multiple reset lines?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel Dec. 2, 2016, 2:10 p.m. | #4
Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:

> > Hi, Arnd

> > 

> > On 2016年12月01日 20:05, Arnd Bergmann wrote:

> > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

> > >> +               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@..... {

> > >> +                ...

> > >> +               resets = <&iomcu_rst 0>;

> > >> +                ...

> > >> +        };

> > > I don't really like this approach, since now the information is

> > > in two places. Why not put the data into the reset specifier

> > > directly when it is used?


From my point of view, with the binding above, all reset controller
register/bit layout information is in a single place and can be easily
compared to a list in the reference manual, whereas with your suggestion
the description of the reset controller register layout is spread
throughout one or even several dtsi files.
Also, since no two reset controllers are exactly the same, we get a
proliferation of different slightly phandle argument meanings.

> > Any example, still not understand.

> > They are consumer and provider.

> 

> I mean in the i2c node, have

> 

> 	i2c0: i2c@..... {

> 		...

> 		resets = <&iomcu_rst 0x20 0x8>;

> 		...

> 	}


There already are a few drivers that use this, and I fear people having
to change their bindings because new flags are needed that have not been
previously thought of.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Dec. 5, 2016, 11:40 p.m. | #5
On Fri, Dec 02, 2016 at 03:10:13PM +0100, Philipp Zabel wrote:
> Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:

> > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:

> > > Hi, Arnd

> > > 

> > > On 2016年12月01日 20:05, Arnd Bergmann wrote:

> > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

> > > >> +               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@..... {

> > > >> +                ...

> > > >> +               resets = <&iomcu_rst 0>;

> > > >> +                ...

> > > >> +        };

> > > > I don't really like this approach, since now the information is

> > > > in two places. Why not put the data into the reset specifier

> > > > directly when it is used?

> 

> From my point of view, with the binding above, all reset controller

> register/bit layout information is in a single place and can be easily

> compared to a list in the reference manual, whereas with your suggestion

> the description of the reset controller register layout is spread

> throughout one or even several dtsi files.


Which can be solved by tools.

> Also, since no two reset controllers are exactly the same, we get a

> proliferation of different slightly phandle argument meanings.


phandle args are supposed to be specific to the phandle it points to. 
Otherwise, we'd never need more than 1 cell and everything could be a 
lookup table.

> 

> > > Any example, still not understand.

> > > They are consumer and provider.

> > 

> > I mean in the i2c node, have

> > 

> > 	i2c0: i2c@..... {

> > 		...

> > 		resets = <&iomcu_rst 0x20 0x8>;

> > 		...

> > 	}

> 

> There already are a few drivers that use this, and I fear people having

> to change their bindings because new flags are needed that have not been

> previously thought of.

> 


Drivers that use what?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao Dec. 6, 2016, 1:10 a.m. | #6
Hi, Arnd


On 2016年12月02日 20:32, Arnd Bergmann wrote:
> On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:

>> Hi, Arnd

>>

>> On 2016年12月01日 20:05, Arnd Bergmann wrote:

>>> On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

>>>> +               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@..... {

>>>> +                ...

>>>> +               resets = <&iomcu_rst 0>;

>>>> +                ...

>>>> +        };

>>> I don't really like this approach, since now the information is

>>> in two places. Why not put the data into the reset specifier

>>> directly when it is used?

>> Any example, still not understand.

>> They are consumer and provider.

> I mean in the i2c node, have

>

> 	i2c0: i2c@..... {

> 		...

> 		resets = <&iomcu_rst 0x20 0x8>;

> 		...

> }

Got it.
There is function of_xlate in reset_controller_dev can parse the dts
when devm_reset_control_get

* @of_xlate: translation function to translate from specifier as found 
in the
*            device tree to id as given to the reset control ops

Will use this instead.

>>> Also the format seems a little too close to the actual register

>>> layout and could be a little more abstract, using bit numbers instead

>>> of a bitmask and register numbers instead of offsets.

>> We use bit numbers first.

>> But in the developing process, we found several bits may be required for

>> one driver.

>> And they may not be continuous as the bits may already be occupied.

>> Directly using offset, we can set several bits together for simple, to

>> give more flexibility.

>> So after discussion, we directly use offset.

> Can you give an example for why this is needed? Is this different

> from a device that has multiple reset lines?

Yes, we can use multiple reset lines, which is also our original method.
But it may have too many reset lines, like pcie driver will have 5 resets.
So just thinking it can be optimized.

However, when using of_xlate, parsing offset & bit to rstc->id (unsigned 
int),
It only support u32, so will use bit numbers again.
rstc_id = rcdev->of_xlate(rcdev, &args);

Will update v3 patch, help take a look.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel Dec. 6, 2016, 1:40 p.m. | #7
Am Freitag, den 02.12.2016, 23:11 +0100 schrieb Arnd Bergmann:
> On Friday, December 2, 2016 3:10:13 PM CET Philipp Zabel wrote:

> > Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:

> > > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:

> > > > On 2016年12月01日 20:05, Arnd Bergmann wrote:

> > > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

> > > > >> +               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@..... {

> > > > >> +                ...

> > > > >> +               resets = <&iomcu_rst 0>;

> > > > >> +                ...

> > > > >> +        };

> > > > > I don't really like this approach, since now the information is

> > > > > in two places. Why not put the data into the reset specifier

> > > > > directly when it is used?

> > 

> > From my point of view, with the binding above, all reset controller

> > register/bit layout information is in a single place and can be easily

> > compared to a list in the reference manual, whereas with your suggestion

> > the description of the reset controller register layout is spread

> > throughout one or even several dtsi files.

> > Also, since no two reset controllers are exactly the same, we get a

> > proliferation of different slightly phandle argument meanings.

> 

> There is no reason for this to be any different from other subsystems

> that all do it the same way: interrupts, gpios, dma, clk, ... all

> define #foo-cells to be used for addressing uniform things,

> and the data is only in the reference, so that the node that

> describes the controller needs no knowledge of what it's being

> used for.


For most of those bindings the knowledge about which register and bit
position(s) correspond to the handles resides in the driver.

> One exception is the case (often on clk bindings) where the register

> layout is anything but uniform 


The register layout is very non-uniform for many reset controllers. Some
share the same register space with some of those clock controllers.

> and every input line has a completely different behavior.


I can't argue that the behavior is non-uniform for the sane reset
controllers though, most of them have just a single bit, for most
of them all reset lines behave the same.

>  For that case, we define our own numbering

> system in the driver and hardcode those tables there.

>

> This reset driver does not seem to belong into that category though.


Yes. From what has been shown so far, it seems that in this case, while
the resets are distributed sparsely, the relative layout (set/clear
registers right next to each other) is uniform.

> Even if it did, we putting information about the controller into

> its own node is redundant as the driver already identifies the

> register layout by the compatible string.


Indeed I would prefer the driver to carry the register layout tables
instead of putting this information into the device tree at all.

> > > > Any example, still not understand.

> > > > They are consumer and provider.

> > > 

> > > I mean in the i2c node, have

> > > 

> > > 	i2c0: i2c@..... {

> > > 		...

> > > 		resets = <&iomcu_rst 0x20 0x8>;

> > > 		...

> > > 	}

> > 

> > There already are a few drivers that use this, and I fear people having

> > to change their bindings because new flags are needed that have not been

> > previously thought of.

> 

> It rarely happens on other subsystems, and the binding can

> always specify different behavior depending on #reset-cells.


I recognize I am biased here. So feel free to ignore this point.

What I'd like to make sure is that we have thought about and are happy
to spread (partial) information about the reset controller register
layout throughout the device tree like this.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel Dec. 6, 2016, 1:40 p.m. | #8
Am Montag, den 05.12.2016, 17:40 -0600 schrieb Rob Herring:
> On Fri, Dec 02, 2016 at 03:10:13PM +0100, Philipp Zabel wrote:

> > Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:

> > > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:

> > > > Hi, Arnd

> > > > 

> > > > On 2016年12月01日 20:05, Arnd Bergmann wrote:

> > > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:

> > > > >> +               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@..... {

> > > > >> +                ...

> > > > >> +               resets = <&iomcu_rst 0>;

> > > > >> +                ...

> > > > >> +        };

> > > > > I don't really like this approach, since now the information is

> > > > > in two places. Why not put the data into the reset specifier

> > > > > directly when it is used?

> > 

> > From my point of view, with the binding above, all reset controller

> > register/bit layout information is in a single place and can be easily

> > compared to a list in the reference manual, whereas with your suggestion

> > the description of the reset controller register layout is spread

> > throughout one or even several dtsi files.

> 

> Which can be solved by tools.


True.

> > Also, since no two reset controllers are exactly the same, we get a

> > proliferation of different slightly phandle argument meanings.

> 

> phandle args are supposed to be specific to the phandle it points to. 

> Otherwise, we'd never need more than 1 cell and everything could be a 

> lookup table.


What I mean is that the clk bindings mostly use <&label index> or
<&label type index> phandles, not <&label register-offset bit-offset>
phandles. Usually the bindings don't spread information about the
register layout of the clock controller throughout the device tree,
because that is contained in the driver, as determined by the compatible
property. I assumed the same should be true for reset controllers, if
possible.

> > > > Any example, still not understand.

> > > > They are consumer and provider.

> > > 

> > > I mean in the i2c node, have

> > > 

> > > 	i2c0: i2c@..... {

> > > 		...

> > > 		resets = <&iomcu_rst 0x20 0x8>;

> > > 		...

> > > 	}

> > 

> > There already are a few drivers that use this, and I fear people having

> > to change their bindings because new flags are needed that have not been

> > previously thought of. 

> 

> Drivers that use what?


Drivers that use <&label register-offset bit-offset> phandles instead of
<&label index> phandles.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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@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@..... {
+                ...
+		resets = <&iomcu_rst 0>;
+                ...
+        };
+
+	i2c1: i2c@..... {
+                ...
+		resets = <&iomcu_rst 1>;
+                ...
+        };