Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Thu, Sep 22, 2016 at 12:02 AM, Sriram Dash  wrote:
>>From: Arnd Bergmann [mailto:a...@arndb.de]
>>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September
>>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>>
>>> ==
>>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>>> From: Sriram Dash 
>>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>>> from  parent dev
>>>
>>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>>
>>> Signed-off-by: Sriram Dash 
>>> ---
>>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>>  drivers/usb/host/xhci.c | 20 ++--
>>>  2 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index 6afe323..79608df 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>
>>All the changes you did to this file seem fine, I completely missed that part.
>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>>> 01d96c9..9a1ff09 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>
> Yes. Some sanity is required over this patch.

Hi Sriram,

Have you been able to do the sanity check on the patch?  I will be
good to have the formal patch submitted for integration as soon as
possible because the dwc3 USB functionality has been broken for a
while in upstream kernel.

Regards,
Leo

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Thu, Sep 22, 2016 at 12:02 AM, Sriram Dash  wrote:
>>From: Arnd Bergmann [mailto:a...@arndb.de]
>>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September
>>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>>
>>> ==
>>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>>> From: Sriram Dash 
>>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>>> from  parent dev
>>>
>>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>>
>>> Signed-off-by: Sriram Dash 
>>> ---
>>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>>  drivers/usb/host/xhci.c | 20 ++--
>>>  2 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index 6afe323..79608df 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>
>>All the changes you did to this file seem fine, I completely missed that part.
>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>>> 01d96c9..9a1ff09 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>
> Yes. Some sanity is required over this patch.

Hi Sriram,

Have you been able to do the sanity check on the patch?  I will be
good to have the formal patch submitted for integration as soon as
possible because the dwc3 USB functionality has been broken for a
while in upstream kernel.

Regards,
Leo

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: Arnd Bergmann [mailto:a...@arndb.de]
>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September
>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>
>> ==
>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>> From: Sriram Dash 
>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>> from  parent dev
>>
>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>
>> Signed-off-by: Sriram Dash 
>> ---
>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>  drivers/usb/host/xhci.c | 20 ++--
>>  2 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 6afe323..79608df 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>
>All the changes you did to this file seem fine, I completely missed that part.
>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>> 01d96c9..9a1ff09 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c

Yes. Some sanity is required over this patch.

>> @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
>> static int xhci_setup_msi(struct xhci_hcd *xhci)  {
>>  int ret;
>> -struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
>> +struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
>>
>>  ret = pci_enable_msi(pdev);
>>  if (ret) {
>
>This one is interesting as I stumbled over some code comment mentioning that 
>for
>dwc3-pci, we don't support MSI. I think with this change, we /can/ actually 
>support
>MSI, but this could be a separate patch and we'd have to test it on dwc3-pci
>hardware. Same for most of this file.
>
>> @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
>>  struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>
>>  if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
>> -usb_disable_xhci_ports(to_pci_dev(hcd->self.controller));
>> +usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
>>
>>  spin_lock_irq(>lock);
>>  xhci_halt(xhci);
>
>This seems obviously correct, but I don't yet see why it currently works. We
>probably don't call this function on dwc3.
>
>>  #ifdef CONFIG_PM
>> @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct
>usb_device *udev)
>>   * if no devices remain.
>>   */
>>  if (xhci->quirks & XHCI_RESET_ON_RESUME)
>> -pm_runtime_put_noidle(hcd->self.controller);
>> +pm_runtime_put_noidle(hcd->self.sysdev);
>>  #endif
>>
>>  ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
>
>I suspect this one is wrong, based on what Felipe explained earlier:
>the power management should propagate down from the child to the parent
>device.
>
>Someone who understands this better than I do should look at it.
>
>> @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
>usb_device *udev)
>>   * suspend if there is a device attached.
>>   */
>>  if (xhci->quirks & XHCI_RESET_ON_RESUME)
>> -pm_runtime_get_noresume(hcd->self.controller);
>> +pm_runtime_get_noresume(hcd->self.sysdev);
>>  #endif
>>
>>
>
>Same here.
>
>> @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd)  int
>> xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)  {
>>  struct xhci_hcd *xhci;
>> -struct device   *dev = hcd->self.controller;
>> +struct device   *dev = hcd->self.sysdev;
>>  int retval;
>
>
>This one calls
>
>get_quirks(dev, xhci);
>
>not sure whether this should be called with self.controller or self.sysdev, we 
>should
>audit every one of the callers here to be sure:
>
>drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks);
>drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks);
>drivers/usb/host/xhci-plat.c:   return xhci_gen_setup(hcd, xhci_plat_quirks);
>drivers/usb/host/xhci-rcar.c:* xhci_gen_setup().
>drivers/usb/host/xhci-tegra.c:  return xhci_gen_setup(hcd, tegra_xhci_quirks);
>
>   Arnd

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: Arnd Bergmann [mailto:a...@arndb.de]
>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September
>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>
>> ==
>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>> From: Sriram Dash 
>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>> from  parent dev
>>
>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>
>> Signed-off-by: Sriram Dash 
>> ---
>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>  drivers/usb/host/xhci.c | 20 ++--
>>  2 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 6afe323..79608df 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>
>All the changes you did to this file seem fine, I completely missed that part.
>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>> 01d96c9..9a1ff09 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c

Yes. Some sanity is required over this patch.

>> @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
>> static int xhci_setup_msi(struct xhci_hcd *xhci)  {
>>  int ret;
>> -struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
>> +struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
>>
>>  ret = pci_enable_msi(pdev);
>>  if (ret) {
>
>This one is interesting as I stumbled over some code comment mentioning that 
>for
>dwc3-pci, we don't support MSI. I think with this change, we /can/ actually 
>support
>MSI, but this could be a separate patch and we'd have to test it on dwc3-pci
>hardware. Same for most of this file.
>
>> @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
>>  struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>
>>  if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
>> -usb_disable_xhci_ports(to_pci_dev(hcd->self.controller));
>> +usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
>>
>>  spin_lock_irq(>lock);
>>  xhci_halt(xhci);
>
>This seems obviously correct, but I don't yet see why it currently works. We
>probably don't call this function on dwc3.
>
>>  #ifdef CONFIG_PM
>> @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct
>usb_device *udev)
>>   * if no devices remain.
>>   */
>>  if (xhci->quirks & XHCI_RESET_ON_RESUME)
>> -pm_runtime_put_noidle(hcd->self.controller);
>> +pm_runtime_put_noidle(hcd->self.sysdev);
>>  #endif
>>
>>  ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
>
>I suspect this one is wrong, based on what Felipe explained earlier:
>the power management should propagate down from the child to the parent
>device.
>
>Someone who understands this better than I do should look at it.
>
>> @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
>usb_device *udev)
>>   * suspend if there is a device attached.
>>   */
>>  if (xhci->quirks & XHCI_RESET_ON_RESUME)
>> -pm_runtime_get_noresume(hcd->self.controller);
>> +pm_runtime_get_noresume(hcd->self.sysdev);
>>  #endif
>>
>>
>
>Same here.
>
>> @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd)  int
>> xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)  {
>>  struct xhci_hcd *xhci;
>> -struct device   *dev = hcd->self.controller;
>> +struct device   *dev = hcd->self.sysdev;
>>  int retval;
>
>
>This one calls
>
>get_quirks(dev, xhci);
>
>not sure whether this should be called with self.controller or self.sysdev, we 
>should
>audit every one of the callers here to be sure:
>
>drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks);
>drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks);
>drivers/usb/host/xhci-plat.c:   return xhci_gen_setup(hcd, xhci_plat_quirks);
>drivers/usb/host/xhci-rcar.c:* xhci_gen_setup().
>drivers/usb/host/xhci-tegra.c:  return xhci_gen_setup(hcd, tegra_xhci_quirks);
>
>   Arnd

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: Arnd Bergmann [mailto:a...@arndb.de]
>On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>
>> Hello Arnd,
>>
>> We tried this patch on NXP platforms (ls2085 and ls1043) which use
>> dwc3 controller without any glue layer. On first go, this did not
>> work. But after minimal reworks mention snippet below, we are able to
>> verify that the USB was working OK.
>>
>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>  drivers/usb/host/xhci.c | 20 ++--
>>
>> -   struct device *dev = xhci_to_hcd(xhci)->self.controller;
>> +   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>>
>> We believe the patch needs little modification to work or there might
>> be chances we may have missed something. Any idea?
>
>
>I had not tried the patch, it was just sent for clarification what I meant, so 
>I'm glad
>you got it working with just minimal changes.
>
>Unfortunately, I can't tell from your lines above what exactly you changed, 
>can you
>send that again as a proper patch?
>

Sure.

==
>From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
From: Sriram Dash 
Date: Wed, 21 Sep 2016 11:39:30 +0530
Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from
 parent dev

Fixes the patch https://patchwork.kernel.org/patch/9319527/
("usb: dwc3: host: inherit dma configuration from parent dev").

Signed-off-by: Sriram Dash 
---
 drivers/usb/host/xhci-mem.c | 12 ++--
 drivers/usb/host/xhci.c | 20 ++--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..79608df 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -586,7 +586,7 @@ static void xhci_free_stream_ctx(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
struct xhci_stream_ctx *stream_ctx, dma_addr_t dma)
 {
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
 
if (size > MEDIUM_STREAM_ARRAY_SIZE)
@@ -614,7 +614,7 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct 
xhci_hcd *xhci,
unsigned int num_stream_ctxs, dma_addr_t *dma,
gfp_t mem_flags)
 {
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
 
if (size > MEDIUM_STREAM_ARRAY_SIZE)
@@ -1644,7 +1644,7 @@ void xhci_slot_copy(struct xhci_hcd *xhci,
 static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 {
int i;
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2);
 
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
@@ -1716,7 +1716,7 @@ static void scratchpad_free(struct xhci_hcd *xhci)
 {
int num_sp;
int i;
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 
if (!xhci->scratchpad)
return;
@@ -1792,7 +1792,7 @@ void xhci_free_command(struct xhci_hcd *xhci,
 
 void xhci_mem_cleanup(struct xhci_hcd *xhci)
 {
-   struct device   *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device   *dev = xhci_to_hcd(xhci)->self.sysdev;
int size;
int i, j, num_ports;
 
@@ -2334,7 +2334,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, 
gfp_t flags)
 int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 {
dma_addr_t  dma;
-   struct device   *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device   *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned intval, val2;
u64 val_64;
struct xhci_segment *seg;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 01d96c9..9a1ff09 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
 static int xhci_setup_msi(struct xhci_hcd *xhci)
 {
int ret;
-   struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
+   struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
 
ret = pci_enable_msi(pdev);
if (ret) {
@@ -257,7 +257,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
  */
 static void xhci_free_irq(struct xhci_hcd *xhci)
 {
-   struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
+   struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
int ret;
 
/* return if using legacy interrupt */
@@ -280,7 +280,7 @@ static int 

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: Arnd Bergmann [mailto:a...@arndb.de]
>On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>
>> Hello Arnd,
>>
>> We tried this patch on NXP platforms (ls2085 and ls1043) which use
>> dwc3 controller without any glue layer. On first go, this did not
>> work. But after minimal reworks mention snippet below, we are able to
>> verify that the USB was working OK.
>>
>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>  drivers/usb/host/xhci.c | 20 ++--
>>
>> -   struct device *dev = xhci_to_hcd(xhci)->self.controller;
>> +   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>>
>> We believe the patch needs little modification to work or there might
>> be chances we may have missed something. Any idea?
>
>
>I had not tried the patch, it was just sent for clarification what I meant, so 
>I'm glad
>you got it working with just minimal changes.
>
>Unfortunately, I can't tell from your lines above what exactly you changed, 
>can you
>send that again as a proper patch?
>

Sure.

==
>From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
From: Sriram Dash 
Date: Wed, 21 Sep 2016 11:39:30 +0530
Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from
 parent dev

Fixes the patch https://patchwork.kernel.org/patch/9319527/
("usb: dwc3: host: inherit dma configuration from parent dev").

Signed-off-by: Sriram Dash 
---
 drivers/usb/host/xhci-mem.c | 12 ++--
 drivers/usb/host/xhci.c | 20 ++--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..79608df 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -586,7 +586,7 @@ static void xhci_free_stream_ctx(struct xhci_hcd *xhci,
unsigned int num_stream_ctxs,
struct xhci_stream_ctx *stream_ctx, dma_addr_t dma)
 {
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
 
if (size > MEDIUM_STREAM_ARRAY_SIZE)
@@ -614,7 +614,7 @@ static struct xhci_stream_ctx *xhci_alloc_stream_ctx(struct 
xhci_hcd *xhci,
unsigned int num_stream_ctxs, dma_addr_t *dma,
gfp_t mem_flags)
 {
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
size_t size = sizeof(struct xhci_stream_ctx) * num_stream_ctxs;
 
if (size > MEDIUM_STREAM_ARRAY_SIZE)
@@ -1644,7 +1644,7 @@ void xhci_slot_copy(struct xhci_hcd *xhci,
 static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
 {
int i;
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2);
 
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
@@ -1716,7 +1716,7 @@ static void scratchpad_free(struct xhci_hcd *xhci)
 {
int num_sp;
int i;
-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 
if (!xhci->scratchpad)
return;
@@ -1792,7 +1792,7 @@ void xhci_free_command(struct xhci_hcd *xhci,
 
 void xhci_mem_cleanup(struct xhci_hcd *xhci)
 {
-   struct device   *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device   *dev = xhci_to_hcd(xhci)->self.sysdev;
int size;
int i, j, num_ports;
 
@@ -2334,7 +2334,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, 
gfp_t flags)
 int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 {
dma_addr_t  dma;
-   struct device   *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device   *dev = xhci_to_hcd(xhci)->self.sysdev;
unsigned intval, val2;
u64 val_64;
struct xhci_segment *seg;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 01d96c9..9a1ff09 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
 static int xhci_setup_msi(struct xhci_hcd *xhci)
 {
int ret;
-   struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
+   struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
 
ret = pci_enable_msi(pdev);
if (ret) {
@@ -257,7 +257,7 @@ static int xhci_setup_msi(struct xhci_hcd *xhci)
  */
 static void xhci_free_irq(struct xhci_hcd *xhci)
 {
-   struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
+   struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
int ret;
 
/* return if using legacy interrupt */
@@ -280,7 +280,7 @@ static int xhci_setup_msix(struct xhci_hcd *xhci)
 {
 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
> >From: Arnd Bergmann [mailto:a...@arndb.de]
> >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
> 
> ==
> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
> From: Sriram Dash 
> Date: Wed, 21 Sep 2016 11:39:30 +0530
> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from
>  parent dev
> 
> Fixes the patch https://patchwork.kernel.org/patch/9319527/
> ("usb: dwc3: host: inherit dma configuration from parent dev").
> 
> Signed-off-by: Sriram Dash 
> ---
>  drivers/usb/host/xhci-mem.c | 12 ++--
>  drivers/usb/host/xhci.c | 20 ++--
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..79608df 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c

All the changes you did to this file seem fine, I completely missed that part.

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 01d96c9..9a1ff09 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
>  static int xhci_setup_msi(struct xhci_hcd *xhci)
>  {
>   int ret;
> - struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> + struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
>  
>   ret = pci_enable_msi(pdev);
>   if (ret) {

This one is interesting as I stumbled over some code comment mentioning
that for dwc3-pci, we don't support MSI. I think with this change,
we /can/ actually support MSI, but this could be a separate patch
and we'd have to test it on dwc3-pci hardware. Same for most of
this file.

> @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
>   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  
>   if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
> - usb_disable_xhci_ports(to_pci_dev(hcd->self.controller));
> + usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
>  
>   spin_lock_irq(>lock);
>   xhci_halt(xhci);

This seems obviously correct, but I don't yet see why it currently
works. We probably don't call this function on dwc3.

>  #ifdef CONFIG_PM
> @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct 
> usb_device *udev)
>* if no devices remain.
>*/
>   if (xhci->quirks & XHCI_RESET_ON_RESUME)
> - pm_runtime_put_noidle(hcd->self.controller);
> + pm_runtime_put_noidle(hcd->self.sysdev);
>  #endif
>  
>   ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);

I suspect this one is wrong, based on what Felipe explained earlier:
the power management should propagate down from the child to the
parent device.

Someone who understands this better than I do should look at it.

> @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct 
> usb_device *udev)
>* suspend if there is a device attached.
>*/
>   if (xhci->quirks & XHCI_RESET_ON_RESUME)
> - pm_runtime_get_noresume(hcd->self.controller);
> + pm_runtime_get_noresume(hcd->self.sysdev);
>  #endif
>  
>  

Same here.

> @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd)
>  int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  {
>   struct xhci_hcd *xhci;
> - struct device   *dev = hcd->self.controller;
> + struct device   *dev = hcd->self.sysdev;
>   int retval;


This one calls

get_quirks(dev, xhci);

not sure whether this should be called with self.controller or
self.sysdev, we should audit every one of the callers here to
be sure:

drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks);
drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks);
drivers/usb/host/xhci-plat.c:   return xhci_gen_setup(hcd, xhci_plat_quirks);
drivers/usb/host/xhci-rcar.c:* xhci_gen_setup().
drivers/usb/host/xhci-tegra.c:  return xhci_gen_setup(hcd, tegra_xhci_quirks);

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
> >From: Arnd Bergmann [mailto:a...@arndb.de]
> >On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
> 
> ==
> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
> From: Sriram Dash 
> Date: Wed, 21 Sep 2016 11:39:30 +0530
> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration from
>  parent dev
> 
> Fixes the patch https://patchwork.kernel.org/patch/9319527/
> ("usb: dwc3: host: inherit dma configuration from parent dev").
> 
> Signed-off-by: Sriram Dash 
> ---
>  drivers/usb/host/xhci-mem.c | 12 ++--
>  drivers/usb/host/xhci.c | 20 ++--
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..79608df 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c

All the changes you did to this file seem fine, I completely missed that part.

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 01d96c9..9a1ff09 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -231,7 +231,7 @@ static int xhci_free_msi(struct xhci_hcd *xhci)
>  static int xhci_setup_msi(struct xhci_hcd *xhci)
>  {
>   int ret;
> - struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> + struct pci_dev  *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.sysdev);
>  
>   ret = pci_enable_msi(pdev);
>   if (ret) {

This one is interesting as I stumbled over some code comment mentioning
that for dwc3-pci, we don't support MSI. I think with this change,
we /can/ actually support MSI, but this could be a separate patch
and we'd have to test it on dwc3-pci hardware. Same for most of
this file.

> @@ -745,7 +745,7 @@ void xhci_shutdown(struct usb_hcd *hcd)
>   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>  
>   if (xhci->quirks & XHCI_SPURIOUS_REBOOT)
> - usb_disable_xhci_ports(to_pci_dev(hcd->self.controller));
> + usb_disable_xhci_ports(to_pci_dev(hcd->self.sysdev));
>  
>   spin_lock_irq(>lock);
>   xhci_halt(xhci);

This seems obviously correct, but I don't yet see why it currently
works. We probably don't call this function on dwc3.

>  #ifdef CONFIG_PM
> @@ -3605,7 +3605,7 @@ void xhci_free_dev(struct usb_hcd *hcd, struct 
> usb_device *udev)
>* if no devices remain.
>*/
>   if (xhci->quirks & XHCI_RESET_ON_RESUME)
> - pm_runtime_put_noidle(hcd->self.controller);
> + pm_runtime_put_noidle(hcd->self.sysdev);
>  #endif
>  
>   ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);

I suspect this one is wrong, based on what Felipe explained earlier:
the power management should propagate down from the child to the
parent device.

Someone who understands this better than I do should look at it.

> @@ -3745,7 +3745,7 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct 
> usb_device *udev)
>* suspend if there is a device attached.
>*/
>   if (xhci->quirks & XHCI_RESET_ON_RESUME)
> - pm_runtime_get_noresume(hcd->self.controller);
> + pm_runtime_get_noresume(hcd->self.sysdev);
>  #endif
>  
>  

Same here.

> @@ -4834,7 +4834,7 @@ int xhci_get_frame(struct usb_hcd *hcd)
>  int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  {
>   struct xhci_hcd *xhci;
> - struct device   *dev = hcd->self.controller;
> + struct device   *dev = hcd->self.sysdev;
>   int retval;


This one calls

get_quirks(dev, xhci);

not sure whether this should be called with self.controller or
self.sysdev, we should audit every one of the callers here to
be sure:

drivers/usb/host/xhci-mtk.c:return xhci_gen_setup(hcd, xhci_mtk_quirks);
drivers/usb/host/xhci-pci.c:retval = xhci_gen_setup(hcd, xhci_pci_quirks);
drivers/usb/host/xhci-plat.c:   return xhci_gen_setup(hcd, xhci_plat_quirks);
drivers/usb/host/xhci-rcar.c:* xhci_gen_setup().
drivers/usb/host/xhci-tegra.c:  return xhci_gen_setup(hcd, tegra_xhci_quirks);

Arnd

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org]
>On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
>>
>> Hi,
>>
>> Arnd Bergmann  writes:
>>
>> [...]
>>
>> > Regarding the DMA configuration that you mention in
>> > ci_hdrc_add_device(), I think we should replace
>> >
>> > pdev->dev.dma_mask = dev->dma_mask;
>> > pdev->dev.dma_parms = dev->dma_parms;
>> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>> >
>> > with of_dma_configure(), which has the chance to configure more than
>> > just those three, as the dma API might look into different aspects:
>> >
>> > - iommu specific configuration
>> > - cache coherency information
>> > - bus type
>> > - dma offset
>> > - dma_map_ops pointer
>> >
>> > We try to handle everything in of_dma_configure() at configuration
>> > time, and that would be the place to add anything else that we might
>> > need in the future.
>>
>> There are a couple problems with this:
>>
>> 1) won't work for PCI-based systems.
>>
>> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
>> platform (FPGA that appears like a PCI card to host PC)
>
>Right, I was specifically talking about the code in chipidea here, which I 
>think is
>never used on the PCI bus, and how the current code is broken. We can probably 
>do
>better than of_dma_configure() (see below), but it would be an improvement.
>
>> 2) not very robust solution
>>
>> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat
>> because that's not created by DT. The only reason why this works at
>> all is because of the default 32-bit dma mask thing :-) So, how is it
>> any different than copying 32-bit dma mask from parent?
>
>The idea here is that you pass in the parent of_node along with the child 
>device
>pointer, so it would behave exactly like the parent already does.
>The difference is that it also handles all the other attributes besides the 
>mask.
>
>However, to summarize the discussion so far, I agree that
>of_dma_configure() is not the solution to these problems, and I think we can do
>much better:
>
>Splitting the usb_bus->controller field into the Linux-internal device (used 
>for the
>sysfs hierarchy, for printks and for power management) and a new pointer (used 
>for
>DMA, DT enumeration and phy lookup) probably covers all that we really need.
>
>I've prototyped it below, with the dwc3, xhci and chipidea changes together 
>with
>the core changes. I've surely made mistakes there and don't expect it to work 
>out
>of the box, but this should give an idea of how I think this can all be solved 
>in the
>least invasive way.
>

Hello Arnd,

We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 
controller without any glue layer. On first go, this did not work. But after
minimal reworks mention snippet below, we are able to verify that the USB
was working OK.

 drivers/usb/host/xhci-mem.c | 12 ++--
 drivers/usb/host/xhci.c | 20 ++--

-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;

We believe the patch needs little modification to work or there might be chances
we may have missed something. Any idea?

Regards,
Sriram

>I noticed that the gadget interface already has a way to handle the DMA 
>allocation
>by device, so I added that in as well.
>
>Signed-off-by: Arnd Bergmann 
>
> drivers/usb/chipidea/core.c|  4 
> drivers/usb/chipidea/host.c|  3 ++-
> drivers/usb/chipidea/udc.c |  8 
> drivers/usb/core/buffer.c  | 12 ++--
> drivers/usb/core/hcd.c | 48 
> +--
>-
> drivers/usb/core/usb.c | 16 
> drivers/usb/dwc3/core.c| 28 +++-
> drivers/usb/dwc3/core.h|  1 +
> drivers/usb/dwc3/dwc3-exynos.c | 10 --
> drivers/usb/dwc3/dwc3-st.c |  1 -
> drivers/usb/dwc3/ep0.c |  8 
> drivers/usb/dwc3/gadget.c  | 34 +-
> drivers/usb/dwc3/host.c| 13 -
> drivers/usb/host/ehci-fsl.c|  4 ++--
> drivers/usb/host/xhci-plat.c   | 32 +---
> include/linux/usb.h|  1 +
> include/linux/usb/hcd.h|  3 +++
>
>diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
>69426e644d17..dff69837b349 100644
>--- a/drivers/usb/chipidea/core.c
>+++ b/drivers/usb/chipidea/core.c
>@@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device
>*dev,
>   }
>
>   pdev->dev.parent = dev;
>-  pdev->dev.dma_mask = dev->dma_mask;
>-  pdev->dev.dma_parms = dev->dma_parms;
>-  dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>-
>   ret = platform_device_add_resources(pdev, res, nres);
>   if (ret)
>   goto err;
>diff --git a/drivers/usb/chipidea/host.c 

RE: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Sriram Dash]

>From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org]
>On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
>>
>> Hi,
>>
>> Arnd Bergmann  writes:
>>
>> [...]
>>
>> > Regarding the DMA configuration that you mention in
>> > ci_hdrc_add_device(), I think we should replace
>> >
>> > pdev->dev.dma_mask = dev->dma_mask;
>> > pdev->dev.dma_parms = dev->dma_parms;
>> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>> >
>> > with of_dma_configure(), which has the chance to configure more than
>> > just those three, as the dma API might look into different aspects:
>> >
>> > - iommu specific configuration
>> > - cache coherency information
>> > - bus type
>> > - dma offset
>> > - dma_map_ops pointer
>> >
>> > We try to handle everything in of_dma_configure() at configuration
>> > time, and that would be the place to add anything else that we might
>> > need in the future.
>>
>> There are a couple problems with this:
>>
>> 1) won't work for PCI-based systems.
>>
>> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
>> platform (FPGA that appears like a PCI card to host PC)
>
>Right, I was specifically talking about the code in chipidea here, which I 
>think is
>never used on the PCI bus, and how the current code is broken. We can probably 
>do
>better than of_dma_configure() (see below), but it would be an improvement.
>
>> 2) not very robust solution
>>
>> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat
>> because that's not created by DT. The only reason why this works at
>> all is because of the default 32-bit dma mask thing :-) So, how is it
>> any different than copying 32-bit dma mask from parent?
>
>The idea here is that you pass in the parent of_node along with the child 
>device
>pointer, so it would behave exactly like the parent already does.
>The difference is that it also handles all the other attributes besides the 
>mask.
>
>However, to summarize the discussion so far, I agree that
>of_dma_configure() is not the solution to these problems, and I think we can do
>much better:
>
>Splitting the usb_bus->controller field into the Linux-internal device (used 
>for the
>sysfs hierarchy, for printks and for power management) and a new pointer (used 
>for
>DMA, DT enumeration and phy lookup) probably covers all that we really need.
>
>I've prototyped it below, with the dwc3, xhci and chipidea changes together 
>with
>the core changes. I've surely made mistakes there and don't expect it to work 
>out
>of the box, but this should give an idea of how I think this can all be solved 
>in the
>least invasive way.
>

Hello Arnd,

We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 
controller without any glue layer. On first go, this did not work. But after
minimal reworks mention snippet below, we are able to verify that the USB
was working OK.

 drivers/usb/host/xhci-mem.c | 12 ++--
 drivers/usb/host/xhci.c | 20 ++--

-   struct device *dev = xhci_to_hcd(xhci)->self.controller;
+   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;

We believe the patch needs little modification to work or there might be chances
we may have missed something. Any idea?

Regards,
Sriram

>I noticed that the gadget interface already has a way to handle the DMA 
>allocation
>by device, so I added that in as well.
>
>Signed-off-by: Arnd Bergmann 
>
> drivers/usb/chipidea/core.c|  4 
> drivers/usb/chipidea/host.c|  3 ++-
> drivers/usb/chipidea/udc.c |  8 
> drivers/usb/core/buffer.c  | 12 ++--
> drivers/usb/core/hcd.c | 48 
> +--
>-
> drivers/usb/core/usb.c | 16 
> drivers/usb/dwc3/core.c| 28 +++-
> drivers/usb/dwc3/core.h|  1 +
> drivers/usb/dwc3/dwc3-exynos.c | 10 --
> drivers/usb/dwc3/dwc3-st.c |  1 -
> drivers/usb/dwc3/ep0.c |  8 
> drivers/usb/dwc3/gadget.c  | 34 +-
> drivers/usb/dwc3/host.c| 13 -
> drivers/usb/host/ehci-fsl.c|  4 ++--
> drivers/usb/host/xhci-plat.c   | 32 +---
> include/linux/usb.h|  1 +
> include/linux/usb/hcd.h|  3 +++
>
>diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
>69426e644d17..dff69837b349 100644
>--- a/drivers/usb/chipidea/core.c
>+++ b/drivers/usb/chipidea/core.c
>@@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device
>*dev,
>   }
>
>   pdev->dev.parent = dev;
>-  pdev->dev.dma_mask = dev->dma_mask;
>-  pdev->dev.dma_parms = dev->dma_parms;
>-  dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>-
>   ret = platform_device_add_resources(pdev, res, nres);
>   if (ret)
>   goto err;
>diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
> 
> Hello Arnd,
> 
> We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 
> controller without any glue layer. On first go, this did not work. But after
> minimal reworks mention snippet below, we are able to verify that the USB
> was working OK.
> 
>  drivers/usb/host/xhci-mem.c | 12 ++--
>  drivers/usb/host/xhci.c | 20 ++--
> 
> -   struct device *dev = xhci_to_hcd(xhci)->self.controller;
> +   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
> 
> We believe the patch needs little modification to work or there might be 
> chances
> we may have missed something. Any idea?


I had not tried the patch, it was just sent for clarification what I
meant, so I'm glad you got it working with just minimal changes.

Unfortunately, I can't tell from your lines above what exactly you
changed, can you send that again as a proper patch?

I think I also had some minimal changes that I did myself in order
to fix a build regression I introduced.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 21, 2016 11:06:47 AM CEST Sriram Dash wrote:
> 
> Hello Arnd,
> 
> We tried this patch on NXP platforms (ls2085 and ls1043) which use dwc3 
> controller without any glue layer. On first go, this did not work. But after
> minimal reworks mention snippet below, we are able to verify that the USB
> was working OK.
> 
>  drivers/usb/host/xhci-mem.c | 12 ++--
>  drivers/usb/host/xhci.c | 20 ++--
> 
> -   struct device *dev = xhci_to_hcd(xhci)->self.controller;
> +   struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
> 
> We believe the patch needs little modification to work or there might be 
> chances
> we may have missed something. Any idea?


I had not tried the patch, it was just sent for clarification what I
meant, so I'm glad you got it working with just minimal changes.

Unfortunately, I can't tell from your lines above what exactly you
changed, can you send that again as a proper patch?

I think I also had some minimal changes that I did myself in order
to fix a build regression I introduced.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 14, 2016 5:31:36 PM CEST Lorenzo Pieralisi wrote:
> On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Robin Murphy  writes:
> > > On 07/09/16 10:55, Peter Chen wrote:
> > > [...]
> > >>> Regarding the DMA configuration that you mention in 
> > >>> ci_hdrc_add_device(),
> > >>> I think we should replace 
> > >>>
> > >>> pdev->dev.dma_mask = dev->dma_mask;
> > >>> pdev->dev.dma_parms = dev->dma_parms;
> > >>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> > >>>
> > >>> with of_dma_configure(), which has the chance to configure more than
> > >>> just those three, as the dma API might look into different aspects:
> > >>>
> > >>> - iommu specific configuration
> > >>> - cache coherency information
> > >>> - bus type
> > >>> - dma offset
> > >>> - dma_map_ops pointer
> > >>>
> > >>> We try to handle everything in of_dma_configure() at configuration
> > >>> time, and that would be the place to add anything else that we might
> > >>> need in the future.
> > >>>
> > >> 
> > >> Yes, I agree with you, but just like Felipe mentioned, we also need to
> > >> consider PCI device, can we do something like gpiod_get_index does? Are
> > >> there any similar APIs like of_dma_configure for ACPI?
> > >
> > > Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> > > abstracting away the IOMMU configuration.
> > >
> > > Robin.
> > >
> > > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html
> > 
> > not exported for drivers to use. If Lorenzo is trying to making a
> > matching API for ACPI systems, then it needs to follow what
> > of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()
> 
> That's easy enough, not sure I understand though why
> of_dma_deconfigure() is not exported then. The second question mark
> is about the dma-ranges equivalent in ACPI world; the _DMA method
> seems to be the exact equivalent but to the best of my knowledge
> it is ignored by the kernel, to really have an of_dma_configure()
> equivalent that's really necessary, unless we want to resort to
> arch specific methods (is that what x86 is currently doing ?) to
> retrieve/build the dma masks.

Please see the follow-up emails after my proposed patch: if we add
a pointer to the device that firmware knows about in the USB core layer,
there is no longer a problem to be solved and the DMA operations will
do the right thing.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 14, 2016 5:31:36 PM CEST Lorenzo Pieralisi wrote:
> On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Robin Murphy  writes:
> > > On 07/09/16 10:55, Peter Chen wrote:
> > > [...]
> > >>> Regarding the DMA configuration that you mention in 
> > >>> ci_hdrc_add_device(),
> > >>> I think we should replace 
> > >>>
> > >>> pdev->dev.dma_mask = dev->dma_mask;
> > >>> pdev->dev.dma_parms = dev->dma_parms;
> > >>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> > >>>
> > >>> with of_dma_configure(), which has the chance to configure more than
> > >>> just those three, as the dma API might look into different aspects:
> > >>>
> > >>> - iommu specific configuration
> > >>> - cache coherency information
> > >>> - bus type
> > >>> - dma offset
> > >>> - dma_map_ops pointer
> > >>>
> > >>> We try to handle everything in of_dma_configure() at configuration
> > >>> time, and that would be the place to add anything else that we might
> > >>> need in the future.
> > >>>
> > >> 
> > >> Yes, I agree with you, but just like Felipe mentioned, we also need to
> > >> consider PCI device, can we do something like gpiod_get_index does? Are
> > >> there any similar APIs like of_dma_configure for ACPI?
> > >
> > > Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> > > abstracting away the IOMMU configuration.
> > >
> > > Robin.
> > >
> > > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html
> > 
> > not exported for drivers to use. If Lorenzo is trying to making a
> > matching API for ACPI systems, then it needs to follow what
> > of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()
> 
> That's easy enough, not sure I understand though why
> of_dma_deconfigure() is not exported then. The second question mark
> is about the dma-ranges equivalent in ACPI world; the _DMA method
> seems to be the exact equivalent but to the best of my knowledge
> it is ignored by the kernel, to really have an of_dma_configure()
> equivalent that's really necessary, unless we want to resort to
> arch specific methods (is that what x86 is currently doing ?) to
> retrieve/build the dma masks.

Please see the follow-up emails after my proposed patch: if we add
a pointer to the device that firmware knows about in the USB core layer,
there is no longer a problem to be solved and the DMA operations will
do the right thing.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Lorenzo Pieralisi]

On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Robin Murphy  writes:
> > On 07/09/16 10:55, Peter Chen wrote:
> > [...]
> >>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> >>> I think we should replace 
> >>>
> >>> pdev->dev.dma_mask = dev->dma_mask;
> >>> pdev->dev.dma_parms = dev->dma_parms;
> >>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >>>
> >>> with of_dma_configure(), which has the chance to configure more than
> >>> just those three, as the dma API might look into different aspects:
> >>>
> >>> - iommu specific configuration
> >>> - cache coherency information
> >>> - bus type
> >>> - dma offset
> >>> - dma_map_ops pointer
> >>>
> >>> We try to handle everything in of_dma_configure() at configuration
> >>> time, and that would be the place to add anything else that we might
> >>> need in the future.
> >>>
> >> 
> >> Yes, I agree with you, but just like Felipe mentioned, we also need to
> >> consider PCI device, can we do something like gpiod_get_index does? Are
> >> there any similar APIs like of_dma_configure for ACPI?
> >
> > Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> > abstracting away the IOMMU configuration.
> >
> > Robin.
> >
> > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html
> 
> not exported for drivers to use. If Lorenzo is trying to making a
> matching API for ACPI systems, then it needs to follow what
> of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()

That's easy enough, not sure I understand though why
of_dma_deconfigure() is not exported then. The second question mark
is about the dma-ranges equivalent in ACPI world; the _DMA method
seems to be the exact equivalent but to the best of my knowledge
it is ignored by the kernel, to really have an of_dma_configure()
equivalent that's really necessary, unless we want to resort to
arch specific methods (is that what x86 is currently doing ?) to
retrieve/build the dma masks.

Lorenzo

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Lorenzo Pieralisi]

On Wed, Sep 07, 2016 at 01:47:22PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Robin Murphy  writes:
> > On 07/09/16 10:55, Peter Chen wrote:
> > [...]
> >>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> >>> I think we should replace 
> >>>
> >>> pdev->dev.dma_mask = dev->dma_mask;
> >>> pdev->dev.dma_parms = dev->dma_parms;
> >>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >>>
> >>> with of_dma_configure(), which has the chance to configure more than
> >>> just those three, as the dma API might look into different aspects:
> >>>
> >>> - iommu specific configuration
> >>> - cache coherency information
> >>> - bus type
> >>> - dma offset
> >>> - dma_map_ops pointer
> >>>
> >>> We try to handle everything in of_dma_configure() at configuration
> >>> time, and that would be the place to add anything else that we might
> >>> need in the future.
> >>>
> >> 
> >> Yes, I agree with you, but just like Felipe mentioned, we also need to
> >> consider PCI device, can we do something like gpiod_get_index does? Are
> >> there any similar APIs like of_dma_configure for ACPI?
> >
> > Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> > abstracting away the IOMMU configuration.
> >
> > Robin.
> >
> > [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html
> 
> not exported for drivers to use. If Lorenzo is trying to making a
> matching API for ACPI systems, then it needs to follow what
> of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()

That's easy enough, not sure I understand though why
of_dma_deconfigure() is not exported then. The second question mark
is about the dma-ranges equivalent in ACPI world; the _DMA method
seems to be the exact equivalent but to the best of my knowledge
it is ignored by the kernel, to really have an of_dma_configure()
equivalent that's really necessary, unless we want to resort to
arch specific methods (is that what x86 is currently doing ?) to
retrieve/build the dma masks.

Lorenzo

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote:
> On 09/08/2016 03:28 PM, Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >>> Arnd Bergmann  writes:
>  On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> If we do that, we have to put child devices of the dwc3 devices into
> >> the platform glue, and it also breaks those dwc3 devices that don't
> >> have a parent driver.
> >
> > Well, this is easy to fix:
> >
> > if (dwc->dev->parent) {
> > dwc->sysdev = dwc->dev->parent;
> > } else {
> > dev_info(dwc->dev, "Please provide a glue layer!\n");
> > dwc->sysdev = dwc->dev;
> > }
> 
>  I don't understand. Do you mean we should have an extra level of
>  stacking and splitting "static struct platform_driver dwc3_driver"
>  in two so instead of
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> 
>  we do this?
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
>  "xhci" (usb_bus.dev)
> >>>
> >>> no 
> >>>
> >>> If we have a parent device, use that as sysdev, otherwise use self as
> >>> sysdev.
> >>
> >> But there is often a parent device in DT, as the xhci device is
> >> attached to some internal bus that gets turned into a platform_device
> >> as well, so checking whether there is a parent will get the wrong
> >> device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index ed56bf9..fd57c0d 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > struct clk  *clk;
> > int ret;
> > int irq;
> > +   struct device *dev = >dev, *sysdev;
> >  
> > if (usb_disabled())
> > return -ENODEV;
> > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> > if (irq < 0)
> > return -ENODEV;
> >  
> > +   if (dev->parent) {
> > +   sysdev = dev->parent;
> > +   } else {
> > +   sysdev = dev;
> > +   }
> > +
> 
> Shouldn't we be more careful with that?
> 

Above code does not consider pci device case, Arnd's patch covers
all cases.

> armada-375.dtsi
> 
>   soc {
>   compatible = "marvell,armada375-mbus", "simple-bus";
> 
>   internal-regs {
>   compatible = "simple-bus";
> 
>   usb3@58000 {
>   compatible = "marvell,armada-375-xhci";
>   reg = <0x58000 0x2>,<0x5b880 0x80>;
>   interrupts = ;
>   clocks = < 16>;
>   phys = < PHY_TYPE_USB3>;
>   phy-names = "usb";
>   status = "disabled";
>   };
> 
> 
> What will be the parent dev in above case?
> 

In this case, no parent dev for above case, it will use itself as sysdev
since it has of_node at dts.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote:
> On 09/08/2016 03:28 PM, Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >>> Arnd Bergmann  writes:
>  On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> If we do that, we have to put child devices of the dwc3 devices into
> >> the platform glue, and it also breaks those dwc3 devices that don't
> >> have a parent driver.
> >
> > Well, this is easy to fix:
> >
> > if (dwc->dev->parent) {
> > dwc->sysdev = dwc->dev->parent;
> > } else {
> > dev_info(dwc->dev, "Please provide a glue layer!\n");
> > dwc->sysdev = dwc->dev;
> > }
> 
>  I don't understand. Do you mean we should have an extra level of
>  stacking and splitting "static struct platform_driver dwc3_driver"
>  in two so instead of
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> 
>  we do this?
> 
>    "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
>  "xhci" (usb_bus.dev)
> >>>
> >>> no 
> >>>
> >>> If we have a parent device, use that as sysdev, otherwise use self as
> >>> sysdev.
> >>
> >> But there is often a parent device in DT, as the xhci device is
> >> attached to some internal bus that gets turned into a platform_device
> >> as well, so checking whether there is a parent will get the wrong
> >> device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index ed56bf9..fd57c0d 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > struct clk  *clk;
> > int ret;
> > int irq;
> > +   struct device *dev = >dev, *sysdev;
> >  
> > if (usb_disabled())
> > return -ENODEV;
> > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> > if (irq < 0)
> > return -ENODEV;
> >  
> > +   if (dev->parent) {
> > +   sysdev = dev->parent;
> > +   } else {
> > +   sysdev = dev;
> > +   }
> > +
> 
> Shouldn't we be more careful with that?
> 

Above code does not consider pci device case, Arnd's patch covers
all cases.

> armada-375.dtsi
> 
>   soc {
>   compatible = "marvell,armada375-mbus", "simple-bus";
> 
>   internal-regs {
>   compatible = "simple-bus";
> 
>   usb3@58000 {
>   compatible = "marvell,armada-375-xhci";
>   reg = <0x58000 0x2>,<0x5b880 0x80>;
>   interrupts = ;
>   clocks = < 16>;
>   phys = < PHY_TYPE_USB3>;
>   phy-names = "usb";
>   status = "disabled";
>   };
> 
> 
> What will be the parent dev in above case?
> 

In this case, no parent dev for above case, it will use itself as sysdev
since it has of_node at dts.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 02:52:29PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > > Arnd Bergmann  writes:
> > > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > > If we have a parent device, use that as sysdev, otherwise use self as
> > > > sysdev.
> > > 
> > > But there is often a parent device in DT, as the xhci device is
> > > attached to some internal bus that gets turned into a platform_device
> > > as well, so checking whether there is a parent will get the wrong
> > > device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> 
> Ok, thanks. That leaves the PCI glue, right?

If pci's firmware information can only get from dwc3-pci, I was wrong.
I am almost sure your patch covers all 3 cases. dwc3->sysdev covers
dwc3 core and gadget side, hcd->self.sysdev cover host side. The only
possible improvement may be how to detect pci device.

> 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index d2e3f65..563600b 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
> > /* Did the HC die before the root hub was registered? */
> > if (HCD_DEAD(hcd))
> > usb_hc_died (hcd);  /* This time clean up */
> > -   usb_dev->dev.of_node = parent_dev->of_node;
> > +   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
> > }
> > mutex_unlock(_bus_idr_lock);
> > 
> > At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> > of_node, which is from firmware or from its parent (it is dwc3 core
> > device).
> 
> Just to make sure I understand you right:
> 
> in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
> dwc3 device, not the qcom,dwc3 device.
> 

Yes, since there is a DT node for dwc3, and firmware information is there,
that's why the original patch (Grygorii Strashko's) can work.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 02:52:29PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > > Arnd Bergmann  writes:
> > > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > > If we have a parent device, use that as sysdev, otherwise use self as
> > > > sysdev.
> > > 
> > > But there is often a parent device in DT, as the xhci device is
> > > attached to some internal bus that gets turned into a platform_device
> > > as well, so checking whether there is a parent will get the wrong
> > > device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> 
> Ok, thanks. That leaves the PCI glue, right?

If pci's firmware information can only get from dwc3-pci, I was wrong.
I am almost sure your patch covers all 3 cases. dwc3->sysdev covers
dwc3 core and gadget side, hcd->self.sysdev cover host side. The only
possible improvement may be how to detect pci device.

> 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index d2e3f65..563600b 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
> > /* Did the HC die before the root hub was registered? */
> > if (HCD_DEAD(hcd))
> > usb_hc_died (hcd);  /* This time clean up */
> > -   usb_dev->dev.of_node = parent_dev->of_node;
> > +   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
> > }
> > mutex_unlock(_bus_idr_lock);
> > 
> > At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> > of_node, which is from firmware or from its parent (it is dwc3 core
> > device).
> 
> Just to make sure I understand you right:
> 
> in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
> dwc3 device, not the qcom,dwc3 device.
> 

Yes, since there is a DT node for dwc3, and firmware information is there,
that's why the original patch (Grygorii Strashko's) can work.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 03:28 PM, Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..fd57c0d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   struct clk  *clk;
>   int ret;
>   int irq;
> + struct device *dev = >dev, *sysdev;
>  
>   if (usb_disabled())
>   return -ENODEV;
> @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   if (irq < 0)
>   return -ENODEV;
>  
> + if (dev->parent) {
> + sysdev = dev->parent;
> + } else {
> + sysdev = dev;
> + }
> +

Shouldn't we be more careful with that?

armada-375.dtsi

soc {
compatible = "marvell,armada375-mbus", "simple-bus";

internal-regs {
compatible = "simple-bus";

usb3@58000 {
compatible = "marvell,armada-375-xhci";
reg = <0x58000 0x2>,<0x5b880 0x80>;
interrupts = ;
clocks = < 16>;
phys = < PHY_TYPE_USB3>;
phy-names = "usb";
status = "disabled";
};


What will be the parent dev in above case?

-- 
regards,
-grygorii

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 03:28 PM, Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..fd57c0d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   struct clk  *clk;
>   int ret;
>   int irq;
> + struct device *dev = >dev, *sysdev;
>  
>   if (usb_disabled())
>   return -ENODEV;
> @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   if (irq < 0)
>   return -ENODEV;
>  
> + if (dev->parent) {
> + sysdev = dev->parent;
> + } else {
> + sysdev = dev;
> + }
> +

Shouldn't we be more careful with that?

armada-375.dtsi

soc {
compatible = "marvell,armada375-mbus", "simple-bus";

internal-regs {
compatible = "simple-bus";

usb3@58000 {
compatible = "marvell,armada-375-xhci";
reg = <0x58000 0x2>,<0x5b880 0x80>;
interrupts = ;
clocks = < 16>;
phys = < PHY_TYPE_USB3>;
phy-names = "usb";
status = "disabled";
};


What will be the parent dev in above case?

-- 
regards,
-grygorii

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > Arnd Bergmann  writes:
> > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > If we have a parent device, use that as sysdev, otherwise use self as
> > > sysdev.
> > 
> > But there is often a parent device in DT, as the xhci device is
> > attached to some internal bus that gets turned into a platform_device
> > as well, so checking whether there is a parent will get the wrong
> > device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.

Ok, thanks. That leaves the PCI glue, right?

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d2e3f65..563600b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
>   /* Did the HC die before the root hub was registered? */
>   if (HCD_DEAD(hcd))
>   usb_hc_died (hcd);  /* This time clean up */
> - usb_dev->dev.of_node = parent_dev->of_node;
> + usb_dev->dev.of_node = parent_dev->sysdev->of_node;
>   }
>   mutex_unlock(_bus_idr_lock);
> 
> At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> of_node, which is from firmware or from its parent (it is dwc3 core
> device).

Just to make sure I understand you right:

in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
dwc3 device, not the qcom,dwc3 device.

> > > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > > The advantage is that xhci can always use the grandparent device
> > > > as sysdev whenever it isn't probed through PCI or firmware
> > > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > > >
> > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > > device when that is created from the PCI driver and checking for that
> > > > with the device property interface instead? If it's "snps,dwc3"
> > > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > > 
> 
> For pci glue device, it is always the parent for dwc3 core device.
> In your patch, you may not need to split pci or non-pci, just using
> if (dev->parent).

Here we have the pci-dwc3 -> dwc3 -> xhci hierarchy, and we want
sysdev to point to pci-dwc3, not dwc3!

The point is that the pci_dev is where we have the dma settings
and (optionally) additional DT or ACPI data for that device.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 8:28:10 PM CEST Peter Chen wrote:
> On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > > Arnd Bergmann  writes:
> > > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > > If we have a parent device, use that as sysdev, otherwise use self as
> > > sysdev.
> > 
> > But there is often a parent device in DT, as the xhci device is
> > attached to some internal bus that gets turned into a platform_device
> > as well, so checking whether there is a parent will get the wrong
> > device node.
> 
> From my point, all platform and firmware information at dwc3 are
> correct, so we don't need to change dwc3/core.c, only changing for
> xhci-plat.c is ok.

Ok, thanks. That leaves the PCI glue, right?

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d2e3f65..563600b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
>   /* Did the HC die before the root hub was registered? */
>   if (HCD_DEAD(hcd))
>   usb_hc_died (hcd);  /* This time clean up */
> - usb_dev->dev.of_node = parent_dev->of_node;
> + usb_dev->dev.of_node = parent_dev->sysdev->of_node;
>   }
>   mutex_unlock(_bus_idr_lock);
> 
> At above changes, the root hub's of_node equals to xhci-hcd sysdev's
> of_node, which is from firmware or from its parent (it is dwc3 core
> device).

Just to make sure I understand you right:

in the qcom,dwc3 -> dwc3 -> xhci hierarchy, this would be the
dwc3 device, not the qcom,dwc3 device.

> > > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > > The advantage is that xhci can always use the grandparent device
> > > > as sysdev whenever it isn't probed through PCI or firmware
> > > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > > >
> > > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > > device when that is created from the PCI driver and checking for that
> > > > with the device property interface instead? If it's "snps,dwc3"
> > > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > > 
> 
> For pci glue device, it is always the parent for dwc3 core device.
> In your patch, you may not need to split pci or non-pci, just using
> if (dev->parent).

Here we have the pci-dwc3 -> dwc3 -> xhci hierarchy, and we want
sysdev to point to pci-dwc3, not dwc3!

The point is that the pci_dev is where we have the dma settings
and (optionally) additional DT or ACPI data for that device.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> If we make dwc3.ko a library which glue calls directly then all these
> >> problems are solved but we break all current DTs and fall into the trap
> >> of having another MUSB.
> >
> > I don't see how we'd break the current DTs, I'm fairly sure we could turn 
> > dwc3
> 
> well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
> look at possible children for their own quirks and properties.
> 
> > into a library without changing the DT representation. However the parts
> > that I think would change are
> >
> > - The sysfs representation for dwc3-pci, as we would no longer have
> >   a parent-child relationship there.
> 
> that's a no-brainer, I think
> 
> > - The power management handling might need a rework, since you currently
> >   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
> >   power on and off
> 
> simple enough to do as well.
> 
> > - turning dwc3 into a library probably implies also turning xhci into
> >   a library, in part for consistency.
> 
> yeah, I considered that too. We could still do it in parts, though.
> 
> > - if we don't do the whole usb_bus->sysdev thing, we need to not just
> >   do this for dwc3 but also chipidea and maybe a couple of others.
> 
> MUSB comes to mind

Right.

> > There should not be any show-stoppers here, but it's a lot of work.
> 
> I think the biggest work will making sure people don't abuse functions
> just because they're now part of a single binary. Having them as
> separate modules helped a lot reducing the maintenance overhead. There
> was only one occasion where someone sent a glue layer which iterated
> over its children to find struct dwc3 * from child's drvdata.

This is where it get a bit philosophical ;-)

I understand that you like the strict separation that the current model
provides, and I agree that can be an advantage.

Changing the abstraction model to a set of library modules the way that
other drivers (e.g. ehci, sdhci, or libata) work to me means changing
this separation model into a different model and once we do that I would
not consider it a mistake for the platform specific driver to take
advantage of that. You still get a bit of separation since the drivers
would be in separate modules that can only access exported symbols,
and the library can still hide its data structures (to some degree).

I still think that turning xhci (and dwc3) into a library would be
an overall win, but if we solve the problems of DMA settings and
usb_device DT properties without it, I'd prefer not to fight over
that with you again ;-)

> >> If we try to pass DMA bits from parent to child, then we have the fact
> >> that DT ends up, in practice, always having a parent device.
> >
> > I don't understand what you mean here, but I agree that the various ways
> 
> well, we can't simply use what I pointed out a few emails back:
> 
> if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent
> else
>   dwc->sysdev = dwc->dev

Ok, I see.

> > we discussed for copying the DMA flags from one 'struct device' to another
> > all turned out to be flawed in at least one way.
> >
> > Do you see any problems with the patch I posted other than the ugliness
> > of the dwc3 and xhci drivers finding out which pointer to use for
> > usb_bus->sysdev? If we can solve this, we shouldn't need any new
> > of_dma_configure/acpi_dma_configure calls and we won't have to
> > turn the drivers into a library, so maybe let's try to come up with
> > better ideas for that sub-problem.
> 
> No big problems with that, no. Just the ifdef looking for a PCI bus in
> the parent. How about passing a flag via device_properties? I don't
> wanna change dwc3 core's device name with a platform_device_id because
> there probably already are scripts relying on the names to enable
> pm_runtime for example.

Sounds ok to me. Grygorii's solution might a be a bit more elegant,
but also a bit more error-prone:
If we get a platform that mistakenly sets the dma_mask pointer of
the child device, or a platform that does not set the dma_mask
pointer of the parent, things break.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:52:46 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> If we make dwc3.ko a library which glue calls directly then all these
> >> problems are solved but we break all current DTs and fall into the trap
> >> of having another MUSB.
> >
> > I don't see how we'd break the current DTs, I'm fairly sure we could turn 
> > dwc3
> 
> well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
> look at possible children for their own quirks and properties.
> 
> > into a library without changing the DT representation. However the parts
> > that I think would change are
> >
> > - The sysfs representation for dwc3-pci, as we would no longer have
> >   a parent-child relationship there.
> 
> that's a no-brainer, I think
> 
> > - The power management handling might need a rework, since you currently
> >   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
> >   power on and off
> 
> simple enough to do as well.
> 
> > - turning dwc3 into a library probably implies also turning xhci into
> >   a library, in part for consistency.
> 
> yeah, I considered that too. We could still do it in parts, though.
> 
> > - if we don't do the whole usb_bus->sysdev thing, we need to not just
> >   do this for dwc3 but also chipidea and maybe a couple of others.
> 
> MUSB comes to mind

Right.

> > There should not be any show-stoppers here, but it's a lot of work.
> 
> I think the biggest work will making sure people don't abuse functions
> just because they're now part of a single binary. Having them as
> separate modules helped a lot reducing the maintenance overhead. There
> was only one occasion where someone sent a glue layer which iterated
> over its children to find struct dwc3 * from child's drvdata.

This is where it get a bit philosophical ;-)

I understand that you like the strict separation that the current model
provides, and I agree that can be an advantage.

Changing the abstraction model to a set of library modules the way that
other drivers (e.g. ehci, sdhci, or libata) work to me means changing
this separation model into a different model and once we do that I would
not consider it a mistake for the platform specific driver to take
advantage of that. You still get a bit of separation since the drivers
would be in separate modules that can only access exported symbols,
and the library can still hide its data structures (to some degree).

I still think that turning xhci (and dwc3) into a library would be
an overall win, but if we solve the problems of DMA settings and
usb_device DT properties without it, I'd prefer not to fight over
that with you again ;-)

> >> If we try to pass DMA bits from parent to child, then we have the fact
> >> that DT ends up, in practice, always having a parent device.
> >
> > I don't understand what you mean here, but I agree that the various ways
> 
> well, we can't simply use what I pointed out a few emails back:
> 
> if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent
> else
>   dwc->sysdev = dwc->dev

Ok, I see.

> > we discussed for copying the DMA flags from one 'struct device' to another
> > all turned out to be flawed in at least one way.
> >
> > Do you see any problems with the patch I posted other than the ugliness
> > of the dwc3 and xhci drivers finding out which pointer to use for
> > usb_bus->sysdev? If we can solve this, we shouldn't need any new
> > of_dma_configure/acpi_dma_configure calls and we won't have to
> > turn the drivers into a library, so maybe let's try to come up with
> > better ideas for that sub-problem.
> 
> No big problems with that, no. Just the ifdef looking for a PCI bus in
> the parent. How about passing a flag via device_properties? I don't
> wanna change dwc3 core's device name with a platform_device_id because
> there probably already are scripts relying on the names to enable
> pm_runtime for example.

Sounds ok to me. Grygorii's solution might a be a bit more elegant,
but also a bit more error-prone:
If we get a platform that mistakenly sets the dma_mask pointer of
the child device, or a platform that does not set the dma_mask
pointer of the parent, things break.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > Arnd Bergmann  writes:
> > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > >> > If we do that, we have to put child devices of the dwc3 devices into
> > >> > the platform glue, and it also breaks those dwc3 devices that don't
> > >> > have a parent driver.
> > >> 
> > >> Well, this is easy to fix:
> > >> 
> > >> if (dwc->dev->parent) {
> > >> dwc->sysdev = dwc->dev->parent;
> > >> } else {
> > >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> > >> dwc->sysdev = dwc->dev;
> > >> }
> > >
> > > I don't understand. Do you mean we should have an extra level of
> > > stacking and splitting "static struct platform_driver dwc3_driver"
> > > in two so instead of
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> > >
> > > we do this?
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
> > > "xhci" (usb_bus.dev)
> > 
> > no 
> > 
> > If we have a parent device, use that as sysdev, otherwise use self as
> > sysdev.
> 
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

>From my point, all platform and firmware information at dwc3 are
correct, so we don't need to change dwc3/core.c, only changing for
xhci-plat.c is ok.

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ed56bf9..fd57c0d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct clk  *clk;
int ret;
int irq;
+   struct device *dev = >dev, *sysdev;
 
if (usb_disabled())
return -ENODEV;
@@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (irq < 0)
return -ENODEV;
 
+   if (dev->parent) {
+   sysdev = dev->parent;
+   } else {
+   sysdev = dev;
+   }
+
/* Try to set 64-bit DMA first */
if (WARN_ON(!pdev->dev.dma_mask))
/* Platform did not initialize dma_mask */
@@ -170,7 +177,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
return ret;
}
 
