Re: [PATCH 5/7] MdeModulePkg/XhciDxe: enable 64-bit PCI DMA

[Laszlo Ersek <lersek@redhat.com>]

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/XhciDxe/Xhci.c | 22 +++++++++++++++++++-
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h |  2 ++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 4798bea86061..cdff1c3b8849 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -125,7 +125,7 @@ XhcGetCapability (
>    Xhc             = XHC_FROM_THIS (This);
>    *MaxSpeed       = EFI_USB_SPEED_SUPER;
>    *PortNumber     = (UINT8) (Xhc->HcSParams1.Data.MaxPorts);
> -  *Is64BitCapable = (UINT8) (Xhc->HcCParams.Data.Ac64);
> +  *Is64BitCapable = (UINT8) Xhc->Support64BitDma;
>    DEBUG ((EFI_D_INFO, "XhcGetCapability: %d ports, 64 bit %d\n", *PortNumber, *Is64BitCapable));
>  
>    gBS->RestoreTPL (OldTpl);
> @@ -2020,6 +2020,26 @@ XhcDriverBindingStart (
>      return EFI_OUT_OF_RESOURCES;
>    }
>  
> +  //
> +  // Enable 64-bit DMA support in the PCI layer if this controller
> +  // supports it.
> +  //
> +  if ((Xhc->HcCParams.Data.Ac64) != 0) {

I think the inner parens are superfluous.

> +    Status = PciIo->Attributes (
> +                      PciIo,
> +                      EfiPciIoAttributeOperationEnable,
> +                      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> +                      NULL
> +                      );
> +    if (!EFI_ERROR (Status)) {
> +      Xhc->Support64BitDma = TRUE;
> +    } else {
> +      DEBUG ((EFI_D_WARN,
> +        "XhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable controller @ %p (%r)\n",

I prefer to use "%a" with __FUNCTION__, rather than open-coding the
containing function's name; for brevity and for robustness against code
movement.

I'll leave it to you if you want to change these things.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +        Controller, Status));
> +    }
> +  }
> +
>    XhcSetBiosOwnership (Xhc);
>  
>    XhcResetHC (Xhc, XHC_RESET_TIMEOUT);
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> index 7999151b3fde..0f53bb0eff7c 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.h
> @@ -256,6 +256,8 @@ struct _USB_XHCI_INSTANCE {
>    // The array supports up to 255 devices, entry 0 is reserved and should not be used.
>    //
>    USB_DEV_CONTEXT           UsbDevContext[256];
> +
> +  BOOLEAN                   Support64BitDma; // Whether 64 bit DMA may be used with this device
>  };
>  
>  
>