Re: [RFC v2] xhci: fix dma mask setup in xhci.c
On Fri, Jun 07, 2013 at 11:37:53PM +0300, Felipe Balbi wrote: Hi, On Fri, Jun 07, 2013 at 10:19:23AM -0700, Sarah Sharp wrote: Adding Felipe. Xenia, Felipe is the USB gadget maintainer, and also added code for non-PCI xHCI hosts. Felipe, Xenia is my summer intern through the FOSS Outreach Program for Women: http://kernelnewbies.org/OPWIntro Hey, cool Sarah :-) Welcome Xenia :-) Ah, slight correction, my intern wants to be called Ksenia. Xenia is her legal name, but everyone calls her Ksenia. :) On Fri, Jun 07, 2013 at 03:00:47PM +0300, Mathias Nyman wrote: Andy explained his concern in more detail to me irl. The beef is that the dma_mask is a pointer while dma_coherent_mask is a u64 value in struct device, and dma_set_mask() fails if dev-dma_mask doesn't point anywhere. This is not a problem for PCI enumerated devices because PCI probe sets a default 32bit mask and makes the dma_mask of all new PCI devices point to this value. For non-pci enumerated devices the device dma_mask may be NULL and dma_set_mask() may fail even if dma is supported on the host machine. for example ehci-platform.c solves this by adding: if (!dev-dev.dma_mask) dev-dev.dma_mask = dev-dev.coherent_dma_mask; if (!dev-dev.coherent_dma_mask) dev-dev.coherent_dma_mask = DMA_BIT_MASK(32); So it sounds like xhci_plat_setup() in xhci-plat.c needs to do something similar before it calls xhci_gen_setup(). Felipe, do you agree? (now, after reading the whole thing...) I'll agree with that :) It's funny that the code worked before. Perhaps setting the dma_mask pointer should be a separate patch for stable? sounds like a good plan to me :-) Great! Ok, Ksenia, can you create those two patches? The first patch in the series should add setting the dma_mask pointer to xhci_plat_setup(). The second patch will be the same as this patch. Let me know via IRC if you have any questions. Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] xhci: fix dma mask setup in xhci.c
On 06/07/2013 01:54 AM, Sarah Sharp wrote: On Thu, Jun 06, 2013 at 09:37:51PM +0300, Andy Shevchenko wrote: On Thu, Jun 6, 2013 at 5:21 PM, Xenia Ragiadakouburzalod...@gmail.com wrote: This patch adds a check on whether the host machine supports the xHC DMA address mask and sets the DMA mask for coherent DMA address allocation via an explicit call to dma_set_coherent_mask(). According to DMA-API-HOWTO, if coherent DMA address mask has not been set explicitly via dma_set_coherent_mask(), and the driver calls dma_alloc_coherent() or dma_pool_create() to allocate consistent DMA memory blocks, the consistent DMA mapping interface will return by default DMA addresses which are 32-bit addressable. Hence, if 64-bit DMA mapping is supported, it is appropriate to call dma_set_coherent_mask() with DMA_BIT_MASK(64) to take advantage of it. Also, according to DMA-API-HOWTO, dma_set_coherent_mask() is guaranteed to set successfully the same or a smaller mask as dma_set_mask(). It looks for me overcomplicated. Why? We have *dma_mask and dma_coherent mask in the struct device. First question, who is allocating memory for dma_mask? The xHCI driver is allocating memory for the host hardware structures, and the USB core is using the DMA mask of the host controller in order to move USB buffers into bounce buffers as necessary. The xHCI driver allocates memory from both DMA pools and with kmalloc. We need both calls to dma_set_coherent_mask() and dma_set_coherent_mask() to get 64-bit DMA addresses for both types of memory. Second, in case of dma_mask == NULL, dma_set_mask fails with -EIO. It doesn't mean we have no support of this one. How do you handle this case? When would the dma_mask be NULL? Note that xHCI PCI hosts *must* be able to support DMA. I think it's pretty simple to set dma_coherent_mask and then apply its address to the dma_mask, because you set the same values anyway. I'm not quite understanding what you want to do. (I'll blame lack of sleep, sorry.) Can you write some pseudo code for me? Andy explained his concern in more detail to me irl. The beef is that the dma_mask is a pointer while dma_coherent_mask is a u64 value in struct device, and dma_set_mask() fails if dev-dma_mask doesn't point anywhere. This is not a problem for PCI enumerated devices because PCI probe sets a default 32bit mask and makes the dma_mask of all new PCI devices point to this value. For non-pci enumerated devices the device dma_mask may be NULL and dma_set_mask() may fail even if dma is supported on the host machine. for example ehci-platform.c solves this by adding: if (!dev-dev.dma_mask) dev-dev.dma_mask = dev-dev.coherent_dma_mask; if (!dev-dev.coherent_dma_mask) dev-dev.coherent_dma_mask = DMA_BIT_MASK(32); -Mathias -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] xhci: fix dma mask setup in xhci.c
Adding Felipe. Xenia, Felipe is the USB gadget maintainer, and also added code for non-PCI xHCI hosts. Felipe, Xenia is my summer intern through the FOSS Outreach Program for Women: http://kernelnewbies.org/OPWIntro Felipe, we're discussing the fact that the xHCI host currently does not set up 64-bit coherent DMA, so we get 64-bit addresses for USB buffers, but not xHCI data structures. Xenia has a patch to fix it, which can be found here: http://marc.info/?l=linux-usbm=137052855612729w=2 Andy thinks the patch has issues with non-PCI xHCI hosts. On Fri, Jun 07, 2013 at 03:00:47PM +0300, Mathias Nyman wrote: Andy explained his concern in more detail to me irl. The beef is that the dma_mask is a pointer while dma_coherent_mask is a u64 value in struct device, and dma_set_mask() fails if dev-dma_mask doesn't point anywhere. This is not a problem for PCI enumerated devices because PCI probe sets a default 32bit mask and makes the dma_mask of all new PCI devices point to this value. For non-pci enumerated devices the device dma_mask may be NULL and dma_set_mask() may fail even if dma is supported on the host machine. for example ehci-platform.c solves this by adding: if (!dev-dev.dma_mask) dev-dev.dma_mask = dev-dev.coherent_dma_mask; if (!dev-dev.coherent_dma_mask) dev-dev.coherent_dma_mask = DMA_BIT_MASK(32); So it sounds like xhci_plat_setup() in xhci-plat.c needs to do something similar before it calls xhci_gen_setup(). Felipe, do you agree? It's funny that the code worked before. Perhaps setting the dma_mask pointer should be a separate patch for stable? Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] xhci: fix dma mask setup in xhci.c
Hi, On Fri, Jun 07, 2013 at 10:19:23AM -0700, Sarah Sharp wrote: Adding Felipe. Xenia, Felipe is the USB gadget maintainer, and also added code for non-PCI xHCI hosts. Felipe, Xenia is my summer intern through the FOSS Outreach Program for Women: http://kernelnewbies.org/OPWIntro Hey, cool Sarah :-) Welcome Xenia :-) Felipe, we're discussing the fact that the xHCI host currently does not set up 64-bit coherent DMA, so we get 64-bit addresses for USB buffers, but not xHCI data structures. Xenia has a patch to fix it, which can be found here: http://marc.info/?l=linux-usbm=137052855612729w=2 Andy thinks the patch has issues with non-PCI xHCI hosts. Looking at the patch, I don't see why it would have any issues, unless some HW engineer flipped the wrong bits in hcc_params, but that would be a silicon bug anyway. I don't have access to anything to test right now (on vacations, last day :-( ) but I'll see if anyone from TI can help testing that patch. Maybe Kishon can help us out here ? Kishon, could you spend a few minutes testing Xenia's patch on OMAP5 to make sure nothing goes wrong with that platform. Again, I can't see how it would fail. On Fri, Jun 07, 2013 at 03:00:47PM +0300, Mathias Nyman wrote: Andy explained his concern in more detail to me irl. The beef is that the dma_mask is a pointer while dma_coherent_mask is a u64 value in struct device, and dma_set_mask() fails if dev-dma_mask doesn't point anywhere. This is not a problem for PCI enumerated devices because PCI probe sets a default 32bit mask and makes the dma_mask of all new PCI devices point to this value. For non-pci enumerated devices the device dma_mask may be NULL and dma_set_mask() may fail even if dma is supported on the host machine. for example ehci-platform.c solves this by adding: if (!dev-dev.dma_mask) dev-dev.dma_mask = dev-dev.coherent_dma_mask; if (!dev-dev.coherent_dma_mask) dev-dev.coherent_dma_mask = DMA_BIT_MASK(32); So it sounds like xhci_plat_setup() in xhci-plat.c needs to do something similar before it calls xhci_gen_setup(). Felipe, do you agree? (now, after reading the whole thing...) I'll agree with that :) It's funny that the code worked before. Perhaps setting the dma_mask pointer should be a separate patch for stable? sounds like a good plan to me :-) -- balbi signature.asc Description: Digital signature
[RFC v2] xhci: fix dma mask setup in xhci.c
This patch adds a check on whether the host machine
supports the xHC DMA address mask and sets the DMA
mask for coherent DMA address allocation via an
explicit call to dma_set_coherent_mask().
According to DMA-API-HOWTO, if coherent DMA address
mask has not been set explicitly via dma_set_coherent_mask(),
and the driver calls dma_alloc_coherent() or
dma_pool_create() to allocate consistent DMA memory
blocks, the consistent DMA mapping interface will
return by default DMA addresses which are 32-bit
addressable.
Hence, if 64-bit DMA mapping is supported, it
is appropriate to call dma_set_coherent_mask()
with DMA_BIT_MASK(64) to take advantage of it.
Also, according to DMA-API-HOWTO, dma_set_coherent_mask()
is guaranteed to set successfully the same or a smaller
mask as dma_set_mask().
Signed-off-by: Xenia Ragiadakou burzalod...@gmail.com
---
Changes from v1:
fix of the following checkpatch warning, triggered in v1
WARNING: line over 80 characters
drivers/usb/host/xhci.c | 33 +++--
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index b4aa79d..2167b98 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4662,11 +4662,22 @@ int xhci_gen_setup(struct usb_hcd *hcd,
xhci_get_quirks_t get_quirks)
*/
xhci = hcd_to_xhci(hcd);
temp = xhci_readl(xhci, xhci-cap_regs-hcc_params);
- if (HCC_64BIT_ADDR(temp)) {
+ /*
+* Check if host machine supports 64 bit DMA address mask
+* and enable it for both streaming and coherent DMA transfers.
+* Otherwise, use 32bit DMA mask, if it is supported.
+*/
+ if (HCC_64BIT_ADDR(temp)
+ !dma_set_mask(hcd-self.controller, DMA_BIT_MASK(64))) {
xhci_dbg(xhci, Enabling 64-bit DMA addresses.\n);
- dma_set_mask(hcd-self.controller, DMA_BIT_MASK(64));
+ dma_set_coherent_mask(hcd-self.controller,
+ DMA_BIT_MASK(64));
} else {
- dma_set_mask(hcd-self.controller, DMA_BIT_MASK(32));
+ if (dma_set_mask(hcd-self.controller,
+DMA_BIT_MASK(32)))
+ goto error;
+ dma_set_coherent_mask(hcd-self.controller,
+ DMA_BIT_MASK(32));
}
return 0;
}
@@ -4700,11 +4711,21 @@ int xhci_gen_setup(struct usb_hcd *hcd,
xhci_get_quirks_t get_quirks)
xhci_dbg(xhci, Reset complete\n);
temp = xhci_readl(xhci, xhci-cap_regs-hcc_params);
- if (HCC_64BIT_ADDR(temp)) {
+ /*
+* Check if host machine supports 64 bit DMA address mask
+* and enable it for both streaming and coherent DMA transfers.
+* Otherwise, use 32bit DMA mask, if it is supported.
+*/
+ if (HCC_64BIT_ADDR(temp)
+ !dma_set_mask(hcd-self.controller, DMA_BIT_MASK(64))) {
xhci_dbg(xhci, Enabling 64-bit DMA addresses.\n);
- dma_set_mask(hcd-self.controller, DMA_BIT_MASK(64));
+ dma_set_coherent_mask(hcd-self.controller,
+ DMA_BIT_MASK(64));
} else {
- dma_set_mask(hcd-self.controller, DMA_BIT_MASK(32));
+ if (dma_set_mask(hcd-self.controller, DMA_BIT_MASK(32)))
+ goto error;
+ dma_set_coherent_mask(hcd-self.controller,
+ DMA_BIT_MASK(32));
}
xhci_dbg(xhci, Calling HCD init\n);
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] xhci: fix dma mask setup in xhci.c
On Thu, Jun 6, 2013 at 5:21 PM, Xenia Ragiadakou burzalod...@gmail.com wrote: This patch adds a check on whether the host machine supports the xHC DMA address mask and sets the DMA mask for coherent DMA address allocation via an explicit call to dma_set_coherent_mask(). According to DMA-API-HOWTO, if coherent DMA address mask has not been set explicitly via dma_set_coherent_mask(), and the driver calls dma_alloc_coherent() or dma_pool_create() to allocate consistent DMA memory blocks, the consistent DMA mapping interface will return by default DMA addresses which are 32-bit addressable. Hence, if 64-bit DMA mapping is supported, it is appropriate to call dma_set_coherent_mask() with DMA_BIT_MASK(64) to take advantage of it. Also, according to DMA-API-HOWTO, dma_set_coherent_mask() is guaranteed to set successfully the same or a smaller mask as dma_set_mask(). It looks for me overcomplicated. We have *dma_mask and dma_coherent mask in the struct device. First question, who is allocating memory for dma_mask? Second, in case of dma_mask == NULL, dma_set_mask fails with -EIO. It doesn't mean we have no support of this one. How do you handle this case? I think it's pretty simple to set dma_coherent_mask and then apply its address to the dma_mask, because you set the same values anyway. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] xhci: fix dma mask setup in xhci.c
On Thu, Jun 06, 2013 at 09:37:51PM +0300, Andy Shevchenko wrote: On Thu, Jun 6, 2013 at 5:21 PM, Xenia Ragiadakou burzalod...@gmail.com wrote: This patch adds a check on whether the host machine supports the xHC DMA address mask and sets the DMA mask for coherent DMA address allocation via an explicit call to dma_set_coherent_mask(). According to DMA-API-HOWTO, if coherent DMA address mask has not been set explicitly via dma_set_coherent_mask(), and the driver calls dma_alloc_coherent() or dma_pool_create() to allocate consistent DMA memory blocks, the consistent DMA mapping interface will return by default DMA addresses which are 32-bit addressable. Hence, if 64-bit DMA mapping is supported, it is appropriate to call dma_set_coherent_mask() with DMA_BIT_MASK(64) to take advantage of it. Also, according to DMA-API-HOWTO, dma_set_coherent_mask() is guaranteed to set successfully the same or a smaller mask as dma_set_mask(). It looks for me overcomplicated. Why? We have *dma_mask and dma_coherent mask in the struct device. First question, who is allocating memory for dma_mask? The xHCI driver is allocating memory for the host hardware structures, and the USB core is using the DMA mask of the host controller in order to move USB buffers into bounce buffers as necessary. The xHCI driver allocates memory from both DMA pools and with kmalloc. We need both calls to dma_set_coherent_mask() and dma_set_coherent_mask() to get 64-bit DMA addresses for both types of memory. Second, in case of dma_mask == NULL, dma_set_mask fails with -EIO. It doesn't mean we have no support of this one. How do you handle this case? When would the dma_mask be NULL? Note that xHCI PCI hosts *must* be able to support DMA. I think it's pretty simple to set dma_coherent_mask and then apply its address to the dma_mask, because you set the same values anyway. I'm not quite understanding what you want to do. (I'll blame lack of sleep, sorry.) Can you write some pseudo code for me? Sarah Sharp -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
