Re: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS per policy

["Zeng, Star" <star.zeng@intel.com>]

Yes, that is why I was saying to use AhciSetFeaturePuis as the function name.


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Tuesday, June 5, 2018 11:41 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Chiu, Chasel <chasel.chiu@intel.com>
Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS per policy

The Set Feature cmd is used to enable/disable PUIS.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, June 5, 2018 11:35 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> enable/disable PUIS per policy
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> How about using function name AhciSetFeaturePuis instead of 
> AhciPuisEnable as the function is not just to enable Puis?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, June 4, 2018 3:04 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Chiu, Chasel 
> <chasel.chiu@intel.com>
> Subject: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable 
> PUIS per policy
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c   | 48
> ++++++++++++++++++++++
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 19 ++++++++-
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  3 ++
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf  |  3 +-
>  4 files changed, 71 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> index 14578c0f94..841b6a0e60 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> @@ -2316,6 +2316,38 @@ AhciSpinUpDisk (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Enable/disable/skip PUIS of the disk according to policy.
> +
> +  @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.
> +
> +**/
> +EFI_STATUS
> +AhciPuisEnable (
> +  IN EFI_PCI_IO_PROTOCOL           *PciIo,
> +  IN EFI_AHCI_REGISTERS            *AhciRegisters,
> +  IN UINT8                         Port,
> +  IN UINT8                         PortMultiplier
> +  )
> +{
> +  EFI_STATUS                       Status;
> +
> +  Status = EFI_SUCCESS;
> +  if (mAtaAtapiPolicy->PuisEnable == 0) {
> +    Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 
> +PortMultiplier, ATA_SUB_CMD_DISABLE_PUIS, 0x00,
> ATA_ATAPI_TIMEOUT);
> +  } else if (mAtaAtapiPolicy->PuisEnable == 1) {
> +    Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 
> +PortMultiplier, ATA_SUB_CMD_ENABLE_PUIS, 0x00,
> ATA_ATAPI_TIMEOUT);
> +  }
> +  DEBUG ((DEBUG_INFO, "%a PUIS feature at port [%d] PortMultiplier 
> + [%d] -
>  %r!\n",
> +    (mAtaAtapiPolicy->PuisEnable == 0) ? "Disable" : (
> +    (mAtaAtapiPolicy->PuisEnable == 1) ? "Enable" : "Skip"
> +      ), Port, PortMultiplier, Status));
> +  return Status;
> +}
> +
>  /**
>    Initialize ATA host controller at AHCI mode.
> 
> @@ -2658,6 +2690,22 @@ AhciModeInitialization (
>        if (DeviceType == EfiIdeHarddisk) {
>          REPORT_STATUS_CODE (EFI_PROGRESS_CODE, 
> (EFI_PERIPHERAL_FIXED_MEDIA | EFI_P_PC_ENABLE));
>        }
> +
> +      //
> +      // Enable/disable PUIS according to policy setting if PUIS is 
> + capable
> (Word[83].BIT5 is set).
> +      //
> +      if ((Buffer.AtaData.command_set_supported_83 & BIT5) != 0) {
> +        Status = AhciPuisEnable (
> +                   PciIo,
> +                   AhciRegisters,
> +                   Port,
> +                   0
> +                   );
> +        if (EFI_ERROR (Status)) {
> +          DEBUG ((DEBUG_ERROR, "PUIS enable/disable failed, Status = 
> + %r\n",
> Status));
> +          continue;
> +        }
> +      }
>      }
>    }
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> index a48b295d26..aab704bcd3 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> @@ -2,7 +2,7 @@
>    This file implements ATA_PASSTHRU_PROCTOCOL and 
> EXT_SCSI_PASSTHRU_PROTOCOL interfaces
>    for managed ATA controllers.
> 
> -  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 @@ -142,6 +142,15 @@ UINT8 mScsiId[TARGET_MAX_BYTES] = {
>    0xFF, 0xFF, 0xFF, 0xFF
>  };
> 
> +EDKII_ATA_ATAPI_POLICY_PROTOCOL *mAtaAtapiPolicy; 
> +EDKII_ATA_ATAPI_POLICY_PROTOCOL mDefaultAtaAtapiPolicy = {
> +  EDKII_ATA_ATAPI_POLICY_VERSION,
> +  2,  // PuisEnable
> +  0,  // DeviceSleepEnable
> +  0,  // AggressiveDeviceSleepEnable
> +  0   // Reserved
> +};
> +
>  /**
>    Sends an ATA command to an ATA device that is attached to the ATA 
> controller. This function
>    supports both blocking I/O and non-blocking I/O. The blocking I/O 
> functionality is required, @@ -739,6 +748,14 @@ AtaAtapiPassThruStart (
>      goto ErrorExit;
>    }
> 
> +  Status = gBS->LocateProtocol (&gEdkiiAtaAtapiPolicyProtocolGuid,
> + NULL, (VOID **)&mAtaAtapiPolicy);  if (EFI_ERROR (Status)) {
> +    //
> +    // If there is no AtaAtapiPolicy exposed, use the default policy.
> +    //
> +    mAtaAtapiPolicy = &mDefaultAtaAtapiPolicy;  }
> +
>    //
>    // Allocate a buffer to store the ATA_ATAPI_PASS_THRU_INSTANCE data 
> structure
>    //
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> index 31b005f2f6..b07bcbbb3e 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> @@ -24,6 +24,7 @@
>  #include <Protocol/IdeControllerInit.h>  #include 
> <Protocol/AtaPassThru.h> #include <Protocol/ScsiPassThruExt.h>
> +#include <Protocol/AtaAtapiPolicy.h>
> 
>  #include <Library/DebugLib.h>
>  #include <Library/BaseLib.h>
> @@ -45,6 +46,8 @@ extern EFI_DRIVER_BINDING_PROTOCOL 
> gAtaAtapiPassThruDriverBinding;  extern EFI_COMPONENT_NAME_PROTOCOL  
> gAtaAtapiPassThruComponentName; extern EFI_COMPONENT_NAME2_PROTOCOL 
> gAtaAtapiPassThruComponentName2;
> 
> +extern EDKII_ATA_ATAPI_POLICY_PROTOCOL *mAtaAtapiPolicy;
> +
>  #define ATA_ATAPI_PASS_THRU_SIGNATURE  SIGNATURE_32 ('a', 'a', 'p', 't')
>  #define ATA_ATAPI_DEVICE_SIGNATURE     SIGNATURE_32 ('a', 'd', 'e', 'v')
>  #define ATA_NONBLOCKING_TASK_SIGNATURE  SIGNATURE_32 ('a', 't', 's',
> 'k') diff --git
> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> index 82d5f7a46c..d1ce859091 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> @@ -4,7 +4,7 @@
>  #  This driver installs AtaPassThru and ExtScsiPassThru protocol in 
> each ide/sata controller  #  to access to all attached Ata/Atapi devices.
>  #
> -#  Copyright (c) 2010 - 2014, 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 @@ 
> -67,6 +67,7 @@ [Protocols]
>    gEfiIdeControllerInitProtocolGuid             ## TO_START
>    gEfiDevicePathProtocolGuid                    ## TO_START
>    gEfiPciIoProtocolGuid                         ## TO_START
> +  gEdkiiAtaAtapiPolicyProtocolGuid              ## CONSUMES
> 
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable   ##
> SOMETIMES_CONSUMES
> --
> 2.16.1.windows.1