diff mbox

[U-Boot,3/5] ARM: OMAP: Correct save_boot_params and replace with 'C' function

Message ID 1366038520-7492-4-git-send-email-r.sricharan@ti.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

SRICHARAN R April 15, 2013, 3:08 p.m. UTC 
Currently save_boot_params saves the boot parameters passed
from romcode. But this is not stored in a writable location
consistently. So the current code would not work for a
'XIP' boot. Change this by saving the boot parameters in
'gd' which is always writable. Also add a 'C' function
instead of an assembly code that is more readable.

Signed-off-by: Sricharan R <r.sricharan@ti.com>
---
 There is a checkpatch warning because of multiple
 assignments. The code looks readable this way.

 arch/arm/cpu/armv7/omap-common/hwinit-common.c |   50 +++++++++++++++++++++---
 arch/arm/include/asm/global_data.h             |    8 ++++
 arch/arm/include/asm/omap_boot.h               |    1 +
 arch/arm/include/asm/omap_common.h             |    4 +-
 4 files changed, 56 insertions(+), 7 deletions(-)

Comments

Tom Rini April 15, 2013, 3:28 p.m. UTC | #1 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/15/2013 11:08 AM, Sricharan R wrote:
> Currently save_boot_params saves the boot parameters passed from
> romcode. But this is not stored in a writable location 
> consistently. So the current code would not work for a 'XIP' boot.
> Change this by saving the boot parameters in 'gd' which is always
> writable. Also add a 'C' function instead of an assembly code that
> is more readable.
> 
> Signed-off-by: Sricharan R <r.sricharan@ti.com> --- There is a
> checkpatch warning because of multiple assignments. The code looks
> readable this way.
> 

What/where?

