[edk2-devel] [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion
Wu, Hao A
hao.a.wu at intel.com
Wed Nov 4 05:33:16 UTC 2020
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66959): https://edk2.groups.io/g/devel/message/66959
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