Input: omap-keypad: Fix keyboard debounce configuration

With PM enabled, I noticed that pressing a key on the droid4 keyboard will block deeper idle states for the SoC. Looks like we can fix this by managing the idle register to gether with the interrupt similar to what we already do for the GPIO controller.

Note that we now must also disable OMAP4_DEF_IRQENABLE_LONGKEY as it should not be used together with debounce according to the TRM.

Cc Axel Haslam <[email protected]>

Cc Illia Smyrnov <[email protected]>

Cc Marcel Partap <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Michael Scott <[email protected]>

Cc NeKit <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Sebastian Reichel <[email protected]>

Reported-by Pavel Machek <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/input/keyboard/omap4-keypad.c | 28 ++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

drivers/input/keyboard/omap4-keypad.c +3 -2

@@ -53,11 +53,12 @@
 /* OMAP4 bit definitions */
 #define OMAP4_DEF_IRQENABLE_EVENTEN	BIT(0)
 #define OMAP4_DEF_IRQENABLE_LONGKEY	BIT(1)
-#define OMAP4_DEF_WUP_EVENT_ENA		BIT(0)
-#define OMAP4_DEF_WUP_LONG_KEY_ENA	BIT(1)
 #define OMAP4_DEF_CTRL_NOSOFTMODE	BIT(1)
 #define OMAP4_DEF_CTRL_PTV_SHIFT	2
 
+#define OMAP4_KBD_IRQ_MASK		(OMAP4_DEF_IRQENABLE_LONGKEY | \
+					 OMAP4_DEF_IRQENABLE_EVENTEN)
+
 /* OMAP4 values */
 #define OMAP4_VAL_IRQDISABLE		0x0
 

On Sun, Dec 02, 2018 at 05:29:33PM -0800, Tony Lindgren wrote:

Quoted 2 lines

Note that we now must also disable OMAP4_DEF_IRQENABLE_LONGKEY as it should not be used together with debounce according to the TRM.

Quoted 5 lines

drivers/input/keyboard/omap4-keypad.c +0 -0

@@ -127,9 +128,11 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 	struct omap4_keypad *keypad_data = dev_id;
 

drivers/input/keyboard/omap4-keypad.c static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id) · line 133

Quoted 5 lines

drivers/input/keyboard/omap4-keypad.c +1 -1

 	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
-		/* Disable interrupts */
+		/* Disable interrupts and wake-up events */
 		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
 				 OMAP4_VAL_IRQDISABLE);

I wonder, do we need to turn off interrupts at keyboard controller level, or we should simply use IRQF_ONESHOT?

drivers/input/keyboard/omap4-keypad.c static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id) · line 134

Quoted 1 line

drivers/input/keyboard/omap4-keypad.c +1 -0

+		kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE, 0);

So we are saying that disabling wakeup for the time between hard interrupt firing, and when interrupt thread runs, makes much difference? It is surprising to me... How long does it take to schedule interrupt thread?

From tony Mon Sep 17 00:00:00 2001

With PM enabled, I noticed that pressing a key on the droid4 keyboard will block deeper idle states for the SoC. Looks like we can fix this by managing the idle register to gether with the interrupt similar to what we already do for the GPIO controller.

And there's no need to keep enabling and disabling interrupts and wake-up events for normal use if we use IRQF_ONESHOT as suggested by Dmitry Torokhov <[email protected]> so let's do that too.

Cc Axel Haslam <[email protected]>

Cc Illia Smyrnov <[email protected]>

Cc Marcel Partap <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Michael Scott <[email protected]>

Cc NeKit <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Sebastian Reichel <[email protected]>

Reported-by Pavel Machek <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/input/keyboard/omap4-keypad.c | 30 ++++++++++-----------------
 1 file changed, 11 insertions(+), 19 deletions(-)

drivers/input/keyboard/omap4-keypad.c +11 -19

@@ -53,11 +53,12 @@
 /* OMAP4 bit definitions */
 #define OMAP4_DEF_IRQENABLE_EVENTEN	BIT(0)
 #define OMAP4_DEF_IRQENABLE_LONGKEY	BIT(1)
-#define OMAP4_DEF_WUP_EVENT_ENA		BIT(0)
-#define OMAP4_DEF_WUP_LONG_KEY_ENA	BIT(1)
 #define OMAP4_DEF_CTRL_NOSOFTMODE	BIT(1)
 #define OMAP4_DEF_CTRL_PTV_SHIFT	2
 
+#define OMAP4_KBD_IRQ_MASK		(OMAP4_DEF_IRQENABLE_LONGKEY | \
+					 OMAP4_DEF_IRQENABLE_EVENTEN)
+
 /* OMAP4 values */
 #define OMAP4_VAL_IRQDISABLE		0x0
 

@@ -126,12 +127,8 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
 
-	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
-		/* Disable interrupts */
-		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-				 OMAP4_VAL_IRQDISABLE);
+	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS))
 		return IRQ_WAKE_THREAD;
-	}
 
 	return IRQ_NONE;
 }

