[RFC v2 4/5] ARM: shmobile: r8a7790: let rst module allow watchdog resets if desired

[RFC v2 4/5] ARM: shmobile: r8a7790: let rst module allow watchdog resets if desired

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 arch/arm/mach-shmobile/setup-rcar-gen2.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
index aa3339258d9c02..39976ae23a3df2 100644
--- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
@@ -28,7 +28,18 @@
 #include "common.h"
 #include "rcar-gen2.h"
 
-#define MODEMR 0xe6160060
+#define WDTRSTCR	0xe6160054
+#define MODEMR		0xe6160060
+
+void __init rcar_gen2_wdt_rst_init(void)
+{
+#if defined(CONFIG_WATCHDOG)
+	void __iomem *p = ioremap_nocache(WDTRSTCR, 4);
+	BUG_ON(!p);
+	iowrite32(0xa55a0000, p);
+	iounmap(p);
+#endif
+}
 
 u32 rcar_gen2_read_mode_pins(void)
 {
@@ -128,6 +139,10 @@ void __init rcar_gen2_timer_init(void)
 #endif /* CONFIG_ARM_ARCH_TIMER */
 
 	rcar_gen2_clocks_init(mode);
+
+	/* allow watchdog timers to trigger reset */
+	rcar_gen2_wdt_rst_init();
+
 	clocksource_of_init();
 }
 
-- 
2.1.4

Re: [RFC v2 4/5] ARM: shmobile: r8a7790: let rst module allow watchdog resets if desired

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 12 October 2015 07:54:35 Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  arch/arm/mach-shmobile/setup-rcar-gen2.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> b/arch/arm/mach-shmobile/setup-rcar-gen2.c index
> aa3339258d9c02..39976ae23a3df2 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -28,7 +28,18 @@
>  #include "common.h"
>  #include "rcar-gen2.h"
> 
> -#define MODEMR 0xe6160060
> +#define WDTRSTCR	0xe6160054
> +#define MODEMR		0xe6160060
> +
> +void __init rcar_gen2_wdt_rst_init(void)
> +{
> +#if defined(CONFIG_WATCHDOG)
> +	void __iomem *p = ioremap_nocache(WDTRSTCR, 4);
> +	BUG_ON(!p);
> +	iowrite32(0xa55a0000, p);
> +	iounmap(p);
> +#endif

That's a bit of a hack. We should aim at removing code from mach-shmobile, not 
adding new code :-) Furthermore it will cause issues with virtualization as 
hypervisors commonly map memory based on DT, the above code will thus generate 
a write access to an unmapped piece of memory as there's no corresponding DT 
node.

Time for a reset driver it seems ;-)

> +}
> 
>  u32 rcar_gen2_read_mode_pins(void)
>  {
> @@ -128,6 +139,10 @@ void __init rcar_gen2_timer_init(void)
>  #endif /* CONFIG_ARM_ARCH_TIMER */
> 
>  	rcar_gen2_clocks_init(mode);
> +
> +	/* allow watchdog timers to trigger reset */
> +	rcar_gen2_wdt_rst_init();
> +
>  	clocksource_of_init();
>  }

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 4/5] ARM: shmobile: r8a7790: let rst module allow watchdog resets if desired

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]


> > +#if defined(CONFIG_WATCHDOG)
> > +	void __iomem *p = ioremap_nocache(WDTRSTCR, 4);
> > +	BUG_ON(!p);
> > +	iowrite32(0xa55a0000, p);
> > +	iounmap(p);
> > +#endif
> 
> That's a bit of a hack. We should aim at removing code from mach-shmobile, not 
> adding new code :-) Furthermore it will cause issues with virtualization as 
> hypervisors commonly map memory based on DT, the above code will thus generate 
> a write access to an unmapped piece of memory as there's no corresponding DT 
> node.

I know. As I said in the cover letter (hint! ;)) it only works for me with
UP on Lager. These arch patches are only to be able to test the driver.
Totally not to be upstreamed.

> Time for a reset driver it seems ;-)

Can of worms for Gen2...

Let's hope it will be easier on Gen3 and we can submit the main driver
upstream this way.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC v2 4/5] ARM: shmobile: r8a7790: let rst module allow watchdog resets if desired

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Oct 12, 2015 at 8:54 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> index aa3339258d9c02..39976ae23a3df2 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -28,7 +28,18 @@
>  #include "common.h"
>  #include "rcar-gen2.h"
>
> -#define MODEMR 0xe6160060
> +#define WDTRSTCR       0xe6160054
> +#define MODEMR         0xe6160060
> +
> +void __init rcar_gen2_wdt_rst_init(void)
> +{
> +#if defined(CONFIG_WATCHDOG)
> +       void __iomem *p = ioremap_nocache(WDTRSTCR, 4);
> +       BUG_ON(!p);
> +       iowrite32(0xa55a0000, p);
> +       iounmap(p);
> +#endif

I think (most of) my comments from "Re: [RFC 4/5] ARM: shmobile: r8a7790: let
rst module allow watchdog resets if desired" are still valid
(http://www.spinics.net/lists/linux-sh/msg39799.html):

| This is dangerous. If the xWDT was left running, the system will be
| restarted soon.
|
| I think clearing the xWDT reset mask should be handled in the WDT driver
| itself, when enabling the watchdog.
|
| As the wiring between WDT and reset controller differs on the various SoCs
| (R-Car Gen2 uses the RST module, R-Mobile APE6 uses the SYSC module),
| I was thinking about representing this into DT, e.g. adding a node
for the RST:
|
|        rst: reset-controller@e6160000 {
|                compatible = "renesas,rst-r8a7791", "syscon";
|                reg = <0 0xe6160000 0 0x64>;
|        };
|
| and adding a link to the RST to the wdt node:
|
|        syscon = <&rst 0>; /* 0 for RWDT, 1 for SWDT */
|
| On APE6 it can become a link to the SYSC instead.
| [...]
| On older SH-Mobile / R-Mobile, the link can be left out, as the reset is
| not configurable on these SoCs, I think.

However, the below is no longer true, as syscon no longer interferes with
another driver for the same device:

| It's slightly more complicated there, as we already have a driver for the
| SYSC on R-Mobile (rmobile-reset), and a device node can't be bound
| by both the syscon and the rmobile-reset driver.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds