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

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

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Monday, January 13, 2020 9:49 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: RE: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> command error detection
> 
> Hi,
> 
> I will fix the 2 issues in separate patch probably before doing the refactor to
> avoid reverting it if the refactor introduces some unexpected issues.
> 
> Please also see inline.
> 
> Thanks,
> Mateusz
> 
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Friday, January 10, 2020 6:38 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>;
> devel@edk2.groups.io
> > Cc: Marcin Wojtas <mw@semihalf.com>; Gao, Zhichao
> > <zhichao.gao@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor
> > command error detection
> >
> > 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 ) {
> >
> >
> 
> OK
> 
> > > +    //
> > > +    // 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.
> >
> >
> 
> You are right, both error and normal interrupt status are going to be reset
> during SdMmcExecTrb. I didn't notice it and that is why I thought it is a bug.
> Reading SD host controller spec some more it seems to me like we should
> assume that if Transfer complete is set then no other error except for
> transfer timeout can be set and, as you proposed, return directly with
> EFI_SUCCESS. It would match previous implementation behavior.
> 
> > > +    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
> >
> >
> 
> We still need to check in normal interrupt register the status of the transfer
> complete, DMA interrupt and buffer ready so I think the call has to stay
> there. That said I can pass the IntStatus to SdMmcCheckAndRecoverErrors or
> move the call to SdMmcHcRwMmio after the call to
> SdMmcCheckAndRecoverErrors. I don't really have a preference so let me
> know if you want me to make any changes.


Hello Mateusz,

I think you can pass the 'IntStatus' parameter into function
SdMmcCheckAndRecoverErrors(). Doing so can avoid reading the
'SD_MMC_HC_NOR_INT_STS' register multiple times.

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
> >
>