From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x234.google.com (mail-wr0-x234.google.com [IPv6:2a00:1450:400c:c0c::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D5EA21A00AD1 for ; Wed, 28 Jun 2017 02:56:49 -0700 (PDT) Received: by mail-wr0-x234.google.com with SMTP id k67so171101773wrc.2 for ; Wed, 28 Jun 2017 02:58:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=xDo59N5toyUubtuOg8dnw2P98F7BLNjdtjrD6dFlJqM=; b=EtR4wrA6lTv1n13CGgyC1T9rXJw72RjkM0grwW9K+cc6kSyZxpb05RyJ22dWRE6mh0 hNMLXMibOpplF9KLS6i9EhoFkF8XeT+vraALXLLQXXN3FaZbcO5UdLI8mBMx2zwqIoJv z2rsd5H7F+5j1hOsFbK6/DJtsbCMC25AM+MjA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=xDo59N5toyUubtuOg8dnw2P98F7BLNjdtjrD6dFlJqM=; b=Rx1rPqfesv/nPCqMvqFEFb+NRsYfyTITROsX463gP+ClNsfl0GHUokhlVstjlOHRpZ BLu29ZFCYdIFuPUCjoG5XekJ2Onlgey/WWW4QDHJNDsEBvHn6yTCCLLW1lK1it0pqHcJ A55eiO3hi6wR+REKQgZLBfoTkC21gtTZsaUhHX/DUi2iALgpKMftajh486XdpLzxjOu0 XKIJrYYYJzyln+nEb+pFaRYfUBXtEMNIOZ3YiCStQO8ynHM1Vtq8QeSEyu4o/ciMz0V8 2AAfgMZOIli6Zs2/JSXIhW7wSPDmoSbxEahz+jLA1LMHN/GbPMsXAObN1ImkJvIxvn5C 5FMw== X-Gm-Message-State: AKS2vOzGVfV5oA/a2Z5j4Xno/tWF+vdV93C9rcLUfS6ZK2ZsXKJTft/l /DWGPojCT0YFmSeIXvw3+w== X-Received: by 10.223.143.10 with SMTP id p10mr18142175wrb.120.1498643898835; Wed, 28 Jun 2017 02:58:18 -0700 (PDT) Received: from [197.130.58.15] ([197.130.58.15]) by smtp.gmail.com with ESMTPSA id z99sm2017952wrc.12.2017.06.28.02.58.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Jun 2017 02:58:18 -0700 (PDT) Mime-Version: 1.0 (1.0) From: Ard Biesheuvel X-Mailer: iPhone Mail (14F89) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF68E@shsmsx102.ccr.corp.intel.com> Date: Wed, 28 Jun 2017 09:58:10 +0000 Cc: "Tian, Feng" , "leif.lindholm@linaro.org" , "edk2-devel@lists.01.org" , "Dong, Eric" Message-Id: 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> To: "Zeng, Star" 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:56:49 -0000 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > 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 af= ter 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 will= take at least 10 ms to proceed, given that the phy may legally take 10 ms t= o report presence of a link. So we can never report 'no link' in less than 1= 0 ms. Or am I missing something here? >=20 > -----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 de= tect 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 eff= ect for normal case. >>=20 >> 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". That= '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 des= cribes the requirements for the hardware side, which are the opposite of tho= se 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 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 softw= are *must* wait for no more than 10 ms, but this is not what the spec says. >>=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 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. >>>=20 >>> So let's allow for a bit of margin, and increase the PHY detect timeout v= alue 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 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 { >>>=20 >>> // >>> // Refer SATA1.0a spec section 5.2, the Phy detection time should be les= s 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