Re: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection

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

Hello Mateusz,

Some inline comments below:


> -----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 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command
> error detection
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1140
> 
> Error detection function will now check if the command
> failure has been caused by one of the errors that can
> appear randomly on link(CRC error + end bit error). If
> such an error has been a cause of failure function will
> return EFI_CRC_ERROR instead of EFI_DEVICE_ERROR to indicate
> to the higher level that command has a chance of succeeding if
> resent. In addition this patch also fixes 2 small bugs. First one
> is DAT lane being reset on current limit error. Second one is
> data timeout error not being cleared after transfer has been completed.


For the 2 small issues, I would suggest to split them into separate patches,
which would make this patch into 3 patches. You can either do the refactoring
or fixing the 2 bugs first.


> 
> 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/SdMmcPciHci.c | 234
> +++++++++++++++--------
>  1 file changed, 158 insertions(+), 76 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index e7f2fac69b..8b5e54f321 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.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
> 
>  **/
> @@ -2137,6 +2137,154 @@ SdMmcExecTrb (
>    return Status;
>  }
> 
> +/**
> +  Performs SW reset based on passed error status mask.
> +
> +  @param[in]  Private       Pointer to driver private data.
> +  @param[in]  Slot          Index of the slot to reset.
> +  @param[in]  ErrIntStatus  Error interrupt status mask.
> +
> +  @retval EFI_SUCCESS  Software reset performed successfully.
> +  @retval Other        Software reset failed.
> +**/
> +EFI_STATUS
> +SdMmcSoftwareReset (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN UINT8                   Slot,
> +  IN UINT16                  ErrIntStatus
> +  )
> +{
> +  UINT8       SwReset;
> +  EFI_STATUS  Status;
> +
> +  SwReset = 0;
> +  if ((ErrIntStatus & 0x0F) != 0) {
> +    SwReset |= BIT1;
> +  }
> +  if ((ErrIntStatus & 0x70) != 0) {


Thanks for this catch.
Could you help to separate this fix to another patch?


> +    SwReset |= BIT2;
> +  }
> +
> +  Status  = SdMmcHcRwMmio (
> +              Private->PciIo,
> +              Slot,
> +              SD_MMC_HC_SW_RST,
> +              FALSE,
> +              sizeof (SwReset),
> +              &SwReset
> +              );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = SdMmcHcWaitMmioSet (
> +             Private->PciIo,
> +             Slot,
> +             SD_MMC_HC_SW_RST,
> +             sizeof (SwReset),
> +             0xFF,
> +             0,
> +             SD_MMC_HC_GENERIC_TIMEOUT
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Checks the error status in error status register
> +  and issues appropriate software reset as described in
> +  SD specification section 3.10.
> +
> +  @param[in] Private  Pointer to driver private data.
> +  @param[in] Trb      Pointer to currently executing TRB.
> +
> +  @retval EFI_CRC_ERROR  CRC error happened during CMD execution.
> +  @retval EFI_SUCCESS    No error reported.
> +  @retval Others         Some other error happened.
> +
> +**/
> +EFI_STATUS
> +SdMmcCheckAndRecoverErrors (
> +  IN SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN UINT8                   Slot
> +  )
> +{
> +  UINT16      IntStatus;
> +  UINT16      ErrIntStatus;
> +  UINT16      ErrIntStatusOr;
> +  EFI_STATUS  Status;
> +  EFI_STATUS  ErrorStatus;
> +
> +  Status = SdMmcHcRwMmio (
> +             Private->PciIo,
> +             Slot,
> +             SD_MMC_HC_NOR_INT_STS,
> +             TRUE,
> +             sizeof (IntStatus),
> +             &IntStatus
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if ((IntStatus & BIT15) == 0) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  Status = SdMmcHcRwMmio (
> +             Private->PciIo,
> +             Slot,
> +             SD_MMC_HC_ERR_INT_STS,
> +             TRUE,
> +             sizeof (ErrIntStatus),
> +             &ErrIntStatus
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // We treat both CMD and DAT CRC errors and
> +  // end bits errors as EFI_CRC_ERROR. This will
> +  // let higher layer know that the error possibly
> +  // happened due to random bus condition and the
> +  // command can be retried.
> +  //
> +  if (ErrIntStatus & (BIT1 | BIT2 | BIT5 | BIT6)) {
> +    ErrorStatus = EFI_CRC_ERROR;
> +  } else if ((ErrIntStatus & BIT4) && (IntStatus & BIT1)){


A coding style comment here, please help to explicitly compare the '&' operation
result with 0 for the above checks, like:

  if ((ErrIntStatus & (BIT1 | BIT2 | BIT5 | BIT6)) != 0 ) {


> +    //
> +    // If the data timeout error is reported
> +    // but data transfer is signaled as completed we
> +    // have to ignore data timeout.
> +    //
> +    ErrorStatus = EFI_SUCCESS;


I am thinking it might be more clean to directly return for this case. Since
for this case, a recovery procedure is not required, doing so can avoid
explicitly clearing the Timeout error bit in the 'SD_MMC_HC_ERR_INT_STS'
register.

We can still leave the clear of the bits in 'SD_MMC_HC_ERR_INT_STS' register to
function SdMmcExecTrb() before the execution of next command.


> +    ErrIntStatusOr = BIT4;
> +    Status = SdMmcHcOrMmio (
> +               Private->PciIo,
> +               Slot,
> +               SD_MMC_HC_ERR_INT_STS,
> +               sizeof (ErrIntStatus),
> +               &ErrIntStatusOr
> +               );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  } else {
> +    ErrorStatus = EFI_DEVICE_ERROR;
> +  }
> +
> +  Status = SdMmcSoftwareReset (Private, Slot, ErrIntStatus);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return ErrorStatus;
> +}
> +
>  /**
>    Check the TRB execution result.
> 
> @@ -2160,10 +2308,8 @@ SdMmcCheckTrbResult (
>    UINT32                              Response[4];
>    UINT64                              SdmaAddr;
>    UINT8                               Index;
> -  UINT8                               SwReset;
>    UINT32                              PioLength;
> 
> -  SwReset = 0;
>    Packet  = Trb->Packet;
>    //
>    // Check Trb execution result by reading Normal Interrupt Status register.
> @@ -2179,87 +2325,23 @@ SdMmcCheckTrbResult (
>    if (EFI_ERROR (Status)) {
>      goto Done;
>    }


I think the below call in SdMmcCheckTrbResult():

  Status = SdMmcHcRwMmio (
             Private->PciIo,
             Trb->Slot,
             SD_MMC_HC_NOR_INT_STS,
             TRUE,
             sizeof (IntStatus),
             &IntStatus
             );

can be removed. I found that the same call will be made in
SdMmcCheckAndRecoverErrors().

Best Regards,
Hao Wu


> +
>    //
> -  // Check Transfer Complete bit is set or not.
> +  // Check if there are any errors reported by host controller
> +  // and if neccessary recover the controller before next command is
> executed.
>    //
> -  if ((IntStatus & BIT1) == BIT1) {
> -    if ((IntStatus & BIT15) == BIT15) {
> -      //
> -      // Read Error Interrupt Status register to check if the error is
> -      // Data Timeout Error.
> -      // If yes, treat it as success as Transfer Complete has higher
> -      // priority than Data Timeout Error.
> -      //
> -      Status = SdMmcHcRwMmio (
> -                 Private->PciIo,
> -                 Trb->Slot,
> -                 SD_MMC_HC_ERR_INT_STS,
> -                 TRUE,
> -                 sizeof (IntStatus),
> -                 &IntStatus
> -                 );
> -      if (!EFI_ERROR (Status)) {
> -        if ((IntStatus & BIT4) == BIT4) {
> -          Status = EFI_SUCCESS;
> -        } else {
> -          Status = EFI_DEVICE_ERROR;
> -        }
> -      }
> -    }
> -
> +  Status = SdMmcCheckAndRecoverErrors (Private, Trb->Slot);
> +  if (EFI_ERROR (Status)) {
>      goto Done;
>    }
> +
>    //
> -  // Check if there is a error happened during cmd execution.
> -  // If yes, then do error recovery procedure to follow SD Host Controller
> -  // Simplified Spec 3.0 section 3.10.1.
> +  // Check Transfer Complete bit is set or not.
>    //
> -  if ((IntStatus & BIT15) == BIT15) {
> -    Status = SdMmcHcRwMmio (
> -               Private->PciIo,
> -               Trb->Slot,
> -               SD_MMC_HC_ERR_INT_STS,
> -               TRUE,
> -               sizeof (IntStatus),
> -               &IntStatus
> -               );
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    if ((IntStatus & 0x0F) != 0) {
> -      SwReset |= BIT1;
> -    }
> -    if ((IntStatus & 0xF0) != 0) {
> -      SwReset |= BIT2;
> -    }
> -
> -    Status  = SdMmcHcRwMmio (
> -                Private->PciIo,
> -                Trb->Slot,
> -                SD_MMC_HC_SW_RST,
> -                FALSE,
> -                sizeof (SwReset),
> -                &SwReset
> -                );
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -    Status = SdMmcHcWaitMmioSet (
> -               Private->PciIo,
> -               Trb->Slot,
> -               SD_MMC_HC_SW_RST,
> -               sizeof (SwReset),
> -               0xFF,
> -               0,
> -               SD_MMC_HC_GENERIC_TIMEOUT
> -               );
> -    if (EFI_ERROR (Status)) {
> -      goto Done;
> -    }
> -
> -    Status = EFI_DEVICE_ERROR;
> +  if ((IntStatus & BIT1) == BIT1) {
>      goto Done;
>    }
> +
>    //
>    // Check if DMA interrupt is signalled for the SDMA transfer.
>    //
> --
> 2.14.1.windows.1