| Message ID | 1473067049-16252-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 167c3fb45674c274219b3af0bc811992cd4d6116 |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > 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)? > + 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__. Again, whether you want to change these is up to you. Reviewed-by: Laszlo Ersek <lersek@redhat.com> (I won't try to review the rest of the patches.) Thanks Laszlo > + } > + } > + > Status = gBS->InstallProtocolInterface ( > &Controller, > &gEfiUsb2HcProtocolGuid, > diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h > index 7177658092c3..be81bde40d9b 100644 > --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h > +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h > @@ -173,6 +173,8 @@ struct _USB2_HC_DEV { > UINT16 DebugPortOffset; // The offset of debug port mmio register > UINT8 DebugPortBarNum; // The bar number of debug port mmio register > UINT8 DebugPortNum; // The port number of usb debug port > + > + BOOLEAN Support64BitDma; // Whether 64 bit DMA may be used with this device > }; > > > diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c > index 5594e6699ea6..036c00b32b40 100644 > --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c > +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c > @@ -178,7 +178,7 @@ EhcInitSched ( > // > Ehc->MemPool = UsbHcInitMemPool ( > PciIo, > - EHC_BIT_IS_SET (Ehc->HcCapParams, HCCP_64BIT), > + Ehc->Support64BitDma, > EHC_HIGH_32BIT (PhyAddr) > ); > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 5 September 2016 at 13:19, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org> >> --- >> 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 <lersek@redhat.com> > 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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/05/16 14:44, Ard Biesheuvel wrote: > On 5 September 2016 at 13:19, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org> >>> --- >>> 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 <lersek@redhat.com> >> > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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) { + 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)); + } + } + Status = gBS->InstallProtocolInterface ( &Controller, &gEfiUsb2HcProtocolGuid, diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h index 7177658092c3..be81bde40d9b 100644 --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.h @@ -173,6 +173,8 @@ struct _USB2_HC_DEV { UINT16 DebugPortOffset; // The offset of debug port mmio register UINT8 DebugPortBarNum; // The bar number of debug port mmio register UINT8 DebugPortNum; // The port number of usb debug port + + BOOLEAN Support64BitDma; // Whether 64 bit DMA may be used with this device }; diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c index 5594e6699ea6..036c00b32b40 100644 --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c @@ -178,7 +178,7 @@ EhcInitSched ( // Ehc->MemPool = UsbHcInitMemPool ( PciIo, - EHC_BIT_IS_SET (Ehc->HcCapParams, HCCP_64BIT), + Ehc->Support64BitDma, EHC_HIGH_32BIT (PhyAddr) );
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 <ard.biesheuvel@linaro.org> --- 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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel