[PATCH] mpc512x: Reset module

[Wolfram Sang]

Hi,

On Mon, Sep 07, 2009 at 10:06:01AM +1000, Damien Dusha wrote:
> Dear all,
> 
> I have written a small patch to get the software reset working on the
> mpc5121.  It is similar to the reset driver found in
> arch/powerpc/platforms/83xx/misc.c except it has been modified to look
> for the reset node in the device tree before writing to the reset
> module.

Heh, my patch looks quite similar :)

From: Wolfram Sang <w.sang at pengutronix.de>
Subject: mpc5121: add restart capability

Hack? Not intended for upstream yet.

Signed-off-by: Wolfram Sang <w.sang at pengutronix.de>
---
 arch/powerpc/platforms/512x/mpc5121_generic.c |   32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Index: arch/powerpc/platforms/512x/mpc5121_generic.c
===================================================================
--- arch/powerpc/platforms/512x/mpc5121_generic.c.orig
+++ arch/powerpc/platforms/512x/mpc5121_generic.c
@@ -20,6 +20,7 @@
 #include <asm/ipic.h>
 #include <asm/prom.h>
 #include <asm/time.h>
+#include <asm/mpc512x.h>
 
 #include "mpc512x.h"
 
@@ -49,6 +50,36 @@ static int __init mpc5121_generic_probe(
 	return board[i] != NULL;
 }
 
+static void mpc5121_restart(char *cmd) {
+
+	struct mpc512x_reset_module *rm;
+	struct device_node *rmnode;
+
+	/* HACK: Proably not good from IRQ context */
+
+	rmnode = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-reset");
+	if (!rmnode) {
+		printk(KERN_ERR "Missing 'fsl,mpc5121-reset' "
+						"node in device tree!\n");
+		goto out;
+	}
+
+	rm = of_iomap(rmnode, 0);
+	if (!rm) {
+		printk(KERN_ERR	"Error mapping reset module node!\n");
+		goto out;
+	}
+
+	local_irq_disable();
+
+	/* Unlock register first, then reset */
+	out_be32(&rm->rpr, 0x52535445);
+	out_be32(&rm->rcr, 0x00000002);
+out:
+	while (1) ;
+}
+
 define_machine(mpc5121_generic) {
 	.name			= "MPC5121 generic",
 	.probe			= mpc5121_generic_probe,
@@ -56,4 +89,5 @@ define_machine(mpc5121_generic) {
 	.init_IRQ		= mpc512x_init_IRQ,
 	.get_irq		= ipic_get_irq,
 	.calibrate_decr		= generic_calibrate_decr,
+	.restart		= mpc5121_restart,
 };



There is one reason which I wanted to check before submitting this (alas, no
time :( ). From mpc52xx_common.c:

...
/*
 * This variable is mapped in mpc52xx_map_wdt() and used in mpc52xx_restart().
 * Permanent mapping is required because mpc52xx_restart() can be called
 * from interrupt context while node mapping (which calls ioremap())
 * cannot be used at such point.
 */
static DEFINE_SPINLOCK(mpc52xx_lock);
static struct mpc52xx_gpt __iomem *mpc52xx_wdt;
...

I think this may be needed here also...

> 
> Comments are welcome.  It's my first patch to the mailing list, so
> I'll apologise in advance for any problems with the submission
> procedure.
> 
> Damien.

> diff --git a/arch/powerpc/platforms/512x/mpc5121_ads.c b/arch/powerpc/platforms/512x/mpc5121_ads.c
> index a8976b4..16ca250 100644
> --- a/arch/powerpc/platforms/512x/mpc5121_ads.c
> +++ b/arch/powerpc/platforms/512x/mpc5121_ads.c
> @@ -70,4 +70,5 @@ define_machine(mpc5121_ads) {
>  	.init_IRQ		= mpc5121_ads_init_IRQ,
>  	.get_irq		= ipic_get_irq,
>  	.calibrate_decr		= generic_calibrate_decr,
> +    .restart        = mpc512x_restart,

Please use tabs and not spaces (See CodingStyle).

>  };
> diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platforms/512x/mpc512x.h
> index f4db8a7..d77f0ab 100644
> --- a/arch/powerpc/platforms/512x/mpc512x.h
> +++ b/arch/powerpc/platforms/512x/mpc512x.h
> @@ -14,5 +14,6 @@
>  extern unsigned long mpc512x_find_ips_freq(struct device_node *node);
>  extern void __init mpc512x_init_IRQ(void);
>  extern void __init mpc512x_init_i2c(void);
> +extern void mpc512x_restart(char *cmd);

Um, do we need that?

>  void __init mpc512x_declare_of_platform_devices(void);
>  #endif				/* __MPC512X_H__ */
> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
> index 135fd6b..deddafc 100644
> --- a/arch/powerpc/platforms/512x/mpc512x_shared.c
> +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> @@ -22,6 +22,7 @@
>  #include <asm/prom.h>
>  #include <asm/time.h>
>  
> +#include <asm/mpc512x.h>
>  #include "mpc512x.h"
>  
>  unsigned long
> @@ -89,6 +90,33 @@ void __init mpc512x_init_i2c(void)
>  	}
>  }
>  
> +void mpc512x_restart(char *cmd)
> +{
> +    struct device_node *np;

Again spaces instead of a tab.

> +    struct mpc512x_reset_module *rm;
> +
> +    local_irq_disable();
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-reset");
> +
> +	if (np) {
> +
> +        rm = of_iomap(np, 0);
> +		if (rm) {

Here you used tabs :)

> +
> +            /* Enable software reset "RSTE" (in hex) */
> +            out_be32( &(rm->rpr) , 0x52535445);
> +
> +            /* Set the software hard reset */
> +            out_be32( &(rm->rcr), 0x2);
> +        }
> +	}

While one probably won't see that because the reset comes immediately, the flow
of code may lead to the false assumption that the following error message gets
printed if somebody doesn't have knowledge about this controller.

> +
> +    printk (KERN_EMERG "Error: Unable to map reset module, spinning forever!\n");

KERN_ERR should be enough, I think.

> +    for(;;);
> +}
> +
> +
>  /*
>   * Nodes to do bus probe on, soc and localbus
>   */

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20090907/3c591ad9/attachment.pgp>