[PATCH v4]: Fix e500 v2 core reboot bug

[PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

From: Roy Zang <tie-fei.zang@freescale.com>

Fix the reset bug on 8548CDS board.
>From MPC8548CDS with e500 v2 core, a new reset control register is added.
This register is used for the cpu reset.

Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
Please ignore all the previous ones :-(
Segher, do you have any comment?
If yes, I will revise again; if No, pick it up!
Thanks!

 arch/powerpc/boot/dts/mpc8548cds.dts |    6 ++++++
 arch/powerpc/platforms/85xx/misc.c   |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index ad96381..0550a3c 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -177,6 +177,12 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		global-utilities@e0000 {	//global utilities reg
+			compatible = "fsl,mpc8548-guts";
+			reg = <e0000 1000>;
+			fsl,has-rstcr;
+		};
+
 		pci1: pci@8000 {
 			interrupt-map-mask = <1f800 0 0 7>;
 			interrupt-map = <
diff --git a/arch/powerpc/platforms/85xx/misc.c b/arch/powerpc/platforms/85xx/misc.c
index 3e62fcb..8286393 100644
--- a/arch/powerpc/platforms/85xx/misc.c
+++ b/arch/powerpc/platforms/85xx/misc.c
@@ -13,11 +13,43 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/prom.h>
+#include <sysdev/fsl_soc.h>
+
+static __be32 __iomem *rstcr;
 
 extern void abort(void);
 
+static int __init mpc85xx_rstcr(void)
+{
+	struct device_node *np;
+	np = of_find_node_by_name(NULL, "global-utilities");
+	if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
+		const u32 *prop = of_get_property(np, "reg", NULL);
+		if (prop) {
+			/* map reset control register
+			 * 0xE00B0 is offset of reset control register
+			 */
+			rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
+			if (!rstcr)
+				printk (KERN_EMERG "Error: reset control \
+					register not mapped, spinning!\n");
+		}
+	} else
+		printk (KERN_INFO "rstcr compatible register does not exist!\n");
+	if (np)
+		of_node_put(np);
+	return 0;
+}
+
+arch_initcall(mpc85xx_rstcr);
+
 void mpc85xx_restart(char *cmd)
 {
 	local_irq_disable();
+	if (rstcr)
+		/* set reset control register */
+		out_be32(rstcr, 0x2);	/* HRESET_REQ */
 	abort();
 }
-- 
1.5.1

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Segher Boessenkool]

> Fix the reset bug on 8548CDS board.

> Segher, do you have any comment?

This one looks fine, only some cosmetics left :-)

> If yes, I will revise again; if No, pick it up!

Well it's not me who picks things up around here...

> +				printk (KERN_EMERG "Error: reset control \
> +					register not mapped, spinning!\n");

...(... "bla bla bla "
	"bla bla bla\n");

instead?  Or there'll be a lot of whitespace in your string.
I don't see anything spinning like the warning says, btw (or
it must be something in abort()?)


Segher

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

On Wed, 2007-06-13 at 15:03, Segher Boessenkool wrote:
> > Fix the reset bug on 8548CDS board.
> 
> > Segher, do you have any comment?
> 
> This one looks fine, only some cosmetics left :-)
> 
> > If yes, I will revise again; if No, pick it up!
> 
> Well it's not me who picks things up around here...
I know, it's Paul or Kumar! I just like to hear your valuable comment...
> 
> > +                             printk (KERN_EMERG "Error: reset
> control \
> > +                                     register not mapped,
> spinning!\n");
> 
> ...(... "bla bla bla "
>         "bla bla bla\n");
> 
> instead?  Or there'll be a lot of whitespace in your string.
I just do not know the difference of these two style.
> I don't see anything spinning like the warning says, btw (or
> it must be something in abort()?)
You can find the similar thing in 83xx/mis.c
[snip]
	} else {
		printk (KERN_EMERG "Error: Restart registers not mapped,
spinning!\n");
	}
[snip]
Roy

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Segher Boessenkool]

>>> +                             printk (KERN_EMERG "Error: reset
>> control \
>>> +                                     register not mapped,
>> spinning!\n");
>>
>> ...(... "bla bla bla "
>>         "bla bla bla\n");
>>
>> instead?  Or there'll be a lot of whitespace in your string.
> I just do not know the difference of these two style.

Your code puts a bunch of extra whitespace in the string (all
the tabs before "register not mapped").  Two strings after
each other (like in my example) are just pasted together.

There is no real reason to break the string in two anyway, it
is fine to have a more-than-80-char line in printk messages.

>> I don't see anything spinning like the warning says, btw (or
>> it must be something in abort()?)
> You can find the similar thing in 83xx/mis.c
> [snip]
> 	} else {
> 		printk (KERN_EMERG "Error: Restart registers not mapped,
> spinning!\n");
> 	}
> [snip]

You shouldn't have snipped, right after that it says

         for (;;) ;


Segher

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

On Wed, 2007-06-13 at 15:26, Segher Boessenkool wrote:
> >>> +                             printk (KERN_EMERG "Error: reset
> >> control \
> >>> +                                     register not mapped,
> >> spinning!\n");
> >>
> >> ...(... "bla bla bla "
> >>         "bla bla bla\n");
> >>
> >> instead?  Or there'll be a lot of whitespace in your string.
> > I just do not know the difference of these two style.
> 
> Your code puts a bunch of extra whitespace in the string (all
> the tabs before "register not mapped").  Two strings after
> each other (like in my example) are just pasted together.
> 
> There is no real reason to break the string in two anyway, it
> is fine to have a more-than-80-char line in printk messages.
> 
I separate it, because I am afraid I will be  challenged by 80 char
column rule.

Please check the CodeStyle file and its example:

Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are
placed
substantially to the right. The same applies to function headers with a
long
argument list. Long strings are as well broken into shorter strings.
[snip]
void fun(int a, int b, int c)
{
	if (condition)
		printk(KERN_WARNING "Warning this is a long printk with "
						"3 parameters a: %u b: %u "
						"c: %u \n", a, b, c);
	else
		next_statement;
}
[snip]
If it does not matter, I'd like to put them in one line :-).
Roy

RE: [PATCH v4]: Fix e500 v2 core reboot bug

[Li Yang-r58472]

> -----Original Message-----
> From: linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org
[mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org] On Behalf
Of
> Zang Roy-r61911
> Sent: Wednesday, June 13, 2007 3:44 PM
> To: Segher Boessenkool
> Cc: linuxppc-dev list; Paul Mackerras
> Subject: Re: [PATCH v4]: Fix e500 v2 core reboot bug
>=20
> On Wed, 2007-06-13 at 15:26, Segher Boessenkool wrote:
> > >>> +                             printk (KERN_EMERG "Error: reset
> > >> control \
> > >>> +                                     register not mapped,
> > >> spinning!\n");
> > >>
> > >> ...(... "bla bla bla "
> > >>         "bla bla bla\n");
> > >>
> > >> instead?  Or there'll be a lot of whitespace in your string.
> > > I just do not know the difference of these two style.
> >
> > Your code puts a bunch of extra whitespace in the string (all
> > the tabs before "register not mapped").  Two strings after
> > each other (like in my example) are just pasted together.
> >
> > There is no real reason to break the string in two anyway, it
> > is fine to have a more-than-80-char line in printk messages.
> >
> I separate it, because I am afraid I will be  challenged by 80 char
> column rule.
>=20
> Please check the CodeStyle file and its example:
>=20
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are
> placed
> substantially to the right. The same applies to function headers with
a
> long
> argument list. Long strings are as well broken into shorter strings.
> [snip]
> void fun(int a, int b, int c)
> {
> 	if (condition)
> 		printk(KERN_WARNING "Warning this is a long printk with
"
> 						"3 parameters a: %u b:
%u "
> 						"c: %u \n", a, b, c);
> 	else
> 		next_statement;
> }
It will be good if you do:
+				printk (KERN_EMERG "Error: reset
control"
+					"register not mapped,
spinning!\n");

Instead of:
+				printk (KERN_EMERG "Error: reset control
\
+					register not mapped,
spinning!\n");

- Leo

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Segher Boessenkool]

>> There is no real reason to break the string in two anyway, it
>> is fine to have a more-than-80-char line in printk messages.
>>
> I separate it, because I am afraid I will be  challenged by 80 char
> column rule.

Yeah, I understand.

> Please check the CodeStyle file and its example:

[snip]

> If it does not matter, I'd like to put them in one line :-).

Current thinking on this seems to be that long literal strings
are fine.  You are right that some people might challenge you
on this, of course ;-)


Segher

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Segher Boessenkool]

> It will be good if you do:
> +				printk (KERN_EMERG "Error: reset
> control"
> +					"register not mapped,
> spinning!\n");
>
> Instead of:
> +				printk (KERN_EMERG "Error: reset control
> \
> +					register not mapped,
> spinning!\n");

Except you forgot a space.  Too much whitespace in the
resulting string is bad, but too little doesn't help
either ;-)


Segher

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

On Wed, 2007-06-13 at 15:55, Segher Boessenkool wrote:
> > It will be good if you do:
> > +                             printk (KERN_EMERG "Error: reset
> > control"
> > +                                     "register not mapped,
> > spinning!\n");
> >
> > Instead of:
> > +                             printk (KERN_EMERG "Error: reset
> control
> > \
> > +                                     register not mapped,
> > spinning!\n");
> 
> Except you forgot a space.  Too much whitespace in the
> resulting string is bad, but too little doesn't help
> either ;-)
I noticed that space, except that I am not sure the difference between \
and ".
I decided to leave it as two line with " not \.
Thanks
Roy

RE: [PATCH v4]: Fix e500 v2 core reboot bug

[Li Yang-r58472]

> -----Original Message-----
> From: Zang Roy-r61911 [mailto:tie-fei.zang@freescale.com]
> Sent: Wednesday, June 13, 2007 4:07 PM
> To: Segher Boessenkool
> Cc: Li Yang-r58472; linuxppc-dev list; Paul Mackerras
> Subject: Re: [PATCH v4]: Fix e500 v2 core reboot bug
>=20
> On Wed, 2007-06-13 at 15:55, Segher Boessenkool wrote:
> > > It will be good if you do:
> > > +                             printk (KERN_EMERG "Error: reset
> > > control"
> > > +                                     "register not mapped,
> > > spinning!\n");
> > >
> > > Instead of:
> > > +                             printk (KERN_EMERG "Error: reset
> > control
> > > \
> > > +                                     register not mapped,
> > > spinning!\n");
> >
> > Except you forgot a space.  Too much whitespace in the
> > resulting string is bad, but too little doesn't help
> > either ;-)
> I noticed that space, except that I am not sure the difference between
\
> and ".
The difference is that you will get all the indentation in the next line
into the string if you use \. :-)
The output will be "Error: reset control<tab><tab><tab><tab> register
not mapped, spinning!\n"

- Leo

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

On Wed, 2007-06-13 at 15:54, Segher Boessenkool wrote:
[snip]
> 
> > If it does not matter, I'd like to put them in one line :-).
> 
> Current thinking on this seems to be that long literal strings
> are fine.  You are right that some people might challenge you
> on this, of course ;-)
Then, I will say Segher tell me to do that ...
Roy

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Zang Roy-r61911]

From: Roy Zang <tie-fei.zang@freescale.com>

Fix the reset bug on 8548CDS board.
Begin from MPC8548 with e500 v2 core, a new reset 
control register is added.
This register is used for the cpu reset.

Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
---
Pick up this one :-(

 arch/powerpc/boot/dts/mpc8548cds.dts |    6 ++++++
 arch/powerpc/platforms/85xx/misc.c   |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/boot/dts/mpc8548cds.dts
index ad96381..0550a3c 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dts
+++ b/arch/powerpc/boot/dts/mpc8548cds.dts
@@ -177,6 +177,12 @@
 			interrupt-parent = <&mpic>;
 		};
 
+		global-utilities@e0000 {	//global utilities reg
+			compatible = "fsl,mpc8548-guts";
+			reg = <e0000 1000>;
+			fsl,has-rstcr;
+		};
+
 		pci1: pci@8000 {
 			interrupt-map-mask = <1f800 0 0 7>;
 			interrupt-map = <
diff --git a/arch/powerpc/platforms/85xx/misc.c b/arch/powerpc/platforms/85xx/misc.c
index 3e62fcb..4ac0b20 100644
--- a/arch/powerpc/platforms/85xx/misc.c
+++ b/arch/powerpc/platforms/85xx/misc.c
@@ -13,11 +13,43 @@
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <asm/irq.h>
+#include <asm/io.h>
+#include <asm/prom.h>
+#include <sysdev/fsl_soc.h>
+
+static __be32 __iomem *rstcr;
 
 extern void abort(void);
 
+static int __init mpc85xx_rstcr(void)
+{
+	struct device_node *np;
+	np = of_find_node_by_name(NULL, "global-utilities");
+	if ((np && of_get_property(np, "fsl,has-rstcr", NULL))) {
+		const u32 *prop = of_get_property(np, "reg", NULL);
+		if (prop) {
+			/* map reset control register
+			 * 0xE00B0 is offset of reset control register
+			 */
+			rstcr = ioremap(get_immrbase() + *prop + 0xB0, 0xff);
+			if (!rstcr)
+				printk (KERN_EMERG "Error: reset control "
+						"register not mapped!\n");
+		}
+	} else
+		printk (KERN_INFO "rstcr compatible register does not exist!\n");
+	if (np)
+		of_node_put(np);
+	return 0;
+}
+
+arch_initcall(mpc85xx_rstcr);
+
 void mpc85xx_restart(char *cmd)
 {
 	local_irq_disable();
+	if (rstcr)
+		/* set reset control register */
+		out_be32(rstcr, 0x2);	/* HRESET_REQ */
 	abort();
 }
-- 
1.5.1

Re: [PATCH v4]: Fix e500 v2 core reboot bug

[Kumar Gala]

On Jun 13, 2007, at 4:13 AM, Zang Roy-r61911 wrote:

> From: Roy Zang <tie-fei.zang@freescale.com>
>
> Fix the reset bug on 8548CDS board.
> Begin from MPC8548 with e500 v2 core, a new reset
> control register is added.
> This register is used for the cpu reset.
>
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> ---
> Pick up this one :-(
>
>  arch/powerpc/boot/dts/mpc8548cds.dts |    6 ++++++
>  arch/powerpc/platforms/85xx/misc.c   |   32 +++++++++++++++++++++++ 
> +++++++++
>  2 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/mpc8548cds.dts b/arch/powerpc/ 
> boot/dts/mpc8548cds.dts
> index ad96381..0550a3c 100644
> --- a/arch/powerpc/boot/dts/mpc8548cds.dts
> +++ b/arch/powerpc/boot/dts/mpc8548cds.dts
> @@ -177,6 +177,12 @@
>  			interrupt-parent = <&mpic>;
>  		};
>
> +		global-utilities@e0000 {	//global utilities reg
> +			compatible = "fsl,mpc8548-guts";
> +			reg = <e0000 1000>;
> +			fsl,has-rstcr;
> +		};
> +
>  		pci1: pci@8000 {
>  			interrupt-map-mask = <1f800 0 0 7>;
>  			interrupt-map = <

Now that we resolved this can you work up a second patch to update  
Documentation/powerpc/booting-without-of.txt for this node.

- k