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

From: Dinh Nguyen
Date: Thu Sep 18 2014 - 11:59:57 EST


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@xxxxxxxxxxxxxxxxxxxxx wrote:
>> From: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxx>
>> Acked-by: Paul Zimmerman <paulz@xxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/