[snip]
> +	if ((boot_device >= BOOT_DEVICE_XIP) && +	    (boot_device <=
> BOOT_DEVICE_MMC2)) {

This will need to be rebased to use MMC_BOOT_DEVICES_START/END and I
know you didn't test from eMMC on omap5_uevm then.

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

iQIcBAEBAgAGBQJRbByPAAoJENk4IS6UOR1W9tgQAIO3+Ejnxgy+oqo10SieOsw0
n4dgRvYwppNgpaR4RND5ldjHnYAdr7g3TPOZ8X0tPuw/VwgvpaSkMfAtxzKF3S8m
QjVlPj2+XCTHI9fbg3Wz3kiHRTBYjlBnQ4o3fbnL+h7Sunci8a0HGrLzmfPfz6eA
SR/jLFNtBCQNfh3p7ZikH4pnTCcf/kTfHWkGjIImt5/2Viv8XoQnbwFyTeJS9upZ
s3BPlST+Az7p/D6qFTGR70fmq55H/cIF8iW6KxmD8I8Ezwa2S+hj6FIJ+NmRtEw8
6SOzGw61hGV4y60NelfbRA4KR8GqiFpbb6NtX+bBhX6Vu8DGJpsU/PE1jngA9xSt
RCWqUVxP3LOGCxgI3brZe26kCgHkI8hGx/tDK2e9LF9MspkujL7TVxOTWMEARdZ/
XdJVGljTjovfbIVdWwt0NMASs9aY9gm3lbFtWfST9uZ8esEI1xRc9i19s1aGUf3k
ND87nLgsf6xW1hbJbC+f5+UdUkBizu3+MSCAivNcsc8Haqubm/Ue1s+e7Bh3rkzA
3fo4HfYGqJnAcZiD6EtXhtObOSCEIl3tHOpHkl2kf29G5H8SbPTosXuEZvNESjzx
PSNqyn9jSJHUPQLmPxRhi9ltfm4forZgYyBqQjHMUOBhhMZGdlrqaCSwTdxmwN2L
z62KZtWwP9g5Ac64fWJ+
=8BS/
-----END PGP SIGNATURE-----
SRICHARAN R April 15, 2013, 3:33 p.m. UTC | #2 
On Monday 15 April 2013 08:58 PM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 04/15/2013 11:08 AM, Sricharan R wrote:
>> Currently save_boot_params saves the boot parameters passed from
>> romcode. But this is not stored in a writable location 
>> consistently. So the current code would not work for a 'XIP' boot.
>> Change this by saving the boot parameters in 'gd' which is always
>> writable. Also add a 'C' function instead of an assembly code that
>> is more readable.
>>
>> Signed-off-by: Sricharan R <r.sricharan@ti.com> --- There is a
>> checkpatch warning because of multiple assignments. The code looks
>> readable this way.
>>
> 
> What/where?
>
 In the below line pf the patch.
 gd->arch.omap_boot_params.omap_bootdevice = boot_device =

> [snip]
>> +	if ((boot_device >= BOOT_DEVICE_XIP) && +	    (boot_device <=
>> BOOT_DEVICE_MMC2)) {
> 
> This will need to be rebased to use MMC_BOOT_DEVICES_START/END and I
> know you didn't test from eMMC on omap5_uevm then.
> 
  Yes, i was aware of this. Infact i saw before this series that
  emmc was broken and your patch was fixing that. When i started this
  series, your patch was not merged then. I can rebase on V2.

Regards,
 Sricharan
Michael Cashwell April 15, 2013, 4:22 p.m. UTC | #3 
Hi Sricharan,

I very much like how you've structured this. A vast improvement!

I haven't yet tried to apply the whole series but have one quick comment. In the new function:

static void save_omap_boot_params(void)
{
...
  if (!(omap_hw_init_context() ==
       OMAP_INIT_CONTEXT_UBOOT_AFTER_SPL)) {
      ...
  } else {
      ...
  }

wouldn't it be clearer to drop the boolean negation "!" and exchange the if/else bodies?

Best regards,
-Michael Cashwell
SRICHARAN R April 16, 2013, 4:04 a.m. UTC | #4 
On Monday 15 April 2013 09:52 PM, Michael Cashwell wrote:
> Hi Sricharan,
> 
> I very much like how you've structured this. A vast improvement!
> 
> I haven't yet tried to apply the whole series but have one quick comment. In the new function:
> 
> static void save_omap_boot_params(void)
> {
> ...
>   if (!(omap_hw_init_context() ==
>        OMAP_INIT_CONTEXT_UBOOT_AFTER_SPL)) {
>       ...
>   } else {
>       ...
>   }
> 
> wouldn't it be clearer to drop the boolean negation "!" and exchange the if/else bodies?
> 
 hmm, will do and add a comment as well.

Regards,
 Sricharan
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/omap-common/hwinit-common.c b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
index 70d16a8..602e76e 100644
--- a/arch/arm/cpu/armv7/omap-common/hwinit-common.c
+++ b/arch/arm/cpu/armv7/omap-common/hwinit-common.c
@@ -101,11 +101,6 @@  void omap_rev_string(void)
 }
 
 #ifdef CONFIG_SPL_BUILD
-static void init_boot_params(void)
-{
-	boot_params_ptr = (u32 *) &boot_params;
-}
-
 void spl_display_print(void)
 {
 	omap_rev_string();
@@ -116,6 +111,42 @@  void __weak srcomp_enable(void)
 {
 }
 
+static void save_omap_boot_params(void)
+{
+	u32 rom_params = *((u32 *)OMAP_SRAM_SCRATCH_BOOT_PARAMS);
+	u8 boot_device;
+	u32 dev_desc, dev_data;
+
+	if ((rom_params <  NON_SECURE_SRAM_START) ||
+	    (rom_params > NON_SECURE_SRAM_END))
+		return;
+
+	/*
+	 * rom_params can be type casted to omap_boot_parameters and
+	 * used. But it not correct to assume that romcode structure
+	 * encoding would be same as u-boot. So use the defined offsets.
+	 */
+	gd->arch.omap_boot_params.omap_bootdevice = boot_device =
+				   *((u8 *)(rom_params + BOOT_DEVICE_OFFSET));
+
+	gd->arch.omap_boot_params.ch_flags =
+				*((u8 *)(rom_params + CH_FLAGS_OFFSET));
+
+	if ((boot_device >= BOOT_DEVICE_XIP) &&
+	    (boot_device <= BOOT_DEVICE_MMC2)) {
+		if (!(omap_hw_init_context() ==
+				      OMAP_INIT_CONTEXT_UBOOT_AFTER_SPL)) {
+			dev_desc = *((u32 *)(rom_params + DEV_DESC_PTR_OFFSET));
+			dev_data = *((u32 *)(dev_desc + DEV_DATA_PTR_OFFSET));
+			gd->arch.omap_boot_params.omap_bootmode =
+					*((u32 *)(dev_data + BOOT_MODE_OFFSET));
+		} else {
+			gd->arch.omap_boot_params.omap_bootmode =
+				*((u8 *)(rom_params + BOOT_MODE_OFFSET));
+		}
+	}
+}
+
 /*
  * Routine: s_init
  * Description: Does early system init of watchdog, muxing,  andclocks
@@ -132,6 +163,14 @@  void __weak srcomp_enable(void)
  */
 void s_init(void)
 {
+	/*
+	 * Save the boot parameters passed from romcode.
+	 * We cannot delay the saving further than this,
+	 * to prevent overwrites.
+	 */
+#ifdef CONFIG_SPL_BUILD
+	save_omap_boot_params();
+#endif
 	init_omap_revision();
 	hw_data_init();
 
@@ -156,7 +195,6 @@  void s_init(void)
 
 	/* For regular u-boot sdram_init() is called from dram_init() */
 	sdram_init();
-	init_boot_params();
 #endif
 }
 
diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
index 37ac0da..7611d0a 100644
--- a/arch/arm/include/asm/global_data.h
+++ b/arch/arm/include/asm/global_data.h
@@ -24,6 +24,10 @@ 
 #ifndef	__ASM_GBL_DATA_H
 #define __ASM_GBL_DATA_H
 
+#ifdef CONFIG_OMAP
+#include <asm/omap_boot.h>
+#endif
+
 /* Architecture-specific global data */
 struct arch_global_data {
 #if defined(CONFIG_FSL_ESDHC)
@@ -51,6 +55,10 @@  struct arch_global_data {
 	unsigned long tlb_addr;
 	unsigned long tlb_size;
 #endif
+
+#ifdef CONFIG_OMAP
+	struct omap_boot_parameters omap_boot_params;
+#endif
 };
 
 #include <asm-generic/global_data.h>
diff --git a/arch/arm/include/asm/omap_boot.h b/arch/arm/include/asm/omap_boot.h
index 87a9530..a803965 100644
--- a/arch/arm/include/asm/omap_boot.h
+++ b/arch/arm/include/asm/omap_boot.h
@@ -45,5 +45,6 @@  struct omap_boot_parameters {
 	unsigned char omap_bootdevice;
 	unsigned char reset_reason;
 	unsigned char ch_flags;
+	unsigned long omap_bootmode;
 };
 #endif
diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h
index 6b70dbb..6b73d86 100644
--- a/arch/arm/include/asm/omap_common.h
+++ b/arch/arm/include/asm/omap_common.h
@@ -596,5 +596,7 @@  static inline u32 omap_revision(void)
 #define OMAP_SRAM_SCRATCH_DPLLS_PTR     (SRAM_SCRATCH_SPACE_ADDR + 0x18)
 #define OMAP_SRAM_SCRATCH_VCORES_PTR    (SRAM_SCRATCH_SPACE_ADDR + 0x1C)
 #define OMAP_SRAM_SCRATCH_SYS_CTRL	(SRAM_SCRATCH_SPACE_ADDR + 0x20)
-#define OMAP_SRAM_SCRATCH_SPACE_END	(SRAM_SCRATCH_SPACE_ADDR + 0x24)
+#define OMAP_SRAM_SCRATCH_BOOT_PARAMS	(SRAM_SCRATCH_SPACE_ADDR + 0x24)
+#define OMAP5_SRAM_SCRATCH_SPACE_END	(SRAM_SCRATCH_SPACE_ADDR + 0x28)
+
 #endif /* _OMAP_COMMON_H_ */