Message ID | 20221112212125.448824-14-gsomlo@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1433028wru; Sat, 12 Nov 2022 13:25:37 -0800 (PST) X-Google-Smtp-Source: AA0mqf54aeNRwvhj/gy4P0oFjrrAaU+clbbenSY4uYs7uqGekvB5x2JvrYvKVqlJMK5TD3VGwaNB X-Received: by 2002:a17:906:f8d6:b0:7ad:a0cb:f79e with SMTP id lh22-20020a170906f8d600b007ada0cbf79emr5818468ejb.458.1668288337357; Sat, 12 Nov 2022 13:25:37 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668288337; cv=none; d=google.com; s=arc-20160816; b=nUdWKYhkJ5BBeHmp9ufIiWNd7g12RXTuTJGWjCHZoTocpDGGycuZBun0zZdetM3Qnk yVOWNfOhrMV2oE5lzXHeudl5JEqPLc1UmjD+l/vuJD5suujZtvX3OgHWP4coxTUwH3J5 RDZN2g15QMXlrRfSmTtTktQdDHX2uw5ImagISRgGIrZHwqE7aNWV+oXVRR5i1rNoZyk6 AShV4x10KSGp4ReTn6+jdmtW6Ik3SufTbjQO0KCpwAX3bW1tSZj+UqQtsqWpdqvTACTp b1IbqASDNTDmeSOZjyAYcrQerFI8S+cDxdN+3+E3KBZpp3JtU8ZcoxNTI0iqPL/W64YM QJgA== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=RnlgHyHxWkwP8NBkj8ItqSrcSz+jgPdiCkwmRSzVrD8=; b=eHgSpuSllBOow9TmYxDsg8i6Hh/jH8okWfGPEEwVu+YdAhhSk2MfXO/rAuUHAlVXgw j9jD6np4HldNkTygIhZ2wGJIkjAF1p4mZf53HbaVvkVJiSpQsnZdAbavkn3wqE+XcsIa vF31+w2sTN11N7v9bQFxC0WvY3+5eo2HsO4LtScIZx/cilF+bjV8fjOY13wxv2+nm7U6 TGahfJMYQ/bqhOsDouq7hbbVxrKaJM27l9cK7hTVC9QmvWT5cFwvRQoZyD9Z3SX0uDpc LLNesrV2U9Hxy03145bWNPPiiUd8AM8kUkBc1Qn2C3w4eaukzFfjpAZDTxGoCrRyQ+Rk Udig== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="N/5v4fSS"; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id qb39-20020a1709077ea700b007aeaacd5592si2968128ejc.124.2022.11.12.13.25.13; Sat, 12 Nov 2022 13:25:37 -0800 (PST) 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=@gmail.com header.s=20210112 header.b="N/5v4fSS"; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235158AbiKLVWa (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Sat, 12 Nov 2022 16:22:30 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53762 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235159AbiKLVV6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 12 Nov 2022 16:21:58 -0500 Received: from mail-qk1-x736.google.com (mail-qk1-x736.google.com [IPv6:2607:f8b0:4864:20::736]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3441418E3F; Sat, 12 Nov 2022 13:21:45 -0800 (PST) Received: by mail-qk1-x736.google.com with SMTP id z1so5323835qkl.9; Sat, 12 Nov 2022 13:21:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=RnlgHyHxWkwP8NBkj8ItqSrcSz+jgPdiCkwmRSzVrD8=; b=N/5v4fSSSIAGk863cGwJ2LOZwHTkOlTFVZw5GBTZwG0fYggDGLhFLWEbS3SyaPoxGu bybG+F5QBHDGxXXIBEQAVFBTapHVzBwoE4+bix/ZqF+Zv6THz+qlPHG6LJAXOI2ieBi9 y5clfWPmRUsCgTSXm8Xg7XCFeWCHRRKfjZT+cayXiy7b1OKRMJRFOWcsHJT78V5laOI6 5qwdJVRt70KJt8CWEzCWWv+obmTrj2EFiRAv2FXVZVC6IIpvboeCwA5r5c+Vk+9cqf7B 1Z9snQ+4S8f6LCk7FilZj32bZUGhtTrESff7AGqGj5kEUoXJGOquWGl7iqr9XP8cz+jm +5NQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=RnlgHyHxWkwP8NBkj8ItqSrcSz+jgPdiCkwmRSzVrD8=; b=PubWIGmneq80zX3VUf/ZugHI7xLOwb8sIXb0z7V5TBK96Vl24nMMMSqrwQWu/grMYi DfTyyXtHbphLfNKdGLsBUcgvgRoZjQ9RAtmlHNkWZBge3YnxIWZnoVs4RFie1gP1zL5o S/q6VRJapN68YWxXUgjpcP8PyOgicjzrpWQUqqMUaQXTCVTGuN7XKwYGTz5hQPBxARcS mOsTmXyTpb4gcspuHaQo7HKdP1AWhLdirgXJ0ppntgy9sWtscx3RksycQx9WOrWQpbjb 7TalEiBOOvzvVlvJeYD1l0TksD1jW2R1aAmvCnaW39+ZDw2EtonSbFcOP5IMK5P06F/m f9eg== X-Gm-Message-State: ANoB5pnJKyVZNxAVUJvGgQOvE+Ol8Yc63VEmmsTpXkKSwpzX9Hwo7xxR pkWOakNUrobqo4dw7LZff8P0SFXn3QfwsA== X-Received: by 2002:a05:620a:1202:b0:6fa:5f94:8b16 with SMTP id u2-20020a05620a120200b006fa5f948b16mr5871509qkj.215.1668288103983; Sat, 12 Nov 2022 13:21:43 -0800 (PST) Received: from glsvmlin.ini.cmu.edu (GLSVMLIN.INI.CMU.EDU. [128.2.16.9]) by smtp.gmail.com with ESMTPSA id t8-20020a37ea08000000b006fa313bf185sm3811839qkj.8.2022.11.12.13.21.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 12 Nov 2022 13:21:43 -0800 (PST) From: Gabriel Somlo <gsomlo@gmail.com> To: linux-kernel@vger.kernel.org Cc: linux-serial@vger.kernel.org, gregkh@linuxfoundation.org, jirislaby@kernel.org, kgugala@antmicro.com, mholenko@antmicro.com, joel@jms.id.au, david.abdurachmanov@gmail.com, florent@enjoy-digital.fr, geert@linux-m68k.org Subject: [PATCH v3 13/14] serial: liteuart: add IRQ support for the TX path Date: Sat, 12 Nov 2022 16:21:24 -0500 Message-Id: <20221112212125.448824-14-gsomlo@gmail.com> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20221112212125.448824-1-gsomlo@gmail.com> References: <20221112212125.448824-1-gsomlo@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,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?1749327111434482470?= X-GMAIL-MSGID: =?utf-8?q?1749327111434482470?= |
Series |
serial: liteuart: add IRQ support
|
|
Commit Message
Gabriel L. Somlo
Nov. 12, 2022, 9:21 p.m. UTC
Modify the TX path to operate in an IRQ-compatible way, while
maintaining support for polling mode via the poll timer.
Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
---
drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 20 deletions(-)
Comments
On Sat, Nov 12, 2022 at 04:21:24PM -0500, Gabriel Somlo wrote: > Modify the TX path to operate in an IRQ-compatible way, while > maintaining support for polling mode via the poll timer. > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > --- > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 20 deletions(-) > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > index e30adb30277f..307c27398e30 100644 > --- a/drivers/tty/serial/liteuart.c > +++ b/drivers/tty/serial/liteuart.c > @@ -46,6 +46,7 @@ struct liteuart_port { > struct uart_port port; > struct timer_list timer; > u32 id; > + bool poll_tx_started; > }; > > #define to_liteuart_port(port) container_of(port, struct liteuart_port, port) > @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch) > > static void liteuart_stop_tx(struct uart_port *port) > { > - /* not used in LiteUART, but called unconditionally from serial_core */ > + if (port->irq) { > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > + } else { > + struct liteuart_port *uart = to_liteuart_port(port); > + uart->poll_tx_started = false; > + } > } > > static void liteuart_start_tx(struct uart_port *port) > { > - struct circ_buf *xmit = &port->state->xmit; > - unsigned char ch; > - > - if (unlikely(port->x_char)) { > - litex_write8(port->membase + OFF_RXTX, port->x_char); > - port->icount.tx++; > - port->x_char = 0; > - } else if (!uart_circ_empty(xmit)) { > - while (xmit->head != xmit->tail) { > - ch = xmit->buf[xmit->tail]; I just realized that this: > - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > - port->icount.tx++; ... conflicts with another accepted patch (by Ilpo) that's currently making its way to being released: https://lore.kernel.org/all/20221019091151.6692-20-ilpo.jarvinen@linux.intel.com/ > - liteuart_putchar(port, ch); > - } > + if (port->irq) { > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX); > + } else { > + struct liteuart_port *uart = to_liteuart_port(port); > + uart->poll_tx_started = true; > } > - > - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > - uart_write_wakeup(port); > } > > static void liteuart_stop_rx(struct uart_port *port) > @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port) > tty_flip_buffer_push(&port->state->port); > } > > +static void liteuart_tx_chars(struct uart_port *port) > +{ > + struct circ_buf *xmit = &port->state->xmit; > + > + if (unlikely(port->x_char)) { > + litex_write8(port->membase + OFF_RXTX, port->x_char); > + port->x_char = 0; > + port->icount.tx++; > + return; > + } > + > + while (!litex_read8(port->membase + OFF_TXFULL)) { > + if (xmit->head == xmit->tail) > + break; > + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]); Also, this: > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > + port->icount.tx++; should now be `uart_xmit_advance(port, 1);` instead. I'm going to take that into account in v4, in addition to any extra feedback I'll receive. Thanks! > + } > + > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(port); > + > + if (uart_circ_empty(xmit)) > + liteuart_stop_tx(port); > +} > + > static irqreturn_t liteuart_interrupt(int irq, void *data) > { > struct liteuart_port *uart = data; > struct uart_port *port = &uart->port; > u8 isr = litex_read8(port->membase + OFF_EV_PENDING); > > - /* for now, only rx path triggers interrupts */ > - isr &= EV_RX; > + if (!(port->irq || uart->poll_tx_started)) > + isr &= ~EV_TX; /* polling mode with tx stopped */ > > spin_lock(&port->lock); > if (isr & EV_RX) > liteuart_rx_chars(port); > + if (isr & EV_TX) { > + liteuart_tx_chars(port); > + } > spin_unlock(&port->lock); > > return IRQ_RETVAL(isr); > @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port) > } > > if (!port->irq) { > + uart->poll_tx_started = false; > timer_setup(&uart->timer, liteuart_timer, 0); > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); > } > @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port) > struct liteuart_port *uart = to_liteuart_port(port); > > litex_write8(port->membase + OFF_EV_ENABLE, 0); > + uart->poll_tx_started = false; > > if (port->irq) > free_irq(port->irq, port); > -- > 2.37.3 >
On Sat, 12 Nov 2022, Gabriel Somlo wrote: > Modify the TX path to operate in an IRQ-compatible way, while > maintaining support for polling mode via the poll timer. > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > --- > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 20 deletions(-) > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > index e30adb30277f..307c27398e30 100644 > --- a/drivers/tty/serial/liteuart.c > +++ b/drivers/tty/serial/liteuart.c > @@ -46,6 +46,7 @@ struct liteuart_port { > struct uart_port port; > struct timer_list timer; > u32 id; > + bool poll_tx_started; > }; > > #define to_liteuart_port(port) container_of(port, struct liteuart_port, port) > @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch) > > static void liteuart_stop_tx(struct uart_port *port) > { > - /* not used in LiteUART, but called unconditionally from serial_core */ Drop this comment from the earlier patch too given you remove it here. It just adds useless churn in diff for no useful reason. > + if (port->irq) { > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); If you put irq_mask into liteuart_port you wouldn't need to read it back here? > + } else { > + struct liteuart_port *uart = to_liteuart_port(port); > + uart->poll_tx_started = false; > + } > } > > static void liteuart_start_tx(struct uart_port *port) > { > - struct circ_buf *xmit = &port->state->xmit; > - unsigned char ch; > - > - if (unlikely(port->x_char)) { > - litex_write8(port->membase + OFF_RXTX, port->x_char); > - port->icount.tx++; > - port->x_char = 0; > - } else if (!uart_circ_empty(xmit)) { > - while (xmit->head != xmit->tail) { > - ch = xmit->buf[xmit->tail]; > - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > - port->icount.tx++; This is not based on tty-next tree. Please rebase on top of it (you might have noted it already, IIRC, somebody noted uart_xmit_advance conflict in some patch, perhaps it was you :-)). > - liteuart_putchar(port, ch); > - } > + if (port->irq) { > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX); ->irq_mask? > + } else { > + struct liteuart_port *uart = to_liteuart_port(port); > + uart->poll_tx_started = true; > } > - > - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > - uart_write_wakeup(port); > } > > static void liteuart_stop_rx(struct uart_port *port) > @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port) > tty_flip_buffer_push(&port->state->port); > } > > +static void liteuart_tx_chars(struct uart_port *port) > +{ > + struct circ_buf *xmit = &port->state->xmit; > + > + if (unlikely(port->x_char)) { > + litex_write8(port->membase + OFF_RXTX, port->x_char); > + port->x_char = 0; > + port->icount.tx++; > + return; > + } > + > + while (!litex_read8(port->membase + OFF_TXFULL)) { > + if (xmit->head == xmit->tail) There exists a helper for this condition. > + break; > + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]); > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > + port->icount.tx++; uart_xmit_advance() > + } > + > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(port); > + > + if (uart_circ_empty(xmit)) > + liteuart_stop_tx(port); > +} You might want to check if you can generate this whole function with Jiri's tx helpers (IIRC, they're only in tty-next tree currently). > + > static irqreturn_t liteuart_interrupt(int irq, void *data) > { > struct liteuart_port *uart = data; > struct uart_port *port = &uart->port; > u8 isr = litex_read8(port->membase + OFF_EV_PENDING); > > - /* for now, only rx path triggers interrupts */ > - isr &= EV_RX; > + if (!(port->irq || uart->poll_tx_started)) > + isr &= ~EV_TX; /* polling mode with tx stopped */ > > spin_lock(&port->lock); > if (isr & EV_RX) > liteuart_rx_chars(port); > + if (isr & EV_TX) { > + liteuart_tx_chars(port); > + } Extra braces. > spin_unlock(&port->lock); > > return IRQ_RETVAL(isr); > @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port) > } > > if (!port->irq) { > + uart->poll_tx_started = false; Can poll_tx_started ever be true here? > timer_setup(&uart->timer, liteuart_timer, 0); > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); > } > @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port) > struct liteuart_port *uart = to_liteuart_port(port); > > litex_write8(port->membase + OFF_EV_ENABLE, 0); > + uart->poll_tx_started = false; > > if (port->irq) > free_irq(port->irq, port); >
On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo Järvinen wrote: > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > Modify the TX path to operate in an IRQ-compatible way, while > > maintaining support for polling mode via the poll timer. > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > --- > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > > 1 file changed, 47 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > index e30adb30277f..307c27398e30 100644 > > --- a/drivers/tty/serial/liteuart.c > > +++ b/drivers/tty/serial/liteuart.c > > @@ -46,6 +46,7 @@ struct liteuart_port { > > struct uart_port port; > > struct timer_list timer; > > u32 id; > > + bool poll_tx_started; > > }; > > > > #define to_liteuart_port(port) container_of(port, struct liteuart_port, port) > > @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch) > > > > static void liteuart_stop_tx(struct uart_port *port) > > { > > - /* not used in LiteUART, but called unconditionally from serial_core */ > > Drop this comment from the earlier patch too given you remove it here. It > just adds useless churn in diff for no useful reason. Right -- I already had this lined up for v4 :) > > + if (port->irq) { > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > > If you put irq_mask into liteuart_port you wouldn't need to read it > back here? So, instead of `bool poll_tx_started` I should just keep a copy of the irq_mask there, and take `&port->lock` whenever I need to *both* update the mask *and* write it out to the actual device register? > > + } else { > > + struct liteuart_port *uart = to_liteuart_port(port); > > + uart->poll_tx_started = false; > > + } > > } > > > > static void liteuart_start_tx(struct uart_port *port) > > { > > - struct circ_buf *xmit = &port->state->xmit; > > - unsigned char ch; > > - > > - if (unlikely(port->x_char)) { > > - litex_write8(port->membase + OFF_RXTX, port->x_char); > > - port->icount.tx++; > > - port->x_char = 0; > > - } else if (!uart_circ_empty(xmit)) { > > - while (xmit->head != xmit->tail) { > > - ch = xmit->buf[xmit->tail]; > > - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > > - port->icount.tx++; > > This is not based on tty-next tree. Please rebase on top of it (you > might have noted it already, IIRC, somebody noted uart_xmit_advance > conflict in some patch, perhaps it was you :-)). Yeah, I did notice that right after I sent out v3. I've already rebased it on top of your patch using uart_xmit_advance. > > - liteuart_putchar(port, ch); > > - } > > + if (port->irq) { > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX); > > ->irq_mask? I'll switch to s/bool poll_tx_started/u8 irq_mask/g in v4, hopefully it should make it all look cleaner. > > + } else { > > + struct liteuart_port *uart = to_liteuart_port(port); > > + uart->poll_tx_started = true; > > } > > - > > - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > - uart_write_wakeup(port); > > } > > > > static void liteuart_stop_rx(struct uart_port *port) > > @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port) > > tty_flip_buffer_push(&port->state->port); > > } > > > > +static void liteuart_tx_chars(struct uart_port *port) > > +{ > > + struct circ_buf *xmit = &port->state->xmit; > > + > > + if (unlikely(port->x_char)) { > > + litex_write8(port->membase + OFF_RXTX, port->x_char); > > + port->x_char = 0; > > + port->icount.tx++; > > + return; > > + } > > + > > + while (!litex_read8(port->membase + OFF_TXFULL)) { > > + if (xmit->head == xmit->tail) > > There exists a helper for this condition. Is that in the released linus tree, or still only in tty-next? > > + break; > > + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]); > > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > > + port->icount.tx++; > > uart_xmit_advance() Already lined up for v4. > > > + } > > + > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > + uart_write_wakeup(port); > > + > > + if (uart_circ_empty(xmit)) > > + liteuart_stop_tx(port); > > +} > > You might want to check if you can generate this whole function with > Jiri's tx helpers (IIRC, they're only in tty-next tree currently). Looks like I should switch to tty-next for this whole series, which makes sense, since it's a tty I'm working on :) I'll rebase on top of that before I send out v4, hopefully later this afternoon. > > + > > static irqreturn_t liteuart_interrupt(int irq, void *data) > > { > > struct liteuart_port *uart = data; > > struct uart_port *port = &uart->port; > > u8 isr = litex_read8(port->membase + OFF_EV_PENDING); > > > > - /* for now, only rx path triggers interrupts */ > > - isr &= EV_RX; > > + if (!(port->irq || uart->poll_tx_started)) > > + isr &= ~EV_TX; /* polling mode with tx stopped */ > > > > spin_lock(&port->lock); > > if (isr & EV_RX) > > liteuart_rx_chars(port); > > + if (isr & EV_TX) { > > + liteuart_tx_chars(port); > > + } > > Extra braces. Got it, thanks! > > spin_unlock(&port->lock); > > > > return IRQ_RETVAL(isr); > > @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port) > > } > > > > if (!port->irq) { > > + uart->poll_tx_started = false; > > Can poll_tx_started ever be true here? Proably not, but it shouldn't matter if I switch to using `u8 irq_mask`, instead, which should be initialized to 0 during probe(). Thanks again for the feedback! Best, --Gabriel > > timer_setup(&uart->timer, liteuart_timer, 0); > > mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); > > } > > @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port) > > struct liteuart_port *uart = to_liteuart_port(port); > > > > litex_write8(port->membase + OFF_EV_ENABLE, 0); > > + uart->poll_tx_started = false; > > > > if (port->irq) > > free_irq(port->irq, port); > > > > -- > i. >
On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo Järvinen wrote: > > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > > > Modify the TX path to operate in an IRQ-compatible way, while > > > maintaining support for polling mode via the poll timer. > > > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > > --- > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > > > 1 file changed, 47 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > > index e30adb30277f..307c27398e30 100644 > > > --- a/drivers/tty/serial/liteuart.c > > > +++ b/drivers/tty/serial/liteuart.c > > > + if (port->irq) { > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > > > > If you put irq_mask into liteuart_port you wouldn't need to read it > > back here? > > So, instead of `bool poll_tx_started` I should just keep a copy of the > irq_mask there, and take `&port->lock` whenever I need to *both* update > the mask *and* write it out to the actual device register? I was mostly thinking of storing EV_RX there but then it could be derived from port->irq that is checked by all paths already. > > > + if (unlikely(port->x_char)) { > > > + litex_write8(port->membase + OFF_RXTX, port->x_char); > > > + port->x_char = 0; > > > + port->icount.tx++; > > > + return; > > > + } > > > + > > > + while (!litex_read8(port->membase + OFF_TXFULL)) { > > > + if (xmit->head == xmit->tail) > > > > There exists a helper for this condition. > > Is that in the released linus tree, or still only in tty-next? uart_circ_empty() has been around for ages. > > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > > + uart_write_wakeup(port); > > > + > > > + if (uart_circ_empty(xmit)) > > > + liteuart_stop_tx(port); > > > +} > > > > You might want to check if you can generate this whole function with > > Jiri's tx helpers (IIRC, they're only in tty-next tree currently). > > Looks like I should switch to tty-next for this whole series, which > makes sense, since it's a tty I'm working on :) > > I'll rebase on top of that before I send out v4, hopefully later this > afternoon. Ok. As I now looked it up, Jiri's tx helpers is 8275b48b278096edc1e3ea5aa9cf946a10022f79 and you'll find some example conversions in the changes following it.
On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo Järvinen wrote: > On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > > > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo Järvinen wrote: > > > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > > > > > Modify the TX path to operate in an IRQ-compatible way, while > > > > maintaining support for polling mode via the poll timer. > > > > > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > > > --- > > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > > > > 1 file changed, 47 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > > > index e30adb30277f..307c27398e30 100644 > > > > --- a/drivers/tty/serial/liteuart.c > > > > +++ b/drivers/tty/serial/liteuart.c > > > > > + if (port->irq) { > > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > > > > > > If you put irq_mask into liteuart_port you wouldn't need to read it > > > back here? > > > > So, instead of `bool poll_tx_started` I should just keep a copy of the > > irq_mask there, and take `&port->lock` whenever I need to *both* update > > the mask *and* write it out to the actual device register? > > I was mostly thinking of storing EV_RX there but then it could be derived > from port->irq that is checked by all paths already. I'm a bit confused here -- the reason I was reading in irq_mask from the mmio port and then flipping the EV_TX bit before writing it back out was to preserve the current value of the EV_RX bit in that register, whatever it may happen to be at the time. If I shadowed the entire register value (with both EV_RX and EV_TX bits reflecting their current value), I could get away with only ever *writing* the MMIO register each time the shadow register value is modified (one or both of the bits are flipped), and only when port->irq is non-zero (i.e., the shadow register would work for polling-mode also). I think that's how altera_uart.c is doing it (I've been using that as a source of inspiration). I thought that's what you were suggesting earlie, but based on your response above I'm no longer sure I follow. > > > > + if (unlikely(port->x_char)) { > > > > + litex_write8(port->membase + OFF_RXTX, port->x_char); > > > > + port->x_char = 0; > > > > + port->icount.tx++; > > > > + return; > > > > + } > > > > + > > > > + while (!litex_read8(port->membase + OFF_TXFULL)) { > > > > + if (xmit->head == xmit->tail) > > > > > > There exists a helper for this condition. > > > > Is that in the released linus tree, or still only in tty-next? > > uart_circ_empty() has been around for ages. Oh, I misunderstood what you meant here originally -- just use `uart_circ_empty()` like I'm already doing elsewhere in the code, got it! > > > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > > > + uart_write_wakeup(port); > > > > + > > > > + if (uart_circ_empty(xmit)) > > > > + liteuart_stop_tx(port); > > > > +} > > > > > > You might want to check if you can generate this whole function with > > > Jiri's tx helpers (IIRC, they're only in tty-next tree currently). > > > > Looks like I should switch to tty-next for this whole series, which > > makes sense, since it's a tty I'm working on :) > > > > I'll rebase on top of that before I send out v4, hopefully later this > > afternoon. > > Ok. > > As I now looked it up, Jiri's tx helpers is > 8275b48b278096edc1e3ea5aa9cf946a10022f79 and you'll find some example > conversions in the changes following it. I'll check that out and follow the examples. Thanks again, --G
On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo Järvinen wrote: > On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > > > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo Järvinen wrote: > > > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > > > > > Modify the TX path to operate in an IRQ-compatible way, while > > > > maintaining support for polling mode via the poll timer. > > > > > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > > > --- > > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > > > > 1 file changed, 47 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > > > index e30adb30277f..307c27398e30 100644 > > > > --- a/drivers/tty/serial/liteuart.c > > > > +++ b/drivers/tty/serial/liteuart.c > > > > > + if (port->irq) { > > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > > > > > > If you put irq_mask into liteuart_port you wouldn't need to read it > > > back here? > > > > So, instead of `bool poll_tx_started` I should just keep a copy of the > > irq_mask there, and take `&port->lock` whenever I need to *both* update > > the mask *and* write it out to the actual device register? > > I was mostly thinking of storing EV_RX there but then it could be derived > from port->irq that is checked by all paths already. So, just to clear up the best course of action here (before I rebase everything on top of tty-next: How about the patch below (currently applied separately at the end of the entire series, but I can respin it as part of the rx path (12/14) and tx path (13/14) as appropriate. Let me know what you think. Thanks, --Gabriel diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c index eb0ae8c8bd94..185db423c65f 100644 --- a/drivers/tty/serial/liteuart.c +++ b/drivers/tty/serial/liteuart.c @@ -47,7 +47,7 @@ struct liteuart_port { struct uart_port port; struct timer_list timer; u32 id; - bool poll_tx_started; + u8 irq_reg; }; #define to_liteuart_port(port) container_of(port, struct liteuart_port, port) @@ -70,26 +70,27 @@ static struct uart_driver liteuart_driver = { #endif }; +static void liteuart_update_irq_reg(struct uart_port *port, bool set, u8 bits) +{ + struct liteuart_port *uart = to_liteuart_port(port); + + if (set) + uart->irq_reg |= bits; + else + uart->irq_reg &= ~bits; + + if (port->irq) + litex_write8(port->membase + OFF_EV_ENABLE, uart->irq_reg); +} + static void liteuart_stop_tx(struct uart_port *port) { - if (port->irq) { - u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); - } else { - struct liteuart_port *uart = to_liteuart_port(port); - uart->poll_tx_started = false; - } + liteuart_update_irq_reg(port, false, EV_TX); } static void liteuart_start_tx(struct uart_port *port) { - if (port->irq) { - u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX); - } else { - struct liteuart_port *uart = to_liteuart_port(port); - uart->poll_tx_started = true; - } + liteuart_update_irq_reg(port, true, EV_TX); } static void liteuart_stop_rx(struct uart_port *port) @@ -149,12 +150,10 @@ static irqreturn_t liteuart_interrupt(int irq, void *data) { struct liteuart_port *uart = data; struct uart_port *port = &uart->port; - u8 isr = litex_read8(port->membase + OFF_EV_PENDING); - - if (!(port->irq || uart->poll_tx_started)) - isr &= ~EV_TX; /* polling mode with tx stopped */ + u8 isr; spin_lock(&port->lock); + isr = litex_read8(port->membase + OFF_EV_PENDING) & uart->irq_reg; if (isr & EV_RX) liteuart_rx_chars(port); if (isr & EV_TX) @@ -195,39 +194,40 @@ static unsigned int liteuart_get_mctrl(struct uart_port *port) static int liteuart_startup(struct uart_port *port) { struct liteuart_port *uart = to_liteuart_port(port); + unsigned long flags; int ret; - u8 irq_mask = 0; if (port->irq) { ret = request_irq(port->irq, liteuart_interrupt, 0, KBUILD_MODNAME, uart); - if (ret == 0) { - /* only enabling rx interrupts at startup */ - irq_mask = EV_RX; - } else { + if (ret) { pr_err(pr_fmt("line %d irq %d failed: using polling\n"), port->line, port->irq); port->irq = 0; } } + spin_lock_irqsave(&port->lock, flags); + /* only enabling rx irqs during startup */ + liteuart_update_irq_reg(port, true, EV_RX); + spin_unlock_irqrestore(&port->lock, flags); + if (!port->irq) { - uart->poll_tx_started = false; timer_setup(&uart->timer, liteuart_timer, 0); mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); } - litex_write8(port->membase + OFF_EV_ENABLE, irq_mask); - return 0; } static void liteuart_shutdown(struct uart_port *port) { struct liteuart_port *uart = to_liteuart_port(port); + unsigned long flags; - litex_write8(port->membase + OFF_EV_ENABLE, 0); - uart->poll_tx_started = false; + spin_lock_irqsave(&port->lock, flags); + liteuart_update_irq_reg(port, false, EV_RX | EV_TX); + spin_unlock_irqrestore(&port->lock, flags); if (port->irq) free_irq(port->irq, port);
On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > On Tue, Nov 15, 2022 at 07:30:09PM +0200, Ilpo Järvinen wrote: > > On Tue, 15 Nov 2022, Gabriel L. Somlo wrote: > > > > > On Tue, Nov 15, 2022 at 06:14:50PM +0200, Ilpo Järvinen wrote: > > > > On Sat, 12 Nov 2022, Gabriel Somlo wrote: > > > > > > > > > Modify the TX path to operate in an IRQ-compatible way, while > > > > > maintaining support for polling mode via the poll timer. > > > > > > > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com> > > > > > --- > > > > > drivers/tty/serial/liteuart.c | 67 ++++++++++++++++++++++++----------- > > > > > 1 file changed, 47 insertions(+), 20 deletions(-) > > > > > > > > > > diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c > > > > > index e30adb30277f..307c27398e30 100644 > > > > > --- a/drivers/tty/serial/liteuart.c > > > > > +++ b/drivers/tty/serial/liteuart.c > > > > > > > + if (port->irq) { > > > > > + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); > > > > > + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); > > > > > > > > If you put irq_mask into liteuart_port you wouldn't need to read it > > > > back here? > > > > > > So, instead of `bool poll_tx_started` I should just keep a copy of the > > > irq_mask there, and take `&port->lock` whenever I need to *both* update > > > the mask *and* write it out to the actual device register? > > > > I was mostly thinking of storing EV_RX there but then it could be derived > > from port->irq that is checked by all paths already. > > So, just to clear up the best course of action here (before I rebase > everything on top of tty-next: How about the patch below (currently > applied separately at the end of the entire series, but I can respin > it as part of the rx path (12/14) and tx path (13/14) as appropriate. > > Let me know what you think. I'm fine with that I think. I'd change to bits parameter name to something more meaningful though, I guess the irq_mask could do ok there.
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c index e30adb30277f..307c27398e30 100644 --- a/drivers/tty/serial/liteuart.c +++ b/drivers/tty/serial/liteuart.c @@ -46,6 +46,7 @@ struct liteuart_port { struct uart_port port; struct timer_list timer; u32 id; + bool poll_tx_started; }; #define to_liteuart_port(port) container_of(port, struct liteuart_port, port) @@ -78,29 +79,24 @@ static void liteuart_putchar(struct uart_port *port, unsigned char ch) static void liteuart_stop_tx(struct uart_port *port) { - /* not used in LiteUART, but called unconditionally from serial_core */ + if (port->irq) { + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask & ~EV_TX); + } else { + struct liteuart_port *uart = to_liteuart_port(port); + uart->poll_tx_started = false; + } } static void liteuart_start_tx(struct uart_port *port) { - struct circ_buf *xmit = &port->state->xmit; - unsigned char ch; - - if (unlikely(port->x_char)) { - litex_write8(port->membase + OFF_RXTX, port->x_char); - port->icount.tx++; - port->x_char = 0; - } else if (!uart_circ_empty(xmit)) { - while (xmit->head != xmit->tail) { - ch = xmit->buf[xmit->tail]; - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); - port->icount.tx++; - liteuart_putchar(port, ch); - } + if (port->irq) { + u8 irq_mask = litex_read8(port->membase + OFF_EV_ENABLE); + litex_write8(port->membase + OFF_EV_ENABLE, irq_mask | EV_TX); + } else { + struct liteuart_port *uart = to_liteuart_port(port); + uart->poll_tx_started = true; } - - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) - uart_write_wakeup(port); } static void liteuart_stop_rx(struct uart_port *port) @@ -131,18 +127,47 @@ static void liteuart_rx_chars(struct uart_port *port) tty_flip_buffer_push(&port->state->port); } +static void liteuart_tx_chars(struct uart_port *port) +{ + struct circ_buf *xmit = &port->state->xmit; + + if (unlikely(port->x_char)) { + litex_write8(port->membase + OFF_RXTX, port->x_char); + port->x_char = 0; + port->icount.tx++; + return; + } + + while (!litex_read8(port->membase + OFF_TXFULL)) { + if (xmit->head == xmit->tail) + break; + litex_write8(port->membase + OFF_RXTX, xmit->buf[xmit->tail]); + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); + port->icount.tx++; + } + + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) + uart_write_wakeup(port); + + if (uart_circ_empty(xmit)) + liteuart_stop_tx(port); +} + static irqreturn_t liteuart_interrupt(int irq, void *data) { struct liteuart_port *uart = data; struct uart_port *port = &uart->port; u8 isr = litex_read8(port->membase + OFF_EV_PENDING); - /* for now, only rx path triggers interrupts */ - isr &= EV_RX; + if (!(port->irq || uart->poll_tx_started)) + isr &= ~EV_TX; /* polling mode with tx stopped */ spin_lock(&port->lock); if (isr & EV_RX) liteuart_rx_chars(port); + if (isr & EV_TX) { + liteuart_tx_chars(port); + } spin_unlock(&port->lock); return IRQ_RETVAL(isr); @@ -196,6 +221,7 @@ static int liteuart_startup(struct uart_port *port) } if (!port->irq) { + uart->poll_tx_started = false; timer_setup(&uart->timer, liteuart_timer, 0); mod_timer(&uart->timer, jiffies + uart_poll_timeout(port)); } @@ -210,6 +236,7 @@ static void liteuart_shutdown(struct uart_port *port) struct liteuart_port *uart = to_liteuart_port(port); litex_write8(port->membase + OFF_EV_ENABLE, 0); + uart->poll_tx_started = false; if (port->irq) free_irq(port->irq, port);