Re: [PATCH] powerpc/4xx: Add optional "reset_type" property to control reboot via dts

[Josh Boyer <jwboyer@linux.vnet.ibm.com>]

On Mon, Apr 26, 2010 at 03:39:01PM +0200, Stefan Roese wrote:
>By setting "reset_type" to one of the following values, the default
>software reset mechanism may be overidden. Here the possible values of
>"reset_type":

NEAT!  A 4xx patch!  I haven't gotten enough of these lately, so forgive my
nit picking ;)

>
>  1 - PPC4xx core reset
>  2 - PPC4xx chip reset
>  3 - PPC4xx system reset (default)

We should probably put a brief description of this in the dts bindings under
Documentation (or whereever we're storing them these days.  I saw something
about a wiki?).

Also, while it's not a large issue, I wonder if there will be confusion on
whether 'reset-type' is "the type of reset to use" or "the type of reset that
was just done".  There are some products that actually care about the latter for
various RAS issues.  I don't, however, have a great alternative property name
that comes to mind though.

>This will be used by a new PPC440SPe board port, which needs a "chip
>reset" instead of the default "system reset" to be asserted.

I'm curious why that is?

>Signed-off-by: Stefan Roese <sr@denx.de>
>Cc: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>---
> arch/powerpc/sysdev/ppc4xx_soc.c |   17 +++++++++++++++--
> 1 files changed, 15 insertions(+), 2 deletions(-)
>
>diff --git a/arch/powerpc/sysdev/ppc4xx_soc.c b/arch/powerpc/sysdev/ppc4xx_soc.c
>index 5c01435..fe54216 100644
>--- a/arch/powerpc/sysdev/ppc4xx_soc.c
>+++ b/arch/powerpc/sysdev/ppc4xx_soc.c
>@@ -191,11 +191,24 @@ static int __init ppc4xx_l2c_probe(void)
> arch_initcall(ppc4xx_l2c_probe);
>
> /*
>- * At present, this routine just applies a system reset.
>+ * Apply a system reset. Alternatively a board specific value may be
>+ * provided via the "reset-type" property in the cpu node.
>  */
> void ppc4xx_reset_system(char *cmd)
> {
>-	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_RST_SYSTEM);
>+	struct device_node *np;
>+	u32 reset_type = DBCR0_RST_SYSTEM;
>+	const u32 *prop;
>+
>+	np = of_find_node_by_type(NULL, "cpu");
>+	if (np) {
>+		prop = of_get_property(np, "reset-type", NULL);
>+		if (prop)
>+			reset_type = prop[0] << 28;
>+	}

While I don't think it's a big issue, I wonder if we should sanity check the
resulting value here.  I could see someone being dumb and doing:

	reset-type = "system";

or something like that.  If that is done, what would the resulting shift on it
turn into?

josh