Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

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  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 
 Date:   Thu Nov 26 08:52:12 2015 +

  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  = 100
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 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

[...]



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?



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?
Should platform BDS code to explicitly change them with the 
SetAttributes() call?


Thanks,
Star



Thanks
Laszlo


[...]

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

On 01/18/16 17:17, Brian J. Johnson wrote:
> On 01/18/2016 05:55 AM, Laszlo Ersek wrote:
>> On 01/18/16 12:41, Ryan Harkin wrote:
>>> >On 18 January 2016 at 11: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.
 >>
>>> >
>>> >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.
>> I tend to think this comes, at least partly, from the fact that UEFI
>> doesn't do interrupt-driven drivers. There's timer based polling (in
>> drivers), and there are busy loops (in applications, I guess).
>>
>> I think you'll see the same behavior with network packet reception.
>>
>> I said "reliably" beause in my environment I've had practically zero
>> issues with the serial terminal (beyond the one, very lacking, textual
>> resolution list -- but I patched that for myself permanently).
>>
>> Thanks
>> Laszlo
> 
> Can't you just enable hardware flow control (RTS/CTS) on the UART?

>From the spec:

Flow control is the responsibility of the software that uses the
protocol. Hardware flow control can be implemented through the use
of the GetControl() and SetControl() functions (described below) to
monitor and assert the flow control signals. The XON/XOFF flow
control algorithm can be implemented in software by inserting XON
and XOFF characters into the serial data stream as required.

(That's all I know about flow control over serial. :))

Grepping for EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE and
UART_FLOW_CONTROL_HARDWARE yields some results, but none in TerminalDxe.

Which makes me believe that most of the platform-specific serial IO
implementations offer hw flow control, but TerminalDxe doesn't use it.

Interestingly though, some hits are from the (now deprecated) Intel BDS:
"IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint".

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

On 01/18/16 12:41, Ryan Harkin wrote:
> On 18 January 2016 at 11: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.
>>
> 
> 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.

I tend to think this comes, at least partly, from the fact that UEFI
doesn't do interrupt-driven drivers. There's timer based polling (in
drivers), and there are busy loops (in applications, I guess).

I think you'll see the same behavior with network packet reception.

I said "reliably" beause in my environment I've had practically zero
issues with the serial terminal (beyond the one, very lacking, textual
resolution list -- but I patched that for myself permanently).

Thanks
Laszlo

> 
>>>
>
> 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 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

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

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

On 18 January 2016 at 11: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.
>

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
___

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Brian J. Johnson]

On 01/18/2016 05:55 AM, Laszlo Ersek wrote:

On 01/18/16 12:41, Ryan Harkin wrote:

>On 18 January 2016 at 11: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.
>>

>
>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.

I tend to think this comes, at least partly, from the fact that UEFI
doesn't do interrupt-driven drivers. There's timer based polling (in
drivers), and there are busy loops (in applications, I guess).

I think you'll see the same behavior with network packet reception.

I said "reliably" beause in my environment I've had practically zero
issues with the serial terminal (beyond the one, very lacking, textual
resolution list -- but I patched that for myself permanently).

Thanks
Laszlo


Can't you just enable hardware flow control (RTS/CTS) on the UART?
--

Brian J. Johnson


  Email:   bjohn...@sgi.com   U.S. Mail: SGI
  Office:  (651) 683-75212750 Blue Water Road
  VNET:6-233-7521Eagan, MN  55121-1400
  Fax: (651) 683-7696
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

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

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

[...]



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?

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.


Try to explain the thought by below patch (not tested yet).

3cc4f3e688471946048de7ea67e0a73631844fad
 MdeModulePkg/Universal/SerialDxe/SerialDxe.inf |  7 ---
 MdeModulePkg/Universal/SerialDxe/SerialIo.c| 69 
+-

 2 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf 
b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf

index 164060b..9e35c03 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+++ b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
@@ -33,19 +33,12 @@
   UefiDriverEntryPoint
   UefiBootServicesTableLib
   DebugLib
-  PcdLib
   SerialPortLib

 [Protocols]
   gEfiSerialIoProtocolGuid  ## PRODUCES
   gEfiDevicePathProtocolGuid## PRODUCES

-[Pcd]
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity   ## CONSUMES
-  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES
-
 [Depex]
   TRUE

diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c 
b/MdeModulePkg/Universal/SerialDxe/SerialIo.c

index de928d1..3985598 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 

 #include 
 #include 
@@ -180,15 +179,7 @@ SERIAL_DEVICE_PATH mSerialDevicePath = {
 //
 // Template used to initialize the Serial IO protocols.
 //
-EFI_SERIAL_IO_MODE mSerialIoMode = {
-  0, // ControlMask
-  0, // Timeout
-  0, // BaudRate
-  1, // ReceiveFifoDepth
-  0, // DataBits
-  0, // Parity
-  0  // StopBits
-};
+EFI_SERIAL_IO_MODE mSerialIoMode;

 EFI_SERIAL_IO_PROTOCOL mSerialIoTemplate = {
   SERIAL_IO_INTERFACE_REVISION,
@@ -201,6 +192,34 @@ EFI_SERIAL_IO_PROTOCOL mSerialIoTemplate = {
   
 };

+EFI_STATUS
+SerialDeviceInitialize (
+  IN EFI_SERIAL_IO_MODE *SerialIoMode
+  )
+{
+  EFI_STATUSStatus;
+
+  Status = SerialPortInitialize ();
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  //
+  // Get the real default value of the device.
+  //
+  ZeroMem (, sizeof (SerialIoMode));
+  Status = SerialPortSetAttributes (
+ >BaudRate,
+ >ReceiveFifoDepth,
+ >Timeout,
+ (EFI_PARITY_TYPE *) >Parity,
+ >DataBits,
+ (EFI_STOP_BITS_TYPE *) >StopBits);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+}
+
 /**
   Reset the serial device.

@@ -219,28 +238,14 @@ SerialReset (
   EFI_STATUSStatus;
   EFI_TPL   Tpl;

-  Status = SerialPortInitialize ();
+  Status = SerialDeviceInitialize (>Mode);
   if (EFI_ERROR (Status)) {
 return Status;
   }

-  //
-  // Set the Serial I/O mode and update the device path
-  //
-
   Tpl = gBS->RaiseTPL (TPL_NOTIFY);

   //
-  // Set the Serial I/O mode
-  //
-  This->Mode->ReceiveFifoDepth  = 1;
-  This->Mode->Timeout   = 0;
-  This->Mode->BaudRate  = PcdGet64 (PcdUartDefaultBaudRate);
-  This->Mode->DataBits  = (UINT32) PcdGet8 
(PcdUartDefaultDataBits);

-  This->Mode->Parity= (UINT32) PcdGet8 (PcdUartDefaultParity);
-  This->Mode->StopBits  = (UINT32) PcdGet8 
(PcdUartDefaultStopBits);

-
-  //
   // Check if the device path has actually changed
   //
   if (mSerialDevicePath.Uart.BaudRate == This->Mode->BaudRate &&
@@ -496,19 +501,15 @@ SerialDxeInitialize (
 {
   EFI_STATUSStatus;

-  Status = SerialPortInitialize ();
+  Status = SerialDeviceInitialize ();
   if (EFI_ERROR (Status)) {
 return Status;
   }

-  mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
-  mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
-  mSerialIoMode.Parity   = (UINT32) PcdGet8 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

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  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 
Date:   Thu Nov 26 08:52:12 2015 +

 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  = 100
   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:

 ReceiveFifoDepthThe 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 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

On 15 January 2016 at 10:03, Zeng, Star  wrote:
> On 2016/1/15 16:56, Ryan Harkin wrote:
>>
>> "
>>
>> On 15 January 2016 at 08:51, Zeng, Star  wrote:
>>>
>>> On 2016/1/15 16:08, Ryan Harkin wrote:


 On 15 January 2016 at 06:50, Zeng, Star  wrote:
>
>
> On 2016/1/15 14:36, Ryan Harkin wrote:
>>
>>
>>
>> On 15 Jan 2016 01:41, "Zeng, Star"  wrote:
>>>
>>>
>>>
>>>
>>> Hi Ryan,
>>>
>>>
>>> On 2016/1/15 3:10, Ryan Harkin wrote:




 Hi Star,

 This patch breaks the serial terminal for ARM FVP and Juno
 platforms.
 I assume it also breaks TC2 and other such "vexpress" platforms
 effected by this change.

 Whilst simple text input seems to work ok, cursor support does not.
 And we need cursor support for Intel BDS.

 Below is my hack at fixing the problem.

 Basically, I reintroduce PL011SerialPortExtLib and stub out the
 functionality in the functions copied into PL011SerialPortLib.
>>>
>>>
>>>
>>>
>>>
>>> You just copied the function implementation back to
>>
>>
>>
>> PL011SerialPortExtLib, then it would work? It is so weird,
>>
>> Yes, that's exactly what I did!  Very weird!
>>
>>> may I know where your fork?
>>>
>>
>> I haven't pushed this branch yet because I had only just started
>> moving
>> to
>> the latest EDK2 tree.   But my branch is only tianocore plus
>> OpenPlatformPkg and my patch below.  I see the same problem on FVP
>> models
>> on upstream and Juno on upstream if I hack it to use IntelBDS.
>
>
>
>
> You means it works well with latest EDK2 TRUNK code if no any change?
> But
> it
> will not work any more after using IntelBDS?
>

 No, it does not work well using EDK2 trunk code.  Normal text entry
 works on trunk, it is cursor keys that are broken.  Probably some
 other special keys are broken too.

 You can use FVP to test trunk.  So one can test this problem using the
 free FVP Foundation model from ARM.

 But if you want to test Juno on trunk, you have to make a change to
 ArmJuno.dsc to use Intel BDS instead of ARM BDS.  Just like this FVP
 change:

 f46ac5f  2015-09-01  ArmPlatformPkg/ArmVExpress-FVP: add support for
 the Intel BDS  [Ard Biesheuvel]

 git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378
 6f19259b-4bc3-4df7-8a09-765794883524

 This is because Intel BDS using cursor keys, ARM BDS does not.
>>>
>>>
>>>
>>> Sorry, I am not familiar with ARM platform and can only analyze the
>>> problem
>>> by code review. Up to now, I have no idea about the reason since the code
>>> implementation are same but just in different library instances.
>>>
>>> Could you push your code into your fork? Then we can get it to have a
>>> look.
>>>
>>
>> Sure, I've just pushed it:
>>
>>
>> https://git.linaro.org/landing-teams/working/arm/edk2.git/shortlog/refs/heads/serialdxe-fix-005
>>
>> It's in the "serialdxe-fix-005" branch.
>>
>
> I checked out the code and had a look. I found there is no one to link the
> added back PL011SerialPortExtLib, and I do not understand why the added back
> PL011SerialPortExtLib will cause PL011SerialPortLib to have compile failure
> you said in previous email and commit log.
>

You misunderstand me, or I was not clear, sorry.

This is library all voodoo to me, so I was just reverting your patch
in a way that compiles.

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.

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^

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Leif Lindholm]

Hi Star,

On Fri, Jan 15, 2016 at 06:03:10PM +0800, Zeng, Star wrote:
> >>>git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378
> >>>6f19259b-4bc3-4df7-8a09-765794883524
> >>>
> >>>This is because Intel BDS using cursor keys, ARM BDS does not.
> >>
> >>
> >>Sorry, I am not familiar with ARM platform and can only analyze the problem
> >>by code review. Up to now, I have no idea about the reason since the code
> >>implementation are same but just in different library instances.
> >>
> >>Could you push your code into your fork? Then we can get it to have a look.
> >>
> >
> >Sure, I've just pushed it:
> >
> >https://git.linaro.org/landing-teams/working/arm/edk2.git/shortlog/refs/heads/serialdxe-fix-005
> >
> >It's in the "serialdxe-fix-005" branch.
> 
> I checked out the code and had a look. I found there is no one to link the
> added back PL011SerialPortExtLib, and I do not understand why the added back
> PL011SerialPortExtLib will cause PL011SerialPortLib to have compile failure
> you said in previous email and commit log.
> 
> Could you check out to *SHA-1: 1b96428d92c01383dc437717db679f57cf70d980*
> that just before my SerialDxe series changes, and help to confirm if there
> is regression?

I can confirm this regression, and I can also confirm I tested your
refactoring without spotting the issue, since serial _output_ still
works :|

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

On 15 January 2016 at 17:05, Laszlo Ersek  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 
> Date:   Thu Nov 26 08:52:12 2015 +
>
> 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  = 100
>   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:
>
> ReceiveFifoDepthThe 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, 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

On 01/15/16 18:33, Ryan Harkin wrote:
> On 15 January 2016 at 17:05, Laszlo Ersek  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 
>> Date:   Thu Nov 26 08:52:12 2015 +
>>
>> 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  = 100
>>   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:
>>
>> ReceiveFifoDepthThe 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. 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ard Biesheuvel]

On 15 January 2016 at 18:05, Laszlo Ersek  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 
> Date:   Thu Nov 26 08:52:12 2015 +
>
> 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
>
>>
>> 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  = 100
>   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:
>
> ReceiveFifoDepthThe 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 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

On 15 January 2016 at 18:32, Laszlo Ersek  wrote:
> On 01/15/16 18:59, Ryan Harkin wrote:
>> On 15 January 2016 at 17:39, Laszlo Ersek  wrote:
>>> On 01/15/16 18:33, Ryan Harkin wrote:
 On 15 January 2016 at 17:05, Laszlo Ersek  wrote:
>
> 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?
>
> "irrespective" would be a violation of the UEFI spec (end to end);

What if the spec changes and the device cannot support the spec?
Surely it has to do something sane?

> the
> Serial IO Protocol implementation must not set a larger queue depth than
> what the client requests. If the client requests 1, the driver must not
> set 16 (for example).
>

Even if it cannot support the value requested?


> In my reading, the Serial IO Protocol spec is reasonably flexible, and
> considers the case when a given serial port cannot provide the exact
> queue depth requested. I think if the SerialDxe driver sticks to the
> UEFI spec requirements, and the TerminalDxe driver also assumes nothing
> more and nothing less (about the Serial IO Protocol) than specified,
> then things should "just work".
>
> Your concern applies to all drivers and libraries under MdeModulePkg --
> some of the central modules have the potential to break any and all
> platforms. I don't think we should try to work around that in
> platform-specific libraries that the central modules delegate some work
> to, especially if the UEFI spec goes into some detail on the relevant
> behavior.
>
> Anyway, I wouldn't like to force my opinion on others -- I just thought
> it was an interesting problem, with the solution coming more or less
> directly from the spec. Admittedly, there are places where edk2 deviates
> from the spec. At the moment I think it wouldn't be justified here.
>

To be fair, I don't know why I'm getting involved.  I'd simply like to
rebase to the tip of Tianocore and for it to actually work.

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Andrew Fish]

> On Jan 15, 2016, at 11:14 AM, Ryan Harkin  wrote:
> 
> On 15 January 2016 at 18:32, Laszlo Ersek  wrote:
>> On 01/15/16 18:59, Ryan Harkin wrote:
>>> On 15 January 2016 at 17:39, Laszlo Ersek  wrote:
 On 01/15/16 18:33, Ryan Harkin wrote:
> On 15 January 2016 at 17:05, Laszlo Ersek  wrote:
>> 
>> 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?
>> 
>> "irrespective" would be a violation of the UEFI spec (end to end);
> 
> What if the spec changes and the device cannot support the spec?

Generally the specification changes are additive and backward comparable. The 
higher level features of spec generally use verbiage like if the platform 
supports feature X it must implement it with protocol Y. 

> Surely it has to do something sane?
> 

Return EFI_UNSUPPORTED

Thanks,

Andrew Fish

>> the
>> Serial IO Protocol implementation must not set a larger queue depth than
>> what the client requests. If the client requests 1, the driver must not
>> set 16 (for example).
>> 
> 
> Even if it cannot support the value requested?
> 
> 
>> In my reading, the Serial IO Protocol spec is reasonably flexible, and
>> considers the case when a given serial port cannot provide the exact
>> queue depth requested. I think if the SerialDxe driver sticks to the
>> UEFI spec requirements, and the TerminalDxe driver also assumes nothing
>> more and nothing less (about the Serial IO Protocol) than specified,
>> then things should "just work".
>> 
>> Your concern applies to all drivers and libraries under MdeModulePkg --
>> some of the central modules have the potential to break any and all
>> platforms. I don't think we should try to work around that in
>> platform-specific libraries that the central modules delegate some work
>> to, especially if the UEFI spec goes into some detail on the relevant
>> behavior.
>> 
>> Anyway, I 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Laszlo Ersek]

On 01/15/16 18:59, Ryan Harkin wrote:
> On 15 January 2016 at 17:39, Laszlo Ersek  wrote:
>> On 01/15/16 18:33, Ryan Harkin wrote:
>>> On 15 January 2016 at 17:05, Laszlo Ersek  wrote:

 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?

"irrespective" would be a violation of the UEFI spec (end to end); the
Serial IO Protocol implementation must not set a larger queue depth than
what the client requests. If the client requests 1, the driver must not
set 16 (for example).

In my reading, the Serial IO Protocol spec is reasonably flexible, and
considers the case when a given serial port cannot provide the exact
queue depth requested. I think if the SerialDxe driver sticks to the
UEFI spec requirements, and the TerminalDxe driver also assumes nothing
more and nothing less (about the Serial IO Protocol) than specified,
then things should "just work".

Your concern applies to all drivers and libraries under MdeModulePkg --
some of the central modules have the potential to break any and all
platforms. I don't think we should try to work around that in
platform-specific libraries that the central modules delegate some work
to, especially if the UEFI spec goes into some detail on the relevant
behavior.

Anyway, I wouldn't like to force my opinion on others -- I just thought
it was an interesting problem, with the solution coming more or less
directly from the spec. Admittedly, there are places where edk2 deviates
from the spec. At the moment I think it wouldn't be justified here.

Thanks
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Leif Lindholm]

On Fri, Jan 15, 2016 at 06:05:03PM +0100, Laszlo Ersek wrote:
> snipping context liberally...

Snipping clear and very detailed (and useful) explanation.

> (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()).

Yes, that was why I agreed with that change in Ard's patch, but after
your explanation I agree that your proposed solution is the correct
location to change that.

Now, SetAttributes also lets you set default timeout by specifying 0,
but I don't think that would make much sense in this case- the
terminal input scan rate is a property of the terminal, not the
device.

> (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,

So I'm happy with the above fix.

Thanks!

/
Leif
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

On 2016/1/15 16:08, Ryan Harkin wrote:

On 15 January 2016 at 06:50, Zeng, Star  wrote:

On 2016/1/15 14:36, Ryan Harkin wrote:


On 15 Jan 2016 01:41, "Zeng, Star"  wrote:



Hi Ryan,


On 2016/1/15 3:10, Ryan Harkin wrote:



Hi Star,

This patch breaks the serial terminal for ARM FVP and Juno platforms.
I assume it also breaks TC2 and other such "vexpress" platforms
effected by this change.

Whilst simple text input seems to work ok, cursor support does not.
And we need cursor support for Intel BDS.

Below is my hack at fixing the problem.

Basically, I reintroduce PL011SerialPortExtLib and stub out the
functionality in the functions copied into PL011SerialPortLib.




You just copied the function implementation back to


PL011SerialPortExtLib, then it would work? It is so weird,

Yes, that's exactly what I did!  Very weird!


may I know where your fork?



I haven't pushed this branch yet because I had only just started moving to
the latest EDK2 tree.   But my branch is only tianocore plus
OpenPlatformPkg and my patch below.  I see the same problem on FVP models
on upstream and Juno on upstream if I hack it to use IntelBDS.



You means it works well with latest EDK2 TRUNK code if no any change? But it
will not work any more after using IntelBDS?



No, it does not work well using EDK2 trunk code.  Normal text entry
works on trunk, it is cursor keys that are broken.  Probably some
other special keys are broken too.

You can use FVP to test trunk.  So one can test this problem using the
free FVP Foundation model from ARM.

But if you want to test Juno on trunk, you have to make a change to
ArmJuno.dsc to use Intel BDS instead of ARM BDS.  Just like this FVP
change:

f46ac5f  2015-09-01  ArmPlatformPkg/ArmVExpress-FVP: add support for
the Intel BDS  [Ard Biesheuvel]

   git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378
6f19259b-4bc3-4df7-8a09-765794883524

This is because Intel BDS using cursor keys, ARM BDS does not.


Sorry, I am not familiar with ARM platform and can only analyze the 
problem by code review. Up to now, I have no idea about the reason since 
the code implementation are same but just in different library instances.


Could you push your code into your fork? Then we can get it to have a look.









Thanks,
Star




Any suggestions about why this is broken or proposals of a proper fix
are welcome.  In the meantime, I guess I'll have to carry this patch
in my own fork.

Regards,
Ryan.


   From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 2001
From: Ryan Harkin 
Date: Thu, 14 Jan 2016 18:41:54 +
Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
instead of EmbeddedPkg"

The SerialDxe patch below breaks ARM Ltd. platforms:


commit 921e987b2b26602dc85eaee856d494b97b6e02b0
Author: Star Zeng 
Date:   Thu Nov 26 08:51:05 2015 +

 ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of
EmbeddedPkg

 It is also to integrate PL011SerialPortExtLib to
PL011SerialPortLib.

 Cc: Michael D Kinney 
 Cc: Liming Gao 
 Cc: Leif Lindholm 
 Cc: Ard Biesheuvel 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Star Zeng 
 Reviewed-by: Ard Biesheuvel 

 git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971


6f19259b-4bc3-4df7-8a09-765794883524




I found that also reverting the change in PL011SerialPortLib.c resulted
in failure to compilate, so I simply changed the return values of the
copied in functions to zero.

Signed-off-by: Ryan Harkin 
---
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
.../PL011SerialPortLib/PL011SerialPortExtLib.c | 137


+


.../PL011SerialPortLib/PL011SerialPortExtLib.inf   |  43 +++
.../PL011SerialPortLib/PL011SerialPortLib.c|  12 +-
4 files changed, 184 insertions(+), 9 deletions(-)
create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1b8127d..19c1955 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,7 @@
  # ARM PL011 UART Driver
  PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf



SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf


+



SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf


  # ARM SP804 Dual Timer Driver
  TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf

diff --git

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

"

On 15 January 2016 at 08:51, Zeng, Star  wrote:
> On 2016/1/15 16:08, Ryan Harkin wrote:
>>
>> On 15 January 2016 at 06:50, Zeng, Star  wrote:
>>>
>>> On 2016/1/15 14:36, Ryan Harkin wrote:


 On 15 Jan 2016 01:41, "Zeng, Star"  wrote:
>
>
>
> Hi Ryan,
>
>
> On 2016/1/15 3:10, Ryan Harkin wrote:
>>
>>
>>
>> Hi Star,
>>
>> This patch breaks the serial terminal for ARM FVP and Juno platforms.
>> I assume it also breaks TC2 and other such "vexpress" platforms
>> effected by this change.
>>
>> Whilst simple text input seems to work ok, cursor support does not.
>> And we need cursor support for Intel BDS.
>>
>> Below is my hack at fixing the problem.
>>
>> Basically, I reintroduce PL011SerialPortExtLib and stub out the
>> functionality in the functions copied into PL011SerialPortLib.
>
>
>
>
> You just copied the function implementation back to


 PL011SerialPortExtLib, then it would work? It is so weird,

 Yes, that's exactly what I did!  Very weird!

> may I know where your fork?
>

 I haven't pushed this branch yet because I had only just started moving
 to
 the latest EDK2 tree.   But my branch is only tianocore plus
 OpenPlatformPkg and my patch below.  I see the same problem on FVP
 models
 on upstream and Juno on upstream if I hack it to use IntelBDS.
>>>
>>>
>>>
>>> You means it works well with latest EDK2 TRUNK code if no any change? But
>>> it
>>> will not work any more after using IntelBDS?
>>>
>>
>> No, it does not work well using EDK2 trunk code.  Normal text entry
>> works on trunk, it is cursor keys that are broken.  Probably some
>> other special keys are broken too.
>>
>> You can use FVP to test trunk.  So one can test this problem using the
>> free FVP Foundation model from ARM.
>>
>> But if you want to test Juno on trunk, you have to make a change to
>> ArmJuno.dsc to use Intel BDS instead of ARM BDS.  Just like this FVP
>> change:
>>
>> f46ac5f  2015-09-01  ArmPlatformPkg/ArmVExpress-FVP: add support for
>> the Intel BDS  [Ard Biesheuvel]
>>
>>git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378
>> 6f19259b-4bc3-4df7-8a09-765794883524
>>
>> This is because Intel BDS using cursor keys, ARM BDS does not.
>
>
> Sorry, I am not familiar with ARM platform and can only analyze the problem
> by code review. Up to now, I have no idea about the reason since the code
> implementation are same but just in different library instances.
>
> Could you push your code into your fork? Then we can get it to have a look.
>

Sure, I've just pushed it:

https://git.linaro.org/landing-teams/working/arm/edk2.git/shortlog/refs/heads/serialdxe-fix-005

It's in the "serialdxe-fix-005" branch.

>
>>
>>
>>>

> Thanks,
> Star
>
>
>>
>> Any suggestions about why this is broken or proposals of a proper fix
>> are welcome.  In the meantime, I guess I'll have to carry this patch
>> in my own fork.
>>
>> Regards,
>> Ryan.
>>
>>
>>From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00
>> 2001
>> From: Ryan Harkin 
>> Date: Thu, 14 Jan 2016 18:41:54 +
>> Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
>> instead of EmbeddedPkg"
>>
>> The SerialDxe patch below breaks ARM Ltd. platforms:
>>
>>> commit 921e987b2b26602dc85eaee856d494b97b6e02b0
>>> Author: Star Zeng 
>>> Date:   Thu Nov 26 08:51:05 2015 +
>>>
>>>  ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of
>>> EmbeddedPkg
>>>
>>>  It is also to integrate PL011SerialPortExtLib to
>>> PL011SerialPortLib.
>>>
>>>  Cc: Michael D Kinney 
>>>  Cc: Liming Gao 
>>>  Cc: Leif Lindholm 
>>>  Cc: Ard Biesheuvel 
>>>  Contributed-under: TianoCore Contribution Agreement 1.0
>>>  Signed-off-by: Star Zeng 
>>>  Reviewed-by: Ard Biesheuvel 
>>>
>>>  git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971


 6f19259b-4bc3-4df7-8a09-765794883524
>>
>>
>>
>>
>> I found that also reverting the change in PL011SerialPortLib.c
>> resulted
>> in failure to compilate, so I simply changed the return values of the
>> copied in functions to zero.
>>
>> Signed-off-by: Ryan Harkin 
>> ---
>> ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
>> .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137


 +
>>
>>
>> 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

On 2016/1/15 16:56, Ryan Harkin wrote:

"

On 15 January 2016 at 08:51, Zeng, Star  wrote:

On 2016/1/15 16:08, Ryan Harkin wrote:


On 15 January 2016 at 06:50, Zeng, Star  wrote:


On 2016/1/15 14:36, Ryan Harkin wrote:



On 15 Jan 2016 01:41, "Zeng, Star"  wrote:




Hi Ryan,


On 2016/1/15 3:10, Ryan Harkin wrote:




Hi Star,

This patch breaks the serial terminal for ARM FVP and Juno platforms.
I assume it also breaks TC2 and other such "vexpress" platforms
effected by this change.

Whilst simple text input seems to work ok, cursor support does not.
And we need cursor support for Intel BDS.

Below is my hack at fixing the problem.

Basically, I reintroduce PL011SerialPortExtLib and stub out the
functionality in the functions copied into PL011SerialPortLib.





You just copied the function implementation back to



PL011SerialPortExtLib, then it would work? It is so weird,

Yes, that's exactly what I did!  Very weird!


may I know where your fork?



I haven't pushed this branch yet because I had only just started moving
to
the latest EDK2 tree.   But my branch is only tianocore plus
OpenPlatformPkg and my patch below.  I see the same problem on FVP
models
on upstream and Juno on upstream if I hack it to use IntelBDS.




You means it works well with latest EDK2 TRUNK code if no any change? But
it
will not work any more after using IntelBDS?



No, it does not work well using EDK2 trunk code.  Normal text entry
works on trunk, it is cursor keys that are broken.  Probably some
other special keys are broken too.

You can use FVP to test trunk.  So one can test this problem using the
free FVP Foundation model from ARM.

But if you want to test Juno on trunk, you have to make a change to
ArmJuno.dsc to use Intel BDS instead of ARM BDS.  Just like this FVP
change:

f46ac5f  2015-09-01  ArmPlatformPkg/ArmVExpress-FVP: add support for
the Intel BDS  [Ard Biesheuvel]

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18378
6f19259b-4bc3-4df7-8a09-765794883524

This is because Intel BDS using cursor keys, ARM BDS does not.



Sorry, I am not familiar with ARM platform and can only analyze the problem
by code review. Up to now, I have no idea about the reason since the code
implementation are same but just in different library instances.

Could you push your code into your fork? Then we can get it to have a look.



Sure, I've just pushed it:

https://git.linaro.org/landing-teams/working/arm/edk2.git/shortlog/refs/heads/serialdxe-fix-005

It's in the "serialdxe-fix-005" branch.



I checked out the code and had a look. I found there is no one to link 
the added back PL011SerialPortExtLib, and I do not understand why the 
added back PL011SerialPortExtLib will cause PL011SerialPortLib to have 
compile failure you said in previous email and commit log.


Could you check out to *SHA-1: 1b96428d92c01383dc437717db679f57cf70d980* 
that just before my SerialDxe series changes, and help to confirm if 
there is regression?


Thanks,
Star

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

Hi Ryan,

On 2016/1/15 3:10, Ryan Harkin wrote:

Hi Star,

This patch breaks the serial terminal for ARM FVP and Juno platforms.
I assume it also breaks TC2 and other such "vexpress" platforms
effected by this change.

Whilst simple text input seems to work ok, cursor support does not.
And we need cursor support for Intel BDS.

Below is my hack at fixing the problem.

Basically, I reintroduce PL011SerialPortExtLib and stub out the
functionality in the functions copied into PL011SerialPortLib.


You just copied the function implementation back to 
PL011SerialPortExtLib, then it would work? It is so weird, may I know 
where your fork?


Thanks,
Star



Any suggestions about why this is broken or proposals of a proper fix
are welcome.  In the meantime, I guess I'll have to carry this patch
in my own fork.

Regards,
Ryan.


 From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 2001
From: Ryan Harkin 
Date: Thu, 14 Jan 2016 18:41:54 +
Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
instead of EmbeddedPkg"

The SerialDxe patch below breaks ARM Ltd. platforms:


commit 921e987b2b26602dc85eaee856d494b97b6e02b0
Author: Star Zeng 
Date:   Thu Nov 26 08:51:05 2015 +

   ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

   It is also to integrate PL011SerialPortExtLib to PL011SerialPortLib.

   Cc: Michael D Kinney 
   Cc: Liming Gao 
   Cc: Leif Lindholm 
   Cc: Ard Biesheuvel 
   Contributed-under: TianoCore Contribution Agreement 1.0
   Signed-off-by: Star Zeng 
   Reviewed-by: Ard Biesheuvel 

   git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971 
6f19259b-4bc3-4df7-8a09-765794883524


I found that also reverting the change in PL011SerialPortLib.c resulted
in failure to compilate, so I simply changed the return values of the
copied in functions to zero.

Signed-off-by: Ryan Harkin 
---
  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
  .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137 +
  .../PL011SerialPortLib/PL011SerialPortExtLib.inf   |  43 +++
  .../PL011SerialPortLib/PL011SerialPortLib.c|  12 +-
  4 files changed, 184 insertions(+), 9 deletions(-)
  create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
  create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1b8127d..19c1955 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,7 @@
# ARM PL011 UART Driver
PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf

SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
+  
SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf
# ARM SP804 Dual Timer Driver
TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf

diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
new file mode 100644
index 000..c60e7a4
--- /dev/null
+++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
@@ -0,0 +1,137 @@
+/** @file
+  Serial I/O Port library functions with no library constructor/destructor
+
+  Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of
the BSD License
+  which accompanies this distribution.  The full text of the license
may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+  Set new attributes to PL011.
+
+  @param  BaudRateThe baud rate of the serial device.
If the baud rate is not supported,
+  the speed will be reduced down to
the nearest supported one and the
+  variable's value will be updated accordingly.
+  @param  ReceiveFifoDepthThe number of characters the device
will buffer on input. If the specified
+  value is not supported, the
variable's value will be reduced down to the
+  nearest supported one.
+  @param  Timeout If applicable, the number of
microseconds the device will wait
+  before timing out a Read or a Write
operation.
+  @param  Parity  

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

Hi Star,

This patch breaks the serial terminal for ARM FVP and Juno platforms.
I assume it also breaks TC2 and other such "vexpress" platforms
effected by this change.

Whilst simple text input seems to work ok, cursor support does not.
And we need cursor support for Intel BDS.

Below is my hack at fixing the problem.

Basically, I reintroduce PL011SerialPortExtLib and stub out the
functionality in the functions copied into PL011SerialPortLib.

Any suggestions about why this is broken or proposals of a proper fix
are welcome.  In the meantime, I guess I'll have to carry this patch
in my own fork.

Regards,
Ryan.


>From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 2001
From: Ryan Harkin 
Date: Thu, 14 Jan 2016 18:41:54 +
Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
instead of EmbeddedPkg"

The SerialDxe patch below breaks ARM Ltd. platforms:

> commit 921e987b2b26602dc85eaee856d494b97b6e02b0
> Author: Star Zeng 
> Date:   Thu Nov 26 08:51:05 2015 +
>
>   ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg
>
>   It is also to integrate PL011SerialPortExtLib to PL011SerialPortLib.
>
>   Cc: Michael D Kinney 
>   Cc: Liming Gao 
>   Cc: Leif Lindholm 
>   Cc: Ard Biesheuvel 
>   Contributed-under: TianoCore Contribution Agreement 1.0
>   Signed-off-by: Star Zeng 
>   Reviewed-by: Ard Biesheuvel 
>
>   git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971 
> 6f19259b-4bc3-4df7-8a09-765794883524

I found that also reverting the change in PL011SerialPortLib.c resulted
in failure to compilate, so I simply changed the return values of the
copied in functions to zero.

Signed-off-by: Ryan Harkin 
---
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
 .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137 +
 .../PL011SerialPortLib/PL011SerialPortExtLib.inf   |  43 +++
 .../PL011SerialPortLib/PL011SerialPortLib.c|  12 +-
 4 files changed, 184 insertions(+), 9 deletions(-)
 create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
 create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1b8127d..19c1955 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,7 @@
   # ARM PL011 UART Driver
   PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf
   
SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
+  
SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf
   # ARM SP804 Dual Timer Driver
   TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf

diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
new file mode 100644
index 000..c60e7a4
--- /dev/null
+++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
@@ -0,0 +1,137 @@
+/** @file
+  Serial I/O Port library functions with no library constructor/destructor
+
+  Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of
the BSD License
+  which accompanies this distribution.  The full text of the license
may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+  Set new attributes to PL011.
+
+  @param  BaudRateThe baud rate of the serial device.
If the baud rate is not supported,
+  the speed will be reduced down to
the nearest supported one and the
+  variable's value will be updated accordingly.
+  @param  ReceiveFifoDepthThe number of characters the device
will buffer on input. If the specified
+  value is not supported, the
variable's value will be reduced down to the
+  nearest supported one.
+  @param  Timeout If applicable, the number of
microseconds the device will wait
+  before timing out a Read or a Write
operation.
+  @param  Parity  If applicable, this is the
EFI_PARITY_TYPE that is computed or checked
+  as each character is transmitted or
received. If the device does not
+  

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Ryan Harkin]

On 15 Jan 2016 01:41, "Zeng, Star"  wrote:
>
> Hi Ryan,
>
>
> On 2016/1/15 3:10, Ryan Harkin wrote:
>>
>> Hi Star,
>>
>> This patch breaks the serial terminal for ARM FVP and Juno platforms.
>> I assume it also breaks TC2 and other such "vexpress" platforms
>> effected by this change.
>>
>> Whilst simple text input seems to work ok, cursor support does not.
>> And we need cursor support for Intel BDS.
>>
>> Below is my hack at fixing the problem.
>>
>> Basically, I reintroduce PL011SerialPortExtLib and stub out the
>> functionality in the functions copied into PL011SerialPortLib.
>
>
> You just copied the function implementation back to
PL011SerialPortExtLib, then it would work? It is so weird,

Yes, that's exactly what I did!  Very weird!

> may I know where your fork?
>

I haven't pushed this branch yet because I had only just started moving to
the latest EDK2 tree.   But my branch is only tianocore plus
OpenPlatformPkg and my patch below.  I see the same problem on FVP models
on upstream and Juno on upstream if I hack it to use IntelBDS.

> Thanks,
> Star
>
>
>>
>> Any suggestions about why this is broken or proposals of a proper fix
>> are welcome.  In the meantime, I guess I'll have to carry this patch
>> in my own fork.
>>
>> Regards,
>> Ryan.
>>
>>
>>  From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 2001
>> From: Ryan Harkin 
>> Date: Thu, 14 Jan 2016 18:41:54 +
>> Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
>> instead of EmbeddedPkg"
>>
>> The SerialDxe patch below breaks ARM Ltd. platforms:
>>
>>> commit 921e987b2b26602dc85eaee856d494b97b6e02b0
>>> Author: Star Zeng 
>>> Date:   Thu Nov 26 08:51:05 2015 +
>>>
>>>ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg
>>>
>>>It is also to integrate PL011SerialPortExtLib to PL011SerialPortLib.
>>>
>>>Cc: Michael D Kinney 
>>>Cc: Liming Gao 
>>>Cc: Leif Lindholm 
>>>Cc: Ard Biesheuvel 
>>>Contributed-under: TianoCore Contribution Agreement 1.0
>>>Signed-off-by: Star Zeng 
>>>Reviewed-by: Ard Biesheuvel 
>>>
>>>git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971
6f19259b-4bc3-4df7-8a09-765794883524
>>
>>
>> I found that also reverting the change in PL011SerialPortLib.c resulted
>> in failure to compilate, so I simply changed the return values of the
>> copied in functions to zero.
>>
>> Signed-off-by: Ryan Harkin 
>> ---
>>   ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
>>   .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137
+
>>   .../PL011SerialPortLib/PL011SerialPortExtLib.inf   |  43 +++
>>   .../PL011SerialPortLib/PL011SerialPortLib.c|  12 +-
>>   4 files changed, 184 insertions(+), 9 deletions(-)
>>   create mode 100644
>> ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
>>   create mode 100644
>> ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>> index 1b8127d..19c1955 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>> @@ -95,6 +95,7 @@
>> # ARM PL011 UART Driver
>> PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf
>>
SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
>> +
SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf
>> # ARM SP804 Dual Timer Driver
>> TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf
>>
>> diff --git
a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
>> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
>> new file mode 100644
>> index 000..c60e7a4
>> --- /dev/null
>> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
>> @@ -0,0 +1,137 @@
>> +/** @file
>> +  Serial I/O Port library functions with no library
constructor/destructor
>> +
>> +  Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of
>> the BSD License
>> +  which accompanies this distribution.  The full text of the license
>> may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +/**
>> +  Set new attributes to PL011.
>> +
>> +  @param  BaudRate

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

[Zeng, Star]

On 2016/1/15 14:36, Ryan Harkin wrote:

On 15 Jan 2016 01:41, "Zeng, Star"  wrote:


Hi Ryan,


On 2016/1/15 3:10, Ryan Harkin wrote:


Hi Star,

This patch breaks the serial terminal for ARM FVP and Juno platforms.
I assume it also breaks TC2 and other such "vexpress" platforms
effected by this change.

Whilst simple text input seems to work ok, cursor support does not.
And we need cursor support for Intel BDS.

Below is my hack at fixing the problem.

Basically, I reintroduce PL011SerialPortExtLib and stub out the
functionality in the functions copied into PL011SerialPortLib.



You just copied the function implementation back to

PL011SerialPortExtLib, then it would work? It is so weird,

Yes, that's exactly what I did!  Very weird!


may I know where your fork?



I haven't pushed this branch yet because I had only just started moving to
the latest EDK2 tree.   But my branch is only tianocore plus
OpenPlatformPkg and my patch below.  I see the same problem on FVP models
on upstream and Juno on upstream if I hack it to use IntelBDS.


You means it works well with latest EDK2 TRUNK code if no any change? 
But it will not work any more after using IntelBDS?





Thanks,
Star




Any suggestions about why this is broken or proposals of a proper fix
are welcome.  In the meantime, I guess I'll have to carry this patch
in my own fork.

Regards,
Ryan.


  From 76352a589d5eaf6e9be28469aba63288f4defb25 Mon Sep 17 00:00:00 2001
From: Ryan Harkin 
Date: Thu, 14 Jan 2016 18:41:54 +
Subject: [PATCH] Fix "ArmPlatformPkg: Use SerialDxe in MdeModulePkg
instead of EmbeddedPkg"

The SerialDxe patch below breaks ARM Ltd. platforms:


commit 921e987b2b26602dc85eaee856d494b97b6e02b0
Author: Star Zeng 
Date:   Thu Nov 26 08:51:05 2015 +

ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

It is also to integrate PL011SerialPortExtLib to PL011SerialPortLib.

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Leif Lindholm 
Cc: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
Reviewed-by: Ard Biesheuvel 

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@18971

6f19259b-4bc3-4df7-8a09-765794883524



I found that also reverting the change in PL011SerialPortLib.c resulted
in failure to compilate, so I simply changed the return values of the
copied in functions to zero.

Signed-off-by: Ryan Harkin 
---
   ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc  |   1 +
   .../PL011SerialPortLib/PL011SerialPortExtLib.c | 137

+

   .../PL011SerialPortLib/PL011SerialPortExtLib.inf   |  43 +++
   .../PL011SerialPortLib/PL011SerialPortLib.c|  12 +-
   4 files changed, 184 insertions(+), 9 deletions(-)
   create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
   create mode 100644
ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
index 1b8127d..19c1955 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
+++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,7 @@
 # ARM PL011 UART Driver
 PL011UartLib|ArmPlatformPkg/Drivers/PL011Uart/PL011Uart.inf


SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf

+

SerialPortExtLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.inf

 # ARM SP804 Dual Timer Driver
 TimerLib|ArmPlatformPkg/Library/SP804TimerLib/SP804TimerLib.inf

diff --git

a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c

b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
new file mode 100644
index 000..c60e7a4
--- /dev/null
+++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortExtLib.c
@@ -0,0 +1,137 @@
+/** @file
+  Serial I/O Port library functions with no library

constructor/destructor

+
+  Copyright (c) 2012-2014, ARM Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of
the BSD License
+  which accompanies this distribution.  The full text of the license
may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR

IMPLIED.

+
+**/
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+  Set new attributes to PL011.
+
+  @param  BaudRateThe baud rate of the serial device.
If the baud rate is not supported,
+  the speed will be reduced down to
the