Re: [linux-sunxi] Designware UART bug

[Tim Kryger]

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.

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