Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-07-11 Thread Dirk Behme

On 22.06.2012 18:04, Eric Nelson wrote:

On 06/21/2012 03:57 PM, Wolfgang Denk wrote:

Dear Eric Nelson,

In message4fcd17a5.6030...@boundarydevices.com  you wrote:

The function of the watchdog should be reserved for auch systems who
need it to implement security related functions; messing with it in
unrelated places (like setting it into unexpected states and or
timeouts) may cause undefined behaviour on such systems.

Don't do it.


My comment is that reset_cpu() should probably not return, and
at the moment it will. The minimum WDT timeout value is 1/2 second, so
execution will continue for some time less than that.

It seems to me that this might be a bad thing, allowing unexpected
execution of commands in a script after the execution of the 'reset'
command.

If I understand your comment, you're saying that reset_cpu() should
not be implemented using the watchdog.

Is that right?

No, that was not my intention.  I must have missed the fact that you
were talking about reset_cpu() and only this function.  Please ignore
me.



NP. I'll submit a patch to address the fact that the reset returns.


Ping on this whole patch series ;)

Best regards

Dirk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-07-11 Thread Troy Kisky

On 7/10/2012 11:32 PM, Dirk Behme wrote:



Ping on this whole patch series ;)

Best regards

Dirk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


The last version of the series was
[RFC v2 1/3] imx-common/cmd_resetmode.c: add imx resetmode command


But it received no comments or acks.
I was going to resend after next version of u-boot released.

Troy


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-07-11 Thread Dirk Behme

On 11.07.2012 20:01, Troy Kisky wrote:

On 7/10/2012 11:32 PM, Dirk Behme wrote:



Ping on this whole patch series ;)

Best regards

Dirk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


The last version of the series was
[RFC v2 1/3] imx-common/cmd_resetmode.c: add imx resetmode command


But it received no comments or acks.
I was going to resend after next version of u-boot released.


Do as you like. But as this patch series was sent while the merge 
window was open, I would think it would be possible to still get it 
into _this_ u-boot release. Just my $0.02, though ;)


Best regards

Dirk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-22 Thread Eric Nelson

On 06/21/2012 03:57 PM, Wolfgang Denk wrote:

Dear Eric Nelson,

In message4fcd17a5.6030...@boundarydevices.com  you wrote:



The function of the watchdog should be reserved for auch systems who
need it to implement security related functions; messing with it in
unrelated places (like setting it into unexpected states and or
timeouts) may cause undefined behaviour on such systems.

Don't do it.



My comment is that reset_cpu() should probably not return, and
at the moment it will. The minimum WDT timeout value is 1/2 second, so
execution will continue for some time less than that.

It seems to me that this might be a bad thing, allowing unexpected
execution of commands in a script after the execution of the 'reset'
command.

If I understand your comment, you're saying that reset_cpu() should
not be implemented using the watchdog.

Is that right?


No, that was not my intention.  I must have missed the fact that you
were talking about reset_cpu() and only this function.  Please ignore
me.



NP. I'll submit a patch to address the fact that the reset returns.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-21 Thread Wolfgang Denk
Dear Eric Nelson,

In message 4fcd17a5.6030...@boundarydevices.com you wrote:

  The function of the watchdog should be reserved for auch systems who
  need it to implement security related functions; messing with it in
  unrelated places (like setting it into unexpected states and or
  timeouts) may cause undefined behaviour on such systems.
 
  Don't do it.
 
 
 My comment is that reset_cpu() should probably not return, and
 at the moment it will. The minimum WDT timeout value is 1/2 second, so
 execution will continue for some time less than that.
 
 It seems to me that this might be a bad thing, allowing unexpected
 execution of commands in a script after the execution of the 'reset'
 command.
 
 If I understand your comment, you're saying that reset_cpu() should
 not be implemented using the watchdog.
 
 Is that right?

No, that was not my intention.  I must have missed the fact that you
were talking about reset_cpu() and only this function.  Please ignore
me.

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: w...@denx.de
In Christianity neither morality nor religion come into contact with
reality at any point.  - Friedrich Nietzsche
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-04 Thread Wolfgang Denk
Dear Troy Kisky,

