[U-Boot,v4] Add initial support for Wandboard dual lite and solo.

Hi Wolfgang,

On Thu, Mar 14, 2013 at 9:31 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Fabio Estevam,
>
> In message <1363228354-29534-1-git-send-email-festevam@gmail.com> you wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> Add initial support for Wandboard.
>
> Subject and commit message are redundant, resulting in text like this:
>
>         Add initial support for Wandboard dual lite and solo.
>
>         Add initial support for Wandboard.
>
> Please remove the second line.

Thanks for the review. I will address your comments and send v5 after
the nitrogen patches are applied into u-boot.imx.

>> +#define CONFIG_BOOTDELAY            1
>
> Is there any reason for not chosing the more standard 5 second delay?

Ok, so let's go with 3 seconds then ;-)

> I can confirm that the code boots on a wanboard_dl system, but it does
> not find the environment as used by the original Technixion port. Is
> this intentional?

To be honest I haven't really checked the environment settings used in
the original Technixion port.

Hopefully this is not a problem.

> Can we please remove the "Reset cause: WDOG" line in production mode?

Do you mean the change below?


Since this is common code I can address it separately with other
patch. Just let me know if this is OK.

Regards,

Fabio Estevam

On Thu, Mar 14, 2013 at 02:01:20PM -0300, Fabio Estevam wrote:
> Hi Wolfgang,
> 
> On Thu, Mar 14, 2013 at 9:31 AM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Fabio Estevam,
> >
> > In message <1363228354-29534-1-git-send-email-festevam@gmail.com> you wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> Add initial support for Wandboard.
> >
> > Subject and commit message are redundant, resulting in text like this:
> >
> >         Add initial support for Wandboard dual lite and solo.
> >
> >         Add initial support for Wandboard.
> >
> > Please remove the second line.
> 
> Thanks for the review. I will address your comments and send v5 after
> the nitrogen patches are applied into u-boot.imx.
> 
> >> +#define CONFIG_BOOTDELAY            1
> >
> > Is there any reason for not chosing the more standard 5 second delay?
> 
> Ok, so let's go with 3 seconds then ;-)

There's a pretty even distribution of 1 3 and 5 second delays (with a
few 10s, 2s and 6s).  If they want 1, let them have 1, it's not hard to
break into, you have U-Boot starting + 1sec.

Dear Fabio,

In message <CAOMZO5AVbVjXhHvMmZYRmroBWZYXb_-_4eNiQVTUjOOXD=iDhg@mail.gmail.com> you wrote:
> 
> > Is there any reason for not chosing the more standard 5 second delay?
> 
> Ok, so let's go with 3 seconds then ;-)

Why not 5?  For development, 1 or 3 is often quite short.  I've seen
enough cases where connecting to a serial port (for example, through
one of thos @#&^!*y USB devices) eats a major part of that time.  And
for production mode when boot times are important you don;t want such
a dealy anyway, not even with 1 or with 3 seconds.

> > I can confirm that the code boots on a wanboard_dl system, but it does
> > not find the environment as used by the original Technixion port. Is
> > this intentional?

Sorry for my typo.  The company name is Technexion.

> To be honest I haven't really checked the environment settings used in
> the original Technixion port.
> 
> Hopefully this is not a problem.

Not really a problem.  I just wondered if there was a good reason to
chose a different location for the environment.  BTW - would it not
make sense to enable redundant environment?

Actually I don;t think it is worth keepin the old settings, but that's
just my opinion.  Other users might be surprised to lose their whole
environment settings when updating from the vendor provided version to
mainline, so at least a warning should go to the board specific
README.

> > Can we please remove the "Reset cause: WDOG" line in production mode?
> 
> Do you mean the change below?
> 
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -148,7 +148,7 @@ int print_cpuinfo(void)
>                 (cpurev & 0x000F0) >> 4,
>                 (cpurev & 0x0000F) >> 0,
>                 mxc_get_clock(MXC_ARM_CLK) / 1000000);
> -       printf("Reset cause: %s\n", get_reset_cause());
> +       debug("Reset cause: %s\n", get_reset_cause());
>         return 0;
>  }
>  #endif
> 
> Since this is common code I can address it separately with other
> patch. Just let me know if this is OK.

Indeed this is common code, I see it now.  So yes, if we change this,
it should be done as separate patch.  I think debug() makes a lot of
sense here to reduce the output at boot time to a reasonable minimum,
but then - is there another way for the user to inquire for this
information.  If not, should we add it to the bdinfo output?

Best regards,

Wolfgang Denk

Dear Tom,

In message <20130314173648.GX23324@bill-the-cat> you wrote:
> 
> > > Is there any reason for not chosing the more standard 5 second delay?
> > 
> > Ok, so let's go with 3 seconds then ;-)
> 
> There's a pretty even distribution of 1 3 and 5 second delays (with a
> few 10s, 2s and 6s).  If they want 1, let them have 1, it's not hard to
> break into, you have U-Boot starting + 1sec.

