usb: cdns3: attempt to fix Kconfig dependencies

My patch has seen some fairly light build testing at this point and is clearly an improvement over what is in linux-next, but there is still a risk that I missed some other corner cases.

From: Arnd Bergmann <[email protected]>

The way that dependencies between host and gadget mode, as well as cdns3 and cdnsp were handled was rather fragile before commit 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support").

After those changes, I get randconfig build failures:

arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `__cdnsp_gadget_init':
cdnsp-gadget.c:(.text+0x12da): undefined reference to `cdns_drd_gadget_on'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_pullup':
cdnsp-gadget.c:(.text+0x3030): undefined reference to `cdns_clear_vbus'
arm-linux-gnueabi-ld: cdnsp-gadget.c:(.text+0x3138): undefined reference to `cdns_set_vbus'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_exit':
cdnsp-gadget.c:(.text+0xe0): undefined reference to `cdns_drd_gadget_off'

and I see additional configurations that are broken. The main problem here is that the 'common' module links against both host and gadget support if they are enabled, but there are insufficient protections agains it being built-in if only one of them is built-in and the other is in a loadable module, causing link failures.

The use of IS_REACHABLE() in gadget-export.h works around a similar problem if one of cdns3 and cdnsp is built-in but the other one is =m. This one is worse because instead of a clear link failure, the logic just makes it not work at all despite support being enabled.

To improve this mess, throw out both the Makefile hacks and the IS_REACHABLE() hack and replace these with regular Kconfig dependencies that ensure each driver is only enabled when its dependencies are there, as we do in most other drivers. The main downside here is that there is no good way to have built-in gadget support on cdn3 along with USB=m. Fixing this part proper would require cleaning up the code to turn the 'common' parts into a library module that only gets called by the other drivers but does not interact with either host or gadget support itself.

Another problem that is not solved by this patch is the way that platform specific glue logic in this driver relies on having a soc specific device as the parent of a generic child, instead of the specific driver just calling into a common helper module. This may be impossible to fix without breaking the DT bindings.

Fixes 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support")

Signed-off-by Arnd Bergmann <[email protected]>

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

drivers/usb/cdns3/Kconfig +17 -5

+	tristate "Cadence USB3 generic platform support"
+	depends on USB_CDNSP || USB_CDNS3
+	depends on USB_CDNSP || !USB_CDNSP
+	depends on USB_CDNS3 || !USB_CDNS3
+	help
+	  The platform driver support is needed on any SoC integrating
+	  a variant of the Cadence USB3 or USBSSP dual-role controllers,
+	  e.g. the TI K3 or NXP i.MX series of SoCs.
 
 if USB_CDNS_SUPPORT
 
 config USB_CDNS3
 	tristate "Cadence USB3 Dual-Role Controller"
 	depends on USB_CDNS_SUPPORT
+	select USB_CDNS_HOST if USB_CDNS3_HOST
 	help
 	  Say Y here if your system has a Cadence USB3 dual-role controller.
 	  It supports: dual-role switch, Host-only, and Peripheral-only.

@@ -23,8 +36,9 @@ config USB_CDNS3
 if USB_CDNS3
 
 config USB_CDNS3_GADGET
-	bool "Cadence USB3 device controller"
+	tristate "Cadence USB3 device controller"
 	depends on USB_GADGET=y || USB_GADGET=USB_CDNS3
+	depends on USB_CDNS_SUPPORT
 	help
 	  Say Y here to enable device controller functionality of the
 	  Cadence USBSS-DEV driver.

@@ -34,8 +48,7 @@ config USB_CDNS3_GADGET
 
 config USB_CDNS3_HOST
 	bool "Cadence USB3 host controller"
-	depends on USB=y || USB=USB_CDNS3
-	select USB_CDNS_HOST
+	depends on USB=y || USB=USB_CDNS_SUPPORT
 	help
 	  Say Y here to enable host controller functionality of the
 	  Cadence driver.

@@ -57,6 +70,7 @@ config USB_CDNS3_PCI_WRAP
 config USB_CDNS3_TI
 	tristate "Cadence USB3 support on TI platforms"
 	depends on ARCH_K3 || COMPILE_TEST
+	depends on USB_CDNS_PLATFORM
 	default USB_CDNS3
 	help
 	  Say 'Y' or 'M' here if you are building for Texas Instruments

@@ -67,6 +81,7 @@ config USB_CDNS3_TI
 config USB_CDNS3_IMX
 	tristate "Cadence USB3 support on NXP i.MX platforms"
 	depends on ARCH_MXC || COMPILE_TEST
+	depends on USB_CDNS_PLATFORM
 	default USB_CDNS3
 	help
 	  Say 'Y' or 'M' here if you are building for NXP i.MX

@@ -77,6 +92,7 @@ config USB_CDNS3_IMX
 config USB_CDNS3_STARFIVE
 	tristate "Cadence USB3 support on StarFive SoC platforms"
 	depends on ARCH_STARFIVE || COMPILE_TEST
+	depends on USB_CDNS_PLATFORM
 	help
 	  Say 'Y' or 'M' here if you are building for StarFive SoCs
 	  platforms that contain Cadence USB3 controller core.

@@ -91,6 +107,7 @@ endif # USB_CDNS3
 config USB_CDNSP
 	tristate "Cadence USBSSP Dual-Role Controller"
 	depends on USB_CDNS_SUPPORT
+	select USB_CDNS_HOST if USB_CDNSP_HOST
 	help
 	  Say Y here if your system has a Cadence USBSSP dual-role controller.
 	  It supports: dual-role switch, Host-only, and Peripheral-only.

@@ -115,8 +132,7 @@ config USB_CDNSP_GADGET
 
 config USB_CDNSP_HOST
 	bool "Cadence USBSSP host controller"
-	depends on USB=y || USB=USB_CDNSP
-	select USB_CDNS_HOST
+	depends on USB=y || USB=USB_CDNS_SUPPORT
 	help
 	  Say Y here to enable host controller functionality of the
 	  Cadence driver.

drivers/usb/cdns3/Makefile +5 -12

index 63484f145bb9..f1e00ab3d729 100644
@@ -5,32 +5,25 @@ CFLAGS_cdnsp-trace.o				:= -I$(src)
 
 cdns-usb-common-y				:= core.o drd.o
 
-ifeq ($(CONFIG_USB),m)
-obj-m						+= cdns-usb-common.o
-obj-m						+= cdns3-plat.o
-else
 obj-$(CONFIG_USB_CDNS_SUPPORT)			+= cdns-usb-common.o
-obj-$(CONFIG_USB_CDNS_SUPPORT)			+= cdns3-plat.o
-endif
+
+obj-$(CONFIG_USB_CDNS_PLATFORM)			+= cdns3-plat.o
 
 cdns-usb-common-$(CONFIG_USB_CDNS_HOST) 	+= host.o
 
 # For CDNS3 gadget
-ifneq ($(CONFIG_USB_CDNS3_GADGET),)
 cdns3-y						:= cdns3-gadget.o cdns3-ep0.o
 cdns3-$(CONFIG_TRACING)				+= cdns3-trace.o
-obj-$(CONFIG_USB_CDNS3)				+= cdns3.o
-endif
+obj-$(CONFIG_USB_CDNS3_GADGET)			+= cdns3.o
+
 obj-$(CONFIG_USB_CDNS3_PCI_WRAP)		+= cdns3-pci-wrap.o
 obj-$(CONFIG_USB_CDNS3_TI)			+= cdns3-ti.o
 obj-$(CONFIG_USB_CDNS3_IMX)			+= cdns3-imx.o
 obj-$(CONFIG_USB_CDNS3_STARFIVE)		+= cdns3-starfive.o
 
 # For CDNSP gadget
-ifneq ($(CONFIG_USB_CDNSP_GADGET),)
 cdnsp-y						:= cdnsp-ring.o cdnsp-gadget.o \
 						   cdnsp-mem.o cdnsp-ep0.o
 cdnsp-$(CONFIG_TRACING)				+= cdnsp-trace.o
-obj-$(CONFIG_USB_CDNSP)				+= cdnsp.o
-endif
+obj-$(CONFIG_USB_CDNSP_GADGET)			+= cdnsp.o
 obj-$(CONFIG_USB_CDNSP_PCI)			+= cdnsp-pci.o

drivers/usb/cdns3/gadget-export.h +2 -2

index 0cb600e2b5d2..c37b6269b001 100644
@@ -10,7 +10,7 @@
 #ifndef __LINUX_CDNS3_GADGET_EXPORT
 #define __LINUX_CDNS3_GADGET_EXPORT
 
-#if defined(CONFIG_USB_CDNSP_GADGET) && IS_REACHABLE(CONFIG_USB_CDNSP)
+#if IS_ENABLED(CONFIG_USB_CDNSP_GADGET)
 
 int cdnsp_gadget_init(struct cdns *cdns);
 #else

@@ -22,7 +22,7 @@ static inline int cdnsp_gadget_init(struct cdns *cdns)
 
 #endif /* CONFIG_USB_CDNSP_GADGET */
 
-#if defined(CONFIG_USB_CDNS3_GADGET) && IS_REACHABLE(CONFIG_USB_CDNS3)
+#if IS_ENABLED(CONFIG_USB_CDNS3_GADGET)
 
 int cdns3_gadget_init(struct cdns *cdns);
 #else

Signature · 1 line

2.39.5

Quoted 4 lines · Arnd Bergmann

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

It depends on Greg, I am okay for both ways. If Greg reverts the patch, I will do below improvements and adapts for most of your changes for v3 patch.

cdns-usb-common.ko is a libary, and no USB/GADGET dependency, could builds in.

  ├── core.o  
  └── drd.o

cdns3-host.ko -> depends on USB/XHCI（it is m when USB = m
cdns3.ko (gadget) -> depends on USB_GADGET
cdnsp.ko (gadget) -> depends on USB_GADGET
cdns3-plat.ko -> assign host_init/gadget_init function pointer

drivers/usb/cdns3/Kconfig config USB_CDNS_SUPPORT · line 16

Quoted 7 lines

drivers/usb/cdns3/Kconfig +3 -0

@@ -10,12 +11,24 @@ config USB_CDNS_SUPPORT
 
 config USB_CDNS_HOST
 	bool
+	depends on USB=y || USB=USB_CDNS_SUPPORT
+
+config CONFIG_USB_CDNS_PLATFORM

%s/CONFIG_USB_CDNS_PLATFORM/USB_CDNS_PLATFORM

Quoted 14 lines · Peter Chen (CIX)

Quoted 4 lines · Arnd Bergmann

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

It depends on Greg, I am okay for both ways. If Greg reverts the patch, I will do below improvements and adapts for most of your changes for v3 patch.

cdns-usb-common.ko is a libary, and no USB/GADGET dependency, could builds in.

  ├── core.o  
  └── drd.o

Quoted 4 lines

cdns3-host.ko -> depends on USB/XHCI（it is m when USB = m
cdns3.ko (gadget) -> depends on USB_GADGET
cdnsp.ko (gadget) -> depends on USB_GADGET
cdns3-plat.ko -> assign host_init/gadget_init function pointer

This all sounds good. Ideally cdns3-plat.ko would also lose the dependency on cdns3.ko/cdnsp.ko

Quoted 10 lines

Review config USB_CDNS_SUPPORT · line 16

Quoted 8 lines

(quoted diff) +3 -0

@@ -10,12 +11,24 @@ config USB_CDNS_SUPPORT
 
 config USB_CDNS_HOST
 	bool
+	depends on USB=y || USB=USB_CDNS_SUPPORT
+
+config CONFIG_USB_CDNS_PLATFORM

%s/CONFIG_USB_CDNS_PLATFORM/USB_CDNS_PLATFORM

Oh, this means my patch did not actually work at all, and my testing was pointless. I've fixed that now in my randconfig tree to see if anything else comes up.

Arnd

Quoted 1 line · Arnd Bergmann

On Fri, Apr 3, 2026, at 09:50, Peter Chen (CIX) wrote:

I am afraid it may need to change more for your patch, otherwise, it could have error with below combinations:

USB=y USB_GADGET=y USB_CDNS_SUPPORT=y USB_CDNS_HOST=y USB_CDNS_PLATFORM=y USB_CDNS3=y
USB_CDNS3_GADGET=y USB_CDNS3_HOST=y USB_CDNSP=y USB_CDNSP_GADGET=m USB_CDNSP_HOST=y

USB_CDNS_PLATFORM reference to the cdnsp_gadget_init which is built as module.

drivers/usb/cdns3/cdns3-plat.c:52: undefined reference to `cdnsp_gadget_init'

drivers/usb/cdns3/Kconfig +0 -1

@@ -13,11 +13,13 @@ config USB_CDNS_HOST
 	bool
 	depends on USB=y || USB=USB_CDNS_SUPPORT
 
-config CONFIG_USB_CDNS_PLATFORM

Quoted 6 lines

drivers/usb/cdns3/Kconfig +0 -1

@@ -13,11 +13,13 @@ config USB_CDNS_HOST
 	bool
 	depends on USB=y || USB=USB_CDNS_SUPPORT
 
-config CONFIG_USB_CDNS_PLATFORM

drivers/usb/cdns3/Kconfig config USB_CDNS_HOST · line 25

Quoted 10 lines

drivers/usb/cdns3/Kconfig +3 -0

+config USB_CDNS_PLATFORM
 	tristate "Cadence USB3 generic platform support"
 	depends on USB_CDNSP || USB_CDNS3
 	depends on USB_CDNSP || !USB_CDNSP
 	depends on USB_CDNS3 || !USB_CDNS3
+	depends on USB_CDNS3_GADGET || !USB_CDNS3_GADGET
+	depends on USB_CDNSP_GADGET || !USB_CDNSP_GADGET
 	help
 	  The platform driver support is needed on any SoC integrating
 	  a variant of the Cadence USB3 or USBSSP dual-role controllers,

The dependencies here are rather unwieldy, but it does look like this is the minimum set we need with the current code.

The only other alternative I see would be to split up the platform driver support into separate modules for cdns3 and cdnsp as well, which would make the dependencies trivial but require reworking of the actual in a way that I haven't been able to figure out yet. If you are already integrating other changes for the next attempt, maybe you can try to come up with a solution for this as well.

Quoted 8 lines · Arnd Bergmann

The only other alternative I see would be to split up the platform driver support into separate modules for cdns3 and cdnsp as well, which would make the dependencies trivial but require reworking of the actual in a way that I haven't been able to figure out yet. If you are already integrating other changes for the next attempt, maybe you can try to come up with a solution for this as well.

Thanks for your suggestion, creating different platform driver between cdns3 and cdnsp is the way we used at downstream, but when I try to upstream cdsnp platform driver support, I find the two platforms driver are 95% identical in content, so I would like to keep one platform driver and one binding doc.

By the way, are there any randconfigs I could run to avoid the similar break, of course, I will pass my local USB randconfigs tests?

Quoted 10 lines · Peter Chen (CIX)

Quoted 8 lines · Arnd Bergmann

The only other alternative I see would be to split up the platform driver support into separate modules for cdns3 and cdnsp as well, which would make the dependencies trivial but require reworking of the actual in a way that I haven't been able to figure out yet. If you are already integrating other changes for the next attempt, maybe you can try to come up with a solution for this as well.