-   hcd = usb_create_hcd(driver, >dev, dev_name(>dev));
+   hcd = __usb_create_hcd(driver, sysdev, >dev,
+   dev_name(>dev), NULL);
if (!hcd)
return -ENOMEM;
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..563600b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
/* Did the HC die before the root hub was registered? */
if (HCD_DEAD(hcd))
usb_hc_died (hcd);  /* This time clean up */
-   usb_dev->dev.of_node = parent_dev->of_node;
+   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
}
mutex_unlock(_bus_idr_lock);

At above changes, the root hub's of_node equals to xhci-hcd sysdev's
of_node, which is from firmware or from its parent (it is dwc3 core
device).

> 
> > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > The advantage is that xhci can always use the grandparent device
> > > as sysdev whenever it isn't probed through PCI or firmware
> > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > >
> > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > device when that is created from the PCI driver and checking for that
> > > with the device property interface instead? If it's "snps,dwc3"
> > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > 

For pci glue device, it is always the parent for dwc3 core device.
In your patch, you may not need to split pci or non-pci, just using
if (dev->parent).

> > Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> 
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
> 
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> > Arnd Bergmann  writes:
> > > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > >> > If we do that, we have to put child devices of the dwc3 devices into
> > >> > the platform glue, and it also breaks those dwc3 devices that don't
> > >> > have a parent driver.
> > >> 
> > >> Well, this is easy to fix:
> > >> 
> > >> if (dwc->dev->parent) {
> > >> dwc->sysdev = dwc->dev->parent;
> > >> } else {
> > >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> > >> dwc->sysdev = dwc->dev;
> > >> }
> > >
> > > I don't understand. Do you mean we should have an extra level of
> > > stacking and splitting "static struct platform_driver dwc3_driver"
> > > in two so instead of
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> > >
> > > we do this?
> > >
> > >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> 
> > > "xhci" (usb_bus.dev)
> > 
> > no 
> > 
> > If we have a parent device, use that as sysdev, otherwise use self as
> > sysdev.
> 
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

>From my point, all platform and firmware information at dwc3 are
correct, so we don't need to change dwc3/core.c, only changing for
xhci-plat.c is ok.

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ed56bf9..fd57c0d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct clk  *clk;
int ret;
int irq;
+   struct device *dev = >dev, *sysdev;
 
if (usb_disabled())
return -ENODEV;
@@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (irq < 0)
return -ENODEV;
 
+   if (dev->parent) {
+   sysdev = dev->parent;
+   } else {
+   sysdev = dev;
+   }
+
/* Try to set 64-bit DMA first */
if (WARN_ON(!pdev->dev.dma_mask))
/* Platform did not initialize dma_mask */
@@ -170,7 +177,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
return ret;
}
 
-   hcd = usb_create_hcd(driver, >dev, dev_name(>dev));
+   hcd = __usb_create_hcd(driver, sysdev, >dev,
+   dev_name(>dev), NULL);
if (!hcd)
return -ENOMEM;
 
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..563600b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1118,7 +1118,7 @@ static int register_root_hub(struct usb_hcd *hcd)
/* Did the HC die before the root hub was registered? */
if (HCD_DEAD(hcd))
usb_hc_died (hcd);  /* This time clean up */
-   usb_dev->dev.of_node = parent_dev->of_node;
+   usb_dev->dev.of_node = parent_dev->sysdev->of_node;
}
mutex_unlock(_bus_idr_lock);

At above changes, the root hub's of_node equals to xhci-hcd sysdev's
of_node, which is from firmware or from its parent (it is dwc3 core
device).

> 
> > > That sounds a bit clumsy for the sake of consistency with PCI.
> > > The advantage is that xhci can always use the grandparent device
> > > as sysdev whenever it isn't probed through PCI or firmware
> > > itself, but the purpose of the dwc3-glue is otherwise questionable.
> > >
> > > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > > device when that is created from the PCI driver and checking for that
> > > with the device property interface instead? If it's "snps,dwc3"
> > > we use the device itself while for "snps,dwc3-pci", we use the parent?
> > 

For pci glue device, it is always the parent for dwc3 core device.
In your patch, you may not need to split pci or non-pci, just using
if (dev->parent).

> > Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> 
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
> 
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,
>   

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 3:02:56 PM CEST Grygorii Strashko wrote:
> dwc3: probe()
> if (!>dev->of_node)
>  legacy case - hard-code DMA props
> dwc->sysdev = >dev;

The PCI case will fall into this too, as we almost never have an
->of_node pointer for a PCI device.

Do we actually have any legacy dwc3 users in Linux that are neither DT
nor PCI based? Maybe we can just skip that.

> else
> dev = >dev;
> do {
> if (is_device_dma_capable(dev)) {
> dwc->sysdev = dev;
> break;
> }
>dev = dev->parent;
> while (dev);
> ^this cycle can be limited in depth (2 for PCI)

Right, this could work by itself and looks generic enough.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 3:02:56 PM CEST Grygorii Strashko wrote:
> dwc3: probe()
> if (!>dev->of_node)
>  legacy case - hard-code DMA props
> dwc->sysdev = >dev;

The PCI case will fall into this too, as we almost never have an
->of_node pointer for a PCI device.

Do we actually have any legacy dwc3 users in Linux that are neither DT
nor PCI based? Maybe we can just skip that.

> else
> dev = >dev;
> do {
> if (is_device_dma_capable(dev)) {
> dwc->sysdev = dev;
> break;
> }
>dev = dev->parent;
> while (dev);
> ^this cycle can be limited in depth (2 for PCI)

Right, this could work by itself and looks generic enough.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 02:00 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> oh, that makes things more interesting :-s
> 
 That sounds a bit clumsy for the sake of consistency with PCI.
 The advantage is that xhci can always use the grandparent device
 as sysdev whenever it isn't probed through PCI or firmware
 itself, but the purpose of the dwc3-glue is otherwise questionable.

 How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
 device when that is created from the PCI driver and checking for that
 with the device property interface instead? If it's "snps,dwc3"
 we use the device itself while for "snps,dwc3-pci", we use the parent?
>>>
>>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>>
>> That would be incompatible with the USB binding, as the sysdev
>> is assumed to be a USB host controller with #address-cells=<1>
>> and #size-cells=<0> in order to hold the child devices, for
>> example:
>>
>> / {
>>  omap_dwc3_1: omap_dwc3_1@4888 {
>> compatible = "ti,dwc3";
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges;
>> usb1: usb@4889 {
>> compatible = "snps,dwc3";
>> reg = <0x4889 0x17000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> interrupts = ,
>>  ,
>>  ;
>> interrupt-names = "peripheral",
>>   "host",
>>   "otg";
>> phys = <_phy1>, <_phy1>;
>> phy-names = "usb2-phy", "usb3-phy";
>>
>> hub@1 {
>> compatible = "usb5e3,608";
>> reg = <1>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> ethernet@1 {
>> compatible = "usb424,ec00";
>> mac-address = [00 11 22 33 44 55];
>> reg = <1>;
>> };
>> };
>> };
>> };
>>
>> It's also the node that contains the "phys" properties and
>> presumably other properties like "otg-rev", "maximum-speed"
>> etc.
>>
>> If we make the sysdev point to the parent, then we can no longer
>> look up those properties and child devices from the USB core code
>> by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s
> 
> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?
> 
> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 09/08/2016 02:00 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
>> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>>> Arnd Bergmann  writes:
 On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> If we do that, we have to put child devices of the dwc3 devices into
>> the platform glue, and it also breaks those dwc3 devices that don't
>> have a parent driver.
>
> Well, this is easy to fix:
>
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

 I don't understand. Do you mean we should have an extra level of
 stacking and splitting "static struct platform_driver dwc3_driver"
 in two so instead of

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

 we do this?

   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
 (usb_bus.dev)
>>>
>>> no 
>>>
>>> If we have a parent device, use that as sysdev, otherwise use self as
>>> sysdev.
>>
>> But there is often a parent device in DT, as the xhci device is
>> attached to some internal bus that gets turned into a platform_device
>> as well, so checking whether there is a parent will get the wrong
>> device node.
> 
> oh, that makes things more interesting :-s
> 
 That sounds a bit clumsy for the sake of consistency with PCI.
 The advantage is that xhci can always use the grandparent device
 as sysdev whenever it isn't probed through PCI or firmware
 itself, but the purpose of the dwc3-glue is otherwise questionable.

 How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
 device when that is created from the PCI driver and checking for that
 with the device property interface instead? If it's "snps,dwc3"
 we use the device itself while for "snps,dwc3-pci", we use the parent?
>>>
>>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>>
>> That would be incompatible with the USB binding, as the sysdev
>> is assumed to be a USB host controller with #address-cells=<1>
>> and #size-cells=<0> in order to hold the child devices, for
>> example:
>>
>> / {
>>  omap_dwc3_1: omap_dwc3_1@4888 {
>> compatible = "ti,dwc3";
>> #address-cells = <1>;
>> #size-cells = <1>;
>> ranges;
>> usb1: usb@4889 {
>> compatible = "snps,dwc3";
>> reg = <0x4889 0x17000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> interrupts = ,
>>  ,
>>  ;
>> interrupt-names = "peripheral",
>>   "host",
>>   "otg";
>> phys = <_phy1>, <_phy1>;
>> phy-names = "usb2-phy", "usb3-phy";
>>
>> hub@1 {
>> compatible = "usb5e3,608";
>> reg = <1>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> ethernet@1 {
>> compatible = "usb424,ec00";
>> mac-address = [00 11 22 33 44 55];
>> reg = <1>;
>> };
>> };
>> };
>> };
>>
>> It's also the node that contains the "phys" properties and
>> presumably other properties like "otg-rev", "maximum-speed"
>> etc.
>>
>> If we make the sysdev point to the parent, then we can no longer
>> look up those properties and child devices from the USB core code
>> by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s
> 
> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?
> 
> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   if 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> >> that's easy, but for DT devices, seems like it should be in of
>> >> core. Below is, clearly, not enough but should show the idea:
>> >> 
>> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> >> index fd5cfad7c403..a54610198946 100644
>> >> --- a/drivers/of/device.c
>> >> +++ b/drivers/of/device.c
>> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> >> device_node *np)
>> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
>> >> to
>> >>  * setup the correct supported mask.
>> >>  */
>> >> -   if (!dev->coherent_dma_mask)
>> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   if (!dev->coherent_dma_mask) {
>> >> +   if (!dev->parent->coherent_dma_mask)
>> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   else
>> >> +   dev->coherent_dma_mask = 
>> >> dev->parent->coherent_dma_mask;
>> >> +   }
>> >>  
>> >
>> > As the comment above that code says, the default 32-bit mask is 
>> > intentional,
>> > and you need the driver to ask for the mask it wants using
>> > dma_set_mask_and_coherent(), while the platform code should be able to use
>> > dev->of_node to figure out whether that mask is supported.
>> >
>> > Just setting the initial mask to something else based on what the parent
>> > supports will not do the right thing elsewhere.
>> 
>> oh man, it gets more and more complex. Seems like either path we take
>> will cause problems somewhere 
>> 
>> If we make dwc3.ko a library which glue calls directly then all these
>> problems are solved but we break all current DTs and fall into the trap
>> of having another MUSB.
>
> I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3

well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
look at possible children for their own quirks and properties.

> into a library without changing the DT representation. However the parts
> that I think would change are
>
> - The sysfs representation for dwc3-pci, as we would no longer have
>   a parent-child relationship there.

that's a no-brainer, I think

> - The power management handling might need a rework, since you currently
>   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
>   power on and off

simple enough to do as well.

> - turning dwc3 into a library probably implies also turning xhci into
>   a library, in part for consistency.

yeah, I considered that too. We could still do it in parts, though.

> - if we don't do the whole usb_bus->sysdev thing, we need to not just
>   do this for dwc3 but also chipidea and maybe a couple of others.

MUSB comes to mind

> There should not be any show-stoppers here, but it's a lot of work.

I think the biggest work will making sure people don't abuse functions
just because they're now part of a single binary. Having them as
separate modules helped a lot reducing the maintenance overhead. There
was only one occasion where someone sent a glue layer which iterated
over its children to find struct dwc3 * from child's drvdata.

>> If we try to pass DMA bits from parent to child, then we have the fact
>> that DT ends up, in practice, always having a parent device.
>
> I don't understand what you mean here, but I agree that the various ways

well, we can't simply use what I pointed out a few emails back:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent
else
dwc->sysdev = dwc->dev

> we discussed for copying the DMA flags from one 'struct device' to another
> all turned out to be flawed in at least one way.
>
> Do you see any problems with the patch I posted other than the ugliness
> of the dwc3 and xhci drivers finding out which pointer to use for
> usb_bus->sysdev? If we can solve this, we shouldn't need any new
> of_dma_configure/acpi_dma_configure calls and we won't have to
> turn the drivers into a library, so maybe let's try to come up with
> better ideas for that sub-problem.

No big problems with that, no. Just the ifdef looking for a PCI bus in
the parent. How about passing a flag via device_properties? I don't
wanna change dwc3 core's device name with a platform_device_id because
there probably already are scripts relying on the names to enable
pm_runtime for example.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> >> that's easy, but for DT devices, seems like it should be in of
>> >> core. Below is, clearly, not enough but should show the idea:
>> >> 
>> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> >> index fd5cfad7c403..a54610198946 100644
>> >> --- a/drivers/of/device.c
>> >> +++ b/drivers/of/device.c
>> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> >> device_node *np)
>> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
>> >> to
>> >>  * setup the correct supported mask.
>> >>  */
>> >> -   if (!dev->coherent_dma_mask)
>> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   if (!dev->coherent_dma_mask) {
>> >> +   if (!dev->parent->coherent_dma_mask)
>> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> >> +   else
>> >> +   dev->coherent_dma_mask = 
>> >> dev->parent->coherent_dma_mask;
>> >> +   }
>> >>  
>> >
>> > As the comment above that code says, the default 32-bit mask is 
>> > intentional,
>> > and you need the driver to ask for the mask it wants using
>> > dma_set_mask_and_coherent(), while the platform code should be able to use
>> > dev->of_node to figure out whether that mask is supported.
>> >
>> > Just setting the initial mask to something else based on what the parent
>> > supports will not do the right thing elsewhere.
>> 
>> oh man, it gets more and more complex. Seems like either path we take
>> will cause problems somewhere 
>> 
>> If we make dwc3.ko a library which glue calls directly then all these
>> problems are solved but we break all current DTs and fall into the trap
>> of having another MUSB.
>
> I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3

well, at a minimum dwc3-{pci,exynos,st,omap,of-simple}.c would have to
look at possible children for their own quirks and properties.

> into a library without changing the DT representation. However the parts
> that I think would change are
>
> - The sysfs representation for dwc3-pci, as we would no longer have
>   a parent-child relationship there.

that's a no-brainer, I think

> - The power management handling might need a rework, since you currently
>   rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
>   power on and off

simple enough to do as well.

> - turning dwc3 into a library probably implies also turning xhci into
>   a library, in part for consistency.

yeah, I considered that too. We could still do it in parts, though.

> - if we don't do the whole usb_bus->sysdev thing, we need to not just
>   do this for dwc3 but also chipidea and maybe a couple of others.

MUSB comes to mind

> There should not be any show-stoppers here, but it's a lot of work.

I think the biggest work will making sure people don't abuse functions
just because they're now part of a single binary. Having them as
separate modules helped a lot reducing the maintenance overhead. There
was only one occasion where someone sent a glue layer which iterated
over its children to find struct dwc3 * from child's drvdata.

>> If we try to pass DMA bits from parent to child, then we have the fact
>> that DT ends up, in practice, always having a parent device.
>
> I don't understand what you mean here, but I agree that the various ways

well, we can't simply use what I pointed out a few emails back:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent
else
dwc->sysdev = dwc->dev

> we discussed for copying the DMA flags from one 'struct device' to another
> all turned out to be flawed in at least one way.
>
> Do you see any problems with the patch I posted other than the ugliness
> of the dwc3 and xhci drivers finding out which pointer to use for
> usb_bus->sysdev? If we can solve this, we shouldn't need any new
> of_dma_configure/acpi_dma_configure calls and we won't have to
> turn the drivers into a library, so maybe let's try to come up with
> better ideas for that sub-problem.

No big problems with that, no. Just the ifdef looking for a PCI bus in
the parent. How about passing a flag via device_properties? I don't
wanna change dwc3 core's device name with a platform_device_id because
there probably already are scripts relying on the names to enable
pm_runtime for example.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:20:58 PM CEST Felipe Balbi wrote:
> >> It's quite a hack, though. I still think that inheriting DMA (or
> >> manually initializing a child with parent's DMA bits and pieces) is the
> >> best way to go. So we're back to of_dma_configure() and
> >> acpi_dma_configure(), right?
> >
> > That won't solve the problems with the DT properties or the
> > dma configuration for PCI devices though.
> 
> acpi_dma_configure() is supposed to pass along DMA bits from PCI to
> child devices, no?

I don't know, haven't looked at that code.

> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> >> that's easy, but for DT devices, seems like it should be in of
> >> core. Below is, clearly, not enough but should show the idea:
> >> 
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index fd5cfad7c403..a54610198946 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> >> device_node *np)
> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
> >> to
> >>  * setup the correct supported mask.
> >>  */
> >> -   if (!dev->coherent_dma_mask)
> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   if (!dev->coherent_dma_mask) {
> >> +   if (!dev->parent->coherent_dma_mask)
> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   else
> >> +   dev->coherent_dma_mask = 
> >> dev->parent->coherent_dma_mask;
> >> +   }
> >>  
> >
> > As the comment above that code says, the default 32-bit mask is intentional,
> > and you need the driver to ask for the mask it wants using
> > dma_set_mask_and_coherent(), while the platform code should be able to use
> > dev->of_node to figure out whether that mask is supported.
> >
> > Just setting the initial mask to something else based on what the parent
> > supports will not do the right thing elsewhere.
> 
> oh man, it gets more and more complex. Seems like either path we take
> will cause problems somewhere 
> 
> If we make dwc3.ko a library which glue calls directly then all these
> problems are solved but we break all current DTs and fall into the trap
> of having another MUSB.

I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3
into a library without changing the DT representation. However the parts
that I think would change are

- The sysfs representation for dwc3-pci, as we would no longer have
  a parent-child relationship there.
- The power management handling might need a rework, since you currently
  rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
  power on and off
- turning dwc3 into a library probably implies also turning xhci into
  a library, in part for consistency.
- if we don't do the whole usb_bus->sysdev thing, we need to not just
  do this for dwc3 but also chipidea and maybe a couple of others.

There should not be any show-stoppers here, but it's a lot of work.

> If we try to pass DMA bits from parent to child, then we have the fact
> that DT ends up, in practice, always having a parent device.

I don't understand what you mean here, but I agree that the various ways
we discussed for copying the DMA flags from one 'struct device' to another
all turned out to be flawed in at least one way.

Do you see any problems with the patch I posted other than the ugliness
of the dwc3 and xhci drivers finding out which pointer to use for
usb_bus->sysdev? If we can solve this, we shouldn't need any new
of_dma_configure/acpi_dma_configure calls and we won't have to
turn the drivers into a library, so maybe let's try to come up with
better ideas for that sub-problem.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:20:58 PM CEST Felipe Balbi wrote:
> >> It's quite a hack, though. I still think that inheriting DMA (or
> >> manually initializing a child with parent's DMA bits and pieces) is the
> >> best way to go. So we're back to of_dma_configure() and
> >> acpi_dma_configure(), right?
> >
> > That won't solve the problems with the DT properties or the
> > dma configuration for PCI devices though.
> 
> acpi_dma_configure() is supposed to pass along DMA bits from PCI to
> child devices, no?

I don't know, haven't looked at that code.

> >> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> >> that's easy, but for DT devices, seems like it should be in of
> >> core. Below is, clearly, not enough but should show the idea:
> >> 
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index fd5cfad7c403..a54610198946 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> >> device_node *np)
> >>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected 
> >> to
> >>  * setup the correct supported mask.
> >>  */
> >> -   if (!dev->coherent_dma_mask)
> >> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   if (!dev->coherent_dma_mask) {
> >> +   if (!dev->parent->coherent_dma_mask)
> >> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> >> +   else
> >> +   dev->coherent_dma_mask = 
> >> dev->parent->coherent_dma_mask;
> >> +   }
> >>  
> >
> > As the comment above that code says, the default 32-bit mask is intentional,
> > and you need the driver to ask for the mask it wants using
> > dma_set_mask_and_coherent(), while the platform code should be able to use
> > dev->of_node to figure out whether that mask is supported.
> >
> > Just setting the initial mask to something else based on what the parent
> > supports will not do the right thing elsewhere.
> 
> oh man, it gets more and more complex. Seems like either path we take
> will cause problems somewhere 
> 
> If we make dwc3.ko a library which glue calls directly then all these
> problems are solved but we break all current DTs and fall into the trap
> of having another MUSB.

I don't see how we'd break the current DTs, I'm fairly sure we could turn dwc3
into a library without changing the DT representation. However the parts
that I think would change are

- The sysfs representation for dwc3-pci, as we would no longer have
  a parent-child relationship there.
- The power management handling might need a rework, since you currently
  rely on the hierarchy between dwc3-pci, dwc3 and xhci for turning
  power on and off
- turning dwc3 into a library probably implies also turning xhci into
  a library, in part for consistency.
- if we don't do the whole usb_bus->sysdev thing, we need to not just
  do this for dwc3 but also chipidea and maybe a couple of others.

There should not be any show-stoppers here, but it's a lot of work.

> If we try to pass DMA bits from parent to child, then we have the fact
> that DT ends up, in practice, always having a parent device.

I don't understand what you mean here, but I agree that the various ways
we discussed for copying the DMA flags from one 'struct device' to another
all turned out to be flawed in at least one way.

Do you see any problems with the patch I posted other than the ugliness
of the dwc3 and xhci drivers finding out which pointer to use for
usb_bus->sysdev? If we can solve this, we shouldn't need any new
of_dma_configure/acpi_dma_configure calls and we won't have to
turn the drivers into a library, so maybe let's try to come up with
better ideas for that sub-problem.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> >> Arnd Bergmann  writes:
>> >> > That sounds a bit clumsy for the sake of consistency with PCI.
>> >> > The advantage is that xhci can always use the grandparent device
>> >> > as sysdev whenever it isn't probed through PCI or firmware
>> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >> >
>> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> >> > device when that is created from the PCI driver and checking for that
>> >> > with the device property interface instead? If it's "snps,dwc3"
>> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> >> 
>> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>> >
>> > That would be incompatible with the USB binding, as the sysdev
>> > is assumed to be a USB host controller with #address-cells=<1>
>> > and #size-cells=<0> in order to hold the child devices, for
>> > example:
>> >
>> > / {
>> >  omap_dwc3_1: omap_dwc3_1@4888 {
>> > compatible = "ti,dwc3";
>> > #address-cells = <1>;
>> > #size-cells = <1>;
>> > ranges;
>> > usb1: usb@4889 {
>> > compatible = "snps,dwc3";
>> > reg = <0x4889 0x17000>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> > interrupts = ,
>> >  ,
>> >  ;
>> > interrupt-names = "peripheral",
>> >   "host",
>> >   "otg";
>> > phys = <_phy1>, <_phy1>;
>> > phy-names = "usb2-phy", "usb3-phy";
>> >
>> > hub@1 {
>> > compatible = "usb5e3,608";
>> > reg = <1>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> >
>> > ethernet@1 {
>> > compatible = "usb424,ec00";
>> > mac-address = [00 11 22 33 44 55];
>> > reg = <1>;
>> > };
>> > };
>> > };
>> > };
>> >
>> > It's also the node that contains the "phys" properties and
>> > presumably other properties like "otg-rev", "maximum-speed"
>> > etc.
>> >
>> > If we make the sysdev point to the parent, then we can no longer
>> > look up those properties and child devices from the USB core code
>> > by looking at "sysdev->of_node".
>> 
>> this also makes things more interesting. I can't of anything other than
>> having some type of flag passed via e.g. device_properties by dwc3-pci.c
>> :-s
>
> Ok.

man, I have been skipping words rather frequently when typing lately. I
meant "I can't THINK of anything other "

>> It's quite a hack, though. I still think that inheriting DMA (or
>> manually initializing a child with parent's DMA bits and pieces) is the
>> best way to go. So we're back to of_dma_configure() and
>> acpi_dma_configure(), right?
>
> That won't solve the problems with the DT properties or the
> dma configuration for PCI devices though.

acpi_dma_configure() is supposed to pass along DMA bits from PCI to
child devices, no?

>> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> that's easy, but for DT devices, seems like it should be in of
>> core. Below is, clearly, not enough but should show the idea:
>> 
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index fd5cfad7c403..a54610198946 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> device_node *np)
>>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>>  * setup the correct supported mask.
>>  */
>> -   if (!dev->coherent_dma_mask)
>> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   if (!dev->coherent_dma_mask) {
>> +   if (!dev->parent->coherent_dma_mask)
>> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   else
>> +   dev->coherent_dma_mask = 
>> dev->parent->coherent_dma_mask;
>> +   }
>>  
>
> As the comment above that code says, the default 32-bit mask is intentional,
> and you need the driver to ask for the mask it wants using
> dma_set_mask_and_coherent(), while the platform code should be able to use
> dev->of_node to figure out whether that mask is supported.
>
> Just setting the initial mask to something else based on what the parent
> supports will not do the right thing elsewhere.

oh man, it gets more and more complex. Seems like either path we take
will 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> >> Arnd Bergmann  writes:
>> >> > That sounds a bit clumsy for the sake of consistency with PCI.
>> >> > The advantage is that xhci can always use the grandparent device
>> >> > as sysdev whenever it isn't probed through PCI or firmware
>> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >> >
>> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> >> > device when that is created from the PCI driver and checking for that
>> >> > with the device property interface instead? If it's "snps,dwc3"
>> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> >> 
>> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>> >
>> > That would be incompatible with the USB binding, as the sysdev
>> > is assumed to be a USB host controller with #address-cells=<1>
>> > and #size-cells=<0> in order to hold the child devices, for
>> > example:
>> >
>> > / {
>> >  omap_dwc3_1: omap_dwc3_1@4888 {
>> > compatible = "ti,dwc3";
>> > #address-cells = <1>;
>> > #size-cells = <1>;
>> > ranges;
>> > usb1: usb@4889 {
>> > compatible = "snps,dwc3";
>> > reg = <0x4889 0x17000>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> > interrupts = ,
>> >  ,
>> >  ;
>> > interrupt-names = "peripheral",
>> >   "host",
>> >   "otg";
>> > phys = <_phy1>, <_phy1>;
>> > phy-names = "usb2-phy", "usb3-phy";
>> >
>> > hub@1 {
>> > compatible = "usb5e3,608";
>> > reg = <1>;
>> > #address-cells = <1>;
>> > #size-cells = <0>;
>> >
>> > ethernet@1 {
>> > compatible = "usb424,ec00";
>> > mac-address = [00 11 22 33 44 55];
>> > reg = <1>;
>> > };
>> > };
>> > };
>> > };
>> >
>> > It's also the node that contains the "phys" properties and
>> > presumably other properties like "otg-rev", "maximum-speed"
>> > etc.
>> >
>> > If we make the sysdev point to the parent, then we can no longer
>> > look up those properties and child devices from the USB core code
>> > by looking at "sysdev->of_node".
>> 
>> this also makes things more interesting. I can't of anything other than
>> having some type of flag passed via e.g. device_properties by dwc3-pci.c
>> :-s
>
> Ok.

man, I have been skipping words rather frequently when typing lately. I
meant "I can't THINK of anything other "

>> It's quite a hack, though. I still think that inheriting DMA (or
>> manually initializing a child with parent's DMA bits and pieces) is the
>> best way to go. So we're back to of_dma_configure() and
>> acpi_dma_configure(), right?
>
> That won't solve the problems with the DT properties or the
> dma configuration for PCI devices though.

acpi_dma_configure() is supposed to pass along DMA bits from PCI to
child devices, no?

>> But this needs to be done before dwc3_probe() executes. For dwc3-pci
>> that's easy, but for DT devices, seems like it should be in of
>> core. Below is, clearly, not enough but should show the idea:
>> 
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index fd5cfad7c403..a54610198946 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
>> device_node *np)
>>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>>  * setup the correct supported mask.
>>  */
>> -   if (!dev->coherent_dma_mask)
>> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   if (!dev->coherent_dma_mask) {
>> +   if (!dev->parent->coherent_dma_mask)
>> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +   else
>> +   dev->coherent_dma_mask = 
>> dev->parent->coherent_dma_mask;
>> +   }
>>  
>
> As the comment above that code says, the default 32-bit mask is intentional,
> and you need the driver to ask for the mask it wants using
> dma_set_mask_and_coherent(), while the platform code should be able to use
> dev->of_node to figure out whether that mask is supported.
>
> Just setting the initial mask to something else based on what the parent
> supports will not do the right thing elsewhere.

oh man, it gets more and more complex. Seems like either path we take
will cause problems somewhere :-s

If we make dwc3.ko 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >> Arnd Bergmann  writes:
> >> > That sounds a bit clumsy for the sake of consistency with PCI.
> >> > The advantage is that xhci can always use the grandparent device
> >> > as sysdev whenever it isn't probed through PCI or firmware
> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >> >
> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> >> > device when that is created from the PCI driver and checking for that
> >> > with the device property interface instead? If it's "snps,dwc3"
> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> >> 
> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> >
> > That would be incompatible with the USB binding, as the sysdev
> > is assumed to be a USB host controller with #address-cells=<1>
> > and #size-cells=<0> in order to hold the child devices, for
> > example:
> >
> > / {
> >  omap_dwc3_1: omap_dwc3_1@4888 {
> > compatible = "ti,dwc3";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> > usb1: usb@4889 {
> > compatible = "snps,dwc3";
> > reg = <0x4889 0x17000>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > interrupts = ,
> >  ,
> >  ;
> > interrupt-names = "peripheral",
> >   "host",
> >   "otg";
> > phys = <_phy1>, <_phy1>;
> > phy-names = "usb2-phy", "usb3-phy";
> >
> > hub@1 {
> > compatible = "usb5e3,608";
> > reg = <1>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ethernet@1 {
> > compatible = "usb424,ec00";
> > mac-address = [00 11 22 33 44 55];
> > reg = <1>;
> > };
> > };
> > };
> > };
> >
> > It's also the node that contains the "phys" properties and
> > presumably other properties like "otg-rev", "maximum-speed"
> > etc.
> >
> > If we make the sysdev point to the parent, then we can no longer
> > look up those properties and child devices from the USB core code
> > by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s

Ok.

> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?

That won't solve the problems with the DT properties or the
dma configuration for PCI devices though.

> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   if (!dev->coherent_dma_mask) {
> +   if (!dev->parent->coherent_dma_mask)
> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   else
> +   dev->coherent_dma_mask = 
> dev->parent->coherent_dma_mask;
> +   }
>  

As the comment above that code says, the default 32-bit mask is intentional,
and you need the driver to ask for the mask it wants using
dma_set_mask_and_coherent(), while the platform code should be able to use
dev->of_node to figure out whether that mask is supported.

Just setting the initial mask to something else based on what the parent
supports will not do the right thing elsewhere.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 2:00:13 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >> Arnd Bergmann  writes:
> >> > That sounds a bit clumsy for the sake of consistency with PCI.
> >> > The advantage is that xhci can always use the grandparent device
> >> > as sysdev whenever it isn't probed through PCI or firmware
> >> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >> >
> >> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> >> > device when that is created from the PCI driver and checking for that
> >> > with the device property interface instead? If it's "snps,dwc3"
> >> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> >> 
> >> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
> >
> > That would be incompatible with the USB binding, as the sysdev
> > is assumed to be a USB host controller with #address-cells=<1>
> > and #size-cells=<0> in order to hold the child devices, for
> > example:
> >
> > / {
> >  omap_dwc3_1: omap_dwc3_1@4888 {
> > compatible = "ti,dwc3";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> > usb1: usb@4889 {
> > compatible = "snps,dwc3";
> > reg = <0x4889 0x17000>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > interrupts = ,
> >  ,
> >  ;
> > interrupt-names = "peripheral",
> >   "host",
> >   "otg";
> > phys = <_phy1>, <_phy1>;
> > phy-names = "usb2-phy", "usb3-phy";
> >
> > hub@1 {
> > compatible = "usb5e3,608";
> > reg = <1>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ethernet@1 {
> > compatible = "usb424,ec00";
> > mac-address = [00 11 22 33 44 55];
> > reg = <1>;
> > };
> > };
> > };
> > };
> >
> > It's also the node that contains the "phys" properties and
> > presumably other properties like "otg-rev", "maximum-speed"
> > etc.
> >
> > If we make the sysdev point to the parent, then we can no longer
> > look up those properties and child devices from the USB core code
> > by looking at "sysdev->of_node".
> 
> this also makes things more interesting. I can't of anything other than
> having some type of flag passed via e.g. device_properties by dwc3-pci.c
> :-s

Ok.

> It's quite a hack, though. I still think that inheriting DMA (or
> manually initializing a child with parent's DMA bits and pieces) is the
> best way to go. So we're back to of_dma_configure() and
> acpi_dma_configure(), right?

That won't solve the problems with the DT properties or the
dma configuration for PCI devices though.

> But this needs to be done before dwc3_probe() executes. For dwc3-pci
> that's easy, but for DT devices, seems like it should be in of
> core. Below is, clearly, not enough but should show the idea:
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad7c403..a54610198946 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
>  * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
>  * setup the correct supported mask.
>  */
> -   if (!dev->coherent_dma_mask)
> -   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   if (!dev->coherent_dma_mask) {
> +   if (!dev->parent->coherent_dma_mask)
> +   dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +   else
> +   dev->coherent_dma_mask = 
> dev->parent->coherent_dma_mask;
> +   }
>  

As the comment above that code says, the default 32-bit mask is intentional,
and you need the driver to ask for the mask it wants using
dma_set_mask_and_coherent(), while the platform code should be able to use
dev->of_node to figure out whether that mask is supported.

Just setting the initial mask to something else based on what the parent
supports will not do the right thing elsewhere.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> >> > If we do that, we have to put child devices of the dwc3 devices into
>> >> > the platform glue, and it also breaks those dwc3 devices that don't
>> >> > have a parent driver.
>> >> 
>> >> Well, this is easy to fix:
>> >> 
>> >> if (dwc->dev->parent) {
>> >> dwc->sysdev = dwc->dev->parent;
>> >> } else {
>> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> >> dwc->sysdev = dwc->dev;
>> >> }
>> >
>> > I don't understand. Do you mean we should have an extra level of
>> > stacking and splitting "static struct platform_driver dwc3_driver"
>> > in two so instead of
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>> >
>> > we do this?
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
>> > (usb_bus.dev)
>> 
>> no 
>> 
>> If we have a parent device, use that as sysdev, otherwise use self as
>> sysdev.
>
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

oh, that makes things more interesting :-s

>> > That sounds a bit clumsy for the sake of consistency with PCI.
>> > The advantage is that xhci can always use the grandparent device
>> > as sysdev whenever it isn't probed through PCI or firmware
>> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >
>> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> > device when that is created from the PCI driver and checking for that
>> > with the device property interface instead? If it's "snps,dwc3"
>> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> 
>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
>
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,
>  ;
> interrupt-names = "peripheral",
>   "host",
>   "otg";
> phys = <_phy1>, <_phy1>;
> phy-names = "usb2-phy", "usb3-phy";
>
> hub@1 {
> compatible = "usb5e3,608";
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> ethernet@1 {
> compatible = "usb424,ec00";
> mac-address = [00 11 22 33 44 55];
> reg = <1>;
> };
> };
> };
> };
>
> It's also the node that contains the "phys" properties and
> presumably other properties like "otg-rev", "maximum-speed"
> etc.
>
> If we make the sysdev point to the parent, then we can no longer
> look up those properties and child devices from the USB core code
> by looking at "sysdev->of_node".

this also makes things more interesting. I can't of anything other than
having some type of flag passed via e.g. device_properties by dwc3-pci.c
:-s

It's quite a hack, though. I still think that inheriting DMA (or
manually initializing a child with parent's DMA bits and pieces) is the
best way to go. So we're back to of_dma_configure() and
acpi_dma_configure(), right?

But this needs to be done before dwc3_probe() executes. For dwc3-pci
that's easy, but for DT devices, seems like it should be in of
core. Below is, clearly, not enough but should show the idea:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad7c403..a54610198946 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node 
*np)
 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
 * setup the correct supported mask.
 */
-   if (!dev->coherent_dma_mask)
-   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+   if (!dev->coherent_dma_mask) {
+   if (!dev->parent->coherent_dma_mask)
+   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+  

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> >> > If we do that, we have to put child devices of the dwc3 devices into
>> >> > the platform glue, and it also breaks those dwc3 devices that don't
>> >> > have a parent driver.
>> >> 
>> >> Well, this is easy to fix:
>> >> 
>> >> if (dwc->dev->parent) {
>> >> dwc->sysdev = dwc->dev->parent;
>> >> } else {
>> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> >> dwc->sysdev = dwc->dev;
>> >> }
>> >
>> > I don't understand. Do you mean we should have an extra level of
>> > stacking and splitting "static struct platform_driver dwc3_driver"
>> > in two so instead of
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>> >
>> > we do this?
>> >
>> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
>> > (usb_bus.dev)
>> 
>> no 
>> 
>> If we have a parent device, use that as sysdev, otherwise use self as
>> sysdev.
>
> But there is often a parent device in DT, as the xhci device is
> attached to some internal bus that gets turned into a platform_device
> as well, so checking whether there is a parent will get the wrong
> device node.

oh, that makes things more interesting :-s

>> > That sounds a bit clumsy for the sake of consistency with PCI.
>> > The advantage is that xhci can always use the grandparent device
>> > as sysdev whenever it isn't probed through PCI or firmware
>> > itself, but the purpose of the dwc3-glue is otherwise questionable.
>> >
>> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
>> > device when that is created from the PCI driver and checking for that
>> > with the device property interface instead? If it's "snps,dwc3"
>> > we use the device itself while for "snps,dwc3-pci", we use the parent?
>> 
>> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?
>
> That would be incompatible with the USB binding, as the sysdev
> is assumed to be a USB host controller with #address-cells=<1>
> and #size-cells=<0> in order to hold the child devices, for
> example:
>
> / {
>  omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> #address-cells = <1>;
> #size-cells = <0>;
> interrupts = ,
>  ,
>  ;
> interrupt-names = "peripheral",
>   "host",
>   "otg";
> phys = <_phy1>, <_phy1>;
> phy-names = "usb2-phy", "usb3-phy";
>
> hub@1 {
> compatible = "usb5e3,608";
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
>
> ethernet@1 {
> compatible = "usb424,ec00";
> mac-address = [00 11 22 33 44 55];
> reg = <1>;
> };
> };
> };
> };
>
> It's also the node that contains the "phys" properties and
> presumably other properties like "otg-rev", "maximum-speed"
> etc.
>
> If we make the sysdev point to the parent, then we can no longer
> look up those properties and child devices from the USB core code
> by looking at "sysdev->of_node".

this also makes things more interesting. I can't of anything other than
having some type of flag passed via e.g. device_properties by dwc3-pci.c
:-s

It's quite a hack, though. I still think that inheriting DMA (or
manually initializing a child with parent's DMA bits and pieces) is the
best way to go. So we're back to of_dma_configure() and
acpi_dma_configure(), right?

But this needs to be done before dwc3_probe() executes. For dwc3-pci
that's easy, but for DT devices, seems like it should be in of
core. Below is, clearly, not enough but should show the idea:

diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad7c403..a54610198946 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -94,8 +94,12 @@ void of_dma_configure(struct device *dev, struct device_node 
*np)
 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
 * setup the correct supported mask.
 */
