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

[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@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

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

[Prafulla Wadaskar]

I am sorry for previous post v2, pls ignore it, this is the right patch for the same

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
executed if pre-defined in environment variables.
This feature will be disabled if "sysrstdelay" variable is unset.

for-ex.
setenv sysrst_cmd "echo starting factory reset;
		   nand erase 0xa0000 0x20000;
		   echo finish ed sysrst command;"
will erase particular nand sector if triggered by this event

Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
---
Change log:
v2: updated as per review feedback for v1
bug fix in the previous post (V2) fixed

 cpu/arm926ejs/kirkwood/cpu.c        |   75 +++++++++++++++++++++++++++++++++++
 include/asm-arm/arch-kirkwood/cpu.h |    2 +
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/cpu/arm926ejs/kirkwood/cpu.c b/cpu/arm926ejs/kirkwood/cpu.c
index 795a739..97318ff 100644
--- a/cpu/arm926ejs/kirkwood/cpu.c
+++ b/cpu/arm926ejs/kirkwood/cpu.c
@@ -195,6 +195,78 @@ int kw_config_mpp(u32 mpp0_7, u32 mpp8_15, u32 mpp16_23, u32 mpp24_31,
 	return 0;
 }
 
+/*
+ * SYSRSTn Duration Counter Support
+ *
+ * Kirkwood SoC implements a hardware-based SYSRSTn duration counter.
+ * When SYSRSTn is asserted low, a SYSRSTn duration counter is running.
+ * The SYSRSTn duration counter is useful for implementing a manufacturer
+ * or factory reset. Upon a long reset assertion that is greater than a
+ * pre-configured environment variable value for sysrstdelay,
+ * 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
+ */
+static void kw_sysrst_action(void)
+{
+	int ret;
+	char *s = getenv("sysrstcmd");
+
+	if (!s) {
+		printf("Error.. %s failed, check sysrstcmd\n",
+			__FUNCTION__);
+		return;
+	}
+
+	printf("Starting %s process...\n", __FUNCTION__);
+#if !defined(CONFIG_SYS_HUSH_PARSER)
+	ret = run_command (s, 0);
+#else
+	ret = parse_string_outer(s, FLAG_PARSE_SEMICOLON
+				  | FLAG_EXIT_FROM_LOOP);
+#endif
+	if (ret < 0)
+		printf("Error.. %s failed\n", __FUNCTION__);
+	else
+		printf("%s process finished\n", __FUNCTION__);
+}
+
+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);
+
+	/* clear the counter for next valid read*/
+	writel(1 << 31, KW_REG_SYSRST_CNT);
+
+	/*
+	 * sysrst_action:
+	 * if H/w Reset key is pressed and hold for time
+	 * more than sysrst_dly in seconds
+	 */
+	if (sysrst_cnt >= SYSRST_CNT_1SEC_VAL * sysrst_dly)
+		kw_sysrst_action();
+}
+
 #if defined(CONFIG_DISPLAY_CPUINFO)
 int print_cpuinfo(void)
 {
@@ -298,6 +370,9 @@ int arch_misc_init(void)
 	temp = get_cr();
 	set_cr(temp & ~CR_V);
 
+	/* checks and execute resset to factory event */
+	kw_sysrst_check();
+
 	return 0;
 }
 #endif /* CONFIG_ARCH_MISC_INIT */
diff --git a/include/asm-arm/arch-kirkwood/cpu.h b/include/asm-arm/arch-kirkwood/cpu.h
index d1440af..b3022a3 100644
--- a/include/asm-arm/arch-kirkwood/cpu.h
+++ b/include/asm-arm/arch-kirkwood/cpu.h
@@ -36,6 +36,8 @@
 		((_x ? KW_EGIGA0_BASE : KW_EGIGA1_BASE) + 0x44c)
 
 #define KW_REG_DEVICE_ID		(KW_MPP_BASE + 0x34)
+#define KW_REG_SYSRST_CNT		(KW_MPP_BASE + 0x50)
+#define SYSRST_CNT_1SEC_VAL		(25*1000000)
 #define KW_REG_MPP_OUT_DRV_REG		(KW_MPP_BASE + 0xE0)
 
 enum memory_bank {
-- 
1.5.3.3

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:24 Wed 19 Aug     , Prafulla Wadaskar wrote:
> I am sorry for previous post v2, pls ignore it, this is the right patch for the same
> 
> 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
> executed if pre-defined in environment variables.
> This feature will be disabled if "sysrstdelay" variable is unset.
> 
> for-ex.
> setenv sysrst_cmd "echo starting factory reset;
> 		   nand erase 0xa0000 0x20000;
> 		   echo finish ed sysrst command;"
> will erase particular nand sector if triggered by this event
> 
> Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
> ---
> Change log:
> v2: updated as per review feedback for v1
> bug fix in the previous post (V2) fixed
ok

but I think make optionnal will be better

Best Regards,
J.

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

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com] 
> Sent: Thursday, August 20, 2009 4:00 AM
> 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
> 
> On 16:24 Wed 19 Aug     , Prafulla Wadaskar wrote:
> > I am sorry for previous post v2, pls ignore it, this is the 
> right patch for the same
> > 
> > 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
> > executed if pre-defined in environment variables.
> > This feature will be disabled if "sysrstdelay" variable is unset.
> > 
> > for-ex.
> > setenv sysrst_cmd "echo starting factory reset;
> > 		   nand erase 0xa0000 0x20000;
> > 		   echo finish ed sysrst command;"
> > will erase particular nand sector if triggered by this event
> > 
> > Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
> > ---
> > Change log:
> > v2: updated as per review feedback for v1
> > bug fix in the previous post (V2) fixed
> ok
> 
> but I think make optionnal will be better
Hi Jean
Thanks..
I didn't understod what you want to say here, can you pls explain?

