common/cmd_rsmode.c: add imx reset mode command

PATCH New Troy Kisky 5 participants 17 messages May 26, 2012 – Jul 11, 2012

Subsystems (2)

Arm (32-bit) Arm i.MX and Layerscape

lore

Share & tools

lore

Troy Kisky

author

2012-05-26 21:01 UTC #0 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

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 <[email protected]>

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

arch/arm/include/asm/arch-mx5/imx-regs.h +18 -0

index cef4190..4f27c00 100644
@@ -456,6 +456,24 @@ struct src {
 	u32	simr;
 };
 
+struct srtc_regs {
+	u32	lpscmr;		/* 0x00 */
+	u32	lpsclr;		/* 0x04 */
+	u32	lpsar;		/* 0x08 */
+	u32	lpsmcr;		/* 0x0c */
+	u32	lpcr;		/* 0x10 */
+	u32	lpsr;		/* 0x14 */
+	u32	lppdr;		/* 0x18 */
+	u32	lpgr;		/* 0x1c */
+	u32	hpcmr;		/* 0x20 */
+	u32	hpclr;		/* 0x24 */
+	u32	hpamr;		/* 0x28 */
+	u32	hpalr;		/* 0x2c */
+	u32	hpcr;		/* 0x30 */
+	u32	hpisr;		/* 0x34 */
+	u32	hpienr;		/* 0x38 */
+};
+
 /* CSPI registers */
 struct cspi_regs {
 	u32 rxdata;

arch/arm/include/asm/arch-mx6/imx-regs.h +21 -0

index e165810..5acc9ca 100644
@@ -445,5 +445,26 @@ struct iomuxc_base_regs {
 	u32     daisy[104];     /* 0x7b0..94c */
 };
 
+struct src_regs {
+	u32	scr;		/* 0x00 */
+	u32	sbmr1;		/* 0x04 */
+	u32	srsr;		/* 0x08 */
+	u32	reserved1;	/* 0x0c */
+	u32	reserved2;	/* 0x10 */
+	u32	sisr;		/* 0x14 */
+	u32	simr;		/* 0x18 */
+	u32	sbmr2;		/* 0x1c */
+	u32	gpr1;		/* 0x20 */
+	u32	gpr2;		/* 0x24 */
+	u32	gpr3;		/* 0x28 */
+	u32	gpr4;		/* 0x2c */
+	u32	gpr5;		/* 0x30 */
+	u32	gpr6;		/* 0x34 */
+	u32	gpr7;		/* 0x38 */
+	u32	gpr8;		/* 0x3c */
+	u32	gpr9;		/* 0x40 */
+	u32	gpr10;		/* 0x44 */
+};
+
 #endif /* __ASSEMBLER__*/
 #endif /* __ASM_ARCH_MX6_IMX_REGS_H__ */

common/Makefile +1 -0

index d9f10f3..8e17cec 100644
@@ -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

common/cmd_rsmode.c new file +118 -0

118 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Troy Kisky

author

2012-05-26 21:01 UTC #1 lore raw

PATCH 2/3 mx6qsabrelite: select CONFIG_CMD_RSMODE

This allows a watchdog reset to start the ROM's
usb downloader, or boot from an sdcard.

Signed-off-by Troy Kisky <[email protected]>

---
 include/configs/mx6qsabrelite.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

include/configs/mx6qsabrelite.h +3 -0

index feabc05..c8bb4cf 100644
@@ -111,6 +111,9 @@
 #define CONFIG_MXC_USB_PORTSC	(PORT_PTS_UTMI | PORT_PTS_PTW)
 #define CONFIG_MXC_USB_FLAGS	0
 
+/* Miscellaneous commands */
+#define CONFIG_CMD_RSMODE
+
 /* allow to overwrite serial and ethaddr */
 #define CONFIG_ENV_OVERWRITE
 #define CONFIG_CONS_INDEX	       1

Signature · 1 line

1.7.5.4

Troy Kisky

author

2012-05-26 21:01 UTC #2 lore raw

PATCH 3/3 mx53evk: select CONFIG_CMD_RSMODE

In reply to Troy Kisky

This allows a watchdog reset to start the ROM's
usb/serial downloader, or boot from an sdcard.

Signed-off-by Troy Kisky <[email protected]>

Compile tested only, I don't have a mx53evk.

---
 include/configs/mx53evk.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

include/configs/mx53evk.h +3 -0

index a77e5b2..fab2f20 100644
@@ -90,6 +90,9 @@
 #define CONFIG_CMD_NET
 #define CONFIG_CMD_DATE
 
+/* Miscellaneous commands */
+#define CONFIG_CMD_RSMODE
+
 /* allow to overwrite serial and ethaddr */
 #define CONFIG_ENV_OVERWRITE
 #define CONFIG_CONS_INDEX		1

Signature · 1 line

1.7.5.4

Dirk Behme

2012-05-27 06:04 UTC #3 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

In reply to Troy Kisky

Hi Troy,

Quoted 3 lines · Troy Kisky

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

Eric Nelson

2012-05-27 17:16 UTC #4 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Hi Dirk,

Quoted 7 lines · Dirk Behme

Hi Troy,
Quoted 3 lines · Troy Kisky
This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.

Quoted 6 lines

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

Dirk Behme

2012-05-28 17:36 UTC #5 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 18 lines · Eric Nelson

Hi Dirk,
Quoted 7 lines · Dirk Behme
Hi Troy,
Quoted 3 lines · Troy Kisky
This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.
Quoted 8 lines
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?

Quoted 8 lines

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?

Quoted 9 lines

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.

Quoted 6 lines

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?

Quoted 32 lines

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

Quoted 8 lines

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

Quoted 3 lines

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

Eric Nelson

2012-05-29 14:25 UTC #6 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Hi Dirk,

Quoted 25 lines · Dirk Behme

Quoted 24 lines · Eric Nelson
Hi Dirk,
Quoted 7 lines · Dirk Behme
Hi Troy,
Quoted 3 lines · Troy Kisky
This is useful for forcing the ROM's
usb downloader to activate upon a watchdog reset.
Or, you can boot from either SD Card.
Quoted 8 lines
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

Quoted 8 lines

Quoted 2 lines
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.

Quoted 77 lines

Quoted 9 lines
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.
Quoted 6 lines
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?
Quoted 32 lines
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
Quoted 8 lines
...
#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.

Quoted 9 lines

Quoted 3 lines
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

Wolfgang Denk

2012-06-04 19:52 UTC #7 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Dear Eric Nelson,

Quoted 4 lines · In message <[email protected]> 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

Signature · 6 lines

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
Name one thing windows is better than unix in?
                                          Making money for Microsoft?
         -- Randal L. Schwartz in <[email protected]>

Eric Nelson

2012-06-04 20:16 UTC #8 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 8 lines · Wolfgang Denk

Dear Eric Nelson,
Quoted 4 lines · In message<[email protected]> 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.

Quoted 9 lines

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

Wolfgang Denk

2012-06-21 20:57 UTC #9 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Dear Eric Nelson,

Quoted 13 lines · In message <[email protected]> you wrote:

Quoted 8 lines
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.

Quoted 8 lines

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

Signature · 5 lines

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
"In Christianity neither morality nor religion come into contact with
reality at any point."                          - Friedrich Nietzsche

Eric Nelson

2012-06-22 16:04 UTC #10 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 20 lines · Wolfgang Denk

Dear Eric Nelson,
Quoted 17 lines · In message<[email protected]> you wrote:
Quoted 8 lines
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.

Quoted 9 lines

Quoted 4 lines
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.

Dirk Behme

2012-07-11 06:32 UTC #11 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 22 lines · Eric Nelson

Quoted 21 lines · Wolfgang Denk
Dear Eric Nelson,
Quoted 18 lines · In message<[email protected]> you wrote:
Quoted 7 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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

Troy Kisky

author

2012-07-11 18:01 UTC #12 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 4 lines · Dirk Behme

Ping on this whole patch series ;)

Quoted 8 lines

Best regards
Dirk
_______________________________________________
U-Boot mailing list
U-Boot at 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

Dirk Behme

2012-07-11 18:44 UTC #13 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 13 lines · Troy Kisky

Quoted 4 lines · Dirk Behme
Ping on this whole patch series ;)
Quoted 8 lines
Best regards
Dirk
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 6 lines

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

Eric Nelson

2012-05-27 17:33 UTC #14 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

In reply to Troy Kisky

Hi Troy,

Nitpicks below.

Quoted 24 lines · Troy Kisky

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<[email protected]>
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>
common/Makefile +0 -0
index d9f10f3..8e17cec 100644

common/Makefile COBJS-$(CONFIG_CMD_PORTIO) += cmd_portio.o · line 145

Quoted 7 lines

common/Makefile +1 -0

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

common/cmd_rsmode.c line 92

Quoted 13 lines

common/cmd_rsmode.c new file +8 -0

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

Quoted 6 lines

common/cmd_rsmode.c new file +6 -0

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

Wolfgang Denk

2012-06-04 19:46 UTC #15 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

In reply to Troy Kisky

Dear Troy Kisky,

Quoted 6 lines · In message <[email protected]>…

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 <[email protected]>

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

Quoted 3 lines

common/cmd_rsmode.c new file +0 -0
@@ -0,0 +1,118 @@

