Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Pavel Machek]

On Wed 2010-01-06 15:17:39, Eric Miao wrote:
> On Wed, Jan 6, 2010 at 3:10 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Sharp-SL code uses strange, complex grouping of gpios for wakeups
> > toggling. Fortunately, it is unneeded in recent kernels (and actually
> > provokes WARN_ONs during resume). Remove it.
> >
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> Pavel,
> 
> The code to be removed below is used to support pxa27x_keypad
> to be able to resume from sleep. What's the exact reason to remove
> this on spitz?

Well, otherwise I get this during resume: (2.6.32 regression)

(and similar for matrix-keypad, but dmitry worked around that
already).

Problem is that gpio-keys and matrix-keypad want to set_wake for each
gpio individually, hw can do that, but pxa27x.c breaks it.

I don't get it; what is pxa27x_keypad used on? It looks like
matrix-keypad subset.

								Pavel

pxa2xx-spi pxa2xx-spi.2: resume
sharp-scoop sharp-scoop.0: resume
matrix-keypad matrix-keypad: resume
gpio-keys gpio-keys: resume
------------[ cut here ]------------
WARNING: at kernel/irq/manage.c:361 set_irq_wake+0x7c/0x138()
Unbalanced IRQ 191 wake disable
Modules linked in:
[<c0024900>] (unwind_backtrace+0x0/0xe4) from [<c00354f4>] (warn_slowpath_common+0x4c/0x80)
[<c00354f4>] (warn_slowpath_common+0x4c/0x80) from [<c0035564>] (warn_slowpath_fmt+0x28/0x38)
[<c0035564>] (warn_slowpath_fmt+0x28/0x38) from [<c00622f4>] (set_irq_wake+0x7c/0x138)
[<c00622f4>] (set_irq_wake+0x7c/0x138) from [<c01d2370>] (gpio_keys_resume+0x94/0xd0)
[<c01d2370>] (gpio_keys_resume+0x94/0xd0) from [<c0193a50>] (platform_pm_resume+0x30/0x54)
[<c0193a50>] (platform_pm_resume+0x30/0x54) from [<c01961e4>] (pm_op+0xa0/0xc0)
[<c01961e4>] (pm_op+0xa0/0xc0) from [<c0196c94>] (dpm_resume_end+0x100/0x474)
[<c0196c94>] (dpm_resume_end+0x100/0x474) from [<c005ef7c>] (suspend_devices_and_enter+0x8c/0x1dc)
[<c005ef7c>] (suspend_devices_and_enter+0x8c/0x1dc) from [<c005f1b4>] (enter_state+0xe8/0x120)
[<c005f1b4>] (enter_state+0xe8/0x120) from [<c005e8b8>] (state_store+0x90/0xc4)
[<c005e8b8>] (state_store+0x90/0xc4) from [<c01481a4>] (kobj_attr_store+0x1c/0x24)
[<c01481a4>] (kobj_attr_store+0x1c/0x24) from [<c00daab0>] (sysfs_write_file+0x104/0x18c)
[<c00daab0>] (sysfs_write_file+0x104/0x18c) from [<c00922cc>] (vfs_write+0xb0/0x164)
[<c00922cc>] (vfs_write+0xb0/0x164) from [<c0092450>] (sys_write+0x40/0x70)
[<c0092450>] (sys_write+0x40/0x70) from [<c001ff60>] (ret_fast_syscall+0x0/0x28)
---[ end trace d1b2070efbc838e4 ]---
leds-gpio leds-gpio: resume
sharpsl-nand sharpsl-nand: resume


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Eric Miao]

On Thu, Jan 7, 2010 at 2:52 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2010-01-06 15:17:39, Eric Miao wrote:
>> On Wed, Jan 6, 2010 at 3:10 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >
>> > Sharp-SL code uses strange, complex grouping of gpios for wakeups
>> > toggling. Fortunately, it is unneeded in recent kernels (and actually
>> > provokes WARN_ONs during resume). Remove it.
>> >
>> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>
>> Pavel,
>>
>> The code to be removed below is used to support pxa27x_keypad
>> to be able to resume from sleep. What's the exact reason to remove
>> this on spitz?
>
> Well, otherwise I get this during resume: (2.6.32 regression)
>
> (and similar for matrix-keypad, but dmitry worked around that
> already).
>
> Problem is that gpio-keys and matrix-keypad want to set_wake for each
> gpio individually, hw can do that, but pxa27x.c breaks it.
>
> I don't get it; what is pxa27x_keypad used on? It looks like
> matrix-keypad subset.
>

pxa27x has its own specific keypad controller. And since we now
use enable_irq_wake, and the keypad controller has only one
such IRQ_KEYPAD, will have to setup the keypad GPIO wakeup
as a whole.

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Stanislav Brabec]

Eric Miao wrote:
> >
> > I don't get it; what is pxa27x_keypad used on? It looks like
> > matrix-keypad subset.

> pxa27x has its own specific keypad controller. And since we now
> use enable_irq_wake, and the keypad controller has only one
> such IRQ_KEYPAD, will have to setup the keypad GPIO wakeup
> as a whole.

Looking deeper into it, the problem seems to be elsewhere:
I just added debug message to start and end of set_irq_wake().

And here is what I got during suspend:

set_irq_wake() started IRQ: 191 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 191 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 108 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 108 to 1, desc->wake_depth: 1
set_irq_wake() started IRQ: 113 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 113 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 187 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 187 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 130 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 130 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 132 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 132 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 134 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 134 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 135 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 135 to 1, desc->wake_depth: 0
set_irq_wake() started IRQ: 31 to 1, desc->wake_depth: 0
set_irq_wake() done IRQ: 31 to 1, desc->wake_depth: 1

I think that if (desc->wake_depth == 0) when set_irq_wake(foo, 1) is
done, then something went wrong. And it is not surprise, that it reports
Unbalanced IRQ on resume.

Note that other people (Cyril Hrubiš) with a bit different .config
report success with gpio keys driver turned on.

My config:
http://www.penguin.cz/~utx/zaurus/feed/images/spitz/config-2.6.33-rc4-spitz

OT: Serial console resume did not work properly even with
no_console_suspend and my patch:
[PATCH 4/8] serial-core: resume serial hardware with no_console_suspend


________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx/zaurus

gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Stanislav Brabec]

Stanislav Brabec wrote:

> Looking deeper into it, the problem seems to be elsewhere:
> I just added debug message to start and end of set_irq_wake().

> I think that if (desc->wake_depth == 0) when set_irq_wake(foo, 1) is
> done, then something went wrong. And it is not surprise, that it reports
> Unbalanced IRQ on resume.

Going even deeper, I see:
set_irq_wake() started IRQ: 191 to 1, desc->wake_depth: 0
pxa27x_set_wake() IRQ: 191 to 1, GPIO: 95
pxa27x_set_wake() calling gpio_set_wake()
gpio_set_wake() GPIO: 95, on: 1
gpio_set_wake() returning EINVAL (keypad_gpio)
set_irq_wake() done IRQ: 191 to 1, desc->wake_depth: 0

IRQ 191 is associated with GPIO 95. GPIO 95 is listed as one of GPIO
supported by Power Manager Keyboard Wake-Up Enable Register (PKWR, see
pxa27x_pkwr_gpio[] in arch/arm/mach-pxa/mfp-pxa2xx.c) (section 3.8.1.15
of Intel PXA27x Processor Family Developer's Manual).

gpio_set_wake() in arch/arm/mach-pxa/mfp-pxa2xx.c explicitly refuses to
configure PWER capable registers.

To finish the mess, spitz_presuspend() from arch/arm/mach-pxa/spitz_pm.c
reprograms all wake-up registers on its own.

So I see two possible fixes:

1) Follow include/linux/interrupt.h:
   When
 * requesting an interrupt without specifying a IRQF_TRIGGER, the
 * setting should be assumed to be "as already configured", which
 * may be as per machine or firmware initialisation.

and keep the spitz_pm.c code.

It would mean that gpio_set_wake() should be able to set keypad
interrpupts. The code would be very uncomfortable. Without knowing
whether the GPIO is programmed for by PKWR or PWER, it needs one extra
translation and register lookup. Or one extra record in the table and a
more complicated initialization in the platform file.

2) gpio_keys driver should be capable to set IRQF_TRIGGER_* and no
settings of wake-up registers in spitz_pm.c would not be needed.

On platforms with shared interrupts it would introduce possible multiple
trigger initialization (not a big problem). But it would easily
introduce breakage if programmer makes a mistake and configures
interrupt with different trigger in different drivers.


I am not sure what solution of these two is in spirit of modern kernels.
I guess that 2. Especially because somebody may want to use gpio_keys on
a different machine/GPIO layout that would require different
IRQF_TRIGGER_*.


Notes:

Automatic PKWR/PWER logic is impossible for PXA270. GPIO 38 can be
programmed to use both PKWR or PWER.

The gpio_keys seems to have problems with debounce - in 50% of all
resumes, Zaurus goes back to sleep in a second or so.

Adding debugging patch for your convenience:

diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
index cf6b720..4e82cea 100644
--- a/arch/arm/mach-pxa/mfp-pxa2xx.c
+++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
@@ -168,22 +168,35 @@ int gpio_set_wake(unsigned int gpio, unsigned int on)
 {
 	struct gpio_desc *d;
 	unsigned long c, mux_taken;
+printk(KERN_INFO "gpio_set_wake() GPIO: %d, on: %d\n", gpio, on);
 
 	if (gpio > mfp_to_gpio(MFP_PIN_GPIO127))
+{
+printk(KERN_INFO "gpio_set_wake() returning EINVAL\n");
 		return -EINVAL;
+}
 
 	d = &gpio_desc[gpio];
 	c = d->config;
 
 	if (!d->valid)
+{
+printk(KERN_INFO "gpio_set_wake() returning EINVAL (not valid)\n");
 		return -EINVAL;
+}
 
 	if (d->keypad_gpio)
+{
+printk(KERN_INFO "gpio_set_wake() returning EINVAL (keypad_gpio)\n");
 		return -EINVAL;
+}
 
 	mux_taken = (PWER & d->mux_mask) & (~d->mask);
 	if (on && mux_taken)
+{
+printk(KERN_INFO "gpio_set_wake() returning EBUSY\n");
 		return -EBUSY;
+}
 
 	if (d->can_wakeup && (c & MFP_LPM_CAN_WAKEUP)) {
 		if (on) {
@@ -203,6 +216,7 @@ int gpio_set_wake(unsigned int gpio, unsigned int on)
 			PRER &= ~d->mask;
 			PFER &= ~d->mask;
 		}
+printk(KERN_INFO "gpio_set_wake() modified P?ER\n");
 	}
 	return 0;
 }
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 6a0b731..7fef6b1 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -320,11 +320,18 @@ static int pxa27x_set_wake(unsigned int irq, unsigned int on)
 	int gpio = IRQ_TO_GPIO(irq);
 	uint32_t mask;
 
