Re: [PATCH] Add idle wait support for 44x platforms

[Jerone Young <jyoung5@us.ibm.com>]

On Thu, 2008-04-03 at 21:00 -0500, Josh Boyer wrote:
> On Thu, 03 Apr 2008 17:43:02 -0500
> Jerone Young <jyoung5@us.ibm.com> wrote:
> 
> > # HG changeset patch
> > # User Jerone Young <jyoung5@us.ibm.com>
> > # Date 1207262487 18000
> > # Node ID 7226bef216680748a50327900572c2fbc3e762b0
> > # Parent  a5b2aebbc6ebd2439c655f1c047ed7e3c1991ec1
> 
> As a complete and unrelated side note to the actual patch, wtf is this
> hg stuff?  I can't really tell what tree you're even basing this off of.

hehe...I primary use hg as it's just much easier to deal with. Yes I do
go through the conversion. But it is worth it. The patches I send are in
git format.

> 
> > Add idle wait support for 44x platforms
> > 
> > This patch adds the ability for the CPU to go into wait state while in cpu_idle loop. This helps virtulization solutions know when the guest Linux kernel is in an idle state. There are two ways to do it.
> 
> This huge single line needs fixing in the next version of the patch.
> 
> > 
> > 1) Command line
> > 	idle=spin <-- CPU will spin (this is the default)
> > 	idle=wait <-- set CPU into wait state when idle
> > 
> > 2) The device tree will be checked for the "/hypervisor" node
> >    If this node is seen it will use "wait" for idle, so that
> >    the hypervisor can know when guest Linux kernel it is in
> >    an idle state.
> > 
> > This patch, unlike the last, isolates the code to 44x platforms.
> 
> In addition to the comments Tony and Hollis made, I have a few of my
> own.
> 
> > Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> > 
> > diff --git a/arch/powerpc/platforms/44x/44x.h b/arch/powerpc/platforms/44x/44x.h
> > --- a/arch/powerpc/platforms/44x/44x.h
> > +++ b/arch/powerpc/platforms/44x/44x.h
> > @@ -5,4 +5,6 @@ extern void as1_writeb(u8 data, volatile
> >  extern void as1_writeb(u8 data, volatile u8 __iomem *addr);
> >  extern void ppc44x_reset_system(char *cmd);
> > 
> > +extern int ppc44x_idle_init(void);
> > +
> >  #endif /* __POWERPC_PLATFORMS_44X_44X_H */
> 
> The changes to this file aren't needed.  See below.
> 
> > diff --git a/arch/powerpc/platforms/44x/Makefile b/arch/powerpc/platforms/44x/Makefile
> > --- a/arch/powerpc/platforms/44x/Makefile
> > +++ b/arch/powerpc/platforms/44x/Makefile
> > @@ -1,4 +1,5 @@ obj-$(CONFIG_44x)	:= misc_44x.o
> >  obj-$(CONFIG_44x)	:= misc_44x.o
> > +obj-$(CONFIG_44x)	+= idle.o
> 
> Just add this target to the already existing obj-(CONFIG_44x)
> 
> >  obj-$(CONFIG_EBONY)	+= ebony.o
> >  obj-$(CONFIG_TAISHAN)	+= taishan.o
> >  obj-$(CONFIG_BAMBOO)	+= bamboo.o
> > diff --git a/arch/powerpc/platforms/44x/bamboo.c b/arch/powerpc/platforms/44x/bamboo.c
> > --- a/arch/powerpc/platforms/44x/bamboo.c
> > +++ b/arch/powerpc/platforms/44x/bamboo.c
> > @@ -61,3 +61,5 @@ define_machine(bamboo) {
> >  	.restart			= ppc44x_reset_system,
> >  	.calibrate_decr 	= generic_calibrate_decr,
> >  };
> > +
> > +machine_late_initcall(bamboo, ppc44x_idle_init);
> 
> Ugh.  Don't add an init call to every 4xx board like this.  It's not
> needed.  See below.
> 
> > diff --git a/arch/powerpc/platforms/44x/idle.c b/arch/powerpc/platforms/44x/idle.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/44x/idle.c
> 
> If you're ever going to extend bare metal support for this to 40x, then
> this is the wrong place for it.  It should reside in
> arch/powerpc/sysdev/ppc4xx_soc.c in that case.

So I did this at first after your suggestions but then I changed it
because:

- it depended on CONFIG_4xx_SOC .. which I could not figure out who
actually enabled this

- wanted to make a patch that could also go into earlier kernels 


> 
> > +
> > +#include <linux/of.h>
> > +#include <asm/machdep.h>
> > +
> > +static void ppc44x_idle(void);
> 
> This isn't needed.  Move the structures below the function.

This can be done
> 
> > +struct sleep_mode {
> > +	char *name;
> > +	void (*entry)(void);
> > +};
> > +
> > +static struct sleep_mode modes[] = {
> > +	{ .name = "spin", .entry = NULL },
> > +	{ .name = "wait", .entry = &ppc44x_idle },
> > +};
> 
> <snip>
> 
> > +int __init ppc44x_idle_init(void)
> > +{
> > +	if(of_find_node_by_path("/hypervisor") != NULL) {
> > +		/* if we find /hypervisor node is in device tree,
> > +		   set idle mode to wait */
> > +		current_mode = 1; /* wait mode */
> > +	}
> > +
> > +	ppc_md.power_save = modes[current_mode].entry;
> > +	return 0;
> 
> I liked Hollis' method of assignment here.
> 
> > +}
> 
> Add an arch_initcall(ppc44x_idle_init); here and dispense with
> changing every board .c file in the 44x directory.
ah didn't know about arch_initcall

> 
> josh