Re: [PATCH v2] MdeModulePkg/AtaAtapiPassThru: Ensure GHC.AE bit is always set in Ahci

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

On 24 November 2016 at 08:01, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Ard,
>
> 2016-11-24 8:56 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
>> On 24 November 2016 at 07:54, Marcin Wojtas <mw@semihalf.com> wrote:
>>> According to AHCI Spec 1.3 GHC.AE bit description:
>>> "The implementation of this bit is dependent upon the value of the
>>> CAP.SAM bit. If CAP.SAM is '0', then GHC.AE shall be read-write and shall
>>> have a reset value of '0'. If CAP.SAM is '1', then AE shall be read-only
>>> and shall have a reset value of '1'."
>>>
>>> Being in AhciMode, for proper operation it is required, that GHC.AE bit
>>> is always set, before any other AHCI registers are written to. Current
>>> AhciMode implementation, both in AhciReset() and AhciModeInitialization()
>>> functions, set GHC.AE bit only depending on 'CAP.SAM == 0' condition,
>>> assuming (according to the AHCI spec), that otherwise it has to be set
>>> anyway. It may however happen, that even if 'CAP.SAM == 1', GHC.AE
>>> requires updating by software.
>>>
>>> This patch enables in AhciMode setting GHC.AE in case its initial value
>>> is '0'. It fixes AHCI support for Marvell Armada 70x0 and 80x0 SoC
>>> families. The change is transparent to all other platforms.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> Signed-off-by: Jan Dabros <jsd@semihalf.com>
>>>
>>> ---
>>> Changelog:
>>> v1 -> v2
>>>
>>> * Instead of doing it uncoditionally, enable setting GHC.AE bit only in
>>>   case its initial value is '0'
>>>
>>> ---
>>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 21 ++++++++++-----------
>>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>>> index 533d201..4d01c1d 100644
>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
>>> @@ -1451,17 +1451,13 @@ AhciReset (
>>>  {
>>>    UINT64                 Delay;
>>>    UINT32                 Value;
>>> -  UINT32                 Capability;
>>>
>>>    //
>>> -  // Collect AHCI controller information
>>> -  //
>>> -  Capability = AhciReadReg (PciIo, EFI_AHCI_CAPABILITY_OFFSET);
>>> -
>>> -  //
>>> -  // Enable AE before accessing any AHCI registers if Supports AHCI Mode Only is not set
>>> +  // Make sure that GHC.AE bit is set before accessing any AHCI registers.
>>>    //
>>> -  if ((Capability & EFI_AHCI_CAP_SAM) == 0) {
>>> +  Value = AhciReadReg(PciIo, EFI_AHCI_GHC_OFFSET);
>>> +
>>> +  if ((Value & EFI_AHCI_GHC_ENABLE) == 0) {
>>>      AhciOrReg (PciIo, EFI_AHCI_GHC_OFFSET, EFI_AHCI_GHC_ENABLE);
>>>    }
>>>
>>
>> If we are ignoring the capability bits now, do we still need to read them?
>
> In the context above (AhciReset() function) reading capability is
> removed. In AhciModeInitialization() however, it has to remain,
> because of following lines:
>
>   //
>   // Enable 64-bit DMA support in the PCI layer if this controller
>   // supports it.
>   //
>   if ((Capability & EFI_AHCI_CAP_S64A) != 0) {
>
> Which, I can see, were added by you:)
>

Ah, apologies, I misread the patch.

In that case

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>