From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: hao.a.wu@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Sun, 14 Apr 2019 22:56:31 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Apr 2019 22:56:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,352,1549958400"; d="scan'208";a="149466767" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 14 Apr 2019 22:56:30 -0700 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 14 Apr 2019 22:56:30 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 14 Apr 2019 22:56:30 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.92]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.164]) with mapi id 14.03.0415.000; Mon, 15 Apr 2019 13:56:28 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: "Ni, Ray" Subject: Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Thread-Topic: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Thread-Index: AQHU5W4RTm/r+LRx+EqLhxrOxnMTvaYiS15ggBNyaYCAAmrVUIAAk64AgAQOfMA= Date: Mon, 15 Apr 2019 05:56:27 +0000 Message-ID: References: <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com> <92CF190FF2351747A47C1708F0E09C0875DEA729@IRSMSX102.ger.corp.intel.com> In-Reply-To: <92CF190FF2351747A47C1708F0E09C0875DEA729@IRSMSX102.ger.corp.intel.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 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Albecki, Mateusz > Sent: Saturday, April 13, 2019 7:23 AM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Ni, Ray > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info > protocol >=20 > I wasn't really thinking about efi code that can come in from device. I w= as > thinking more of a discrete device that you can solder down on the board = and > manufacturer of such device can distribute a efi binary with it that will= contain > only this small piece of code instead of the full blown driver. Single in= stance is Hello Mateusz, Judging from the existing cases like ATA and SD/MMC, (MdeModulePkg/Bus/Ata/AtaAtapiPassThru, MdeModulePkg/Bus/Pci/SdMmcPciHcDxe) it seems that we have not met such request yet. Since this one will not bring big impact to the driver implementation, how about make this protocol a platform singleton at this moment (using a global variable to store the protocol instance)? We may update the codes if there comes an actual request. How does this sound to you? > also OK it is just that I think it is slightly less comfortable for produ= cers of the > protocol. >=20 > I can't think of anything outside of DME_SET that would make sense but yo= u are > right that specification doesn't forbid other opcodes. Maybe we need to b= e a > little more flexible here and introduce different protocol to handle that= ? How > about something like this >=20 > UFS_HC_INFO_PROTOCOL { > Supported // If we do decide to go with multi instance > GetNextCommand > CommandFinishedCallback (This, CommandResults) > } >=20 > I think this would be able to support DME_GET commands. Let me know what > you think. For this one I was thinking commands like DME_PEER_SET (or other commands maybe added in future) which does not require subsequent process by the system host. Handling commands like DME_GET seems a bit overshot to me. Best Regards, Hao Wu >=20 > Thanks, > Mateusz >=20 > -----Original Message----- > From: Wu, Hao A > Sent: Friday, April 12, 2019 8:35 AM > To: Albecki, Mateusz ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info > protocol >=20 > Resend this one to the new mailing list. >=20 > Hello Mateusz, >=20 > Incline comments below. >=20 > > -----Original Message----- > > From: Albecki, Mateusz > > Sent: Thursday, April 11, 2019 9:39 AM > > To: Wu, Hao A; edk2-devel@lists.01.org > > Cc: Ni, Ray > > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info > > protocol > > > > Hi, > > > > I will be addressing most of the comments in next patch however I want > > to discuss 2 things. > > > > 1. I really thing that supporting multiple instances of the protocol > > is a better way here. I can imagine a board which has 2 UFS > > controllers one integrated into the SoC and the second one connected > > through PCI(maybe for debug or maybe that's the production > > configuration due to problems with the integrated controller). I would > > like the company that produced the discrete UFS controller to be able > > to deliver a efi binary which will install this protocol just for > > their device(identified by device id) while not conflicting with the > > code that installs configuration for the integrated controller. With > > single instance we would need BIOS team working on the product to > > modify their code instead of just dropping in the binary. This use > > case is a little exotic but I think it's valid especially if we ever > > consider extending this protocol. Choosing the first instance that retu= rns > supported should be clear once I add description in the protocol header. >=20 > For controllers attached through the PCI, my understanding is that this b= inary > will reside in the option ROM. If the device produce company want to > customize the initialization process, I think they can directly put a dif= ferent > controller/device driver binary in the ROM instead. The Bus Specific Driv= er > Override Protocol ensures the driver in the option ROM will manage the de= vice. > So I think for BIOS, it is reasonable to focus on the integrated one. >=20 > > > > 2. About making the service more general and allowing the producer of > > the protocol to choose UIC command to execute. I only tried to satisfy > > the UFS specification and I am not sure what possible use cases > > platform could have that would require sending commands other than > > DME_SET so maybe we should refrain from extending it now? Extension > > would also raise questions how to handle DME_GET command for instance > > since right now we do not have any callback to platform producer. >=20 > I am not very sure about this one, what I found in the UFSHCI spec seems = that > the spec does not explicitly forbid other UIC to be sent. >=20 > According to UFSHCI spec Version 2.1, Section 7.1.1: >=20 > > Additional commands, such as DME_SET commands may be sent from the > > system host to the UFS host controller to provide configuration > > flexibility. >=20 > If you think commands other than 'DME_SET' will not make much sense here,= I > am okay with the current 'UIC_ATTRIBUTE' definition. >=20 > (I slightly prefer using 'Arg1'~'Arg3' in structure 'UIC_ATTRIBUTE', it s= eems a > little bit cleaner to me. But this is only my thought and you may choose = your > own implementation for this.) >=20 > Best Regards, > Hao Wu >=20 > > > > Thanks, > > Mateusz > > > > -----Original Message----- > > From: Wu, Hao A > > Sent: Monday, April 1, 2019 8:53 AM > > To: Albecki, Mateusz ; > > edk2-devel@lists.01.org > > Cc: Ni, Ray > > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info > > protocol > > > > Hello Mateusz, > > > > A couple of general comments: > > > > A. I suggest to break this commit into 2 patches: > > > > The first one will just introduce the new protocol. > > The second one will update the UfsPassThruDxe driver to consume this > > new protocol. > > > > > > B. There has been a Bugzilla tracker for the feature you add in this > > patch: > > https://bugzilla.tianocore.org/show_bug.cgi?id=3D1343 > > > > Could you help to add this information in the commit log message? Thank= s. > > > > > > C. I prefer the new protocol to have platform level singleton instance: > > > > I do not see much value for a platform to produce multiple instances > > of this protocol, I think the additional UIC commands needed during > > the UFS HC initialization for a specific platform is deterministic. > > > > Also, the selection of protocol instance implemented in function > > GetUfsHcInfoProtocol() is by: > > 1) LocateHandleBuffer() to get all protocol instances; > > 2) Choose the 1st instance if the 'Supported' service returns without > > error. > > > > I think this will bring some kind of confusion to the protocol > > producers, since they do not know which one will be selected when > > there are multiple instances of the protocol. > > > > > > > -----Original Message----- > > > From: Albecki, Mateusz > > > Sent: Thursday, March 28, 2019 9:56 PM > > > To: edk2-devel@lists.01.org > > > Cc: Albecki, Mateusz; Wu, Hao A > > > Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info > > > protocol > > > > > > UFS host controller specification allows for implementation specific > > > UIC programming to take place just after host controller enable and > > > before device detection. Since there is no way for generic driver to > > > anticipate implementation specific programming we add a UFS info > > > protocol which allows the implementation specific code to pass this > > > information to generic driver. UFS info protocol is located by the > > > driver at the BindingStart call and the supported instance is > > > retained throught entire > > > > throught -> through > > > > > > > driver lifetime. Presence of the protocol is optional. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Cc: Hao Wu > > > Signed-off-by: Albecki Mateusz > > > --- > > > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 62 > > > ++++++++++++++++++ > > > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h | 2 + > > > .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf | 1 + > > > .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 60 > > +++++++++++++++++ > > > .../Include/Protocol/UfsHostControllerInfo.h | 75 > > > ++++++++++++++++++++++ > > > MdeModulePkg/MdeModulePkg.dec | 3 + > > > 6 files changed, 203 insertions(+) > > > create mode 100644 > > > MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h > > > > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > index ea329618dc..a41079e311 100644 > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA > > gUfsPassThruTemplate =3D { > > > UfsRwUfsAttribute > > > }, > > > 0, // UfsHostController > > > + NULL, // UfsInfo > > > 0, // UfsHcBase > > > 0, // Capabilities > > > 0, // TaskTag > > > @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization ( > > > return EFI_SUCCESS; > > > } > > > > > > +/** > > > + Locates UFS HC infor protocol suitable for this controller. > > > > infor -> info > > > > > > > + > > > + @param[in] DriverBinding Pointer to driver binding protocol > > > + @param[in] Private Pointer to host controller private da= ta > > > + @param[in] ControllerHandle Controller to which driver is bound > > > +**/ VOID GetUfsHcInfoProtocol ( > > > + IN EFI_DRIVER_BINDING_PROTOCOL *DriverBinding, > > > + IN UFS_PASS_THRU_PRIVATE_DATA *Private, > > > + IN EFI_HANDLE ControllerHandle > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + EFI_HANDLE *ProtocolHandleArray; > > > + UINTN NoHandles; > > > + UINTN HandleIndex; > > > + UFS_HC_INFO_PROTOCOL *UfsHcInfo; > > > + > > > + Status =3D gBS->LocateHandleBuffer ( > > > + ByProtocol, > > > + &gUfsHcInfoProtocolGuid, > > > + NULL, > > > + &NoHandles, > > > + &ProtocolHandleArray > > > + ); > > > + if (EFI_ERROR (Status)) { > > > + return; > > > + } > > > + > > > + for (HandleIndex =3D 0; HandleIndex < NoHandles; HandleIndex++) { > > > + Status =3D gBS->OpenProtocol ( > > > + ProtocolHandleArray[HandleIndex], > > > + &gUfsHcInfoProtocolGuid, > > > + &UfsHcInfo, > > > + DriverBinding->DriverBindingHandle, > > > + ControllerHandle, > > > + EFI_OPEN_PROTOCOL_BY_DRIVER > > > + ); > > > + if (EFI_ERROR (Status)) { > > > + continue; > > > + } > > > + if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle= ))) { > > > + Private->UfsHcInfo =3D UfsHcInfo; > > > + break; > > > + } else { > > > + Status =3D gBS->CloseProtocol ( > > > + ProtocolHandleArray[HandleIndex], > > > + &gUfsHcInfoProtocolGuid, > > > + DriverBinding->DriverBindingHandle, > > > + ControllerHandle > > > + ); > > > + ASSERT_EFI_ERROR (Status); > > > + } > > > + } > > > + > > > + FreePool (ProtocolHandleArray); > > > +} > > > + > > > /** > > > Starts a device controller or a bus controller. > > > > > > @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart ( > > > Private->UfsHostController =3D UfsHc; > > > Private->UfsHcBase =3D UfsHcBase; > > > InitializeListHead (&Private->Queue); > > > + GetUfsHcInfoProtocol (This, Private, Controller); > > > > > > // > > > // Initialize UFS Host Controller H/W. > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > > > index e591e78661..55a8ed9bdf 100644 > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h > > > @@ -29,6 +29,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > A minor comment here: > > You can put the above '#include' together with other protocol header > > files rather than appending it after the libraries includes. > > > > > > > > > > #include "UfsPassThruHci.h" > > > > > > @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA { > > > EFI_EXT_SCSI_PASS_THRU_PROTOCOL ExtScsiPassThru; > > > EFI_UFS_DEVICE_CONFIG_PROTOCOL UfsDevConfig; > > > EDKII_UFS_HOST_CONTROLLER_PROTOCOL *UfsHostController; > > > + UFS_HC_INFO_PROTOCOL *UfsHcInfo; > > > UINTN UfsHcBase; > > > UINT32 Capabilities; > > > > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf > > > index e550cd02b4..12b01771ff 100644 > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf > > > @@ -59,6 +59,7 @@ > > > gEfiExtScsiPassThruProtocolGuid ## BY_START > > > gEfiUfsDeviceConfigProtocolGuid ## BY_START > > > gEdkiiUfsHostControllerProtocolGuid ## TO_START > > > + gUfsHcInfoProtocolGuid > > > > gUfsHcInfoProtocolGuid -> > > gUfsHcInfoProtocolGuid ## SOMETIMES_CONSUMES > > > > > > > > > > [UserExtensions.TianoCore."ExtraFiles"] > > > UfsPassThruExtra.uni > > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > index c37161c4ae..125f2f2516 100644 > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList ( > > > return EFI_SUCCESS; > > > } > > > > > > +/** > > > + Performs additional UIC programming if it has been specified by > > > +platform in > > > UFS info protocol. > > > + > > > + @param[in] Private Pointer to host controller private data > > > + > > > + @retval EFI_SUCCESS Programming finished successfully or not > > requested > > > + @retval others Programming failed > > > +**/ > > > +EFI_STATUS > > > +UfsProgramAdditionalUicAttributes ( > > > + IN UFS_PASS_THRU_PRIVATE_DATA *Private > > > + ) > > > +{ > > > + EFI_STATUS Status; > > > + UIC_ATTRIBUTE *UicAttrArray; > > > + UINTN NoAttributes; > > > + UINTN AttributeIndex; > > > + > > > + // > > > + // No info protocol means that no additional programming should > > > + be > > > performed > > > + // > > > + if (Private->UfsHcInfo =3D=3D NULL) { > > > + return EFI_SUCCESS; > > > + } > > > + > > > + // > > > + // If we failed to get the programming table we assume that > > > + platform > > > doesn't want to do any additional programming > > > + // > > > + Status =3D Private->UfsHcInfo->GetUicProgrammingTable > > > + (Private->UfsHcInfo, > > > &UicAttrArray, &NoAttributes); > > > + if (EFI_ERROR (Status)) { > > > + return EFI_SUCCESS; > > > + } > > > > Please help to free the content pointed by 'UicAttrArray'. > > > > > > > + > > > + for (AttributeIndex =3D 0; AttributeIndex < NoAttributes; > > > + AttributeIndex++) > > { > > > + DEBUG ((DEBUG_ERROR, "Programing UIC attribute =3D %d, selector > > > + index > > > > DEBUG_INFO should be enough. > > > > > > > =3D %d, set type =3D %d, value =3D %d\n", > > > + > > > + UicAttrArray[AttributeIndex].MibAttribute, > > > UicAttrArray[AttributeIndex].GenSelectorIndex, > > > + UicAttrArray[AttributeIndex].AttrSetType, > > > UicAttrArray[AttributeIndex].AttributeValue)); > > > + Status =3D UfsExecUicCommands ( > > > + Private, > > > + UfsUicDmeSet, > > > + (UicAttrArray[AttributeIndex].MibAttribute << 16) | > > > (UicAttrArray[AttributeIndex].GenSelectorIndex), > > > + UicAttrArray[AttributeIndex].AttrSetType << 16, > > > + UicAttrArray[AttributeIndex].AttributeValue > > > + ); > > > + if (EFI_ERROR (Status)) { > > > + DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute =3D %d, > > > + selector > > > index =3D %d, set type =3D %d, value =3D %d\n", > > > + > > > + UicAttrArray[AttributeIndex].MibAttribute, > > > UicAttrArray[AttributeIndex].GenSelectorIndex, > > > + > > > + UicAttrArray[AttributeIndex].AttrSetType, > > > UicAttrArray[AttributeIndex].AttributeValue)); > > > + return Status; > > > + } > > > + } > > > + > > > + return EFI_SUCCESS; > > > +} > > > + > > > /** > > > Initialize the UFS host controller. > > > > > > @@ -2090,6 +2145,11 @@ UfsControllerInit ( > > > return Status; > > > } > > > > > > + Status =3D UfsProgramAdditionalUicAttributes (Private); if > > > + (EFI_ERROR (Status)) { > > > + DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform > > > + additional > > > UIC programming\n")); > > > + } > > > + > > > Status =3D UfsDeviceDetection (Private); > > > if (EFI_ERROR (Status)) { > > > DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection > > > Fails, Status =3D %r\n", Status)); diff --git > > > a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h > > > b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h > > > new file mode 100644 > > > index 0000000000..c730127482 > > > --- /dev/null > > > +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h > > > @@ -0,0 +1,75 @@ > > > +/** @file > > > + > > > + Universal Flash Storage Host Controller info Protocol. > > > + > > > +Copyright (c) 2019, Intel Corporation. All rights reserved.
> > > +This program and the accompanying materials are licensed and made > > > available under > > > +the terms and conditions of the BSD License that accompanies this > > distribution. > > > +The full text of the license may be found at > > > +http://opensource.org/licenses/bsd-license.php. > > > + > > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > BASIS, > > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > EXPRESS > > > OR IMPLIED. > > > + > > > +**/ > > > + > > > +#ifndef __UFS_HC_INFO_PROTOCOL_H__ > > > +#define __UFS_HC_INFO_PROTOCOL_H__ > > > + > > > +#define UFS_HC_INFO_PROTOCOL_VERSION 1 > > > + > > > +extern EFI_GUID gUfsHcInfoProtocolGuid; > > > > I suggest to rename gUfsHcInfoProtocolGuid to: > > gEdkiiUfsHcInfoProtocolGuid > > > > which I think is a common naming convention for non UEFI spec > > protocols within MdeModulePkg. > > > > > > > + > > > +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL; > > > + > > > +typedef struct { > > > + UINT16 MibAttribute; > > > + UINT16 GenSelectorIndex; > > > + UINT8 AttrSetType; > > > + UINT32 AttributeValue; > > > +} UIC_ATTRIBUTE; > > > + > > > +/** > > > + Checks if this instance of the info protocol supports > > > + given host controller. > > > + > > > + @param[in] This Pointer to this instance of > > > UFS_HC_INFO_PROTOCOL > > > + @param[in] ControllerHandle Controller to check > > > + > > > + @retval EFI_SUCCESS This instance of info protocol supports > > > + given > > controller > > > + @retval others This instance of info protocol doesn't suppor= t given > > > controller > > > +**/ > > > +typedef > > > +EFI_STATUS > > > +(*UFS_INFO_CONTROLLER_SUPPORTED) ( > > > + IN UFS_HC_INFO_PROTOCOL *This, > > > + IN EFI_HANDLE ControllerHandle > > > + ); > > > + > > > +/** > > > + Get the UIC programming table to be used just after host > > > +controller > > > + enabling and before device detection. > > > + > > > + @param[in] This Pointer to this instance of > > > UFS_HC_INFO_PROTOCOL > > > + @param[in] UicAttributeArray Out variable for address of the tabl= e > > > + @param[in] NoAttributes Out variable for number of attribute= s in the > > > array > > > > '@param[out]' for UicAttributeArray & NoAttributes. > > > > Also, for 'UicAttributeArray', I think the content pointed by this > > pointer is allocated by the protocol producer and should be freed by > > the consumer after being used, right? > > > > If so, please help to refine the description comments for 'UicAttribute= Array'. > > > > > > > + > > > + @retval EFI_SUCCESS Table successfully found and returned > > > + @retval others Table wasn't located successfully UIC program= ming > > > shouldn't be performed. > > > +**/ > > > +typedef > > > +EFI_STATUS > > > +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) ( > > > + IN UFS_HC_INFO_PROTOCOL *This, > > > + OUT UIC_ATTRIBUTE **UicAttributeArray, > > > + OUT UINTN *NoAttributes > > > + ); > > > > Judging from the usage of this service in function > > UfsProgramAdditionalUicAttributes(), I think there is an assumption > > that platforms will only require additional 'DME_SET' UIC commands. > > > > I am not sure if we need to make this service a bit more general. So > > how about changing the 'UIC_ATTRIBUTE' structure to: > > > > typedef struct { > > UINT8 UicOpcode; > > UINT32 Arg1; > > UINT32 Arg2; > > UINT32 Arg3; > > } UIC_COMMAND; > > > > and update the service to: > > > > typedef > > EFI_STATUS > > (*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) ( > > IN UFS_HC_INFO_PROTOCOL *This, > > OUT UIC_COMMAND **UicCommandArray, > > OUT UINTN *NoCommand > > ); > > > > > > > + > > > +struct _UFS_HC_INFO_PROTOCOL { > > > + UINT32 Version; > > > + UFS_INFO_CONTROLLER_SUPPORTED Supported; > > > > If there will be only one protocol instance within system, do you > > think we can drop the above 'Supported' service? > > > > Best Regards, > > Hao Wu > > > > > > > + UFS_INFO_GET_UIC_PROGRAMMING_TABLE > > GetUicProgrammingTable; }; > > > + > > > +#endif > > > + > > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > > b/MdeModulePkg/MdeModulePkg.dec index a2130bc439..c6be8f12a4 > > 100644 > > > --- a/MdeModulePkg/MdeModulePkg.dec > > > +++ b/MdeModulePkg/MdeModulePkg.dec > > > @@ -581,6 +581,9 @@ > > > ## Include/Protocol/UfsHostController.h > > > gEdkiiUfsHostControllerProtocolGuid =3D { 0xebc01af5, 0x7a9, > > > 0x489e, { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } } > > > > > > + ## Include/Protocol/UfsHostControllerInfo.h > > > + gUfsHcInfoProtocolGuid =3D { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, > > > + 0x16, 0xd3, > > > 0x07, 0x96, 0x53, 0x9e, 0xd8}} > > > + > > > ## Include/Protocol/EsrtManagement.h > > > gEsrtManagementProtocolGuid =3D { 0xa340c064, 0x723c, 0x4a= 9c, > { 0xa4, > > > 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }} > > > > > > -- > > > 2.14.1.windows.1