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

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

On 26-04-16 15:19:06, Arnd Bergmann wrote:
> 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.

Hi Arnd,

I agree the parent device (SoC glue layer) is the direct interface to
system bus, and child device (IP) is in it at SoC architecture. For USB
IP driver, it may not consider well for it, eg, dwc3/cdns3/chipidea.
So, there are below code for this consideration.

	dwc->sysdev_is_parent = device_property_read_bool(dev,
				"linux,sysdev_is_parent");
	if (dwc->sysdev_is_parent)
		dwc->sysdev = dwc->dev->parent;
	else
		dwc->sysdev = dwc->dev;

For PCIe, it uses the architecture you suggested that is the
IP part works like library and SoC glue layer is the only struct device.

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

Thanks for the pointer. I will trace the call paths that matter for cdns3
(and any Type-C/connector helpers we rely on) and see where we can switch
to the fwnode variant without breaking existing boards.

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

Fair point, I should have said we strongly prefer keeping a real struct device
anchored at the IP child node given how role-switch, Resources, IRQ, PHY,
runtime PM works today, while still making the driver logic robust if
we ever have to attach at the glue device instead.

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

Thanks for confirming this is an acceptable 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.

Understood that it is mostly cosmetic for configuration. I am not trying
to do that in this series; if we pursue it, it can be a follow-up once
the module layout is settled.

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

Noted, I will wait for any comment from Greg.

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

Good suggestion. I have applied this change and did more simplification.
I added your suggested-by tag, is it okay for you?

https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/cix.git/
branch: cdns3_kconfig_reorg

Below are changes compared to last version:

    Tighten CONFIG_USB_CDNS_SUPPORT dependencies so the umbrella follows
    host or gadget when either is built as a module. Match host and gadget
    bools to the cdns.ko tristate with USB=USB_CDNS3 and USB_GADGET=USB_CDNS3
    instead of comparing against USB_CDNS_SUPPORT.

    Link host.o when CONFIG_USB_CDNS3_HOST is enabled and use that symbol in
    host-export.h, removing the redundant CONFIG_USB_CDNS_HOST indirection.


-- 

Best regards,
Peter