Re: [edk2] Transition to GitHub Update

2016-01-16 Thread Ard Biesheuvel 
On 15 January 2016 at 22:14, Jarlstrom, Laurie
 wrote:
> To: EDK II Community
>
> This message is an update on the transition from SourceForge to GitHub for 
> EDK II development.  The schedule is currently targeting the last week of 
> January or the first week of February to perform the transition.  The 
> transition process should only take one to two days to complete.  A 
> notification message will be sent the week prior to the actual dates that the 
> repositories will be impacted.  This should provide adequate notification for 
> the scheduled down time.
> As part of this transition some documentation and process changes have been 
> required.  The updated process as well as other GitHub specific information 
> can be found on the tianocore Wiki at the link provided below.  Feedback on 
> this documentation is welcome and needed to make sure the transition is as 
> smooth as possible.
>
> https://github.com/tianocore/tianocore.github.io/wiki/SourceForge-to-Github-Quick-Start
>
> Please post any comments or questions related to this transition to the 
> edk2-devel mailing list or reply to the email message.
>

Hello,

Thanks for getting all of this set up.

Regarding the following line

'Commits should build / boot if possible to support use of bisect'

could we make that a bit stronger please? Breaking bisect is really
not ok, and it usually does not take that much extra work to avoid it,
as long as you are aware of it. Given that many people are new to Git,
presenting it as an opt-in feature like this does not send the right
message imo.

In fact, having some guidelines about how to avoid breaking bisect
could be helpful, especially that something like adding some temporary
code in an early patch only to remove it again at the end of the
series is perfectly ok, and highly preferred over losing
bisectability. Not everybody might expect that.

Thanks,
Ard.
___
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

2016-01-16 Thread 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

2016-01-16 Thread 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