Message ID | 20231026-mbly-uart-v1-6-9258eea297d3@bootlin.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp573366vqb; Thu, 26 Oct 2023 03:42:40 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFgo5wYSZwCi2zBnPFSZEjl4zYl1Q4X2NxYmbgfZ3JKbNb4mtydK0yNaq/sS9EGCt88iFMt X-Received: by 2002:a05:6102:2005:b0:44d:626b:94da with SMTP id p5-20020a056102200500b0044d626b94damr17384332vsr.32.1698316960259; Thu, 26 Oct 2023 03:42:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698316960; cv=none; d=google.com; s=arc-20160816; b=K/CwkjndD/FyMAPiCyPFHV5j/7NX5xNc0vGNqajFOTOgTp/nfJwx/wY7ExcpF7wHhr YJWq+6Og3QHZpmGY5GkFoDXvjXtbs9n2tqjz+gjrjPLZf0F87254A8nCH2R3FJbHPkWV k1SUM7F3XOCKefggbe3gFNHw2Wr4TchOvjuqc8LqBNIGJZixS4qJiEs0uBg2smUGGJOD zEQMfImxBkx7GjLaJZZ+7OSyDTOosFFTsT/xbPKt3nATQ1fayZPmQ5x4YdrAkisM9NkR 6Y/9KeY3Q/1wzTEikKmrklEe4WMzQYme3Qnuxrinv1MH3GzUmbOnOWC3FIF1uyHOAk7W mxdg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=89QZghIzrhbdFSkiY8cfj50jVqvDezZ7qykG0hXVcLg=; fh=0GQDSxykeLgRwmrHkUZ9jyOse/Zvi+zOzBe9zh1Wo7Y=; b=Fb2xKROUj/hGrNS49KpkknuMRg06cMSGxlaoxdh0n3V/lcXEAhq9Keg91iSHyyHl3A +LTC8wJp6P7w4qhA7UR/NzKi4qx5qwAxaJayVKqmLIGTgJIr4QcPfgaHjJNw8IpuVitw 2cy1yjfOwdRa+cRJUQfQZzzzRywakJq9eI0seyzxyEzb2nLM9GhqjAjmGf2iagUSTHxj mvmSGeWe3TvXAG0/NnV1LAj3loV3m3z+6pUli6j8kvWoYUHpcyLyAMAdiZZ5w7+iYIs9 e614j7nWnW/jkjptb2y19zFV44Fv3o8dGQ9eOWONSJ08Yrc1cIM3rN8x8Jxb6NGaF/wL bcqg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=X87hLIeP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id f1-20020a25a401000000b00da028890e3esi8289415ybi.358.2023.10.26.03.42.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Oct 2023 03:42:40 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=X87hLIeP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 5046F8135CFB; Thu, 26 Oct 2023 03:42:29 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344743AbjJZKmQ (ORCPT <rfc822;aposhian.dev@gmail.com> + 26 others); Thu, 26 Oct 2023 06:42:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58720 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235012AbjJZKl5 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 26 Oct 2023 06:41:57 -0400 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CC3D4D42; Thu, 26 Oct 2023 03:41:53 -0700 (PDT) Received: by mail.gandi.net (Postfix) with ESMTPSA id BD21024000C; Thu, 26 Oct 2023 10:41:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1698316912; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=89QZghIzrhbdFSkiY8cfj50jVqvDezZ7qykG0hXVcLg=; b=X87hLIePGytsjaYLRg0arx9Bl9oHNsdE6MvxwwnFpzyIW4SBLwzF8LBQjdtjzJGJUjnJU9 Xq+dAcQUJ87nfTzOpq+VzDum0q6v+s+BVhXPoG3PQY6l7ROEgx8NcPPexfcwcHFG23E3FX YBF621ORbbZnEf25HB83TF+7gmk1pRmGOdmR9Ve4zS/Czav72svci23wHftEGl7Wjw/G8T BzbkPeKJyQrly/u+eqkiITRAI8u1zXG5/RVRayCGpi8Ywz4Kx6jeePPn0PKtaAC1TtAfcv BRbuMVohUSmmAyWicWanqn3fHzsn3L6Whccyhm4MohTgoa06v0f6xxa7MI3YIA== From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com> Date: Thu, 26 Oct 2023 12:41:23 +0200 Subject: [PATCH 6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20231026-mbly-uart-v1-6-9258eea297d3@bootlin.com> References: <20231026-mbly-uart-v1-0-9258eea297d3@bootlin.com> In-Reply-To: <20231026-mbly-uart-v1-0-9258eea297d3@bootlin.com> To: Russell King <linux@armlinux.org.uk>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org> Cc: linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>, Gregory CLEMENT <gregory.clement@bootlin.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>, Tawfik Bayouk <tawfik.bayouk@mobileye.com>, =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com> X-Mailer: b4 0.12.3 X-GND-Sasl: theo.lebrun@bootlin.com X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 26 Oct 2023 03:42:29 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780814404792042872 X-GMAIL-MSGID: 1780814404792042872 |
Series |
Cleanup AMBA PL011 driver
|
|
Commit Message
Théo Lebrun
Oct. 26, 2023, 10:41 a.m. UTC
pl011_console_get_options() gets called to retrieve currently configured
options from the registers. Previously, LCRH_TX.WLEN was being parsed
as either 7 or 8 (fallback). Hardware supports values from 5 to 8
inclusive, which pl011_set_termios() exploits for example.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/tty/serial/amba-pl011.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Comments
On Thu, 26 Oct 2023, Théo Lebrun wrote: > pl011_console_get_options() gets called to retrieve currently configured > options from the registers. Previously, LCRH_TX.WLEN was being parsed > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > inclusive, which pl011_set_termios() exploits for example. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/tty/serial/amba-pl011.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 5774d48c7f16..b2062e4cbbab 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, > *parity = 'o'; > } > > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) > - *bits = 7; > - else > - *bits = 8; > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ 0x60 needs to be replaced with a named define!
Hi, On Thu Oct 26, 2023 at 1:13 PM CEST, Ilpo Järvinen wrote: > On Thu, 26 Oct 2023, Théo Lebrun wrote: > > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > > index 5774d48c7f16..b2062e4cbbab 100644 > > --- a/drivers/tty/serial/amba-pl011.c > > +++ b/drivers/tty/serial/amba-pl011.c > > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, > > *parity = 'o'; > > } > > > > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) > > - *bits = 7; > > - else > > - *bits = 8; > > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ > > 0x60 needs to be replaced with a named define! Fixed locally for the next revision, thanks! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > pl011_console_get_options() gets called to retrieve currently configured > options from the registers. Previously, LCRH_TX.WLEN was being parsed > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > inclusive, which pl011_set_termios() exploits for example. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> With Ilpo's comment fixed: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hello, On Thu Oct 26, 2023 at 3:48 PM CEST, Linus Walleij wrote: > On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > pl011_console_get_options() gets called to retrieve currently configured > > options from the registers. Previously, LCRH_TX.WLEN was being parsed > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > > inclusive, which pl011_set_termios() exploits for example. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > With Ilpo's comment fixed: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> It's been fixed locally. Thank you for your review Linus! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, 26 Oct 2023 12:41:23 +0200 Théo Lebrun <theo.lebrun@bootlin.com> wrote: Hi, I would change the commit title to better indicate that you add support for bits 5 and 6, which was missing. Maybe "Add support for 5 and 6 bits in..." ? > pl011_console_get_options() gets called to retrieve currently configured > options from the registers. Previously, LCRH_TX.WLEN was being parsed It took me some time to understand your explanation :) Maybe change to: "Previously, only 7 or 8 bits were supported." > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 Add bits: "5 to 8 bits..." And indicate that this patch adds support for 5 and 6 bits. > inclusive, which pl011_set_termios() exploits for example. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/tty/serial/amba-pl011.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 5774d48c7f16..b2062e4cbbab 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, > *parity = 'o'; > } > > - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) > - *bits = 7; > - else > - *bits = 8; > + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ Capital "F" -> "From...". And add "bits" -> "From 5 to 8 bits..." Hugo. > > ibrd = pl011_read(uap, REG_IBRD); > fbrd = pl011_read(uap, REG_FBRD); > > -- > 2.41.0 >
Hello, On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote: > On Thu, 26 Oct 2023 12:41:23 +0200 > Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hi, > I would change the commit title to better indicate that you add support > for bits 5 and 6, which was missing. > > Maybe "Add support for 5 and 6 bits in..." ? > > > pl011_console_get_options() gets called to retrieve currently configured > > options from the registers. Previously, LCRH_TX.WLEN was being parsed > > It took me some time to understand your explanation :) Maybe change > to: > > "Previously, only 7 or 8 bits were supported." > > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > > Add bits: > > "5 to 8 bits..." > > And indicate that this patch adds support for 5 and 6 bits. I agree the whole commit message is unclear. Let's rewrite it. What do you think of the following: tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup If no options are given at console setup, we parse hardware register LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6 or 8 bits per word, we fallback to 8 bits. Change that behavior to parse the whole range available: from 5 to 8 bits per word. Note that we don't add support for 5/6 bits, we only update the parsing of the regs (if no options are passed at setup) to reflect the current hardware config. The behavior will be different only if the inherited value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits word length, now we guess the right value. What's your opinion on this new commit message? Thanks! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Oct 31, 2023 at 10:35:29AM +0100, Théo Lebrun wrote: > Hello, > > On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote: > > On Thu, 26 Oct 2023 12:41:23 +0200 > > Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Hi, > > I would change the commit title to better indicate that you add support > > for bits 5 and 6, which was missing. > > > > Maybe "Add support for 5 and 6 bits in..." ? > > > > > pl011_console_get_options() gets called to retrieve currently configured > > > options from the registers. Previously, LCRH_TX.WLEN was being parsed > > > > It took me some time to understand your explanation :) Maybe change > > to: > > > > "Previously, only 7 or 8 bits were supported." > > > > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > > > > Add bits: > > > > "5 to 8 bits..." > > > > And indicate that this patch adds support for 5 and 6 bits. > > I agree the whole commit message is unclear. Let's rewrite it. What do > you think of the following: > > tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup > > If no options are given at console setup, we parse hardware register > LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits > value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6 > or 8 bits per word, we fallback to 8 bits. > > Change that behavior to parse the whole range available: from 5 to 8 > bits per word. > > Note that we don't add support for 5/6 bits, we only update the parsing > of the regs (if no options are passed at setup) to reflect the current > hardware config. The behavior will be different only if the inherited > value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits > word length, now we guess the right value. > > What's your opinion on this new commit message? There is no point in supporting 5 or 6 bits for console usage. Think about it. What values are going to be sent over the console? It'll be ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha characters into control characters, punctuation and numbers. 5-bit would be all control characters. So there's no point trying to do anything with 5 or 6 bits per byte, and I decided we might as well take that as an error (or maybe a case that the hardware has not been setup) and default to 8 bits per byte.
Hello, On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > There is no point in supporting 5 or 6 bits for console usage. Think > about it. What values are going to be sent over the console? It'll be > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > characters into control characters, punctuation and numbers. 5-bit > would be all control characters. > > So there's no point trying to do anything with 5 or 6 bits per byte, > and I decided we might as well take that as an error (or maybe a > case that the hardware has not been setup) and default to 8 bits per > byte. I see your point. Two things come to mind: - I added this parsing of 5/6 bits to be symmetrical with pl011_set_termios that handles 5/6 properly. Should pl011_set_termios be modified then? - If a value of 5 or 6 means the hardware has not been setup, shouldn't we ignore all other parsed values? If you decide to keep the current behavior, I'd be down to adding a comment to explicit this choice in pl011_console_get_options. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > Hello, > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > There is no point in supporting 5 or 6 bits for console usage. Think > > about it. What values are going to be sent over the console? It'll be > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > characters into control characters, punctuation and numbers. 5-bit > > would be all control characters. > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > and I decided we might as well take that as an error (or maybe a > > case that the hardware has not been setup) and default to 8 bits per > > byte. > > I see your point. Two things come to mind: > > - I added this parsing of 5/6 bits to be symmetrical with > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > be modified then? Why should it? Note that I said above about _console_ usage which is what you were referring to - the early code that sets up the console by either reading the current settings (so that we can transparently use the UART when its handed over already setup by a boot loader). This is completely different to what happens once the kernel is running. Userspace might very well have a reason to set 5 or 6 bits if it wants to communicate with a device that uses those sizes. However, such a device won't be a console for the reasons I outlined above (it will truncate the ASCII characters turning console messages into garbage.) > If you decide to keep the current behavior, I'd be down to adding a > comment to explicit this choice in pl011_console_get_options. Well, honestly I don't think it needs a comment _if_ one thinks about what these sizes mean for what is supposed to be a console displaying ASCII characters. It feels to me like pointing out the obvious, and would be on the level of teaching people how to suck eggs... but then again, maybe there are times when people need to be taught how to suck eggs... So yes, add a comment if you think it's a good idea, but should that comment be replicated in almost every driver or should it be documented elsewhere?
On Tue, 31 Oct 2023 10:35:29 +0100 Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Hello, > > On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote: > > On Thu, 26 Oct 2023 12:41:23 +0200 > > Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > > Hi, > > I would change the commit title to better indicate that you add support > > for bits 5 and 6, which was missing. > > > > Maybe "Add support for 5 and 6 bits in..." ? > > > > > pl011_console_get_options() gets called to retrieve currently configured > > > options from the registers. Previously, LCRH_TX.WLEN was being parsed > > > > It took me some time to understand your explanation :) Maybe change > > to: > > > > "Previously, only 7 or 8 bits were supported." > > > > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8 > > > > Add bits: > > > > "5 to 8 bits..." > > > > And indicate that this patch adds support for 5 and 6 bits. > > I agree the whole commit message is unclear. Let's rewrite it. What do > you think of the following: > > tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup > > If no options are given at console setup, we parse hardware register > LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits > value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6 > or 8 bits per word, we fallback to 8 bits. > > Change that behavior to parse the whole range available: from 5 to 8 > bits per word. > > Note that we don't add support for 5/6 bits, we only update the parsing > of the regs (if no options are passed at setup) to reflect the current > hardware config. The behavior will be different only if the inherited > value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits > word length, now we guess the right value. > > What's your opinion on this new commit message? Hi, that's fine with me. Hugo.
On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote: > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > > There is no point in supporting 5 or 6 bits for console usage. Think > > > about it. What values are going to be sent over the console? It'll be > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > > characters into control characters, punctuation and numbers. 5-bit > > > would be all control characters. > > > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > > and I decided we might as well take that as an error (or maybe a > > > case that the hardware has not been setup) and default to 8 bits per > > > byte. > > > > I see your point. Two things come to mind: > > > > - I added this parsing of 5/6 bits to be symmetrical with > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > > be modified then? > > Why should it? Note that I said above about _console_ usage which is > what you were referring to - the early code that sets up the console > by either reading the current settings (so that we can transparently > use the UART when its handed over already setup by a boot loader). > > This is completely different to what happens once the kernel is running. > Userspace might very well have a reason to set 5 or 6 bits if it wants > to communicate with a device that uses those sizes. > > However, such a device won't be a console for the reasons I outlined > above (it will truncate the ASCII characters turning console messages > into garbage.) I'm not sure I get it. (1) We assume it is a console so it's ASCII so no reason to set to 5 or 6 bits per word. But (2) there might be a reason to set the UART to 5 or 6 bits, the userspace decides. How do the two interact? Say we boot to Linux, userspace configures to 6 bits because reasons and we reset. At second probe we see a config of 6 bits per word but assume that can't be logical, even though it is. What makes us suppose at probe that it must be a console? I won't die on a hill for this topic; we'll go the way you prefer! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Oct 31, 2023 at 02:51:45PM +0100, Théo Lebrun wrote: > On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote: > > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > > > There is no point in supporting 5 or 6 bits for console usage. Think > > > > about it. What values are going to be sent over the console? It'll be > > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > > > characters into control characters, punctuation and numbers. 5-bit > > > > would be all control characters. > > > > > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > > > and I decided we might as well take that as an error (or maybe a > > > > case that the hardware has not been setup) and default to 8 bits per > > > > byte. > > > > > > I see your point. Two things come to mind: > > > > > > - I added this parsing of 5/6 bits to be symmetrical with > > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > > > be modified then? > > > > Why should it? Note that I said above about _console_ usage which is > > what you were referring to - the early code that sets up the console > > by either reading the current settings (so that we can transparently > > use the UART when its handed over already setup by a boot loader). > > > > This is completely different to what happens once the kernel is running. > > Userspace might very well have a reason to set 5 or 6 bits if it wants > > to communicate with a device that uses those sizes. > > > > However, such a device won't be a console for the reasons I outlined > > above (it will truncate the ASCII characters turning console messages > > into garbage.) > > I'm not sure I get it. (1) We assume it is a console so it's ASCII so no > reason to set to 5 or 6 bits per word. But (2) there might be a reason > to set the UART to 5 or 6 bits, the userspace decides. Precisely. > How do the two interact? Say we boot to Linux, userspace configures to 6 > bits because reasons and we reset. At second probe we see a config of 6 > bits per word but assume that can't be logical, even though it is. I think you're conflating "serial console" with "serial port". A "serial port" can support multiple different formats, and in this case, such as 5, 6, 7, and 8 bits. 5 and 6 bits are likely to be a specialised application which uses a binary protocol, not ASCII. A "serial console" is one application of a "serial port" and a "serial console" is used to send ASCII characters, not a binary protocol. > What makes us suppose at probe that it must be a console? Sorry, but no, we don't assume every serial port is a serial console. Unless something has changed since I was involved with the serial layer, we only read the parameters from a serial port _if_ and only if that port is being used as a serial console. TTYs under Linux have a standard initial set of parameters at boot - 9600 baud, 8 bits, etc. The exception to this is if a serial port *is being used* as a serial console, where these settings are overriden by the serial console configuration. The reason for that is... imagine the chaos that would occur if: - Your boot loader configures (through user configuration) the serial port for 115200 baud. - The boot loader loads the kernel and passes control to it, with a command line specifying that this serial port is to be used for the serial console, but not specifying any parameters. - The kernel boots, and starts outputting messages at 115200 baud. - Userspace starts running, opens /dev/console, and switches the port to 9600 baud. Now you have utter garbage being received from the serial console. So, the serial console is special in that regard. Now, when we configure the serial console, we attempt to: 1) parse the options provided on the console= line to set the serial port appropriately, or 2) if there are no options, then we attempt to set the serial port to something sane *for use as a serial console*, which uses ASCII protocol not some random binary protocol. 5 and 6 bits make no sense here.
Hello, On Tue Oct 31, 2023 at 3:05 PM CET, Russell King (Oracle) wrote: > On Tue, Oct 31, 2023 at 02:51:45PM +0100, Théo Lebrun wrote: > > On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote: > > > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > > > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > > > > There is no point in supporting 5 or 6 bits for console usage. Think > > > > > about it. What values are going to be sent over the console? It'll be > > > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > > > > characters into control characters, punctuation and numbers. 5-bit > > > > > would be all control characters. > > > > > > > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > > > > and I decided we might as well take that as an error (or maybe a > > > > > case that the hardware has not been setup) and default to 8 bits per > > > > > byte. > > > > > > > > I see your point. Two things come to mind: > > > > > > > > - I added this parsing of 5/6 bits to be symmetrical with > > > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > > > > be modified then? > > > > > > Why should it? Note that I said above about _console_ usage which is > > > what you were referring to - the early code that sets up the console > > > by either reading the current settings (so that we can transparently > > > use the UART when its handed over already setup by a boot loader). > > > > > > This is completely different to what happens once the kernel is running. > > > Userspace might very well have a reason to set 5 or 6 bits if it wants > > > to communicate with a device that uses those sizes. > > > > > > However, such a device won't be a console for the reasons I outlined > > > above (it will truncate the ASCII characters turning console messages > > > into garbage.) > > > > I'm not sure I get it. (1) We assume it is a console so it's ASCII so no > > reason to set to 5 or 6 bits per word. But (2) there might be a reason > > to set the UART to 5 or 6 bits, the userspace decides. > > Precisely. > > > How do the two interact? Say we boot to Linux, userspace configures to 6 > > bits because reasons and we reset. At second probe we see a config of 6 > > bits per word but assume that can't be logical, even though it is. > > I think you're conflating "serial console" with "serial port". A > "serial port" can support multiple different formats, and in this case, > such as 5, 6, 7, and 8 bits. 5 and 6 bits are likely to be a specialised > application which uses a binary protocol, not ASCII. > > A "serial console" is one application of a "serial port" and a "serial > console" is used to send ASCII characters, not a binary protocol. That was all clear in my mind; I was missing the following bit: > Sorry, but no, we don't assume every serial port is a serial console. > Unless something has changed since I was involved with the serial > layer, **we only read the parameters from a serial port _if_ and only > if that port is being used as a serial console.** Thank you for the time you took; I'll get rid of the patch and send a V2 fixing nits for other patches. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 5774d48c7f16..b2062e4cbbab 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, *parity = 'o'; } - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) - *bits = 7; - else - *bits = 8; + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ ibrd = pl011_read(uap, REG_IBRD); fbrd = pl011_read(uap, REG_FBRD);