...

Quoted 1 line

+#ifdef CONFIG_MX53

...

Quoted 1 line

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

Quoted 1 line

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

common/cmd_rsmode.c line 88

Quoted 4 lines

common/cmd_rsmode.c new file +4 -0

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

common/cmd_rsmode.c line 93

Quoted 5 lines

common/cmd_rsmode.c new file +5 -0

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

common/cmd_rsmode.c line 100

Quoted 2 lines

common/cmd_rsmode.c new file +2 -0
+	if (i >= ARRAY_SIZE(modes))
+		goto options;

Please get rid of this goto.

common/cmd_rsmode.c line 111

Quoted 11 lines

common/cmd_rsmode.c new file +11 -0

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

common/cmd_rsmode.c line 117

Quoted 3 lines

common/cmd_rsmode.c new file +3 -0

+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

Signature · 6 lines

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
"The question of whether a computer can think is no more  interesting
than the question of whether a submarine can swim"
                                                - Edsgar W.  Dijkstra

Troy Kisky

author

2012-06-04 20:43 UTC #16 lore raw

PATCH 1/3 common/cmd_rsmode.c: add imx reset mode command

Quoted 63 lines · Wolfgang Denk

Dear Troy Kisky,
Quoted 6 lines · In message<[email protected]>…
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<[email protected]>
This is highly architecture specific code. I do not want to have this in the common/ directory.
Quoted 3 lines
common/cmd_rsmode.c new file +0 -0
@@ -0,0 +1,118 @@
...
Quoted 1 line
+#ifdef CONFIG_MX53
...
Quoted 1 line
+#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.
Quoted 1 line
+#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.
common/cmd_rsmode.c
Quoted 4 lines
common/cmd_rsmode.c new file +4 -0
+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.
common/cmd_rsmode.c line 93
Quoted 5 lines
common/cmd_rsmode.c new file +5 -0
+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.
common/cmd_rsmode.c
Quoted 2 lines
common/cmd_rsmode.c new file +2 -0
+	if (i>= ARRAY_SIZE(modes))
+		goto options;
Please get rid of this goto.
common/cmd_rsmode.c line 111
Quoted 11 lines
common/cmd_rsmode.c new file +11 -0
+	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.

Quoted 6 lines

common/cmd_rsmode.c line 117
Quoted 3 lines
common/cmd_rsmode.c new file +3 -0
+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.

Quoted 5 lines

Best regards,
Wolfgang Denk