Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1518866 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-11-10 12:10 +0100
    RE: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Sriram Dash <sriram.dash@nxp.com> - 2016-11-11 11:50 +0100
      RE: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-11-11 12:00 +0100
    Re: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Arnd Bergmann <arnd@arndb.de> - 2016-11-11 21:40 +0100
      Re: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Peter Chen <hzpeterchen@gmail.com> - 2016-11-14 03:00 +0100
        RE: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration Sriram Dash <sriram.dash@nxp.com> - 2016-11-14 06:30 +0100

#1518866 — Re: [PATCH v3 5/6] usb: dwc3: use bus->sysdev for DMA configuration

[Multipart message — attachments visible in raw view] — view raw

Hi,

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;

> @@ -112,9 +108,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	return 0;
>  err2:
>  	phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> -			  dev_name(&xhci->dev));
> +			  dev_name(dwc->dev));
>  	phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> -			  dev_name(&xhci->dev));
> +			  dev_name(dwc->dev));
>  err1:
>  	platform_device_put(xhci);
>  	return ret;
> @@ -123,8 +119,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>  void dwc3_host_exit(struct dwc3 *dwc)
>  {
>  	phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> -			  dev_name(&dwc->xhci->dev));
> +			  dev_name(dwc->dev));
>  	phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> -			  dev_name(&dwc->xhci->dev));
> +			  dev_name(dwc->dev));

this looks unrelated to $subject. Care to explain?

-- 
balbi

[toc] | [next] | [standalone]

#1519683

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

>> @@ -112,9 +108,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>>  	return 0;
>>  err2:
>>  	phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
>> -			  dev_name(&xhci->dev));
>> +			  dev_name(dwc->dev));
>>  	phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
>> -			  dev_name(&xhci->dev));
>> +			  dev_name(dwc->dev));
>>  err1:
>>  	platform_device_put(xhci);
>>  	return ret;
>> @@ -123,8 +119,8 @@ int dwc3_host_init(struct dwc3 *dwc)  void
>> dwc3_host_exit(struct dwc3 *dwc)  {
>>  	phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
>> -			  dev_name(&dwc->xhci->dev));
>> +			  dev_name(dwc->dev));
>>  	phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
>> -			  dev_name(&dwc->xhci->dev));
>> +			  dev_name(dwc->dev));
>
>this looks unrelated to $subject. Care to explain?
>

Apologies. Will remove unrelated changes in next version.

>--
>balbi

[toc] | [prev] | [next] | [standalone]

#1519690

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1520013

On Thursday, November 10, 2016 1:02:11 PM CET Felipe Balbi wrote:
> > @@ -123,8 +119,8 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  void dwc3_host_exit(struct dwc3 *dwc)
> >  {
> >       phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> > -                       dev_name(&dwc->xhci->dev));
> > +                       dev_name(dwc->dev));
> >       phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> > -                       dev_name(&dwc->xhci->dev));
> > +                       dev_name(dwc->dev));
> 
> this looks unrelated to $subject. Care to explain?

bus->sysdev is used for retrieving any information that comes from
the platform (DT or otherwise), and the phy lookup in has to be done
through sysdev as well because the platform cannot add it to
the child device it has no knowledge of.

When we set the sysdev to the parent, the phy lookup has to be
added to that device as well, rather than the child device.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1520752

On Fri, Nov 11, 2016 at 09:31:09PM +0100, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 1:02:11 PM CET Felipe Balbi wrote:
> > > @@ -123,8 +119,8 @@ int dwc3_host_init(struct dwc3 *dwc)
> > >  void dwc3_host_exit(struct dwc3 *dwc)
> > >  {
> > >       phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
> > > -                       dev_name(&dwc->xhci->dev));
> > > +                       dev_name(dwc->dev));
> > >       phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
> > > -                       dev_name(&dwc->xhci->dev));
> > > +                       dev_name(dwc->dev));
> > 
> > this looks unrelated to $subject. Care to explain?
> 
> bus->sysdev is used for retrieving any information that comes from
> the platform (DT or otherwise), and the phy lookup in has to be done
> through sysdev as well because the platform cannot add it to
> the child device it has no knowledge of.
> 
> When we set the sysdev to the parent, the phy lookup has to be
> added to that device as well, rather than the child device.
> 

Then, the device should change to dwc->sysdev, and phy_create_lookup
in this file needs to change too.

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1521326

>From: Peter Chen [mailto:hzpeterchen@gmail.com]
>On Fri, Nov 11, 2016 at 09:31:09PM +0100, Arnd Bergmann wrote:
>> On Thursday, November 10, 2016 1:02:11 PM CET Felipe Balbi wrote:
>> > > @@ -123,8 +119,8 @@ int dwc3_host_init(struct dwc3 *dwc)  void
>> > > dwc3_host_exit(struct dwc3 *dwc)  {
>> > >       phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
>> > > -                       dev_name(&dwc->xhci->dev));
>> > > +                       dev_name(dwc->dev));
>> > >       phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
>> > > -                       dev_name(&dwc->xhci->dev));
>> > > +                       dev_name(dwc->dev));
>> >
>> > this looks unrelated to $subject. Care to explain?
>>
>> bus->sysdev is used for retrieving any information that comes from
>> the platform (DT or otherwise), and the phy lookup in has to be done
>> through sysdev as well because the platform cannot add it to the child
>> device it has no knowledge of.
>>
>> When we set the sysdev to the parent, the phy lookup has to be added
>> to that device as well, rather than the child device.
>>
>
>Then, the device should change to dwc->sysdev, and phy_create_lookup in this file
>needs to change too.
>

Yes Peter. I agree to that. 

>--
>
>Best Regards,
>Peter Chen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web