[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: Wednesday, August 19, 2009 12:50 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 
> <1250679240-17557-1-git-send-email-prafulla at marvell.com> you wrote:
> > I am sorry for previous post v2, pls ignore it, this is the 
> right patch for the same
> 
> This comment does not belong to the commit message. Please move below
> the "---" line.
Okay I will add this in my next post

> 
> > This feature can be used to trigger special command 
> "sysrstcmd" using
> > reset key long press event and environment variable 
> "sysrstdelay" is set
> > (useful for reset to factory or manufacturing mode execution)
> > 
> > Kirkwood SoC implements a hardware-based SYSRSTn duration counter.
> > When SYSRSTn is asserted low, a SYSRSTn duration counter is running.
> > The counter value is stored in the SYSRSTn Length Counter Register
> > The counter is based on the 25-MHz reference clock (40ns)
> > It is a 29-bit counter, yielding a maximum counting duration of
> > 2^29/25 MHz (21.4 seconds). When the counter reach its 
> maximum value,
> > it remains at this value until counter reset is triggered by setting
> > bit 31 of KW_REG_SYSRST_CNT
> > 
> > Implementation:
> > Upon long reset assertion (> ${sysrstdleay} in secs) 
> sysrstcmd will be
> 
> That's a typo, it's "sysrstdelay", right? Please fix while we are at
> it.
Thanks.. I will take care

> 
> > +static void kw_sysrst_action(void)
> > +{
> > +	int ret;
> > +	char *s = getenv("sysrstcmd");
> > +
> > +	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,
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 :-)

> 
> > +	}
> > +
> > +	printf("Starting %s process...\n", __FUNCTION__);
> 
> This should be a debug(), I think. Don't produce too much output.
> 
> > +	if (ret < 0)
> > +		printf("Error.. %s failed\n", __FUNCTION__);
> > +	else
> > +		printf("%s process finished\n", __FUNCTION__);
> 
> Ditto - please turn into debug().
Okay no issues..I will do it

> 
> > +
> > +static void kw_sysrst_check(void)
> > +{
> > +	u32 sysrst_cnt, sysrst_dly;
> > +	char *s;
> > +
> > +	/*
> > +	 * no action if sysrstdelay environment variable is not defined
> > +	 */
> > +	s = getenv("sysrstdelay");
> > +	if (s == NULL)
> > +		return;
> > +
> > +	/* read sysrstdelay value */
> > +	sysrst_dly = (u32) simple_strtoul(s, NULL, 10);
> > +
> > +	/* read SysRst Length counter register (bits 28:0) */
> > +	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?
It is just like "cpu name, speed etc".
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

Regards..
Prafulla . .

> 
> 
> Thanks.
> 
> 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
> If a person (a) is poorly, (b) receives treatment  intended  to  make
> him  better, and (c) gets better, then no power of reasoning known to
> medical science can convince him  that  it  may  not  have  been  the
> treatment that restored his health.
> - Sir Peter Medawar, The Art of the Soluble
>