Hello,
On Wed, Jun 27, 2018 at 10:01:06AM +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/devices/soc0/soc/2100000.aips-bus/21ec000.serial/period_number
> /sys/devices/soc0/soc/2100000.aips-bus/21ec000.serial/period_length
I don't think "period_number" and "period_length" are good choices. I'd
suggest "dma_buffer_count" and "dma_buffer_size".
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@xxxxxxxxxxxxx>
> ---
> drivers/tty/serial/imx.c | 100 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 4e85357..b35381c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -218,8 +218,11 @@ 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;
> + struct mutex rx_period_lock;
Please document, what this lock protects.
> dma_cookie_t rx_cookie;
> unsigned int tx_bytes;
> unsigned int dma_tx_nents;
> @@ -1028,8 +1031,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 +1125,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 +1244,10 @@ static int imx_uart_dma_init(struct imx_port *sport)
> goto err;
> }
>
> - sport->rx_buf = kzalloc(RX_BUF_SIZE, GFP_KERNEL);
> + mutex_lock(&sport->rx_period_lock);
> + sport->rx_buf_size = sport->rx_period_length * sport->rx_periods;
> + mutex_unlock(&sport->rx_period_lock);
> + sport->rx_buf = kzalloc(sport->rx_buf_size, GFP_KERNEL);
> if (!sport->rx_buf) {
> ret = -ENOMEM;
> goto err;
> @@ -2133,6 +2137,84 @@ static void imx_uart_console_early_write(struct console *con, const char *s,
> .cons = IMX_CONSOLE,
> };
>
> +static ssize_t imx_uart_set_period_length(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned int plen;
> + int ret;
> + struct imx_port *sport = dev_get_drvdata(dev);
> +
> + if (count == 0)
> + return -EINVAL;
Can this happen?
> + if (sport->dma_chan_rx) {
> + dev_warn(dev, "You cannot set period length if DMA is inited");
missing \n
> + return -EBUSY;
> + }
> +
> + ret = sscanf(buf, "%u", &plen);
> + if (ret <= 0)
> + return ret;
Is it right to return 0 here? I think there are helpers more suitable
than sscanf.
> + mutex_lock(&sport->rx_period_lock);
> + sport->rx_period_length = plen;
> + mutex_unlock(&sport->rx_period_lock);
> +
> + return strnlen(buf, count);
Is it right to return 5 if I write "5 a b" to the sysfs file?
> +}
> +
> +static ssize_t imx_uart_get_period_length(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct imx_port *sport = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", sport->rx_period_length);
> +}
> +
> +static ssize_t imx_uart_set_period_number(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned int periods;
> + int ret;
> + struct imx_port *sport = dev_get_drvdata(dev);
> +
> + if (count == 0)
> + return -EINVAL;
> +
> + if (sport->dma_chan_rx) {
> + dev_warn(dev,
> + "You cannot set number of period if DMA is inited");
missing \n
> + return -EBUSY;
> + }
> +
> + ret = sscanf(buf, "%u", &periods);
> + if (ret <= 0)
> + return ret;
> +
> + mutex_lock(&sport->rx_period_lock);
> + sport->rx_periods = periods;
> + mutex_unlock(&sport->rx_period_lock);
> + return strnlen(buf, count);
> +}
> +
> +static ssize_t imx_uart_get_period_number(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct imx_port *sport = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", sport->rx_periods);
> +}
> +
> +static DEVICE_ATTR(period_length, 0660,
> + imx_uart_get_period_length, imx_uart_set_period_length);
> +static DEVICE_ATTR(period_number, 0660,
> + imx_uart_get_period_number, imx_uart_set_period_number);
I would have used 0644, consider using DEVICE_ATTR_RW.
> +
> #ifdef CONFIG_OF
> /*
> * This function returns 1 iff pdev isn't a device instatiated by dt, 0 iff it
> @@ -2216,6 +2297,13 @@ static int imx_uart_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> + if (device_create_file(&pdev->dev,
> + (const struct device_attribute *)&dev_attr_period_length.attr) != 0)
> + dev_err(&pdev->dev, "Cannot create period length attribute");
> + if (device_create_file(&pdev->dev,
> + (const struct device_attribute *)&dev_attr_period_number.attr) != 0)
> + dev_err(&pdev->dev, "Cannot create period number attribute");
> +
This is bad and wrong. You must not add attributes to already registered
devices. This also needs API documentation.
Also should this only be added if the device will use DMA on the next
startup?
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(base))
> @@ -2257,6 +2345,10 @@ static int imx_uart_probe(struct platform_device *pdev)
>
> sport->port.uartclk = clk_get_rate(sport->clk_per);
>
> + sport->rx_period_length = PAGE_SIZE / RX_DMA_PERIODS;
> + sport->rx_periods = RX_DMA_PERIODS;
> + mutex_init(&sport->rx_period_lock);
> +
> /* For register access, we only need to enable the ipg clock. */
> ret = clk_prepare_enable(sport->clk_ipg);
> if (ret) {
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
