[PATCH v4 0/2] serial: add KEBA UART driver

[PATCH v4 0/2] serial: add KEBA UART driver

[Gerhard Engleder]

First the serial subsystem is prepared to keep rs485 settings from the
driver if no firmware node exists. This enables drivers to configure a
default rs485 mode, which is set by the serial subsystem.

Second the driver for the KEBA UART is added. This driver supports
multiple rs485 modes and selects RS485 as default mode. This UART is
found in KEBA PLC devices. The auxiliary devices for this driver are
created by the cp500 driver.

v4:
- fix spelling, exist -> exists (Lukas Wunner)
- remove duplicate include (Lukas Wunner)
- order includes alphabetically (Lukas Wunner)

v3:
- add info to commit why device tree / ACPI is not possible (Lukas Wunner)
- separate if and comment for change in uart_get_rs485_mode() (Lukas Wunner)

v2:
- use BIT() for flag definition (Jiri Slaby)
- use enum for UART mode definition (Jiri Slaby)
- use BIT() for capability flag definition (Jiri Slaby)
- use GENMASK() for capability mask (Jiri Slaby)
- use unsigned int for serial line number (Jiri Slaby)
- use unsigned int for flags (Jiri Slaby)

Gerhard Engleder (2):
  serial: Keep rs485 settings for devices without firmware node
  serial: 8250: add driver for KEBA UART

 drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |  13 ++
 drivers/tty/serial/8250/Makefile    |   1 +
 drivers/tty/serial/serial_core.c    |   8 +
 4 files changed, 302 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_keba.c

-- 
2.39.5

[PATCH v4 1/2] serial: Keep rs485 settings for devices without firmware node

[Gerhard Engleder]

From: Gerhard Engleder <eg@keba.com>

Commit fe7f0fa43cef ("serial: 8250: Support rs485 devicetree properties")
retrieves rs485 properties for 8250 drivers. These properties are read
from the firmware node of the device within uart_get_rs485_mode(). If the
firmware node does not exist, then the rs485 flags are still reset. Thus,
8250 driver cannot set rs485 flags to enable a defined rs485 mode during
driver loading. This is no problem so far, as no 8250 driver sets the
rs485 flags.

The default rs485 mode can also be set by firmware nodes. But for some
devices a firmware node does not exist. E.g., for a PCIe based serial
interface on x86 no device tree is available and the ACPI information of
the BIOS often cannot by modified. In this case it shall be possible,
that a driver works out of the box by setting a reasonable default rs485
mode.

If no firmware node exists, then it should be possible for the driver to
set a reasonable default rs485 mode. Therefore, reset rs485 flags only
if a firmware node exists.

Signed-off-by: Gerhard Engleder <eg@keba.com>
Cc: Lukas Wunner <lukas@wunner.de>
---
 drivers/tty/serial/serial_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4757293ece8c..70cac77cac61 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3536,6 +3536,14 @@ int uart_get_rs485_mode(struct uart_port *port)
 	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
 		return 0;
 
+	/*
+	 * Retrieve properties only if a firmware node exists. If no firmware
+	 * node exists, then don't touch rs485 config and keep initial rs485
+	 * properties set by driver.
+	 */
+	if (!dev_fwnode(dev))
+		return 0;
+
 	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
 					     rs485_delay, 2);
 	if (!ret) {
-- 
2.39.5

[PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Gerhard Engleder]

From: Gerhard Engleder <eg@keba.com>

The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
mostly 8250 compatible with extension for some UART modes.

3 different variants exist. The simpliest variant supports only RS-232
and is used for debug interfaces. The next variant supports only RS-485
and is used mostly for communication with KEBA panel devices. The third
variant is able to support RS-232, RS-485 and RS-422. For this variant
not only the mode of the UART is configured, also the physics and
transceivers are switched according to the mode.

Signed-off-by: Gerhard Engleder <eg@keba.com>
Tested-by: Daniel Gierlinger <gida@keba.com>
---
 drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |  13 ++
 drivers/tty/serial/8250/Makefile    |   1 +
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_keba.c

diff --git a/drivers/tty/serial/8250/8250_keba.c b/drivers/tty/serial/8250/8250_keba.c
new file mode 100644
index 000000000000..c05b89551b12
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_keba.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 KEBA Industrial Automation GmbH
+ *
+ * Driver for KEBA UART FPGA IP core
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/misc/keba.h>
+#include <linux/module.h>
+
+#include "8250.h"
+
+#define KUART "kuart"
+
+/* flags */
+#define KUART_RS485		BIT(0)
+#define KUART_USE_CAPABILITY	BIT(1)
+
+/* registers */
+#define KUART_VERSION		0x0000
+#define KUART_REVISION		0x0001
+#define KUART_CAPABILITY	0x0002
+#define KUART_CONTROL		0x0004
+#define KUART_BASE		0x000C
+#define KUART_REGSHIFT		2
+#define KUART_CLK		1843200
+
+/* mode flags */
+enum kuart_mode {
+	KUART_MODE_NONE = 0,
+	KUART_MODE_RS485,
+	KUART_MODE_RS422,
+	KUART_MODE_RS232
+};
+
+/* capability flags */
+#define KUART_CAPABILITY_NONE	BIT(KUART_MODE_NONE)
+#define KUART_CAPABILITY_RS485	BIT(KUART_MODE_RS485)
+#define KUART_CAPABILITY_RS422	BIT(KUART_MODE_RS422)
+#define KUART_CAPABILITY_RS232	BIT(KUART_MODE_RS232)
+#define KUART_CAPABILITY_MASK	GENMASK(3, 0)
+
+/* Additional Control Register DTR line configuration */
+#define UART_ACR_DTRLC_MASK		0x18
+#define UART_ACR_DTRLC_COMPAT		0x00
+#define UART_ACR_DTRLC_ENABLE_LOW	0x10
+
+struct kuart {
+	struct keba_uart_auxdev *auxdev;
+	void __iomem *base;
+	unsigned int line;
+
+	unsigned int flags;
+	u8 capability;
+	enum kuart_mode mode;
+};
+
+static void kuart_set_phy_mode(struct kuart *kuart, enum kuart_mode mode)
+{
+	iowrite8(mode, kuart->base + KUART_CONTROL);
+}
+
+static void kuart_enhanced_mode(struct uart_8250_port *up, bool enable)
+{
+	u8 lcr, efr;
+
+	/* backup LCR register */
+	lcr = serial_in(up, UART_LCR);
+
+	/* enable 650 compatible register set (EFR, ...) */
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+	/* enable/disable enhanced mode with indexed control registers */
+	efr = serial_in(up, UART_EFR);
+	if (enable)
+		efr |= UART_EFR_ECB;
+	else
+		efr &= ~UART_EFR_ECB;
+	serial_out(up, UART_EFR, efr);
+
+	/* disable 650 compatible register set, restore LCR */
+	serial_out(up, UART_LCR, lcr);
+}
+
+static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
+{
+	u8 acr;
+
+	/* set index register to 0 to access ACR register */
+	serial_out(up, UART_SCR, UART_ACR);
+
+	/* set value register to 0x10 writing DTR mode (1,0) */
+	acr = serial_in(up, UART_LSR);
+	acr &= ~UART_ACR_DTRLC_MASK;
+	acr |= dtrlc;
+	serial_out(up, UART_LSR, acr);
+}
+
+static int kuart_rs485_config(struct uart_port *port, struct ktermios *termios,
+			      struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct kuart *kuart = port->private_data;
+	enum kuart_mode mode;
+	u8 dtrlc;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		if (rs485->flags & SER_RS485_MODE_RS422)
+			mode = KUART_MODE_RS422;
+		else
+			mode = KUART_MODE_RS485;
+	} else {
+		mode = KUART_MODE_RS232;
+	}
+
+	if (mode == kuart->mode)
+		return 0;
+
+	if (kuart->flags & KUART_USE_CAPABILITY) {
+		/* deactivate physical interface, break before make */
+		kuart_set_phy_mode(kuart, KUART_MODE_NONE);
+	}
+
+	if (mode == KUART_MODE_RS485) {
+		/*
+		 * Set DTR line configuration of 95x UART to DTR mode (1,0).
+		 * In this mode the DTR pin drives the active-low enable pin of
+		 * an external RS485 buffer. The DTR pin will be forced low
+		 * whenever the transmitter is not empty, otherwise DTR pin is
+		 * high.
+		 */
+		dtrlc = UART_ACR_DTRLC_ENABLE_LOW;
+	} else {
+		/*
+		 * Set DTR line configuration of 95x UART to DTR mode (0,0).
+		 * In this mode the DTR pin is compatible with 16C450, 16C550,
+		 * 16C650 and 16c670 (i.e. normal).
+		 */
+		dtrlc = UART_ACR_DTRLC_COMPAT;
+	}
+
+	kuart_enhanced_mode(up, true);
+	kuart_dtr_line_config(up, dtrlc);
+	kuart_enhanced_mode(up, false);
+
+	if (kuart->flags & KUART_USE_CAPABILITY) {
+		/* activate selected physical interface */
+		kuart_set_phy_mode(kuart, mode);
+	}
+
+	kuart->mode = mode;
+
+	return 0;
+}
+
+static int kuart_probe(struct auxiliary_device *auxdev,
+		       const struct auxiliary_device_id *id)
+{
+	struct device *dev = &auxdev->dev;
+	struct uart_8250_port uart = {};
+	struct resource res;
+	struct kuart *kuart;
+	int retval;
+
+	kuart = devm_kzalloc(dev, sizeof(*kuart), GFP_KERNEL);
+	if (!kuart)
+		return -ENOMEM;
+	kuart->auxdev = container_of(auxdev, struct keba_uart_auxdev, auxdev);
+	kuart->flags = id->driver_data;
+	auxiliary_set_drvdata(auxdev, kuart);
+
+	/*
+	 * map only memory in front of UART registers, UART registers will be
+	 * mapped by serial port
+	 */
+	res = kuart->auxdev->io;
+	res.end = res.start + KUART_BASE - 1;
+	kuart->base = devm_ioremap_resource(dev, &res);
+	if (IS_ERR(kuart->base))
+		return PTR_ERR(kuart->base);
+
+	if (kuart->flags & KUART_USE_CAPABILITY) {
+		/*
+		 * supported modes are read from capability register, at least
+		 * one mode other than none must be supported
+		 */
+		kuart->capability = ioread8(kuart->base + KUART_CAPABILITY) &
+				    KUART_CAPABILITY_MASK;
+		if ((kuart->capability & ~KUART_CAPABILITY_NONE) == 0)
+			return -EIO;
+	}
+
+	spin_lock_init(&uart.port.lock);
+	uart.port.dev = dev;
+	uart.port.mapbase = kuart->auxdev->io.start + KUART_BASE;
+	uart.port.irq = kuart->auxdev->irq;
+	uart.port.uartclk = KUART_CLK;
+	uart.port.private_data = kuart;
+
+	/* 8 bit registers are 32 bit aligned => shift register offset */
+	uart.port.iotype = UPIO_MEM32;
+	uart.port.regshift = KUART_REGSHIFT;
+
+	/*
+	 * UART mixes 16550, 16750 and 16C950 (for RS485) standard => auto
+	 * configuration works best
+	 */
+	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_IOREMAP;
+
+	/*
+	 * UART supports RS485, RS422 and RS232 with switching of physical
+	 * interface
+	 */
+	uart.port.rs485_config = kuart_rs485_config;
+	if (kuart->flags & KUART_RS485) {
+		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
+						  SER_RS485_RTS_ON_SEND;
+		uart.port.rs485.flags = SER_RS485_ENABLED |
+					SER_RS485_RTS_ON_SEND;
+	}
+	if (kuart->flags & KUART_USE_CAPABILITY) {
+		/* default mode priority is RS485 > RS422 > RS232 */
+		if (kuart->capability & KUART_CAPABILITY_RS422) {
+			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
+							   SER_RS485_RTS_ON_SEND |
+							   SER_RS485_MODE_RS422;
+			uart.port.rs485.flags = SER_RS485_ENABLED |
+						SER_RS485_RTS_ON_SEND |
+						SER_RS485_MODE_RS422;
+		}
+		if (kuart->capability & KUART_CAPABILITY_RS485) {
+			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
+							   SER_RS485_RTS_ON_SEND;
+			uart.port.rs485.flags = SER_RS485_ENABLED |
+						SER_RS485_RTS_ON_SEND;
+		}
+	}
+
+	retval = serial8250_register_8250_port(&uart);
+	if (retval < 0) {
+		dev_err(&auxdev->dev, "UART registration failed!\n");
+		return retval;
+	}
+	kuart->line = retval;
+
+	return 0;
+}
+
+static void kuart_remove(struct auxiliary_device *auxdev)
+{
+	struct kuart *kuart = auxiliary_get_drvdata(auxdev);
+
+	if (kuart->flags & KUART_USE_CAPABILITY)
+		kuart_set_phy_mode(kuart, KUART_MODE_NONE);
+
+	serial8250_unregister_port(kuart->line);
+}
+
+static const struct auxiliary_device_id kuart_devtype_aux[] = {
+	{ .name = "keba.rs485-uart", .driver_data = KUART_RS485 },
+	{ .name = "keba.rs232-uart", .driver_data = 0 },
+	{ .name = "keba.uart", .driver_data = KUART_USE_CAPABILITY },
+	{}
+};
+MODULE_DEVICE_TABLE(auxiliary, kuart_devtype_aux);
+
+static struct auxiliary_driver kuart_driver_aux = {
+	.name = KUART,
+	.id_table = kuart_devtype_aux,
+	.probe  = kuart_probe,
+	.remove = kuart_remove,
+};
+module_auxiliary_driver(kuart_driver_aux);
+
+MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
+MODULE_DESCRIPTION("KEBA 8250 serial port driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index f64ef0819cd4..5c3e4bcb3b93 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -430,6 +430,19 @@ config SERIAL_8250_IOC3
 	  behind the IOC3 device on those systems.  Maximum baud speed is
 	  38400bps using this driver.
 
+config SERIAL_8250_KEBA
+	tristate "Support for KEBA 8250 UART"
+	depends on SERIAL_8250
+	depends on KEBA_CP500
+	help
+	  Selecting this option will add support for KEBA UARTs. These UARTs
+	  are used for the serial interfaces of KEBA PLCs.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called 8250_keba.
+
+	  If unsure, say N.
+
 config SERIAL_8250_RT288X
 	bool "Ralink RT288x/RT305x/RT3662/RT3883 serial port support"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 513a0941c284..f7a463c9860a 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_KEBA)		+= 8250_keba.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
-- 
2.39.5

Re: [PATCH v4 1/2] serial: Keep rs485 settings for devices without firmware node

[Lukas Wunner]

On Thu, Oct 23, 2025 at 05:12:28PM +0200, Gerhard Engleder wrote:
> Commit fe7f0fa43cef ("serial: 8250: Support rs485 devicetree properties")
> retrieves rs485 properties for 8250 drivers. These properties are read
> from the firmware node of the device within uart_get_rs485_mode(). If the
> firmware node does not exist, then the rs485 flags are still reset. Thus,
> 8250 driver cannot set rs485 flags to enable a defined rs485 mode during
> driver loading. This is no problem so far, as no 8250 driver sets the
> rs485 flags.
> 
> The default rs485 mode can also be set by firmware nodes. But for some
> devices a firmware node does not exist. E.g., for a PCIe based serial
> interface on x86 no device tree is available and the ACPI information of
> the BIOS often cannot by modified. In this case it shall be possible,
> that a driver works out of the box by setting a reasonable default rs485
> mode.
> 
> If no firmware node exists, then it should be possible for the driver to
> set a reasonable default rs485 mode. Therefore, reset rs485 flags only
> if a firmware node exists.
> 
> Signed-off-by: Gerhard Engleder <eg@keba.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Lukas Wunner]

On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:
> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
> mostly 8250 compatible with extension for some UART modes.
> 
> 3 different variants exist. The simpliest variant supports only RS-232
> and is used for debug interfaces. The next variant supports only RS-485
> and is used mostly for communication with KEBA panel devices. The third
> variant is able to support RS-232, RS-485 and RS-422. For this variant
> not only the mode of the UART is configured, also the physics and
> transceivers are switched according to the mode.
> 
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> Tested-by: Daniel Gierlinger <gida@keba.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

Re: [PATCH v4 1/2] serial: Keep rs485 settings for devices without firmware node

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 05:12:28PM +0200, Gerhard Engleder wrote:
> 
> Commit fe7f0fa43cef ("serial: 8250: Support rs485 devicetree properties")
> retrieves rs485 properties for 8250 drivers. These properties are read
> from the firmware node of the device within uart_get_rs485_mode(). If the
> firmware node does not exist, then the rs485 flags are still reset. Thus,
> 8250 driver cannot set rs485 flags to enable a defined rs485 mode during
> driver loading. This is no problem so far, as no 8250 driver sets the
> rs485 flags.
> 
> The default rs485 mode can also be set by firmware nodes. But for some
> devices a firmware node does not exist. E.g., for a PCIe based serial
> interface on x86 no device tree is available and the ACPI information of
> the BIOS often cannot by modified.

While the code is okay per se, the above statement is not fully true.
We specifically have device properties and software nodes to provide
the missing bits.

> In this case it shall be possible,
> that a driver works out of the box by setting a reasonable default rs485
> mode.
> 
> If no firmware node exists, then it should be possible for the driver to
> set a reasonable default rs485 mode. Therefore, reset rs485 flags only
> if a firmware node exists.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:
> 
> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
> mostly 8250 compatible with extension for some UART modes.
> 
> 3 different variants exist. The simpliest variant supports only RS-232
> and is used for debug interfaces. The next variant supports only RS-485
> and is used mostly for communication with KEBA panel devices. The third
> variant is able to support RS-232, RS-485 and RS-422. For this variant
> not only the mode of the UART is configured, also the physics and
> transceivers are switched according to the mode.

...

> +#include <linux/auxiliary_bus.h>

+ bits.h
+ container_of.h

> +#include <linux/device.h>

I don't see how it's being used.
What I see are

+ dev_printk.h
+ device/devres.h

+ err.h

> +#include <linux/io.h>
> +#include <linux/misc/keba.h>

+ mod_devicetable.h

> +#include <linux/module.h>

+ spinlock.h
+ types.h

...

> +static int kuart_probe(struct auxiliary_device *auxdev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct uart_8250_port uart = {};
> +	struct resource res;
> +	struct kuart *kuart;
> +	int retval;
> +
> +	kuart = devm_kzalloc(dev, sizeof(*kuart), GFP_KERNEL);
> +	if (!kuart)
> +		return -ENOMEM;
> +	kuart->auxdev = container_of(auxdev, struct keba_uart_auxdev, auxdev);
> +	kuart->flags = id->driver_data;
> +	auxiliary_set_drvdata(auxdev, kuart);
> +
> +	/*
> +	 * map only memory in front of UART registers, UART registers will be
> +	 * mapped by serial port
> +	 */
> +	res = kuart->auxdev->io;
> +	res.end = res.start + KUART_BASE - 1;
> +	kuart->base = devm_ioremap_resource(dev, &res);
> +	if (IS_ERR(kuart->base))
> +		return PTR_ERR(kuart->base);
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/*
> +		 * supported modes are read from capability register, at least
> +		 * one mode other than none must be supported
> +		 */
> +		kuart->capability = ioread8(kuart->base + KUART_CAPABILITY) &
> +				    KUART_CAPABILITY_MASK;
> +		if ((kuart->capability & ~KUART_CAPABILITY_NONE) == 0)
> +			return -EIO;
> +	}
> +
> +	spin_lock_init(&uart.port.lock);
> +	uart.port.dev = dev;
> +	uart.port.mapbase = kuart->auxdev->io.start + KUART_BASE;
> +	uart.port.irq = kuart->auxdev->irq;
> +	uart.port.uartclk = KUART_CLK;
> +	uart.port.private_data = kuart;
> +
> +	/* 8 bit registers are 32 bit aligned => shift register offset */
> +	uart.port.iotype = UPIO_MEM32;
> +	uart.port.regshift = KUART_REGSHIFT;

Can't you call uart_read_port_properties()?

If ever you gain some properties either via FW description or via software
nodes, they will be automatically used without need to update the driver!

> +	/*
> +	 * UART mixes 16550, 16750 and 16C950 (for RS485) standard => auto
> +	 * configuration works best
> +	 */
> +	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_IOREMAP;
> +
> +	/*
> +	 * UART supports RS485, RS422 and RS232 with switching of physical
> +	 * interface
> +	 */
> +	uart.port.rs485_config = kuart_rs485_config;
> +	if (kuart->flags & KUART_RS485) {
> +		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
> +						  SER_RS485_RTS_ON_SEND;
> +		uart.port.rs485.flags = SER_RS485_ENABLED |
> +					SER_RS485_RTS_ON_SEND;
> +	}
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/* default mode priority is RS485 > RS422 > RS232 */
> +		if (kuart->capability & KUART_CAPABILITY_RS422) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND |
> +							   SER_RS485_MODE_RS422;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND |
> +						SER_RS485_MODE_RS422;
> +		}
> +		if (kuart->capability & KUART_CAPABILITY_RS485) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND;
> +		}
> +	}
> +
> +	retval = serial8250_register_8250_port(&uart);
> +	if (retval < 0) {

> +		dev_err(&auxdev->dev, "UART registration failed!\n");
> +		return retval;

		return dev_err_probe(...);

> +	}
> +	kuart->line = retval;
> +
> +	return 0;
> +}

...

Since driver is about to be applied to serial-next, I suggest to send a
followup(s) to address my comments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Ilpo Järvinen]

On Thu, 23 Oct 2025, Gerhard Engleder wrote:

> From: Gerhard Engleder <eg@keba.com>
> 
> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
> mostly 8250 compatible with extension for some UART modes.
> 
> 3 different variants exist. The simpliest variant supports only RS-232
> and is used for debug interfaces. The next variant supports only RS-485
> and is used mostly for communication with KEBA panel devices. The third
> variant is able to support RS-232, RS-485 and RS-422. For this variant
> not only the mode of the UART is configured, also the physics and
> transceivers are switched according to the mode.
> 
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> Tested-by: Daniel Gierlinger <gida@keba.com>
> ---
>  drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig     |  13 ++
>  drivers/tty/serial/8250/Makefile    |   1 +
>  3 files changed, 294 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_keba.c
> 
> diff --git a/drivers/tty/serial/8250/8250_keba.c b/drivers/tty/serial/8250/8250_keba.c
> new file mode 100644
> index 000000000000..c05b89551b12
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_keba.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 KEBA Industrial Automation GmbH
> + *
> + * Driver for KEBA UART FPGA IP core
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/misc/keba.h>
> +#include <linux/module.h>

+ linux/serial_core.h

> +
> +#include "8250.h"
> +
> +#define KUART "kuart"
> +
> +/* flags */
> +#define KUART_RS485		BIT(0)
> +#define KUART_USE_CAPABILITY	BIT(1)
> +
> +/* registers */
> +#define KUART_VERSION		0x0000
> +#define KUART_REVISION		0x0001
> +#define KUART_CAPABILITY	0x0002
> +#define KUART_CONTROL		0x0004
> +#define KUART_BASE		0x000C
> +#define KUART_REGSHIFT		2
> +#define KUART_CLK		1843200
> +
> +/* mode flags */
> +enum kuart_mode {
> +	KUART_MODE_NONE = 0,
> +	KUART_MODE_RS485,
> +	KUART_MODE_RS422,
> +	KUART_MODE_RS232
> +};
> +
> +/* capability flags */
> +#define KUART_CAPABILITY_NONE	BIT(KUART_MODE_NONE)
> +#define KUART_CAPABILITY_RS485	BIT(KUART_MODE_RS485)
> +#define KUART_CAPABILITY_RS422	BIT(KUART_MODE_RS422)
> +#define KUART_CAPABILITY_RS232	BIT(KUART_MODE_RS232)
> +#define KUART_CAPABILITY_MASK	GENMASK(3, 0)
> +
> +/* Additional Control Register DTR line configuration */
> +#define UART_ACR_DTRLC_MASK		0x18
> +#define UART_ACR_DTRLC_COMPAT		0x00
> +#define UART_ACR_DTRLC_ENABLE_LOW	0x10
> +
> +struct kuart {
> +	struct keba_uart_auxdev *auxdev;
> +	void __iomem *base;
> +	unsigned int line;
> +
> +	unsigned int flags;
> +	u8 capability;
> +	enum kuart_mode mode;
> +};
> +
> +static void kuart_set_phy_mode(struct kuart *kuart, enum kuart_mode mode)
> +{
> +	iowrite8(mode, kuart->base + KUART_CONTROL);
> +}
> +
> +static void kuart_enhanced_mode(struct uart_8250_port *up, bool enable)
> +{
> +	u8 lcr, efr;
> +
> +	/* backup LCR register */

Save + restore is quite obvious thing. IMO, no comment is needed about it.

> +	lcr = serial_in(up, UART_LCR);
> +
> +	/* enable 650 compatible register set (EFR, ...) */
> +	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> +
> +	/* enable/disable enhanced mode with indexed control registers */
> +	efr = serial_in(up, UART_EFR);
> +	if (enable)
> +		efr |= UART_EFR_ECB;
> +	else
> +		efr &= ~UART_EFR_ECB;
> +	serial_out(up, UART_EFR, efr);
> +
> +	/* disable 650 compatible register set, restore LCR */
> +	serial_out(up, UART_LCR, lcr);
> +}
> +
> +static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
> +{
> +	u8 acr;
> +
> +	/* set index register to 0 to access ACR register */
> +	serial_out(up, UART_SCR, UART_ACR);

So the scratch register has some special use on this UART (register 
multiplexer?), it would probably better name it with define, if that's the 
case.

> +
> +	/* set value register to 0x10 writing DTR mode (1,0) */
> +	acr = serial_in(up, UART_LSR);
> +	acr &= ~UART_ACR_DTRLC_MASK;
> +	acr |= dtrlc;
> +	serial_out(up, UART_LSR, acr);
> +}
> +
> +static int kuart_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			      struct serial_rs485 *rs485)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	struct kuart *kuart = port->private_data;
> +	enum kuart_mode mode;
> +	u8 dtrlc;
> +
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		if (rs485->flags & SER_RS485_MODE_RS422)
> +			mode = KUART_MODE_RS422;
> +		else
> +			mode = KUART_MODE_RS485;
> +	} else {
> +		mode = KUART_MODE_RS232;
> +	}
> +
> +	if (mode == kuart->mode)
> +		return 0;
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/* deactivate physical interface, break before make */
> +		kuart_set_phy_mode(kuart, KUART_MODE_NONE);
> +	}
> +
> +	if (mode == KUART_MODE_RS485) {
> +		/*
> +		 * Set DTR line configuration of 95x UART to DTR mode (1,0).
> +		 * In this mode the DTR pin drives the active-low enable pin of
> +		 * an external RS485 buffer. The DTR pin will be forced low
> +		 * whenever the transmitter is not empty, otherwise DTR pin is
> +		 * high.
> +		 */
> +		dtrlc = UART_ACR_DTRLC_ENABLE_LOW;
> +	} else {
> +		/*
> +		 * Set DTR line configuration of 95x UART to DTR mode (0,0).
> +		 * In this mode the DTR pin is compatible with 16C450, 16C550,
> +		 * 16C650 and 16c670 (i.e. normal).
> +		 */
> +		dtrlc = UART_ACR_DTRLC_COMPAT;
> +	}
> +
> +	kuart_enhanced_mode(up, true);
> +	kuart_dtr_line_config(up, dtrlc);
> +	kuart_enhanced_mode(up, false);
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/* activate selected physical interface */
> +		kuart_set_phy_mode(kuart, mode);
> +	}
> +
> +	kuart->mode = mode;
> +
> +	return 0;
> +}
> +
> +static int kuart_probe(struct auxiliary_device *auxdev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct uart_8250_port uart = {};
> +	struct resource res;
> +	struct kuart *kuart;
> +	int retval;
> +
> +	kuart = devm_kzalloc(dev, sizeof(*kuart), GFP_KERNEL);
> +	if (!kuart)
> +		return -ENOMEM;
> +	kuart->auxdev = container_of(auxdev, struct keba_uart_auxdev, auxdev);
> +	kuart->flags = id->driver_data;
> +	auxiliary_set_drvdata(auxdev, kuart);
> +
> +	/*
> +	 * map only memory in front of UART registers, UART registers will be
> +	 * mapped by serial port
> +	 */
> +	res = kuart->auxdev->io;
> +	res.end = res.start + KUART_BASE - 1;
> +	kuart->base = devm_ioremap_resource(dev, &res);
> +	if (IS_ERR(kuart->base))
> +		return PTR_ERR(kuart->base);
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/*
> +		 * supported modes are read from capability register, at least
> +		 * one mode other than none must be supported
> +		 */
> +		kuart->capability = ioread8(kuart->base + KUART_CAPABILITY) &
> +				    KUART_CAPABILITY_MASK;
> +		if ((kuart->capability & ~KUART_CAPABILITY_NONE) == 0)
> +			return -EIO;
> +	}
> +
> +	spin_lock_init(&uart.port.lock);
> +	uart.port.dev = dev;
> +	uart.port.mapbase = kuart->auxdev->io.start + KUART_BASE;
> +	uart.port.irq = kuart->auxdev->irq;
> +	uart.port.uartclk = KUART_CLK;
> +	uart.port.private_data = kuart;
> +
> +	/* 8 bit registers are 32 bit aligned => shift register offset */
> +	uart.port.iotype = UPIO_MEM32;
> +	uart.port.regshift = KUART_REGSHIFT;
> +
> +	/*
> +	 * UART mixes 16550, 16750 and 16C950 (for RS485) standard => auto
> +	 * configuration works best
> +	 */
> +	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_IOREMAP;
> +
> +	/*
> +	 * UART supports RS485, RS422 and RS232 with switching of physical
> +	 * interface
> +	 */
> +	uart.port.rs485_config = kuart_rs485_config;
> +	if (kuart->flags & KUART_RS485) {
> +		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
> +						  SER_RS485_RTS_ON_SEND;
> +		uart.port.rs485.flags = SER_RS485_ENABLED |
> +					SER_RS485_RTS_ON_SEND;
> +	}
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/* default mode priority is RS485 > RS422 > RS232 */
> +		if (kuart->capability & KUART_CAPABILITY_RS422) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND |
> +							   SER_RS485_MODE_RS422;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND |
> +						SER_RS485_MODE_RS422;
> +		}
> +		if (kuart->capability & KUART_CAPABILITY_RS485) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND;
> +		}
> +	}

Is it so that only one mode is supported or can that be changes using 
kuart_rs485_config() in which case you should have all flags listed (you 
seem to talk about priority so that sounds like all are supported)?

> +
> +	retval = serial8250_register_8250_port(&uart);
> +	if (retval < 0) {
> +		dev_err(&auxdev->dev, "UART registration failed!\n");

Missing header.

> +		return retval;
> +	}
> +	kuart->line = retval;
> +
> +	return 0;
> +}
> +
> +static void kuart_remove(struct auxiliary_device *auxdev)
> +{
> +	struct kuart *kuart = auxiliary_get_drvdata(auxdev);
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY)
> +		kuart_set_phy_mode(kuart, KUART_MODE_NONE);
> +
> +	serial8250_unregister_port(kuart->line);
> +}
> +
> +static const struct auxiliary_device_id kuart_devtype_aux[] = {
> +	{ .name = "keba.rs485-uart", .driver_data = KUART_RS485 },
> +	{ .name = "keba.rs232-uart", .driver_data = 0 },
> +	{ .name = "keba.uart", .driver_data = KUART_USE_CAPABILITY },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, kuart_devtype_aux);
> +
> +static struct auxiliary_driver kuart_driver_aux = {
> +	.name = KUART,
> +	.id_table = kuart_devtype_aux,
> +	.probe  = kuart_probe,
> +	.remove = kuart_remove,
> +};
> +module_auxiliary_driver(kuart_driver_aux);
> +
> +MODULE_AUTHOR("Gerhard Engleder <eg@keba.com>");
> +MODULE_DESCRIPTION("KEBA 8250 serial port driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index f64ef0819cd4..5c3e4bcb3b93 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -430,6 +430,19 @@ config SERIAL_8250_IOC3
>  	  behind the IOC3 device on those systems.  Maximum baud speed is
>  	  38400bps using this driver.
>  
> +config SERIAL_8250_KEBA
> +	tristate "Support for KEBA 8250 UART"
> +	depends on SERIAL_8250
> +	depends on KEBA_CP500
> +	help
> +	  Selecting this option will add support for KEBA UARTs. These UARTs
> +	  are used for the serial interfaces of KEBA PLCs.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called 8250_keba.
> +
> +	  If unsure, say N.
> +
>  config SERIAL_8250_RT288X
>  	bool "Ralink RT288x/RT305x/RT3662/RT3883 serial port support"
>  	depends on SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 513a0941c284..f7a463c9860a 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_KEBA)		+= 8250_keba.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
> 

-- 
 i.

Re: [PATCH v4 1/2] serial: Keep rs485 settings for devices without firmware node

[Gerhard Engleder]

On 26.11.25 14:10, Andy Shevchenko wrote:
> On Thu, Oct 23, 2025 at 05:12:28PM +0200, Gerhard Engleder wrote:
>>
>> Commit fe7f0fa43cef ("serial: 8250: Support rs485 devicetree properties")
>> retrieves rs485 properties for 8250 drivers. These properties are read
>> from the firmware node of the device within uart_get_rs485_mode(). If the
>> firmware node does not exist, then the rs485 flags are still reset. Thus,
>> 8250 driver cannot set rs485 flags to enable a defined rs485 mode during
>> driver loading. This is no problem so far, as no 8250 driver sets the
>> rs485 flags.
>>
>> The default rs485 mode can also be set by firmware nodes. But for some
>> devices a firmware node does not exist. E.g., for a PCIe based serial
>> interface on x86 no device tree is available and the ACPI information of
>> the BIOS often cannot by modified.
> 
> While the code is okay per se, the above statement is not fully true.
> We specifically have device properties and software nodes to provide
> the missing bits.

Yes, Lukas Wunner teached me that possibility. I did not mention this
alternative solution in the commit, because I did not use it.

Gerhard

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Gerhard Engleder]

On 26.11.25 14:17, Andy Shevchenko wrote:
> On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:
>>
>> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
>> mostly 8250 compatible with extension for some UART modes.
>>
>> 3 different variants exist. The simpliest variant supports only RS-232
>> and is used for debug interfaces. The next variant supports only RS-485
>> and is used mostly for communication with KEBA panel devices. The third
>> variant is able to support RS-232, RS-485 and RS-422. For this variant
>> not only the mode of the UART is configured, also the physics and
>> transceivers are switched according to the mode.
> 
> ...
> 
>> +#include <linux/auxiliary_bus.h>
> 
> + bits.h
> + container_of.h
> 
>> +#include <linux/device.h>
> 
> I don't see how it's being used.
> What I see are
> 
> + dev_printk.h
> + device/devres.h
> 
> + err.h
> 
>> +#include <linux/io.h>
>> +#include <linux/misc/keba.h>
> 
> + mod_devicetable.h
> 
>> +#include <linux/module.h>
> 
> + spinlock.h
> + types.h

Is there any extra tool to check for missing headers? Or do I have to
check the header for every used function?

