Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT

[Bjorn Andersson]

On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:

> 
> 
> On 22/02/16 05:32, Bjorn Andersson wrote:
> >On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
> >to be able to do DMA allocations, so use the of_dma_configure() helper
> >to populate the dma properties and assign an appropriate dma_ops.
> >
> >Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >---
> >  drivers/usb/chipidea/core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> >index 7404064b9bbc..047b9d4e67aa 100644
> >--- a/drivers/usb/chipidea/core.c
> >+++ b/drivers/usb/chipidea/core.c
> >@@ -62,6 +62,7 @@
> >  #include <linux/usb/chipidea.h>
> >  #include <linux/usb/of.h>
> >  #include <linux/of.h>
> >+#include <linux/of_device.h>
> >  #include <linux/phy.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/usb/ehci_def.h>
> >@@ -834,6 +835,9 @@ struct platform_device *ci_hdrc_add_device(struct device *dev,
> >  	pdev->dev.dma_parms = dev->dma_parms;
> >  	dma_set_coherent_mask(&pdev->dev, dev->coherent_dma_mask);
> >
> >+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> >+		of_dma_configure(&pdev->dev, dev->of_node);
> >+
> Would we hit the same issue if we are on non Device tree platforms like ACPI
> or platform device style itself?
> 

As far as I can see, yes.

> 
> >  	ret = platform_device_add_resources(pdev, res, nres);
> >  	if (ret)
> >  		goto err;
> >
> 
> I think this is the side effect of commit
> 1dccb598df549d892b6450c261da54cdd7af44b4(arm64: simplify dma_get_ops)
> 

I agree, before that we would have hit:

__generic_dma_ops() {
..
       else if (acpi_disabled)
               return dma_ops;
...
}

with dma_ops being swiotlb_dma_ops from arm64_dma_init().


But this would not have saved us in the ACPI case, i.e. the result would
have been as with my suggested patch. Poking Arnd here to see if he has
any input.

> None of the drivers call of_dma_configure() explicitly, which makes me feel
> that we are doing something wrong. TBH, this should be handled in more
> generic way rather than driver like this having an explicit call to
> of_dma_configure().
> 

I agree, trying to figure out if it should be inherited or something.

> 
> On the other hand, I think we could also solve the issue by using correct
> device(parent device) while allocating dma/dma-pool.
> 

Unfortunately this solves the allocation part, but in udc-core we try to
map and unmap buffers based on the gadget's parent pointer (i.e. the
chipidea device).


I'm still puzzled to why the chipidea lives as a child device of the msm
device; but as this is a rather common structure I believe this still
needs to be figured out.


@Arnd, the Qualcomm (msm) chipidea driver instantiates a chipidea
device, which tries to do DMA mapping operations on ARM64 and fails,
because we don't have dma_ops specified on the child. Using
of_dma_configure() we can populate the child with appropriate settings
and ops, but we would be the first driver doing so. Do you have any
pointers to follow on what to do here?

Regards,
Bjorn