Re: [PATCH v5 2/3] serial: 8250: Add Loongson uart driver support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 24 Sep 2025, Binbin Zhou wrote:

> Add the driver for on-chip UART used on Loongson family chips.
> 
> The hardware is similar to 8250, but there are the following
> differences:
>  - Some chips (such as Loongson-2K2000) have added a fractional division
>    register to obtain the required baud rate accurately, so the
>    {get,set}_divisor callback is overridden.
>  - Due to hardware defects, quirk handling is required for
>    UART_MCR/UART_MSR.
> 
> Co-developed-by: Haowei Zheng <zhenghaowei@loongson.cn>
> Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  drivers/tty/serial/8250/8250_loongson.c | 202 ++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_port.c     |   8 +
>  drivers/tty/serial/8250/Kconfig         |  10 ++
>  drivers/tty/serial/8250/Makefile        |   1 +
>  include/uapi/linux/serial_core.h        |   1 +
>  5 files changed, 222 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_loongson.c
> 
> diff --git a/drivers/tty/serial/8250/8250_loongson.c b/drivers/tty/serial/8250/8250_loongson.c
> new file mode 100644
> index 000000000000..a114b4e6d5c3
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_loongson.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Serial Port driver for Loongson family chips
> + *
> + * Copyright (C) 2020-2025 Loongson Technology Corporation Limited
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/reset.h>
> +
> +#include "8250.h"
> +
> +/* Divisor Latch Fraction Register */
> +#define LOONGSON_UART_DLF		0x2
> +
> +/* Flags */
> +#define LOONGSON_UART_HAS_FRAC		BIT(0)
> +#define LOONGSON_UART_QUIRK_MCR		BIT(1)
> +#define LOONGSON_UART_QUIRK_MSR		BIT(2)
> +
> +#define LS2K0500_UART_FLAG	(LOONGSON_UART_QUIRK_MCR | LOONGSON_UART_QUIRK_MSR)
> +#define LS2K1500_UART_FLAG	(LOONGSON_UART_HAS_FRAC | LOONGSON_UART_QUIRK_MCR)
> +
> +struct loongson_uart_data {
> +	int line;
> +	int mcr_invert;
> +	int msr_invert;
> +	struct reset_control *rst;
> +};
> +
> +static unsigned int serial_fixup(struct uart_port *p, unsigned int offset, unsigned int val)
> +{
> +	struct loongson_uart_data *ddata = p->private_data;
> +
> +	if (offset == UART_MCR)
> +		val ^= ddata->mcr_invert;
> +
> +	if (offset == UART_MSR)
> +		val ^= ddata->msr_invert;
> +
> +	return val;
> +}
> +
> +static u32 loongson_serial_in(struct uart_port *p, unsigned int offset)
> +{
> +	unsigned int val;
> +
> +	val = readb(p->membase + (offset << p->regshift));
> +
> +	return serial_fixup(p, offset, val);
> +}
> +
> +static void loongson_serial_out(struct uart_port *p, unsigned int offset, unsigned int value)
> +{
> +	offset <<= p->regshift;
> +	writeb(serial_fixup(p, offset, value), p->membase + offset);

This would be cleaner if you do that serial_fixup() on a preceeding line.

Include for writeb()?

> +}
> +
> +static unsigned int loongson_frac_get_divisor(struct uart_port *port, unsigned int baud,
> +					      unsigned int *frac)
> +{
> +	unsigned int quot;
> +
> +	quot = DIV_ROUND_CLOSEST((port->uartclk << 4), baud);

Missing include.

> +	*frac = FIELD_GET(GENMASK(7, 0), quot);
> +
> +	return FIELD_GET(GENMASK(15, 8), quot);

Please name the fields properly with defines.

You're also missing #include for GENMASK().

> +}
> +
> +static void loongson_frac_set_divisor(struct uart_port *port, unsigned int baud,
> +				      unsigned int quot, unsigned int quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +
> +	serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);
> +	serial_dl_write(up, quot);
> +	serial_port_out(port, LOONGSON_UART_DLF, quot_frac);
> +}
> +
> +static int loongson_uart_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct uart_8250_port uart = {};
> +	struct loongson_uart_data *ddata;
> +	struct resource *res;
> +	unsigned int flags;
> +	int ret;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	uart.port.irq = platform_get_irq(pdev, 0);
> +	if (uart.port.irq < 0)
> +		return -EINVAL;
> +
> +	device_property_read_u32(dev, "clock-frequency", &uart.port.uartclk);
> +
> +	spin_lock_init(&uart.port.lock);
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_IOREMAP;
> +	uart.port.iotype = UPIO_MEM;
> +	uart.port.regshift = 0;
> +	uart.port.dev = dev;
> +	uart.port.type = PORT_LOONGSON;
> +	uart.port.private_data = ddata;
> +
> +	uart.port.mapbase = res->start;
> +	uart.port.mapsize = resource_size(res);
> +	uart.port.serial_in = loongson_serial_in;
> +	uart.port.serial_out = loongson_serial_out;
> +
> +	flags = (uintptr_t)device_get_match_data(dev);

Perhaps flags should be unsigned long?

> +	if (flags & LOONGSON_UART_HAS_FRAC) {
> +		uart.port.get_divisor = loongson_frac_get_divisor;
> +		uart.port.set_divisor = loongson_frac_set_divisor;
> +	}
> +
> +	if (flags & LOONGSON_UART_QUIRK_MCR)
> +		ddata->mcr_invert |= (UART_MCR_RTS | UART_MCR_DTR);
> +
> +	if (flags & LOONGSON_UART_QUIRK_MSR)
> +		ddata->msr_invert |= (UART_MSR_CTS | UART_MSR_DSR);

I think it would be better to put these invert masks directly into a 
struct which is then put into .data. LOONGSON_UART_HAS_FRAC can be bool 
in that struct.

> +	ddata->rst = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(ddata->rst))
> +		return PTR_ERR(ddata->rst);
> +
> +	ret = reset_control_deassert(ddata->rst);
> +	if (ret)
> +		return ret;
> +
> +	ret = serial8250_register_8250_port(&uart);
> +	if (ret < 0) {
> +		reset_control_assert(ddata->rst);
> +		return ret;
> +	}
> +
> +	ddata->line = ret;
> +	platform_set_drvdata(pdev, ddata);
> +
> +	return 0;
> +}
> +
> +static void loongson_uart_remove(struct platform_device *pdev)
> +{
> +	struct loongson_uart_data *ddata = platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(ddata->line);
> +	reset_control_assert(ddata->rst);
> +}
> +
> +static int loongson_uart_suspend(struct device *dev)
> +{
> +	struct loongson_uart_data *ddata = dev_get_drvdata(dev);
> +
> +	serial8250_suspend_port(ddata->line);
> +
> +	return 0;
> +}
> +
> +static int loongson_uart_resume(struct device *dev)
> +{
> +	struct loongson_uart_data *data = dev_get_drvdata(dev);
> +
> +	serial8250_resume_port(data->line);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(loongson_uart_pm_ops, loongson_uart_suspend,
> +				loongson_uart_resume);

Include? Please check if you miss even more includes beyond those I've 
highlighted here.

> +static const struct of_device_id loongson_uart_of_ids[] = {
> +	{ .compatible = "loongson,ls2k0500-uart", .data = (void *)LS2K0500_UART_FLAG },
> +	{ .compatible = "loongson,ls2k1500-uart", .data = (void *)LS2K1500_UART_FLAG },
> +	{ /* sentinel */ },

Nit, no need for comma in the terminator entry.

> +};
> +MODULE_DEVICE_TABLE(of, loongson_uart_of_ids);
> +
> +static struct platform_driver loongson_uart_driver = {
> +	.probe = loongson_uart_probe,
> +	.remove = loongson_uart_remove,
> +	.driver = {
> +		.name = "loongson-uart",
> +		.pm = pm_ptr(&loongson_uart_pm_ops),
> +		.of_match_table = loongson_uart_of_ids,
> +	},
> +};
> +
> +module_platform_driver(loongson_uart_driver);
> +
> +MODULE_DESCRIPTION("Loongson UART driver");
> +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 719faf92aa8a..53efe841656f 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -310,6 +310,14 @@ static const struct serial8250_config uart_config[] = {
>  		.rxtrig_bytes	= {1, 8, 16, 30},
>  		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
>  	},
> +	[PORT_LOONGSON] = {
> +		.name		= "Loongson",
> +		.fifo_size	= 16,
> +		.tx_loadsz	= 16,
> +		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> +		.rxtrig_bytes   = {1, 4, 8, 14},
> +		.flags		= UART_CAP_FIFO,
> +	},

This is exactly identical to PORT_16550A. Just use PORT_16550A instead of 
adding a new one unnecessarily.

--
 i.

>  };
>  
>  /* Uart divisor latch read */
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index f64ef0819cd4..98236b3bec10 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -468,6 +468,16 @@ config SERIAL_8250_OMAP_TTYO_FIXUP
>  	  not booting kernel because the serial console remains silent in case
>  	  they forgot to update the command line.
>  
> +config SERIAL_8250_LOONGSON
> +	tristate "Loongson 8250 based serial port"
> +	depends on SERIAL_8250
> +	depends on LOONGARCH || COMPILE_TEST
> +	help
> +	  If you have a machine based on LoongArch CPU you can enable
> +	  its onboard serial ports by enabling this option. The option
> +	  is applicable to both devicetree and ACPI, say Y to this option.
> +	  If unsure, say N.
> +
>  config SERIAL_8250_LPC18XX
>  	tristate "NXP LPC18xx/43xx serial port support"
>  	depends on SERIAL_8250 && OF && (ARCH_LPC18XX || COMPILE_TEST)
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 513a0941c284..e318a3240789 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_SERIAL_8250_HP300)		+= 8250_hp300.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_INGENIC)	+= 8250_ingenic.o
>  obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o
> +obj-$(CONFIG_SERIAL_8250_LOONGSON)	+= 8250_loongson.o
>  obj-$(CONFIG_SERIAL_8250_LPC18XX)	+= 8250_lpc18xx.o
>  obj-$(CONFIG_SERIAL_8250_LPSS)		+= 8250_lpss.o
>  obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 9c007a106330..607cf060a72a 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -31,6 +31,7 @@
>  #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
>  #define PORT_RT2880	29	/* Ralink RT2880 internal UART */
>  #define PORT_16550A_FSL64 30	/* Freescale 16550 UART with 64 FIFOs */
> +#define PORT_LOONGSON	31	/* Loongson 16550 UART */
>  
>  /*
>   * ARM specific type numbers.  These are not currently guaranteed
>