[edk2-devel] [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion
Albecki, Mateusz
mateusz.albecki at intel.com
Wed Nov 4 11:54:47 UTC 2020
I will fix those 2 issues in v2.
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu at intel.com>
> Sent: Wednesday, November 4, 2020 6:33 AM
> To: Albecki, Mateusz <mateusz.albecki at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at intel.com>
> Subject: RE: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to
> check for command completion
>
> > -----Original Message-----
> > From: Albecki, Mateusz <mateusz.albecki at intel.com>
> > Sent: Tuesday, November 3, 2020 9:24 PM
> > To: devel at edk2.groups.io
> > Cc: Albecki, Mateusz <mateusz.albecki at intel.com>; Ni, Ray
> > <ray.ni at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
> > Subject: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check
> > for command completion
> >
> > From: Albecki <mateusz.albecki at intel.com>
>
>
> Hello Albecki,
>
> Could you help to check the author field for the patch series?
> (All 4 patches seem to have the same issue)
>
> I found that the author field shows:
> Albecki <mateusz.albecki at intel.com>
>
> Which does not match exactly with your Signed-off tag.
>
> You can use the below command to change the information for one
> repository:
> git config user.name "FIRST_NAME LAST_NAME"
> Or below command to change it globally among all repositories:
> git config --global user.name "FIRST_NAME LAST_NAME"
> And then, redo the commit process again.
>
> Also, could you help to update the copyright year information for
> AhciMode.c and AhciMode.h?
>
> With above 2 things handled:
> Reviewed-by: Hao A Wu <hao.a.wu at intel.com>
>
> Best Regards,
> Hao Wu
>
>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3024
> >
> > AHCI driver used to poll D2H register type to determine whether the
> > FIS has been received. This caused a problem of long timeouts when the
> > link got a CRC error and the FIS never arrives. To fix this this
> > change switches AHCI driver to poll the IS register which will signal
> > both the reception of FIS and the occurance of error.
> >
> > Signed-off-by: Mateusz Albecki <mateusz.albecki at intel.com>
> >
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> >
> > ---
> > .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 292 ++++++++----------
> > .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 9 +-
> > 2 files changed, 131 insertions(+), 170 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > index 7e2fade400..4b42e72226 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > @@ -248,32 +248,23 @@ AhciWaitMemSet (
> > /**
> > Check the memory status to the test value.
> >
> > - @param[in] Address The memory address to test.
> > - @param[in] MaskValue The mask value of memory.
> > - @param[in] TestValue The test value of memory.
> > - @param[in, out] Task Optional. Pointer to the
> > ATA_NONBLOCK_TASK used by
> > - non-blocking mode. If NULL, then just try once.
> > -
> > - @retval EFI_NOTREADY The memory is not set.
> > - @retval EFI_TIMEOUT The memory setting retry times out.
> > - @retval EFI_SUCCESS The memory is correct set.
> > + @param[in] Address The memory address to test.
> > + @param[in] MaskValue The mask value of memory.
> > + @param[in] TestValue The test value of memory.
> >
> > + @retval EFI_NOT_READY The memory is not set.
> > + @retval EFI_SUCCESS The memory is correct set.
> > **/
> > EFI_STATUS
> > EFIAPI
> > AhciCheckMemSet (
> > IN UINTN Address,
> > IN UINT32 MaskValue,
> > - IN UINT32 TestValue,
> > - IN OUT ATA_NONBLOCK_TASK *Task
> > + IN UINT32 TestValue
> > )
> > {
> > UINT32 Value;
> >
> > - if (Task != NULL) {
> > - Task->RetryTimes--;
> > - }
> > -
> > Value = *(volatile UINT32 *) Address;
> > Value &= MaskValue;
> >
> > @@ -281,11 +272,7 @@ AhciCheckMemSet (
> > return EFI_SUCCESS;
> > }
> >
> > - if ((Task != NULL) && !Task->InfiniteWait && (Task->RetryTimes == 0)) {
> > - return EFI_TIMEOUT;
> > - } else {
> > - return EFI_NOT_READY;
> > - }
> > + return EFI_NOT_READY;
> > }
> >
> >
> > @@ -357,7 +344,7 @@ AhciDumpPortStatus (
> > FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> >
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_REGISTER_D2H, NULL);
> > + Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > + EFI_AHCI_FIS_REGISTER_D2H);
> > if (!EFI_ERROR (Status)) {
> > //
> > // If D2H FIS is received, update StatusBlock with its content.
> > @@ -621,6 +608,102 @@ AhciBuildCommandFis (
> > CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock-
> > >AtaDeviceHead | 0xE0);
> > }
> >
> > +/**
> > + Checks if specified FIS has been received.
> > +
> > + @param[in] PciIo Pointer to AHCI controller PciIo.
> > + @param[in] Port SATA port index on which to check.
> > + @param[in] FisType FIS type for which to check.
> > +
> > + @retval EFI_SUCCESS FIS received.
> > + @retval EFI_NOT_READY FIS not received yet.
> > + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> > +**/
> > +EFI_STATUS
> > +AhciCheckFisReceived (
> > + IN EFI_PCI_IO_PROTOCOL *PciIo,
> > + IN UINT8 Port,
> > + IN SATA_FIS_TYPE FisType
> > + )
> > +{
> > + UINT32 Offset;
> > + UINT32 PortInterrupt;
> > + UINT32 PortTfd;
> > +
> > + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > + EFI_AHCI_PORT_IS; PortInterrupt = AhciReadReg (PciIo, Offset); if
> > + ((PortInterrupt & EFI_AHCI_PORT_IS_ERROR_MASK) != 0) {
> > + DEBUG ((DEBUG_ERROR, "AHCI: Error interrupt reported PxIS: %X\n",
> > PortInterrupt));
> > + return EFI_DEVICE_ERROR;
> > + }
> > + //
> > + // For PIO setup FIS - According to SATA 2.6 spec section 11.7, D2h
> > + FIS
> > means an error encountered.
> > + // But Qemu and Marvel 9230 sata controller may just receive a D2h
> > + FIS from device // after the transaction is finished successfully.
> > + // To get better device compatibilities, we further check if the
> > + PxTFD's ERR
> > bit is set.
> > + // By this way, we can know if there is a real error happened.
> > + //
> > + if (((FisType == SataFisD2H) && ((PortInterrupt &
> > EFI_AHCI_PORT_IS_DHRS) != 0)) ||
> > + ((FisType == SataFisPioSetup) && (PortInterrupt &
> > (EFI_AHCI_PORT_IS_PSS | EFI_AHCI_PORT_IS_DHRS)) != 0) ||
> > + ((FisType == SataFisDmaSetup) && (PortInterrupt &
> > (EFI_AHCI_PORT_IS_DSS | EFI_AHCI_PORT_IS_DHRS)) != 0)) {
> > + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH
> +
> > EFI_AHCI_PORT_TFD;
> > + PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > + if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > + return EFI_DEVICE_ERROR;
> > + } else {
> > + return EFI_SUCCESS;
> > + }
> > + }
> > +
> > + return EFI_NOT_READY;
> > +}
> > +
> > +/**
> > + Waits until specified FIS has been received.
> > +
> > + @param[in] PciIo Pointer to AHCI controller PciIo.
> > + @param[in] Port SATA port index on which to check.
> > + @param[in] Timeout Time after which function should stop polling.
> > + @param[in] FisType FIS type for which to check.
> > +
> > + @retval EFI_SUCCESS FIS received.
> > + @retval EFI_TIMEOUT FIS failed to arrive within a specified time
> period.
> > + @retval EFI_DEVICE_ERROR AHCI controller reported an error on port.
> > +**/
> > +EFI_STATUS
> > +AhciWaitUntilFisReceived (
> > + IN EFI_PCI_IO_PROTOCOL *PciIo,
> > + IN UINT8 Port,
> > + IN UINT64 Timeout,
> > + IN SATA_FIS_TYPE FisType
> > + )
> > +{
> > + EFI_STATUS Status;
> > + BOOLEAN InfiniteWait;
> > + UINT64 Delay;
> > +
> > + Delay = DivU64x32 (Timeout, 1000) + 1; if (Timeout == 0) {
> > + InfiniteWait = TRUE;
> > + } else {
> > + InfiniteWait = FALSE;
> > + }
> > +
> > + do {
> > + Status = AhciCheckFisReceived (PciIo, Port, FisType);
> > + if (Status != EFI_NOT_READY) {
> > + return Status;
> > + }
> > + //
> > + // Stall for 100 microseconds.
> > + //
> > + MicroSecondDelay (100);
> > + Delay--;
> > + } while (InfiniteWait || (Delay > 0));
> > +
> > + return EFI_TIMEOUT;
> > +}
> > +
> > /**
> > Start a PIO data transfer on specific port.
> >
> > @@ -665,26 +748,13 @@ AhciPioTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN FisBaseAddr;
> > - UINTN Offset;
> > EFI_PHYSICAL_ADDRESS PhyAddr;
> > VOID *Map;
> > UINTN MapLength;
> > EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> > - UINT64 Delay;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> > - UINT32 PortTfd;
> > UINT32 PrdCount;
> > - BOOLEAN InfiniteWait;
> > - BOOLEAN PioFisReceived;
> > - BOOLEAN D2hFisReceived;
> > -
> > - if (Timeout == 0) {
> > - InfiniteWait = TRUE;
> > - } else {
> > - InfiniteWait = FALSE;
> > - }
> >
> > if (Read) {
> > Flag = EfiPciIoOperationBusMasterWrite; @@ -743,87 +813,18 @@
> > AhciPioTransfer (
> > goto Exit;
> > }
> >
> > - //
> > - // Check the status and wait the driver sending data
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > -
> > if (Read && (AtapiCommand == 0)) {
> > - //
> > - // Wait device sends the PIO setup fis before data transfer
> > - //
> > - Status = EFI_TIMEOUT;
> > - Delay = DivU64x32 (Timeout, 1000) + 1;
> > - do {
> > - PioFisReceived = FALSE;
> > - D2hFisReceived = FALSE;
> > - Offset = FisBaseAddr + EFI_AHCI_PIO_FIS_OFFSET;
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_PIO_SETUP, NULL);
> > - if (!EFI_ERROR (Status)) {
> > - PioFisReceived = TRUE;
> > - }
> > - //
> > - // According to SATA 2.6 spec section 11.7, D2h FIS means an error
> > encountered.
> > - // But Qemu and Marvel 9230 sata controller may just receive a D2h FIS
> > from device
> > - // after the transaction is finished successfully.
> > - // To get better device compatibilities, we further check if the PxTFD's
> > ERR bit is set.
> > - // By this way, we can know if there is a real error happened.
> > - //
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciCheckMemSet (Offset, EFI_AHCI_FIS_TYPE_MASK,
> > EFI_AHCI_FIS_REGISTER_D2H, NULL);
> > - if (!EFI_ERROR (Status)) {
> > - D2hFisReceived = TRUE;
> > - }
> > -
> > - if (PioFisReceived || D2hFisReceived) {
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH
> +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - //
> > - // PxTFD will be updated if there is a D2H or SetupFIS received.
> > - //
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > - break;
> > - }
> > -
> > - PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> > >AhciCmdList[0].AhciCmdPrdbc));
> > - if (PrdCount == DataCount) {
> > - Status = EFI_SUCCESS;
> > - break;
> > - }
> > - }
> > -
> > - //
> > - // Stall for 100 microseconds.
> > - //
> > - MicroSecondDelay(100);
> > -
> > - Delay--;
> > - if (Delay == 0) {
> > - Status = EFI_TIMEOUT;
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> SataFisPioSetup);
> > + if (Status == EFI_SUCCESS) {
> > + PrdCount = *(volatile UINT32 *) (&(AhciRegisters-
> > >AhciCmdList[0].AhciCmdPrdbc));
> > + if (PrdCount == DataCount) {
> > + Status = EFI_SUCCESS;
> > + } else {
> > + Status = EFI_DEVICE_ERROR;
> > }
> > - } while (InfiniteWait || (Delay > 0));
> > - } else {
> > - //
> > - // Wait for D2H Fis is received
> > - //
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > }
> > + } else {
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> > }
> >
> > Exit:
> > @@ -893,15 +894,12 @@ AhciDmaTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN Offset;
> > EFI_PHYSICAL_ADDRESS PhyAddr;
> > VOID *Map;
> > UINTN MapLength;
> > EFI_PCI_IO_PROTOCOL_OPERATION Flag;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> > - UINTN FisBaseAddr;
> > - UINT32 PortTfd;
> >
> > EFI_PCI_IO_PROTOCOL *PciIo;
> > EFI_TPL OldTpl;
> > @@ -996,38 +994,17 @@ AhciDmaTransfer (
> > }
> > }
> >
> > - //
> > - // Wait for command complete
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > if (Task != NULL) {
> > - //
> > - // For Non-blocking
> > - //
> > - Status = AhciCheckMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Task
> > - );
> > + Status = AhciCheckFisReceived (PciIo, Port, SataFisD2H);
> > + if (Status == EFI_NOT_READY) {
> > + if (!Task->InfiniteWait && Task->RetryTimes == 0) {
> > + Status = EFI_TIMEOUT;
> > + } else {
> > + Task->RetryTimes--;
> > + }
> > + }
> > } else {
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > - }
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> > }
> >
> > Exit:
> > @@ -1105,9 +1082,6 @@ AhciNonDataTransfer (
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN FisBaseAddr;
> > - UINTN Offset;
> > - UINT32 PortTfd;
> > EFI_AHCI_COMMAND_FIS CFis;
> > EFI_AHCI_COMMAND_LIST CmdList;
> >
> > @@ -1144,27 +1118,7 @@ AhciNonDataTransfer (
> > goto Exit;
> > }
> >
> > - //
> > - // Wait device sends the Response Fis
> > - //
> > - FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof
> > (EFI_AHCI_RECEIVED_FIS);
> > - Offset = FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET;
> > - Status = AhciWaitMemSet (
> > - Offset,
> > - EFI_AHCI_FIS_TYPE_MASK,
> > - EFI_AHCI_FIS_REGISTER_D2H,
> > - Timeout
> > - );
> > -
> > - if (EFI_ERROR (Status)) {
> > - goto Exit;
> > - }
> > -
> > - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> > EFI_AHCI_PORT_TFD;
> > - PortTfd = AhciReadReg (PciIo, (UINT32) Offset);
> > - if ((PortTfd & EFI_AHCI_PORT_TFD_ERR) != 0) {
> > - Status = EFI_DEVICE_ERROR;
> > - }
> > + Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout,
> > + SataFisD2H);
> >
> > Exit:
> > AhciStopCommand (
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > index 786413930a..a3cd351f6e 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> > @@ -96,7 +96,7 @@ typedef union {
> > #define EFI_AHCI_PORT_IS 0x0010
> > #define EFI_AHCI_PORT_IS_DHRS BIT0
> > #define EFI_AHCI_PORT_IS_PSS BIT1
> > -#define EFI_AHCI_PORT_IS_SSS BIT2
> > +#define EFI_AHCI_PORT_IS_DSS BIT2
> > #define EFI_AHCI_PORT_IS_SDBS BIT3
> > #define EFI_AHCI_PORT_IS_UFS BIT4
> > #define EFI_AHCI_PORT_IS_DPS BIT5
> > @@ -113,6 +113,7 @@ typedef union {
> > #define EFI_AHCI_PORT_IS_CPDS BIT31
> > #define EFI_AHCI_PORT_IS_CLEAR 0xFFFFFFFF
> > #define EFI_AHCI_PORT_IS_FIS_CLEAR 0x0000000F
> > +#define EFI_AHCI_PORT_IS_ERROR_MASK
> (EFI_AHCI_PORT_IS_INFS
> > | EFI_AHCI_PORT_IS_IFS | EFI_AHCI_PORT_IS_HBDS |
> > EFI_AHCI_PORT_IS_HBFS | EFI_AHCI_PORT_IS_TFES)
> >
> > #define EFI_AHCI_PORT_IE 0x0014
> > #define EFI_AHCI_PORT_CMD 0x0018
> > @@ -240,6 +241,12 @@ typedef struct {
> > UINT8 AhciCFisRsvd5[44];
> > } EFI_AHCI_COMMAND_FIS;
> >
> > +typedef enum {
> > + SataFisD2H = 0,
> > + SataFisPioSetup,
> > + SataFisDmaSetup
> > +} SATA_FIS_TYPE;
> > +
> > //
> > // ACMD: ATAPI command (12 or 16 bytes) //
> > --
> > 2.28.0.windows.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66982): https://edk2.groups.io/g/devel/message/66982
Mute This Topic: https://groups.io/mt/78005848/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list