linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs
@ 2021-06-10 18:38 Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 1/5] serial: 8250: Document SMSC Super I/O UART peculiarities Maciej W. Rozycki
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

Hi,

 I've noticed our Malta platform serial driver does not take advantage of 
support for the special UART divisor values for the bit rates of 230400 
and 460800 bits per second its SMSC FDC37M817 Super I/O hardware provides.  

 Our 8250 core provides for these divisors via the UPF_MAGIC_MULTIPLIER 
flag, so I thought it would be a straightforward platform change, but it 
has turned out that the flag has AFAICT never worked (I wonder how people 
test those things).  The only platform currently in our tree that sets the 
flag uses it for a different purpose.

 I've fixed UPF_MAGIC_MULTIPLIER handling then in our 8250 core, added 
reporting so that people have a chance to know the rates are supported and 
then added the flag to Malta platform initialisers for Super I/O UARTs.  
I have examined YAMON sources to verify that we don't have to enable the 
special UART divisor values ourselves.  See individual change descriptions 
for further details.

 Verified with a WTI CPM-800 site manager device which supports bit rates 
of up to 230400bps.  Also verified at 230400bps and 460800bps with an 
EXSYS EX-44072 option card under Linux, based on the Oxford Semiconductor 
OXPCIe952 device in its native mode; our clock settings are however off 
enough for OxSemi PCIe devices for the former data rate only just working 
and the latter making transmission go out of sync with data corruption 
resulting.  Another patch series to fix OxSemi handling bugs follows then.

 Verification of the OxSemi patch series actually revealed an interesting 
peculiarity of the SMSC Super I/O IC that contradicts SMSC documentation.  
I have added a clarification as 1/5 of this patch series, originally meant 
to contain four patches only.  This has been confirmed both with the Malta 
board concerned here and my old x86 server, which also has a similar SMSC 
Super I/O part, although with high-speed operation not enabled by the 
BIOS.

 NB occasional input overruns were observed at 460800bps (with a MIPS 5Kc 
@160MHz running this Malta), which could be eliminated by lowering the 
`rx_trig_bytes' parameter to 4 from the default of 8 (the UART core in the 
FDC37M817 has a 16-byte FIFO), via sysfs.  Perhaps it would make sense to 
change the default, at least for the higher-speed ports?

 Please apply.

  Maciej

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] serial: 8250: Document SMSC Super I/O UART peculiarities
  2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
@ 2021-06-10 18:38 ` Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 2/5] serial: 8250: Actually allow UPF_MAGIC_MULTIPLIER baud rates Maciej W. Rozycki
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

Contrary to what SMSC documentation says and unlike NS16C550A UARTs the 
SMSC Super I/O IC claims compatibility with the SMSC UART implementation 
does not support dividing the internal PLL clock by any divisor from 1 
to 65535[1], with the exception of two magic divisors of 32769 and 32770 
used respectively to select the high-speed data rates of 460800bps and 
230400bps[2] if enabled on a port-by-port basis in with the Serial Port 
Mode Register in the Device Configuration Space[3][4].

Instead empirical evidence indicates that the divisor, as stored in the 
DLL and DLM register pair, has the range of 1 to 32767 only, and bit 7 
of the DLM register (bit 15 of the divisor) effectively serves as a 
selection bit for the prescaler from the base frequency of 7.3728MHz, 
either 4 if the bit is 0, or 1 if the bit is 1 and high-speed operation 
has been enabled with the Serial Port Mode Register.

So if high-speed operation has not been enabled, then say the values of 
1 and 32769 (0x8001) written into the combined DLL and DLM register pair 
both select the divisor of 1 and the baud rate of 115200bps.

[1] "FDC37M81x, PC98/99 Compliant Enhanced Super I/O Controller with
    Keyboard/Mouse Wake-Up", Standard Microsystems Corporation, Rev.
    03/27/2000, Section "Programmable Baud Rate Generator (and Divisor 
    Latches DLH, DLL)", p. 75

