[PATCH 0/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH 0/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
@ 2017-08-16 12:10 Laszlo Ersek
  2017-08-16 12:10 ` [PATCH 1/1] " Laszlo Ersek
  0 siblings, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2017-08-16 12:10 UTC (permalink / raw)
  To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Heyi Guo, Star Zeng

Repo:   https://github.com/lersek/edk2.git
Branch: fdtpl011_getcontrol

I tested it with TCG, and with KVM on Mustang. The boot hang is gone,
the UEFI shell works fine over serial. So does grub2.

Testing from others would be appreciated!

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Heyi Guo <guoheyi@huawei.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (1):
  ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all
    SerialPortLib APIs

 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 38 ++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-16 12:10 [PATCH 0/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs Laszlo Ersek
@ 2017-08-16 12:10 ` Laszlo Ersek
  2017-08-16 14:42   ` Ard Biesheuvel
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Laszlo Ersek @ 2017-08-16 12:10 UTC (permalink / raw)
  To: edk2-devel-01; +Cc: Ard Biesheuvel, Brijesh Singh, Heyi Guo, Star Zeng

With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe:
Process timeout consistently in SerialRead", 2017-07-18), setting
EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
GetControl() SerialPortLib function, is no longer a "small optimization".
Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in
TerminalDxe's TerminalConInTimerHandler() can take very long if the input
queue is empty, even if GetOneKeyFromSerial()'s return value causes the
loop to be exited right after, in the first iteration.

This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
TerminalConInTimerHandler() takes so long to return that, by the time it
returns, there's another execution queued already (due to the associated
timer event being signaled meanwhile). The boot process is stuck in the
timer event handler.

Therefore even the first GetOneKeyFromSerial() iteration must be prevented
in TerminalConInTimerHandler() if the input queue is empty, and that
requires implementing GetControl() for real.

Implement the SetAttributes(), SetControl() and GetControl() APIs (of
SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching
PL011UartLib functions. This follows the example of
"ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's
original idea under [1].

The patch can be considered a continuation of commit ad7f6bc2e116
("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg",
2015-11-26), based on the mailing list threads [1] [2] [3].

[1] http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
[2] http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
[3] http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Heyi Guo <guoheyi@huawei.com>
Cc: Star Zeng <star.zeng@intel.com>
Originally-suggested-by: Star Zeng <star.zeng@intel.com>
Reported-by: Brijesh Singh <brijesh.singh@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 38 ++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index 48a0530dcc2f..05d3547fda91 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -200,7 +200,23 @@ SerialPortSetAttributes (
   IN OUT EFI_STOP_BITS_TYPE *StopBits
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartInitializePort (
+               mSerialBaseAddress,
+               FixedPcdGet32 (PL011UartClkInHz),
+               BaudRate,
+               ReceiveFifoDepth,
+               Parity,
+               DataBits,
+               StopBits
+               );
+  }
+
+  return Status;
 }

 /**
@@ -219,7 +235,15 @@ SerialPortSetControl (
   IN UINT32 Control
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartSetControl (mSerialBaseAddress, Control);
+  }
+
+  return Status;
 }

 /**
@@ -238,6 +262,14 @@ SerialPortGetControl (
   OUT UINT32 *Control
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartGetControl (mSerialBaseAddress, Control);
+  }
+
+  return Status;
 }

-- 
2.14.1.3.gb7cf6e02401b

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-16 12:10 ` [PATCH 1/1] " Laszlo Ersek
@ 2017-08-16 14:42   ` Ard Biesheuvel
  2017-08-16 15:03   ` Brijesh Singh
  2017-08-17  1:02   ` Zeng, Star
  2 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2017-08-16 14:42 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel-01, Heyi Guo, Star Zeng

On 16 August 2017 at 13:10, Laszlo Ersek <lersek@redhat.com> wrote:
> With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe:
> Process timeout consistently in SerialRead", 2017-07-18), setting
> EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
> GetControl() SerialPortLib function, is no longer a "small optimization".
> Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in
> TerminalDxe's TerminalConInTimerHandler() can take very long if the input
> queue is empty, even if GetOneKeyFromSerial()'s return value causes the
> loop to be exited right after, in the first iteration.
>
> This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
> TerminalConInTimerHandler() takes so long to return that, by the time it
> returns, there's another execution queued already (due to the associated
> timer event being signaled meanwhile). The boot process is stuck in the
> timer event handler.
>
> Therefore even the first GetOneKeyFromSerial() iteration must be prevented
> in TerminalConInTimerHandler() if the input queue is empty, and that
> requires implementing GetControl() for real.
>
> Implement the SetAttributes(), SetControl() and GetControl() APIs (of
> SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching
> PL011UartLib functions. This follows the example of
> "ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's
> original idea under [1].
>
> The patch can be considered a continuation of commit ad7f6bc2e116
> ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg",
> 2015-11-26), based on the mailing list threads [1] [2] [3].
>
> [1] http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
> [2] http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
> [3] http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Heyi Guo <guoheyi@huawei.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Originally-suggested-by: Star Zeng <star.zeng@intel.com>
> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks Laszlo!

> ---
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 38 ++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> index 48a0530dcc2f..05d3547fda91 100644
> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> @@ -200,7 +200,23 @@ SerialPortSetAttributes (
>    IN OUT EFI_STOP_BITS_TYPE *StopBits
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartInitializePort (
> +               mSerialBaseAddress,
> +               FixedPcdGet32 (PL011UartClkInHz),
> +               BaudRate,
> +               ReceiveFifoDepth,
> +               Parity,
> +               DataBits,
> +               StopBits
> +               );
> +  }
> +
> +  return Status;
>  }
>
>  /**
> @@ -219,7 +235,15 @@ SerialPortSetControl (
>    IN UINT32 Control
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartSetControl (mSerialBaseAddress, Control);
> +  }
> +
> +  return Status;
>  }
>
>  /**
> @@ -238,6 +262,14 @@ SerialPortGetControl (
>    OUT UINT32 *Control
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartGetControl (mSerialBaseAddress, Control);
> +  }
> +
> +  return Status;
>  }
>
> --
> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-16 12:10 ` [PATCH 1/1] " Laszlo Ersek
  2017-08-16 14:42   ` Ard Biesheuvel
@ 2017-08-16 15:03   ` Brijesh Singh
  2017-08-16 15:36     ` Laszlo Ersek
  2017-08-17  1:02   ` Zeng, Star
  2 siblings, 1 reply; 7+ messages in thread
From: Brijesh Singh @ 2017-08-16 15:03 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-01
  Cc: brijesh.singh, Ard Biesheuvel, Heyi Guo, Star Zeng

Hi Laszlo,

Thanks for the patch.


On 08/16/2017 07:10 AM, Laszlo Ersek wrote:
> With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe:
> Process timeout consistently in SerialRead", 2017-07-18), setting
> EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
> GetControl() SerialPortLib function, is no longer a "small optimization".
> Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in
> TerminalDxe's TerminalConInTimerHandler() can take very long if the input
> queue is empty, even if GetOneKeyFromSerial()'s return value causes the
> loop to be exited right after, in the first iteration.
> 
> This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
> TerminalConInTimerHandler() takes so long to return that, by the time it
> returns, there's another execution queued already (due to the associated
> timer event being signaled meanwhile). The boot process is stuck in the
> timer event handler.
> 
> Therefore even the first GetOneKeyFromSerial() iteration must be prevented
> in TerminalConInTimerHandler() if the input queue is empty, and that
> requires implementing GetControl() for real.
> 
> Implement the SetAttributes(), SetControl() and GetControl() APIs (of
> SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching
> PL011UartLib functions. This follows the example of
> "ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's
> original idea under [1].
> 
> The patch can be considered a continuation of commit ad7f6bc2e116
> ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg",
> 2015-11-26), based on the mailing list threads [1] [2] [3].
> 
> [1] http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
> [2] http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
> [3] http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Heyi Guo <guoheyi@huawei.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Originally-suggested-by: Star Zeng <star.zeng@intel.com>
> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---


