Re: [linux-sunxi] Designware UART bug

[Olliver Schinagl]

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].

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.