OLPC power management patches - merge for 3.1?

OLPC power management patches - merge for 3.1?

[Daniel Drake]

Hi,

Many thanks for committing the OLPC power management patches to
linux-tip a few weeks ago.

Even though they were never merged into the master branch of linux-tip
(where I presume linux-next pulls from), they have been included in
linux-next for even longer via Andrew Morton's tree.

Is there any chance you could send them for inclusion in Linux 3.1?
The functionality is working well, but a small fixup is needed for
XO-1.5 (see "[PATCH] x86, olpc-xo15-sci: enable EC wakeup capability"
from 2011-07-09).

Thanks!
Daniel

Re: OLPC power management patches - merge for 3.1?

[Randy Dunlap]

On Sat, 23 Jul 2011 23:44:09 +0100 Daniel Drake wrote:

> Hi,
> 
> Many thanks for committing the OLPC power management patches to
> linux-tip a few weeks ago.
> 
> Even though they were never merged into the master branch of linux-tip
> (where I presume linux-next pulls from), they have been included in
> linux-next for even longer via Andrew Morton's tree.
> 
> Is there any chance you could send them for inclusion in Linux 3.1?
> The functionality is working well, but a small fixup is needed for
> XO-1.5 (see "[PATCH] x86, olpc-xo15-sci: enable EC wakeup capability"
> from 2011-07-09).

Hi,

I should have posted this sooner...

I have no objection to merging OLPC patches, but they should build
without errors.

>From linux-next 20110722:

arch/x86/built-in.o: In function `battery_status_changed':
olpc-xo15-sci.c:(.text+0x3acdd): undefined reference to `power_supply_get_by_name'
olpc-xo15-sci.c:(.text+0x3ad04): undefined reference to `power_supply_changed'
arch/x86/built-in.o: In function `ac_status_changed':
olpc-xo15-sci.c:(.text+0x3ad4d): undefined reference to `power_supply_get_by_name'
olpc-xo15-sci.c:(.text+0x3ad74): undefined reference to `power_supply_changed'

Looks like this problem is caused by:

config OLPC_XO1_SCI
	bool "OLPC XO-1 SCI extras"
	depends on OLPC && OLPC_XO1_PM && POWER_SUPPLY

or
config OLPC_XO15_SCI
	bool "OLPC XO-1.5 SCI extras"
	depends on OLPC && ACPI && POWER_SUPPLY

In both cases, the 'bool' depends on one or more tristate symbols, so
the tristates are satisfied if they are =m or =y.  This should work fine
if these kconfig symbols (listed above) were tristate instead of bool.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: OLPC power management patches - merge for 3.1?

[Daniel Drake]

On 24 July 2011 04:20, Randy Dunlap <rdunlap@xenotime.net> wrote:
> I have no objection to merging OLPC patches, but they should build
> without errors.

Agreed! Thanks for reporting the issue.

> Looks like this problem is caused by:
>
> config OLPC_XO1_SCI
>        bool "OLPC XO-1 SCI extras"
>        depends on OLPC && OLPC_XO1_PM && POWER_SUPPLY
>
> or
> config OLPC_XO15_SCI
>        bool "OLPC XO-1.5 SCI extras"
>        depends on OLPC && ACPI && POWER_SUPPLY
>
> In both cases, the 'bool' depends on one or more tristate symbols, so
> the tristates are satisfied if they are =m or =y.  This should work fine
> if these kconfig symbols (listed above) were tristate instead of bool.

In this case, we do need them to be bool options. At least for now. We
tried for a modular design earlier in the review process but it added
too much complexity.

So, whats the best way to fix the Kconfig? Should we:

depends on POWER_SUPPLY=y

or

depends on POWER_SUPPLY
select POWER_SUPPLY

?

Thanks,
Daniel

Re: OLPC power management patches - merge for 3.1?

[Randy Dunlap]

On Sun, 24 Jul 2011 09:59:20 +0100 Daniel Drake wrote:

> On 24 July 2011 04:20, Randy Dunlap <rdunlap@xenotime.net> wrote:
> > I have no objection to merging OLPC patches, but they should build
> > without errors.
> 
> Agreed! Thanks for reporting the issue.
> 
> > Looks like this problem is caused by:
> >
> > config OLPC_XO1_SCI
> >        bool "OLPC XO-1 SCI extras"
> >        depends on OLPC && OLPC_XO1_PM && POWER_SUPPLY
> >
> > or
> > config OLPC_XO15_SCI
> >        bool "OLPC XO-1.5 SCI extras"
> >        depends on OLPC && ACPI && POWER_SUPPLY
> >
> > In both cases, the 'bool' depends on one or more tristate symbols, so
> > the tristates are satisfied if they are =m or =y.  This should work fine
> > if these kconfig symbols (listed above) were tristate instead of bool.
> 
> In this case, we do need them to be bool options. At least for now. We
> tried for a modular design earlier in the review process but it added
> too much complexity.
> 
> So, whats the best way to fix the Kconfig? Should we:

If I knew a good solution, I would have posted it.

> depends on POWER_SUPPLY=y

You could do that.

> or
> 
> depends on POWER_SUPPLY
> select POWER_SUPPLY
> 
> ?

You can't do both of those AFAIK -- and it already depends on POWER_SUPPLY.
I guess you could remove "depends on POWER_SUPPLY" and add "select POWER_SUPPLY".
I'll see if that works.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: OLPC power management patches - merge for 3.1?

[Daniel Drake]

On 24 July 2011 18:13, Randy Dunlap <rdunlap@xenotime.net> wrote:
> You can't do both of those AFAIK -- and it already depends on POWER_SUPPLY.
> I guess you could remove "depends on POWER_SUPPLY" and add "select POWER_SUPPLY".
> I'll see if that works.

It does indeed. I'm just wrapping up testing it and making sure that
all the other options are fine.

Thanks,
Daniel

Re: OLPC power management patches - merge for 3.1?

[Randy Dunlap]

On Sun, 24 Jul 2011 18:16:33 +0100 Daniel Drake wrote:

> On 24 July 2011 18:13, Randy Dunlap <rdunlap@xenotime.net> wrote:
> > You can't do both of those AFAIK -- and it already depends on POWER_SUPPLY.
> > I guess you could remove "depends on POWER_SUPPLY" and add "select POWER_SUPPLY".
> > I'll see if that works.
> 
> It does indeed. I'm just wrapping up testing it and making sure that
> all the other options are fine.

Yes, my build works OK with that change.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: OLPC power management patches - merge for 3.1?

[Andres Salomon]

On Sun, 24 Jul 2011 10:13:23 -0700
Randy Dunlap <rdunlap@xenotime.net> wrote:
[...]
> > or
> > 
> > depends on POWER_SUPPLY
> > select POWER_SUPPLY
> > 
> > ?
> 
> You can't do both of those AFAIK -- and it already depends on
> POWER_SUPPLY. I guess you could remove "depends on POWER_SUPPLY" and
> add "select POWER_SUPPLY". I'll see if that works.
> 


Yeah, select appears to be the only option so long as bool stuff in
arch/ depends upon tristate stuff in drivers/.  It'd be nice to get the
OLPC PM code to be modular, but getting it upstream (the code is from
2007 or 2008!) is a nice first step.

Re: OLPC power management patches - merge for 3.1?

[Ingo Molnar]

* Daniel Drake <dsd@laptop.org> wrote:

> Hi,
> 
> Many thanks for committing the OLPC power management patches to
> linux-tip a few weeks ago.
> 
> Even though they were never merged into the master branch of linux-tip

it's excluded because in testing i found a boot crash with it.  
Thomas did a quick fix for it but i have not heard about it since 
then - Thomas/Peter, is this all resolved in x86/olpc?

(There's also the build failure Randy reported.)

> (where I presume linux-next pulls from), they have been included in 
> linux-next for even longer via Andrew Morton's tree.

Andrew probably thought they were missed - they werent.

Thanks,

	Ingo

------------->
Subject: drivers/serial/apbuart: Fix boot crash
From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 16 Dec 2010 12:38:03 +0100

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/serial/apbuart.c |   58 ++++++++++++++++++-----------------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

Index: linux-2.6-tip/drivers/serial/apbuart.c
===================================================================
--- linux-2.6-tip.orig/drivers/serial/apbuart.c
+++ linux-2.6-tip/drivers/serial/apbuart.c
@@ -593,71 +593,57 @@ static struct of_platform_driver grlib_a
 };
 
 
-static void grlib_apbuart_configure(void)
+static int grlib_apbuart_configure(void)
 {
-	static int enum_done;
 	struct device_node *np, *rp;
-	struct uart_port *port = NULL;
 	const u32 *prop;
-	int freq_khz;
-	int v = 0, d = 0;
-	unsigned int addr;
-	int irq, line;
-	struct amba_prom_registers *regs;
-
-	if (enum_done)
-		return;
+	int freq_khz, line = 0;
 
 	/* Get bus frequency */
 	rp = of_find_node_by_path("/");
+	if (!rp)
+		return -ENODEV;
 	rp = of_get_next_child(rp, NULL);
+	if (!rp)
+		return -ENODEV;
 	prop = of_get_property(rp, "clock-frequency", NULL);
+	if (!prop)
+		return -ENODEV;
 	freq_khz = *prop;
 
-	line = 0;
 	for_each_matching_node(np, apbuart_match) {
+		const int *irqs = of_get_property(np, "interrupts", NULL);
+		const struct amba_prom_registers *regs;
+		struct uart_port *port;
 
-		int *vendor = (int *) of_get_property(np, "vendor", NULL);
-		int *device = (int *) of_get_property(np, "device", NULL);
-		int *irqs = (int *) of_get_property(np, "interrupts", NULL);
-		regs = (struct amba_prom_registers *)
-		    of_get_property(np, "reg", NULL);
-
-		if (vendor)
-			v = *vendor;
-		if (device)
-			d = *device;
+		regs = of_get_property(np, "reg", NULL);
 
 		if (!irqs || !regs)
-			return;
+			return -ENODEV;
 
 		grlib_apbuart_nodes[line] = np;
 
-		addr = regs->phys_addr;
-		irq = *irqs;
-
 		port = &grlib_apbuart_ports[line];
-
-		port->mapbase = addr;
-		port->membase = ioremap(addr, sizeof(struct grlib_apbuart_regs_map));
-		port->irq = irq;
+		port->mapbase = regs->phys_addr;
+		port->membase = ioremap(regs->phys_addr,
+					sizeof(struct grlib_apbuart_regs_map));
+		port->irq = *irqs;
 		port->iotype = UPIO_MEM;
 		port->ops = &grlib_apbuart_ops;
 		port->flags = UPF_BOOT_AUTOCONF;
 		port->line = line;
 		port->uartclk = freq_khz * 1000;
-		port->fifosize = apbuart_scan_fifo_size((struct uart_port *) port, line);
+		port->fifosize =
+			apbuart_scan_fifo_size((struct uart_port *) port, line);
 		line++;
 
 		/* We support maximum UART_NR uarts ... */
 		if (line == UART_NR)
 			break;
-
 	}
 
-	enum_done = 1;
-
 	grlib_apbuart_driver.nr = grlib_apbuart_port_nr = line;
+	return line ? 0 : -ENODEV;
 }
 
 static int __init grlib_apbuart_init(void)
@@ -665,7 +651,9 @@ static int __init grlib_apbuart_init(voi
 	int ret;
 
 	/* Find all APBUARTS in device the tree and initialize their ports */
-	grlib_apbuart_configure();
+	ret = grlib_apbuart_configure();
+	if (ret)
+		return ret;
 
 	printk(KERN_INFO "Serial: GRLIB APBUART driver\n");
 

Re: OLPC power management patches - merge for 3.1?

[Daniel Drake]

On 24 July 2011 08:27, Ingo Molnar <mingo@elte.hu> wrote:
> it's excluded because in testing i found a boot crash with it.
> Thomas did a quick fix for it but i have not heard about it since
> then - Thomas/Peter, is this all resolved in x86/olpc?

Ah, sorry if we broke something. Can you go into more details on the
crash? We haven't heard anything on these lines. I'm willing to help
out fixing it.

Is it related to the apbuart serial patch you included in your mail? I
don't see how that could have been affected by OLPC patches - but I am
probably missing something.

> (There's also the build failure Randy reported.)

Thanks for reporting, I'll send a patch to fix that shortly.

Thanks!
Daniel

Re: OLPC power management patches - merge for 3.1?

[Ingo Molnar]

* Daniel Drake <dsd@laptop.org> wrote:

> On 24 July 2011 08:27, Ingo Molnar <mingo@elte.hu> wrote:
> > it's excluded because in testing i found a boot crash with it.
> > Thomas did a quick fix for it but i have not heard about it since
> > then - Thomas/Peter, is this all resolved in x86/olpc?
> 
> Ah, sorry if we broke something. Can you go into more details on the
> crash? We haven't heard anything on these lines. I'm willing to help
> out fixing it.
> 
> Is it related to the apbuart serial patch you included in your mail? I
> don't see how that could have been affected by OLPC patches - but I am
> probably missing something.

i don't have the details anymore, i'll re-test once the build failure 
is fixed. We can certainly push it in this merge window if it's 
stable.

Thanks,

	Ingo