From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::234]) (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 B541681EEE for ; Thu, 24 Nov 2016 00:02:07 -0800 (PST) Received: by mail-io0-x234.google.com with SMTP id c21so62391614ioj.1 for ; Thu, 24 Nov 2016 00:02:07 -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=Vt/htBRj8QtIOMs9zpV2iGExdLlFQUbO6dAIq67O3M0=; b=QotT8YiIuUYs4htP2Fcu5zXMUGNwe5NiuOPSJ7eSP6KnkUiaXe7i48NJ2s+3yFNWoT MNn1EBf+qGaoDuo8sKmPqkoCZPzsE3R4OmTGpiCMriHYZY6+xEZdnUNZ++pFS6j8x68H +sXYXHwoIM+4PItPCxiGnDfQ/AhyVKF2C9OBY= 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=Vt/htBRj8QtIOMs9zpV2iGExdLlFQUbO6dAIq67O3M0=; b=gJki1uEF8UbpKjmk867JxhSoBmomU3VT890GHxgXaZ4xQNxaa69xN/L42UM8Y5wZac BwBOOlh97F/2eApoFUbq6qIECgKvZArivLLoclCuZF/MGGzQtPhl4VHgbynmqFWXLzgA 7QEcata/Y2p5bwBopU2AVQmYJcAJhswBG2/KV798l2NL3cuZUfjXI74rHa8a9YPjc1yJ zL5unee5MHRQZV2QNxGdUYMdmhlKLQb5EZXV4WKN1Q3poYEELux6rpKqkRYp2PFzSm6T 6plb+abJEomdafQgEFkothG1d5w3VCLOOMCm9au72n45TLdNKaFTBfoIb0ISnDUYNFDG VoWg== X-Gm-Message-State: AKaTC01zEays79ZjGfWvEymjC+5gVDE6UTbt3cUQatmJahgpQ7JwW6/pt+2qieZQRTeZ5E/XHrJmhxVzE9jR/Gw8 X-Received: by 10.36.111.212 with SMTP id x203mr904907itb.59.1479974526983; Thu, 24 Nov 2016 00:02:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.59.147 with HTTP; Thu, 24 Nov 2016 00:02:06 -0800 (PST) In-Reply-To: References: <1479974073-29154-1-git-send-email-mw@semihalf.com> From: Ard Biesheuvel Date: Thu, 24 Nov 2016 08:02:06 +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 08:02:07 -0000 Content-Type: text/plain; charset=UTF-8 On 24 November 2016 at 08:01, Marcin Wojtas wrote: > Hi Ard, > > 2016-11-24 8:56 GMT+01:00 Ard Biesheuvel : >> 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? > > 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