On Tue, 17 Mar 2015 13:58:44 +0000, Dave P Martin wrote:
> On Mon, Mar 16, 2015 at 11:15:29PM +0000, Jakub Kicinski wrote:
> > From: Jakub Kicinski <kubakici@xxxxx>
> >
> > Since pre-git era PL011 used an elaborate scheme to load data
> > to TX FIFO. Only TX IRQ handler was loading data into the FIFO,
> > which required the IRQ to fire before any transmission started
> > (to load the first batch of characters). Initial IRQ was fired
> > by putting UART into loopback mode and writing an arbitrary
> > character during .startup().
> >
> > Unfortunately some PL011-compatible UART (most notably BCM2708
> > in Raspberry Pi) would transmit the arbitrary character even
> > though the device was in loopback mode. Commit 734745caeb9f
> > ("serial/amba-pl011: Activate TX IRQ passively") solved this
> > issue by loading the first batch explicitly from .start_tx()
> > handler. It employed quite a complex scheme involving IRQ
> > counting and a delayed work.
>
> It is indeed overcomplicated -- I reposted a simplified version last
> week, see:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330619.html
>
> (The message threading is broken on the server due to the patches being
> received out of order -- there are two patches following the cover
> letter the link points to.)
I'm a bit confused by this. The first version of the patch seems to be
already present in Greg's tty-next [1] as well as linux-next. However,
the v2 of your patch which you pointed me to is based on tty-next
without it applied. I did see the v2 but assumed the version already
committed to the repository is the final one...
How would this be solved? Are rebases allowed in tty-next?
> Can you try to rebase?
>
> My update still keeps the softirq stuff. I wanted to avoid adding
> status polling inside the interrupt handler's core loop, due to
> concerns about performance overhead: without the polling, the
> writes to DR are fire-and-forget, whereas polling FR each time
> involves a whole round-trip to the UART which may involve significant
> extra time cost and/or IRQ disable latency; however.
>
> Your approach does mitigate some of the cost by only starting to
> poll after count chars have been transmitted, and will typically
> halve the number of IRQs taken -- which could lead to a net benefit.
> It would be good to see some benchmarks to understand how much
> difference it makes to performance.
I will *try* to come up with some figures but I don't have any
high-speed UARTs so my tests run at 115k tops. I think dropping the FF
poll at the end of load from IRQ could be a good idea. And I have to
respin anyway because I just noticed I added a double new line in
pl011_tx_chars()...
> Looping in pl011_tx_chars() until either the FIFO is full or
> we run out of chars to transmit gets rid of the need for the
> softirq, so I wouldn't be opposed to keeping that if the
> performance impact is not too significant.
TBH the softirq stuff I don't quite understand. I had never seen it
fire in my tests. Can you explain its purpose? Is there HW out there
which fails to fire the IRQ (but it wouldn't work until now anyhow)?
> > f2ee6dfa0e85 ("serial/amba-pl011: Leave the TX IRQ alone when
> > the UART is not open") was an attempt to optimise the loading
> > by assuming that when the device is opened second time TX IRQ
> > from the previous transmission will still be pending. This
> > assumption is incorrect if the device is closed with FIFO full
> > because FIFO will be programmatically flushed and therefore no
> > IRQ will be pending on next .open().
>
> Have you actually observed this as a bug?
>
> My understanding is that because closing the port explicitly
> drains the FIFO, that will leave the TX interrupt asserted.
> This is just the same as what happens when the driver runs out
> of data to transmit while it is open.
>
> On port shutdown, the TX interrupt will be masked by IMSC, but should
> remain asserted inside the PL011, triggering an interrupt the next time
> it is unmasked.
>
> If you observed something that conflicts with these assumptions,
> I'd be interested to know what's going on...
Actually seeing this bug in practice was what motivated this work.
I triggered it by catting a long file over the UART and then hitting
^C. UART died every time... That being said I test this on BCM2708
which is also notorious for transmitting while device in loopback
configuration, so perhaps it's implementation-dependent...
[1]
https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=734745caeb9f155ab58918834a8c70e83fa6afd3
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
