RE: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration

[Felipe Balbi <felipe.balbi@linux.intel.com>]

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


Hi,

Sriram Dash <sriram.dash@nxp.com> writes:
>>From: Felipe Balbi [mailto:felipe.balbi@linux.intel.com]
>>
>>
>>Hi,
>
> Hello Felipe,
>
>>
>>Sriram Dash <sriram.dash@nxp.com> writes:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> The dma ops for dwc3 devices are not set properly. So, use a physical
>>> device sysdev, which will be inherited from parent, to set the
>>> hardware / firmware parameters like dma.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
>>> ---
>>> Changes in v3:
>>>   - No update
>>>
>>> Changes in v2:
>>>   - integrate dwc3 driver changes together
>>>
>>>  drivers/usb/dwc3/core.c   | 28 +++++++++++++++-------------
>>>  drivers/usb/dwc3/core.h   |  1 +
>>>  drivers/usb/dwc3/ep0.c    |  8 ++++----
>>>  drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++------------------
>>>  drivers/usb/dwc3/host.c   | 12 ++++--------
>>>  5 files changed, 43 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
>>> 7287a76..0af0dc0 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/slab.h>
>>>  #include <linux/spinlock.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pci.h>
>>
>>I'd prefer to add a device property instead of checking for PCI bus type.
>>
>>> @@ -943,6 +944,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 == &pci_bus_type)
>>> +		dwc->sysdev = dwc->dev->parent;
>>> +	else
>>> +#endif
>>
>>IOW:
>>
>>        dwc->sysdev_is_parent = device_property_read_bool(dev,
>>        		"linux,sysdev_is_parent");
>>
>>[...]
>>
>>	if (dwc->sysdev_is_parent)
>>        	dwc->sysdev = dwc->dev->parent;
>>	else
>>        	dwc->sysdev = dwc->dev;
>>
>
> I am with you in the fact that the core should not worry about pci.
> This change you proposed is also appealing. But, if we are going with
> this solution, all the clients which are using dwc3 pci have to 
> mention the dts property "linux,sysdev_is_parent". This will be requiring

PCI doesn't use DTS ;-) You're gonna need something like so:

1 file changed, 10 insertions(+)
drivers/usb/dwc3/dwc3-pci.c | 10 ++++++++++

modified   drivers/usb/dwc3/dwc3-pci.c
@@ -73,6 +73,16 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
 	struct platform_device		*dwc3 = dwc->dwc3;
 	struct pci_dev			*pdev = dwc->pci;
+	int				ret;
+
+	struct property_entry sysdev_property[] = {
+		PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
+		{ },
+	};
+
+	ret = platform_device_add_properties(dwc3, sysdev_property);
+	if (ret)
+		return ret;
 
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
 	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {

> a lot of testing and proper changes from the dts, or it might break the
> functionality for dwc3 pci clients. So, IMO, we could postpone this change
> and try it out in future.

not taking any ifdefs, sorry.

-- 
balbi

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