From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oa1-f44.google.com (mail-oa1-f44.google.com [209.85.160.44]) by mx.groups.io with SMTP id smtpd.web11.151302.1680757229718488611 for ; Wed, 05 Apr 2023 22:00:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=DkYW/dRN; spf=pass (domain: ventanamicro.com, ip: 209.85.160.44, mailfrom: sunilvl@ventanamicro.com) Received: by mail-oa1-f44.google.com with SMTP id 586e51a60fabf-183f4efa98aso31778fac.2 for ; Wed, 05 Apr 2023 22:00:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1680757229; x=1683349229; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=6w9FoSayr9kJ1EleJ9x3pYNWj8JVjYznKEw1YHezf6c=; b=DkYW/dRNqnYCn92cTeeLa8xMPGhjzrInHI+odIxyfwdrLsha9izU73TXKj4cHY6UgK EgmxAoXKO9T0cnctAupnNOUIhDKFXhTNYiuA2IiIg5Jv0H4sKYJIgBgufl4BrPeCBMW1 ySWmdO++1tx7Q9gzwzqmLZ2h21ikf0TpqdMqV6G3/n6vx2s/9BcuCLTT9baG8THtUC0O a7Fqbun/8rhwrGzyVC+oVF65tTOocieLQhvYMwVRP8oqBjX6C5aPUWF3BrthVS0iudio mZEy0jHkkRcCv8o5AkrFSqAjaAev1X+sc3LkCeboytiYaJOuZsf73x+0siwqWHYltlIc Z+5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680757229; x=1683349229; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=6w9FoSayr9kJ1EleJ9x3pYNWj8JVjYznKEw1YHezf6c=; b=QjbUZgImkd8kXlCCoswh5hFhe2jFP1optNfpuEv1amDQOfrdKEa0PngyvALDSdQyMV M1onRBC9RpAZSAPKx7sCWnDUqMG8WtEBVh2lWcai1DlEv9H1v4IHE30A03frk3xdBi+c aH5i5kVGlsEtMORNGrf35rOwNo5X4Z5fSVrk/yKsVtIfWTAvy4VWWxhxa6JwJgmM9NnC EQbOnu9evzldSL7R1fqTuP3hkC5obqkan4IWqMFNMLKKDVcsaYS2xM24p6FS3F/odOY2 UxnNME5ly6W9ZAhb9IcAr+Q8n8AVMKFGvn5IxOqUI8hgSN192ZojL4s7c7BEPfNto+mZ XYUQ== X-Gm-Message-State: AAQBX9cweAetx8ioB/JpjoQ89Frl0jlpmIr0x3TMGkMTWr8TGfNKHLwA /OJ/zmmu+LjaeGwQT0EfvjHv3A== X-Google-Smtp-Source: AKy350aPrYuF0JZs5HqB9bZ9IMrzktbbOT3K+AzTnt541uQSMuuMUlT4Irj2HbTm2X5AfaESVdPCPw== X-Received: by 2002:a05:6870:b494:b0:17f:c80c:5731 with SMTP id y20-20020a056870b49400b0017fc80c5731mr5001353oap.18.1680757228997; Wed, 05 Apr 2023 22:00:28 -0700 (PDT) Return-Path: Received: from sunil-laptop ([106.51.83.242]) by smtp.gmail.com with ESMTPSA id ud10-20020a0568714bca00b00176d49bb898sm298428oab.44.2023.04.05.22.00.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Apr 2023 22:00:28 -0700 (PDT) Date: Thu, 6 Apr 2023 10:30:21 +0530 From: "Sunil V L" To: Andrei Warkentin Cc: devel@edk2.groups.io, Daniel Schaefer , Michael D Kinney , Liming Gao , Zhiguang Liu , Gerd Hoffmann Subject: Re: [PATCH v6 2/3] MdePkg: add SBI-based SerialPortLib for RISC-V Message-ID: References: <20230404164359.25852-1-andrei.warkentin@intel.com> <20230404164359.25852-3-andrei.warkentin@intel.com> MIME-Version: 1.0 In-Reply-To: <20230404164359.25852-3-andrei.warkentin@intel.com> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Andrei, I had couple of questions on previous version of this patch. Adding them inline again in case you had missed them. Please check if they are valid. Also, I think it would be better to have single directory BaseSerialPortLibRiscVSbiLib and inside that we can have both INF files. This way we can share UNI files etc between both libraries. On Tue, Apr 04, 2023 at 11:43:58AM -0500, Andrei Warkentin wrote: > These are implementations of SerialPortLib using SBI console services. > - BaseSerialPortLibRiscVSbiLib is appropriate for SEC/PEI (XIP) environments > - BaseSerialPortLibRiscVSbiLibRam is appropriate for PrePI/DXE environments > > Tested with: > - Qemu RiscVVirt (non-DBCN case, backed by UART) > - TinyEMU + RiscVVirt (non-DBCN case, HTIF) > - TinyEMU + RiscVVirt (DBCN case, HTIF) > > Cc: Daniel Schaefer > Cc: Sunil V L > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Acked-by: Gerd Hoffmann > Signed-off-by: Andrei Warkentin > --- > MdePkg/MdePkg.dsc | 2 + > MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.inf | 39 +++ > MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.inf | 36 +++ > MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.c | 233 ++++++++++++++++ > MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.c | 288 ++++++++++++++++++++ > MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.uni | 16 ++ > MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.uni | 16 ++ > 7 files changed, 630 insertions(+) > > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc > index 0ac7618b4623..ceccc078ff04 100644 > --- a/MdePkg/MdePkg.dsc > +++ b/MdePkg/MdePkg.dsc > @@ -192,5 +192,7 @@ [Components.ARM, Components.AARCH64] > > [Components.RISCV64] > MdePkg/Library/BaseRiscVSbiLib/BaseRiscVSbiLib.inf > + MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.inf > + MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.inf > > [BuildOptions] > diff --git a/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.inf b/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.inf > new file mode 100644 > index 000000000000..09cf59f190f6 > --- /dev/null > +++ b/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.inf > @@ -0,0 +1,39 @@ > +## @file > +# Serial Port Library backed by SBI console. > +# > +# Meant for SEC and PEI (XIP) environments. > +# > +# Due to limitations of SBI console interface and XIP environments > +# (on use of globals), this library instance does not implement reading > +# and polling the serial port. See PrePiDxeSerialPortLibRiscVSbi for > +# the full-featured variant meant for PrePi and DXE environments. > +# > +# Copyright (c) 2023, Intel Corporation. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = BaseSerialPortLibRiscVSbiLib > + MODULE_UNI_FILE = BaseSerialPortLibRiscVSbiLib.uni > + FILE_GUID = 639fad38-4bfd-4eb9-9f09-e97c7947d480 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = SerialPortLib | SEC PEI_CORE PEIM > + > + > +# > +# VALID_ARCHITECTURES = RISCV64 > +# > + > +[Sources] > + BaseSerialPortLibRiscVSbiLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + RiscVSbiLib > diff --git a/MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.inf b/MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.inf > new file mode 100644 > index 000000000000..b7ad1548a309 > --- /dev/null > +++ b/MdePkg/Library/BaseSerialPortLibRiscVSbiLibRam/BaseSerialPortLibRiscVSbiLibRam.inf > @@ -0,0 +1,36 @@ > +## @file > +# Serial Port Library backed by SBI console. > +# > +# Meant for PrePi and DXE environments (where globals are allowed). See > +# BaseSerialPortLibRiscVSbiLib for a reduced variant appropriate for SEC > +# and PEI (XIP) environments. > +# > +# Copyright (c) 2023, Intel Corporation. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = BaseSerialPortLibRiscVSbiLibRam > + MODULE_UNI_FILE = BaseSerialPortLibRiscVSbiLibRam.uni > + FILE_GUID = 872af743-ab56-45b4-a065-602567f4820c > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = SerialPortLib | SEC DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_DRIVER UEFI_APPLICATION > + > + > +# > +# VALID_ARCHITECTURES = RISCV64 > +# > + > +[Sources] > + BaseSerialPortLibRiscVSbiLibRam.c > + > +[Packages] > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + RiscVSbiLib > diff --git a/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.c b/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.c > new file mode 100644 > index 000000000000..0ad5931be3ae > --- /dev/null > +++ b/MdePkg/Library/BaseSerialPortLibRiscVSbiLib/BaseSerialPortLibRiscVSbiLib.c > @@ -0,0 +1,233 @@ > +/** @file > + Serial Port Library backed by SBI console. > + > + Meant for SEC and PEI (XIP) environments. > + > + Due to limitations of SBI console interface and XIP environments > + (on use of globals), this library instance does not implement reading > + and polling the serial port. See PrePiDxeSerialPortLibRiscVSbi for > + the full-featured variant meant for PrePi and DXE environments. > + > + Copyright (c) 2023, Intel Corporation. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +/** > + Initialize the serial device hardware. > + > + If no initialization is required, then return RETURN_SUCCESS. > + If the serial device was successfully initialized, then return RETURN_SUCCESS. > + If the serial device could not be initialized, then return RETURN_DEVICE_ERROR. > + > + @retval RETURN_SUCCESS The serial device was initialized. > + @retval RETURN_DEVICE_ERROR The serial device could not be initialized. > + > +**/ > +RETURN_STATUS > +EFIAPI > +SerialPortInitialize ( > + VOID > + ) > +{ > + return RETURN_SUCCESS; > +} > + > +/** > + Write data from buffer to serial device. > + > + Writes NumberOfBytes data bytes from Buffer to the serial device. > + The number of bytes actually written to the serial device is returned. > + If the return value is less than NumberOfBytes, then the write operation failed. > + If Buffer is NULL, then ASSERT(). > + If NumberOfBytes is zero, then return 0. > + > + @param Buffer The pointer to the data buffer to be written. > + @param NumberOfBytes The number of bytes to written to the serial device. > + > + @retval 0 NumberOfBytes is 0. > + @retval >0 The number of bytes written to the serial device. > + If this value is less than NumberOfBytes, then the write operation failed. > + > +**/ > +UINTN > +EFIAPI > +SerialPortWrite ( > + IN UINT8 *Buffer, > + IN UINTN NumberOfBytes > + ) > +{ > + SBI_RET Ret; > + UINTN Index; > + > + Ret = SbiCall (SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, 1, SBI_EXT_DBCN); > + if ((TranslateError (Ret.Error) == EFI_SUCCESS) && > + (Ret.Value != 0)) > + { > + Ret = SbiCall ( > + SBI_EXT_DBCN, > + SBI_EXT_DBCN_WRITE, > + 3, > + NumberOfBytes, > + ((UINTN)Buffer), > + 0 > + ); > + if (TranslateError (Ret.Error) != EFI_SUCCESS) { > + return 0; > + } > + > + return Ret.Value; > + } > + > + Ret = SbiCall (SBI_EXT_BASE, SBI_EXT_BASE_PROBE_EXT, 1, SBI_EXT_0_1_CONSOLE_PUTCHAR); > + if ((TranslateError (Ret.Error) == EFI_SUCCESS) && > + (Ret.Value != 0)) > + { > + for (Index = 0; Index < NumberOfBytes; Index++) { > + SbiCall (SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, 1, Buffer[Index]); What happens if it fails? Shouldn't it return immediately instead of continuing with the loop? > + } > + > + return Index; > + } > + > + /* > + * Neither DBCN or legacy extension were present. > + */ > + return NumberOfBytes; Isn't it an error? And for earlier DBCN_WRITE error, 0 is returned. So, shouldn't we return 0 here also instead of NumberOfBytes? Same questions for similar place in other library also.