> -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, January 15, 2016 9:22 AM > To: Ryan Harkin <ryan.har...@linaro.org>; Ard Biesheuvel > <ard.biesheu...@linaro.org> > Cc: edk2-de...@ml01.01.org; Zeng, Star <star.z...@intel.com>; Leif > Lindholm <leif.lindh...@linaro.org> > Subject: Re: [edk2] [PATCH] MdeModulePkg/SerialDxe: fix porting error from > EmbeddedPkg > > On 01/15/16 17:07, Ryan Harkin wrote: > > On 15 January 2016 at 15:42, Ard Biesheuvel <ard.biesheu...@linaro.org> > 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.) > Agree.
Isn't there a SVN->Move operation (and a GIT version) that should have been used and would inherently have maintained any and all behaviors? Then if there are name changes required for PCDs or the like they can be changed after the move in the patch series? We researched better move methods than "add then delete" after the move of libraries from ShellPkg to MdeModulePkg. Another major benefit was continuity of file history... -Jaben > > > > > >> 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 <ard.biesheu...@linaro.org> > > Tested-by: Ryan Harkin <ryan.har...@linaro.org> > > 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 > > 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
