On 01/15/16 17:07, Ryan Harkin wrote: > On 15 January 2016 at 15:42, Ard Biesheuvel <[email protected]> wrote: >> SerialDxe was migrated to MdeModulePkg from EmbeddedPkg, and all >> users of the latter were moved to the former. However, the new >> version is not quite identical to the original, in ways that break >> ARM platforms that use the PL011 driver. >> > > The original patch should never have passed review.
I agree it's unfortunate that the patch silently changed behavior in the course of the conversion. Stuff like that is very hard to catch in review; in my analysis that I posted a few minutes ago, I actually tried to diff the two drivers against each other, and gave up. I had to review both files in separate windows at the same time. Ard is right -- extreme discipline is required when converting things. If the programmer notices a bug or non-conformance in the course of the conversion, he should make a note somewhere, for later, but the conversion *must* preserve the original bugs! Bugfixing is a separate step. It can occur after conversion, as a separate patch. It can even occur *before* the conversion (and then the corrected code will be converted.) > > >> In SerialReset(), the serial port is reset to its default values, >> but the defaults used by the new version for ReceiveFifoDepth and >> Timeout deviate from the original values. So put them back. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <[email protected]> > Tested-by: Ryan Harkin <[email protected]> Can you please test my proposal as well? Thanks Laszlo > >> --- >> MdeModulePkg/Universal/SerialDxe/SerialIo.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> index de928d1719e9..9e9db28ce5cc 100644 >> --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c >> @@ -233,8 +233,8 @@ SerialReset ( >> // >> // Set the Serial I/O mode >> // >> - This->Mode->ReceiveFifoDepth = 1; >> - This->Mode->Timeout = 0; >> + This->Mode->ReceiveFifoDepth = 0; >> + This->Mode->Timeout = 1000000; >> This->Mode->BaudRate = PcdGet64 (PcdUartDefaultBaudRate); >> This->Mode->DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits); >> This->Mode->Parity = (UINT32) PcdGet8 (PcdUartDefaultParity); >> -- >> 2.5.0 >> > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
