Message ID | 1453748564-6429-2-git-send-email-afd@ti.com |
---|---|
State | Changes Requested, archived |
Headers | show |
On Mon, Jan 25, 2016 at 01:02:43PM -0600, Andrew F. Davis wrote: > Add syscon reset controller binding. This will hook to the reset > framework and use syscon/regmap to set reset bits. This allows > reset control of individual SoC subsytems and devices with > memory-mapped reset registers in a common register memory > space. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > [s-anna@ti.com: revise the binding format] > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > .../devicetree/bindings/reset/syscon-reset.txt | 84 ++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt > > diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt > new file mode 100644 > index 0000000..466378a > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt > @@ -0,0 +1,84 @@ > +SysCon Reset Controller > +======================= > + > +Almost all SoCs have hardware modules that require reset control in addition > +to clock and power control for their functionality. The reset control is > +typically provided by means of memory-mapped I/O registers. These registers are > +sometimes a part of a larger register space region implementing various > +functionalities. This register range is best represented as a syscon node to > +allow multiple entities to access their relevant registers in the common > +register space. > + > +A SysCon Reset Controller node defines a device that uses a syscon node > +and provides reset management functionality for various hardware modules > +present on the SoC. This may be one of those cases that is too low level to put into DT. > + > +SysCon Reset Controller Node > +============================ > +Each of the reset provider/controller nodes should have the following > +properties. > + > +Required properties: > +-------------------- > + - compatible : Should be "syscon-reset" > + - syscon : phandle to the syscon node containing the reset registers > + - #reset-cells : Should be 6. Please see the reset consumer node below for > + usage details > + > +SysCon Reset Consumer Nodes > +=========================== > +Each of the reset consumer nodes should have the following properties, > +in addition to their own properties. > + > +Required properties: > +-------------------- > + - resets : A phandle and reset specifier pair, one pair for each reset > + signal that affects the device, or that the device manages. > + The phandle should point to the syscon node containing the > + reset registers, and the reset specifier should have 6 > + cell-values. The reset specifier contains two similar pairs > + of 3 cell-values each, the first of the pair containing the > + reset control register information, and the second of the pair > + containing the reset status register information. The reset > + control and status registers can be same on some devices/SoCs. What if there are no status bits? > + > + Each of the pairs of 3 cell-values should have the following > + values: > + Cell #1 : register offset of the reset control/status > + register from the syscon register base > + Cell #2 : bit shift value for the reset in the respective > + reset control/status register > + Cell #3 : polarity of the reset bit. Should be 1 for resets > + that are asserted when the bit is set, 0 for > + resets that are asserted when the bit is cleared > + > +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for > +common reset controller usage by consumers. > + > + > +Example: > +-------- > +The following example demonstrates a syscon node, the reset controller node > +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 > +Hawking SoC. > + > +/ { > + soc { > + psc: power-sleep-controller@02350000 { > + compatible = "syscon"; > + reg = <0x02350000 0x1000>; > + }; > + > + pscrst: psc-reset { > + compatible = "syscon-reset"; > + syscon = <&psc>; > + #reset-cells = <6>; > + }; Any reason not to make this a child of psc? > + > + dsp0: dsp0 { > + ... > + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; > + ... > + }; > + }; > +}; > -- > 2.7.0 > -- 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
On 01/28/2016 09:22 PM, Rob Herring wrote: > On Mon, Jan 25, 2016 at 01:02:43PM -0600, Andrew F. Davis wrote: >> Add syscon reset controller binding. This will hook to the reset >> framework and use syscon/regmap to set reset bits. This allows >> reset control of individual SoC subsytems and devices with >> memory-mapped reset registers in a common register memory >> space. >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> [s-anna@ti.com: revise the binding format] >> Signed-off-by: Suman Anna <s-anna@ti.com> >> --- >> .../devicetree/bindings/reset/syscon-reset.txt | 84 ++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt >> >> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt >> new file mode 100644 >> index 0000000..466378a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt >> @@ -0,0 +1,84 @@ >> +SysCon Reset Controller >> +======================= >> + >> +Almost all SoCs have hardware modules that require reset control in addition >> +to clock and power control for their functionality. The reset control is >> +typically provided by means of memory-mapped I/O registers. These registers are >> +sometimes a part of a larger register space region implementing various >> +functionalities. This register range is best represented as a syscon node to >> +allow multiple entities to access their relevant registers in the common >> +register space. >> + >> +A SysCon Reset Controller node defines a device that uses a syscon node >> +and provides reset management functionality for various hardware modules >> +present on the SoC. > > This may be one of those cases that is too low level to put into DT. > I can understand the worry about directly representing hardware register functionality in DT, but I believe this case is a useful abstraction, otherwise we end up with reset driver mods for every minor spin of a chip. >> + >> +SysCon Reset Controller Node >> +============================ >> +Each of the reset provider/controller nodes should have the following >> +properties. >> + >> +Required properties: >> +-------------------- >> + - compatible : Should be "syscon-reset" >> + - syscon : phandle to the syscon node containing the reset registers >> + - #reset-cells : Should be 6. Please see the reset consumer node below for >> + usage details >> + >> +SysCon Reset Consumer Nodes >> +=========================== >> +Each of the reset consumer nodes should have the following properties, >> +in addition to their own properties. >> + >> +Required properties: >> +-------------------- >> + - resets : A phandle and reset specifier pair, one pair for each reset >> + signal that affects the device, or that the device manages. >> + The phandle should point to the syscon node containing the >> + reset registers, and the reset specifier should have 6 >> + cell-values. The reset specifier contains two similar pairs >> + of 3 cell-values each, the first of the pair containing the >> + reset control register information, and the second of the pair >> + containing the reset status register information. The reset >> + control and status registers can be same on some devices/SoCs. > > What if there are no status bits? > The last three can be the same as the first three, then if status is read we will check if the corresponding control register bit has been set (which may be set as non-volatile, then the cached result from the last write is read). If there is absolutely no way to read status information then that chip will need a less generic driver solution, but I have not seen any like this. >> + >> + Each of the pairs of 3 cell-values should have the following >> + values: >> + Cell #1 : register offset of the reset control/status >> + register from the syscon register base >> + Cell #2 : bit shift value for the reset in the respective >> + reset control/status register >> + Cell #3 : polarity of the reset bit. Should be 1 for resets >> + that are asserted when the bit is set, 0 for >> + resets that are asserted when the bit is cleared >> + >> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for >> +common reset controller usage by consumers. >> + >> + >> +Example: >> +-------- >> +The following example demonstrates a syscon node, the reset controller node >> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 >> +Hawking SoC. >> + >> +/ { >> + soc { >> + psc: power-sleep-controller@02350000 { >> + compatible = "syscon"; >> + reg = <0x02350000 0x1000>; >> + }; >> + >> + pscrst: psc-reset { >> + compatible = "syscon-reset"; >> + syscon = <&psc>; >> + #reset-cells = <6>; >> + }; > > Any reason not to make this a child of psc? > I have not tested that, but if it gets registered the same I see no reason it couldn't be if someone wanted it organized that way. >> + >> + dsp0: dsp0 { >> + ... >> + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; >> + ... >> + }; >> + }; >> +}; >> -- >> 2.7.0 >> -- 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
Hi Andrew, I like the idea to introduce a generic binding in principle, but it should be able to cover a lot of the cases in the wild. And I'm not sure we know this to be the case yet. Currently we have three syscon users in drivers/reset: reset-berlin, reset-zynq, and sti/reset-syscfg. berlin is special in that it only has trigger bits, and no state. zynq would fit this binding, but it already chose a binding with a single address cell because all its resets are in a contiguous range and the state and control bits are in the same place. sti also chose a single address cell and a logical number to enumerate the resets and to store the actual reset control and status bit position in a table in the driver. Is there a reason not to follow the same approach for ti? This approach of specifying bits in the device tree at the consumer side doesn't allow any error handling in the driver to determine if the bits are in fact valid. Am Montag, den 25.01.2016, 13:02 -0600 schrieb Andrew F. Davis: > Add syscon reset controller binding. This will hook to the reset > framework and use syscon/regmap to set reset bits. This allows > reset control of individual SoC subsytems and devices with > memory-mapped reset registers in a common register memory > space. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > [s-anna@ti.com: revise the binding format] > Signed-off-by: Suman Anna <s-anna@ti.com> > --- > .../devicetree/bindings/reset/syscon-reset.txt | 84 ++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt > > diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt > new file mode 100644 > index 0000000..466378a > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt > @@ -0,0 +1,84 @@ > +SysCon Reset Controller > +======================= > + > +Almost all SoCs have hardware modules that require reset control in addition > +to clock and power control for their functionality. The reset control is > +typically provided by means of memory-mapped I/O registers. These registers are > +sometimes a part of a larger register space region implementing various > +functionalities. This register range is best represented as a syscon node to > +allow multiple entities to access their relevant registers in the common > +register space. So far, so good. > +A SysCon Reset Controller node defines a device that uses a syscon node > +and provides reset management functionality for various hardware modules > +present on the SoC. But this wording is a bit strange when there is no device. > + > +SysCon Reset Controller Node > +============================ > +Each of the reset provider/controller nodes should have the following > +properties. > + > +Required properties: > +-------------------- > + - compatible : Should be "syscon-reset" What if later an erratum turns up and something special needs to be done (delays, special care about other bits in the same register, etc.)? This should always contain a soc specific compatible. > + - syscon : phandle to the syscon node containing the reset registers This is not needed, the reset node can be mad a child of the syscon and then grab the regmap from its parent of_node. > + - #reset-cells : Should be 6. Please see the reset consumer node below for > + usage details This is a binding for single reset bits that are spread throughout the register space. For any syscon that has a few registers of contiguous resets this is rather suboptimal. > +SysCon Reset Consumer Nodes > +=========================== > +Each of the reset consumer nodes should have the following properties, > +in addition to their own properties. > + > +Required properties: > +-------------------- > + - resets : A phandle and reset specifier pair, one pair for each reset > + signal that affects the device, or that the device manages. > + The phandle should point to the syscon node containing the > + reset registers, and the reset specifier should have 6 > + cell-values. The reset specifier contains two similar pairs One pair, then, not two? > + of 3 cell-values each, the first of the pair containing the > + reset control register information, and the second of the pair > + containing the reset status register information. The reset > + control and status registers can be same on some devices/SoCs. > + > + Each of the pairs of 3 cell-values should have the following > + values: > + Cell #1 : register offset of the reset control/status > + register from the syscon register base > + Cell #2 : bit shift value for the reset in the respective > + reset control/status register > + Cell #3 : polarity of the reset bit. Should be 1 for resets > + that are asserted when the bit is set, 0 for > + resets that are asserted when the bit is cleared The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using numbers in the gpio phandle bindings in the beginning has caused a lot of problems over time. Do you really have varying polarity across the resets? > +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for > +common reset controller usage by consumers. > + > +Example: > +-------- > +The following example demonstrates a syscon node, the reset controller node > +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 > +Hawking SoC. > + > +/ { > + soc { > + psc: power-sleep-controller@02350000 { > + compatible = "syscon"; Add "simple-mfd", and then ... > + reg = <0x02350000 0x1000>; > + }; > + > + pscrst: psc-reset { > + compatible = "syscon-reset"; > + syscon = <&psc>; > + #reset-cells = <6>; > + }; ... psc-reset can be put inside power-sleep-controller, and the syscon property can be removed. > + dsp0: dsp0 { > + ... > + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; This sounds a bit error prone and rather verbose for all controllers that don't have control and status bits peppered randomly around the register space. Personally, I'd prefer if you chose to take the sti approach and maybe share code with reset-syscfg. 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
On 02/02/2016 10:44 AM, Philipp Zabel wrote: > Hi Andrew, > > I like the idea to introduce a generic binding in principle, but it > should be able to cover a lot of the cases in the wild. And I'm not sure > we know this to be the case yet. > > Currently we have three syscon users in drivers/reset: reset-berlin, > reset-zynq, and sti/reset-syscfg. > berlin is special in that it only has trigger bits, and no state. > zynq would fit this binding, but it already chose a binding with a > single address cell because all its resets are in a contiguous range and > the state and control bits are in the same place. > sti also chose a single address cell and a logical number to enumerate > the resets and to store the actual reset control and status bit position > in a table in the driver. Is there a reason not to follow the same > approach for ti? The number of reset-able modules is rather large, and we would have to have an entry for them, and then a table of them for each chip, I would like to avoid this as it may become unmaintainable with the number of devices we would like to support. We currently only need to reset one module this way currently anyway, we will be moving away from toggling bits from the host side to perform resets, but rather ask a power management controller to perform the reset for us (I also have a reset driver that communicates with this controller that I will post when the rest of the needed framework is upstreamed). So, for TI, this syscon based driver will probably mostly be used for compatibility with older SoCs that do not support the management controller, allowing new device drivers to use the reset framework and still function with older SoCs. > This approach of specifying bits in the device tree at > the consumer side doesn't allow any error handling in the driver to > determine if the bits are in fact valid. > Yes, that is lost by not having a table of all valid resets, but this would be like many DT driver/node that allow registers/addresses to be specified and then write/read from those. > Am Montag, den 25.01.2016, 13:02 -0600 schrieb Andrew F. Davis: >> Add syscon reset controller binding. This will hook to the reset >> framework and use syscon/regmap to set reset bits. This allows >> reset control of individual SoC subsytems and devices with >> memory-mapped reset registers in a common register memory >> space. >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> [s-anna@ti.com: revise the binding format] >> Signed-off-by: Suman Anna <s-anna@ti.com> >> --- >> .../devicetree/bindings/reset/syscon-reset.txt | 84 ++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/syscon-reset.txt >> >> diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt >> new file mode 100644 >> index 0000000..466378a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt >> @@ -0,0 +1,84 @@ >> +SysCon Reset Controller >> +======================= >> + >> +Almost all SoCs have hardware modules that require reset control in addition >> +to clock and power control for their functionality. The reset control is >> +typically provided by means of memory-mapped I/O registers. These registers are >> +sometimes a part of a larger register space region implementing various >> +functionalities. This register range is best represented as a syscon node to >> +allow multiple entities to access their relevant registers in the common >> +register space. > > So far, so good. > >> +A SysCon Reset Controller node defines a device that uses a syscon node >> +and provides reset management functionality for various hardware modules >> +present on the SoC. > > But this wording is a bit strange when there is no device. > Agreed, I'll clarify this. >> + >> +SysCon Reset Controller Node >> +============================ >> +Each of the reset provider/controller nodes should have the following >> +properties. >> + >> +Required properties: >> +-------------------- >> + - compatible : Should be "syscon-reset" > > What if later an erratum turns up and something special needs to be done > (delays, special care about other bits in the same register, etc.)? > This should always contain a soc specific compatible. > In this case a specific reset driver would be needed for that device, this driver only intends to cover the more traditional base cases. >> + - syscon : phandle to the syscon node containing the reset registers > > This is not needed, the reset node can be mad a child of the syscon and > then grab the regmap from its parent of_node. > Rob also made this suggestion, so I'll change this as it seems to be the way the community would like to move forward with syscon based drivers. >> + - #reset-cells : Should be 6. Please see the reset consumer node below for >> + usage details > > > This is a binding for single reset bits that are spread throughout the > register space. For any syscon that has a few registers of contiguous > resets this is rather suboptimal. > Yes, this is only intended for when a few resets need controlling out of a large reset space without having to directly encode the reset information into the device driver for that hardware module. >> +SysCon Reset Consumer Nodes >> +=========================== >> +Each of the reset consumer nodes should have the following properties, >> +in addition to their own properties. >> + >> +Required properties: >> +-------------------- >> + - resets : A phandle and reset specifier pair, one pair for each reset >> + signal that affects the device, or that the device manages. >> + The phandle should point to the syscon node containing the >> + reset registers, and the reset specifier should have 6 >> + cell-values. The reset specifier contains two similar pairs > > One pair, then, not two? > Err, two similar triples, may make more sense, will fix. >> + of 3 cell-values each, the first of the pair containing the >> + reset control register information, and the second of the pair >> + containing the reset status register information. The reset >> + control and status registers can be same on some devices/SoCs. >> + >> + Each of the pairs of 3 cell-values should have the following >> + values: >> + Cell #1 : register offset of the reset control/status >> + register from the syscon register base >> + Cell #2 : bit shift value for the reset in the respective >> + reset control/status register >> + Cell #3 : polarity of the reset bit. Should be 1 for resets >> + that are asserted when the bit is set, 0 for >> + resets that are asserted when the bit is cleared > > The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using > numbers in the gpio phandle bindings in the beginning has caused a lot > of problems over time. That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work? > Do you really have varying polarity across the resets? > Not on the part I'm using, but this should keep things generic. The alternative would be to set the polarity per reset controller node, then if a system has both it could have two controllers and the consumers would have a phandle to the correct one, then all consumers would only need 4 instead of 6 args. Actually now that I think about it, this is probably the way to go as most systems I would imagine only have one polarity and it still can work for systems that do have both. I think I'll make this change. >> +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for >> +common reset controller usage by consumers. >> + >> +Example: >> +-------- >> +The following example demonstrates a syscon node, the reset controller node >> +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 >> +Hawking SoC. >> + >> +/ { >> + soc { >> + psc: power-sleep-controller@02350000 { >> + compatible = "syscon"; > > Add "simple-mfd", and then ... > >> + reg = <0x02350000 0x1000>; >> + }; >> + >> + pscrst: psc-reset { >> + compatible = "syscon-reset"; >> + syscon = <&psc>; >> + #reset-cells = <6>; >> + }; > > ... psc-reset can be put inside power-sleep-controller, and the syscon > property can be removed. > Agreed, will change. >> + dsp0: dsp0 { >> + ... >> + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; > > This sounds a bit error prone and rather verbose for all controllers > that don't have control and status bits peppered randomly around the > register space. > I was also thinking about adding the ability to have only one set of args for control, then we just return ENOTSUPP when asked for status when only the control register is provided. With the above polarity change, we end up allowing: resets = <&pscrst 0xa3c 8>; when appropriate. This would cover many common use-cases and keep the framework clean for unique case drivers when needed. It would eliminate the need for many reset-berlin like drivers that only differentiate themselves in trivial ways, like offsets/polarity, etc.. Thanks, Andrew -- 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
Hi Andrew, Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis: [...] > > sti also chose a single address cell and a logical number to enumerate > > the resets and to store the actual reset control and status bit position > > in a table in the driver. Is there a reason not to follow the same > > approach for ti? > > The number of reset-able modules is rather large, and we would have to > have an entry for them, and then a table of them for each chip, I > would like to avoid this as it may become unmaintainable with the number > of devices we would like to support. Maybe I underestimate the amount of work necessary to translate the register bit positions from the reference manuals into tables in a driver (after all the imx-src driver that started this framework only has 5 reset controls registered with it), but to me this doesn't sound like a very strong argument. > We currently only need to reset one module this way currently anyway, we > will be moving away from toggling bits from the host side to perform resets, > but rather ask a power management controller to perform the reset for us > (I also have a reset driver that communicates with this controller that I > will post when the rest of the needed framework is upstreamed). So, for TI, > this syscon based driver will probably mostly be used for compatibility with > older SoCs that do not support the management controller, allowing new device > drivers to use the reset framework and still function with older SoCs. So this will only be used for a few legacy devices? I'd probably be less hesitant if you proposed this as some ti chip specific binding, as right now I just don't expect that many other devices to use this supposedly generic binding. > > This approach of specifying bits in the device tree at > > the consumer side doesn't allow any error handling in the driver to > > determine if the bits are in fact valid. > > Yes, that is lost by not having a table of all valid resets, but this would > be like many DT driver/node that allow registers/addresses to be specified > and then write/read from those. True. Still, I'd argue that having a list of registers and bit-shifts in a single place (whether that is in the driver or in the reset controller node), that can be easily checked against a manual. On the other hand sprinkling these values around the device tree at the consumer sites makes this much harder to review. [...] > >> +Required properties: > >> +-------------------- > >> + - compatible : Should be "syscon-reset" > > > > What if later an erratum turns up and something special needs to be done > > (delays, special care about other bits in the same register, etc.)? > > This should always contain a soc specific compatible. > > In this case a specific reset driver would be needed for that device, this > driver only intends to cover the more traditional base cases. And for that to work in a backwards compatible way, you need to have the SoC specific compatible value already in the device tree. [...] > > This is a binding for single reset bits that are spread throughout the > > register space. For any syscon that has a few registers of contiguous > > resets this is rather suboptimal. > > Yes, this is only intended for when a few resets need controlling out of > a large reset space without having to directly encode the reset information > into the device driver for that hardware module. How about if we came up with a way to encode the bit fields in the reset controller node? [...] > > The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using > > numbers in the gpio phandle bindings in the beginning has caused a lot > > of problems over time. > > That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work? Yes, this is better. > > Do you really have varying polarity across the resets? > > Not on the part I'm using, but this should keep things generic. We already established that this binding is not generic enough for most of the current cases, so I'm not convinced it is valuable to complicate it for some theoretical case. > The alternative would be to set the polarity per reset controller node, then > if a system has both it could have two controllers and the consumers would > have a phandle to the correct one, then all consumers would only need 4 > instead of 6 args. Actually now that I think about it, this is probably the > way to go as most systems I would imagine only have one polarity and it still > can work for systems that do have both. I think I'll make this change. How about going one more step and also moving the register and bit-shift description into a property of the reset controller node? > >> + dsp0: dsp0 { > >> + ... > >> + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; > > > > This sounds a bit error prone and rather verbose for all controllers > > that don't have control and status bits peppered randomly around the > > register space. > > I was also thinking about adding the ability to have only one set of args > for control, then we just return ENOTSUPP when asked for status when only > the control register is provided. Yes, that should work. > With the above polarity change, we end up allowing: > > resets = <&pscrst 0xa3c 8>; Would this be a reset controller with no control bit, but just a trigger that is triggered when the bit is set? (or cleared?). > when appropriate. This would cover many common use-cases and keep the > framework clean for unique case drivers when needed. It would eliminate > the need for many reset-berlin like drivers that only differentiate > themselves in trivial ways, like offsets/polarity, etc.. That's not a good example. reset-berlin needs a bit to be set to trigger the reset pulse, and doesn't have support for manual assert/deassert. Also according to the driver it doesn't signal reset status, so there is a fixed delay needed. Honestly, I expect most drivers in the near future to fall into two categories: either they need special attention, or they are not a good fit for this binding because all the resets are (at least mostly) contiguous. 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
On 02/04/2016 09:49 AM, Philipp Zabel wrote: > Hi Andrew, > > Am Dienstag, den 02.02.2016, 13:25 -0600 schrieb Andrew F. Davis: > [...] >>> sti also chose a single address cell and a logical number to enumerate >>> the resets and to store the actual reset control and status bit position >>> in a table in the driver. Is there a reason not to follow the same >>> approach for ti? >> >> The number of reset-able modules is rather large, and we would have to >> have an entry for them, and then a table of them for each chip, I >> would like to avoid this as it may become unmaintainable with the number >> of devices we would like to support. > > Maybe I underestimate the amount of work necessary to translate the > register bit positions from the reference manuals into tables in a > driver (after all the imx-src driver that started this framework only > has 5 reset controls registered with it), but to me this doesn't sound > like a very strong argument. > You're right it's not a pain for a single device, in fact I think we only need 2 or 3 resets currently for any given device, I was more concerned about the number of different devices leading to hard to maintain sets of tables. But I concede this isn't the best argument, I'd just like as little chip layout specific information in the driver as possible. >> We currently only need to reset one module this way currently anyway, we >> will be moving away from toggling bits from the host side to perform resets, >> but rather ask a power management controller to perform the reset for us >> (I also have a reset driver that communicates with this controller that I >> will post when the rest of the needed framework is upstreamed). So, for TI, >> this syscon based driver will probably mostly be used for compatibility with >> older SoCs that do not support the management controller, allowing new device >> drivers to use the reset framework and still function with older SoCs. > > So this will only be used for a few legacy devices? I'd probably be less > hesitant if you proposed this as some ti chip specific binding, as right > now I just don't expect that many other devices to use this supposedly > generic binding. > I'll probably do that as a last resort if we cant get something more useful from this driver. >>> This approach of specifying bits in the device tree at >>> the consumer side doesn't allow any error handling in the driver to >>> determine if the bits are in fact valid. >> >> Yes, that is lost by not having a table of all valid resets, but this would >> be like many DT driver/node that allow registers/addresses to be specified >> and then write/read from those. > > True. Still, I'd argue that having a list of registers and bit-shifts in > a single place (whether that is in the driver or in the reset controller > node), that can be easily checked against a manual. On the other hand > sprinkling these values around the device tree at the consumer sites > makes this much harder to review. > Hmmm, I wouldn't be opposed to that, it's suggested a couple times below, so I might see if that works well. I've worked out a little example, so before I implement these changes, would something like this be more acceptable?: (child node of syscon node) reset: reset { compatible = "syscon-reset"; #reset-cells = <1>; #address-cells = <1>; #size-cells = <0>; dsp@0 { reg = <0>; control = <0xa44 8 RESET_ASSERT_SET>; }; imu@1 { reg = <1>; control = <0xa46 7 RESET_ASSERT_SET>; status = <0x844 7 RESET_ASSERT_CLEAR>; toggle-only; }; }; then consumers could just resets = <&reset 0>, <&reset 1>; resest-names = "dsp", "imu"; like normal. > [...] >>>> +Required properties: >>>> +-------------------- >>>> + - compatible : Should be "syscon-reset" >>> >>> What if later an erratum turns up and something special needs to be done >>> (delays, special care about other bits in the same register, etc.)? >>> This should always contain a soc specific compatible. >> >> In this case a specific reset driver would be needed for that device, this >> driver only intends to cover the more traditional base cases. > > And for that to work in a backwards compatible way, you need to have the > SoC specific compatible value already in the device tree. > Then it can be added, "syscon-reset" would just be the generic fall-back when no other more specific driver matches. This is already the case though, or are you saying you would like the example DT to contain one? > [...] >>> This is a binding for single reset bits that are spread throughout the >>> register space. For any syscon that has a few registers of contiguous >>> resets this is rather suboptimal. >> >> Yes, this is only intended for when a few resets need controlling out of >> a large reset space without having to directly encode the reset information >> into the device driver for that hardware module. > > How about if we came up with a way to encode the bit fields in the reset > controller node? > Above. > [...] >>> The polarity should have some RESET_ACTIVE_HIGH/LOW #define. Using >>> numbers in the gpio phandle bindings in the beginning has caused a lot >>> of problems over time. >> >> That works, I'll add these. Something like RESET_ASSERT_{SET,CLEAR} work? > > Yes, this is better. > >>> Do you really have varying polarity across the resets? >> >> Not on the part I'm using, but this should keep things generic. > > We already established that this binding is not generic enough for most > of the current cases, so I'm not convinced it is valuable to complicate > it for some theoretical case. > >> The alternative would be to set the polarity per reset controller node, then >> if a system has both it could have two controllers and the consumers would >> have a phandle to the correct one, then all consumers would only need 4 >> instead of 6 args. Actually now that I think about it, this is probably the >> way to go as most systems I would imagine only have one polarity and it still >> can work for systems that do have both. I think I'll make this change. > > How about going one more step and also moving the register and bit-shift > description into a property of the reset controller node? > Above. >>>> + dsp0: dsp0 { >>>> + ... >>>> + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; >>> >>> This sounds a bit error prone and rather verbose for all controllers >>> that don't have control and status bits peppered randomly around the >>> register space. >> >> I was also thinking about adding the ability to have only one set of args >> for control, then we just return ENOTSUPP when asked for status when only >> the control register is provided. > > Yes, that should work. > >> With the above polarity change, we end up allowing: >> >> resets = <&pscrst 0xa3c 8>; > > Would this be a reset controller with no control bit, but just a trigger > that is triggered when the bit is set? (or cleared?). > Should be addressed by above binding change. >> when appropriate. This would cover many common use-cases and keep the >> framework clean for unique case drivers when needed. It would eliminate >> the need for many reset-berlin like drivers that only differentiate >> themselves in trivial ways, like offsets/polarity, etc.. > > That's not a good example. reset-berlin needs a bit to be set to trigger > the reset pulse, and doesn't have support for manual assert/deassert. > Also according to the driver it doesn't signal reset status, so there is > a fixed delay needed. > Honestly, I expect most drivers in the near future to fall into two > categories: either they need special attention, or they are not a good > fit for this binding because all the resets are (at least mostly) > contiguous. > I'm thinking the same, but at least for the devices I'm working with, this could still be a useful thing to have. Thanks, Andrew > 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
diff --git a/Documentation/devicetree/bindings/reset/syscon-reset.txt b/Documentation/devicetree/bindings/reset/syscon-reset.txt new file mode 100644 index 0000000..466378a --- /dev/null +++ b/Documentation/devicetree/bindings/reset/syscon-reset.txt @@ -0,0 +1,84 @@ +SysCon Reset Controller +======================= + +Almost all SoCs have hardware modules that require reset control in addition +to clock and power control for their functionality. The reset control is +typically provided by means of memory-mapped I/O registers. These registers are +sometimes a part of a larger register space region implementing various +functionalities. This register range is best represented as a syscon node to +allow multiple entities to access their relevant registers in the common +register space. + +A SysCon Reset Controller node defines a device that uses a syscon node +and provides reset management functionality for various hardware modules +present on the SoC. + +SysCon Reset Controller Node +============================ +Each of the reset provider/controller nodes should have the following +properties. + +Required properties: +-------------------- + - compatible : Should be "syscon-reset" + - syscon : phandle to the syscon node containing the reset registers + - #reset-cells : Should be 6. Please see the reset consumer node below for + usage details + +SysCon Reset Consumer Nodes +=========================== +Each of the reset consumer nodes should have the following properties, +in addition to their own properties. + +Required properties: +-------------------- + - resets : A phandle and reset specifier pair, one pair for each reset + signal that affects the device, or that the device manages. + The phandle should point to the syscon node containing the + reset registers, and the reset specifier should have 6 + cell-values. The reset specifier contains two similar pairs + of 3 cell-values each, the first of the pair containing the + reset control register information, and the second of the pair + containing the reset status register information. The reset + control and status registers can be same on some devices/SoCs. + + Each of the pairs of 3 cell-values should have the following + values: + Cell #1 : register offset of the reset control/status + register from the syscon register base + Cell #2 : bit shift value for the reset in the respective + reset control/status register + Cell #3 : polarity of the reset bit. Should be 1 for resets + that are asserted when the bit is set, 0 for + resets that are asserted when the bit is cleared + +Please also refer to Documentation/devicetree/bindings/reset/reset.txt for +common reset controller usage by consumers. + + +Example: +-------- +The following example demonstrates a syscon node, the reset controller node +using the syscon node, and a consumer (a DSP device) on the TI Keystone 2 +Hawking SoC. + +/ { + soc { + psc: power-sleep-controller@02350000 { + compatible = "syscon"; + reg = <0x02350000 0x1000>; + }; + + pscrst: psc-reset { + compatible = "syscon-reset"; + syscon = <&psc>; + #reset-cells = <6>; + }; + + dsp0: dsp0 { + ... + resets = <&pscrst 0xa3c 8 0 0x83c 8 0>; + ... + }; + }; +};