From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web12.6297.1578634668294041632 for ; Thu, 09 Jan 2020 21:37:48 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2020 21:37:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,415,1571727600"; d="scan'208";a="272302569" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 09 Jan 2020 21:37:47 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 9 Jan 2020 21:37:47 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 9 Jan 2020 21:37:46 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.39]) with mapi id 14.03.0439.000; Fri, 10 Jan 2020 13:37:44 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: Marcin Wojtas , "Gao, Zhichao" , "Gao, Liming" Subject: Re: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection Thread-Topic: [PATCH 1/3] MdeModulePkg/SdMmcPciHcDxe: Refactor command error detection Thread-Index: AQHVxUqSP080Zt8q/E+VTdrkIuyHlqfgbuuA Date: Fri, 10 Jan 2020 05:37:44 +0000 Message-ID: References: <20200107110621.232-1-mateusz.albecki@intel.com> <20200107110621.232-2-mateusz.albecki@intel.com> In-Reply-To: <20200107110621.232-2-mateusz.albecki@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1140 >=20 > 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 refactori= ng or fixing the 2 bugs first. >=20 > Cc: Hao A Wu > Cc: Marcin Wojtas > Cc: Zhichao Gao > Cc: Liming Gao >=20 > Signed-off-by: Mateusz Albecki > --- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 234 > +++++++++++++++-------- > 1 file changed, 158 insertions(+), 76 deletions(-) >=20 > 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. >=20 > Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved. > - Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
> + Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ > @@ -2137,6 +2137,154 @@ SdMmcExecTrb ( > return Status; > } >=20 > +/** > + 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 =3D 0; > + if ((ErrIntStatus & 0x0F) !=3D 0) { > + SwReset |=3D BIT1; > + } > + if ((ErrIntStatus & 0x70) !=3D 0) { Thanks for this catch. Could you help to separate this fix to another patch? > + SwReset |=3D BIT2; > + } > + > + Status =3D SdMmcHcRwMmio ( > + Private->PciIo, > + Slot, > + SD_MMC_HC_SW_RST, > + FALSE, > + sizeof (SwReset), > + &SwReset > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status =3D 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 =3D SdMmcHcRwMmio ( > + Private->PciIo, > + Slot, > + SD_MMC_HC_NOR_INT_STS, > + TRUE, > + sizeof (IntStatus), > + &IntStatus > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if ((IntStatus & BIT15) =3D=3D 0) { > + return EFI_SUCCESS; > + } > + > + Status =3D 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 =3D EFI_CRC_ERROR; > + } else if ((ErrIntStatus & BIT4) && (IntStatus & BIT1)){ A coding style comment here, please help to explicitly compare the '&' oper= ation result with 0 for the above checks, like: if ((ErrIntStatus & (BIT1 | BIT2 | BIT5 | BIT6)) !=3D 0 ) { > + // > + // If the data timeout error is reported > + // but data transfer is signaled as completed we > + // have to ignore data timeout. > + // > + ErrorStatus =3D EFI_SUCCESS; I am thinking it might be more clean to directly return for this case. Sinc= e 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' registe= r to function SdMmcExecTrb() before the execution of next command. > + ErrIntStatusOr =3D BIT4; > + Status =3D SdMmcHcOrMmio ( > + Private->PciIo, > + Slot, > + SD_MMC_HC_ERR_INT_STS, > + sizeof (ErrIntStatus), > + &ErrIntStatusOr > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + } else { > + ErrorStatus =3D EFI_DEVICE_ERROR; > + } > + > + Status =3D SdMmcSoftwareReset (Private, Slot, ErrIntStatus); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return ErrorStatus; > +} > + > /** > Check the TRB execution result. >=20 > @@ -2160,10 +2308,8 @@ SdMmcCheckTrbResult ( > UINT32 Response[4]; > UINT64 SdmaAddr; > UINT8 Index; > - UINT8 SwReset; > UINT32 PioLength; >=20 > - SwReset =3D 0; > Packet =3D Trb->Packet; > // > // Check Trb execution result by reading Normal Interrupt Status regis= ter. > @@ -2179,87 +2325,23 @@ SdMmcCheckTrbResult ( > if (EFI_ERROR (Status)) { > goto Done; > } I think the below call in SdMmcCheckTrbResult(): Status =3D 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) =3D=3D BIT1) { > - if ((IntStatus & BIT15) =3D=3D 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 =3D SdMmcHcRwMmio ( > - Private->PciIo, > - Trb->Slot, > - SD_MMC_HC_ERR_INT_STS, > - TRUE, > - sizeof (IntStatus), > - &IntStatus > - ); > - if (!EFI_ERROR (Status)) { > - if ((IntStatus & BIT4) =3D=3D BIT4) { > - Status =3D EFI_SUCCESS; > - } else { > - Status =3D EFI_DEVICE_ERROR; > - } > - } > - } > - > + Status =3D 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 Controll= er > - // Simplified Spec 3.0 section 3.10.1. > + // Check Transfer Complete bit is set or not. > // > - if ((IntStatus & BIT15) =3D=3D BIT15) { > - Status =3D SdMmcHcRwMmio ( > - Private->PciIo, > - Trb->Slot, > - SD_MMC_HC_ERR_INT_STS, > - TRUE, > - sizeof (IntStatus), > - &IntStatus > - ); > - if (EFI_ERROR (Status)) { > - goto Done; > - } > - if ((IntStatus & 0x0F) !=3D 0) { > - SwReset |=3D BIT1; > - } > - if ((IntStatus & 0xF0) !=3D 0) { > - SwReset |=3D BIT2; > - } > - > - Status =3D SdMmcHcRwMmio ( > - Private->PciIo, > - Trb->Slot, > - SD_MMC_HC_SW_RST, > - FALSE, > - sizeof (SwReset), > - &SwReset > - ); > - if (EFI_ERROR (Status)) { > - goto Done; > - } > - Status =3D 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 =3D EFI_DEVICE_ERROR; > + if ((IntStatus & BIT1) =3D=3D BIT1) { > goto Done; > } > + > // > // Check if DMA interrupt is signalled for the SDMA transfer. > // > -- > 2.14.1.windows.1