From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Ni, Ray" <ray.ni@intel.com>
Subject: Re: [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow
Date: Wed, 4 Nov 2020 05:33:26 +0000 [thread overview]
Message-ID: <BN8PR11MB3666EC7DB06F570A3FFB5B2ACAEF0@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201103132348.2916-3-mateusz.albecki@intel.com>
> -----Original Message-----
> From: Albecki, Mateusz <mateusz.albecki@intel.com>
> Sent: Tuesday, November 3, 2020 9:24 PM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error
> recovery flow
>
> From: Albecki <mateusz.albecki@intel.com>
Similar to patch 1, please help to check the author information for the patch.
The patch itself looks good to me:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3025
>
> This commit adds error recovery flow on SATA port when the error condition
> is reported. Commit only implements SATA port reset flow which is executed
> when PxTFD indicates BSY or DRQ. Commit does not implement HBA level
> reset.
>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
>
> ---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 178 +++++++++++++++---
> .../Bus/Ata/AtaAtapiPassThru/AhciMode.h | 5 +-
> 2 files changed, 159 insertions(+), 24 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index 4b42e72226..cf735d5983 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -608,6 +608,148 @@ AhciBuildCommandFis (
> CmdFis->AhciCFisDevHead = (UINT8) (AtaCommandBlock-
> >AtaDeviceHead | 0xE0);
> }
>
> +/**
> + Wait until SATA device reports it is ready for operation.
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to reset.
> +
> + @retval EFI_SUCCESS Device ready for operation.
> + @retval EFI_TIMEOUT Device failed to get ready within required period.
> +**/
> +EFI_STATUS
> +AhciWaitDeviceReady (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 PhyDetectDelay;
> + UINT32 Data;
> + UINT32 Offset;
> +
> + //
> + // According to SATA1.0a spec section 5.2, we need to wait for
> + PxTFD.BSY and PxTFD.DRQ // and PxTFD.ERR to be zero. The maximum
> wait time is 16s which is defined at ATA spec.
> + //
> + PhyDetectDelay = 16 * 1000;
> + do {
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_SERR;
> + if (AhciReadReg(PciIo, Offset) != 0) {
> + AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
> + }
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_TFD;
> +
> + Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
> + if (Data == 0) {
> + break;
> + }
> +
> + MicroSecondDelay (1000);
> + PhyDetectDelay--;
> + } while (PhyDetectDelay > 0);
> +
> + if (PhyDetectDelay == 0) {
> + DEBUG ((DEBUG_ERROR, "Port %d Device not ready (TFD=0x%X)\n", Port,
> Data));
> + return EFI_TIMEOUT;
> + } else {
> + return EFI_SUCCESS;
> + }
> +}
> +
> +
> +/**
> + Reset the SATA port. Algorithm follows AHCI spec 1.3.1 section 10.4.2
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to reset.
> +
> + @retval EFI_SUCCESS Port reset.
> + @retval Others Failed to reset the port.
> +**/
> +EFI_STATUS
> +AhciResetPort (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 Offset;
> + EFI_STATUS Status;
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_SCTL; AhciOrReg (PciIo, Offset,
> + EFI_AHCI_PORT_SCTL_DET_INIT); // // SW is required to keep DET set
> + to 0x1 at least for 1 milisecond to ensure that // at least one
> + COMRESET signal is sent.
> + //
> + MicroSecondDelay(1000);
> + AhciAndReg (PciIo, Offset, ~(UINT32)EFI_AHCI_PORT_SSTS_DET_MASK);
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_SSTS; Status = AhciWaitMmioSet (PciIo, Offset,
> + EFI_AHCI_PORT_SSTS_DET_MASK, EFI_AHCI_PORT_SSTS_DET_PCE,
> ATA_ATAPI_TIMEOUT); if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return AhciWaitDeviceReady (PciIo, Port); }
> +
> +/**
> + Recovers the SATA port from error condition.
> + This function implements algorithm described in
> + AHCI spec 1.3.1 section 6.2.2
> +
> + @param[in] PciIo Pointer to AHCI controller PciIo.
> + @param[in] Port SATA port index on which to check.
> +
> + @retval EFI_SUCCESS Port recovered.
> + @retval Others Failed to recover port.
> +**/
> +EFI_STATUS
> +AhciRecoverPortError (
> + IN EFI_PCI_IO_PROTOCOL *PciIo,
> + IN UINT8 Port
> + )
> +{
> + UINT32 Offset;
> + UINT32 PortInterrupt;
> + UINT32 PortTfd;
> + EFI_STATUS Status;
> +
> + 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_FATAL_ERROR_MASK) == 0) {
> + //
> + // No fatal error detected. Exit with success as port should still be
> operational.
> + // No need to clear IS as it will be cleared when the next command starts.
> + //
> + return EFI_SUCCESS;
> + }
> +
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_CMD; AhciAndReg (PciIo, Offset,
> + ~(UINT32)EFI_AHCI_PORT_CMD_ST);
> +
> + Status = AhciWaitMmioSet (PciIo, Offset, EFI_AHCI_PORT_CMD_CR, 0,
> + ATA_ATAPI_TIMEOUT); if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Ahci port %d is in hung state, aborting
> recovery\n", Port));
> + return Status;
> + }
> +
> + //
> + // If TFD.BSY or TFD.DRQ is still set it means that drive is hung and
> + software has // to reset it before sending any additional commands.
> + //
> + Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> + EFI_AHCI_PORT_TFD; PortTfd = AhciReadReg (PciIo, Offset); if
> + ((PortTfd & (EFI_AHCI_PORT_TFD_BSY | EFI_AHCI_PORT_TFD_DRQ)) != 0)
> {
> + Status = AhciResetPort (PciIo, Port);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Failed to reset the port %d\n", Port));
> + }
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Checks if specified FIS has been received.
>
> @@ -827,6 +969,10 @@ AhciPioTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> }
>
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
> +
> Exit:
> AhciStopCommand (
> PciIo,
> @@ -1007,6 +1153,10 @@ AhciDmaTransfer (
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> }
>
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
> +
> Exit:
> //
> // For Blocking mode, the command should be stopped, the Fis should be
> disabled @@ -1119,6 +1269,9 @@ AhciNonDataTransfer (
> }
>
> Status = AhciWaitUntilFisReceived (PciIo, Port, Timeout, SataFisD2H);
> + if (Status == EFI_DEVICE_ERROR) {
> + AhciRecoverPortError (PciIo, Port); }
>
> Exit:
> AhciStopCommand (
> @@ -2583,29 +2736,8 @@ AhciModeInitialization (
> continue;
> }
>
> - //
> - // According to SATA1.0a spec section 5.2, we need to wait for PxTFD.BSY
> and PxTFD.DRQ
> - // and PxTFD.ERR to be zero. The maximum wait time is 16s which is
> defined at ATA spec.
> - //
> - PhyDetectDelay = 16 * 1000;
> - do {
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_SERR;
> - if (AhciReadReg(PciIo, Offset) != 0) {
> - AhciWriteReg (PciIo, Offset, AhciReadReg(PciIo, Offset));
> - }
> - Offset = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH +
> EFI_AHCI_PORT_TFD;
> -
> - Data = AhciReadReg (PciIo, Offset) & EFI_AHCI_PORT_TFD_MASK;
> - if (Data == 0) {
> - break;
> - }
> -
> - MicroSecondDelay (1000);
> - PhyDetectDelay--;
> - } while (PhyDetectDelay > 0);
> -
> - if (PhyDetectDelay == 0) {
> - DEBUG ((EFI_D_ERROR, "Port %d Device presence detected but phy not
> ready (TFD=0x%X)\n", Port, Data));
> + Status = AhciWaitDeviceReady (PciIo, Port);
> + if (EFI_ERROR (Status)) {
> continue;
> }
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> index a3cd351f6e..4d2aafe483 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h
> @@ -114,6 +114,7 @@ typedef union {
> #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_IS_FATAL_ERROR_MASK
> (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
> @@ -122,9 +123,11 @@ typedef union {
> #define EFI_AHCI_PORT_CMD_SUD BIT1
> #define EFI_AHCI_PORT_CMD_POD BIT2
> #define EFI_AHCI_PORT_CMD_CLO BIT3
> -#define EFI_AHCI_PORT_CMD_CR BIT15
> #define EFI_AHCI_PORT_CMD_FRE BIT4
> +#define EFI_AHCI_PORT_CMD_CCS_MASK (BIT8 | BIT9 | BIT10 |
> BIT11 | BIT12)
> +#define EFI_AHCI_PORT_CMD_CCS_SHIFT 8
> #define EFI_AHCI_PORT_CMD_FR BIT14
> +#define EFI_AHCI_PORT_CMD_CR BIT15
> #define EFI_AHCI_PORT_CMD_MASK ~(EFI_AHCI_PORT_CMD_ST |
> EFI_AHCI_PORT_CMD_FRE | EFI_AHCI_PORT_CMD_COL)
> #define EFI_AHCI_PORT_CMD_PMA BIT17
> #define EFI_AHCI_PORT_CMD_HPCP BIT18
> --
> 2.28.0.windows.1
next prev parent reply other threads:[~2020-11-04 5:33 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-03 13:23 [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Albecki, Mateusz
2020-11-03 13:23 ` [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Check IS to check for command completion Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-04 11:54 ` Albecki, Mateusz
2020-11-03 13:23 ` [PATCH 2/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery flow Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A [this message]
2020-11-03 13:23 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: Restart failed packets Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-03 13:23 ` [PATCH 4/4] MdeModulePkg/AtaAtapiPassThru: Trace ATA packets Albecki, Mateusz
2020-11-04 5:33 ` Wu, Hao A
2020-11-04 1:57 ` [PATCH 0/4] MdeModulePkg/AtaAtapiPassThru: Add SATA error recovery Wu, Hao A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB3666EC7DB06F570A3FFB5B2ACAEF0@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox