From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web11.6146.1578975369101582545 for ; Mon, 13 Jan 2020 20:16:09 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jan 2020 20:16:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,431,1571727600"; d="scan'208";a="256137176" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 13 Jan 2020 20:16:07 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jan 2020 20:16:07 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jan 2020 20:16:07 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.55]) with mapi id 14.03.0439.000; Tue, 14 Jan 2020 12:16:04 +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+VTdrkIuyHlqfgbuuAgAewvoCAAXPuIA== Date: Tue, 14 Jan 2020 04:16:04 +0000 Message-ID: References: <20200107110621.232-1-mateusz.albecki@intel.com> <20200107110621.232-2-mateusz.albecki@intel.com> In-Reply-To: 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 > -----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 >=20 > Hi, >=20 > I will fix the 2 issues in separate patch probably before doing the refac= tor to > avoid reverting it if the refactor introduces some unexpected issues. >=20 > Please also see inline. >=20 > Thanks, > Mateusz >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Friday, January 10, 2020 6:38 AM > > 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 > > > > 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=3D1140 > > > > > > 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 pat= ches, > > which would make this patch into 3 patches. You can either do the > > refactoring or fixing the 2 bugs first. > > > > > > > > > > Cc: Hao A Wu > > > Cc: Marcin Wojtas > > > Cc: Zhichao Gao > > > Cc: Liming Gao > > > > > > Signed-off-by: Mateusz Albecki > > > --- > > > 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.
> > > + Copyright (c) 2015 - 2020, Intel Corporation. All rights > > > + reserved.
> > > 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 =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 '&' > > operation > > result with 0 for the above checks, like: > > > > if ((ErrIntStatus & (BIT1 | BIT2 | BIT5 | BIT6)) !=3D 0 ) { > > > > >=20 > OK >=20 > > > + // > > > + // 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. = 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. > > > > >=20 > You are right, both error and normal interrupt status are going to be res= et > 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. >=20 > > > + 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. > > > > > > @@ -2160,10 +2308,8 @@ SdMmcCheckTrbResult ( > > > UINT32 Response[4]; > > > UINT64 SdmaAddr; > > > UINT8 Index; > > > - UINT8 SwReset; > > > UINT32 PioLength; > > > > > > - SwReset =3D 0; > > > Packet =3D 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 =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 > > > > >=20 > We still need to check in normal interrupt register the status of the tra= nsfer > complete, DMA interrupt and buffer ready so I think the call has to stay > there. That said I can pass the IntStatus to SdMmcCheckAndRecoverErrors o= r > 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 >=20 > > > + > > > // > > > - // 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 Cont= roller > > > - // 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 > > >=20