On 15 January 2016 at 17:39, Laszlo Ersek <ler...@redhat.com> 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 problem I have is that is someone decides that TerminalDxe needs to set the depth to 1 again, or some other value, the ARM platforms will break, because they clearly can set that value, but it isn't useful/usable. So perhaps PL011 needs to use the device's default value, irrespective of what TerminalDxe asks for? > 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