Regards..
Prafulla . .

> 
> Best Regards,
> J.
> 

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

[Jean-Christophe PLAGNIOL-VILLARD]

> > > Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
> > > ---
> > > Change log:
> > > v2: updated as per review feedback for v1
> > > bug fix in the previous post (V2) fixed
> > ok
> > 
> > but I think make optionnal will be better
> Hi Jean
> Thanks..
> I didn't understod what you want to say here, can you pls explain?
the patch is fine for I but I think we may create a CONFIG_ somethink
to enable it only if the use want it and do not impact the U-Boot size
otherwise

Best Regards,
J.

[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@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
> 

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

[Wolfgang Denk]

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

...
> > > +	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@netlabs.com>

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

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com] 
> Sent: Thursday, August 20, 2009 10:50 AM
> 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
> 
> > > > Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
> > > > ---
> > > > Change log:
> > > > v2: updated as per review feedback for v1
> > > > bug fix in the previous post (V2) fixed
> > > ok
> > > 
> > > but I think make optionnal will be better
> > Hi Jean
> > Thanks..
> > I didn't understod what you want to say here, can you pls explain?
> the patch is fine for I but I think we may create a CONFIG_ somethink
> to enable it only if the use want it and do not impact the U-Boot size
> otherwise
Okay I got it
WE can do this but,
There are just two APIs, overall size impact is not much (<100 bytes max). And this is very useful feature.
So can we make it by default enabled?

Regards...
Prafulla .. 

> 
> Best Regards,
> J.
> 

[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@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@netlabs.com>
> 

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

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Prafulla Wadaskar
> Sent: Thursday, August 20, 2009 3:16 PM
> To: Jean-Christophe PLAGNIOL-VILLARD
> 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
> 
>  
> 
> > -----Original Message-----
> > From: Jean-Christophe PLAGNIOL-VILLARD 
> [mailto:plagnioj at jcrosoft.com] 
> > Sent: Thursday, August 20, 2009 10:50 AM
> > 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
> > 
> > > > > Signed-off-by: Prafulla Wadaskar <prafulla@marvell.com>
> > > > > ---
> > > > > Change log:
> > > > > v2: updated as per review feedback for v1
> > > > > bug fix in the previous post (V2) fixed
> > > > ok
> > > > 
> > > > but I think make optionnal will be better
> > > Hi Jean
> > > Thanks..
> > > I didn't understod what you want to say here, can you pls explain?
> > the patch is fine for I but I think we may create a CONFIG_ 
> somethink
> > to enable it only if the use want it and do not impact the 
> U-Boot size
> > otherwise
> Okay I got it
> WE can do this but,
> There are just two APIs, overall size impact is not much 
> (<100 bytes max).
The actual u-boot.bin size diff for newly posted patch is (169464- 169344= 120 bytes)

Regards..
Prafulla . .

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

[Jean-Christophe PLAGNIOL-VILLARD]

> > Okay I got it
> > WE can do this but,
> > There are just two APIs, overall size impact is not much 
> > (<100 bytes max).
> The actual u-boot.bin size diff for newly posted patch is (169464- 169344= 120 bytes)
ok fine

Best Regards,
J.

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

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <73173D32E9439E4ABB5151606C3E19E202E39159C0@SC-VEXCH1.marvell.com> you wrote:
> 
> > > > > +	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 :-)

I don;t see why that should make any difference?

> > 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 :-)

What for? He can see the difference from different behaviour.

If you really feel you want to be verbose you can lard your
definitions of bootcmd, altbootcmd, failbootcmd, sysrstcmd, etc. with
any number of "echo" commands you like.

But please don't make the default output more verbose than really
necessary.

> > Please make it a debug().
> Should I? even though by default it will not show up :-)

Please do. Keep in mind: No news is good news.

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
Everyting looks interesting until you do it. Then you find it's  just
another job.                     - Terry Pratchett, _Moving Pictures_

[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: Saturday, August 22, 2009 3:37 AM
> 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,
> 
<snip..>
> 
> > > Please make it a debug().
> > Should I? even though by default it will not show up :-)
> 
> Please do. Keep in mind: No news is good news.
Patch v3 is already posted for the same
Thanks...
Regards.
Prafulla . .