From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tang Yuantian-B29983 Date: Mon, 24 Feb 2014 15:47:47 +0800 Subject: [U-Boot] [U-Boot, 2/3] mpc85xx: Add deep sleep framework support In-Reply-To: <1392416485.6733.616.camel@snotra.buserror.net> References: <1390716041-13541-2-git-send-email-Yuantian.Tang@freescale.com> <20140213004412.GA9660@home.buserror.net> <07dc877206434a4ea634d912c02da3a5@BL2PR03MB115.namprd03.prod.outlook.com> <1392416485.6733.616.camel@snotra.buserror.net> Message-ID: <530AF923.2040009@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> >>>> 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 >>> 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 >>>> >>>> >>>> #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 > >