Quoted 5 lines

Thanks for your suggestion, creating different platform driver between cdns3 and cdnsp is the way we used at downstream, but when I try to upstream cdsnp platform driver support, I find the two platforms driver are 95% identical in content, so I would like to keep one platform driver and one binding doc.

I gave this some more thought and realized that the best way to handle it is probably by reworking the cdns3 driver to no longer require the separate platform_device registration for the child device. This would make it work like most other drivers in the kernel, which helps both with the module dependencies and with new developers working on it.

The way I think this can work would be:

- turn drivers/usb/cdns3/cdns3-plat.c into a library module that exports the probe/remove/suspend/resume functions
- Remove the of_platform_populate()/platform_device_unregister() calls from soc specific drivers
- Change the individual probe/remove callbacks to
call the exported functions from the generic driver
- Integrate cdns3_platform_data into struct cdns, and pass that from the soc specific driver into the common code
- Set cdns->gadget_init in the soc specific driver

There may be additional steps needed to make this work, but the result should be much cleaner.

Quoted 13 lines · Arnd Bergmann

Quoted 1 line · Peter Chen (CIX)

On 26-04-03 20:50:52, Arnd Bergmann wrote:

- turn drivers/usb/cdns3/cdns3-plat.c into a library module that exports the probe/remove/suspend/resume functions
- Remove the of_platform_populate()/platform_device_unregister() calls from soc specific drivers
- Change the individual probe/remove callbacks to
call the exported functions from the generic driver
- Integrate cdns3_platform_data into struct cdns, and pass that from the soc specific driver into the common code
- Set cdns->gadget_init in the soc specific driver