In message 1338066111-5835-1-git-send-email-troy.ki...@boundarydevices.com 
you wrote:
 This is useful for forcing the ROM's
 usb downloader to activate upon a watchdog reset.
 Or, you can boot from either SD Card.
 
 Currently, support added for MX53 and MX6Q
 Signed-off-by: Troy Kisky troy.ki...@boundarydevices.com

This is highly architecture specific code.  I do not want to have this
in the common/ directory.

 --- /dev/null
 +++ b/common/cmd_rsmode.c
 @@ -0,0 +1,118 @@
...
 +#ifdef CONFIG_MX53
...
 +#ifdef CONFIG_MX6Q
...

Instead of starting a (probably growing) list of #ifdef's here, you
whould rather just include a header file which gets auto-selected for
the CPU in use by the usual config mechanism.

So please move allthese defines into appropriate header files.

 +#define SBMR_COPY_ADDR ((struct srtc_regs *)SRTC_BASE_ADDR)-lpgr

NAK.  This is ugly and does not scale.  Feel free to provide
architecturre specific (inline?) accessor functions to this register.

 +int do_rsmode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 +{
 + int i;
 + if (argc  2) {

Empty line between declarations and code, please.

 +options:
 + printf(Options:\n);
 + for (i = 0; i  ARRAY_SIZE(modes); i++)
 + printf(%s\n, modes[i].name);
 + return 0;

NAK.  The command should return proper error codes.  Please use the
regular usage() call here.

 + if (i = ARRAY_SIZE(modes))
 + goto options;

Please get rid of this goto.

 + writel(modes[i].cfg_val, SBMR_COPY_ADDR);
 +#ifdef SMBR_COPY_ENABLE_ADDR
 + {
 + unsigned reg = readl(SMBR_COPY_ENABLE_ADDR);
 + if (i)
 + reg |= 1  28;
 + else
 + reg = ~(1  28);
 + writel(reg, SMBR_COPY_ENABLE_ADDR);
 + }
 +#endif

You mean, if SMBR_COPY_ENABLE_ADDR was not defined, the command would
silently turn into a NOP?  One more reson not to allow such code.

 +U_BOOT_CMD(
 + rsmode, 2, 0, do_rsmode,
 + change reset mode to normal/usb/sata/ecspi1:1/esdhc1,

How do you intend to keep this doucmentation in sync with the code?
Either the above code (reading the entries to mode[]) is redundant, or
this is incorrect - or probably both.

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: w...@denx.de
The question of whether a computer can think is no more  interesting
than the question of whether a submarine can swim
- Edsgar W.  Dijkstra
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-04 Thread Wolfgang Denk
Dear Eric Nelson,

In message 4fc4dc4e.9030...@boundarydevices.com you wrote:
 
 Writing a 4 to the WDOG_BASE register enables the watchdog with a 1/2s delay.
 I think there should be a delay or while(1) loop afterwards though. It appears
 that the reset_cpu() routine will return.

I recommend NOT messing with the watchdog for any unrelated functions.

The function of the watchdog should be reserved for auch systems who
need it to implement security related functions; messing with it in
unrelated places (like setting it into unexpected states and or
timeouts) may cause undefined behaviour on such systems.

Don't do it.

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: w...@denx.de
Name one thing windows is better than unix in?
  Making money for Microsoft?
 -- Randal L. Schwartz in 8cvi5t4c3t@gadget.cscaper.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-04 Thread Eric Nelson

On 06/04/2012 12:52 PM, Wolfgang Denk wrote:

Dear Eric Nelson,

In message4fc4dc4e.9030...@boundarydevices.com  you wrote:


Writing a 4 to the WDOG_BASE register enables the watchdog with a 1/2s delay.
I think there should be a delay or while(1) loop afterwards though. It appears
that the reset_cpu() routine will return.


I recommend NOT messing with the watchdog for any unrelated functions.

The function of the watchdog should be reserved for auch systems who
need it to implement security related functions; messing with it in
unrelated places (like setting it into unexpected states and or
timeouts) may cause undefined behaviour on such systems.

Don't do it.



Hi Wolfgang,

My comment is that reset_cpu() should probably not return, and
at the moment it will. The minimum WDT timeout value is 1/2 second, so
execution will continue for some time less than that.

It seems to me that this might be a bad thing, allowing unexpected
execution of commands in a script after the execution of the 'reset'
command.

If I understand your comment, you're saying that reset_cpu() should
not be implemented using the watchdog.

Is that right?

Please advise,


Eric
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-06-04 Thread Troy Kisky

On 6/4/2012 12:46 PM, Wolfgang Denk wrote:

Dear Troy Kisky,

In message1338066111-5835-1-git-send-email-troy.ki...@boundarydevices.com  
you wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.

Currently, support added for MX53 and MX6Q
Signed-off-by: Troy Kiskytroy.ki...@boundarydevices.com

This is highly architecture specific code.  I do not want to have this
in the common/ directory.


--- /dev/null
+++ b/common/cmd_rsmode.c
@@ -0,0 +1,118 @@

...

+#ifdef CONFIG_MX53

...

+#ifdef CONFIG_MX6Q

...

Instead of starting a (probably growing) list of #ifdef's here, you
whould rather just include a header file which gets auto-selected for
the CPU in use by the usual config mechanism.

So please move allthese defines into appropriate header files.


+#define SBMR_COPY_ADDR((struct srtc_regs *)SRTC_BASE_ADDR)-lpgr

NAK.  This is ugly and does not scale.  Feel free to provide
architecturre specific (inline?) accessor functions to this register.


+int do_rsmode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+   int i;
+   if (argc  2) {

Empty line between declarations and code, please.


+options:
+   printf(Options:\n);
+   for (i = 0; i  ARRAY_SIZE(modes); i++)
+   printf(%s\n, modes[i].name);
+   return 0;

NAK.  The command should return proper error codes.  Please use the
regular usage() call here.


+   if (i= ARRAY_SIZE(modes))
+   goto options;

Please get rid of this goto.


+   writel(modes[i].cfg_val, SBMR_COPY_ADDR);
+#ifdef SMBR_COPY_ENABLE_ADDR
+   {
+   unsigned reg = readl(SMBR_COPY_ENABLE_ADDR);
+   if (i)
+   reg |= 1  28;
+   else
+   reg= ~(1  28);
+   writel(reg, SMBR_COPY_ENABLE_ADDR);
+   }
+#endif

You mean, if SMBR_COPY_ENABLE_ADDR was not defined, the command would
silently turn into a NOP?  One more reson not to allow such code.


+U_BOOT_CMD(
+   rsmode, 2, 0, do_rsmode,
+   change reset mode to normal/usb/sata/ecspi1:1/esdhc1,

How do you intend to keep this doucmentation in sync with the code?
Either the above code (reading the entries to mode[]) is redundant, or
this is incorrect - or probably both.


Thanks for the feedback. I'll try building this string at run-time to 
keep consistent.




Best regards,

Wolfgang Denk



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-05-29 Thread Eric Nelson

Hi Dirk,

On 05/28/2012 10:36 AM, Dirk Behme wrote:

On 27.05.2012 19:16, Eric Nelson wrote:

Hi Dirk,

On 05/26/2012 11:04 PM, Dirk Behme wrote:

Hi Troy,

On 26.05.2012 23:01, Troy Kisky wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.


Thanks!

Just for my understanding: Who will enable/run the watchdog reset?
It was my
understanding that after writing gpr9 and gpr10 someone has to
enable the
watchdog to execute the reset?


Or just issue the 'reset' command, so we can prevent the need to
re-program the SPI ROM with the boot to SD shim with something like
this:

U-Boot set bootcmd 'rsmode esdhc3  reset'  saveenv

I discussed this with Troy yesterday and would like to get some feedback
from those most likely to use the features of this code.


I like this idea.

But I haven't looked into the detailed implementation of the 'reset' command for
i.MX6. Does it run/execute the necessary watchdog reset?


It does. I just traced this code:
common/cmd_boot defines the command
arch/arm/lib/reset.c calls reset_cpu()

reset_cpu() is defined here:

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/imx-common/cpu.c;h=b3195dd6fb2a9817d37f2d6e0e37b83833ed2d7b;hb=HEAD#l127

Writing a 4 to the WDOG_BASE register enables the watchdog with a 1/2s delay.
I think there should be a delay or while(1) loop afterwards though. It appears
that the reset_cpu() routine will return.



Troy's implementation exposes the internals and all of the power
of the reset mode override feature. Through the 'rsmode' command,
you can set up what happens during the next watchdog reset and then do
something else.

For instance, you could say 'rsmode esdhc1', set up the watchdog and
then launch an O/S which should take over the watchdog. If something
fails, the ROM will attempt to boot to esdhc1, providing a fallback
mechanism of sorts.


What I read from above is that we need to flavors of the reset (watchdog setup)
command? Or better: Two ways of enabling the watchdog for the two usage
scenarios I read above:

Usage scenario 1: We wan't to reset/reboot as soon as possible after doing the
rsmode command. This would be used as a replacement for what we use at the
moment with the SabreLite boards: They boot from SPI NOR, but we like to switch
as fast as possible to SD boot. This implies that the watchdog reset is needed
as fast as possible after the rsmode command is done.

Usage scenario 2: This is what you describe above: Set the rsmode, set up the
watchdog and let the OS take over the watchdog. This would imply a longer
watchdog reset period, to give the OS some time to take over.


While there may be some conditions under which this will be used,
a few dominant use cases and these could be made simpler to understand
and use.

1. Change 'rsmode' to 'rstmode'. I suspect that everyone will have
an easier time understanding that the latter means 'reset mode'.


Yes, this sounds better :) I'm not sure about the naming conventions, but
spending some additional characters to name it 'resetmode' would work, too?


2. Implement 'download_mode' or 'downusb' as a short-hand for the
sequence:

U-Boot rsmode usb  reset

This sequence will be executed mostly by those on this list and
the abbreviation is worth the small addition of code.

I already have 'down' in muscle memory and using it will ease
the pain for the (hopefully short) interval while I'm using
both the Freescale 2009.08 code base and mainline.

3. Similarly, implement 'bootsd' or 'bootmmc' with a parameter that
matches the SD card naming used by other U-Boot commands.

How many of you know without grepping or pulling up a schematic
which device corresponds to 'mmc 0'?

It seems more intuitive to issue this command:

U-Boot bootmmc 0

instead of this:

U-Boot rsmode esdhc3  reset

4. Same rationale for 'bootsata'.

5. Allow boards to say which devices are even exposed in their config
headers. i.e.
#define BOARD_HAS_ESDHC3

...

#ifdef BOARD_HAS_ESDHC3
{esdhc1, mkval(0x40, 0x20, 0x00, 0x12)},
#endif

This will allow a form of self-documentation when running 'rsmode'
with no parameters.


This sounds good, too.


I doubt any board supports all of the boot options.

Let us know your thoughts on this and thanks for the code Troy!


Yes, many thanks for the code!

I think we should clarify how the watchdog is handled in this scenario. By the
reset command? And do we need a 'long timeout' and a 'immediate timeout' 
watchdog?


These are probably different things. It seems that the fact that the watchdog
timer is used to implement reset is just an implementation detail.

Implementation of a true watchdog timer (i.e. 'long timeout') seems like a
separate feature.

Regards,


Eric
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-05-28 Thread Dirk Behme

On 27.05.2012 19:16, Eric Nelson wrote:

Hi Dirk,

On 05/26/2012 11:04 PM, Dirk Behme wrote:

Hi Troy,

On 26.05.2012 23:01, Troy Kisky wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.


Thanks!

Just for my understanding: Who will enable/run the watchdog reset?
It was my
understanding that after writing gpr9 and gpr10 someone has to
enable the
watchdog to execute the reset?


Or just issue the 'reset' command, so we can prevent the need to
re-program the SPI ROM with the boot to SD shim with something like
this:

U-Boot set bootcmd 'rsmode esdhc3  reset'  saveenv

I discussed this with Troy yesterday and would like to get some feedback
from those most likely to use the features of this code.


I like this idea.

But I haven't looked into the detailed implementation of the 'reset' 
command for i.MX6. Does it run/execute the necessary watchdog reset?



Troy's implementation exposes the internals and all of the power
of the reset mode override feature. Through the 'rsmode' command,
you can set up what happens during the next watchdog reset and then do
something else.

For instance, you could say 'rsmode esdhc1', set up the watchdog and
then launch an O/S which should take over the watchdog. If something
fails, the ROM will attempt to boot to esdhc1, providing a fallback
mechanism of sorts.


What I read from above is that we need to flavors of the reset 
(watchdog setup) command? Or better: Two ways of enabling the watchdog 
for the two usage scenarios I read above:


Usage scenario 1: We wan't to reset/reboot as soon as possible after 
doing the rsmode command. This would be used as a replacement for what 
we use at the moment with the SabreLite boards: They boot from SPI 
NOR, but we like to switch as fast as possible to SD boot. This 
implies that the watchdog reset is needed as fast as possible after 
the rsmode command is done.


Usage scenario 2: This is what you describe above: Set the rsmode, set 
up the watchdog and let the OS take over the watchdog. This would 
imply a longer watchdog reset period, to give the OS some time to take 
over.



While there may be some conditions under which this will be used,
a few dominant use cases and these could be made simpler to understand
and use.

1. Change 'rsmode' to 'rstmode'. I suspect that everyone will have
an easier time understanding that the latter means 'reset mode'.


Yes, this sounds better :) I'm not sure about the naming conventions, 
but spending some additional characters to name it 'resetmode' would 
work, too?



2. Implement 'download_mode' or 'downusb' as a short-hand for the
sequence:

U-Boot rsmode usb  reset

This sequence will be executed mostly by those on this list and
the abbreviation is worth the small addition of code.

I already have 'down' in muscle memory and using it will ease
the pain for the (hopefully short) interval while I'm using
both the Freescale 2009.08 code base and mainline.

3. Similarly, implement 'bootsd' or 'bootmmc' with a parameter that
matches the SD card naming used by other U-Boot commands.

How many of you know without grepping or pulling up a schematic
which device corresponds to 'mmc 0'?

It seems more intuitive to issue this command:

U-Boot bootmmc 0

instead of this:

U-Boot rsmode esdhc3  reset

4. Same rationale for 'bootsata'.

5. Allow boards to say which devices are even exposed in their config
headers. i.e.
#define BOARD_HAS_ESDHC3

...

#ifdef BOARD_HAS_ESDHC3
{esdhc1, mkval(0x40, 0x20, 0x00, 0x12)},
#endif

This will allow a form of self-documentation when running 'rsmode'
with no parameters.


This sounds good, too.


I doubt any board supports all of the boot options.

Let us know your thoughts on this and thanks for the code Troy!


Yes, many thanks for the code!

I think we should clarify how the watchdog is handled in this 
scenario. By the reset command? And do we need a 'long timeout' and a 
'immediate timeout' watchdog?


Best regards

Dirk

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-05-27 Thread Dirk Behme

Hi Troy,

On 26.05.2012 23:01, Troy Kisky wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.


Thanks!

Just for my understanding: Who will enable/run the watchdog reset? It 
was my understanding that after writing gpr9 and gpr10 someone has to 
enable the watchdog to execute the reset?


Best regards

Dirk

P.S.: I pushed this to my staging branch

https://github.com/dirkbehme/u-boot-imx6/commits/u-boot-imx-staging


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-05-27 Thread Eric Nelson

Hi Dirk,

On 05/26/2012 11:04 PM, Dirk Behme wrote:

Hi Troy,

On 26.05.2012 23:01, Troy Kisky wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.


Thanks!

Just for my understanding: Who will enable/run the watchdog reset? It was my
understanding that after writing gpr9 and gpr10 someone has to enable the
watchdog to execute the reset?


Or just issue the 'reset' command, so we can prevent the need to
re-program the SPI ROM with the boot to SD shim with something like
this:

U-Boot set bootcmd 'rsmode esdhc3  reset'  saveenv

I discussed this with Troy yesterday and would like to get some feedback
from those most likely to use the features of this code.

Troy's implementation exposes the internals and all of the power
of the reset mode override feature. Through the 'rsmode' command,
you can set up what happens during the next watchdog reset and then do
something else.

For instance, you could say 'rsmode esdhc1', set up the watchdog and
then launch an O/S which should take over the watchdog. If something
fails, the ROM will attempt to boot to esdhc1, providing a fallback
mechanism of sorts.

While there may be some conditions under which this will be used,
a few dominant use cases and these could be made simpler to understand
and use.

1. Change 'rsmode' to 'rstmode'. I suspect that everyone will have
   an easier time understanding that the latter means 'reset mode'.

2. Implement 'download_mode' or 'downusb' as a short-hand for the
   sequence:

U-Boot rsmode usb  reset

   This sequence will be executed mostly by those on this list and
   the abbreviation is worth the small addition of code.

   I already have 'down' in muscle memory and using it will ease
   the pain for the (hopefully short) interval while I'm using
   both the Freescale 2009.08 code base and mainline.

3. Similarly, implement 'bootsd' or 'bootmmc' with a parameter that
   matches the SD card naming used by other U-Boot commands.

   How many of you know without grepping or pulling up a schematic
   which device corresponds to 'mmc 0'?

   It seems more intuitive to issue this command:

U-Boot bootmmc 0

   instead of this:

U-Boot rsmode esdhc3  reset

4. Same rationale for 'bootsata'.

5. Allow boards to say which devices are even exposed in their config
   headers. i.e.
#define BOARD_HAS_ESDHC3

...

#ifdef BOARD_HAS_ESDHC3
{esdhc1,mkval(0x40, 0x20, 0x00, 0x12)},
#endif

   This will allow a form of self-documentation when running 'rsmode'
   with no parameters.

   I doubt any board supports all of the boot options.

Let us know your thoughts on this and thanks for the code Troy!

Regards,


Eric
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 1/3] common/cmd_rsmode.c: add imx reset mode command

2012-05-27 Thread Eric Nelson

Hi Troy,

Nitpicks below.

On 05/26/2012 02:01 PM, Troy Kisky wrote:

This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.

Currently, support added for MX53 and MX6Q
Signed-off-by: Troy Kiskytroy.ki...@boundarydevices.com

Note: MX53 support untested.
---
  arch/arm/include/asm/arch-mx5/imx-regs.h |   18 +
  arch/arm/include/asm/arch-mx6/imx-regs.h |   21 +
  common/Makefile  |1 +
  common/cmd_rsmode.c  |  118 ++
  4 files changed, 158 insertions(+), 0 deletions(-)
  create mode 100644 common/cmd_rsmode.c



 snip


diff --git a/common/Makefile b/common/Makefile
index d9f10f3..8e17cec 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -139,6 +139,7 @@ COBJS-$(CONFIG_CMD_PORTIO) += cmd_portio.o
  COBJS-$(CONFIG_CMD_PXE) += cmd_pxe.o
  COBJS-$(CONFIG_CMD_REGINFO) += cmd_reginfo.o
  COBJS-$(CONFIG_CMD_REISER) += cmd_reiser.o
+COBJS-$(CONFIG_CMD_RSMODE) += cmd_rsmode.o
  COBJS-$(CONFIG_CMD_SATA) += cmd_sata.o
  COBJS-$(CONFIG_CMD_SF) += cmd_sf.o
  COBJS-$(CONFIG_CMD_SCSI) += cmd_scsi.o


Since this code is very machine-specific, should this code
go into the board/freescale/common/ or
arch/arm/cpu/armv7/imx-common/ directory?


diff --git a/common/cmd_rsmode.c b/common/cmd_rsmode.c
new file mode 100644


 snip


+int do_rsmode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+   int i;
+   if (argc  2) {
+options:
+   printf(Options:\n);
+   for (i = 0; i  ARRAY_SIZE(modes); i++)
+   printf(%s\n, modes[i].name);


A leading tab here will make the output easier to parse:

U-Boot rsmode
Options:
usb
sata

U-Boot rsmode
Options:
usb
sata
ecspi.0
...


+   return 0;
+   }
+   for (i = 0; i  ARRAY_SIZE(modes); i++) {
+   if (!strcmp(modes[i].name, argv[1]))
+   break;
+   }

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot