From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751836AbdECL06 (ORCPT ); Wed, 3 May 2017 07:26:58 -0400 Received: from 7of9.schinagl.nl ([62.251.20.244]:41692 "EHLO 7of9.schinagl.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751990AbdECL0o (ORCPT ); Wed, 3 May 2017 07:26:44 -0400 Subject: Re: [linux-sunxi] Designware UART bug To: Chen-Yu Tsai References: <86057f94-3b52-7c12-21b5-be90564fcf85@schinagl.nl> Cc: jamie@jamieiles.com, tim.kryger@linaro.org, "linux-kernel@vger.kernel.org" , dev , Maxime Ripard From: Olliver Schinagl Message-ID: Date: Wed, 3 May 2017 13:26:37 +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 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]. 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.