serial: msm_serial: add missing sysrq handling

Stephen,

Can you test this patch on v 1.3 hardware? It works on my v 1.4.

If you use kdmx2, the way to send a break is '~B'. The previous key pressed must be <enter> for the '~' escape to be recognized.

Thanks!

-Frank

From: Frank Rowand <[email protected]>

Add missing sysrq handling to msm_serial.

Signed-off-by Frank Rowand <[email protected]>

---

 drivers/tty/serial/msm_serial.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c +2 -0

@@ -126,6 +126,8 @@ static void handle_rx_dm(struct uart_por
 
 	while (count > 0) {
 		unsigned int c;
+		unsigned char *cp;
+		int res;
 
 		sr = msm_read(port, UART_SR);
 		if ((sr & UART_SR_RX_READY) == 0) {

Quoted 13 lines · Frank Rowand

Stephen,

Can you test this patch on v 1.3 hardware? It works on my v 1.4.

If you use kdmx2, the way to send a break is '~B'. The previous key pressed must be <enter> for the '~' escape to be recognized.

Thanks!

-Frank

Quoted 7 lines

From: Frank Rowand <[email protected]>

Add missing sysrq handling to msm_serial.

Signed-off-by Frank Rowand <[email protected]>

---

It works but I have questions.

Quoted 17 lines

 drivers/tty/serial/msm_serial.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Index: b/drivers/tty/serial/msm_serial.c
===================================================================

drivers/tty/serial/msm_serial.c +2 -0

@@ -126,6 +126,8 @@ static void handle_rx_dm(struct uart_por
 
 	while (count > 0) {
 		unsigned int c;
+		unsigned char *cp;
+		int res;
 
 		sr = msm_read(port, UART_SR);
 		if ((sr & UART_SR_RX_READY) == 0) {

drivers/tty/serial/msm_serial.c static void handle_rx_dm(struct uart_por · line 143

Quoted 8 lines

drivers/tty/serial/msm_serial.c +3 -1

@@ -135,15 +137,29 @@ static void handle_rx_dm(struct uart_por
 		c = msm_read(port, UARTDM_RF);
 		if (sr & UART_SR_RX_BREAK) {
 			port->icount.brk++;
-			if (uart_handle_break(port))
+			if (uart_handle_break(port)) {
+				count -= 1;
 				continue;
+			}

This looks wrong. If it's a break then I think the fifo takes in a break character indicated by all zeros. We could possibly have 3 other characters after it in the fifo, or maybe 2 characters in front of it, or it could be 30 characters in. We can change this behavior by setting a bit in the MR2 register so that the all zero character doesn't enter the fifo. The same goes for the parity and frame error conditions, we can drop those characters too.

I asked the designers how we're supposed to deal with a break in the middle of the fifo and they recommended using the start/stop rx break interrupts. Unfortunately, I don't think we can rely on the interrupts being distinct so that might not work (but see attached patch). There's also a break interrupt that triggers on the start and stop rx break events. I don't know how this is useful either because we might get two interrupts or we might get one.

So perhaps we need to scan through the 4 characters for a zero character when the SR_RX_BREAK bit it set? The second diff does that.

Add another twist with port->read_status_mask. I guess that's there to make it so that break characters still enter the tty layer? Is there any documentation on this stuff? I'm lost on what the tty layer expects.

Quoted 1 line · Stephen Boyd

On 08/06/14 17:16, Frank Rowand wrote:

The patches you sent are a little hard to read since they modify further code that my patch modified. So I have redone your patches, as if my patch was not previously applied. Hopefully I did not make any mistakes there. I will reply to this email with each of your redone patches.

Quoted 1 line · Stephen Boyd

On 08/06/14 17:16, Frank Rowand wrote:

The patches you sent are a little hard to read since they modify further code that my patch modified. So I have redone your patches, as if my patch was not previously applied. Hopefully I did not make any mistakes there. I will reply to this email with each of your redone patches.

Quoted 2 lines · Frank Rowand

Quoted 1 line · Stephen Boyd

On 08/06/14 17:16, Frank Rowand wrote:

< snip >

Quoted 4 lines

The patches you sent are a little hard to read since they modify further code that my patch modified. So I have redone your patches, as if my patch was not previously applied. Hopefully I did not make any mistakes there. I will reply to this email with each of your redone patches.

Stephen's patch alternative number 2:

Hi Frank,

Quoted 10 lines · Frank Rowand

Quoted 2 lines · Frank Rowand

Quoted 1 line · Stephen Boyd

On 08/06/14 17:16, Frank Rowand wrote:

< snip >

Quoted 4 lines

The patches you sent are a little hard to read since they modify further code that my patch modified. So I have redone your patches, as if my patch was not previously applied. Hopefully I did not make any mistakes there. I will reply to this email with each of your redone patches.

Stephen's patch alternative number 2:

I had a discussion with the hardware engineer. Apparently the break bit in the SR register is not "sticky" so it doesn't always stay set when the handle_rx_dm() function runs and a break has entered the fifo. I used your debug patches to confirm this (I never see the break status bit when the fifo has multiple characters in it). It sounds like this bit can't really be used reliably. The recommendation is to use either the break start or break stop interrupt to detect when a break has occurred and then search the fifo for the break character (an all zero character). If there are two such characters then we can't be certain when the break was, but the chances of this seem really slim considering that the stale timeout probably triggers first before a human can type a character after the break.

On 1.4 hardware we can change the mode to be single character and then we can reliably detect the break character because only one character enters the fifo and the higher bits of the fifo can be used to detect if it is a break or not. Making that change is probably not that hard, I believe we can reuse all the handle_rx_dm logic and force single character mode on consoles, but 1.3 hardware doesn't benefit from this change. I'll try to get something together soon that tries to make this all work as best as it can.