Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

Hello,

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

On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> 
> 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?

That doesn't sound like a good fix but more like a work around. Which
other options did you test to fix your problem?

> 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 <nandor.han-JJi787mZWgc@public.gmane.org>
> 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.
 
>  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.

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

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:
> Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> > 
> > 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?
> 
> That doesn't sound like a good fix but more like a work around. Which
> other options did you test to fix your problem?
> 
> > 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 <nandor.han-JJi787mZWgc@public.gmane.org>
> > 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.
>  
> >  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.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Romain Perier]

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:
>> Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>
>> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
>>> From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
>>>
>>> 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

>>> 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 <nandor.han-JJi787mZWgc@public.gmane.org>
>>> 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.
>>  
>>>  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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

On Tue, Jul 04, 2017 at 09:01:24AM +0200, Romain Perier wrote:
> Le 03/07/2017 à 20:17, Uwe Kleine-König a écrit :
> > On Thu, Jun 29, 2017 at 08:26:18PM +0200, Uwe Kleine-König wrote:
> >> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> >>> From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> >>>
> >>> 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,

That means you're waiting for urgent data that already sits in the
hardware's RX fifo but as you send in parallel the hardware is busy and
doesn't process the RX fifo?

Would it help to decrease UFCR.RXTL? Or is this a HW bug that the RX
fifo isn't considered idle when you send much?

Or is this a software problem and the urgent data sits somewhere in the
tty or dma layer already?

> with the current size of the DMA buffer (PAGE_SIZE) you might introduce
> latencies between data being produced and data being consumed.

Making DMA buffers smaller doesn't sound like the right approach to
minimize delays. If you choose to do so you increase the number of irqs
and so this is a user policy what you want and shouldn't fixed in the
oftree.

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Greg Kroah-Hartman]

On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> Hello Greg,

Please don't top-post :(

> 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). :-|

And now it's in Linus's tree! :)

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

Should I revert it, or can I get a fix for it?  Reviewing the patch
when it was submitted would have been best, not after it ends up in one
of my trees...

thanks,

greg k-h
--
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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

On Tue, Jul 04, 2017 at 09:41:59AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> > Hello Greg,
> 
> Please don't top-post :(

I only kept the mail for reference, as I didn't reply to specific stuff
in the mail I consider writing my stuff at the top of the mail ok (and
even better than below the mail because you don't need to scroll down to
see what I wrote and I think I gave enough context).

> > 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). :-|
> 
> And now it's in Linus's tree! :)

Should I better answer to the pull request mail next time?

> > I'm still convinced that this patch is wrong in its current form.
> 
> Should I revert it, or can I get a fix for it?

I'd say revert for now. At least it's not clear to me how it should be
fixed as the property added to dt shouldn't be there (given my
current understanding of the problem).

> Reviewing the patch when it was submitted would have been best, not
> after it ends up in one of my trees...

Well, I reviewed in less than 24h. I now that Linux development is
quick, but please don't make it that quick.

Otherwise I might consider a procmail rule that sends a "please don't
apply yet" mail until I come around looking at imx-uart patches. :-)

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Romain Perier]

Hello,


Le 04/07/2017 à 09:41, Greg Kroah-Hartman a écrit :
> On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
>> Hello Greg,
> Please don't top-post :(
>
>> 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). :-|
> And now it's in Linus's tree! :)
>
>> I'm still convinced that this patch is wrong in its current form.
> Should I revert it, or can I get a fix for it?  Reviewing the patch
> when it was submitted would have been best, not after it ends up in one
> of my trees...

I don't think there is a fix for it. Either you revert the patch and you
code the fix differently, as I suggested by changing dma_rx_callback...
but that's more intrusive... or you keep this patch.

>
> thanks,
>
> greg k-h

--
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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Greg Kroah-Hartman]

On Tue, Jul 04, 2017 at 10:15:23AM +0200, Romain Perier wrote:
> Hello,
> 
> 
> Le 04/07/2017 à 09:41, Greg Kroah-Hartman a écrit :
> > On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> >> Hello Greg,
> > Please don't top-post :(
> >
> >> 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). :-|
> > And now it's in Linus's tree! :)
> >
> >> I'm still convinced that this patch is wrong in its current form.
> > Should I revert it, or can I get a fix for it?  Reviewing the patch
> > when it was submitted would have been best, not after it ends up in one
> > of my trees...
> 
> I don't think there is a fix for it. Either you revert the patch and you
> code the fix differently, as I suggested by changing dma_rx_callback...
> but that's more intrusive... or you keep this patch.

I'll revert it after 4.13-rc1 is out, unless there are major objections
to that...

thanks,

greg k-h
--
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

[PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: 29 June 2017 21:26
> To: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby
> <jslaby-IBi9RG/b67k@public.gmane.org>; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Han, Nandor (GE
> Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> to DT
> 
> Hello,
> 
> Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> >
> > 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.

Hi Uwe,
   Maybe I can explain a bit better the situation. At least I've tried to explain well enough
the problem and the fix. :)

> 
> 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?

Yes

> What is the DMA buffer?
> You don't mean the FIFO here, do you?
> 

DMA buffer is not HW FIFO. Is the buffer provided by serial driver to DMA to store data.

> That doesn't sound like a good fix but more like a work around. Which other
> options did you test to fix your problem?
> 

I haven't tried any other, because except using maybe, ioctl I haven't got anything better.

Our problem is that in our system some serial ports needs to have really low data latency, where others trade more bytes over data latency. This situation results in a need of beeing able to have different DMA buffer size for different ports. 

How can DMA buffer size affect latency?
DMA works like this: (To answer to your question DMA buffer is not FIFO)
 1. Transfer the data from HW FIFO to DMA buffer based on some interrupts (character received, etc)
 2. Transfer the DMA buffer back to serial port based on some events (buffer full, aging timer, etc)
 3. Serial port forwards to tty buffer.

Data availability to consumer depends on: DMA buffer size, baud rate and communication pattern. By communication patter I'm refering that we send data continuoselly (serial line is never idle) or packet by packet (serial line is idle in between)
Example:
      Baud: 19200 (1Byte = 0.52 ms)
      DMA buffer size: 100 bytes
      Communication pattern: continuously 
      =>  DMA will return data to serial port only when DMA buffer is full, since the communication is continuously. This result in a data latency of 0.52 ms* 100bytes = 52ms. In case the buffer will be 200bytes the letency will be double.

I agree with you, this is not directly a hw property but a DMA configuration item. 
But I've found this to be the best way to configure this comparing with using ioctl.

Let me know if you need more clarification and I would really be open to other options that will solve our problem.

<snip>

> > +- 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.
> 

Serial driver configures a circular ring of buffers for DMA. Here we can configure the size and the number of buffers.

> > +/* 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.
> 

Agree. 


Regards,
    Nandor
--
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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

Hello,

On Mon, Oct 02, 2017 at 01:17:41PM +0000, Han, Nandor (GE Healthcare) wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > Sent: 29 June 2017 21:26
> > To: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby
> > <jslaby-IBi9RG/b67k@public.gmane.org>; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Han, Nandor (GE
> > Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> > to DT
> > 
> > Hello,
> > 
> > Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > 
> > On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> > >
> > > 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.
> 
> Hi Uwe,
>    Maybe I can explain a bit better the situation. At least I've tried to explain well enough
> the problem and the fix. :)
> 
> > 
> > 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?
> 
> Yes
> 
> > What is the DMA buffer?
> > You don't mean the FIFO here, do you?
> > 
> 
> DMA buffer is not HW FIFO. Is the buffer provided by serial driver to DMA to store data.
> 
> > That doesn't sound like a good fix but more like a work around. Which other
> > options did you test to fix your problem?
> > 
> 
> I haven't tried any other, because except using maybe, ioctl I haven't
> got anything better.

My question didn't target where to configure the buffer size instead of
dts. I wonder if it would help to change the fifo watermark limits for
example.

> Our problem is that in our system some serial ports needs to have
> really low data latency, where others trade more bytes over data
> latency. This situation results in a need of beeing able to have
> different DMA buffer size for different ports. 
> 
> How can DMA buffer size affect latency?
> DMA works like this: (To answer to your question DMA buffer is not FIFO)
>  1. Transfer the data from HW FIFO to DMA buffer based on some interrupts (character received, etc)
>  2. Transfer the DMA buffer back to serial port based on some events (buffer full, aging timer, etc)
>  3. Serial port forwards to tty buffer.

BTW In the past I saw the serial core introduce latency, too. Are you
sure that's not your bottle neck?

> Data availability to consumer depends on: DMA buffer size, baud rate
> and communication pattern. By communication patter I'm refering that
> we send data continuoselly (serial line is never idle) or packet by
> packet (serial line is idle in between)
> Example:
>       Baud: 19200 (1Byte = 0.52 ms)
>       DMA buffer size: 100 bytes
>       Communication pattern: continuously 
>       =>  DMA will return data to serial port only when DMA buffer is
>       full, since the communication is continuously. This result in a
>       data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
>       will be 200bytes the letency will be double.
> 
> I agree with you, this is not directly a hw property but a DMA configuration item. 
> But I've found this to be the best way to configure this comparing with using ioctl.
> 
> Let me know if you need more clarification and I would really be open
> to other options that will solve our problem.
> 
> <snip>
> 
> > > +- 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.
> > 
> 
> Serial driver configures a circular ring of buffers for DMA. Here we
> can configure the size and the number of buffers.

The problem is: How should a person, who wants to make available a port
on a machine via dts, choose what value to use for fsl,dma-size?

What you want (for a low latency port) is that also small amounts of
data received are passed quickly to the upper layer. The knob you
identified to be available for that is the dma buffer size.

I'd prefer to talk about low latency instead of buffer sizes when
setting parameters for the port. That this influences the buffer size
(and maybe watermark settings) under the hood shouldn't matter for the
person configuring the low latency property.

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: 05 October 2017 00:50
> To: Han, Nandor (GE Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>
> Cc: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>; Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>; linux-
> serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> to DT
> 
> Hello,
> 
> On Mon, Oct 02, 2017 at 01:17:41PM +0000, Han, Nandor (GE Healthcare)
> wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > > Sent: 29 June 2017 21:26
> > > To: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby
> > > <jslaby-IBi9RG/b67k@public.gmane.org>; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Han, Nandor (GE
> > > Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer
> > > configuration to DT
> > >
> > > Hello,
> > >
> > > Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > >
> > > On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > > > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> > > >
> > > > 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.
> >
> > Hi Uwe,
> >    Maybe I can explain a bit better the situation. At least I've tried
> > to explain well enough the problem and the fix. :)
> >
> > >
> > > 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?
> >
> > Yes
> >
> > > What is the DMA buffer?
> > > You don't mean the FIFO here, do you?
> > >
> >
> > DMA buffer is not HW FIFO. Is the buffer provided by serial driver to DMA to
> store data.
> >
> > > That doesn't sound like a good fix but more like a work around.
> > > Which other options did you test to fix your problem?
> > >
> >
> > I haven't tried any other, because except using maybe, ioctl I haven't
> > got anything better.
> 
> My question didn't target where to configure the buffer size instead of dts. I
> wonder if it would help to change the fifo watermark limits for example.
> 

Ok. Sorry I didn't understand correct. Watermark will not help in this case because it is used only to trigger
the moving of the data from RX FIFO to DMA buffer.  The iMX DMA (check the iMX6 RM A.3.1.2.4 since is missing
from iMX53 RM) will only return data to serial driver when no more data exist in the RX FIFO (is using aging timer for this check)
or BD (DMA buffer) is full. If data is sent continuously DMA will return data to driver only when BD is full.

So what we need here is a trigger that will force DMA to return data faster to serial driver. The only one that I could find was the size of the buffer. Of course another way will be to create different SDMA scripts that can do all kinds of handling -- but dint' want to go on that route :) 

> > Our problem is that in our system some serial ports needs to have
> > really low data latency, where others trade more bytes over data
> > latency. This situation results in a need of beeing able to have
> > different DMA buffer size for different ports.
> >
> > How can DMA buffer size affect latency?
> > DMA works like this: (To answer to your question DMA buffer is not
> > FIFO)  1. Transfer the data from HW FIFO to DMA buffer based on some
> > interrupts (character received, etc)  2. Transfer the DMA buffer back
> > to serial port based on some events (buffer full, aging timer, etc)  3. Serial
> port forwards to tty buffer.
> 
> BTW In the past I saw the serial core introduce latency, too. Are you sure
> that's not your bottle neck?
> 

Can you be more specific? Receiving data seems very straight forward and serial core is not involved that much with or without DMA.
Basically the driver handles the moving of the data from rx FIFO to tty buffer. Beside this lowering the DMA buffer fixed our issue. So I will assume that no. 

> > Data availability to consumer depends on: DMA buffer size, baud rate
> > and communication pattern. By communication patter I'm refering that
> > we send data continuoselly (serial line is never idle) or packet by
> > packet (serial line is idle in between)
> > Example:
> >       Baud: 19200 (1Byte = 0.52 ms)
> >       DMA buffer size: 100 bytes
> >       Communication pattern: continuously
> >       =>  DMA will return data to serial port only when DMA buffer is
> >       full, since the communication is continuously. This result in a
> >       data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> >       will be 200bytes the letency will be double.
> >
> > I agree with you, this is not directly a hw property but a DMA configuration
> item.
> > But I've found this to be the best way to configure this comparing with using
> ioctl.
> >
> > Let me know if you need more clarification and I would really be open
> > to other options that will solve our problem.
> >
> > <snip>
> >
> > > > +- 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.
> > >
> >
> > Serial driver configures a circular ring of buffers for DMA. Here we
> > can configure the size and the number of buffers.
> 
> The problem is: How should a person, who wants to make available a port on a
> machine via dts, choose what value to use for fsl,dma-size?

It doesn't have too. If this configuration is not provided the serial port will have a default value.
The user has the possibility to tweak this settings in case is needed.

> 
> What you want (for a low latency port) is that also small amounts of data
> received are passed quickly to the upper layer. The knob you identified to be
> available for that is the dma buffer size.
> 
> I'd prefer to talk about low latency instead of buffer sizes when setting
> parameters for the port. That this influences the buffer size (and maybe
> watermark settings) under the hood shouldn't matter for the person
> configuring the low latency property.
> 

I understand your point of view. A simple grep in "Documentation/devicetree/bindings" shows me that both options are used (latency/buffer configurations). We could use latency here as configuration and then translate it to DMA buffer size taking in consideration the baud rate but it's a bit more work to do and beside this we have also the number of buffers configuration which will be trickier to convert it from latency. In my opinion, I don't see a significant difference between latency and buffer since most of the serial port users (userspace level) don't touch these settings. Of course, will be a different story if this configuration will be provided using termios or other means accessible from userspace. 

> Best regards
> Uwe
> 

Thanks Uwe :)

Regards,
     Nandor

> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Uwe Kleine-König]

Hello,

On Tue, Oct 10, 2017 at 07:58:31AM +0000, Han, Nandor (GE Healthcare) wrote:
> > > > That doesn't sound like a good fix but more like a work around.
> > > > Which other options did you test to fix your problem?
> > > >
> > >
> > > I haven't tried any other, because except using maybe, ioctl I haven't
> > > got anything better.
> > 
> > My question didn't target where to configure the buffer size instead of dts. I
> > wonder if it would help to change the fifo watermark limits for example.
> > 
> 
> Ok. Sorry I didn't understand correct. Watermark will not help in this case because it is used only to trigger
> the moving of the data from RX FIFO to DMA buffer.  The iMX DMA (check the iMX6 RM A.3.1.2.4 since is missing
> from iMX53 RM) will only return data to serial driver when no more data exist in the RX FIFO (is using aging timer for this check)
> or BD (DMA buffer) is full. If data is sent continuously DMA will return data to driver only when BD is full.

I read once over the description but I failed to understand it. At least
it talks about water mark levels and min(WML-1,Count) which makes me
think that the DMA script makes use of water marking too and my idea to
play with that instead of buffer sizes might be sensible.

> So what we need here is a trigger that will force DMA to return data
> faster to serial driver. The only one that I could find was the size
> of the buffer. Of course another way will be to create different SDMA
> scripts that can do all kinds of handling -- but dint' want to go on
> that route :) 

:-)

> > > Our problem is that in our system some serial ports needs to have
> > > really low data latency, where others trade more bytes over data
> > > latency. This situation results in a need of beeing able to have
> > > different DMA buffer size for different ports.
> > >
> > > How can DMA buffer size affect latency?
> > > DMA works like this: (To answer to your question DMA buffer is not
> > > FIFO)  1. Transfer the data from HW FIFO to DMA buffer based on some
> > > interrupts (character received, etc)  2. Transfer the DMA buffer back
> > > to serial port based on some events (buffer full, aging timer, etc)  3. Serial
> > port forwards to tty buffer.
> > 
> > BTW In the past I saw the serial core introduce latency, too. Are you sure
> > that's not your bottle neck?
> 
> Can you be more specific?

http://www.spinics.net/lists/linux-serial/msg17767.html

> > > Data availability to consumer depends on: DMA buffer size, baud rate
> > > and communication pattern. By communication patter I'm refering that
> > > we send data continuoselly (serial line is never idle) or packet by
> > > packet (serial line is idle in between)
> > > Example:
> > >       Baud: 19200 (1Byte = 0.52 ms)
> > >       DMA buffer size: 100 bytes
> > >       Communication pattern: continuously
> > >       =>  DMA will return data to serial port only when DMA buffer is
> > >       full, since the communication is continuously. This result in a
> > >       data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > >       will be 200bytes the letency will be double.
> > >
> > > I agree with you, this is not directly a hw property but a DMA configuration
> > item.
> > > But I've found this to be the best way to configure this comparing with using
> > ioctl.
> > >
> > > Let me know if you need more clarification and I would really be open
> > > to other options that will solve our problem.
> > >
> > > <snip>
> > >
> > > > > +- 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.
> > > >
> > >
> > > Serial driver configures a circular ring of buffers for DMA. Here we
> > > can configure the size and the number of buffers.
> > 
> > The problem is: How should a person, who wants to make available a port on a
> > machine via dts, choose what value to use for fsl,dma-size?
> 
> It doesn't have too. If this configuration is not provided the serial port will have a default value.
> The user has the possibility to tweak this settings in case is needed.

"The default value works in general" is no good justification to not
provide easy-to-grasp and well documented knobs.

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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: 10 October 2017 11:26
> To: Han, Nandor (GE Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>
> Cc: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>; Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>; linux-
> serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> to DT
> 
> Hello,
> 
> On Tue, Oct 10, 2017 at 07:58:31AM +0000, Han, Nandor (GE Healthcare)
> wrote:
> > > > > That doesn't sound like a good fix but more like a work around.
> > > > > Which other options did you test to fix your problem?
> > > > >
> > > >
> > > > I haven't tried any other, because except using maybe, ioctl I
> > > > haven't got anything better.
> > >
> > > My question didn't target where to configure the buffer size instead
> > > of dts. I wonder if it would help to change the fifo watermark limits for
> example.
> > >
> >
> > Ok. Sorry I didn't understand correct. Watermark will not help in this
> > case because it is used only to trigger the moving of the data from RX
> > FIFO to DMA buffer.  The iMX DMA (check the iMX6 RM A.3.1.2.4 since is
> > missing from iMX53 RM) will only return data to serial driver when no more
> data exist in the RX FIFO (is using aging timer for this check) or BD (DMA
> buffer) is full. If data is sent continuously DMA will return data to driver only
> when BD is full.
> 
> I read once over the description but I failed to understand it. At least it talks
> about water mark levels and min(WML-1,Count) which makes me think that
> the DMA script makes use of water marking too and my idea to play with that
> instead of buffer sizes might be sensible.
> 

:). It is tricky, I agree. I will try to explain it even though I think the most important 
part is to understand that watermark level control when data is moved from out from rx FIFO
 to DMA buffer (BD) and is not the trigger that returns the data from DMA buffer (BD) to serial driver
(which is the one that I need). 

The SDMA script is using 2 triggers to move data from rx FIFO to DMA buffer (BD): 1) watermark hit and 2) aging timer. It ignores completely the "Idle line" trigger. When watermark trigger is received it will move "min(WML-1,BD count)" data from rx FIFO to BD. After this step in the rx FIFO will remain always at least 1 byte (since we move WML-1). The last byte will be moved based on the aging timer. During which SDMA will check if more data is available and move the rest. SDMA will return the BD in 2 situations: 1) BD is full 2) during aging time trigger the rx FIFO is empty. 

> > So what we need here is a trigger that will force DMA to return data
> > faster to serial driver. The only one that I could find was the size
> > of the buffer. Of course another way will be to create different SDMA
> > scripts that can do all kinds of handling -- but dint' want to go on
> > that route :)
> 
> :-)
> 
> > > > Our problem is that in our system some serial ports needs to have
> > > > really low data latency, where others trade more bytes over data
> > > > latency. This situation results in a need of beeing able to have
> > > > different DMA buffer size for different ports.
> > > >
> > > > How can DMA buffer size affect latency?
> > > > DMA works like this: (To answer to your question DMA buffer is not
> > > > FIFO)  1. Transfer the data from HW FIFO to DMA buffer based on
> > > > some interrupts (character received, etc)  2. Transfer the DMA
> > > > buffer back to serial port based on some events (buffer full,
> > > > aging timer, etc)  3. Serial
> > > port forwards to tty buffer.
> > >
> > > BTW In the past I saw the serial core introduce latency, too. Are
> > > you sure that's not your bottle neck?
> >
> > Can you be more specific?
> 
> http://www.spinics.net/lists/linux-serial/msg17767.html
> 
> > > > Data availability to consumer depends on: DMA buffer size, baud
> > > > rate and communication pattern. By communication patter I'm
> > > > refering that we send data continuoselly (serial line is never
> > > > idle) or packet by packet (serial line is idle in between)
> > > > Example:
> > > >       Baud: 19200 (1Byte = 0.52 ms)
> > > >       DMA buffer size: 100 bytes
> > > >       Communication pattern: continuously
> > > >       =>  DMA will return data to serial port only when DMA buffer is
> > > >       full, since the communication is continuously. This result in a
> > > >       data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > > >       will be 200bytes the letency will be double.
> > > >
> > > > I agree with you, this is not directly a hw property but a DMA
> > > > configuration
> > > item.
> > > > But I've found this to be the best way to configure this comparing
> > > > with using
> > > ioctl.
> > > >
> > > > Let me know if you need more clarification and I would really be
> > > > open to other options that will solve our problem.
> > > >
> > > > <snip>
> > > >
> > > > > > +- 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.
> > > > >
> > > >
> > > > Serial driver configures a circular ring of buffers for DMA. Here
> > > > we can configure the size and the number of buffers.
> > >
> > > The problem is: How should a person, who wants to make available a
> > > port on a machine via dts, choose what value to use for fsl,dma-size?
> >
> > It doesn't have too. If this configuration is not provided the serial port will
> have a default value.
> > The user has the possibility to tweak this settings in case is needed.
> 
> "The default value works in general" is no good justification to not provide
> easy-to-grasp and well documented knobs.
> 

I agree with you on this. But my intention was to say that the driver is using the same settings as before
and users are not impacted by this change, and I only add the option to control that value.

> Best regards
> Uwe
> 

Regards,
    Nandor

> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

RE: [PATCH] serial: imx-serial - move DMA buffer configuration to DT

[Han, Nandor (GE Healthcare)]

> -----Original Message-----
> From: linux-serial-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-serial-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Han, Nandor (GE Healthcare)
> Sent: 10 October 2017 13:17
> To: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby
> <jslaby-IBi9RG/b67k@public.gmane.org>; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> Martyn Welch <martyn.welch-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Subject: EXT: [PATCH] serial: imx-serial - move DMA buffer configuration to
> DT
> 
> 
> 
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > Sent: 10 October 2017 11:26
> > To: Han, Nandor (GE Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>
> > Cc: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>; Greg Kroah-Hartman
> > <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>; linux-
> > serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer
> > configuration to DT
> >

Anything that I could still clarify here I'll be happy to explain :)

> > Hello,
> >
> > On Tue, Oct 10, 2017 at 07:58:31AM +0000, Han, Nandor (GE Healthcare)
> > wrote:
> > > > > > That doesn't sound like a good fix but more like a work around.
> > > > > > Which other options did you test to fix your problem?
> > > > > >
> > > > >
> > > > > I haven't tried any other, because except using maybe, ioctl I
> > > > > haven't got anything better.
> > > >
> > > > My question didn't target where to configure the buffer size
> > > > instead of dts. I wonder if it would help to change the fifo
> > > > watermark limits for
> > example.
> > > >
> > >
> > > Ok. Sorry I didn't understand correct. Watermark will not help in
> > > this case because it is used only to trigger the moving of the data
> > > from RX FIFO to DMA buffer.  The iMX DMA (check the iMX6 RM
> > > A.3.1.2.4 since is missing from iMX53 RM) will only return data to
> > > serial driver when no more
> > data exist in the RX FIFO (is using aging timer for this check) or BD
> > (DMA
> > buffer) is full. If data is sent continuously DMA will return data to
> > driver only when BD is full.
> >
> > I read once over the description but I failed to understand it. At
> > least it talks about water mark levels and min(WML-1,Count) which
> > makes me think that the DMA script makes use of water marking too and
> > my idea to play with that instead of buffer sizes might be sensible.
> >
> 
> :). It is tricky, I agree. I will try to explain it even though I think the most
> important part is to understand that watermark level control when data is
> moved from out from rx FIFO  to DMA buffer (BD) and is not the trigger that
> returns the data from DMA buffer (BD) to serial driver (which is the one that I
> need).
> 
> The SDMA script is using 2 triggers to move data from rx FIFO to DMA buffer
> (BD): 1) watermark hit and 2) aging timer. It ignores completely the "Idle line"
> trigger. When watermark trigger is received it will move "min(WML-1,BD
> count)" data from rx FIFO to BD. After this step in the rx FIFO will remain
> always at least 1 byte (since we move WML-1). The last byte will be moved
> based on the aging timer. During which SDMA will check if more data is
> available and move the rest. SDMA will return the BD in 2 situations: 1) BD is
> full 2) during aging time trigger the rx FIFO is empty.
> 
> > > So what we need here is a trigger that will force DMA to return data
> > > faster to serial driver. The only one that I could find was the size
> > > of the buffer. Of course another way will be to create different
> > > SDMA scripts that can do all kinds of handling -- but dint' want to
> > > go on that route :)
> >
> > :-)
> >
> > > > > Our problem is that in our system some serial ports needs to
> > > > > have really low data latency, where others trade more bytes over
> > > > > data latency. This situation results in a need of beeing able to
> > > > > have different DMA buffer size for different ports.
> > > > >
> > > > > How can DMA buffer size affect latency?
> > > > > DMA works like this: (To answer to your question DMA buffer is
> > > > > not
> > > > > FIFO)  1. Transfer the data from HW FIFO to DMA buffer based on
> > > > > some interrupts (character received, etc)  2. Transfer the DMA
> > > > > buffer back to serial port based on some events (buffer full,
> > > > > aging timer, etc)  3. Serial
> > > > port forwards to tty buffer.
> > > >
> > > > BTW In the past I saw the serial core introduce latency, too. Are
> > > > you sure that's not your bottle neck?
> > >
> > > Can you be more specific?
> >
> > http://www.spinics.net/lists/linux-serial/msg17767.html
> >
> > > > > Data availability to consumer depends on: DMA buffer size, baud
> > > > > rate and communication pattern. By communication patter I'm
> > > > > refering that we send data continuoselly (serial line is never
> > > > > idle) or packet by packet (serial line is idle in between)
> > > > > Example:
> > > > >       Baud: 19200 (1Byte = 0.52 ms)
> > > > >       DMA buffer size: 100 bytes
> > > > >       Communication pattern: continuously
> > > > >       =>  DMA will return data to serial port only when DMA buffer is
> > > > >       full, since the communication is continuously. This result in a
> > > > >       data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > > > >       will be 200bytes the letency will be double.
> > > > >
> > > > > I agree with you, this is not directly a hw property but a DMA
> > > > > configuration
> > > > item.
> > > > > But I've found this to be the best way to configure this
> > > > > comparing with using
> > > > ioctl.
> > > > >
> > > > > Let me know if you need more clarification and I would really be
> > > > > open to other options that will solve our problem.
> > > > >
> > > > > <snip>
> > > > >
> > > > > > > +- 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.
> > > > > >
> > > > >
> > > > > Serial driver configures a circular ring of buffers for DMA.
> > > > > Here we can configure the size and the number of buffers.
> > > >
> > > > The problem is: How should a person, who wants to make available a
> > > > port on a machine via dts, choose what value to use for fsl,dma-size?
> > >
> > > It doesn't have too. If this configuration is not provided the
> > > serial port will
> > have a default value.
> > > The user has the possibility to tweak this settings in case is needed.
> >
> > "The default value works in general" is no good justification to not
> > provide easy-to-grasp and well documented knobs.
> >
> 
> I agree with you on this. But my intention was to say that the driver is using
> the same settings as before and users are not impacted by this change, and I
> only add the option to control that value.
> 
> > Best regards
> > Uwe
> >
> 
> Regards,
>     Nandor
> 
> > --
> > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
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