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