Re: [PATCH v2 1/1] serial: imx - Add dma buffer confugration via sysfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux