Thanks for your good comments. :) Since there is no clear description for the behavior of Reset() :(, I prone to align the behavior with MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c, that means I agree the fix.
Laszlo and Gary, if you can help do some simple regression test with the patch, that will be better. Thanks, Star -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Sunday, September 17, 2017 4:18 AM To: Zeng, Star <[email protected]>; Pankaj Bansal <[email protected]>; [email protected] Cc: Ni, Ruiyu <[email protected]> Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes On 09/16/17 03:21, Zeng, Star wrote: > Pankaj, > > Thanks for the contribution. > Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXXXXXX? > > In fact, I agree the fix as it matches the code at > MdeModulePkg/Bus/Pci/PciSioSerialDxe/SerialIo.c and > IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c. > But I am curious about how you reproduce the issue by sermode command as I > see sermode command only calls SerialIo->SetAttributes() but not > SerialIo->Reset(). > > > Ray, Laszlo, > Do you have any comment? Thanks for the CC. The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It only says, "The Reset() function resets the hardware of a serial device." It doesn't define what state the device should return to after resetting the hardware. Under the generic description of EFI_SERIAL_IO_PROTOCOL, we have "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." Now, the PCDs that are currently used in the code may differ from the above standard-mandated values, but that's the platform's responsibility. The point is that the UEFI spec seems to require that the device be returned to a predefined state, and the PCDs make that possible. I don't think that the argument in the commit message, "Serial Reset command should set the attributes which have been changed by user after calling SerialSetAttributes.", is consistent with the UEFI spec's intent. However, I could easily be wrong about this, given especially that the other two SerialIo implementations already follow the suggested practice. I guess I'll have to stay neutral on this patch. Hopefully it won't regress anything. Thanks, Laszlo > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Pankaj Bansal > Sent: Friday, September 15, 2017 9:14 PM > To: [email protected] > Cc: Pankaj Bansal <[email protected]> > Subject: [edk2] [PATCH] Fix not able to change serial attributes > > Issue : When try to change serial attributes using sermode command, the > default values are set Cause : The SerialReset command resets the attributes' > values to default Fix : Serial Reset command should set the attributes which > have been changed by user after calling SerialSetAttributes. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Pankaj Bansal <[email protected]> > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66 > ++++++++--------------------- > 1 file changed, 18 insertions(+), 48 deletions(-) > > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > index 43d33db..dc7e13a 100644 > --- a/MdeModulePkg/Universal/SerialDxe/SerialIo.c > +++ b/MdeModulePkg/Universal/SerialDxe/SerialIo.c > @@ -220,7 +220,6 @@ SerialReset ( > ) > { > EFI_STATUS Status; > - EFI_TPL Tpl; > > Status = SerialPortInitialize (); > if (EFI_ERROR (Status)) { > @@ -228,49 +227,17 @@ SerialReset ( > } > > // > - // 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 = PcdGet16 (PcdUartDefaultReceiveFifoDepth); > - This->Mode->Timeout = 1000 * 1000; > - 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 && > - mSerialDevicePath.Uart.DataBits == (UINT8) This->Mode->DataBits && > - mSerialDevicePath.Uart.Parity == (UINT8) This->Mode->Parity && > - mSerialDevicePath.Uart.StopBits == (UINT8) This->Mode->StopBits > - ) { > - gBS->RestoreTPL (Tpl); > - return EFI_SUCCESS; > - } > - > - // > - // Update the device path > + // Go set the current attributes > // > - mSerialDevicePath.Uart.BaudRate = This->Mode->BaudRate; > - mSerialDevicePath.Uart.DataBits = (UINT8) This->Mode->DataBits; > - mSerialDevicePath.Uart.Parity = (UINT8) This->Mode->Parity; > - mSerialDevicePath.Uart.StopBits = (UINT8) This->Mode->StopBits; > - > - Status = gBS->ReinstallProtocolInterface ( > - mSerialHandle, > - &gEfiDevicePathProtocolGuid, > - &mSerialDevicePath, > - &mSerialDevicePath > - ); > - > - gBS->RestoreTPL (Tpl); > + Status = This->SetAttributes ( > + This, > + This->Mode->BaudRate, > + This->Mode->ReceiveFifoDepth, > + This->Mode->Timeout, > + (EFI_PARITY_TYPE) This->Mode->Parity, > + (UINT8) This->Mode->DataBits, > + (EFI_STOP_BITS_TYPE) This->Mode->StopBits > + ); > > return Status; > } > @@ -513,11 +480,6 @@ SerialDxeInitialize ( { > EFI_STATUS Status; > > - Status = SerialPortInitialize (); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > mSerialIoMode.BaudRate = PcdGet64 (PcdUartDefaultBaudRate); > mSerialIoMode.DataBits = (UINT32) PcdGet8 (PcdUartDefaultDataBits); > mSerialIoMode.Parity = (UINT32) PcdGet8 (PcdUartDefaultParity); > @@ -529,6 +491,14 @@ SerialDxeInitialize ( > mSerialDevicePath.Uart.StopBits = PcdGet8 (PcdUartDefaultStopBits); > > // > + // Issue a reset to initialize the COM port // Status = > + mSerialIoTemplate.Reset (&mSerialIoTemplate); if (EFI_ERROR > + (Status)) { > + return Status; > + } > + > + // > // Make a new handle with Serial IO protocol and its device path on it. > // > Status = gBS->InstallMultipleProtocolInterfaces ( > -- > 2.7.4 > > _______________________________________________ > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
