On 01/16/16 15:46, Zeng, Star wrote: > On 2016/1/16 1:39, Laszlo Ersek wrote: >> On 01/15/16 18:33, Ryan Harkin wrote: >>> On 15 January 2016 at 17:05, Laszlo Ersek <ler...@redhat.com> wrote: >>>> Hi, >>>> >>>> snipping context liberally... >>>> >>>>>>>>>>>>> Whilst simple text input seems to work ok, cursor support >>>>>>>>>>>>> does not. >>>>>>>>>>>>> And we need cursor support for Intel BDS. >>>> >>>> (1) I think this is important. See below. >>>> >>>>> When trying to revert your patch on the latest trunk code, the build >>>>> fails because your patch is doing too many things in one step. It >>>>> should have been three (or more) patches, in my opinion. >>>> >>>> (2) >>>> >>>> (Caveat: what I'm about to say / reference here may not apply fully.) >>>> >>>> I just want to say that I followed / partially reviewed Star's patch >>>> series (earlier versions of it than v4), and I had the exact same >>>> impression (= "this is too big"), about the patch that did more or >>>> less the same for ArmVirtPkg. >>>> >>>> Surprisingly :), I was wrong. Please see the message of commit >>>> >>>> commit ad7f6bc2e1163125cdb26f6b0e6695554028626a >>>> Author: Star Zeng <star.z...@intel.com> >>>> Date: Thu Nov 26 08:52:12 2015 +0000 >>>> >>>> ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg >>>> >>>> Also, this sub-thread could be interesting: >>>> >>>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/4582/focus=4593 >>>> >>> >>> I don't think this is the same as ARM platform support. I think there >>> could have been a single patch to move the code from ExtLib to Lib. >>> Another to remove ExtLib and another to change from to MdeModulePkg. >>> >>> >>>>> >>>>> Instead of reverting, I manually added back PL011SerialPortExtLib and >>>>> removed the code from PL011SerialPortLib at the same time. Then, the >>>>> code did not compile. >>>>> >>>>> So I changed my patch to also make PL011SerialPortLib return 0 instead >>>>> of calling the PL011 functions. And everything started to work. I >>>>> assumed this was because I added back the ExtLib, not because I >>>>> changed the added code. I was wrong. >>>>> >>>>> I've just pushed a new branch, serialdxe-fix-006, that only changes >>>>> PL011SerialPortLib.c to "return 0;" for all the new functions - and >>>>> that works also. >>>>> >>>>> So I don't need PL011SerialPortExtLib at all. I guess it was never >>>>> called from there? When you moved the code to PL011SerialPortLib, it >>>>> started getting called - and the code is causing problems. >>>>> >>>>> >>>>>> Could you check out to *SHA-1: >>>>>> 1b96428d92c01383dc437717db679f57cf70d980* >>>>>> that just before my SerialDxe series changes, and help to confirm >>>>>> if there >>>>>> is regression? >>>>>> >>>>> >>>>> I already checked out the commit before yours [1] and it works fine. >>>>> >>>>> [1] git checkout 921e987b2b26602dc85eaee856d494b97b6e02b0^ >>>> >>>> (3) Alright, so SVN r18974 (git 1bccab910700) is the commit that >>>> removes the old SerialDxe from EmbeddedPkg. At that stage, all >>>> client packages have been converted to the new driver in >>>> MdeModulePkg. Therefore, if we want to compare the two *drivers*, we >>>> have to check out the parent of this commit (that is, check out SVN >>>> r18973 == git 1bccab910700^ == git ad7f6bc2e1), and compare the >>>> following files: >>>> >>>> - EmbeddedPkg/SerialDxe/SerialIo.c >>>> - MdeModulePkg/Universal/SerialDxe/SerialIo.c >>>> >>>> I don't recommend to diff them -- instead, open them both side by >>>> side, and read them in parallel. >>>> >>>> This way I noticed the following difference: >>>> >>>> (a) In the EmbeddedPkg driver, we have the following *initial* >>>> values (not quoting everything, just what I think is relevant): >>>> >>>> EFI_SERIAL_IO_PROTOCOL.Mode->Timeout = 0 >>>> EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 1 >>>> >>>> (see gSerialIoTemplate and gSerialIoMode), however the SerialReset() >>>> function (implementing the Reset protocol member) sets the >>>> following, different values: >>>> >>>> EFI_SERIAL_IO_PROTOCOL.Mode->Timeout = 1000000 >>>> EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 0 >>>> >>>> Inconsistent, right? But that's not the difference I think is relevant: >>>> >>>> (b) The MdeModulePkg driver stores the following settings *both* at >>>> driver initialization and in SerialReset(): >>>> >>>> EFI_SERIAL_IO_PROTOCOL.Mode->Timeout = 0 >>>> EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 1 >>>> >>>> (See mSerialIoTemplate and mSerialIoMode.) >>>> >>>> That is, the initial state for Mode is identical between the (a) old >>>> and (b) new drivers. However, these relevant-looking fields *differ* >>>> between old and new driver after a client calls >>>> EFI_SERIAL_IO_PROTOCOL.Reset(). Then the old driver sets a >>>> ReceiveFifoDepth of zero, while the new driver sets 1. (I'll ignore >>>> Timeout for now.) >>>> >>>> The UEFI spec says the following about ReceiveFifoDepth: >>>> >>>> ReceiveFifoDepth The number of characters the device will >>>> buffer on input. >>>> >>>> [...] >>>> >>>> The default attributes for all UART-style serial device interfaces >>>> are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond >>>> timeout per character, no parity, 8 data bits, and 1 stop bit. >>>> [...] >>>> >>>> Special care must be taken if a significant amount of data is >>>> going >>>> to be read from a serial device. Since UEFI drivers are polled >>>> mode >>>> drivers, characters received on a serial device might be >>>> missed. It >>>> is the responsibility of the software that uses the protocol to >>>> check for new data often enough to guarantee that no characters >>>> will >>>> be missed. The required polling frequency depends on the baud rate >>>> of the connection and the depth of the receive FIFO. >>>> >>>> Okay, thus far it seems that the *new* driver is the correct one; >>>> the old ReceiveFifoDepth=0 setting, after Reset() was incorrect. >>>> >>>> Let's see though if that zero value is special in some way. >>>> >>>> Both old and new drivers depend on SerialPortLib. If I understand >>>> correctly, this class is resolved in both cases to the >>>> ArmPlatformPkg/Library/PL011SerialPortLib instance. That instance in >>>> turn depends on PL011UartLib. >>>> >>>> PL011UartLib is resolved, by >>>> "ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc", to >>>> "ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf". This DSC include >>>> file seems to be used by the Juno platform. So in the end we should >>>> look at how "ArmPlatformPkg/Drivers/PL011Uart" handles >>>> ReceiveFifoDepth. >>>> >>>> In PL011UartInitializePort(), we have: >>>> >>>> // The PL011 supports a buffer of 1, 16 or 32 chars. Therefore we >>>> can accept >>>> // 1 char buffer as the minimum fifo size. Because everything can >>>> be rounded down, >>>> // there is no maximum fifo size. >>>> if ((*ReceiveFifoDepth == 0) || (*ReceiveFifoDepth >= 32)) { >>>> LineControl |= PL011_UARTLCR_H_FEN; >>>> if (PL011_UARTPID2_VER (MmioRead32 (UartBase + UARTPID2)) > >>>> PL011_VER_R1P4) >>>> *ReceiveFifoDepth = 32; >>>> else >>>> *ReceiveFifoDepth = 16; >>>> } else { >>>> ASSERT (*ReceiveFifoDepth < 32); >>>> // Nothing else to do. 1 byte fifo is default. >>>> *ReceiveFifoDepth = 1; >>>> } >>>> >>>> Now, I didn't even *try* to trace the actual control flow, but, >>>> post-Reset(), >>>> - with the old driver we seem to have room for at least 16 >>>> characters in the receive FIFO, and the PL011_UARTLCR_H_FEN (== >>>> "FIFOs Enable") bit gets set, >>>> - whereas with the new driver, we end up with a single-byte buffer, >>>> and the "FIFOs Enable" bit is cleared in the line control register. >>>> >>>> I think this looks quite consistent with the facts that: >>>> - simple text input (= single byte "scan codes", with human-scale >>>> pauses between them) works, >>>> - but cursor movement (= multibyte escape sequences, produced in >>>> machine-scale bursts) breaks. >>>> >>>> (4) My opinion: >>>> >>>> - The new driver's ReceiveFifoDepth setting, on init and on Reset, >>>> is correct. (Conforms to the spec.) >>>> >>>> - The Timeout defaults seem wrong however: 1 million usecs should be >>>> set. >>>> >>>> - Whatever *uses* the Serial IO Protocol directly, should explicitly >>>> set some ReceiveFifoDepth (with the SetAttributes() member >>>> function), such that the depth accommodates the longest escape >>>> sequence that the client wishes to handle. I believe this >>>> requirement falls on the TerminalDxe driver, doesn't it? >>>> >>>> In any case, the explicitly configured FIFO depth can be "zero", >>>> apparently, for selecting the device's "default" FIFO depth. (See >>>> the documentation of SetAttributes()). >>>> >>>> (BTW, the SetAttributes() spec also explains the "everything can be >>>> rounded down" comment from PL011UartInitializePort(): "The nearest >>>> FIFO size supported by the serial device will be selected without >>>> exceeding the ReceiveFifoDepth parameter.") >>>> >>>> Ryan, can you please test the following patch? >>>> >>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> index 6fde3b2..e1a6527 100644 >>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>> @@ -806,7 +806,7 @@ TerminalDriverBindingStart ( >>>>> Status = TerminalDevice->SerialIo->SetAttributes ( >>>>> TerminalDevice->SerialIo, >>>>> Mode->BaudRate, >>>>> - Mode->ReceiveFifoDepth, >>>>> + 0, // device's default >>>>> FIFO depth. >>>>> (UINT32) SerialInTimeOut, >>>>> (EFI_PARITY_TYPE) >>>>> (Mode->Parity), >>>>> (UINT8) Mode->DataBits, >>>>> diff --git >>>>> a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> index 3be877b..ffdc8f2 100644 >>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>> @@ -547,7 +547,7 @@ TerminalConInTimerHandler ( >>>>> Status = SerialIo->SetAttributes ( >>>>> SerialIo, >>>>> Mode->BaudRate, >>>>> - Mode->ReceiveFifoDepth, >>>>> + 0, // device's default FIFO depth. >>>>> (UINT32) SerialInTimeOut, >>>>> (EFI_PARITY_TYPE) (Mode->Parity), >>>>> (UINT8) Mode->DataBits, >>>> >>> >>> Yes, this works for me. >> >> Thanks for the testing (to Ard too)! >> >>> So what do you think the proper solution is? Is Ard's patch not what >>> you were thinking? >> >> I think the UEFI spec is clear enough on EFI_SERIAL_IO_PROTOCOL that we >> can derive what the right thing is: >> >> - SerialDxe needs a fix so that not only its ReceiveFifoDepth defaults >> are correct (beause they are correct now!), but also the Timeout >> defaults. >> >> - TerminalDxe needs a fix too: it should recognize the defaults set by >> SerialDxe (as per UEFI spec requirements), and set ReceiveFifoDepth >> explicitly, with a SetAttributes() call. A good value for that looks >> like zero (= device's default FIFO depth). I believe the value zero also >> happens to restore the previous behavior, in practice -- it's just not >> SerialDxe's responsibility. > > The above analysis is very clear, thanks. I am a little concern about if > the code changes below follow the comments in the code. > > In Terminal.c: > // > // Set the timeout value of serial buffer for > // keystroke response performance issue > // > In TerminalConIn.c: > // > // if current timeout value for serial device is not identical with > // the value saved in TERMINAL_DEV structure, then recalculate the > // timeout value again and set serial attribute according to this value. > // > > And I also checked the > IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM > platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH > value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then > SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of > the functions is "A ReceiveFifoDepth value of 0 will use the device's > default FIFO depth", so what's the device's default FIFO depth?
Well, that's exactly what SerialDxe delegates to SerialPortLib :) SerialDxe need not be aware of the device-specific value; the platform will provide a device-specific library instance. > I have a thought that updates SerialDxe to call SerialSetAttributes(with > 0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize() > and SerialReset() to get the real default values of the device, then the > driver can also eliminate the use of PcdUartDefault####. > > What's your opinion? I don't think this is a good idea -- the UEFI spec is very clear on this. Under "11.8 Serial I/O Protocol", in the general description part, you find: The default attributes for all UART-style serial device interfaces are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond timeout per character, no parity, 8 data bits, and 1 stop bit. [...] So this is what must be in effect right after initialization, and -- with all likelihood -- after re-set as well. If a Serial IO protocol client is not content with these settings, then it must explicitly change them, with the SetAttributes() call. Thanks Laszlo > > Thanks, > Star > >> >> Thanks >> Laszlo >> >>> >>>> I expect the following call chain: >>>> >>>> TerminalDriverBindingStart() | TerminalConInTimerHandler() >>>> [MdeModulePkg/Universal/Console/TerminalDxe] >>>> SerialSetAttributes() >>>> [MdeModulePkg/Universal/SerialDxe/SerialIo.c] >>>> SerialPortSetAttributes() >>>> [ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c] >>>> PL011UartInitializePort() >>>> [ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.c] >>>> >>>> Thanks! >>>> Laszlo > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel