Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-29 Thread Brian J. Johnson 

On 03/28/2016 10:06 PM, Heyi Guo wrote:



On 03/25/2016 01:56 PM, Ard Biesheuvel wrote:

On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:

Heyi,
I was not able to remove the blue bar in the beginning of every line
if I embedded my reply.
So I will directly write down here.


1.  I would like to know in what circumstance the key loss happens.
Because your UART hardware has 32 FIFO buffer to receive the keys.
A typical escape keys (corresponding to Arrow Left, Arrow Right, or
F1-F12) combination
would only need 3-4 bytes. The FIFO buffer is sufficient to hold
about 10
combinations.


The loss of input characters occurs typically when you copy-paste
something into the terminal window.


Thanks Ard for helping me to reply; that's really the case I want to fix :)



Can you enable hardware flow control (RTS/CTS)?  That's the only way 
I've found to make serial links reliable.





2.  Your system's timer hardware is fast enough so your timer
callback can be triggered
every 2 (2.78) ms, then terminal driver should be able to capture all
the escape keys.
Now since terminal driver cannot capture all the escape keys, so you
guess the callback is
not triggered every 2ms.
Can you find a way to prove it? for example, save the time every time
the callback is called.
Then after like 100 rounds of callback, dump all the 100 time values.
Let's see the real time interval value.

OK, I'll try that and let you know the result.

Thanks and regards.

Heyi



3.  I forget about the FIFO depth change. The FIFO depth change
doesn't cause the device

path reinstallation. I agree we need to detect the FIFO depth change
in timer call back

to update the interval.


Regards,
Ray

From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Thursday, March 24, 2016 10:59 PM
To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set
polling rate by serial IO mode

Hi Ruiyu,

I had seen the other comments and I just needed more time to think
about them :)

Please see my comments below.
On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
Heyi,
I had 7 comments in previous mail. I guess you may miss the other
comments.

What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may
be larger than
what you expected.
We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf
as timer driver.
The frequency of the timer is 200MHz. Timer interrupt period is set
to 1ms.



And can you tell me all the UART parameters in your case? What timer
interval
did you get using the equation?
Baud rate = 115200
FIFO depth = 32
Parity = 0
stop = 1
Data size = 8
Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)

And please continue to see my opinions for other comments below:



Regards,
Ray



-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, March 23, 2016 5:44 PM
To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>;
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>;
Zeng, Star <star.z...@intel.com><mailto:star.z...@intel.com>;
Kinney, Michael D
<michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set
polling rate by serial IO mode

Hi Ruiyu,

Many thanks for your review.

For questions 2#, I tested equation #1 with copy-paste on serial
terminal, and found it still missing some characters when the FIFO was
almost full (i.e. near the end of input). I think it is because time
event is not real-time interrupt; it may be pending for equal or
higher TPL.

And a little faster polling rate tested well on real environment.

Please let me know your suggestion.

Regards.

Heyi

On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:

Heyi,
I have comments in below.

Regards,
Ray



-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On
Behalf Of Heyi Guo
Sent: Thursday, March 17, 2016 10:37 PM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Heyi Guo <heyi@linaro.org><mailto:heyi@linaro.org>;
Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>;
Zeng, Star <star.z...@intel.com><mailto:star.z...@intel.com>
Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set
polling rate by serial IO mode

Calculate serial input polling rate according to parameters from
serial IO mode as below, to fix potential input truncation with fixed
polling interval 0.02s.

Polling interval (100ns) =
FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 /
BaudRate

1. The above equation (let's call it equation #1) looks good.


Sorry I think I missed the start bit, which should alwa

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-29 Thread Ni, Ruiyu 
Heyi,
I just pushed your 1/3 and 2/3 to the trunk because I think:

1.  we may need spend long time to investigate the proper fix for 3/3.

2.  3/3 seems like a standalone patch serial. No close relationship between 
1-2/3 and 3/3.


Regards,
Ray

From: Ni, Ruiyu
Sent: Friday, March 25, 2016 10:52 AM
To: Heyi Guo <heyi@linaro.org>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode

Heyi,
I was not able to remove the blue bar in the beginning of every line if I 
embedded my reply.
So I will directly write down here.


1.  I would like to know in what circumstance the key loss happens.
Because your UART hardware has 32 FIFO buffer to receive the keys.
A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12) 
combination
would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
combinations.


2.  Your system's timer hardware is fast enough so your timer callback can 
be triggered
every 2 (2.78) ms, then terminal driver should be able to capture all the 
escape keys.
Now since terminal driver cannot capture all the escape keys, so you guess the 
callback is
not triggered every 2ms.
Can you find a way to prove it? for example, save the time every time the 
callback is called.
Then after like 100 rounds of callback, dump all the 100 time values.
Let's see the real time interval value.


3.  I forget about the FIFO depth change. The FIFO depth change doesn't 
cause the device

path reinstallation. I agree we need to detect the FIFO depth change in timer 
call back

to update the interval.


Regards,
Ray

From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Thursday, March 24, 2016 10:59 PM
To: Ni, Ruiyu <ruiyu...@intel.com<mailto:ruiyu...@intel.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Tian, Feng <feng.t...@intel.com<mailto:feng.t...@intel.com>>; Zeng, Star 
<star.z...@intel.com<mailto:star.z...@intel.com>>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode

Hi Ruiyu,

I had seen the other comments and I just needed more time to think about them :)

Please see my comments below.
On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
Heyi,
I had 7 comments in previous mail. I guess you may miss the other comments.

What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may be larger 
than
what you expected.
We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as timer 
driver.
The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.


And can you tell me all the UART parameters in your case? What timer interval
did you get using the equation?
Baud rate = 115200
FIFO depth = 32
Parity = 0
stop = 1
Data size = 8
Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)

And please continue to see my opinions for other comments below:


Regards,
Ray


>-Original Message-
>From: Heyi Guo [mailto:heyi@linaro.org]
>Sent: Wednesday, March 23, 2016 5:44 PM
>To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>; 
>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
><star.z...@intel.com><mailto:star.z...@intel.com>; Kinney, Michael D 
><michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com>
>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>by serial IO mode
>
>Hi Ruiyu,
>
>Many thanks for your review.
>
>For questions 2#, I tested equation #1 with copy-paste on serial
>terminal, and found it still missing some characters when the FIFO was
>almost full (i.e. near the end of input). I think it is because time
>event is not real-time interrupt; it may be pending for equal or higher TPL.
>
>And a little faster polling rate tested well on real environment.
>
>Please let me know your suggestion.
>
>Regards.
>
>Heyi
>
>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:
>> Heyi,
>> I have comments in below.
>>
>> Regards,
>> Ray
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
>>> Guo
>>> Sent: Thursday, March 17, 2016 10:37 PM
>>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> Cc: Heyi Guo <heyi@linaro.org><mailto:heyi@linaro.org>; Tian, Feng 
>>> <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
>>> <star.z...@intel.com><mailto:star.z...@intel.com>
>>> Subj

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-29 Thread Ard Biesheuvel 
On 29 March 2016 at 08:26, Kinney, Michael D  wrote:
> Ard,
>
> Many handlers running at the same TPL are ok as long as each of the handlers 
> run for very short periods of time.  Any event handlers that execute as 
> raised TPL for extended periods of time will impact other event handlers.
>

That is *exactly* the point Heyi made before.

> In a worst case scenario, if the execution time at raised TPL in a handler or 
> group of handlers is greater than or equal to their poll rates, then a live 
> lock condition can occur where the code at lower TPLs never gets to make 
> forward progress.
>
> This means that event handlers at raised TPL need to be very efficient and do 
> their work quickly and potentially just set some flags or queue some data for 
> work to be done at a lower TPL at a slower event rate.
>

... to the extent possible, which is limited by the FIFO size in this
case. So we need to poll at a rate which guarantees, based on the baud
rate and the FIFO size, that the number of characters received during
the poll interval cannot exceed the size of the FIFO.

So I think we are all in agreement about the mechanics of TPL
priorities and the underlying problem Heyi is trying to solve.

What I would like to clarify is whether polling at exactly the rate
calculated by Heyi's formula should guarantee us no dropped
characters, and if the observation to the contrary is the result of
flaws in the ARM implementation.

We do account for time spent dispatching events in the timer code when
setting the next timeout, and to some extent, nested interrupts are
supported (i.e., as soon as the TPL is lowered, a new timer interrupt
may fire which completes before the old timer interrupt handling is
fully completed)

Or we may simply have some drivers that are hogging the timer event
and interfering with the polling.

In any case, the copy-paste issue is just a bit annoying, but
considering the above, it may be a symptom of something more severe
going on.

Thanks,
Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-29 Thread Kinney, Michael D 
Ard,

Many handlers running at the same TPL are ok as long as each of the handlers 
run for very short periods of time.  Any event handlers that execute as raised 
TPL for extended periods of time will impact other event handlers.

In a worst case scenario, if the execution time at raised TPL in a handler or 
group of handlers is greater than or equal to their poll rates, then a live 
lock condition can occur where the code at lower TPLs never gets to make 
forward progress.

This means that event handlers at raised TPL need to be very efficient and do 
their work quickly and potentially just set some flags or queue some data for 
work to be done at a lower TPL at a slower event rate.

Mike

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, March 28, 2016 11:08 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Heyi Guo <heyi@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com>; edk2-
> de...@lists.01.org; Tian, Feng <feng.t...@intel.com>; Zeng, Star 
> <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
> by serial
> IO mode
> 
> On 29 March 2016 at 06:09, Kinney, Michael D <michael.d.kin...@intel.com> 
> wrote:
> > Ard,
> >
> > What is the timer rate in the system you are using?
> >
> 
> I am going to let Heyi respond with the actual numbers.
> 
> > The timer event rate of a UEFI driver can not go any faster or a smaller 
> > granularity
> than the system timer rate.
> >
> > The original formula may be correct, but may not be compensating for the 
> > granularity
> of the timer rate.
> >
> > Is the issue also resolved if you increase the system timer tick rate?
> >
> 
> I agree that there is likely a significant quantization error once the
> poll rate approaches the system tick rate. But I think Heyi's analysis
> is correct that handlers executing at the same TPL may cause jitter
> that needs to be compensated for by bias towards zero on the poll
> rate.
> 
> 
> Ard.
> 
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> >> Heyi Guo
> >> Sent: Monday, March 28, 2016 8:06 PM
> >> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Ni, Ruiyu 
> >> <ruiyu...@intel.com>
> >> Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>; Zeng, Star
> >> <star.z...@intel.com>
> >> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling 
> >> rate by
> serial
> >> IO mode
> >>
> >>
> >>
> >> On 03/25/2016 01:56 PM, Ard Biesheuvel wrote:
> >> > On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:
> >> >> Heyi,
> >> >> I was not able to remove the blue bar in the beginning of every line if 
> >> >> I
> embedded
> >> my reply.
> >> >> So I will directly write down here.
> >> >>
> >> >>
> >> >> 1.  I would like to know in what circumstance the key loss happens.
> >> >> Because your UART hardware has 32 FIFO buffer to receive the keys.
> >> >> A typical escape keys (corresponding to Arrow Left, Arrow Right, or 
> >> >> F1-F12)
> >> combination
> >> >> would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 
> >> >> 10
> >> >> combinations.
> >> >>
> >> > The loss of input characters occurs typically when you copy-paste
> >> > something into the terminal window.
> >>
> >> Thanks Ard for helping me to reply; that's really the case I want to fix :)
> >>
> >> >
> >> >> 2.  Your system's timer hardware is fast enough so your timer 
> >> >> callback can be
> >> triggered
> >> >> every 2 (2.78) ms, then terminal driver should be able to capture all 
> >> >> the escape
> >> keys.
> >> >> Now since terminal driver cannot capture all the escape keys, so you 
> >> >> guess the
> >> callback is
> >> >> not triggered every 2ms.
> >> >> Can you find a way to prove it? for example, save the time every time 
> >> >> the
> callback
> >> is called.
> >> >> Then after like 100 rounds of callback, dump all the 100 time values.
> >> >> Let's see the real time interval value.
> >> OK, I'll try that and let you know the result.
> >>
> >> Thank

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-29 Thread Ard Biesheuvel 
On 29 March 2016 at 06:09, Kinney, Michael D <michael.d.kin...@intel.com> wrote:
> Ard,
>
> What is the timer rate in the system you are using?
>

I am going to let Heyi respond with the actual numbers.

> The timer event rate of a UEFI driver can not go any faster or a smaller 
> granularity than the system timer rate.
>
> The original formula may be correct, but may not be compensating for the 
> granularity of the timer rate.
>
> Is the issue also resolved if you increase the system timer tick rate?
>

I agree that there is likely a significant quantization error once the
poll rate approaches the system tick rate. But I think Heyi's analysis
is correct that handlers executing at the same TPL may cause jitter
that needs to be compensated for by bias towards zero on the poll
rate.


Ard.

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
>> Guo
>> Sent: Monday, March 28, 2016 8:06 PM
>> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Ni, Ruiyu 
>> <ruiyu...@intel.com>
>> Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>; Zeng, Star
>> <star.z...@intel.com>
>> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling 
>> rate by serial
>> IO mode
>>
>>
>>
>> On 03/25/2016 01:56 PM, Ard Biesheuvel wrote:
>> > On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:
>> >> Heyi,
>> >> I was not able to remove the blue bar in the beginning of every line if I 
>> >> embedded
>> my reply.
>> >> So I will directly write down here.
>> >>
>> >>
>> >> 1.  I would like to know in what circumstance the key loss happens.
>> >> Because your UART hardware has 32 FIFO buffer to receive the keys.
>> >> A typical escape keys (corresponding to Arrow Left, Arrow Right, or 
>> >> F1-F12)
>> combination
>> >> would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
>> >> combinations.
>> >>
>> > The loss of input characters occurs typically when you copy-paste
>> > something into the terminal window.
>>
>> Thanks Ard for helping me to reply; that's really the case I want to fix :)
>>
>> >
>> >> 2.  Your system's timer hardware is fast enough so your timer 
>> >> callback can be
>> triggered
>> >> every 2 (2.78) ms, then terminal driver should be able to capture all the 
>> >> escape
>> keys.
>> >> Now since terminal driver cannot capture all the escape keys, so you 
>> >> guess the
>> callback is
>> >> not triggered every 2ms.
>> >> Can you find a way to prove it? for example, save the time every time the 
>> >> callback
>> is called.
>> >> Then after like 100 rounds of callback, dump all the 100 time values.
>> >> Let's see the real time interval value.
>> OK, I'll try that and let you know the result.
>>
>> Thanks and regards.
>>
>> Heyi
>>
>> >>
>> >> 3.  I forget about the FIFO depth change. The FIFO depth change 
>> >> doesn't cause
>> the device
>> >>
>> >> path reinstallation. I agree we need to detect the FIFO depth change in 
>> >> timer call
>> back
>> >>
>> >> to update the interval.
>> >>
>> >>
>> >> Regards,
>> >> Ray
>> >>
>> >> From: Heyi Guo [mailto:heyi@linaro.org]
>> >> Sent: Thursday, March 24, 2016 10:59 PM
>> >> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
>> >> Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
>> >> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling 
>> >> rate by
>> serial IO mode
>> >>
>> >> Hi Ruiyu,
>> >>
>> >> I had seen the other comments and I just needed more time to think about 
>> >> them :)
>> >>
>> >> Please see my comments below.
>> >> On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
>> >> Heyi,
>> >> I had 7 comments in previous mail. I guess you may miss the other 
>> >> comments.
>> >>
>> >> What Timer driver are you using? How many ms does one system tick cost?
>> >> If you are using a less-precise timer driver/hardware, one tick may be 
>> >> larger than
>> >> what you expected.

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-28 Thread Kinney, Michael D 
Ard,

What is the timer rate in the system you are using?

The timer event rate of a UEFI driver can not go any faster or a smaller 
granularity than the system timer rate.

The original formula may be correct, but may not be compensating for the 
granularity of the timer rate.

Is the issue also resolved if you increase the system timer tick rate?

Thanks,

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
> Guo
> Sent: Monday, March 28, 2016 8:06 PM
> To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com>
> Cc: edk2-devel@lists.01.org; Tian, Feng <feng.t...@intel.com>; Zeng, Star
> <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
> by serial
> IO mode
> 
> 
> 
> On 03/25/2016 01:56 PM, Ard Biesheuvel wrote:
> > On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:
> >> Heyi,
> >> I was not able to remove the blue bar in the beginning of every line if I 
> >> embedded
> my reply.
> >> So I will directly write down here.
> >>
> >>
> >> 1.  I would like to know in what circumstance the key loss happens.
> >> Because your UART hardware has 32 FIFO buffer to receive the keys.
> >> A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12)
> combination
> >> would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
> >> combinations.
> >>
> > The loss of input characters occurs typically when you copy-paste
> > something into the terminal window.
> 
> Thanks Ard for helping me to reply; that's really the case I want to fix :)
> 
> >
> >> 2.  Your system's timer hardware is fast enough so your timer callback 
> >> can be
> triggered
> >> every 2 (2.78) ms, then terminal driver should be able to capture all the 
> >> escape
> keys.
> >> Now since terminal driver cannot capture all the escape keys, so you guess 
> >> the
> callback is
> >> not triggered every 2ms.
> >> Can you find a way to prove it? for example, save the time every time the 
> >> callback
> is called.
> >> Then after like 100 rounds of callback, dump all the 100 time values.
> >> Let's see the real time interval value.
> OK, I'll try that and let you know the result.
> 
> Thanks and regards.
> 
> Heyi
> 
> >>
> >> 3.  I forget about the FIFO depth change. The FIFO depth change 
> >> doesn't cause
> the device
> >>
> >> path reinstallation. I agree we need to detect the FIFO depth change in 
> >> timer call
> back
> >>
> >> to update the interval.
> >>
> >>
> >> Regards,
> >> Ray
> >>
> >> From: Heyi Guo [mailto:heyi@linaro.org]
> >> Sent: Thursday, March 24, 2016 10:59 PM
> >> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> >> Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
> >> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling 
> >> rate by
> serial IO mode
> >>
> >> Hi Ruiyu,
> >>
> >> I had seen the other comments and I just needed more time to think about 
> >> them :)
> >>
> >> Please see my comments below.
> >> On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
> >> Heyi,
> >> I had 7 comments in previous mail. I guess you may miss the other comments.
> >>
> >> What Timer driver are you using? How many ms does one system tick cost?
> >> If you are using a less-precise timer driver/hardware, one tick may be 
> >> larger than
> >> what you expected.
> >> We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as 
> >> timer driver.
> >> The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.
> >>
> >>
> >>
> >> And can you tell me all the UART parameters in your case? What timer 
> >> interval
> >> did you get using the equation?
> >> Baud rate = 115200
> >> FIFO depth = 32
> >> Parity = 0
> >> stop = 1
> >> Data size = 8
> >> Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)
> >>
> >> And please continue to see my opinions for other comments below:
> >>
> >>
> >>
> >> Regards,
> >> Ray
> >>
> >>
> >>> -Original Message-
> >>> From

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-28 Thread Heyi Guo 



