| Message ID | 1345717983-18179-1-git-send-email-balbi@ti.com |
|---|---|
| State | New |
| Headers | show |
Forgot to Cc greg initially, my bad On Thu, Aug 23, 2012 at 01:32:40PM +0300, Felipe Balbi wrote: > Hi guys, > > here's v3 and hopefully final version of this series. A whole bunch of new > patches added but the good thing is that now I had another engineer's help to > test, so he's got his Tested-by in all patches. > > Changes since v2: > . Added a bunch of new patches > . Fixed a problem where we would always return IRQ_NONE even though we > handled IRQ > > Changes since v1: > . improved commit log on patch 9/13 (formerly 10/13) > . removed patch 2/13 > . added a new patch switching from spin_lock_irqsave() to spin_lock and > spin_unlock_irqrestore to spin_unlock > > Alan, if you prefer in pull request form, here it is: > > The following changes since commit d9875690d9b89a866022ff49e3fcea892345ad92: > > Linux 3.6-rc2 (2012-08-16 14:51:24 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart > > for you to fetch changes up to a29230f14d8466c9b8c25171715378bf52189453: > > serial: omap: enable RX and TX FIFO usage (2012-08-23 09:25:16 +0300) > > ---------------------------------------------------------------- > Felipe Balbi (20): > serial: omap: define and use to_uart_omap_port() > serial: omap: define helpers for pdata function pointers > serial: omap: don't access the platform_device > serial: omap: drop DMA support > serial: add OMAP-specific defines > serial: omap: simplify IRQ handling > serial: omap: refactor receive_chars() into rdi/rlsi handlers > serial: omap: move THRE check to transmit_chars() > serial: omap: stick to put_autosuspend > serial: omap: set dev->drvdata before enabling pm_runtime > serial: omap: drop unnecessary check from remove > serial: omap: make sure to suspend device before remove > serial: omap: don't save IRQ flags on hardirq > serial: omap: optimization with section annotations > serial: omap: drop "inline" from IRQ handler prototype > serial: omap: implement set_wake > serial: omap: make sure to put() on poll_get_char > serial: omap: remove unnecessary header and add a missing one > serial: omap: move uart_omap_port definition to C file > serial: omap: enable RX and TX FIFO usage > > Ruchika Kharwar (2): > serial: omap: fix sequence of pm_runtime_* calls. > serial: omap: unlock the port lock > > Vikram Pandita (1): > serial: omap: fix software flow control > > arch/arm/mach-omap2/serial.c | 15 +- > arch/arm/plat-omap/include/plat/omap-serial.h | 47 +- > drivers/tty/serial/omap-serial.c | 808 ++++++++++---------------- > include/linux/serial_reg.h | 4 + > 4 files changed, 330 insertions(+), 544 deletions(-) > > -- > 1.7.12.rc3 >
* Felipe Balbi <balbi@ti.com> [120823 03:37]: > current code only works because struct uart_port > is the first member on the uart_omap_port structure. > > If, for whatever reason, someone puts another > member as the first of the structure, that cast > won't work anymore. In order to be safe, let's use > a container_of() which, for now, gets optimized into > a cast anyway. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > arch/arm/plat-omap/include/plat/omap-serial.h | 2 ++ > drivers/tty/serial/omap-serial.c | 36 +++++++++++++-------------- > 2 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h > index 1a52725..f3b35d9 100644 > --- a/arch/arm/plat-omap/include/plat/omap-serial.h > +++ b/arch/arm/plat-omap/include/plat/omap-serial.h > @@ -137,4 +137,6 @@ struct uart_omap_port { > struct work_struct qos_work; > }; > > +#define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port))) > + > #endif /* __OMAP_SERIAL_H__ */ For the arch/arm/*omap*/* parts: Acked-by: Tony Lindgren <tony@atomide.com> FYI, note that there are ARM SoC patches to move various header files to live under include/linux/platform_data which may cause conflicts with these changes. Regards, Tony
* Felipe Balbi <balbi@ti.com> [120823 03:37]: > The driver doesn't need to know about its platform_device. > > Everything the driver needs can be done through the > struct device pointer. In case we need to use the > OMAP-specific PM function pointers, those can make > sure to find the device's platform_device pointer > so they can find the struct omap_device through > pdev->archdata field. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> Acked-by: Tony Lindgren <tony@atomide.com>
* Felipe Balbi <balbi@ti.com> [120823 03:38]: > From: Vikram Pandita <vikram.pandita@ti.com> > > Software flow control register bits were not defined correctly. > > Also clarify the IXON and IXOFF logic to reflect what userspace wants. > > Cc: stable@vger.kernel.org > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> Acked-by: Tony Lindgren <tony@atomide.com>
* Felipe Balbi <balbi@ti.com> [120823 03:38]: > nobody needs to access the uart_omap_port structure > other than omap-serial.c file. Let's move that > structure definition to the C source file in order > to prevent anyone from accessing our structure. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> Acked-by: Tony Lindgren <tony@atomide.com>
Hi, On Fri, Aug 24, 2012 at 01:40:47PM +0300, Felipe Balbi wrote: > Forgot to Cc greg initially, my bad a gentle ping on this series so we don't miss it for v3.7 merge window. Greg, let me know if you need me to resend. cheers > On Thu, Aug 23, 2012 at 01:32:40PM +0300, Felipe Balbi wrote: > > Hi guys, > > > > here's v3 and hopefully final version of this series. A whole bunch of new > > patches added but the good thing is that now I had another engineer's help to > > test, so he's got his Tested-by in all patches. > > > > Changes since v2: > > . Added a bunch of new patches > > . Fixed a problem where we would always return IRQ_NONE even though we > > handled IRQ > > > > Changes since v1: > > . improved commit log on patch 9/13 (formerly 10/13) > > . removed patch 2/13 > > . added a new patch switching from spin_lock_irqsave() to spin_lock and > > spin_unlock_irqrestore to spin_unlock > > > > Alan, if you prefer in pull request form, here it is: > > > > The following changes since commit d9875690d9b89a866022ff49e3fcea892345ad92: > > > > Linux 3.6-rc2 (2012-08-16 14:51:24 -0700) > > > > are available in the git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart > > > > for you to fetch changes up to a29230f14d8466c9b8c25171715378bf52189453: > > > > serial: omap: enable RX and TX FIFO usage (2012-08-23 09:25:16 +0300) > > > > ---------------------------------------------------------------- > > Felipe Balbi (20): > > serial: omap: define and use to_uart_omap_port() > > serial: omap: define helpers for pdata function pointers > > serial: omap: don't access the platform_device > > serial: omap: drop DMA support > > serial: add OMAP-specific defines > > serial: omap: simplify IRQ handling > > serial: omap: refactor receive_chars() into rdi/rlsi handlers > > serial: omap: move THRE check to transmit_chars() > > serial: omap: stick to put_autosuspend > > serial: omap: set dev->drvdata before enabling pm_runtime > > serial: omap: drop unnecessary check from remove > > serial: omap: make sure to suspend device before remove > > serial: omap: don't save IRQ flags on hardirq > > serial: omap: optimization with section annotations > > serial: omap: drop "inline" from IRQ handler prototype > > serial: omap: implement set_wake > > serial: omap: make sure to put() on poll_get_char > > serial: omap: remove unnecessary header and add a missing one > > serial: omap: move uart_omap_port definition to C file > > serial: omap: enable RX and TX FIFO usage > > > > Ruchika Kharwar (2): > > serial: omap: fix sequence of pm_runtime_* calls. > > serial: omap: unlock the port lock > > > > Vikram Pandita (1): > > serial: omap: fix software flow control > > > > arch/arm/mach-omap2/serial.c | 15 +- > > arch/arm/plat-omap/include/plat/omap-serial.h | 47 +- > > drivers/tty/serial/omap-serial.c | 808 ++++++++++---------------- > > include/linux/serial_reg.h | 4 + > > 4 files changed, 330 insertions(+), 544 deletions(-) > > > > -- > > 1.7.12.rc3 > > > > -- > balbi
On Tue, Sep 04, 2012 at 02:45:41PM +0300, Felipe Balbi wrote: > Hi, > > On Fri, Aug 24, 2012 at 01:40:47PM +0300, Felipe Balbi wrote: > > Forgot to Cc greg initially, my bad > > a gentle ping on this series so we don't miss it for v3.7 merge window. > > Greg, let me know if you need me to resend. No need, sorry, was on vacation and then the Kernel summit. I'll get to these now... greg k-h
On Thu, Aug 23, 2012 at 01:32:43PM +0300, Felipe Balbi wrote: > The driver doesn't need to know about its platform_device. > > Everything the driver needs can be done through the > struct device pointer. In case we need to use the > OMAP-specific PM function pointers, those can make > sure to find the device's platform_device pointer > so they can find the struct omap_device through > pdev->archdata field. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> This series fails when applying to my tty-next branch at this patch :( Care to refresh it, and resend it, with Tony's acks added, so I can apply it? Note, I've already applied the first two. thanks, greg k-h
On Thu, Aug 23, 2012 at 01:33:00PM +0300, Felipe Balbi wrote: > From: Vikram Pandita <vikram.pandita@ti.com> > > Software flow control register bits were not defined correctly. > > Also clarify the IXON and IXOFF logic to reflect what userspace wants. > > Cc: stable@vger.kernel.org > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> > --- If this is a stable patch, should it be pulled out and put also in for 3.6? thanks, greg k-h
On Wed, Sep 05, 2012 at 01:27:21PM -0700, Greg KH wrote: > On Thu, Aug 23, 2012 at 01:32:43PM +0300, Felipe Balbi wrote: > > The driver doesn't need to know about its platform_device. > > > > Everything the driver needs can be done through the > > struct device pointer. In case we need to use the > > OMAP-specific PM function pointers, those can make > > sure to find the device's platform_device pointer > > so they can find the struct omap_device through > > pdev->archdata field. > > > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > Acked-by: Tony Lindgren <tony@atomide.com> > > This series fails when applying to my tty-next branch at this patch :( > > Care to refresh it, and resend it, with Tony's acks added, so I can > apply it? Note, I've already applied the first two. Sure, will do and reply to this mail with the new series. Thanks
On Wed, Sep 05, 2012 at 01:27:49PM -0700, Greg KH wrote: > On Thu, Aug 23, 2012 at 01:33:00PM +0300, Felipe Balbi wrote: > > From: Vikram Pandita <vikram.pandita@ti.com> > > > > Software flow control register bits were not defined correctly. > > > > Also clarify the IXON and IXOFF logic to reflect what userspace wants. > > > > Cc: stable@vger.kernel.org > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com> > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > Acked-by: Tony Lindgren <tony@atomide.com> > > --- > > If this is a stable patch, should it be pulled out and put also in for > 3.6? I'll leave that for Vikram to answer. Vikram, do you think this is something so extreme that we _must_ apply it now, or can it wait for the next merge window ? I mean, I think this bug has been in the driver for a long time, right ?
Hi guys,
here's v4 of the omap uart patchset. No changes other than a rebase on top of
Greg's tty-next branch and Tony's Acked-by being added to a couple patches
Note: I'm resending the series with Vikram's Software Flow Control fix anyway
as it can just be ignored if it's decided it needs to go into this merge
window.
Changes since v3:
. Rebased on top of Greg's tty-next branch
. Added Tony's Acked-by
Changes since v2:
. Added a bunch of new patches
. Fixed a problem where we would always return IRQ_NONE even though we
handled IRQ
Changes since v1:
. improved commit log on patch 9/13 (formerly 10/13)
. removed patch 2/13
. added a new patch switching from spin_lock_irqsave() to spin_lock and
spin_unlock_irqrestore to spin_unlock
Felipe Balbi (18):
serial: omap: don't access the platform_device
serial: omap: drop DMA support
serial: add OMAP-specific defines
serial: omap: simplify IRQ handling
serial: omap: refactor receive_chars() into rdi/rlsi handlers
serial: omap: move THRE check to transmit_chars()
serial: omap: stick to put_autosuspend
serial: omap: set dev->drvdata before enabling pm_runtime
serial: omap: drop unnecessary check from remove
serial: omap: make sure to suspend device before remove
serial: omap: don't save IRQ flags on hardirq
serial: omap: optimization with section annotations
serial: omap: drop "inline" from IRQ handler prototype
serial: omap: implement set_wake
serial: omap: make sure to put() on poll_get_char
serial: omap: remove unnecessary header and add a missing one
serial: omap: move uart_omap_port definition to C file
serial: omap: enable RX and TX FIFO usage
Ruchika Kharwar (2):
serial: omap: fix sequence of pm_runtime_* calls.
serial: omap: unlock the port lock
Vikram Pandita (1):
serial: omap: fix software flow control
arch/arm/mach-omap2/serial.c | 15 +-
arch/arm/plat-omap/include/plat/omap-serial.h | 49 +-
drivers/tty/serial/omap-serial.c | 740 +++++++++-----------------
include/linux/serial_reg.h | 4 +
4 files changed, 282 insertions(+), 526 deletions(-)
On Thu, Sep 06, 2012 at 03:45:19PM +0300, Felipe Balbi wrote: > Hi guys, > > here's v4 of the omap uart patchset. No changes other than a rebase on top of > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > as it can just be ignored if it's decided it needs to go into this merge > window. > > Changes since v3: > . Rebased on top of Greg's tty-next branch > . Added Tony's Acked-by Thanks for rebasing, all now applied. greg k-h
On Thu, Sep 06, 2012 at 09:18:35AM -0700, Greg KH wrote: > On Thu, Sep 06, 2012 at 03:45:19PM +0300, Felipe Balbi wrote: > > Hi guys, > > > > here's v4 of the omap uart patchset. No changes other than a rebase on top of > > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > > > > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > > as it can just be ignored if it's decided it needs to go into this merge > > window. > > > > Changes since v3: > > . Rebased on top of Greg's tty-next branch > > . Added Tony's Acked-by > > Thanks for rebasing, all now applied. thank you, actually ;-)
Hi On Thu, 6 Sep 2012, Felipe Balbi wrote: > Hi guys, > > here's v4 of the omap uart patchset. No changes other than a rebase on top of > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > as it can just be ignored if it's decided it needs to go into this merge > window. Was curious to know if these have been PM-tested, and if so, under what configurations? - Paul
Felipe Balbi <balbi@ti.com> writes: > nobody needs to access the uart_omap_port structure > other than omap-serial.c file. Let's move that > structure definition to the C source file in order > to prevent anyone from accessing our structure. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Acked-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> This one is clearly broken, and causes compile breakage... > --- > arch/arm/plat-omap/include/plat/omap-serial.h | 37 -------------------------- > drivers/tty/serial/omap-serial.c | 38 +++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+), 37 deletions(-) > > diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h > index a79ed8b..3c9fd3e 100644 > --- a/arch/arm/plat-omap/include/plat/omap-serial.h > +++ b/arch/arm/plat-omap/include/plat/omap-serial.h > @@ -105,45 +105,8 @@ struct uart_omap_dma { > unsigned int rx_timeout; > }; > > -struct uart_omap_port { > - struct uart_port port; > - struct uart_omap_dma uart_dma; > - struct device *dev; > - > - unsigned char ier; > - unsigned char lcr; > - unsigned char mcr; > - unsigned char fcr; > - unsigned char efr; > - unsigned char dll; > - unsigned char dlh; > - unsigned char mdr1; > - unsigned char scr; > - > - int use_dma; > - /* > - * Some bits in registers are cleared on a read, so they must > - * be saved whenever the register is read but the bits will not > - * be immediately processed. > - */ > - unsigned int lsr_break_flag; > - unsigned char msr_saved_flags; > - char name[20]; > - unsigned long port_activity; > - u32 context_loss_cnt; > - u32 errata; > - u8 wakeups_enabled; > > int DTR_gpio; > int DTR_inverted; > int DTR_active; The whole struct has moved, but some fields left behind? This looks to be caused by the rebase which includes Neil Brown's DTR GPIO change[1] > - > - struct pm_qos_request pm_qos_request; > - u32 latency; > - u32 calc_latency; > - struct work_struct qos_work; > -}; > - > -#define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port))) > - > #endif /* __OMAP_SERIAL_H__ */ > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 881b652..164c3c9 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -70,6 +70,44 @@ > #define OMAP_UART_MVR_MAJ_SHIFT 8 > #define OMAP_UART_MVR_MIN_MASK 0x3f > > +struct uart_omap_port { > + struct uart_port port; > + struct uart_omap_dma uart_dma; > + struct device *dev; > + > + unsigned char ier; > + unsigned char lcr; > + unsigned char mcr; > + unsigned char fcr; > + unsigned char efr; > + unsigned char dll; > + unsigned char dlh; > + unsigned char mdr1; > + unsigned char scr; > + > + int use_dma; > + /* > + * Some bits in registers are cleared on a read, so they must > + * be saved whenever the register is read but the bits will not > + * be immediately processed. > + */ > + unsigned int lsr_break_flag; > + unsigned char msr_saved_flags; > + char name[20]; > + unsigned long port_activity; > + u32 context_loss_cnt; > + u32 errata; > + u8 wakeups_enabled; > + unsigned int irq_pending:1; And this field was not in the original, yet the changelog describes this as a move. Why is a this new field added (and not described in the changelog.) Kevin [1] commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 Author: NeilBrown <neilb@suse.de> Date: Mon Jul 30 10:30:26 2012 +1000 OMAP/serial: Add support for driving a GPIO as DTR. OMAP hardware doesn't provide a phyisical DTR line, but some configurations may need a DTR line which tracks whether the device is open or not. So allow a gpio to be configured as the DTR line. Signed-off-by: NeilBrown <neilb@suse.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Felipe Balbi <balbi@ti.com> writes: > Hi guys, > > here's v4 of the omap uart patchset. No changes other than a rebase on top of > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > as it can just be ignored if it's decided it needs to go into this merge > window. Sorry to be late to the party... just getting back from some time off. I'm assuming that this was not tested with PM, so decided I better do it myself seeing that Greg is has already merge it. To test, I merged Greg's tty-next branch with v3.6-rc4 and did some PM testing. The bad news is that it doesn't even compile (see reply to [PATCH v4 20/21]). Also, there is a big WARNING on boot[1], which seems to be triggered by a new check added for v3.6-rc3[2]. This appears to be introduced by $SUBJECT series, because I don't see it on vanilla v3.6-rc4. The good news is that after hacking to fix up the compile problems, basic PM testing seems to be fine: idle to retention and off as well as suspend to retention and off work fine on 3430/n900, 3530/Overo, 3730/OveroSTORM, 3730/Beagle-xM. Kevin [1] [ 8.745666] WARNING: at /work/kernel/omap/pm/drivers/tty/tty_io.c:1420 tty_init_dev+0x14c/0x17c() [ 8.755218] tty_init_dev: ttyO driver does not set tty->port. This will crash the kernel later. Fix the driver! [ 8.765991] Modules linked in: [ 8.769287] [<c0013d90>] (unwind_backtrace+0x0/0xf0) from [<c0036eec>] (warn_slowpath_common+0x4c/0x64) [ 8.779327] [<c0036eec>] (warn_slowpath_common+0x4c/0x64) from [<c0036f98>] (warn_slowpath_fmt+0x30/0x40) [ 8.789550] [<c0036f98>] (warn_slowpath_fmt+0x30/0x40) from [<c02c626c>] (tty_init_dev+0x14c/0x17c) [ 8.799224] [<c02c626c>] (tty_init_dev+0x14c/0x17c) from [<c02c68a4>] (tty_open+0x11c/0x52c) [ 8.808258] [<c02c68a4>] (tty_open+0x11c/0x52c) from [<c00f86ac>] (chrdev_open+0x90/0x15c) [ 8.817108] [<c00f86ac>] (chrdev_open+0x90/0x15c) from [<c00f2b74>] (do_dentry_open+0x1e8/0x270) [ 8.826507] [<c00f2b74>] (do_dentry_open+0x1e8/0x270) from [<c00f2c30>] (finish_open+0x34/0x4c) [ 8.835784] [<c00f2c30>] (finish_open+0x34/0x4c) from [<c0102028>] (do_last.isra.21+0x5b0/0xbbc) [ 8.845184] [<c0102028>] (do_last.isra.21+0x5b0/0xbbc) from [<c01026dc>] (path_openat+0xa8/0x44c) [ 8.854675] [<c01026dc>] (path_openat+0xa8/0x44c) from [<c0102d34>] (do_filp_open+0x2c/0x80) [ 8.863708] [<c0102d34>] (do_filp_open+0x2c/0x80) from [<c00f3b6c>] (do_sys_open+0xe8/0x184) [ 8.872711] [<c00f3b6c>] (do_sys_open+0xe8/0x184) from [<c000db20>] (ret_fast_syscall+0x0/0x3c) [ 8.882019] ---[ end trace e9bf408c37051346 ]--- [2] commit 5d4121c04b3577e37e389b3553d442f44bb346d7 Author: Jiri Slaby <jslaby@suse.cz> Date: Fri Aug 17 14:27:52 2012 +0200 TTY: check if tty->port is assigned And if not, complain loudly. None in-kernel module should trigger that, but let us find out for sure. On the other hand, all the out-of-tree modules will hit that. Give them some time (maybe one release) to catch up. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hi, On Thu, Sep 06, 2012 at 03:44:13PM -0700, Kevin Hilman wrote: > Felipe Balbi <balbi@ti.com> writes: > > > Hi guys, > > > > here's v4 of the omap uart patchset. No changes other than a rebase on top of > > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > > > > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > > as it can just be ignored if it's decided it needs to go into this merge > > window. > > Sorry to be late to the party... just getting back from some time off. > > I'm assuming that this was not tested with PM, so decided I better do it you assumed wrong. See the previous versions of the series and you'll see I mention all the basic pm testing I did. > myself seeing that Greg is has already merge it. To test, I merged > Greg's tty-next branch with v3.6-rc4 and did some PM testing. > > The bad news is that it doesn't even compile (see reply to [PATCH v4 > 20/21]). yeah, that was an automerge issue when rebasing on greg's tty-next branch, plus me assuming omap serial was already enabled on my .config and not checking the compile output. Sent a patch now. > Also, there is a big WARNING on boot[1], which seems to be triggered by > a new check added for v3.6-rc3[2]. This appears to be introduced by > $SUBJECT series, because I don't see it on vanilla v3.6-rc4. > > The good news is that after hacking to fix up the compile problems, > basic PM testing seems to be fine: idle to retention and off as well as > suspend to retention and off work fine on 3430/n900, 3530/Overo, > 3730/OveroSTORM, 3730/Beagle-xM. > > Kevin > > > [1] > [ 8.745666] WARNING: at /work/kernel/omap/pm/drivers/tty/tty_io.c:1420 tty_init_dev+0x14c/0x17c() > [ 8.755218] tty_init_dev: ttyO driver does not set tty->port. This will crash the kernel later. Fix the driver! > [ 8.765991] Modules linked in: > [ 8.769287] [<c0013d90>] (unwind_backtrace+0x0/0xf0) from [<c0036eec>] (warn_slowpath_common+0x4c/0x64) > [ 8.779327] [<c0036eec>] (warn_slowpath_common+0x4c/0x64) from [<c0036f98>] (warn_slowpath_fmt+0x30/0x40) > [ 8.789550] [<c0036f98>] (warn_slowpath_fmt+0x30/0x40) from [<c02c626c>] (tty_init_dev+0x14c/0x17c) > [ 8.799224] [<c02c626c>] (tty_init_dev+0x14c/0x17c) from [<c02c68a4>] (tty_open+0x11c/0x52c) > [ 8.808258] [<c02c68a4>] (tty_open+0x11c/0x52c) from [<c00f86ac>] (chrdev_open+0x90/0x15c) > [ 8.817108] [<c00f86ac>] (chrdev_open+0x90/0x15c) from [<c00f2b74>] (do_dentry_open+0x1e8/0x270) > [ 8.826507] [<c00f2b74>] (do_dentry_open+0x1e8/0x270) from [<c00f2c30>] (finish_open+0x34/0x4c) > [ 8.835784] [<c00f2c30>] (finish_open+0x34/0x4c) from [<c0102028>] (do_last.isra.21+0x5b0/0xbbc) > [ 8.845184] [<c0102028>] (do_last.isra.21+0x5b0/0xbbc) from [<c01026dc>] (path_openat+0xa8/0x44c) > [ 8.854675] [<c01026dc>] (path_openat+0xa8/0x44c) from [<c0102d34>] (do_filp_open+0x2c/0x80) > [ 8.863708] [<c0102d34>] (do_filp_open+0x2c/0x80) from [<c00f3b6c>] (do_sys_open+0xe8/0x184) > [ 8.872711] [<c00f3b6c>] (do_sys_open+0xe8/0x184) from [<c000db20>] (ret_fast_syscall+0x0/0x3c) > [ 8.882019] ---[ end trace e9bf408c37051346 ]--- This doesn't seem to be caused by $SUBJECT at all. See that we are calling uart_add_one_port() which will call tty_port_register_device() which, in turn, will call tty_port_link_device() and that will set driver->ports[index] correctly. Have you checked if this doesn't happen without my series before waving your blame hammer ? FWIW, that part of the code wasn't change by $SUBJECT at all.
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Thu, Sep 06, 2012 at 03:44:13PM -0700, Kevin Hilman wrote: >> Felipe Balbi <balbi@ti.com> writes: >> >> > Hi guys, >> > >> > here's v4 of the omap uart patchset. No changes other than a rebase on top of >> > Greg's tty-next branch and Tony's Acked-by being added to a couple patches >> > >> > Note: I'm resending the series with Vikram's Software Flow Control fix anyway >> > as it can just be ignored if it's decided it needs to go into this merge >> > window. >> >> Sorry to be late to the party... just getting back from some time off. >> >> I'm assuming that this was not tested with PM, so decided I better do it > > you assumed wrong. See the previous versions of the series and you'll > see I mention all the basic pm testing I did. My apologies for not reading the previous versions. I don't think it's unusual that a reviewer should expect everything he to know about a series (including how it was tested) is in the cover letter or in the changelogs of the latest series. I don't expect to have to look through all the previous versions for this kind of info. Since I wasn't around to review/test the earlier versions, I just looked at the latest (v4) and didn't see any mention of testing of any sort in the cover letter. Looking back at the previous cover letters, I don't see any description of the PM testing either. I only see it was tested on pandaboard. Since mainline doesn't have full PM support for OMAP4, testing on panda doesn't really test UART PM at all. Could you please point me to the descriptions in earlier mails of how you did PM testing, and on what platforms? In addition, IMO, if this was only tested on Panda (as suggested by earlier cover letters), it really should not have been merged until it got some broader testing. >> myself seeing that Greg is has already merge it. To test, I merged >> Greg's tty-next branch with v3.6-rc4 and did some PM testing. >> >> The bad news is that it doesn't even compile (see reply to [PATCH v4 >> 20/21]). > > yeah, that was an automerge issue when rebasing on greg's tty-next > branch, plus me assuming omap serial was already enabled on my .config > and not checking the compile output. Sent a patch now. As I reported in my reply to [PATCH v4 20/21], that patch also had another problem where it introduced a new (but unused) field. Maybe another rebase problem? I see the same problem in v3 and v4. >> Also, there is a big WARNING on boot[1], which seems to be triggered by >> a new check added for v3.6-rc3[2]. This appears to be introduced by >> $SUBJECT series, because I don't see it on vanilla v3.6-rc4. [...] > This doesn't seem to be caused by $SUBJECT at all. See that we are > calling uart_add_one_port() which will call tty_port_register_device() > which, in turn, will call tty_port_link_device() and that will set > driver->ports[index] correctly. > > Have you checked if this doesn't happen without my series before waving > your blame hammer ? FWIW, that part of the code wasn't change by > $SUBJECT at all. Whoa. This was only test report. No need to get personal. All I said is that it "seemed" to introduced by $SUBJECT series. Hardly waiving "blame hammer." And yes, I did check without your series. As I reported above, the warning didn't exist with v3.6-rc4, and it did with yesterday's tty-next branch. The WARNING pointed a finger at ttyO (omap-serial) so I assumed it was in $SUBJECT series. Testing with todays tty-next, the problem is gone. The patch 'tty_register_device_attr updated for tty-next'[1] seems to have made the problem go away. So it's now clear that it wasn't introduced by $SUBJECT series. My bad. Yesterday, it wasn't that obvious, so I made an assumption in order to report a problem uncovered in my testing in the hopes that it would be helpful to you in fixing a potential problem. My assumption was wrong, I was wrong. I'm wrong a lot, and I'm OK with that. The bug was elsewhere, and is already fixed. My apologies if it seemed like I was blaming you. Kevin [1] Author: Tomas Hlavacek <tmshlvck@gmail.com> 2012-09-06 14:17:47 Committer: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 2012-09-06 14:40:18 Parent: 6915c0e487c822e2436683e14302c0b8a6155cc7 (tty: uartclk value from serial_core exposed to sysfs) Child: e36851d0fa94b0f7802b3cc80406dbd3ef4f2f16 (serial: omap: fix compile breakage) Branch: tmp/uart-test-2 Follows: v3.6-rc3 Precedes: tty_register_device_attr updated for tty-next Added tty_device_create_release() and bound to dev->release in tty_register_device_attr(). Added tty_port_register_device_attr() and used in uart_add_one_port() instead of tty_register_device_attr(). Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hi, On Fri, Sep 07, 2012 at 01:53:11PM -0700, Kevin Hilman wrote: > Felipe Balbi <balbi@ti.com> writes: > > > Hi, > > > > On Thu, Sep 06, 2012 at 03:44:13PM -0700, Kevin Hilman wrote: > >> Felipe Balbi <balbi@ti.com> writes: > >> > >> > Hi guys, > >> > > >> > here's v4 of the omap uart patchset. No changes other than a rebase on top of > >> > Greg's tty-next branch and Tony's Acked-by being added to a couple patches > >> > > >> > Note: I'm resending the series with Vikram's Software Flow Control fix anyway > >> > as it can just be ignored if it's decided it needs to go into this merge > >> > window. > >> > >> Sorry to be late to the party... just getting back from some time off. > >> > >> I'm assuming that this was not tested with PM, so decided I better do it > > > > you assumed wrong. See the previous versions of the series and you'll > > see I mention all the basic pm testing I did. > > My apologies for not reading the previous versions. I don't think it's > unusual that a reviewer should expect everything he to know about a > series (including how it was tested) is in the cover letter or in the > changelogs of the latest series. I don't expect to have to look through you've got a point there. My bad, should've kept series history on all revisions. > all the previous versions for this kind of info. Since I wasn't around > to review/test the earlier versions, I just looked at the latest (v4) > and didn't see any mention of testing of any sort in the cover letter. Well, fair enough. Maybe I wasn't too verbose, but here's what I did on panda: . check that console still works . check that IRQs increase as I type on console . check that pm_runtime suspend callback is called after 1 second of inactivity (with printk) . check that device resumes properly when I type on console again . enable UART wakeup (through sysfs) and 'echo mem > /sys/power/state' then check that I can wakeup from suspend and check that all powerdomains actually reached low power state > Looking back at the previous cover letters, I don't see any description > of the PM testing either. I only see it was tested on pandaboard. Yeah, initially I wasn't testing PM because UART wakeup was known to be broken, but then I decided to test and sent it as a reply to cover letter v2, then I forgot to keep the history on v3 and v4. My bad. > Since mainline doesn't have full PM support for OMAP4, testing on panda > doesn't really test UART PM at all. Fair enough. What's missing for omap4 panda ? I could reach suspend2ram with echo mem > /sys/power/state and wakeup from it. > Could you please point me to the descriptions in earlier mails of how > you did PM testing, and on what platforms? Though not on a cover letter, this is how I was testing from v2 and onwards: http://marc.info/?l=linux-omap&m=134555434407362&w=2 > In addition, IMO, if this was only tested on Panda (as suggested by > earlier cover letters), it really should not have been merged until it > got some broader testing. Shubhro's got his Tested-by tag. I believe he tested on beagleboard and omap4sdp. Shubhro, can you confirm which platforms you tested the UART patches ? cheers > >> myself seeing that Greg is has already merge it. To test, I merged > >> Greg's tty-next branch with v3.6-rc4 and did some PM testing. > >> > >> The bad news is that it doesn't even compile (see reply to [PATCH v4 > >> 20/21]). > > > > yeah, that was an automerge issue when rebasing on greg's tty-next > > branch, plus me assuming omap serial was already enabled on my .config > > and not checking the compile output. Sent a patch now. > > As I reported in my reply to [PATCH v4 20/21], that patch also had > another problem where it introduced a new (but unused) field. Maybe > another rebase problem? I see the same problem in v3 and v4. I'll check it out and make sure to delete any such unused fields. Thanks > >> Also, there is a big WARNING on boot[1], which seems to be triggered by > >> a new check added for v3.6-rc3[2]. This appears to be introduced by > >> $SUBJECT series, because I don't see it on vanilla v3.6-rc4. > > [...] > > > This doesn't seem to be caused by $SUBJECT at all. See that we are > > calling uart_add_one_port() which will call tty_port_register_device() > > which, in turn, will call tty_port_link_device() and that will set > > driver->ports[index] correctly. > > > > Have you checked if this doesn't happen without my series before waving > > your blame hammer ? FWIW, that part of the code wasn't change by > > $SUBJECT at all. > > Whoa. This was only test report. No need to get personal. All I said > is that it "seemed" to introduced by $SUBJECT series. Hardly waiving > "blame hammer." fair enough. > And yes, I did check without your series. As I reported above, the What about v3.6-rc4 + the patch which added the warning ? :-) > warning didn't exist with v3.6-rc4, and it did with yesterday's tty-next > branch. The WARNING pointed a finger at ttyO (omap-serial) so I assumed > it was in $SUBJECT series. > > Testing with todays tty-next, the problem is gone. The patch > 'tty_register_device_attr updated for tty-next'[1] seems to have made > the problem go away. So it's now clear that it wasn't introduced by > $SUBJECT series. My bad. Thankfully... > Yesterday, it wasn't that obvious, so I made an assumption in order to > report a problem uncovered in my testing in the hopes that it would be > helpful to you in fixing a potential problem. My assumption was wrong, I > was wrong. I'm wrong a lot, and I'm OK with that. The bug was > elsewhere, and is already fixed. > > My apologies if it seemed like I was blaming you. I'm the one who owes you an apology for misunderstanding your bug report. I'm sorry.
On Sunday 09 September 2012 12:34 AM, Felipe Balbi wrote: >> In addition, IMO, if this was only tested on Panda (as suggested by >> > earlier cover letters), it really should not have been merged until it >> > got some broader testing. > Shubhro's got his Tested-by tag. I believe he tested on beagleboard and > omap4sdp. Shubhro, can you confirm which platforms you tested the UART > patches ? cheers Felipe I had boot tested on omap3sdp and onap4 sdp and did pm testing on both with omap3 hitting off.
On Tue, Sep 11, 2012 at 05:01:06PM +0530, Shubhrajyoti wrote: > On Sunday 09 September 2012 12:34 AM, Felipe Balbi wrote: > >> In addition, IMO, if this was only tested on Panda (as suggested by > >> > earlier cover letters), it really should not have been merged until it > >> > got some broader testing. > > Shubhro's got his Tested-by tag. I believe he tested on beagleboard and > > omap4sdp. Shubhro, can you confirm which platforms you tested the UART > > patches ? cheers > Felipe I had boot tested on omap3sdp and onap4 sdp and did pm testing > on both with omap3 hitting off. Thanks ;-)
Hi Felipe
Just tested these OMAP serial changes at commit
e36851d0fa94b0f7802b3cc80406dbd3ef4f2f16 ("serial: omap: fix compile
breakage"). There's good news and bad news...
The good news is that after applying this series, the 'OMAP4 UART garbage
on long transmit buffers when PM is enabled' bug:
http://www.pwsan.com/omap/testlogs/test_v3.6-rc5/20120908202511/pm/4430es2panda/4430es2panda_log.txt
... goes away. (Unclear if it's because the underlying bug was fixed, or
if unrelated changes are masking it.) Not sure what caused the problem to
go away exactly, but it's one of the changes between d37c6ceb and bf63a08.
So that's good!
The bad news is that N800 no longer boots -- or the UART dies during
serial init:
http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt
The problem doesn't seem to affect the 2430SDP.
Could you put together a patch to fix N800?
regards
- Paul
Hi, On Tue, Sep 11, 2012 at 10:02:48PM +0000, Paul Walmsley wrote: > > Hi Felipe > > Just tested these OMAP serial changes at commit > e36851d0fa94b0f7802b3cc80406dbd3ef4f2f16 ("serial: omap: fix compile > breakage"). There's good news and bad news... > > The good news is that after applying this series, the 'OMAP4 UART garbage > on long transmit buffers when PM is enabled' bug: > > http://www.pwsan.com/omap/testlogs/test_v3.6-rc5/20120908202511/pm/4430es2panda/4430es2panda_log.txt > > ... goes away. (Unclear if it's because the underlying bug was fixed, or > if unrelated changes are masking it.) Not sure what caused the problem to > go away exactly, but it's one of the changes between d37c6ceb and bf63a08. > So that's good! I'd point the finger at sticking to put_autosuspend() as that would give HW enough time to actually unload all bytes from its FIFO through the UART lines. can you check if forcing the autosuspend_delay to -1 triggers the issue again ? If it does, it's some help from autosuspend, which would mean we still have the problem and needs to be properly fixed. > The bad news is that N800 no longer boots -- or the UART dies during > serial init: > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt > > The problem doesn't seem to affect the 2430SDP. > > Could you put together a patch to fix N800? I'll see what I can do. BTW, is that log with DEBUG_LL enabled ? cheers
Hi On Wed, 12 Sep 2012, Felipe Balbi wrote: > On Tue, Sep 11, 2012 at 10:02:48PM +0000, Paul Walmsley wrote: > > > The bad news is that N800 no longer boots -- or the UART dies during > > serial init: > > > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt > > > > The problem doesn't seem to affect the 2430SDP. > > > > Could you put together a patch to fix N800? > > I'll see what I can do. BTW, is that log with DEBUG_LL enabled ? Yes. Here's the .config that was used on N800: http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/build/n800_b_testconfig/Kconfig - Paul
On Wed, Sep 12, 2012 at 08:25:30PM +0000, Paul Walmsley wrote: > Hi > > On Wed, 12 Sep 2012, Felipe Balbi wrote: > > > On Tue, Sep 11, 2012 at 10:02:48PM +0000, Paul Walmsley wrote: > > > > > The bad news is that N800 no longer boots -- or the UART dies during > > > serial init: > > > > > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt > > > > > > The problem doesn't seem to affect the 2430SDP. > > > > > > Could you put together a patch to fix N800? > > > > I'll see what I can do. BTW, is that log with DEBUG_LL enabled ? > > Yes. Here's the .config that was used on N800: > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/build/n800_b_testconfig/Kconfig Thanks :-)
On Wed, 12 Sep 2012, Felipe Balbi wrote: > On Tue, Sep 11, 2012 at 10:02:48PM +0000, Paul Walmsley wrote: > > > The bad news is that N800 no longer boots -- or the UART dies during > > serial init: > > > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt > > > > The problem doesn't seem to affect the 2430SDP. > > > > Could you put together a patch to fix N800? Bisected this down. It's this one that causes the problem on N800: commit 93220dcc3052182e7156c09655ad1316055564b9 Author: Felipe Balbi <balbi@ti.com> Date: Thu Sep 6 15:45:27 2012 +0300 serial: omap: set dev->drvdata before enabling pm_runtime by the time we call our first pm_runtme_get_sync() after enable pm_runtime, our resume method might be called. To avoid problems, we must make sure that our dev->drvdata is set correctly before our resume method gets called. Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Signed-off-by: Felipe Balbi <balbi@ti.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> - Paul
Hi, On Sun, Sep 16, 2012 at 01:22:01AM +0000, Paul Walmsley wrote: > On Wed, 12 Sep 2012, Felipe Balbi wrote: > > > On Tue, Sep 11, 2012 at 10:02:48PM +0000, Paul Walmsley wrote: > > > > > The bad news is that N800 no longer boots -- or the UART dies during > > > serial init: > > > > > > http://www.pwsan.com/omap/testlogs/test_tty_next_e36851d0/20120910020323/boot/2420n800/2420n800_log.txt > > > > > > The problem doesn't seem to affect the 2430SDP. > > > > > > Could you put together a patch to fix N800? > > Bisected this down. It's this one that causes the problem on N800: > > commit 93220dcc3052182e7156c09655ad1316055564b9 > Author: Felipe Balbi <balbi@ti.com> > Date: Thu Sep 6 15:45:27 2012 +0300 > > serial: omap: set dev->drvdata before enabling pm_runtime > > by the time we call our first pm_runtme_get_sync() > after enable pm_runtime, our resume method might > be called. To avoid problems, we must make sure > that our dev->drvdata is set correctly before > our resume method gets called. > > Tested-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Felipe Balbi <balbi@ti.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Interesting. That simply moves platform_set_drvdata() to a saner location... The only way for this to cause problems is if we're trying to restore a context which was never saved. Is there a way to prevent runtime_resume() to be called during probe() if I know the HW is already enabled ? Maybe with pm_runtime_set_active() ?
Hi guys, here's v3 and hopefully final version of this series. A whole bunch of new patches added but the good thing is that now I had another engineer's help to test, so he's got his Tested-by in all patches. Changes since v2: . Added a bunch of new patches . Fixed a problem where we would always return IRQ_NONE even though we handled IRQ Changes since v1: . improved commit log on patch 9/13 (formerly 10/13) . removed patch 2/13 . added a new patch switching from spin_lock_irqsave() to spin_lock and spin_unlock_irqrestore to spin_unlock Alan, if you prefer in pull request form, here it is: The following changes since commit d9875690d9b89a866022ff49e3fcea892345ad92: Linux 3.6-rc2 (2012-08-16 14:51:24 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git uart for you to fetch changes up to a29230f14d8466c9b8c25171715378bf52189453: serial: omap: enable RX and TX FIFO usage (2012-08-23 09:25:16 +0300) ---------------------------------------------------------------- Felipe Balbi (20): serial: omap: define and use to_uart_omap_port() serial: omap: define helpers for pdata function pointers serial: omap: don't access the platform_device serial: omap: drop DMA support serial: add OMAP-specific defines serial: omap: simplify IRQ handling serial: omap: refactor receive_chars() into rdi/rlsi handlers serial: omap: move THRE check to transmit_chars() serial: omap: stick to put_autosuspend serial: omap: set dev->drvdata before enabling pm_runtime serial: omap: drop unnecessary check from remove serial: omap: make sure to suspend device before remove serial: omap: don't save IRQ flags on hardirq serial: omap: optimization with section annotations serial: omap: drop "inline" from IRQ handler prototype serial: omap: implement set_wake serial: omap: make sure to put() on poll_get_char serial: omap: remove unnecessary header and add a missing one serial: omap: move uart_omap_port definition to C file serial: omap: enable RX and TX FIFO usage Ruchika Kharwar (2): serial: omap: fix sequence of pm_runtime_* calls. serial: omap: unlock the port lock Vikram Pandita (1): serial: omap: fix software flow control arch/arm/mach-omap2/serial.c | 15 +- arch/arm/plat-omap/include/plat/omap-serial.h | 47 +- drivers/tty/serial/omap-serial.c | 808 ++++++++++---------------- include/linux/serial_reg.h | 4 + 4 files changed, 330 insertions(+), 544 deletions(-)