I can confirm that with this patch I am able to boot to the UEFI shell prompt.

Tested-by: Brijesh Singh <brijesh.singh@amd.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-16 15:03   ` Brijesh Singh
@ 2017-08-16 15:36     ` Laszlo Ersek
  0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2017-08-16 15:36 UTC (permalink / raw)
  To: Brijesh Singh, edk2-devel-01; +Cc: Ard Biesheuvel, Heyi Guo, Star Zeng

On 08/16/17 17:03, Brijesh Singh wrote:
> Hi Laszlo,
> 
> Thanks for the patch.
> 
> 
> On 08/16/2017 07:10 AM, Laszlo Ersek wrote:
>> With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg
>> SerialDxe:
>> Process timeout consistently in SerialRead", 2017-07-18), setting
>> EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
>> GetControl() SerialPortLib function, is no longer a "small optimization".
>> Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in
>> TerminalDxe's TerminalConInTimerHandler() can take very long if the input
>> queue is empty, even if GetOneKeyFromSerial()'s return value causes the
>> loop to be exited right after, in the first iteration.
>>
>> This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
>> TerminalConInTimerHandler() takes so long to return that, by the time it
>> returns, there's another execution queued already (due to the associated
>> timer event being signaled meanwhile). The boot process is stuck in the
>> timer event handler.
>>
>> Therefore even the first GetOneKeyFromSerial() iteration must be
>> prevented
>> in TerminalConInTimerHandler() if the input queue is empty, and that
>> requires implementing GetControl() for real.
>>
>> Implement the SetAttributes(), SetControl() and GetControl() APIs (of
>> SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching
>> PL011UartLib functions. This follows the example of
>> "ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's
>> original idea under [1].
>>
>> The patch can be considered a continuation of commit ad7f6bc2e116
>> ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg",
>> 2015-11-26), based on the mailing list threads [1] [2] [3].
>>
>> [1]
>> http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
>>
>> [2]
>> http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
>>
>> [3]
>> http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com
>>
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Heyi Guo <guoheyi@huawei.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Originally-suggested-by: Star Zeng <star.zeng@intel.com>
>> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
> 
> 
> I can confirm that with this patch I am able to boot to the UEFI shell
> prompt.
> 
> Tested-by: Brijesh Singh <brijesh.singh@amd.com>
> 

