[RFC PATCH 06/17] ARM: kernel: save/restore generic infrastructure
Lorenzo Pieralisi
lorenzo.pieralisi at arm.com
Mon Jul 11 07:33:45 EDT 2011
On Sat, Jul 09, 2011 at 11:01:23AM +0100, Russell King - ARM Linux wrote:
> The idea of splitting a large patch up into smaller patches is to do
> it in a logical way so that:
>
> 1. Each patch is self-contained, adding a single new - and where possible
> complete - feature or bug fix.
> 2. Ease of review.
>
> Carving your big patch up by file does not do either of this, because
> all patches interact with each other. There's people within ARM Ltd who
> have been dealing with patch sets for some time who you could ask advice
> from - or who you could ask to review your patches before you send them
> out on the mailing list.
Thanks for looking at it anyway.
My apologies Russell, point taken, and it is all my fault. Consider all
the comments on the patch splitting below as taken into account from now
onwards.
>
> On Thu, Jul 07, 2011 at 04:50:19PM +0100, Lorenzo Pieralisi wrote:
> > diff --git a/arch/arm/kernel/sr.h b/arch/arm/kernel/sr.h
> > new file mode 100644
> > index 0000000..6b24e53
> > --- /dev/null
> > +++ b/arch/arm/kernel/sr.h
> > @@ -0,0 +1,162 @@
> > +#define SR_NR_CLUSTERS 1
> > +
> > +#define STACK_SIZE 512
> > +
> > +#define CPU_A5 0x4100c050
>
> Not used in this patch - should it be in some other patch?
>
> > +#define CPU_A8 0x4100c080
>
> Not used in this patch - should it be in some other patch?
>
> > +#define CPU_A9 0x410fc090
>
> Not used in this patch - and if this is generic code then it should not
> be specific to any CPU. Please get rid of all these definitions, and
> if they're used you need to rework these patches to have proper separation
> of generic code from CPU specific code.
>
> > +#define L2_DATA_SIZE 16
>
> Not used in this patch.
>
> > +#define CONTEXT_SPACE_UNCACHED (2 * PAGE_SIZE)
> > +#define PA(f) ((typeof(f) *)virt_to_phys((void *)f))
>
> This is just messed up. Physical addresses aren't pointers, they're
> numeric. PA() is not used in this patch either, so it appears it can
> be deleted.
Just a bad name for a macro. It is used to call a function through its
physical address pointer. I will rework it.
>
> > +extern int lookup_arch(void);
>
> This doesn't exist in this patch, should it be in another patch or
> deleted?
>
> > +/*
> > + * Global variables
> > + */
> > +extern struct sr_main_table main_table;
> > +extern unsigned long idle_save_context;
> > +extern unsigned long idle_restore_context;
> > +extern unsigned long idle_mt;
> > +extern void *context_memory_uncached;
>
> Why does this need to be a global?
>
> > +
> > +/*
> > + * Context save/restore
> > + */
> > +typedef u32 (sr_save_context_t)
> > + (struct sr_cluster *,
> > + struct sr_cpu*, u32);
>
> Fits on one line so should be on one line.
>
Ok.
> > +typedef u32 (sr_restore_context_t)
> > + (struct sr_cluster *,
> > + struct sr_cpu*);
>
> Fits on one line so should be on one line.
>
Ok.
> > +
> > +extern sr_save_context_t sr_save_context;
>
> This doesn't exist in this patch, should it be in another patch?
>
> > +extern sr_restore_context_t sr_restore_context;
>
> Ditto.
>
> > +
> > +
> > +extern struct sr_arch *get_arch(void);
>
> Ditto.
>
> > +
> > +
> > +/*
> > + * 1:1 mappings
> > + */
> > +
> > +extern int linux_sr_setup_translation_tables(void);
>
> Ditto.
>
> > +
> > +/*
> > + * dumb memory allocator
> > + */
> > +
> > +extern void *get_memory(unsigned int size);
> > +
> > +/*
> > + * Entering/Leaving C-states function entries
> > + */
> > +
> > +extern int sr_platform_enter_cstate(unsigned cpu_index, struct sr_cpu *cpu,
> > + struct sr_cluster *cluster);
> > +extern int sr_platform_leave_cstate(unsigned cpu_index, struct sr_cpu *cpu,
> > + struct sr_cluster *cluster);
>
> See comment at the bottom - would inline functions here be better
> or maybe even place them at the callsite to make the code easier to
> understand if they're only used at one location.
>
Ok.
> > +
> > +/* save/restore main table */
> > +extern struct sr_main_table main_table;
>
> Why do we have two 'main_table' declarations in this same header file?
>
Sorry, my mistake.
> > +
> > +/*
> > + * Init functions
> > + */
> > +
> > +extern int sr_platform_runtime_init(void);
>
> Not defined in this patch.
>
> > +extern int sr_platform_init(void);
> > +extern int sr_context_init(void);
> > +
> > +
> > +/*
> > + * v7 specific
> > + */
> > +
> > +extern char *cpu_v7_suspend_size;
>
> No - stop going underneath the covers of existing APIs to get what you
> want. Use cpu_suspend() and cpu_resume() directly. Note that they've
> changed to be more flexible and those patches have been on this mailing
> list, and will be going in for 3.1.
>
> If they still don't do what you need, I'm going to be *pissed* because
> you've obviously known that they don't yet you haven't taken the time
> to get involved on this mailing list with the discussions over it.
>
No Russell, by using cpu_do_suspend and cpu_do_resume I wanted to
avoid reusing cpu_suspend/cpu_resume in a way that might clash with cpu idle
required behaviour. We talked about this and I decided to avoid using it
for cpuidle. The reason is two-fold:
- cpu_suspend/cpu_resume use the stack to save cpu registers and I would
like to avoid that to use non-cacheable memory. By using the *_do_*
functions I can pass a pointer to the memory I want, but if you
consider it an abuse I have to change it.
- cpu_suspend flush the cache but it does not clear the C bit in SCTLR,
which should be done when powering down a single CPU
I did follow the discussions and I tried to reuse the bits of code I can
reuse for carrying out what I need, but I did not want to ask for changes
in cpu_suspend that might not be the way to go forward and might disrupt
other bits of code relying on it.
> > +extern void scu_cpu_mode(void __iomem *base, int state);
>
> Not defined in this patch - should it be in another patch or deleted?
>
> > +
> > +/*
> > + * These arrays keep suitable stack pointers for CPUs.
> > + *
> > + * The memory must be 8-byte aligned.
> > + */
> > +
> > +extern unsigned long platform_cpu_stacks[CONFIG_NR_CPUS];
>
> Ditto.
>
> > +extern unsigned long platform_cpu_nc_stacks[CONFIG_NR_CPUS];
>
> Ditto.
>
> And should these be per-cpu variables? In any case, CONFIG_NR_CPUS
> doesn't look right here, NR_CPUS is probably what you want.
>
It is per-cpu stack pointer as used in sleep.S, where CONFIR_NR_CPUS is
used as well.
But I have to split this patch up in a better way to avoid taking your
time to track my mistakes.
> > +#endif
> > diff --git a/arch/arm/kernel/sr_context.c b/arch/arm/kernel/sr_context.c
> > new file mode 100644
> > index 0000000..25eaa43
> > --- /dev/null
> > +++ b/arch/arm/kernel/sr_context.c
> > @@ -0,0 +1,23 @@
> > +/*
> > + * Copyright (C) 2008-2011 ARM Limited
> > + * Author(s): Jon Callan, Lorenzo Pieralisi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/cache.h>
> > +#include <asm/cacheflush.h>
> > +#include "sr.h"
> > +
> > +int sr_context_init(void)
> > +{
> > + idle_save_context = (unsigned long) arch->save_context;
>
> This looks wrong. idle_save_context probably has the wrong type.
>
> > + idle_restore_context = __pa(arch->restore_context);
> > + __cpuc_flush_dcache_area(&idle_restore_context, sizeof(unsigned long));
> > + outer_clean_range(__pa(&idle_restore_context),
> > + __pa(&idle_restore_context + 1));
>
> This kind of thing needs rethinking - calling these internal functions
> directly just isn't on.
Yes, you are right.
> > + return 0;
> > +}
>
> And why have a single .c file for just one function? With all the
> copyright headers on each file this just results in extra LOC bloat,
> which given the situation we find ourselves in with mainline is
> *definitely* a bad idea.
>
I will rework that.
> > diff --git a/arch/arm/kernel/sr_helpers.h b/arch/arm/kernel/sr_helpers.h
> > new file mode 100644
> > index 0000000..1ae3a9a
> > --- /dev/null
> > +++ b/arch/arm/kernel/sr_helpers.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright (C) 2008-2011 ARM Limited
> > + * Author(s): Jon Callan, Lorenzo Pieralisi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +static inline int sr_platform_get_cpu_index(void)
> > +{
> > + unsigned int cpu;
> > + __asm__ __volatile__(
> > + "mrc p15, 0, %0, c0, c0, 5\n\t"
> > + : "=r" (cpu));
> > + return cpu & 0xf;
> > +}
>
> Use smp_processor_id() for indexes into kernel based structures, which
> has to be a unique CPU number for any CPU in the system. You only need
> the physical CPU ID when talking to the hardware.
>
Ok, I will reuse it where I can, there are some code paths where
I cannot (wake-up) and there I am afraid I have to resort to it.
> > +
> > +/*
> > + * Placeholder for further extensions
> > + */
> > +static inline int sr_platform_get_cluster_index(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void __iomem *sr_platform_cbar(void)
> > +{
> > + void __iomem *base;
> > + __asm__ __volatile__(
> > + "mrc p15, 4, %0, c15, c0, 0\n\t"
> > + : "=r" (base));
> > + return base;
> > +}
>
> This I imagine is another CPU specific register. On ARM926 its a register
> which controls the cache mode (writeback vs writethrough).
>
Yes, it is there to get the PERIPHBASE and the SCU physical address.
I just use it when I detect an A9 through the processor id.
> > +
> > +#ifdef CONFIG_SMP
> > +static inline void exit_coherency(void)
> > +{
> > + unsigned int v;
> > + asm volatile (
> > + "mrc p15, 0, %0, c1, c0, 1\n"
> > + "bic %0, %0, %1\n"
> > + "mcr p15, 0, %0, c1, c0, 1\n"
> > + : "=&r" (v)
> > + : "Ir" (0x40)
> > + : );
> > +}
>
> Firstly, this is specific to ARM CPUs. Secondly, the bit you require
> is very CPU specific even then. Some it's 0x40, others its 0x20. So
> this just does not deserve to be in generic code.
>
Right.
> > +#else
> > +static inline void exit_coherency(void) { }
> > +#endif
> > +
> > +extern void default_sleep(void);
> > +extern void sr_suspend(void *);
> > +extern void sr_resume(void *, int);
> > +extern void disable_clean_inv_dcache_v7_all(void);
> > diff --git a/arch/arm/kernel/sr_platform.c b/arch/arm/kernel/sr_platform.c
> > new file mode 100644
> > index 0000000..530aa1b
> > --- /dev/null
> > +++ b/arch/arm/kernel/sr_platform.c
> > @@ -0,0 +1,48 @@
> > +/*
> > + * Copyright (C) 2008-2011 ARM Limited
> > + *
> > + * Author(s): Jon Callan, Lorenzo Pieralisi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <asm/memory.h>
> > +#include <asm/page.h>
> > +#include <asm/sr_platform_api.h>
> > +#include "sr.h"
> > +
> > +void *context_memory_uncached;
> > +
> > +/*
> > + * Simple memory allocator function.
> > + * Returns start address of allocated region
> > + * Memory is zero-initialized.
> > + */
> > +
> > +static unsigned int watermark;
> > +
> > +void *get_memory(unsigned int size)
> > +{
> > + unsigned ret;
> > + void *vmem = NULL;
> > +
> > + ret = watermark;
> > + watermark += size;
> > + BUG_ON(watermark >= CONTEXT_SPACE_UNCACHED);
> > + vmem = (context_memory_uncached + ret);
> > + watermark = ALIGN(watermark, sizeof(long long));
> > +
> > + return vmem;
> > +}
> > +
> > +int sr_platform_init(void)
> > +{
> > + memset(context_memory_uncached, 0, CONTEXT_SPACE_UNCACHED);
> > + return arch->init();
> > +}
>
> sr_platform_init() looks like its a pointless additional function just
> here to obfuscate the code. This code could very well be at the
> sr_platform_init() callsite, which would help make the code more
> understandable.
>
> And why the initialization of your simple memory allocator is part of
> the platform code I've no idea.
>
Yes, I have to improve the layout as mentioned above.
> > diff --git a/arch/arm/kernel/sr_power.c b/arch/arm/kernel/sr_power.c
> > new file mode 100644
> > index 0000000..2585559
> > --- /dev/null
> > +++ b/arch/arm/kernel/sr_power.c
> > @@ -0,0 +1,26 @@
> > +/*
> > + * Copyright (C) 2008-2011 ARM Limited
> > + *
> > + * Author(s): Jon Callan, Lorenzo Pieralisi
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include "sr.h"
> > +
> > +int sr_platform_enter_cstate(unsigned cpu_index,
> > + struct sr_cpu *cpu,
> > + struct sr_cluster *cluster)
> > +{
> > + return arch->enter_cstate(cpu_index, cpu, cluster);
> > +}
> > +
> > +int sr_platform_leave_cstate(unsigned cpu_index,
> > + struct sr_cpu *cpu,
> > + struct sr_cluster *cluster)
> > +{
> > + return arch->leave_cstate(cpu_index, cpu, cluster);
> > +}
>
> Is this really worth being a new separate .c file when all it does is
> call other functions via pointers. What about an inline function
> doing this in a header file.
>
You are right throughout Russell, I will rework it.
But please keep in mind the cpu_suspend/cpu_resume bits and let me know
how you think we can best use them within cpuidle.
Thank you very much indeed,
Lorenzo
More information about the linux-arm-kernel
mailing list