From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: "Wu, Hao A" <hao.a.wu@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: Enable/disable DEVSLP per policy
Date: Mon, 4 Jun 2018 06:36:49 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BD235FF@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <B80AF82E9BFB8E4FBD8C89DA810C6A0931E05FB4@SHSMSX104.ccr.corp.intel.com>
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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>
> 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/macros.
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.
> 3. Please help to adjust the debug level for the below line within function
> AhciEnableDevSlp():
> DEBUG ((DEBUG_ERROR, "Port CMD/DEVSLP = %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 <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 | 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_REGISTERS.
> > + @param Port The number of port.
> > + @param PortMultiplier The timeout value of stop.
> > + @param Buffer The data buffer to store IDENTIFY PACKET 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 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 == NULL || AhciRegisters == NULL || Buffer == 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 = ATA_CMD_READ_LOG_EXT;
> > + AtaCommandBlock.AtaSectorCount = 1;
> > + AtaCommandBlock.AtaSectorNumber = LogNumber;
> > + AtaCommandBlock.AtaCylinderLow = 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 == 0) {
> > + return EFI_SUCCESS;
> > + }
> > +
> > + //
> > + // Do not enable DevSlp if DevSlp is not supported.
> > + //
> > + Capability2 = AhciReadReg (PciIo, AHCI_CAPABILITY2_OFFSET); DEBUG
> > + ((DEBUG_INFO, "AHCI CAPABILITY2 = %08x\n", Capability2)); if
> > + ((Capability2 & AHCI_CAP2_SDS) == 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 = EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH;
> > + PortCmd = AhciReadReg (PciIo, Offset + EFI_AHCI_PORT_CMD);
> > + PortDevSlp = AhciReadReg (PciIo, Offset + AHCI_PORT_DEVSLP); DEBUG
> > + ((DEBUG_ERROR, "Port CMD/DEVSLP = %08x / %08x\n", PortCmd,
> > PortDevSlp));
> > + if (((PortDevSlp & AHCI_PORT_DEVSLP_DSP) == 0) ||
> > + ((PortCmd & (EFI_AHCI_PORT_CMD_HPCP |
> > EFI_AHCI_PORT_CMD_MPSP)) != 0)
> > + ) {
> > + return EFI_UNSUPPORTED;
> > + }
> > +
> > + //
> > + // Do not enable DevSlp if the device doesn't support DevSlp //
> > + DEBUG ((DEBUG_INFO, "IDENTIFY DEVICE: [77] = %04x, [78] = %04x, [79]
> > = %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) == 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) != 0) {
> > + Status = 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 = 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 &= ~AHCI_PORT_DEVSLP_ADSE; AhciWriteReg (PciIo, Offset
> > + + AHCI_PORT_DEVSLP, PortDevSlp);
> > +
> > + //
> > + // Set PxDEVSLP.DETO and PxDEVSLP.MDAT to 0.
> > + //
> > + PortDevSlp &= ~AHCI_PORT_DEVSLP_DETO_MASK; PortDevSlp &=
> > + ~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 = 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 == 0) || (DevSlpTiming.Deto == 0)) {
> > + DevSlpTiming.Deto = 20;
> > + }
> > +
> > + //
> > + // Use 10ms as default MADT when DEVSLP TIMING VARIABLES is not
> > supported or the MADT is 0.
> > + //
> > + if ((DevSlpTiming.Supported == 0) || (DevSlpTiming.Madt == 0)) {
> > + DevSlpTiming.Madt = 10;
> > + }
> > +
> > + PortDevSlp |= DevSlpTiming.Deto << 2; PortDevSlp |=
> > + DevSlpTiming.Madt << 10; AhciOrReg (PciIo, Offset +
> > + AHCI_PORT_DEVSLP, PortDevSlp);
> > +
> > + if (mAtaAtapiPolicy->AggressiveDeviceSleepEnable == 1) {
> > + if ((Capability2 & AHCI_CAP2_SADM) != 0) {
> > + PortDevSlp &= ~AHCI_PORT_DEVSLP_DITO_MASK;
> > + PortDevSlp |= (625 << 15);
> > + AhciWriteReg (PciIo, Offset + AHCI_PORT_DEVSLP, PortDevSlp);
> > +
> > + PortDevSlp |= 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 = %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 == 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.<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 @@ -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
next prev parent reply other threads:[~2018-06-04 6:37 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 5:39 [PATCH 0/4] Support PUIS and DEVSLP feature Ruiyu Ni
2018-06-01 5:39 ` [PATCH 1/4] MdeModulePkg/AtaAtapiPassThru: Spin up Power up in Standby devices Ruiyu Ni
2018-06-04 3:51 ` Chiu, Chasel
2018-06-04 5:53 ` Wu, Hao A
2018-06-01 5:39 ` [PATCH 2/4] MdeModulePkg: Add AtaAtapiPolicy protocol definition Ruiyu Ni
2018-06-04 6:29 ` Zeng, Star
2018-06-04 6:49 ` Ni, Ruiyu
2018-06-01 5:39 ` [PATCH 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS per policy Ruiyu Ni
2018-06-04 3:51 ` Chiu, Chasel
2018-06-01 5:39 ` [PATCH 4/4] MdeModulePkg/Ata/AtaAtapiPassThru: Enable/disable DEVSLP " Ruiyu Ni
2018-06-04 3:51 ` Chiu, Chasel
2018-06-04 6:20 ` Wu, Hao A
2018-06-04 6:36 ` Ni, Ruiyu [this message]
2018-06-04 6:45 ` Wu, Hao A
2018-06-04 6:48 ` Ni, Ruiyu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5BD235FF@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox