[1/2] Documentation: dt: reset: Add syscon reset binding

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

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