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

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

[Bjorn Andersson]

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@linaro.org>
---
 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);
+
 	ret = platform_device_add_resources(pdev, res, nres);
 	if (ret)
 		goto err;
-- 
2.5.0

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

[Peter Chen]

On Sun, Feb 21, 2016 at 09:32:13PM -0800, 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@linaro.org>
> ---
>  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);
> +
>  	ret = platform_device_add_resources(pdev, res, nres);
>  	if (ret)
>  		goto err;

Just would like to confirm, it will not affect the default behavior
which the "dma-ranges" is not set at those platforms?

-- 

Best Regards,
Peter Chen

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

[Bjorn Andersson]

On Sun 21 Feb 22:02 PST 2016, Peter Chen wrote:

> On Sun, Feb 21, 2016 at 09:32:13PM -0800, 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@linaro.org>
> > ---
> >  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);
> > +
> >  	ret = platform_device_add_resources(pdev, res, nres);
> >  	if (ret)
> >  		goto err;
> 
> Just would like to confirm, it will not affect the default behavior
> which the "dma-ranges" is not set at those platforms?
> 

If I read the code correctly, the only difference if you don't specify 
dma-ranges, dma-coherent or specify an iommu is that the dma_ops gets
assigned.

Regards,
Bjorn

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

[Srinivas Kandagatla]

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@linaro.org>
> ---
>   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?


>   	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)

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().


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


------------------------>cut<-----------------------------

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 6e53c24..0293ed5 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1844,13 +1844,13 @@ static int udc_start(struct ci_hdrc *ci)
         INIT_LIST_HEAD(&ci->gadget.ep_list);

         /* alloc resources */
-       ci->qh_pool = dma_pool_create("ci_hw_qh", dev,
+       ci->qh_pool = dma_pool_create("ci_hw_qh", dev->parent,
                                        sizeof(struct ci_hw_qh),
                                        64, CI_HDRC_PAGE_SIZE);
         if (ci->qh_pool == NULL)
                 return -ENOMEM;

-       ci->td_pool = dma_pool_create("ci_hw_td", dev,
+       ci->td_pool = dma_pool_create("ci_hw_td", dev->parent,
                                        sizeof(struct ci_hw_td),
                                        64, CI_HDRC_PAGE_SIZE);
         if (ci->td_pool == NULL) {
------------------------>cut<-----------------------------


--srini

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@linaro.org>
> >---
> >  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

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

[Peter Chen]

On Mon, Feb 22, 2016 at 02:07:50PM -0800, Bjorn Andersson wrote:
> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:
> 
> 
> 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.
> 

Chipidea is core driver; msm driver is glue layer, it has something
platform related.

-- 

Best Regards,
Peter Chen

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

[Li Yang]

On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> 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.

We also hit the same issue with the dwc3 driver.

>> >
>> >Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> >---
>> >  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.

I also agree.  We need address it in a more generic way.  I did a
search for platform_device_add()/platform_device_register() in the
kernel source code.  I found a lot of them and many could be also
doing DMA.  Looks like it is still too early to assume every device is
already getting dma_ops set through bus probe.  Otherwise, many
drivers are potentially broken by this assumption.

Regards,
Leo

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

[Li Yang]

On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
>> 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.
>
> We also hit the same issue with the dwc3 driver.
>
>>> >
>>> >Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> >---
>>> >  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.
>
> I also agree.  We need address it in a more generic way.  I did a
> search for platform_device_add()/platform_device_register() in the
> kernel source code.  I found a lot of them and many could be also
> doing DMA.  Looks like it is still too early to assume every device is
> already getting dma_ops set through bus probe.  Otherwise, many
> drivers are potentially broken by this assumption.

Any further comment on this topic?  I added the linux-arm mailing list
which was missing from previous discussion.

Regards,
Leo

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

[Bjorn Andersson]

On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>> 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.
[..]
>>>> 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.
>>
>> I also agree.  We need address it in a more generic way.  I did a
>> search for platform_device_add()/platform_device_register() in the
>> kernel source code.  I found a lot of them and many could be also
>> doing DMA.  Looks like it is still too early to assume every device is
>> already getting dma_ops set through bus probe.  Otherwise, many
>> drivers are potentially broken by this assumption.
>
> Any further comment on this topic?  I added the linux-arm mailing list
> which was missing from previous discussion.
>

I had the chance to go through this with Arnd and the verdict is that
devices not described in DT should not do DMA (or allocate buffers for
doing DMA).

So I believe the solution is to fall back on Peter's description; the
chipidea driver is the core driver and the Qualcomm code should just
be a platform layer.

My suggestion is that we turn the chipidea core into a set of APIs
that can called by the platform specific pieces. That way we will have
the chipidea core be the device described in the DT.

I'll try to find some time to prototype this after Connect.

Regards,
Bjorn

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

[Li Yang]

On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
>> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
>>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
>>> <bjorn.andersson@linaro.org> wrote:
>>>> 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.
> [..]
>>>>> 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.
>>>
>>> I also agree.  We need address it in a more generic way.  I did a
>>> search for platform_device_add()/platform_device_register() in the
>>> kernel source code.  I found a lot of them and many could be also
>>> doing DMA.  Looks like it is still too early to assume every device is
>>> already getting dma_ops set through bus probe.  Otherwise, many
>>> drivers are potentially broken by this assumption.
>>
>> Any further comment on this topic?  I added the linux-arm mailing list
>> which was missing from previous discussion.
>>
>
> I had the chance to go through this with Arnd and the verdict is that
> devices not described in DT should not do DMA (or allocate buffers for
> doing DMA).
>
> So I believe the solution is to fall back on Peter's description; the
> chipidea driver is the core driver and the Qualcomm code should just
> be a platform layer.
>
> My suggestion is that we turn the chipidea core into a set of APIs
> that can called by the platform specific pieces. That way we will have
> the chipidea core be the device described in the DT.

But like I said, this problem is not just existing for chipidea
driver.  We already found that the dwc3 driver is also suffering from
the same issue.  I don't know how many other drivers are impacted by
this change, but I suspect there will be some. A grep of
platform_device_add() in driver/ directory returns many possible
drivers to be impacted.  As far as I know, the
drivers/net/ethernet/freescale/fman/mac.c is registering child
ethernet devices that definitely will do dma.   If you want to do this
kind of rework to all these drivers, it will be a really big effort.

Regards,
Leo

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

[Peter Chen]

On Wed, Mar 09, 2016 at 05:16:50PM -0600, Li Yang wrote:
> On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> >> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> >>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> >>> <bjorn.andersson@linaro.org> wrote:
> >>>> 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.
> > [..]
> >>>>> 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.
> >>>
> >>> I also agree.  We need address it in a more generic way.  I did a
> >>> search for platform_device_add()/platform_device_register() in the
> >>> kernel source code.  I found a lot of them and many could be also
> >>> doing DMA.  Looks like it is still too early to assume every device is
> >>> already getting dma_ops set through bus probe.  Otherwise, many
> >>> drivers are potentially broken by this assumption.
> >>
> >> Any further comment on this topic?  I added the linux-arm mailing list
> >> which was missing from previous discussion.
> >>
> >
> > I had the chance to go through this with Arnd and the verdict is that
> > devices not described in DT should not do DMA (or allocate buffers for
> > doing DMA).
> >
> > So I believe the solution is to fall back on Peter's description; the
> > chipidea driver is the core driver and the Qualcomm code should just
> > be a platform layer.
> >
> > My suggestion is that we turn the chipidea core into a set of APIs
> > that can called by the platform specific pieces. That way we will have
> > the chipidea core be the device described in the DT.
> 
> But like I said, this problem is not just existing for chipidea
> driver.  We already found that the dwc3 driver is also suffering from
> the same issue.  I don't know how many other drivers are impacted by
> this change, but I suspect there will be some. A grep of
> platform_device_add() in driver/ directory returns many possible
> drivers to be impacted.  As far as I know, the
> drivers/net/ethernet/freescale/fman/mac.c is registering child
> ethernet devices that definitely will do dma.   If you want to do this
> kind of rework to all these drivers, it will be a really big effort.
> 

+1

Yes, I think this DMA things should be covered by driver core too.

-- 

Best Regards,
Peter Chen

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

[Arnd Bergmann]

On Monday 14 March 2016 18:51:08 Peter Chen wrote:
> On Wed, Mar 09, 2016 at 05:16:50PM -0600, Li Yang wrote:
> > On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> > >> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> > >>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> > >>> <bjorn.andersson@linaro.org> wrote:
> > >>>> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:
> > >
> > > I had the chance to go through this with Arnd and the verdict is that
> > > devices not described in DT should not do DMA (or allocate buffers for
> > > doing DMA).
> > >
> > > So I believe the solution is to fall back on Peter's description; the
> > > chipidea driver is the core driver and the Qualcomm code should just
> > > be a platform layer.
> > >
> > > My suggestion is that we turn the chipidea core into a set of APIs
> > > that can called by the platform specific pieces. That way we will have
> > > the chipidea core be the device described in the DT.
> > 
> > But like I said, this problem is not just existing for chipidea
> > driver.  We already found that the dwc3 driver is also suffering from
> > the same issue.  I don't know how many other drivers are impacted by
> > this change, but I suspect there will be some. A grep of
> > platform_device_add() in driver/ directory returns many possible
> > drivers to be impacted.  As far as I know, the
> > drivers/net/ethernet/freescale/fman/mac.c is registering child
> > ethernet devices that definitely will do dma.   If you want to do this
> > kind of rework to all these drivers, it will be a really big effort.
> > 
> 
> +1
> 
> Yes, I think this DMA things should be covered by driver core too.
> 

I don't think it's a very widespread problem, there are only very few
developers that intentionally use this method, and some use the
platform_device_register_full() call to create a device with a known
mask, which is generally ok for the limited case where the driver
is only ever going to run on a single platform, but not in the
more general case that of_dma_configure is designed to handle.

I think we should fix the drivers to consistently use the device
that was created by the platform (DT or ACPI or board file)
to pass that into the DMA API, anything else will just cause
more subtle bugs.

	Arnd

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

[Peter Chen]

On Thu, Mar 17, 2016 at 04:52:55PM +0100, Arnd Bergmann wrote:
> On Monday 14 March 2016 18:51:08 Peter Chen wrote:
> > On Wed, Mar 09, 2016 at 05:16:50PM -0600, Li Yang wrote:
> > > On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> > > >> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> > > >>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> > > >>> <bjorn.andersson@linaro.org> wrote:
> > > >>>> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:
> > > >
> > > > I had the chance to go through this with Arnd and the verdict is that
> > > > devices not described in DT should not do DMA (or allocate buffers for
> > > > doing DMA).
> > > >
> > > > So I believe the solution is to fall back on Peter's description; the
> > > > chipidea driver is the core driver and the Qualcomm code should just
> > > > be a platform layer.
> > > >
> > > > My suggestion is that we turn the chipidea core into a set of APIs
> > > > that can called by the platform specific pieces. That way we will have
> > > > the chipidea core be the device described in the DT.
> > > 
> > > But like I said, this problem is not just existing for chipidea
> > > driver.  We already found that the dwc3 driver is also suffering from
> > > the same issue.  I don't know how many other drivers are impacted by
> > > this change, but I suspect there will be some. A grep of
> > > platform_device_add() in driver/ directory returns many possible
> > > drivers to be impacted.  As far as I know, the
> > > drivers/net/ethernet/freescale/fman/mac.c is registering child
> > > ethernet devices that definitely will do dma.   If you want to do this
> > > kind of rework to all these drivers, it will be a really big effort.
> > > 
> > 
> > +1
> > 
> > Yes, I think this DMA things should be covered by driver core too.
> > 
> 
> I don't think it's a very widespread problem, there are only very few
> developers that intentionally use this method, and some use the
> platform_device_register_full() call to create a device with a known
> mask, which is generally ok for the limited case where the driver
> is only ever going to run on a single platform, but not in the
> more general case that of_dma_configure is designed to handle.

Even only for qualcomm platforms, it may be possible have different
DMA masks at ARM64 platforms, so we may can't use a fixed value
at glue layer driver. So, using of_dma_configure is suitable choice
for DT platforms for this case, right?

> 
> I think we should fix the drivers to consistently use the device
> that was created by the platform (DT or ACPI or board file)
> to pass that into the DMA API, anything else will just cause
> more subtle bugs.
> 

Although I don't know what kinds of bugs it may have, it may be
met before, otherwise, why most of platform drivers need to call
dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly

-- 

Best Regards,
Peter Chen

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

[Rajesh Bhagat]

> -----Original Message-----
> From: Peter Chen [mailto:hzpeterchen@gmail.com]
> Sent: Friday, March 18, 2016 7:24 AM
> To: Arnd Bergmann <arnd@arndb.de>
> Cc: Li Yang <leoli@freescale.com>; Bjorn Andersson <bjorn.andersson@linaro.org>;
> Peter Chen <peter.chen@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Rajesh Bhagat <rajesh.bhagat@nxp.com>; linux-
> usb@vger.kernel.org; lkml <linux-kernel@vger.kernel.org>; Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org>; linux-arm-msm <linux-arm-
> msm@vger.kernel.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] usb: chipidea: Configure DMA properties and ops from DT
> 
> On Thu, Mar 17, 2016 at 04:52:55PM +0100, Arnd Bergmann wrote:
> > On Monday 14 March 2016 18:51:08 Peter Chen wrote:
> > > On Wed, Mar 09, 2016 at 05:16:50PM -0600, Li Yang wrote:
> > > > On Tue, Mar 8, 2016 at 9:40 PM, Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > > > On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> > > > >> On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> > > > >>> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> > > > >>> <bjorn.andersson@linaro.org> wrote:
> > > > >>>> On Mon 22 Feb 02:03 PST 2016, Srinivas Kandagatla wrote:
> > > > >
> > > > > I had the chance to go through this with Arnd and the verdict is
> > > > > that devices not described in DT should not do DMA (or allocate
> > > > > buffers for doing DMA).
> > > > >
> > > > > So I believe the solution is to fall back on Peter's
> > > > > description; the chipidea driver is the core driver and the
> > > > > Qualcomm code should just be a platform layer.
> > > > >
> > > > > My suggestion is that we turn the chipidea core into a set of
> > > > > APIs that can called by the platform specific pieces. That way
> > > > > we will have the chipidea core be the device described in the DT.
> > > >
> > > > But like I said, this problem is not just existing for chipidea
> > > > driver.  We already found that the dwc3 driver is also suffering
> > > > from the same issue.  I don't know how many other drivers are
> > > > impacted by this change, but I suspect there will be some. A grep
> > > > of
> > > > platform_device_add() in driver/ directory returns many possible
> > > > drivers to be impacted.  As far as I know, the
> > > > drivers/net/ethernet/freescale/fman/mac.c is registering child
> > > > ethernet devices that definitely will do dma.   If you want to do this
> > > > kind of rework to all these drivers, it will be a really big effort.
> > > >
> > >
> > > +1
> > >
> > > Yes, I think this DMA things should be covered by driver core too.
> > >
> >
> > I don't think it's a very widespread problem, there are only very few
> > developers that intentionally use this method, and some use the
> > platform_device_register_full() call to create a device with a known
> > mask, which is generally ok for the limited case where the driver is
> > only ever going to run on a single platform, but not in the more
> > general case that of_dma_configure is designed to handle.
> 
> Even only for qualcomm platforms, it may be possible have different DMA masks at
> ARM64 platforms, so we may can't use a fixed value at glue layer driver. So, using
> of_dma_configure is suitable choice for DT platforms for this case, right?
> 
> >
> > I think we should fix the drivers to consistently use the device that
> > was created by the platform (DT or ACPI or board file) to pass that
> > into the DMA API, anything else will just cause more subtle bugs.
> >
> 
> Although I don't know what kinds of bugs it may have, it may be met before,
> otherwise, why most of platform drivers need to call dma_set_coherent_mask or
> dma_coerce_mask_and_coherent explicitly
> 
> --
> 
> Best Regards,
> Peter Chen

Though chipidea platform drivers are calling functions mentioned by you i.e. dma_set_coherent_mask
or dma_coerce_mask_and_coherent explicity e.g. in file drivers/usb/chipidea/ci_hdrc_imx.c. 

Still the mentioned error is coming while calling ci_hdrc_add_device which lies in chipidea/core.c. similar is 
the case with DWC3 driver. 

Best Regards,
Rajesh Bhagat 

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

[Arnd Bergmann]

On Friday 18 March 2016 09:54:14 Peter Chen wrote:
> > 
> > I don't think it's a very widespread problem, there are only very few
> > developers that intentionally use this method, and some use the
> > platform_device_register_full() call to create a device with a known
> > mask, which is generally ok for the limited case where the driver
> > is only ever going to run on a single platform, but not in the
> > more general case that of_dma_configure is designed to handle.
> 
> Even only for qualcomm platforms, it may be possible have different
> DMA masks at ARM64 platforms, so we may can't use a fixed value
> at glue layer driver. So, using of_dma_configure is suitable choice
> for DT platforms for this case, right?

Yes.

> > I think we should fix the drivers to consistently use the device
> > that was created by the platform (DT or ACPI or board file)
> > to pass that into the DMA API, anything else will just cause
> > more subtle bugs.
> > 
> 
> Although I don't know what kinds of bugs it may have, it may be
> met before, otherwise, why most of platform drivers need to call
> dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly

Any driver that wants to do 64-bit addressing on DMA should call
dma_set_mask()/dma_set_coherent_mask() on its device and check the
return code.

No driver should call dma_coerce_mask_and_coherent() on its own
device, it's basically always a bug and we named the function
to make that more obvious. The problem with dma_coerce_mask_and_coherent()
is that it just overrides whatever the platform knows about the
device when the driver thinks it knows better. 

The reason for having those calls in a lot of drivers is that
traditionally, ARM platforms booting with DT did not set up any DMA
mask and the drivers worked around it by manually setting up a mask
that happened to work for them (almost all 32-bit ARM devices need
a 32-bit mask without coherency or offset or iommu, so that's easy).
We now call of_dma_configure() for all devices that get probed from
DT, so we should be removing those calls.

	Arnd

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

[Russell King - ARM Linux]

On Fri, Mar 18, 2016 at 09:54:14AM +0800, Peter Chen wrote:
> Although I don't know what kinds of bugs it may have, it may be
> met before, otherwise, why most of platform drivers need to call
> dma_set_coherent_mask or dma_coerce_mask_and_coherent explicitly

See Documentation/DMA-API.txt, specifically the section starting

  Part Ic - DMA addressing limitations
  ------------------------------------

and also Documentation/DMA-API-HOWTO.txt, the section on

  DMA addressing limitations

which provides further information.

Drivers using DMA should be using dma_set_mask_and_coherent() _or_
one of dma_set_mask() and dma_set_coherent_mask() depending on which
types of DMA they wish to perform.  Drivers should not use
dma_coerce_mask_and_coherent() except in exceptional circumstances:
that function is more a marker that they or some bus/platform code
is doing something wrong.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Peter Chen]

On Tue, Mar 08, 2016 at 07:40:08PM -0800, Bjorn Andersson wrote:
> On Tue, Mar 8, 2016 at 11:52 AM, Li Yang <leoli@freescale.com> wrote:
> > On Wed, Mar 2, 2016 at 4:59 PM, Li Yang <leoli@freescale.com> wrote:
> >> On Mon, Feb 22, 2016 at 4:07 PM, Bjorn Andersson
> >> <bjorn.andersson@linaro.org> wrote:
> >>> 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.
> [..]
> >>>> 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 had the chance to go through this with Arnd and the verdict is that
> devices not described in DT should not do DMA (or allocate buffers for
> doing DMA).
> 
> So I believe the solution is to fall back on Peter's description; the
> chipidea driver is the core driver and the Qualcomm code should just
> be a platform layer.
> 
> My suggestion is that we turn the chipidea core into a set of APIs
> that can called by the platform specific pieces. That way we will have
> the chipidea core be the device described in the DT.
> 

Hi Bjorn,

After reading the DMA documentation Russell supplied and several related
DMA APIs, would you please try below patch on your ARM64 platform?
Since the core device has no device node at all, I don't know why
your patch can work, or am I missing something?

>From bcf7eaf694d29fb7557a9406fb6c89213216069c Mon Sep 17 00:00:00 2001
From: Peter Chen <peter.chen@freescale.com>
Date: Fri, 25 Mar 2016 11:54:21 +0800
Subject: [PATCH 1/1] usb: chipidea: add DMA mask configuration API

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_msm.c |    6 ++++++
 drivers/usb/chipidea/core.c        |   25 +++++++++++++++++++++++++
 include/linux/usb/chipidea.h       |    2 ++
 3 files changed, 33 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
index 3889809..43ceb38 100644
--- a/drivers/usb/chipidea/ci_hdrc_msm.c
+++ b/drivers/usb/chipidea/ci_hdrc_msm.c
@@ -5,6 +5,7 @@
  * only version 2 as published by the Free Software Foundation.
  */
 
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -56,6 +57,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
 {
 	struct platform_device *plat_ci;
 	struct usb_phy *phy;
+	int ret;
 
 	dev_dbg(&pdev->dev, "ci_hdrc_msm_probe\n");
 
@@ -70,6 +72,10 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
 
 	ci_hdrc_msm_platdata.usb_phy = phy;
 
+	ret = ci_hdrc_set_dma_mask(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret)
+		return ret;
+
 	plat_ci = ci_hdrc_add_device(&pdev->dev,
 				pdev->resource, pdev->num_resources,
 				&ci_hdrc_msm_platdata);
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 69426e6..b8ca5e3 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>
@@ -811,6 +812,30 @@ static void ci_extcon_unregister(struct ci_hdrc *ci)
 
 static DEFINE_IDA(ci_ida);
 
+/**
+ * ci_hdrc_set_dma_mask
+ *
+ * Set dma mask and coherent dma mask for glue layer device, and the core
+ * device will inherit these values. If the 'dma-ranges' is specified at
+ * DT, it will use this value for both dma mask and coherent dma mask.
+ *
+ * @dev: a pointer to the device struct of glue layer device
+ * @ci_coherent_dma_mask: the mask for both dma_mask and cohrent_dma_mask
+ */
+int ci_hdrc_set_dma_mask(struct device *dev, u64 ci_coherent_dma_mask)
+{
+	int ret = dma_set_mask_and_coherent(dev, ci_coherent_dma_mask);
+	if (ret) {
+		dev_err(dev, "dma_set_mask_and_coherent fails\n");
+		return ret;
+	}
+
+	if (dev_of_node(dev))
+		of_dma_configure(dev, dev->of_node);
+
+	return ret;
+}
+
 struct platform_device *ci_hdrc_add_device(struct device *dev,
 			struct resource *res, int nres,
 			struct ci_hdrc_platform_data *platdata)
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 5dd75fa..8649930 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -84,4 +84,6 @@ struct platform_device *ci_hdrc_add_device(struct device *dev,
 /* Remove ci hdrc device */
 void ci_hdrc_remove_device(struct platform_device *pdev);
 
+int ci_hdrc_set_dma_mask(struct device *dev, u64 ci_coherent_dma_mask);
+
 #endif

-- 
Best Regards,
Peter Chen

Re: [PATCH 1/1] usb: chipidea: add DMA mask configuration API

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

Hi Peter,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160324]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/usb-chipidea-add-DMA-mask-configuration-API/20160325-120535
config: i386-randconfig-s1-201612 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "ci_hdrc_set_dma_mask" [drivers/usb/chipidea/ci_hdrc_msm.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23634 bytes --]

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

[Bjorn Andersson]

hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent
memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated manually
it will not have any dma_mem or dma_ops assigned, which makes the
dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves
this by assigning the dma_mem and dma_ops based on the parent's DeviceTree
node.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Hi Peter,

After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm
systems I realized that we never concluded on this patch. Unfortunately I can't
find it in my mailbox either, so resending it to restart the discussion.

Regards,
Bjorn

 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 69426e644d17..6218d83cca25 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>
@@ -837,6 +838,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);
+
 	ret = platform_device_add_resources(pdev, res, nres);
 	if (ret)
 		goto err;
-- 
2.5.0

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

[Stephen Boyd]

On 10/21, Bjorn Andersson wrote:
> hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent
> memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated manually
> it will not have any dma_mem or dma_ops assigned, which makes the
> dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves
> this by assigning the dma_mem and dma_ops based on the parent's DeviceTree
> node.
> 
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Hi Peter,
> 
> After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm
> systems I realized that we never concluded on this patch. Unfortunately I can't
> find it in my mailbox either, so resending it to restart the discussion.
> 

I thought we were going to go down the route that Arnd has been
pushing[1]? That should work, but I haven't tried it yet and
there are some more fixes on top from Sriram. I think Sriram is
taking over the patch now?

[1] https://patchwork.kernel.org/patch/9319527/

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Bjorn Andersson]

On Fri 21 Oct 10:38 PDT 2016, Stephen Boyd wrote:

> On 10/21, Bjorn Andersson wrote:
> > hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating coherent
> > memory on behalf of ci_hdrc driver. But as the ci_hdrc is instantiated manually
> > it will not have any dma_mem or dma_ops assigned, which makes the
> > dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch solves
> > this by assigning the dma_mem and dma_ops based on the parent's DeviceTree
> > node.
> > 
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Hi Peter,
> > 
> > After (once more) debugging why USB doesn't work up on the 64-bit Qualcomm
> > systems I realized that we never concluded on this patch. Unfortunately I can't
> > find it in my mailbox either, so resending it to restart the discussion.
> > 
> 
> I thought we were going to go down the route that Arnd has been
> pushing[1]? That should work, but I haven't tried it yet and
> there are some more fixes on top from Sriram. I think Sriram is
> taking over the patch now?
> 
> [1] https://patchwork.kernel.org/patch/9319527/

Thanks for the pointer, I've heard about it but couldn't find it.

It does make me further wonder about the multi-device model of these
drivers, but I agree with you that it looks like the patch would
solve our issue.

Regards,
Bjorn

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

[Sriram Dash]

>From: Stephen Boyd [mailto:sboyd@codeaurora.org]
>On 10/21, Bjorn Andersson wrote:
>> hcd_alloc_coherent() and usb_alloc_coherent() ends up allocating
>> coherent memory on behalf of ci_hdrc driver. But as the ci_hdrc is
>> instantiated manually it will not have any dma_mem or dma_ops
>> assigned, which makes the
>> dma_alloc_coherent() fail on some platforms (e.g. arm64). This patch
>> solves this by assigning the dma_mem and dma_ops based on the parent's
>> DeviceTree node.
>>
>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>
>> Hi Peter,
>>
>> After (once more) debugging why USB doesn't work up on the 64-bit
>> Qualcomm systems I realized that we never concluded on this patch.
>> Unfortunately I can't find it in my mailbox either, so resending it to restart the
>discussion.
>>
>
>I thought we were going to go down the route that Arnd has been pushing[1]? That
>should work, but I haven't tried it yet and there are some more fixes on top from
>Sriram. I think Sriram is taking over the patch now?
>

Yes Stephen. I am incorporating the idea from Arnd and working on those patches.

Regards,
Sriram

>[1] https://patchwork.kernel.org/patch/9319527/
>
>--
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
>Foundation Collaborative Project