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

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

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