From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::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 A85392095D213 for ; Wed, 28 Jun 2017 01:56:38 -0700 (PDT) Received: by mail-io0-x234.google.com with SMTP id h64so31840533iod.0 for ; Wed, 28 Jun 2017 01:58:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=QyraOy25bfMp3JWDrePoElP6vOUN3oWe74rQ65BlJYM=; b=KXliZIMSlPuafuNnK7RV5R4QhqXYt2tPXxxxuAJtp3/PK2nh4MP3MsadUch7kO8iAk YEVVhnbocDWHwDr68waRyURB0wdWEVVMqeeQBzKJsKMBTgvz6rCZ62Hfj7mrmHCCLK/R KbMYUnMLcNkF5mY9opCLyiqfm0lenRHuEqYzY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=QyraOy25bfMp3JWDrePoElP6vOUN3oWe74rQ65BlJYM=; b=SBKqIZXgwHChEUn8lgfCxY0GTEn+6uG59UBOKkpkM9eKLM9N+QbUANKvqUQYpz+v/f Fkt+LAus/ZvfXDFSxtpObQHnA67MnZhEQ+R4Z69nVN78/HwQIcabjH2oX2OKgFF6F2AQ c+6mpXKN8AAIQdNCaqVZe6dFSrniU7cjvH6XYqZrARUopVNFlfUg3N548Vc2pPphe652 MHi1YJ+HPPrPx+T/OD/pMjtZkyp5tWW93S5v3x3eDO77Jnp/dBqeBTHYInzLHAwMmJ9q 3Fdmo+h0F5W7nDKb0MvqiLiwKFrLyIgtmEO6wO8RqjU86necd48KAy5blWHMbZaasVpD 5jSA== X-Gm-Message-State: AKS2vOyyLp9zdzu8ikWq9v/115ejW+quQxQ1hLYcbibd6zZafRmhWQNI r5H1BR5+B+skAqM3d6esmt8n02O77Ed8 X-Received: by 10.107.180.20 with SMTP id d20mr10170702iof.47.1498640289421; Wed, 28 Jun 2017 01:58:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.134.134 with HTTP; Wed, 28 Jun 2017 01:58:08 -0700 (PDT) In-Reply-To: <7F1BAD85ADEA444D97065A60D2E97EE569A14A33@SHSMSX101.ccr.corp.intel.com> References: <20170628082323.30270-1-ard.biesheuvel@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF571@shsmsx102.ccr.corp.intel.com> <7F1BAD85ADEA444D97065A60D2E97EE569A14A33@SHSMSX101.ccr.corp.intel.com> From: Ard Biesheuvel Date: Wed, 28 Jun 2017 08:58:08 +0000 Message-ID: To: "Tian, Feng" Cc: "Zeng, Star" , "edk2-devel@lists.01.org" , "Dong, Eric" , "leif.lindholm@linaro.org" 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 08:56:38 -0000 Content-Type: text/plain; charset="UTF-8" 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 effect 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". That'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 describes the requirements for the hardware side, which are the opposite of those 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, Eric ; leif.lindholm@linaro.org > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax 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 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 10 ms or *more*, so 'at least 10ms'. The original comment said that the software *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 Of >> Ard Biesheuvel >> Sent: Wednesday, June 28, 2017 4:23 PM >> To: edk2-devel@lists.01.org; Zeng, Star >> Cc: Tian, Feng ; Dong, Eric >> ; leif.lindholm@linaro.org; Ard Biesheuvel >> >> Subject: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY >> detect timeout >> >> The SATA spec mandates that link detection by the PHY completes within >> 10 ms after receiving a reset signal. However, there is no obligation >> to uphold this requirement at the driver end as strictly as we do, and >> as it turns out, some combinations of host and device (e.g., Samsung >> 850 EVO connected to a LeMaker Cello) are only borderline compliant, which 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 +++-- >> 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 presence 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 device. >> + // It's the requirement from SATA1.0a spec section 5.2. >> + // Add a bit of margin for robustness. >> // >> PhyDetectDelay = EFI_AHCI_BUS_PHY_DETECT_TIMEOUT; >> Offset = 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 { >> >> // >> // Refer SATA1.0a spec section 5.2, the Phy detection time should be less 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