Re: [PATCH 3/3] arm: pm: add deep sleep support for LS1

[Chenhui Zhao]

On Fri, Sep 26, 2014 at 01:14:27PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 26, 2014 at 07:25:03PM +0800, Chenhui Zhao wrote:
> > +static int ls1_start_deepsleep(unsigned long addr)
> > +{
> > +	ls1_do_deepsleep(addr);
> > +
> > +	return 0;
> > +}
> ...
> > +	cpu_suspend(SRAM_CODE_BASE_PHY, ls1_start_deepsleep);
> 
> What's the point of this function?  Why can't ls1_do_deepsleep() just
> return zero?

Just leave space for adding C code in the future.

> 
> > +/*
> > + * r0: the physical entry address of SRAM code
> > + *
> > + */
> > +ENTRY(ls1_do_deepsleep)
> > +	mov	r13, r0
> > +
> > +	/* flush cache */
> > +	bl	v7_flush_dcache_all
> 
> Please explain the purpose of this call via a comment in the code.
> 
> The generic code will have saved the CPU state, and will have called
> flush_cache_louis() to flush the caches to the point of unification.
> 
> The only data which will have been loaded into the cache between that
> point is the stack for the return from __cpu_suspend_save, and
> speculative prefetches.
> 
> So, the only reason I can gather is that you need to flush data from
> lower levels of the cache below the point of unification.
> 

In deep sleep process, all caches will lost, so flush all caches to prevent
data loss.

> > +
> > +	/* disable cache, C bit in SCTLR */
> > +	mrc	p15, 0, r0, c1, c0, 0
> > +	bic	r0, r0, #(1 << 2)
> > +	mcr	p15, 0, r0, c1, c0, 0
> > +	isb
> > +
> > +	/* disable coherency, SMP bit in ACTLR */
> > +	mrc	p15, 0, r0, c1, c0, 1
> > +	bic	r0, r0, #(1 << 6)
> > +	mcr	p15, 0, r0, c1, c0, 1
> > +	isb
> > +	dsb
> > +
> > +	/* disable MMU, M bit in SCTLR */
> > +	mrc	p15, 0, r0, c1, c0, 0
> > +	bic	r0, r0, #1
> > +	mcr	p15, 0, r0, c1, c0, 0
> > +	isb
> > +
> > +	/* jump to sram code using physical address */
> > +	bx	r13
> 
> This looks extremely fragile.  You are running in virtual space, and you
> turn the MMU off.  You are reliant on the MMU still being on for the
> following instructions to already be in the pipeline.  That's not a
> very good assumption to make (we've made it in the past and it breaks
> every so often when things change, eg when the code is no longer laid
> out how we expect.)
> 
> You need to disable the MMU safely, which means using the identity map
> page tables and executing code in the identity map region.

Yes, the code will switch off MMU, and switch to physical address space.
On LS1021, the DDR memory located at the physical address space started from
0x80000000, the kernel space also started at 0x80000000 (CONFIG_PAGE_OFFSET = 0x80000000).
So the virtual address of kernel code is equal to the physical address.

Chenhui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/