From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 245CC1A1E2D for ; Mon, 5 Sep 2016 06:06:11 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9BE1E4E4D0; Mon, 5 Sep 2016 13:06:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-76.phx2.redhat.com [10.3.116.76]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u85D68Ut010563; Mon, 5 Sep 2016 09:06:09 -0400 To: Ard Biesheuvel References: <1473067049-16252-1-git-send-email-ard.biesheuvel@linaro.org> <1473067049-16252-3-git-send-email-ard.biesheuvel@linaro.org> <6c4c3202-81ef-c97f-11be-37b714381a5d@redhat.com> Cc: "edk2-devel@lists.01.org" , "Tian, Feng" , "Zeng, Star" , "Gao, Liming" , Leif Lindholm From: Laszlo Ersek Message-ID: <44e51f48-931e-5dc1-f5ee-5fa051a07b1e@redhat.com> Date: Mon, 5 Sep 2016 15:06:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 05 Sep 2016 13:06:10 +0000 (UTC) Subject: Re: [PATCH 2/7] MdeModulePkg/EhciDxe: enable 64-bit PCI DMA 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: Mon, 05 Sep 2016 13:06:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 09/05/16 14:44, Ard Biesheuvel wrote: > On 5 September 2016 at 13:19, Laszlo Ersek wrote: >> On 09/05/16 11:17, Ard Biesheuvel wrote: >>> PCI controller drivers must set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE >>> attribute if the controller supports 64-bit DMA addressing. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 22 +++++++++++++++++++- >>> MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h | 2 ++ >>> MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 2 +- >>> 3 files changed, 24 insertions(+), 2 deletions(-) >>> >>> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c >>> index 4e9e05f0e431..e4c7e59526de 100644 >>> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c >>> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c >>> @@ -89,7 +89,7 @@ EhcGetCapability ( >>> >>> *MaxSpeed = EFI_USB_SPEED_HIGH; >>> *PortNumber = (UINT8) (Ehc->HcStructParams & HCSP_NPORTS); >>> - *Is64BitCapable = (UINT8) (Ehc->HcCapParams & HCCP_64BIT); >>> + *Is64BitCapable = (UINT8) Ehc->Support64BitDma; >>> >>> DEBUG ((EFI_D_INFO, "EhcGetCapability: %d ports, 64 bit %d\n", *PortNumber, *Is64BitCapable)); >>> >>> @@ -1877,6 +1877,26 @@ EhcDriverBindingStart ( >>> goto CLOSE_PCIIO; >>> } >>> >>> + // >>> + // Enable 64-bit DMA support in the PCI layer if this controller >>> + // supports it. >>> + // >>> + if ((Ehc->HcCapParams & HCCP_64BIT) != 0) { >> >> How about using the nice EHC_BIT_IS_SET macro here (inspired by the >> previous use in EhcInitSched(), at the bottom of this patch)? >> > > I just moved the test from the previous hunk here. I don't care either > way, so I will let the maintainers decide. Feng, Star? > >>> + Status = PciIo->Attributes ( >>> + PciIo, >>> + EfiPciIoAttributeOperationEnable, >>> + EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, >>> + NULL >>> + ); >>> + if (!EFI_ERROR (Status)) { >>> + Ehc->Support64BitDma = TRUE; >>> + } else { >>> + DEBUG ((EFI_D_WARN, >>> + "EhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable controller @ %p (%r)\n", >>> + Controller, Status)); >> >> Same comment as for 5/7, i.e. %a and __FUNCTION__. >> > > The surrounding code never uses that. I started out using %a and > __FUNCTION__, and then removed it again to align with the existing > code. > >> Again, whether you want to change these is up to you. >> >> Reviewed-by: Laszlo Ersek >> > > Thanks a lot! > >> (I won't try to review the rest of the patches.) >> > > No worries. I did build test all of them, but I currently have no way > of testing the NVM and SDHCI patches, so I am hoping they are > 'obviously correct' > You can test the NVMe change with QEMU (x86_64) + OVMF: https://tianocore.acgmultimedia.com/show_bug.cgi?id=79 Thanks Laszlo