[PATCH v3 0/2] usb:dwc3: Enable USB DWC3 support for 64-bit system

[PATCH v3 0/2] usb:dwc3: Enable USB DWC3 support for 64-bit system

[Thang Q. Nguyen]

From: "Thang Q. Nguyen" <tqnguyen@apm.com>

When CONFIG_DMA_CMA is not set, the coherent mask is not set.
These patches enable the USB DWC3 driver to set the coherent mask
correctly by first set coherent DMA mask to 64-bit. If this
failed, attempt again with 32-bit.
In addition, pass the archdata to the xhci-hcd child so that
xhci-hcd can configure DMA operations correctly.

Thang Q. Nguyen (2):
  usb:dwc3: Enable support for 64-bit system
  usb:dwc3: pass arch data to xhci-hcd child

 drivers/usb/dwc3/core.c | 15 +++++++++++++++
 drivers/usb/dwc3/host.c |  1 +
 2 files changed, 16 insertions(+)

-- 
2.2.0

[PATCH v3 1/2] usb:dwc3: Enable support for 64-bit system

[Thang Q. Nguyen]

From: "Thang Q. Nguyen" <tqnguyen@apm.com>

Add 64-bit DMA operation support to the USB DWC3 driver.
First attempt to set the coherent DMA mask for 64-bit DMA.
If that failed, attempt again with 32-bit DMA.

Changes from v2:
	- None.

Changes from v1:
	- Remove WARN_ON if dma_mask is NULL

Signed-off-by: Thang Q. Nguyen <tqnguyen@apm.com>
---
 drivers/usb/dwc3/core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index de5e01f..2479c24 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -831,6 +831,21 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->mem = mem;
 	dwc->dev = dev;
 
