From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6F89B21D046AF for ; Mon, 18 Sep 2017 03:19:02 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2017 03:22:05 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,412,1500966000"; d="scan'208";a="136518044" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga002.jf.intel.com with ESMTP; 18 Sep 2017 03:22:05 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Sep 2017 03:22:04 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Sep 2017 03:22:03 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Mon, 18 Sep 2017 18:22:01 +0800 From: "Zeng, Star" To: Laszlo Ersek , Heyi Guo , "Pankaj Bansal" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] Fix not able to change serial attributes Thread-Index: AQHTLiQ4T4coPMiVMUG9U6kJri3yPKK2tTCwgAC6GQCAAwG/UA== Date: Mon, 18 Sep 2017 10:22:01 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B970AB3@shsmsx102.ccr.corp.intel.com> References: <0C09AFA07DD0434D9E2A0C6AEB0483103B969DE8@shsmsx102.ccr.corp.intel.com> <135e15b4-9db2-04ac-2ae9-244bb8711945@redhat.com> In-Reply-To: <135e15b4-9db2-04ac-2ae9-244bb8711945@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] Fix not able to change serial attributes X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Sep 2017 10:19:02 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ag= ree the fix. Laszlo and Gary, if you can help do some simple regression test with the pa= tch, that will be better. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Sunday, September 17, 2017 4:18 AM To: Zeng, Star ; Pankaj Bansal = ; edk2-devel@lists.01.org Cc: Ni, Ruiyu Subject: Re: [edk2] [PATCH] Fix not able to change serial attributes On 09/16/17 03:21, Zeng, Star wrote: > Pankaj, >=20 > Thanks for the contribution. > Could you help update the title to be like MdeModulePkg SerialDxe: XXXXXX= XXXX? >=20 > In fact, I agree the fix as it matches the code at MdeModulePkg/Bus/Pci/P= ciSioSerialDxe/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(). >=20 >=20 > Ray, Laszlo, > Do you have any comment? Thanks for the CC. The UEFI spec is very superficial on EFI_SERIAL_IO_PROTOCOL.Reset(). It onl= y says, "The Reset() function resets the hardware of a serial device." It d= oesn't define what state the device should return to after resetting the ha= rdware. 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 ch= aracter, 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 po= int 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 attrib= utes which have been changed by user after calling SerialSetAttributes.", i= s consistent with the UEFI spec's intent. However, I could easily be wrong about this, given especially that the othe= r 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:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Pankaj Bansal > Sent: Friday, September 15, 2017 9:14 PM > To: edk2-devel@lists.01.org > Cc: Pankaj Bansal > Subject: [edk2] [PATCH] Fix not able to change serial attributes >=20 > Issue : When try to change serial attributes using sermode command, the d= efault values are set Cause : The SerialReset command resets the attributes= ' values to default Fix : Serial Reset command should set the attributes wh= ich have been changed by user after calling SerialSetAttributes. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Pankaj Bansal > --- > MdeModulePkg/Universal/SerialDxe/SerialIo.c | 66=20 > ++++++++--------------------- > 1 file changed, 18 insertions(+), 48 deletions(-) >=20 > diff --git a/MdeModulePkg/Universal/SerialDxe/SerialIo.c=20 > 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; > =20 > Status =3D SerialPortInitialize (); > if (EFI_ERROR (Status)) { > @@ -228,49 +227,17 @@ SerialReset ( > } > =20 > // > - // Set the Serial I/O mode and update the device path > - // > - > - Tpl =3D gBS->RaiseTPL (TPL_NOTIFY); > - > - // > - // Set the Serial I/O mode > - // > - This->Mode->ReceiveFifoDepth =3D PcdGet16 (PcdUartDefaultReceiveFifoD= epth); > - This->Mode->Timeout =3D 1000 * 1000; > - This->Mode->BaudRate =3D PcdGet64 (PcdUartDefaultBaudRate); > - This->Mode->DataBits =3D (UINT32) PcdGet8 (PcdUartDefaultData= Bits); > - This->Mode->Parity =3D (UINT32) PcdGet8 (PcdUartDefaultPari= ty); > - This->Mode->StopBits =3D (UINT32) PcdGet8 (PcdUartDefaultStop= Bits); > - > - // > - // Check if the device path has actually changed > - // > - if (mSerialDevicePath.Uart.BaudRate =3D=3D This->Mode->BaudRate && > - mSerialDevicePath.Uart.DataBits =3D=3D (UINT8) This->Mode->DataBit= s && > - mSerialDevicePath.Uart.Parity =3D=3D (UINT8) This->Mode->Parity = && > - mSerialDevicePath.Uart.StopBits =3D=3D (UINT8) This->Mode->StopBit= s > - ) { > - gBS->RestoreTPL (Tpl); > - return EFI_SUCCESS; > - } > - > - // > - // Update the device path > + // Go set the current attributes > // > - mSerialDevicePath.Uart.BaudRate =3D This->Mode->BaudRate; > - mSerialDevicePath.Uart.DataBits =3D (UINT8) This->Mode->DataBits; > - mSerialDevicePath.Uart.Parity =3D (UINT8) This->Mode->Parity; > - mSerialDevicePath.Uart.StopBits =3D (UINT8) This->Mode->StopBits; > - > - Status =3D gBS->ReinstallProtocolInterface ( > - mSerialHandle, > - &gEfiDevicePathProtocolGuid, > - &mSerialDevicePath, > - &mSerialDevicePath > - ); > - > - gBS->RestoreTPL (Tpl); > + Status =3D 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 > + ); > =20 > return Status; > } > @@ -513,11 +480,6 @@ SerialDxeInitialize ( { > EFI_STATUS Status; > =20 > - Status =3D SerialPortInitialize (); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > mSerialIoMode.BaudRate =3D PcdGet64 (PcdUartDefaultBaudRate); > mSerialIoMode.DataBits =3D (UINT32) PcdGet8 (PcdUartDefaultDataBits); > mSerialIoMode.Parity =3D (UINT32) PcdGet8 (PcdUartDefaultParity); > @@ -529,6 +491,14 @@ SerialDxeInitialize ( > mSerialDevicePath.Uart.StopBits =3D PcdGet8 (PcdUartDefaultStopBits); > =20 > // > + // Issue a reset to initialize the COM port // Status =3D=20 > + mSerialIoTemplate.Reset (&mSerialIoTemplate); if (EFI_ERROR=20 > + (Status)) { > + return Status; > + } > + > + // > // Make a new handle with Serial IO protocol and its device path on it= . > // > Status =3D gBS->InstallMultipleProtocolInterfaces ( > -- > 2.7.4 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >=20 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel