From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x231.google.com (mail-it0-x231.google.com [IPv6:2607:f8b0:4001:c0b::231]) (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 ADA0621CB57D9 for ; Wed, 28 Jun 2017 03:43:13 -0700 (PDT) Received: by mail-it0-x231.google.com with SMTP id b205so30972325itg.1 for ; Wed, 28 Jun 2017 03:44:45 -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=lBSUsw7v/xciTpzJua3yohLZS1RxucbUJmo66p0DbW0=; b=cEd/bpsYY422X9Bwat/tgpWURtRYJu9MDFmtpbTfXaeEt+XQnjjuQ7ts/rQmh4PaQF qr2gaKURjowIK1n35sn/6QQMAKbbGtWxVqjwUMw5h5auEJc+KQVZHkjBuICkTE5yCEeM TkDYeEn7qZI/CNE422OYIw44xXwdOQLMip8aM= 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=lBSUsw7v/xciTpzJua3yohLZS1RxucbUJmo66p0DbW0=; b=DYWC0CRqGXL/PCHuX7zQ21HbRy+itH6NGV4scg3kfn27XbwL9geQ43D1GgJWfGKF1d ta2T9mgBibB9HV5Ed1nPhdwVJqTYT5VhF3FZX5DFBhoxXaDDt/Wd75PIhHjf0K+p2vle cS3KyANwX4/SSa3gE3CMgURdLUjBZPisTZd+iACJgCjkobrZfhpJpQoFQuQtXTURGGTe E+2S3TB9agSznouXTzPql/uzGgjqVv8bgkoIFNZqc7IkH5HV+a+c87+t9Gl1oWf7nK1h yFv59Hj7rGmi1xRigT0xm3fKAh8Ps8H5cuJP7ISiCtbF9EjoMcDkwjBFbKmfx8tjyxee ADSQ== X-Gm-Message-State: AKS2vOzDotbzrsO2h4u/D5IMW1nZY7c3UdqHxRQcl6iQi2cU9RtZfev+ ojuKfakBw1RS8/gI6hhoCT69iGCFD4S9 X-Received: by 10.36.4.4 with SMTP id 4mr7825964itb.73.1498646684523; Wed, 28 Jun 2017 03:44:44 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.134.134 with HTTP; Wed, 28 Jun 2017 03:44:44 -0700 (PDT) In-Reply-To: <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> <0C09AFA07DD0434D9E2A0C6AEB0483103B8EF6E4@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Wed, 28 Jun 2017 10:44:44 +0000 Message-ID: To: "Zeng, Star" Cc: "Tian, Feng" , "leif.lindholm@linaro.org" , "edk2-devel@lists.01.org" , "Dong, Eric" 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:43:14 -0000 Content-Type: text/plain; charset="UTF-8" On 28 June 2017 at 10:44, Zeng, Star wrote: > The delay duration may be 0 - EFI_AHCI_BUS_PHY_DETECT_TIMEOUT ms, and the comments 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. > // > OK, I will change it. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > 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 detect timeout > > >> On 28 Jun 2017, at 09:53, Zeng, Star wrote: >> >> The real delay may be 0ms if the DET register value just pass the check after the first AhciReadReg(). >> But what does "Wait at least 10 ms" mean? >> > > 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 to report presence of a link. So we can never report 'no link' in less than 10 ms. > > Or am I missing something here? > > >> >> -----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 detect 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 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 >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel