| Message ID | 1282946147-28164-1-git-send-email-msm@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
> <msm@freescale.com> wrote: > + > + for_each_node_by_name(np, "global-utilities") { > + if ((of_get_property(np, "fsl,has-rstcr", NULL))) { > + rstcr = of_iomap(np, 0) + 0xb0; > + if (!rstcr) > + printk (KERN_EMERG "Error: reset control " I'm not sure KERN_EMERG is warranted for this kind of error. > + "register not mapped!\n"); > + } So if a node has an fsl,rstcr property, but the of_iomap() fails, we jump to the next global-utilities node? Perhaps you need a 'break' after the printk()? > + } > + > + if (!rstcr && ppc_md.restart == fsl_rstcr_restart) Wouldn't it make more sense to assign fsl_rstcr_restart to ppc_md.restart only if we find a valid fsl,has-rstcr property?
On Aug 28, 2010, at 5:34 PM, Timur Tabi wrote: >> <msm@freescale.com> wrote: > >> + >> + for_each_node_by_name(np, "global-utilities") { >> + if ((of_get_property(np, "fsl,has-rstcr", NULL))) { >> + rstcr = of_iomap(np, 0) + 0xb0; >> + if (!rstcr) >> + printk (KERN_EMERG "Error: reset control " > > I'm not sure KERN_EMERG is warranted for this kind of error. I'm not sure either - I left it as it was before. > >> + "register not mapped!\n"); >> + } > > So if a node has an fsl,rstcr property, but the of_iomap() fails, we > jump to the next global-utilities node? Perhaps you need a 'break' > after the printk()? Or potentially a continue to be more robust? Or would two (or more) "has-rstcr" nodes be wrong? > >> + } >> + >> + if (!rstcr && ppc_md.restart == fsl_rstcr_restart) > > Wouldn't it make more sense to assign fsl_rstcr_restart to > ppc_md.restart only if we find a valid fsl,has-rstcr property? Again I'm not entirely sure, I left this as it was before. Is there another way to reset the board if the rstcr node was not found correctly? -M > > -- > Timur Tabi > Linux kernel developer at Freescale >
On Tue, Aug 31, 2010 at 11:26 AM, Matthew McClintock <msm@freescale.com> wrote: >> I'm not sure KERN_EMERG is warranted for this kind of error. > > I'm not sure either - I left it as it was before. My vote is to change it to KERN_ERR, but it's your call. >> So if a node has an fsl,rstcr property, but the of_iomap() fails, we >> jump to the next global-utilities node? Perhaps you need a 'break' >> after the printk()? > > Or potentially a continue to be more robust? Or would two (or more) "has-rstcr" nodes be wrong? My point is that if the of_iomap() call fails, looking for another global-utilities node is not the right course of action. of_iomap() failing is a serious error that should result in an immediate exit. >> Wouldn't it make more sense to assign fsl_rstcr_restart to >> ppc_md.restart only if we find a valid fsl,has-rstcr property? > > Again I'm not entirely sure, I left this as it was before. Is there another way to reset the board if the rstcr node was not found correctly? Not on our 85xx boards. On 83xx, there's mpc83xx_restart(). I just don't like "ppc_md.restart == fsl_rstcr_restart", because it assumes that the define_machine() entry is incorrect.
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c index b91f7ac..e2c8e47 100644 --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -378,17 +378,19 @@ static __be32 __iomem *rstcr; static int __init setup_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))) { - rstcr = of_iomap(np, 0) + 0xb0; - if (!rstcr) - printk (KERN_EMERG "Error: reset control register " - "not mapped!\n"); - } else if (ppc_md.restart == fsl_rstcr_restart) + + for_each_node_by_name(np, "global-utilities") { + if ((of_get_property(np, "fsl,has-rstcr", NULL))) { + rstcr = of_iomap(np, 0) + 0xb0; + if (!rstcr) + printk (KERN_EMERG "Error: reset control " + "register not mapped!\n"); + } + } + + if (!rstcr && ppc_md.restart == fsl_rstcr_restart) printk(KERN_ERR "No RSTCR register, warm reboot won't work\n"); - if (np) - of_node_put(np); return 0; }
The first global-utilities node might not contain the rstcr property, so we should search all the nodes Signed-off-by: Matthew McClintock <msm@freescale.com> --- arch/powerpc/sysdev/fsl_soc.c | 20 +++++++++++--------- 1 files changed, 11 insertions(+), 9 deletions(-)