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 <[email protected]>
--- a/arch/arm/mach-pxa/mfp-pxa2xx.c 2009-09-10 00:13:59.000000000 +0200
+++ b/arch/arm/mach-pxa/mfp-pxa2xx.c 2009-12-29 03:16:40.000000000 +0100
@@ -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;
@@ -231,32 +227,6 @@
#endif /* CONFIG_PXA25x */
#ifdef CONFIG_PXA27x
-static int pxa27x_pkwr_gpio[] = {
- 13, 16, 17, 34, 36, 37, 38, 39, 90, 91, 93, 94,
- 95, 96, 97, 98, 99, 100, 101, 102
-};
-
-int keypad_set_wake(unsigned int on)
-{
- unsigned int i, gpio, mask = 0;
-
- if (!on) {
- PKWR = 0;
- return 0;
- }
-
- for (i = 0; i < ARRAY_SIZE(pxa27x_pkwr_gpio); i++) {
-
- gpio = pxa27x_pkwr_gpio[i];
-
- if (gpio_desc[gpio].config & MFP_LPM_CAN_WAKEUP)
- mask |= gpio_desc[gpio].mask;
- }
-
- PKWR = mask;
- return 0;
-}
-
#define PWER_WEMUX2_GPIO38 (1 << 16)
#define PWER_WEMUX2_GPIO53 (2 << 16)
#define PWER_WEMUX2_GPIO40 (3 << 16)
@@ -273,6 +243,12 @@
gpio_desc[(gpio)].mux_mask = PWER_ ## mux ## _MASK; \
} while (0)
+
+static int pxa27x_pkwr_gpio[] = {
+ 13, 16, 17, 34, 36, 37, 38, 39, 90, 91, 93, 94,
+ 95, 96, 97, 98, 99, 100, 101, 102
+};
+
static void __init pxa27x_mfp_init(void)
{
int i, gpio;
@@ -291,7 +267,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;
}
--- a/arch/arm/mach-pxa/pxa27x.c 2009-09-10 00:13:59.000000000 +0200
+++ b/arch/arm/mach-pxa/pxa27x.c 2009-12-29 03:13:00.000000000 +0100
@@ -323,9 +323,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
On Wed, Jan 6, 2010 at 3:10 PM, Pavel Machek <[email protected]> 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 <[email protected]>
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?
On Wed 2010-01-06 15:17:39, Eric Miao wrote:
> On Wed, Jan 6, 2010 at 3:10 PM, Pavel Machek <[email protected]> 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 <[email protected]>
>
> 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
On Thu, Jan 7, 2010 at 2:52 PM, Pavel Machek <[email protected]> wrote:
> On Wed 2010-01-06 15:17:39, Eric Miao wrote:
>> On Wed, Jan 6, 2010 at 3:10 PM, Pavel Machek <[email protected]> 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 <[email protected]>
>>
>> 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.
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
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
> 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.
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
2010/1/26 Stanislav Brabec <[email protected]>:
> 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.
Eric Miao wrote:
> 2010/1/26 Stanislav Brabec <[email protected]>:
> > 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
Hello,
2010/1/26 Stanislav Brabec <[email protected]>:
> 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
2010/1/26 Stanislav Brabec <[email protected]>:
> Eric Miao wrote:
>> 2010/1/26 Stanislav Brabec <[email protected]>:
>
>> > 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
>
>
Eric Miao wrote:
> 2010/1/26 Stanislav Brabec <[email protected]>:
> > Eric Miao wrote:
> >> 2010/1/26 Stanislav Brabec <[email protected]>:
> >
> >> > 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: [email protected]
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/
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 <[email protected]>
--- ./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