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
>
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> ---
The "changes" should go below the --- line so they don't show up in the
changelog.
> 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.
How does rx_periods relate to dma_buffer_count?
> +
> +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.
Same here, I don't understand the relationship. Please spell it out.
> +
> 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;
Why not both next to each other after rx_ring?
> 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");
What does this mean?
> + return -EBUSY;
> + }
> + ret = kstrtou32(buf, 0, &plen);
> + if (ret < 0)
> + dev_warn(dev, "Bad argument %s\n", buf);
Look, a trivial way to DoS the kernel log :(
Hint, never do this, it's not needed at all.
> + 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);
No need to use snprintf(), just use sprintf, you "know" your string will
fit in the buffer. Yeah, it causes automatic checkers to go crazy, I
love it :)
Same comments for the implementation of the other attribute.
thanks,
greg k-h
--
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