[2] same, Table 31 - "Baud Rates", p. 77

[3] same, Table 60 - "Serial Port 1, Logical Device 4 [Logical Device 
    Number = 0x04]", p. 153

[4] same, Table 61 - "Serial Port 2, Logical Device 5 [Logical Device 
    Number = 0x05]", p. 153

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 drivers/tty/serial/8250/8250_port.c |   31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

linux-serial-8250-magic-multiplier-doc.diff
Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_port.c
+++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
@@ -2519,9 +2519,36 @@ static unsigned int serial8250_do_get_di
 	unsigned int quot;
 
 	/*
-	 * Handle magic divisors for baud rates above baud_base on
-	 * SMSC SuperIO chips.
+	 * Handle magic divisors for baud rates above baud_base on SMSC
+	 * Super I/O chips.  We clamp custom rates from clk/6 and clk/12
+	 * up to clk/4 (0x8001) and clk/8 (0x8002) respectively.  These
+	 * magic divisors actually reprogram the baud rate generator's
+	 * reference clock derived from chips's 14.318MHz clock input.
+	 *
+	 * Documentation claims that with these magic divisors the base
+	 * frequencies of 7.3728MHz and 3.6864MHz are used respectively
+	 * for the extra baud rates of 460800bps and 230400bps rather
+	 * than the usual base frequency of 1.8462MHz.  However empirical
+	 * evidence contradicts that.
 	 *
+	 * Instead bit 7 of the DLM register (bit 15 of the divisor) is
+	 * effectively used as a clock prescaler selection bit for the
+	 * base frequency of 7.3728MHz, always used.  If set to 0, then
+	 * the base frequency is divided by 4 for use by the Baud Rate
+	 * Generator, for the usual arrangement where the value of 1 of
+	 * the divisor produces the baud rate of 115200bps.  Conversely,
+	 * if set to 1 and high-speed operation has been enabled with the
+	 * Serial Port Mode Register in the Device Configuration Space,
+	 * then the base frequency is supplied directly to the Baud Rate
+	 * Generator, so for the divisor values of 0x8001, 0x8002, 0x8003,
+	 * 0x8004, etc. the respective baud rates produced are 460800bps,
+	 * 230400bps, 153600bps, 115200bps, etc.
+	 *
+	 * In all cases only low 15 bits of the divisor are used to divide
+	 * the baud base and therefore 32767 is the maximum divisor value
+	 * possible, even though documentation says that the programmable
+	 * Baud Rate Generator is capable of dividing the internal PLL
+	 * clock by any divisor from 1 to 65535.
 	 */
 	if ((port->flags & UPF_MAGIC_MULTIPLIER) &&
 	    baud == (port->uartclk/4))

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/5] serial: 8250: Actually allow UPF_MAGIC_MULTIPLIER baud rates
  2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 1/5] serial: 8250: Document SMSC Super I/O UART peculiarities Maciej W. Rozycki
@ 2021-06-10 18:38 ` Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 3/5] serial: 8250: Handle custom baud rates in UPF_MAGIC_MULTIPLIER range Maciej W. Rozycki
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

Support for magic baud rate divisors of 32770 and 32769 used with SMSC 
Super I/O chips for extra baud rates of 230400 and 460800 respectively 
where base rate is 115200[1] has been added around Linux 2.5.64, which 
predates our repo history, but the origin could be identified as commit 
2a717aad772f ("Merge with Linux 2.5.64.") with the old MIPS/Linux repo 
also at: <git://git.kernel.org/pub/scm/linux/kernel/git/ralf/linux.git>.

Code that is now in `serial8250_do_get_divisor' was added back then to 
`serial8250_get_divisor', but that code would only ever trigger if one 
of the higher baud rates was actually requested, and that cannot ever 
happen, because the earlier call to `serial8250_get_baud_rate' never 
returns them.  This is because it calls `uart_get_baud_rate' with the 
maximum requested being the base rate, that is clk/16 or 115200 for SMSC 
chips at their nominal clock rate.

Fix it then and allow UPF_MAGIC_MULTIPLIER baud rates to be selected, by 
requesting the maximum baud rate of clk/4 rather than clk/16 if the flag 
has been set.  Also correct the minimum baud rate, observing that these 
ports only support actual (non-magic) divisors of up to 32767 only.

References:

[1] "FDC37M81x, PC98/99 Compliant Enhanced Super I/O Controller with 
    Keyboard/Mouse Wake-Up", Standard Microsystems Corporation, Rev. 
    03/27/2000, Table 31 - "Baud Rates", p. 77

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
---
 drivers/tty/serial/8250/8250_port.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

linux-serial-8250-magic-multiplier-baud-rate.diff
Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_port.c
+++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
@@ -2659,6 +2659,21 @@ static unsigned int serial8250_get_baud_
 					     struct ktermios *old)
 {
 	unsigned int tolerance = port->uartclk / 100;
+	unsigned int min;
+	unsigned int max;
+
+	/*
+	 * Handle magic divisors for baud rates above baud_base on SMSC
+	 * Super I/O chips.  Enable custom rates of clk/4 and clk/8, but
+	 * disable divisor values beyond 32767, which are unavailable.
+	 */
+	if (port->flags & UPF_MAGIC_MULTIPLIER) {
+		min = port->uartclk / 16 / UART_DIV_MAX >> 1;
+		max = (port->uartclk + tolerance) / 4;
+	} else {
+		min = port->uartclk / 16 / UART_DIV_MAX;
+		max = (port->uartclk + tolerance) / 16;
+	}
 
 	/*
 	 * Ask the core to calculate the divisor for us.
@@ -2666,9 +2681,7 @@ static unsigned int serial8250_get_baud_
 	 * slower than nominal still match standard baud rates without
 	 * causing transmission errors.
 	 */
-	return uart_get_baud_rate(port, termios, old,
-				  port->uartclk / 16 / UART_DIV_MAX,
-				  (port->uartclk + tolerance) / 16);
+	return uart_get_baud_rate(port, termios, old, min, max);
 }
 
 /*

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/5] serial: 8250: Handle custom baud rates in UPF_MAGIC_MULTIPLIER range
  2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 1/5] serial: 8250: Document SMSC Super I/O UART peculiarities Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 2/5] serial: 8250: Actually allow UPF_MAGIC_MULTIPLIER baud rates Maciej W. Rozycki
@ 2021-06-10 18:38 ` Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting Maciej W. Rozycki
  2021-06-10 18:38 ` [PATCH 5/5] MIPS: Malta: Enable magic multipliers for Super I/O UARTs Maciej W. Rozycki
  4 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

Handle custom baud rates requested in the UPF_MAGIC_MULTIPLIER range 
with BOTHER.  Currently matching is exact, that is if a baud rate that 
is not either of clk/4 or clk/8 is requested, then we fall through to 
the default case, which will just divide the clock rate by 16 times the 
rate requested, round it to closest integer, and possibly yield even 
worse results then if clamping to the extra baud rates was chosen.

So for example if we have the usual base rate of 115200 and request a 
rate of 230399, then the fall-through divisor calculation will yield 1, 
and consequently the baud rate of 115200 will be programmed even though 
obviously the magic rate of 230400 would be more appropriate.

Make the selection of the magic rates range-qualified then and use clk/4 
for rates from clk/6 up (assuming `serial8250_get_baud_rate' has already 
rejected any rates too far beyond clk/4), and otherwise use clk/8 for 
rates from clk/12 up, and finally fall through to the standard divisor 
calculation.  Explicitly void using the undocumented rate of 153600bps 
and stick to documented divisor values only.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 drivers/tty/serial/8250/8250_port.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

