On Fri, Jul 18, 2014 at 10:35:10AM +0200, Sebastian Andrzej Siewior wrote: > On 07/17/2014 06:18 PM, Felipe Balbi wrote: > > >> No, this is okay. If you look, it checks for "up->ier & > >> UART_IER_THRI". On the second invocation it will see that this > >> bit is already set and therefore won't call get_sync() for the > >> second time. That bit is removed in the _stop_tx() path. > > > > oh, right. But that's actually unnecessary. Calling > > pm_runtime_get() multiple times will just increment the usage > > counter multiple times, which means you can call __stop_tx() > > multiple times too and everything gets balanced, right ? > > No. start_tx() will be called multiple times but only the first > invocation invoke pm_runtime_get(). Now I noticed that I forgot to
right, but that's unnecessary. You can pm_runtime_get() every time start_tx() is called. Just make sure to put everytime stop_tx() is called too. > remove pm_runtime_put_autosuspend() at the bottom of it. But you get > the idea right? > pm_get() on the while the UART_IER_THRI is not yet set. pm_put() once > the fifo is completely empty. > > >> Do you have other ideas? It doesn't look like this is exported at > >> all. If we call _stop_tx() right away, then we have 64 bytes in > >> the TX fifo in the worst case. They should be gone "soon" but the > >> HW-flow control may delay it (in theory for a long time)). > > > > this can be problematic, specially for OMAP which can go into OFF > > while idle. Whatever is in the FIFO would get lost. It seems like > > omap-serial solved this within transmit_chars(). > > No, it didn't. > > > See how transmit_chars() is called from within IRQ handler with > > clocks enabled then it conditionally calls serial_omap_stop_tx() > > which will pm_runtime_get_sync() -> do_the_harlem_shake() -> > > pm_runtime_put_autosuspend(). That leaves one unbalanced > > pm_runtime_get() which is balanced when we're exitting the IRQ > > handler. > > omap-serial and the 8250 do the following on tx path: > - start_tx() > -> sets UART_IER_THRI. This will generate an interrupt once the FIFO > is empty. > - interrupt, notices the empty fifo, invokes serial8250_start_tx()/ > transmit_chars(). > Both have a while loop that fills the FIFO. This loop is left once > the tty-buffer is empty (uart_circ_empty() is true) or the FIFO full. > > Lets say you filled 64 bytes into the FIFO and then left because your > FIFO is full and tty-buffer is empty. That means you will invoke > serial_omap_stop_tx() and remove UART_IER_THRI bit. > This is okay because you are not interested in further FIFO empty > interrupts because you don't have any TX-bytes to be sent. However, > once you leave the transmit_chars() you leave serial_omap_irq() which > does the last pm_put(). That means you have data in the TX FIFO that is > about to be sent and the device is in auto-suspend. > This is "fine" as long as the timeout is greater then the time required > for the data be sent (plus assuming HW-float control does not stall it > for too long) so nobody notices a thing. the time is set to -1 by default. I guess this only works because nobody has ever tested long transfers with slow baud rates :-p > For that reason I added the hack / #if0 block in the 8250 driver. To > ensure we do not disable the TX-FIFO-empty interrupt even if there is > nothing to send. Instead we enter serial8250_tx_chars() once again with > empty FIFO and empty tty-buffer and will invoke _stop_tx() which also > finally does the pm_put(). > That is the plan. The problem I have is how to figure out that the > device is using auto-suspend. If I don't then I would have to remove > the #if0 block and that would mean for everybody an extra interrupt > (which I wanted to avoid). looks like the closest you have is: if (pm_runtime_autosuspend_expiration(dev) > 0) foo(); Another possibility would be to implement the ->runtime_idle() callback and only return 0 if fifo is empty, otherwise return -EAGAIN ? then, if the autosuspend timer expires, ->runtime_idle gets called and you can do the right thing depending on fifo empty or not. Take a look at drivers/usb/core/driver.c::usb_runtime_{idle,resume,suspend} for examples. That seems to work pretty well. > > This seems work fine and dandy without DMA, but for DMA work, I > > think we need to make sure this IP stays powered until we get DMA > > completion callback. But that's future, I guess. > > Yes, probably. That means one get at dma start, one put at dma complete > callback. And I assume we get that callbacks once the DMA transfer is > complete, not when the FIFO is empty :) So lets leave it to the future > for now… k -- balbi
signature.asc
Description: Digital signature