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.65; helo=mga03.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 E9684203B8CE6 for ; Sun, 3 Jun 2018 23:20:53 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jun 2018 23:20:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,475,1520924400"; d="scan'208";a="53448200" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga002.fm.intel.com with ESMTP; 03 Jun 2018 23:20:53 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 3 Jun 2018 23:20:53 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 3 Jun 2018 23:20:52 -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:20:51 +0800 From: "Wu, Hao A" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: Enable/disable DEVSLP per policy Thread-Index: AQHT+WrnS4/mCzNmL0ipZveTQHAGFqRPo+mA Date: Mon, 4 Jun 2018 06:20:50 +0000 Message-ID: References: <20180601053921.34724-1-ruiyu.ni@intel.com> <20180601053921.34724-5-ruiyu.ni@intel.com> In-Reply-To: <20180601053921.34724-5-ruiyu.ni@intel.com> Accept-Language: zh-CN, en-US 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:20:54 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray, Some comments: 1. Please help to add the 'EFI_' prefix for the new definitions added in .h files. 2. Do we need to handle the case when AhciEnableDevSlp() returns with an er= ror? 3. Please help to adjust the debug level for the below line within function AhciEnableDevSlp(): DEBUG ((DEBUG_ERROR, "Port CMD/DEVSLP =3D %08x / %08x\n", PortCmd, PortD= evSlp)); 4. The function description for AhciReadLogExt() does not match it prototyp= e. 5. Please add the return value descriptions for function AhciEnableDevSlp()= . Also, could you help to provide what kind of tests have been performed for = this patch and for the whole series? Thanks. 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 >=20 > 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(-) >=20 > 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; > } >=20 > +/** > + Read logs from SATA device. > + > + @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 timeout value of stop. > + @param Buffer The data buffer to store IDENTIFY PACKET d= ata. > + > + @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 executing. > + @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 NU= LL) { > + 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_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 > +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, [7= 9] > =3D %04x\n", > + IdentifyData->AtaData.reserved_77, > + IdentifyData->AtaData.serial_ata_features_supported, IdentifyD= ata- > >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; > +} >=20 > /** > 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 > + ); > } >=20 > // > 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. >=20 > - 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 @@ >=20 > #define EFI_AHCI_MAX_PORTS 32 >=20 > +#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 >=20 > #pragma pack(1) > // > @@ -283,6 +293,15 @@ typedef struct { > UINT8 AhciUnknownFisRsvd[0x60]; > } EFI_AHCI_RECEIVED_FIS; >=20 > +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() >=20 > typedef struct { > -- > 2.16.1.windows.1