On 15 January 2016 at 18: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 > >> >> 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, > > 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] >
This patch also fixes the problem for me. Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
