On Tue, Jul 03, 2018 at 11:11:09AM +0200, Fabien Lahoudere wrote:
> In order to optimize serial communication on imx53 and imx6, we may
> need to tweak DMA period and buffer length per period.
> This patch add sysfs attributes to configure thoses values before
> initialising DMA.
>
> For example, you can access values by reading/writing:
>
> /sys/class/tty/ttymxc*/dma_buffer_size
> /sys/class/tty/ttymxc*/dma_buffer_count
>
> Changes since v1:
> - use uart_port attr_group field instead of using sysfs_create_file.
> - remove useless mutex
> - Fix typo
> - Document sysfs attributes
> - Change attributes name
This list doesn't belong into the commit log, so please put it below the
triple-dash below your sob.
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> ---
> Documentation/ABI/stable/sysfs-driver-imx-uart | 10 +++
> drivers/tty/serial/imx.c | 94 ++++++++++++++++++++++++--
> 2 files changed, 99 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/ABI/stable/sysfs-driver-imx-uart
>
> diff --git a/Documentation/ABI/stable/sysfs-driver-imx-uart b/Documentation/ABI/stable/sysfs-driver-imx-uart
> new file mode 100644
> index 0000000..0ec9ee2
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-driver-imx-uart
> @@ -0,0 +1,10 @@
> +What: /sys/class/tty/ttymxc*/dma_buffer_count
> +Date: July 2018
> +Contact: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> +Description: This attribute handles field rx_periods in struct imx_uart.
> +
> +What: /sys/class/tty/ttymxc*/dma_buffer_size
> +Date: July 2018
> +Contact: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> +Description: This attribute handles field rx_period_length in struct imx_uart.
> +
Trailing empty line at eof. I wonder if this should be more
understandable as a reference to a driver-data structure.
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 4e85357..726f644 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -218,7 +218,9 @@ struct imx_port {
> struct dma_chan *dma_chan_rx, *dma_chan_tx;
> struct scatterlist rx_sgl, tx_sgl[2];
> void *rx_buf;
> + unsigned int rx_buf_size;
> struct circ_buf rx_ring;
> + unsigned int rx_period_length;
> unsigned int rx_periods;
> dma_cookie_t rx_cookie;
> unsigned int tx_bytes;
> @@ -1028,8 +1030,6 @@ static void imx_uart_timeout(struct timer_list *t)
> }
> }
>
> -#define RX_BUF_SIZE (PAGE_SIZE)
> -
> /*
> * There are two kinds of RX DMA interrupts(such as in the MX6Q):
> * [1] the RX DMA buffer is full.
> @@ -1124,9 +1124,8 @@ static int imx_uart_start_rx_dma(struct imx_port *sport)
>
> sport->rx_ring.head = 0;
> sport->rx_ring.tail = 0;
> - sport->rx_periods = RX_DMA_PERIODS;
>
> - sg_init_one(sgl, sport->rx_buf, RX_BUF_SIZE);
> + sg_init_one(sgl, sport->rx_buf, sport->rx_buf_size);
> ret = dma_map_sg(dev, sgl, 1, DMA_FROM_DEVICE);
> if (ret == 0) {
> dev_err(dev, "DMA mapping error for RX.\n");
> @@ -1244,7 +1243,8 @@ static int imx_uart_dma_init(struct imx_port *sport)
> goto err;
> }
>
> - sport->rx_buf = kzalloc(RX_BUF_SIZE, GFP_KERNEL);
> + sport->rx_buf_size = sport->rx_period_length * sport->rx_periods;
> + sport->rx_buf = kzalloc(sport->rx_buf_size, GFP_KERNEL);
> if (!sport->rx_buf) {
> ret = -ENOMEM;
> goto err;
> @@ -1705,6 +1705,87 @@ static const char *imx_uart_type(struct uart_port *port)
> return sport->port.type == PORT_IMX ? "IMX" : NULL;
> }
>
> +
> +static ssize_t dma_buffer_size_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned int plen;
> + int ret;
> + struct device *port_device = dev->parent;
> + struct imx_port *sport = dev_get_drvdata(port_device);
> +
> + if (sport->dma_chan_rx) {
> + dev_warn(dev,
> + "You cannot set dma buffer size if DMA is inited\n");
> + return -EBUSY;
> + }
> + ret = kstrtou32(buf, 0, &plen);
> + if (ret < 0)
> + dev_warn(dev, "Bad argument %s\n", buf);
> + else {
> + sport->rx_period_length = plen;
> + ret = count;
> + }
> + return ret;
> +}
> +
> +static ssize_t dma_buffer_size_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct device *port_device = dev->parent;
> + struct imx_port *sport = dev_get_drvdata(port_device);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", sport->rx_period_length);
> +}
> +
> +static ssize_t dma_buffer_count_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned int periods;
> + int ret;
> + struct device *port_device = dev->parent;
> + struct imx_port *sport = dev_get_drvdata(port_device);
> +
> + if (sport->dma_chan_rx) {
> + dev_warn(dev,
> + "You cannot set dma buffer count if DMA is inited\n");
s/inited/initialized/
> + return -EBUSY;
> + }
> + ret = kstrtou32(buf, 0, &periods);
> + if (ret < 0)
> + dev_warn(dev, "Bad argument %s\n", buf);
> + else {
> + sport->rx_periods = periods;
> + ret = count;
> + }
If the else branch has curly braces, the if branch must have, too.
> + return ret;
> +}
> +
> +static ssize_t dma_buffer_count_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct device *port_device = dev->parent;
> + struct imx_port *sport = dev_get_drvdata(port_device);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", sport->rx_periods);
> +}
> +
> +static DEVICE_ATTR_RW(dma_buffer_size);
> +static DEVICE_ATTR_RW(dma_buffer_count);
I would have put DEVICE_ATTR_RW(dma_buffer_size); below
dma_buffer_size_show().
> +
> +static struct attribute *imx_uart_attrs[] = {
> + &dev_attr_dma_buffer_size.attr,
> + &dev_attr_dma_buffer_count.attr,
> + NULL,
No , after the terminating entry please.
> +};
> +static struct attribute_group imx_uart_attr_group = {
> + .attrs = imx_uart_attrs,
> +};
> +
> /*
> * Configure/autoconfigure the port.
> */
> @@ -2236,6 +2317,9 @@ static int imx_uart_probe(struct platform_device *pdev)
> sport->port.rs485_config = imx_uart_rs485_config;
> sport->port.flags = UPF_BOOT_AUTOCONF;
> timer_setup(&sport->timer, imx_uart_timeout, 0);
> + sport->rx_period_length = PAGE_SIZE / RX_DMA_PERIODS;
> + sport->rx_periods = RX_DMA_PERIODS;
> + sport->port.attr_group = &imx_uart_attr_group;
>
> sport->gpios = mctrl_gpio_init(&sport->port, 0);
> if (IS_ERR(sport->gpios))
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
