On Tue, 23 Aug 2011, Felipe Balbi wrote: > > Okay. But consider this case for a moment. Merely because the OMAP > > implementation requires a bridge device between the PCI and USB layers, > > that doesn't mean the Intel implementation should be forced to use one. > > Alan, my whole point is that this is hardly an OMAP-only thing. Just > look into the many different ARM SoCs we have. All right, try this instead: Merely because OMAP and a bunch of other SoC architectures require a bridge device between the PCI and USB layers, that doesn't mean the Intel implementation should be forced to use one. > > As I understand it, Sebastian's patch set would have created a fake > > platform-bus device even in the Intel case, not corresponding to any > > real hardware. > > > > Was my understanding wrong? > > Because the XHCI is a separate entity. This comment doesn't seem to be relevant to the question I asked. > The way things go at the level is > somewhat below (in a very bad pseudo-code): > > module xhci_hcd ( > <list of inputs>, > <list of outputs>, > <CPU-related signals>, > <IRQ line signal>, > <DMA related signals>, > <etc etc etc>); > > CPU_interface_block(<assign inputs and outputs correctly>); > real_usb_logic(<assign inputs and outputs correctly>); > > so there is a block which handles the BUS interconnection logic. Thet > CPU_interface_block is decoding the bus protocol. No matter if it's PCI, > AXI, OCP, AHB, or whatever else, you will have some entity handly the > integration with the CPU/SoC/Bus. So what? Sure, every PCI device has such an entity. Does that mean every pci_device structure needs to have a platform_device child? > > That's what I was trying to present. Namely, write an xhci-core > > module, with routines that can be invoked by various driver modules > > such as xhci-pci or xhci-omap, and do it in such a way that these > > routines take a struct device as an argument -- and are agnostic as to > > whether that struct device is embedded in a struct pci_device or a > > struct platform_device. > > Then what happens when you need two different devices (let's PCIe and > platform_device) on the same board, working at the same time ? Will you > build two modules ? Yes, there will be two modules: xhci-pci and xhci-platform. Each will contain nothing but glue code; the real work will be done by a third xhci-core module. > The way we proposed will have driver-core allocate two instances of the > xhci-hcd automatically, because two separate platform_devices will be > added. Actually, the way you proposed, one pci_device and two platform_devices would be added. Instead of one pci_device and one platform_device, which is what people would normally expect. > > Maybe Sebastian's work can be changed to work like this, perhaps even > > without the need for a separate xhci-core module. That would be fine > > with me. All I really object to is creating a platform_device > > structure in cases where it doesn't correspond to any real hardware. > > but the 'real hardware' exists. Even though we don't need to configure > it. You're missing the point. Sure, even Intel controllers contain real hardware to interface with the PCI bus. But that hardware is already bound to the pci_device structure; we don't need an extra do-nothing platform_device layer in this case. > > Besides, PCI _does_ provide things XHCI needs to use. Things like > > memory-mapped IO addresses, interrupt numbers, and other resources. > > memory-mapped IO, interrupt numbers and other resources isn't a PCI only > thing. And if you just platform_add_resources(res, ARRAY_SIZE(res)) the > platform_device will be able to use the same resources without having to > know about PCI. Or, if the drivers would call xhci_probe() directly, they could pass these resources as arguments. Other subsystems use this sort of approach, by the way. For example, it's analogous to scsi_add_host(), although the division of labor is somewhat different. In fact, if you so keen to add another device layer, it would make more sense to register the usb_hcd structure (or something like it) between the pci/platform device and the root hub. I'm not sure how much it would help, however -- apart from giving us a good way to break the one-to-one relationship between usb_hcd and usb_bus structures that Sarah has complained about. > > Finally, as far as the $SUBJECT patch is concerned, it looks like > > Sebastian was trying to avoid creating an hcd-platform.c source file by > > sharing the code in hcd-pci.c. Looking at it more closely, it appears > > to contain some layering violations. For example, consider where > > hcd_plat_pci_suspend() calls _suspend_common(). The first argument it > > passes, dev, points to the platform device whereas it's expected to > > point to the pci_device. > > > > I suspect the code sharing wasn't done correctly. Creating a new file > > instead might well work out better. The amount of code duplication > > probably wouldn't be all that large. > > Fine, then let's duplicate. I just thought the whole point was to share > code where possible to avoid duplicating. The driver tree has been > adding code at a very fast rate and sooner or later we will be the next > target wrt to code sharing. At this point it's not clear how much code Sebastian really ended up sharing. I've got a feeling it wasn't very much. And in the process, he made a mess of hcd-pci.c. Separating the files could easily end up being better. Besides, I'm talking about adding a single file that would handle _all_ the platform devices. Not a separate file for each architecture or platform. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
