serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
Subsystems (2)
cover serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
Hi Paul and serial forks,
Here are sh-sci serial driver fixes that have been cooking for months privately on my end:
* console PM rework with the help of serial-core's uart_change_pm()
* provide missing piece in SCIFB support (from Yoshii-san)
* address console write function vs. SMP lockup issue
and so on. Comments and suggestions are welcome.
Build tested with vanilla v3.6, but not boot- nor PM-tested with newer kernels after v3.4. Anyone with can-sleep shmobile systems, please give it try.
Thanks in advance,
----------------------------------------------------------------
Shinya Kuribayashi (5):
Revert "sh-sci / PM: Avoid deadlocking runtime PM"
Revert "sh-sci / PM: Use power.irq_safe"
Partially revert "serial: sh-sci: console Runtime PM support"
serial: sh-sci: add locking to console write function to avoid SMP lockup
serial: sh-sci: fix possible race cases on SCSCR register accesses
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definition
Teppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)
drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)
Signature · 2 lines
Shinya Kuribayashi Renesas Electronics
PATCH 1/10 Revert "sh-sci / PM: Avoid deadlocking runtime PM"
This reverts commit 048be431e4 (sh-sci / PM: Avoid deadlocking runtime PM, from Rafael J. Wysocki <[email protected]>, 2012-03-09).
In order to get console PM work properly, we should implement uart_ops ->pm() operation, rather than sprinkle band-ading runtime PM calls in the driver.
--- drivers/tty/serial/sh-sci.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/tty/serial/sh-sci.c +0 -5
@@ -1752,8 +1752,6 @@ static int sci_startup(struct uart_port *port)dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); -pm_runtime_put_noidle (port->dev); -sci_port_enable (s); ret =sci_request_irq (s);
@@ -1781,8 +1779,6 @@ static void sci_shutdown(struct uart_port *port)sci_free_irq (s);sci_port_disable (s); - -pm_runtime_get_noresume (port->dev); } static unsigned int sci_scbrr_calc(unsigned int algo_id, unsigned int bps,
@@ -2121,7 +2117,6 @@ static int __devinit sci_init_single(struct platform_device *dev,sci_init_gpios (sci_port);pm_runtime_irq_safe (&dev->dev); -pm_runtime_get_noresume (&dev->dev);pm_runtime_enable (&dev->dev); }
Signature · 1 line
1.7.12.4
PATCH 2/10 Revert "sh-sci / PM: Use power.irq_safe"
In reply to Shinya KuribayashiThis reverts commit 5a50a01bf0 (sh-sci / PM: Use power.irq_safe, from Rafael J. Wysocki <[email protected]>, 2011-08-24).
In order to get console PM work properly, we should implement uart_ops ->pm() operation, rather than sprinkle band-ading runtime PM calls in the driver.
--- drivers/tty/serial/sh-sci.c | 1 - 1 file changed, 1 deletion(-)
drivers/tty/serial/sh-sci.c +0 -1
@@ -2116,7 +2116,6 @@ static int __devinit sci_init_single(struct platform_device *dev,sci_init_gpios (sci_port); -pm_runtime_irq_safe (&dev->dev);pm_runtime_enable (&dev->dev); }
Signature · 1 line
1.7.12.4
PATCH 3/10 Partially revert "serial: sh-sci: console Runtime PM support"
In reply to Shinya KuribayashiThis partially reverts commit 1ba7622094 (serial: sh-sci: console Runtime PM support, from Magnus Damm <[email protected]>, 2011-08-03).
The generic 'serial_core' can take care of console PM maintenance, so all (or at least the first thing) we have to do to get console PM work properly, is to implement uart_ops ->pm() operation in the sh-sci serial client driver.
This patch partially reverts the commit above, but leaving sci_reset() change in place, because sci_reset() is already part of another commit (73c3d53f38 serial: sh-sci: Avoid FIFO clear for MCE toggle.).
A revised version of console PM support follows next.
--- drivers/tty/serial/sh-sci.c | 53 +-------------------------------------------- 1 file changed, 1 insertion(+), 52 deletions(-)
drivers/tty/serial/sh-sci.c +1 -52
@@ -99,12 +99,6 @@ struct sci_port {#endif struct notifier_block freq_transition; - -#ifdef CONFIG_SERIAL_SH_SCI_CONSOLE -unsigned short saved_smr; -unsigned short saved_fcr; -unsigned char saved_brr; -#endif };/* Function prototypes */
@@ -2247,8 +2241,7 @@ static int __devinit serial_console_setup(struct console *co, char *options)if (options)uart_parse_options (options, &baud, &parity, &bits, &flow); -sci_port_disable (sci_port); - +/* TODO: disable clock */ return uart_set_options (port, co, baud, parity, bits, flow); }
@@ -2291,46 +2284,6 @@ static int __devinit sci_probe_earlyprintk(struct platform_device *pdev) return 0; } -#define uart_console (port) ((port)->cons->index == (port)->line) - -static int sci_runtime_suspend (struct device *dev) -{ -struct sci_port *sci_port =dev_get_drvdata (dev); -struct uart_port *port = &sci_port->port; - -if (uart_console (port)) { -struct plat_sci_reg *reg; - - sci_port->saved_smr =serial_port_in (port,SCSMR ); - sci_port->saved_brr =serial_port_in (port,SCBRR ); - - reg =sci_getreg (port,SCFCR ); -if (reg->size) - sci_port->saved_fcr =serial_port_in (port,SCFCR ); -else - sci_port->saved_fcr =0 ; - } -return 0 ; -} - -static int sci_runtime_resume (struct device *dev) -{ -struct sci_port *sci_port =dev_get_drvdata (dev); -struct uart_port *port = &sci_port->port; - -if (uart_console (port)) { -sci_reset (port); -serial_port_out (port,SCSMR , sci_port->saved_smr); -serial_port_out (port,SCBRR , sci_port->saved_brr); - -if (sci_port->saved_fcr) -serial_port_out (port,SCFCR , sci_port->saved_fcr); - -serial_port_out (port,SCSCR , sci_port->cfg->scscr); - } -return 0 ; -} -#define SCI_CONSOLE (&serial_console)#else
@@ -2340,8 +2293,6 @@ static inline int __devinit sci_probe_earlyprintk(struct platform_device *pdev) }#define SCI_CONSOLE NULL -#define sci_runtime_suspend NULL -#define sci_runtime_resume NULL#endif /* CONFIG_SERIAL_SH_SCI_CONSOLE */
@@ -2459,8 +2410,6 @@ static int sci_resume(struct device *dev) }static const struct dev_pm_ops sci_dev_pm_ops = { - .runtime_suspend = sci_runtime_suspend, - .runtime_resume = sci_runtime_resume, .suspend = sci_suspend, .resume = sci_resume, };
Signature · 1 line
1.7.12.4
PATCH 4/10 serial: sh-sci: console runtime PM support (revisit)
In reply to Shinya KuribayashiThe commit 1ba7622094 (serial: sh-sci: console Runtime PM support, from Magnus Damm <[email protected]>, 2011-08-03), tried to support console runtime PM, but unfortunately it didn't work for us for some reason. We did not investigated further at that time, instead would like to propose a different approach.
In Linux tty/serial world, to get console PM work properly, a serial client driver does not have to maintain .runtime_suspend()/..resume() calls itself, but can leave console power power management handling to the serial core driver.
This patch moves the sh-sci driver in that direction.
Notes:
* There is room to optimize console runtime PM more aggressively by maintaining additional local runtime PM calls, but as a first step having .pm() operation would suffice.
* We still have a couple of direct calls to sci_port_enable/..disable left in the driver. We have to live with them, because they're out of serial core's help.
--- drivers/tty/serial/sh-sci.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
drivers/tty/serial/sh-sci.c +16 -11
@@ -1746,8 +1746,6 @@ static int sci_startup(struct uart_port *port)dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); -sci_port_enable (s); - ret =sci_request_irq (s);if (unlikely (ret <0 ))return ret;
@@ -1771,8 +1769,6 @@ static void sci_shutdown(struct uart_port *port)sci_free_dma (port);sci_free_irq (s); - -sci_port_disable (s); } static unsigned int sci_scbrr_calc(unsigned int algo_id, unsigned int bps,
@@ -1921,6 +1917,21 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,sci_port_disable (s); } +static void sci_pm (struct uart_port *port,unsigned int state, +unsigned int oldstate) +{ +struct sci_port *sci_port =to_sci_port (port); + +switch (state) { +case 3 : +sci_port_disable (sci_port); +break ; +default : +sci_port_enable (sci_port); +break ; + } +} +static const char *sci_type (struct uart_port *port) {switch (port->type) {
@@ -2042,6 +2053,7 @@ static struct uart_ops sci_uart_ops = { .startup = sci_startup, .shutdown = sci_shutdown, .set_termios = sci_set_termios, + .pm = sci_pm, .type = sci_type, .release_port = sci_release_port, .request_port = sci_request_port,
@@ -2195,16 +2207,12 @@ static void serial_console_write(struct console *co, const char *s,struct uart_port *port = &sci_port->port;unsigned short bits; -sci_port_enable (sci_port); -uart_console_write (port, s, count, serial_console_putchar);/* wait until fifo is empty and last bit has been transmitted */ bits =SCxSR_TDxE (port) |SCxSR_TEND (port);while ((serial_port_in (port, SCxSR) & bits) != bits)cpu_relax (); - -sci_port_disable (sci_port); }static int __devinit serial_console_setup (struct console *co,char *options)
@@ -2236,12 +2244,9 @@ static int __devinit serial_console_setup(struct console *co, char *options)if (unlikely (ret !=0 ))return ret; -sci_port_enable (sci_port); -if (options)uart_parse_options (options, &baud, &parity, &bits, &flow); -/* TODO: disable clock */ return uart_set_options (port, co, baud, parity, bits, flow); }
Signature · 1 line
1.7.12.4
PATCH 5/10 serial: sh-sci: fix condition test to set SCBRR
In reply to Shinya KuribayashiSCBRR == 0 is valid value (divide by 1).
--- drivers/tty/serial/sh-sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/sh-sci.c +1 -1
@@ -1853,7 +1853,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,dev_dbg (port->dev,"%s: SMR %x, t %x, SCSCR %x\n" , __func__, smr_val, t, s->cfg->scscr); -if (t >0 ) { +if (t >=0 ) {if (t >=256 ) {serial_port_out (port,SCSMR , (serial_port_in (port,SCSMR ) & ~3 ) |1 ); t >>=2 ;
Signature · 1 line
1.7.12.4
PATCH 6/10 serial: sh-sci: support lower baud rate
In reply to Shinya KuribayashiSupport prescaler 1/16 and 1/64, in addition to current 1 and 1/4.
Supporting below 2400bps was dropped long time ago in mainline. Since then, setting lower rate has been resulting in erroneous register value, without indicating any errors through API.
This patch adds more prescaler to support lower rates again. This still doesn't check range, but we won't hit the case because even 50bps at 48MHz clock is now supported.
--- drivers/tty/serial/sh-sci.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/tty/serial/sh-sci.c +8 -11
@@ -1814,7 +1814,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, {struct sci_port *s =to_sci_port (port);struct plat_sci_reg *reg; -unsigned int baud, smr_val, max_baud; +unsigned int baud, smr_val, max_baud, cks;int t =-1 ;/*
@@ -1848,21 +1848,18 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,uart_update_timeout (port, termios->c_cflag, baud); -serial_port_out (port,SCSMR , smr_val); +for (cks =0 ; t >=256 && cks <=3 ; cks++) + t >>=2 ; -dev_dbg (port->dev,"%s: SMR %x, t %x, SCSCR %x\n" , __func__, smr_val, t, - s->cfg->scscr); +dev_dbg (port->dev,"%s: SMR %x, cks %x, t %x, SCSCR %x\n" , + __func__, smr_val, cks, t, s->cfg->scscr);if (t >=0 ) { -if (t >=256 ) { -serial_port_out (port,SCSMR , (serial_port_in (port,SCSMR ) & ~3 ) |1 ); - t >>=2 ; - }else -serial_port_out (port,SCSMR ,serial_port_in (port,SCSMR ) & ~3 ); - +serial_port_out (port,SCSMR , (smr_val & ~3 ) | cks);serial_port_out (port,SCBRR , t);udelay ((1000000 +(baud-1 )) / baud);/* Wait one bit interval */ - } + }else +serial_port_out (port,SCSMR , smr_val);sci_init_pins (port, termios->c_cflag);
Signature · 1 line
1.7.12.4
PATCH 7/10 serial: sh-sci: mask SCTFDR/RFDR according to fifosize
In reply to Shinya KuribayashiCurrent mask 0xff to SCTFDR/RFDR damages SCIFB, because the registers on SCIFB have 9-bit data (0 to 256).
This patch changes the mask according to port->fifosize. Though I'm not sure if the mask is really needed (I don't know if there are variants which have non-zero upper bits), it is safer.
--- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/sh-sci.c +2 -2
@@ -485,7 +485,7 @@ static int sci_txfill(struct uart_port *port) reg =sci_getreg (port,SCTFDR );if (reg->size) -return serial_port_in (port,SCTFDR ) &0xff ; +return serial_port_in (port,SCTFDR ) & ((port->fifosize <<1 ) -1 ); reg =sci_getreg (port,SCFDR );if (reg->size)
@@ -505,7 +505,7 @@ static int sci_rxfill(struct uart_port *port) reg =sci_getreg (port,SCRFDR );if (reg->size) -return serial_port_in (port,SCRFDR ) &0xff ; +return serial_port_in (port,SCRFDR ) & ((port->fifosize <<1 ) -1 ); reg =sci_getreg (port,SCFDR );if (reg->size)
Signature · 1 line
1.7.12.4
PATCH 8/10 serial: sh-sci: fix common SCIFB regmap definition
In reply to Shinya KuribayashiAbout FIFO count, there are two variants of SCIFs which show
a) TX count in upper, RX count in lower byte of FDR register
b) TX count in TFDR register, RX count in RFDR register
Common SCIFB regmap in current source code is defined as "a". At least 7372 and 73a0 HW manual say their SICFB are "b".
This patch alters the definition to "b", considering the current one has come from a mistake. The reason is as follows.
The flag SCIFB sh-sci driver means it has 256 byte FIFO. The count is from 0(empty) to 256(full), that makes 9-bit. Because FDR is 16-bit register, it can not hold two 9-bits. That's why, SCIFB can not be "a".
--- drivers/tty/serial/sh-sci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/tty/serial/sh-sci.c +3 -3
@@ -196,9 +196,9 @@ static struct plat_sci_reg sci_regmap[SCIx_NR_REGTYPES][SCIx_NR_REGS] = { [SCxSR] = { 0x14, 16 }, [SCxRDR] = { 0x60, 8 }, [SCFCR] = { 0x18, 16 }, - [SCFDR] = { 0x1c, 16 }, - [SCTFDR] = sci_reg_invalid, - [SCRFDR] = sci_reg_invalid, + [SCFDR] = sci_reg_invalid, + [SCTFDR] = { 0x38, 16 }, + [SCRFDR] = { 0x3c, 16 }, [SCSPTR] = sci_reg_invalid, [SCLSR] = sci_reg_invalid, },
Signature · 1 line
1.7.12.4
PATCH 9/10 serial: sh-sci: add locking to console write function to avoid SMP lockup
In reply to Shinya KuribayashiSymptom:
When entering the suspend with Android logcat running, printk() call gets stuck and never returns. The issue can be observed at printk()s on nonboot CPUs when going to offline with their interrupts disabled, and never seen at boot CPU (core0 in our case).
Details:
serial_console_write() lacks of appropriate spinlock handling.
In SMP systems, as long as sci_transmit_chars() is being processed at one CPU core, serial_console_write() can stuck at the other CPU core(s), when it tries to access to the same serial port _without_ a proper locking. serial_console_write() waits for the transmit FIFO getting empty, while sci_transmit_chars() writes data to the FIFO.
In general, peripheral interrupts are routed to boot CPU (core0) by Linux ARM standard affinity settings. SCI(F) interrupts are handled by core0, so sci_transmit_chars() is processed on core0 as well.
When logcat is running, it writes enormous log data to the kernel at every moment, forever. So core0 can repeatedly continue to process sci_transmit_chars() in its interrupt handler, which eventually makes the other CPU core(s) stuck at serial_console_write().
Looking at serial/8250.c, this is a known console write lockup issue with SMP kernels. Fix the sh-sci driver in the same way 8250.c does.
|commit ef605fdb33883d687cff5ba75095a91b313b4966 |Author: Rabin Vincent <[email protected]> |Date: Tue Jan 17 11:52:28 2012 +0100 | | serial: amba-pl011: lock console writes against interrupts
|commit 9ec1882df244c4ee1baa692676fef5e8b0f5487d |Author: Xinyu Chen <[email protected]> |Date: Mon Aug 27 09:36:51 2012 +0200 | | tty: serial: imx: console write routing is unsafe on SMP --- drivers/tty/serial/sh-sci.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/tty/serial/sh-sci.c +22 -1
@@ -2202,7 +2202,21 @@ static void serial_console_write(struct console *co, const char *s, {struct sci_port *sci_port = &sci_ports[co->index];struct uart_port *port = &sci_port->port; -unsigned short bits; +unsigned short bits, ctrl; +unsigned long flags; +int locked =1 ; + +local_irq_save (flags); +if (port->sysrq) + locked =0 ; +else if (oops_in_progress) + locked =spin_trylock (&port->lock); +else +spin_lock (&port->lock); + +/* first save the SCSCR then disable the interrupts */ + ctrl =serial_port_in (port,SCSCR ); +serial_port_out (port,SCSCR , sci_port->cfg->scscr);uart_console_write (port, s, count, serial_console_putchar);
@@ -2210,6 +2224,13 @@ static void serial_console_write(struct console *co, const char *s, bits =SCxSR_TDxE (port) |SCxSR_TEND (port);while ((serial_port_in (port, SCxSR) & bits) != bits)cpu_relax (); + +/* restore the SCSCR */ +serial_port_out (port,SCSCR , ctrl); + +if (locked) +spin_unlock (&port->lock); +local_irq_restore (flags); }static int __devinit serial_console_setup (struct console *co,char *options)
Signature · 1 line
1.7.12.4
PATCH 10/10 serial: sh-sci: fix possible race cases on SCSCR register accesses
In reply to Shinya KuribayashiIn the previous commit, console write function (serial_console_write) is changed to disable SCI interrupts while printing console strings. This introduces possible race cases in the serial startup / shutdown functions on SMP systems.
This patch fixes the sh-sci in the same way as commit 9ec1882df2 (tty: serial: imx: console write routing is unsafe on SMP, from Xinyu Chen <[email protected]>, 2012-08-27) did.
There could be several consumers of the console,
* the kernel printk
* the init process using /dev/kmsg to call printk to show log
* shell, which opens /dev/console and writes with sys_write()
The shell goes into the normal UART open() and write() system calls, while the other two go into the console operations. The open() call invokes serial startup function (sci_startup), which will write to the SCSCR register (to enable or disable SCI interrupts) without any locking. This will conflict with the console serial function.
Add spinlock protections in sci_startup() and sci_shutdown() properly.
--- drivers/tty/serial/sh-sci.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/tty/serial/sh-sci.c +6 -0
@@ -1742,6 +1742,7 @@ static inline void sci_free_dma(struct uart_port *port)static int sci_startup (struct uart_port *port) {struct sci_port *s =to_sci_port (port); +unsigned long flags;int ret;dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line);
@@ -1752,8 +1753,10 @@ static int sci_startup(struct uart_port *port)sci_request_dma (port); +spin_lock_irqsave (&port->lock, flags);sci_start_tx (port);sci_start_rx (port); +spin_unlock_irqrestore (&port->lock, flags);return 0 ; }
@@ -1761,11 +1764,14 @@ static int sci_startup(struct uart_port *port)static void sci_shutdown (struct uart_port *port) {struct sci_port *s =to_sci_port (port); +unsigned long flags;dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); +spin_lock_irqsave (&port->lock, flags);sci_stop_rx (port);sci_stop_tx (port); +spin_unlock_irqrestore (&port->lock, flags);sci_free_dma (port);sci_free_irq (s);
Signature · 1 line
1.7.12.4
cover serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
In reply to Shinya KuribayashiQuoted 1 line · Shinya Kuribayashi
Hi Paul and serial forks,
[...]
Quoted 11 lines
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definitionTeppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)
Oops, I forgot to append proper From: author lines in five commits (patch 04/10 to 09/10). Didn't mean to supersede your authoship.
I'd respin the patchset later, waiting for some feedback. --
Shinya Kuribayashi
Renesas Electronics
Greg KH
maintainercover serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
Quoted 12 lines · Shinya Kuribayashi
Quoted 1 line · Shinya Kuribayashi
Hi Paul and serial forks,
[...]
Quoted 9 lines
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definitionTeppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)
Quoted 7 lines
Quoted 2 lines
drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)Oops, I forgot to append proper From: author lines in five commits (patch 04/10 to 09/10). Didn't mean to supersede your authoship.
I'd respin the patchset later, waiting for some feedback.
Can you please resend this with this fixed up if you want to get this into 3.8?
thanks,
greg k-h
v2 cover serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
Hi Greg,
Quoted 15 lines · Shinya Kuribayashi
Quoted 15 lines · Shinya Kuribayashi
Quoted 1 line
Hi Paul and serial forks,
[...]
Quoted 11 lines
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definitionTeppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)
Quoted 7 lines
Quoted 4 lines
Oops, I forgot to append proper From: author lines in five commits (patch 04/10 to 09/10). Didn't mean to supersede your authoship.
I'd respin the patchset later, waiting for some feedback.
Can you please resend this with this fixed up if you want to get this into 3.8?
Thanks for your kind reminder. Here's v2, providing From: authorship lines in five patches (04 to 08). No logical change in the patches.
--- 8< ---
V2: provide missing authorship From: lines in five patches (04 to 08)
Here are sh-sci serial driver fixes that have been cooking for months privately on my end:
* rework console PM with the help of serial-core's uart_change_pm()
* provide missing piece in SCIFB support (from Yoshii-san)
* address console write function vs. SMP lockup issue
and so on. Comments and suggestions are welcome.
Build tested with vanilla v3.6, but not boot- nor PM-tested with newer kernels after v3.4. Anyone with can-sleep shmobile systems, please give it try.
Thanks in advance,
----------------------------------------------------------------
Shinya Kuribayashi (5):
Revert "sh-sci / PM: Avoid deadlocking runtime PM"
Revert "sh-sci / PM: Use power.irq_safe"
Partially revert "serial: sh-sci: console Runtime PM support"
serial: sh-sci: add locking to console write function to avoid SMP lockup
serial: sh-sci: fix possible race cases on SCSCR register accesses
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definition
Teppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)
drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)
Signature · 2 lines
Shinya Kuribayashi Renesas Electronics
PATCH v2 1/10 Revert "sh-sci / PM: Avoid deadlocking runtime PM"
This reverts commit 048be431e4 (sh-sci / PM: Avoid deadlocking runtime PM, from Rafael J. Wysocki <[email protected]>, 2012-03-09).
In order to get console PM work properly, we should implement uart_ops ->pm() operation, rather than sprinkle band-ading runtime PM calls in the driver.
--- drivers/tty/serial/sh-sci.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/tty/serial/sh-sci.c +0 -5
@@ -1752,8 +1752,6 @@ static int sci_startup(struct uart_port *port)dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); -pm_runtime_put_noidle (port->dev); -sci_port_enable (s); ret =sci_request_irq (s);
@@ -1781,8 +1779,6 @@ static void sci_shutdown(struct uart_port *port)sci_free_irq (s);sci_port_disable (s); - -pm_runtime_get_noresume (port->dev); } static unsigned int sci_scbrr_calc(unsigned int algo_id, unsigned int bps,
@@ -2121,7 +2117,6 @@ static int __devinit sci_init_single(struct platform_device *dev,sci_init_gpios (sci_port);pm_runtime_irq_safe (&dev->dev); -pm_runtime_get_noresume (&dev->dev);pm_runtime_enable (&dev->dev); }
Signature · 1 line
1.7.12.4
PATCH v2 2/10 Revert "sh-sci / PM: Use power.irq_safe"
In reply to Shinya KuribayashiThis reverts commit 5a50a01bf0 (sh-sci / PM: Use power.irq_safe, from Rafael J. Wysocki <[email protected]>, 2011-08-24).
In order to get console PM work properly, we should implement uart_ops ->pm() operation, rather than sprinkle band-ading runtime PM calls in the driver.
--- drivers/tty/serial/sh-sci.c | 1 - 1 file changed, 1 deletion(-)
drivers/tty/serial/sh-sci.c +0 -1
@@ -2116,7 +2116,6 @@ static int __devinit sci_init_single(struct platform_device *dev,sci_init_gpios (sci_port); -pm_runtime_irq_safe (&dev->dev);pm_runtime_enable (&dev->dev); }
Signature · 1 line
1.7.12.4
PATCH v2 3/10 Partially revert "serial: sh-sci: console Runtime PM support"
In reply to Shinya KuribayashiThis partially reverts commit 1ba7622094 (serial: sh-sci: console Runtime PM support, from Magnus Damm <[email protected]>, 2011-08-03).
The generic 'serial_core' can take care of console PM maintenance, so all (or at least the first thing) we have to do to get console PM work properly, is to implement uart_ops ->pm() operation in the sh-sci serial client driver.
This patch partially reverts the commit above, but leaving sci_reset() change in place, because sci_reset() is already part of another commit (73c3d53f38 serial: sh-sci: Avoid FIFO clear for MCE toggle.).
A revised version of console PM support follows next.
--- drivers/tty/serial/sh-sci.c | 53 +-------------------------------------------- 1 file changed, 1 insertion(+), 52 deletions(-)
drivers/tty/serial/sh-sci.c +1 -52
@@ -99,12 +99,6 @@ struct sci_port {#endif struct notifier_block freq_transition; - -#ifdef CONFIG_SERIAL_SH_SCI_CONSOLE -unsigned short saved_smr; -unsigned short saved_fcr; -unsigned char saved_brr; -#endif };/* Function prototypes */
@@ -2247,8 +2241,7 @@ static int __devinit serial_console_setup(struct console *co, char *options)if (options)uart_parse_options (options, &baud, &parity, &bits, &flow); -sci_port_disable (sci_port); - +/* TODO: disable clock */ return uart_set_options (port, co, baud, parity, bits, flow); }
@@ -2291,46 +2284,6 @@ static int __devinit sci_probe_earlyprintk(struct platform_device *pdev) return 0; } -#define uart_console (port) ((port)->cons->index == (port)->line) - -static int sci_runtime_suspend (struct device *dev) -{ -struct sci_port *sci_port =dev_get_drvdata (dev); -struct uart_port *port = &sci_port->port; - -if (uart_console (port)) { -struct plat_sci_reg *reg; - - sci_port->saved_smr =serial_port_in (port,SCSMR ); - sci_port->saved_brr =serial_port_in (port,SCBRR ); - - reg =sci_getreg (port,SCFCR ); -if (reg->size) - sci_port->saved_fcr =serial_port_in (port,SCFCR ); -else - sci_port->saved_fcr =0 ; - } -return 0 ; -} - -static int sci_runtime_resume (struct device *dev) -{ -struct sci_port *sci_port =dev_get_drvdata (dev); -struct uart_port *port = &sci_port->port; - -if (uart_console (port)) { -sci_reset (port); -serial_port_out (port,SCSMR , sci_port->saved_smr); -serial_port_out (port,SCBRR , sci_port->saved_brr); - -if (sci_port->saved_fcr) -serial_port_out (port,SCFCR , sci_port->saved_fcr); - -serial_port_out (port,SCSCR , sci_port->cfg->scscr); - } -return 0 ; -} -#define SCI_CONSOLE (&serial_console)#else
@@ -2340,8 +2293,6 @@ static inline int __devinit sci_probe_earlyprintk(struct platform_device *pdev) }#define SCI_CONSOLE NULL -#define sci_runtime_suspend NULL -#define sci_runtime_resume NULL#endif /* CONFIG_SERIAL_SH_SCI_CONSOLE */
@@ -2459,8 +2410,6 @@ static int sci_resume(struct device *dev) }static const struct dev_pm_ops sci_dev_pm_ops = { - .runtime_suspend = sci_runtime_suspend, - .runtime_resume = sci_runtime_resume, .suspend = sci_suspend, .resume = sci_resume, };
Signature · 1 line
1.7.12.4
PATCH v2 4/10 serial: sh-sci: console runtime PM support (revisit)
In reply to Shinya KuribayashiFrom: Teppei Kamijou <[email protected]>
The commit 1ba7622094 (serial: sh-sci: console Runtime PM support, from Magnus Damm <[email protected]>, 2011-08-03), tried to support console runtime PM, but unfortunately it didn't work for us for some reason. We did not investigated further at that time, instead would like to propose a different approach.
In Linux tty/serial world, to get console PM work properly, a serial client driver does not have to maintain .runtime_suspend()/..resume() calls itself, but can leave console power power management handling to the serial core driver.
This patch moves the sh-sci driver in that direction.
Notes:
* There is room to optimize console runtime PM more aggressively by maintaining additional local runtime PM calls, but as a first step having .pm() operation would suffice.
* We still have a couple of direct calls to sci_port_enable/..disable left in the driver. We have to live with them, because they're out of serial core's help.
--- drivers/tty/serial/sh-sci.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
drivers/tty/serial/sh-sci.c +16 -11
@@ -1746,8 +1746,6 @@ static int sci_startup(struct uart_port *port)dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); -sci_port_enable (s); - ret =sci_request_irq (s);if (unlikely (ret <0 ))return ret;
@@ -1771,8 +1769,6 @@ static void sci_shutdown(struct uart_port *port)sci_free_dma (port);sci_free_irq (s); - -sci_port_disable (s); } static unsigned int sci_scbrr_calc(unsigned int algo_id, unsigned int bps,
@@ -1921,6 +1917,21 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,sci_port_disable (s); } +static void sci_pm (struct uart_port *port,unsigned int state, +unsigned int oldstate) +{ +struct sci_port *sci_port =to_sci_port (port); + +switch (state) { +case 3 : +sci_port_disable (sci_port); +break ; +default : +sci_port_enable (sci_port); +break ; + } +} +static const char *sci_type (struct uart_port *port) {switch (port->type) {
@@ -2042,6 +2053,7 @@ static struct uart_ops sci_uart_ops = { .startup = sci_startup, .shutdown = sci_shutdown, .set_termios = sci_set_termios, + .pm = sci_pm, .type = sci_type, .release_port = sci_release_port, .request_port = sci_request_port,
@@ -2195,16 +2207,12 @@ static void serial_console_write(struct console *co, const char *s,struct uart_port *port = &sci_port->port;unsigned short bits; -sci_port_enable (sci_port); -uart_console_write (port, s, count, serial_console_putchar);/* wait until fifo is empty and last bit has been transmitted */ bits =SCxSR_TDxE (port) |SCxSR_TEND (port);while ((serial_port_in (port, SCxSR) & bits) != bits)cpu_relax (); - -sci_port_disable (sci_port); }static int __devinit serial_console_setup (struct console *co,char *options)
@@ -2236,12 +2244,9 @@ static int __devinit serial_console_setup(struct console *co, char *options)if (unlikely (ret !=0 ))return ret; -sci_port_enable (sci_port); -if (options)uart_parse_options (options, &baud, &parity, &bits, &flow); -/* TODO: disable clock */ return uart_set_options (port, co, baud, parity, bits, flow); }
Signature · 1 line
1.7.12.4
PATCH v2 5/10 serial: sh-sci: fix condition test to set SCBRR
In reply to Shinya KuribayashiFrom: Takashi Yoshii <[email protected]>
SCBRR == 0 is valid value (divide by 1).
--- drivers/tty/serial/sh-sci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/sh-sci.c +1 -1
@@ -1853,7 +1853,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,dev_dbg (port->dev,"%s: SMR %x, t %x, SCSCR %x\n" , __func__, smr_val, t, s->cfg->scscr); -if (t >0 ) { +if (t >=0 ) {if (t >=256 ) {serial_port_out (port,SCSMR , (serial_port_in (port,SCSMR ) & ~3 ) |1 ); t >>=2 ;
Signature · 1 line
1.7.12.4
PATCH v2 6/10 serial: sh-sci: support lower baud rate
In reply to Shinya KuribayashiFrom: Takashi Yoshii <[email protected]>
Support prescaler 1/16 and 1/64, in addition to current 1 and 1/4.
Supporting below 2400bps was dropped long time ago in mainline. Since then, setting lower rate has been resulting in erroneous register value, without indicating any errors through API.
This patch adds more prescaler to support lower rates again. This still doesn't check range, but we won't hit the case because even 50bps at 48MHz clock is now supported.
--- drivers/tty/serial/sh-sci.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/tty/serial/sh-sci.c +8 -11
@@ -1814,7 +1814,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios, {struct sci_port *s =to_sci_port (port);struct plat_sci_reg *reg; -unsigned int baud, smr_val, max_baud; +unsigned int baud, smr_val, max_baud, cks;int t =-1 ;/*
@@ -1848,21 +1848,18 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,uart_update_timeout (port, termios->c_cflag, baud); -serial_port_out (port,SCSMR , smr_val); +for (cks =0 ; t >=256 && cks <=3 ; cks++) + t >>=2 ; -dev_dbg (port->dev,"%s: SMR %x, t %x, SCSCR %x\n" , __func__, smr_val, t, - s->cfg->scscr); +dev_dbg (port->dev,"%s: SMR %x, cks %x, t %x, SCSCR %x\n" , + __func__, smr_val, cks, t, s->cfg->scscr);if (t >=0 ) { -if (t >=256 ) { -serial_port_out (port,SCSMR , (serial_port_in (port,SCSMR ) & ~3 ) |1 ); - t >>=2 ; - }else -serial_port_out (port,SCSMR ,serial_port_in (port,SCSMR ) & ~3 ); - +serial_port_out (port,SCSMR , (smr_val & ~3 ) | cks);serial_port_out (port,SCBRR , t);udelay ((1000000 +(baud-1 )) / baud);/* Wait one bit interval */ - } + }else +serial_port_out (port,SCSMR , smr_val);sci_init_pins (port, termios->c_cflag);
Signature · 1 line
1.7.12.4
PATCH v2 7/10 serial: sh-sci: mask SCTFDR/RFDR according to fifosize
In reply to Shinya KuribayashiFrom: Takashi Yoshii <[email protected]>
Current mask 0xff to SCTFDR/RFDR damages SCIFB, because the registers on SCIFB have 9-bit data (0 to 256).
This patch changes the mask according to port->fifosize. Though I'm not sure if the mask is really needed (I don't know if there are variants which have non-zero upper bits), it is safer.
--- drivers/tty/serial/sh-sci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/sh-sci.c +2 -2
@@ -485,7 +485,7 @@ static int sci_txfill(struct uart_port *port) reg =sci_getreg (port,SCTFDR );if (reg->size) -return serial_port_in (port,SCTFDR ) &0xff ; +return serial_port_in (port,SCTFDR ) & ((port->fifosize <<1 ) -1 ); reg =sci_getreg (port,SCFDR );if (reg->size)
@@ -505,7 +505,7 @@ static int sci_rxfill(struct uart_port *port) reg =sci_getreg (port,SCRFDR );if (reg->size) -return serial_port_in (port,SCRFDR ) &0xff ; +return serial_port_in (port,SCRFDR ) & ((port->fifosize <<1 ) -1 ); reg =sci_getreg (port,SCFDR );if (reg->size)
Signature · 1 line
1.7.12.4
PATCH v2 8/10 serial: sh-sci: fix common SCIFB regmap definition
In reply to Shinya KuribayashiFrom: Takashi Yoshii <[email protected]>
About FIFO count, there are two variants of SCIFs which show
a) TX count in upper, RX count in lower byte of FDR register
b) TX count in TFDR register, RX count in RFDR register
Common SCIFB regmap in current source code is defined as "a". At least 7372 and 73a0 HW manual say their SICFB are "b".
This patch alters the definition to "b", considering the current one has come from a mistake. The reason is as follows.
The flag SCIFB sh-sci driver means it has 256 byte FIFO. The count is from 0(empty) to 256(full), that makes 9-bit. Because FDR is 16-bit register, it can not hold two 9-bits. That's why, SCIFB can not be "a".
--- drivers/tty/serial/sh-sci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/tty/serial/sh-sci.c +3 -3
@@ -196,9 +196,9 @@ static struct plat_sci_reg sci_regmap[SCIx_NR_REGTYPES][SCIx_NR_REGS] = { [SCxSR] = { 0x14, 16 }, [SCxRDR] = { 0x60, 8 }, [SCFCR] = { 0x18, 16 }, - [SCFDR] = { 0x1c, 16 }, - [SCTFDR] = sci_reg_invalid, - [SCRFDR] = sci_reg_invalid, + [SCFDR] = sci_reg_invalid, + [SCTFDR] = { 0x38, 16 }, + [SCRFDR] = { 0x3c, 16 }, [SCSPTR] = sci_reg_invalid, [SCLSR] = sci_reg_invalid, },
Signature · 1 line
1.7.12.4
PATCH v2 9/10 serial: sh-sci: add locking to console write function to avoid SMP lockup
In reply to Shinya KuribayashiSymptom:
When entering the suspend with Android logcat running, printk() call gets stuck and never returns. The issue can be observed at printk()s on nonboot CPUs when going to offline with their interrupts disabled, and never seen at boot CPU (core0 in our case).
Details:
serial_console_write() lacks of appropriate spinlock handling.
In SMP systems, as long as sci_transmit_chars() is being processed at one CPU core, serial_console_write() can stuck at the other CPU core(s), when it tries to access to the same serial port _without_ a proper locking. serial_console_write() waits for the transmit FIFO getting empty, while sci_transmit_chars() writes data to the FIFO.
In general, peripheral interrupts are routed to boot CPU (core0) by Linux ARM standard affinity settings. SCI(F) interrupts are handled by core0, so sci_transmit_chars() is processed on core0 as well.
When logcat is running, it writes enormous log data to the kernel at every moment, forever. So core0 can repeatedly continue to process sci_transmit_chars() in its interrupt handler, which eventually makes the other CPU core(s) stuck at serial_console_write().
Looking at serial/8250.c, this is a known console write lockup issue with SMP kernels. Fix the sh-sci driver in the same way 8250.c does.
---
Similar fixes can be found at:
|commit ef605fdb33883d687cff5ba75095a91b313b4966 |Author: Rabin Vincent <[email protected]> |Date: Tue Jan 17 11:52:28 2012 +0100 | | serial: amba-pl011: lock console writes against interrupts
|commit 9ec1882df244c4ee1baa692676fef5e8b0f5487d |Author: Xinyu Chen <[email protected]> |Date: Mon Aug 27 09:36:51 2012 +0200 | | tty: serial: imx: console write routing is unsafe on SMP
drivers/tty/serial/sh-sci.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/tty/serial/sh-sci.c +22 -1
@@ -2202,7 +2202,21 @@ static void serial_console_write(struct console *co, const char *s, {struct sci_port *sci_port = &sci_ports[co->index];struct uart_port *port = &sci_port->port; -unsigned short bits; +unsigned short bits, ctrl; +unsigned long flags; +int locked =1 ; + +local_irq_save (flags); +if (port->sysrq) + locked =0 ; +else if (oops_in_progress) + locked =spin_trylock (&port->lock); +else +spin_lock (&port->lock); + +/* first save the SCSCR then disable the interrupts */ + ctrl =serial_port_in (port,SCSCR ); +serial_port_out (port,SCSCR , sci_port->cfg->scscr);uart_console_write (port, s, count, serial_console_putchar);
@@ -2210,6 +2224,13 @@ static void serial_console_write(struct console *co, const char *s, bits =SCxSR_TDxE (port) |SCxSR_TEND (port);while ((serial_port_in (port, SCxSR) & bits) != bits)cpu_relax (); + +/* restore the SCSCR */ +serial_port_out (port,SCSCR , ctrl); + +if (locked) +spin_unlock (&port->lock); +local_irq_restore (flags); }static int __devinit serial_console_setup (struct console *co,char *options)
Signature · 1 line
1.7.12.4
PATCH v2 10/10 serial: sh-sci: fix possible race cases on SCSCR register accesses
In reply to Shinya KuribayashiIn the previous commit, console write function (serial_console_write) is changed to disable SCI interrupts while printing console strings. This introduces possible race cases in the serial startup / shutdown functions on SMP systems.
This patch fixes the sh-sci in the same way as commit 9ec1882df2 (tty: serial: imx: console write routing is unsafe on SMP, from Xinyu Chen <[email protected]>, 2012-08-27) did.
There could be several consumers of the console,
* the kernel printk
* the init process using /dev/kmsg to call printk to show log
* shell, which opens /dev/console and writes with sys_write()
The shell goes into the normal UART open() and write() system calls, while the other two go into the console operations. The open() call invokes serial startup function (sci_startup), which will write to the SCSCR register (to enable or disable SCI interrupts) without any locking. This will conflict with the console serial function.
Add spinlock protections in sci_startup() and sci_shutdown() properly.
--- drivers/tty/serial/sh-sci.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/tty/serial/sh-sci.c +6 -0
@@ -1742,6 +1742,7 @@ static inline void sci_free_dma(struct uart_port *port)static int sci_startup (struct uart_port *port) {struct sci_port *s =to_sci_port (port); +unsigned long flags;int ret;dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line);
@@ -1752,8 +1753,10 @@ static int sci_startup(struct uart_port *port)sci_request_dma (port); +spin_lock_irqsave (&port->lock, flags);sci_start_tx (port);sci_start_rx (port); +spin_unlock_irqrestore (&port->lock, flags);return 0 ; }
@@ -1761,11 +1764,14 @@ static int sci_startup(struct uart_port *port)static void sci_shutdown (struct uart_port *port) {struct sci_port *s =to_sci_port (port); +unsigned long flags;dev_dbg (port->dev,"%s(%d)\n" , __func__, port->line); +spin_lock_irqsave (&port->lock, flags);sci_stop_rx (port);sci_stop_tx (port); +spin_unlock_irqrestore (&port->lock, flags);sci_free_dma (port);sci_free_irq (s);
Signature · 1 line
1.7.12.4
Greg KH
maintainerv2 cover serial: sh-sci fixes - console PM, SCIFB, SMP lockup, etc.
In reply to Shinya KuribayashiQuoted 21 lines · Shinya Kuribayashi
Hi Greg,
Quoted 18 lines · Shinya Kuribayashi
Quoted 18 lines · Shinya Kuribayashi
Quoted 1 line
Hi Paul and serial forks,
[...]
Quoted 11 lines
Takashi YOSHII (4):
serial: sh-sci: fix condition test to set SCBRR
serial: sh-sci: support lower baud rate
serial: sh-sci: mask SCTFDR/RFDR according to fifosize
serial: sh-sci: fix common SCIFB regmap definitionTeppei Kamijou (1):
serial: sh-sci: console runtime PM support (revisit)drivers/tty/serial/sh-sci.c | 140 ++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 84 deletions(-)Oops, I forgot to append proper From: author lines in five commits (patch 04/10 to 09/10). Didn't mean to supersede your authoship.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
I'd respin the patchset later, waiting for some feedback.
Can you please resend this with this fixed up if you want to get this into 3.8?
Thanks for your kind reminder. Here's v2, providing From: authorship lines in five patches (04 to 08). No logical change in the patches.
All now applied, thanks.
greg k-h