imx.c RXTL

imx.c RXTL

[Frazer, Will]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

Hi All,

I've been looking at
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/tty/serial/imx.c

& notice:

#define TXTL 2 /* reset default */
#define RXTL 1 /* reset default */

This seems to be setting the RX Threshold FIFO to 1 - rather than something more sensible like 14 which is more normal for 16550 compatible UARTS.

In tests I can see that with the value set as 1, under intensive serial use, CPU use goes towards about 20% (solo core). With the value set to 14 it's closer to 2%.

I can't think of a good reason why a FIFO would be set to interrupt for every character so is there some known bug/reason that anyone can advise me on as to why it is so?

Perhaps it's like this because it was only considered for console operation??

Thanks,
Will Frazer

Eurotech UK

[-- Attachment #2: Type: text/html, Size: 3492 bytes --]

Re: imx.c RXTL

[Fabio Estevam]

Hi Will,

On Wed, Apr 29, 2015 at 9:08 AM, Frazer, Will <will.frazer@eurotech.com> wrote:

> In tests I can see that with the value set as 1, under intensive serial use,
> CPU use goes towards about 20% (solo core). With the value set to 14 it’s
> closer to 2%.

I think your proposal makes sense.

Should we also change TXTL?

Would you like to prepare and submit a patch with your RXTL change to
linux-serial list?

Regards,

Fabio Estevam

Re: imx.c RXTL

[Fabio Estevam]

Hi Will,

On Wed, Apr 29, 2015 at 10:24 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Will,
>
> On Wed, Apr 29, 2015 at 9:08 AM, Frazer, Will <will.frazer@eurotech.com> wrote:
>
>> In tests I can see that with the value set as 1, under intensive serial use,
>> CPU use goes towards about 20% (solo core). With the value set to 14 it’s
>> closer to 2%.
>
> I think your proposal makes sense.
>
> Should we also change TXTL?

I tested your proposal and it seems we need some logic to adjust the RXTL value.

For console operation we need it to be RXTL, otherwise we miss echoing
the chars.

Regards,

Fabio Estevam

Re: imx.c RXTL

[Nikolay Dimitrov]

Hi Fabio, Will,

On 04/29/2015 06:29 PM, Fabio Estevam wrote:
> Hi Will,
>
> On Wed, Apr 29, 2015 at 10:24 AM, Fabio Estevam <festevam@gmail.com>
> wrote:
>> Hi Will,
>>
>> On Wed, Apr 29, 2015 at 9:08 AM, Frazer, Will
>> <will.frazer@eurotech.com> wrote:
>>
>>> In tests I can see that with the value set as 1, under intensive
>>> serial use, CPU use goes towards about 20% (solo core). With the
>>> value set to 14 it’s closer to 2%.
>>
>> I think your proposal makes sense.
>>
>> Should we also change TXTL?
>
> I tested your proposal and it seems we need some logic to adjust the
> RXTL value.
>
> For console operation we need it to be RXTL, otherwise we miss
> echoing the chars.

It's normal to have this behavior when RXTL is 1, this essentially
generates 1 interrupt per character. In order to reduce interrupt load,
it's not enough to just increase the RXTL to a higher value, for reasons
observed by Fabio.

If one wants to use RXFIFO > 1 character, the solution is called "Aging
Timer" (it was available even back in the 16550 days). It's
functionality is to assert an interrupt when:
- there's at least 1 character in RXFIFO
- no characters have been received for several character periods (I
think for imx6 it's 8)

Both interrupts (RRDY & AgingTimeout) together handle all situations in
appropriate manner. Let's look at the code
(http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/tty/serial/imx.c?h=imx_3.14.28_1.0.0_ga):


static irqreturn_t imx_int(int irq, void *dev_id)
{
...
	sts = readl(sport->port.membase + USR1);

	if ((sts & USR1_RRDY || sts & USR1_AGTIM) &&
		!sport->dma_is_enabled) {
		if (sts & USR1_AGTIM)
			writel(USR1_AGTIM, sport->port.membase + USR1);
		imx_rxint(irq, dev_id);
	}
...
}


It seems to me that AGTIM is not handled if DMA is enabled.
Unfortunately these 2 features are not connected at all, imho.

It's also good to note that the RXTL level is different for the system
console and other serial ports:


...
#define RXTL 1 /* For console port */
#define RXTL_UART 16 /* For uart */

static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
{
...
	if (uart_console(&sport->port))
		rx_fifo_trig = RXTL;
	else
		rx_fifo_trig = RXTL_UART;
...
}


What can you do to solve this? You can try disabling the dma and test
how it works for you, or you can fix the driver logic to use the AgingTimer.

Regards,
Nikolay

Re: imx.c RXTL

[Fabio Estevam]

On Wed, Apr 29, 2015 at 9:08 AM, Frazer, Will <will.frazer@eurotech.com> wrote:

> In tests I can see that with the value set as 1, under intensive serial use,
> CPU use goes towards about 20% (solo core). With the value set to 14 it’s
> closer to 2%.

Have you tried using DMA?

Re: imx.c RXTL

[Frazer, Will]

Hi Fabio,

Thanks for the response...

On Wed, Apr 29, 2015 at 10:24 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Will,
>
> On Wed, Apr 29, 2015 at 9:08 AM, Frazer, Will <will.frazer@eurotech.com> wrote:
>
>> In tests I can see that with the value set as 1, under intensive 
>> serial use, CPU use goes towards about 20% (solo core). With the 
>> value set to 14 it’s closer to 2%.
>
> I think your proposal makes sense.
>
> Should we also change TXTL?
Probably.  Not looked too closely at what a sensible value should be. For now, I'm only interested in RX data. Open to suggestions ;-)

>I tested your proposal and it seems we need some logic to adjust the RXTL value.
>
>For console operation we need it to be RXTL, otherwise we miss echoing the chars.
Yes. We added a check to see if it was a console for our test - leaving it set to 1 for a console tty.

>Have you tried using DMA?
Not explicitly - just used opened it as standard serial device. I assumed any DMA logic in the driver would take care of this if needed. If it doesn't, I presume it's still an issue right? Or are you saying that this needs testing separately? I guess this is kinda where I'm coming from... is there a known reason why it is the way it is?

I ought to add that I'm testing on an old kernel  (3.0.35) so driver operation may differ w.r.t DMA. The RXTL=1 setting has been around for a while but maybe other things work slightly differently. Code looks v.similar  at an overview level.

regards,
Will  

Re: imx.c RXTL

[Nikolay Dimitrov]

Hi guys,

On 04/29/2015 08:00 PM, Frazer, Will wrote:
> I ought to add that I'm testing on an old kernel  (3.0.35) so driver
> operation may differ w.r.t DMA. The RXTL=1 setting has been around
> for a while but maybe other things work slightly differently. Code
> looks v.similar  at an overview level.

Sorry, just saw this, so probably some of my comments are not relevant.

Regards,
Nikolay

Re: imx.c RXTL

[Frazer, Will]

Hi Nikolay,


On 04/29/2015 08:00 PM, Frazer, Will wrote:
> I ought to add that I'm testing on an old kernel  (3.0.35) so driver 
> operation may differ w.r.t DMA. The RXTL=1 setting has been around for 
> a while but maybe other things work slightly differently. Code looks 
> v.similar  at an overview level.

> Sorry, just saw this, so probably some of my comments are not relevant.
I think you got it right enough for me & ties in roughly with what was done to handle it here.

...
>#define RXTL 1 /* For console port */
>#define RXTL_UART 16 /* For uart */

 Looks like this did get fixed upstream which I somehow missed.

Thanks,
Will