From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5CA2E207DF2B0 for ; Sun, 3 Jun 2018 23:48:48 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jun 2018 23:48:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,475,1520924400"; d="scan'208";a="60131856" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga004.fm.intel.com with ESMTP; 03 Jun 2018 23:48:47 -0700 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 3 Jun 2018 23:48:47 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 3 Jun 2018 23:48:46 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.87]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.51]) with mapi id 14.03.0319.002; Mon, 4 Jun 2018 14:48:45 +0800 From: "Ni, Ruiyu" To: "Wu, Hao A" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: Enable/disable DEVSLP per policy Thread-Index: AQHT+8wveo2Q1Lt6TEaufvx0eUgRmaRPoVrA//+ADwCAAIcJwA== Date: Mon, 4 Jun 2018 06:48:44 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BD23648@SHSMSX104.ccr.corp.intel.com> References: <20180601053921.34724-1-ruiyu.ni@intel.com> <20180601053921.34724-5-ruiyu.ni@intel.com> <734D49CCEBEEF84792F5B80ED585239D5BD235FF@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: Enable/disable DEVSLP per policy X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jun 2018 06:48:48 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks/Ray > -----Original Message----- > From: Wu, Hao A > Sent: Monday, June 4, 2018 2:45 PM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Chiu, Chasel > Subject: RE: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: > Enable/disable DEVSLP per policy >=20 > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Monday, June 04, 2018 2:37 PM > > To: Wu, Hao A; edk2-devel@lists.01.org > > Cc: Chiu, Chasel > > Subject: RE: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: > > Enable/disable DEVSLP per policy > > > > Hao, > > Thanks for the comments. > > Reply in below. > > > > Thanks/Ray > > > > > -----Original Message----- > > > From: Wu, Hao A > > > Sent: Monday, June 4, 2018 2:21 PM > > > To: Ni, Ruiyu ; edk2-devel@lists.01.org > > > Cc: Chiu, Chasel > > > Subject: RE: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: > > > Enable/disable DEVSLP per policy > > > > > > Hi Ray, > > > > > > Some comments: > > > 1. Please help to add the 'EFI_' prefix for the new definitions added= in .h > > > files. > > The "EFI_" prefix was removed by purpose. > > The offset and the bits location don't change even for non EFI. > > I think "EFI_" can only be used for UEFI/PI spec defined protocols/macr= os. > > I don't want to change existing macros. But for new added macros, I'd > > like to follow this guide line. > > > > > 2. Do we need to handle the case when AhciEnableDevSlp() returns > > > with an error? > > It's fine for DEVSLP not enabled. HOST controllers or devices may not > > support it. >=20 > Got it. > But what if the error status returned is not related with the support of > DEVSLP. > For example, if a call to the AhciDeviceSetFeature() to enable the DEVSLP > feature? >=20 > Maybe, adding a debug error message for the above case is enough? I have a debug message for the set feature failure: DEBUG ((DEBUG_INFO, "DevSlp set feature for device at port [%d] PortMul= tiplier [%d] - %r\n", Port, PortMultiplier, Status)); >=20 > Best Regards, > Hao Wu >=20 > > > > > 3. Please help to adjust the debug level for the below line within fu= nction > > > AhciEnableDevSlp(): > > > DEBUG ((DEBUG_ERROR, "Port CMD/DEVSLP =3D %08x / %08x\n", > PortCmd, > > > PortDevSlp)); > > Thanks. I will change to DEBUG_INFO. > > > > > 4. The function description for AhciReadLogExt() does not match it > > > prototype. > > Thanks. I will update the function header comments. > > > 5. Please add the return value descriptions for function > AhciEnableDevSlp(). > > Thanks. I will update the function header comments. > > I will run ECC for all code changes. > > > > > > Also, could you help to provide what kind of tests have been > > > performed for this patch and for the whole series? Thanks. > > Test performed: > > 1. Use Seagate/Matrox/WestDigital non-SSD hard drive to test enable > > PUIS 2. Use the same hard drive to boot. Make sure code can properly > > spin up the harddrive from PUIS state. > > 3. Use Intel 530 SSD to test DEVSLP enabling logic. > > > > > > > > Best Regards, > > > Hao Wu > > > > > > > -----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 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: > > > Enable/disable > > > > DEVSLP per policy > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Ruiyu Ni > > > > Cc: Chasel Chiu > > > > Cc: Hao A Wu > > > > --- > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 214 > > > > +++++++++++++++++++++++ > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h | 23 ++- > > > > 2 files changed, 235 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > > > index 46d5c68996..3741e3b782 100644 > > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c > > > > @@ -2217,6 +2217,213 @@ Error6: > > > > return Status; > > > > } > > > > > > > > +/** > > > > + Read logs from SATA device. > > > > + > > > > + @param PciIo The PCI IO protocol instance. > > > > + @param AhciRegisters The pointer to the EFI_AHCI_REGISTER= S. > > > > + @param Port The number of port. > > > > + @param PortMultiplier The timeout value of stop. > > > > + @param Buffer The data buffer to store IDENTIFY PA= CKET data. > > > > + > > > > + @retval EFI_DEVICE_ERROR The cmd abort with error occurs. > > > > + @retval EFI_TIMEOUT The operation is time out. > > > > + @retval EFI_UNSUPPORTED The device is not ready for executin= g. > > > > + @retval EFI_SUCCESS The cmd executes successfully. > > > > + > > > > +**/ > > > > +EFI_STATUS > > > > +AhciReadLogExt ( > > > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > + IN EFI_AHCI_REGISTERS *AhciRegisters, > > > > + IN UINT8 Port, > > > > + IN UINT8 PortMultiplier, > > > > + IN OUT UINT8 *Buffer, > > > > + IN UINT8 LogNumber, > > > > + IN UINT8 PageNumber > > > > + ) > > > > +{ > > > > + EFI_ATA_COMMAND_BLOCK AtaCommandBlock; > > > > + EFI_ATA_STATUS_BLOCK AtaStatusBlock; > > > > + > > > > + if (PciIo =3D=3D NULL || AhciRegisters =3D=3D NULL || Buffer =3D= =3D NULL) { > > > > + return EFI_INVALID_PARAMETER; } > > > > + > > > > + /// > > > > + /// Read log from device > > > > + /// > > > > + ZeroMem (&AtaCommandBlock, sizeof > > (EFI_ATA_COMMAND_BLOCK)); > > > > + ZeroMem (&AtaStatusBlock, sizeof (EFI_ATA_STATUS_BLOCK)); > > > ZeroMem > > > > + (Buffer, 512); > > > > + > > > > + AtaCommandBlock.AtaCommand =3D ATA_CMD_READ_LOG_EXT; > > > > + AtaCommandBlock.AtaSectorCount =3D 1; > > > > + AtaCommandBlock.AtaSectorNumber =3D LogNumber; > > > > + AtaCommandBlock.AtaCylinderLow =3D PageNumber; > > > > + > > > > + return AhciPioTransfer ( > > > > + PciIo, > > > > + AhciRegisters, > > > > + Port, > > > > + PortMultiplier, > > > > + NULL, > > > > + 0, > > > > + TRUE, > > > > + &AtaCommandBlock, > > > > + &AtaStatusBlock, > > > > + Buffer, > > > > + 512, > > > > + ATA_ATAPI_TIMEOUT, > > > > + NULL > > > > + ); > > > > +} > > > > + > > > > +/** > > > > + Enable DEVSLP command of the disk if supported. > > > > + Support for S3 resume to be added later > > > > + > > > > + @param PciIo The PCI IO protocol instance. > > > > + @param AhciRegisters The pointer to the EFI_AHCI_REGISTER= S. > > > > + @param Port The number of port. > > > > + @param PortMultiplier The multiplier of port. > > > > + @param IdentifyData A pointer to data buffer which is us= ed to > > > contain > > > > IDENTIFY data. > > > > + > > > > +**/ > > > > +EFI_STATUS > > > > +AhciEnableDevSlp ( > > > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > + IN EFI_AHCI_REGISTERS *AhciRegisters, > > > > + IN UINT8 Port, > > > > + IN UINT8 PortMultiplier, > > > > + IN EFI_IDENTIFY_DATA *IdentifyData > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + UINT32 Offset; > > > > + UINT32 Capability2; > > > > + UINT8 LogData[512]; > > > > + DEVSLP_TIMING_VARIABLES DevSlpTiming; > > > > + UINT32 PortCmd; > > > > + UINT32 PortDevSlp; > > > > + > > > > + if (mAtaAtapiPolicy->DeviceSleepEnable =3D=3D 0) { > > > > + return EFI_SUCCESS; > > > > + } > > > > + > > > > + // > > > > + // Do not enable DevSlp if DevSlp is not supported. > > > > + // > > > > + Capability2 =3D AhciReadReg (PciIo, AHCI_CAPABILITY2_OFFSET); > > > > + DEBUG ((DEBUG_INFO, "AHCI CAPABILITY2 =3D %08x\n", Capability2)); > > > > + if > > > > + ((Capability2 & AHCI_CAP2_SDS) =3D=3D 0) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + // > > > > + // Do not enable DevSlp if DevSlp is not present // Do not > > > > + enable DevSlp if Hot Plug or Mechanical Presence Switch is > > > > supported > > > > + // > > > > + Offset =3D EFI_AHCI_PORT_START + Port * > > EFI_AHCI_PORT_REG_WIDTH; > > > > + PortCmd =3D AhciReadReg (PciIo, Offset + EFI_AHCI_PORT_CMD); > > > > + PortDevSlp =3D AhciReadReg (PciIo, Offset + AHCI_PORT_DEVSLP); > > DEBUG > > > > + ((DEBUG_ERROR, "Port CMD/DEVSLP =3D %08x / %08x\n", PortCmd, > > > > PortDevSlp)); > > > > + if (((PortDevSlp & AHCI_PORT_DEVSLP_DSP) =3D=3D 0) || > > > > + ((PortCmd & (EFI_AHCI_PORT_CMD_HPCP | > > > > EFI_AHCI_PORT_CMD_MPSP)) !=3D 0) > > > > + ) { > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + // > > > > + // Do not enable DevSlp if the device doesn't support DevSlp > > > > + // DEBUG ((DEBUG_INFO, "IDENTIFY DEVICE: [77] =3D %04x, [78] =3D > > > > + %04x, [79] > > > > =3D %04x\n", > > > > + IdentifyData->AtaData.reserved_77, > > > > + IdentifyData->AtaData.serial_ata_features_supported, > > > > + IdentifyData- > > > > >AtaData.serial_ata_features_enabled)); > > > > + if ((IdentifyData->AtaData.serial_ata_features_supported & > > > > + BIT8) =3D=3D 0) > > { > > > > + DEBUG ((DEBUG_INFO, "DevSlp feature is not supported for > > > > + device at > > > > port [%d] PortMultiplier [%d]!\n", > > > > + Port, PortMultiplier)); > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + // > > > > + // Enable DevSlp when it is not enabled. > > > > + // > > > > + if ((IdentifyData->AtaData.serial_ata_features_enabled & BIT8) != =3D 0) > { > > > > + Status =3D AhciDeviceSetFeature ( > > > > + PciIo, AhciRegisters, Port, 0, > > ATA_SUB_CMD_ENABLE_SATA_FEATURE, > > > > 0x09, ATA_ATAPI_TIMEOUT > > > > + ); > > > > + DEBUG ((DEBUG_INFO, "DevSlp set feature for device at port > > > > + [%d] > > > > PortMultiplier [%d] - %r\n", > > > > + Port, PortMultiplier, Status)); > > > > + if (EFI_ERROR (Status)) { > > > > + return Status; > > > > + } > > > > + } > > > > + > > > > + Status =3D AhciReadLogExt(PciIo, AhciRegisters, Port, > > > > + PortMultiplier, LogData, > > > > 0x30, 0x08); > > > > + > > > > + // > > > > + // Clear PxCMD.ST and PxDEVSLP.ADSE before updating > > PxDEVSLP.DITO > > > > and PxDEVSLP.MDAT. > > > > + // > > > > + AhciWriteReg (PciIo, Offset + EFI_AHCI_PORT_CMD, PortCmd & > > > > ~EFI_AHCI_PORT_CMD_ST); > > > > + PortDevSlp &=3D ~AHCI_PORT_DEVSLP_ADSE; AhciWriteReg (PciIo, > > Offset > > > > + + AHCI_PORT_DEVSLP, PortDevSlp); > > > > + > > > > + // > > > > + // Set PxDEVSLP.DETO and PxDEVSLP.MDAT to 0. > > > > + // > > > > + PortDevSlp &=3D ~AHCI_PORT_DEVSLP_DETO_MASK; PortDevSlp &=3D > > > > + ~AHCI_PORT_DEVSLP_MDAT_MASK; AhciWriteReg (PciIo, Offset + > > > > + AHCI_PORT_DEVSLP, PortDevSlp); DEBUG ((DEBUG_INFO, "Read Log > > Ext > > > at > > > > + port [%d] PortMultiplier [%d] - > > > > %r\n", Port, PortMultiplier, Status)); > > > > + if (EFI_ERROR (Status)) { > > > > + // > > > > + // Assume DEVSLP TIMING VARIABLES is not supported if the > > > > + Identify > > > > Device Data log (30h, 8) fails > > > > + // > > > > + DevSlpTiming.Supported =3D 0; > > > > + } else { > > > > + CopyMem (&DevSlpTiming, &LogData[48], sizeof (DevSlpTiming)); > > > > + DEBUG ((DEBUG_INFO, "DevSlpTiming: Supported(%d), Deto(%d), > > > > Madt(%d)\n", > > > > + DevSlpTiming.Supported, DevSlpTiming.Deto, > > > > + DevSlpTiming.Madt)); } > > > > + > > > > + // > > > > + // Use 20ms as default DETO when DEVSLP TIMING VARIABLES is not > > > > supported or the DETO is 0. > > > > + // > > > > + if ((DevSlpTiming.Supported =3D=3D 0) || (DevSlpTiming.Deto =3D= =3D 0)) { > > > > + DevSlpTiming.Deto =3D 20; > > > > + } > > > > + > > > > + // > > > > + // Use 10ms as default MADT when DEVSLP TIMING VARIABLES is not > > > > supported or the MADT is 0. > > > > + // > > > > + if ((DevSlpTiming.Supported =3D=3D 0) || (DevSlpTiming.Madt =3D= =3D 0)) { > > > > + DevSlpTiming.Madt =3D 10; > > > > + } > > > > + > > > > + PortDevSlp |=3D DevSlpTiming.Deto << 2; PortDevSlp |=3D > > > > + DevSlpTiming.Madt << 10; AhciOrReg (PciIo, Offset + > > > > + AHCI_PORT_DEVSLP, PortDevSlp); > > > > + > > > > + if (mAtaAtapiPolicy->AggressiveDeviceSleepEnable =3D=3D 1) { > > > > + if ((Capability2 & AHCI_CAP2_SADM) !=3D 0) { > > > > + PortDevSlp &=3D ~AHCI_PORT_DEVSLP_DITO_MASK; > > > > + PortDevSlp |=3D (625 << 15); > > > > + AhciWriteReg (PciIo, Offset + AHCI_PORT_DEVSLP, > > > > + PortDevSlp); > > > > + > > > > + PortDevSlp |=3D AHCI_PORT_DEVSLP_ADSE; > > > > + AhciWriteReg (PciIo, Offset + AHCI_PORT_DEVSLP, PortDevSlp); > > > > + } > > > > + } > > > > + > > > > + > > > > + AhciWriteReg (PciIo, Offset + EFI_AHCI_PORT_CMD, PortCmd); > > > > + > > > > + DEBUG ((DEBUG_INFO, "Enabled DevSlp feature at port [%d] > > > > PortMultiplier [%d], Port CMD/DEVSLP =3D %08x / %08x\n", > > > > + Port, PortMultiplier, PortCmd, PortDevSlp)); > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > > > > > /** > > > > Spin-up disk if IDD was incomplete or PUIS feature is enabled > > > > @@ > > > > -2688,6 +2895,13 @@ AhciModeInitialization ( > > > > CreateNewDeviceInfo (Instance, Port, 0xFFFF, DeviceType, > &Buffer); > > > > if (DeviceType =3D=3D EfiIdeHarddisk) { > > > > REPORT_STATUS_CODE (EFI_PROGRESS_CODE, > > > > (EFI_PERIPHERAL_FIXED_MEDIA | EFI_P_PC_ENABLE)); > > > > + AhciEnableDevSlp ( > > > > + PciIo, > > > > + AhciRegisters, > > > > + Port, > > > > + 0, > > > > + &Buffer > > > > + ); > > > > } > > > > > > > > // > > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h > > > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h > > > > index 809bcc307f..0ef749b4c7 100644 > > > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h > > > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h > > > > @@ -1,7 +1,7 @@ > > > > /** @file > > > > Header file for AHCI mode of ATA host controller. > > > > > > > > - Copyright (c) 2010 - 2014, Intel Corporation. All rights > > > > reserved.
> > > > + Copyright (c) 2010 - 2018, Intel Corporation. All rights > > > > + reserved.
> > > > 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 @@ -29,6 +29,10 @@ > > > > > > > > #define EFI_AHCI_MAX_PORTS 32 > > > > > > > > +#define AHCI_CAPABILITY2_OFFSET 0x0024 > > > > +#define AHCI_CAP2_SDS BIT3 > > > > +#define AHCI_CAP2_SADM BIT4 > > > > + > > > > typedef struct { > > > > UINT32 Lower32; > > > > UINT32 Upper32; > > > > @@ -182,7 +186,13 @@ typedef union { > > > > #define EFI_AHCI_PORT_SACT 0x0034 > > > > #define EFI_AHCI_PORT_CI 0x0038 > > > > #define EFI_AHCI_PORT_SNTF 0x003C > > > > - > > > > +#define AHCI_PORT_DEVSLP 0x0044 > > > > +#define AHCI_PORT_DEVSLP_ADSE BIT0 > > > > +#define AHCI_PORT_DEVSLP_DSP BIT1 > > > > +#define AHCI_PORT_DEVSLP_DETO_MASK 0x000003FC > > > > +#define AHCI_PORT_DEVSLP_MDAT_MASK 0x00007C00 > > > > +#define AHCI_PORT_DEVSLP_DITO_MASK 0x01FF8000 > > > > +#define AHCI_PORT_DEVSLP_DM_MASK 0x1E000000 > > > > > > > > #pragma pack(1) > > > > // > > > > @@ -283,6 +293,15 @@ typedef struct { > > > > UINT8 AhciUnknownFisRsvd[0x60]; > > > > } EFI_AHCI_RECEIVED_FIS; > > > > > > > > +typedef struct { > > > > + UINT8 Madt : 5; > > > > + UINT8 Reserved_5 : 3; > > > > + UINT8 Deto; > > > > + UINT16 Reserved_16; > > > > + UINT32 Reserved_32 : 31; > > > > + UINT32 Supported : 1; > > > > +} DEVSLP_TIMING_VARIABLES; > > > > + > > > > #pragma pack() > > > > > > > > typedef struct { > > > > -- > > > > 2.16.1.windows.1