public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: "Zeng, Star" <star.zeng@intel.com>,
	Pankaj Bansal <pankaj.bansal@nxp.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>
Subject: Re: [PATCH] Fix not able to change serial attributes
Date: Sat, 16 Sep 2017 22:17:38 +0200	[thread overview]
Message-ID: <135e15b4-9db2-04ac-2ae9-244bb8711945@redhat.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B969DE8@shsmsx102.ccr.corp.intel.com>

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:edk2-devel-bounces@lists.01.org] On Behalf Of Pankaj Bansal
> Sent: Friday, September 15, 2017 9:14 PM
> To: edk2-devel@lists.01.org
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> 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 <pankaj.bansal@nxp.com>
> ---
>  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
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



  reply	other threads:[~2017-09-16 20:14 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-15 13:13 [PATCH] Fix not able to change serial attributes Pankaj Bansal
2017-09-16  1:21 ` Zeng, Star
2017-09-16 20:17   ` Laszlo Ersek [this message]
2017-09-18 10:22     ` Zeng, Star
2017-09-18 10:24       ` Pankaj Bansal
2017-09-18 15:55       ` Laszlo Ersek
2017-09-19  1:57         ` Zeng, Star
2017-09-18  7:42 ` (No subject) Pankaj Bansal
2017-09-18  7:42   ` [PATCH v2] MdeModulePkg/SerialDxe: Fix not able to change serial attributes Pankaj Bansal
2017-09-18 10:11     ` Zeng, Star
2017-09-19  3:00       ` Zeng, Star
2017-09-19  3:05         ` Pankaj Bansal
2017-09-19  3:22           ` Zeng, Star

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=135e15b4-9db2-04ac-2ae9-244bb8711945@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox