Re: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Spin up Power up in Standby devices

["Wu, Hao A" <hao.a.wu@intel.com>]

One minor comment below.

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, June 01, 2018 1:39 PM
> To: edk2-devel@lists.01.org
> Cc: Chiu, Chasel; Wu, Hao A
> Subject: [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Spin up Power up
> in Standby devices
> 
> The patch adds support to certain devices that support PUIS (Power
> up in Standby).
> For those devices that supports SET_FEATURE spin up, SW needs to
> send SET_FEATURE subcommand to spin up the devices.
> For those devices that doesn't support SET_FEATURE spin up, SW needs
> to send read sectors command to spin up the devices.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c   | 129
> ++++++++++++++++++++-
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |   3 +-
>  2 files changed, 127 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index e6de5d65bc..6ab7870570 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -1,7 +1,7 @@
>  /** @file
>    The file for AHCI mode of ATA host controller.
> 
> -  Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
>    (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -1841,7 +1841,8 @@ AhciDeviceSetFeature (
>    IN UINT8                  Port,
>    IN UINT8                  PortMultiplier,
>    IN UINT16                 Feature,
> -  IN UINT32                 FeatureSpecificData
> +  IN UINT32                 FeatureSpecificData,
> +  IN UINT64                 Timeout
>    )
>  {
>    EFI_STATUS               Status;
> @@ -1868,7 +1869,7 @@ AhciDeviceSetFeature (
>               0,
>               &AtaCommandBlock,
>               &AtaStatusBlock,
> -             ATA_ATAPI_TIMEOUT,
> +             Timeout,
>               NULL
>               );
> 
> @@ -2216,6 +2217,104 @@ Error6:
>    return Status;
>  }
> 
> +
> +/**
> +  Spin-up disk if IDD was incomplete or PUIS feature is enabled
> +
> +  @param  PciIo               The PCI IO protocol instance.
> +  @param  AhciRegisters       The pointer to the EFI_AHCI_REGISTERS.
> +  @param  Port                The number of port.
> +  @param  PortMultiplier      The multiplier of port.
> +  @param  IdentifyData        A pointer to data buffer which is used to contain
> IDENTIFY data.
> +
> +**/
> +EFI_STATUS
> +AhciSpinUpDisk (
> +  IN EFI_PCI_IO_PROTOCOL           *PciIo,
> +  IN EFI_AHCI_REGISTERS            *AhciRegisters,
> +  IN UINT8                         Port,
> +  IN UINT8                         PortMultiplier,
> +  IN OUT EFI_IDENTIFY_DATA         *IdentifyData
> +  )
> +{
> +  EFI_STATUS               Status;
> +  EFI_ATA_COMMAND_BLOCK    AtaCommandBlock;
> +  EFI_ATA_STATUS_BLOCK     AtaStatusBlock;
> +  UINT8                    Buffer[512];
> +
> +  if (IdentifyData->AtaData.specific_config ==
> ATA_SPINUP_CFG_REQUIRED_IDD_INCOMPLETE) {
> +    //
> +    // Use SET_FEATURE subcommand to spin up the device.
> +    //
> +    Status = AhciDeviceSetFeature (
> +               PciIo, AhciRegisters, Port, PortMultiplier,
> +               ATA_SUB_CMD_PUIS_SET_DEVICE_SPINUP, 0x00,
> ATA_SPINUP_TIMEOUT
> +               );
> +    DEBUG ((DEBUG_INFO, "CMD_PUIS_SET_DEVICE_SPINUP for device at
> port [%d] PortMultiplier [%d] - %r!\n",
> +            Port, PortMultiplier, Status));
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  } else {
> +    ASSERT (IdentifyData->AtaData.specific_config ==
> ATA_SPINUP_CFG_NOT_REQUIRED_IDD_INCOMPLETE);
> +
> +    //
> +    // Use READ_SECTORS to spin up the device if SpinUp SET FEATURE
> subcommand is not supported
> +    //
> +    ZeroMem (&AtaCommandBlock, sizeof (EFI_ATA_COMMAND_BLOCK));
> +    ZeroMem (&AtaStatusBlock, sizeof (EFI_ATA_STATUS_BLOCK));
> +    //
> +    // Perform READ SECTORS PIO Data-In command to Read LBA 0
> +    //
> +    AtaCommandBlock.AtaCommand      = ATA_CMD_READ_SECTORS;
> +    AtaCommandBlock.AtaSectorCount  = 0x1;
> +
> +    Status = AhciPioTransfer (
> +               PciIo,
> +               AhciRegisters,
> +               Port,
> +               PortMultiplier,
> +               NULL,
> +               0,
> +               TRUE,
> +               &AtaCommandBlock,
> +               &AtaStatusBlock,
> +               &Buffer,
> +               sizeof (Buffer),
> +               ATA_SPINUP_TIMEOUT,
> +               NULL
> +               );
> +    DEBUG ((DEBUG_INFO, "Read LBA 0 for device at port [%d] PortMultiplier
> [%d] - %r!\n",
> +            Port, PortMultiplier, Status));
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  }
> +
> +  //
> +  // Read the complete IDENTIFY DEVICE data.
> +  //
> +  ZeroMem (IdentifyData, sizeof (*IdentifyData));
> +  Status = AhciIdentify (PciIo, AhciRegisters, Port, PortMultiplier,
> IdentifyData);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Read IDD failed for device at port [%d]
> PortMultiplier [%d] - %r!\n",
> +            Port, PortMultiplier, Status));
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "IDENTIFY DEVICE: [0] = %016x, [2] = %016x, [83]
> = %016x, [86] = %016x\n",
> +          IdentifyData->AtaData.config, IdentifyData->AtaData.specific_config,
> +          IdentifyData->AtaData.command_set_supported_83, IdentifyData-
> >AtaData.command_set_feature_enb_86));
> +  //
> +  // Check if IDD is incomplete
> +  //
> +  if ((IdentifyData->AtaData.config & BIT2) != 0) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Initialize ATA host controller at AHCI mode.
> 
> @@ -2458,6 +2557,28 @@ AhciModeInitialization (
>            continue;
>          }
> 
> +        DEBUG ((
> +          DEBUG_INFO, "IDENTIFY DEVICE: [0] = %016x, [2] = %016x, [83]
> = %016x, [86] = %016x\n",
> +          Buffer.AtaData.config, Buffer.AtaData.specific_config,
> +          Buffer.AtaData.command_set_supported_83,
> Buffer.AtaData.command_set_feature_enb_86
> +          ));
> +        if ((Buffer.AtaData.config & BIT2) != 0) {
> +          //
> +          // SpinUp disk if device reported incomplete IDENTIFY DEVICE.
> +          //
> +          Status = AhciSpinUpDisk (
> +                     PciIo,
> +                     AhciRegisters,
> +                     Port,
> +                     0,
> +                     &Buffer
> +                     );
> +          if (EFI_ERROR (Status)) {
> +            DEBUG ((DEBUG_ERROR, "Spin up standby device failed - %r\n",
> Status));
> +            continue;
> +          }
> +        }
> +
>          DeviceType = EfiIdeHarddisk;
>        } else {
>          continue;
> @@ -2523,7 +2644,7 @@ AhciModeInitialization (
>          TransferMode.ModeNumber = (UINT8) SupportedModes-
> >MultiWordDmaMode.Mode;
>        }
> 
> -      Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 0, 0x03,
> (UINT32)(*(UINT8 *)&TransferMode));
> +      Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 0, 0x03,
> (UINT32)(*(UINT8 *)&TransferMode), ATA_ATAPI_TIMEOUT); // RPPO-SKL-
> TBD

Please help to remove the comment here.
With this change,
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

>        if (EFI_ERROR (Status)) {
>          DEBUG ((EFI_D_ERROR, "Set transfer Mode Fail, Status = %r\n", Status));
>          continue;
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> index 85e5a55539..31b005f2f6 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Header file for ATA/ATAPI PASS THRU driver.
> 
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -148,6 +148,7 @@ struct _ATA_NONBLOCK_TASK {
>  // It means 3 second span.
>  //
>  #define ATA_ATAPI_TIMEOUT           EFI_TIMER_PERIOD_SECONDS(3)
> +#define ATA_SPINUP_TIMEOUT          EFI_TIMER_PERIOD_SECONDS(10)
> 
>  #define IS_ALIGNED(addr, size)      (((UINTN) (addr) & (size - 1)) == 0)
> 
> --
> 2.16.1.windows.1