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

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

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?


Thanks,
Star
-----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