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 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. >+ >+ 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. >+ > // > // Check whether serial buffer is empty. > // Skip the key transfer loop only if the SerialIo protocol instance >-- >2.7.0 > >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel