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 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?

> @@ -2252,6 +2248,7 @@ AhciModeInitialization (
>    EFI_ATA_COLLECTIVE_MODE          *SupportedModes;
>    EFI_ATA_TRANSFER_MODE            TransferMode;
>    UINT32                           PhyDetectDelay;
> +  UINT32                           Value;
>
>    if (Instance == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -2270,11 +2267,13 @@ AhciModeInitialization (
>    // 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);
>    }
>
> --
> 1.8.3.1
>