Re: [PATCH 2/3] MdeModulePkg/SdMmcPciHcDxe: Add retries for sync commands

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

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Tuesday, January 07, 2020 7:06 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: [PATCH 2/3] MdeModulePkg/SdMmcPciHcDxe: Add retries for sync
> commands
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1140
> 
> To increase the resiliency driver will now attempt to
> retry the commands that failed due to the CRC error up
> to 5 times. This should address the problems with the commands
> that fail due to random condition on links. This should also
> help the boards on which CMD13 is particularly unstable after
> switching the link frequency.


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Marcin Wojtas <mw@semihalf.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> 
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 83
> ++++++++++++++--------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  5 +-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  1 +
>  3 files changed, 59 insertions(+), 30 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 373f1bed45..193b0f24e2 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -7,7 +7,7 @@
>    It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
> 
>    Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
> -  Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -974,6 +974,58 @@ SdMmcPciHcDriverBindingStop (
>    return Status;
>  }
> 
> +/**
> +  Execute TRB synchronously.
> +
> +  @param[in] Private  Pointer to driver private data.
> +  @param[in] Trb      Pointer to TRB to execute.
> +
> +  @retval EFI_SUCCESS  TRB executed successfully.
> +  @retval Other        TRB failed.
> +**/
> +EFI_STATUS
> +SdMmcPassThruExecSyncTrb (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN SD_MMC_HC_TRB           *Trb
> +  )
> +{
> +  EFI_STATUS  Status;
> +  EFI_TPL     OldTpl;
> +
> +  //
> +  // Wait async I/O list is empty before execute sync I/O operation.
> +  //
> +  while (TRUE) {
> +    OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> +    if (IsListEmpty (&Private->Queue)) {
> +      gBS->RestoreTPL (OldTpl);
> +      break;
> +    }
> +    gBS->RestoreTPL (OldTpl);
> +  }
> +
> +  while (Trb->Retries) {
> +    Status = SdMmcWaitTrbEnv (Private, Trb);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    Status = SdMmcExecTrb (Private, Trb);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    Status = SdMmcWaitTrbResult (Private, Trb);
> +    if (Status == EFI_CRC_ERROR) {
> +      Trb->Retries--;
> +    } else {
> +      return Status;
> +    }
> +  }
> +
> +  return Status;
> +}
> +
>  /**
>    Sends SD command to an SD card that is attached to the SD controller.
> 
> @@ -1023,7 +1075,6 @@ SdMmcPassThruPassThru (
>    EFI_STATUS                      Status;
>    SD_MMC_HC_PRIVATE_DATA          *Private;
>    SD_MMC_HC_TRB                   *Trb;
> -  EFI_TPL                         OldTpl;
> 
>    if ((This == NULL) || (Packet == NULL)) {
>      return EFI_INVALID_PARAMETER;
> @@ -1066,34 +1117,8 @@ SdMmcPassThruPassThru (
>      return EFI_SUCCESS;
>    }
> 
> -  //
> -  // Wait async I/O list is empty before execute sync I/O operation.
> -  //
> -  while (TRUE) {
> -    OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
> -    if (IsListEmpty (&Private->Queue)) {
> -      gBS->RestoreTPL (OldTpl);
> -      break;
> -    }
> -    gBS->RestoreTPL (OldTpl);
> -  }
> -
> -  Status = SdMmcWaitTrbEnv (Private, Trb);
> -  if (EFI_ERROR (Status)) {
> -    goto Done;
> -  }
> -
> -  Status = SdMmcExecTrb (Private, Trb);
> -  if (EFI_ERROR (Status)) {
> -    goto Done;
> -  }
> +  Status = SdMmcPassThruExecSyncTrb (Private, Trb);
> 
> -  Status = SdMmcWaitTrbResult (Private, Trb);
> -  if (EFI_ERROR (Status)) {
> -    goto Done;
> -  }
> -
> -Done:
>    SdMmcFreeTrb (Trb);
> 
>    return Status;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index 0304960132..5bc3577ba2 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -3,7 +3,7 @@
>    Provides some data structure definitions used by the SD/MMC host
> controller driver.
> 
>  Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
> -Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -130,6 +130,8 @@ typedef struct {
> 
>  #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
> 
> +#define SD_MMC_TRB_RETRIES            5
> +
>  //
>  // TRB (Transfer Request Block) contains information for the cmd request.
>  //
> @@ -152,6 +154,7 @@ typedef struct {
>    EFI_EVENT                           Event;
>    BOOLEAN                             Started;
>    UINT64                              Timeout;
> +  UINT32                              Retries;
> 
>    SD_MMC_HC_ADMA_32_DESC_LINE         *Adma32Desc;
>    SD_MMC_HC_ADMA_64_V3_DESC_LINE      *Adma64V3Desc;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 8b5e54f321..676ace847b 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -1683,6 +1683,7 @@ SdMmcCreateTrb (
>    Trb->Event     = Event;
>    Trb->Started   = FALSE;
>    Trb->Timeout   = Packet->Timeout;
> +  Trb->Retries   = SD_MMC_TRB_RETRIES;
>    Trb->Private   = Private;
> 
>    if ((Packet->InTransferLength != 0) && (Packet->InDataBuffer != NULL)) {
> --
> 2.14.1.windows.1