+	/* Try to set 64-bit DMA first */
+	if (!pdev->dev.dma_mask)
+		/* Platform did not initialize dma_mask */
+		ret = dma_coerce_mask_and_coherent(&pdev->dev,
+						   DMA_BIT_MASK(64));
+	else
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+
+	/* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!res) {
 		dev_err(dev, "missing IRQ\n");
-- 
2.2.0

Re: [PATCH v3 1/2] usb:dwc3: Enable support for 64-bit system

[Felipe Balbi]

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


Hi,

"Thang Q. Nguyen" <tqnguyen@apm.com> writes:
> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>
> Add 64-bit DMA operation support to the USB DWC3 driver.
> First attempt to set the coherent DMA mask for 64-bit DMA.
> If that failed, attempt again with 32-bit DMA.
>
> Changes from v2:
> 	- None.
>
> Changes from v1:
> 	- Remove WARN_ON if dma_mask is NULL

these changes lines should be between the tearline (---) and diffstat
below.

> Signed-off-by: Thang Q. Nguyen <tqnguyen@apm.com>
> ---
>  drivers/usb/dwc3/core.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index de5e01f..2479c24 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -831,6 +831,21 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc->mem = mem;
>  	dwc->dev = dev;
>  
> +	/* Try to set 64-bit DMA first */
> +	if (!pdev->dev.dma_mask)
> +		/* Platform did not initialize dma_mask */
> +		ret = dma_coerce_mask_and_coherent(&pdev->dev,
> +						   DMA_BIT_MASK(64));
> +	else
> +		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +
> +	/* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
> +	if (ret) {
> +		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +		if (ret)
> +			return ret;
> +	}

Also, why is it so that you need this now ? glue layers are copying dma
mask from parent device and that should be set properly. This really
shouldn't be necessary in dwc3-core; it would mean that glue layer
didn't set this device up properly.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 1/2] usb:dwc3: Enable support for 64-bit system

[Thang Q. Nguyen]

Hi Balbi,
If CONFIG_DMA_CMA=y, dma mask is set properly. The issue just happen
when CONFIG_DMA_CMA is not set. In this case, dma mask is not set and
we need this code to check if dma mask should be manually set to 32 or
64.

----
Thang

On Wed, Mar 30, 2016 at 8:09 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
> Hi,
>
> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>>
>> Add 64-bit DMA operation support to the USB DWC3 driver.
>> First attempt to set the coherent DMA mask for 64-bit DMA.
>> If that failed, attempt again with 32-bit DMA.
>>
>> Changes from v2:
>>       - None.
>>
>> Changes from v1:
>>       - Remove WARN_ON if dma_mask is NULL
>
> these changes lines should be between the tearline (---) and diffstat
> below.
>
>> Signed-off-by: Thang Q. Nguyen <tqnguyen@apm.com>
>> ---
>>  drivers/usb/dwc3/core.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index de5e01f..2479c24 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -831,6 +831,21 @@ static int dwc3_probe(struct platform_device *pdev)
>>       dwc->mem = mem;
>>       dwc->dev = dev;
>>
>> +     /* Try to set 64-bit DMA first */
>> +     if (!pdev->dev.dma_mask)
>> +             /* Platform did not initialize dma_mask */
>> +             ret = dma_coerce_mask_and_coherent(&pdev->dev,
>> +                                                DMA_BIT_MASK(64));
>> +     else
>> +             ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +
>> +     /* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
>> +     if (ret) {
>> +             ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +             if (ret)
>> +                     return ret;
>> +     }
>
> Also, why is it so that you need this now ? glue layers are copying dma
> mask from parent device and that should be set properly. This really
> shouldn't be necessary in dwc3-core; it would mean that glue layer
> didn't set this device up properly.
>
> --
> balbi



-- 

Thang Q. Nguyen    | Staff SW Eng.

C: +849.7684.7606 | O: +848.3770.0640

F: +848.3770.0641  | tqnguyen@apm.com

Re: [PATCH v3 1/2] usb:dwc3: Enable support for 64-bit system

[Felipe Balbi]

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


Hi,

(please don't top-post)

"Thang Q. Nguyen" <tqnguyen@apm.com> writes:
> Hi Balbi,
> If CONFIG_DMA_CMA=y, dma mask is set properly. The issue just happen
> when CONFIG_DMA_CMA is not set. In this case, dma mask is not set and
> we need this code to check if dma mask should be manually set to 32 or
> 64.

Can you point me to the code which has this conditional ? Why would
DMA_CMA=n mean that dma_mask isn't initialized ? According to DMA_CMA's
help text (see below) that's supposed to allow drivers to *allocate*
large contiguous buffers, but that's not the case here.

config DMA_CMA
	bool "DMA Contiguous Memory Allocator"
	depends on HAVE_DMA_CONTIGUOUS && CMA
	help
	  This enables the Contiguous Memory Allocator which allows drivers
	  to allocate big physically-contiguous blocks of memory for use with
	  hardware components that do not support I/O map nor scatter-gather.

	  You can disable CMA by specifying "cma=0" on the kernel's command
	  line.

	  For more information see <include/linux/dma-contiguous.h>.
	  If unsure, say "n".



>
> ----
> Thang
>
> On Wed, Mar 30, 2016 at 8:09 PM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>>
>> Hi,
>>
>> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>>> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>>>
>>> Add 64-bit DMA operation support to the USB DWC3 driver.
>>> First attempt to set the coherent DMA mask for 64-bit DMA.
>>> If that failed, attempt again with 32-bit DMA.
>>>
>>> Changes from v2:
>>>       - None.
>>>
>>> Changes from v1:
>>>       - Remove WARN_ON if dma_mask is NULL
>>
>> these changes lines should be between the tearline (---) and diffstat
>> below.
>>
>>> Signed-off-by: Thang Q. Nguyen <tqnguyen@apm.com>
>>> ---
>>>  drivers/usb/dwc3/core.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index de5e01f..2479c24 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -831,6 +831,21 @@ static int dwc3_probe(struct platform_device *pdev)
>>>       dwc->mem = mem;
>>>       dwc->dev = dev;
>>>
>>> +     /* Try to set 64-bit DMA first */
>>> +     if (!pdev->dev.dma_mask)
>>> +             /* Platform did not initialize dma_mask */
>>> +             ret = dma_coerce_mask_and_coherent(&pdev->dev,
>>> +                                                DMA_BIT_MASK(64));
>>> +     else
>>> +             ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>> +
>>> +     /* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
>>> +     if (ret) {
>>> +             ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>
>> Also, why is it so that you need this now ? glue layers are copying dma
>> mask from parent device and that should be set properly. This really
>> shouldn't be necessary in dwc3-core; it would mean that glue layer
>> didn't set this device up properly.
>>
>> --
>> balbi
>
>
>
> -- 
>
> Thang Q. Nguyen    | Staff SW Eng.
>
> C: +849.7684.7606 | O: +848.3770.0640
>
> F: +848.3770.0641  | tqnguyen@apm.com

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Thang Q. Nguyen]

From: "Thang Q. Nguyen" <tqnguyen@apm.com>

The xhci-hcd child node needs to inherit archdata attribute to use
dma_ops functions and attributes. This patch enables the USB DWC3
driver to pass archdata attributes to its xhci-hcd child node.

Changes from v2:
	- None

Changes from v1:
	- None

Signed-off-by: Thang Q. Nguyen <tqnguyen@apm.com>
---

 drivers/usb/dwc3/host.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..661fbae 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -37,6 +37,7 @@ int dwc3_host_init(struct dwc3 *dwc)
 	xhci->dev.parent	= dwc->dev;
 	xhci->dev.dma_mask	= dwc->dev->dma_mask;
 	xhci->dev.dma_parms	= dwc->dev->dma_parms;
+	xhci->dev.archdata      = dwc->dev->archdata;
 
 	dwc->xhci = xhci;
 
-- 
2.2.0

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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

"Thang Q. Nguyen" <tqnguyen@apm.com> writes:

> [ text/plain ]
> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>
> The xhci-hcd child node needs to inherit archdata attribute to use
> dma_ops functions and attributes. This patch enables the USB DWC3
> driver to pass archdata attributes to its xhci-hcd child node.
>
> Changes from v2:
> 	- None
>
> Changes from v1:
> 	- None

changes should be between tearline and diffstat.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Grygorii Strashko]

On 03/30/2016 04:10 PM, Felipe Balbi wrote:
> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
> 
>> [ text/plain ]
>> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>>
>> The xhci-hcd child node needs to inherit archdata attribute to use
>> dma_ops functions and attributes. This patch enables the USB DWC3
>> driver to pass archdata attributes to its xhci-hcd child node.
>>
>> Changes from v2:
>> 	- None
>>
>> Changes from v1:
>> 	- None
> 
> changes should be between tearline and diffstat.
> 

uh. This become a real problem :(, especially with LPAE enabled.
DMA properties need to be inherited not only here, but also in
usb_add_gadget_udc_release(). And probably in other places
 where devices are created manually - the worst case : device is created
manually but doesn't belong to any bus.

And DMA configuration must include dma_pfn_offset also!
And how about iommu staff?

FYI. Solution used for PCI
c49b8fc of/pci: Add of_pci_dma_configure() to update DMA configuration

Rejected: introduce dma_init_dev_from_parent() or smth. like this
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/378317
https://lkml.org/lkml/2014/11/4/519


-- 
regards,
-grygorii

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> [ text/plain ]
> On 03/30/2016 04:10 PM, Felipe Balbi wrote:
>> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>> 
>>> [ text/plain ]
>>> From: "Thang Q. Nguyen" <tqnguyen@apm.com>
>>>
>>> The xhci-hcd child node needs to inherit archdata attribute to use
>>> dma_ops functions and attributes. This patch enables the USB DWC3
>>> driver to pass archdata attributes to its xhci-hcd child node.
>>>
>>> Changes from v2:
>>> 	- None
>>>
>>> Changes from v1:
>>> 	- None
>> 
>> changes should be between tearline and diffstat.
>> 
>
> uh. This become a real problem :(, especially with LPAE enabled.
> DMA properties need to be inherited not only here, but also in
> usb_add_gadget_udc_release(). And probably in other places
>  where devices are created manually - the worst case : device is created
> manually but doesn't belong to any bus.
>
> And DMA configuration must include dma_pfn_offset also!
> And how about iommu staff?
>
> FYI. Solution used for PCI
> c49b8fc of/pci: Add of_pci_dma_configure() to update DMA configuration
>
> Rejected: introduce dma_init_dev_from_parent() or smth. like this
> http://permalink.gmane.org/gmane.linux.ports.arm.kernel/378317

I like this very much. Meanwhile, we need something (although, $subject
is not very good).

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Thang Q. Nguyen]

Thanks Grygorii for information.
I checked but do not see dma_init_dev_from_parent is used in
linux-next repository. Can you give me more information for what
branch I can checkout to use it for USB DWC3?

Thanks,
Thang --

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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

"Thang Q. Nguyen" <tqnguyen@apm.com> writes:
> [ text/plain ]
> Thanks Grygorii for information.
> I checked but do not see dma_init_dev_from_parent is used in
> linux-next repository. Can you give me more information for what
> branch I can checkout to use it for USB DWC3?

dma_init_dev_from_parent() is still a proposal ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Grygorii Strashko]

On 03/31/2016 11:04 AM, Felipe Balbi wrote:
> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>> [ text/plain ]
>> Thanks Grygorii for information.
>> I checked but do not see dma_init_dev_from_parent is used in
>> linux-next repository. Can you give me more information for what
>> branch I can checkout to use it for USB DWC3?
> 
> dma_init_dev_from_parent() is still a proposal ;-)
> 

Felipe,

After some experiments I came up with below fix (not common, but fixes USB
case on keystone 2). if you agree with proposed fix I'll send proper
patches to fix usb_add_gadget_udc_release() and dwc3_host_init() in the same
way.

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index c679f63..3fe1c65 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -17,6 +17,7 @@
 
 #include <linux/platform_device.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/of_device.h>
 
 #include "core.h"
 
@@ -35,8 +36,6 @@ int dwc3_host_init(struct dwc3 *dwc)
        dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
 
        xhci->dev.parent        = dwc->dev;
-       xhci->dev.dma_mask      = dwc->dev->dma_mask;
-       xhci->dev.dma_parms     = dwc->dev->dma_parms;
 
        dwc->xhci = xhci;
 
@@ -62,6 +61,12 @@ int dwc3_host_init(struct dwc3 *dwc)
        phy_create_lookup(dwc->usb3_generic_phy, "usb3-phy",
                          dev_name(&xhci->dev));
 
+       if (!dwc->dev->of_node) {
+               xhci->dev.dma_mask      = dwc->dev->dma_mask;
+               xhci->dev.dma_parms     = dwc->dev->dma_parms;
+       } else 
+               of_dma_configure(&xhci->dev, dwc->dev->of_node);
+
        ret = platform_device_add(xhci);
        if (ret) {
                dev_err(dwc->dev, "failed to register xHCI device\n");



-- 
regards,
-grygorii

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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


Hi,

Grygorii Strashko <grygorii.strashko@ti.com> writes:
> On 03/31/2016 11:04 AM, Felipe Balbi wrote:
>> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>>> [ text/plain ]
>>> Thanks Grygorii for information.
>>> I checked but do not see dma_init_dev_from_parent is used in
>>> linux-next repository. Can you give me more information for what
>>> branch I can checkout to use it for USB DWC3?
>> 
>> dma_init_dev_from_parent() is still a proposal ;-)
>> 
>
> Felipe,
>
> After some experiments I came up with below fix (not common, but fixes USB
> case on keystone 2). if you agree with proposed fix I'll send proper
> patches to fix usb_add_gadget_udc_release() and dwc3_host_init() in the same
> way.
>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index c679f63..3fe1c65 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -17,6 +17,7 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/usb/xhci_pdriver.h>
> +#include <linux/of_device.h>
>  
>  #include "core.h"
>  
> @@ -35,8 +36,6 @@ int dwc3_host_init(struct dwc3 *dwc)
>         dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
>  
>         xhci->dev.parent        = dwc->dev;
> -       xhci->dev.dma_mask      = dwc->dev->dma_mask;
> -       xhci->dev.dma_parms     = dwc->dev->dma_parms;
>  
>         dwc->xhci = xhci;
>  
> @@ -62,6 +61,12 @@ int dwc3_host_init(struct dwc3 *dwc)
>         phy_create_lookup(dwc->usb3_generic_phy, "usb3-phy",
>                           dev_name(&xhci->dev));
>  
> +       if (!dwc->dev->of_node) {
> +               xhci->dev.dma_mask      = dwc->dev->dma_mask;
> +               xhci->dev.dma_parms     = dwc->dev->dma_parms;
> +       } else 
> +               of_dma_configure(&xhci->dev, dwc->dev->of_node);

if of_dma_configure() does what you want, why don't you just stick it in
dwc3-keystone.c and let the driver continue to copy things for now ?
Something like below, perhaps ?

diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 2be268d2423d..a4bd7f16090f 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -39,8 +39,6 @@
 #define USBSS_IRQ_COREIRQ_EN	BIT(0)
 #define USBSS_IRQ_COREIRQ_CLR	BIT(0)
 
-static u64 kdwc3_dma_mask;
-
 struct dwc3_keystone {
 	struct device			*dev;
 	struct clk			*clk;
@@ -108,9 +106,7 @@ static int kdwc3_probe(struct platform_device *pdev)
 	if (IS_ERR(kdwc->usbss))
 		return PTR_ERR(kdwc->usbss);
 
-	kdwc3_dma_mask = dma_get_mask(dev);
-	dev->dma_mask = &kdwc3_dma_mask;
-
+	of_dma_configure(&kdwc->dev, node);
 	kdwc->clk = devm_clk_get(kdwc->dev, "usb");
 
 	error = clk_prepare_enable(kdwc->clk);


-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Grygorii Strashko]

On 04/01/2016 10:58 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>> On 03/31/2016 11:04 AM, Felipe Balbi wrote:
>>> "Thang Q. Nguyen" <tqnguyen@apm.com> writes:
>>>> [ text/plain ]
>>>> Thanks Grygorii for information.
>>>> I checked but do not see dma_init_dev_from_parent is used in
>>>> linux-next repository. Can you give me more information for what
>>>> branch I can checkout to use it for USB DWC3?
>>>
>>> dma_init_dev_from_parent() is still a proposal ;-)
>>>
>>
>> Felipe,
>>
>> After some experiments I came up with below fix (not common, but fixes USB
>> case on keystone 2). if you agree with proposed fix I'll send proper
>> patches to fix usb_add_gadget_udc_release() and dwc3_host_init() in the same
>> way.
>>
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index c679f63..3fe1c65 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -17,6 +17,7 @@
>>   
>>   #include <linux/platform_device.h>
>>   #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/of_device.h>
>>   
>>   #include "core.h"
>>   
>> @@ -35,8 +36,6 @@ int dwc3_host_init(struct dwc3 *dwc)
>>          dma_set_coherent_mask(&xhci->dev, dwc->dev->coherent_dma_mask);
>>   
>>          xhci->dev.parent        = dwc->dev;
>> -       xhci->dev.dma_mask      = dwc->dev->dma_mask;
>> -       xhci->dev.dma_parms     = dwc->dev->dma_parms;
>>   
>>          dwc->xhci = xhci;
>>   
>> @@ -62,6 +61,12 @@ int dwc3_host_init(struct dwc3 *dwc)
>>          phy_create_lookup(dwc->usb3_generic_phy, "usb3-phy",
>>                            dev_name(&xhci->dev));
>>   
>> +       if (!dwc->dev->of_node) {
>> +               xhci->dev.dma_mask      = dwc->dev->dma_mask;
>> +               xhci->dev.dma_parms     = dwc->dev->dma_parms;
>> +       } else
>> +               of_dma_configure(&xhci->dev, dwc->dev->of_node);
> 
> if of_dma_configure() does what you want, why don't you just stick it in
> dwc3-keystone.c and let the driver continue to copy things for now ?
> Something like below, perhaps ?
> 

I know (and i have patch to fix that which I'm going to send) that DMA config 
 in dwc3-keystone.c is not correct and we are good till now just 
because dwc3_keystone is not used for DMA operations directly.

Now about xhci and friends:
dwc3_keystone *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
|- dwc3 *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
   |- [1] *creates* xhci dev manually : DMA configuration copied manually in dwc3_host_init()
   |- [2] *creates* usb_gadget dev manually: DMA configuration copied manually in usb_add_gadget_udc_release()
   |- *creates* usb_udc dev manually : not used for DMA operations directly (as I've checked)

Now cases [1] & [2] introduces failures, because DMA configuration is not complete for
these devices.

I can confirm that if I fix [1] & [2] as above USB Device/Dual modes will start 
working on K2E.

-- 
regards,
-grygorii

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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


Hi,

Grygorii Strashko <grygorii.strashko@ti.com> writes:
>> if of_dma_configure() does what you want, why don't you just stick it in
>> dwc3-keystone.c and let the driver continue to copy things for now ?
>> Something like below, perhaps ?
>> 
>
> I know (and i have patch to fix that which I'm going to send) that DMA config 
>  in dwc3-keystone.c is not correct and we are good till now just 
> because dwc3_keystone is not used for DMA operations directly.
>
> Now about xhci and friends:
> dwc3_keystone *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
> |- dwc3 *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
>    |- [1] *creates* xhci dev manually : DMA configuration copied manually in dwc3_host_init()
>    |- [2] *creates* usb_gadget dev manually: DMA configuration copied manually in usb_add_gadget_udc_release()
>    |- *creates* usb_udc dev manually : not used for DMA operations directly (as I've checked)
>
> Now cases [1] & [2] introduces failures, because DMA configuration is not complete for
> these devices.

right, then we just copy whatever's missing, right ? Until there's a
generic way of copying these bits, I want to avoid introducing any of_*
specific methodologies and prefer to have the manual copy.

> I can confirm that if I fix [1] & [2] as above USB Device/Dual modes will start 
> working on K2E.

cool, I'd be happy to take both patches ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Grygorii Strashko]

On 04/01/2016 01:20 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>> if of_dma_configure() does what you want, why don't you just stick it in
>>> dwc3-keystone.c and let the driver continue to copy things for now ?
>>> Something like below, perhaps ?
>>>
>>
>> I know (and i have patch to fix that which I'm going to send) that DMA config
>>   in dwc3-keystone.c is not correct and we are good till now just
>> because dwc3_keystone is not used for DMA operations directly.
>>
>> Now about xhci and friends:
>> dwc3_keystone *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
>> |- dwc3 *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
>>     |- [1] *creates* xhci dev manually : DMA configuration copied manually in dwc3_host_init()
>>     |- [2] *creates* usb_gadget dev manually: DMA configuration copied manually in usb_add_gadget_udc_release()
>>     |- *creates* usb_udc dev manually : not used for DMA operations directly (as I've checked)
>>
>> Now cases [1] & [2] introduces failures, because DMA configuration is not complete for
>> these devices.
> 
> right, then we just copy whatever's missing, right ? Until there's a
> generic way of copying these bits, I want to avoid introducing any of_*
> specific methodologies and prefer to have the manual copy.

Sry, I've found no other way (right now) to fix it, except by using of_dma_configure()
which will do all work in DT case (including calling of arch specific callbacks).
[it might be unsafe to just copy archdata, for example, as it might(will for arm)
 contain pointers] 

> 
>> I can confirm that if I fix [1] & [2] as above USB Device/Dual modes will start
>> working on K2E.

Above is for 4.1 kernel

> 
> cool, I'd be happy to take both patches ;-)
> 

ok. And seems gadget case is fixed already
commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Date:   Mon Jul 13 18:10:05 2015 +0900

    usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
    
    The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
    instead of "&gadget->dev" in the first argument because the parent has
    a udc controller's device pointer.
    Otherwise, iommu functions are not called in ARM environment.
    
    Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
    Signed-off-by: Felipe Balbi <balbi@ti.com>

Above actually means that DMA configuration code can be dropped from
usb_add_gadget_udc_release() completely. Right?:

diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index 4151597..e4e70e1 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -371,12 +371,6 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
        INIT_WORK(&gadget->work, usb_gadget_state_work);
        gadget->dev.parent = parent;
 
-#ifdef CONFIG_HAS_DMA
-       dma_set_coherent_mask(&gadget->dev, parent->coherent_dma_mask);
-       gadget->dev.dma_parms = parent->dma_parms;
-       gadget->dev.dma_mask = parent->dma_mask;
-#endif
-
        if (release)
                gadget->dev.release = release;
        else


-- 
regards,
-grygorii

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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


Hi,

Grygorii Strashko <grygorii.strashko@ti.com> writes:
> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>>> if of_dma_configure() does what you want, why don't you just stick it in
>>>> dwc3-keystone.c and let the driver continue to copy things for now ?
>>>> Something like below, perhaps ?
>>>>
>>>
>>> I know (and i have patch to fix that which I'm going to send) that DMA config
>>>   in dwc3-keystone.c is not correct and we are good till now just
>>> because dwc3_keystone is not used for DMA operations directly.
>>>
>>> Now about xhci and friends:
>>> dwc3_keystone *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
>>> |- dwc3 *is created* from DT : of_platform_device_create() -> of_platform_device_create_pdata() -> of_dma_configure()
>>>     |- [1] *creates* xhci dev manually : DMA configuration copied manually in dwc3_host_init()
>>>     |- [2] *creates* usb_gadget dev manually: DMA configuration copied manually in usb_add_gadget_udc_release()
>>>     |- *creates* usb_udc dev manually : not used for DMA operations directly (as I've checked)
>>>
>>> Now cases [1] & [2] introduces failures, because DMA configuration is not complete for
>>> these devices.
>> 
>> right, then we just copy whatever's missing, right ? Until there's a
>> generic way of copying these bits, I want to avoid introducing any of_*
>> specific methodologies and prefer to have the manual copy.
>
> Sry, I've found no other way (right now) to fix it, except by using of_dma_configure()
> which will do all work in DT case (including calling of arch specific callbacks).
> [it might be unsafe to just copy archdata, for example, as it might(will for arm)
>  contain pointers] 
>
>> 
>>> I can confirm that if I fix [1] & [2] as above USB Device/Dual modes will start
>>> working on K2E.
>
> Above is for 4.1 kernel
>
>> 
>> cool, I'd be happy to take both patches ;-)
>> 
>
> ok. And seems gadget case is fixed already
> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
> Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Date:   Mon Jul 13 18:10:05 2015 +0900
>
>     usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
>     
>     The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
>     instead of "&gadget->dev" in the first argument because the parent has
>     a udc controller's device pointer.
>     Otherwise, iommu functions are not called in ARM environment.
>     
>     Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>     Signed-off-by: Felipe Balbi <balbi@ti.com>
>
> Above actually means that DMA configuration code can be dropped from
> usb_add_gadget_udc_release() completely. Right?:

true, but now I'm not sure what's better: copy all necessary bits from
parent or just pass the parent device to all DMA API.

Anybody to shed a light here ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[santosh shilimkar]

+Arnd, RMK,

On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:

[...]

>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>> Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> Date:   Mon Jul 13 18:10:05 2015 +0900
>>
>>      usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
>>
>>      The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
>>      instead of "&gadget->dev" in the first argument because the parent has
>>      a udc controller's device pointer.
>>      Otherwise, iommu functions are not called in ARM environment.
>>
>>      Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>      Signed-off-by: Felipe Balbi <balbi@ti.com>
>>
>> Above actually means that DMA configuration code can be dropped from
>> usb_add_gadget_udc_release() completely. Right?:
>
> true, but now I'm not sure what's better: copy all necessary bits from
> parent or just pass the parent device to all DMA API.
>
> Anybody to shed a light here ?
>
The expectation is drivers should pass the proper dev pointers and let 
core DMA code deal with it since it knows the per device dma properties.
RMK did massive series of patches to fix many drivers which were not
adhering to dma APIs.

Regrds,
Santosh

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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

santosh shilimkar <santosh.shilimkar@oracle.com> writes:
> +Arnd, RMK,
>
> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>
> [...]
>
>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>> Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> Date:   Mon Jul 13 18:10:05 2015 +0900
>>>
>>>      usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
>>>
>>>      The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
>>>      instead of "&gadget->dev" in the first argument because the parent has
>>>      a udc controller's device pointer.
>>>      Otherwise, iommu functions are not called in ARM environment.
>>>
>>>      Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>      Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>
>>> Above actually means that DMA configuration code can be dropped from
>>> usb_add_gadget_udc_release() completely. Right?:
>>
>> true, but now I'm not sure what's better: copy all necessary bits from
>> parent or just pass the parent device to all DMA API.
>>
>> Anybody to shed a light here ?
>>
> The expectation is drivers should pass the proper dev pointers and let 
> core DMA code deal with it since it knows the per device dma properties.

okay, so how do you get proper DMA pointers with something like this:

	kdwc3_dma_mask = dma_get_mask(dev);
	dev->dma_mask = &kdwc3_dma_mask;

This doesn't anything.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[santosh shilimkar]

On 4/3/2016 11:28 PM, Felipe Balbi wrote:
> santosh shilimkar <santosh.shilimkar@oracle.com> writes:
>> +Arnd, RMK,
>>
>> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>>
>> [...]
>>
>>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>>> Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>> Date:   Mon Jul 13 18:10:05 2015 +0900
>>>>
>>>>       usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
>>>>
>>>>       The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
>>>>       instead of "&gadget->dev" in the first argument because the parent has
>>>>       a udc controller's device pointer.
>>>>       Otherwise, iommu functions are not called in ARM environment.
>>>>
>>>>       Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>       Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>>
>>>> Above actually means that DMA configuration code can be dropped from
>>>> usb_add_gadget_udc_release() completely. Right?:
>>>
>>> true, but now I'm not sure what's better: copy all necessary bits from
>>> parent or just pass the parent device to all DMA API.
>>>
>>> Anybody to shed a light here ?
>>>
>> The expectation is drivers should pass the proper dev pointers and let
>> core DMA code deal with it since it knows the per device dma properties.
>
> okay, so how do you get proper DMA pointers with something like this:
>
> 	kdwc3_dma_mask = dma_get_mask(dev);
> 	dev->dma_mask = &kdwc3_dma_mask;
>
> This doesn't anything.
>
Drivers actually needs to touch dma_mask(s) only if the core DMA
code hasn't populated it it. I see Grygorii pointed out couple
of things already.

Reagrds,
Santosh

Re: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[Felipe Balbi]

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

santosh shilimkar <santosh.shilimkar@oracle.com> writes:
> On 4/3/2016 11:28 PM, Felipe Balbi wrote:
>> santosh shilimkar <santosh.shilimkar@oracle.com> writes:
>>> +Arnd, RMK,
>>>
>>> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>>>
>>> [...]
>>>
>>>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>>>> Author: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>> Date:   Mon Jul 13 18:10:05 2015 +0900
>>>>>
>>>>>       usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU
>>>>>
>>>>>       The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
>>>>>       instead of "&gadget->dev" in the first argument because the parent has
>>>>>       a udc controller's device pointer.
>>>>>       Otherwise, iommu functions are not called in ARM environment.
>>>>>
>>>>>       Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>>       Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>>>
>>>>> Above actually means that DMA configuration code can be dropped from
>>>>> usb_add_gadget_udc_release() completely. Right?:
>>>>
>>>> true, but now I'm not sure what's better: copy all necessary bits from
>>>> parent or just pass the parent device to all DMA API.
>>>>
>>>> Anybody to shed a light here ?
>>>>
>>> The expectation is drivers should pass the proper dev pointers and let
>>> core DMA code deal with it since it knows the per device dma properties.
>>
>> okay, so how do you get proper DMA pointers with something like this:
>>
>> 	kdwc3_dma_mask = dma_get_mask(dev);
>> 	dev->dma_mask = &kdwc3_dma_mask;
>>
>> This doesn't anything.
>>
> Drivers actually needs to touch dma_mask(s) only if the core DMA
> code hasn't populated it it.

that's fair, but when driver _do_ touch it, I'd rather it be useful ;-)

> I see Grygorii pointed out couple of things already.

yeah

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]