On 2016/1/18 19:33, Laszlo Ersek 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.
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
Ok, would you please help submit the formal patches(include the 115200
Timeout in SerialDxe)? :)
Thanks,
Star
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
