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

[PATCH v2 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.

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 | 281 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |  13 ++
 drivers/tty/serial/8250/Makefile    |   1 +
 drivers/tty/serial/serial_core.c    |   8 +-
 4 files changed, 302 insertions(+), 1 deletion(-)
 create mode 100644 drivers/tty/serial/8250/8250_keba.c

-- 
2.39.5

[PATCH v2 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. 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.

If no firmware node exist, 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, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4757293ece8c..366101f85048 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
 	u32 rs485_delay[2];
 	int ret;
 
-	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
+	/*
+	 * Retrieve properties only if rs485 is supported and if a firmware node
+	 * exist. If no firmware node exist, then don't touch rs485 config and
+	 * keep initial rs485 properties set by driver.
+	 */
+	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
+	    !dev_fwnode(dev))
 		return 0;
 
 	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
-- 
2.39.5

[PATCH v2 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 | 281 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |  13 ++
 drivers/tty/serial/8250/Makefile    |   1 +
 3 files changed, 295 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..b923556139ee
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_keba.c
@@ -0,0 +1,281 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025 KEBA Industrial Automation GmbH
+ *
+ * Driver for KEBA UART FPGA IP core
+ */
+
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/misc/keba.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 v2 1/2] serial: Keep rs485 settings for devices without firmware node

[Lukas Wunner]

On Fri, Oct 17, 2025 at 04:42:08PM +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. 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.
> 
> If no firmware node exist, 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.
[...]
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
>  	u32 rs485_delay[2];
>  	int ret;
>  
> -	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
> +	/*
> +	 * Retrieve properties only if rs485 is supported and if a firmware node
> +	 * exist. If no firmware node exist, then don't touch rs485 config and
> +	 * keep initial rs485 properties set by driver.
> +	 */
> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
> +	    !dev_fwnode(dev))
>  		return 0;
>  
>  	ret = device_property_read_u32_array(dev, "rs485-rts-delay",

Hm, this will also skip the call to uart_sanitize_serial_rs485_delays().

I'm wondering if a better approach might be to move the check for
!dev_fwnode(dev) further down, after the invocation of
uart_sanitize_serial_rs485_delays()?

It may be necessary then to change the else-branch for the delays to
"else if (ret != -EINVAL)" because -EINVAL is returned from
device_property_read_u32_array() if there's no fw_node.

If you decide to keep the check at the top of the function, then
style-wise it would seem cleaner to not insert it into the existing
if-condition, but add a separate if-condition.  It doesn't matter
IMO that they both return 0.  The way the patch is now, it creates
a little confusion to which of the two if-conditions the code
comment pertains.

Thanks,

Lukas

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

[Gerhard Engleder]

On 19.10.25 10:50, Lukas Wunner wrote:
> On Fri, Oct 17, 2025 at 04:42:08PM +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. 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.
>>
>> If no firmware node exist, 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.
> [...]
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
>>   	u32 rs485_delay[2];
>>   	int ret;
>>   
>> -	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
>> +	/*
>> +	 * Retrieve properties only if rs485 is supported and if a firmware node
>> +	 * exist. If no firmware node exist, then don't touch rs485 config and
>> +	 * keep initial rs485 properties set by driver.
>> +	 */
>> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
>> +	    !dev_fwnode(dev))
>>   		return 0;
>>   
>>   	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
> 
> Hm, this will also skip the call to uart_sanitize_serial_rs485_delays().
> 
> I'm wondering if a better approach might be to move the check for
> !dev_fwnode(dev) further down, after the invocation of
> uart_sanitize_serial_rs485_delays()?

Calling uart_sanitize_serial_rs485_delays() would make sense as it helps
to prevent driver bugs. But on the other hand this check is done within
a function, which implements a device tree binding. So it is confusing
that device tree binding code issue warnings for none device tree
configurations.

> It may be necessary then to change the else-branch for the delays to
> "else if (ret != -EINVAL)" because -EINVAL is returned from
> device_property_read_u32_array() if there's no fw_node.

Yes, that could be done. But is relying on return values future proof?
EINVAL for a missing fw_node is not a good fit in my opinion. So this
may break in the future silently?

> If you decide to keep the check at the top of the function, then
> style-wise it would seem cleaner to not insert it into the existing
> if-condition, but add a separate if-condition.  It doesn't matter
> IMO that they both return 0.  The way the patch is now, it creates
> a little confusion to which of the two if-conditions the code
> comment pertains.

I will add a separate if-condition for better readability.

Thank you for your review!

Gerhard

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

[Lukas Wunner]

On Sun, Oct 19, 2025 at 04:10:26PM +0200, Gerhard Engleder wrote:
> On 19.10.25 10:50, Lukas Wunner wrote:
> > On Fri, Oct 17, 2025 at 04:42:08PM +0200, Gerhard Engleder wrote:
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
> > >   	u32 rs485_delay[2];
> > >   	int ret;
> > > -	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
> > > +	/*
> > > +	 * Retrieve properties only if rs485 is supported and if a firmware node
> > > +	 * exist. If no firmware node exist, then don't touch rs485 config and
> > > +	 * keep initial rs485 properties set by driver.
> > > +	 */
> > > +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
> > > +	    !dev_fwnode(dev))
> > >   		return 0;
> > >   	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
> > 
> > Hm, this will also skip the call to uart_sanitize_serial_rs485_delays().
> > 
> > I'm wondering if a better approach might be to move the check for
> > !dev_fwnode(dev) further down, after the invocation of
> > uart_sanitize_serial_rs485_delays()?
> 
> Calling uart_sanitize_serial_rs485_delays() would make sense as it helps
> to prevent driver bugs. But on the other hand this check is done within
> a function, which implements a device tree binding. So it is confusing
> that device tree binding code issue warnings for none device tree
> configurations.

This isn't just for devicetree.  E.g. ACPI systems may have these
properties specified in a _DSD object.  It's meant to be a generic
function to retrieve and sanitize rs485 settings.

BTW is there a good reason that you don't have a fwnode for your UART?
It seems odd to have a UART but not describe it in the devicetree.
Maybe that's the real problem and fixing it obviates the need for this
patch?

> > It may be necessary then to change the else-branch for the delays to
> > "else if (ret != -EINVAL)" because -EINVAL is returned from
> > device_property_read_u32_array() if there's no fw_node.
> 
> Yes, that could be done. But is relying on return values future proof?

In general, yes.

> EINVAL for a missing fw_node is not a good fit in my opinion. So this
> may break in the future silently?

I agree it's a poor choice and something like -ENODEV may have been
more appropriate.

Thanks,

Lukas

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

[Gerhard Engleder]

On 19.10.25 16:42, Lukas Wunner wrote:
> On Sun, Oct 19, 2025 at 04:10:26PM +0200, Gerhard Engleder wrote:
>> On 19.10.25 10:50, Lukas Wunner wrote:
>>> On Fri, Oct 17, 2025 at 04:42:08PM +0200, Gerhard Engleder wrote:
>>>> +++ b/drivers/tty/serial/serial_core.c
>>>> @@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
>>>>    	u32 rs485_delay[2];
>>>>    	int ret;
>>>> -	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
>>>> +	/*
>>>> +	 * Retrieve properties only if rs485 is supported and if a firmware node
>>>> +	 * exist. If no firmware node exist, then don't touch rs485 config and
>>>> +	 * keep initial rs485 properties set by driver.
>>>> +	 */
>>>> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
>>>> +	    !dev_fwnode(dev))
>>>>    		return 0;
>>>>    	ret = device_property_read_u32_array(dev, "rs485-rts-delay",

...

> 
> BTW is there a good reason that you don't have a fwnode for your UART?
> It seems odd to have a UART but not describe it in the devicetree.
> Maybe that's the real problem and fixing it obviates the need for this
> patch?

This auxiliary device is part of a FPGA based PCIe device. It is mostly
used on x86 but arm64 is also possible in the future. There is no device
tree or ACPI information available for this device. Think about an x86
CPU module where you cannot influence the BIOS implementation and device
tree is not available. IMO having a self describing PCIe device which
works out of the box is best in this case.

Thanks for your comments!

Gerhard

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

[Lukas Wunner]

On Sun, Oct 19, 2025 at 05:21:30PM +0200, Gerhard Engleder wrote:
> On 19.10.25 16:42, Lukas Wunner wrote:
> > BTW is there a good reason that you don't have a fwnode for your UART?
> > It seems odd to have a UART but not describe it in the devicetree.
> > Maybe that's the real problem and fixing it obviates the need for this
> > patch?
> 
> This auxiliary device is part of a FPGA based PCIe device. It is mostly
> used on x86 but arm64 is also possible in the future. There is no device
> tree or ACPI information available for this device. Think about an x86
> CPU module where you cannot influence the BIOS implementation and device
> tree is not available. IMO having a self describing PCIe device which
> works out of the box is best in this case.

It would be good to have that information included in the commit message
to avoid head-scratching by reviewers and leave breadcrumbs for people
who happen upon the commit through git blame etc.

Thanks,

Lukas

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

[Lukas Wunner]

On Sun, Oct 19, 2025 at 05:21:30PM +0200, Gerhard Engleder wrote:
> On 19.10.25 16:42, Lukas Wunner wrote:
> > BTW is there a good reason that you don't have a fwnode for your UART?
> > It seems odd to have a UART but not describe it in the devicetree.
> > Maybe that's the real problem and fixing it obviates the need for this
> > patch?
> 
> This auxiliary device is part of a FPGA based PCIe device. It is mostly
> used on x86 but arm64 is also possible in the future. There is no device
> tree or ACPI information available for this device. Think about an x86
> CPU module where you cannot influence the BIOS implementation and device
> tree is not available. IMO having a self describing PCIe device which
> works out of the box is best in this case.

In case you're not aware of it, it's possible to assign a software
fwnode to devices through device_add_software_node().  There is
precedent for its usage among 8250 drivers, see 8250_bcm2835aux.c
and 8250_exar.c.

So that would be an alternative to this patch.  Conceivably, your
FPGA might support different UART types and each might default to
different rs485 settings.  A software node as used by 8250_bcm2835aux.c
would allow you to define those settings through the driver_data.

Thanks,

Lukas

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

[Gerhard Engleder]

On 19.10.25 19:02, Lukas Wunner wrote:
> On Sun, Oct 19, 2025 at 05:21:30PM +0200, Gerhard Engleder wrote:
>> On 19.10.25 16:42, Lukas Wunner wrote:
>>> BTW is there a good reason that you don't have a fwnode for your UART?
>>> It seems odd to have a UART but not describe it in the devicetree.
>>> Maybe that's the real problem and fixing it obviates the need for this
>>> patch?
>>
>> This auxiliary device is part of a FPGA based PCIe device. It is mostly
>> used on x86 but arm64 is also possible in the future. There is no device
>> tree or ACPI information available for this device. Think about an x86
>> CPU module where you cannot influence the BIOS implementation and device
>> tree is not available. IMO having a self describing PCIe device which
>> works out of the box is best in this case.
> 
> It would be good to have that information included in the commit message
> to avoid head-scratching by reviewers and leave breadcrumbs for people
> who happen upon the commit through git blame etc.

I will enhance the commit message.

Thank you!

Gerhard

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

[Gerhard Engleder]

On 19.10.25 19:20, Lukas Wunner wrote:
> On Sun, Oct 19, 2025 at 05:21:30PM +0200, Gerhard Engleder wrote:
>> On 19.10.25 16:42, Lukas Wunner wrote:
>>> BTW is there a good reason that you don't have a fwnode for your UART?
>>> It seems odd to have a UART but not describe it in the devicetree.
>>> Maybe that's the real problem and fixing it obviates the need for this
>>> patch?
>>
>> This auxiliary device is part of a FPGA based PCIe device. It is mostly
>> used on x86 but arm64 is also possible in the future. There is no device
>> tree or ACPI information available for this device. Think about an x86
>> CPU module where you cannot influence the BIOS implementation and device
>> tree is not available. IMO having a self describing PCIe device which
>> works out of the box is best in this case.
> 
> In case you're not aware of it, it's possible to assign a software
> fwnode to devices through device_add_software_node().  There is
> precedent for its usage among 8250 drivers, see 8250_bcm2835aux.c
> and 8250_exar.c.
> 
> So that would be an alternative to this patch.  Conceivably, your
> FPGA might support different UART types and each might default to
> different rs485 settings.  A software node as used by 8250_bcm2835aux.c
> would allow you to define those settings through the driver_data.

I was not aware of device_add_software_node(). Thank you for the hint!

IMO keeping driver defaults if no fwnode exist is more straight forward
than adding a software_node. So I would keep this patch.

Gerhard

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

[Andy Shevchenko]

On Fri, Oct 17, 2025 at 04:42:07PM +0200, Gerhard Engleder wrote:
> 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.

I just realised (thanks, Lukas!) that this is for some kind of FPGA which makes
even bigger question here.

First of all, we have the FPGA framework, which handles (re-)configurations of
FPGAs. Second, why do you need a new driver when we have already one, i.e.
8250_dfl that follows some kind of standards?

Third, the expect approach is to see DT overlay provided along with the FPGA
configuration. So, basically your cp500.c should not have been existed to begin
with.

Can you elaborate on these considerations?

-- 
With Best Regards,
Andy Shevchenko

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

[Gerhard Engleder]

On 27.11.25 17:05, Andy Shevchenko wrote:
> On Fri, Oct 17, 2025 at 04:42:07PM +0200, Gerhard Engleder wrote:
>> 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.
> 
> I just realised (thanks, Lukas!) that this is for some kind of FPGA which makes
> even bigger question here.
> 
> First of all, we have the FPGA framework, which handles (re-)configurations of
> FPGAs. Second, why do you need a new driver when we have already one, i.e.
> 8250_dfl that follows some kind of standards?

Yes, there is FPGA framework, which handles FPGA re-configuration.
There is no FPGA re-configuration in the driver and neither in the
system. The FPGA does not even support re-configuration. The FPGA is
just a variant to implement a PCIe target. The FPGA loads its
configuration on power up once from flash before the BIOS starts. So
the operating system does not need to know that it is an FPGA. It is
just a PCIe target.

Why not 8250_dfl? Because this is a driver for a different IP core.
The UART is no Altera/Intel or AMD/Xilinx IP core. It is a KEBA
IP core and the 8250_keba driver only implements feature added by
KEBA. The rest is 8250 and uses the 8250 infrastructure.

> Third, the expect approach is to see DT overlay provided along with the FPGA
> configuration. So, basically your cp500.c should not have been existed to begin
> with.

Like first, it is just a PCIe target and this PCIe target is divided
into auxiliary devices, which was suggested by gregkh. Initially I
divided it into platform devices, but the suggestion of gregkh is a
better fit. It is a single PCIe target, which consists of multiple
logical devices with its own registers and interrupts. This logical
devices are then separated by auxiliary devices.

3 years ago Intel was not able to deliver enough FPGAs. With just being
a PCIe target, it was possible to switch the FPGA vendor without
touching the software in the field. With Linux re-configuring the FPGA
that would not have been possible. So re-configuring FPGAs during
runtime is something that should be only used if needed.

> Can you elaborate on these considerations?

I hope you get a better impression what this is about. For the drivers
it is not relevant that the device is FPGA based.

Gerhard

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

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 08:26:13PM +0100, Gerhard Engleder wrote:
> On 27.11.25 17:05, Andy Shevchenko wrote:
> > On Fri, Oct 17, 2025 at 04:42:07PM +0200, Gerhard Engleder wrote:
> > > 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.
> > 
> > I just realised (thanks, Lukas!) that this is for some kind of FPGA which makes
> > even bigger question here.
> > 
> > First of all, we have the FPGA framework, which handles (re-)configurations of
> > FPGAs. Second, why do you need a new driver when we have already one, i.e.
> > 8250_dfl that follows some kind of standards?
> 
> Yes, there is FPGA framework, which handles FPGA re-configuration.
> There is no FPGA re-configuration in the driver and neither in the
> system. The FPGA does not even support re-configuration. The FPGA is
> just a variant to implement a PCIe target. The FPGA loads its
> configuration on power up once from flash before the BIOS starts. So
> the operating system does not need to know that it is an FPGA. It is
> just a PCIe target.
> 
> Why not 8250_dfl? Because this is a driver for a different IP core.
> The UART is no Altera/Intel or AMD/Xilinx IP core. It is a KEBA
> IP core and the 8250_keba driver only implements feature added by
> KEBA. The rest is 8250 and uses the 8250 infrastructure.
> 
> > Third, the expect approach is to see DT overlay provided along with the FPGA
> > configuration. So, basically your cp500.c should not have been existed to begin
> > with.
> 
> Like first, it is just a PCIe target and this PCIe target is divided
> into auxiliary devices, which was suggested by gregkh. Initially I
> divided it into platform devices, but the suggestion of gregkh is a
> better fit. It is a single PCIe target, which consists of multiple
> logical devices with its own registers and interrupts. This logical
> devices are then separated by auxiliary devices.

This is implementation detail in Linux, but in HW why are those not a proper
PCI endpoints / functions? With that done, the drivers can be just normal
PCI device drivers.

> 3 years ago Intel was not able to deliver enough FPGAs. With just being
> a PCIe target, it was possible to switch the FPGA vendor without
> touching the software in the field. With Linux re-configuring the FPGA
> that would not have been possible. So re-configuring FPGAs during
> runtime is something that should be only used if needed.
> 
> > Can you elaborate on these considerations?
> 
> I hope you get a better impression what this is about. For the drivers
> it is not relevant that the device is FPGA based.

Yes, so it's rather something with a custom FW that is not supposed to be
changed (at least from the device hierarchy / functionality perspective),
or very little from version to version. Is this a correct summary?

-- 
With Best Regards,
Andy Shevchenko

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

[Gerhard Engleder]

On 27.11.25 20:56, Andy Shevchenko wrote:
> On Thu, Nov 27, 2025 at 08:26:13PM +0100, Gerhard Engleder wrote:
>> On 27.11.25 17:05, Andy Shevchenko wrote:
>>> On Fri, Oct 17, 2025 at 04:42:07PM +0200, Gerhard Engleder wrote:
>>>> 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.
>>>
>>> I just realised (thanks, Lukas!) that this is for some kind of FPGA which makes
>>> even bigger question here.
>>>
>>> First of all, we have the FPGA framework, which handles (re-)configurations of
>>> FPGAs. Second, why do you need a new driver when we have already one, i.e.
>>> 8250_dfl that follows some kind of standards?
>>
>> Yes, there is FPGA framework, which handles FPGA re-configuration.
>> There is no FPGA re-configuration in the driver and neither in the
>> system. The FPGA does not even support re-configuration. The FPGA is
>> just a variant to implement a PCIe target. The FPGA loads its
>> configuration on power up once from flash before the BIOS starts. So
>> the operating system does not need to know that it is an FPGA. It is
>> just a PCIe target.
>>
>> Why not 8250_dfl? Because this is a driver for a different IP core.
>> The UART is no Altera/Intel or AMD/Xilinx IP core. It is a KEBA
>> IP core and the 8250_keba driver only implements feature added by
>> KEBA. The rest is 8250 and uses the 8250 infrastructure.
>>
>>> Third, the expect approach is to see DT overlay provided along with the FPGA
>>> configuration. So, basically your cp500.c should not have been existed to begin
>>> with.
>>
>> Like first, it is just a PCIe target and this PCIe target is divided
>> into auxiliary devices, which was suggested by gregkh. Initially I
>> divided it into platform devices, but the suggestion of gregkh is a
>> better fit. It is a single PCIe target, which consists of multiple
>> logical devices with its own registers and interrupts. This logical
>> devices are then separated by auxiliary devices.
> 
> This is implementation detail in Linux, but in HW why are those not a proper
> PCI endpoints / functions? With that done, the drivers can be just normal
> PCI device drivers.

Yes, PCI subfunctions would have been much nicer, with direct drivers
and no auxiliary/platform stuff in between. But these were not
supported by the FPGA vendor.

>> 3 years ago Intel was not able to deliver enough FPGAs. With just being
>> a PCIe target, it was possible to switch the FPGA vendor without
>> touching the software in the field. With Linux re-configuring the FPGA
>> that would not have been possible. So re-configuring FPGAs during
>> runtime is something that should be only used if needed.
>>
>>> Can you elaborate on these considerations?
>>
>> I hope you get a better impression what this is about. For the drivers
>> it is not relevant that the device is FPGA based.
> 
> Yes, so it's rather something with a custom FW that is not supposed to be
> changed (at least from the device hierarchy / functionality perspective),
> or very little from version to version. Is this a correct summary?

Yes, the only changes are bugfixes. The register interface must be kept
compatible. Like a microcontroller with a firmware which implements an
USB device.

Gerhard

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

[Andy Shevchenko]

On Thu, Nov 27, 2025 at 09:07:10PM +0100, Gerhard Engleder wrote:
> On 27.11.25 20:56, Andy Shevchenko wrote:
> > On Thu, Nov 27, 2025 at 08:26:13PM +0100, Gerhard Engleder wrote:
> > > On 27.11.25 17:05, Andy Shevchenko wrote:
> > > > On Fri, Oct 17, 2025 at 04:42:07PM +0200, Gerhard Engleder wrote:

...

> > > > Third, the expect approach is to see DT overlay provided along with the FPGA
> > > > configuration. So, basically your cp500.c should not have been existed to begin
> > > > with.
> > > 
> > > Like first, it is just a PCIe target and this PCIe target is divided
> > > into auxiliary devices, which was suggested by gregkh. Initially I
> > > divided it into platform devices, but the suggestion of gregkh is a
> > > better fit. It is a single PCIe target, which consists of multiple
> > > logical devices with its own registers and interrupts. This logical
> > > devices are then separated by auxiliary devices.
> > 
> > This is implementation detail in Linux, but in HW why are those not a proper
> > PCI endpoints / functions? With that done, the drivers can be just normal
> > PCI device drivers.
> 
> Yes, PCI subfunctions would have been much nicer, with direct drivers
> and no auxiliary/platform stuff in between. But these were not
> supported by the FPGA vendor.

Unfortunately...

> > > 3 years ago Intel was not able to deliver enough FPGAs. With just being
> > > a PCIe target, it was possible to switch the FPGA vendor without
> > > touching the software in the field. With Linux re-configuring the FPGA
> > > that would not have been possible. So re-configuring FPGAs during
> > > runtime is something that should be only used if needed.
> > > 
> > > > Can you elaborate on these considerations?
> > > 
> > > I hope you get a better impression what this is about. For the drivers
> > > it is not relevant that the device is FPGA based.
> > 
> > Yes, so it's rather something with a custom FW that is not supposed to be
> > changed (at least from the device hierarchy / functionality perspective),
> > or very little from version to version. Is this a correct summary?
> 
> Yes, the only changes are bugfixes. The register interface must be kept
> compatible. Like a microcontroller with a firmware which implements an
> USB device.

Got it, thanks for the elaboration on this.

-- 
With Best Regards,
Andy Shevchenko