Fine.
I just had the thought from my first view to the code.
When I saw the name AhciPuisEnable, I thought there should be AhciPuisDisable
as well, but I did not find it.
Then I must check the comments and function header to know the details.
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Tuesday, June 5, 2018 1:40 PM
To: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org
Cc: Chiu, Chasel <chasel.c...@intel.com>
Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS
per policy
I see your point.
I prefer to use AhciPuisEnable().
The function name describe the purpose.
The function body explains the details.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, June 5, 2018 1:17 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.c...@intel.com>; Zeng, Star
> <star.z...@intel.com>
> Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> enable/disable PUIS per policy
>
> 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.z...@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.c...@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...@intel.com>; edk2-devel@lists.01.org
> > Cc: Chiu, Chasel <chasel.c...@intel.com>; Zeng, Star
> > <star.z...@intel.com>
> > Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> > enable/disable PUIS per policy
> >
> > Reviewed-by: Star Zeng <star.z...@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.z...@intel.com>; Chiu, Chasel
> > <chasel.c...@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...@intel.com>
> > Cc: Star Zeng <star.z...@intel.com>
> > Cc: Chasel Chiu <chasel.c...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
