[U-Boot] [U-Boot, 2/3] mpc85xx: Add deep sleep framework support
Tang Yuantian-B29983
Yuantian.Tang at freescale.com
Mon Feb 24 08:47:47 CET 2014
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 at theia.denx.de; u-boot at 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 at 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 at 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
>
>
More information about the U-Boot
mailing list