On 01/16/16 15:46, Zeng, Star wrote:
> On 2016/1/16 1:39, Laszlo Ersek 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 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.
> //
>
> 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.
Thanks
Laszlo
>
> Thanks,
> Star
>
>>
>> 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
