[Test] test2
Yuantian Tang
Yuantian.Tang at freescale.com
Wed Mar 5 09:27:22 CET 2014
Have a nice day.
Thanks,
Yuantian
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, February 13, 2014 8:44 AM
> 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