Hi, On Thu, Aug 25, 2011 at 04:27:45PM -0400, Alan Stern wrote: > On Thu, 25 Aug 2011, Felipe Balbi wrote: > > > > Then why not start making modifications to ehci-hcd by uniting all > > > those different platform drivers into a single source file? You'd have > > > to do that anyway; might as well begin now since it's > > > non-controversial. > > > > > > If that can be made to work well, it will be a very strong argument for > > > making PCI use a platform device too. > > > > Yes, that can be done. Sounds to me, though, like we will have a > > duplication of effort. To see how things look like when we have a PCI > > device bridging to a platform device, you just have to look into Greg's > > for-next branch. The DesignWare USB3 driver which I wrote with > > Sebastian follow that approach and we have PCI and OMAP glue layers. > > That's not what I meant. DesignWare is a single platform, but I want DesignWare USB3 is one IP/Device. It currenlty supports 2 platforms. Namely PCI/x86 and OMAP/ARM. > to see how it looks when somebody handles a bunch of platforms in a > single source file. For example, is it possible to merge ehci-fsl.c, > ehci-xilinx-of.c, ehci-sh.c, and ehci-omap.c into a single file without > getting something that looks like Frankenstein's monster? I find it very unlikely, but what's the point ? That's really not what we want. What's common should definitely go to a common file - like when I moved the hc_driver from xhci-pci.c to xhci.c -, on the other hand, if it's platform-specific code, why do we want to combine them ? When I talk about common code, I mean the duplicated hc_driver on ehci-<arch>.c files. That could all go into a common source (ehci-hcd.c?) and the arch files become smaller and easier to maintain (less code == less trouble). > > > > I remember proposing that long ago and Dave was against it because it > > > > would make "copies" of the entire ehci stack right ? So if we had an > > > > OMAP board with a PCIe controller, we would have two big ehci drivers. > > > > My proposal was to reuse ehci-hcd.ko without having to link it to every > > > > single ehci-<arch>.o > > > > > > It wouldn't have to be linked to every ehci-<arch>.o; only the one > > > that the kernel is configured for. Right now you can't build more than > > > one of them, right? Otherwise there would be multiple definitions for > > > the PLATFORM_DRIVER symbol. > > > > You don't seem to get the big picture. At the end of the day we want to > > have a distro-like kernel for embedded too. And that would mean e.g. > > building all ehci-<arch>.o which are ARM-based plus the PCI since few > > ARM-based board have PCI. > > Ah, that's a different matter. In fact, right now there's no way to > bundle all those drivers in a single distribution, is there? I didn't Isn't that my point ? I'm trying to get to that goal. > realize you were concerned about this; I thought you were just bothered > by all the #ifdefs. The presence of those ifdefs (or decisions taken at compile time) prevents me from achieving that goal :-) > > > > > The one disadvantage would be that you couldn't have a single driver > > > > > module to handle both PCI and a platform device; they would have to be > > > > > separate drivers. > > > > > > > > true. > > > > > > There's also the PS3 system bus thingy -- yet another bus. > > > > So we _do_ agree that it's time to unify all of those, that's great. We > > just need to agree on the implementation details. > > Yes. > > There is another model to consider, the one used by usb-storage. The > main usb-storage module handles devices that use the standard CB, CBI, > and BBB transports, but there's a bunch of submodules that handle other > devices using proprietary transports. I see. > It works like this: usb-storage contains a usb_device_id table that > matches the standard subclass/protocol values, and it exports routines > that can be used by the submodules to do their work. Each submodule > has an id table that matches only the devices it can support. Sounds similar to the approach I took with dwc3. Where the core IP driver has a platform_device_id where we pass in "feature flags" for the driver to take decisions. Clearly, instead of a bunch of flags, we could pass a pointer to a structure (see the patch where I pass quirks via driver_data on XHCI). > The end result is that if you plug in a standard mass-storage device, > the main usb-storage module gets loaded and acts as the driver. If you > plug in one of the non-standard devices then the appropriate submodule > acts as a driver, and the main module is also loaded in order to > satisfy the export dependencies. Generally I like to avoid exported symbols where possible. That's why using the device hierarchy from driver core came to mind. > xhci-hcd could be made to work similarly, except that there would be no > "standard" driver -- there would just be a submodule driver for PCI > plus one or more for various platform devices. The main module would > contain only the xHCI core routines. > > I realize this involves an extra layer of software compared to what you > want, but it might end up being cleaner -- especially if other vendors > design their own USB-3 IP blocks that are not sufficiently compatible > with your DesignWare code. We could handle them simply by adding > another IP-specific platform-driver submodule, whereas your scheme > really couldn't handle them without great difficulty. I didn't get this. DesignWare is one IP. Any licensee from Synopsys will get that IP, configure the way they want wrt FIFO sizes, number of endpoints, number of interrupters, number of RAMs, etc etc etc. But the IP itself (and the programming model) remains the same. So if SoC Vendor XYZ decides to license the IP from Synopsys, all they have to do is provide the parent device and allocate the core platform_device. If they need something a bit more complex, they can pass in platform_data or use the driver_data depending on the case. Now, with regards to xhci, this will become very similar. No matter where xhci is being used, other than Silicon bugs/quirks and compliance to XHCI 0.96 or 1.0, the programming model is the same. Meaning that struct hc_driver for xhci will be the same thing throughout. We can already see what I'm talking about with ehci-<arch>.c files. Take a look at their struct hc_driver structure and you'll see that they are just using ehci_* functions exported by ehci-core code. What I'm trying to say is that no matter where [ouex]hci is being used, the standard is the same, correct ? So ehci_urb_enqueue, ehci_irq, ehci_urb_dequeue and all their friends really don't need to be exported. If [ouex]hci have their own platform_device/platform_driver, those could be made static and [ouex]hci-<arch>.c can, at least, remove the hc_driver definition, since that's just a big pile of duplicated code. Now, I apologize if I didn't get your point and I ask you to describe further what you meant ;-) -- balbi
Attachment:
signature.asc
Description: Digital signature