-   if (!dev->coherent_dma_mask)
-   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+   if (!dev->coherent_dma_mask) {
+   if (!dev->parent->coherent_dma_mask)
+   dev->coherent_dma_mask = DMA_BIT_MASK(32);
+   else
+   

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> > If we do that, we have to put child devices of the dwc3 devices into
> >> > the platform glue, and it also breaks those dwc3 devices that don't
> >> > have a parent driver.
> >> 
> >> Well, this is easy to fix:
> >> 
> >> if (dwc->dev->parent) {
> >> dwc->sysdev = dwc->dev->parent;
> >> } else {
> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> >> dwc->sysdev = dwc->dev;
> >> }
> >
> > I don't understand. Do you mean we should have an extra level of
> > stacking and splitting "static struct platform_driver dwc3_driver"
> > in two so instead of
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> >
> > we do this?
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> > (usb_bus.dev)
> 
> no 
> 
> If we have a parent device, use that as sysdev, otherwise use self as
> sysdev.

But there is often a parent device in DT, as the xhci device is
attached to some internal bus that gets turned into a platform_device
as well, so checking whether there is a parent will get the wrong
device node.

> > That sounds a bit clumsy for the sake of consistency with PCI.
> > The advantage is that xhci can always use the grandparent device
> > as sysdev whenever it isn't probed through PCI or firmware
> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >
> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > device when that is created from the PCI driver and checking for that
> > with the device property interface instead? If it's "snps,dwc3"
> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> 
> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

That would be incompatible with the USB binding, as the sysdev
is assumed to be a USB host controller with #address-cells=<1>
and #size-cells=<0> in order to hold the child devices, for
example:

/ {
 omap_dwc3_1: omap_dwc3_1@4888 {
compatible = "ti,dwc3";
#address-cells = <1>;
#size-cells = <1>;
ranges;
usb1: usb@4889 {
compatible = "snps,dwc3";
reg = <0x4889 0x17000>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = ,
 ,
 ;
interrupt-names = "peripheral",
  "host",
  "otg";
phys = <_phy1>, <_phy1>;
phy-names = "usb2-phy", "usb3-phy";

hub@1 {
compatible = "usb5e3,608";
reg = <1>;
#address-cells = <1>;
#size-cells = <0>;

ethernet@1 {
compatible = "usb424,ec00";
mac-address = [00 11 22 33 44 55];
reg = <1>;
};
};
};
};

It's also the node that contains the "phys" properties and
presumably other properties like "otg-rev", "maximum-speed"
etc.

If we make the sysdev point to the parent, then we can no longer
look up those properties and child devices from the USB core code
by looking at "sysdev->of_node".

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >> > If we do that, we have to put child devices of the dwc3 devices into
> >> > the platform glue, and it also breaks those dwc3 devices that don't
> >> > have a parent driver.
> >> 
> >> Well, this is easy to fix:
> >> 
> >> if (dwc->dev->parent) {
> >> dwc->sysdev = dwc->dev->parent;
> >> } else {
> >> dev_info(dwc->dev, "Please provide a glue layer!\n");
> >> dwc->sysdev = dwc->dev;
> >> }
> >
> > I don't understand. Do you mean we should have an extra level of
> > stacking and splitting "static struct platform_driver dwc3_driver"
> > in two so instead of
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> >
> > we do this?
> >
> >   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> > (usb_bus.dev)
> 
> no 
> 
> If we have a parent device, use that as sysdev, otherwise use self as
> sysdev.

But there is often a parent device in DT, as the xhci device is
attached to some internal bus that gets turned into a platform_device
as well, so checking whether there is a parent will get the wrong
device node.

> > That sounds a bit clumsy for the sake of consistency with PCI.
> > The advantage is that xhci can always use the grandparent device
> > as sysdev whenever it isn't probed through PCI or firmware
> > itself, but the purpose of the dwc3-glue is otherwise questionable.
> >
> > How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> > device when that is created from the PCI driver and checking for that
> > with the device property interface instead? If it's "snps,dwc3"
> > we use the device itself while for "snps,dwc3-pci", we use the parent?
> 
> Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

That would be incompatible with the USB binding, as the sysdev
is assumed to be a USB host controller with #address-cells=<1>
and #size-cells=<0> in order to hold the child devices, for
example:

/ {
 omap_dwc3_1: omap_dwc3_1@4888 {
compatible = "ti,dwc3";
#address-cells = <1>;
#size-cells = <1>;
ranges;
usb1: usb@4889 {
compatible = "snps,dwc3";
reg = <0x4889 0x17000>;
#address-cells = <1>;
#size-cells = <0>;
interrupts = ,
 ,
 ;
interrupt-names = "peripheral",
  "host",
  "otg";
phys = <_phy1>, <_phy1>;
phy-names = "usb2-phy", "usb3-phy";

hub@1 {
compatible = "usb5e3,608";
reg = <1>;
#address-cells = <1>;
#size-cells = <0>;

ethernet@1 {
compatible = "usb424,ec00";
mac-address = [00 11 22 33 44 55];
reg = <1>;
};
};
};
};

It's also the node that contains the "phys" properties and
presumably other properties like "otg-rev", "maximum-speed"
etc.

If we make the sysdev point to the parent, then we can no longer
look up those properties and child devices from the USB core code
by looking at "sysdev->of_node".

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> > If we do that, we have to put child devices of the dwc3 devices into
>> > the platform glue, and it also breaks those dwc3 devices that don't
>> > have a parent driver.
>> 
>> Well, this is easy to fix:
>> 
>> if (dwc->dev->parent) {
>> dwc->sysdev = dwc->dev->parent;
>> } else {
>> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> dwc->sysdev = dwc->dev;
>> }
>
> I don't understand. Do you mean we should have an extra level of
> stacking and splitting "static struct platform_driver dwc3_driver"
> in two so instead of
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>
> we do this?
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> (usb_bus.dev)

no :-)

If we have a parent device, use that as sysdev, otherwise use self as
sysdev.

> That sounds a bit clumsy for the sake of consistency with PCI.
> The advantage is that xhci can always use the grandparent device
> as sysdev whenever it isn't probed through PCI or firmware
> itself, but the purpose of the dwc3-glue is otherwise questionable.
>
> How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> device when that is created from the PCI driver and checking for that
> with the device property interface instead? If it's "snps,dwc3"
> we use the device itself while for "snps,dwc3-pci", we use the parent?

Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
>> > If we do that, we have to put child devices of the dwc3 devices into
>> > the platform glue, and it also breaks those dwc3 devices that don't
>> > have a parent driver.
>> 
>> Well, this is easy to fix:
>> 
>> if (dwc->dev->parent) {
>> dwc->sysdev = dwc->dev->parent;
>> } else {
>> dev_info(dwc->dev, "Please provide a glue layer!\n");
>> dwc->sysdev = dwc->dev;
>> }
>
> I don't understand. Do you mean we should have an extra level of
> stacking and splitting "static struct platform_driver dwc3_driver"
> in two so instead of
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
>
> we do this?
>
>   "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
> (usb_bus.dev)

no :-)

If we have a parent device, use that as sysdev, otherwise use self as
sysdev.

> That sounds a bit clumsy for the sake of consistency with PCI.
> The advantage is that xhci can always use the grandparent device
> as sysdev whenever it isn't probed through PCI or firmware
> itself, but the purpose of the dwc3-glue is otherwise questionable.
>
> How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
> device when that is created from the PCI driver and checking for that
> with the device property interface instead? If it's "snps,dwc3"
> we use the device itself while for "snps,dwc3-pci", we use the parent?

Any reason why we wouldn't use e.g. dwc3-omap.dev as sysdev?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > If we do that, we have to put child devices of the dwc3 devices into
> > the platform glue, and it also breaks those dwc3 devices that don't
> > have a parent driver.
> 
> Well, this is easy to fix:
> 
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

I don't understand. Do you mean we should have an extra level of
stacking and splitting "static struct platform_driver dwc3_driver"
in two so instead of

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

we do this?

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
(usb_bus.dev)

That sounds a bit clumsy for the sake of consistency with PCI.
The advantage is that xhci can always use the grandparent device
as sysdev whenever it isn't probed through PCI or firmware
itself, but the purpose of the dwc3-glue is otherwise questionable.

How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
device when that is created from the PCI driver and checking for that
with the device property interface instead? If it's "snps,dwc3"
we use the device itself while for "snps,dwc3-pci", we use the parent?

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> > If we do that, we have to put child devices of the dwc3 devices into
> > the platform glue, and it also breaks those dwc3 devices that don't
> > have a parent driver.
> 
> Well, this is easy to fix:
> 
> if (dwc->dev->parent) {
> dwc->sysdev = dwc->dev->parent;
> } else {
> dev_info(dwc->dev, "Please provide a glue layer!\n");
> dwc->sysdev = dwc->dev;
> }

I don't understand. Do you mean we should have an extra level of
stacking and splitting "static struct platform_driver dwc3_driver"
in two so instead of

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)

we do this?

"qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" 
(usb_bus.dev)

That sounds a bit clumsy for the sake of consistency with PCI.
The advantage is that xhci can always use the grandparent device
as sysdev whenever it isn't probed through PCI or firmware
itself, but the purpose of the dwc3-glue is otherwise questionable.

How about adding a 'compatible="snps,dwc3-pci"' property for the dwc3
device when that is created from the PCI driver and checking for that
with the device property interface instead? If it's "snps,dwc3"
we use the device itself while for "snps,dwc3-pci", we use the parent?

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
>> > *dwc)
>> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>> >struct dwc3_event_buffer *evt)
>> >  {
>> > -  dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
>> > +  dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
>> 
>> how about "dma_dev" instead? Is this used for anything other than DMA?
>
> The two other things we have discussed in this thread are:
>
> - connecting of_node pointers to usb_device structures for children
>   of sysdev->of_node. Note that this can happen even for PCI devices
>   in case you have a USB ethernet device hardwired to a PCI-USB bridge
>   and put the mac address in DT.
>
> - finding the PHY device for a HCD
>
> There might be others. Basically sysdev here is what the USB core code
> can use for looking up any kind of properties provided by the firmware.

fair enough

>> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>> >dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>> >dwc->mem = mem;
>> >dwc->dev = dev;
>> > +#ifdef CONFIG_PCI
>> > +  /* TODO: or some other way of detecting this? */
>> > +  if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
>> > +  dwc->sysdev = dwc->dev->parent;
>> > +  else
>> > +#endif
>> > +  dwc->sysdev = dwc->dev;
>> 
>> Well, we can remove this ifdef and *always* use the parent. We will just
>> require that dwc3 users provide a glue layer. In that case, your check
>> becomes:
>> 
>>  if (dwc->dev->parent)
>>  dwc->sysdev = dwc->dev->parent;
>>  else
>>  dev_info(dwc->dev, "Please provide a glue layer!\n");
>
> If we do that, we have to put child devices of the dwc3 devices into
> the platform glue, and it also breaks those dwc3 devices that don't
> have a parent driver.

Well, this is easy to fix:

if (dwc->dev->parent) {
dwc->sysdev = dwc->dev->parent;
} else {
dev_info(dwc->dev, "Please provide a glue layer!\n");
dwc->sysdev = dwc->dev;
}

>> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
>> > index 89a2f712fdfe..4d7439cb8cd8 100644
>> > --- a/drivers/usb/dwc3/dwc3-st.c
>> > +++ b/drivers/usb/dwc3/dwc3-st.c
>> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
>> >if (IS_ERR(regmap))
>> >return PTR_ERR(regmap);
>> >  
>> > -  dma_set_coherent_mask(dev, dev->coherent_dma_mask);
>> 
>> so is this.
>> 
>> All in all, I like where you're going with this, we just need a matching
>> acpi_dma_configure() and problems will be sorted out.
>
> With this patch, I don't think we even need that any more, as the device
> that we use the dma-mapping API is the one that already gets configured
> correctly by the platform code for all cases: PCI, OF, ACPI and combinations
> of those.

sounds good to me

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
>> > *dwc)
>> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>> >struct dwc3_event_buffer *evt)
>> >  {
>> > -  dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
>> > +  dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
>> 
>> how about "dma_dev" instead? Is this used for anything other than DMA?
>
> The two other things we have discussed in this thread are:
>
> - connecting of_node pointers to usb_device structures for children
>   of sysdev->of_node. Note that this can happen even for PCI devices
>   in case you have a USB ethernet device hardwired to a PCI-USB bridge
>   and put the mac address in DT.
>
> - finding the PHY device for a HCD
>
> There might be others. Basically sysdev here is what the USB core code
> can use for looking up any kind of properties provided by the firmware.

fair enough

>> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>> >dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>> >dwc->mem = mem;
>> >dwc->dev = dev;
>> > +#ifdef CONFIG_PCI
>> > +  /* TODO: or some other way of detecting this? */
>> > +  if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
>> > +  dwc->sysdev = dwc->dev->parent;
>> > +  else
>> > +#endif
>> > +  dwc->sysdev = dwc->dev;
>> 
>> Well, we can remove this ifdef and *always* use the parent. We will just
>> require that dwc3 users provide a glue layer. In that case, your check
>> becomes:
>> 
>>  if (dwc->dev->parent)
>>  dwc->sysdev = dwc->dev->parent;
>>  else
>>  dev_info(dwc->dev, "Please provide a glue layer!\n");
>
> If we do that, we have to put child devices of the dwc3 devices into
> the platform glue, and it also breaks those dwc3 devices that don't
> have a parent driver.

Well, this is easy to fix:

if (dwc->dev->parent) {
dwc->sysdev = dwc->dev->parent;
} else {
dev_info(dwc->dev, "Please provide a glue layer!\n");
dwc->sysdev = dwc->dev;
}

>> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
>> > index 89a2f712fdfe..4d7439cb8cd8 100644
>> > --- a/drivers/usb/dwc3/dwc3-st.c
>> > +++ b/drivers/usb/dwc3/dwc3-st.c
>> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
>> >if (IS_ERR(regmap))
>> >return PTR_ERR(regmap);
>> >  
>> > -  dma_set_coherent_mask(dev, dev->coherent_dma_mask);
>> 
>> so is this.
>> 
>> All in all, I like where you're going with this, we just need a matching
>> acpi_dma_configure() and problems will be sorted out.
>
> With this patch, I don't think we even need that any more, as the device
> that we use the dma-mapping API is the one that already gets configured
> correctly by the platform code for all cases: PCI, OF, ACPI and combinations
> of those.

sounds good to me

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> Arnd Bergmann  writes:
> just look at the history of the file, you'll see that an Intel employee
> was a maintainer of chipidea driver. Also:
> 
> $ git ls-files drivers/usb/chipidea/ | grep pci
> drivers/usb/chipidea/ci_hdrc_pci.c

Right, Peter pointed that one out too.

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 35d092456bec..08db66c64c66 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> actually, we don't want the core to know what it's attached to.

Agreed. This was just a first draft and I couldn't come up with
a better way to detect the case in which the parent device is
probed from another HW bus and the child is not known to the
firmware.

> >  #include 
> >  #include 
> >  #include 
> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
> > *dwc)
> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
> > struct dwc3_event_buffer *evt)
> >  {
> > -   dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> > +   dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
> 
> how about "dma_dev" instead? Is this used for anything other than DMA?

The two other things we have discussed in this thread are:

- connecting of_node pointers to usb_device structures for children
  of sysdev->of_node. Note that this can happen even for PCI devices
  in case you have a USB ethernet device hardwired to a PCI-USB bridge
  and put the mac address in DT.

- finding the PHY device for a HCD

There might be others. Basically sysdev here is what the USB core code
can use for looking up any kind of properties provided by the firmware.

> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
> > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
> > dwc->mem = mem;
> > dwc->dev = dev;
> > +#ifdef CONFIG_PCI
> > +   /* TODO: or some other way of detecting this? */
> > +   if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> > +   dwc->sysdev = dwc->dev->parent;
> > +   else
> > +#endif
> > +   dwc->sysdev = dwc->dev;
> 
> Well, we can remove this ifdef and *always* use the parent. We will just
> require that dwc3 users provide a glue layer. In that case, your check
> becomes:
> 
>   if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent;
>   else
>   dev_info(dwc->dev, "Please provide a glue layer!\n");

If we do that, we have to put child devices of the dwc3 devices into
the platform glue, and it also breaks those dwc3 devices that don't
have a parent driver.

> > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> > index 2f1fb7e7aa54..e27899bb5706 100644
> > --- a/drivers/usb/dwc3/dwc3-exynos.c
> > +++ b/drivers/usb/dwc3/dwc3-exynos.c
> > @@ -20,7 +20,6 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> > *pdev)
> > if (!exynos)
> > return -ENOMEM;
> >  
> > -   /*
> > -* Right now device-tree probed devices don't get dma_mask set.
> > -* Since shared usb code relies on it, set it here for now.
> > -* Once we move to full device tree support this will vanish off.
> > -*/
> > -   ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > -   if (ret)
> > -   return ret;
> 
> this is a separate patch, right?

Yes, this is probably just a cleanup that we can apply regardless.
We have not needed this in a long time.

> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> > index 89a2f712fdfe..4d7439cb8cd8 100644
> > --- a/drivers/usb/dwc3/dwc3-st.c
> > +++ b/drivers/usb/dwc3/dwc3-st.c
> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
> > if (IS_ERR(regmap))
> > return PTR_ERR(regmap);
> >  
> > -   dma_set_coherent_mask(dev, dev->coherent_dma_mask);
> 
> so is this.
> 
> All in all, I like where you're going with this, we just need a matching
> acpi_dma_configure() and problems will be sorted out.

With this patch, I don't think we even need that any more, as the device
that we use the dma-mapping API is the one that already gets configured
correctly by the platform code for all cases: PCI, OF, ACPI and combinations
of those.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 11:03:10 AM CEST Felipe Balbi wrote:
> Arnd Bergmann  writes:
> >> Arnd Bergmann  writes:
> just look at the history of the file, you'll see that an Intel employee
> was a maintainer of chipidea driver. Also:
> 
> $ git ls-files drivers/usb/chipidea/ | grep pci
> drivers/usb/chipidea/ci_hdrc_pci.c

Right, Peter pointed that one out too.

> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 35d092456bec..08db66c64c66 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> 
> actually, we don't want the core to know what it's attached to.

Agreed. This was just a first draft and I couldn't come up with
a better way to detect the case in which the parent device is
probed from another HW bus and the child is not known to the
firmware.

> >  #include 
> >  #include 
> >  #include 
> > @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
> > *dwc)
> >  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
> > struct dwc3_event_buffer *evt)
> >  {
> > -   dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> > +   dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);
> 
> how about "dma_dev" instead? Is this used for anything other than DMA?

The two other things we have discussed in this thread are:

- connecting of_node pointers to usb_device structures for children
  of sysdev->of_node. Note that this can happen even for PCI devices
  in case you have a USB ethernet device hardwired to a PCI-USB bridge
  and put the mac address in DT.

- finding the PHY device for a HCD

There might be others. Basically sysdev here is what the USB core code
can use for looking up any kind of properties provided by the firmware.

> > @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
> > dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
> > dwc->mem = mem;
> > dwc->dev = dev;
> > +#ifdef CONFIG_PCI
> > +   /* TODO: or some other way of detecting this? */
> > +   if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> > +   dwc->sysdev = dwc->dev->parent;
> > +   else
> > +#endif
> > +   dwc->sysdev = dwc->dev;
> 
> Well, we can remove this ifdef and *always* use the parent. We will just
> require that dwc3 users provide a glue layer. In that case, your check
> becomes:
> 
>   if (dwc->dev->parent)
>   dwc->sysdev = dwc->dev->parent;
>   else
>   dev_info(dwc->dev, "Please provide a glue layer!\n");

If we do that, we have to put child devices of the dwc3 devices into
the platform glue, and it also breaks those dwc3 devices that don't
have a parent driver.

> > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> > index 2f1fb7e7aa54..e27899bb5706 100644
> > --- a/drivers/usb/dwc3/dwc3-exynos.c
> > +++ b/drivers/usb/dwc3/dwc3-exynos.c
> > @@ -20,7 +20,6 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> > *pdev)
> > if (!exynos)
> > return -ENOMEM;
> >  
> > -   /*
> > -* Right now device-tree probed devices don't get dma_mask set.
> > -* Since shared usb code relies on it, set it here for now.
> > -* Once we move to full device tree support this will vanish off.
> > -*/
> > -   ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > -   if (ret)
> > -   return ret;
> 
> this is a separate patch, right?

Yes, this is probably just a cleanup that we can apply regardless.
We have not needed this in a long time.

> > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> > index 89a2f712fdfe..4d7439cb8cd8 100644
> > --- a/drivers/usb/dwc3/dwc3-st.c
> > +++ b/drivers/usb/dwc3/dwc3-st.c
> > @@ -218,7 +218,6 @@ static int st_dwc3_probe(struct platform_device *pdev)
> > if (IS_ERR(regmap))
> > return PTR_ERR(regmap);
> >  
> > -   dma_set_coherent_mask(dev, dev->coherent_dma_mask);
> 
> so is this.
> 
> All in all, I like where you're going with this, we just need a matching
> acpi_dma_configure() and problems will be sorted out.

With this patch, I don't think we even need that any more, as the device
that we use the dma-mapping API is the one that already gets configured
correctly by the platform code for all cases: PCI, OF, ACPI and combinations
of those.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> Arnd Bergmann  writes:
>> 
>> [...]
>> 
>> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>> > I think we should replace 
>> >
>> > pdev->dev.dma_mask = dev->dma_mask;
>> > pdev->dev.dma_parms = dev->dma_parms;
>> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>> >
>> > with of_dma_configure(), which has the chance to configure more than
>> > just those three, as the dma API might look into different aspects:
>> >
>> > - iommu specific configuration
>> > - cache coherency information
>> > - bus type
>> > - dma offset
>> > - dma_map_ops pointer
>> >
>> > We try to handle everything in of_dma_configure() at configuration
>> > time, and that would be the place to add anything else that we might
>> > need in the future.
>> 
>> There are a couple problems with this:
>> 
>> 1) won't work for PCI-based systems.
>> 
>> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
>> platform (FPGA that appears like a PCI card to host PC)
>
> Right, I was specifically talking about the code in chipidea here,
> which I think is never used on the PCI bus, and how the current

just look at the history of the file, you'll see that an Intel employee
was a maintainer of chipidea driver. Also:

$ git ls-files drivers/usb/chipidea/ | grep pci
drivers/usb/chipidea/ci_hdrc_pci.c

> code is broken. We can probably do better than of_dma_configure()
> (see below), but it would be an improvement.
>
>> 2) not very robust solution
>> 
>> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
>> that's not created by DT. The only reason why this works at all is
>> because of the default 32-bit dma mask thing :-) So, how is it any
>> different than copying 32-bit dma mask from parent?
>
> The idea here is that you pass in the parent of_node along with the child
> device pointer, so it would behave exactly like the parent already does.
> The difference is that it also handles all the other attributes besides the 
> mask.

Now we're talking :-) I like that. We just need a matching API for
ACPI/PCI-based systems.

> However, to summarize the discussion so far, I agree that
> of_dma_configure() is not the solution to these problems, and I think
> we can do much better:
>
> Splitting the usb_bus->controller field into the Linux-internal device
> (used for the sysfs hierarchy, for printks and for power management)
> and a new pointer (used for DMA, DT enumeration and phy lookup) probably
> covers all that we really need.
>
> I've prototyped it below, with the dwc3, xhci and chipidea changes
> together with the core changes. I've surely made mistakes there and
> don't expect it to work out of the box, but this should give an
> idea of how I think this can all be solved in the least invasive
> way.
>
> I noticed that the gadget interface already has a way to handle the
> DMA allocation by device, so I added that in as well.

yeah, I wanna use that :-)

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 35d092456bec..08db66c64c66 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

actually, we don't want the core to know what it's attached to.

>  #include 
>  #include 
>  #include 
> @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>   struct dwc3_event_buffer *evt)
>  {
> - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);

how about "dma_dev" instead? Is this used for anything other than DMA?

> @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>   dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>   dwc->mem = mem;
>   dwc->dev = dev;
> +#ifdef CONFIG_PCI
> + /* TODO: or some other way of detecting this? */
> + if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> + dwc->sysdev = dwc->dev->parent;
> + else
> +#endif
> + dwc->sysdev = dwc->dev;

Well, we can remove this ifdef and *always* use the parent. We will just
require that dwc3 users provide a glue layer. In that case, your check
becomes:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent;
else
dev_info(dwc->dev, "Please provide a glue layer!\n");

> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 2f1fb7e7aa54..e27899bb5706 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> *pdev)
>   if (!exynos)
>   return -ENOMEM;
>  
> - /*
> -  * 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
>> Arnd Bergmann  writes:
>> 
>> [...]
>> 
>> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>> > I think we should replace 
>> >
>> > pdev->dev.dma_mask = dev->dma_mask;
>> > pdev->dev.dma_parms = dev->dma_parms;
>> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>> >
>> > with of_dma_configure(), which has the chance to configure more than
>> > just those three, as the dma API might look into different aspects:
>> >
>> > - iommu specific configuration
>> > - cache coherency information
>> > - bus type
>> > - dma offset
>> > - dma_map_ops pointer
>> >
>> > We try to handle everything in of_dma_configure() at configuration
>> > time, and that would be the place to add anything else that we might
>> > need in the future.
>> 
>> There are a couple problems with this:
>> 
>> 1) won't work for PCI-based systems.
>> 
>> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
>> platform (FPGA that appears like a PCI card to host PC)
>
> Right, I was specifically talking about the code in chipidea here,
> which I think is never used on the PCI bus, and how the current

just look at the history of the file, you'll see that an Intel employee
was a maintainer of chipidea driver. Also:

$ git ls-files drivers/usb/chipidea/ | grep pci
drivers/usb/chipidea/ci_hdrc_pci.c

> code is broken. We can probably do better than of_dma_configure()
> (see below), but it would be an improvement.
>
>> 2) not very robust solution
>> 
>> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
>> that's not created by DT. The only reason why this works at all is
>> because of the default 32-bit dma mask thing :-) So, how is it any
>> different than copying 32-bit dma mask from parent?
>
> The idea here is that you pass in the parent of_node along with the child
> device pointer, so it would behave exactly like the parent already does.
> The difference is that it also handles all the other attributes besides the 
> mask.

Now we're talking :-) I like that. We just need a matching API for
ACPI/PCI-based systems.

> However, to summarize the discussion so far, I agree that
> of_dma_configure() is not the solution to these problems, and I think
> we can do much better:
>
> Splitting the usb_bus->controller field into the Linux-internal device
> (used for the sysfs hierarchy, for printks and for power management)
> and a new pointer (used for DMA, DT enumeration and phy lookup) probably
> covers all that we really need.
>
> I've prototyped it below, with the dwc3, xhci and chipidea changes
> together with the core changes. I've surely made mistakes there and
> don't expect it to work out of the box, but this should give an
> idea of how I think this can all be solved in the least invasive
> way.
>
> I noticed that the gadget interface already has a way to handle the
> DMA allocation by device, so I added that in as well.

yeah, I wanna use that :-)

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 35d092456bec..08db66c64c66 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

actually, we don't want the core to know what it's attached to.

>  #include 
>  #include 
>  #include 
> @@ -178,7 +179,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>  static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>   struct dwc3_event_buffer *evt)
>  {
> - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> + dma_free_coherent(dwc->sysdev, evt->length, evt->buf, evt->dma);

how about "dma_dev" instead? Is this used for anything other than DMA?

> @@ -846,6 +847,13 @@ static int dwc3_probe(struct platform_device *pdev)
>   dwc = PTR_ALIGN(mem, DWC3_ALIGN_MASK + 1);
>   dwc->mem = mem;
>   dwc->dev = dev;
> +#ifdef CONFIG_PCI
> + /* TODO: or some other way of detecting this? */
> + if (dwc->dev->parent && dwc->dev->parent->bus == _bus_type)
> + dwc->sysdev = dwc->dev->parent;
> + else
> +#endif
> + dwc->sysdev = dwc->dev;

Well, we can remove this ifdef and *always* use the parent. We will just
require that dwc3 users provide a glue layer. In that case, your check
becomes:

if (dwc->dev->parent)
dwc->sysdev = dwc->dev->parent;
else
dev_info(dwc->dev, "Please provide a glue layer!\n");

> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index 2f1fb7e7aa54..e27899bb5706 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -117,15 +116,6 @@ static int dwc3_exynos_probe(struct platform_device 
> *pdev)
>   if (!exynos)
>   return -ENOMEM;
>  
> - /*
> -  * Right now device-tree probed 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 9:15:36 AM CEST Peter Chen wrote:
> > 
> > Right, I was specifically talking about the code in chipidea here,
> > which I think is never used on the PCI bus, and how the current
> > code is broken. We can probably do better than of_dma_configure()
> > (see below), but it would be an improvement.
> 
> Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c
> 

Ok, I see.

The experimental patch I posted should actually handle this just fine,
as it simply assumes that dev->parent is the device used for the DMA
API in chipidea, and I think this holds true for both the PCI and the
DT based uses of this driver.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Thursday, September 8, 2016 9:15:36 AM CEST Peter Chen wrote:
> > 
> > Right, I was specifically talking about the code in chipidea here,
> > which I think is never used on the PCI bus, and how the current
> > code is broken. We can probably do better than of_dma_configure()
> > (see below), but it would be an improvement.
> 
> Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c
> 

Ok, I see.

The experimental patch I posted should actually handle this just fine,
as it simply assumes that dev->parent is the device used for the DMA
API in chipidea, and I think this holds true for both the PCI and the
DT based uses of this driver.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 05:24:08PM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Arnd Bergmann  writes:
> > 
> > [...]
> > 
> > > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> > > I think we should replace 
> > >
> > > pdev->dev.dma_mask = dev->dma_mask;
> > > pdev->dev.dma_parms = dev->dma_parms;
> > > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> > >
> > > with of_dma_configure(), which has the chance to configure more than
> > > just those three, as the dma API might look into different aspects:
> > >
> > > - iommu specific configuration
> > > - cache coherency information
> > > - bus type
> > > - dma offset
> > > - dma_map_ops pointer
> > >
> > > We try to handle everything in of_dma_configure() at configuration
> > > time, and that would be the place to add anything else that we might
> > > need in the future.
> > 
> > There are a couple problems with this:
> > 
> > 1) won't work for PCI-based systems.
> > 
> > DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
> > platform (FPGA that appears like a PCI card to host PC)
> 
> Right, I was specifically talking about the code in chipidea here,
> which I think is never used on the PCI bus, and how the current
> code is broken. We can probably do better than of_dma_configure()
> (see below), but it would be an improvement.

Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 05:24:08PM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Arnd Bergmann  writes:
> > 
> > [...]
> > 
> > > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> > > I think we should replace 
> > >
> > > pdev->dev.dma_mask = dev->dma_mask;
> > > pdev->dev.dma_parms = dev->dma_parms;
> > > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> > >
> > > with of_dma_configure(), which has the chance to configure more than
> > > just those three, as the dma API might look into different aspects:
> > >
> > > - iommu specific configuration
> > > - cache coherency information
> > > - bus type
> > > - dma offset
> > > - dma_map_ops pointer
> > >
> > > We try to handle everything in of_dma_configure() at configuration
> > > time, and that would be the place to add anything else that we might
> > > need in the future.
> > 
> > There are a couple problems with this:
> > 
> > 1) won't work for PCI-based systems.
> > 
> > DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
> > platform (FPGA that appears like a PCI card to host PC)
> 
> Right, I was specifically talking about the code in chipidea here,
> which I think is never used on the PCI bus, and how the current
> code is broken. We can probably do better than of_dma_configure()
> (see below), but it would be an improvement.

Chipidea is also used at PCI bus too, see drivers/usb/chipidea/ci_hdrc_pci.c

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 12:08:20 PM CEST Alan Stern wrote:
> On Wed, 7 Sep 2016, Arnd Bergmann wrote:
> 
> >  drivers/usb/host/ehci-fsl.c|  4 ++--
> 
> How did this driver end up in the patch?
>
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 9f5ffb629973..b2419950221f 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device 
> > *pdev)
> > }
> > irq = res->start;
> >  
> > -   hcd = usb_create_hcd(_ehci_hc_driver, >dev,
> > -   dev_name(>dev));
> > +   hcd = __usb_create_hcd(_ehci_hc_driver, >dev.parent,
> > +   >dev, dev_name(>dev), NULL);
> 
> Based on the
> 
>   if (of_device_is_compatible(dev->parent->of_node,
>   "fsl,mpc5121-usb2-dr")) {
> 
> lines in the driver?

No, based on the "fsl-ehci" name, which is only used as a child
of the drivers/usb/host/fsl-mph-dr-of.c dual-role driver.

I looked for drivers that call platform_device_add() and manipulate
the dma_mask of that child.

Sorry for missing this one when I did the description. I believe
it is correct though. The DMA settings on powerpc are probably correct
in this case, but the existing code doesn't allow you to describe
on-board USB devices with additional properties as children of
the dual-role device node.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 12:08:20 PM CEST Alan Stern wrote:
> On Wed, 7 Sep 2016, Arnd Bergmann wrote:
> 
> >  drivers/usb/host/ehci-fsl.c|  4 ++--
> 
> How did this driver end up in the patch?
>
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 9f5ffb629973..b2419950221f 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device 
> > *pdev)
> > }
> > irq = res->start;
> >  
> > -   hcd = usb_create_hcd(_ehci_hc_driver, >dev,
> > -   dev_name(>dev));
> > +   hcd = __usb_create_hcd(_ehci_hc_driver, >dev.parent,
> > +   >dev, dev_name(>dev), NULL);
> 
> Based on the
> 
>   if (of_device_is_compatible(dev->parent->of_node,
>   "fsl,mpc5121-usb2-dr")) {
> 
> lines in the driver?

No, based on the "fsl-ehci" name, which is only used as a child
of the drivers/usb/host/fsl-mph-dr-of.c dual-role driver.

I looked for drivers that call platform_device_add() and manipulate
the dma_mask of that child.

Sorry for missing this one when I did the description. I believe
it is correct though. The DMA settings on powerpc are probably correct
in this case, but the existing code doesn't allow you to describe
on-board USB devices with additional properties as children of
the dual-role device node.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Alan Stern]

On Wed, 7 Sep 2016, Arnd Bergmann wrote:

> However, to summarize the discussion so far, I agree that
> of_dma_configure() is not the solution to these problems, and I think
> we can do much better:
> 
> Splitting the usb_bus->controller field into the Linux-internal device
> (used for the sysfs hierarchy, for printks and for power management)
> and a new pointer (used for DMA, DT enumeration and phy lookup) probably
> covers all that we really need.
> 
> I've prototyped it below, with the dwc3, xhci and chipidea changes
> together with the core changes. I've surely made mistakes there and
> don't expect it to work out of the box, but this should give an
> idea of how I think this can all be solved in the least invasive
> way.
> 
> I noticed that the gadget interface already has a way to handle the
> DMA allocation by device, so I added that in as well.
> 
> Signed-off-by: Arnd Bergmann 
> 
>  drivers/usb/chipidea/core.c|  4 
>  drivers/usb/chipidea/host.c|  3 ++-
>  drivers/usb/chipidea/udc.c |  8 
>  drivers/usb/core/buffer.c  | 12 ++--
>  drivers/usb/core/hcd.c | 48 
> +---
>  drivers/usb/core/usb.c | 16 
>  drivers/usb/dwc3/core.c| 28 +++-
>  drivers/usb/dwc3/core.h|  1 +
>  drivers/usb/dwc3/dwc3-exynos.c | 10 --
>  drivers/usb/dwc3/dwc3-st.c |  1 -
>  drivers/usb/dwc3/ep0.c |  8 
>  drivers/usb/dwc3/gadget.c  | 34 +-
>  drivers/usb/dwc3/host.c| 13 -
>  drivers/usb/host/ehci-fsl.c|  4 ++--

How did this driver end up in the patch?

>  drivers/usb/host/xhci-plat.c   | 32 +---
>  include/linux/usb.h|  1 +
>  include/linux/usb/hcd.h|  3 +++

> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 9f5ffb629973..b2419950221f 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
>   }
>   irq = res->start;
>  
> - hcd = usb_create_hcd(_ehci_hc_driver, >dev,
> - dev_name(>dev));
> + hcd = __usb_create_hcd(_ehci_hc_driver, >dev.parent,
> + >dev, dev_name(>dev), NULL);

Based on the

if (of_device_is_compatible(dev->parent->of_node,
"fsl,mpc5121-usb2-dr")) {

lines in the driver?

>   if (!hcd) {
>   retval = -ENOMEM;
>   goto err1;

Alan Stern

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Alan Stern]

On Wed, 7 Sep 2016, Arnd Bergmann wrote:

> However, to summarize the discussion so far, I agree that
> of_dma_configure() is not the solution to these problems, and I think
> we can do much better:
> 
> Splitting the usb_bus->controller field into the Linux-internal device
> (used for the sysfs hierarchy, for printks and for power management)
> and a new pointer (used for DMA, DT enumeration and phy lookup) probably
> covers all that we really need.
> 
> I've prototyped it below, with the dwc3, xhci and chipidea changes
> together with the core changes. I've surely made mistakes there and
> don't expect it to work out of the box, but this should give an
> idea of how I think this can all be solved in the least invasive
> way.
> 
> I noticed that the gadget interface already has a way to handle the
> DMA allocation by device, so I added that in as well.
> 
> Signed-off-by: Arnd Bergmann 
> 
>  drivers/usb/chipidea/core.c|  4 
>  drivers/usb/chipidea/host.c|  3 ++-
>  drivers/usb/chipidea/udc.c |  8 
>  drivers/usb/core/buffer.c  | 12 ++--
>  drivers/usb/core/hcd.c | 48 
> +---
>  drivers/usb/core/usb.c | 16 
>  drivers/usb/dwc3/core.c| 28 +++-
>  drivers/usb/dwc3/core.h|  1 +
>  drivers/usb/dwc3/dwc3-exynos.c | 10 --
>  drivers/usb/dwc3/dwc3-st.c |  1 -
>  drivers/usb/dwc3/ep0.c |  8 
>  drivers/usb/dwc3/gadget.c  | 34 +-
>  drivers/usb/dwc3/host.c| 13 -
>  drivers/usb/host/ehci-fsl.c|  4 ++--

How did this driver end up in the patch?

>  drivers/usb/host/xhci-plat.c   | 32 +---
>  include/linux/usb.h|  1 +
>  include/linux/usb/hcd.h|  3 +++

> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 9f5ffb629973..b2419950221f 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -96,8 +96,8 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
>   }
>   irq = res->start;
>  
> - hcd = usb_create_hcd(_ehci_hc_driver, >dev,
> - dev_name(>dev));
> + hcd = __usb_create_hcd(_ehci_hc_driver, >dev.parent,
> + >dev, dev_name(>dev), NULL);

Based on the

if (of_device_is_compatible(dev->parent->of_node,
"fsl,mpc5121-usb2-dr")) {

lines in the driver?

>   if (!hcd) {
>   retval = -ENOMEM;
>   goto err1;

Alan Stern

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
> 
> [...]
> 
> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> > I think we should replace 
> >
> > pdev->dev.dma_mask = dev->dma_mask;
> > pdev->dev.dma_parms = dev->dma_parms;
> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >
> > with of_dma_configure(), which has the chance to configure more than
> > just those three, as the dma API might look into different aspects:
> >
> > - iommu specific configuration
> > - cache coherency information
> > - bus type
> > - dma offset
> > - dma_map_ops pointer
> >
> > We try to handle everything in of_dma_configure() at configuration
> > time, and that would be the place to add anything else that we might
> > need in the future.
> 
> There are a couple problems with this:
> 
> 1) won't work for PCI-based systems.
> 
> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
> platform (FPGA that appears like a PCI card to host PC)

Right, I was specifically talking about the code in chipidea here,
which I think is never used on the PCI bus, and how the current
code is broken. We can probably do better than of_dma_configure()
(see below), but it would be an improvement.

> 2) not very robust solution
> 
> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
> that's not created by DT. The only reason why this works at all is
> because of the default 32-bit dma mask thing :-) So, how is it any
> different than copying 32-bit dma mask from parent?

The idea here is that you pass in the parent of_node along with the child
device pointer, so it would behave exactly like the parent already does.
The difference is that it also handles all the other attributes besides the 
mask.

However, to summarize the discussion so far, I agree that
of_dma_configure() is not the solution to these problems, and I think
we can do much better:

Splitting the usb_bus->controller field into the Linux-internal device
(used for the sysfs hierarchy, for printks and for power management)
and a new pointer (used for DMA, DT enumeration and phy lookup) probably
covers all that we really need.

I've prototyped it below, with the dwc3, xhci and chipidea changes
together with the core changes. I've surely made mistakes there and
don't expect it to work out of the box, but this should give an
idea of how I think this can all be solved in the least invasive
way.

I noticed that the gadget interface already has a way to handle the
DMA allocation by device, so I added that in as well.

Signed-off-by: Arnd Bergmann 

 drivers/usb/chipidea/core.c|  4 
 drivers/usb/chipidea/host.c|  3 ++-
 drivers/usb/chipidea/udc.c |  8 
 drivers/usb/core/buffer.c  | 12 ++--
 drivers/usb/core/hcd.c | 48 
+---
 drivers/usb/core/usb.c | 16 
 drivers/usb/dwc3/core.c| 28 +++-
 drivers/usb/dwc3/core.h|  1 +
 drivers/usb/dwc3/dwc3-exynos.c | 10 --
 drivers/usb/dwc3/dwc3-st.c |  1 -
 drivers/usb/dwc3/ep0.c |  8 
 drivers/usb/dwc3/gadget.c  | 34 +-
 drivers/usb/dwc3/host.c| 13 -
 drivers/usb/host/ehci-fsl.c|  4 ++--
 drivers/usb/host/xhci-plat.c   | 32 +---
 include/linux/usb.h|  1 +
 include/linux/usb/hcd.h|  3 +++

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 69426e644d17..dff69837b349 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device 
*dev,
}
 
pdev->dev.parent = dev;
-   pdev->dev.dma_mask = dev->dma_mask;
-   pdev->dev.dma_parms = dev->dma_parms;
-   dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
-
ret = platform_device_add_resources(pdev, res, nres);
if (ret)
goto err;
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 053bac9d983c..40d29c4d7772 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -113,7 +113,8 @@ static int host_start(struct ci_hdrc *ci)
if (usb_disabled())
return -ENODEV;
 
-   hcd = usb_create_hcd(_ehci_hc_driver, ci->dev, dev_name(ci->dev));
+   hcd = __usb_create_hcd(_ehci_hc_driver, ci->dev->parent,
+  ci->dev, dev_name(ci->dev), NULL);
if (!hcd)
return -ENOMEM;
 
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 0f692fcda638..4cbfff7934c6 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -424,7 +424,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct 
ci_hw_req *hwreq)
 
hwreq->req.status = 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 1:24:07 PM CEST Felipe Balbi wrote:
> 
> Hi,
> 
> Arnd Bergmann  writes:
> 
> [...]
> 
> > Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> > I think we should replace 
> >
> > pdev->dev.dma_mask = dev->dma_mask;
> > pdev->dev.dma_parms = dev->dma_parms;
> > dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> >
> > with of_dma_configure(), which has the chance to configure more than
> > just those three, as the dma API might look into different aspects:
> >
> > - iommu specific configuration
> > - cache coherency information
> > - bus type
> > - dma offset
> > - dma_map_ops pointer
> >
> > We try to handle everything in of_dma_configure() at configuration
> > time, and that would be the place to add anything else that we might
> > need in the future.
> 
> There are a couple problems with this:
> 
> 1) won't work for PCI-based systems.
> 
> DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
> platform (FPGA that appears like a PCI card to host PC)

Right, I was specifically talking about the code in chipidea here,
which I think is never used on the PCI bus, and how the current
code is broken. We can probably do better than of_dma_configure()
(see below), but it would be an improvement.

> 2) not very robust solution
> 
> of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
> that's not created by DT. The only reason why this works at all is
> because of the default 32-bit dma mask thing :-) So, how is it any
> different than copying 32-bit dma mask from parent?

The idea here is that you pass in the parent of_node along with the child
device pointer, so it would behave exactly like the parent already does.
The difference is that it also handles all the other attributes besides the 
mask.

However, to summarize the discussion so far, I agree that
of_dma_configure() is not the solution to these problems, and I think
we can do much better:

Splitting the usb_bus->controller field into the Linux-internal device
(used for the sysfs hierarchy, for printks and for power management)
and a new pointer (used for DMA, DT enumeration and phy lookup) probably
covers all that we really need.

I've prototyped it below, with the dwc3, xhci and chipidea changes
together with the core changes. I've surely made mistakes there and
don't expect it to work out of the box, but this should give an
idea of how I think this can all be solved in the least invasive
way.

I noticed that the gadget interface already has a way to handle the
DMA allocation by device, so I added that in as well.

Signed-off-by: Arnd Bergmann 

 drivers/usb/chipidea/core.c|  4 
 drivers/usb/chipidea/host.c|  3 ++-
 drivers/usb/chipidea/udc.c |  8 
 drivers/usb/core/buffer.c  | 12 ++--
 drivers/usb/core/hcd.c | 48 
+---
 drivers/usb/core/usb.c | 16 
 drivers/usb/dwc3/core.c| 28 +++-
 drivers/usb/dwc3/core.h|  1 +
 drivers/usb/dwc3/dwc3-exynos.c | 10 --
 drivers/usb/dwc3/dwc3-st.c |  1 -
 drivers/usb/dwc3/ep0.c |  8 
 drivers/usb/dwc3/gadget.c  | 34 +-
 drivers/usb/dwc3/host.c| 13 -
 drivers/usb/host/ehci-fsl.c|  4 ++--
 drivers/usb/host/xhci-plat.c   | 32 +---
 include/linux/usb.h|  1 +
 include/linux/usb/hcd.h|  3 +++

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 69426e644d17..dff69837b349 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -833,10 +833,6 @@ struct platform_device *ci_hdrc_add_device(struct device 
*dev,
}
 
pdev->dev.parent = dev;
-   pdev->dev.dma_mask = dev->dma_mask;
-   pdev->dev.dma_parms = dev->dma_parms;
-   dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
-
ret = platform_device_add_resources(pdev, res, nres);
if (ret)
goto err;
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 053bac9d983c..40d29c4d7772 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -113,7 +113,8 @@ static int host_start(struct ci_hdrc *ci)
if (usb_disabled())
return -ENODEV;
 
-   hcd = usb_create_hcd(_ehci_hc_driver, ci->dev, dev_name(ci->dev));
+   hcd = __usb_create_hcd(_ehci_hc_driver, ci->dev->parent,
+  ci->dev, dev_name(ci->dev), NULL);
if (!hcd)
return -ENOMEM;
 
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 0f692fcda638..4cbfff7934c6 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -424,7 +424,7 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct 
ci_hw_req *hwreq)
 
hwreq->req.status = -EALREADY;
 
-   ret = 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 4:04:52 PM CEST Roger Quadros wrote:
> > The main use for it is to let you specify a MAC address for on-board
> > ethernet devices that lack an EPROM, but any other information can be
> > added that way too.
> > 
> >> There is a bug  in the USB core because of which the ISB device and 
> >> interfaces
> >> do not inherit dma_pfn_offset correctly for which I've sent a patch
> >> https://lkml.org/lkml/2016/8/17/275
> > 
> > I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
> > also set the dma_pfn_offset, but what exactly is this used for in USB
> > devices?
> 
> Consider the mass storage device case.
> USB storage driver creates a scsi host for the mass storage interface in
> drivers/usb/storage/usb.c
> The scsi host parent device is nothing but the the USB interface device.
> 
> Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out
> and set the block layer bounce limit.
> 
> scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the 
> bounce_limit.
> 
> host_dev is nothing but the device representing the mass storage interface.
> 
> If that device doesn't have the right dma_pfn_offset, then dma_max_pfn()
> is messed up and the bounce buffer limit is wrong.

I see. The same thing probably happens in the network and mmc subsystems,
which have similar code.

This shows the inconsistencies we have in the handling for bounce buffers
in the kernel, which are sometimes handled by subsystems but sometimes
rely on swiotlb instead. I don't have any better idea than your patch
here, but maybe we should add a comment explaining that next to the
code.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 4:04:52 PM CEST Roger Quadros wrote:
> > The main use for it is to let you specify a MAC address for on-board
> > ethernet devices that lack an EPROM, but any other information can be
> > added that way too.
> > 
> >> There is a bug  in the USB core because of which the ISB device and 
> >> interfaces
> >> do not inherit dma_pfn_offset correctly for which I've sent a patch
> >> https://lkml.org/lkml/2016/8/17/275
> > 
> > I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
> > also set the dma_pfn_offset, but what exactly is this used for in USB
> > devices?
> 
> Consider the mass storage device case.
> USB storage driver creates a scsi host for the mass storage interface in
> drivers/usb/storage/usb.c
> The scsi host parent device is nothing but the the USB interface device.
> 
> Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out
> and set the block layer bounce limit.
> 
> scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the 
> bounce_limit.
> 
> host_dev is nothing but the device representing the mass storage interface.
> 
> If that device doesn't have the right dma_pfn_offset, then dma_max_pfn()
> is messed up and the bounce buffer limit is wrong.

I see. The same thing probably happens in the network and mmc subsystems,
which have similar code.

This shows the inconsistencies we have in the handling for bounce buffers
in the kernel, which are sometimes handled by subsystems but sometimes
rely on swiotlb instead. I don't have any better idea than your patch
here, but maybe we should add a comment explaining that next to the
code.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Roger Quadros]

On 07/09/16 11:29, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote:
>>>
>>> Speaking of that flag, I suppose we need the same logic to know where
>>> to look for USB devices attached to a dwc3 host when we need to describe
>>> them in DT. By default we look for child device nodes under the
>>> node of the HCD device node, but that would be wrong here too.
>>
>> I didn't get this part. Information about USB devices attached to a USB host
>> is never provided in DT because they are always dynamically created via
>> usb_new_device(), whether they are hard-wired on the board or hot-plugged.
>>
>> These USB devices inherit their DMA masks in the usb_alloc_dev() routine
>> whereas each interface within the USB device inherits its DMA mask in
>> usb_set_configuration().
> 
> We had talked about adding support for this for at least six years (probably
> much more), but Peter Chen finally added it this year in commit 69bec72598
> ("USB: core: let USB device know device node").

OK. Thanks for this pointer.
> 
> The main use for it is to let you specify a MAC address for on-board
> ethernet devices that lack an EPROM, but any other information can be
> added that way too.
> 
>> There is a bug  in the USB core because of which the ISB device and 
>> interfaces
>> do not inherit dma_pfn_offset correctly for which I've sent a patch
>> https://lkml.org/lkml/2016/8/17/275
> 
> I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
> also set the dma_pfn_offset, but what exactly is this used for in USB
> devices?

Consider the mass storage device case.
USB storage driver creates a scsi host for the mass storage interface in
drivers/usb/storage/usb.c
The scsi host parent device is nothing but the the USB interface device.

Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out
and set the block layer bounce limit.

scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the 
bounce_limit.

host_dev is nothing but the device representing the mass storage interface.

If that device doesn't have the right dma_pfn_offset, then dma_max_pfn()
is messed up and the bounce buffer limit is wrong.

> 
> As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA
> mapping interface, but that can't really be used on USB devices, which
> I assume use usb_alloc_coherent() and the URB interfaces for passing data
> between a USB driver and the HCD. My knowledge of USB device drivers
> is a bit lacking, so it's possible I'm misunderstanding things here.
> 

cheers,
-roger

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Roger Quadros]

On 07/09/16 11:29, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote:
>>>
>>> Speaking of that flag, I suppose we need the same logic to know where
>>> to look for USB devices attached to a dwc3 host when we need to describe
>>> them in DT. By default we look for child device nodes under the
>>> node of the HCD device node, but that would be wrong here too.
>>
>> I didn't get this part. Information about USB devices attached to a USB host
>> is never provided in DT because they are always dynamically created via
>> usb_new_device(), whether they are hard-wired on the board or hot-plugged.
>>
>> These USB devices inherit their DMA masks in the usb_alloc_dev() routine
>> whereas each interface within the USB device inherits its DMA mask in
>> usb_set_configuration().
> 
> We had talked about adding support for this for at least six years (probably
> much more), but Peter Chen finally added it this year in commit 69bec72598
> ("USB: core: let USB device know device node").

OK. Thanks for this pointer.
> 
> The main use for it is to let you specify a MAC address for on-board
> ethernet devices that lack an EPROM, but any other information can be
> added that way too.
> 
>> There is a bug  in the USB core because of which the ISB device and 
>> interfaces
>> do not inherit dma_pfn_offset correctly for which I've sent a patch
>> https://lkml.org/lkml/2016/8/17/275
> 
> I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
> also set the dma_pfn_offset, but what exactly is this used for in USB
> devices?

Consider the mass storage device case.
USB storage driver creates a scsi host for the mass storage interface in
drivers/usb/storage/usb.c
The scsi host parent device is nothing but the the USB interface device.

Now, __scsi_init_queue() calls scsi_calculate_bounce_limit() to find out
and set the block layer bounce limit.

scsi_calculate_bounce_limit() uses dma_max_pfn(host_dev) to get the 
bounce_limit.

host_dev is nothing but the device representing the mass storage interface.

If that device doesn't have the right dma_pfn_offset, then dma_max_pfn()
is messed up and the bounce buffer limit is wrong.

> 
> As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA
> mapping interface, but that can't really be used on USB devices, which
> I assume use usb_alloc_coherent() and the URB interfaces for passing data
> between a USB driver and the HCD. My knowledge of USB device drivers
> is a bit lacking, so it's possible I'm misunderstanding things here.
> 

cheers,
-roger

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Robin Murphy  writes:
> On 07/09/16 10:55, Peter Chen wrote:
> [...]
>>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>>> I think we should replace 
>>>
>>> pdev->dev.dma_mask = dev->dma_mask;
>>> pdev->dev.dma_parms = dev->dma_parms;
>>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>>>
>>> with of_dma_configure(), which has the chance to configure more than
>>> just those three, as the dma API might look into different aspects:
>>>
>>> - iommu specific configuration
>>> - cache coherency information
>>> - bus type
>>> - dma offset
>>> - dma_map_ops pointer
>>>
>>> We try to handle everything in of_dma_configure() at configuration
>>> time, and that would be the place to add anything else that we might
>>> need in the future.
>>>
>> 
>> Yes, I agree with you, but just like Felipe mentioned, we also need to
>> consider PCI device, can we do something like gpiod_get_index does? Are
>> there any similar APIs like of_dma_configure for ACPI?
>
> Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> abstracting away the IOMMU configuration.
>
> Robin.
>
> [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html

not exported for drivers to use. If Lorenzo is trying to making a
matching API for ACPI systems, then it needs to follow what
of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Robin Murphy  writes:
> On 07/09/16 10:55, Peter Chen wrote:
> [...]
>>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>>> I think we should replace 
>>>
>>> pdev->dev.dma_mask = dev->dma_mask;
>>> pdev->dev.dma_parms = dev->dma_parms;
>>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>>>
>>> with of_dma_configure(), which has the chance to configure more than
>>> just those three, as the dma API might look into different aspects:
>>>
>>> - iommu specific configuration
>>> - cache coherency information
>>> - bus type
>>> - dma offset
>>> - dma_map_ops pointer
>>>
>>> We try to handle everything in of_dma_configure() at configuration
>>> time, and that would be the place to add anything else that we might
>>> need in the future.
>>>
>> 
>> Yes, I agree with you, but just like Felipe mentioned, we also need to
>> consider PCI device, can we do something like gpiod_get_index does? Are
>> there any similar APIs like of_dma_configure for ACPI?
>
> Not yet, but Lorenzo has one in progress[1], primarily for the sake of
> abstracting away the IOMMU configuration.
>
> Robin.
>
> [1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html

not exported for drivers to use. If Lorenzo is trying to making a
matching API for ACPI systems, then it needs to follow what
of_dma_configure() is doing, and add an EXPORT_SYMBOL_GPL()

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Robin Murphy]

On 07/09/16 10:55, Peter Chen wrote:
[...]
>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>> I think we should replace 
>>
>> pdev->dev.dma_mask = dev->dma_mask;
>> pdev->dev.dma_parms = dev->dma_parms;
>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>>
>> with of_dma_configure(), which has the chance to configure more than
>> just those three, as the dma API might look into different aspects:
>>
>> - iommu specific configuration
>> - cache coherency information
>> - bus type
>> - dma offset
>> - dma_map_ops pointer
>>
>> We try to handle everything in of_dma_configure() at configuration
>> time, and that would be the place to add anything else that we might
>> need in the future.
>>
> 
> Yes, I agree with you, but just like Felipe mentioned, we also need to
> consider PCI device, can we do something like gpiod_get_index does? Are
> there any similar APIs like of_dma_configure for ACPI?

Not yet, but Lorenzo has one in progress[1], primarily for the sake of
abstracting away the IOMMU configuration.

Robin.

[1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Robin Murphy]

On 07/09/16 10:55, Peter Chen wrote:
[...]
>> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
>> I think we should replace 
>>
>> pdev->dev.dma_mask = dev->dma_mask;
>> pdev->dev.dma_parms = dev->dma_parms;
>> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>>
>> with of_dma_configure(), which has the chance to configure more than
>> just those three, as the dma API might look into different aspects:
>>
>> - iommu specific configuration
>> - cache coherency information
>> - bus type
>> - dma offset
>> - dma_map_ops pointer
>>
>> We try to handle everything in of_dma_configure() at configuration
>> time, and that would be the place to add anything else that we might
>> need in the future.
>>
> 
> Yes, I agree with you, but just like Felipe mentioned, we also need to
> consider PCI device, can we do something like gpiod_get_index does? Are
> there any similar APIs like of_dma_configure for ACPI?

Not yet, but Lorenzo has one in progress[1], primarily for the sake of
abstracting away the IOMMU configuration.

Robin.

[1]:http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1209911.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:

[...]

> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> I think we should replace 
>
> pdev->dev.dma_mask = dev->dma_mask;
> pdev->dev.dma_parms = dev->dma_parms;
> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>
> with of_dma_configure(), which has the chance to configure more than
> just those three, as the dma API might look into different aspects:
>
> - iommu specific configuration
> - cache coherency information
> - bus type
> - dma offset
> - dma_map_ops pointer
>
> We try to handle everything in of_dma_configure() at configuration
> time, and that would be the place to add anything else that we might
> need in the future.

There are a couple problems with this:

1) won't work for PCI-based systems.

DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
platform (FPGA that appears like a PCI card to host PC)


2) not very robust solution

of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
that's not created by DT. The only reason why this works at all is
because of the default 32-bit dma mask thing :-) So, how is it any
different than copying 32-bit dma mask from parent?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:

[...]

> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> I think we should replace 
>
> pdev->dev.dma_mask = dev->dma_mask;
> pdev->dev.dma_parms = dev->dma_parms;
> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
>
> with of_dma_configure(), which has the chance to configure more than
> just those three, as the dma API might look into different aspects:
>
> - iommu specific configuration
> - cache coherency information
> - bus type
> - dma offset
> - dma_map_ops pointer
>
> We try to handle everything in of_dma_configure() at configuration
> time, and that would be the place to add anything else that we might
> need in the future.

There are a couple problems with this:

1) won't work for PCI-based systems.

DWC3 is used in production PCI-based HW and also in Synopsys HAPS DX
platform (FPGA that appears like a PCI card to host PC)


2) not very robust solution

of_dma_configure() will hardcode 32-bit DMA dmask for xhci-plat because
that's not created by DT. The only reason why this works at all is
because of the default 32-bit dma mask thing :-) So, how is it any
different than copying 32-bit dma mask from parent?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Russell King - ARM Linux  writes:
> On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote:
>> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
>> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
>> > > 
>> > > The pre-condition of DT function at USB HCD core works is the host
>> > > controller device has of_node, since it is the root node for USB tree
>> > > described at DT. If the host controller device is not at DT, it needs
>> > > to try to get its of_node, the chipidea driver gets it through its
>> > > parent node [1]
>> > 
>> > > 
>> > > [1] https://lkml.org/lkml/2016/8/8/119
>> > > 
>> > 
>> > Ah, this is what I was referring to in the other mail.
>> > 
>> > However, the way you set the of_node might be dangerous too:
>> > We should generally not have two platform_device structures with
>> > the same of_node pointer, most importantly it may cause the
>> > child device to be bound to the same driver as the parent
>> > device since the probing is done by compatible string.
>> > 
>> > As you tested it successfully, it must work at the moment on your
>> > machine, but it could easily break depending on deferred probing
>> > or module load order.
>> > 
>> 
>> Currently, I work around above problems by setting core device of_node
>> as NULL at both probe error path and platform driver .remove routine.
>> 
>> I admit it is not a good way, but if we only have of_node at device's
>> life periods after probe, it seems ok currently. It is hard to create
>> of_node dynamically when create device, and keep some contents
>> of parent's of_node, and some are not.
>
> How about turning dwc3 into a library which can be used by a range of
> platform devices?  Wouldn't that solve all the current problems, and
> completely avoid the need to copy resources from one platform device
> to another?

This will break all existing DTs out there. Also, there are other
benefits from keeping current design, these have been discussed before
but here's a short summary:

. PM callbacks are kept simple
. We avoid abuse of internal dwc3 functions
. It's a lot less work to "port" dwc3 to "your SoC"
. We prevent another MUSB (drivers/usb/musb/)

And few others. Sure, they are rather subjective benefits, but it has
worked well so far. Also, breaking DT ABI is kind of a big deal.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Russell King - ARM Linux  writes:
> On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote:
>> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
>> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
>> > > 
>> > > The pre-condition of DT function at USB HCD core works is the host
>> > > controller device has of_node, since it is the root node for USB tree
>> > > described at DT. If the host controller device is not at DT, it needs
>> > > to try to get its of_node, the chipidea driver gets it through its
>> > > parent node [1]
>> > 
>> > > 
>> > > [1] https://lkml.org/lkml/2016/8/8/119
>> > > 
>> > 
>> > Ah, this is what I was referring to in the other mail.
>> > 
>> > However, the way you set the of_node might be dangerous too:
>> > We should generally not have two platform_device structures with
>> > the same of_node pointer, most importantly it may cause the
>> > child device to be bound to the same driver as the parent
>> > device since the probing is done by compatible string.
>> > 
>> > As you tested it successfully, it must work at the moment on your
>> > machine, but it could easily break depending on deferred probing
>> > or module load order.
>> > 
>> 
>> Currently, I work around above problems by setting core device of_node
>> as NULL at both probe error path and platform driver .remove routine.
>> 
>> I admit it is not a good way, but if we only have of_node at device's
>> life periods after probe, it seems ok currently. It is hard to create
>> of_node dynamically when create device, and keep some contents
>> of parent's of_node, and some are not.
>
> How about turning dwc3 into a library which can be used by a range of
> platform devices?  Wouldn't that solve all the current problems, and
> completely avoid the need to copy resources from one platform device
> to another?

This will break all existing DTs out there. Also, there are other
benefits from keeping current design, these have been discussed before
but here's a short summary:

. PM callbacks are kept simple
. We avoid abuse of internal dwc3 functions
. It's a lot less work to "port" dwc3 to "your SoC"
. We prevent another MUSB (drivers/usb/musb/)

And few others. Sure, they are rather subjective benefits, but it has
worked well so far. Also, breaking DT ABI is kind of a big deal.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 10:48:06AM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote:
> > > 
> > > Right, that should make it work with iommu as well. However, it does
> > > not solve the other issue I mentioned above, with boards that have
> > > USB devices hardwired to a chipidea host controller that need
> > > configuration from DT. For that, we still need to come up with another
> > > way to associate the DT hierarchy in the host bridge node with
> > > the Linux platform_device.
> > > 
> > 
> > Why? The DMA configuration is for host controller, not for USB device.
> > No matter there is hardwired or hotplug devices, the DMA configuration
> > for host controller are both inherited from glue layer platform devices,
> > current implementation is at function ci_hdrc_add_device,
> > drivers/usb/chipidea/core.c.
> 
> I wasn't referring to DMA configuration there, but only to how we
> set the of_node pointer in register_root_hub, which you added in
> dc5878abf49 ("usb: core: move root hub's device node assignment after
> it is added to bus") as:
> 
>   usb_dev->dev.of_node = parent_dev->of_node;
> 
> As I understand, parent_dev (aka hcd->self.controller) here
> refers to the device that you add in ci_hdrc_add_device(),
> which does not have an of_node pointer, so we actually
> want parent_dev->parent->of_node.

For platform devices, like chipidea and dwc3, it is correct, since
they don't have of_node. If the host controller has of_node, it
is controller's of_node. In order to let USB HCD core life be easy,
we need to let hcd->self.controller own of_node when it is added
to HCD core, no matter it has from the dts or get it dynamically.

> 
> I'm sure you understand that code better than me, so let me
> know what my mistake is if this indeed works correctly.
> 

Your understanding is correct, just some have of_node from dts, some
(chipidea/dwc3) need to have assignment at its driver.

> 
> 
> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> I think we should replace 
> 
> pdev->dev.dma_mask = dev->dma_mask;
> pdev->dev.dma_parms = dev->dma_parms;
> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> 
> with of_dma_configure(), which has the chance to configure more than
> just those three, as the dma API might look into different aspects:
> 
> - iommu specific configuration
> - cache coherency information
> - bus type
> - dma offset
> - dma_map_ops pointer
> 
> We try to handle everything in of_dma_configure() at configuration
> time, and that would be the place to add anything else that we might
> need in the future.
> 

Yes, I agree with you, but just like Felipe mentioned, we also need to
consider PCI device, can we do something like gpiod_get_index does? Are
there any similar APIs like of_dma_configure for ACPI?

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 10:48:06AM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote:
> > > 
> > > Right, that should make it work with iommu as well. However, it does
> > > not solve the other issue I mentioned above, with boards that have
> > > USB devices hardwired to a chipidea host controller that need
> > > configuration from DT. For that, we still need to come up with another
> > > way to associate the DT hierarchy in the host bridge node with
> > > the Linux platform_device.
> > > 
> > 
> > Why? The DMA configuration is for host controller, not for USB device.
> > No matter there is hardwired or hotplug devices, the DMA configuration
> > for host controller are both inherited from glue layer platform devices,
> > current implementation is at function ci_hdrc_add_device,
> > drivers/usb/chipidea/core.c.
> 
> I wasn't referring to DMA configuration there, but only to how we
> set the of_node pointer in register_root_hub, which you added in
> dc5878abf49 ("usb: core: move root hub's device node assignment after
> it is added to bus") as:
> 
>   usb_dev->dev.of_node = parent_dev->of_node;
> 
> As I understand, parent_dev (aka hcd->self.controller) here
> refers to the device that you add in ci_hdrc_add_device(),
> which does not have an of_node pointer, so we actually
> want parent_dev->parent->of_node.

For platform devices, like chipidea and dwc3, it is correct, since
they don't have of_node. If the host controller has of_node, it
is controller's of_node. In order to let USB HCD core life be easy,
we need to let hcd->self.controller own of_node when it is added
to HCD core, no matter it has from the dts or get it dynamically.

> 
> I'm sure you understand that code better than me, so let me
> know what my mistake is if this indeed works correctly.
> 

Your understanding is correct, just some have of_node from dts, some
(chipidea/dwc3) need to have assignment at its driver.

> 
> 
> Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
> I think we should replace 
> 
> pdev->dev.dma_mask = dev->dma_mask;
> pdev->dev.dma_parms = dev->dma_parms;
> dma_set_coherent_mask(>dev, dev->coherent_dma_mask);
> 
> with of_dma_configure(), which has the chance to configure more than
> just those three, as the dma API might look into different aspects:
> 
> - iommu specific configuration
> - cache coherency information
> - bus type
> - dma offset
> - dma_map_ops pointer
> 
> We try to handle everything in of_dma_configure() at configuration
> time, and that would be the place to add anything else that we might
> need in the future.
> 

Yes, I agree with you, but just like Felipe mentioned, we also need to
consider PCI device, can we do something like gpiod_get_index does? Are
there any similar APIs like of_dma_configure for ACPI?

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Russell King - ARM Linux]

On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote:
> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> > > 
> > > The pre-condition of DT function at USB HCD core works is the host
> > > controller device has of_node, since it is the root node for USB tree
> > > described at DT. If the host controller device is not at DT, it needs
> > > to try to get its of_node, the chipidea driver gets it through its
> > > parent node [1]
> > 
> > > 
> > > [1] https://lkml.org/lkml/2016/8/8/119
> > > 
> > 
> > Ah, this is what I was referring to in the other mail.
> > 
> > However, the way you set the of_node might be dangerous too:
> > We should generally not have two platform_device structures with
> > the same of_node pointer, most importantly it may cause the
> > child device to be bound to the same driver as the parent
> > device since the probing is done by compatible string.
> > 
> > As you tested it successfully, it must work at the moment on your
> > machine, but it could easily break depending on deferred probing
> > or module load order.
> > 
> 
> Currently, I work around above problems by setting core device of_node
> as NULL at both probe error path and platform driver .remove routine.
> 
> I admit it is not a good way, but if we only have of_node at device's
> life periods after probe, it seems ok currently. It is hard to create
> of_node dynamically when create device, and keep some contents
> of parent's of_node, and some are not.

How about turning dwc3 into a library which can be used by a range of
platform devices?  Wouldn't that solve all the current problems, and
completely avoid the need to copy resources from one platform device
to another?

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Russell King - ARM Linux]

On Wed, Sep 07, 2016 at 05:29:01PM +0800, Peter Chen wrote:
> On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> > > 
> > > The pre-condition of DT function at USB HCD core works is the host
> > > controller device has of_node, since it is the root node for USB tree
> > > described at DT. If the host controller device is not at DT, it needs
> > > to try to get its of_node, the chipidea driver gets it through its
> > > parent node [1]
> > 
> > > 
> > > [1] https://lkml.org/lkml/2016/8/8/119
> > > 
> > 
> > Ah, this is what I was referring to in the other mail.
> > 
> > However, the way you set the of_node might be dangerous too:
> > We should generally not have two platform_device structures with
> > the same of_node pointer, most importantly it may cause the
> > child device to be bound to the same driver as the parent
> > device since the probing is done by compatible string.
> > 
> > As you tested it successfully, it must work at the moment on your
> > machine, but it could easily break depending on deferred probing
> > or module load order.
> > 
> 
> Currently, I work around above problems by setting core device of_node
> as NULL at both probe error path and platform driver .remove routine.
> 
> I admit it is not a good way, but if we only have of_node at device's
> life periods after probe, it seems ok currently. It is hard to create
> of_node dynamically when create device, and keep some contents
> of parent's of_node, and some are not.

How about turning dwc3 into a library which can be used by a range of
platform devices?  Wouldn't that solve all the current problems, and
completely avoid the need to copy resources from one platform device
to another?

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> > 
> > The pre-condition of DT function at USB HCD core works is the host
> > controller device has of_node, since it is the root node for USB tree
> > described at DT. If the host controller device is not at DT, it needs
> > to try to get its of_node, the chipidea driver gets it through its
> > parent node [1]
> 
> > 
> > [1] https://lkml.org/lkml/2016/8/8/119
> > 
> 
> Ah, this is what I was referring to in the other mail.
> 
> However, the way you set the of_node might be dangerous too:
> We should generally not have two platform_device structures with
> the same of_node pointer, most importantly it may cause the
> child device to be bound to the same driver as the parent
> device since the probing is done by compatible string.
> 
> As you tested it successfully, it must work at the moment on your
> machine, but it could easily break depending on deferred probing
> or module load order.
> 

Currently, I work around above problems by setting core device of_node
as NULL at both probe error path and platform driver .remove routine.

I admit it is not a good way, but if we only have of_node at device's
life periods after probe, it seems ok currently. It is hard to create
of_node dynamically when create device, and keep some contents
of parent's of_node, and some are not.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Wed, Sep 07, 2016 at 10:52:46AM +0200, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> > 
> > The pre-condition of DT function at USB HCD core works is the host
> > controller device has of_node, since it is the root node for USB tree
> > described at DT. If the host controller device is not at DT, it needs
> > to try to get its of_node, the chipidea driver gets it through its
> > parent node [1]
> 
> > 
> > [1] https://lkml.org/lkml/2016/8/8/119
> > 
> 
> Ah, this is what I was referring to in the other mail.
> 
> However, the way you set the of_node might be dangerous too:
> We should generally not have two platform_device structures with
> the same of_node pointer, most importantly it may cause the
> child device to be bound to the same driver as the parent
> device since the probing is done by compatible string.
> 
> As you tested it successfully, it must work at the moment on your
> machine, but it could easily break depending on deferred probing
> or module load order.
> 

Currently, I work around above problems by setting core device of_node
as NULL at both probe error path and platform driver .remove routine.

I admit it is not a good way, but if we only have of_node at device's
life periods after probe, it seems ok currently. It is hard to create
of_node dynamically when create device, and keep some contents
of parent's of_node, and some are not.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> 
> The pre-condition of DT function at USB HCD core works is the host
> controller device has of_node, since it is the root node for USB tree
> described at DT. If the host controller device is not at DT, it needs
> to try to get its of_node, the chipidea driver gets it through its
> parent node [1]

> 
> [1] https://lkml.org/lkml/2016/8/8/119
> 

Ah, this is what I was referring to in the other mail.

However, the way you set the of_node might be dangerous too:
We should generally not have two platform_device structures with
the same of_node pointer, most importantly it may cause the
child device to be bound to the same driver as the parent
device since the probing is done by compatible string.

As you tested it successfully, it must work at the moment on your
machine, but it could easily break depending on deferred probing
or module load order.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 3:44:28 PM CEST Peter Chen wrote:
> 
> The pre-condition of DT function at USB HCD core works is the host
> controller device has of_node, since it is the root node for USB tree
> described at DT. If the host controller device is not at DT, it needs
> to try to get its of_node, the chipidea driver gets it through its
> parent node [1]

> 
> [1] https://lkml.org/lkml/2016/8/8/119
> 

Ah, this is what I was referring to in the other mail.

However, the way you set the of_node might be dangerous too:
We should generally not have two platform_device structures with
the same of_node pointer, most importantly it may cause the
child device to be bound to the same driver as the parent
device since the probing is done by compatible string.

As you tested it successfully, it must work at the moment on your
machine, but it could easily break depending on deferred probing
or module load order.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote:
> > 
> > Right, that should make it work with iommu as well. However, it does
> > not solve the other issue I mentioned above, with boards that have
> > USB devices hardwired to a chipidea host controller that need
> > configuration from DT. For that, we still need to come up with another
> > way to associate the DT hierarchy in the host bridge node with
> > the Linux platform_device.
> > 
> 
> Why? The DMA configuration is for host controller, not for USB device.
> No matter there is hardwired or hotplug devices, the DMA configuration
> for host controller are both inherited from glue layer platform devices,
> current implementation is at function ci_hdrc_add_device,
> drivers/usb/chipidea/core.c.

I wasn't referring to DMA configuration there, but only to how we
set the of_node pointer in register_root_hub, which you added in
dc5878abf49 ("usb: core: move root hub's device node assignment after
it is added to bus") as:

usb_dev->dev.of_node = parent_dev->of_node;

As I understand, parent_dev (aka hcd->self.controller) here
refers to the device that you add in ci_hdrc_add_device(),
which does not have an of_node pointer, so we actually
want parent_dev->parent->of_node.

I'm sure you understand that code better than me, so let me
know what my mistake is if this indeed works correctly.



Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
I think we should replace 

pdev->dev.dma_mask = dev->dma_mask;
pdev->dev.dma_parms = dev->dma_parms;
dma_set_coherent_mask(>dev, dev->coherent_dma_mask);

with of_dma_configure(), which has the chance to configure more than
just those three, as the dma API might look into different aspects:

- iommu specific configuration
- cache coherency information
- bus type
- dma offset
- dma_map_ops pointer

We try to handle everything in of_dma_configure() at configuration
time, and that would be the place to add anything else that we might
need in the future.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 2:33:13 PM CEST Peter Chen wrote:
> > 
> > Right, that should make it work with iommu as well. However, it does
> > not solve the other issue I mentioned above, with boards that have
> > USB devices hardwired to a chipidea host controller that need
> > configuration from DT. For that, we still need to come up with another
> > way to associate the DT hierarchy in the host bridge node with
> > the Linux platform_device.
> > 
> 
> Why? The DMA configuration is for host controller, not for USB device.
> No matter there is hardwired or hotplug devices, the DMA configuration
> for host controller are both inherited from glue layer platform devices,
> current implementation is at function ci_hdrc_add_device,
> drivers/usb/chipidea/core.c.

I wasn't referring to DMA configuration there, but only to how we
set the of_node pointer in register_root_hub, which you added in
dc5878abf49 ("usb: core: move root hub's device node assignment after
it is added to bus") as:

usb_dev->dev.of_node = parent_dev->of_node;

As I understand, parent_dev (aka hcd->self.controller) here
refers to the device that you add in ci_hdrc_add_device(),
which does not have an of_node pointer, so we actually
want parent_dev->parent->of_node.

I'm sure you understand that code better than me, so let me
know what my mistake is if this indeed works correctly.



Regarding the DMA configuration that you mention in ci_hdrc_add_device(),
I think we should replace 

pdev->dev.dma_mask = dev->dma_mask;
pdev->dev.dma_parms = dev->dma_parms;
dma_set_coherent_mask(>dev, dev->coherent_dma_mask);

with of_dma_configure(), which has the chance to configure more than
just those three, as the dma API might look into different aspects:

- iommu specific configuration
- cache coherency information
- bus type
- dma offset
- dma_map_ops pointer

We try to handle everything in of_dma_configure() at configuration
time, and that would be the place to add anything else that we might
need in the future.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote:
> > 
> > Speaking of that flag, I suppose we need the same logic to know where
> > to look for USB devices attached to a dwc3 host when we need to describe
> > them in DT. By default we look for child device nodes under the
> > node of the HCD device node, but that would be wrong here too.
> 
> I didn't get this part. Information about USB devices attached to a USB host
> is never provided in DT because they are always dynamically created via
> usb_new_device(), whether they are hard-wired on the board or hot-plugged.
> 
> These USB devices inherit their DMA masks in the usb_alloc_dev() routine
> whereas each interface within the USB device inherits its DMA mask in
> usb_set_configuration().

We had talked about adding support for this for at least six years (probably
much more), but Peter Chen finally added it this year in commit 69bec72598
("USB: core: let USB device know device node").

The main use for it is to let you specify a MAC address for on-board
ethernet devices that lack an EPROM, but any other information can be
added that way too.

> There is a bug  in the USB core because of which the ISB device and interfaces
> do not inherit dma_pfn_offset correctly for which I've sent a patch
> https://lkml.org/lkml/2016/8/17/275

I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
also set the dma_pfn_offset, but what exactly is this used for in USB
devices?

As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA
mapping interface, but that can't really be used on USB devices, which
I assume use usb_alloc_coherent() and the URB interfaces for passing data
between a USB driver and the HCD. My knowledge of USB device drivers
is a bit lacking, so it's possible I'm misunderstanding things here.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday, September 7, 2016 10:17:31 AM CEST Roger Quadros wrote:
> > 
> > Speaking of that flag, I suppose we need the same logic to know where
> > to look for USB devices attached to a dwc3 host when we need to describe
> > them in DT. By default we look for child device nodes under the
> > node of the HCD device node, but that would be wrong here too.
> 
> I didn't get this part. Information about USB devices attached to a USB host
> is never provided in DT because they are always dynamically created via
> usb_new_device(), whether they are hard-wired on the board or hot-plugged.
> 
> These USB devices inherit their DMA masks in the usb_alloc_dev() routine
> whereas each interface within the USB device inherits its DMA mask in
> usb_set_configuration().

We had talked about adding support for this for at least six years (probably
much more), but Peter Chen finally added it this year in commit 69bec72598
("USB: core: let USB device know device node").

The main use for it is to let you specify a MAC address for on-board
ethernet devices that lack an EPROM, but any other information can be
added that way too.

> There is a bug  in the USB core because of which the ISB device and interfaces
> do not inherit dma_pfn_offset correctly for which I've sent a patch
> https://lkml.org/lkml/2016/8/17/275

I'm a bit skeptical about this. Clearly if we set the dma_mask, we should
also set the dma_pfn_offset, but what exactly is this used for in USB
devices?

As I understand it, the dma_mask/dma_pfn_offset etc is used for the DMA
mapping interface, but that can't really be used on USB devices, which
I assume use usb_alloc_coherent() and the URB interfaces for passing data
between a USB driver and the HCD. My knowledge of USB device drivers
is a bit lacking, so it's possible I'm misunderstanding things here.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Tue, Sep 06, 2016 at 03:27:43PM +0200, Arnd Bergmann wrote:
> On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
> > Hi,
> > 
> > Arnd Bergmann  writes:
> > > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> > >> 
> > >> this only solves the problem for DT devices. Legacy devices and
> > >> PCI-based systems will still suffer from the same problem. At least for
> > >> dwc3, I will only be taking patches that solve the problem for all
> > >> users, not a subset of them.
> > >
> > > I don't think legacy devices are a worry, because they wouldn't
> > > have this problem. For the PCI case, you are right that it cannot
> > > work, in particular for machines that have complex IOMMU setup.
> > >
> > > Some architectures (at least arm64 and sparc) check the bus_type of
> > > a device in order to find the correct set of dma_map_ops for that
> > > device, so there is no real way to handle this as long as you
> > > pass a platform_device into an API that expects a pci_device.
> > 
> > Then I guess we're left with adding a "struct device *dma_dev" to struct
> > dwc3 and trying to figure out if we should use parent or self. Does
> > anybody see any problems with that?
> 
> I think we actually need the device pointer in the usb_hcd structure,
> so it can be passed in these API calls from the USB core
> 
> drivers/usb/core/buffer.c:  return 
> dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
> drivers/usb/core/buffer.c:  dma_free_coherent(hcd->self.controller, size, 
> addr, dma);
> drivers/usb/core/buffer.c:  (!hcd->self.controller->dma_mask &&
> drivers/usb/core/buffer.c:  hcd->pool[i] = dma_pool_create(name, 
> hcd->self.controller,
> drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/hcd.c: n = dma_map_sg(
> drivers/usb/core/hcd.c: urb->transfer_dma = 
> dma_map_page(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/hcd.c: urb->transfer_dma = 
> dma_map_single(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller,
> drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents,
> drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
> drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
> drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents,
> 
> as these are all called on behalf of the host controller node.

The USB HCD core uses the struct device pointer passed by usb_create_hcd
which is called by each host controller driver, the host controller driver
needs to make sure the information (DMA configurations, of_node, etc)
in struct device are correct before calling usb_create_hcd.

> Looking for more instances of hcd->self.controller, I find this
> instance:
> 
> drivers/usb/core/hcd.c: struct usb_phy *phy = 
> usb_get_phy_dev(hcd->self.controller, 0);
> drivers/usb/core/hcd.c: struct phy *phy = 
> phy_get(hcd->self.controller, "usb");
> 
> I'm unsure which device pointer we want here, but I suspect this also
> needs to be the one that has the device node in order to make the lookup
> of the phy structure by device node work right. Can you clarify how
> this works today?
> 

The above codes are only called when the host controller driver does not
the code which try to get USB PHY. Once the PHY drivers is probed, the
above code can work no matter DT or non-DT.

But by looking at the code, I am wondering how dwc3 host get its USB PHY at
xhci-platform.c, it seems there is no of_node at xhci-hcd for dwc3.

> We probably also need to add the of_node of the host controller device
> to struct usb_hcd in order to make usb_of_get_child_node() work
> in the case where the hcd itself is not device that is listed
> in DT.

The pre-condition of DT function at USB HCD core works is the host
controller device has of_node, since it is the root node for USB tree
described at DT. If the host controller device is not at DT, it needs
to try to get its of_node, the chipidea driver gets it through its
parent node [1]

> It might be a good idea to use 'struct fwnode_handle' for that,
> so we can in the future also allow ACPI platforms to specify 
> 
> > Note, we would NOT be passing device pointers are platform_data, we
> > would have dwc3.ko figure out if it should use self or its parent device
> > for dma.
> 
> Ok, sounds good.
> 
>   Arnd

[1] https://lkml.org/lkml/2016/8/8/119

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Tue, Sep 06, 2016 at 03:27:43PM +0200, Arnd Bergmann wrote:
> On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
> > Hi,
> > 
> > Arnd Bergmann  writes:
> > > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> > >> 
> > >> this only solves the problem for DT devices. Legacy devices and
> > >> PCI-based systems will still suffer from the same problem. At least for
> > >> dwc3, I will only be taking patches that solve the problem for all
> > >> users, not a subset of them.
> > >
> > > I don't think legacy devices are a worry, because they wouldn't
> > > have this problem. For the PCI case, you are right that it cannot
> > > work, in particular for machines that have complex IOMMU setup.
> > >
> > > Some architectures (at least arm64 and sparc) check the bus_type of
> > > a device in order to find the correct set of dma_map_ops for that
> > > device, so there is no real way to handle this as long as you
> > > pass a platform_device into an API that expects a pci_device.
> > 
> > Then I guess we're left with adding a "struct device *dma_dev" to struct
> > dwc3 and trying to figure out if we should use parent or self. Does
> > anybody see any problems with that?
> 
> I think we actually need the device pointer in the usb_hcd structure,
> so it can be passed in these API calls from the USB core
> 
> drivers/usb/core/buffer.c:  return 
> dma_alloc_coherent(hcd->self.controller, size, dma, mem_flags);
> drivers/usb/core/buffer.c:  dma_free_coherent(hcd->self.controller, size, 
> addr, dma);
> drivers/usb/core/buffer.c:  (!hcd->self.controller->dma_mask &&
> drivers/usb/core/buffer.c:  hcd->pool[i] = dma_pool_create(name, 
> hcd->self.controller,
> drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/hcd.c: n = dma_map_sg(
> drivers/usb/core/hcd.c: urb->transfer_dma = 
> dma_map_page(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/hcd.c: urb->transfer_dma = 
> dma_map_single(
> drivers/usb/core/hcd.c: if 
> (dma_mapping_error(hcd->self.controller,
> drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller,
> drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents,
> drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
> drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
> drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents,
> 
> as these are all called on behalf of the host controller node.

The USB HCD core uses the struct device pointer passed by usb_create_hcd
which is called by each host controller driver, the host controller driver
needs to make sure the information (DMA configurations, of_node, etc)
in struct device are correct before calling usb_create_hcd.

> Looking for more instances of hcd->self.controller, I find this
> instance:
> 
> drivers/usb/core/hcd.c: struct usb_phy *phy = 
> usb_get_phy_dev(hcd->self.controller, 0);
> drivers/usb/core/hcd.c: struct phy *phy = 
> phy_get(hcd->self.controller, "usb");
> 
> I'm unsure which device pointer we want here, but I suspect this also
> needs to be the one that has the device node in order to make the lookup
> of the phy structure by device node work right. Can you clarify how
> this works today?
> 

The above codes are only called when the host controller driver does not
the code which try to get USB PHY. Once the PHY drivers is probed, the
above code can work no matter DT or non-DT.

But by looking at the code, I am wondering how dwc3 host get its USB PHY at
xhci-platform.c, it seems there is no of_node at xhci-hcd for dwc3.

> We probably also need to add the of_node of the host controller device
> to struct usb_hcd in order to make usb_of_get_child_node() work
> in the case where the hcd itself is not device that is listed
> in DT.

The pre-condition of DT function at USB HCD core works is the host
controller device has of_node, since it is the root node for USB tree
described at DT. If the host controller device is not at DT, it needs
to try to get its of_node, the chipidea driver gets it through its
parent node [1]

> It might be a good idea to use 'struct fwnode_handle' for that,
> so we can in the future also allow ACPI platforms to specify 
> 
> > Note, we would NOT be passing device pointers are platform_data, we
> > would have dwc3.ko figure out if it should use self or its parent device
> > for dma.
> 
> Ok, sounds good.
> 
>   Arnd

[1] https://lkml.org/lkml/2016/8/8/119

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Roger Quadros]

Hi Arnd,

On 02/09/16 18:51, Arnd Bergmann wrote:
> On Friday, September 2, 2016 10:21:23 AM CEST Alan Stern wrote:
>> On Fri, 2 Sep 2016, Felipe Balbi wrote:
>>
>>> Hi,
>>>
>>> Russell King - ARM Linux  writes:
 On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
>>
>> Hi Felipe and Arnd,
>>
>> It has been a while since the last response to this discussion, but we
>> haven't reached an agreement yet!  Can we get to a conclusion on if it
>> is valid to create child platform device for abstraction purpose?  If
>> yes, can this child device do DMA by itself?
>
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.

 That's absolutely right.  Consider the USB model - only the USB host
 controller can perform DMA, not the USB devices themselves.  All DMA
 mappings need to be mapped using the USB host controller device struct
 not the USB device struct.

 The same _should_ be true everywhere else: the struct device representing
 the device performing DMA must be the one used to map the transfer.
>>>
>>> How do we fix dwc3 in dual-role, then?
>>>
>>> Peripheral-side dwc3 is easy, we just require a glue-layer to be present
>>> and use dwc3.ko's parent device (which will be the PCI device or OF
>>> device). But for host side dwc3, the problem is slightly more complex
>>> because we're using xhci-plat.ko by just instantiating a xhci-platform
>>> device so xhci-plat can probe.
>>>
>>> xhci core has no means to know if its own device or the parent of its
>>> parent should be used for DMA. Any ideas?
>>
>> In theory, you can store a flag somewhere in the platform device,
>> something that would tell xhci-hcd that it has to use the parent's
>> parent for DMA purposes.
>>
>> I know it would be somewhat of a hack, but ought to work.
> 
> Speaking of that flag, I suppose we need the same logic to know where
> to look for USB devices attached to a dwc3 host when we need to describe
> them in DT. By default we look for child device nodes under the
> node of the HCD device node, but that would be wrong here too.

I didn't get this part. Information about USB devices attached to a USB host
is never provided in DT because they are always dynamically created via
usb_new_device(), whether they are hard-wired on the board or hot-plugged.

These USB devices inherit their DMA masks in the usb_alloc_dev() routine
whereas each interface within the USB device inherits its DMA mask in
usb_set_configuration().

There is a bug  in the USB core because of which the ISB device and interfaces
do not inherit dma_pfn_offset correctly for which I've sent a patch
https://lkml.org/lkml/2016/8/17/275

cheers,
-roger

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Roger Quadros]

Hi Arnd,

On 02/09/16 18:51, Arnd Bergmann wrote:
> On Friday, September 2, 2016 10:21:23 AM CEST Alan Stern wrote:
>> On Fri, 2 Sep 2016, Felipe Balbi wrote:
>>
>>> Hi,
>>>
>>> Russell King - ARM Linux  writes:
 On Fri, Sep 02, 2016 at 12:43:39PM +0200, Arnd Bergmann wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
>>
>> Hi Felipe and Arnd,
>>
>> It has been a while since the last response to this discussion, but we
>> haven't reached an agreement yet!  Can we get to a conclusion on if it
>> is valid to create child platform device for abstraction purpose?  If
>> yes, can this child device do DMA by itself?
>
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.

 That's absolutely right.  Consider the USB model - only the USB host
 controller can perform DMA, not the USB devices themselves.  All DMA
 mappings need to be mapped using the USB host controller device struct
 not the USB device struct.

 The same _should_ be true everywhere else: the struct device representing
 the device performing DMA must be the one used to map the transfer.
>>>
>>> How do we fix dwc3 in dual-role, then?
>>>
>>> Peripheral-side dwc3 is easy, we just require a glue-layer to be present
>>> and use dwc3.ko's parent device (which will be the PCI device or OF
>>> device). But for host side dwc3, the problem is slightly more complex
>>> because we're using xhci-plat.ko by just instantiating a xhci-platform
>>> device so xhci-plat can probe.
>>>
>>> xhci core has no means to know if its own device or the parent of its
>>> parent should be used for DMA. Any ideas?
>>
>> In theory, you can store a flag somewhere in the platform device,
>> something that would tell xhci-hcd that it has to use the parent's
>> parent for DMA purposes.
>>
>> I know it would be somewhat of a hack, but ought to work.
> 
> Speaking of that flag, I suppose we need the same logic to know where
> to look for USB devices attached to a dwc3 host when we need to describe
> them in DT. By default we look for child device nodes under the
> node of the HCD device node, but that would be wrong here too.

I didn't get this part. Information about USB devices attached to a USB host
is never provided in DT because they are always dynamically created via
usb_new_device(), whether they are hard-wired on the board or hot-plugged.

These USB devices inherit their DMA masks in the usb_alloc_dev() routine
whereas each interface within the USB device inherits its DMA mask in
usb_set_configuration().

There is a bug  in the USB core because of which the ISB device and interfaces
do not inherit dma_pfn_offset correctly for which I've sent a patch
https://lkml.org/lkml/2016/8/17/275

cheers,
-roger

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Arnd Bergmann  writes:

> On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
>> Hi,
>> 
>> Arnd Bergmann  writes:
>> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
>> >> 
>> >> this only solves the problem for DT devices. Legacy devices and
>> >> PCI-based systems will still suffer from the same problem. At least for
>> >> dwc3, I will only be taking patches that solve the problem for all
>> >> users, not a subset of them.
>> >
>> > I don't think legacy devices are a worry, because they wouldn't
>> > have this problem. For the PCI case, you are right that it cannot
>> > work, in particular for machines that have complex IOMMU setup.
>> >
>> > Some architectures (at least arm64 and sparc) check the bus_type of
>> > a device in order to find the correct set of dma_map_ops for that
>> > device, so there is no real way to handle this as long as you
>> > pass a platform_device into an API that expects a pci_device.
>> 
>> Then I guess we're left with adding a "struct device *dma_dev" to struct
>> dwc3 and trying to figure out if we should use parent or self. Does
>> anybody see any problems with that?
>
> I think we actually need the device pointer in the usb_hcd structure,
> so it can be passed in these API calls from the USB core

that's for host side. I'm concerned about peripheral side

> as these are all called on behalf of the host controller node.
> Looking for more instances of hcd->self.controller, I find this
> instance:
>
> drivers/usb/core/hcd.c: struct usb_phy *phy = 
> usb_get_phy_dev(hcd->self.controller, 0);
> drivers/usb/core/hcd.c: struct phy *phy = 
> phy_get(hcd->self.controller, "usb");
>
> I'm unsure which device pointer we want here, but I suspect this also
> needs to be the one that has the device node in order to make the lookup
> of the phy structure by device node work right. Can you clarify how
> this works today?

sounds correct to me.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Arnd Bergmann  writes:

> On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
>> Hi,
>> 
>> Arnd Bergmann  writes:
>> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
>> >> 
>> >> this only solves the problem for DT devices. Legacy devices and
>> >> PCI-based systems will still suffer from the same problem. At least for
>> >> dwc3, I will only be taking patches that solve the problem for all
>> >> users, not a subset of them.
>> >
>> > I don't think legacy devices are a worry, because they wouldn't
>> > have this problem. For the PCI case, you are right that it cannot
>> > work, in particular for machines that have complex IOMMU setup.
>> >
>> > Some architectures (at least arm64 and sparc) check the bus_type of
>> > a device in order to find the correct set of dma_map_ops for that
>> > device, so there is no real way to handle this as long as you
>> > pass a platform_device into an API that expects a pci_device.
>> 
>> Then I guess we're left with adding a "struct device *dma_dev" to struct
>> dwc3 and trying to figure out if we should use parent or self. Does
>> anybody see any problems with that?
>
> I think we actually need the device pointer in the usb_hcd structure,
> so it can be passed in these API calls from the USB core

that's for host side. I'm concerned about peripheral side

> as these are all called on behalf of the host controller node.
> Looking for more instances of hcd->self.controller, I find this
> instance:
>
> drivers/usb/core/hcd.c: struct usb_phy *phy = 
> usb_get_phy_dev(hcd->self.controller, 0);
> drivers/usb/core/hcd.c: struct phy *phy = 
> phy_get(hcd->self.controller, "usb");
>
> I'm unsure which device pointer we want here, but I suspect this also
> needs to be the one that has the device node in order to make the lookup
> of the phy structure by device node work right. Can you clarify how
> this works today?

sounds correct to me.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Tue, Sep 06, 2016 at 12:38:29PM +0200, Arnd Bergmann wrote:
> On Tuesday, September 6, 2016 2:35:29 PM CEST Peter Chen wrote:
> > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote:
> > > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote:
> 
> > > 
> > > Most of these are probably never used with any nonstandard
> > > DMA settings (IOMMU, cache coherency, offset, ...).
> > > 
> > > One thing we could possibly do is to go through these and
> > > replace the hardcoded dma mask setup with of_dma_configure()
> > > in all cases in which we actually use DT for probing, which
> > > should cover the interesting cases.
> > > 
> > 
> > One case I am going to work is to let USB chipidea driver support iommu,
> > the chipidea core device is no of_node, and created by
> > platform_add_device on the runtime. Using of_dma_configure with parent
> > of_node is a solution from my point, like [1].
> > 
> > https://lkml.org/lkml/2016/2/22/7
> 
> Right, that should make it work with iommu as well. However, it does
> not solve the other issue I mentioned above, with boards that have
> USB devices hardwired to a chipidea host controller that need
> configuration from DT. For that, we still need to come up with another
> way to associate the DT hierarchy in the host bridge node with
> the Linux platform_device.
> 

Why? The DMA configuration is for host controller, not for USB device.
No matter there is hardwired or hotplug devices, the DMA configuration
for host controller are both inherited from glue layer platform devices,
current implementation is at function ci_hdrc_add_device,
drivers/usb/chipidea/core.c.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Tue, Sep 06, 2016 at 12:38:29PM +0200, Arnd Bergmann wrote:
> On Tuesday, September 6, 2016 2:35:29 PM CEST Peter Chen wrote:
> > On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote:
> > > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote:
> 
> > > 
> > > Most of these are probably never used with any nonstandard
> > > DMA settings (IOMMU, cache coherency, offset, ...).
> > > 
> > > One thing we could possibly do is to go through these and
> > > replace the hardcoded dma mask setup with of_dma_configure()
> > > in all cases in which we actually use DT for probing, which
> > > should cover the interesting cases.
> > > 
> > 
> > One case I am going to work is to let USB chipidea driver support iommu,
> > the chipidea core device is no of_node, and created by
> > platform_add_device on the runtime. Using of_dma_configure with parent
> > of_node is a solution from my point, like [1].
> > 
> > https://lkml.org/lkml/2016/2/22/7
> 
> Right, that should make it work with iommu as well. However, it does
> not solve the other issue I mentioned above, with boards that have
> USB devices hardwired to a chipidea host controller that need
> configuration from DT. For that, we still need to come up with another
> way to associate the DT hierarchy in the host bridge node with
> the Linux platform_device.
> 

Why? The DMA configuration is for host controller, not for USB device.
No matter there is hardwired or hotplug devices, the DMA configuration
for host controller are both inherited from glue layer platform devices,
current implementation is at function ci_hdrc_add_device,
drivers/usb/chipidea/core.c.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
> Hi,
> 
> Arnd Bergmann  writes:
> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> >> 
> >> this only solves the problem for DT devices. Legacy devices and
> >> PCI-based systems will still suffer from the same problem. At least for
> >> dwc3, I will only be taking patches that solve the problem for all
> >> users, not a subset of them.
> >
> > I don't think legacy devices are a worry, because they wouldn't
> > have this problem. For the PCI case, you are right that it cannot
> > work, in particular for machines that have complex IOMMU setup.
> >
> > Some architectures (at least arm64 and sparc) check the bus_type of
> > a device in order to find the correct set of dma_map_ops for that
> > device, so there is no real way to handle this as long as you
> > pass a platform_device into an API that expects a pci_device.
> 
> Then I guess we're left with adding a "struct device *dma_dev" to struct
> dwc3 and trying to figure out if we should use parent or self. Does
> anybody see any problems with that?

I think we actually need the device pointer in the usb_hcd structure,
so it can be passed in these API calls from the USB core

drivers/usb/core/buffer.c:  return dma_alloc_coherent(hcd->self.controller, 
size, dma, mem_flags);
drivers/usb/core/buffer.c:  dma_free_coherent(hcd->self.controller, size, 
addr, dma);
drivers/usb/core/buffer.c:  (!hcd->self.controller->dma_mask &&
drivers/usb/core/buffer.c:  hcd->pool[i] = dma_pool_create(name, 
hcd->self.controller,
drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: n = dma_map_sg(
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_page(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller,
drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents,

as these are all called on behalf of the host controller node.
Looking for more instances of hcd->self.controller, I find this
instance:

drivers/usb/core/hcd.c: struct usb_phy *phy = 
usb_get_phy_dev(hcd->self.controller, 0);
drivers/usb/core/hcd.c: struct phy *phy = phy_get(hcd->self.controller, 
"usb");

I'm unsure which device pointer we want here, but I suspect this also
needs to be the one that has the device node in order to make the lookup
of the phy structure by device node work right. Can you clarify how
this works today?

We probably also need to add the of_node of the host controller device
to struct usb_hcd in order to make usb_of_get_child_node() work
in the case where the hcd itself is not device that is listed
in DT. It might be a good idea to use 'struct fwnode_handle' for that,
so we can in the future also allow ACPI platforms to specify 

> Note, we would NOT be passing device pointers are platform_data, we
> would have dwc3.ko figure out if it should use self or its parent device
> for dma.

Ok, sounds good.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
> Hi,
> 
> Arnd Bergmann  writes:
> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> >> 
> >> this only solves the problem for DT devices. Legacy devices and
> >> PCI-based systems will still suffer from the same problem. At least for
> >> dwc3, I will only be taking patches that solve the problem for all
> >> users, not a subset of them.
> >
> > I don't think legacy devices are a worry, because they wouldn't
> > have this problem. For the PCI case, you are right that it cannot
> > work, in particular for machines that have complex IOMMU setup.
> >
> > Some architectures (at least arm64 and sparc) check the bus_type of
> > a device in order to find the correct set of dma_map_ops for that
> > device, so there is no real way to handle this as long as you
> > pass a platform_device into an API that expects a pci_device.
> 
> Then I guess we're left with adding a "struct device *dma_dev" to struct
> dwc3 and trying to figure out if we should use parent or self. Does
> anybody see any problems with that?

I think we actually need the device pointer in the usb_hcd structure,
so it can be passed in these API calls from the USB core

drivers/usb/core/buffer.c:  return dma_alloc_coherent(hcd->self.controller, 
size, dma, mem_flags);
drivers/usb/core/buffer.c:  dma_free_coherent(hcd->self.controller, size, 
addr, dma);
drivers/usb/core/buffer.c:  (!hcd->self.controller->dma_mask &&
drivers/usb/core/buffer.c:  hcd->pool[i] = dma_pool_create(name, 
hcd->self.controller,
drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: n = dma_map_sg(
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_page(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller,
drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents,

as these are all called on behalf of the host controller node.
Looking for more instances of hcd->self.controller, I find this
instance:

drivers/usb/core/hcd.c: struct usb_phy *phy = 
usb_get_phy_dev(hcd->self.controller, 0);
drivers/usb/core/hcd.c: struct phy *phy = phy_get(hcd->self.controller, 
"usb");

I'm unsure which device pointer we want here, but I suspect this also
needs to be the one that has the device node in order to make the lookup
of the phy structure by device node work right. Can you clarify how
this works today?

We probably also need to add the of_node of the host controller device
to struct usb_hcd in order to make usb_of_get_child_node() work
in the case where the hcd itself is not device that is listed
in DT. It might be a good idea to use 'struct fwnode_handle' for that,
so we can in the future also allow ACPI platforms to specify 

> Note, we would NOT be passing device pointers are platform_data, we
> would have dwc3.ko figure out if it should use self or its parent device
> for dma.

Ok, sounds good.

Arnd

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
>> 
>> this only solves the problem for DT devices. Legacy devices and
>> PCI-based systems will still suffer from the same problem. At least for
>> dwc3, I will only be taking patches that solve the problem for all
>> users, not a subset of them.
>
> I don't think legacy devices are a worry, because they wouldn't
> have this problem. For the PCI case, you are right that it cannot
> work, in particular for machines that have complex IOMMU setup.
>
> Some architectures (at least arm64 and sparc) check the bus_type of
> a device in order to find the correct set of dma_map_ops for that
> device, so there is no real way to handle this as long as you
> pass a platform_device into an API that expects a pci_device.

Then I guess we're left with adding a "struct device *dma_dev" to struct
dwc3 and trying to figure out if we should use parent or self. Does
anybody see any problems with that?

Note, we would NOT be passing device pointers are platform_data, we
would have dwc3.ko figure out if it should use self or its parent device
for dma.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
>> 
>> this only solves the problem for DT devices. Legacy devices and
>> PCI-based systems will still suffer from the same problem. At least for
>> dwc3, I will only be taking patches that solve the problem for all
>> users, not a subset of them.
>
> I don't think legacy devices are a worry, because they wouldn't
> have this problem. For the PCI case, you are right that it cannot
> work, in particular for machines that have complex IOMMU setup.
>
> Some architectures (at least arm64 and sparc) check the bus_type of
> a device in order to find the correct set of dma_map_ops for that
> device, so there is no real way to handle this as long as you
> pass a platform_device into an API that expects a pci_device.

Then I guess we're left with adding a "struct device *dma_dev" to struct
dwc3 and trying to figure out if we should use parent or self. Does
anybody see any problems with that?

Note, we would NOT be passing device pointers are platform_data, we
would have dwc3.ko figure out if it should use self or its parent device
for dma.

-- 
balbi


signature.asc
Description: PGP signature