@@ -173,11 +170,6 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 
-	/* enable interrupts */
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-		OMAP4_DEF_IRQENABLE_EVENTEN |
-				OMAP4_DEF_IRQENABLE_LONGKEY);
-
 	return IRQ_HANDLED;
 }
 

@@ -197,11 +189,10 @@ static int omap4_keypad_open(struct input_dev *input)
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-			OMAP4_DEF_IRQENABLE_EVENTEN |
-				OMAP4_DEF_IRQENABLE_LONGKEY);
-	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE,
-			OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA);
+
+	/* enable interrupts and wake-up events */
+	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE, OMAP4_KBD_IRQ_MASK);
+	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE, OMAP4_KBD_IRQ_MASK);
 
 	enable_irq(keypad_data->irq);
 

@@ -214,9 +205,10 @@ static void omap4_keypad_close(struct input_dev *input)
 
 	disable_irq(keypad_data->irq);
 
-	/* Disable interrupts */
+	/* Disable interrupts and wake-up events */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
 			 OMAP4_VAL_IRQDISABLE);
+	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE, 0);
 
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,

@@ -365,7 +357,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	}
 
 	error = request_threaded_irq(keypad_data->irq, omap4_keypad_irq_handler,
-				     omap4_keypad_irq_thread_fn, 0,
+				     omap4_keypad_irq_thread_fn, IRQF_ONESHOT,
 				     "omap4-keypad", keypad_data);
 	if (error) {
 		dev_err(&pdev->dev, "failed to register interrupt\n");

Signature · 1 line

2.19.2

Hi Tony,

Quoted 5 lines · Tony Lindgren

With PM enabled, I noticed that pressing a key on the droid4 keyboard will block deeper idle states for the SoC. Looks like we can fix this by managing the idle register to gether with the interrupt similar to what we already do for the GPIO controller.

Can you show me where exactly we are doing this? I can't seem to find the matching code.

From tony Mon Sep 17 00:00:00 2001

From the hardware point of view, looks like we need to manage the registers

Hi,

Quoted 5 lines · Dmitry Torokhov

Hi Tony,

Quoted 2 lines · Tony Lindgren

With PM enabled, I noticed that pressing a key on the droid4

keyboard will

Quoted 2 lines

Quoted 2 lines

block deeper idle states for the SoC. Looks like we can fix this by managing the idle register to gether with the interrupt similar to

what

Quoted 2 lines

Quoted 1 line

we already do for the GPIO controller.

Quoted 2 lines

Can you show me where exactly we are doing this? I can't seem to find the matching code.

With your change it now becomes the fix, and we're just missing the clearing of the OMAP4_KBD_WAKEUPENABLE register in omap4_keypad_close.

Does the following minimal version with updated comments make more sense now?

Regards,

Tony

8< --------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <[email protected]>
Date: Tue, 4 Dec 2018 11:07:56 -0800
Subject: [PATCH] Input: omap-keypad: Fix idle configration to not block SoC idle states

With PM enabled, I noticed that pressing a key on the droid4 keyboard will
block deeper idle states for the SoC. Let's fix this by using IRQF_ONESHOT
and stop constantly toggling the device OMAP4_KBD_IRQENABLE register as suggested by Dmitry Torokhov <[email protected]>.

From the hardware point of view, looks like we need to manage the registers
for OMAP4_KBD_IRQENABLE and OMAP4_KBD_WAKEUPENABLE together to avoid blocking deeper SoC idle states. And with toggling of OMAP4_KBD_IRQENABLE
register now gone with IRQF_ONESHOT, also the SoC idle state problem is gone during runtime. We still also need to clear OMAP4_KBD_WAKEUPENABLE in
omap4_keypad_close() though to pair it with omap4_keypad_open() to prevent
blocking deeper SoC idle states after rmmod omap4-keypad.

Cc Axel Haslam <[email protected]>

Cc Illia Smyrnov <[email protected]>

Cc Marcel Partap <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Michael Scott <[email protected]>

Cc NeKit <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Sebastian Reichel <[email protected]>

Reported-by Pavel Machek <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
drivers/input/keyboard/omap4-keypad.c | 16 ++++------------

1 file changed, 4 insertions(+), 12 deletions(-)

drivers/input/keyboard/omap4-keypad.c +1 -5

@@ -126,12 +126,8 @@ static irqreturn_t omap4_keypad_irq_handler(int
 irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
 
-	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
-		/* Disable interrupts */
-		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-				 OMAP4_VAL_IRQDISABLE);
+	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS))
 		return IRQ_WAKE_THREAD;
-	}
 
 	return IRQ_NONE;

}

drivers/input/keyboard/omap4-keypad.c +2 -6

@@ -173,11 +169,6 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int
 irq, void *dev_id)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 
-	/* enable interrupts */
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-		OMAP4_DEF_IRQENABLE_EVENTEN |
-				OMAP4_DEF_IRQENABLE_LONGKEY);
-
 	return IRQ_HANDLED;
 }