+	printk(KERN_INFO "pxa27x_set_wake() IRQ: %d to %d, GPIO: %d\n", irq, on, gpio);
 	if (gpio >= 0 && gpio < 128)
+{
+	printk(KERN_INFO "pxa27x_set_wake() calling gpio_set_wake()\n");
 		return gpio_set_wake(gpio, on);
+}
 
 	if (irq == IRQ_KEYPAD)
+{
+	printk(KERN_INFO "pxa27x_set_wake() calling keypad_set_wake(()\n");
 		return keypad_set_wake(on);
+}
 
 	switch (irq) {
 	case IRQ_RTCAlrm:
@@ -334,6 +341,7 @@ static int pxa27x_set_wake(unsigned int irq, unsigned int on)
 		mask = 1u << 26;
 		break;
 	default:
+	printk(KERN_INFO "pxa27x_set_wake() returning EINVAL\n");
 		return -EINVAL;
 	}
 
@@ -341,6 +349,7 @@ static int pxa27x_set_wake(unsigned int irq, unsigned int on)
 		PWER |= mask;
 	else
 		PWER &=~mask;
+	printk(KERN_INFO "pxa27x_set_wake() modified PWER\n");
 
 	return 0;
 }
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..e5d2187 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -344,6 +344,7 @@ int set_irq_wake(unsigned int irq, unsigned int on)
 	unsigned long flags;
 	int ret = 0;
 
+printk(KERN_INFO "set_irq_wake() started IRQ: %d to %d, desc->wake_depth: %d\n", irq, on, desc->wake_depth);
 	/* wakeup-capable irqs can be shared between drivers that
 	 * don't need to have the same sleep mode behaviors.
 	 */
@@ -369,6 +370,7 @@ int set_irq_wake(unsigned int irq, unsigned int on)
 	}
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
+printk(KERN_INFO "set_irq_wake() done IRQ: %d to %d, desc->wake_depth: %d\n", irq, on, desc->wake_depth);
 	return ret;
 }
 EXPORT_SYMBOL(set_irq_wake);



________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx/zaurus

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Eric Miao]

> 2) gpio_keys driver should be capable to set IRQF_TRIGGER_* and no
> settings of wake-up registers in spitz_pm.c would not be needed.
>
> On platforms with shared interrupts it would introduce possible multiple
> trigger initialization (not a big problem). But it would easily
> introduce breakage if programmer makes a mistake and configures
> interrupt with different trigger in different drivers.
>
>
> I am not sure what solution of these two is in spirit of modern kernels.
> I guess that 2. Especially because somebody may want to use gpio_keys on
> a different machine/GPIO layout that would require different
> IRQF_TRIGGER_*.
>

I prefer 2) - the ugly and hardcoded setup in spitz_pm.c should really
be removed. That's why the gpio_set_wake() and keypad_set_wake()
are introduced.

keypad_set_wake() is really specifically introduced for use by pxa27x_keypad
and no generic GPIO stuffs. So it's really annoying a GPIO will use
the PKWR as a wakeup GPIO, I'd recommend one still get this hard coded
into the platform file, with combination of WAKEUP_ON_LEVEL_HIGH (which
is specifically designed for keypad GPIOs) and keypad_set_wake().

The spitz, however, is doing a good job on this though it's using a GPIO
emulated matrix keypad, that there is a separate SPITZ_GPIO_KEY_INT,
which triggers whenever there is any key press on this matrix (I don't
know how that's designed in HW, but it seems to do that job), and
which can be setup as a GPIO wakeup.

>
> Notes:
>
> Automatic PKWR/PWER logic is impossible for PXA270. GPIO 38 can be
> programmed to use both PKWR or PWER.
>

That's a shame of pxa27x, but so far no awkward usage of this
has ever been seen.

> The gpio_keys seems to have problems with debounce - in 50% of all
> resumes, Zaurus goes back to sleep in a second or so.
>

There is a routine checking wakeup source (cause) which will put the
system back into sleep in some cases, I guess you need to check if
something weird there.

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Stanislav Brabec]

Eric Miao wrota:

> I prefer 2) - the ugly and hardcoded setup in spitz_pm.c should really
> be removed. That's why the gpio_set_wake() and keypad_set_wake()
> are introduced.

I am unsure, whether gpio_keys driver is interested in the way, how wake
happens. I guess that is interested only in the fact, that wake
happened.

Handling platform specific edge/level wake setup would only complicate
the code. (In fact, even the PXA270 platform code does not exist yet -
arch/arm/mach-pxa/mfp-pxa2xx.c:__mfp_config_gpio() is not capable to
configure Power Manager Keyboard Wake-Up Enable Register (PKWR).)

