On 01/18/16 11:24, Zeng, Star wrote: > [...] > >>> >>> 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. >>> // > > Any comments about above concern?
Not really. I don't know what the purpose of the Timeout calculation is (what is the "keystroke response performance issue"?), but in any case, it is a good sign that TerminalDxe sets *some* Timeout explicitly. Plus, it has worked reliably until now, so we shouldn't change the Timeout argument. > >>> >>> 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. > > So TerminalDxe could know it is not content with the settings? I think so, yes. TerminalDxe knows in advance that it will handle escape sequences (= bursts of scan codes). The longest such sequence (3 or 4 scan codes I guess?) should fit in the receive FIFO. So TerminalDxe could try to figure out the longest sequence it wishes to handle, then configure such a receive FIFO depth with SerialIO. Or else, what I think would be better, just call SetAttributes() from the platform's SerialPortLib instance, with ReceiveFifoDepth=0: - in practice, this would restore the previous behavior, - I expect that this value should enable the FIFO on most UARTs, with a sufficient depth for the escape sequence processing > Should platform BDS code to explicitly change them with the > SetAttributes() call? Well, PlatformBDS is the ultimate fallback, always :), but I think TerminalDxe has much more business dealing with the Serial IO attributes. TerminalDxe already modifies the Timeout attribute, and in the end, the FIFO depth too is important to TerminalDxe (for escape seq processing). I think it's not a BDS responsibility. Consider for example "MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a UEFI driver (unlike SerialDxe, which is a DXE_DRIVER). If PlatformBDS wanted to control the receive FIFO depth of the SerialIo protocol instance procuded by PciSioSerialDxe, then it would have to connect PciIo instances (so SerialIo would exist), then set the attribute, then connect SerialIo recursively (so that TerminalDxe would sit on top). In other words, PlatformBds would have to intrude to the UEFI driver model -- a full bottom-up recursive ConnectAll would no longer work. Also, the user could disconnect/reconnect the SerialIo protocol provider in the UEFI shell; maybe even unload/reload the driver. So I think setting the attributes is the responsibility of the driver that sets directly atop. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel