[PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[Tony Lindgren]

We can support the RSTCTRL reset registers on many TI SoCs with
reset-simple.

Note that some devices will also need to check the RSTST bits
for reset reason. Support for these could be possibly added to
the reset controller framework later on.

Cc: Dave Gerlach <d-gerlach@ti.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Nishant Menon <nm@ti.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Suman Anna <s-anna@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:
- Update patch description to mention the unhandled RSTST bits
- Rebase against Linux next

---
 .../devicetree/bindings/reset/ti-rstctrl.txt         | 20 ++++++++++++++++++++
 drivers/reset/Kconfig                                |  3 ++-
 drivers/reset/reset-simple.c                         |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/reset/ti-rstctrl.txt

diff --git a/Documentation/devicetree/bindings/reset/ti-rstctrl.txt b/Documentation/devicetree/bindings/reset/ti-rstctrl.txt
new file mode 100644
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/ti-rstctrl.txt
@@ -0,0 +1,20 @@
+TI RSTCTRL Reset Controller
+
+Required properties:
+- compatible : "ti,rstctrl"
+- reg : Should contain 1 register ranges(address and length)
+- #reset-cells: 1
+
+Example:
+	prm_gfx: prm@1100 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x1100 0x100>;
+
+		gfx_rstctrl: rstctrl@4 {
+			compatible = "ti,rstctrl";
+			reg = <0x4 0x4>;
+			#reset-cells = <1>;
+		};
+	};
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -84,7 +84,7 @@ config RESET_PISTACHIO
 
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
-	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
+	default ARCH_OMAP2PLUS || ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
 	help
 	  This enables a simple reset controller driver for reset lines that
 	  that can be asserted and deasserted by toggling bits in a contiguous,
@@ -95,6 +95,7 @@ config RESET_SIMPLE
 	   - ASPEED BMC SoCs
 	   - RCC reset controller in STM32 MCUs
 	   - Allwinner SoCs
+	   - TI SoCs
 	   - ZTE's zx2967 family
 
 config RESET_SUNXI
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -123,6 +123,7 @@ static const struct of_device_id reset_simple_dt_ids[] = {
 	{ .compatible = "st,stm32-rcc", },
 	{ .compatible = "allwinner,sun6i-a31-clock-reset",
 		.data = &reset_simple_active_low },
+	{ .compatible = "ti,rstctrl", },
 	{ .compatible = "zte,zx296718-reset",
 		.data = &reset_simple_active_low },
 	{ .compatible = "aspeed,ast2400-lpc-reset" },
-- 
2.16.2

Re: [PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[Rob Herring]

On Wed, Mar 07, 2018 at 10:21:43AM -0800, Tony Lindgren wrote:
> We can support the RSTCTRL reset registers on many TI SoCs with
> reset-simple.
> 
> Note that some devices will also need to check the RSTST bits
> for reset reason. Support for these could be possibly added to
> the reset controller framework later on.
> 
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Nishant Menon <nm@ti.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Suman Anna <s-anna@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v1:
> - Update patch description to mention the unhandled RSTST bits
> - Rebase against Linux next
> 
> ---
>  .../devicetree/bindings/reset/ti-rstctrl.txt         | 20 ++++++++++++++++++++
>  drivers/reset/Kconfig                                |  3 ++-
>  drivers/reset/reset-simple.c                         |  1 +
>  3 files changed, 23 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/reset/ti-rstctrl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/ti-rstctrl.txt b/Documentation/devicetree/bindings/reset/ti-rstctrl.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/ti-rstctrl.txt
> @@ -0,0 +1,20 @@
> +TI RSTCTRL Reset Controller
> +
> +Required properties:
> +- compatible : "ti,rstctrl"
> +- reg : Should contain 1 register ranges(address and length)
> +- #reset-cells: 1
> +
> +Example:
> +	prm_gfx: prm@1100 {
> +		compatible = "simple-bus";

What's a PRM?

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x1100 0x100>;

And what else is in this range?

> +
> +		gfx_rstctrl: rstctrl@4 {
> +			compatible = "ti,rstctrl";
> +			reg = <0x4 0x4>;

Anytime I see a single register in DT I worry about scaling. How many of 
these in an SoC?

> +			#reset-cells = <1>;
> +		};
> +	};

Re: [PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[Tony Lindgren]

Hi,

* Rob Herring <robh@kernel.org> [180308 02:49]:
> On Wed, Mar 07, 2018 at 10:21:43AM -0800, Tony Lindgren wrote:
> > +TI RSTCTRL Reset Controller
> > +
> > +Required properties:
> > +- compatible : "ti,rstctrl"
> > +- reg : Should contain 1 register ranges(address and length)
> > +- #reset-cells: 1
> > +
> > +Example:
> > +	prm_gfx: prm@1100 {
> > +		compatible = "simple-bus";
> 
> What's a PRM?

PRM is power and reset manager. There is one instance per
interconnect instance (clockdomain). PRM shows the status of
the connected devices in the interconnect, such as device
context lost and hardware wake-up dependencies. It also
contains a single reset controller register for external
accelerators such as DSP. The reset controller instance then
has 1 - 3 bits for external accelerator sub device resets.
Then there is a reset status register that shows the reset
reason for the external accelerator.

> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges = <0 0x1100 0x100>;
> 
> And what else is in this range?

In PRM, there are also registers for each interconnect device
context lost and wake-up dependencies. We don't have a driver
for that yet, it's handled by the SoC init code currently.

Unlike the binding for reset controller, the binding for
wake-up dependencies and context lost should look similar
binding to the clkctrl clock binding we have. That's because
there are tons of those registers.

> > +
> > +		gfx_rstctrl: rstctrl@4 {
> > +			compatible = "ti,rstctrl";
> > +			reg = <0x4 0x4>;
> 
> Anytime I see a single register in DT I worry about scaling. How many of 
> these in an SoC?

There are not many instances of the reset controller. There
is one register per interconnect instance for external
accelerators, so about 3 - 10 reset controller registers
per SoC.

The reg offset above is wrong BTW, it should be 0x10 instead
of 0x4. So I need to update this patch for that at least.

Regards,

Tony

Re: [PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[Rob Herring]

On Thu, Mar 8, 2018 at 10:02 AM, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Rob Herring <robh@kernel.org> [180308 02:49]:
>> On Wed, Mar 07, 2018 at 10:21:43AM -0800, Tony Lindgren wrote:
>> > +TI RSTCTRL Reset Controller
>> > +
>> > +Required properties:
>> > +- compatible : "ti,rstctrl"
>> > +- reg : Should contain 1 register ranges(address and length)
>> > +- #reset-cells: 1
>> > +
>> > +Example:
>> > +   prm_gfx: prm@1100 {
>> > +           compatible = "simple-bus";
>>
>> What's a PRM?
>
> PRM is power and reset manager. There is one instance per
> interconnect instance (clockdomain). PRM shows the status of
> the connected devices in the interconnect, such as device
> context lost and hardware wake-up dependencies. It also
> contains a single reset controller register for external
> accelerators such as DSP. The reset controller instance then
> has 1 - 3 bits for external accelerator sub device resets.
> Then there is a reset status register that shows the reset
> reason for the external accelerator.
>
>> > +           #address-cells = <1>;
>> > +           #size-cells = <1>;
>> > +           ranges = <0 0x1100 0x100>;
>>
>> And what else is in this range?
>
> In PRM, there are also registers for each interconnect device
> context lost and wake-up dependencies. We don't have a driver
> for that yet, it's handled by the SoC init code currently.

Regardless of having/needing a driver, you should take a stab at doing
the binding at least. It doesn't make sense to do the binding of the
child without doing the parent.

> Unlike the binding for reset controller, the binding for
> wake-up dependencies and context lost should look similar
> binding to the clkctrl clock binding we have. That's because
> there are tons of those registers.
>
>> > +
>> > +           gfx_rstctrl: rstctrl@4 {
>> > +                   compatible = "ti,rstctrl";
>> > +                   reg = <0x4 0x4>;
>>
>> Anytime I see a single register in DT I worry about scaling. How many of
>> these in an SoC?
>
> There are not many instances of the reset controller. There
> is one register per interconnect instance for external
> accelerators, so about 3 - 10 reset controller registers
> per SoC.

Okay, seems a reasonable number.

However, couldn't you just have PRM node(s) and have that register as
a simple reset driver (along with anything else it handles).

Rob

Re: [PATCHv2] reset: ti-rstctrl: use the reset-simple driver

[Tony Lindgren]

* Rob Herring <robh@kernel.org> [180308 22:27]:
> On Thu, Mar 8, 2018 at 10:02 AM, Tony Lindgren <tony@atomide.com> wrote:
> > In PRM, there are also registers for each interconnect device
> > context lost and wake-up dependencies. We don't have a driver
> > for that yet, it's handled by the SoC init code currently.
> 
> Regardless of having/needing a driver, you should take a stab at doing
> the binding at least. It doesn't make sense to do the binding of the
> child without doing the parent.

Sure, we have already partial binding documentation for
PRM at Documentation/devicetree/bindings/arm/omap/prcm.txt.
I'll take a look at updating it for the reset controller.

> > Unlike the binding for reset controller, the binding for
> > wake-up dependencies and context lost should look similar
> > binding to the clkctrl clock binding we have. That's because
> > there are tons of those registers.
> >
> >> > +
> >> > +           gfx_rstctrl: rstctrl@4 {
> >> > +                   compatible = "ti,rstctrl";
> >> > +                   reg = <0x4 0x4>;
> >>
> >> Anytime I see a single register in DT I worry about scaling. How many of
> >> these in an SoC?
> >
> > There are not many instances of the reset controller. There
> > is one register per interconnect instance for external
> > accelerators, so about 3 - 10 reset controller registers
> > per SoC.
> 
> Okay, seems a reasonable number.
> 
> However, couldn't you just have PRM node(s) and have that register as
> a simple reset driver (along with anything else it handles).

We could have a driver for PRM, but I'm not sure if doing a
separate PRM driver helps much here. It seems like reset-simple
already does the job for most part and can be enhanced as
needed :)

The missing piece is how to get the extra pieces of information
to the consumer drivers.. That is the reset status and context
lost status of each device.

Regards,

Tony