> -----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...@intel.com>
> > Sent: Friday, January 10, 2020 6:38 AM
> > To: Albecki, Mateusz <mateusz.albe...@intel.com>;
> devel@edk2.groups.io
> > Cc: Marcin Wojtas <m...@semihalf.com>; Gao, Zhichao
> > <zhichao....@intel.com>; Gao, Liming <liming....@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...@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 ) {
> >
> >
>
> 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
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53205): https://edk2.groups.io/g/devel/message/53205
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]
-=-=-=-=-=-=-=-=-=-=-=-
