-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Thursday, August 20, 2009 3:08 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik; Ronen Shitrit Subject: Re: [U-Boot] [PATCH v2][repost] arm: Kirkwood: add SYSRSTn Duration Counter Support
Dear Prafulla Wadaskar,
In message <73173D32E9439E4ABB5151606C3E19E202E391599F@SC-VEXCH1.marvell. com> you wrote:
- if (!s) {
printf("Error.. %s failed, check sysrstcmd\n",__FUNCTION__);return;Why is this considered an error? I think it is perfectly
legal to not
define this environment variable. For example, it is also
no error to
set "bootdelay" and not define "bootcmd". I think we
should implement
consistent behaviour.
It is similar with one difference- sysrstcmd is
additionally gated with h/w trigger,
Um... yes... agreed, but that's not actually so special. Consider for example the use of "altbootcmd" in connection with the boot count limit feature, or the "failbootcmd" which gets run in case of critical POST errors. None of these produce any such error messages. For consistency I recommend to remove this message here, too.
Secondly it is not as known as bootcmd, so it is always
better to throw some error message.
This save some of developer's time and email exchanges :-)
Well, for developers it may be useful during test - but it should not be present for regular users of the production version. Maybe you change it into a debug() ?
Agreed I will do this.
...
- sysrst_cnt = (0x1fffffff & readl(KW_REG_SYSRST_CNT));
- printf("H/w Rst hold time: %d.%d secs\n",
sysrst_cnt / SYSRST_CNT_1SEC_VAL,sysrst_cnt % SYSRST_CNT_1SEC_VAL);This should be debvug(), too ?
Does it harm if we keep this info?
Well, yes, it does. It adds output, which makes the boot process more noisy and addds to the boot time. And normally none of the end users will actually ever look at this information.
That's understood but only in case sysrstdelay is defined which is not default case :-)
It is just like "cpu name, speed etc".
Well, this _is_ information which the end users regularly check and pay attention to.
SysRST is a feature provided by h/w that we are supporting, It may help users who are willing to use this feature Any way it is gated by "sysrstdelay" So I think we must keep this print alive
Really? What is the advantage for the enduser to know if he pressed the button for 5.1 or 5.3 seconds?
No, I mean it is useful in case of 4.9 or 5.1 :-)
Please make it a debug().
Should I? even though by default it will not show up :-)
Regards.. Prafulla . .
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "...this does not mean that some of us should not want, in a rather dispassionate sort of way, to put a bullet through csh's head." - Larry Wall in 1992Aug6.221512.5963@netlabs.com