On 03/25/2016 01:56 PM, Ard Biesheuvel wrote:

On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:

Heyi,
I was not able to remove the blue bar in the beginning of every line if I 
embedded my reply.
So I will directly write down here.


1.  I would like to know in what circumstance the key loss happens.
Because your UART hardware has 32 FIFO buffer to receive the keys.
A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12) 
combination
would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
combinations.


The loss of input characters occurs typically when you copy-paste
something into the terminal window.


Thanks Ard for helping me to reply; that's really the case I want to fix :)




2.  Your system's timer hardware is fast enough so your timer callback can 
be triggered
every 2 (2.78) ms, then terminal driver should be able to capture all the 
escape keys.
Now since terminal driver cannot capture all the escape keys, so you guess the 
callback is
not triggered every 2ms.
Can you find a way to prove it? for example, save the time every time the 
callback is called.
Then after like 100 rounds of callback, dump all the 100 time values.
Let's see the real time interval value.

OK, I'll try that and let you know the result.

Thanks and regards.

Heyi



3.  I forget about the FIFO depth change. The FIFO depth change doesn't 
cause the device

path reinstallation. I agree we need to detect the FIFO depth change in timer 
call back

to update the interval.


Regards,
Ray

From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Thursday, March 24, 2016 10:59 PM
To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode

Hi Ruiyu,

I had seen the other comments and I just needed more time to think about them :)

Please see my comments below.
On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
Heyi,
I had 7 comments in previous mail. I guess you may miss the other comments.

What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may be larger 
than
what you expected.
We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as timer 
driver.
The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.



And can you tell me all the UART parameters in your case? What timer interval
did you get using the equation?
Baud rate = 115200
FIFO depth = 32
Parity = 0
stop = 1
Data size = 8
Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)

And please continue to see my opinions for other comments below:



Regards,
Ray



-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Wednesday, March 23, 2016 5:44 PM
To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
<star.z...@intel.com><mailto:star.z...@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode

Hi Ruiyu,

Many thanks for your review.

For questions 2#, I tested equation #1 with copy-paste on serial
terminal, and found it still missing some characters when the FIFO was
almost full (i.e. near the end of input). I think it is because time
event is not real-time interrupt; it may be pending for equal or higher TPL.

And a little faster polling rate tested well on real environment.

Please let me know your suggestion.

Regards.

Heyi

On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:

Heyi,
I have comments in below.

Regards,
Ray



-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi Guo
Sent: Thursday, March 17, 2016 10:37 PM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Heyi Guo <heyi@linaro.org><mailto:heyi@linaro.org>; Tian, Feng 
<feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
<star.z...@intel.com><mailto:star.z...@intel.com>
Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by 
serial IO mode

Calculate serial input polling rate according to parameters from
serial IO mode as below, to fix potential input truncation with fixed
polling interval 0.02s.

Polling interval (100ns) =
FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate

1. The above equation (let's call it equation #1) looks good.


Sorry I think I missed the start bit, which should always be 1 bit.



However, as UEFI events will probably delayed by other code of higher
TPL, we use below equation to make polling rate f

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-24 Thread Ard Biesheuvel 
On 25 March 2016 at 03:52, Ni, Ruiyu <ruiyu...@intel.com> wrote:
> Heyi,
> I was not able to remove the blue bar in the beginning of every line if I 
> embedded my reply.
> So I will directly write down here.
>
>
> 1.  I would like to know in what circumstance the key loss happens.
> Because your UART hardware has 32 FIFO buffer to receive the keys.
> A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12) 
> combination
> would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
> combinations.
>

The loss of input characters occurs typically when you copy-paste
something into the terminal window.

>
> 2.  Your system's timer hardware is fast enough so your timer callback 
> can be triggered
> every 2 (2.78) ms, then terminal driver should be able to capture all the 
> escape keys.
> Now since terminal driver cannot capture all the escape keys, so you guess 
> the callback is
> not triggered every 2ms.
> Can you find a way to prove it? for example, save the time every time the 
> callback is called.
> Then after like 100 rounds of callback, dump all the 100 time values.
> Let's see the real time interval value.
>
>
> 3.  I forget about the FIFO depth change. The FIFO depth change doesn't 
> cause the device
>
> path reinstallation. I agree we need to detect the FIFO depth change in timer 
> call back
>
> to update the interval.
>
>
> Regards,
> Ray
>
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Thursday, March 24, 2016 10:59 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
> by serial IO mode
>
> Hi Ruiyu,
>
> I had seen the other comments and I just needed more time to think about them 
> :)
>
> Please see my comments below.
> On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
> Heyi,
> I had 7 comments in previous mail. I guess you may miss the other comments.
>
> What Timer driver are you using? How many ms does one system tick cost?
> If you are using a less-precise timer driver/hardware, one tick may be larger 
> than
> what you expected.
> We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as timer 
> driver.
> The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.
>
>
>
> And can you tell me all the UART parameters in your case? What timer interval
> did you get using the equation?
> Baud rate = 115200
> FIFO depth = 32
> Parity = 0
> stop = 1
> Data size = 8
> Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)
>
> And please continue to see my opinions for other comments below:
>
>
>
> Regards,
> Ray
>
>
>>-Original Message-
>>From: Heyi Guo [mailto:heyi@linaro.org]
>>Sent: Wednesday, March 23, 2016 5:44 PM
>>To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>; 
>>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
>><star.z...@intel.com><mailto:star.z...@intel.com>; Kinney, Michael D 
>><michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com>
>>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>>by serial IO mode
>>
>>Hi Ruiyu,
>>
>>Many thanks for your review.
>>
>>For questions 2#, I tested equation #1 with copy-paste on serial
>>terminal, and found it still missing some characters when the FIFO was
>>almost full (i.e. near the end of input). I think it is because time
>>event is not real-time interrupt; it may be pending for equal or higher TPL.
>>
>>And a little faster polling rate tested well on real environment.
>>
>>Please let me know your suggestion.
>>
>>Regards.
>>
>>Heyi
>>
>>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:
>>> Heyi,
>>> I have comments in below.
>>>
>>> Regards,
>>> Ray
>>>
>>>
>>>> -Original Message-
>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>>> Heyi Guo
>>>> Sent: Thursday, March 17, 2016 10:37 PM
>>>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>>> Cc: Heyi Guo <heyi@linaro.org><mailto:heyi@linaro.org>; Tian, Feng 
>>>> <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
>>>> <star.z...@intel.com><

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-24 Thread Ni, Ruiyu 
Heyi,
I was not able to remove the blue bar in the beginning of every line if I 
embedded my reply.
So I will directly write down here.


1.  I would like to know in what circumstance the key loss happens.
Because your UART hardware has 32 FIFO buffer to receive the keys.
A typical escape keys (corresponding to Arrow Left, Arrow Right, or F1-F12) 
combination
would only need 3-4 bytes. The FIFO buffer is sufficient to hold about 10
combinations.


2.  Your system's timer hardware is fast enough so your timer callback can 
be triggered
every 2 (2.78) ms, then terminal driver should be able to capture all the 
escape keys.
Now since terminal driver cannot capture all the escape keys, so you guess the 
callback is
not triggered every 2ms.
Can you find a way to prove it? for example, save the time every time the 
callback is called.
Then after like 100 rounds of callback, dump all the 100 time values.
Let's see the real time interval value.


3.  I forget about the FIFO depth change. The FIFO depth change doesn't 
cause the device

path reinstallation. I agree we need to detect the FIFO depth change in timer 
call back

to update the interval.


Regards,
Ray

From: Heyi Guo [mailto:heyi@linaro.org]
Sent: Thursday, March 24, 2016 10:59 PM
To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode

Hi Ruiyu,

I had seen the other comments and I just needed more time to think about them :)

Please see my comments below.
On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:
Heyi,
I had 7 comments in previous mail. I guess you may miss the other comments.

What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may be larger 
than
what you expected.
We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as timer 
driver.
The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.



And can you tell me all the UART parameters in your case? What timer interval
did you get using the equation?
Baud rate = 115200
FIFO depth = 32
Parity = 0
stop = 1
Data size = 8
Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)

And please continue to see my opinions for other comments below:



Regards,
Ray


>-Original Message-
>From: Heyi Guo [mailto:heyi@linaro.org]
>Sent: Wednesday, March 23, 2016 5:44 PM
>To: Ni, Ruiyu <ruiyu...@intel.com><mailto:ruiyu...@intel.com>; 
>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>Cc: Tian, Feng <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
><star.z...@intel.com><mailto:star.z...@intel.com>; Kinney, Michael D 
><michael.d.kin...@intel.com><mailto:michael.d.kin...@intel.com>
>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>by serial IO mode
>
>Hi Ruiyu,
>
>Many thanks for your review.
>
>For questions 2#, I tested equation #1 with copy-paste on serial
>terminal, and found it still missing some characters when the FIFO was
>almost full (i.e. near the end of input). I think it is because time
>event is not real-time interrupt; it may be pending for equal or higher TPL.
>
>And a little faster polling rate tested well on real environment.
>
>Please let me know your suggestion.
>
>Regards.
>
>Heyi
>
>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:
>> Heyi,
>> I have comments in below.
>>
>> Regards,
>> Ray
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
>>> Guo
>>> Sent: Thursday, March 17, 2016 10:37 PM
>>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>>> Cc: Heyi Guo <heyi....@linaro.org><mailto:heyi@linaro.org>; Tian, Feng 
>>> <feng.t...@intel.com><mailto:feng.t...@intel.com>; Zeng, Star 
>>> <star.z...@intel.com><mailto:star.z...@intel.com>
>>> Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>>> by serial IO mode
>>>
>>> Calculate serial input polling rate according to parameters from
>>> serial IO mode as below, to fix potential input truncation with fixed
>>> polling interval 0.02s.
>>>
>>> Polling interval (100ns) =
>>> FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate
>> 1. The above equation (let's call it equation #1) looks good.
>>
Sorry I think I missed the start bit, which should always be 1 bit.


>>> However, as UEFI events will probably delayed by other code of higher
>>>

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-24 Thread Heyi Guo 

Hi Ruiyu,

I had seen the other comments and I just needed more time to think about 
them :)


Please see my comments below.

On 03/24/2016 03:26 PM, Ni, Ruiyu wrote:


Heyi,

I had 7 comments in previous mail. I guess you may miss the other 
comments.



What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may be larger 
than


what you expected.

We are using ArmPlatformPkg/Drivers/SP804TimerDxe/SP804TimerDxe.inf as 
timer driver.

The frequency of the timer is 200MHz. Timer interrupt period is set to 1ms.

And can you tell me all the UART parameters in your case? What timer 
interval


did you get using the equation?


Baud rate = 115200
FIFO depth = 32
Parity = 0
stop = 1
Data size = 8
Polling interval = (1 + 8 + 0 + 1) * 32 / 115200 = 0.00278 (2.78ms)

And please continue to see my opinions for other comments below:


Regards,
Ray


>-Original Message-
>From: Heyi Guo [mailto:heyi@linaro.org]
>Sent: Wednesday, March 23, 2016 5:44 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
>Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>; Kinney, 
Michael D <michael.d.kin...@intel.com>
>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode
>
>Hi Ruiyu,
>
>Many thanks for your review.
>
>For questions 2#, I tested equation #1 with copy-paste on serial
>terminal, and found it still missing some characters when the FIFO was
>almost full (i.e. near the end of input). I think it is because time
>event is not real-time interrupt; it may be pending for equal or higher TPL.
>
>And a little faster polling rate tested well on real environment.
>
>Please let me know your suggestion.
>
>Regards.
>
>Heyi
>
>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:
>> Heyi,
>> I have comments in below.
>>
>> Regards,
>> Ray
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
Guo
>>> Sent: Thursday, March 17, 2016 10:37 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Heyi Guo <heyi@linaro.org>; Tian, Feng <feng.t...@intel.com>; Zeng, Star 
<star.z...@intel.com>
>>> Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
by serial IO mode
>>>
>>> Calculate serial input polling rate according to parameters from
>>> serial IO mode as below, to fix potential input truncation with fixed
>>> polling interval 0.02s.
>>>
>>> Polling interval (100ns) =
>>> FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate
>> 1. The above equation (let's call it equation #1) looks good.
>>


Sorry I think I missed the start bit, which should always be 1 bit.


>>> However, as UEFI events will probably delayed by other code of higher
>>> TPL, we use below equation to make polling rate fast enough:
>>>
>>> FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3)
>> 2. Can you tell me why you multiple two in the left of "/" and multiple 3 in 
the right?
>> Did you meet any real problem when using the original equation (#1)?
>>
>> I have more comments in below.
>>
>>> Signed-off-by: Heyi Guo <heyi@linaro.org>
>>> Cc: Feng Tian <feng.t...@intel.com>
>>> Cc: Star Zeng <star.z...@intel.com>
>>> ---
>>> .../Universal/Console/TerminalDxe/Terminal.c   |  5 +-
>>> .../Universal/Console/TerminalDxe/Terminal.h   | 27 -
>>> .../Universal/Console/TerminalDxe/TerminalConIn.c  | 68 
++
>>> 3 files changed, 98 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> index 5adaa97..db790f3 100644
>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> @@ -71,6 +71,7 @@ TERMINAL_DEV  mTerminalDevTemplate = {
>>>},
>>>NULL, // TerminalConsoleModeData
>>>0,  // SerialInTimeOut
>>> +  DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval
>> 3. Can you remove the default timer interval?
>> We should be able to calculate the timer interval from the default
>> setting (DataBits, StopBits, BaudRate, etc).
>>


See below.


>>>NULL, // RawFifo
>>>NULL, // UnicodeFiFo
>>> @@ -984,10 +985,12 @@ TerminalDriverBindingStart (
>>>  );
>>>  AS

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-24 Thread Ni, Ruiyu 
Heyi,
I had 7 comments in previous mail. I guess you may miss the other comments.

What Timer driver are you using? How many ms does one system tick cost?
If you are using a less-precise timer driver/hardware, one tick may be larger 
than
what you expected.

And can you tell me all the UART parameters in your case? What timer interval
did you get using the equation?

Regards,
Ray


>-Original Message-
>From: Heyi Guo [mailto:heyi@linaro.org]
>Sent: Wednesday, March 23, 2016 5:44 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
>Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>; 
>Kinney, Michael D <michael.d.kin...@intel.com>
>Subject: Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>by serial IO mode
>
>Hi Ruiyu,
>
>Many thanks for your review.
>
>For questions 2#, I tested equation #1 with copy-paste on serial
>terminal, and found it still missing some characters when the FIFO was
>almost full (i.e. near the end of input). I think it is because time
>event is not real-time interrupt; it may be pending for equal or higher TPL.
>
>And a little faster polling rate tested well on real environment.
>
>Please let me know your suggestion.
>
>Regards.
>
>Heyi
>
>On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:
>> Heyi,
>> I have comments in below.
>>
>> Regards,
>> Ray
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi 
>>> Guo
>>> Sent: Thursday, March 17, 2016 10:37 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Heyi Guo <heyi@linaro.org>; Tian, Feng <feng.t...@intel.com>; Zeng, 
>>> Star <star.z...@intel.com>
>>> Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate 
>>> by serial IO mode
>>>
>>> Calculate serial input polling rate according to parameters from
>>> serial IO mode as below, to fix potential input truncation with fixed
>>> polling interval 0.02s.
>>>
>>> Polling interval (100ns) =
>>> FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate
>> 1. The above equation (let's call it equation #1) looks good.
>>
>>> However, as UEFI events will probably delayed by other code of higher
>>> TPL, we use below equation to make polling rate fast enough:
>>>
>>> FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3)
>> 2. Can you tell me why you multiple two in the left of "/" and multiple 3 in 
>> the right?
>> Did you meet any real problem when using the original equation (#1)?
>>
>> I have more comments in below.
>>
>>> Signed-off-by: Heyi Guo <heyi@linaro.org>
>>> Cc: Feng Tian <feng.t...@intel.com>
>>> Cc: Star Zeng <star.z...@intel.com>
>>> ---
>>> .../Universal/Console/TerminalDxe/Terminal.c   |  5 +-
>>> .../Universal/Console/TerminalDxe/Terminal.h   | 27 -
>>> .../Universal/Console/TerminalDxe/TerminalConIn.c  | 68 
>>> ++
>>> 3 files changed, 98 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> index 5adaa97..db790f3 100644
>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>>> @@ -71,6 +71,7 @@ TERMINAL_DEV  mTerminalDevTemplate = {
>>>},
>>>NULL, // TerminalConsoleModeData
>>>0,  // SerialInTimeOut
>>> +  DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval
>> 3. Can you remove the default timer interval?
>> We should be able to calculate the timer interval from the default
>> setting (DataBits, StopBits, BaudRate, etc).
>>
>>>NULL, // RawFifo
>>>NULL, // UnicodeFiFo
>>> @@ -984,10 +985,12 @@ TerminalDriverBindingStart (
>>>  );
>>>  ASSERT_EFI_ERROR (Status);
>>>
>>> +TerminalDevice->KeyboardTimerInterval = GetKeyboardTimerInterval 
>>> (Mode);
>>> +
>>>  Status = gBS->SetTimer (
>>>  TerminalDevice->TimerEvent,
>>>  TimerPeriodic,
>>> -KEYBOARD_TIMER_INTERVAL
>>> +TerminalDevice->KeyboardTimerInterval
>>>  );
>>>  ASSERT_EFI_ERROR (Status);
>>>
>>>

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-23 Thread Heyi Guo 

Hi Ruiyu,

Many thanks for your review.

For questions 2#, I tested equation #1 with copy-paste on serial 
terminal, and found it still missing some characters when the FIFO was 
almost full (i.e. near the end of input). I think it is because time 
event is not real-time interrupt; it may be pending for equal or higher TPL.


And a little faster polling rate tested well on real environment.

Please let me know your suggestion.

Regards.

Heyi

On 03/23/2016 04:03 PM, Ni, Ruiyu wrote:

Heyi,
I have comments in below.

Regards,
Ray



-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi Guo
Sent: Thursday, March 17, 2016 10:37 PM
To: edk2-devel@lists.01.org
Cc: Heyi Guo <heyi@linaro.org>; Tian, Feng <feng.t...@intel.com>; Zeng, Star 
<star.z...@intel.com>
Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by 
serial IO mode

Calculate serial input polling rate according to parameters from
serial IO mode as below, to fix potential input truncation with fixed
polling interval 0.02s.

Polling interval (100ns) =
FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate

1. The above equation (let's call it equation #1) looks good.


However, as UEFI events will probably delayed by other code of higher
TPL, we use below equation to make polling rate fast enough:

FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3)

2. Can you tell me why you multiple two in the left of "/" and multiple 3 in 
the right?
Did you meet any real problem when using the original equation (#1)?

I have more comments in below.


Signed-off-by: Heyi Guo <heyi@linaro.org>
Cc: Feng Tian <feng.t...@intel.com>
Cc: Star Zeng <star.z...@intel.com>
---
.../Universal/Console/TerminalDxe/Terminal.c   |  5 +-
.../Universal/Console/TerminalDxe/Terminal.h   | 27 -
.../Universal/Console/TerminalDxe/TerminalConIn.c  | 68 ++
3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 5adaa97..db790f3 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -71,6 +71,7 @@ TERMINAL_DEV  mTerminalDevTemplate = {
   },
   NULL, // TerminalConsoleModeData
   0,  // SerialInTimeOut
+  DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval

3. Can you remove the default timer interval?
We should be able to calculate the timer interval from the default
setting (DataBits, StopBits, BaudRate, etc).


   NULL, // RawFifo
   NULL, // UnicodeFiFo
@@ -984,10 +985,12 @@ TerminalDriverBindingStart (
 );
 ASSERT_EFI_ERROR (Status);

+TerminalDevice->KeyboardTimerInterval = GetKeyboardTimerInterval (Mode);
+
 Status = gBS->SetTimer (
 TerminalDevice->TimerEvent,
 TimerPeriodic,
-KEYBOARD_TIMER_INTERVAL
+TerminalDevice->KeyboardTimerInterval
 );
 ASSERT_EFI_ERROR (Status);

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
index 269d2ae..58d5664 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
@@ -68,7 +68,7 @@ typedef struct {
   UINTN   Rows;
} TERMINAL_CONSOLE_MODE_DATA;

-#define KEYBOARD_TIMER_INTERVAL 20  // 0.02s
+#define DEFAULT_KEYBOARD_TIMER_INTERVAL 20  // 0.02s


4. Same question as #3.


#define TERMINAL_DEV_SIGNATURE  SIGNATURE_32 ('t', 'm', 'n', 'l')

@@ -91,6 +91,7 @@ typedef struct {
   EFI_SIMPLE_TEXT_OUTPUT_MODE SimpleTextOutputMode;
   TERMINAL_CONSOLE_MODE_DATA  *TerminalConsoleModeData;
   UINTN   SerialInTimeOut;
+  UINT64  KeyboardTimerInterval;
   RAW_DATA_FIFO   *RawFiFo;
   UNICODE_FIFO*UnicodeFiFo;
   EFI_KEY_FIFO*EfiKeyFiFo;
@@ -1358,4 +1359,28 @@ TerminalConInTimerHandler (
   IN EFI_EVENTEvent,
   IN VOID *Context
   );
+
+/**
+  Calculate input polling timer interval by serial IO mode.
+
+  @param  Mode  Pointer to serial IO mode.
+
+  @retval The required polling timer interval in 100ns.
+
+**/
+UINT64
+GetKeyboardTimerInterval (
+  IN EFI_SERIAL_IO_MODE   *Mode
+  );
+
+/**
+  Update period of polling timer event.
+
+  @param  TerminalDeviceThe terminal device to update.
+**/
+VOID
+UpdatePollingRate (
+  IN TERMINAL_DEV *TerminalDevice
+  );
+
#endif
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 2215df6..22349a0 100644
--- a/MdeModulePkg/Universal/Console/Termin

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-23 Thread Ni, Ruiyu 
Heyi,
I have comments in below.

Regards,
Ray


>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi Guo
>Sent: Thursday, March 17, 2016 10:37 PM
>To: edk2-devel@lists.01.org
>Cc: Heyi Guo <heyi@linaro.org>; Tian, Feng <feng.t...@intel.com>; Zeng, 
>Star <star.z...@intel.com>
>Subject: [edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by 
>serial IO mode
>
>Calculate serial input polling rate according to parameters from
>serial IO mode as below, to fix potential input truncation with fixed
>polling interval 0.02s.
>
>Polling interval (100ns) =
>FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate

1. The above equation (let's call it equation #1) looks good.

>
>However, as UEFI events will probably delayed by other code of higher
>TPL, we use below equation to make polling rate fast enough:
>
>FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3)

2. Can you tell me why you multiple two in the left of "/" and multiple 3 in 
the right?
Did you meet any real problem when using the original equation (#1)?

I have more comments in below.

>
>Signed-off-by: Heyi Guo <heyi@linaro.org>
>Cc: Feng Tian <feng.t...@intel.com>
>Cc: Star Zeng <star.z...@intel.com>
>---
> .../Universal/Console/TerminalDxe/Terminal.c   |  5 +-
> .../Universal/Console/TerminalDxe/Terminal.h   | 27 -
> .../Universal/Console/TerminalDxe/TerminalConIn.c  | 68 ++
> 3 files changed, 98 insertions(+), 2 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>index 5adaa97..db790f3 100644
>--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
>@@ -71,6 +71,7 @@ TERMINAL_DEV  mTerminalDevTemplate = {
>   },
>   NULL, // TerminalConsoleModeData
>   0,  // SerialInTimeOut
>+  DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval

3. Can you remove the default timer interval?
We should be able to calculate the timer interval from the default
setting (DataBits, StopBits, BaudRate, etc).

>
>   NULL, // RawFifo
>   NULL, // UnicodeFiFo
>@@ -984,10 +985,12 @@ TerminalDriverBindingStart (
> );
> ASSERT_EFI_ERROR (Status);
>
>+TerminalDevice->KeyboardTimerInterval = GetKeyboardTimerInterval (Mode);
>+
> Status = gBS->SetTimer (
> TerminalDevice->TimerEvent,
> TimerPeriodic,
>-KEYBOARD_TIMER_INTERVAL
>+TerminalDevice->KeyboardTimerInterval
> );
> ASSERT_EFI_ERROR (Status);
>
>diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>index 269d2ae..58d5664 100644
>--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
>@@ -68,7 +68,7 @@ typedef struct {
>   UINTN   Rows;
> } TERMINAL_CONSOLE_MODE_DATA;
>
>-#define KEYBOARD_TIMER_INTERVAL 20  // 0.02s
>+#define DEFAULT_KEYBOARD_TIMER_INTERVAL 20  // 0.02s


4. Same question as #3.

>
> #define TERMINAL_DEV_SIGNATURE  SIGNATURE_32 ('t', 'm', 'n', 'l')
>
>@@ -91,6 +91,7 @@ typedef struct {
>   EFI_SIMPLE_TEXT_OUTPUT_MODE SimpleTextOutputMode;
>   TERMINAL_CONSOLE_MODE_DATA  *TerminalConsoleModeData;
>   UINTN   SerialInTimeOut;
>+  UINT64  KeyboardTimerInterval;
>   RAW_DATA_FIFO   *RawFiFo;
>   UNICODE_FIFO*UnicodeFiFo;
>   EFI_KEY_FIFO*EfiKeyFiFo;
>@@ -1358,4 +1359,28 @@ TerminalConInTimerHandler (
>   IN EFI_EVENTEvent,
>   IN VOID *Context
>   );
>+
>+/**
>+  Calculate input polling timer interval by serial IO mode.
>+
>+  @param  Mode  Pointer to serial IO mode.
>+
>+  @retval The required polling timer interval in 100ns.
>+
>+**/
>+UINT64
>+GetKeyboardTimerInterval (
>+  IN EFI_SERIAL_IO_MODE   *Mode
>+  );
>+
>+/**
>+  Update period of polling timer event.
>+
>+  @param  TerminalDeviceThe terminal device to update.
>+**/
>+VOID
>+UpdatePollingRate (
>+  IN TERMINAL_DEV *TerminalDevice
>+  );
>+
> #endif
>diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>index 2215df6..22349a0 100644
>--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
>+++ b/MdeModul

[edk2] [PATCH v2 3/3] MdeModulePkg/TerminalDxe: Set polling rate by serial IO mode

2016-03-20 Thread Heyi Guo 
Calculate serial input polling rate according to parameters from
serial IO mode as below, to fix potential input truncation with fixed
polling interval 0.02s.

Polling interval (100ns) =
FifoDepth * (ParityBits + StopBits + DataBits) * 10,000,000 / BaudRate

However, as UEFI events will probably delayed by other code of higher
TPL, we use below equation to make polling rate fast enough:

FifoDepth * DataBits * 10,000,000 * 2 / (BaudRate * 3)

Signed-off-by: Heyi Guo 
Cc: Feng Tian 
Cc: Star Zeng 
---
 .../Universal/Console/TerminalDxe/Terminal.c   |  5 +-
 .../Universal/Console/TerminalDxe/Terminal.h   | 27 -
 .../Universal/Console/TerminalDxe/TerminalConIn.c  | 68 ++
 3 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c 
b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index 5adaa97..db790f3 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -71,6 +71,7 @@ TERMINAL_DEV  mTerminalDevTemplate = {
   },
   NULL, // TerminalConsoleModeData
   0,  // SerialInTimeOut
+  DEFAULT_KEYBOARD_TIMER_INTERVAL, // KeyboardTimerInterval
 
   NULL, // RawFifo
   NULL, // UnicodeFiFo
@@ -984,10 +985,12 @@ TerminalDriverBindingStart (
 );
 ASSERT_EFI_ERROR (Status);
 
+TerminalDevice->KeyboardTimerInterval = GetKeyboardTimerInterval (Mode);
+
 Status = gBS->SetTimer (
 TerminalDevice->TimerEvent,
 TimerPeriodic,
-KEYBOARD_TIMER_INTERVAL
+TerminalDevice->KeyboardTimerInterval
 );
 ASSERT_EFI_ERROR (Status);
 
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h 
b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
index 269d2ae..58d5664 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h
@@ -68,7 +68,7 @@ typedef struct {
   UINTN   Rows;
 } TERMINAL_CONSOLE_MODE_DATA;
 
-#define KEYBOARD_TIMER_INTERVAL 20  // 0.02s
+#define DEFAULT_KEYBOARD_TIMER_INTERVAL 20  // 0.02s
 
 #define TERMINAL_DEV_SIGNATURE  SIGNATURE_32 ('t', 'm', 'n', 'l')
 
@@ -91,6 +91,7 @@ typedef struct {
   EFI_SIMPLE_TEXT_OUTPUT_MODE SimpleTextOutputMode;
   TERMINAL_CONSOLE_MODE_DATA  *TerminalConsoleModeData;
   UINTN   SerialInTimeOut;
+  UINT64  KeyboardTimerInterval;
   RAW_DATA_FIFO   *RawFiFo;
   UNICODE_FIFO*UnicodeFiFo;
   EFI_KEY_FIFO*EfiKeyFiFo;
@@ -1358,4 +1359,28 @@ TerminalConInTimerHandler (
   IN EFI_EVENTEvent,
   IN VOID *Context
   );
+
+/**
+  Calculate input polling timer interval by serial IO mode.
+
+  @param  Mode  Pointer to serial IO mode.
+
+  @retval The required polling timer interval in 100ns.
+
+**/
+UINT64
+GetKeyboardTimerInterval (
+  IN EFI_SERIAL_IO_MODE   *Mode
+  );
+
+/**
+  Update period of polling timer event.
+
+  @param  TerminalDeviceThe terminal device to update.
+**/
+VOID
+UpdatePollingRate (
+  IN TERMINAL_DEV *TerminalDevice
+  );
+
 #endif
diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c 
b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
index 2215df6..22349a0 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c
@@ -502,6 +502,71 @@ TerminalConInWaitForKey (
 }
 
 /**
+  Calculate input polling timer interval by serial IO mode.
+
+  @param  Mode  Pointer to serial IO mode.
+
+  @retval The required polling timer interval in 100ns.
+
+**/
+UINT64
+GetKeyboardTimerInterval (
+  IN EFI_SERIAL_IO_MODE *Mode
+  )
+{
+  UINT32FifoDepth;
+
+  if (Mode->BaudRate == 0) {
+return DEFAULT_KEYBOARD_TIMER_INTERVAL;
+  }
+
+  FifoDepth = Mode->ReceiveFifoDepth;
+  // Fix incorrect FIFO depth
+  if (FifoDepth == 0) {
+FifoDepth = 1;
+  }
+
+  // We ignore stop bits and parity bit, and reduce the interval by 1/3,
+  // to make polling rate fast enough to avoid serial input truncation.
+  return DivU64x64Remainder (
+FifoDepth * Mode->DataBits * 1000 * 2,
+Mode->BaudRate * 3,
+NULL
+);
+}
+
+
+/**
+  Update period of polling timer event.
+
+  @param  TerminalDeviceThe terminal device to update.
+**/
+VOID
+UpdatePollingRate (
+  IN TERMINAL_DEV *TerminalDevice
+  )
+{
+  UINT64  NewInterval;
+  EFI_STATUS  Status;
+
+  NewInterval = GetKeyboardTimerInterval (TerminalDevice->SerialIo->Mode);
+
+  if (TerminalDevice->KeyboardTimerInterval == NewInterval) {
+return;
+  }
+
+  Status = gBS->SetTimer (
+