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 <[email protected]> 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:[email protected]]
Sent: Thursday, March 24, 2016 10:59 PM
To: Ni, Ruiyu <[email protected]>; [email protected]
Cc: Tian, Feng <[email protected]>; Zeng, Star <[email protected]>
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:[email protected]]
Sent: Wednesday, March 23, 2016 5:44 PM
To: Ni, Ruiyu <[email protected]><mailto:[email protected]>;
[email protected]<mailto:[email protected]>
Cc: Tian, Feng <[email protected]><mailto:[email protected]>;
Zeng, Star <[email protected]><mailto:[email protected]>;
Kinney, Michael D
<[email protected]><mailto:[email protected]>
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:[email protected]] On
Behalf Of Heyi Guo
Sent: Thursday, March 17, 2016 10:37 PM
To: [email protected]<mailto:[email protected]>
Cc: Heyi Guo <[email protected]><mailto:[email protected]>;
Tian, Feng <[email protected]><mailto:[email protected]>;
Zeng, Star <[email protected]><mailto:[email protected]>
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
<[email protected]><mailto:[email protected]>
Cc: Feng Tian <[email protected]><mailto:[email protected]>
Cc: Star Zeng <[email protected]><mailto:[email protected]>
---
.../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 (
);
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 200000 // 0.02s
+#define DEFAULT_KEYBOARD_TIMER_INTERVAL 200000 // 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_EVENT Event,
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 TerminalDevice The 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
+ )
+{
+ UINT32 FifoDepth;
+
+ if (Mode->BaudRate == 0) {
+ return DEFAULT_KEYBOARD_TIMER_INTERVAL;
+ }
5. Now I understand you need the default timer interval for the
default setting.
Because UEFI spec says when the baud rate has the value of 0 to
indicate the device
runs at the device's *designed speed*.
But I still think you can calculate the timer interval assuming the
baudrate is 115200.
Because if using 115200 the timer interval is about 0.001s which is
still shorter than 0.02s.
Sounds good for me.
+
+ 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 * 10000000 * 2,
+ Mode->BaudRate * 3,
+ NULL
+ );
+}
6. Same question. Can you use the original equation #1?
+
+
+/**
+ Update period of polling timer event.
+
+ @param TerminalDevice The 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 (
+ TerminalDevice->TimerEvent,
+ TimerPeriodic,
+ NewInterval
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ TerminalDevice->KeyboardTimerInterval = NewInterval;
+}
+
+
+/**
Timer handler to poll the key from serial.
@param Event Indicates the event that
invoke this function.
@@ -560,6 +625,9 @@ TerminalConInTimerHandler (
TerminalDevice->SerialInTimeOut = SerialInTimeOut;
}
}
+
+ UpdatePollingRate (TerminalDevice);
7. All the parameters (baudrate, databits, stopbits,etc) are stored
in the UART device path.
Any change to these parameters triggers the UART device path
reinstallation, ultimately
triggers the Terminal driver's restart.
So the polling rate doesn't need to update here.
How about FIFO depth change?
Thanks for your review :)
Heyi
+
//
// Check whether serial buffer is empty.
// Skip the key transfer loop only if the SerialIo protocol
instance
--
2.7.0
_______________________________________________
edk2-devel mailing list
[email protected]<mailto:[email protected]>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
--
Brian J. Johnson
--------------------------------------------------------------------
My statements are my own, are not authorized by SGI, and do not
necessarily represent SGI’s positions.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