>> +static int kuart_probe(struct auxiliary_device *auxdev,
>> +		       const struct auxiliary_device_id *id)
>> +{

...

>> +	spin_lock_init(&uart.port.lock);
>> +	uart.port.dev = dev;
>> +	uart.port.mapbase = kuart->auxdev->io.start + KUART_BASE;
>> +	uart.port.irq = kuart->auxdev->irq;
>> +	uart.port.uartclk = KUART_CLK;
>> +	uart.port.private_data = kuart;
>> +
>> +	/* 8 bit registers are 32 bit aligned => shift register offset */
>> +	uart.port.iotype = UPIO_MEM32;
>> +	uart.port.regshift = KUART_REGSHIFT;
> 
> Can't you call uart_read_port_properties()?
> 
> If ever you gain some properties either via FW description or via software
> nodes, they will be automatically used without need to update the driver!

Yes that would be some nice behavior. But __uart_read_properties()
sets some defaults even if no firmware node exist (UPF_SHARE_IRQ, 0 as
irq number or it fails if not irq number is found). So
__uart_read_properties() would need to be changed. IMO it makes no sense
to change __uart_read_properties() as this functionality is currently
not required.

> 
>> +	/*
>> +	 * UART mixes 16550, 16750 and 16C950 (for RS485) standard => auto
>> +	 * configuration works best

...

>> +	}
>> +
>> +	retval = serial8250_register_8250_port(&uart);
>> +	if (retval < 0) {
> 
>> +		dev_err(&auxdev->dev, "UART registration failed!\n");
>> +		return retval;
> 
> 		return dev_err_probe(...);

Yes that's simpler.

> 
>> +	}
>> +	kuart->line = retval;
>> +
>> +	return 0;
>> +}
> 
> ...
> 
> Since driver is about to be applied to serial-next, I suggest to send a
> followup(s) to address my comments.

I will do a follow up.

Gerhard

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Gerhard Engleder]

On 26.11.25 16:46, Ilpo Järvinen wrote:
> On Thu, 23 Oct 2025, Gerhard Engleder wrote:
> 
>> From: Gerhard Engleder <eg@keba.com>
>>
>> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
>> mostly 8250 compatible with extension for some UART modes.
>>
>> 3 different variants exist. The simpliest variant supports only RS-232
>> and is used for debug interfaces. The next variant supports only RS-485
>> and is used mostly for communication with KEBA panel devices. The third
>> variant is able to support RS-232, RS-485 and RS-422. For this variant
>> not only the mode of the UART is configured, also the physics and
>> transceivers are switched according to the mode.
>>
>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>> Tested-by: Daniel Gierlinger <gida@keba.com>
>> ---
>>   drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
>>   drivers/tty/serial/8250/Kconfig     |  13 ++
>>   drivers/tty/serial/8250/Makefile    |   1 +
>>   3 files changed, 294 insertions(+)
>>   create mode 100644 drivers/tty/serial/8250/8250_keba.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_keba.c b/drivers/tty/serial/8250/8250_keba.c
>> new file mode 100644
>> index 000000000000..c05b89551b12
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_keba.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2025 KEBA Industrial Automation GmbH
>> + *
>> + * Driver for KEBA UART FPGA IP core
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/misc/keba.h>
>> +#include <linux/module.h>
> 
> + linux/serial_core.h

Is this really necessary even with the include of 8250.h below?

>> +
>> +#include "8250.h"
>> +
>> +#define KUART "kuart"

...

>> +static void kuart_enhanced_mode(struct uart_8250_port *up, bool enable)
>> +{
>> +	u8 lcr, efr;
>> +
>> +	/* backup LCR register */
> 
> Save + restore is quite obvious thing. IMO, no comment is needed about it.

Yes it could be ommited. The patch is already merged, so I would keep
it. Ok?

>> +	lcr = serial_in(up, UART_LCR);
>> +
>> +	/* enable 650 compatible register set (EFR, ...) */
>> +	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>> +
>> +	/* enable/disable enhanced mode with indexed control registers */
>> +	efr = serial_in(up, UART_EFR);
>> +	if (enable)
>> +		efr |= UART_EFR_ECB;
>> +	else
>> +		efr &= ~UART_EFR_ECB;
>> +	serial_out(up, UART_EFR, efr);
>> +
>> +	/* disable 650 compatible register set, restore LCR */
>> +	serial_out(up, UART_LCR, lcr);
>> +}
>> +
>> +static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
>> +{
>> +	u8 acr;
>> +
>> +	/* set index register to 0 to access ACR register */
>> +	serial_out(up, UART_SCR, UART_ACR);
> 
> So the scratch register has some special use on this UART (register
> multiplexer?), it would probably better name it with define, if that's the
> case.

This UART supports an enhanced mode, which changes the behavior of some
registers. But the register still have their normal use with enhanced
mode disabled. So I would keep the register name.

>> +
>> +	/* set value register to 0x10 writing DTR mode (1,0) */
>> +	acr = serial_in(up, UART_LSR);
>> +	acr &= ~UART_ACR_DTRLC_MASK;
>> +	acr |= dtrlc;
>> +	serial_out(up, UART_LSR, acr);
>> +}

...

>> +	/*
>> +	 * UART supports RS485, RS422 and RS232 with switching of physical
>> +	 * interface
>> +	 */
>> +	uart.port.rs485_config = kuart_rs485_config;
>> +	if (kuart->flags & KUART_RS485) {
>> +		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
>> +						  SER_RS485_RTS_ON_SEND;
>> +		uart.port.rs485.flags = SER_RS485_ENABLED |
>> +					SER_RS485_RTS_ON_SEND;
>> +	}
>> +	if (kuart->flags & KUART_USE_CAPABILITY) {
>> +		/* default mode priority is RS485 > RS422 > RS232 */
>> +		if (kuart->capability & KUART_CAPABILITY_RS422) {
>> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
>> +							   SER_RS485_RTS_ON_SEND |
>> +							   SER_RS485_MODE_RS422;
>> +			uart.port.rs485.flags = SER_RS485_ENABLED |
>> +						SER_RS485_RTS_ON_SEND |
>> +						SER_RS485_MODE_RS422;
>> +		}
>> +		if (kuart->capability & KUART_CAPABILITY_RS485) {
>> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
>> +							   SER_RS485_RTS_ON_SEND;
>> +			uart.port.rs485.flags = SER_RS485_ENABLED |
>> +						SER_RS485_RTS_ON_SEND;
>> +		}
>> +	}
> 
> Is it so that only one mode is supported or can that be changes using
> kuart_rs485_config() in which case you should have all flags listed (you
> seem to talk about priority so that sounds like all are supported)?

Both. As written in the commit message, there are 3 variants of the
device. 2 variants support only one mode and 1 variant supports up to
3 modes.

>> +
>> +	retval = serial8250_register_8250_port(&uart);
>> +	if (retval < 0) {
>> +		dev_err(&auxdev->dev, "UART registration failed!\n");
> 
> Missing header.

I will check for the header.

As this patch is already merged, I will do a follow up.

regards, gerhard

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Andy Shevchenko]

On Wed, Nov 26, 2025 at 10:06:17PM +0100, Gerhard Engleder wrote:
> On 26.11.25 14:17, Andy Shevchenko wrote:
> > On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:

...

> > > +#include <linux/auxiliary_bus.h>
> > 
> > + bits.h
> > + container_of.h
> > 
> > > +#include <linux/device.h>
> > 
> > I don't see how it's being used.
> > What I see are
> > 
> > + dev_printk.h
> > + device/devres.h
> > 
> > + err.h
> > 
> > > +#include <linux/io.h>
> > > +#include <linux/misc/keba.h>
> > 
> > + mod_devicetable.h
> > 
> > > +#include <linux/module.h>
> > 
> > + spinlock.h
> > + types.h
> 
> Is there any extra tool to check for missing headers? Or do I have to
> check the header for every used function?

There is iwyu, which is heavily tweaked by Jonathan Cameron for use in Linux
kernel and even though it gives some false positives.

I did this manually by reading the code of the driver and remembering which
header provides which API.

...

> > Can't you call uart_read_port_properties()?
> > 
> > If ever you gain some properties either via FW description or via software
> > nodes, they will be automatically used without need to update the driver!
> 
> Yes that would be some nice behavior. But __uart_read_properties()
> sets some defaults even if no firmware node exist (UPF_SHARE_IRQ,

Is it a problem? Even a single IRQ may be marked shared.

> 0 as irq number

It doesn't do that, it just skips setting it in that case.

> or it fails if not irq number is found).

Yeah, this is most "problematic" part in case of this driver. Why is it
auxiliary and not platform? With that the __uart_read_properties() needs
to be updated to also query IRQ from respective aux device, and aux dev
needs implementation of dev_is_auxiliary(). Not that big deal, though.

> So __uart_read_properties() would need to be changed. IMO it makes no sense
> to change __uart_read_properties() as this functionality is currently
> not required.

Yes, but as I said it will give you for free the possibility to use those
properties without future modifications of the driver. OTOH, driver is in
tree and modifications will be needed one way or another :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5998 bytes --]

On Wed, 26 Nov 2025, Gerhard Engleder wrote:

> On 26.11.25 16:46, Ilpo Järvinen wrote:
> > On Thu, 23 Oct 2025, Gerhard Engleder wrote:
> > 
> > > From: Gerhard Engleder <eg@keba.com>
> > > 
> > > The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
> > > mostly 8250 compatible with extension for some UART modes.
> > > 
> > > 3 different variants exist. The simpliest variant supports only RS-232
> > > and is used for debug interfaces. The next variant supports only RS-485
> > > and is used mostly for communication with KEBA panel devices. The third
> > > variant is able to support RS-232, RS-485 and RS-422. For this variant
> > > not only the mode of the UART is configured, also the physics and
> > > transceivers are switched according to the mode.
> > > 
> > > Signed-off-by: Gerhard Engleder <eg@keba.com>
> > > Tested-by: Daniel Gierlinger <gida@keba.com>
> > > ---
> > >   drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
> > >   drivers/tty/serial/8250/Kconfig     |  13 ++
> > >   drivers/tty/serial/8250/Makefile    |   1 +
> > >   3 files changed, 294 insertions(+)
> > >   create mode 100644 drivers/tty/serial/8250/8250_keba.c
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_keba.c
> > > b/drivers/tty/serial/8250/8250_keba.c
> > > new file mode 100644
> > > index 000000000000..c05b89551b12
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_keba.c
> > > @@ -0,0 +1,280 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2025 KEBA Industrial Automation GmbH
> > > + *
> > > + * Driver for KEBA UART FPGA IP core
> > > + */
> > > +
> > > +#include <linux/auxiliary_bus.h>
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/misc/keba.h>
> > > +#include <linux/module.h>
> > 
> > + linux/serial_core.h
> 
> Is this really necessary even with the include of 8250.h below?

Yes. Generally don't rely on indirect includes.

> > > +
> > > +#include "8250.h"
> > > +
> > > +#define KUART "kuart"
> 
> ...
> 
> > > +static void kuart_enhanced_mode(struct uart_8250_port *up, bool enable)
> > > +{
> > > +	u8 lcr, efr;
> > > +
> > > +	/* backup LCR register */
> > 
> > Save + restore is quite obvious thing. IMO, no comment is needed about it.
> 
> Yes it could be ommited. The patch is already merged, so I would keep
> it. Ok?

Ah, I didn't realize this is already merged. Just leave them then, it's 
not that big thing.

> > > +	lcr = serial_in(up, UART_LCR);
> > > +
> > > +	/* enable 650 compatible register set (EFR, ...) */
> > > +	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> > > +
> > > +	/* enable/disable enhanced mode with indexed control registers */
> > > +	efr = serial_in(up, UART_EFR);
> > > +	if (enable)
> > > +		efr |= UART_EFR_ECB;
> > > +	else
> > > +		efr &= ~UART_EFR_ECB;
> > > +	serial_out(up, UART_EFR, efr);
> > > +
> > > +	/* disable 650 compatible register set, restore LCR */
> > > +	serial_out(up, UART_LCR, lcr);
> > > +}
> > > +
> > > +static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
> > > +{
> > > +	u8 acr;
> > > +
> > > +	/* set index register to 0 to access ACR register */
> > > +	serial_out(up, UART_SCR, UART_ACR);
> > 
> > So the scratch register has some special use on this UART (register
> > multiplexer?), it would probably better name it with define, if that's the
> > case.
> 
> This UART supports an enhanced mode, which changes the behavior of some
> registers. But the register still have their normal use with enhanced
> mode disabled. So I would keep the register name.

But this code clearly assume UART is in enhanced mode. Same number can
have different names. You could of course reuse the other define in the 
define:

#define KUART_EMODE_INDEX_REG	UART_SCR

> > > +	/* set value register to 0x10 writing DTR mode (1,0) */
> > > +	acr = serial_in(up, UART_LSR);
> > > +	acr &= ~UART_ACR_DTRLC_MASK;
> > > +	acr |= dtrlc;
> > > +	serial_out(up, UART_LSR, acr);
> > > +}
> 
> ...
> 
> > > +	/*
> > > +	 * UART supports RS485, RS422 and RS232 with switching of physical
> > > +	 * interface
> > > +	 */
> > > +	uart.port.rs485_config = kuart_rs485_config;
> > > +	if (kuart->flags & KUART_RS485) {
> > > +		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
> > > +						  SER_RS485_RTS_ON_SEND;
> > > +		uart.port.rs485.flags = SER_RS485_ENABLED |
> > > +					SER_RS485_RTS_ON_SEND;
> > > +	}
> > > +	if (kuart->flags & KUART_USE_CAPABILITY) {
> > > +		/* default mode priority is RS485 > RS422 > RS232 */
> > > +		if (kuart->capability & KUART_CAPABILITY_RS422) {
> > > +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> > > +
> > > SER_RS485_RTS_ON_SEND |
> > > +
> > > SER_RS485_MODE_RS422;
> > > +			uart.port.rs485.flags = SER_RS485_ENABLED |
> > > +						SER_RS485_RTS_ON_SEND |
> > > +						SER_RS485_MODE_RS422;
> > > +		}
> > > +		if (kuart->capability & KUART_CAPABILITY_RS485) {
> > > +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> > > +
> > > SER_RS485_RTS_ON_SEND;
> > > +			uart.port.rs485.flags = SER_RS485_ENABLED |
> > > +						SER_RS485_RTS_ON_SEND;
> > > +		}
> > > +	}
> > 
> > Is it so that only one mode is supported or can that be changes using
> > kuart_rs485_config() in which case you should have all flags listed (you
> > seem to talk about priority so that sounds like all are supported)?
> 
> Both. As written in the commit message, there are 3 variants of the
> device. 2 variants support only one mode and 1 variant supports up to
> 3 modes.
> 
> > > +
> > > +	retval = serial8250_register_8250_port(&uart);
> > > +	if (retval < 0) {
> > > +		dev_err(&auxdev->dev, "UART registration failed!\n");
> > 
> > Missing header.
> 
> I will check for the header.
> 
> As this patch is already merged, I will do a follow up.
> 
> regards, gerhard
> 

-- 
 i.

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Gerhard Engleder]

On 26.11.25 22:33, Andy Shevchenko wrote:
> On Wed, Nov 26, 2025 at 10:06:17PM +0100, Gerhard Engleder wrote:
>> On 26.11.25 14:17, Andy Shevchenko wrote:
>>> On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:
> 
> ...
> 
>>>> +#include <linux/auxiliary_bus.h>
>>>
>>> + bits.h
>>> + container_of.h
>>>
>>>> +#include <linux/device.h>
>>>
>>> I don't see how it's being used.
>>> What I see are
>>>
>>> + dev_printk.h
>>> + device/devres.h
>>>
>>> + err.h
>>>
>>>> +#include <linux/io.h>
>>>> +#include <linux/misc/keba.h>
>>>
>>> + mod_devicetable.h
>>>
>>>> +#include <linux/module.h>
>>>
>>> + spinlock.h
>>> + types.h
>>
>> Is there any extra tool to check for missing headers? Or do I have to
>> check the header for every used function?
> 
> There is iwyu, which is heavily tweaked by Jonathan Cameron for use in Linux
> kernel and even though it gives some false positives.
> 
> I did this manually by reading the code of the driver and remembering which
> header provides which API.
> 
> ...
> 
>>> Can't you call uart_read_port_properties()?
>>>
>>> If ever you gain some properties either via FW description or via software
>>> nodes, they will be automatically used without need to update the driver!
>>
>> Yes that would be some nice behavior. But __uart_read_properties()
>> sets some defaults even if no firmware node exist (UPF_SHARE_IRQ,
> 
> Is it a problem? Even a single IRQ may be marked shared.
> 
>> 0 as irq number
> 
> It doesn't do that, it just skips setting it in that case.
> 
>> or it fails if not irq number is found).
> 
> Yeah, this is most "problematic" part in case of this driver. Why is it
> auxiliary and not platform? With that the __uart_read_properties() needs
> to be updated to also query IRQ from respective aux device, and aux dev
> needs implementation of dev_is_auxiliary(). Not that big deal, though.

It is auxiliary and not platform, because gregkh suggested to switch
from platform to auxiliary. IMO he is right and that is a better fit,
because auxiliary devices were introduced to split big devices into
sub devices, which results in smaller drivers with one job for the
sub device. That's actually what I tried to do with platform devices
first.

>> So __uart_read_properties() would need to be changed. IMO it makes no sense
>> to change __uart_read_properties() as this functionality is currently
>> not required.
> 
> Yes, but as I said it will give you for free the possibility to use those
> properties without future modifications of the driver. OTOH, driver is in
> tree and modifications will be needed one way or another :-)

Yes, if the KEBA UART changes, then modifications would be needed. We
kept this UART stable now for over 10 years. As it is FPGA based, we
can keep it stable as long as FPGAs are on the market. In industrial
automation the products are in the field for 10 years, 20 years or even
more. We can only support the devices for that long time by keeping
them compatible. So I don't expect changes now and therefore I would
not prepare for them.

Gerhard

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Gerhard Engleder]

On 27.11.25 10:28, Ilpo Järvinen wrote:
> On Wed, 26 Nov 2025, Gerhard Engleder wrote:
> 
>> On 26.11.25 16:46, Ilpo Järvinen wrote:
>>> On Thu, 23 Oct 2025, Gerhard Engleder wrote:
>>>
>>>> From: Gerhard Engleder <eg@keba.com>
>>>>
>>>> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
>>>> mostly 8250 compatible with extension for some UART modes.
>>>>
>>>> 3 different variants exist. The simpliest variant supports only RS-232
>>>> and is used for debug interfaces. The next variant supports only RS-485
>>>> and is used mostly for communication with KEBA panel devices. The third
>>>> variant is able to support RS-232, RS-485 and RS-422. For this variant
>>>> not only the mode of the UART is configured, also the physics and
>>>> transceivers are switched according to the mode.
>>>>
>>>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>>>> Tested-by: Daniel Gierlinger <gida@keba.com>
>>>> ---
>>>>    drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
>>>>    drivers/tty/serial/8250/Kconfig     |  13 ++
>>>>    drivers/tty/serial/8250/Makefile    |   1 +
>>>>    3 files changed, 294 insertions(+)
>>>>    create mode 100644 drivers/tty/serial/8250/8250_keba.c

...

>>>> +#include <linux/auxiliary_bus.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/misc/keba.h>
>>>> +#include <linux/module.h>
>>>
>>> + linux/serial_core.h
>>
>> Is this really necessary even with the include of 8250.h below?
> 
> Yes. Generally don't rely on indirect includes.

I will add it to the follow up.

>>>> +
>>>> +#include "8250.h"
>>>> +
>>>> +#define KUART "kuart"

...

>>>> +static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
>>>> +{
>>>> +	u8 acr;
>>>> +
>>>> +	/* set index register to 0 to access ACR register */
>>>> +	serial_out(up, UART_SCR, UART_ACR);
>>>
>>> So the scratch register has some special use on this UART (register
>>> multiplexer?), it would probably better name it with define, if that's the
>>> case.
>>
>> This UART supports an enhanced mode, which changes the behavior of some
>> registers. But the register still have their normal use with enhanced
>> mode disabled. So I would keep the register name.
> 
> But this code clearly assume UART is in enhanced mode. Same number can
> have different names. You could of course reuse the other define in the
> define:
> 
> #define KUART_EMODE_INDEX_REG	UART_SCR

I will consider it for the follow up.

Thank you for your comments!

Gerhard

Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 08:40:41PM +0100, Gerhard Engleder wrote:
> On 26.11.25 22:33, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 10:06:17PM +0100, Gerhard Engleder wrote:
> > > On 26.11.25 14:17, Andy Shevchenko wrote:
> > > > On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:

...

> > > > Can't you call uart_read_port_properties()?
> > > > 
> > > > If ever you gain some properties either via FW description or via software
> > > > nodes, they will be automatically used without need to update the driver!
> > > 
> > > Yes that would be some nice behavior. But __uart_read_properties()
> > > sets some defaults even if no firmware node exist (UPF_SHARE_IRQ,
> > 
> > Is it a problem? Even a single IRQ may be marked shared.
> > 
> > > 0 as irq number
> > 
> > It doesn't do that, it just skips setting it in that case.
> > 
> > > or it fails if not irq number is found).
> > 
> > Yeah, this is most "problematic" part in case of this driver. Why is it
> > auxiliary and not platform? With that the __uart_read_properties() needs
> > to be updated to also query IRQ from respective aux device, and aux dev
> > needs implementation of dev_is_auxiliary(). Not that big deal, though.
> 
> It is auxiliary and not platform, because gregkh suggested to switch
> from platform to auxiliary. IMO he is right and that is a better fit,
> because auxiliary devices were introduced to split big devices into
> sub devices, which results in smaller drivers with one job for the
> sub device. That's actually what I tried to do with platform devices
> first.
> 
> > > So __uart_read_properties() would need to be changed. IMO it makes no sense
> > > to change __uart_read_properties() as this functionality is currently
> > > not required.
> > 
> > Yes, but as I said it will give you for free the possibility to use those
> > properties without future modifications of the driver. OTOH, driver is in
> > tree and modifications will be needed one way or another :-)
> 
> Yes, if the KEBA UART changes, then modifications would be needed. We
> kept this UART stable now for over 10 years. As it is FPGA based, we
> can keep it stable as long as FPGAs are on the market. In industrial
> automation the products are in the field for 10 years, 20 years or even
> more. We can only support the devices for that long time by keeping
> them compatible. So I don't expect changes now and therefore I would
> not prepare for them.

Fair enough.

-- 
With Best Regards,
Andy Shevchenko