[U-Boot] [PATCH v2][repost] arm: Kirkwood: add SYSRSTn Duration Counter Support

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <73173D32E9439E4ABB5151606C3E19E202E391599F at 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() ?

...
> > > +	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.

> 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?

Please make it a debug().

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 at 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 at netlabs.com>