From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x235.google.com (mail-it0-x235.google.com [IPv6:2607:f8b0:4001:c0b::235]) (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 637521A1E2D for ; Mon, 5 Sep 2016 05:44:10 -0700 (PDT) Received: by mail-it0-x235.google.com with SMTP id i184so144537256itf.1 for ; Mon, 05 Sep 2016 05:44:10 -0700 (PDT) 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=2qbqBW1ebo6I0HerhmPbyavn8ptb9J+GdZturN05HuE=; b=R/ZwvQvsKZTaBZX/34RuQh3yTKcxFlVOCpHnbSDBYI1ka1iqIINq3mTICE3CUKkRFr Dh1KbrnFCax9kmyQa6bHmpkllt02Tfn/p01jqaDR+UmtTILbWlFa0tdvbXkDCBywbAsb ybj7uQdS2wKuGcfYQgwF61zk3leihiFT5KAzs= 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=2qbqBW1ebo6I0HerhmPbyavn8ptb9J+GdZturN05HuE=; b=izluX4NKqJEvgKrvCsjL7JmdlN3z1RKXSqrUlhUVTCV2GIo04XRhISVE18apVmbp8H tO9gtBlbg7b24+B0u3XwR7ZW8sE3Rg+9MKUyX8vd8SOaQUCK3BrWoyfHdLT2+DAesbco mRWOdSgPdnbQY1kvuvpYXU7awzw2T8Dqdpk3cNnqX4H+JfrBMcjiN/F38w3b5fJxq/uQ z4tCTZqzNDWLIsLJ6RSvDPL9DkXAS8szv2C05OWaZlojuk5rshoNxyvaAcCxhpEIomKz /1ecIJMmk0txGJPkot1PaftNvTY0Y23CRDlxYJO5qWYsFDCGlsDe/SnNsfMsbl35dQaQ HGJQ== X-Gm-Message-State: AE9vXwMLrrHVHh8jg7f4hsEfBK1rkNgSPpPzOBD4alYI+GrHHF9Yn1Ur2rQ4KqXGKo1V3zRJRou4zcoptqE7Mol4 X-Received: by 10.36.25.144 with SMTP id b138mr22162709itb.29.1473079449660; Mon, 05 Sep 2016 05:44:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Mon, 5 Sep 2016 05:44:09 -0700 (PDT) In-Reply-To: <6c4c3202-81ef-c97f-11be-37b714381a5d@redhat.com> 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> From: Ard Biesheuvel Date: Mon, 5 Sep 2016 13:44:09 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , "Tian, Feng" , "Zeng, Star" , "Gao, Liming" , Leif Lindholm 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 12:44:10 -0000 Content-Type: text/plain; charset=UTF-8 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' -- Ard.