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

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

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