Re: [PATCH V6] serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers

[Greg Kroah-Hartman]

On Thu, Apr 17, 2014 at 03:06:44PM +0900, Yoshihiro YUNOMAE wrote:
> Add tunable RX interrupt trigger I/F of FIFO buffers.
> Serial devices are used as not only message communication devices but control
> or sending communication devices. For the latter uses, normally small data
> will be exchanged, so user applications want to receive data unit as soon as
> possible for real-time tendency. If we have a sensor which sends a 1 byte data
> each time and must control a device based on the sensor feedback, the RX
> interrupt should be triggered for each data.
> 
> According to HW specification of serial UART devices, RX interrupt trigger
> can be changed, but the trigger is hard-coded. For example, RX interrupt trigger
> in 16550A can be set to 1, 4, 8, or 14 bytes for HW, but current driver sets
> the trigger to only 8bytes.
> 
> This patch makes some devices change RX interrupt trigger from userland.
> 
> <How to use>
> - Read current setting
>  # cat /sys/dev/char/4\:64/rx_int_trig
>  8
> 
> - Write user setting
>  # echo 1 > /sys/dev/char/4\:64/rx_int_trig
>  # cat /sys/dev/char/4\:64/rx_int_trig
>  1
> 
> <Support uart devices>
> - 16550A and Tegra (1, 4, 8, or 14 bytes)
> - 16650V2 (8, 16, 24, or 28 bytes)
> - 16654 (8, 16, 56, or 60 bytes)
> - 16750 (1, 16, 32, or 56 bytes)
> 
> Changed in V2:
>  - Use _IOW for TIOCSFIFORTRIG definition
>  - Pass the interrupt trigger value itself
> 
> Changes in V3:
>  - Change I/F from ioctl(2) to sysfs(rx_int_trig)
> 
> Changes in V4:
>  - Introduce fifo_bug flag in uart_8250_port structure
>    This is enabled only when parity is enabled and UART_BUG_PARITY is enabled
>    for up->bugs. If this flag is enabled, user cannot set RX trigger.
>  - Return -EOPNOTSUPP when it does not support device at convert_fcr2val() and
>    at convert_val2rxtrig()
>  - Set the nearest lower RX trigger when users input a meaningless value at
>    convert_val2rxtrig()
>  - Check whether p->fcr is existing at serial8250_clear_and_reinit_fifos()
>  - Set fcr = up->fcr in the begging of serial8250_do_set_termios()
> 
> Changes in V5:
>  - Support Tegra, 16650V2, 16654, and 16750
>  - Store default FCR value to up->fcr when the port is first created
>  - Add rx_trig_byte[] in uart_config[] for each device and use rx_trig_byte[]
>    in convert_fcr2val() and convert_val2rxtrig()
> 
> Changes in V5.1:
>  - Fix FCR_RX_TRIG_MAX_STATE definition
> 
> Changes in V6:
>  - Move FCR_RX_TRIG_* definition in 8250.h to include/uapi/linux/serial_reg.h,
>    rename those to UART_FCR_R_TRIG_*, and use UART_FCR_TRIGGER_MASK to
>    UART_FCR_R_TRIG_BITS()
>  - Change following function names:
>     convert_fcr2val() => fcr_get_rxtrig_bytes()
>     convert_val2rxtrig() => bytes_to_fcr_rxtrig()
>  - Fix typo in serial8250_do_set_termios()
>  - Delete the verbose error message pr_info() in bytes_to_fcr_rxtrig()
>  - Rename *rx_int_trig/rx_trig* to *rxtrig* for several functions or variables
>    (but UI remains rx_int_trig)
>  - Change the meaningless variable name 'val' to 'bytes' following functions:
>     fcr_get_rxtrig_bytes(), bytes_to_fcr_rxtrig(), do_set_rxtrig(),
>     do_serial8250_set_rxtrig(), and serial8250_set_attr_rxtrig()
>  - Use up->fcr in order to get rxtrig_bytes instead of rx_trig_raw in
>    fcr_get_rxtrig_bytes()
>  - Use conf_type->rxtrig_bytes[0] instead of switch statement for support check
>    in register_dev_spec_attr_grp()
>  - Delete the checking whether a user changed FCR or not when minimum buffer
>    is needed in serial8250_do_set_termios()
> 
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@xxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Jiri Slaby <jslaby@xxxxxxx>
> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Cc: Jingoo Han <jg1.han@xxxxxxxxxxx>
> Cc: Aaron Sierra <asierra@xxxxxxxxxxx>
> Reviewed-by: Stephen Warren <swarren@xxxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250.h      |    2 
>  drivers/tty/serial/8250/8250_core.c |  173 ++++++++++++++++++++++++++++++++---
>  drivers/tty/serial/serial_core.c    |   18 ++--
>  include/linux/serial_8250.h         |    2 
>  include/linux/serial_core.h         |    4 +
>  include/uapi/linux/serial_reg.h     |    5 +
>  6 files changed, 182 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 1ebf853..1b08c91 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -12,6 +12,7 @@
>   */
>  
>  #include <linux/serial_8250.h>
> +#include <linux/serial_reg.h>
>  #include <linux/dmaengine.h>
>  
>  struct uart_8250_dma {
> @@ -60,6 +61,7 @@ struct serial8250_config {
>  	unsigned short	fifo_size;
>  	unsigned short	tx_loadsz;
>  	unsigned char	fcr;
> +	unsigned char	rxtrig_bytes[UART_FCR_R_TRIG_MAX_STATE];
>  	unsigned int	flags;
>  };
>  
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 81f909c..fd1b3ec 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -31,7 +31,6 @@
>  #include <linux/tty.h>
>  #include <linux/ratelimit.h>
>  #include <linux/tty_flip.h>
> -#include <linux/serial_reg.h>
>  #include <linux/serial_core.h>
>  #include <linux/serial.h>
>  #include <linux/serial_8250.h>
> @@ -161,6 +160,7 @@ static const struct serial8250_config uart_config[] = {
>  		.fifo_size	= 16,
>  		.tx_loadsz	= 16,
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
> +		.rxtrig_bytes	= {1, 4, 8, 14},
>  		.flags		= UART_CAP_FIFO,
>  	},
>  	[PORT_CIRRUS] = {
> @@ -180,6 +180,7 @@ static const struct serial8250_config uart_config[] = {
>  		.tx_loadsz	= 16,
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 |
>  				  UART_FCR_T_TRIG_00,
> +		.rxtrig_bytes	= {8, 16, 24, 28},
>  		.flags		= UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
>  	},
>  	[PORT_16750] = {
> @@ -188,6 +189,7 @@ static const struct serial8250_config uart_config[] = {
>  		.tx_loadsz	= 64,
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
>  				  UART_FCR7_64BYTE,
> +		.rxtrig_bytes	= {1, 16, 32, 56},
>  		.flags		= UART_CAP_FIFO | UART_CAP_SLEEP | UART_CAP_AFE,
>  	},
>  	[PORT_STARTECH] = {
> @@ -209,6 +211,7 @@ static const struct serial8250_config uart_config[] = {
>  		.tx_loadsz	= 32,
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 |
>  				  UART_FCR_T_TRIG_10,
> +		.rxtrig_bytes	= {8, 16, 56, 60},
>  		.flags		= UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
>  	},
>  	[PORT_16850] = {
> @@ -266,6 +269,7 @@ static const struct serial8250_config uart_config[] = {
>  		.tx_loadsz	= 8,
>  		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_01 |
>  				  UART_FCR_T_TRIG_01,
> +		.rxtrig_bytes	= {1, 4, 8, 14},
>  		.flags		= UART_CAP_FIFO | UART_CAP_RTOIE,
>  	},
>  	[PORT_XR17D15X] = {
> @@ -531,11 +535,8 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>  
>  void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
>  {
> -	unsigned char fcr;
> -
>  	serial8250_clear_fifos(p);
> -	fcr = uart_config[p->port.type].fcr;
> -	serial_out(p, UART_FCR, fcr);
> +	serial_out(p, UART_FCR, p->fcr);
>  }
>  EXPORT_SYMBOL_GPL(serial8250_clear_and_reinit_fifos);
>  
> @@ -2275,10 +2276,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  {
>  	struct uart_8250_port *up =
>  		container_of(port, struct uart_8250_port, port);
> -	unsigned char cval, fcr = 0;
> +	unsigned char cval;
>  	unsigned long flags;
>  	unsigned int baud, quot;
> -	int fifo_bug = 0;
>  
>  	switch (termios->c_cflag & CSIZE) {
>  	case CS5:
> @@ -2301,7 +2301,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  	if (termios->c_cflag & PARENB) {
>  		cval |= UART_LCR_PARITY;
>  		if (up->bugs & UART_BUG_PARITY)
> -			fifo_bug = 1;
> +			up->fifo_bug = true;
>  	}
>  	if (!(termios->c_cflag & PARODD))
>  		cval |= UART_LCR_EPAR;
> @@ -2325,10 +2325,10 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  		quot++;
>  
>  	if (up->capabilities & UART_CAP_FIFO && port->fifosize > 1) {
> -		fcr = uart_config[port->type].fcr;
> -		if ((baud < 2400 && !up->dma) || fifo_bug) {
> -			fcr &= ~UART_FCR_TRIGGER_MASK;
> -			fcr |= UART_FCR_TRIGGER_1;
> +		/* NOTE: If fifo_bug is not set, a user can set RX_trigger. */
> +		if ((baud < 2400 && !up->dma) || up->fifo_bug) {
> +			up->fcr &= ~UART_FCR_TRIGGER_MASK;
> +			up->fcr |= UART_FCR_TRIGGER_1;
>  		}
>  	}
>  
> @@ -2459,15 +2459,15 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  	 * is written without DLAB set, this mode will be disabled.
>  	 */
>  	if (port->type == PORT_16750)
> -		serial_port_out(port, UART_FCR, fcr);
> +		serial_port_out(port, UART_FCR, up->fcr);
>  
>  	serial_port_out(port, UART_LCR, cval);		/* reset DLAB */
>  	up->lcr = cval;					/* Save LCR */
>  	if (port->type != PORT_16750) {
>  		/* emulated UARTs (Lucent Venus 167x) need two steps */
> -		if (fcr & UART_FCR_ENABLE_FIFO)
> +		if (up->fcr & UART_FCR_ENABLE_FIFO)
>  			serial_port_out(port, UART_FCR, UART_FCR_ENABLE_FIFO);
> -		serial_port_out(port, UART_FCR, fcr);		/* set fcr */
> +		serial_port_out(port, UART_FCR, up->fcr);	/* set fcr */
>  	}
>  	serial8250_set_mctrl(port, port->mctrl);
>  	spin_unlock_irqrestore(&port->lock, flags);
> @@ -2660,6 +2660,146 @@ static int serial8250_request_port(struct uart_port *port)
>  	return ret;
>  }
>  
> +static int fcr_get_rxtrig_bytes(struct uart_8250_port *up)
> +{
> +	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> +	unsigned char bytes;
> +
> +	bytes = conf_type->rxtrig_bytes[UART_FCR_R_TRIG_BITS(up->fcr)];
> +
> +	return bytes ? bytes : -EOPNOTSUPP;
> +}
> +
> +static int bytes_to_fcr_rxtrig(struct uart_8250_port *up, unsigned char bytes)
> +{
> +	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> +	int i;
> +
> +	if (!conf_type->rxtrig_bytes[UART_FCR_R_TRIG_BITS(UART_FCR_R_TRIG_00)])
> +		return -EOPNOTSUPP;
> +
> +	for (i = 1; i < UART_FCR_R_TRIG_MAX_STATE; i++) {
> +		if (bytes < conf_type->rxtrig_bytes[i])
> +			/* Use the nearest lower value */
> +			return (--i) << UART_FCR_R_TRIG_SHIFT;
> +	}
> +
> +	return UART_FCR_R_TRIG_11;
> +}
> +
> +static int do_get_rxtrig(struct tty_port *port)
> +{
> +	struct uart_state *state = container_of(port, struct uart_state, port);
> +	struct uart_port *uport = state->uart_port;
> +	struct uart_8250_port *up =
> +		container_of(uport, struct uart_8250_port, port);
> +
> +	if (!(up->capabilities & UART_CAP_FIFO) || uport->fifosize <= 1)
> +		return -EINVAL;
> +
> +	return fcr_get_rxtrig_bytes(up);
> +}
> +
> +static int do_serial8250_get_rxtrig(struct tty_port *port)
> +{
> +	int rxtrig_bytes;
> +
> +	mutex_lock(&port->mutex);
> +	rxtrig_bytes = do_get_rxtrig(port);
> +	mutex_unlock(&port->mutex);
> +
> +	return rxtrig_bytes;
> +}
> +
> +static ssize_t serial8250_get_attr_rx_int_trig(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct tty_port *port = dev_get_drvdata(dev);
> +	int rxtrig_bytes;
> +
> +	rxtrig_bytes = do_serial8250_get_rxtrig(port);
> +	if (rxtrig_bytes < 0)
> +		return rxtrig_bytes;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", rxtrig_bytes);
> +}
> +
> +static int do_set_rxtrig(struct tty_port *port, unsigned char bytes)
> +{
> +	struct uart_state *state = container_of(port, struct uart_state, port);
> +	struct uart_port *uport = state->uart_port;
> +	struct uart_8250_port *up =
> +		container_of(uport, struct uart_8250_port, port);
> +	int rxtrig;
> +
> +	if (!(up->capabilities & UART_CAP_FIFO) || uport->fifosize <= 1 ||
> +	    up->fifo_bug)
> +		return -EINVAL;
> +
> +	rxtrig = bytes_to_fcr_rxtrig(up, bytes);
> +	if (rxtrig < 0)
> +		return rxtrig;
> +
> +	serial8250_clear_fifos(up);
> +	up->fcr &= ~UART_FCR_TRIGGER_MASK;
> +	up->fcr |= (unsigned char)rxtrig;
> +	serial_out(up, UART_FCR, up->fcr);
> +	return 0;
> +}
> +
> +static int do_serial8250_set_rxtrig(struct tty_port *port, unsigned char bytes)
> +{
> +	int ret;
> +
> +	mutex_lock(&port->mutex);
> +	ret = do_set_rxtrig(port, bytes);
> +	mutex_unlock(&port->mutex);
> +
> +	return ret;
> +}
> +
> +static ssize_t serial8250_set_attr_rx_int_trig(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct tty_port *port = dev_get_drvdata(dev);
> +	unsigned char bytes;
> +	int ret;
> +
> +	if (!count)
> +		return -EINVAL;
> +
> +	ret = kstrtou8(buf, 10, &bytes);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = do_serial8250_set_rxtrig(port, bytes);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(rx_int_trig, S_IRUSR | S_IWUSR | S_IRGRP,
> +		   serial8250_get_attr_rx_int_trig,
> +		   serial8250_set_attr_rx_int_trig);
> +

As you are adding a new sysfs attribute, you have to add a
Documentation/ABI/ entry as well.

> +static struct attribute *serial8250_dev_attrs[] = {
> +	&dev_attr_rx_int_trig.attr,
> +	NULL,
> +	};
> +
> +static struct attribute_group serial8250_dev_attr_group = {
> +	.attrs = serial8250_dev_attrs,
> +	};

What's wrong with the macro to create a group?

> +
> +static void register_dev_spec_attr_grp(struct uart_8250_port *up)
> +{
> +	const struct serial8250_config *conf_type = &uart_config[up->port.type];
> +
> +	if (conf_type->rxtrig_bytes[0])
> +		up->port.dev_spec_attr_group = &serial8250_dev_attr_group;
> +}
> +
>  static void serial8250_config_port(struct uart_port *port, int flags)
>  {
>  	struct uart_8250_port *up =
> @@ -2708,6 +2848,9 @@ static void serial8250_config_port(struct uart_port *port, int flags)
>  	if ((port->type == PORT_XR17V35X) ||
>  	   (port->type == PORT_XR17D15X))
>  		port->handle_irq = exar_handle_irq;
> +
> +	register_dev_spec_attr_grp(up);
> +	up->fcr = uart_config[up->port.type].fcr;
>  }
>  
>  static int
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 2cf5649..41ac44b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2548,15 +2548,16 @@ static struct attribute *tty_dev_attrs[] = {
>  	NULL,
>  	};
>  
> -static const struct attribute_group tty_dev_attr_group = {
> +static struct attribute_group tty_dev_attr_group = {
>  	.attrs = tty_dev_attrs,
>  	};
>  
> -static const struct attribute_group *tty_dev_attr_groups[] = {
> -	&tty_dev_attr_group,
> -	NULL
> -	};
> -
> +static void make_uport_attr_grps(struct uart_port *uport)
> +{
> +	uport->attr_grps[0] = &tty_dev_attr_group;
> +	if (uport->dev_spec_attr_group)
> +		uport->attr_grps[1] = uport->dev_spec_attr_group;
> +}
>  
>  /**
>   *	uart_add_one_port - attach a driver-defined port structure
> @@ -2607,12 +2608,15 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>  
>  	uart_configure_port(drv, state, uport);
>  
> +	make_uport_attr_grps(uport);
> +
>  	/*
>  	 * Register the port whether it's detected or not.  This allows
>  	 * setserial to be used to alter this port's parameters.
>  	 */
>  	tty_dev = tty_port_register_device_attr(port, drv->tty_driver,
> -			uport->line, uport->dev, port, tty_dev_attr_groups);
> +			uport->line, uport->dev, port,
> +			(const struct attribute_group **)uport->attr_grps);

If you have to cast that hard, something is wrong here, why are you
doing that?


thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/