Thank you Ard and Brijesh; I'll hold off pushing the patch until
tomorrow, to see if Heyi and Star want to comment.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-16 12:10 ` [PATCH 1/1] " Laszlo Ersek
  2017-08-16 14:42   ` Ard Biesheuvel
  2017-08-16 15:03   ` Brijesh Singh
@ 2017-08-17  1:02   ` Zeng, Star
  2017-08-17 12:17     ` Laszlo Ersek
  2 siblings, 1 reply; 7+ messages in thread
From: Zeng, Star @ 2017-08-17  1:02 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel-01
  Cc: Ard Biesheuvel, Brijesh Singh, Heyi Guo, Zeng, Star

Very good commit log, history information and code change. :)

Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, August 16, 2017 8:11 PM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Brijesh Singh <brijesh.singh@amd.com>; Heyi Guo <guoheyi@huawei.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs

With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe:
Process timeout consistently in SerialRead", 2017-07-18), setting EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
GetControl() SerialPortLib function, is no longer a "small optimization".
Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in TerminalDxe's TerminalConInTimerHandler() can take very long if the input queue is empty, even if GetOneKeyFromSerial()'s return value causes the loop to be exited right after, in the first iteration.

This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
TerminalConInTimerHandler() takes so long to return that, by the time it returns, there's another execution queued already (due to the associated timer event being signaled meanwhile). The boot process is stuck in the timer event handler.

Therefore even the first GetOneKeyFromSerial() iteration must be prevented in TerminalConInTimerHandler() if the input queue is empty, and that requires implementing GetControl() for real.

Implement the SetAttributes(), SetControl() and GetControl() APIs (of SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching PL011UartLib functions. This follows the example of "ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's original idea under [1].

The patch can be considered a continuation of commit ad7f6bc2e116
("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg", 2015-11-26), based on the mailing list threads [1] [2] [3].

[1] http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
[2] http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
[3] http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Heyi Guo <guoheyi@huawei.com>
Cc: Star Zeng <star.zeng@intel.com>
Originally-suggested-by: Star Zeng <star.zeng@intel.com>
Reported-by: Brijesh Singh <brijesh.singh@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 38 ++++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index 48a0530dcc2f..05d3547fda91 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -200,7 +200,23 @@ SerialPortSetAttributes (
   IN OUT EFI_STOP_BITS_TYPE *StopBits
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartInitializePort (
+               mSerialBaseAddress,
+               FixedPcdGet32 (PL011UartClkInHz),
+               BaudRate,
+               ReceiveFifoDepth,
+               Parity,
+               DataBits,
+               StopBits
+               );
+  }
+
+  return Status;
 }

 /**
@@ -219,7 +235,15 @@ SerialPortSetControl (
   IN UINT32 Control
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartSetControl (mSerialBaseAddress, Control);  }
+
+  return Status;
 }

 /**
@@ -238,6 +262,14 @@ SerialPortGetControl (
   OUT UINT32 *Control
   )
 {
-  return RETURN_UNSUPPORTED;
+  RETURN_STATUS Status;
+
+  if (mSerialBaseAddress == 0) {
+    Status = RETURN_UNSUPPORTED;
+  } else {
+    Status = PL011UartGetControl (mSerialBaseAddress, Control);  }
+
+  return Status;
 }

--
2.14.1.3.gb7cf6e02401b

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
  2017-08-17  1:02   ` Zeng, Star
@ 2017-08-17 12:17     ` Laszlo Ersek
  0 siblings, 0 replies; 7+ messages in thread
From: Laszlo Ersek @ 2017-08-17 12:17 UTC (permalink / raw)
  To: Zeng, Star, Ard Biesheuvel, Brijesh Singh; +Cc: edk2-devel-01, Heyi Guo

On 08/17/17 03:02, Zeng, Star wrote:
> Very good commit log, history information and code change. :)
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>

Thank you all for the comments; patch pushed as commit 5f0f5e90ae8c.

Cheers,
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Wednesday, August 16, 2017 8:11 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Brijesh Singh <brijesh.singh@amd.com>; Heyi Guo <guoheyi@huawei.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs
> 
> With the SerialDxe change in commit 4cf3f37c87ba ("MdeModulePkg SerialDxe:
> Process timeout consistently in SerialRead", 2017-07-18), setting EFI_SERIAL_INPUT_BUFFER_EMPTY in the "Control" output parameter, in the
> GetControl() SerialPortLib function, is no longer a "small optimization".
> Namely, due to the SerialDxe change, the GetOneKeyFromSerial() call in TerminalDxe's TerminalConInTimerHandler() can take very long if the input queue is empty, even if GetOneKeyFromSerial()'s return value causes the loop to be exited right after, in the first iteration.
> 
> This issue causes a boot hang in ArmVirtQemu: with the input queue empty,
> TerminalConInTimerHandler() takes so long to return that, by the time it returns, there's another execution queued already (due to the associated timer event being signaled meanwhile). The boot process is stuck in the timer event handler.
> 
> Therefore even the first GetOneKeyFromSerial() iteration must be prevented in TerminalConInTimerHandler() if the input queue is empty, and that requires implementing GetControl() for real.
> 
> Implement the SetAttributes(), SetControl() and GetControl() APIs (of SerialPortExtLib origin) in FdtPL011SerialPortLib with calls to matching PL011UartLib functions. This follows the example of "ArmPlatformPkg/Library/PL011SerialPortLib" and also matches Star's original idea under [1].
> 
> The patch can be considered a continuation of commit ad7f6bc2e116
> ("ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg", 2015-11-26), based on the mailing list threads [1] [2] [3].
> 
> [1] http://mid.mail-archive.com/1447752930-32880-12-git-send-email-star.zeng@intel.com
> [2] http://mid.mail-archive.com/1448243067-1880-12-git-send-email-star.zeng@intel.com
> [3] http://mid.mail-archive.com/b748580c-cb51-32c9-acf9-780841ef15da@redhat.com
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Heyi Guo <guoheyi@huawei.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Originally-suggested-by: Star Zeng <star.zeng@intel.com>
> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 38 ++++++++++++++++++--
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> index 48a0530dcc2f..05d3547fda91 100644
> --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
> @@ -200,7 +200,23 @@ SerialPortSetAttributes (
>    IN OUT EFI_STOP_BITS_TYPE *StopBits
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartInitializePort (
> +               mSerialBaseAddress,
> +               FixedPcdGet32 (PL011UartClkInHz),
> +               BaudRate,
> +               ReceiveFifoDepth,
> +               Parity,
> +               DataBits,
> +               StopBits
> +               );
> +  }
> +
> +  return Status;
>  }
>  
>  /**
> @@ -219,7 +235,15 @@ SerialPortSetControl (
>    IN UINT32 Control
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartSetControl (mSerialBaseAddress, Control);  }
> +
> +  return Status;
>  }
>  
>  /**
> @@ -238,6 +262,14 @@ SerialPortGetControl (
>    OUT UINT32 *Control
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  RETURN_STATUS Status;
> +
> +  if (mSerialBaseAddress == 0) {
> +    Status = RETURN_UNSUPPORTED;
> +  } else {
> +    Status = PL011UartGetControl (mSerialBaseAddress, Control);  }
> +
> +  return Status;
>  }
>  
> --
> 2.14.1.3.gb7cf6e02401b
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-08-17 12:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-16 12:10 [PATCH 0/1] ArmVirtPkg/FdtPL011SerialPortLib: call PL011UartLib in all SerialPortLib APIs Laszlo Ersek
2017-08-16 12:10 ` [PATCH 1/1] " Laszlo Ersek
2017-08-16 14:42   ` Ard Biesheuvel
2017-08-16 15:03   ` Brijesh Singh
2017-08-16 15:36     ` Laszlo Ersek
2017-08-17  1:02   ` Zeng, Star
2017-08-17 12:17     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox