Hi, On Tue, Aug 23, 2011 at 12:57:25PM -0400, Alan Stern wrote: > On Tue, 23 Aug 2011, Felipe Balbi wrote: > > > > There's no doubt that the patch is messy. > > > > > > This is because it tries to cater to two different approaches > > > simultaneously: the old (use the PCI device directly) and the new > > > (interpose a fake platform device below the PCI device). It would be > > > > I wouldn't call it fake. You _do_ have an IP core with a PCIe bridge > > around and that's what the patch tries to model. > > I don't understand. The IP core with the PCIe bridge already > corresponds to a pci_device, doesn't it? So then what does the new not really. Take for example the DesignWare USB3 IP driver which I just sent. The exact same IP can be configured (at RT level) as host only, DRD and device-only. The host side is XHCI. For the purpose of $SUBJECT let's talk about a host-only configuration, so here it goes: That particular IP talks AXI. That's how the verilog code was written, still we have TUSB7340 which is a host-only configuration of that IP (so an XHCI controller) on a PCIe card. Don't you agree that our HW engineers had to create an AXI-to-PCIe bridge ? Don't you aggree those are two different devices (on the same die) ? The same goes for the DRD configuration on OMAP5. OMAP5 has a "wrapper" around the core which is an integration context. It's a way for us to license an IP from some other company and make it fit into OMAP's context wrt to PM, clock network, IRQs, address space, etc. So the USB SS DRD subsystem in OMAP5 is composed of DesignWare USB3 IP + this extra wrapper, which is a different device. The wrapper has its own address space, its own IRQ, its own clocks, etc. > platform_device correspond to? it corresponds to the XHCI IP. > Look, in general, things that bridge between two different kinds of bus > get represented in the device model as two different device structures, > one for each bus type. Thus, a USB mass-storage device bridges a USB > bus with a (trivial) SCSI bus, and it is represented by the usb-storage > interface and the SCSI host. Similarly, a USB controller bridges a PCI > (or platform) bus with a USB bus, so it should be represented by a PCI > (or platform) device and a USB root hub. Not a PCI device plus a fake > platform device plus a root hub. The fact is that those IPs are generally licensed and a company which wants to make a PCIe card of a particular IP, needs to make a wrapper (an extra device). And to make it simpler to be reused, we need to split that functionality. > > If it was the best > > implementation, then we can discuss but the fact that we have TWO > > devices on such PCIe cards is irrefutable. Same goes for EHCI/OHCI/UHCI > > cores and those need to be changed too. > > What do you mean you have TWO devices on such PCIe cards? See above. > Are you talking about something like the traditional arrangement > whereby several controllers are bundled together as separate PCI > functions in the same PCI slot? The way Intel boards have UHCI unfortunately not :-( > controllers at 00:1d.0, 00:1d.1, and 00:1d.2 plus an EHCI controller at > 00:1d.7? Each of those PCI functions corresponds to a separate > pci_device structure, which is the parent of the controller's root hub > device. There's no need for an intervening platform device. see above. > > > Personally, I think it's more than a little odd to have the fake > > > platform device in the middle like that. It also runs the risk of > > > messing up existing userspace programs that do power management on the > > > controller device; they might easily contain references to paths like > > > /sys/bus/usb/devices/usb3/../power/. Certainly I have done things like > > > that in the past. > > > > This would mean those userspace programs are actually broken if they > > have hard-coded paths instead of trying to find the correct devices. > > In this case, the device the program is looking for is the parent of > usb3. The path I gave is the correct way to find that parent; no > searching is needed. ok. > > All in all, we are trying to prevent that big pile of ifdeferry we have > > on e.g. ehci-hcd.c. > > I'm well aware of all that. I'm asking if there isn't a different way > to accomplish the same result. I can't see any, if you come up with a better approach, I'm willing to consider :-) > > > For example, maybe the core parts of xhci-hcd could be made into a > > > standalone module, containing routines that would be called by separate > > > driver modules (one for xhci-pci and others for various platform > > > variants). There would be a disadvantage, because now two modules > > > would have to be loaded to manage an xHCI controller, but overall it > > > would be cleaner. > > > > I don't think so. I think your approach is inverted. Why should a PCI > > glue/bridge call into XHCI ? > > That's like asking why ehci-hcd should call into usbcore. It does so no, it's not. > because hcd-pci.c provides functions that it can use. There's nothing PCI provides that XHCI needs to know about. I can't see the need to invert things. > > All XHCI needs is a memory base and an IRQ > > number (well, quirks, PM, etc) and that can be easily passed by the "bus > > integration context" which I'm calling PCI glue/bridge on this mail. > > It can just as easily be passed by a PCI device driver with no need for > a platform-bus glue layer. For a PCI-only context, of course. But when we need to add PCI-less SoC devices, it starts to get funny and we will end up with ehci-hcd.c all over again. > > > And of course, the same approach could be applied to the other joint > > > PCI/platform HCDs. > > > > you're only thinking about PCI and for embedded platforms we don't have > > PCI. Most will have some OCP, or AXI/AHB bus and if we don't decouple > > PCI from the host stack we will have the current ifdef mess we're into. > > You don't seem to understand that I am proposing a way to do just that: > decouple PCI from the host stack. It's just that I'm proposing a > _different_ way from what Sebastian posted. I didn't see a proposal yet. In fact, all you said was this might not be a good approach. So, if you _do_ have a better approach, please show it to us. But to me splitting the IP into its own platform_device has worked pretty well. It allowed me, with dwc3 driver, to keep all PCI-specificities into dwc3-pci.c as well as all OMAP specificities into dwc3-omap.c without having to export any functions. I can also compile dwc3-pci.c and dwc3-omap.c on the same kernel which means, if I ever add a PCIe controller on OMAP5 and put my HAPS61 platform behind that PCIe controller, I'll have two dwc3 instances on OMAP5 and udev will know exactly which drivers to load ;-) -- balbi
Attachment:
signature.asc
Description: Digital signature
