From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x231.google.com (mail-io0-x231.google.com [IPv6:2607:f8b0:4001:c06::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F0FFC81EDC for ; Wed, 23 Nov 2016 23:56:22 -0800 (PST) Received: by mail-io0-x231.google.com with SMTP id x94so66577157ioi.3 for ; Wed, 23 Nov 2016 23:56:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=uw6ggEavdpfmfKPWLTdVdzGRMRiH97EtCLdncmuk06w=; b=hFlm6C3vvJIjdSMzkZRYUu9djU+A5GxKp+v9zjSIhaCyi6Ej4ewTpXk0PTZa8y/aXJ BM9eRKuiLLEzqDi/iTSGBemE5Cm8VlMVARfb14eNYNbXj1fRJva+I8HOiOyvRjJ3Wu+g /WT5wvt+zWN0hqvH4D1XvTPYADOsGkVIIx7dk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uw6ggEavdpfmfKPWLTdVdzGRMRiH97EtCLdncmuk06w=; b=AfVUd11zKjn7UL4Ms3Y2BCdbliboVlrSdHEASN/s4Ij8veEcYbix6ehL3G3YcFIGYN d473YJse/1gskHcpUHBKTl75DXXob74Q4RHZ0UYlXlgyGM/oRoO2UIdo37ewU7PZ59mD RRjtsdj1c4v4jWfDNjfrj+N1TNJM5RnWr2fEfDEKvboQEoV9y6EkMfV9lr5BztaYasdA kIOeA5BOuk+xdMfNINU11U7drwWkw91rgKbgRtDkmELvrtzZU+/Rp12xJHUptoik02ZF sbE9hSdDNL6W0rmUNhTKGOqG1jmoII3wfk4e90gBm4gzNZ+g5icruRtfip/MPjuIhHrT 5mbw== X-Gm-Message-State: AKaTC0299Xb0I/7kT6PCNUnx0KHlPRbDSR0AV2lzWDOFgh2iFecEQeHRuUknw4vGH594cGgufFFSF/87w8g1VWDS X-Received: by 10.36.178.74 with SMTP id h10mr826667iti.37.1479974181964; Wed, 23 Nov 2016 23:56:21 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Wed, 23 Nov 2016 23:56:21 -0800 (PST) In-Reply-To: <1479974073-29154-1-git-send-email-mw@semihalf.com> References: <1479974073-29154-1-git-send-email-mw@semihalf.com> From: Ard Biesheuvel Date: Thu, 24 Nov 2016 07:56:21 +0000 Message-ID: To: Marcin Wojtas Cc: edk2-devel-01 , "Tian, Feng" , "Kinney, Michael D" , "Gao, Liming" , Leif Lindholm , =?UTF-8?B?SmFuIETEhWJyb8Wb?= Subject: Re: [PATCH v2] MdeModulePkg/AtaAtapiPassThru: Ensure GHC.AE bit is always set in Ahci X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Nov 2016 07:56:23 -0000 Content-Type: text/plain; charset=UTF-8 On 24 November 2016 at 07:54, Marcin Wojtas 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 > Signed-off-by: Jan Dabros > > --- > 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 >