[v1,tty] 8250: microchip: pci1xxxx: Add Burst mode transmission support in uart driver for reading from FIFO
Message ID | 20240125100006.153342-1-rengarajan.s@microchip.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2553:b0:103:945f:af90 with SMTP id p19csp1532095dyi; Thu, 25 Jan 2024 02:02:50 -0800 (PST) X-Google-Smtp-Source: AGHT+IF/PaRlZ2n9LFUcIfkyHqMD+cN3gtNvYkhpVo9OYGffhCdm7j8nr6lYgsqM0xPmPzOIj3Zx X-Received: by 2002:ac8:4e4c:0:b0:42a:2840:a209 with SMTP id e12-20020ac84e4c000000b0042a2840a209mr866599qtw.35.1706176969905; Thu, 25 Jan 2024 02:02:49 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1706176969; cv=pass; d=google.com; s=arc-20160816; b=MNdFEEb4mrS40V5VSyQLN09BYKcr/+kPuYgDaNFWbLjgvqj9V3pO8lykHTltcx/4kP 6XGXFPFwFHvoYe68Q3eGPTr5fDBBorcCjQRPL8whXwhRwB8smY/jPdjcHz5viTVOz8HM VWl1UQpL9utfmq+Mr2sFVMfBfa5AJehQPLfLysABJemXgukhxH9iijqQSDaWhTV3sHQk 7u5Ao2g9JLZSXXpJxiyls+3eAHCFritJ+IZx49YAMMqrQTZMzORaL1ZxFe2tqA4HjxBP LDkcUz6FfjLu0A1kULxrz7YqHCZMzBxRI6hWQos2N+XlM0L6MNQGgYhbOPlb8f/xrkFk F3jw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=G/1/kYzJs5pq1mThScHaFoKa5um/7qjGawA/GPtvyng=; fh=IAwhg5qWvC3IAk6mko9deADI/W9WUhdTZg6+NxZ3AkA=; b=I/bOlMPuy+surygIxqpltOx8XvAylFh5vQ/ecpQsSMtLinPozRd63iu6OnPMeIw7y9 bYmhLfuKKpYfRHgWbazRakH79hcI5T6Ix+nLya3N0gO8K4CBLVS+qXdGX3yWpROjx05h dQIcZp3jmBLJznw0XFiaA3T/BlY5vbmy8URKiPFizzFilXa+vPDJSolVDAsA8i3JA/Id hzlUHQT6MDN10d+OBTmXvgsiT21JkdJi1SBZbYzNTkjkJeVpiliB7FcSUexKu5OvtUjF 1aLvpM8ZTf4PJPdioUhj9eUVgm2Y9Z3eiEc5/tC81TqvEQ6Ow0A60Ik22UF/3b/FftpP CYkw== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=VvEyrU92; arc=pass (i=1 spf=pass spfdomain=microchip.com dkim=pass dkdomain=microchip.com dmarc=pass fromdomain=microchip.com); spf=pass (google.com: domain of linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=REJECT dis=NONE) header.from=microchip.com Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id k5-20020ac85fc5000000b00429d470ea36si12408886qta.592.2024.01.25.02.02.49 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jan 2024 02:02:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=VvEyrU92; arc=pass (i=1 spf=pass spfdomain=microchip.com dkim=pass dkdomain=microchip.com dmarc=pass fromdomain=microchip.com); spf=pass (google.com: domain of linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-38353-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=REJECT dis=NONE) header.from=microchip.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id A0EE81C21451 for <ouuuleilei@gmail.com>; Thu, 25 Jan 2024 10:02:49 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6050E1CA8F; Thu, 25 Jan 2024 10:02:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="VvEyrU92" Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.153.233]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F28C71C686; Thu, 25 Jan 2024 10:02:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=68.232.153.233 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706176949; cv=none; b=GcHimR7tXEasaTWUOxaJ5NUVLYQXqGOdpXDZ1J9Xeg466YDM12q8PYsGLzgT7aqLYLknuUJ9nekO/STpUfK78N+MUWyxzucHGq9bv/ILUibfLxtfTQSgo3xHzqLPe4upb55qr1oiT4F4bnSUsg0lB7+dhXsEAYGdf8XytJOSAoc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706176949; c=relaxed/simple; bh=Tv1+9k+cs9ODrgYL0DhM6PbCDNkcFvwIQHm8qIIPFOw=; h=From:To:CC:Subject:Date:Message-ID:MIME-Version:Content-Type; b=pHqX3409f/4AvmVqyV8jipOnFkTOw2dP3nRRha7iAfzGXYV9x1RCZOmKQbb0ISUZ+hHlSC+0ivFF2bIVpLnaEBb5awGlGQyi8pLQf1gvsAio7bc6yB+hhkBozuhrTq1vN5OtrFoQtW7CTXS/JGLgUnnpQnuoRlVcx1kwheMX8cE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=microchip.com; spf=pass smtp.mailfrom=microchip.com; dkim=pass (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b=VvEyrU92; arc=none smtp.client-ip=68.232.153.233 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=microchip.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=microchip.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1706176948; x=1737712948; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=Tv1+9k+cs9ODrgYL0DhM6PbCDNkcFvwIQHm8qIIPFOw=; b=VvEyrU92g4QY0st1jN98VMuuShbvLKQy/YQAEsT6yP4XLhZ0pWBhQBJd zhge5HH6Vac+PCMmOK1bxgyAJQN5vS7xM+G+kZz3oUIfP0YwtlmhPsSn7 lUVdckmnVfBX5VLPv2TQsoercLw6UxxmPAwtN8611ykrB/APVSM8GyFMs 3zTUQuvVhLMmyHccxWtxugkv7KsDo5pPxFyrqCj+7KQvLxxmWrX4/JFQl 7mTjuGbymJ31VJD9+lb31+xb90+WYmL9TAX3CXB3aKkdZjkHPDjnhtnMf 583N22J5dQTWClrELdQCjxx52X+FsFrAGBOzvPZFWdyQQlGrkCFVvlS7h w==; X-CSE-ConnectionGUID: EcHPnxniQdWGuWl+Gd9iBA== X-CSE-MsgGUID: cPjp1T7gRbWSaY+EDV2BEQ== X-IronPort-AV: E=Sophos;i="6.05,216,1701154800"; d="scan'208";a="245997251" X-Amp-Result: SKIPPED(no attachment in message) Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa5.microchip.iphmx.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 25 Jan 2024 03:02:21 -0700 Received: from chn-vm-ex02.mchp-main.com (10.10.87.72) by chn-vm-ex02.mchp-main.com (10.10.87.72) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Thu, 25 Jan 2024 03:01:56 -0700 Received: from che-dk-ungapp03lx.microchip.com (10.10.85.11) by chn-vm-ex02.mchp-main.com (10.10.85.144) with Microsoft SMTP Server id 15.1.2507.35 via Frontend Transport; Thu, 25 Jan 2024 03:01:53 -0700 From: Rengarajan S <rengarajan.s@microchip.com> To: <kumaravel.thiagarajan@microchip.com>, <tharunkumar.pasumarthi@microchip.com>, <gregkh@linuxfoundation.org>, <jirislaby@kernel.org>, <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org> CC: <UNGLinuxDriver@microchip.com> Subject: [PATCH v1 tty] 8250: microchip: pci1xxxx: Add Burst mode transmission support in uart driver for reading from FIFO Date: Thu, 25 Jan 2024 15:30:06 +0530 Message-ID: <20240125100006.153342-1-rengarajan.s@microchip.com> X-Mailer: git-send-email 2.25.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1789056222692344717 X-GMAIL-MSGID: 1789056222692344717 |
Series |
[v1,tty] 8250: microchip: pci1xxxx: Add Burst mode transmission support in uart driver for reading from FIFO
|
|
Commit Message
Rengarajan S
Jan. 25, 2024, 10 a.m. UTC
pci1xxxx_handle_irq reads the burst status and checks if the FIFO
is empty and is ready to accept the incoming data. The handling is
done in pci1xxxx_tx_burst where each transaction processes data in
block of DWORDs, while any remaining bytes are processed individually,
one byte at a time.
Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>
---
drivers/tty/serial/8250/8250_pci1xxxx.c | 106 ++++++++++++++++++++++++
1 file changed, 106 insertions(+)
Comments
On 25. 01. 24, 11:00, Rengarajan S wrote: > pci1xxxx_handle_irq reads the burst status and checks if the FIFO > is empty and is ready to accept the incoming data. The handling is > done in pci1xxxx_tx_burst where each transaction processes data in > block of DWORDs, while any remaining bytes are processed individually, > one byte at a time. > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > --- > drivers/tty/serial/8250/8250_pci1xxxx.c | 106 ++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c > index 558c4c7f3104..d53605bf908d 100644 > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c .. > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct uart_port *port, u32 uart_status) > } > } > > +static void pci1xxxx_process_write_data(struct uart_port *port, > + struct circ_buf *xmit, > + int *data_empty_count, count is unsigned, right? > + u32 *valid_byte_count) > +{ > + u32 valid_burst_count = *valid_byte_count / UART_BURST_SIZE; > + > + /* > + * Each transaction transfers data in DWORDs. If there are less than > + * four remaining valid_byte_count to transfer or if the circular > + * buffer has insufficient space for a DWORD, the data is transferred > + * one byte at a time. > + */ > + while (valid_burst_count) { > + if (*data_empty_count - UART_BURST_SIZE < 0) Huh? *data_empty_count < UART_BURST_SIZE instead? > + break; > + if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE)) Is this the same as easy to understand: CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) < UART_BURST_SIZE ? > + break; > + writel(*(unsigned int *)&xmit->buf[xmit->tail], > + port->membase + UART_TX_BURST_FIFO); What about unaligned accesses? And you really wanted to spell u32 explicitly, not uint. > + *valid_byte_count -= UART_BURST_SIZE; > + *data_empty_count -= UART_BURST_SIZE; > + valid_burst_count -= UART_BYTE_SIZE; > + > + xmit->tail = (xmit->tail + UART_BURST_SIZE) & > + (UART_XMIT_SIZE - 1); uart_xmit_advance() > + } > + > + while (*valid_byte_count) { > + if (*data_empty_count - UART_BYTE_SIZE < 0) > + break; > + writeb(xmit->buf[xmit->tail], port->membase + > + UART_TX_BYTE_FIFO); > + *data_empty_count -= UART_BYTE_SIZE; > + *valid_byte_count -= UART_BYTE_SIZE; > + > + /* > + * When the tail of the circular buffer is reached, the next > + * byte is transferred to the beginning of the buffer. > + */ > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & > + (UART_XMIT_SIZE - 1); uart_xmit_advance() > + > + /* > + * If there are any pending burst count, data is handled by > + * transmitting DWORDs at a time. > + */ > + if (valid_burst_count && (xmit->tail < > + (UART_XMIT_SIZE - UART_BURST_SIZE))) > + break; > + } > +} This nested double loop is _really_ hard to follow. It's actually terrible with cut & paste mistakes. Could these all loops be simply replaced by something like this pseudo code (a single loop): while (data_empty_count) { cnt = CIRC_CNT_TO_END(); if (!cnt) break; if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned() writeb(); cnt = 1; } else { writel() cnt = UART_BURST_SIZE; } uart_xmit_advance(cnt); data_empty_count -= cnt; } ? > +static void pci1xxxx_tx_burst(struct uart_port *port, u32 uart_status) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + u32 valid_byte_count; > + int data_empty_count; > + struct circ_buf *xmit; > + > + xmit = &port->state->xmit; > + > + if (port->x_char) { > + writeb(port->x_char, port->membase + UART_TX); > + port->icount.tx++; > + port->x_char = 0; > + return; > + } > + > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { > + port->ops->stop_tx(port); This looks weird standing here. You should handle this below along with RPM. > + } else { The condition should be IMO inverted with this block in its body: > + data_empty_count = (pci1xxxx_read_burst_status(port) & > + UART_BST_STAT_TX_COUNT_MASK) >> 8; > + do { > + valid_byte_count = uart_circ_chars_pending(xmit); > + > + pci1xxxx_process_write_data(port, xmit, > + &data_empty_count, > + &valid_byte_count); > + > + port->icount.tx++; Why do you increase the stats only once per burst? (Solved by uart_xmit_advance() above.) > + if (uart_circ_empty(xmit)) > + break; > + } while (data_empty_count && valid_byte_count); > + } > + > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(port); > + > + /* > + * With RPM enabled, we have to wait until the FIFO is empty before > + * the HW can go idle. So we get here once again with empty FIFO and > + * disable the interrupt and RPM in __stop_tx() > + */ > + if (uart_circ_empty(xmit) && !(up->capabilities & UART_CAP_RPM)) > + port->ops->stop_tx(port); I wonder why this driver needs this and others don't? Should they be fixed too? > +} > + > static int pci1xxxx_handle_irq(struct uart_port *port) > { > unsigned long flags; > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct uart_port *port) > if (status & UART_BST_STAT_LSR_RX_MASK) > pci1xxxx_rx_burst(port, status); > > + if (status & UART_BST_STAT_LSR_THRE) > + pci1xxxx_tx_burst(port, status); > + > spin_unlock_irqrestore(&port->lock, flags); > > return 1;
Hi Jiri Slaby, Thanks for reviewing the patch. Will address the comments in the next patch revision. On Mon, 2024-02-05 at 08:56 +0100, Jiri Slaby wrote: > [Some people who received this message don't often get email from > jirislaby@kernel.org. Learn why this is important at > https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 25. 01. 24, 11:00, Rengarajan S wrote: > > pci1xxxx_handle_irq reads the burst status and checks if the FIFO > > is empty and is ready to accept the incoming data. The handling is > > done in pci1xxxx_tx_burst where each transaction processes data in > > block of DWORDs, while any remaining bytes are processed > > individually, > > one byte at a time. > > > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > > --- > > drivers/tty/serial/8250/8250_pci1xxxx.c | 106 > > ++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c > > b/drivers/tty/serial/8250/8250_pci1xxxx.c > > index 558c4c7f3104..d53605bf908d 100644 > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c > ... > > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct > > uart_port *port, u32 uart_status) > > } > > } > > > > +static void pci1xxxx_process_write_data(struct uart_port *port, > > + struct circ_buf *xmit, > > + int *data_empty_count, > > count is unsigned, right? > > > + u32 *valid_byte_count) > > +{ > > + u32 valid_burst_count = *valid_byte_count / UART_BURST_SIZE; > > + > > + /* > > + * Each transaction transfers data in DWORDs. If there are > > less than > > + * four remaining valid_byte_count to transfer or if the > > circular > > + * buffer has insufficient space for a DWORD, the data is > > transferred > > + * one byte at a time. > > + */ > > + while (valid_burst_count) { > > + if (*data_empty_count - UART_BURST_SIZE < 0) > > Huh? > > *data_empty_count < UART_BURST_SIZE > > instead? > > > + break; > > + if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE)) > > Is this the same as easy to understand: > > CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) < > UART_BURST_SIZE > > ? > > > + break; > > + writel(*(unsigned int *)&xmit->buf[xmit->tail], > > + port->membase + UART_TX_BURST_FIFO); > > What about unaligned accesses? > > And you really wanted to spell u32 explicitly, not uint. > > > + *valid_byte_count -= UART_BURST_SIZE; > > + *data_empty_count -= UART_BURST_SIZE; > > + valid_burst_count -= UART_BYTE_SIZE; > > + > > + xmit->tail = (xmit->tail + UART_BURST_SIZE) & > > + (UART_XMIT_SIZE - 1); > > uart_xmit_advance() > > > + } > > + > > + while (*valid_byte_count) { > > + if (*data_empty_count - UART_BYTE_SIZE < 0) > > + break; > > + writeb(xmit->buf[xmit->tail], port->membase + > > + UART_TX_BYTE_FIFO); > > + *data_empty_count -= UART_BYTE_SIZE; > > + *valid_byte_count -= UART_BYTE_SIZE; > > + > > + /* > > + * When the tail of the circular buffer is reached, > > the next > > + * byte is transferred to the beginning of the > > buffer. > > + */ > > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & > > + (UART_XMIT_SIZE - 1); > > uart_xmit_advance() > > > + > > + /* > > + * If there are any pending burst count, data is > > handled by > > + * transmitting DWORDs at a time. > > + */ > > + if (valid_burst_count && (xmit->tail < > > + (UART_XMIT_SIZE - UART_BURST_SIZE))) > > + break; > > + } > > +} > > This nested double loop is _really_ hard to follow. It's actually > terrible with cut & paste mistakes. > > Could these all loops be simply replaced by something like this > pseudo > code (a single loop): > > while (data_empty_count) { > cnt = CIRC_CNT_TO_END(); > if (!cnt) > break; > if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned() > writeb(); > cnt = 1; > } else { > writel() > cnt = UART_BURST_SIZE; > } > uart_xmit_advance(cnt); > data_empty_count -= cnt; > } > > ? > > > > +static void pci1xxxx_tx_burst(struct uart_port *port, u32 > > uart_status) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + u32 valid_byte_count; > > + int data_empty_count; > > + struct circ_buf *xmit; > > + > > + xmit = &port->state->xmit; > > + > > + if (port->x_char) { > > + writeb(port->x_char, port->membase + UART_TX); > > + port->icount.tx++; > > + port->x_char = 0; > > + return; > > + } > > + > > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { > > + port->ops->stop_tx(port); > > This looks weird standing here. You should handle this below along > with RPM. > > > + } else { > > The condition should be IMO inverted with this block in its body: > > > + data_empty_count = (pci1xxxx_read_burst_status(port) > > & > > + UART_BST_STAT_TX_COUNT_MASK) >> > > 8; > > + do { > > + valid_byte_count = > > uart_circ_chars_pending(xmit); > > + > > + pci1xxxx_process_write_data(port, xmit, > > + > > &data_empty_count, > > + > > &valid_byte_count); > > + > > + port->icount.tx++; > > Why do you increase the stats only once per burst? (Solved by > uart_xmit_advance() above.) > > > + if (uart_circ_empty(xmit)) > > + break; > > + } while (data_empty_count && valid_byte_count); > > + } > > + > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > + uart_write_wakeup(port); > > + > > + /* > > + * With RPM enabled, we have to wait until the FIFO is empty > > before > > + * the HW can go idle. So we get here once again with empty > > FIFO and > > + * disable the interrupt and RPM in __stop_tx() > > + */ > > + if (uart_circ_empty(xmit) && !(up->capabilities & > > UART_CAP_RPM)) > > + port->ops->stop_tx(port); > > I wonder why this driver needs this and others don't? Should they be > fixed too? > > > +} > > + > > static int pci1xxxx_handle_irq(struct uart_port *port) > > { > > unsigned long flags; > > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct uart_port > > *port) > > if (status & UART_BST_STAT_LSR_RX_MASK) > > pci1xxxx_rx_burst(port, status); > > > > + if (status & UART_BST_STAT_LSR_THRE) > > + pci1xxxx_tx_burst(port, status); > > + > > spin_unlock_irqrestore(&port->lock, flags); > > > > return 1; > > -- > js > suse labs >
Hi Jiri Slaby, The patch has been accepted and added in the tty-git tree and will be merged during the next merged window. Should the changes be given as a seperate incremental patch or should we re-submit this patch again. On Thu, 2024-02-08 at 02:49 +0000, Rengarajan S - I69107 wrote: > Hi Jiri Slaby, > > Thanks for reviewing the patch. Will address the comments in the next > patch revision. > > On Mon, 2024-02-05 at 08:56 +0100, Jiri Slaby wrote: > > [Some people who received this message don't often get email from > > jirislaby@kernel.org. Learn why this is important at > > https://aka.ms/LearnAboutSenderIdentification ] > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On 25. 01. 24, 11:00, Rengarajan S wrote: > > > pci1xxxx_handle_irq reads the burst status and checks if the FIFO > > > is empty and is ready to accept the incoming data. The handling > > > is > > > done in pci1xxxx_tx_burst where each transaction processes data > > > in > > > block of DWORDs, while any remaining bytes are processed > > > individually, > > > one byte at a time. > > > > > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > > > --- > > > drivers/tty/serial/8250/8250_pci1xxxx.c | 106 > > > ++++++++++++++++++++++++ > > > 1 file changed, 106 insertions(+) > > > > > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c > > > b/drivers/tty/serial/8250/8250_pci1xxxx.c > > > index 558c4c7f3104..d53605bf908d 100644 > > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c > > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c > > ... > > > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct > > > uart_port *port, u32 uart_status) > > > } > > > } > > > > > > +static void pci1xxxx_process_write_data(struct uart_port *port, > > > + struct circ_buf *xmit, > > > + int *data_empty_count, > > > > count is unsigned, right? > > > > > + u32 *valid_byte_count) > > > +{ > > > + u32 valid_burst_count = *valid_byte_count / > > > UART_BURST_SIZE; > > > + > > > + /* > > > + * Each transaction transfers data in DWORDs. If there are > > > less than > > > + * four remaining valid_byte_count to transfer or if the > > > circular > > > + * buffer has insufficient space for a DWORD, the data is > > > transferred > > > + * one byte at a time. > > > + */ > > > + while (valid_burst_count) { > > > + if (*data_empty_count - UART_BURST_SIZE < 0) > > > > Huh? > > > > *data_empty_count < UART_BURST_SIZE > > > > instead? > > > > > + break; > > > + if (xmit->tail > (UART_XMIT_SIZE - > > > UART_BURST_SIZE)) > > > > Is this the same as easy to understand: > > > > CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) < > > UART_BURST_SIZE > > > > ? > > > > > + break; > > > + writel(*(unsigned int *)&xmit->buf[xmit->tail], > > > + port->membase + UART_TX_BURST_FIFO); > > > > What about unaligned accesses? > > > > And you really wanted to spell u32 explicitly, not uint. > > > > > + *valid_byte_count -= UART_BURST_SIZE; > > > + *data_empty_count -= UART_BURST_SIZE; > > > + valid_burst_count -= UART_BYTE_SIZE; > > > + > > > + xmit->tail = (xmit->tail + UART_BURST_SIZE) & > > > + (UART_XMIT_SIZE - 1); > > > > uart_xmit_advance() > > > > > + } > > > + > > > + while (*valid_byte_count) { > > > + if (*data_empty_count - UART_BYTE_SIZE < 0) > > > + break; > > > + writeb(xmit->buf[xmit->tail], port->membase + > > > + UART_TX_BYTE_FIFO); > > > + *data_empty_count -= UART_BYTE_SIZE; > > > + *valid_byte_count -= UART_BYTE_SIZE; > > > + > > > + /* > > > + * When the tail of the circular buffer is reached, > > > the next > > > + * byte is transferred to the beginning of the > > > buffer. > > > + */ > > > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & > > > + (UART_XMIT_SIZE - 1); > > > > uart_xmit_advance() > > > > > + > > > + /* > > > + * If there are any pending burst count, data is > > > handled by > > > + * transmitting DWORDs at a time. > > > + */ > > > + if (valid_burst_count && (xmit->tail < > > > + (UART_XMIT_SIZE - UART_BURST_SIZE))) > > > + break; > > > + } > > > +} > > > > This nested double loop is _really_ hard to follow. It's actually > > terrible with cut & paste mistakes. > > > > Could these all loops be simply replaced by something like this > > pseudo > > code (a single loop): > > > > while (data_empty_count) { > > cnt = CIRC_CNT_TO_END(); > > if (!cnt) > > break; > > if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned() > > writeb(); > > cnt = 1; > > } else { > > writel() > > cnt = UART_BURST_SIZE; > > } > > uart_xmit_advance(cnt); > > data_empty_count -= cnt; > > } > > > > ? > > > > > > > +static void pci1xxxx_tx_burst(struct uart_port *port, u32 > > > uart_status) > > > +{ > > > + struct uart_8250_port *up = up_to_u8250p(port); > > > + u32 valid_byte_count; > > > + int data_empty_count; > > > + struct circ_buf *xmit; > > > + > > > + xmit = &port->state->xmit; > > > + > > > + if (port->x_char) { > > > + writeb(port->x_char, port->membase + UART_TX); > > > + port->icount.tx++; > > > + port->x_char = 0; > > > + return; > > > + } > > > + > > > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { > > > + port->ops->stop_tx(port); > > > > This looks weird standing here. You should handle this below along > > with RPM. > > > > > + } else { > > > > The condition should be IMO inverted with this block in its body: > > > > > + data_empty_count = > > > (pci1xxxx_read_burst_status(port) > > > & > > > + UART_BST_STAT_TX_COUNT_MASK) >> > > > 8; > > > + do { > > > + valid_byte_count = > > > uart_circ_chars_pending(xmit); > > > + > > > + pci1xxxx_process_write_data(port, xmit, > > > + > > > &data_empty_count, > > > + > > > &valid_byte_count); > > > + > > > + port->icount.tx++; > > > > Why do you increase the stats only once per burst? (Solved by > > uart_xmit_advance() above.) > > > > > + if (uart_circ_empty(xmit)) > > > + break; > > > + } while (data_empty_count && valid_byte_count); > > > + } > > > + > > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > > + uart_write_wakeup(port); > > > + > > > + /* > > > + * With RPM enabled, we have to wait until the FIFO is > > > empty > > > before > > > + * the HW can go idle. So we get here once again with > > > empty > > > FIFO and > > > + * disable the interrupt and RPM in __stop_tx() > > > + */ > > > + if (uart_circ_empty(xmit) && !(up->capabilities & > > > UART_CAP_RPM)) > > > + port->ops->stop_tx(port); > > > > I wonder why this driver needs this and others don't? Should they > > be > > fixed too? > > > > > +} > > > + > > > static int pci1xxxx_handle_irq(struct uart_port *port) > > > { > > > unsigned long flags; > > > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct > > > uart_port > > > *port) > > > if (status & UART_BST_STAT_LSR_RX_MASK) > > > pci1xxxx_rx_burst(port, status); > > > > > > + if (status & UART_BST_STAT_LSR_THRE) > > > + pci1xxxx_tx_burst(port, status); > > > + > > > spin_unlock_irqrestore(&port->lock, flags); > > > > > > return 1; > > > > -- > > js > > suse labs > > >
On 09. 02. 24, 5:52, Rengarajan.S@microchip.com wrote: > Hi Jiri Slaby, > > The patch has been accepted and added in the tty-git tree and will be > merged during the next merged window. Should the changes be given as a > seperate incremental patch or should we re-submit this patch again. Hi, as you write, you cannot change the patch as it is already queued. PLease submit changes on top. thanks,
On Fri, Feb 09, 2024 at 07:38:49AM +0100, Jiri Slaby wrote: > On 09. 02. 24, 5:52, Rengarajan.S@microchip.com wrote: > > Hi Jiri Slaby, > > > > The patch has been accepted and added in the tty-git tree and will be > > merged during the next merged window. Should the changes be given as a > > seperate incremental patch or should we re-submit this patch again. > > Hi, > > as you write, you cannot change the patch as it is already queued. PLease > submit changes on top. Or, if the original is so bad, we can revert them now and wait for new ones later, just let me know. thanks, greg k-h
Hi Greg, Since, Most of the comments were alignment related and no functionalities were affected, the current patch can be merged in the next window. Will share another patch with the suggested incremental changes shortly. Thanks!! On Fri, 2024-02-09 at 10:20 +0000, Greg KH wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Feb 09, 2024 at 07:38:49AM +0100, Jiri Slaby wrote: > > On 09. 02. 24, 5:52, Rengarajan.S@microchip.com wrote: > > > Hi Jiri Slaby, > > > > > > The patch has been accepted and added in the tty-git tree and > > > will be > > > merged during the next merged window. Should the changes be given > > > as a > > > seperate incremental patch or should we re-submit this patch > > > again. > > > > Hi, > > > > as you write, you cannot change the patch as it is already queued. > > PLease > > submit changes on top. > > Or, if the original is so bad, we can revert them now and wait for > new > ones later, just let me know. > > thanks, > > greg k-h
Hi Jiri Slaby, On Mon, 2024-02-05 at 08:56 +0100, Jiri Slaby wrote: > [Some people who received this message don't often get email from > jirislaby@kernel.org. Learn why this is important at > https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On 25. 01. 24, 11:00, Rengarajan S wrote: > > pci1xxxx_handle_irq reads the burst status and checks if the FIFO > > is empty and is ready to accept the incoming data. The handling is > > done in pci1xxxx_tx_burst where each transaction processes data in > > block of DWORDs, while any remaining bytes are processed > > individually, > > one byte at a time. > > > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> > > --- > > drivers/tty/serial/8250/8250_pci1xxxx.c | 106 > > ++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c > > b/drivers/tty/serial/8250/8250_pci1xxxx.c > > index 558c4c7f3104..d53605bf908d 100644 > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c > ... > > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct > > uart_port *port, u32 uart_status) > > } > > } > > > > +static void pci1xxxx_process_write_data(struct uart_port *port, > > + struct circ_buf *xmit, > > + int *data_empty_count, > > count is unsigned, right? > > > + u32 *valid_byte_count) > > +{ > > + u32 valid_burst_count = *valid_byte_count / UART_BURST_SIZE; > > + > > + /* > > + * Each transaction transfers data in DWORDs. If there are > > less than > > + * four remaining valid_byte_count to transfer or if the > > circular > > + * buffer has insufficient space for a DWORD, the data is > > transferred > > + * one byte at a time. > > + */ > > + while (valid_burst_count) { > > + if (*data_empty_count - UART_BURST_SIZE < 0) > > Huh? > > *data_empty_count < UART_BURST_SIZE > > instead? > > > + break; > > + if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE)) > > Is this the same as easy to understand: > > CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) < > UART_BURST_SIZE > > ? > > > + break; > > + writel(*(unsigned int *)&xmit->buf[xmit->tail], > > + port->membase + UART_TX_BURST_FIFO); > > What about unaligned accesses? > > And you really wanted to spell u32 explicitly, not uint. > > > + *valid_byte_count -= UART_BURST_SIZE; > > + *data_empty_count -= UART_BURST_SIZE; > > + valid_burst_count -= UART_BYTE_SIZE; > > + > > + xmit->tail = (xmit->tail + UART_BURST_SIZE) & > > + (UART_XMIT_SIZE - 1); > > uart_xmit_advance() > > > + } > > + > > + while (*valid_byte_count) { > > + if (*data_empty_count - UART_BYTE_SIZE < 0) > > + break; > > + writeb(xmit->buf[xmit->tail], port->membase + > > + UART_TX_BYTE_FIFO); > > + *data_empty_count -= UART_BYTE_SIZE; > > + *valid_byte_count -= UART_BYTE_SIZE; > > + > > + /* > > + * When the tail of the circular buffer is reached, > > the next > > + * byte is transferred to the beginning of the > > buffer. > > + */ > > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & > > + (UART_XMIT_SIZE - 1); > > uart_xmit_advance() > > > + > > + /* > > + * If there are any pending burst count, data is > > handled by > > + * transmitting DWORDs at a time. > > + */ > > + if (valid_burst_count && (xmit->tail < > > + (UART_XMIT_SIZE - UART_BURST_SIZE))) > > + break; > > + } > > +} > > This nested double loop is _really_ hard to follow. It's actually > terrible with cut & paste mistakes. Will avoid nested loops in the above statement in the next patch revision. > > Could these all loops be simply replaced by something like this > pseudo > code (a single loop): > > while (data_empty_count) { > cnt = CIRC_CNT_TO_END(); > if (!cnt) > break; > if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned() > writeb(); > cnt = 1; > } else { > writel() > cnt = UART_BURST_SIZE; > } > uart_xmit_advance(cnt); > data_empty_count -= cnt; > } > > ? > In order to differentiate Burst mode handling with byte mode handling had seperate loops for both. Since, having single while loop also does not align with rx implementation(where we have seperate handling of burst and byte) will it be ok to retain the current implementation? > > > +static void pci1xxxx_tx_burst(struct uart_port *port, u32 > > uart_status) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + u32 valid_byte_count; > > + int data_empty_count; > > + struct circ_buf *xmit; > > + > > + xmit = &port->state->xmit; > > + > > + if (port->x_char) { > > + writeb(port->x_char, port->membase + UART_TX); > > + port->icount.tx++; > > + port->x_char = 0; > > + return; > > + } > > + > > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { > > + port->ops->stop_tx(port); > > This looks weird standing here. You should handle this below along > with RPM. > > > + } else { > > The condition should be IMO inverted with this block in its body: > > > + data_empty_count = (pci1xxxx_read_burst_status(port) > > & > > + UART_BST_STAT_TX_COUNT_MASK) >> > > 8; > > + do { > > + valid_byte_count = > > uart_circ_chars_pending(xmit); > > + > > + pci1xxxx_process_write_data(port, xmit, > > + > > &data_empty_count, > > + > > &valid_byte_count); > > + > > + port->icount.tx++; > > Why do you increase the stats only once per burst? (Solved by > uart_xmit_advance() above.) > > > + if (uart_circ_empty(xmit)) > > + break; > > + } while (data_empty_count && valid_byte_count); > > + } > > + > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > + uart_write_wakeup(port); > > + > > + /* > > + * With RPM enabled, we have to wait until the FIFO is empty > > before > > + * the HW can go idle. So we get here once again with empty > > FIFO and > > + * disable the interrupt and RPM in __stop_tx() > > + */ > > + if (uart_circ_empty(xmit) && !(up->capabilities & > > UART_CAP_RPM)) > > + port->ops->stop_tx(port); > > I wonder why this driver needs this and others don't? Should they be > fixed too? > > > +} > > + > > static int pci1xxxx_handle_irq(struct uart_port *port) > > { > > unsigned long flags; > > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct uart_port > > *port) > > if (status & UART_BST_STAT_LSR_RX_MASK) > > pci1xxxx_rx_burst(port, status); > > > > + if (status & UART_BST_STAT_LSR_THRE) > > + pci1xxxx_tx_burst(port, status); > > + > > spin_unlock_irqrestore(&port->lock, flags); > > > > return 1; > > -- > js > suse labs >
diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c b/drivers/tty/serial/8250/8250_pci1xxxx.c index 558c4c7f3104..d53605bf908d 100644 --- a/drivers/tty/serial/8250/8250_pci1xxxx.c +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c @@ -67,6 +67,7 @@ #define SYSLOCK_RETRY_CNT 1000 #define UART_RX_BYTE_FIFO 0x00 +#define UART_TX_BYTE_FIFO 0x00 #define UART_FIFO_CTL 0x02 #define UART_ACTV_REG 0x11 @@ -100,6 +101,7 @@ #define UART_RESET_D3_RESET_DISABLE BIT(16) #define UART_BURST_STATUS_REG 0x9C +#define UART_TX_BURST_FIFO 0xA0 #define UART_RX_BURST_FIFO 0xA4 #define MAX_PORTS 4 @@ -109,6 +111,7 @@ #define UART_BURST_SIZE 4 #define UART_BST_STAT_RX_COUNT_MASK 0x00FF +#define UART_BST_STAT_TX_COUNT_MASK 0xFF00 #define UART_BST_STAT_IIR_INT_PEND 0x100000 #define UART_LSR_OVERRUN_ERR_CLR 0x43 #define UART_BST_STAT_LSR_RX_MASK 0x9F000000 @@ -116,6 +119,7 @@ #define UART_BST_STAT_LSR_OVERRUN_ERR 0x2000000 #define UART_BST_STAT_LSR_PARITY_ERR 0x4000000 #define UART_BST_STAT_LSR_FRAME_ERR 0x8000000 +#define UART_BST_STAT_LSR_THRE 0x20000000 struct pci1xxxx_8250 { unsigned int nr; @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct uart_port *port, u32 uart_status) } } +static void pci1xxxx_process_write_data(struct uart_port *port, + struct circ_buf *xmit, + int *data_empty_count, + u32 *valid_byte_count) +{ + u32 valid_burst_count = *valid_byte_count / UART_BURST_SIZE; + + /* + * Each transaction transfers data in DWORDs. If there are less than + * four remaining valid_byte_count to transfer or if the circular + * buffer has insufficient space for a DWORD, the data is transferred + * one byte at a time. + */ + while (valid_burst_count) { + if (*data_empty_count - UART_BURST_SIZE < 0) + break; + if (xmit->tail > (UART_XMIT_SIZE - UART_BURST_SIZE)) + break; + writel(*(unsigned int *)&xmit->buf[xmit->tail], + port->membase + UART_TX_BURST_FIFO); + *valid_byte_count -= UART_BURST_SIZE; + *data_empty_count -= UART_BURST_SIZE; + valid_burst_count -= UART_BYTE_SIZE; + + xmit->tail = (xmit->tail + UART_BURST_SIZE) & + (UART_XMIT_SIZE - 1); + } + + while (*valid_byte_count) { + if (*data_empty_count - UART_BYTE_SIZE < 0) + break; + writeb(xmit->buf[xmit->tail], port->membase + + UART_TX_BYTE_FIFO); + *data_empty_count -= UART_BYTE_SIZE; + *valid_byte_count -= UART_BYTE_SIZE; + + /* + * When the tail of the circular buffer is reached, the next + * byte is transferred to the beginning of the buffer. + */ + xmit->tail = (xmit->tail + UART_BYTE_SIZE) & + (UART_XMIT_SIZE - 1); + + /* + * If there are any pending burst count, data is handled by + * transmitting DWORDs at a time. + */ + if (valid_burst_count && (xmit->tail < + (UART_XMIT_SIZE - UART_BURST_SIZE))) + break; + } +} + +static void pci1xxxx_tx_burst(struct uart_port *port, u32 uart_status) +{ + struct uart_8250_port *up = up_to_u8250p(port); + u32 valid_byte_count; + int data_empty_count; + struct circ_buf *xmit; + + xmit = &port->state->xmit; + + if (port->x_char) { + writeb(port->x_char, port->membase + UART_TX); + port->icount.tx++; + port->x_char = 0; + return; + } + + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) { + port->ops->stop_tx(port); + } else { + data_empty_count = (pci1xxxx_read_burst_status(port) & + UART_BST_STAT_TX_COUNT_MASK) >> 8; + do { + valid_byte_count = uart_circ_chars_pending(xmit); + + pci1xxxx_process_write_data(port, xmit, + &data_empty_count, + &valid_byte_count); + + port->icount.tx++; + if (uart_circ_empty(xmit)) + break; + } while (data_empty_count && valid_byte_count); + } + + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) + uart_write_wakeup(port); + + /* + * With RPM enabled, we have to wait until the FIFO is empty before + * the HW can go idle. So we get here once again with empty FIFO and + * disable the interrupt and RPM in __stop_tx() + */ + if (uart_circ_empty(xmit) && !(up->capabilities & UART_CAP_RPM)) + port->ops->stop_tx(port); +} + static int pci1xxxx_handle_irq(struct uart_port *port) { unsigned long flags; @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct uart_port *port) if (status & UART_BST_STAT_LSR_RX_MASK) pci1xxxx_rx_burst(port, status); + if (status & UART_BST_STAT_LSR_THRE) + pci1xxxx_tx_burst(port, status); + spin_unlock_irqrestore(&port->lock, flags); return 1;