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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Thursday, August 20, 2009 3:08 PM
> To: Prafulla Wadaskar
> Cc: u-boot at 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 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() ?
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 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>
>