On 03/24/2016 10:47 AM, Ni, Ruiyu wrote:
Regards,
Ray
-----Original Message-----
From: Heyi Guo [mailto:heyi....@linaro.org]
Sent: Thursday, March 24, 2016 9:09 AM
To: Ryan Harkin <ryan.har...@linaro.org>; Laszlo Ersek <ler...@redhat.com>
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Tian, Feng <feng.t...@intel.com>;
edk2-devel@lists.01.org <edk2-de...@ml01.01.org>;
Zeng, Star <star.z...@intel.com>; Ard Biesheuvel <ard.biesheu...@linaro.org>
Subject: Re: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with
PCD
Hi folks,
Please see my thoughts below:
On 03/24/2016 12:26 AM, Ryan Harkin wrote:
On 23 March 2016 at 16:24, Ryan Harkin <ryan.har...@linaro.org> wrote:
On 23 March 2016 at 08:53, Laszlo Ersek <ler...@redhat.com> wrote:
On 03/23/16 09:33, Ni, Ruiyu wrote:
Laszlo,
Since the patch below lets SerialDxe driver use the new introduced
PCD gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth.
Would you mind to revert the check in @ 31ae446b
--> MdeModulePkg: TerminalDxe: select the UART's default receive FIFO depth?
If I understand correctly, this series increases the terminal polling
rate, so that cursor movement escape sequences (and other burst-like
sequences) can be processed even with a receive fifo depth of 1. Hence
31ae446b should become unnecessary.
If Ryan and Ard (CC'd), who tested 31ae446b originally, can also test
your revert of 31ae446b -- on top of this series from Heyi --, and the
cursor keys work with 31ae446b reverted, then I don't mind, sure.
Unfortunately TC2 and Juno no longer with with 31ae446b reverted and
these 3 patches from the series applied.
I think my patches may not be sufficient to replace patch 31ae446b; they
have some different results.
The result of patch 31ae446b is absolute; it does not rely on timer
event or code execution but only on UART hardware.
My patches are actually complementary software solution for platforms
which have certain length of FIFO depth, e.g. 40 characters continuous
input to UART with 32 FIFO depth. I don't think it will work well on
FIFO depth of 1, as it does not have enough scalability for software
processing. This also the reason why I reduce the precisely calculated
interval.
It also depends on the timer interrupt period (should be
gEmbeddedTokenSpaceGuid.PcdTimerPeriod on ARM platforms), which must be
less than the calculated polling interval.
So my opinion is to keep patch 31ae446b if it is reasonable on its own,
and take my patches as further enhancement.
NO.
I understand the fact that the patch 31ae446b did fix an issue in certain
platform.
But the patch lets TerminalDxe driver changes the underlying UART's Receive
FIFO depth. Such behavior is bad.
Now since we have added the new PCD PcdDefaultUartReceiveFifoDepth,
we can fix the original problem by setting the PCD value to 16 from platform
DSC.
In all, I wanted to say, with the new PCD patch, the patch 31ae446b can be
reverted
without functionality impact.
Sorry I misunderstood your previous email. Does it make sense to set the
default value of PCD to 0 directly?
And I will still talk with Heyi to understand why 3/3 timer interval patch
needs to
coded like that.
Did you see my early mail to explain the calculation?
Regards.
Heyi
Thanks.
Heyi
I'm sure you would all work it out ok, but that should have been:
"Unfortunately TC2 and Juno no longer work with 31ae446b reverted and
these 3 patches from the series applied."
It's a shame, because it works on FVP models and copy/paste then works.
I haven't made any investigations on what's going wrong. But regular
ASCII keys work, control codes like cursor keys don't. So it looks
like the old FIFO setting problem we discussed a few weeks ago.
Thanks
Laszlo
Regards,
Ray
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Heyi Guo
Sent: Thursday, March 17, 2016 10:37 PM
To: edk2-devel@lists.01.org
Cc: Heyi Guo <heyi....@linaro.org>; Tian, Feng <feng.t...@intel.com>; Zeng, Star
<star.z...@intel.com>
Subject: [edk2] [PATCH v2 2/3] MdeModulePkg/SerialDxe: Set FIFO depth with PCD
Set UART receive FIFO depth with PCD instead of fixed number "1".
The default value of PCD is also 1, so it makes no difference for
platforms which do not explicitly set this PCD.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <heyi....@linaro.org>
Cc: Feng Tian <feng.t...@intel.com>
Cc: Star Zeng <star.z...@intel.com>
---
MdeModulePkg/Universal/SerialDxe/SerialDxe.inf | 9 +++++----
MdeModulePkg/Universal/SerialDxe/SerialIo.c | 3 ++-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
index 164060b..a1453bd 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
+++ b/MdeModulePkg/Universal/SerialDxe/SerialDxe.inf
@@ -41,10 +41,11 @@
gEfiDevicePathProtocolGuid ## PRODUCES
[Pcd]
- gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
- gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES
- gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES
- gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## CONSUMES
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth ## CONSUMES
[Depex]
TRUE
diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
index f5b3064..d2383e5 100644
--- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c
+++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c
@@ -236,7 +236,7 @@ SerialReset (
//
// Set the Serial I/O mode
//
- This->Mode->ReceiveFifoDepth = 1;
+ This->Mode->ReceiveFifoDepth = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
This->Mode->Timeout = 1000 * 1000;
This->Mode->BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
This->Mode->DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
@@ -508,6 +508,7 @@ SerialDxeInitialize (
mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits);
mSerialIoMode.Parity = (UINT32) PcdGet8 (PcdUartDefaultParity);
mSerialIoMode.StopBits = (UINT32) PcdGet8 (PcdUartDefaultStopBits);
+ mSerialIoMode.ReceiveFifoDepth = PcdGet16 (PcdUartDefaultReceiveFifoDepth);
mSerialDevicePath.Uart.BaudRate = PcdGet64 (PcdUartDefaultBaudRate);
mSerialDevicePath.Uart.DataBits = PcdGet8 (PcdUartDefaultDataBits);
mSerialDevicePath.Uart.Parity = PcdGet8 (PcdUartDefaultParity);
--
2.7.0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
