Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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
> 
> I wasn't really thinking about efi code that can come in from device. I was
> 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 instance 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 producers of the
> protocol.
> 
> I can't think of anything outside of DME_SET that would make sense but you are
> right that specification doesn't forbid other opcodes. Maybe we need to be a
> little more flexible here and introduce different protocol to handle that? How
> about something like this
> 
> UFS_HC_INFO_PROTOCOL {
>   Supported // If we do decide to go with multi instance
>   GetNextCommand
>   CommandFinishedCallback (This, CommandResults)
> }
> 
> 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

> 
> Thanks,
> Mateusz
> 
> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, April 12, 2019 8:35 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> protocol
> 
> Resend this one to the new mailing list.
> 
> Hello Mateusz,
> 
> Incline comments below.
> 
> > -----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 returns
> supported should be clear once I add description in the protocol header.
> 
> For controllers attached through the PCI, my understanding is that this binary
> will reside in the option ROM. If the device produce company want to
> customize the initialization process, I think they can directly put a different
> controller/device driver binary in the ROM instead. The Bus Specific Driver
> Override Protocol ensures the driver in the option ROM will manage the device.
> So I think for BIOS, it is reasonable to focus on the integrated one.
> 
> >
> > 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.
> 
> 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.
> 
> According to UFSHCI spec Version 2.1, Section 7.1.1:
> 
> > Additional commands, such as DME_SET commands may be sent from the
> > system host to the UFS host controller to provide configuration
> > flexibility.
> 
> If you think commands other than 'DME_SET' will not make much sense here, I
> am okay with the current 'UIC_ATTRIBUTE' definition.
> 
> (I slightly prefer using 'Arg1'~'Arg3' in structure 'UIC_ATTRIBUTE', it seems a
> little bit cleaner to me. But this is only my thought and you may choose your
> own implementation for this.)
> 
> Best Regards,
> Hao Wu
> 
> >
> > Thanks,
> > Mateusz
> >
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Monday, April 1, 2019 8:53 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ray <ray.ni@intel.com>
> > 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=1343
> >
> > Could you help to add this information in the commit log message? Thanks.
> >
> >
> > 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 <hao.a.wu@intel.com>
> > > Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> > > ---
> > >  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 = {
> > >      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 data
> > > +  @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 = gBS->LocateHandleBuffer (
> > > +                  ByProtocol,
> > > +                  &gUfsHcInfoProtocolGuid,
> > > +                  NULL,
> > > +                  &NoHandles,
> > > +                  &ProtocolHandleArray
> > > +                  );
> > > +  if (EFI_ERROR (Status)) {
> > > +    return;
> > > +  }
> > > +
> > > +  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> > > +    Status = 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 = UfsHcInfo;
> > > +      break;
> > > +    } else {
> > > +      Status = 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    = UfsHc;
> > >    Private->UfsHcBase            = 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 <Library/UefiBootServicesTableLib.h>
> > >  #include <Library/DevicePathLib.h>
> > >  #include <Library/TimerLib.h>
> > > +#include <Protocol/UfsHostControllerInfo.h>
> >
> > 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 == 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 = 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 = 0; AttributeIndex < NoAttributes;
> > > + AttributeIndex++)
> > {
> > > +    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector
> > > + index
> >
> > DEBUG_INFO should be enough.
> >
> >
> > > = %d, set type = %d, value = %d\n",
> > > +
> > > + UicAttrArray[AttributeIndex].MibAttribute,
> > > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > > +                          UicAttrArray[AttributeIndex].AttrSetType,
> > > UicAttrArray[AttributeIndex].AttributeValue));
> > > +    Status = 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 = %d,
> > > + selector
> > > index = %d, set type = %d, value = %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 = UfsProgramAdditionalUicAttributes (Private);  if
> > > + (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform
> > > + additional
> > > UIC programming\n"));
> > > +  }
> > > +
> > >    Status = UfsDeviceDetection (Private);
> > >    if (EFI_ERROR (Status)) {
> > >      DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection
> > > Fails, Status = %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.<BR>
> > > +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 support 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 table
> > > +  @param[in] NoAttributes       Out variable for number of attributes 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 'UicAttributeArray'.
> >
> >
> > > +
> > > +  @retval EFI_SUCCESS  Table successfully found and returned
> > > +  @retval others       Table wasn't located successfully UIC programming
> > > 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 = { 0xebc01af5, 0x7a9,
> > > 0x489e, { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
> > >
> > > +  ## Include/Protocol/UfsHostControllerInfo.h
> > > +  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9,
> > > + 0x16, 0xd3,
> > > 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> > > +
> > >    ## Include/Protocol/EsrtManagement.h
> > >    gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c,
> { 0xa4,
> > > 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
> > >
> > > --
> > > 2.14.1.windows.1