| Message ID | 1480553321-17400-2-git-send-email-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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. One exception is the case (often on clk bindings) where the register layout is anything but uniform and every input line has a completely different behavior. 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. 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. > > > 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. Arnd
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
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
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
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
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>; + ... + };
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