Re: [PATCH 4/4] MdeModulePkg/UfsPassThruDxe: Implement EDKII_UFS_HC_PLATFORM_PROTOCOL

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

Hello Mateusz,

One inline comment below:


> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, August 08, 2019 12:51 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [PATCH 4/4] MdeModulePkg/UfsPassThruDxe: Implement
> EDKII_UFS_HC_PLATFORM_PROTOCOL
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> 
> This commit adds EDKII_UFS_HC_PLATFORM_PROTOCOL implementation
> in UfsPassThruDxe driver in version 1. Driver assumes that at
> most one instance of the protocol exists in the system. Presence
> of the protocol is not mandatory.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 17 ++++++
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  | 26 +++++++++
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  3 +-
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 68
> ++++++++++++++++++++++
>  4 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> index 09333c51d6..1559efe191 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> @@ -36,6 +36,7 @@ UFS_PASS_THRU_PRIVATE_DATA
> gUfsPassThruTemplate = {
>    0,                              // UfsHostController
>    0,                              // UfsHcBase
>    {0, 0},                         // UfsHcInfo
> +  {NULL, NULL},                   // UfsHcDriverInterface
>    0,                              // TaskTag
>    0,                              // UtpTrlBase
>    0,                              // Nutrs
> @@ -92,6 +93,8 @@ UFS_DEVICE_PATH    mUfsDevicePathTemplate = {
> 
>  UINT8 mUfsTargetId[TARGET_MAX_BYTES];
> 
> +GLOBAL_REMOVE_IF_UNREFERENCED
> EDKII_UFS_HC_PLATFORM_PROTOCOL  *mUfsHcPlatform;
> +
>  /**
>    Sends a SCSI Request Packet to a SCSI device that is attached to the SCSI
> channel. This function
>    supports both blocking I/O and nonblocking I/O. The blocking I/O
> functionality is required, and the
> @@ -864,7 +867,21 @@ UfsPassThruDriverBindingStart (
>    Private->ExtScsiPassThru.Mode = &Private->ExtScsiPassThruMode;
>    Private->UfsHostController    = UfsHc;
>    Private->UfsHcBase            = UfsHcBase;
> +  Private->Handle               = Controller;
> +  Private->UfsHcDriverInterface.UfsHcProtocol = UfsHc;
> +  Private->UfsHcDriverInterface.UfsExecUicCommand =
> UfsHcDriverInterfaceExecUicCommand;
>    InitializeListHead (&Private->Queue);
> +
> +  //
> +  // This has to be done before initializing UfsHcInfo or calling the
> UfsControllerInit
> +  //
> +  if (mUfsHcPlatform == NULL) {
> +    Status = gBS->LocateProtocol (&gEdkiiUfsHcPlatformProtocolGuid, NULL,
> (VOID**)&mUfsHcPlatform);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_INFO, "No UfsHcPlatformProtocol present\n"));
> +    }
> +  }
> +
>    Status = GetUfsHcInfo (Private);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "Failed to initialize UfsHcInfo\n"));
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index c511aa8c7a..cbc0c2126e 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -63,6 +63,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
>    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
>    UINTN                               UfsHcBase;
>    EDKII_UFS_HC_INFO                   UfsHcInfo;
> +  EDKII_UFS_HC_DRIVER_INTERFACE       UfsHcDriverInterface;
> 
>    UINT8                               TaskTag;
> 
> @@ -126,6 +127,13 @@ typedef struct {
>        UFS_PASS_THRU_SIG \
>        )
> 
> +#define UFS_PASS_THRU_PRIVATE_DATA_FROM_DRIVER_INTF(a) \
> +  CR (a, \
> +      UFS_PASS_THRU_PRIVATE_DATA, \
> +      UfsHcDriverInterface, \
> +      UFS_PASS_THRU_SIG \
> +      )
> +
>  typedef struct _UFS_DEVICE_MANAGEMENT_REQUEST_PACKET {
>    UINT64           Timeout;
>    VOID             *DataBuffer;
> @@ -959,6 +967,23 @@ UfsRwUfsAttribute (
>    IN OUT UINT32                        *AttrSize
>    );
> 
> +/**
> +  Execute UIC command.
> +
> +  @param[in]      This        Pointer to driver interface produced by the UFS
> controller.
> +  @param[in, out] UicCommand  Descriptor of the command that will be
> executed.
> +
> +  @retval EFI_SUCCESS            Command executed successfully.
> +  @retval EFI_INVALID_PARAMETER  This or UicCommand is NULL.
> +  @retval Others                 Command failed to execute.
> +**/
> +EFI_STATUS
> +EFIAPI
> +UfsHcDriverInterfaceExecUicCommand (
> +  IN     EDKII_UFS_HC_DRIVER_INTERFACE  *This,
> +  IN OUT EDKII_UIC_COMMAND              *UicCommand
> +  );
> +
>  /**
>    Initializes UfsHcInfo field in private data.
> 
> @@ -975,5 +1000,6 @@ GetUfsHcInfo (
>  extern EFI_COMPONENT_NAME_PROTOCOL
> gUfsPassThruComponentName;
>  extern EFI_COMPONENT_NAME2_PROTOCOL
> gUfsPassThruComponentName2;
>  extern EFI_DRIVER_BINDING_PROTOCOL  gUfsPassThruDriverBinding;
> +extern EDKII_UFS_HC_PLATFORM_PROTOCOL  *mUfsHcPlatform;
> 
>  #endif
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> index 24f5ea3a8f..92dc25714b 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  # Description file for the Universal Flash Storage (UFS) Pass Thru driver.
>  #
> -# Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -54,6 +54,7 @@
>    gEfiExtScsiPassThruProtocolGuid               ## BY_START
>    gEfiUfsDeviceConfigProtocolGuid               ## BY_START
>    gEdkiiUfsHostControllerProtocolGuid           ## TO_START
> +  gEdkiiUfsHcPlatformProtocolGuid               ## SOMETIMES_CONSUMES
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
>    UfsPassThruExtra.uni
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 74be3efc41..e2c104b103 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -1835,6 +1835,14 @@ UfsEnableHostController (
>    EFI_STATUS             Status;
>    UINT32                 Data;
> 
> +  if (mUfsHcPlatform != NULL && mUfsHcPlatform->Callback != NULL) {
> +    Status = mUfsHcPlatform->Callback (Private->Handle, EdkiiUfsHcPreHce,
> &Private->UfsHcDriverInterface);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPreHce, Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> +
>    //
>    // UFS 2.0 spec section 7.1.1 - Host Controller Initialization
>    //
> @@ -1878,6 +1886,14 @@ UfsEnableHostController (
>      return EFI_DEVICE_ERROR;
>    }
> 
> +  if (mUfsHcPlatform != NULL && mUfsHcPlatform->Callback != NULL) {
> +    Status = mUfsHcPlatform->Callback (Private->Handle, EdkiiUfsHcPostHce,
> &Private->UfsHcDriverInterface);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPostHce, Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> +
>    return EFI_SUCCESS;
>  }
> 
> @@ -1901,6 +1917,14 @@ UfsDeviceDetection (
>    UINT32             Data;
>    EDKII_UIC_COMMAND  LinkStartupCommand;
> 
> +  if (mUfsHcPlatform != NULL && mUfsHcPlatform->Callback != NULL) {
> +    Status = mUfsHcPlatform->Callback (Private->Handle,
> EdkiiUfsHcPreLinkStartup, &Private->UfsHcDriverInterface);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPreLinkStartup, Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> +
>    //
>    // Start UFS device detection.
>    // Try up to 3 times for establishing data link with device.
> @@ -1930,6 +1954,14 @@ UfsDeviceDetection (
>      }
>    }
> 
> +  if (mUfsHcPlatform != NULL && mUfsHcPlatform->Callback != NULL) {
> +    Status = mUfsHcPlatform->Callback (Private->Handle,
> EdkiiUfsHcPostLinkStartup, &Private->UfsHcDriverInterface);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform driver during
> EdkiiUfsHcPostLinkStartup, Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> +


It looks to me that the 'PostLinkStartup' callback is triggered when the UFS
device detection fails (placed above "return EFI_NOT_FOUND;").

Per my understanding, the callback should be invoked when such detection
process succeeds. Could you help to confirm on this?

Best Regards,
Hao Wu


>    return EFI_NOT_FOUND;
>  }
> 
> @@ -2350,6 +2382,34 @@ ProcessAsyncTaskList (
>    }
>  }
> 
> +/**
> +  Execute UIC command.
> +
> +  @param[in]      This        Pointer to driver interface produced by the UFS
> controller.
> +  @param[in, out] UicCommand  Descriptor of the command that will be
> executed.
> +
> +  @retval EFI_SUCCESS            Command executed successfully.
> +  @retval EFI_INVALID_PARAMETER  This or UicCommand is NULL.
> +  @retval Others                 Command failed to execute.
> +**/
> +EFI_STATUS
> +EFIAPI
> +UfsHcDriverInterfaceExecUicCommand (
> +  IN     EDKII_UFS_HC_DRIVER_INTERFACE  *This,
> +  IN OUT EDKII_UIC_COMMAND              *UicCommand
> +  )
> +{
> +  UFS_PASS_THRU_PRIVATE_DATA    *Private;
> +
> +  if (This == NULL || UicCommand == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Private = UFS_PASS_THRU_PRIVATE_DATA_FROM_DRIVER_INTF (This);
> +
> +  return UfsExecUicCommands (Private, UicCommand);
> +}
> +
>  /**
>    Initializes UfsHcInfo field in private data.
> 
> @@ -2380,6 +2440,14 @@ GetUfsHcInfo (
> 
>    Private->UfsHcInfo.Capabilities = Data;
> 
> +  if (mUfsHcPlatform != NULL && mUfsHcPlatform->OverrideHcInfo != NULL)
> {
> +    Status = mUfsHcPlatform->OverrideHcInfo (Private->Handle, &Private-
> >UfsHcInfo);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failure from platform on OverrideHcInfo,
> Status = %r\n", Status));
> +      return Status;
> +    }
> +  }
> +
>    return EFI_SUCCESS;
>  }
> 
> --
> 2.14.1.windows.1