I talked to Vojtěch Pavlík and he told that 1 is correct: Follow
include/linux/interrupt.h. Setting edge/level wake mode should be done
in the platform file. The driver could use just irq_set_wake() and don't
care about details. And irq_set_wake() should do something useful even
for PKWR capable GPIO.

> keypad_set_wake() is really specifically introduced for use by pxa27x_keypad
> and no generic GPIO stuffs. So it's really annoying a GPIO will use
> the PKWR as a wakeup GPIO, I'd recommend one still get this hard coded
> into the platform file, with combination of WAKEUP_ON_LEVEL_HIGH (which
> is specifically designed for keypad GPIOs) and keypad_set_wake().

Well, keypad_set_wake() seems to be possibly broken for GPIO 38. Imagine
a device, that has a small keypad, but GPIO 38 has a different purpose
that requires an edge triggered wakeup (PWER). I think that
keypad_set_wake() reprograms it to PKWR.

The problem affects gpio_keys: It is a driver implementing "one key per
gpio". It now handles On/Off and lid switches on Zaurus. Lid switches
are on "normal" GPIOs, On/Off switch is wired to PKWR capable GPIO.

> The spitz, however, is doing a good job on this though it's using a GPIO
> emulated matrix keypad, that there is a separate SPITZ_GPIO_KEY_INT,
> which triggers whenever there is any key press on this matrix (I don't
> know how that's designed in HW, but it seems to do that job), and
> which can be setup as a GPIO wakeup.

SPITZ_GPIO_KEY_INT happens if AC adapter is connected or key is pressed.
Surprisingly, the key press logic is part of NAND flash controller CPLD.
SPITZ_GPIO_KEY_INT==0 - it makes possible to wake Zaurus even from deep
sleep by any key press. It would be impossible only with PKWR.

I guess that this and implementation of keypad_set_wake() is a reason,
why most devices suspend and resume correctly even if the irq_set_wake()
refuses to configure wake and the warning is only visible symptom.


________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Eric Miao]

2010/1/26 Stanislav Brabec <utx@penguin.cz>:
> Eric Miao wrota:
>
>> I prefer 2) - the ugly and hardcoded setup in spitz_pm.c should really
>> be removed. That's why the gpio_set_wake() and keypad_set_wake()
>> are introduced.
>
> I am unsure, whether gpio_keys driver is interested in the way, how wake
> happens. I guess that is interested only in the fact, that wake
> happened.
>
> Handling platform specific edge/level wake setup would only complicate
> the code. (In fact, even the PXA270 platform code does not exist yet -
> arch/arm/mach-pxa/mfp-pxa2xx.c:__mfp_config_gpio() is not capable to
> configure Power Manager Keyboard Wake-Up Enable Register (PKWR).)
>

That's why WAKEUP_ON_EDGE_* is introduced, no need for gpio-keys
to know this.

> I talked to Vojtěch Pavlík and he told that 1 is correct: Follow
> include/linux/interrupt.h. Setting edge/level wake mode should be done
> in the platform file. The driver could use just irq_set_wake() and don't
> care about details. And irq_set_wake() should do something useful even
> for PKWR capable GPIO.
>

I don't mind if IRQF_TRIGGER_ will always be correct regarding the
wakeup edge/level settings in MFP, but honestly - I don't think so.

>> keypad_set_wake() is really specifically introduced for use by pxa27x_keypad
>> and no generic GPIO stuffs. So it's really annoying a GPIO will use
>> the PKWR as a wakeup GPIO, I'd recommend one still get this hard coded
>> into the platform file, with combination of WAKEUP_ON_LEVEL_HIGH (which
>> is specifically designed for keypad GPIOs) and keypad_set_wake().
>
> Well, keypad_set_wake() seems to be possibly broken for GPIO 38. Imagine
> a device, that has a small keypad, but GPIO 38 has a different purpose
> that requires an edge triggered wakeup (PWER). I think that
> keypad_set_wake() reprograms it to PKWR.

Unless someone specifies that by

GPIO38_GPIO | WAKEUP_ON_LEVEL_HIGH,

keypad_set_wake() will never try to enable the bit in PKWR.

>
> The problem affects gpio_keys: It is a driver implementing "one key per
> gpio". It now handles On/Off and lid switches on Zaurus. Lid switches
> are on "normal" GPIOs, On/Off switch is wired to PKWR capable GPIO.
>

Ain't On/Off switch one of the matrix key? And so SPITZ_GPIO_KEY_INT
could be used to handle that?

>> The spitz, however, is doing a good job on this though it's using a GPIO
>> emulated matrix keypad, that there is a separate SPITZ_GPIO_KEY_INT,
>> which triggers whenever there is any key press on this matrix (I don't
>> know how that's designed in HW, but it seems to do that job), and
>> which can be setup as a GPIO wakeup.
>
> SPITZ_GPIO_KEY_INT happens if AC adapter is connected or key is pressed.
> Surprisingly, the key press logic is part of NAND flash controller CPLD.
> SPITZ_GPIO_KEY_INT==0 - it makes possible to wake Zaurus even from deep
> sleep by any key press. It would be impossible only with PKWR.
>
> I guess that this and implementation of keypad_set_wake() is a reason,
> why most devices suspend and resume correctly even if the irq_set_wake()
> refuses to configure wake and the warning is only visible symptom.

_______________________________________________
Zaurus-devel mailing list
Zaurus-devel@lists.linuxtogo.org
http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/zaurus-devel

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Stanislav Brabec]

Eric Miao wrote:
> 2010/1/26 Stanislav Brabec <utx-ZsETY1VsSgLrBKCeMvbIDA@public.gmane.org>:

> > Handling platform specific edge/level wake setup would only complicate
> > the code. (In fact, even the PXA270 platform code does not exist yet -
> > arch/arm/mach-pxa/mfp-pxa2xx.c:__mfp_config_gpio() is not capable to
> > configure Power Manager Keyboard Wake-Up Enable Register (PKWR).)
> That's why WAKEUP_ON_EDGE_* is introduced, no need for gpio-keys
> to know this.

But WAKEUP_ON_EDGE_* is impossible for GPIO 95,
enable_irq_wake()/gpio_set_wake() returns EINVAL and disable_irq_wake()
complains on "Unbalanced IRQ 191".

In case of Zaurus, gpio_keys.c have to live with wakeup on level (PKWR)
when using enable/disable_irq_wake().

> > The problem affects gpio_keys: It is a driver implementing "one key per
> > gpio". It now handles On/Off and lid switches on Zaurus. Lid switches
> > are on "normal" GPIOs, On/Off switch is wired to PKWR capable GPIO.
> Ain't On/Off switch one of the matrix key? And so SPITZ_GPIO_KEY_INT
> could be used to handle that?

No. On/Off is connected directly to GPIO 95. Other keys use strobe/sense
matrix keypad.

SPITZ_GPIO_KEY_INT is a bit unspecific. I think it means: Charger was
inserted or key was pressed or On/Off was pressed.



________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx/zaurus

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Dmitry Eremin-Solenikov]

Hello,

2010/1/26 Stanislav Brabec <utx@penguin.cz>:
> Eric Miao wrote:
>> > The problem affects gpio_keys: It is a driver implementing "one key per
>> > gpio". It now handles On/Off and lid switches on Zaurus. Lid switches
>> > are on "normal" GPIOs, On/Off switch is wired to PKWR capable GPIO.
>> Ain't On/Off switch one of the matrix key? And so SPITZ_GPIO_KEY_INT
>> could be used to handle that?
>
> No. On/Off is connected directly to GPIO 95. Other keys use strobe/sense
> matrix keypad.
>
> SPITZ_GPIO_KEY_INT is a bit unspecific. I think it means: Charger was
> inserted or key was pressed or On/Off was pressed.

This seems to be true. At least they used the same design on tosa and other
zaurii.


-- 
With best wishes
Dmitry

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Eric Miao]

2010/1/26 Stanislav Brabec <utx-ZsETY1VsSgLrBKCeMvbIDA@public.gmane.org>:
> Eric Miao wrote:
>> 2010/1/26 Stanislav Brabec <utx-ZsETY1VsSgLrBKCeMvbIDA@public.gmane.org>:
>
>> > Handling platform specific edge/level wake setup would only complicate
>> > the code. (In fact, even the PXA270 platform code does not exist yet -
>> > arch/arm/mach-pxa/mfp-pxa2xx.c:__mfp_config_gpio() is not capable to
>> > configure Power Manager Keyboard Wake-Up Enable Register (PKWR).)
>> That's why WAKEUP_ON_EDGE_* is introduced, no need for gpio-keys
>> to know this.
>
> But WAKEUP_ON_EDGE_* is impossible for GPIO 95,
> enable_irq_wake()/gpio_set_wake() returns EINVAL and disable_irq_wake()
> complains on "Unbalanced IRQ 191".
>

Now I see, but I don't know why disable_irq_wake() will complains about
unbalance since it should really manage it well.

A quick dirty solution would be the platform to call keypad_set_wake()
directly somewhere. Otherwise we have to let gpio_set_wake() to handle
those keypad GPIOs and to live together with keypad_set_wake() happily,
which is really difficult.

> In case of Zaurus, gpio_keys.c have to live with wakeup on level (PKWR)
> when using enable/disable_irq_wake().
>
>> > The problem affects gpio_keys: It is a driver implementing "one key per
>> > gpio". It now handles On/Off and lid switches on Zaurus. Lid switches
>> > are on "normal" GPIOs, On/Off switch is wired to PKWR capable GPIO.
>> Ain't On/Off switch one of the matrix key? And so SPITZ_GPIO_KEY_INT
>> could be used to handle that?
>
> No. On/Off is connected directly to GPIO 95. Other keys use strobe/sense
> matrix keypad.
>
> SPITZ_GPIO_KEY_INT is a bit unspecific. I think it means: Charger was
> inserted or key was pressed or On/Off was pressed.
>
>
>
> ________________________________________________________________________
> Stanislav Brabec
> http://www.penguin.cz/~utx/zaurus
>
>

Re: gpio_keys and how PXA27x sets gpio_set_wake() (was Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1)

[Stanislav Brabec]