@@ -214,9 +205,10 @@ static void omap4_keypad_close(struct input_dev
 *input)
 
 	disable_irq(keypad_data->irq);
 
-	/* Disable interrupts */
+	/* Disable interrupts and wake-up events */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
 			 OMAP4_VAL_IRQDISABLE);
+	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE, 0);
 
 	/* clear pending interrupts */

kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,

drivers/input/keyboard/omap4-keypad.c +1 -1

@@ -365,7 +357,7 @@ static int omap4_keypad_probe(struct
 platform_device *pdev)
 	}
 
 	error = request_threaded_irq(keypad_data->irq,
 omap4_keypad_irq_handler,
-				     omap4_keypad_irq_thread_fn, 0,
+				     omap4_keypad_irq_thread_fn, IRQF_ONESHOT,
 				     "omap4-keypad", keypad_data);

	if (error) {
		dev_err(&pdev->dev, "failed to register interrupt\n");

I noticed that the Android v3.0.8 kernel on droid4 is using different keypad values from the mainline kernel and does not have issues with keys occasionally being stuck until pressed again. Turns out there was an earlier patch posted to fix this as "Input: omap-keypad: errata i689:
Correct debounce time", but it was never reposted to fix use macros for timing calculations.

This updated version is using macros, and also fixes the use of the input clock rate to use 32768KiHz instead of 32000KiHz. And we want to use the known good Android kernel values of 3 and 6 instead of 2 and 6 in the earlier patch.

Cc Axel Haslam <[email protected]>

Cc Felipe Balbi <[email protected]>

Cc Illia Smyrnov <[email protected]>

Cc Marcel Partap <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Michael Scott <[email protected]>

Cc NeKit <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Sebastian Reichel <[email protected]>

Reported-by Pavel Machek <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/input/keyboard/omap4-keypad.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

drivers/input/keyboard/omap4-keypad.c +14 -4

@@ -60,8 +60,18 @@
 
 /* OMAP4 values */
 #define OMAP4_VAL_IRQDISABLE		0x0
-#define OMAP4_VAL_DEBOUNCINGTIME	0x7
-#define OMAP4_VAL_PVT			0x7
+
+/*
+ * Errata i689: If a key is released for a time shorter than debounce time,
+ * the keyboard will idle and never detect the key release. The workaround
+ * is to use at least a 12ms debounce time. See omap5432 TRM chapter
+ * "26.4.6.2 Keyboard Controller Timer" for more information.
+ */
+#define OMAP4_KEYPAD_PTV_DIV_128        0x6
+#define OMAP4_KEYPAD_DEBOUNCINGTIME_MS(dbms, ptv)     \
+	((((dbms) * 1000) / ((1 << ((ptv) + 1)) * (1000000 / 32768))) - 1)
+#define OMAP4_VAL_DEBOUNCINGTIME_16MS					\
+	OMAP4_KEYPAD_DEBOUNCINGTIME_MS(16, OMAP4_KEYPAD_PTV_DIV_128)
 
 enum {
 	KBD_REVISION_OMAP4 = 0,

@@ -181,9 +191,9 @@ static int omap4_keypad_open(struct input_dev *input)
 
 	kbd_writel(keypad_data, OMAP4_KBD_CTRL,
 			OMAP4_DEF_CTRL_NOSOFTMODE |
-			(OMAP4_VAL_PVT << OMAP4_DEF_CTRL_PTV_SHIFT));
+			(OMAP4_KEYPAD_PTV_DIV_128 << OMAP4_DEF_CTRL_PTV_SHIFT));
 	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
-			OMAP4_VAL_DEBOUNCINGTIME);
+			OMAP4_VAL_DEBOUNCINGTIME_16MS);
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));

Signature · 1 line

2.19.2

I noticed that the Android v3.0.8 kernel on droid4 is using different keypad values from the mainline kernel and does not have issues with keys occasionally being stuck until pressed again. Turns out there was an earlier patch posted to fix this as "Input: omap-keypad: errata i689:
Correct debounce time", but it was never reposted to fix use macros for timing calculations.

This updated version is using macros, and also fixes the use of the input clock rate to use 32768KiHz instead of 32000KiHz. And we want to use the known good Android kernel values of 3 and 6 instead of 2 and 6 in the earlier patch.

Cc Axel Haslam <[email protected]>

Cc Felipe Balbi <[email protected]>

Cc Illia Smyrnov <[email protected]>

Cc Marcel Partap <[email protected]>

Cc Merlijn Wajer <[email protected]>

Cc Michael Scott <[email protected]>

Cc NeKit <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Sebastian Reichel <[email protected]>

Reported-by Pavel Machek <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

So, if I press key "." quickly on console in 4.18.0-rc4-88970-gf075a2c-dirty it starts repeating. I could reproduce it 3 times from tree tries. Then I decided to test your patch, and so I booted 4.20-rc5+ using kexec. Turns out, I can't reproduce that repeat there... I tried more than 10 times. But that's before your patch.

So -- bad news for you -- seems like someone -- maybe you -- already fixed this. (But that does not mean the patch is bad idea).