Quoted 2 lines

There may be additional steps needed to make this work, but the result should be much cleaner.

Thanks for your time to improve this issue. But for Cadence IP, we began the architecture with parent (SoC) and child (IP) topology, created parent/child device tree yaml files
(eg, fsl,imx8qm-cdns3.yaml/cdns,usb3.yaml) as well.

If we kept the DT node but dropped a real struct device for the IP controller (e.g. only the glue struct device existed while the IP stayed "node-only"), several things become fragile or outright wrong, even we could change cdns3 code to use parent device, and work out for solution like "device_property_read_bool(dev, "usb-role-switch") on cdns->dev. But for usb-role-switch and Type-C graph/connection logic are
the painful case, and could not easy to find the solution.

When something (e.g. Type-C/connector path) tries to resolve a "usb-role-switch" connection, the match callback does:

static void *usb_role_switch_match(const struct fwnode_handle *fwnode,
const char *id,
void *data)

{
	struct device *dev;
	if (id && !fwnode_property_present(fwnode, id))
		return NULL;
	dev = class_find_device_by_fwnode(&role_class, fwnode);
	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}

class_find_device_by_fwnode() ultimately uses device_match_fwnode, which is pointer equality on dev_fwnode(dev):

int device_match_fwnode(struct device *dev, const void *fwnode)

{
	return fwnode && dev_fwnode(dev) == fwnode;
}

So the only role switch that matches is one whose registered sw->dev has dev_fwnode(&sw->dev) equal to the fwnode passed into usb_role_switch_match() (whatever the graph/connection layer produced for that link often is
cdns,usb3 child, but not the SoC glue parent).

So we are not arguing for "DT for documentation only"; we need the child platform device as the anchor that matches the IP node and the properties that the Cadence DRD code actually consumes.

Brief summary of what I did:

Expose Cadence USBSSP through the same platform path as USBSS, trim Kconfig and Makefile: one core loadable object plus separate glue .ko files.

Single cdns.ko bundles core, DRD, the generic "cdns,usb3" platform driver in cdns3-plat.c, optional host.o, and optional gadget objects. Use CONFIG_USB_CDNS3_GADGET as a bool to compile gadget support into that module. Remove duplicate MODULE_* declarations from cdns3-plat.c now that it links into the same module.

Kconfig: the generic platform driver is selected via CONFIG_USB_CDNS3. Move CONFIG_USB_CDNSP_PCI beside CONFIG_USB_CDNS3_PCI_WRAP under "Platform glue driver support". SoC glue entries (TI, i.MX, StarFive) depend only on CONFIG_USB_CDNS3.

Export cdns_core_init_role and re-orginize the function cdns_init, and controller version could be gotten before the gadget init function is decided per controller.

Keep host_init / gadget_init callbacks in struct cdns, so core.c does not need direct linkage to host or gadget objects. Refactor cdnsp-pci.c into a thin PCI-to-platform wrapper.

drivers/usb/Makefile: descend into drivers/usb/cdns3/ only when CONFIG_USB_CDNS_SUPPORT is enabled.

Is this solution okay for you?

Hi Peter,

I agree that this will stay a little awkward due to the currently defined DT binding, but I would expect that even for the driver itself, the normal way to handle this becomes simpler rather than adding problems. E.g. the use of DMA on the child device is rather questionable since the parent device is the actual DMA master, and any properties defining how DMA works on the parent bus (dma-ranges, iommu, ...) don't directly propagate to the cdns3 device when that bypasses the normal platform device probing.

Quoted 19 lines

When something (e.g. Type-C/connector path) tries to resolve a "usb-role-switch" connection, the match callback does:

static void *usb_role_switch_match(const struct fwnode_handle *fwnode,
const char *id,
void *data)

{
	struct device *dev;
	if (id && !fwnode_property_present(fwnode, id))
		return NULL;
	dev = class_find_device_by_fwnode(&role_class, fwnode);
	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
}

class_find_device_by_fwnode() ultimately uses device_match_fwnode, which is pointer equality on dev_fwnode(dev):

int device_match_fwnode(struct device *dev, const void *fwnode)

Quoted 8 lines

{
	return fwnode && dev_fwnode(dev) == fwnode;
}

So the only role switch that matches is one whose registered sw->dev has dev_fwnode(&sw->dev) equal to the fwnode passed into usb_role_switch_match() (whatever the graph/connection layer produced for that link often is
cdns,usb3 child, but not the SoC glue parent).

Could the callers of usb_role_switch_get() change to calling fwnode_usb_role_switch_get() instead? It looks like that already handles the case of an device node without a struct device attached to it.

Quoted 4 lines

So we are not arguing for "DT for documentation only"; we need the child platform device as the anchor that matches the IP node and the properties that the Cadence DRD code actually consumes.

I'm not convinced that "need" is correct here, since no other subsystem aside from cdns3 and xhci seems to follow the idea of having a separate platform device for a common component inside of a SoC integration. The DT binding of course can't be fixed any more, but the code should be flexible enough to just make it work either way.

Quoted 7 lines

Brief summary of what I did:

Expose Cadence USBSSP through the same platform path as USBSS, trim Kconfig and Makefile: one core loadable object plus separate glue .ko files.

Quoted 5 lines

Single cdns.ko bundles core, DRD, the generic "cdns,usb3" platform driver in cdns3-plat.c, optional host.o, and optional gadget objects. Use CONFIG_USB_CDNS3_GADGET as a bool to compile gadget support into that module. Remove duplicate MODULE_* declarations from cdns3-plat.c now that it links into the same module.

Right, as far as I can tell, this should work correctly and is a nice simplification.

Quoted 17 lines

Kconfig: the generic platform driver is selected via CONFIG_USB_CDNS3. Move CONFIG_USB_CDNSP_PCI beside CONFIG_USB_CDNS3_PCI_WRAP under "Platform glue driver support". SoC glue entries (TI, i.MX, StarFive) depend only on CONFIG_USB_CDNS3.

Export cdns_core_init_role and re-orginize the function cdns_init, and controller version could be gotten before the gadget init function is decided per controller.

Keep host_init / gadget_init callbacks in struct cdns, so core.c does not need direct linkage to host or gadget objects. Refactor cdnsp-pci.c into a thin PCI-to-platform wrapper.

drivers/usb/Makefile: descend into drivers/usb/cdns3/ only when CONFIG_USB_CDNS_SUPPORT is enabled.

Quoted 1 line

Is this solution okay for you?

I can certainly live with that. I would still prefer the internal platform device to go away completely, but that would not be a functional change, only internal cosmetics, and it has no impact on configuration questions.

I'll leave it up to Greg to chime in if he has a strong opinion on the matter, but I assume he's fine with your patch as well.

A minor variation you could consider would be to make the common module just

depends on USB || USB_GADGET
depends on USB if !USB_GADGET
depends on USB_GADGET if !USB

to make it always a loadable module if either USB or USB_GADGET is =m, and hide the symbols controlling host and gadget mode individually.

Quoted 36 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

The way that dependencies between host and gadget mode, as well as cdns3 and cdnsp were handled was rather fragile before commit 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support").

After those changes, I get randconfig build failures:

arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `__cdnsp_gadget_init':
cdnsp-gadget.c:(.text+0x12da): undefined reference to `cdns_drd_gadget_on'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_pullup':
cdnsp-gadget.c:(.text+0x3030): undefined reference to `cdns_clear_vbus'
arm-linux-gnueabi-ld: cdnsp-gadget.c:(.text+0x3138): undefined reference to `cdns_set_vbus'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_exit':
cdnsp-gadget.c:(.text+0xe0): undefined reference to `cdns_drd_gadget_off'

and I see additional configurations that are broken. The main problem here is that the 'common' module links against both host and gadget support if they are enabled, but there are insufficient protections agains it being built-in if only one of them is built-in and the other is in a loadable module, causing link failures.

The use of IS_REACHABLE() in gadget-export.h works around a similar problem if one of cdns3 and cdnsp is built-in but the other one is =m. This one is worse because instead of a clear link failure, the logic just makes it not work at all despite support being enabled.

To improve this mess, throw out both the Makefile hacks and the IS_REACHABLE() hack and replace these with regular Kconfig dependencies that ensure each driver is only enabled when its dependencies are there, as we do in most other drivers. The main downside here is that there is no good way to have built-in gadget support on cdn3 along with USB=m. Fixing this part proper would require cleaning up the code to turn the 'common' parts into a library module that only gets called by the other drivers but does not interact with either host or gadget support itself.

Quoted 7 lines

Another problem that is not solved by this patch is the way that platform specific glue logic in this driver relies on having a soc specific device as the parent of a generic child, instead of the specific driver just calling into a common helper module. This may be impossible to fix without breaking the DT bindings.

Fixes 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support")

Hi Arnd,

Thanks for fixing it, I am sorry for taking your effort debug it.

Quoted 5 lines

Signed-off-by Arnd Bergmann <[email protected]>

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

It depends on Greg, I am okay for both ways. If Greg reverts the patch, I will do below improvements and adapts for most of your changes for v3 patch.

Quoted 55 lines · Peter Chen (CIX)

Quoted 36 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

The way that dependencies between host and gadget mode, as well as cdns3 and cdnsp were handled was rather fragile before commit 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support").

After those changes, I get randconfig build failures:

arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `__cdnsp_gadget_init':
cdnsp-gadget.c:(.text+0x12da): undefined reference to `cdns_drd_gadget_on'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_pullup':
cdnsp-gadget.c:(.text+0x3030): undefined reference to `cdns_clear_vbus'
arm-linux-gnueabi-ld: cdnsp-gadget.c:(.text+0x3138): undefined reference to `cdns_set_vbus'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_exit':
cdnsp-gadget.c:(.text+0xe0): undefined reference to `cdns_drd_gadget_off'

and I see additional configurations that are broken. The main problem here is that the 'common' module links against both host and gadget support if they are enabled, but there are insufficient protections agains it being built-in if only one of them is built-in and the other is in a loadable module, causing link failures.

The use of IS_REACHABLE() in gadget-export.h works around a similar problem if one of cdns3 and cdnsp is built-in but the other one is =m. This one is worse because instead of a clear link failure, the logic just makes it not work at all despite support being enabled.

To improve this mess, throw out both the Makefile hacks and the IS_REACHABLE() hack and replace these with regular Kconfig dependencies that ensure each driver is only enabled when its dependencies are there, as we do in most other drivers. The main downside here is that there is no good way to have built-in gadget support on cdn3 along with USB=m. Fixing this part proper would require cleaning up the code to turn the 'common' parts into a library module that only gets called by the other drivers but does not interact with either host or gadget support itself.

Quoted 7 lines

Another problem that is not solved by this patch is the way that platform specific glue logic in this driver relies on having a soc specific device as the parent of a generic child, instead of the specific driver just calling into a common helper module. This may be impossible to fix without breaking the DT bindings.

Fixes 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support")

Hi Arnd,

Thanks for fixing it, I am sorry for taking your effort debug it.

Quoted 5 lines

Signed-off-by Arnd Bergmann <[email protected]>

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

Quoted 3 lines

It depends on Greg, I am okay for both ways. If Greg reverts the patch, I will do below improvements and adapts for most of your changes for v3 patch.

I'll be glad to revert if you want me to, just let me know.

Quoted 60 lines · Greg Kroah-Hartman

Quoted 58 lines · Peter Chen (CIX)

Quoted 36 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>

The way that dependencies between host and gadget mode, as well as cdns3 and cdnsp were handled was rather fragile before commit 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support").

After those changes, I get randconfig build failures:

arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `__cdnsp_gadget_init':
cdnsp-gadget.c:(.text+0x12da): undefined reference to `cdns_drd_gadget_on'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_pullup':
cdnsp-gadget.c:(.text+0x3030): undefined reference to `cdns_clear_vbus'
arm-linux-gnueabi-ld: cdnsp-gadget.c:(.text+0x3138): undefined reference to `cdns_set_vbus'
arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_exit':
cdnsp-gadget.c:(.text+0xe0): undefined reference to `cdns_drd_gadget_off'

and I see additional configurations that are broken. The main problem here is that the 'common' module links against both host and gadget support if they are enabled, but there are insufficient protections agains it being built-in if only one of them is built-in and the other is in a loadable module, causing link failures.

The use of IS_REACHABLE() in gadget-export.h works around a similar problem if one of cdns3 and cdnsp is built-in but the other one is =m. This one is worse because instead of a clear link failure, the logic just makes it not work at all despite support being enabled.

To improve this mess, throw out both the Makefile hacks and the IS_REACHABLE() hack and replace these with regular Kconfig dependencies that ensure each driver is only enabled when its dependencies are there, as we do in most other drivers. The main downside here is that there is no good way to have built-in gadget support on cdn3 along with USB=m. Fixing this part proper would require cleaning up the code to turn the 'common' parts into a library module that only gets called by the other drivers but does not interact with either host or gadget support itself.

Quoted 7 lines

Another problem that is not solved by this patch is the way that platform specific glue logic in this driver relies on having a soc specific device as the parent of a generic child, instead of the specific driver just calling into a common helper module. This may be impossible to fix without breaking the DT bindings.

Fixes 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support")

Hi Arnd,

Thanks for fixing it, I am sorry for taking your effort debug it.

Quoted 5 lines

Signed-off-by Arnd Bergmann <[email protected]>

---
TBH, I would be more comfortable with reverting 6076388ca1ed altogether and asking for a new version with the proper fixups included along with more testing for the next merge window.

It depends on Greg, I am okay for both ways. If Greg reverts the patch, I will do below improvements and adapts for most of your changes for v3 patch.

Quoted 2 lines

I'll be glad to revert if you want me to, just let me know.

Thanks, Greg. Please revert below two patches in your usb-next branch:

6076388ca1ed usb: cdns3: Add USBSSP platform driver support fb14e7f7cbb4 dt-bindings: usb: cdns,usb3: document USBSSP controller support