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

[Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>]

Hi Stephen,

Thank you for your reply.

(2014/03/20 4:51), Stephen Warren wrote:
> On 03/19/2014 04:15 AM, 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.
>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 69932b7..fc17fb2 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -531,11 +531,10 @@ 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);
>> +	if (!p->fcr)
>> +		p->fcr = uart_config[p->port.type].fcr;
>
> Rather than sprinkling this initialization all over the place, can't the
> fcr value be set up one time, when the port is first created?

It's nice idea. If we can store default value of fcr to up->fcr when
the port is first created, we don't need to check whether p->fcr exists
or not.
Maybe by adding the operation after register_dev_spec_attr_grp()
in my patch, we can avoid the check.

>> +static int convert_fcr2val(struct uart_8250_port *up, unsigned char fcr)
>> +{
>> +	unsigned char trig_raw = fcr & UART_FCR_TRIGGER_MASK;
>> +
>> +	switch (up->port.type) {
>> +	case PORT_16550A:
>> +		if (trig_raw == UART_FCR_R_TRIG_00)
>> +			return 1;
>> +		else if (trig_raw == UART_FCR_R_TRIG_01)
>> +			return 4;
>> +		else if (trig_raw == UART_FCR_R_TRIG_10)
>> +			return 8;
>> +		else if (trig_raw == UART_FCR_R_TRIG_11)
>> +			return 14;
>> +		break;
>> +	}
>> +	return -EOPNOTSUPP;
>> +}
>
> Rather than implementing this translation inside this one function
> (well, and convert_val2rxtrig() too), perhaps it would make sense to put
> the values into uart_config[] and just look them up here. That would
> better isolate the type-specific data into one place.

Sure. I'll apply following implementation:

  [PORT_16550A] = {
     .name		= "16550A",
     .fifo_size		= 16,
     .tx_loadsz		= 16,
     .fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
+   .rx_trig_byte	= {1, 4, 8, 14},
     .flags		= UART_CAP_FIFO,
  };


> The comment right before UART_FCR_R_TRIG_00 in
> include/uapi/linux/serial_reg.h might help when filling in any extra
> fields in uart_config[].

OK, I'll add following rx_trig_byte to PORT_16650V2, PORT_16654,
PORT_16750, and PORT_TEGRA in uart_config[].

/*
  * Note: The FIFO trigger levels are chip specific:
  *      RX:76 = 00  01  10  11  TX:54 = 00  01  10  11
  * PC16550D:     1   4   8  14          xx  xx  xx  xx
  * TI16C550A:    1   4   8  14          xx  xx  xx  xx
  * TI16C550C:    1   4   8  14          xx  xx  xx  xx
  * ST16C550:     1   4   8  14          xx  xx  xx  xx
  * ST16C650:     8  16  24  28          16   8  24  30  PORT_16650V2
  * NS16C552:     1   4   8  14          xx  xx  xx  xx
  * ST16C654:     8  16  56  60           8  16  32  56  PORT_16654
  * TI16C750:     1  16  32  56          xx  xx  xx  xx  PORT_16750
  * TI16C752:     8  16  56  60           8  16  32  56
  * Tegra:        1   4   8  14          16   8   4   1  PORT_TEGRA
  */

Thanks,
Yoshihiro YUNOMAE

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com