On Fri, May 30, 2014 at 03:16:22PM +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/class/tty/ttyS0/rx_trig_bytes
> 8
>
> - Write user setting
> # echo 1 > /sys/class/tty/ttyS0/rx_trig_bytes
> # cat /sys/class/tty/ttyS0/rx_trig_bytes
> 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()
>
> Changes in V7:
> - Add Documentation
> - Change I/F name from rx_int_trig to rx_trig_bytes because the name
> rx_int_trig is hard to understand how users specify the value
>
> Changes in V8:
> - Divide this patch from V7's patch based on Greg's comment
>
> Signed-off-by: Yoshihiro YUNOMAE <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-tty | 16 +++
> drivers/tty/serial/8250/8250.h | 2
> drivers/tty/serial/8250/8250_core.c | 173 ++++++++++++++++++++++++++++++++---
> include/linux/serial_8250.h | 2
> include/uapi/linux/serial_reg.h | 5 +
> 5 files changed, 183 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
> index ad22fb0..9eb3c2b 100644
> --- a/Documentation/ABI/testing/sysfs-tty
> +++ b/Documentation/ABI/testing/sysfs-tty
> @@ -138,3 +138,19 @@ Description:
>
> These sysfs values expose the TIOCGSERIAL interface via
> sysfs rather than via ioctls.
> +
> +What: /sys/class/tty/ttyS0/rx_trig_bytes
> +Date: May 2014
> +Contact: Yoshihiro YUNOMAE <[email protected]>
> +Description:
> + Shows current RX interrupt trigger bytes or sets the
> + user specified value to change it for the FIFO buffer.
> + Users can show or set this value regardless of opening the
> + serial device file or not.
> +
> + The RX trigger can be set one of four kinds of values for UART
> + serials. When users input a meaning less value to this I/F,
> + the RX trigger is changed to the nearest lower value for the
> + device specification. For example, when user sets 7bytes on
> + 16550A, which has 1/4/8/14 bytes trigger, the RX trigger is
> + automatically changed to 4 bytes.
I really don't like the way that the tty core has been changed to handle
multiple attribute groups, as I feel tty drivers shouldn't be creating
"special" sysfs files, depending on what driver is bound to them.
Usually we have handled this using tools like 'stty' and ioctls, right?
Surely there is an ioctl to control the interrupt level, right? Hasn't
this been covered before somehow?
thanks,
greg k-h
> I really don't like the way that the tty core has been changed to handle
> multiple attribute groups, as I feel tty drivers shouldn't be creating
> "special" sysfs files, depending on what driver is bound to them.
The intent isn't that it is "special" but that it can be propogated to
others as and when they wish to provide it.
> Usually we have handled this using tools like 'stty' and ioctls, right?
> Surely there is an ioctl to control the interrupt level, right? Hasn't
> this been covered before somehow?
No, and the direction when this started was to use sysfs as we have also
been moving all the other attributes towards sysfs and has been since
2012.
TTY devices do have lots of strange attributes and right now many of them
are only programmable by using device tree and rebooting.
Alan
On Thu, Jul 10, 2014 at 03:31:57PM +0100, One Thousand Gnomes wrote:
> > I really don't like the way that the tty core has been changed to handle
> > multiple attribute groups, as I feel tty drivers shouldn't be creating
> > "special" sysfs files, depending on what driver is bound to them.
>
> The intent isn't that it is "special" but that it can be propogated to
> others as and when they wish to provide it.
>
> > Usually we have handled this using tools like 'stty' and ioctls, right?
> > Surely there is an ioctl to control the interrupt level, right? Hasn't
> > this been covered before somehow?
>
> No, and the direction when this started was to use sysfs as we have also
> been moving all the other attributes towards sysfs and has been since
> 2012.
>
> TTY devices do have lots of strange attributes and right now many of them
> are only programmable by using device tree and rebooting.
Ok. Hm, there has to be a better way to do the group sysfs file
handling...
Let me work on this tomorrow and see what I can come up with. We should
be able to use the is_visable() attribute to create/notcreate the
attribute where needed...
thanks,
greg k-h
On Thu, Jul 10, 2014 at 04:16:37PM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jul 10, 2014 at 03:31:57PM +0100, One Thousand Gnomes wrote:
> > > I really don't like the way that the tty core has been changed to handle
> > > multiple attribute groups, as I feel tty drivers shouldn't be creating
> > > "special" sysfs files, depending on what driver is bound to them.
> >
> > The intent isn't that it is "special" but that it can be propogated to
> > others as and when they wish to provide it.
> >
> > > Usually we have handled this using tools like 'stty' and ioctls, right?
> > > Surely there is an ioctl to control the interrupt level, right? Hasn't
> > > this been covered before somehow?
> >
> > No, and the direction when this started was to use sysfs as we have also
> > been moving all the other attributes towards sysfs and has been since
> > 2012.
> >
> > TTY devices do have lots of strange attributes and right now many of them
> > are only programmable by using device tree and rebooting.
>
> Ok. Hm, there has to be a better way to do the group sysfs file
> handling...
>
> Let me work on this tomorrow and see what I can come up with. We should
> be able to use the is_visable() attribute to create/notcreate the
> attribute where needed...
Ok, how about this patch instead of your first one?
It creates a new port attribute, attr_group, which you should be able to
set in the 8250 driver if you device needs it. Then the serial core
will handle the dynamic group creation, without relying on a "magic"
number of groups. Very close to your patch, but now it's dynamic, and
no fixed array, and no crazy casting in the
tty_port_register_device_attr() call.
I have not tested this, only test built the code.
If it works for you, can you redo your second patch, and then send both
of these back to me so that I can apply them?
thanks,
greg k-h
---------------
Subject: [PATCH] Serial: allow port drivers to have a default attribute group
From: Greg Kroah-Hartman <[email protected]>
Some serial drivers (like 8250), want to add sysfs files. We need to do
so in a race-free way, so allow any port to be able to specify an
attribute group that should be added at device creation time.
Based on a patch from Yoshihiro YUNOMAE
<[email protected]>
Cc: Yoshihiro YUNOMAE <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b70095e55df6..b8c3a5419060 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2564,11 +2564,6 @@ static const 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
- };
-
/**
* uart_add_one_port - attach a driver-defined port structure
@@ -2586,6 +2581,7 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
struct tty_port *port;
int ret = 0;
struct device *tty_dev;
+ int num_groups;
BUG_ON(in_interrupt());
@@ -2619,12 +2615,26 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
uart_configure_port(drv, state, uport);
+ num_groups = 2;
+ if (uport->attr_group)
+ num_groups++;
+
+ uport->tty_groups = kzalloc(sizeof(struct attribute_group *) * num_groups,
+ GFP_KERNEL);
+ if (!uport->tty_groups) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ uport->tty_groups[0] = &tty_dev_attr_group;
+ if (uport->attr_group)
+ uport->tty_groups[1] = uport->attr_group;
+
/*
* 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, uport->tty_groups);
if (likely(!IS_ERR(tty_dev))) {
device_set_wakeup_capable(tty_dev, 1);
} else {
@@ -2703,6 +2713,7 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
*/
if (uport->type != PORT_UNKNOWN)
uport->ops->release_port(uport);
+ kfree(uport->tty_groups);
/*
* Indicate that there isn't a port here anymore.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5bbb809ee197..cf3a1e789bf5 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -199,6 +199,8 @@ struct uart_port {
unsigned char suspended;
unsigned char irq_wake;
unsigned char unused[2];
+ struct attribute_group *attr_group; /* port specific attributes */
+ const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
void *private_data; /* generic platform data pointer */
};
(2014/07/13 6:22), Greg KH wrote:
> On Thu, Jul 10, 2014 at 04:16:37PM -0700, Greg Kroah-Hartman wrote:
>> On Thu, Jul 10, 2014 at 03:31:57PM +0100, One Thousand Gnomes wrote:
>>>> I really don't like the way that the tty core has been changed to handle
>>>> multiple attribute groups, as I feel tty drivers shouldn't be creating
>>>> "special" sysfs files, depending on what driver is bound to them.
>>>
>>> The intent isn't that it is "special" but that it can be propogated to
>>> others as and when they wish to provide it.
>>>
>>>> Usually we have handled this using tools like 'stty' and ioctls, right?
>>>> Surely there is an ioctl to control the interrupt level, right? Hasn't
>>>> this been covered before somehow?
>>>
>>> No, and the direction when this started was to use sysfs as we have also
>>> been moving all the other attributes towards sysfs and has been since
>>> 2012.
>>>
>>> TTY devices do have lots of strange attributes and right now many of them
>>> are only programmable by using device tree and rebooting.
>>
>> Ok. Hm, there has to be a better way to do the group sysfs file
>> handling...
>>
>> Let me work on this tomorrow and see what I can come up with. We should
>> be able to use the is_visable() attribute to create/notcreate the
>> attribute where needed...
>
> Ok, how about this patch instead of your first one?
>
> It creates a new port attribute, attr_group, which you should be able to
> set in the 8250 driver if you device needs it. Then the serial core
> will handle the dynamic group creation, without relying on a "magic"
> number of groups. Very close to your patch, but now it's dynamic, and
> no fixed array, and no crazy casting in the
> tty_port_register_device_attr() call.
Thank you very much for your patch.
I'll use it as 1st patch in next version.
> I have not tested this, only test built the code.
OK, I'll test it.
> If it works for you, can you redo your second patch, and then send both
> of these back to me so that I can apply them?
Sure.
Thank you,
Yoshihiro YUNOMAE
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]