Re: [PATCH] usb: cdns3: attempt to fix Kconfig dependencies

["Arnd Bergmann" <arnd@arndb.de>]

On Thu, Apr 16, 2026, at 13:43, Peter Chen (CIX) wrote:
> On 26-04-13 18:45:38, Arnd Bergmann wrote:
>> On Mon, Apr 6, 2026, at 03:30, Peter Chen (CIX) wrote:
>> > 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
>> 
>> 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.

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.

> 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).

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.

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

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

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

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

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.

       Arnd