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
Attachment:
signature.asc
Description: Digital signature
