Re: [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY detect timeout

["Zeng, Star" <star.zeng@intel.com>]

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.
      //

Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Wednesday, June 28, 2017 5:58 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax PHY detect timeout


> On 28 Jun 2017, at 09:53, Zeng, Star <star.zeng@intel.com> 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 <feng.tian@intel.com>
> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org; Dong, Eric 
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax 
> PHY detect timeout
> 
>> On 28 June 2017 at 08:49, Tian, Feng <feng.tian@intel.com> 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 <star.zeng@intel.com>
>> Cc: edk2-devel@lists.01.org; Tian, Feng <feng.tian@intel.com>; Dong, 
>> Eric <eric.dong@intel.com>; leif.lindholm@linaro.org
>> Subject: Re: [edk2] [PATCH v2] MdeModulePkg/AtaAtapiPassThru: relax 
>> PHY detect timeout
>> 
>>> On 28 June 2017 at 08:31, Zeng, Star <star.zeng@intel.com> 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 <star.zeng@intel.com>
>>> Cc: Tian, Feng <feng.tian@intel.com>; Dong, Eric 
>>> <eric.dong@intel.com>; leif.lindholm@linaro.org; Ard Biesheuvel 
>>> <ard.biesheuvel@linaro.org>
>>> 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 <ard.biesheuvel@linaro.org>
>>> ---
>>> 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