From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 1335321A00AD1 for ; Wed, 28 Jun 2017 02:51:39 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Jun 2017 02:53:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,274,1496127600"; d="scan'208";a="1165560426" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 28 Jun 2017 02:53:09 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Jun 2017 02:53:09 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Jun 2017 02:53:09 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0319.002; Wed, 28 Jun 2017 17:53:07 +0800 From: "Zeng, Star" To: Ard Biesheuvel , "Tian, Feng" CC: "leif.lindholm@linaro.org" , "edk2-devel@lists.01.org" , "Dong, Eric" , "Zeng, Star" Thread-Topic: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY detect timeout Thread-Index: AQHS7+fgPgs8WwAQJkmusOM+IOCtu6I58cjQ//99ZYCAAAHSAIAAAmcAgACUVWA= Date: Wed, 28 Jun 2017 09:53:06 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF68E@shsmsx102.ccr.corp.intel.com> References: <20170628082323.30270-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF571@shsmsx102.ccr.corp.intel.com> <7F1BAD85ADEA444D97065A60D2E97EE569A14A33@SHSMSX101.ccr.corp.intel.com> In-Reply-To: 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 v2] MdeModulePkg/AtaAtapiPassThru: relax PHY detect timeout X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jun 2017 09:51:39 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable The real delay may be 0ms if the DET register value just pass the check aft= er the first AhciReadReg(). But what does "Wait at least 10 ms" mean? Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard = Biesheuvel Sent: Wednesday, June 28, 2017 4:58 PM To: Tian, Feng Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric ; Zeng, Star Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY det= ect timeout On 28 June 2017 at 08:49, Tian, Feng wrote: > Ard & Star, > > I agree this patch to solve device compatible issue as it has no side eff= ect for normal case. > > Just a minor correction: > SATA 2.5 spec says: "If a device is present, the Phy shall take no longer= than 10 ms to indicate that it has detected the presence of a device". Tha= t's why the original code think it *MUST* be less than 10ms. > Yes, I see where the confusion comes from. But the SATA spec primarily desc= ribes the requirements for the hardware side, which are the opposite of tho= se for the software side. Thanks, Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Wednesday, June 28, 2017 4:43 PM > To: Zeng, Star > Cc: edk2-devel@lists.01.org; Tian, Feng ; Dong,=20 > Eric ; leif.lindholm@linaro.org > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax=20 > PHY detect timeout > > On 28 June 2017 at 08:31, Zeng, Star wrote: >> The updated comments "Wait at least 10 ms" seems not correct. >> > > That is the whole point of the change. The SATA spec mandates that the=20 > PHY respond within 10 ms. It does *not* mandate that the software wait > 10 ms or less, rather the opposite, i.e., that the software should wait 1= 0 ms or *more*, so 'at least 10ms'. The original comment said that the soft= ware *must* wait for no more than 10 ms, but this is not what the spec says= . > > > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 >> Of Ard Biesheuvel >> Sent: Wednesday, June 28, 2017 4:23 PM >> To: edk2-devel@lists.01.org; Zeng, Star >> Cc: Tian, Feng ; Dong, Eric=20 >> ; leif.lindholm@linaro.org; Ard Biesheuvel=20 >> >> Subject: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY=20 >> detect timeout >> >> The SATA spec mandates that link detection by the PHY completes=20 >> within >> 10 ms after receiving a reset signal. However, there is no obligation=20 >> to uphold this requirement at the driver end as strictly as we do,=20 >> and as it turns out, some combinations of host and device (e.g.,=20 >> Samsung >> 850 EVO connected to a LeMaker Cello) are only borderline compliant, whi= ch means the device is not detected reliably. >> >> So let's allow for a bit of margin, and increase the PHY detect timeout = value to 15 ms. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> v2: update comment in AhciModeInitialization() as well >> >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 5 +++--=20 >> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 ++- >> 2 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> index 4d01c1dd7fca..4418e5c3763e 100644 >> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c >> @@ -2376,8 +2376,9 @@ AhciModeInitialization ( >> AhciOrReg (PciIo, Offset, EFI_AHCI_PORT_CMD_FRE); >> >> // >> - // Wait no longer than 10 ms to wait the Phy to detect the presen= ce of a device. >> - // It's the requirment from SATA1.0a spec section 5.2. >> + // Wait at least 10 ms for the Phy to detect the presence of a de= vice. >> + // It's the requirement from SATA1.0a spec section 5.2. >> + // Add a bit of margin for robustness. >> // >> PhyDetectDelay =3D EFI_AHCI_BUS_PHY_DETECT_TIMEOUT; >> Offset =3D EFI_AHCI_PORT_START + Port * EFI_AHCI_PORT_REG_WIDTH=20 >> + EFI_AHCI_PORT_SSTS; diff --git=20 >> a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h >> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h >> index 6401fb2e9fcd..809bcc307fc4 100644 >> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h >> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h >> @@ -41,8 +41,9 @@ typedef union { >> >> // >> // Refer SATA1.0a spec section 5.2, the Phy detection time should be le= ss than 10ms. >> +// Add a bit of margin for robustness. >> // >> -#define EFI_AHCI_BUS_PHY_DETECT_TIMEOUT 10 >> +#define EFI_AHCI_BUS_PHY_DETECT_TIMEOUT 15 >> // >> // Refer SATA1.0a spec, the FIS enable time should be less than 500ms. >> // >> -- >> 2.9.3 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel