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...@intel.com> > Cc: Marcin Wojtas <m...@semihalf.com> > Cc: Zhichao Gao <zhichao....@intel.com> > Cc: Liming Gao <liming....@intel.com> > > Signed-off-by: Mateusz Albecki <mateusz.albe...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53128): https://edk2.groups.io/g/devel/message/53128 Mute This Topic: https://groups.io/mt/69499845/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-