On 18 January 2016 at 11:33, Laszlo Ersek <ler...@redhat.com> wrote: > 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. >
I think "reliably" is a bit generous ;-) The most common complaint I get about EDK2 is that it can't handle more than a FIFO's worth of pasted characters on the serial port. >> >>>> >>>> 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
