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

[Paul Zimmerman]

> From: Dinh Nguyen [mailto:dinguyen@xxxxxxxxxxxxxxxxxxxxx]
> 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@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.

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

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