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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 June 2017 at 10:44, Zeng, Star <star.zeng@intel.com> 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 <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