Hi, On Mon, Jul 08, 2013 at 09:55:15PM -0700, Sarah Sharp wrote: > Felipe, Andy, and Seb, I have a couple questions below. > > On Fri, Jul 05, 2013 at 08:24:56PM +0300, Xenia Ragiadakou wrote: > > The function dma_set_mask() tests internally whether the dma_mask pointer > > for the device is initialized and fails if the dma_mask pointer is NULL. > > On pci platforms, the initialization of the device dma_mask pointer is > > performed when pci devices are enumerated and is set to point to the > > pci_dev->dma_mask which is 0xffffffff. However, for non-pci platforms, > > the dma_mask pointer remains uninitialized and dma_set_mask() will fail. > > > > Also, a call to dma_set_mask() does not set the device coherent_dma_mask. > > Since the xhci-hcd driver calls dma_alloc_coherent() and dma_pool_alloc() > > to allocate consistent DMA memory blocks, the coherent DMA address mask > > has to be set explicitly, otherwise the DMA mapping interface will return > > by default DMA addresses which are 32-bit addressable. > > > > This patch removes the dma_mask setup code from xhci_gen_setup() and places > > it in xhci_pci_setup() and xhci_plat_setup(). The dma_mask setup must be > > done before the call to xhci_gen_setup() which allocates and maps to dma > > addresses the xhci-hcd buffers. > > > > For pci platforms, the dma_mask and coherent_dma_mask are set by default > > to 32bit DMA addresses. If both the xHC controller and the host support > > 64bit DMA addresses, the device dma_mask and coherent_dma_mask will be > > set to 64bits. > > Xenia, I'm not sure what you mean by "the xHC controller and the host support > 64 bit DMA addresses". The xHC controller is the xHCI host. Did you maybe > mean "If both the xHCI host and the system support 64-bit DMA"? > > > For non-pci platforms, the dma_mask pointer is initialized to point to > > the coherent_dma_mask since the same value will be assigned to both. > > Felipe, Andy, is there any chance that a platform_device dma_mask > pointer would already be initialized by the time the probe function is > called? We wouldn't want to overwrite it. Can you please check the > xhci_plat_probe code?
yes there is. At least if you're booting with Devicetree, OF core sets
*all* dma_masks to 32-bits (erroneously IMO):
$ git grep -e dma_mask drivers/of/
drivers/of/platform.c: dev->dev.dma_mask = &dev->archdata.dma_mask;
drivers/of/platform.c: dev->archdata.dma_mask = 0xffffffffUL;
drivers/of/platform.c: dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
drivers/of/platform.c: dev->dev.coherent_dma_mask = ~0;
drivers/of/platform.c: dev->dma_mask = ~0;
> I'm also a bit confused as to why the platform device code could work at
> all in the current state. Xenia's patch sets usb_hcd->self.uses_dma.
> The xHCI platform code currently doesn't set this flag. The xHCI driver
> also doesn't set the HCD_LOCAL_MEM flag. So what the heck happens with
> a platform device without either of those flags set in this code:
>
> int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> gfp_t mem_flags)
> {
> enum dma_data_direction dir;
> int ret = 0;
>
> /* Map the URB's buffers for DMA access.
> * Lower level HCD code should use *_dma exclusively,
> * unless it uses pio or talks to another transport,
> * or uses the provided scatter gather list for bulk.
> */
>
> if (usb_endpoint_xfer_control(&urb->ep->desc)) {
> if (hcd->self.uses_pio_for_control)
> return ret;
> if (hcd->self.uses_dma) {
> urb->setup_dma = dma_map_single(
> hcd->self.controller,
> urb->setup_packet,
> sizeof(struct usb_ctrlrequest),
> DMA_TO_DEVICE);
> if (dma_mapping_error(hcd->self.controller,
> urb->setup_dma))
> return -EAGAIN;
> urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
> } else if (hcd->driver->flags & HCD_LOCAL_MEM) {
> ret = hcd_alloc_coherent(
> urb->dev->bus, mem_flags,
> &urb->setup_dma,
> (void **)&urb->setup_packet,
> sizeof(struct usb_ctrlrequest),
> DMA_TO_DEVICE);
> if (ret)
> return ret;
> urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
> }
> }
>
> As far as I can tell, that means the setup packet for control transfers
> doesn't actually get mapped for DMA currently. With Xenia's patch it
> will.
That's a very good finding and I don't know how come we never triggered
it. I am sure we have OMAP5 working with that :-s
> Does that mean xHCI platform devices have just been broken all this
> time? Has this code been tested at all?
sure, we have OMAP5430, OMAP5432, DRA7642 and two pre-silicon platforms
working :-s
--
balbi
signature.asc
Description: Digital signature
