Re: [RFC v3] xhci: fix dma mask setup in xhci.c
Hi, On Tue, Jul 09, 2013 at 02:43:45PM -0700, Sarah Sharp wrote: On Tue, Jul 09, 2013 at 02:59:38PM +0300, Felipe Balbi wrote: 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: 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 = 0xUL; 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; Ok, so that means Xenia needs to make sure to check whether the dma_mask pointer is non-NULL in xhci-plat.c, so that she doesn't overwrite it with a pointer to the coherent dma mask, correct? Or should she just overwrite the pointer because the OF core really shouldn't be setting the DMA mask pointer? I think it's safe to overwrite (and perhaps even better to do so) provided OF core enables 32-bits blindly, without checking anything about the device. Eventually that dma_mask setting should, likely, come via DeviceTree to the platform_devices. Until, we can overwrite with whatever we can discover out of the device's registers. -- balbi signature.asc Description: Digital signature
Re: [RFC v3] xhci: fix dma mask setup in xhci.c
On 07/09/2013 01:59 PM, Felipe Balbi wrote: 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. The DMA mask for PCI is set to 32bit by default [0]. I don't know if the PCI spec hides a bit or two which say that it can do more than that. I know that the ehci limits the DMA mask to 2GiB for some vendors. A bunch of network drivers increase the mask to 64bit depending on the card type (that means to host does not matter). That means if xhci unconditionally supports 64bit addressing we could init at a central point. I don't think ARM/omap5 has a restriction to 32bit only. [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/probe.c#n1348 Sebastian -- 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 v3] xhci: fix dma mask setup in xhci.c
On Tue, 9 Jul 2013, Felipe Balbi wrote: Hi, On Mon, Jul 08, 2013 at 09:55:15PM -0700, Sarah Sharp wrote: 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? I bet Xenia meant the xHC controller and the host system. 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: ... 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 It's because of this line in usb_create_shared_hcd(): hcd-self.uses_dma = (dev-dma_mask != NULL); As a result, HCDs shouldn't have to set this flag themselves. Alan Stern -- 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 v3] xhci: fix dma mask setup in xhci.c
On Tue, Jul 09, 2013 at 02:59:38PM +0300, Felipe Balbi wrote: 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: 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 = 0xUL; 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; Ok, so that means Xenia needs to make sure to check whether the dma_mask pointer is non-NULL in xhci-plat.c, so that she doesn't overwrite it with a pointer to the coherent dma mask, correct? Or should she just overwrite the pointer because the OF core really shouldn't be setting the DMA mask pointer? 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 v3] xhci: fix dma mask setup in xhci.c
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 0x. 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?
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.
Does that mean xHCI platform devices have just been broken all this
time? Has this code been tested at all?
If dma_set_mask() succeeds, for a given bitmask, it is guaranteed that
the given bitmask is also supported for consistent DMA mappings.
That is the reason why this patch does not add checks when setting the
coherent_dma_mask.
Signed-off-by: Xenia Ragiadakou burzalod...@gmail.com
---
Differences from version 1 and 2:
The dma_mask setup code was removed from the xhci_gen_setup()
function defined in xhci.c and was placed in xhci_pci_setup(),
defined in xhci-pci.c, and in xhci_plat_setup(), defined in
xhci-plat.c. The dma mask setup code has to be called after
the mapping of host controller registers and before
[RFC v3] xhci: fix dma mask setup in xhci.c
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 0x. 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.
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.
If dma_set_mask() succeeds, for a given bitmask, it is guaranteed that
the given bitmask is also supported for consistent DMA mappings.
That is the reason why this patch does not add checks when setting the
coherent_dma_mask.
Signed-off-by: Xenia Ragiadakou burzalod...@gmail.com
---
Differences from version 1 and 2:
The dma_mask setup code was removed from the xhci_gen_setup()
function defined in xhci.c and was placed in xhci_pci_setup(),
defined in xhci-pci.c, and in xhci_plat_setup(), defined in
xhci-plat.c. The dma mask setup code has to be called after
the mapping of host controller registers and before the
allocation of xhci memory buffers, so it was placed in the
driver's 'reset' callback function before the call to
xhci_gen_setup().
Initialization of the dma_mask pointer was added for non-pci
platforms, following a remark made by Andy Shevchenco.
drivers/usb/host/xhci-pci.c | 19 ++-
drivers/usb/host/xhci-plat.c | 26 ++
drivers/usb/host/xhci.c | 17 -
3 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index cc24e39..dff5a22 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -122,9 +122,26 @@ static void xhci_pci_quirks(struct device *dev, struct
xhci_hcd *xhci)
static int xhci_pci_setup(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci;
- struct pci_dev *pdev = to_pci_dev(hcd-self.controller);
+ struct pci_dev *pdev;
+ struct device *dev;
int retval;
+ dev = hcd-self.controller;
+ pdev = to_pci_dev(dev);
+
+ if (usb_hcd_is_primary_hcd(hcd)) {
+ struct xhci_cap_regs __iomem*cap_regs;
+ u32 hcc_params;
+
+ cap_regs = hcd-regs;
+ hcc_params = readl(cap_regs-hcc_params);
+ if (HCC_64BIT_ADDR(hcc_params)
+ !dma_set_mask(dev, DMA_BIT_MASK(64))) {
+ dev_dbg(dev, Enabling 64-bit DMA addresses.\n);
+ dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+ }
+ }
+
retval = xhci_gen_setup(hcd, xhci_pci_quirks);
if (retval)
return retval;
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 51e22bf..d718134 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -30,6 +30,32 @@ static void xhci_plat_quirks(struct device *dev, struct
xhci_hcd *xhci)
/* called during probe() after chip reset completes */
static int xhci_plat_setup(struct usb_hcd *hcd)
{
+ struct device *dev;
+ int retval;
+
+ dev = hcd-self.controller;
+ dev-dma_mask = dev-dma_coherent_mask;
+
+ if (usb_hcd_is_primary_hcd(hcd)) {
+ struct xhci_cap_regs __iomem*cap_regs;
+ u32 hcc_params;
+
+ cap_regs = hcd-regs;
+ hcc_params = readl(cap_regs-hcc_params);
+ if (HCC_64BIT_ADDR(hcc_params)
+ !dma_set_mask(dev, DMA_BIT_MASK(64))) {
+ dev_dbg(dev, Enabling 64-bit DMA addresses.\n);
+ } else {
+ retval = dma_set_mask(dev, DMA_BIT_MASK(32));
+ if (retval) {
+ dev-dma_mask = NULL;
+
