From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 A7D6C21A00AD9 for ; Wed, 28 Jun 2017 03:42:40 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP; 28 Jun 2017 03:44:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,275,1496127600"; d="scan'208";a="118347721" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga005.jf.intel.com with ESMTP; 28 Jun 2017 03:44:11 -0700 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Jun 2017 03:44:11 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Jun 2017 03:44:11 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.116]) with mapi id 14.03.0319.002; Wed, 28 Jun 2017 18:44:06 +0800 From: "Zeng, Star" To: Ard Biesheuvel CC: "Tian, Feng" , "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//99ZYCAAAHSAIAAAmcAgACUVWD//3xxAIAAkfGQ Date: Wed, 28 Jun 2017 10:44:04 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF6E4@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> <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF68E@shsmsx102.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 10:42:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable The delay duration may be 0 - EFI_AHCI_BUS_PHY_DETECT_TIMEOUT ms, and the c= omments are for all cases, but not for specific cases. How about to just simplify the comments like below? // // Wait the Phy to detect the presence of a device. // Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]=20 Sent: Wednesday, June 28, 2017 5:58 PM To: Zeng, Star Cc: Tian, Feng ; leif.lindholm@linaro.org; edk2-devel@= lists.01.org; Dong, Eric Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY det= ect timeout > On 28 Jun 2017, at 09:53, Zeng, Star wrote: >=20 > The real delay may be 0ms if the DET register value just pass the check a= fter the first AhciReadReg(). > But what does "Wait at least 10 ms" mean? >=20 If there is a link, it may be detected in 0 ms. If there is no link, it wil= l take at least 10 ms to proceed, given that the phy may legally take 10 ms= to report presence of a link. So we can never report 'no link' in less tha= n 10 ms. Or am I missing something here? >=20 > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > 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=20 > ; Zeng, Star > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax=20 > PHY detect timeout >=20 >> On 28 June 2017 at 08:49, Tian, Feng wrote: >> Ard & Star, >>=20 >> I agree this patch to solve device compatible issue as it has no side ef= fect for normal case. >>=20 >> Just a minor correction: >> SATA 2.5 spec says: "If a device is present, the Phy shall take no longe= r than 10 ms to indicate that it has detected the presence of a device". Th= at's why the original code think it *MUST* be less than 10ms. >>=20 >=20 > Yes, I see where the confusion comes from. But the SATA spec primarily de= scribes the requirements for the hardware side, which are the opposite of t= hose for the software side. >=20 > Thanks, > Ard. >=20 >=20 >> -----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 >>=20 >>> On 28 June 2017 at 08:31, Zeng, Star wrote: >>> The updated comments "Wait at least 10 ms" seems not correct. >>>=20 >>=20 >> That is the whole point of the change. The SATA spec mandates that=20 >> the PHY respond within 10 ms. It does *not* mandate that the software=20 >> wait >> 10 ms or less, rather the opposite, i.e., that the software should wait = 10 ms or *more*, so 'at least 10ms'. The original comment said that the sof= tware *must* wait for no more than 10 ms, but this is not what the spec say= s. >>=20 >>=20 >>=20 >>> -----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 >>>=20 >>> The SATA spec mandates that link detection by the PHY completes=20 >>> within >>> 10 ms after receiving a reset signal. However, there is no=20 >>> obligation to uphold this requirement at the driver end as strictly=20 >>> as we do, and as it turns out, some combinations of host and device=20 >>> (e.g., Samsung >>> 850 EVO connected to a LeMaker Cello) are only borderline compliant, wh= ich means the device is not detected reliably. >>>=20 >>> So let's allow for a bit of margin, and increase the PHY detect timeout= value to 15 ms. >>>=20 >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> v2: update comment in AhciModeInitialization() as well >>>=20 >>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 5 +++--=20 >>> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.h | 3 ++- >>> 2 files changed, 5 insertions(+), 3 deletions(-) >>>=20 >>> 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); >>>=20 >>> // >>> - // Wait no longer than 10 ms to wait the Phy to detect the prese= nce 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 d= evice. >>> + // 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 >>> + EFI_AHCI_PORT_SSTS; diff --git >>> 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 { >>>=20 >>> // >>> // 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 >>>=20 >>> _______________________________________________ >>> 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