| Message ID | 1299754388-31648-1-git-send-email-agust@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Anatolij, --- On Thu, 3/10/11, Anatolij Gustschin <agust@denx.de> wrote: .... > mx31_get_mcu_main_clk() / 1000000); > + struct wdog_regs *wdog = (struct> wdog_regs *)WDOG_BASE; > + > + printf("CPU: Freescale i.MX31 at %d MHz (WRSR=0x%04x)\n", > + > mx31_get_mcu_main_clk() / 1000000, wdog->wrsr); > return 0; Wouldn´t it be better to show a string for the reset source, rather than a pure register value? I sent a similar patch yesterday for MX31PDK: http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/95615 ,but I agree that moving it to the generic code is a better idea. Regards, Fabio Estevam
On 03/10/2011 11:53 AM, Anatolij Gustschin wrote: > Add output of the WRSR register content while booting so that > we can see the source of the last reset. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> Hi Antolij, > --- > arch/arm/cpu/arm1136/mx31/generic.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/cpu/arm1136/mx31/generic.c b/arch/arm/cpu/arm1136/mx31/generic.c > index 1415d6c..788faed 100644 > --- a/arch/arm/cpu/arm1136/mx31/generic.c > +++ b/arch/arm/cpu/arm1136/mx31/generic.c > @@ -93,8 +93,10 @@ void mx31_gpio_mux(unsigned long mode) > #if defined(CONFIG_DISPLAY_CPUINFO) > int print_cpuinfo (void) > { > - printf("CPU: Freescale i.MX31 at %d MHz\n", > - mx31_get_mcu_main_clk() / 1000000); > + struct wdog_regs *wdog = (struct wdog_regs *)WDOG_BASE; > + > + printf("CPU: Freescale i.MX31 at %d MHz (WRSR=0x%04x)\n", > + mx31_get_mcu_main_clk() / 1000000, wdog->wrsr); Why is it better to use the wrsr register instead of the rcsr register ? We are actually using the rcsr register for other i.MX processors (MX51/MX53/MX35). And if we want to print the reset cause, I think it should be better to write directly the cause as string instead of the register value. I do not think printing the reset cause should be implemented in print_cpuinfo(), because it manages a different issue (reset cause against CPU information). The print_cpuinfo() should have only CPU related values, as clock values, and so on, as it is implemented now for this and other processors. Better to add a different function for the reset cause. Best regards, Stefano Babic
On Thu, 10 Mar 2011 16:07:49 +0100 Stefano Babic <sbabic@denx.de> wrote: > On 03/10/2011 11:53 AM, Anatolij Gustschin wrote: > > Add output of the WRSR register content while booting so that > > we can see the source of the last reset. > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > Hi Antolij, Hi Stefano, ... > > + printf("CPU: Freescale i.MX31 at %d MHz (WRSR=0x%04x)\n", > > + mx31_get_mcu_main_clk() / 1000000, wdog->wrsr); > > Why is it better to use the wrsr register instead of the rcsr register ? > We are actually using the rcsr register for other i.MX processors > (MX51/MX53/MX35). And if we want to print the reset cause, I think it > should be better to write directly the cause as string instead of the > register value. The reason for using wrsr register is that when reading this register, we can also recognize the software reset cause (SWFT, bit 0 is set). In our use case we have this requirement. U-Boot and Linux for iMX31 assert system reset by clearing SRS bit in wcr. This software reset is reported by SWFT in the wrsr register. When I read rcsr after a software reset, I always get watchdog timeout reset cause (probably because the reset was performed by writing to the watchdog module register wcr). But actually this reset was a software reset. I'm fine with using strings for reset cause if there is no objection. > I do not think printing the reset cause should be implemented in > print_cpuinfo(), because it manages a different issue (reset cause > against CPU information). The print_cpuinfo() should have only CPU > related values, as clock values, and so on, as it is implemented now for > this and other processors. Better to add a different function for the > reset cause. Were should we call this different function? Should be put it into init_sequence[]? Best regards, Anatolij
Hi Fabio, On Thu, 10 Mar 2011 05:47:15 -0800 (PST) Fabio Estevam <fabioestevam@yahoo.com> wrote: > Hi Anatolij, > > --- On Thu, 3/10/11, Anatolij Gustschin <agust@denx.de> wrote: > .... > > mx31_get_mcu_main_clk() / 1000000); > > + struct wdog_regs *wdog = (struct> wdog_regs *)WDOG_BASE; > > + > > + printf("CPU: Freescale i.MX31 at %d MHz (WRSR=0x%04x)\n", > > + > > mx31_get_mcu_main_clk() / 1000000, wdog->wrsr); > > return 0; > > Wouldn´t it be better to show a string for the reset source, rather than a pure register value? I'm fine with using a string. Viele Grüße, Anatolij
On 03/12/2011 02:15 PM, Anatolij Gustschin wrote: > The reason for using wrsr register is that when reading this register, > we can also recognize the software reset cause (SWFT, bit 0 is set). > In our use case we have this requirement. Ok, understood. >> I do not think printing the reset cause should be implemented in >> print_cpuinfo(), because it manages a different issue (reset cause >> against CPU information). The print_cpuinfo() should have only CPU >> related values, as clock values, and so on, as it is implemented now for >> this and other processors. Better to add a different function for the >> reset cause. > > Were should we call this different function? Should be put it > into init_sequence[]? Well, I have not thought as a general function to be inserted in the init_sequence, I do not know if other ARM processor can export this function. On other i.MX processors, the reset cause is printed inside the checkboard function, if CONFIG_DISPLAY_BOARDINFO is set, and not inside print_cpuinfo(). I understand that the cpuinfo reports only how to identify the CPU, and the reset cause should be handled separately. IMHO should be enough to have a mxc_get_reset_cause() (or something like this) that the board maintainer can call if for some reason he must perform different actions according to the last reset cause. As name, I would prefer to identify not static functions starting with mxc_, as I am trying to uniform all i.MX exponing the same set of functions. Best regards, Stefano
diff --git a/arch/arm/cpu/arm1136/mx31/generic.c b/arch/arm/cpu/arm1136/mx31/generic.c index 1415d6c..788faed 100644 --- a/arch/arm/cpu/arm1136/mx31/generic.c +++ b/arch/arm/cpu/arm1136/mx31/generic.c @@ -93,8 +93,10 @@ void mx31_gpio_mux(unsigned long mode) #if defined(CONFIG_DISPLAY_CPUINFO) int print_cpuinfo (void) { - printf("CPU: Freescale i.MX31 at %d MHz\n", - mx31_get_mcu_main_clk() / 1000000); + struct wdog_regs *wdog = (struct wdog_regs *)WDOG_BASE; + + printf("CPU: Freescale i.MX31 at %d MHz (WRSR=0x%04x)\n", + mx31_get_mcu_main_clk() / 1000000, wdog->wrsr); return 0; } #endif
Add output of the WRSR register content while booting so that we can see the source of the last reset. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- arch/arm/cpu/arm1136/mx31/generic.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)