Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:52 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Update DWC2 kconfig and makefile to support dual-role mode. The platform
> file will always get compiled for the case where the controller is directly
> connected to the CPU. So for loadable modules, only dwc2.ko is needed.

Kconfig and Makefile changes should be done after (or at the same time as)
driver code itself is modified to support dual-role mode.  Each individual
patch of the patchset should be correct itself (not cause any breakages)
in order to keep the whole patchset bisectable.

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
> v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role
>     config options.
> v2: Remove reference to dwc2_gadget
> ---
>  drivers/usb/dwc2/Kconfig  | 63 +++++++++++++++++++++++++++--------------------
>  drivers/usb/dwc2/Makefile | 21 ++++++++--------
>  2 files changed, 47 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
> index f93807b..4396a1f 100644
> --- a/drivers/usb/dwc2/Kconfig
> +++ b/drivers/usb/dwc2/Kconfig
> @@ -1,40 +1,29 @@
>  config USB_DWC2
> -	bool "DesignWare USB2 DRD Core Support"
> +	tristate "DesignWare USB2 DRD Core Support"
>  	depends on USB
>  	help
>  	  Say Y here if your system has a Dual Role Hi-Speed USB
>  	  controller based on the DesignWare HSOTG IP Core.
>  
> -	  For host mode, if you choose to build the driver as dynamically
> -	  linked modules, the core module will be called dwc2.ko, the PCI
> -	  bus interface module (if you have a PCI bus system) will be
> -	  called dwc2_pci.ko, and the platform interface module (for
> -	  controllers directly connected to the CPU) will be called
> -	  dwc2_platform.ko. For gadget mode, there will be a single
> -	  module called dwc2_gadget.ko.
> -
> -	  NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
> -	  host and gadget drivers are still currently separate drivers.
> -	  There are plans to merge the dwc2_gadget driver with the dwc2
> -	  host driver in the near future to create a dual-role driver.
> +	  If you choose to build the driver as dynamically
> +	  linked modules, a single dwc2.ko(regardless of mode of operation)

minort nitpick: " " is missing after dwc2.ko

> +	  will get built for both platform IPs and PCI.

Why do you want ot merge both platform and PCI drivers into one?

To do it properly you need to modify module_init/exit() of the final
module to properly handle both PCI and platform devices.  It should
be easier to leave separate dwc2_pci/platform drivers and just put
the common code into dwc2.ko.

>  if USB_DWC2
>  
> +choice
> +	bool "DWC2 Mode Selection"
> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)
> +
>  config USB_DWC2_HOST
> -	tristate "Host only mode"
> +	bool "Host only mode"
>  	depends on USB
>  	help
>  	  The Designware USB2.0 high-speed host controller
> -	  integrated into many SoCs.
> -
> -config USB_DWC2_PLATFORM
> -	bool "DWC2 Platform"
> -	depends on USB_DWC2_HOST
> -	default USB_DWC2_HOST
> -	help
> -	  The Designware USB2.0 platform interface module for
> -	  controllers directly connected to the CPU. This is only
> -	  used for host mode.
> +	  integrated into many SoCs. Select this option if you want the
> +	  driver to operate in Host-only mode.
>  
>  config USB_DWC2_PCI
>  	bool "DWC2 PCI"

Why have you left this option into middle of 'choice' selection?

You've also left the "depends on USB_DWC2_HOST && PCI" unmodified
which causes DWC2 PCI support to show up only if "Host only mode"
is selected (it is not available if "Dual Role mode" is selected).

> @@ -47,11 +36,31 @@ config USB_DWC2_PCI
>  comment "Gadget mode requires USB Gadget support to be enabled"
>  
>  config USB_DWC2_PERIPHERAL
> -	tristate "Gadget only mode"
> -	depends on USB_GADGET
> +	bool "Gadget only mode"
> +	depends on USB_GADGET=y || USB_GADGET=USB_DWC2
>  	help
>  	  The Designware USB2.0 high-speed gadget controller
> -	  integrated into many SoCs.
> +	  integrated into many SoCs. Select this option if you want the
> +	  driver to operate in Peripheral-only mode. This option requires
> +	  USB_GADGET=y.
> +
> +config USB_DWC2_DUAL_ROLE
> +	bool "Dual Role mode"
> +	depends on ((USB=y || USB=USB_DWC2) && (USB_GADGET=y || USB_GADGET=USB_DWC2))

I believe that extra parentheses are not needed.

> +	help
> +	  Select this option if you want the driver to work in a dual-role
> +	  mode. In this mode both host and gadget features are enabled, and
> +	  the role will be determined by the cable that gets plugged-in. This
> +	  option requires USB_GADGET=y.
> +endchoice
> +
> +config USB_DWC2_PLATFORM
> +	bool
> +        depends on !PCI

Why have you introduced this limitation (without even mentioning it in
the patch description)?  I suspect that by this change and disabling
PCI in your config you've workarounded the issue of having the common
module for PCI and platform parts completely.  Sorry but this is
incorrect as we want to have PCI and platform support in one compiled
kernel.

> +        default y
> +        help
> +          The Designware USB2.0 platform interface module for
> +          controllers directly connected to the CPU.
>  
>  config USB_DWC2_DEBUG
>  	bool "Enable Debugging Messages"
> diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile
> index b73d2a5..3026135 100644
> --- a/drivers/usb/dwc2/Makefile
> +++ b/drivers/usb/dwc2/Makefile
> @@ -1,10 +1,17 @@
>  ccflags-$(CONFIG_USB_DWC2_DEBUG)	+= -DDEBUG
>  ccflags-$(CONFIG_USB_DWC2_VERBOSE)	+= -DVERBOSE_DEBUG
>  
> -obj-$(CONFIG_USB_DWC2_HOST)		+= dwc2.o
> +obj-$(CONFIG_USB_DWC2)			+= dwc2.o
>  dwc2-y					:= core.o core_intr.o
> -dwc2-y					+= hcd.o hcd_intr.o
> -dwc2-y					+= hcd_queue.o hcd_ddma.o
> +
> +ifneq ($(filter y,$(CONFIG_USB_DWC2_HOST) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> +	dwc2-y				+= hcd.o hcd_intr.o
> +	dwc2-y				+= hcd_queue.o hcd_ddma.o
> +endif
> +
> +ifneq ($(filter y,$(CONFIG_USB_DWC2_PERIPHERAL) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> +	dwc2-y       			+= gadget.o
> +endif
>  
>  # NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to
>  # this location and renamed gadget.c. When building for dynamically linked

This comment is no longer true after your changes.

> @@ -19,10 +26,4 @@ ifneq ($(CONFIG_USB_DWC2_PCI),)
>  	dwc2_pci-y			:= pci.o
>  endif

dwc2_pci will still be build as separate module despite what the updated
documentation for USB_DWC2 says.

> -ifneq ($(CONFIG_USB_DWC2_PLATFORM),)
> -	obj-$(CONFIG_USB_DWC2_HOST)	+= dwc2_platform.o
> -	dwc2_platform-y			:= platform.o
> -endif
> -
> -obj-$(CONFIG_USB_DWC2_PERIPHERAL)	+= dwc2_gadget.o
> -dwc2_gadget-y				:= gadget.o
> +dwc2-$(CONFIG_USB_DWC2_PLATFORM) += platform.o

platform.c references code from hcd.c but will be built alone for
USB_DWC2_PERIPHERAL=y config (the link failure will happen).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Dinh Nguyen]

Hi Bartlomiej,

On 09/12/2014 10:49 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added linux-kernel ML to cc: ]
> 
> Hi,
> 
> On Tuesday, August 26, 2014 11:19:52 AM dinguyen@opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Update DWC2 kconfig and makefile to support dual-role mode. The platform
>> file will always get compiled for the case where the controller is directly
>> connected to the CPU. So for loadable modules, only dwc2.ko is needed.
> 
> Kconfig and Makefile changes should be done after (or at the same time as)
> driver code itself is modified to support dual-role mode.  Each individual
> patch of the patchset should be correct itself (not cause any breakages)
> in order to keep the whole patchset bisectable.
> 

Paulz mentioned this in v1 of this patch series and ever since then, I
have been careful to test each patch on it's own, and each version since
then has passed 0-Day kbuild testing. But I may have missed something in
v4. Will try to move the edits to Kconfig/Makefile to end for v5.

>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>> Acked-by: Paul Zimmerman <paulz@synopsys.com>
>> ---
>> v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role
>>     config options.
>> v2: Remove reference to dwc2_gadget
>> ---
>>  drivers/usb/dwc2/Kconfig  | 63 +++++++++++++++++++++++++++--------------------
>>  drivers/usb/dwc2/Makefile | 21 ++++++++--------
>>  2 files changed, 47 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
>> index f93807b..4396a1f 100644
>> --- a/drivers/usb/dwc2/Kconfig
>> +++ b/drivers/usb/dwc2/Kconfig
>> @@ -1,40 +1,29 @@
>>  config USB_DWC2
>> -	bool "DesignWare USB2 DRD Core Support"
>> +	tristate "DesignWare USB2 DRD Core Support"
>>  	depends on USB
>>  	help
>>  	  Say Y here if your system has a Dual Role Hi-Speed USB
>>  	  controller based on the DesignWare HSOTG IP Core.
>>  
>> -	  For host mode, if you choose to build the driver as dynamically
>> -	  linked modules, the core module will be called dwc2.ko, the PCI
>> -	  bus interface module (if you have a PCI bus system) will be
>> -	  called dwc2_pci.ko, and the platform interface module (for
>> -	  controllers directly connected to the CPU) will be called
>> -	  dwc2_platform.ko. For gadget mode, there will be a single
>> -	  module called dwc2_gadget.ko.
>> -
>> -	  NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
>> -	  host and gadget drivers are still currently separate drivers.
>> -	  There are plans to merge the dwc2_gadget driver with the dwc2
>> -	  host driver in the near future to create a dual-role driver.
>> +	  If you choose to build the driver as dynamically
>> +	  linked modules, a single dwc2.ko(regardless of mode of operation)
> 
> minort nitpick: " " is missing after dwc2.ko
> 
>> +	  will get built for both platform IPs and PCI.
> 
> Why do you want ot merge both platform and PCI drivers into one?
> 
> To do it properly you need to modify module_init/exit() of the final
> module to properly handle both PCI and platform devices.  It should
> be easier to leave separate dwc2_pci/platform drivers and just put
> the common code into dwc2.ko.

I need to rework to the comment. I think it should say, "will get built
for either platform IPs or PCI." I am not merging both platform and PCI
drivers into one.

> 
>>  if USB_DWC2
>>  
>> +choice
>> +	bool "DWC2 Mode Selection"
>> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
>> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
>> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)
>> +
>>  config USB_DWC2_HOST
>> -	tristate "Host only mode"
>> +	bool "Host only mode"
>>  	depends on USB
>>  	help
>>  	  The Designware USB2.0 high-speed host controller
>> -	  integrated into many SoCs.
>> -
>> -config USB_DWC2_PLATFORM
>> -	bool "DWC2 Platform"
>> -	depends on USB_DWC2_HOST
>> -	default USB_DWC2_HOST
>> -	help
>> -	  The Designware USB2.0 platform interface module for
>> -	  controllers directly connected to the CPU. This is only
>> -	  used for host mode.
>> +	  integrated into many SoCs. Select this option if you want the
>> +	  driver to operate in Host-only mode.
>>  
>>  config USB_DWC2_PCI
>>  	bool "DWC2 PCI"
> 
> Why have you left this option into middle of 'choice' selection?

Will move...

> 
> You've also left the "depends on USB_DWC2_HOST && PCI" unmodified
> which causes DWC2 PCI support to show up only if "Host only mode"
> is selected (it is not available if "Dual Role mode" is selected).
> 

Does PCI support gadget? I left it unmodified because I didn't think the
PCI driver supported Gadget.


>> @@ -47,11 +36,31 @@ config USB_DWC2_PCI
>>  comment "Gadget mode requires USB Gadget support to be enabled"
>>  
>>  config USB_DWC2_PERIPHERAL
>> -	tristate "Gadget only mode"
>> -	depends on USB_GADGET
>> +	bool "Gadget only mode"
>> +	depends on USB_GADGET=y || USB_GADGET=USB_DWC2
>>  	help
>>  	  The Designware USB2.0 high-speed gadget controller
>> -	  integrated into many SoCs.
>> +	  integrated into many SoCs. Select this option if you want the
>> +	  driver to operate in Peripheral-only mode. This option requires
>> +	  USB_GADGET=y.
>> +
>> +config USB_DWC2_DUAL_ROLE
>> +	bool "Dual Role mode"
>> +	depends on ((USB=y || USB=USB_DWC2) && (USB_GADGET=y || USB_GADGET=USB_DWC2))
> 
> I believe that extra parentheses are not needed.

Yes, I can remove the extra parentheses.

> 
>> +	help
>> +	  Select this option if you want the driver to work in a dual-role
>> +	  mode. In this mode both host and gadget features are enabled, and
>> +	  the role will be determined by the cable that gets plugged-in. This
>> +	  option requires USB_GADGET=y.
>> +endchoice
>> +
>> +config USB_DWC2_PLATFORM
>> +	bool
>> +        depends on !PCI
> 
> Why have you introduced this limitation (without even mentioning it in
> the patch description)?  I suspect that by this change and disabling
> PCI in your config you've workarounded the issue of having the common
> module for PCI and platform parts completely.  Sorry but this is
> incorrect as we want to have PCI and platform support in one compiled
> kernel.

Yes...I will remove.

> 
>> +        default y
>> +        help
>> +          The Designware USB2.0 platform interface module for
>> +          controllers directly connected to the CPU.
>>  
>>  config USB_DWC2_DEBUG
>>  	bool "Enable Debugging Messages"
>> diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile
>> index b73d2a5..3026135 100644
>> --- a/drivers/usb/dwc2/Makefile
>> +++ b/drivers/usb/dwc2/Makefile
>> @@ -1,10 +1,17 @@
>>  ccflags-$(CONFIG_USB_DWC2_DEBUG)	+= -DDEBUG
>>  ccflags-$(CONFIG_USB_DWC2_VERBOSE)	+= -DVERBOSE_DEBUG
>>  
>> -obj-$(CONFIG_USB_DWC2_HOST)		+= dwc2.o
>> +obj-$(CONFIG_USB_DWC2)			+= dwc2.o
>>  dwc2-y					:= core.o core_intr.o
>> -dwc2-y					+= hcd.o hcd_intr.o
>> -dwc2-y					+= hcd_queue.o hcd_ddma.o
>> +
>> +ifneq ($(filter y,$(CONFIG_USB_DWC2_HOST) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
>> +	dwc2-y				+= hcd.o hcd_intr.o
>> +	dwc2-y				+= hcd_queue.o hcd_ddma.o
>> +endif
>> +
>> +ifneq ($(filter y,$(CONFIG_USB_DWC2_PERIPHERAL) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
>> +	dwc2-y       			+= gadget.o
>> +endif
>>  
>>  # NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to
>>  # this location and renamed gadget.c. When building for dynamically linked
> 
> This comment is no longer true after your changes.

Well, this patch series doesn't affect this comment. gadget.c is still
gadget.c that was renamed from s3c-hsotg. I'd like to leave this comment
here for now because people are still forgetting that s3c-hsotg is now
gadget.

> 
>> @@ -19,10 +26,4 @@ ifneq ($(CONFIG_USB_DWC2_PCI),)
>>  	dwc2_pci-y			:= pci.o
>>  endif
> 
> dwc2_pci will still be build as separate module despite what the updated
> documentation for USB_DWC2 says.

Ah...So should I keep for PCD, dwc2-pci.ko or dwc2.ko?

> 
>> -ifneq ($(CONFIG_USB_DWC2_PLATFORM),)
>> -	obj-$(CONFIG_USB_DWC2_HOST)	+= dwc2_platform.o
>> -	dwc2_platform-y			:= platform.o
>> -endif
>> -
>> -obj-$(CONFIG_USB_DWC2_PERIPHERAL)	+= dwc2_gadget.o
>> -dwc2_gadget-y				:= gadget.o
>> +dwc2-$(CONFIG_USB_DWC2_PLATFORM) += platform.o
> 
> platform.c references code from hcd.c but will be built alone for
> USB_DWC2_PERIPHERAL=y config (the link failure will happen).

I believed I have wrapped all the necessary functions from hcd.c so that
the link failure will not happen. But I will check again.

Thanks for your review.
Dinh

RE: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Paul Zimmerman]

> From: Dinh Nguyen [mailto:dinguyen@opensource.altera.com]
> Sent: Thursday, September 18, 2014 8:54 AM
> 
> On 09/12/2014 10:49 AM, Bartlomiej Zolnierkiewicz wrote:
> >
> > [ added linux-kernel ML to cc: ]
> >
> > Hi,
> >
> > On Tuesday, August 26, 2014 11:19:52 AM dinguyen@opensource.altera.com wrote:
> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>
> >> Update DWC2 kconfig and makefile to support dual-role mode. The platform
> >> file will always get compiled for the case where the controller is directly
> >> connected to the CPU. So for loadable modules, only dwc2.ko is needed.
> >
> > Kconfig and Makefile changes should be done after (or at the same time as)
> > driver code itself is modified to support dual-role mode.  Each individual
> > patch of the patchset should be correct itself (not cause any breakages)
> > in order to keep the whole patchset bisectable.
> >
> 
> Paulz mentioned this in v1 of this patch series and ever since then, I
> have been careful to test each patch on it's own, and each version since
> then has passed 0-Day kbuild testing. But I may have missed something in
> v4. Will try to move the edits to Kconfig/Makefile to end for v5.

As I said in another email, try building with ARM multi_v7_defconfig,
you should see the problems then (it defines CONFIG_PCI for example).
Also be sure to test building with all variations of the DWC2 config.

I would suggest deleting all of the object files in the dwc2/ directory
between builds, that will make it more obvious if something is missing
(for example, platform.o doesn't get built if CONFIG_PCI is defined).

> >> diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
> >> index f93807b..4396a1f 100644
> >> --- a/drivers/usb/dwc2/Kconfig
> >> +++ b/drivers/usb/dwc2/Kconfig
> >> @@ -1,40 +1,29 @@
> >>  config USB_DWC2
> >> -	bool "DesignWare USB2 DRD Core Support"
> >> +	tristate "DesignWare USB2 DRD Core Support"
> >>  	depends on USB
> >>  	help
> >>  	  Say Y here if your system has a Dual Role Hi-Speed USB
> >>  	  controller based on the DesignWare HSOTG IP Core.
> >>
> >> -	  For host mode, if you choose to build the driver as dynamically
> >> -	  linked modules, the core module will be called dwc2.ko, the PCI
> >> -	  bus interface module (if you have a PCI bus system) will be
> >> -	  called dwc2_pci.ko, and the platform interface module (for
> >> -	  controllers directly connected to the CPU) will be called
> >> -	  dwc2_platform.ko. For gadget mode, there will be a single
> >> -	  module called dwc2_gadget.ko.
> >> -
> >> -	  NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
> >> -	  host and gadget drivers are still currently separate drivers.
> >> -	  There are plans to merge the dwc2_gadget driver with the dwc2
> >> -	  host driver in the near future to create a dual-role driver.
> >> +	  If you choose to build the driver as dynamically
> >> +	  linked modules, a single dwc2.ko(regardless of mode of operation)
> >
> > minort nitpick: " " is missing after dwc2.ko
> >
> >> +	  will get built for both platform IPs and PCI.
> >
> > Why do you want ot merge both platform and PCI drivers into one?
> >
> > To do it properly you need to modify module_init/exit() of the final
> > module to properly handle both PCI and platform devices.  It should
> > be easier to leave separate dwc2_pci/platform drivers and just put
> > the common code into dwc2.ko.
> 
> I need to rework to the comment. I think it should say, "will get built
> for either platform IPs or PCI." I am not merging both platform and PCI
> drivers into one.

It looks to me like you are building the platform code into the main
dwc2.ko module, with a separate dwc2_pci.ko module. That might work
(I'm not sure), but as Bartlomiej said, I think it would be better to
have just the common code in dwc2.ko, with separate modules for pci
and platform. This is the pattern that DWC3 follows.

> > You've also left the "depends on USB_DWC2_HOST && PCI" unmodified
> > which causes DWC2 PCI support to show up only if "Host only mode"
> > is selected (it is not available if "Dual Role mode" is selected).
> >
> 
> Does PCI support gadget? I left it unmodified because I didn't think the
> PCI driver supported Gadget.

PCI can support gadget, but currently it is not implemented. I plan to
add that after this series goes in, since I think I have the only PCI
platform for DWC2. So I think it's fine to leave this as-is for now,
and I will change it later.
 
-- 
Paul

Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, September 18, 2014 10:54:24 AM Dinh Nguyen wrote:
> Hi Bartlomiej,
> 
> On 09/12/2014 10:49 AM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > [ added linux-kernel ML to cc: ]
> > 
> > Hi,
> > 
> > On Tuesday, August 26, 2014 11:19:52 AM dinguyen@opensource.altera.com wrote:
> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>
> >> Update DWC2 kconfig and makefile to support dual-role mode. The platform
> >> file will always get compiled for the case where the controller is directly
> >> connected to the CPU. So for loadable modules, only dwc2.ko is needed.
> > 
> > Kconfig and Makefile changes should be done after (or at the same time as)
> > driver code itself is modified to support dual-role mode.  Each individual
> > patch of the patchset should be correct itself (not cause any breakages)
> > in order to keep the whole patchset bisectable.
> > 
> 
> Paulz mentioned this in v1 of this patch series and ever since then, I
> have been careful to test each patch on it's own, and each version since
> then has passed 0-Day kbuild testing. But I may have missed something in
> v4. Will try to move the edits to Kconfig/Makefile to end for v5.
> 
> >> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> >> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> >> ---
> >> v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role
> >>     config options.
> >> v2: Remove reference to dwc2_gadget
> >> ---
> >>  drivers/usb/dwc2/Kconfig  | 63 +++++++++++++++++++++++++++--------------------
> >>  drivers/usb/dwc2/Makefile | 21 ++++++++--------
> >>  2 files changed, 47 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
> >> index f93807b..4396a1f 100644
> >> --- a/drivers/usb/dwc2/Kconfig
> >> +++ b/drivers/usb/dwc2/Kconfig
> >> @@ -1,40 +1,29 @@
> >>  config USB_DWC2
> >> -	bool "DesignWare USB2 DRD Core Support"
> >> +	tristate "DesignWare USB2 DRD Core Support"
> >>  	depends on USB
> >>  	help
> >>  	  Say Y here if your system has a Dual Role Hi-Speed USB
> >>  	  controller based on the DesignWare HSOTG IP Core.
> >>  
> >> -	  For host mode, if you choose to build the driver as dynamically
> >> -	  linked modules, the core module will be called dwc2.ko, the PCI
> >> -	  bus interface module (if you have a PCI bus system) will be
> >> -	  called dwc2_pci.ko, and the platform interface module (for
> >> -	  controllers directly connected to the CPU) will be called
> >> -	  dwc2_platform.ko. For gadget mode, there will be a single
> >> -	  module called dwc2_gadget.ko.
> >> -
> >> -	  NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
> >> -	  host and gadget drivers are still currently separate drivers.
> >> -	  There are plans to merge the dwc2_gadget driver with the dwc2
> >> -	  host driver in the near future to create a dual-role driver.
> >> +	  If you choose to build the driver as dynamically
> >> +	  linked modules, a single dwc2.ko(regardless of mode of operation)
> > 
> > minort nitpick: " " is missing after dwc2.ko
> > 
> >> +	  will get built for both platform IPs and PCI.
> > 
> > Why do you want ot merge both platform and PCI drivers into one?
> > 
> > To do it properly you need to modify module_init/exit() of the final
> > module to properly handle both PCI and platform devices.  It should
> > be easier to leave separate dwc2_pci/platform drivers and just put
> > the common code into dwc2.ko.
> 
> I need to rework to the comment. I think it should say, "will get built
> for either platform IPs or PCI." I am not merging both platform and PCI
> drivers into one.

OK, it is good to hear that.

Unfortunately after second look there are even more problems with Kconfig
changes, please see below.

> > 
> >>  if USB_DWC2
> >>  
> >> +choice
> >> +	bool "DWC2 Mode Selection"
> >> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
> >> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
> >> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)

Previously it was possible to have following functionalities in
one kernel (for multiplatform kernels):

- host PCI support
- host platform support
- gadget platform support

Now mode selection will determine the used mode for combined
host+gadget platform driver.  It is no longer possible to have
host platform and gadget platform support in one kernel.

Also there is only one mode selection for both PCI and platform
drivers so it is no longer possible to have both gadget platform
support and host PCI support in one kernel.

I think it would be the best to replace global mode selection
choice option with just two independent suboptions for combined
host+gadget platform driver which will enable host and gadget
support to be compiled into the driver (you would need to enable
both for dual role support).  The actual selection of the mode
used should be done at runtime using device tree.

[ Host PCI support can be left as independent host only driver
  for now until it also gets proper gadget support (according to
  comments from Paul). ]

> >> +
> >>  config USB_DWC2_HOST
> >> -	tristate "Host only mode"
> >> +	bool "Host only mode"
> >>  	depends on USB
> >>  	help
> >>  	  The Designware USB2.0 high-speed host controller
> >> -	  integrated into many SoCs.
> >> -
> >> -config USB_DWC2_PLATFORM
> >> -	bool "DWC2 Platform"
> >> -	depends on USB_DWC2_HOST
> >> -	default USB_DWC2_HOST
> >> -	help
> >> -	  The Designware USB2.0 platform interface module for
> >> -	  controllers directly connected to the CPU. This is only
> >> -	  used for host mode.
> >> +	  integrated into many SoCs. Select this option if you want the
> >> +	  driver to operate in Host-only mode.
> >>  
> >>  config USB_DWC2_PCI
> >>  	bool "DWC2 PCI"
> > 
> > Why have you left this option into middle of 'choice' selection?
> 
> Will move...
> 
> > 
> > You've also left the "depends on USB_DWC2_HOST && PCI" unmodified
> > which causes DWC2 PCI support to show up only if "Host only mode"
> > is selected (it is not available if "Dual Role mode" is selected).
> > 
> 
> Does PCI support gadget? I left it unmodified because I didn't think the
> PCI driver supported Gadget.
> 
> 
> >> @@ -47,11 +36,31 @@ config USB_DWC2_PCI
> >>  comment "Gadget mode requires USB Gadget support to be enabled"
> >>  
> >>  config USB_DWC2_PERIPHERAL
> >> -	tristate "Gadget only mode"
> >> -	depends on USB_GADGET
> >> +	bool "Gadget only mode"
> >> +	depends on USB_GADGET=y || USB_GADGET=USB_DWC2
> >>  	help
> >>  	  The Designware USB2.0 high-speed gadget controller
> >> -	  integrated into many SoCs.
> >> +	  integrated into many SoCs. Select this option if you want the
> >> +	  driver to operate in Peripheral-only mode. This option requires
> >> +	  USB_GADGET=y.
> >> +
> >> +config USB_DWC2_DUAL_ROLE
> >> +	bool "Dual Role mode"
> >> +	depends on ((USB=y || USB=USB_DWC2) && (USB_GADGET=y || USB_GADGET=USB_DWC2))
> > 
> > I believe that extra parentheses are not needed.
> 
> Yes, I can remove the extra parentheses.
> 
> > 
> >> +	help
> >> +	  Select this option if you want the driver to work in a dual-role
> >> +	  mode. In this mode both host and gadget features are enabled, and
> >> +	  the role will be determined by the cable that gets plugged-in. This
> >> +	  option requires USB_GADGET=y.
> >> +endchoice
> >> +
> >> +config USB_DWC2_PLATFORM
> >> +	bool
> >> +        depends on !PCI
> > 
> > Why have you introduced this limitation (without even mentioning it in
> > the patch description)?  I suspect that by this change and disabling
> > PCI in your config you've workarounded the issue of having the common
> > module for PCI and platform parts completely.  Sorry but this is
> > incorrect as we want to have PCI and platform support in one compiled
> > kernel.
> 
> Yes...I will remove.
> 
> > 
> >> +        default y
> >> +        help
> >> +          The Designware USB2.0 platform interface module for
> >> +          controllers directly connected to the CPU.
> >>  
> >>  config USB_DWC2_DEBUG
> >>  	bool "Enable Debugging Messages"
> >> diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile
> >> index b73d2a5..3026135 100644
> >> --- a/drivers/usb/dwc2/Makefile
> >> +++ b/drivers/usb/dwc2/Makefile
> >> @@ -1,10 +1,17 @@
> >>  ccflags-$(CONFIG_USB_DWC2_DEBUG)	+= -DDEBUG
> >>  ccflags-$(CONFIG_USB_DWC2_VERBOSE)	+= -DVERBOSE_DEBUG
> >>  
> >> -obj-$(CONFIG_USB_DWC2_HOST)		+= dwc2.o
> >> +obj-$(CONFIG_USB_DWC2)			+= dwc2.o
> >>  dwc2-y					:= core.o core_intr.o
> >> -dwc2-y					+= hcd.o hcd_intr.o
> >> -dwc2-y					+= hcd_queue.o hcd_ddma.o
> >> +
> >> +ifneq ($(filter y,$(CONFIG_USB_DWC2_HOST) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> >> +	dwc2-y				+= hcd.o hcd_intr.o
> >> +	dwc2-y				+= hcd_queue.o hcd_ddma.o
> >> +endif
> >> +
> >> +ifneq ($(filter y,$(CONFIG_USB_DWC2_PERIPHERAL) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> >> +	dwc2-y       			+= gadget.o
> >> +endif
> >>  
> >>  # NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to
> >>  # this location and renamed gadget.c. When building for dynamically linked
> > 
> > This comment is no longer true after your changes.
> 
> Well, this patch series doesn't affect this comment. gadget.c is still
> gadget.c that was renamed from s3c-hsotg. I'd like to leave this comment
> here for now because people are still forgetting that s3c-hsotg is now
> gadget.

I was referring to the full comment (in the patch there are only two first
lines shown):

# NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to
# this location and renamed gadget.c. When building for dynamically linked
# modules, dwc2_gadget.ko will get built for peripheral mode. For host mode,
# the core module will be dwc2.ko, the PCI bus interface module will called
# dwc2_pci.ko and the platform interface module will be called dwc2_platform.ko.
# At present the host and gadget driver will be separate drivers, but there
# are plans in the near future to create a dual-role driver.

> > 
> >> @@ -19,10 +26,4 @@ ifneq ($(CONFIG_USB_DWC2_PCI),)
> >>  	dwc2_pci-y			:= pci.o
> >>  endif
> > 
> > dwc2_pci will still be build as separate module despite what the updated
> > documentation for USB_DWC2 says.
> 
> Ah...So should I keep for PCD, dwc2-pci.ko or dwc2.ko?
> 
> > 
> >> -ifneq ($(CONFIG_USB_DWC2_PLATFORM),)
> >> -	obj-$(CONFIG_USB_DWC2_HOST)	+= dwc2_platform.o
> >> -	dwc2_platform-y			:= platform.o
> >> -endif
> >> -
> >> -obj-$(CONFIG_USB_DWC2_PERIPHERAL)	+= dwc2_gadget.o
> >> -dwc2_gadget-y				:= gadget.o
> >> +dwc2-$(CONFIG_USB_DWC2_PLATFORM) += platform.o
> > 
> > platform.c references code from hcd.c but will be built alone for
> > USB_DWC2_PERIPHERAL=y config (the link failure will happen).
> 
> I believed I have wrapped all the necessary functions from hcd.c so that
> the link failure will not happen. But I will check again.

Using socfpage_defconfig and USB_DWC2_PERIPHERAL=y:

  LD      init/built-in.o
drivers/built-in.o: In function `dwc2_hc_set_even_odd_frame':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/core.c:1230: undefined reference to `dwc2_hcd_get_frame_number'
drivers/built-in.o: In function `dwc2_handle_otg_intr':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/core_intr.c:211: undefined reference to `dwc2_hcd_start'
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/core_intr.c:248: undefined reference to `dwc2_hcd_start'
drivers/built-in.o: In function `dwc2_handle_usb_suspend_intr':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/core_intr.c:406: undefined reference to `dwc2_hcd_start'
drivers/built-in.o: In function `dwc2_handle_otg_intr':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/core_intr.c:239: undefined reference to `dwc2_hcd_disconnect'
drivers/built-in.o: In function `dwc2_driver_remove':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/platform.c:123: undefined reference to `dwc2_hcd_remove'
drivers/built-in.o: In function `dwc2_driver_probe':
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/platform.c:208: undefined reference to `dwc2_hcd_init'
/home/bzolnier/sam/linux-sprc/drivers/usb/dwc2/platform.c:167: undefined reference to `dwc2_set_all_params'
make: *** [vmlinux] Error 1

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

RE: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Paul Zimmerman]

> From: Bartlomiej Zolnierkiewicz [mailto:b.zolnierkie@samsung.com]
> Sent: Friday, September 19, 2014 7:50 AM
> 
> Unfortunately after second look there are even more problems with Kconfig
> changes, please see below.
> 
> > >
> > >>  if USB_DWC2
> > >>
> > >> +choice
> > >> +	bool "DWC2 Mode Selection"
> > >> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
> > >> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
> > >> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)
> 
> Previously it was possible to have following functionalities in
> one kernel (for multiplatform kernels):
> 
> - host PCI support
> - host platform support
> - gadget platform support
> 
> Now mode selection will determine the used mode for combined
> host+gadget platform driver.  It is no longer possible to have
> host platform and gadget platform support in one kernel.

Why not? Why wouldn't it work if you enable USB_DWC2_DUAL_ROLE? The
dwc3 driver uses the same scheme as this, and I have not heard any
complaints about that not working.
 
> Also there is only one mode selection for both PCI and platform
> drivers so it is no longer possible to have both gadget platform
> support and host PCI support in one kernel.

There will be, once I update the dwc2 PCI support to include the gadget
functionality. Then, if you enable USB_DWC2_DUAL_ROLE and DWC2_PCI, you
should be set.

In the meantime, I believe the only platform today that needs PCI support
for dwc2 is our internal Synopsys development board, which is not an ARM
platform. So I don't think there is a real issue here.

> I think it would be the best to replace global mode selection
> choice option with just two independent suboptions for combined
> host+gadget platform driver which will enable host and gadget
> support to be compiled into the driver (you would need to enable
> both for dual role support).  The actual selection of the mode
> used should be done at runtime using device tree.

See above. The dwc3 driver uses the same scheme as Dinh has implemented,
and I don't think there have been any problems with it.

-- 
Paul

Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, September 19, 2014 07:02:49 PM Paul Zimmerman wrote:
> > From: Bartlomiej Zolnierkiewicz [mailto:b.zolnierkie@samsung.com]
> > Sent: Friday, September 19, 2014 7:50 AM
> > 
> > Unfortunately after second look there are even more problems with Kconfig
> > changes, please see below.
> > 
> > > >
> > > >>  if USB_DWC2
> > > >>
> > > >> +choice
> > > >> +	bool "DWC2 Mode Selection"
> > > >> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
> > > >> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
> > > >> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)
> > 
> > Previously it was possible to have following functionalities in
> > one kernel (for multiplatform kernels):
> > 
> > - host PCI support
> > - host platform support
> > - gadget platform support
> > 
> > Now mode selection will determine the used mode for combined
> > host+gadget platform driver.  It is no longer possible to have
> > host platform and gadget platform support in one kernel.
> 
> Why not? Why wouldn't it work if you enable USB_DWC2_DUAL_ROLE? The
> dwc3 driver uses the same scheme as this, and I have not heard any
> complaints about that not working.

Ok, it seems that this should work fine if:

- USB_DWC2_DUAL_ROLE is set

- dwc2 handles dr_mode properly

[ BTW dwc3 has the following code in its ->probe which dwc2 lacks
  (the code sets the default dr_mode value):

	if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
		dwc->dr_mode = USB_DR_MODE_HOST;
	else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
		dwc->dr_mode = USB_DR_MODE_PERIPHERAL;

	if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
		dwc->dr_mode = USB_DR_MODE_OTG;

  It seems that correspoding changes are needed for dwc2. ]

- proper dr_mode is set in the DT dwc2 entry if the board needs it

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 02/12] usb: dwc2: move "samsung,s3c6400-hsotg" into common platform

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:53 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Move the "samsung,s3c6400-hsotg" binding as the probe function in the gadget
> driver will get removed when the dual-role driver is implemented.

Sorry but no.  In the current form this just breaks gadget support for
Samsung devices as probe function for gadget will no longer be called.

[ This should be done in the same patch that aggregates gadget and host
  initializations into one driver. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
>  drivers/usb/dwc2/gadget.c   | 1 -
>  drivers/usb/dwc2/platform.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 94f7a3f..8c45b4c 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3674,7 +3674,6 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id s3c_hsotg_of_ids[] = {
> -	{ .compatible = "samsung,s3c6400-hsotg", },
>  	{ .compatible = "snps,dwc2", },
>  	{ /* sentinel */ }
>  };
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5f0c4bb..dd2f8f5 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -100,6 +100,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
>  static const struct of_device_id dwc2_of_match_table[] = {
>  	{ .compatible = "brcm,bcm2835-usb", .data = &params_bcm2835 },
>  	{ .compatible = "snps,dwc2", .data = NULL },
> +	{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, dwc2_of_match_table);

Re: [PATCHv4 03/12] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:54 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Adds the gadget data structure and appropriate data structure pointers
> to the common dwc2_hsotg data structure. To keep the driver data
> dereference code looking clean, the gadget variable declares are only available
> for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data
> structure can be used by the hcd and gadget drivers.
> 
> Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers
> that have been moved into the common dwc2_hsotg structure. Along
> with the updating the gadget driver to use the common dwc2_hsotg structure,
> a few other things are required in order for this patch to build properly.
> Those are:
> 
> - Remove gadget module defines. Since the driver probing will be handled
>   by either the platform or pci code.

Removal of gadget module defines should be done in the same patch that
adds gadget functionality probing to platform/PCI new common code.

> - Change the gadget probe function into gadget_init.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Signed-off-by: Paul Zimmerman <paulz@synopsys.com>
> ---
> v3: Updated with paulz's suggestion to avoid double pointers.
> v2: Left the function parameter name as 'hsotg' and just changed its type.
> ---
>  drivers/usb/dwc2/core.h   | 176 +++++++++++++++++++++---------------
>  drivers/usb/dwc2/gadget.c | 226 +++++++++++++++++-----------------------------
>  drivers/usb/dwc2/hcd.h    |  10 --
>  3 files changed, 184 insertions(+), 228 deletions(-)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:55 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Add the proper init calls for either host, gadget or both in platform.c
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
>  drivers/usb/dwc2/core.h     | 13 +++++++++++++
>  drivers/usb/dwc2/gadget.c   |  2 +-
>  drivers/usb/dwc2/platform.c | 28 ++++++++++++++++++++++++----

Where are correspoding changes to pci.c?

I cannot find them in your patchset.

>  3 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index f55e62d..3a49a00 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg);
>   */
>  extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg);
>  
> +/* Gadget defines */
> +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
> +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg);
> +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2);
> +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq);
> +#else
> +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {}
> +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2)
> +{ return 0; }
> +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
> +{ return 0; }
> +#endif
> +
>  #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
>  /**
>   * dwc2_hcd_get_frame_number() - Returns current frame number
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 96f868f..efa68a0 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3572,7 +3572,7 @@ err_clk:
>   * s3c_hsotg_remove - remove function for hsotg driver
>   * @pdev: The platform information for the driver
>   */
> -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
> +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
>  {
>  	usb_del_gadget_udc(&hsotg->gadget);
>  
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index dd2f8f5..2871f351 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev)
>  {
>  	struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
>  
> -	dwc2_hcd_remove(hsotg);
> +	if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL))
> +		s3c_hsotg_remove(hsotg);
> +	else if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
> +		dwc2_hcd_remove(hsotg);
> +	else { /* dual role */
> +		dwc2_hcd_remove(hsotg);
> +		s3c_hsotg_remove(hsotg);

Why not simply always call:

		dwc2_hcd_remove(hsotg);
		s3c_hsotg_remove(hsotg);

and add appropriate stub for dwc2_hcd_remove() for
CONFIG_USB_DWC2_PERIPHERAL=y?

> +	}
>  
>  	return 0;
>  }
> @@ -176,9 +183,22 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  
>  	hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>  
> -	retval = dwc2_hcd_init(hsotg, irq, params);
> -	if (retval)
> -		return retval;
> +	if (IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)) {
> +		retval = dwc2_gadget_init(hsotg, irq);
> +		if (retval)
> +			return retval;
> +		retval = dwc2_hcd_init(hsotg, irq, params);
> +		if (retval)
> +			return retval;
> +	} else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) {
> +		retval = dwc2_hcd_init(hsotg, irq, params);
> +		if (retval)
> +			return retval;
> +	} else if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) {
> +		retval = dwc2_gadget_init(hsotg, irq);
> +		if (retval)
> +			return retval;
> +	}

ditto but for dwc2_hcd_init().

>  	platform_set_drvdata(dev, hsotg);

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 04/12] usb: dwc2: Add the appropriate init calls in platform code

[Dinh Nguyen]

On 09/12/2014 11:18 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added linux-kernel ML to cc: ]
> 
> Hi,
> 
> On Tuesday, August 26, 2014 11:19:55 AM dinguyen@opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Add the proper init calls for either host, gadget or both in platform.c
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>> Acked-by: Paul Zimmerman <paulz@synopsys.com>
>> ---
>>  drivers/usb/dwc2/core.h     | 13 +++++++++++++
>>  drivers/usb/dwc2/gadget.c   |  2 +-
>>  drivers/usb/dwc2/platform.c | 28 ++++++++++++++++++++++++----
> 
> Where are correspoding changes to pci.c?
> 
> I cannot find them in your patchset.

The current PCI driver only supports HCD. If you want PCI to support
gadget, then it can be done in a separate patchset.

> 
>>  3 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index f55e62d..3a49a00 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg);
>>   */
>>  extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg);
>>  
>> +/* Gadget defines */
>> +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
>> +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg);
>> +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2);
>> +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq);
>> +#else
>> +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {}
>> +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2)
>> +{ return 0; }
>> +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>> +{ return 0; }
>> +#endif
>> +
>>  #if defined(CONFIG_USB_DWC2_HOST) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
>>  /**
>>   * dwc2_hcd_get_frame_number() - Returns current frame number
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 96f868f..efa68a0 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3572,7 +3572,7 @@ err_clk:
>>   * s3c_hsotg_remove - remove function for hsotg driver
>>   * @pdev: The platform information for the driver
>>   */
>> -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
>> +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
>>  {
>>  	usb_del_gadget_udc(&hsotg->gadget);
>>  
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index dd2f8f5..2871f351 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct platform_device *dev)
>>  {
>>  	struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
>>  
>> -	dwc2_hcd_remove(hsotg);
>> +	if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL))
>> +		s3c_hsotg_remove(hsotg);
>> +	else if (IS_ENABLED(CONFIG_USB_DWC2_HOST))
>> +		dwc2_hcd_remove(hsotg);
>> +	else { /* dual role */
>> +		dwc2_hcd_remove(hsotg);
>> +		s3c_hsotg_remove(hsotg);
> 
> Why not simply always call:
> 
> 		dwc2_hcd_remove(hsotg);
> 		s3c_hsotg_remove(hsotg);
> 
> and add appropriate stub for dwc2_hcd_remove() for
> CONFIG_USB_DWC2_PERIPHERAL=y?

Yes...I'll do this for v5. Same for the init().

Dinh

Re: [PATCHv4 08/12] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:59 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Since the dwc2 hcd driver is currently not looking for a clock node during
> init, we should not completely fail if there isn't a clock provided.
> Add a check for a valid clock before calling clock functions.

This doesn't look correct at least for the case when we are really missing
the clock and USB_DWC2_PERIPHERAL=y (moreover it just looks wrong to access
gadget functionalities when clock is disabled). It seems that it would be
better to just disable gadget functionality on dwc2_gadget_init() failure
in hcd and not call gadget functions later from hcd if gadget functionality
is disabled.

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
>  drivers/usb/dwc2/gadget.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index a1c93bf..aab1b45 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2852,7 +2852,8 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget,
>  	hsotg->gadget.dev.of_node = hsotg->dev->of_node;
>  	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>  
> -	clk_enable(hsotg->clk);
> +	if (!IS_ERR(hsotg->clk))
> +		clk_enable(hsotg->clk);
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
>  				    hsotg->supplies);
> @@ -2903,7 +2904,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>  	regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>  				hsotg->supplies);
>  
> -	clk_disable(hsotg->clk);
> +	if (!IS_ERR(hsotg->clk))
> +		clk_disable(hsotg->clk);
>  
>  	return 0;
>  }
> @@ -2936,10 +2938,12 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
>  		s3c_hsotg_phy_enable(hsotg);
> -		clk_enable(hsotg->clk);
> +		if (!IS_ERR(hsotg->clk))
> +			clk_enable(hsotg->clk);
>  		s3c_hsotg_core_init(hsotg);
>  	} else {
> -		clk_disable(hsotg->clk);
> +		if (!IS_ERR(hsotg->clk))
> +			clk_disable(hsotg->clk);
>  		s3c_hsotg_phy_disable(hsotg);
>  	}
>  
> @@ -3410,16 +3414,15 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>  	}
>  
>  	hsotg->clk = devm_clk_get(dev, "otg");
> -	if (IS_ERR(hsotg->clk)) {
> -		dev_err(dev, "cannot get otg clock\n");
> -		return PTR_ERR(hsotg->clk);
> -	}
> +	if (IS_ERR(hsotg->clk))
> +		dev_warn(dev, "cannot get otg clock\n");
>  
>  	hsotg->gadget.max_speed = USB_SPEED_HIGH;
>  	hsotg->gadget.ops = &s3c_hsotg_gadget_ops;
>  	hsotg->gadget.name = dev_name(dev);
>  
> -	clk_prepare_enable(hsotg->clk);
> +	if (!IS_ERR(hsotg->clk))
> +		clk_prepare_enable(hsotg->clk);
>  
>  	/* regulators */
>  
> @@ -3452,7 +3455,8 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>  			dev_name(dev), hsotg);
>  	if (ret < 0) {
>  		s3c_hsotg_phy_disable(hsotg);
> -		clk_disable_unprepare(hsotg->clk);
> +		if (!IS_ERR(hsotg->clk))
> +			clk_disable_unprepare(hsotg->clk);
>  		regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>  				       hsotg->supplies);
>  		dev_err(dev, "cannot claim IRQ\n");
> @@ -3521,7 +3525,8 @@ err_ep_mem:
>  err_supplies:
>  	s3c_hsotg_phy_disable(hsotg);
>  err_clk:
> -	clk_disable_unprepare(hsotg->clk);
> +	if (!IS_ERR(hsotg->clk))
> +		clk_disable_unprepare(hsotg->clk);
>  
>  	return ret;
>  }
> @@ -3541,7 +3546,8 @@ int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
>  		usb_gadget_unregister_driver(hsotg->driver);
>  	}
>  
> -	clk_disable_unprepare(hsotg->clk);
> +	if (!IS_ERR(hsotg->clk))
> +		clk_disable_unprepare(hsotg->clk);
>  
>  	return 0;
>  }
> @@ -3568,7 +3574,8 @@ static int s3c_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  
>  		ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
>  					     hsotg->supplies);
> -		clk_disable(hsotg->clk);
> +		if (!IS_ERR(hsotg->clk))
> +			clk_disable(hsotg->clk);
>  	}
>  
>  	return ret;
> @@ -3583,7 +3590,8 @@ static int s3c_hsotg_resume(struct dwc2_hsotg *hsotg)
>  		dev_info(hsotg->dev, "resuming usb gadget %s\n",
>  			 hsotg->driver->driver.name);
>  
> -		clk_enable(hsotg->clk);
> +		if (!IS_ERR(hsotg->clk))
> +			clk_enable(hsotg->clk);
>  		ret = regulator_bulk_enable(ARRAY_SIZE(hsotg->supplies),
>  					   hsotg->supplies);
>  	}

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 08/12] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Dinh Nguyen]

On 9/12/14, 11:28 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added linux-kernel ML to cc: ]
> 
> Hi,
> 
> On Tuesday, August 26, 2014 11:19:59 AM dinguyen@opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Since the dwc2 hcd driver is currently not looking for a clock node during
>> init, we should not completely fail if there isn't a clock provided.
>> Add a check for a valid clock before calling clock functions.
> 
> This doesn't look correct at least for the case when we are really missing
> the clock and USB_DWC2_PERIPHERAL=y (moreover it just looks wrong to access
> gadget functionalities when clock is disabled). It seems that it would be
> better to just disable gadget functionality on dwc2_gadget_init() failure
> in hcd and not call gadget functions later from hcd if gadget functionality
> is disabled.
> 

Yes...this is correct. Will fix up in v5.

Thanks,
Dinh

Re: [PATCHv4 09/12] usb: dwc2: initialize the spin_lock for both host and gadget

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:20:00 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Move spin_lock_init to common location for both host and gadget.

This should be done at the same time that gadget/host probing is merged
to preserve bisectability.

Also patch #12 ("usb: dwc2: pci: Update pci portion of the dwc2 driver")
should be merged into this one.

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
> v4: move spin_lock_init up to make sure sure no locks can be taken before
>     the init.
> ---
>  drivers/usb/dwc2/hcd.c      | 1 -
>  drivers/usb/dwc2/platform.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 07a7bcd..c6778d9 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq,
>  
>  	hcd->has_tt = 1;
>  
> -	spin_lock_init(&hsotg->lock);
>  	((struct wrapper_priv_data *) &hcd->hcd_priv)->hsotg = hsotg;
>  	hsotg->priv = hcd;
>  
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 2871f351..278135d 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -183,6 +183,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  
>  	hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>  
> +	spin_lock_init(&hsotg->lock);
>  	if (IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)) {
>  		retval = dwc2_gadget_init(hsotg, irq);
>  		if (retval)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 10/12] usb: dwc2: Add suspend/resume for gadget

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:20:01 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Move suspend/resume code to common platform code.

This should be done at the same time that gadget/host probing is merged
to preserve bisectability.

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
>  drivers/usb/dwc2/core.h     |  6 ++++++
>  drivers/usb/dwc2/gadget.c   |  4 ++--
>  drivers/usb/dwc2/platform.c | 23 +++++++++++++++++++++++
>  3 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index bbb0f52..5bb7e801 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -964,12 +964,18 @@ extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg);
>  #if defined(CONFIG_USB_DWC2_PERIPHERAL) || defined(CONFIG_USB_DWC2_DUAL_ROLE)
>  extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg);
>  extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2);
> +extern int s3c_hsotg_suspend(struct dwc2_hsotg *dwc2);
> +extern int s3c_hsotg_resume(struct dwc2_hsotg *dwc2);
>  extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq);
>  irqreturn_t s3c_hsotg_irq(int irq, void *pw);
>  #else
>  static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {}
>  static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2)
>  { return 0; }
> +static inline int s3c_hsotg_suspend(struct dwc2_hsotg *dwc2)
> +{ return 0; }
> +static inline int s3c_hsotg_resume(struct dwc2_hsotg *dwc2)
> +{ return 0; }
>  static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>  { return 0; }
>  static inline irqreturn_t s3c_hsotg_irq(int irq, void *pw)
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index aab1b45..09d591e 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3552,7 +3552,7 @@ int s3c_hsotg_remove(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> -static int s3c_hsotg_suspend(struct dwc2_hsotg *hsotg)
> +int s3c_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  {
>  	unsigned long flags;
>  	int ret = 0;
> @@ -3581,7 +3581,7 @@ static int s3c_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  	return ret;
>  }
>  
> -static int s3c_hsotg_resume(struct dwc2_hsotg *hsotg)
> +int s3c_hsotg_resume(struct dwc2_hsotg *hsotg)
>  {
>  	unsigned long flags;
>  	int ret = 0;
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 278135d..59265ad 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -206,6 +206,27 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	return retval;
>  }
>  
> +static int dwc2_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	struct dwc2_hsotg *dwc2 = platform_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (dwc2_is_device_mode(dwc2))
> +		ret = s3c_hsotg_suspend(dwc2);
> +	return ret;
> +}
> +
> +static int dwc2_resume(struct platform_device *dev)
> +{
> +	struct dwc2_hsotg *dwc2 = platform_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (dwc2_is_device_mode(dwc2))
> +		ret = s3c_hsotg_resume(dwc2);
> +
> +	return ret;
> +}
> +
>  static struct platform_driver dwc2_platform_driver = {
>  	.driver = {
>  		.name = dwc2_driver_name,
> @@ -213,6 +234,8 @@ static struct platform_driver dwc2_platform_driver = {
>  	},
>  	.probe = dwc2_driver_probe,
>  	.remove = dwc2_driver_remove,
> +	.suspend = dwc2_suspend,
> +	.resume = dwc2_resume,

Please use the new PM API (struct dev_pm_ops).

>  };
>  
>  module_platform_driver(dwc2_platform_driver);

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 11/12] usb: dwc2: check that the host work queue is valid

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:20:02 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> The Host workqueue will not get initialized if the driver is configured for
> peripheral mode only. Thus we need to check for wq_otg before calling
> queue_work().

This should be done at the same time that gadget/host probing is merged
to preserve bisectability.

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
>  drivers/usb/dwc2/core_intr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 651785d..1240875 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -287,9 +287,11 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  	 * Release lock before scheduling workq as it holds spinlock during
>  	 * scheduling.
>  	 */
> -	spin_unlock(&hsotg->lock);
> -	queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> -	spin_lock(&hsotg->lock);
> +	if (hsotg->wq_otg) {
> +		spin_unlock(&hsotg->lock);
> +		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> +		spin_lock(&hsotg->lock);
> +	}
>  
>  	/* Clear interrupt */
>  	writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 00/12] usb: dwc2: Add support for dual role

[Bartlomiej Zolnierkiewicz]

[ added linux-kernel ML to cc: ]

Hi,

On Tuesday, August 26, 2014 11:19:51 AM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Hello,
> 
> This is version 4 of the patch series that combines the dwc2 gadget and host
> driver into a single dual role driver. Here are the main differences from V2:
> 
> - Patch 9/12 : Move spin_lock_init() earlier up in the function to guarantee
>   no locks can be taken before the initializing the spin_lock.
> 
> - Patch 12/12 : Same as patch 9/12.
> 
> - Added Acked-by: paulz for all patches except patch 9/12 and 12/12.
> 
> For v4, I have rebased the series on top of Greg KH's USB usb-linus tree
> [9b2667f usb: dwc2: gadget: Set the default EP max packet value as 8 bytes]
> and on top of the following patches that have not yet been applied:
> 
> Doug Anderson: usb: dwc2: Read GNPTXFSIZ when in forced HOST mode
> Kever Yang : usb: dwc2: add 'mode' which based on Kconfig select or dts setting
> Robert Baldyga : usb: dwc2/gadget: fix series <- 12 patches
> 
> As usual, tested on SOCFPGA(host, gadget, and dual-role) and on Rpi-B
> (host mode only).
> 
> I have pushed this series to a git repo to make it more convenient for people
> to test/review.
> 
> git://git.rocketboards.org/linux-socfpga-next.git dwc2_dual_role_v4

Please also add linux-kernel ML to next revision of the patchset
(helps to get reviewers).

Also sorry for the late review but people are not yet used to
the fact that dwc2 now also means "our" good old s3c-hsotg. ;-)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv4 00/12] usb: dwc2: Add support for dual role

[Dinh Nguyen]

On 09/12/2014 11:44 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> [ added linux-kernel ML to cc: ]
> 
> Hi,
> 
> On Tuesday, August 26, 2014 11:19:51 AM dinguyen@opensource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> Hello,
>>
>> This is version 4 of the patch series that combines the dwc2 gadget and host
>> driver into a single dual role driver. Here are the main differences from V2:
>>
>> - Patch 9/12 : Move spin_lock_init() earlier up in the function to guarantee
>>   no locks can be taken before the initializing the spin_lock.
>>
>> - Patch 12/12 : Same as patch 9/12.
>>
>> - Added Acked-by: paulz for all patches except patch 9/12 and 12/12.
>>
>> For v4, I have rebased the series on top of Greg KH's USB usb-linus tree
>> [9b2667f usb: dwc2: gadget: Set the default EP max packet value as 8 bytes]
>> and on top of the following patches that have not yet been applied:
>>
>> Doug Anderson: usb: dwc2: Read GNPTXFSIZ when in forced HOST mode
>> Kever Yang : usb: dwc2: add 'mode' which based on Kconfig select or dts setting
>> Robert Baldyga : usb: dwc2/gadget: fix series <- 12 patches
>>
>> As usual, tested on SOCFPGA(host, gadget, and dual-role) and on Rpi-B
>> (host mode only).
>>
>> I have pushed this series to a git repo to make it more convenient for people
>> to test/review.
>>
>> git://git.rocketboards.org/linux-socfpga-next.git dwc2_dual_role_v4
> 
> Please also add linux-kernel ML to next revision of the patchset
> (helps to get reviewers).
> 
> Also sorry for the late review but people are not yet used to
> the fact that dwc2 now also means "our" good old s3c-hsotg. ;-)
> 

Thanks for the review. I'll try to address all of your comments in the
next version.

Dinh