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

["Albecki, Mateusz" <mateusz.albecki@intel.com>]

Hi,

Yes that is bug. It should be placed before return EFI_SUCCESS. I will fix it in v2.

Thanks,
Mateusz

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, August 8, 2019 4:37 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Subject: RE: [PATCH 4/4] MdeModulePkg/UfsPassThruDxe: Implement
> EDKII_UFS_HC_PLATFORM_PROTOCOL
> 
> 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

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.