Message-ID: <541B0030.3000904@opensource.altera.com>
Date:	Thu, 18 Sep 2014 10:54:24 -0500
From:	Dinh Nguyen <dinguyen@...nsource.altera.com>
To:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
CC:	<paulz@...opsys.com>, <gregkh@...uxfoundation.org>, <balbi@...com>,
	<dinh.linux@...il.com>, <swarren@...dotorg.org>,
	<matthijs@...in.nl>, <r.baldyga@...sung.com>,
	<jg1.han@...sung.com>, <sachin.kamat@...aro.org>,
	<ben-linux@...ff.org>, <dianders@...omium.org>,
	<kever.yang@...k-chips.com>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv4 01/12] usb: dwc2: Update Kconfig to support dual-role

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@...nsource.altera.com wrote:
>> From: Dinh Nguyen <dinguyen@...nsource.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@...nsource.altera.com>
>> Acked-by: Paul Zimmerman <paulz@...opsys.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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists