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. 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