linux-serial-8250-magic-multiplier-bother.diff
Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_port.c
+++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_port.c
@@ -2515,6 +2515,7 @@ static unsigned int serial8250_do_get_di
 					      unsigned int baud,
 					      unsigned int *frac)
 {
+	upf_t magic_multiplier = port->flags & UPF_MAGIC_MULTIPLIER;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned int quot;
 
@@ -2550,11 +2551,9 @@ static unsigned int serial8250_do_get_di
 	 * Baud Rate Generator is capable of dividing the internal PLL
 	 * clock by any divisor from 1 to 65535.
 	 */
-	if ((port->flags & UPF_MAGIC_MULTIPLIER) &&
-	    baud == (port->uartclk/4))
+	if (magic_multiplier && baud >= port->uartclk / 6)
 		quot = 0x8001;
-	else if ((port->flags & UPF_MAGIC_MULTIPLIER) &&
-		 baud == (port->uartclk/8))
+	else if (magic_multiplier && baud >= port->uartclk / 12)
 		quot = 0x8002;
 	else if (up->port.type == PORT_NPCM)
 		quot = npcm_get_divisor(up, baud);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting
  2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
                   ` (2 preceding siblings ...)
  2021-06-10 18:38 ` [PATCH 3/5] serial: 8250: Handle custom baud rates in UPF_MAGIC_MULTIPLIER range Maciej W. Rozycki
@ 2021-06-10 18:38 ` Maciej W. Rozycki
  2021-06-15 13:19   ` Greg Kroah-Hartman
  2021-06-10 18:38 ` [PATCH 5/5] MIPS: Malta: Enable magic multipliers for Super I/O UARTs Maciej W. Rozycki
  4 siblings, 1 reply; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

Add a hook for `uart_report_port' to let serial ports report extra 
properties beyond `irq' and `base_baud'.  Use it with the 8250 backend 
to report extra baud rates supported above the base rate for ports with 
the UPF_MAGIC_MULTIPLIER property, so that people have a way to find out 
that they are supported with their system, e.g.:

Serial: 8250/16550 driver, 5 ports, IRQ sharing enabled
printk: console [ttyS0] disabled
serial8250.0: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200 [+230400, 460800]) is a 16550A
printk: console [ttyS0] enabled
printk: console [ttyS0] enabled
printk: bootconsole [uart8250] disabled
printk: bootconsole [uart8250] disabled
serial8250.0: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200 [+230400, 460800]) is a 16550A
serial8250.0: ttyS2 at MMIO 0x1f000900 (irq = 20, base_baud = 230400) is a 16550A

Otherwise there is no clear way to figure this out.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 drivers/tty/serial/8250/8250_core.c |   10 ++++++++++
 drivers/tty/serial/serial_core.c    |   11 +++++++++--
 include/linux/serial_core.h         |    3 +++
 3 files changed, 22 insertions(+), 2 deletions(-)

linux-serial-core-baud-extra.diff
Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_core.c
===================================================================
--- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_core.c
+++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_core.c
@@ -952,6 +952,13 @@ static struct uart_8250_port *serial8250
 	return NULL;
 }
 
+static void serial8250_report_magic(struct uart_port *port,
+				    char *report_buf, size_t report_size)
+{
+	snprintf(report_buf, report_size,
+		 " [+%d, %d]", port->uartclk / 8, port->uartclk / 4);
+}
+
 static void serial_8250_overrun_backoff_work(struct work_struct *work)
 {
 	struct uart_8250_port *up =
@@ -1048,6 +1055,9 @@ int serial8250_register_8250_port(struct
 			}
 		}
 
+		if (up->port.flags & UPF_MAGIC_MULTIPLIER)
+			uart->port.report_extra = serial8250_report_magic;
+
 		serial8250_set_defaults(uart);
 
 		/* Possibly override default I/O functions.  */
Index: linux-malta-cbus-uart/drivers/tty/serial/serial_core.c
===================================================================
--- linux-malta-cbus-uart.orig/drivers/tty/serial/serial_core.c
+++ linux-malta-cbus-uart/drivers/tty/serial/serial_core.c
@@ -2309,6 +2309,7 @@ int uart_resume_port(struct uart_driver
 static inline void
 uart_report_port(struct uart_driver *drv, struct uart_port *port)
 {
+	char report_extra[64];
 	char address[64];
 
 	switch (port->iotype) {
@@ -2333,11 +2334,17 @@ uart_report_port(struct uart_driver *drv
 		break;
 	}
 
-	pr_info("%s%s%s at %s (irq = %d, base_baud = %d) is a %s\n",
+	if (port->report_extra)
+		port->report_extra(port, report_extra, sizeof(report_extra));
+	else
+		report_extra[0] = '\0';
+
+	pr_info("%s%s%s at %s (irq = %d, base_baud = %d%s) is a %s\n",
 	       port->dev ? dev_name(port->dev) : "",
 	       port->dev ? ": " : "",
 	       port->name,
-	       address, port->irq, port->uartclk / 16, uart_type(port));
+	       address, port->irq, port->uartclk / 16, report_extra,
+	       uart_type(port));
 }
 
 static void
Index: linux-malta-cbus-uart/include/linux/serial_core.h
===================================================================
--- linux-malta-cbus-uart.orig/include/linux/serial_core.h
+++ linux-malta-cbus-uart/include/linux/serial_core.h
@@ -135,6 +135,9 @@ struct uart_port {
 						struct serial_rs485 *rs485);
 	int			(*iso7816_config)(struct uart_port *,
 						  struct serial_iso7816 *iso7816);
+	void			(*report_extra)(struct uart_port *port,
+						char *report_buf,
+						size_t report_size);
 	unsigned int		irq;			/* irq number */
 	unsigned long		irqflags;		/* irq flags  */
 	unsigned int		uartclk;		/* base uart clock */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/5] MIPS: Malta: Enable magic multipliers for Super I/O UARTs
  2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
                   ` (3 preceding siblings ...)
  2021-06-10 18:38 ` [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting Maciej W. Rozycki
@ 2021-06-10 18:38 ` Maciej W. Rozycki
  4 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-10 18:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Thomas Bogendoerfer
  Cc: linux-mips, linux-serial, linux-kernel

The SMSC FDC37M817 Super I/O chip has a configuration feature that lets 
it support special UART divisor values of 32770 and 32769 for bit rates 
of 230400 and 460800 bits per second respectively.  Our 8250 driver core 
provides support for these special divisors via the UPF_MAGIC_MULTIPLIER 
flag, and YAMON firmware unconditionally configures the Super I/O chip 
with these divisors enabled as well, so all we need to do in platform 
setup for these bit rates to work is to set the flag.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 arch/mips/mti-malta/malta-platform.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

linux-mips-malta-smsc-uart-magic-multiplier.diff
Index: linux-malta-cbus-uart/arch/mips/mti-malta/malta-platform.c
===================================================================
--- linux-malta-cbus-uart.orig/arch/mips/mti-malta/malta-platform.c
+++ linux-malta-cbus-uart/arch/mips/mti-malta/malta-platform.c
@@ -33,7 +33,8 @@
 	.irq		= int,						\
 	.uartclk	= 1843200,					\
 	.iotype		= UPIO_PORT,					\
-	.flags		= UPF_BOOT_AUTOCONF | UPF_SKIP_TEST,		\
+	.flags		= UPF_BOOT_AUTOCONF | UPF_SKIP_TEST |		\
+			  UPF_MAGIC_MULTIPLIER,				\
 	.regshift	= 0,						\
 }
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting
  2021-06-10 18:38 ` [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting Maciej W. Rozycki
@ 2021-06-15 13:19   ` Greg Kroah-Hartman
  2021-06-26  4:12     ` Maciej W. Rozycki
  0 siblings, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-15 13:19 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Jiri Slaby, Thomas Bogendoerfer, linux-mips, linux-serial, linux-kernel

On Thu, Jun 10, 2021 at 08:38:44PM +0200, Maciej W. Rozycki wrote:
> Add a hook for `uart_report_port' to let serial ports report extra 
> properties beyond `irq' and `base_baud'.  Use it with the 8250 backend 
> to report extra baud rates supported above the base rate for ports with 
> the UPF_MAGIC_MULTIPLIER property, so that people have a way to find out 
> that they are supported with their system, e.g.:
> 
> Serial: 8250/16550 driver, 5 ports, IRQ sharing enabled
> printk: console [ttyS0] disabled
> serial8250.0: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200 [+230400, 460800]) is a 16550A
> printk: console [ttyS0] enabled
> printk: console [ttyS0] enabled
> printk: bootconsole [uart8250] disabled
> printk: bootconsole [uart8250] disabled
> serial8250.0: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200 [+230400, 460800]) is a 16550A
> serial8250.0: ttyS2 at MMIO 0x1f000900 (irq = 20, base_baud = 230400) is a 16550A
> 
> Otherwise there is no clear way to figure this out.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
>  drivers/tty/serial/8250/8250_core.c |   10 ++++++++++
>  drivers/tty/serial/serial_core.c    |   11 +++++++++--
>  include/linux/serial_core.h         |    3 +++
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> linux-serial-core-baud-extra.diff
> Index: linux-malta-cbus-uart/drivers/tty/serial/8250/8250_core.c
> ===================================================================
> --- linux-malta-cbus-uart.orig/drivers/tty/serial/8250/8250_core.c
> +++ linux-malta-cbus-uart/drivers/tty/serial/8250/8250_core.c
> @@ -952,6 +952,13 @@ static struct uart_8250_port *serial8250
>  	return NULL;
>  }
>  
> +static void serial8250_report_magic(struct uart_port *port,
> +				    char *report_buf, size_t report_size)
> +{
> +	snprintf(report_buf, report_size,
> +		 " [+%d, %d]", port->uartclk / 8, port->uartclk / 4);
> +}
> +
>  static void serial_8250_overrun_backoff_work(struct work_struct *work)
>  {
>  	struct uart_8250_port *up =
> @@ -1048,6 +1055,9 @@ int serial8250_register_8250_port(struct
>  			}
>  		}
>  
> +		if (up->port.flags & UPF_MAGIC_MULTIPLIER)
> +			uart->port.report_extra = serial8250_report_magic;
> +
>  		serial8250_set_defaults(uart);
>  
>  		/* Possibly override default I/O functions.  */
> Index: linux-malta-cbus-uart/drivers/tty/serial/serial_core.c
> ===================================================================
> --- linux-malta-cbus-uart.orig/drivers/tty/serial/serial_core.c
> +++ linux-malta-cbus-uart/drivers/tty/serial/serial_core.c
> @@ -2309,6 +2309,7 @@ int uart_resume_port(struct uart_driver
>  static inline void
>  uart_report_port(struct uart_driver *drv, struct uart_port *port)
>  {
> +	char report_extra[64];
>  	char address[64];
>  
>  	switch (port->iotype) {
> @@ -2333,11 +2334,17 @@ uart_report_port(struct uart_driver *drv
>  		break;
>  	}
>  
> -	pr_info("%s%s%s at %s (irq = %d, base_baud = %d) is a %s\n",
> +	if (port->report_extra)
> +		port->report_extra(port, report_extra, sizeof(report_extra));
> +	else
> +		report_extra[0] = '\0';
> +
> +	pr_info("%s%s%s at %s (irq = %d, base_baud = %d%s) is a %s\n",
>  	       port->dev ? dev_name(port->dev) : "",
>  	       port->dev ? ": " : "",
>  	       port->name,
> -	       address, port->irq, port->uartclk / 16, uart_type(port));
> +	       address, port->irq, port->uartclk / 16, report_extra,
> +	       uart_type(port));

Ick, really?  What relies on this print message?  Why do we need a whole
new uart port hook for this?

Isn't there some other way for your specific variant to print out
another message if you really want to do something "odd" like this?

And you did not document what your new change did anywhere in the tree,
so people are going to be confused.

I've taken the other patches here, but not this one.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting
  2021-06-15 13:19   ` Greg Kroah-Hartman
@ 2021-06-26  4:12     ` Maciej W. Rozycki
  0 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2021-06-26  4:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Slaby, Thomas Bogendoerfer, linux-mips, linux-serial, linux-kernel

On Tue, 15 Jun 2021, Greg Kroah-Hartman wrote:

> > Add a hook for `uart_report_port' to let serial ports report extra 
> > properties beyond `irq' and `base_baud'.  Use it with the 8250 backend 
> > to report extra baud rates supported above the base rate for ports with 
> > the UPF_MAGIC_MULTIPLIER property, so that people have a way to find out 
> > that they are supported with their system, e.g.:
[...]
> Ick, really?  What relies on this print message?  Why do we need a whole
> new uart port hook for this?

 As I noted, perhaps too briefly, in the commit description (see the last 
sentence above) people need to be made aware of the extra baud rates above 
`base_baud' supported, or otherwise they'll have no way to figure out they 
can use them.

 Reporting tweaked `base_baud' would I think cause confusion from the 
inconsistency with the TIOCGSERIAL/TIOCSSERIAL ioctls (e.g. `setserial'), 
and the sysfs flags:

$ cat /sys/class/tty/ttyS[0-2]/flags
0x10010040
0x10010040
0x90000040
$ 

(here from a Malta board) are IMO too obscure for anyone to figure this 
out (bit 16 means the two extra baud rates are supported).

 As explained with 1/5 we could set `base_baud' to 460800 instead and 
hardcode bit 15 of the divisor to 1, relying on undocumented Super I/O IC 
behaviour, but that would require more exhaustive verification than I am 
able to do with just a single board and IC type and revision.

> Isn't there some other way for your specific variant to print out
> another message if you really want to do something "odd" like this?

 There's always another way. :)  How about?

serial8250.0: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
serial8250.0: ttyS0 extra baud rates supported: 230400, 460800

> And you did not document what your new change did anywhere in the tree,
> so people are going to be confused.

 We've been somewhat terse about things, but you're probably right here.  
Sorry about that.

> I've taken the other patches here, but not this one.

 Thanks.  I've posted an alternative printing a message like above, with 
some commentary.  Let me know if that makes you feel more convinced.

  Maciej

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-06-26  4:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 18:38 [PATCH 0/5] serial, Malta: Fixes to magic multipliers for SMSC Super I/O UARTs Maciej W. Rozycki
2021-06-10 18:38 ` [PATCH 1/5] serial: 8250: Document SMSC Super I/O UART peculiarities Maciej W. Rozycki
2021-06-10 18:38 ` [PATCH 2/5] serial: 8250: Actually allow UPF_MAGIC_MULTIPLIER baud rates Maciej W. Rozycki
2021-06-10 18:38 ` [PATCH 3/5] serial: 8250: Handle custom baud rates in UPF_MAGIC_MULTIPLIER range Maciej W. Rozycki
2021-06-10 18:38 ` [PATCH 4/5] serial: core, 8250: Add a hook for extra port property reporting Maciej W. Rozycki
2021-06-15 13:19   ` Greg Kroah-Hartman
2021-06-26  4:12     ` Maciej W. Rozycki
2021-06-10 18:38 ` [PATCH 5/5] MIPS: Malta: Enable magic multipliers for Super I/O UARTs Maciej W. Rozycki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).