Message ID | 20221013112339.2540767-1-matthias.schiffer@ew.tq-group.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp218274wrs; Thu, 13 Oct 2022 04:28:37 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7I9koDTG0I1VZPaPxCPL/X29hTHbhofg4uz/ubfobk13HcJFowqxiiuMdJGSvBaIksE1Kc X-Received: by 2002:a63:c111:0:b0:439:103a:6c31 with SMTP id w17-20020a63c111000000b00439103a6c31mr29939515pgf.149.1665660517219; Thu, 13 Oct 2022 04:28:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1665660517; cv=none; d=google.com; s=arc-20160816; b=Uk/lilrJiewnfDtuMk0VHw9OeIsCHBbMFnd2uDuoTv1id612Z5UdjK70QrBoyX+yfK TmKskiYFHYalJwQi8tLsFMfXbM5bRDPUPNBROh4vRYSQPgZ3/A8EEehSXUEgVH+ZJLxz qiib2JjRBETYXoWPN3b0AfnRRIzLLBmGNGU8NQnph1at3MMG04R+qDeLZg7oUSGwFHxl 6RsGU471n5p+hpDcFKvV6nsljabtZltvpGVmfLGlmMtFKatHSyLGiBxHtWv8rCX/bJqL r37D2JupMf//B9ICwRivgv6vWgOw/qlJdY5iZs8khW5JRi15pFgcuAZClTABr4+tqMRM Tp/Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature:dkim-signature; bh=+EOiI+S0zJYFFWtrblP4l1n4aHRxTG4VL1X65gIW8tM=; b=D9qNPVdu+ieijuA2NAEJaGC8hJ3QS8QGqOP2D6MxbwiP1IDWI5UUGem0BAKptTq+4a 3MiE2vd7ePjRtAMb8mIn05+aSR8Llk1VbIv5Ar9L4COStQZcZDCF6ZwMWoUjkwzdj8EV WFMSk5zY9P9EEvWqkf/7HIZ6NQ0y3HER1Rx3kSG9Kerqbr8E/ISKBUYkV9MG9qoKja70 U26NWlFqIudnVyqmuyBHh8DGj2YWRzoX1HXZ2o/31wMRSnoboOP7PMKjx98RcNVxQboG yAieoz/U/YRrO+Evf9xIhHCKdsLpcg3hi2jwmiHABhya9o5YB92U7o/a8fYffWQ2jOWc fnPw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@tq-group.com header.s=key1 header.b=UmITO7wM; dkim=pass header.i=@tq-group.com header.s=key1 header.b=hgqRZ8R3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=tq-group.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f190-20020a6251c7000000b00544ce272399si19782576pfb.173.2022.10.13.04.28.24; Thu, 13 Oct 2022 04:28:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@tq-group.com header.s=key1 header.b=UmITO7wM; dkim=pass header.i=@tq-group.com header.s=key1 header.b=hgqRZ8R3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=tq-group.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229813AbiJMLYq (ORCPT <rfc822;ouuuleilei@gmail.com> + 99 others); Thu, 13 Oct 2022 07:24:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50092 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229788AbiJMLYa (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 13 Oct 2022 07:24:30 -0400 Received: from mx1.tq-group.com (mx1.tq-group.com [93.104.207.81]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 82599127415; Thu, 13 Oct 2022 04:24:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tq-group.com; i=@tq-group.com; q=dns/txt; s=key1; t=1665660259; x=1697196259; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=+EOiI+S0zJYFFWtrblP4l1n4aHRxTG4VL1X65gIW8tM=; b=UmITO7wMOAoaOR2Y/IWrnfc55Iy1HTTBRCKIVlUlvMxNNG3nzvcLNJuc i4nO7GgESfNiROQOrYemklHi4cypw2jucY5mYdCPpoAlHLzaEOVGydQ1u W/RO/6T51COU832YEyPkXFLEwpVFqgNwH9fIq6zUnWmxFFfgKhlGf9oqA wf+HII6Vztpls5cJVNuVlBhTeox/ewDSQUK1PyNHCfqJrUrgoehUZuL0k h+Gdn+onZRacA2qmPuIzTYW5LdYyv4U9K8w83WLJqy7ZTe7nGqbF3EbQK Plwjd1Tb2f7RCAK/gUlHdbPgJuZXynbTbaR4hcKz++N5n9DKmCtg5Zc2u A==; X-IronPort-AV: E=Sophos;i="5.95,180,1661810400"; d="scan'208";a="26733157" Received: from unknown (HELO tq-pgp-pr1.tq-net.de) ([192.168.6.15]) by mx1-pgp.tq-group.com with ESMTP; 13 Oct 2022 13:24:15 +0200 Received: from mx1.tq-group.com ([192.168.6.7]) by tq-pgp-pr1.tq-net.de (PGP Universal service); Thu, 13 Oct 2022 13:24:15 +0200 X-PGP-Universal: processed; by tq-pgp-pr1.tq-net.de on Thu, 13 Oct 2022 13:24:15 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tq-group.com; i=@tq-group.com; q=dns/txt; s=key1; t=1665660255; x=1697196255; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=+EOiI+S0zJYFFWtrblP4l1n4aHRxTG4VL1X65gIW8tM=; b=hgqRZ8R34wzygRlc+e+bnD0T0FRAlIJKwZibw0xdlKNunIRlZfxiG+XE sM7VsvHcGT9QznXJLazyG5jIwsZT36Yy3Xb/OpoqAbPD0KebIAcV5TiHE /bXqeMsctH7TP/H+JuS1V+wvpr1W7SZ/CnER3Gnbaao493dWoaqESi7Qa 1xc+EvjOEqjW8gUXa0evQKWZr/belDOI+g3SKqn3O3bnYRfHmlaEZeT6J 3IOV2TI9xKq+I/OKKU1AG40LKzcLZ0udAIRV8UEr/ey65bGJZBhIJ4oxz 4SZUfzz34rNjrlbvCSBRmOZhApc/aj7RmTLau797Dcl9PXyDCcr7Cq4Re w==; X-IronPort-AV: E=Sophos;i="5.95,180,1661810400"; d="scan'208";a="26733156" Received: from vtuxmail01.tq-net.de ([10.115.0.20]) by mx1.tq-group.com with ESMTP; 13 Oct 2022 13:24:15 +0200 Received: from localhost.localdomain (SCHIFFERM-M2.tq-net.de [10.121.49.14]) by vtuxmail01.tq-net.de (Postfix) with ESMTPA id 7034B280056; Thu, 13 Oct 2022 13:24:15 +0200 (CEST) From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jirislaby@kernel.org>, =?utf-8?q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Tony Lindgren <tony@atomide.com>, Peter Hurley <peter@hurleysoftware.com>, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Matthias Schiffer <matthias.schiffer@ew.tq-group.com> Subject: [PATCH] serial: 8250_omap: remove wait loop from Errata i202 workaround Date: Thu, 13 Oct 2022 13:23:39 +0200 Message-Id: <20221013112339.2540767-1-matthias.schiffer@ew.tq-group.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1746571642478075740?= X-GMAIL-MSGID: =?utf-8?q?1746571642478075740?= |
Series |
serial: 8250_omap: remove wait loop from Errata i202 workaround
|
|
Commit Message
Matthias Schiffer
Oct. 13, 2022, 11:23 a.m. UTC
We were occasionally seeing the "Errata i202: timedout" on an AM335x
board when repeatedly opening and closing a UART connected to an active
sender. As new input may arrive at any time, it is possible to miss the
"RX FIFO empty" condition, forcing the loop to wait until it times out.
Nothing in the i202 Advisory states that such a wait is even necessary;
other FIFO clear functions like serial8250_clear_fifos() do not wait
either. For this reason, it seems safe to remove the wait, fixing the
mentioned issue.
Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
drivers/tty/serial/8250/8250_omap.c | 17 -----------------
1 file changed, 17 deletions(-)
Comments
On Thu, 13 Oct 2022, Matthias Schiffer wrote: > We were occasionally seeing the "Errata i202: timedout" on an AM335x > board when repeatedly opening and closing a UART connected to an active > sender. As new input may arrive at any time, it is possible to miss the > "RX FIFO empty" condition, forcing the loop to wait until it times out. I can see this problem could occur and why your patch fixes it. > Nothing in the i202 Advisory states that such a wait is even necessary; > other FIFO clear functions like serial8250_clear_fifos() do not wait > either. For this reason, it seems safe to remove the wait, fixing the > mentioned issue. Checking the commit that added this driver and the loop along with it, there was no information why it would be needed there either. Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Thanks.
On 2022-10-17 11:12:41 [+0300], Ilpo Järvinen wrote: > On Thu, 13 Oct 2022, Matthias Schiffer wrote: > > > We were occasionally seeing the "Errata i202: timedout" on an AM335x > > board when repeatedly opening and closing a UART connected to an active > > sender. As new input may arrive at any time, it is possible to miss the > > "RX FIFO empty" condition, forcing the loop to wait until it times out. > > I can see this problem could occur and why your patch fixes it. > > > Nothing in the i202 Advisory states that such a wait is even necessary; > > other FIFO clear functions like serial8250_clear_fifos() do not wait > > either. For this reason, it seems safe to remove the wait, fixing the > > mentioned issue. > > Checking the commit that added this driver and the loop along with it, > there was no information why it would be needed there either. I don't remember all the details but I do remember that I never hit it. The idea back then was to document what appears the problem and then once there is a reproducer address it _or_ when there is another problem check if it aligns with the output here (so that _this_ problem's origin could be this). This was part of address all known chip erratas and copied from omap-serial at the time so that the 8250 does not miss anything. Looking closer, this is still part of the omap-serial driver and it was introduced in commit 0003450964357 ("omap2/3/4: serial: errata i202: fix for MDR1 access") If someone found a way to trigger this output which is unrelated to the expected cause then this is clearly not helping nor intended. I would prefer to keep the loop and replace the disturbing output with a comment describing _why_ the FIFO might remain non-empty after a flush. In worst cases that loop causes a delay of less than 0.5ms while setting a baud rate so I doubt that this is causing a real problem. Either way I would like to see Tony's ACK before this is getting removed as suggested in this patch. > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Thanks. Sebastian
On Mon, 17 Oct 2022, Sebastian Andrzej Siewior wrote: > On 2022-10-17 11:12:41 [+0300], Ilpo Järvinen wrote: > > On Thu, 13 Oct 2022, Matthias Schiffer wrote: > > > > > We were occasionally seeing the "Errata i202: timedout" on an AM335x > > > board when repeatedly opening and closing a UART connected to an active > > > sender. As new input may arrive at any time, it is possible to miss the > > > "RX FIFO empty" condition, forcing the loop to wait until it times out. > > > > I can see this problem could occur and why your patch fixes it. > > > > > Nothing in the i202 Advisory states that such a wait is even necessary; > > > other FIFO clear functions like serial8250_clear_fifos() do not wait > > > either. For this reason, it seems safe to remove the wait, fixing the > > > mentioned issue. > > > > Checking the commit that added this driver and the loop along with it, > > there was no information why it would be needed there either. > > I don't remember all the details but I do remember that I never hit it. > The idea back then was to document what appears the problem and then > once there is a reproducer address it _or_ when there is another problem > check if it aligns with the output here (so that _this_ problem's origin > could be this). This was part of address all known chip erratas and > copied from omap-serial at the time so that the 8250 does not miss > anything. > Looking closer, this is still part of the omap-serial driver and it was > introduced in commit > 0003450964357 ("omap2/3/4: serial: errata i202: fix for MDR1 access") I found that one too but it doesn't give any explanation for it either. In fact, the wait for empty is mysteriously missing from the itemized description of the workaround in the commit message. > If someone found a way to trigger this output which is unrelated to the > expected cause then this is clearly not helping nor intended. > > I would prefer to keep the loop and replace the disturbing output with a > comment describing _why_ the FIFO might remain non-empty after a flush. > > In worst cases that loop causes a delay of less than 0.5ms while setting > a baud rate so I doubt that this is causing a real problem. > > Either way I would like to see Tony's ACK before this is getting removed > as suggested in this patch. Thanks for chimming in. I went to do some lore searching and came across this thread (it should be added with Link: tag the patch regardless of its final form): https://lore.kernel.org/linux-omap/4BBF61FE.3060807@ti.com/
Hi, Adding Nishanth to Cc also. * Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [221017 12:06]: > On Mon, 17 Oct 2022, Sebastian Andrzej Siewior wrote: > > > On 2022-10-17 11:12:41 [+0300], Ilpo Järvinen wrote: > > > On Thu, 13 Oct 2022, Matthias Schiffer wrote: > > > > > > > We were occasionally seeing the "Errata i202: timedout" on an AM335x > > > > board when repeatedly opening and closing a UART connected to an active > > > > sender. As new input may arrive at any time, it is possible to miss the > > > > "RX FIFO empty" condition, forcing the loop to wait until it times out. > > > > > > I can see this problem could occur and why your patch fixes it. > > > > > > > Nothing in the i202 Advisory states that such a wait is even necessary; > > > > other FIFO clear functions like serial8250_clear_fifos() do not wait > > > > either. For this reason, it seems safe to remove the wait, fixing the > > > > mentioned issue. > > > > > > Checking the commit that added this driver and the loop along with it, > > > there was no information why it would be needed there either. > > > > I don't remember all the details but I do remember that I never hit it. > > The idea back then was to document what appears the problem and then > > once there is a reproducer address it _or_ when there is another problem > > check if it aligns with the output here (so that _this_ problem's origin > > could be this). This was part of address all known chip erratas and > > copied from omap-serial at the time so that the 8250 does not miss > > anything. > > Looking closer, this is still part of the omap-serial driver and it was > > introduced in commit > > 0003450964357 ("omap2/3/4: serial: errata i202: fix for MDR1 access") > > I found that one too but it doesn't give any explanation for it either. > In fact, the wait for empty is mysteriously missing from the itemized > description of the workaround in the commit message. > > > If someone found a way to trigger this output which is unrelated to the > > expected cause then this is clearly not helping nor intended. > > > > I would prefer to keep the loop and replace the disturbing output with a > > comment describing _why_ the FIFO might remain non-empty after a flush. > > > > In worst cases that loop causes a delay of less than 0.5ms while setting > > a baud rate so I doubt that this is causing a real problem. This sounds like a safe solution for me if it's needed. > > Either way I would like to see Tony's ACK before this is getting removed > > as suggested in this patch. > > Thanks for chimming in. > > I went to do some lore searching and came across this thread (it should > be added with Link: tag the patch regardless of its final form): > https://lore.kernel.org/linux-omap/4BBF61FE.3060807@ti.com/ Nishanth, do you have any more info on checking for fifo empty here? Regards, Tony
On 24/10/22 10:58 am, Tony Lindgren wrote: > Hi, > > Adding Nishanth to Cc also. > > * Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [221017 12:06]: >> On Mon, 17 Oct 2022, Sebastian Andrzej Siewior wrote: >> >>> On 2022-10-17 11:12:41 [+0300], Ilpo Järvinen wrote: >>>> On Thu, 13 Oct 2022, Matthias Schiffer wrote: >>>> >>>>> We were occasionally seeing the "Errata i202: timedout" on an AM335x >>>>> board when repeatedly opening and closing a UART connected to an active >>>>> sender. As new input may arrive at any time, it is possible to miss the >>>>> "RX FIFO empty" condition, forcing the loop to wait until it times out. >>>> >>>> I can see this problem could occur and why your patch fixes it. >>>> >>>>> Nothing in the i202 Advisory states that such a wait is even necessary; >>>>> other FIFO clear functions like serial8250_clear_fifos() do not wait >>>>> either. For this reason, it seems safe to remove the wait, fixing the >>>>> mentioned issue. >>>> >>>> Checking the commit that added this driver and the loop along with it, >>>> there was no information why it would be needed there either. >>> >>> I don't remember all the details but I do remember that I never hit it. >>> The idea back then was to document what appears the problem and then >>> once there is a reproducer address it _or_ when there is another problem >>> check if it aligns with the output here (so that _this_ problem's origin >>> could be this). This was part of address all known chip erratas and >>> copied from omap-serial at the time so that the 8250 does not miss >>> anything. >>> Looking closer, this is still part of the omap-serial driver and it was >>> introduced in commit >>> 0003450964357 ("omap2/3/4: serial: errata i202: fix for MDR1 access") >> >> I found that one too but it doesn't give any explanation for it either. >> In fact, the wait for empty is mysteriously missing from the itemized >> description of the workaround in the commit message. >> >>> If someone found a way to trigger this output which is unrelated to the >>> expected cause then this is clearly not helping nor intended. >>> >>> I would prefer to keep the loop and replace the disturbing output with a >>> comment describing _why_ the FIFO might remain non-empty after a flush. >>> >>> In worst cases that loop causes a delay of less than 0.5ms while setting >>> a baud rate so I doubt that this is causing a real problem. > > This sounds like a safe solution for me if it's needed. > >>> Either way I would like to see Tony's ACK before this is getting removed >>> as suggested in this patch. >> >> Thanks for chimming in. >> >> I went to do some lore searching and came across this thread (it should >> be added with Link: tag the patch regardless of its final form): >> https://lore.kernel.org/linux-omap/4BBF61FE.3060807@ti.com/ > > Nishanth, do you have any more info on checking for fifo empty here? > At least TRMs of newer SoCs such as AM654 [0] have following note: NOTE : Bits UART_FCR[2] TX_FIFO_CLEAR and UART_FCR[1] RX_FIFO_CLEAR are automatically cleared by hardware after 4 × UARTi_ICLK + 5 × UARTi_FCLK clock cycles. This delay is needed to finish the resetting of the corresponding FIFO and DMA control registers. I guess we can drop FIFO empty check and instead add above delay required for FIFOs to be reset. [0] https://www.ti.com/lit/pdf/spruid7e 12.1.5.4.6 UART FIFO Management Regards Vignesh
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 41b8c6b27136..484f791617af 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -193,27 +193,10 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) static void omap_8250_mdr1_errataset(struct uart_8250_port *up, struct omap8250_priv *priv) { - u8 timeout = 255; - serial_out(up, UART_OMAP_MDR1, priv->mdr1); udelay(2); serial_out(up, UART_FCR, up->fcr | UART_FCR_CLEAR_XMIT | UART_FCR_CLEAR_RCVR); - /* - * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and - * TX_FIFO_E bit is 1. - */ - while (UART_LSR_THRE != (serial_in(up, UART_LSR) & - (UART_LSR_THRE | UART_LSR_DR))) { - timeout--; - if (!timeout) { - /* Should *never* happen. we warn and carry on */ - dev_crit(up->port.dev, "Errata i202: timedout %x\n", - serial_in(up, UART_LSR)); - break; - } - udelay(1); - } } static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,