Eric Miao wrote:
> 2010/1/26 Stanislav Brabec <utx@penguin.cz>:
> > Eric Miao wrote:
> >> 2010/1/26 Stanislav Brabec <utx@penguin.cz>:
> >
> >> > Handling platform specific edge/level wake setup would only complicate
> >> > the code. (In fact, even the PXA270 platform code does not exist yet -
> >> > arch/arm/mach-pxa/mfp-pxa2xx.c:__mfp_config_gpio() is not capable to
> >> > configure Power Manager Keyboard Wake-Up Enable Register (PKWR).)
> >> That's why WAKEUP_ON_EDGE_* is introduced, no need for gpio-keys
> >> to know this.
> >
> > But WAKEUP_ON_EDGE_* is impossible for GPIO 95,
> > enable_irq_wake()/gpio_set_wake() returns EINVAL and disable_irq_wake()
> > complains on "Unbalanced IRQ 191".

> Now I see, but I don't know why disable_irq_wake() will complains about
> unbalance since it should really manage it well.

Because gpio_set_wake() returned EINVAL, set_irq_wake() assumed error
and did not increment wake_depth, the whole enable_irq_wake() was a big
NOP. disable_irq_wake() seen wake_depth being zero and complains.

> A quick dirty solution would be the platform to call keypad_set_wake()
> directly somewhere. Otherwise we have to let gpio_set_wake() to handle
> those keypad GPIOs and to live together with keypad_set_wake() happily,
> which is really difficult.

I was thinking about it as well (and even tested that it works):
gpio_set_wake():
if (d->keypad_gpio)
	return keypad_set_wake(on);

But keypad_set_wake() always sets all keypad GPIOs, not just a single
one. And there is GPIO 36, that can be configured in more ways.

gpio_set_wake() and only set wake, keeping the mode as it was before.
(Well, it's again impossible for GPIO 36 without storing this
information somewhere.)

But well, another idea:

Only matrix_keypad driver should be aware of level triggered wakeup. All
other drivers could follow *_irq_wake documentation and not care about
it. If edge triggered interrupt is available, it should be preferred
there, if not, level triggered wake should be used instead. Hardware
designers should know what they are doing.


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12           tel: +420 284 028 966, +49 911 740538747
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Pavel Machek]

Hi!

> >> > Sharp-SL code uses strange, complex grouping of gpios for wakeups
> >> > toggling. Fortunately, it is unneeded in recent kernels (and actually
> >> > provokes WARN_ONs during resume). Remove it.
> >> >
> >> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> >>
> >> Pavel,
> >>
> >> The code to be removed below is used to support pxa27x_keypad
> >> to be able to resume from sleep. What's the exact reason to remove
> >> this on spitz?
> >
> > Well, otherwise I get this during resume: (2.6.32 regression)
> >
> > (and similar for matrix-keypad, but dmitry worked around that
> > already).
> >
> > Problem is that gpio-keys and matrix-keypad want to set_wake for each
> > gpio individually, hw can do that, but pxa27x.c breaks it.
> >
> > I don't get it; what is pxa27x_keypad used on? It looks like
> > matrix-keypad subset.
> 
> pxa27x has its own specific keypad controller. And since we now
> use enable_irq_wake, and the keypad controller has only one
> such IRQ_KEYPAD, will have to setup the keypad GPIO wakeup
> as a whole.

But pxa27x keypad controlled is not used on spitz, right? So why do we
break spitz by tying GPIO wakeup settings together?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Eric Miao]

On Fri, Mar 5, 2010 at 5:23 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> > Sharp-SL code uses strange, complex grouping of gpios for wakeups
>> >> > toggling. Fortunately, it is unneeded in recent kernels (and actually
>> >> > provokes WARN_ONs during resume). Remove it.
>> >> >
>> >> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
>> >>
>> >> Pavel,
>> >>
>> >> The code to be removed below is used to support pxa27x_keypad
>> >> to be able to resume from sleep. What's the exact reason to remove
>> >> this on spitz?
>> >
>> > Well, otherwise I get this during resume: (2.6.32 regression)
>> >
>> > (and similar for matrix-keypad, but dmitry worked around that
>> > already).
>> >
>> > Problem is that gpio-keys and matrix-keypad want to set_wake for each
>> > gpio individually, hw can do that, but pxa27x.c breaks it.
>> >
>> > I don't get it; what is pxa27x_keypad used on? It looks like
>> > matrix-keypad subset.
>>
>> pxa27x has its own specific keypad controller. And since we now
>> use enable_irq_wake, and the keypad controller has only one
>> such IRQ_KEYPAD, will have to setup the keypad GPIO wakeup
>> as a whole.
>
> But pxa27x keypad controlled is not used on spitz, right? So why do we
> break spitz by tying GPIO wakeup settings together?

We are not typing, it's an option only for pxa27x keypad controller driver.
I'm going to modify corgi_pm.c as follows, please check if it's doable to
spitz as well:

commit 1864fa1398440577f7c84e7370a99f773d2d3089
Author: Eric Miao <eric.y.miao@gmail.com>
Date:   Mon Jan 11 21:27:21 2010 +0800

    [ARM] pxa/corgi: cleanup GPIO configurations and low power mode settings

    Signed-off-by: Eric Miao <eric.y.miao@gmail.com>

diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
index ae496b6..adb8ce0 100644
--- a/arch/arm/mach-pxa/corgi.c
+++ b/arch/arm/mach-pxa/corgi.c
@@ -106,18 +106,18 @@ static unsigned long corgi_pin_config[] __initdata = {
 	GPIO8_MMC_CS0,

 	/* GPIO Matrix Keypad */
-	GPIO66_GPIO,	/* column 0 */
-	GPIO67_GPIO,	/* column 1 */
-	GPIO68_GPIO,	/* column 2 */
-	GPIO69_GPIO,	/* column 3 */
-	GPIO70_GPIO,	/* column 4 */
-	GPIO71_GPIO,	/* column 5 */
-	GPIO72_GPIO,	/* column 6 */
-	GPIO73_GPIO,	/* column 7 */
-	GPIO74_GPIO,	/* column 8 */
-	GPIO75_GPIO,	/* column 9 */
-	GPIO76_GPIO,	/* column 10 */
-	GPIO77_GPIO,	/* column 11 */
+	GPIO66_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 0 */
+	GPIO67_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 1 */
+	GPIO68_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 2 */
+	GPIO69_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 3 */
+	GPIO70_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 4 */
+	GPIO71_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 5 */
+	GPIO72_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 6 */
+	GPIO73_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 7 */
+	GPIO74_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 8 */
+	GPIO75_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 9 */
+	GPIO76_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 10 */
+	GPIO77_GPIO | MFP_LPM_DRIVE_HIGH,	/* column 11 */
 	GPIO58_GPIO,	/* row 0 */
 	GPIO59_GPIO,	/* row 1 */
 	GPIO60_GPIO,	/* row 2 */
@@ -128,17 +128,20 @@ static unsigned long corgi_pin_config[] __initdata = {
 	GPIO65_GPIO,	/* row 7 */

 	/* GPIO */
-	GPIO9_GPIO,	/* CORGI_GPIO_nSD_DETECT */
-	GPIO7_GPIO,	/* CORGI_GPIO_nSD_WP */
-	GPIO21_GPIO,	/* CORGI_GPIO_ADC_TEMP */
-	GPIO22_GPIO,	/* CORGI_GPIO_IR_ON */
-	GPIO33_GPIO,	/* CORGI_GPIO_SD_PWR */
-	GPIO38_GPIO,	/* CORGI_GPIO_CHRG_ON */
-	GPIO43_GPIO,	/* CORGI_GPIO_CHRG_UKN */
-	GPIO44_GPIO,	/* CORGI_GPIO_HSYNC */
+	GPIO9_GPIO,				/* CORGI_GPIO_nSD_DETECT */
+	GPIO7_GPIO,				/* CORGI_GPIO_nSD_WP */
+	GPIO11_GPIO | WAKEUP_ON_EDGE_BOTH,	/* CORGI_GPIO_MAIN_BAT_{LOW,COVER} */
+	GPIO13_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_LED_ORANGE */
+	GPIO21_GPIO,				/* CORGI_GPIO_ADC_TEMP */
+	GPIO22_GPIO,				/* CORGI_GPIO_IR_ON */
+	GPIO33_GPIO,				/* CORGI_GPIO_SD_PWR */
+	GPIO38_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_ON */
+	GPIO43_GPIO | MFP_LPM_KEEP_OUTPUT,	/* CORGI_GPIO_CHRG_UKN */
+	GPIO44_GPIO,				/* CORGI_GPIO_HSYNC */

 	GPIO0_GPIO | WAKEUP_ON_EDGE_BOTH,	/* CORGI_GPIO_KEY_INT */
 	GPIO1_GPIO | WAKEUP_ON_EDGE_RISE,	/* CORGI_GPIO_AC_IN */
+	GPIO3_GPIO | WAKEUP_ON_EDGE_BOTH,	/* CORGI_GPIO_WAKEUP */
 };

 /*
@@ -675,6 +678,15 @@ static void __init corgi_init(void)

 	pxa2xx_mfp_config(ARRAY_AND_SIZE(corgi_pin_config));

+	/* allow wakeup from various GPIOs */
+	gpio_set_wake(CORGI_GPIO_KEY_INT, 1);
+	gpio_set_wake(CORGI_GPIO_WAKEUP, 1);
+	gpio_set_wake(CORGI_GPIO_AC_IN, 1);
+	gpio_set_wake(CORGI_GPIO_CHRG_FULL, 1);
+
+	if (!machine_is_corgi())
+		gpio_set_wake(CORGI_GPIO_MAIN_BAT_LOW, 1);
+
 	pxa_set_ffuart_info(NULL);
 	pxa_set_btuart_info(NULL);
 	pxa_set_stuart_info(NULL);
diff --git a/arch/arm/mach-pxa/corgi_pm.c b/arch/arm/mach-pxa/corgi_pm.c
index bb68347..3f1dc74 100644
--- a/arch/arm/mach-pxa/corgi_pm.c
+++ b/arch/arm/mach-pxa/corgi_pm.c
@@ -77,45 +77,6 @@ static void corgi_discharge(int on)

 static void corgi_presuspend(void)
 {
-	int i;
-	unsigned long wakeup_mask;
-
-	/* charging , so CHARGE_ON bit is HIGH during OFF. */
-	if (READ_GPIO_BIT(CORGI_GPIO_CHRG_ON))
-		PGSR1 |= GPIO_bit(CORGI_GPIO_CHRG_ON);
-	else
-		PGSR1 &= ~GPIO_bit(CORGI_GPIO_CHRG_ON);
-
-	if (READ_GPIO_BIT(CORGI_GPIO_LED_ORANGE))
-		PGSR0 |= GPIO_bit(CORGI_GPIO_LED_ORANGE);
-	else
-		PGSR0 &= ~GPIO_bit(CORGI_GPIO_LED_ORANGE);
-
-	if (READ_GPIO_BIT(CORGI_GPIO_CHRG_UKN))
-		PGSR1 |= GPIO_bit(CORGI_GPIO_CHRG_UKN);
-	else
-		PGSR1 &= ~GPIO_bit(CORGI_GPIO_CHRG_UKN);
-
-	/* Resume on keyboard power key */
-	PGSR2 = (PGSR2 & ~CORGI_GPIO_ALL_STROBE_BIT) | CORGI_GPIO_STROBE_BIT(0);
-
-	wakeup_mask = GPIO_bit(CORGI_GPIO_KEY_INT) |
GPIO_bit(CORGI_GPIO_WAKEUP) | GPIO_bit(CORGI_GPIO_AC_IN) |
GPIO_bit(CORGI_GPIO_CHRG_FULL);
-
-	if (!machine_is_corgi())
-		wakeup_mask |= GPIO_bit(CORGI_GPIO_MAIN_BAT_LOW);
-
-	PWER = wakeup_mask | PWER_RTC;
-	PRER = wakeup_mask;
-	PFER = wakeup_mask;
-
-	for (i = 0; i <=15; i++) {
-		if (PRER & PFER & GPIO_bit(i)) {
-			if (GPLR0 & GPIO_bit(i) )
-				PRER &= ~GPIO_bit(i);
-			else
-				PFER &= ~GPIO_bit(i);
-		}
-	}
 }

 static void corgi_postsuspend(void)

Re: sharp c-3000 aka spitz: fix warn_on introduced in 2.6.32-rc1

[Pavel Machek]

Hi!

Okay, maybe this is better fix for the regression?

Simplify irq wakeup code, and fix ugly warning during resume on spitz
as a result.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

--- ./drivers/input/keyboard/pxa27x_keypad.c	2010-04-15 07:50:34.000000000 +0200
+++ ./drivers/input/keyboard/pxa27x_keypad.c	2010-04-27 15:38:02.000000000 +0200
@@ -33,6 +33,8 @@
 
 #include <mach/hardware.h>
 #include <mach/pxa27x_keypad.h>
+#include <mach/mfp-pxa2xx.h>
+
 /*
  * Keypad Controller registers
  */
@@ -412,7 +416,7 @@
 	clk_disable(keypad->clk);
 
 	if (device_may_wakeup(&pdev->dev))
-		enable_irq_wake(keypad->irq);
+		keypad_set_wake(1);
 
 	return 0;
 }
@@ -424,7 +428,7 @@
 	struct input_dev *input_dev = keypad->input_dev;
 
 	if (device_may_wakeup(&pdev->dev))
-		disable_irq_wake(keypad->irq);
+		keypad_set_wake(0);
 
 	mutex_lock(&input_dev->mutex);
 
--- ./arch/arm/mach-pxa/mfp-pxa2xx.c	2009-09-10 00:13:59.000000000 +0200
+++ ./arch/arm/mach-pxa/mfp-pxa2xx.c	2010-04-27 15:22:42.000000000 +0200
@@ -34,7 +34,6 @@
 struct gpio_desc {
 	unsigned	valid		: 1;
 	unsigned	can_wakeup	: 1;
-	unsigned	keypad_gpio	: 1;
 	unsigned	dir_inverted	: 1;
 	unsigned int	mask; /* bit mask in PWER or PKWR */
 	unsigned int	mux_mask; /* bit mask of muxed gpio bits, 0 if no mux */
@@ -178,9 +177,6 @@
 	if (!d->valid)
 		return -EINVAL;
 
-	if (d->keypad_gpio)
-		return -EINVAL;
-
 	mux_taken = (PWER & d->mux_mask) & (~d->mask);
 	if (on && mux_taken)
 		return -EBUSY;
@@ -291,7 +288,6 @@
 	for (i = 0; i < ARRAY_SIZE(pxa27x_pkwr_gpio); i++) {
 		gpio = pxa27x_pkwr_gpio[i];
 		gpio_desc[gpio].can_wakeup = 1;
-		gpio_desc[gpio].keypad_gpio = 1;
 		gpio_desc[gpio].mask = 1 << i;
 	}
 
--- ./arch/arm/mach-pxa/pxa27x.c	2010-03-21 22:09:12.000000000 +0100
+++ ./arch/arm/mach-pxa/pxa27x.c	2010-04-27 07:43:30.000000000 +0200
@@ -342,9 +342,6 @@
 	if (gpio >= 0 && gpio < 128)
 		return gpio_set_wake(gpio, on);
 
-	if (irq == IRQ_KEYPAD)
-		return keypad_set_wake(on);
-
 	switch (irq) {
 	case IRQ_RTCAlrm:
 		mask = PWER_RTC;


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html