Re: [U-Boot] [U-Boot, 2/3] mpc85xx: Add deep sleep framework support
On Thu, 2014-02-13 at 01:05 -0600, Tang Yuantian-B29983 wrote:
Thanks for your review. Please see the reply inline.
Thanks, Yuantian
-----Original Message----- From: Wood Scott-B07421 Sent: 2014年2月13日 星期四 8:44 To: Tang Yuantian-B29983 Cc: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472; Tang@theia.denx.de; u-boot@lists.denx.de; Kushwaha Prabhakar-B32579; Jin Zhengxiong-R64188 Subject: Re: [U-Boot,2/3] mpc85xx: Add deep sleep framework support
On Sun, Jan 26, 2014 at 02:00:40PM +0800, tang yuantian wrote:
From: Tang Yuantian yuantian.tang@freescale.com
When system wakes up from warm reset, control is passed to the primary core that starts executing uboot. After re-initialized some IP blocks, like DDRC, kernel will take responsibility to continue to restore environment it leaves before.
Signed-off-by: Tang Yuantian Yuantian.Tang@freescale.com
Is this for some specific mpc85xx chip (e.g. T1040)? I'm pretty sure this isn't necessary for deep sleep on mpc8536, for example.
Currently, it is used by t1040. But we want it to be more general so that It can be used by later new chips.
But the mechanism is not the same for all mpc85xx derivatives. You'll need a more specific name.
+#ifdef CONFIG_DEEP_SLEEP
CONFIG symbols need to be documented in README...
Where should I add this README?
Under "85xx CPU Options" in the top-level README.
- /* disable the console if boot from warm reset */
- if (in_be32(&gur->scrtsr[0]) & (1 << 3))
gd->flags |=GD_FLG_SILENT | GD_FLG_WARM_BOOT |GD_FLG_DISABLE_CONSOLE; #endif
...and it should probably be a more specific CONFIG_SYS symbol that indicates the presence of this guts bit.
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 33bc900..b3b9250 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -24,6 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; extern void ft_qe_setup(void *blob); extern void ft_fixup_num_cores(void *blob); extern void ft_srio_setup(void *blob); +#ifdef CONFIG_DEEP_SLEEP +extern ulong __bss_end; +#endif
Don't ifdef declarations.
#ifdef CONFIG_MP #include "mp.h" @@ -35,6 +38,9 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) u32 bootpg = determine_mp_bootpg(NULL); u32 id = get_my_id(); const char *enable_method; +#ifdef CONFIG_DEEP_SLEEP
- ulong len;
+#endif
off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "cpu",
4);
while (off != -FDT_ERR_NOTFOUND) { @@ -77,6 +83,25 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) "device_type", "cpu", 4); }
+#ifdef CONFIG_DEEP_SLEEP
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/- len = sizeof(bd_t) + sizeof(gd_t) + (1 << 10);
- /* round up to 4K */
- len = (len + (4096 - 1)) & ~(4096 - 1);
- off = fdt_add_mem_rsv(blob, gd->relocaddr - len,
len + ((ulong)&__bss_end - gd->relocaddr));- if (off < 0)
printf("Failed to reserve memory for warm reset: %s\n",fdt_strerror(off));+#endif
Why do you need to reserve memory for this? We didn't need to for deep sleep on MPC8313ERDB, which also goes through U-Boot on wake. On MPC8313ERDB we transfer control to the kernel before relocating, so U- Boot never touches DDR. bd_t, gd_t, and the stack should be in locked L1 cache, and the u-boot image should be in flash (or locked CPC if this is not a NOR flash boot).
In deep sleep many IP blocks are powered off like DDRC, LIODN, cpu. These IPs are re-initialized after relocating. So, we must jump to kernel after relocating.
The DDR controller is initialized before relocating -- and of course the CPU is powered off on MPC8313ERDB deep sleep as well.
LIODNs are a new concern for deep sleep, but that doesn't mean we should go through a bunch of unrelated code to get there. Refactor the LIODN code to be usable before relocation, and not be tied to fdt fixups.
+#ifndef CONFIG_DEEP_SLEEP /* Reserve spin table page */ if (spin_tbl_addr < memory_limit) { off = fdt_add_mem_rsv(blob, @@ -108,6 +134,7 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) printf("Failed to reserve memory for spin table: %s\n", fdt_strerror(off)); } +#endif
Explain.
Spin_tbl_addr has been reserved already.
Where, and why is it different for non-CONFIG_DEEP_SLEEP?
#if (CONFIG_SYS_NUM_FMAN == 2)
- set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);
- setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,
fman2_liodn_tbl_sz);+#ifdef CONFIG_DEEP_SLEEP
- if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,fman2_liodn_tbl_sz);- }
+#endif #endif #endif
Aren't you breaking non-CONFIG_DEEP_SLEEP here?
No, if deep sleep feature is enabled, we need to further judge whether this boot is Coming from deep sleep by GD_FLG_WARM_BOOT flag.
My point is that if CONFIG_DEEP_SLEEP is not set, you will be skipping those calls to set_liodn() and setup_fman_liodn_base().
#ifdef foo if (!bar) stuff(); #endif
is not equivalent to:
#ifdef foo if (!bar) #endif stuff();
Though the latter is better written as something like:
bool do_stuff = true;
#ifdef foo if (bar) do_stuff = false; #endif
if (do_stuff) stuff();
or
static inline bool is_bar(void) { #ifdef foo return bar; #else return true; #endif }
...
if (!is_bar()) stuff();
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 8e59e8b..1ab05ff 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -117,6 +117,7 @@ struct arch_global_data { #include <asm-generic/global_data.h>
#if 1 +#define GD_FLG_WARM_BOOT 0x10000
Why is this not with the rest of the GD_FLGs, not numerically contiguous, and not commented? What specifically does "warm boot" mean? I think a lot of people would expect it to mean something that looks like a reset at the software level, while possibly not involving a real hardware reset -- coming out of deep sleep is the opposite of that.
Archdef document says it is a warm reset boot. So I name it this way.
Hardware people sometimes use terminology differently than software people.
Other platforms use the same flag, like x86, although I am not sure Whether it is for deep sleep. If you have better name, I love to use it.
It's not yet clear to me that a GD flag is needed for this.
#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm
("r2")
#else /* We could use plain global data, but the resulting code is
bigger */
#define XTRN_DECLARE_GLOBAL_DATA_PTR extern diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 34bbfca..7383e32 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -350,6 +350,13 @@ unsigned long logbuffer_base(void) } #endif
+#ifdef CONFIG_DEEP_SLEEP +int check_warmboot(void) +{
- return !!(gd->flags & GD_FLG_WARM_BOOT); } #endif
Why?
Why what? Why we need it?
It is a help function and used by ASM code in which we can't determine whether it is a warm reset boot.
Why don't you just open code it?
Again, you seem to be breaking the non-CONFIG_DEEP_SLEEP case.
No.
Yes. :-)
+#ifdef CONFIG_DEEP_SLEEP
- /*
* restore 128-byte memory space which corrupted* by DDRC training.*/- if (gd->flags & GD_FLG_WARM_BOOT) {
src = (u64 *)in_be32(&scfg->sparecr[2]);dst = (u64 *)(0);for (i = 0; i < 128/sizeof(u64); i++) {*dst = *src;dst++;src++;}- }
(u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined and GCC has been getting increasingly free with making surprising optimizations based on that (such as assuming any code path that it knows can reach a NULL dereference is dead code and can be removed).
Then how we operate 0 address if not dereferencing NULL pointer?
With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location.
-Scott
On 2014/2/15 星期六 6:21, Scott Wood wrote:
On Thu, 2014-02-13 at 01:05 -0600, Tang Yuantian-B29983 wrote:
Thanks for your review. Please see the reply inline.
Thanks, Yuantian
-----Original Message----- From: Wood Scott-B07421 Sent: 2014年2月13日 星期四 8:44 To: Tang Yuantian-B29983 Cc: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472; Tang@theia.denx.de; u-boot@lists.denx.de; Kushwaha Prabhakar-B32579; Jin Zhengxiong-R64188 Subject: Re: [U-Boot,2/3] mpc85xx: Add deep sleep framework support
On Sun, Jan 26, 2014 at 02:00:40PM +0800, tang yuantian wrote:
From: Tang Yuantian yuantian.tang@freescale.com
When system wakes up from warm reset, control is passed to the primary core that starts executing uboot. After re-initialized some IP blocks, like DDRC, kernel will take responsibility to continue to restore environment it leaves before.
Signed-off-by: Tang Yuantian Yuantian.Tang@freescale.com
Is this for some specific mpc85xx chip (e.g. T1040)? I'm pretty sure this isn't necessary for deep sleep on mpc8536, for example.
Currently, it is used by t1040. But we want it to be more general so that It can be used by later new chips.
But the mechanism is not the same for all mpc85xx derivatives. You'll need a more specific name.
OK, will name it as t104x
+#ifdef CONFIG_DEEP_SLEEP
CONFIG symbols need to be documented in README...
Where should I add this README?
Under "85xx CPU Options" in the top-level README.
Thanks.
- /* disable the console if boot from warm reset */
- if (in_be32(&gur->scrtsr[0]) & (1 << 3))
gd->flags |=GD_FLG_SILENT | GD_FLG_WARM_BOOT |GD_FLG_DISABLE_CONSOLE; #endif
...and it should probably be a more specific CONFIG_SYS symbol that indicates the presence of this guts bit.
where should I put this CONFIG_SYS_'s definition?
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 33bc900..b3b9250 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -24,6 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; extern void ft_qe_setup(void *blob); extern void ft_fixup_num_cores(void *blob); extern void ft_srio_setup(void *blob); +#ifdef CONFIG_DEEP_SLEEP +extern ulong __bss_end; +#endif
Don't ifdef declarations.
#ifdef CONFIG_MP #include "mp.h" @@ -35,6 +38,9 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) u32 bootpg = determine_mp_bootpg(NULL); u32 id = get_my_id(); const char *enable_method; +#ifdef CONFIG_DEEP_SLEEP
- ulong len;
+#endif
off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "cpu",
4);
while (off != -FDT_ERR_NOTFOUND) { @@ -77,6 +83,25 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) "device_type", "cpu", 4); }
+#ifdef CONFIG_DEEP_SLEEP
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/- len = sizeof(bd_t) + sizeof(gd_t) + (1 << 10);
- /* round up to 4K */
- len = (len + (4096 - 1)) & ~(4096 - 1);
- off = fdt_add_mem_rsv(blob, gd->relocaddr - len,
len + ((ulong)&__bss_end - gd->relocaddr));- if (off < 0)
printf("Failed to reserve memory for warm reset: %s\n",fdt_strerror(off));+#endif
Why do you need to reserve memory for this? We didn't need to for deep sleep on MPC8313ERDB, which also goes through U-Boot on wake. On MPC8313ERDB we transfer control to the kernel before relocating, so U- Boot never touches DDR. bd_t, gd_t, and the stack should be in locked L1 cache, and the u-boot image should be in flash (or locked CPC if this is not a NOR flash boot).
In deep sleep many IP blocks are powered off like DDRC, LIODN, cpu. These IPs are re-initialized after relocating. So, we must jump to kernel after relocating.
The DDR controller is initialized before relocating -- and of course the CPU is powered off on MPC8313ERDB deep sleep as well.
LIODNs are a new concern for deep sleep, but that doesn't mean we should go through a bunch of unrelated code to get there. Refactor the LIODN code to be usable before relocation, and not be tied to fdt fixups.
There are other IP blocks that need to be re-initialized, like SerDes, SMP, PCIe and a lot of Errata. I don't want to refactor one by one. Besides, coding in this way will not change the current execute flow.
+#ifndef CONFIG_DEEP_SLEEP /* Reserve spin table page */ if (spin_tbl_addr < memory_limit) { off = fdt_add_mem_rsv(blob, @@ -108,6 +134,7 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) printf("Failed to reserve memory for spin table: %s\n", fdt_strerror(off)); } +#endif
Explain.
Spin_tbl_addr has been reserved already.
Where, and why is it different for non-CONFIG_DEEP_SLEEP?
right above:
+ /* + * reserve the memory space for warm reset. + * This space will be re-used next time when boot from deep sleep. + * The space includes bd_t, gd_t, stack and uboot image. + * Reserve 1K for stack. + */
If deep sleep is supported, the sbin_table space will be reserved above. we don't need to reserved it anymore.
#if (CONFIG_SYS_NUM_FMAN == 2)
- set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);
- setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,
fman2_liodn_tbl_sz);+#ifdef CONFIG_DEEP_SLEEP
- if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,fman2_liodn_tbl_sz);- }
+#endif #endif #endif
Aren't you breaking non-CONFIG_DEEP_SLEEP here?
No, if deep sleep feature is enabled, we need to further judge whether this boot is Coming from deep sleep by GD_FLG_WARM_BOOT flag.
My point is that if CONFIG_DEEP_SLEEP is not set, you will be skipping those calls to set_liodn() and setup_fman_liodn_base().
#ifdef foo if (!bar) stuff(); #endif
is not equivalent to:
#ifdef foo if (!bar) #endif stuff();
Though the latter is better written as something like:
bool do_stuff = true;
#ifdef foo if (bar) do_stuff = false; #endif
if (do_stuff) stuff();
or
static inline bool is_bar(void) { #ifdef foo return bar; #else return true; #endif }
...
if (!is_bar()) stuff();
You are absolutely right. How didn't I find this?
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 8e59e8b..1ab05ff 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -117,6 +117,7 @@ struct arch_global_data { #include <asm-generic/global_data.h>
#if 1 +#define GD_FLG_WARM_BOOT 0x10000
Why is this not with the rest of the GD_FLGs, not numerically contiguous, and not commented? What specifically does "warm boot" mean? I think a lot of people would expect it to mean something that looks like a reset at the software level, while possibly not involving a real hardware reset -- coming out of deep sleep is the opposite of that.
Archdef document says it is a warm reset boot. So I name it this way.
Hardware people sometimes use terminology differently than software people.
How about warm_reset?
Other platforms use the same flag, like x86, although I am not sure Whether it is for deep sleep. If you have better name, I love to use it.
It's not yet clear to me that a GD flag is needed for this.
GD_ flag can be added for our own purpose. We decide to use a GD flag to indicate the deep sleep case and I think it worth it.
#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm
("r2")
#else /* We could use plain global data, but the resulting code is
bigger */
#define XTRN_DECLARE_GLOBAL_DATA_PTR extern diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 34bbfca..7383e32 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -350,6 +350,13 @@ unsigned long logbuffer_base(void) } #endif
+#ifdef CONFIG_DEEP_SLEEP +int check_warmboot(void) +{
- return !!(gd->flags & GD_FLG_WARM_BOOT); } #endif
Why?
Why what? Why we need it?
It is a help function and used by ASM code in which we can't determine whether it is a warm reset boot.
Why don't you just open code it?
I can't check the warmboot status in ASM code. In order to get the warmboot status in ASM code, I wrote this function.
Again, you seem to be breaking the non-CONFIG_DEEP_SLEEP case.
No.
Yes. :-)
+#ifdef CONFIG_DEEP_SLEEP
- /*
* restore 128-byte memory space which corrupted* by DDRC training.*/- if (gd->flags & GD_FLG_WARM_BOOT) {
src = (u64 *)in_be32(&scfg->sparecr[2]);dst = (u64 *)(0);for (i = 0; i < 128/sizeof(u64); i++) {*dst = *src;dst++;src++;}- }
(u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined and GCC has been getting increasingly free with making surprising optimizations based on that (such as assuming any code path that it knows can reach a NULL dereference is dead code and can be removed).
Then how we operate 0 address if not dereferencing NULL pointer?
With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location.
we found the zero address has benefit. I don't know how to achieve this in inline asm or TLB mapping, could you be more specific or write a example for me?
Regards, Yuantian
-Scott
On Mon, 2014-02-24 at 15:47 +0800, Tang Yuantian-B29983 wrote:
On 2014/2/15 星期六 6:21, Scott Wood wrote:
On Thu, 2014-02-13 at 01:05 -0600, Tang Yuantian-B29983 wrote:
Thanks for your review. Please see the reply inline.
Thanks, Yuantian
-----Original Message----- From: Wood Scott-B07421 Sent: 2014年2月13日 星期四 8:44 To: Tang Yuantian-B29983 Cc: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472; Tang@theia.denx.de; u-boot@lists.denx.de; Kushwaha Prabhakar-B32579; Jin Zhengxiong-R64188 Subject: Re: [U-Boot,2/3] mpc85xx: Add deep sleep framework support
On Sun, Jan 26, 2014 at 02:00:40PM +0800, tang yuantian wrote:
From: Tang Yuantian yuantian.tang@freescale.com
When system wakes up from warm reset, control is passed to the primary core that starts executing uboot. After re-initialized some IP blocks, like DDRC, kernel will take responsibility to continue to restore environment it leaves before.
Signed-off-by: Tang Yuantian Yuantian.Tang@freescale.com
Is this for some specific mpc85xx chip (e.g. T1040)? I'm pretty sure this isn't necessary for deep sleep on mpc8536, for example.
Currently, it is used by t1040. But we want it to be more general so that It can be used by later new chips.
But the mechanism is not the same for all mpc85xx derivatives. You'll need a more specific name.
OK, will name it as t104x
+#ifdef CONFIG_DEEP_SLEEP
CONFIG symbols need to be documented in README...
Where should I add this README?
Under "85xx CPU Options" in the top-level README.
Thanks.
- /* disable the console if boot from warm reset */
- if (in_be32(&gur->scrtsr[0]) & (1 << 3))
gd->flags |=GD_FLG_SILENT | GD_FLG_WARM_BOOT |GD_FLG_DISABLE_CONSOLE; #endif
...and it should probably be a more specific CONFIG_SYS symbol that indicates the presence of this guts bit.
where should I put this CONFIG_SYS_'s definition?
Under "85xx CPU Options" in the top-level README. :-)
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 33bc900..b3b9250 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -24,6 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; extern void ft_qe_setup(void *blob); extern void ft_fixup_num_cores(void *blob); extern void ft_srio_setup(void *blob); +#ifdef CONFIG_DEEP_SLEEP +extern ulong __bss_end; +#endif
Don't ifdef declarations.
#ifdef CONFIG_MP #include "mp.h" @@ -35,6 +38,9 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) u32 bootpg = determine_mp_bootpg(NULL); u32 id = get_my_id(); const char *enable_method; +#ifdef CONFIG_DEEP_SLEEP
- ulong len;
+#endif
off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "cpu",
4);
while (off != -FDT_ERR_NOTFOUND) { @@ -77,6 +83,25 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) "device_type", "cpu", 4); }
+#ifdef CONFIG_DEEP_SLEEP
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/- len = sizeof(bd_t) + sizeof(gd_t) + (1 << 10);
- /* round up to 4K */
- len = (len + (4096 - 1)) & ~(4096 - 1);
- off = fdt_add_mem_rsv(blob, gd->relocaddr - len,
len + ((ulong)&__bss_end - gd->relocaddr));- if (off < 0)
printf("Failed to reserve memory for warm reset: %s\n",fdt_strerror(off));+#endif
Why do you need to reserve memory for this? We didn't need to for deep sleep on MPC8313ERDB, which also goes through U-Boot on wake. On MPC8313ERDB we transfer control to the kernel before relocating, so U- Boot never touches DDR. bd_t, gd_t, and the stack should be in locked L1 cache, and the u-boot image should be in flash (or locked CPC if this is not a NOR flash boot).
In deep sleep many IP blocks are powered off like DDRC, LIODN, cpu. These IPs are re-initialized after relocating. So, we must jump to kernel after relocating.
The DDR controller is initialized before relocating -- and of course the CPU is powered off on MPC8313ERDB deep sleep as well.
LIODNs are a new concern for deep sleep, but that doesn't mean we should go through a bunch of unrelated code to get there. Refactor the LIODN code to be usable before relocation, and not be tied to fdt fixups.
There are other IP blocks that need to be re-initialized, like SerDes, SMP, PCIe and a lot of Errata. I don't want to refactor one by one. Besides, coding in this way will not change the current execute flow.
Changing the execution flow is better than adding a bunch of special cases to the current execution flow.
Some of that reinitialization (e.g. PCIe) should be handled by Linux, not U-Boot.
+#ifndef CONFIG_DEEP_SLEEP /* Reserve spin table page */ if (spin_tbl_addr < memory_limit) { off = fdt_add_mem_rsv(blob, @@ -108,6 +134,7 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) printf("Failed to reserve memory for spin table: %s\n", fdt_strerror(off)); } +#endif
Explain.
Spin_tbl_addr has been reserved already.
Where, and why is it different for non-CONFIG_DEEP_SLEEP?
right above:
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/If deep sleep is supported, the sbin_table space will be reserved above. we don't need to reserved it anymore.
So the same memory is used for the spin table as for the deep sleep stuff? There's no conflict there?
#if (CONFIG_SYS_NUM_FMAN == 2)
- set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);
- setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,
fman2_liodn_tbl_sz);+#ifdef CONFIG_DEEP_SLEEP
- if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,fman2_liodn_tbl_sz);- }
+#endif #endif #endif
Aren't you breaking non-CONFIG_DEEP_SLEEP here?
No, if deep sleep feature is enabled, we need to further judge whether this boot is Coming from deep sleep by GD_FLG_WARM_BOOT flag.
My point is that if CONFIG_DEEP_SLEEP is not set, you will be skipping those calls to set_liodn() and setup_fman_liodn_base().
#ifdef foo if (!bar) stuff(); #endif
is not equivalent to:
#ifdef foo if (!bar) #endif stuff();
Though the latter is better written as something like:
bool do_stuff = true;
#ifdef foo if (bar) do_stuff = false; #endif
if (do_stuff) stuff();
or
static inline bool is_bar(void) { #ifdef foo return bar; #else return true; #endif }
...
if (!is_bar()) stuff();
You are absolutely right. How didn't I find this?
By not testing with CONFIG_DEEP_SLEEP off? :-)
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 8e59e8b..1ab05ff 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -117,6 +117,7 @@ struct arch_global_data { #include <asm-generic/global_data.h>
#if 1 +#define GD_FLG_WARM_BOOT 0x10000
Why is this not with the rest of the GD_FLGs, not numerically contiguous, and not commented? What specifically does "warm boot" mean? I think a lot of people would expect it to mean something that looks like a reset at the software level, while possibly not involving a real hardware reset -- coming out of deep sleep is the opposite of that.
Archdef document says it is a warm reset boot. So I name it this way.
Hardware people sometimes use terminology differently than software people.
How about warm_reset?
"warm reset" and "warm boot" mean pretty much the same thing to me. I'd stick with "deep sleep" terminology.
Other platforms use the same flag, like x86, although I am not sure Whether it is for deep sleep. If you have better name, I love to use it.
It's not yet clear to me that a GD flag is needed for this.
GD_ flag can be added for our own purpose. We decide to use a GD flag to indicate the deep sleep case and I think it worth it.
It depends on how far into the boot sequence we go.
#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm
("r2")
#else /* We could use plain global data, but the resulting code is
bigger */
#define XTRN_DECLARE_GLOBAL_DATA_PTR extern diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 34bbfca..7383e32 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -350,6 +350,13 @@ unsigned long logbuffer_base(void) } #endif
+#ifdef CONFIG_DEEP_SLEEP +int check_warmboot(void) +{
- return !!(gd->flags & GD_FLG_WARM_BOOT); } #endif
Why?
Why what? Why we need it?
It is a help function and used by ASM code in which we can't determine whether it is a warm reset boot.
Why don't you just open code it?
I can't check the warmboot status in ASM code. In order to get the warmboot status in ASM code, I wrote this function.
Why can't you check it in asm code? See lib/asm-offsets.c.
- if (gd->flags & GD_FLG_WARM_BOOT) {
src = (u64 *)in_be32(&scfg->sparecr[2]);dst = (u64 *)(0);for (i = 0; i < 128/sizeof(u64); i++) {*dst = *src;dst++;src++;}- }
(u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined and GCC has been getting increasingly free with making surprising optimizations based on that (such as assuming any code path that it knows can reach a NULL dereference is dead code and can be removed).
Then how we operate 0 address if not dereferencing NULL pointer?
With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location.
we found the zero address has benefit. I don't know how to achieve this in inline asm or TLB mapping, could you be more specific or write a example for me?
Inline asm would be something like:
asm("stw %1, 0(%0); stw %2, 4(%0)" : "=r" (dst) : "r" ((u32)(src >> 32)), "r" ((u32)src));
-Scott
On 2014/2/25 星期二 3:11, Scott Wood wrote:
On Mon, 2014-02-24 at 15:47 +0800, Tang Yuantian-B29983 wrote:
On 2014/2/15 星期六 6:21, Scott Wood wrote:
On Thu, 2014-02-13 at 01:05 -0600, Tang Yuantian-B29983 wrote:
Thanks for your review. Please see the reply inline.
Thanks, Yuantian
-----Original Message----- From: Wood Scott-B07421 Sent: 2014年2月13日 星期四 8:44 To: Tang Yuantian-B29983 Cc: Sun York-R58495; Wood Scott-B07421; Li Yang-Leo-R58472; Tang@theia.denx.de; u-boot@lists.denx.de; Kushwaha Prabhakar-B32579; Jin Zhengxiong-R64188 Subject: Re: [U-Boot,2/3] mpc85xx: Add deep sleep framework support
On Sun, Jan 26, 2014 at 02:00:40PM +0800, tang yuantian wrote:
From: Tang Yuantian yuantian.tang@freescale.com
When system wakes up from warm reset, control is passed to the primary core that starts executing uboot. After re-initialized some IP blocks, like DDRC, kernel will take responsibility to continue to restore environment it leaves before.
Signed-off-by: Tang Yuantian Yuantian.Tang@freescale.com
Is this for some specific mpc85xx chip (e.g. T1040)? I'm pretty sure this isn't necessary for deep sleep on mpc8536, for example.
Currently, it is used by t1040. But we want it to be more general so that It can be used by later new chips.
But the mechanism is not the same for all mpc85xx derivatives. You'll need a more specific name.
OK, will name it as t104x
+#ifdef CONFIG_DEEP_SLEEP
CONFIG symbols need to be documented in README...
Where should I add this README?
Under "85xx CPU Options" in the top-level README.
Thanks.
- /* disable the console if boot from warm reset */
- if (in_be32(&gur->scrtsr[0]) & (1 << 3))
gd->flags |=GD_FLG_SILENT | GD_FLG_WARM_BOOT |GD_FLG_DISABLE_CONSOLE; #endif
...and it should probably be a more specific CONFIG_SYS symbol that indicates the presence of this guts bit.
where should I put this CONFIG_SYS_'s definition?
Under "85xx CPU Options" in the top-level README. :-)
I don't find other gut bit that defined in this README. Do I need to? Just we are clear that you want me to add a CONFIG_SYS_'s definition for (1 << 3) bit in GUTS, right?
diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 33bc900..b3b9250 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -24,6 +24,9 @@ DECLARE_GLOBAL_DATA_PTR; extern void ft_qe_setup(void *blob); extern void ft_fixup_num_cores(void *blob); extern void ft_srio_setup(void *blob); +#ifdef CONFIG_DEEP_SLEEP +extern ulong __bss_end; +#endif
Don't ifdef declarations.
#ifdef CONFIG_MP #include "mp.h" @@ -35,6 +38,9 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) u32 bootpg = determine_mp_bootpg(NULL); u32 id = get_my_id(); const char *enable_method; +#ifdef CONFIG_DEEP_SLEEP
- ulong len;
+#endif
off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "cpu",4);
while (off != -FDT_ERR_NOTFOUND) {@@ -77,6 +83,25 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) "device_type", "cpu", 4); }
+#ifdef CONFIG_DEEP_SLEEP
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/- len = sizeof(bd_t) + sizeof(gd_t) + (1 << 10);
- /* round up to 4K */
- len = (len + (4096 - 1)) & ~(4096 - 1);
- off = fdt_add_mem_rsv(blob, gd->relocaddr - len,
len + ((ulong)&__bss_end - gd->relocaddr));- if (off < 0)
printf("Failed to reserve memory for warm reset: %s\n",fdt_strerror(off));+#endif
Why do you need to reserve memory for this? We didn't need to for deep sleep on MPC8313ERDB, which also goes through U-Boot on wake. On MPC8313ERDB we transfer control to the kernel before relocating, so U- Boot never touches DDR. bd_t, gd_t, and the stack should be in locked L1 cache, and the u-boot image should be in flash (or locked CPC if this is not a NOR flash boot).
In deep sleep many IP blocks are powered off like DDRC, LIODN, cpu. These IPs are re-initialized after relocating. So, we must jump to kernel after relocating.
The DDR controller is initialized before relocating -- and of course the CPU is powered off on MPC8313ERDB deep sleep as well.
LIODNs are a new concern for deep sleep, but that doesn't mean we should go through a bunch of unrelated code to get there. Refactor the LIODN code to be usable before relocation, and not be tied to fdt fixups.
There are other IP blocks that need to be re-initialized, like SerDes, SMP, PCIe and a lot of Errata. I don't want to refactor one by one. Besides, coding in this way will not change the current execute flow.
Changing the execution flow is better than adding a bunch of special cases to the current execution flow.
Some of that reinitialization (e.g. PCIe) should be handled by Linux, not U-Boot.
That's not that easy. If you refactor a function you need to refactor many other codes it depends on. There are also a number of Errata. They are not wrapped in one function. Refactoring all these is really a big challenge. Just image how many codes need to be refactored between the first code line in start.S and cpu_init_r(). We believe at least cpu_init_r() needs to be executed before jumping to kernel.
+#ifndef CONFIG_DEEP_SLEEP /* Reserve spin table page */ if (spin_tbl_addr < memory_limit) { off = fdt_add_mem_rsv(blob, @@ -108,6 +134,7 @@ void ft_fixup_cpu(void *blob, u64 memory_limit) printf("Failed to reserve memory for spin table: %s\n", fdt_strerror(off)); } +#endif
Explain.
Spin_tbl_addr has been reserved already.
Where, and why is it different for non-CONFIG_DEEP_SLEEP?
right above:
- /*
* reserve the memory space for warm reset.* This space will be re-used next time when boot from deep sleep.* The space includes bd_t, gd_t, stack and uboot image.* Reserve 1K for stack.*/If deep sleep is supported, the sbin_table space will be reserved above. we don't need to reserved it anymore.
So the same memory is used for the spin table as for the deep sleep stuff? There's no conflict there?
This space is not used by deep sleep. It is used by spin_table and uboot itself at first. I just keep it not to free.
#if (CONFIG_SYS_NUM_FMAN == 2)
- set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);
- setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,
fman2_liodn_tbl_sz);+#ifdef CONFIG_DEEP_SLEEP
- if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
set_liodn(fman2_liodn_tbl, fman2_liodn_tbl_sz);setup_fman_liodn_base(FSL_HW_PORTAL_FMAN2, fman2_liodn_tbl,fman2_liodn_tbl_sz);- }
+#endif #endif #endif
Aren't you breaking non-CONFIG_DEEP_SLEEP here?
No, if deep sleep feature is enabled, we need to further judge whether this boot is Coming from deep sleep by GD_FLG_WARM_BOOT flag.
My point is that if CONFIG_DEEP_SLEEP is not set, you will be skipping those calls to set_liodn() and setup_fman_liodn_base().
#ifdef foo if (!bar) stuff(); #endif
is not equivalent to:
#ifdef foo if (!bar) #endif stuff();
Though the latter is better written as something like:
bool do_stuff = true;
#ifdef foo if (bar) do_stuff = false; #endif
if (do_stuff) stuff();
or
static inline bool is_bar(void) { #ifdef foo return bar; #else return true; #endif }
...
if (!is_bar()) stuff();
You are absolutely right. How didn't I find this?
By not testing with CONFIG_DEEP_SLEEP off? :-)
You are right again. :( When I turn off DEEP_SLEEP, I found some compile warnings.
diff --git a/arch/powerpc/include/asm/global_data.h b/arch/powerpc/include/asm/global_data.h index 8e59e8b..1ab05ff 100644 --- a/arch/powerpc/include/asm/global_data.h +++ b/arch/powerpc/include/asm/global_data.h @@ -117,6 +117,7 @@ struct arch_global_data { #include <asm-generic/global_data.h>
#if 1 +#define GD_FLG_WARM_BOOT 0x10000
Why is this not with the rest of the GD_FLGs, not numerically contiguous, and not commented? What specifically does "warm boot" mean? I think a lot of people would expect it to mean something that looks like a reset at the software level, while possibly not involving a real hardware reset -- coming out of deep sleep is the opposite of that.
Archdef document says it is a warm reset boot. So I name it this way.
Hardware people sometimes use terminology differently than software people.
How about warm_reset?
"warm reset" and "warm boot" mean pretty much the same thing to me. I'd stick with "deep sleep" terminology.
This flag indicates the BOOT method. that is not related to deep sleep. Some other features can use this flag if they want to. We have power on reset, hardware reset currently. So warm_reset is acceptable.
Other platforms use the same flag, like x86, although I am not sure Whether it is for deep sleep. If you have better name, I love to use it.
It's not yet clear to me that a GD flag is needed for this.
GD_ flag can be added for our own purpose. We decide to use a GD flag to indicate the deep sleep case and I think it worth it.
It depends on how far into the boot sequence we go.
As I said above, at least cpu_init_r() is finished.
#define DECLARE_GLOBAL_DATA_PTR register volatile gd_t *gd asm
("r2")
#else /* We could use plain global data, but the resulting code is
bigger */
#define XTRN_DECLARE_GLOBAL_DATA_PTR extern diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 34bbfca..7383e32 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -350,6 +350,13 @@ unsigned long logbuffer_base(void) } #endif
+#ifdef CONFIG_DEEP_SLEEP +int check_warmboot(void) +{
- return !!(gd->flags & GD_FLG_WARM_BOOT); } #endif
Why?
Why what? Why we need it?
It is a help function and used by ASM code in which we can't determine whether it is a warm reset boot.
Why don't you just open code it?
I can't check the warmboot status in ASM code. In order to get the warmboot status in ASM code, I wrote this function.
Why can't you check it in asm code? See lib/asm-offsets.c.
Not find in both kernel and uboot source code.
- if (gd->flags & GD_FLG_WARM_BOOT) {
src = (u64 *)in_be32(&scfg->sparecr[2]);dst = (u64 *)(0);for (i = 0; i < 128/sizeof(u64); i++) {*dst = *src;dst++;src++;}- }
(u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined and GCC has been getting increasingly free with making surprising optimizations based on that (such as assuming any code path that it knows can reach a NULL dereference is dead code and can be removed).
Then how we operate 0 address if not dereferencing NULL pointer?
With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location.
we found the zero address has benefit. I don't know how to achieve this in inline asm or TLB mapping, could you be more specific or write a example for me?
Inline asm would be something like:
asm("stw %1, 0(%0); stw %2, 4(%0)" : "=r" (dst) : "r" ((u32)(src >> 32)), "r" ((u32)src));
Thank you very much. As I said in other thread, if you are sure about 32bits thing, I will change it.
Regards, Yuantian
-Scott
On 2014/2/25 星期二 3:11, Scott Wood wrote:
Why what? Why we need it?
It is a help function and used by ASM code in which we can't determine whether it is a warm reset boot.
Why don't you just open code it?
I can't check the warmboot status in ASM code. In order to get the warmboot status in ASM code, I wrote this function.
Why can't you check it in asm code? See lib/asm-offsets.c.
Found it. Still learn how to use it.
Thanks, Yuantian
- if (gd->flags & GD_FLG_WARM_BOOT) {
src = (u64 *)in_be32(&scfg->sparecr[2]);dst = (u64 *)(0);for (i = 0; i < 128/sizeof(u64); i++) {*dst = *src;dst++;src++;}- }
(u64 *)(0) is a NULL pointer. Dereferencing NULL pointers is undefined and GCC has been getting increasingly free with making surprising optimizations based on that (such as assuming any code path that it knows can reach a NULL dereference is dead code and can be removed).
Then how we operate 0 address if not dereferencing NULL pointer?
With an I/O accessor (or other inline asm), a TLB mapping, or using a different memory location.
we found the zero address has benefit. I don't know how to achieve this in inline asm or TLB mapping, could you be more specific or write a example for me?
Inline asm would be something like:
asm("stw %1, 0(%0); stw %2, 4(%0)" : "=r" (dst) : "r" ((u32)(src >> 32)), "r" ((u32)src));
-Scott
participants (2)
-
Scott Wood -
Tang Yuantian-B29983