From: "Thang Q. Nguyen" <[email protected]>
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
From: "Thang Q. Nguyen" <[email protected]>
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 <[email protected]>
---
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
From: "Thang Q. Nguyen" <[email protected]>
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 <[email protected]>
---
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
"Thang Q. Nguyen" <[email protected]> writes:
> [ text/plain ]
> From: "Thang Q. Nguyen" <[email protected]>
>
> 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
Hi,
"Thang Q. Nguyen" <[email protected]> writes:
> From: "Thang Q. Nguyen" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On 03/30/2016 04:10 PM, Felipe Balbi wrote:
> "Thang Q. Nguyen" <[email protected]> writes:
>
>> [ text/plain ]
>> From: "Thang Q. Nguyen" <[email protected]>
>>
>> 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
Grygorii Strashko <[email protected]> writes:
> [ text/plain ]
> On 03/30/2016 04:10 PM, Felipe Balbi wrote:
>> "Thang Q. Nguyen" <[email protected]> writes:
>>
>>> [ text/plain ]
>>> From: "Thang Q. Nguyen" <[email protected]>
>>>
>>> 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
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
<[email protected]> wrote:
>
> Hi,
>
> "Thang Q. Nguyen" <[email protected]> writes:
>> From: "Thang Q. Nguyen" <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 | [email protected]
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 --
Hi,
(please don't top-post)
"Thang Q. Nguyen" <[email protected]> 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
> <[email protected]> wrote:
>>
>> Hi,
>>
>> "Thang Q. Nguyen" <[email protected]> writes:
>>> From: "Thang Q. Nguyen" <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 | [email protected]
--
balbi
"Thang Q. Nguyen" <[email protected]> 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
On 03/31/2016 11:04 AM, Felipe Balbi wrote:
> "Thang Q. Nguyen" <[email protected]> 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
Hi,
Grygorii Strashko <[email protected]> writes:
> On 03/31/2016 11:04 AM, Felipe Balbi wrote:
>> "Thang Q. Nguyen" <[email protected]> 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
On 04/01/2016 10:58 AM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <[email protected]> writes:
>> On 03/31/2016 11:04 AM, Felipe Balbi wrote:
>>> "Thang Q. Nguyen" <[email protected]> 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
Hi,
Grygorii Strashko <[email protected]> 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
On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
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
Hi,
Grygorii Strashko <[email protected]> writes:
> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Grygorii Strashko <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
>
> 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
+Arnd, RMK,
On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>
> Hi,
>
> Grygorii Strashko <[email protected]> writes:
>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
[...]
>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>> Author: Yoshihiro Shimoda <[email protected]>
>> 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 <[email protected]>
>> Signed-off-by: Felipe Balbi <[email protected]>
>>
>> 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
santosh shilimkar <[email protected]> writes:
> +Arnd, RMK,
>
> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Grygorii Strashko <[email protected]> writes:
>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>
> [...]
>
>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>> Author: Yoshihiro Shimoda <[email protected]>
>>> 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 <[email protected]>
>>> Signed-off-by: Felipe Balbi <[email protected]>
>>>
>>> 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
On 4/3/2016 11:28 PM, Felipe Balbi wrote:
> santosh shilimkar <[email protected]> writes:
>> +Arnd, RMK,
>>
>> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Grygorii Strashko <[email protected]> writes:
>>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>>
>> [...]
>>
>>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>>> Author: Yoshihiro Shimoda <[email protected]>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Felipe Balbi <[email protected]>
>>>>
>>>> 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
santosh shilimkar <[email protected]> writes:
> On 4/3/2016 11:28 PM, Felipe Balbi wrote:
>> santosh shilimkar <[email protected]> writes:
>>> +Arnd, RMK,
>>>
>>> On 4/1/2016 4:57 AM, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Grygorii Strashko <[email protected]> writes:
>>>>> On 04/01/2016 01:20 PM, Felipe Balbi wrote:
>>>
>>> [...]
>>>
>>>>> commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
>>>>> Author: Yoshihiro Shimoda <[email protected]>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Felipe Balbi <[email protected]>
>>>>>
>>>>> 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