The question is who "they" are.  My vote is for 5 :-)

A quick AWK oneliner in include/configs finds this statistics:

	Value Count
	 -1:    49
	  0:    12
	  1:    59
	  2:    30
	  3:   150
	  5:   175
	  6:    14
	 10:    38
	 20:     1

[And this does not take into account that for example amcc-common.h
which defines a value of 5 is included by 28 other board config
files...]

Best regards,

Wolfgang Denk

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/14/2013 04:36 PM, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20130314173648.GX23324@bill-the-cat> you wrote:
>> 
>>>> Is there any reason for not chosing the more standard 5
>>>> second delay?
>>> 
>>> Ok, so let's go with 3 seconds then ;-)
>> 
>> There's a pretty even distribution of 1 3 and 5 second delays
>> (with a few 10s, 2s and 6s).  If they want 1, let them have 1,
>> it's not hard to break into, you have U-Boot starting + 1sec.
> 
> The question is who "they" are.  My vote is for 5 :-)
> 
> A quick AWK oneliner in include/configs finds this statistics:

OK, I forgot to sub -1 from 1, oops.  So it's not very even, but...

> 
> Value Count -1:    49 0:    12 1:    59 2:    30 3:   150 5:   175 
> 6:    14 10:    38 20:     1

We have a lot of variation and no "standard" aside from "board
maintainer picks".  While I'd strongly question anyone who did > 10 I
think this is a board maintainer decision.  Now, if you've convinced
Fabio that 5 or 3 just makes more sense than 1 for all the right
reasons, great.  But "I think 1 is too short" isn't a good one.  1 is
plenty of time to get a keypress in and stop the boot given that you
have roughly from u-boot starting to press any key, not just when the
countdown is going.  Unless you have a bug in your timer code :)

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRQjrHAAoJENk4IS6UOR1WsZIQAJv0BwogMTtfpaijt10frY2Z
9TiVbSz+GLAsHG1KpxmWG8K1D6mFVuQw6/96+8ARdZCeLdcqKXZjPLcydmmO6Ths
HLR22EcGpUDVmfgi15LUGxD/uct/MZYwl0w9NWfEQBeuluEPlGN6Rahv9gtQeZmI
U/3fBLX8S/LhX1WunVDRyDn2YBRedR2AvlLQbkd7JeondPqL72Cspnwr2QgpumJc
mqua9zXHiXzzS0HdLDoQxdwWM9wCF7Xl2+go744gvv3Rr/hot7eWwPy5DjAPDMlL
bjdhgtsg1kcjlJqdvKz3TpV5WC5bkbdHXR7G9xaNCL1ibkN5SWg9YhwelbLYGl+r
Enbov0r0jG5XHfv2QfxutPnIOb1EptH9//926Wu4V7Xc/n5aMeuRLer0Cqzni7Ps
EbjQ8PZSwLbBla9hlW6IjlzjdXxgG74k6n/+St9np5Sfe2jLkfziMoQApkxPairr
ixhCH0w+rfrMeDxZV0vubQWqG9cRlfXwWdKIbdGZm0vpZWC5wFSqbMcqTuz/Edyx
E5H4M6TGmXV+bWG/ShWeQ+ybxBgKmikyMcGYuVr9MtUgaHkwmAf+WEITUWXDi6iX
tUCR+tXwnRvMxQoPu45U1xLA2VedofvTukZaLWrarP5o7yhCU7BE7Kx4gsjQMH2V
E6PGT31eUj3m+Ila3mWd
=PZeB
-----END PGP SIGNATURE-----

On 14/03/2013 21:24, Wolfgang Denk wrote:
> Dear Fabio,
> 

Hi Wolfgang, hi Fabio,

>>> Can we please remove the "Reset cause: WDOG" line in production mode?
>>
>> Do you mean the change below?
>>
>> --- a/arch/arm/imx-common/cpu.c
>> +++ b/arch/arm/imx-common/cpu.c
>> @@ -148,7 +148,7 @@ int print_cpuinfo(void)
>>                 (cpurev & 0x000F0) >> 4,
>>                 (cpurev & 0x0000F) >> 0,
>>                 mxc_get_clock(MXC_ARM_CLK) / 1000000);
>> -       printf("Reset cause: %s\n", get_reset_cause());
>> +       debug("Reset cause: %s\n", get_reset_cause());
>>         return 0;
>>  }
>>  #endif
>>
>> Since this is common code I can address it separately with other
>> patch. Just let me know if this is OK.
> 
> Indeed this is common code, I see it now.  So yes, if we change this,
> it should be done as separate patch.  I think debug() makes a lot of
> sense here to reduce the output at boot time to a reasonable minimum,
> but then - is there another way for the user to inquire for this
> information.  If not, should we add it to the bdinfo output?

Reset cause is a very important information - we can know if the
processor gets a watchdog, or if a power on happened. The last one can
address to hiddden issues with the power supply.

I understand that less redundant information is better, but this is very
important info and it cannot be used only if DEBUG is set. IMHO we
should let it as it is - it helps us when something bad happens.

Best regards,
Stefano Babic

Dear Stefano Babic,

In message <5143365F.5000207@denx.de> you wrote:
> 
> > Indeed this is common code, I see it now.  So yes, if we change this,
> > it should be done as separate patch.  I think debug() makes a lot of
> > sense here to reduce the output at boot time to a reasonable minimum,
> > but then - is there another way for the user to inquire for this
> > information.  If not, should we add it to the bdinfo output?
> 
> Reset cause is a very important information - we can know if the
> processor gets a watchdog, or if a power on happened. The last one can
> address to hiddden issues with the power supply.
> 
> I understand that less redundant information is better, but this is very
> important info and it cannot be used only if DEBUG is set. IMHO we
> should let it as it is - it helps us when something bad happens.

I agree that it is important information, and there should be a way
that the user can get at this information.  But I see no urgent need
to print this for everyboot, where it is just costing us precious boot
time (yes, it's only a few milliseconds, but they are trivial to
save).

That was why I suggested to add thjis information for example to the
output of the "bdinfo" command.


BTW: if I type "reset", I will get a message "Reset cause: WDOG".
I know why, but most users will not know thy they see a watchdog reset
here, so actually this information may be more confusing than helpful
in a number of cases (which is IMO one more reason to not always to
display it - it saves us a number of unproductive support requests).

Best regards,

Wolfgang Denk

On 15/03/2013 18:27, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 

Hi Wolfgang,

> I agree that it is important information, and there should be a way
> that the user can get at this information.  But I see no urgent need
> to print this for everyboot, where it is just costing us precious boot
> time (yes, it's only a few milliseconds, but they are trivial to
> save).
> 
> That was why I suggested to add thjis information for example to the
> output of the "bdinfo" command.

That is fine - but then it should be consistent. We have tried to have a
consistent API between SOCs (I mean: iMX SOCs), that is, each SOC in the
i.MX family should implement the same functions and not invent a new
one, And the behavior must beconsistent, too. I know we are far away to
be perfect, and some code can be further factorized (this was the reason
to add imx_common).

To the specific case, each i.MX SOC implements a get_reset_cause() function:

arch/arm/cpu/arm1136/mx31/generic.c:static char *get_reset_cause(void)
arch/arm/cpu/arm1136/mx31/generic.c:	printf("Reset cause: %s\n",

arch/arm/cpu/arm1136/mx35/generic.c:static char *get_reset_cause(void)
arch/arm/cpu/arm1136/mx35/generic.c:	printf("Reset cause: %s\n",
get_reset_cause());

arch/arm/cpu/arm926ejs/mx25/generic.c:static char *get_reset_cause(void)
arch/arm/cpu/arm926ejs/mx25/generic.c:	printf("Reset cause: %s\n\n",

arch/arm/imx-common/cpu.c:char *get_reset_cause(void)
arch/arm/imx-common/cpu.c:	printf("Reset cause: %s\n", get_reset_cause());

Checking this I see also that the mx53loco does not use general code
(bad !):

board/freescale/mx53loco/mx53loco.c:	printf("Reset cause: %s\n",

It is ok to move the information to bdinfo, but then it should not break
the consistence: all i.MX should follow the same rule. This makes also
easier for board maintainers to switch from a SOC to the next one,
because they have to provide quite the same functions in their code (and
using the same functions, too.).

> BTW: if I type "reset", I will get a message "Reset cause: WDOG".
> I know why, but most users will not know thy they see a watchdog reset
> here, so actually this information may be more confusing than helpful
> in a number of cases (which is IMO one more reason to not always to
> display it - it saves us a number of unproductive support requests).

I agree with you that this should be done by a separate patchset: it is
not related to the Wandboard. But if we go on this way, the same changes
should be done for all i.MX, not only for i.MX5/i.MX6.

Best regards,
Stefano Babic

Dear Stefano,

In message <514367ED.6070101@denx.de> you wrote:
> 
> It is ok to move the information to bdinfo, but then it should not break
> the consistence: all i.MX should follow the same rule. This makes also
> easier for board maintainers to switch from a SOC to the next one,
> because they have to provide quite the same functions in their code (and
> using the same functions, too.).

100% agreement.

> I agree with you that this should be done by a separate patchset: it is
> not related to the Wandboard. But if we go on this way, the same changes
> should be done for all i.MX, not only for i.MX5/i.MX6.

Indeed.  It was just with this specific board that I got awre of the
issue.  I fully agree that we should implement a common solution that
covers all these systems.

Best regards,

Wolfgang Denk