From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752299AbdECPk3 (ORCPT ); Wed, 3 May 2017 11:40:29 -0400 Received: from 7of9.schinagl.nl ([62.251.20.244]:46716 "EHLO 7of9.schinagl.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751601AbdECPkW (ORCPT ); Wed, 3 May 2017 11:40:22 -0400 Subject: Re: [linux-sunxi] Designware UART bug To: Tim Kryger References: <86057f94-3b52-7c12-21b5-be90564fcf85@schinagl.nl> Cc: Chen-Yu Tsai , Jamie Iles , Tim Kryger , "linux-kernel@vger.kernel.org" , dev , Maxime Ripard From: Olliver Schinagl Message-ID: <0ea81628-3df9-3420-235d-da7165423308@schinagl.nl> Date: Wed, 3 May 2017 17:40:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hey Tim, On 03-05-17 16:22, Tim Kryger wrote: > On Wed, May 3, 2017 at 4:26 AM, Olliver Schinagl 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 >>> 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@googlegroups.com. >>>> For more options, visit https://groups.google.com/d/optout.