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

[Wolfgang Denk]

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.

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

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

> +	}
> +
> +	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().

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


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