Re: [1/4] powerpc/cache: add cache flush operation for various e500

[Scott Wood <scottwood@freescale.com>]

On Thu, Mar 26, 2015 at 06:18:12PM +0800, chenhui zhao wrote:
> Various e500 core have different cache architecture, so they
> need different cache flush operations. Therefore, add a callback
> function cpu_flush_caches to the struct cpu_spec. The cache flush
> operation for the specific kind of e500 is selected at init time.
> The callback function will flush all caches inside the current cpu.
> 
> Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com>
> ---
>  arch/powerpc/include/asm/cacheflush.h     |   2 -
>  arch/powerpc/include/asm/cputable.h       |  11 +++
>  arch/powerpc/kernel/asm-offsets.c         |   3 +
>  arch/powerpc/kernel/cpu_setup_fsl_booke.S | 114 +++++++++++++++++++++++++++++-
>  arch/powerpc/kernel/cputable.c            |   4 ++
>  arch/powerpc/kernel/head_fsl_booke.S      |  74 -------------------
>  arch/powerpc/platforms/85xx/smp.c         |   3 +-
>  7 files changed, 133 insertions(+), 78 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> index 30b35ff..729fde4 100644
> --- a/arch/powerpc/include/asm/cacheflush.h
> +++ b/arch/powerpc/include/asm/cacheflush.h
> @@ -30,8 +30,6 @@ extern void flush_dcache_page(struct page *page);
>  #define flush_dcache_mmap_lock(mapping)		do { } while (0)
>  #define flush_dcache_mmap_unlock(mapping)	do { } while (0)
>  
> -extern void __flush_disable_L1(void);
> -
>  extern void flush_icache_range(unsigned long, unsigned long);
>  extern void flush_icache_user_range(struct vm_area_struct *vma,
>  				    struct page *page, unsigned long addr,
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index 5cf5a6d..c776efe4 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -43,6 +43,13 @@ extern int machine_check_e500(struct pt_regs *regs);
>  extern int machine_check_e200(struct pt_regs *regs);
>  extern int machine_check_47x(struct pt_regs *regs);
>  
> +#if defined(CONFIG_E500) || defined(CONFIG_PPC_E500MC)
> +extern void __flush_caches_e500v2(void);
> +extern void __flush_caches_e500mc(void);
> +extern void __flush_caches_e5500(void);
> +extern void __flush_caches_e6500(void);
> +#endif

Why the leading underscores?

>  /* NOTE WELL: Update identify_cpu() if fields are added or removed! */
>  struct cpu_spec {
>  	/* CPU is matched via (PVR & pvr_mask) == pvr_value */
> @@ -59,6 +66,10 @@ struct cpu_spec {
>  	unsigned int	icache_bsize;
>  	unsigned int	dcache_bsize;
>  
> +#if defined(CONFIG_E500) || defined(CONFIG_PPC_E500MC)

CONFIG_PPC_E500MC implies CONFIG_E500.  Why do we need this ifdef?

> +	/* flush caches inside the current cpu */
> +	void (*cpu_flush_caches)(void);
> +#endif

It seems you literally mean "in the cpu" -- If it's a threaded core, then
by "cpu" do you mean "thread" (like we usually do) and thus no caches get
flushed (ignore the fact that it's moot on e6500 -- this is an interface
and needs to be clear).

Also, no-oping L1 flush on e6500 is not compliant with the claim that
you're flushing the cache.  You're relying on an unstated assumption that
you'll invalidate that cache later instead.

If you want to make this "flush whatever needs to be flushed for
suspend/hotplug", call it that.

-Scott