Jan, I will help to add my RB and push the patch to git repo.
Thanks Feng -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Jan Dabros Sent: Tuesday, June 21, 2016 10:39 PM To: Tian, Feng <[email protected]> Cc: [email protected]; [email protected]; [email protected]; [email protected]; Gao, Liming <[email protected]>; Kinney, Michael D <[email protected]> Subject: Re: [edk2] [PATCH] MdedulePkg: AtaAtapiPassThru: Remove polling on PxCMD.FR flag setting Hi Feng, Thanks for understanding. Should I resend this patch with your "Reviewed-by" or rather wait for merge to edk2 tree? Best Regards, Jan 2016-06-21 16:30 GMT+02:00 Tian, Feng <[email protected]>: > Hi, Jan > > Why I added this check is purely because of this sentence "When PxCMD.FRE is > set (causing PxCMD.FR to be set to ‘1’)" in AHCI spec. And except this, no > other places in AHCI speak about the behavior of PxCMD.FR being 1. > > Literally, your controller doesn't follow this single sentence. But just like > you said, setting PxCMD.FRE is enough and linux doesn't check this bit as > well. So I am ok with removing this check. > > Reviewed-by: Feng Tian <[email protected]> > > Thanks > Feng > > -----Original Message----- > From: Jan Dąbroś [mailto:[email protected]] > Sent: Tuesday, June 21, 2016 6:12 PM > To: Tian, Feng <[email protected]> > Cc: Marcin Wojtas <[email protected]>; [email protected]; > [email protected]; Gao, Liming <[email protected]>; > [email protected]; [email protected]; Kinney, Michael D > <[email protected]> > Subject: Re: [edk2] [PATCH] MdedulePkg: AtaAtapiPassThru: Remove > polling on PxCMD.FR flag setting > > Hi Feng, > > 1. In the same chapter of AHCI spec, there is sentence "When this bit is > cleared to ‘0’, software *must* first wait for PxCMD.FR to clear to ‘0’, > indicating that the DMA engine for FIS reception is in an idle condition.". I > wanted to emphasize in commit log, that there is no need to poll for PxCMD.FR > when PxCMD.FRE is set (unlike the situation when we unset this bit). > 2. Our controller, which is fully AHCI compliant, receives FISes from the > device and copies them into system memory after setting PxCMD.FRE, although > PxCMD.FR isn't set to '1'. We remove those lines, because our controller > always timeouts on this condition. I think that in your quotation from AHCI > spec, there is no statement which makes this polling obligatory. > 3. What's more, corresponding AHCI drivers in Uboot and Linux aren't polling > for this bit after setting PxCMD.FRE as well. > > Regards, > Jan > > 2016-06-21 3:36 GMT+02:00 Tian, Feng <[email protected]>: >> Sorry, Macrin. Could you explain more about this fix? >> >> According to AHCI spec, " When PxCMD.FRE is set (causing PxCMD.FR to be set >> to ‘1’), the HBA receives FISes from the device and copies them into system >> memory. " >> >> The lines you removed in the patch is waiting for PxCMD.FR to be set to 1 >> rather than to be clear to 0. So is the patch commit log correct? >> >> Last, what's the side effect even though we check the FR bit value with 1? >> >> Thanks >> Feng >> >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf >> Of Marcin Wojtas >> Sent: Tuesday, June 21, 2016 8:37 AM >> To: [email protected] >> Cc: [email protected]; [email protected]; [email protected]; >> Gao, Liming <[email protected]>; Kinney, Michael D >> <[email protected]> >> Subject: [edk2] [PATCH] MdedulePkg: AtaAtapiPassThru: Remove polling >> on PxCMD.FR flag setting >> >> From: Jan Dąbroś <[email protected]> >> >> It is enough to set PxCMD.FRE bit, which cause HBA to post received FISes >> into the FIS receive area. According to AHCI Specification, only polling on >> PxCMD.FRE to be cleared is necessary, when it is needeed to stop FIS engine >> (eg. in order to change PxCMD.FB address). >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jan Dabros <[email protected]> >> Signed-off-by: Marcin Wojtas <[email protected]> >> --- >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 18 >> +----------------- >> 1 file changed, 1 insertion(+), 17 deletions(-) >> >> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> index 3534d9f..469a40a 100644 >> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> @@ -427,13 +427,7 @@ AhciEnableFisReceive ( >> Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + >> EFI_AHCI_PORT_CMD; >> AhciOrReg (PciIo, Offset, EFI_AHCI_PORT_CMD_FRE); >> >> - return AhciWaitMmioSet ( >> - PciIo, >> - Offset, >> - EFI_AHCI_PORT_CMD_FR, >> - EFI_AHCI_PORT_CMD_FR, >> - Timeout >> - ); >> + return EFI_SUCCESS; >> } >> >> /** >> @@ -2344,16 +2338,6 @@ AhciModeInitialization ( >> // >> Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH + >> EFI_AHCI_PORT_CMD; >> AhciOrReg (PciIo, Offset, EFI_AHCI_PORT_CMD_FRE); >> - Status = AhciWaitMmioSet ( >> - PciIo, >> - Offset, >> - EFI_AHCI_PORT_CMD_FR, >> - EFI_AHCI_PORT_CMD_FR, >> - EFI_AHCI_PORT_CMD_FR_CLEAR_TIMEOUT >> - ); >> - if (EFI_ERROR (Status)) { >> - continue; >> - } >> >> // >> // Wait no longer than 10 ms to wait the Phy to detect the presence >> of a device. >> -- >> 1.8.3.1 >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
