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

["Peter Chen (CIX)" <peter.chen@kernel.org>]

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

Hi Arnd,

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.

I have an new idea for how to improve cdns3 Kconfig/Makefile structure, and I push
the code at: https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/cix.git/
branch: cdns3_kconfig_reorg

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?

-- 

Best regards,
Peter