[edk2-devel] [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection
Wu, Hao A
hao.a.wu at intel.com
Fri Jan 10 05:37:44 UTC 2020
Hello Mateusz,
Some inline comments below:
> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Tuesday, January 07, 2020 7:06 PM
> To: devel at 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 at intel.com>
> Cc: Marcin Wojtas <mw at semihalf.com>
> Cc: Zhichao Gao <zhichao.gao at intel.com>
> Cc: Liming Gao <liming.gao at intel.com>
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list