Hello,


Le 03/07/2017 à 20:17, Uwe Kleine-König a écrit :

Hello Greg,

I'm a bit disappointed that you didn't drop this patch. At the time I
sent my review it was still in your tty-testing branch and now it's part
of your pull request for 4.13-rc1 (as commit
a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|

I'm still convinced that this patch is wrong in its current form.

Best regards
Uwe

On Thu, Jun 29, 2017 at 08:26:18PM +0200, Uwe Kleine-König wrote:

quoted

Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:

quoted

From: Nandor Han <redacted>

The size of the DMA buffer can affect the delta time between data being
produced and data being consumed. Basically the DMA system will move
data to tty buffer when a) DMA buffer is full b) serial line is idle.
The situation is visible when producer generates data continuously and
there is no possibility for idle line. At this point the DMA buffer is
directly affecting the delta time.

This doesn't look like a hw property but a configuration item. Also I
don't understand the problematic case. The i.MX is sending continuously
and then doesn't receive bytes until the DMA buffer is full? What is the
DMA buffer? You don't mean the FIFO here, do you?

by DMA buffer, I mean the size of the RX FIFO. Data will be moved to tty
when the FIFO
is full or the serial line is idle. Now suppose you have an important
work load and the serial line is never idle,
with the current size of the DMA buffer (PAGE_SIZE) you might introduce
latencies between data being produced and data being consumed.


We could also modify dma_rx_callback(), but this current solution is
less intrusive, imho.

Regards,
Romain

quoted

quoted

The patch will add the possibility to configure the DMA buffers in DT,
which case by case can be configured separately for every driver
instance. The DT configuration is optional and in case missing the
driver will use the 4096 buffer with 4 periods (as before), therefore no
clients are impacted by this change.

Signed-off-by: Nandor Han <redacted>
Signed-off-by: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 .../devicetree/bindings/serial/fsl-imx-uart.txt    |  2 ++
 drivers/tty/serial/imx.c                           | 25 +++++++++++++++-------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
index 574c3a2..e6b5724 100644
--- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt

@@ -9,6 +9,7 @@ Optional properties:
 - fsl,irda-mode : Indicate the uart supports irda mode
 - fsl,dte-mode : Indicate the uart works in DTE mode. The uart works
                   in DCE mode by default.
+- fsl,dma-size : Indicate the size of the DMA buffer and its periods

This is a sparse description, just from reading that I don't understand
what it does.
 

quoted

 Please check Documentation/devicetree/bindings/serial/serial.txt
 for the complete list of generic properties.

@@ -28,4 +29,5 @@ uart1: serial@73fbc000 {
 	interrupts = <31>;
 	uart-has-rtscts;
 	fsl,dte-mode;
+	fsl,dma-size = <1024 4>;
 };

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bbefddd..7327477 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c

@@ -186,6 +186,11 @@
 
 #define UART_NR 8
 
+/* RX DMA buffer periods */
+#define RX_DMA_PERIODS 4
+#define RX_BUF_SIZE	(PAGE_SIZE)

These names should include "DEFAULT" in their name now to fit their new
semantic.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html