[Test] test3

Yuantian Tang Yuantian.Tang at freescale.com
Wed Mar 5 09:32:52 CET 2014


Ok.


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.
> 
> > ---
> > arch/powerpc/cpu/mpc85xx/cpu_init.c    |  7 ++++
> >  arch/powerpc/cpu/mpc85xx/fdt.c         | 27 +++++++++++++++
> >  arch/powerpc/cpu/mpc85xx/liodn.c       | 24 +++++++++----
> >  arch/powerpc/cpu/mpc85xx/start.S       |  8 +++++
> >  arch/powerpc/include/asm/global_data.h |  1 +
> >  arch/powerpc/lib/board.c               | 61
> +++++++++++++++++++++++++++++++---
> >  drivers/ddr/fsl/mpc85xx_ddr_gen3.c     | 44 +++++++++++++++++++++---
> >  include/fsl_ddr_sdram.h                |  6 ++++
> >  8 files changed, 163 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > index b31efb7..376c659 100644
> > --- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > +++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > @@ -231,6 +231,7 @@ void cpu_init_f (void)  #ifdef CONFIG_MPC8548
> >  	ccsr_local_ecm_t *ecm = (void *)(CONFIG_SYS_MPC85xx_ECM_ADDR);
> >  	uint svr = get_svr();
> > +	gd = (gd_t *)(CONFIG_SYS_INIT_RAM_ADDR +
> > +CONFIG_SYS_GBL_DATA_OFFSET);
> >
> >  	/*
> >  	 * CPU2 errata workaround: A core hang possible while executing @@
> > -282,6 +283,12 @@ void cpu_init_f (void)
> >  	in_be32(&gur->dcsrcr);
> >  #endif
> >
> > +#ifdef CONFIG_DEEP_SLEEP
> 
> CONFIG symbols need to be documented in 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).
> 
> >  	/* Reserve the boot page so OSes dont use it */
> >  	if ((u64)bootpg < memory_limit) {
> >  		off = fdt_add_mem_rsv(blob, bootpg, (u64)4096); @@ -100,6
> +125,7 @@
> > void ft_fixup_cpu(void *blob, u64 memory_limit)
> >  	}
> >  #endif
> >
> > +#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.
> 
> > diff --git a/arch/powerpc/cpu/mpc85xx/liodn.c
> > b/arch/powerpc/cpu/mpc85xx/liodn.c
> > index 19e130e..898685b 100644
> > --- a/arch/powerpc/cpu/mpc85xx/liodn.c
> > +++ b/arch/powerpc/cpu/mpc85xx/liodn.c
> > @@ -14,6 +14,10 @@
> >  #include <asm/fsl_portals.h>
> >  #include <asm/fsl_liodn.h>
> >
> > +#ifdef CONFIG_DEEP_SLEEP
> > +DECLARE_GLOBAL_DATA_PTR;
> > +#endif
> 
> Don't ifdef declarations.
> 
> >  int get_dpaa_liodn(enum fsl_dpaa_dev dpaa_dev, u32 *liodns, int
> > liodn_offset)  {
> >  	liodns[0] = liodn_bases[dpaa_dev].id[0] + liodn_offset; @@ -180,14
> > +184,22 @@ void set_liodns(void)
> >
> >  	/* setup FMAN block(s) liodn bases & offsets if we have one */
> > #ifdef CONFIG_SYS_DPAA_FMAN
> > -	set_liodn(fman1_liodn_tbl, fman1_liodn_tbl_sz);
> > -	setup_fman_liodn_base(FSL_HW_PORTAL_FMAN1, fman1_liodn_tbl,
> > -				fman1_liodn_tbl_sz);
> > +#ifdef CONFIG_DEEP_SLEEP
> > +	if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
> > +		set_liodn(fman1_liodn_tbl, fman1_liodn_tbl_sz);
> > +		setup_fman_liodn_base(FSL_HW_PORTAL_FMAN1, fman1_liodn_tbl,
> > +					fman1_liodn_tbl_sz);
> > +	}
> > +#endif
> >
> >  #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?
> 
> Why would you be getting this far into the boot in the deep sleep wake
> case?
> 
> >  	/* setup PME liodn base */
> > diff --git a/arch/powerpc/cpu/mpc85xx/start.S
> > b/arch/powerpc/cpu/mpc85xx/start.S
> > index db84d10..4e66edc 100644
> > --- a/arch/powerpc/cpu/mpc85xx/start.S
> > +++ b/arch/powerpc/cpu/mpc85xx/start.S
> > @@ -1671,6 +1671,14 @@ relocate_code:
> >  	 * Now relocate code
> >  	 */
> >
> > +#ifdef CONFIG_DEEP_SLEEP
> > +	/* don't copy uboot again in warm reset case */
> > +	mr r0, r3
> > +	bl check_warmboot
> > +	cmpwi r3, 0
> > +	bne 7f
> > +	mr r3, r0
> > +#endif
> >  	cmplw	cr1,r3,r4
> >  	addi	r0,r5,3
> >  	srwi.	r0,r0,2
> 
> The surrounding asm code puts a tab after the mnemonic.  Why don't you?
> 
> Again, you shouldn't get this far in the deep sleep wake case.
> 
> > 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.
> 
> >  #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?
> 
> >  void board_init_f(ulong bootflag)
> >  {
> >  	bd_t *bd;
> > @@ -475,12 +482,18 @@ void board_init_f(ulong bootflag)
> >
> >  	debug("Reserving %ldk for U-Boot at: %08lx\n", len >> 10, addr);
> >
> > +#ifdef CONFIG_DEEP_SLEEP
> >  	/*
> >  	 * reserve memory for malloc() arena
> > +	 * don't reserve it in warm reset case
> >  	 */
> > -	addr_sp = addr - TOTAL_MALLOC_LEN;
> > -	debug("Reserving %dk for malloc() at: %08lx\n",
> > -	      TOTAL_MALLOC_LEN >> 10, addr_sp);
> > +	if ((gd->flags & GD_FLG_WARM_BOOT) == 0) {
> > +		addr_sp = addr - TOTAL_MALLOC_LEN;
> > +		debug("Reserving %dk for malloc() at: %08lx\n",
> > +			TOTAL_MALLOC_LEN >> 10, addr_sp);
> > +	} else
> > +		addr_sp = addr;
> > +#endif
> 
> If one side of an if/else has braces, U-Boot style says to use braces on
> both.
> 
> Again, you seem to be breaking the non-CONFIG_DEEP_SLEEP case.
> 
> > @@ -591,6 +604,14 @@ void board_init_r(gd_t *id, ulong dest_addr)  {
> >  	bd_t *bd;
> >  	ulong malloc_start;
> > +#ifdef CONFIG_DEEP_SLEEP
> > +	u32 start;
> > +	int i;
> > +	struct ccsr_scfg *scfg = (void *)CONFIG_SYS_MPC85xx_SCFG;
> > +	u64 *src, *dst;
> > +	typedef void (*func_t)(void);
> > +	func_t kernel_resume;
> > +#endif
> >
> >  #ifndef CONFIG_SYS_NO_FLASH
> >  	ulong flash_size;
> > @@ -601,6 +622,21 @@ void board_init_r(gd_t *id, ulong dest_addr)
> >
> >  	gd->flags |= GD_FLG_RELOC;	/* tell others: relocation done */
> >
> > +#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).
> 
> Use an I/O accessor.  And yes, the mpc8313erdb code should be fixed as
> well. :-)
> 
> > @@ -639,7 +675,10 @@ void board_init_r(gd_t *id, ulong dest_addr)
> >  	/*
> >  	 * Setup trap handlers
> >  	 */
> > -	trap_init(dest_addr);
> > +#ifdef CONFIG_DEEP_SLEEP
> > +	if ((gd->flags & GD_FLG_WARM_BOOT) == 0)
> > +		trap_init(dest_addr);
> > +#endif
> 
> More breakage of non-CONFIG_DEEP_SLEEP.  I'll stop here.
> 
> -Scott


More information about the Test mailing list