Re: [linux-sunxi] Designware UART bug

[Olliver Schinagl]

Hey Tim,

On 03-05-17 16:22, Tim Kryger wrote:
> On Wed, May 3, 2017 at 4:26 AM, Olliver Schinagl <oliver@xxxxxxxxxxx> wrote:
> > Hey Chen-Yu
> >
> > On 03-05-17 12:40, Chen-Yu Tsai wrote:
> > > On Wed, May 3, 2017 at 6:17 PM, Olliver Schinagl <oliver@xxxxxxxxxxx>
> > > wrote:
> > > > Hey Jamie,
> > > >
> > > > Several years ago you wrote the glue-code [0] for the DW 8250 IP. Over
> > > > the
> > > > years various 'fixes' have been applied to resolve certain 'weird'
> > > > problems
> > > > that Tim tried to fix with [1].
> > > >
> > > > After going over the datasheets and code with a comb several times now, I
> > > > think I may have found one (of a few others) reasons and would like both
> > > > your and Tim's thoughts here.
> > > >
> > > > The current (and original) code [2] uses the register offset 0x1f for the
> > > > UART_USR register.
> > > >
> > > > I searched far and wide, various datasheet of physical uarts (8250 -
> > > > 16950)
> > > > and some IP cores and none seem to have the USR (and specifically the
> > > > USR[0]
> > > > bit) register, so it seems to be specific to the DW_apb_uart. However
> > > > looking at the only databook available to me [3] of the UART IP, I cannot
> > > > seem to find anything at register offset 0x1f.
> > > >
> > > > The platform I'm using uses the Allwinner A20 SoC, which also features
> > > > the
> > > > DW uart IP and also here, there is nothing at offset 0x1f.
> > > >
> > > > The intended register IS however actually at 0x7c.
> > > >
> > > > My question is thus twofold.
> > > >
> > > > Why was 0x1f used? Is this specific to a certain (version) UART or is
> > > > this a
> > > > long uncaught typo.
> > >
> > >
> > > The original 8250 datasheets have registers at byte offsets. Note that the
> > > registers are only 8 bits wide.
> >
> > Yes, I did account for that difference :) Or rather, it should be the first
> > register after the scratchpad, but there is nothing after the scratchpad on
> > the 8250's.
> > > On Allwinner, and many other platforms, the registers are still 8 bits
> > > wide, but are 32bit-aligned, likely for aligned bus access. 0x7c = 0x1f <<
> > > 2.
> > > The 8250 core accessor functions are supposed to handle this for you.
> >
> > Actually, they are 32 bit registers, but only the lowest 8 are used, to
> > remain code-compatible with true 8 bitters. The end result, is the same of
> > course.
> >
> > As for the shift, I see now!
> >
> > readb(p->membase + (offset << p->regshift));
> >
> > And indeed, the regular defines are all indeed 8 bit offsets. Oversight on
> > my part, and I changed the comment to make this slightly more clear, which
> > goes into my greater uart series.
> >
> > Thanks Chen-Yu for pointing this oversight of mine out!
> >
> > > ChenYu
> > >
> > > > Tim, assuming it is a typo, could this the cause which made you implement
> > > > [1]? From what I understand, you keep writing the LCR until it takes.
> >
> > So with ChenYu's explanation, the USR register holds a valid status, but it
> > was and is never checked and thus the below part is still a valid question?
> >
> > > > Initially, the UART_IIR_BUSY check looked like this:
> > > >         if (serial8250_handle_irq(p, iir)) {
> > > >                 return 1;
> > > >         } else if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
> > > >                 /* Clear the USR and write the LCR again. */
> > > >                 (void)p->serial_in(p, d->usr_reg);
> > > >                 p->serial_out(p, UART_LCR, d->last_lcr);
> > > >
> > > >                 return 1;
> > > >         }
> > > >
> > > > what I'm missing here is, that if UART_IIR_BUSY is set, we have to:
> > > > * check the d->usr_reg (UART_USR) bit 0
> > > > * wait until it becomes cleared (do not allow new data to be pushed out,
> > > > optionally force the data to be pushed out after a timeout)
> > > > * write LCR register (and check if it took (and no longer loop over the
> > > > LCR
> > > > to see if the values stuck, in theory)).
> > > > * if we never get un-busy, something is wrong?
> > > >
> > > > All of this btw is currently moot anyway, as the only way to get into the
> > > > (else) if, is if serial8250_handle_irq returns false. And from what I can
> > > > see, this is only if iir == UART_IIR_NO_IRQ, in which case we never ever
> > > > clear the USR and thus never ever clear the UART_IIR_BUSY flag.
> >
> > especially this point is important I suppose, hence the need for the
> > workaround [1].
>
> I'm not clear on what question you are actually asking here.
>
> The original code didn't work since multiple register writes may occur
> after the initial failed LCR write while interrupts are still disabled.
>
> Thus the driver must read back LCR to ensure writes are going through
> immediately rather than rely upon an interrupt that can be delayed.

Ok, so as far as I understand (from the datasheet) the intended way to 
do this would be to check for the BUSY IRQ & USR[0] IRQ and if it is 
busy, (re-write) the LCR. We no longer do this because it did not work 
due to the delayed interrupt.

So one question is, why are we not checking the USR[0] reg whilst doing 
the loop? Is that register not there for exactly that purpose? But I 
guess brute-forcing it works more reliable I suppose then.

But secondly, when is the UART_IIR_BUSY interrupt handled? And it not 
being handled, could that be the actual reason things where failing? (Or 
as I read somewhere a silicon bug?)

Right now, we have serial8250_handle_irq() and if that returns 0, we 
check if we (still) have an unhandled UART_IIR_BUSY interrupt.
But the only way for serial8250_handle_irq() to return 0, is if it has 
set UART_NO_INT.

If we do not have an IRQ, i'm pretty sure, UART_IIR_BUSY can't be 
triggered right? And if it IS the interrupt that caused us to go into 
the interrupt handler, well, handle_irq does its thing and then returns 
1 for finishing it, which in turn causes the UART_IIR_BUSY check to be 
skipped.

So we never clear the UART_IIR_BUSY interrupt if we manage to trigger 
that. (Please do correct me if I'm wrong.


I'm changing things in the interrupt handler a bit now to first check 
for the busy interrupt first and if that is triggered do the dummy 
return (clear it) and return (since LCR is handled alternativly.

Olliver
> > Olliver
> >
> > > > Olliver
> > > >
> > > >
> > > > [0]
> > > >
> > > > https://github.com/torvalds/linux/commit/6b1a98d1c4851235d9b6764b3f7b7db7909fc760
> > > >
> > > > [1]
> > > >
> > > > https://github.com/torvalds/linux/commit/c49436b657d0a56a6ad90d14a7c3041add7cf64d
> > > >
> > > > [2]
> > > >
> > > > https://github.com/torvalds/linux/commit/6b1a98d1c4851235d9b6764b3f7b7db7909fc760#diff-d6e619fc4c51febf7880632fde5d0208R63
> > > >
> > > > [3] http://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google Groups
> > > > "linux-sunxi" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send an
> > > > email to linux-sunxi+unsubscribe@xxxxxxxxxxxxxxxxx
> > > > For more options, visit https://groups.google.com/d/optout.