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
