Hi, On Fri, Sep 09, 2011 at 02:04:18AM -0400, Alan Stern wrote: > On Fri, 9 Sep 2011, Felipe Balbi wrote: > > > > Apart from the hc_driver and various registration calls, the duplicated > > > code is all stuff to do with locating and acquiring various resources: > > > IRQ vectors and memory-mapped I/O regions. Clearly locating these > > > resources will be vary, depending on the bus type. On the other hand, > > > acquiring them could be moved into usbcore. (That's where it is > > > already for PCI controllers.) > > > > you mean ioremap() and that sort of thing ? ehci-$arch.c should only > > pass the correct resource down to ehci-hcd.c. And ehci-hcd.c should > > request_mem_region() and ioremap_nocache() it. > > Yes, that's what I mean. Except that the request_mem_region() and > ioremap_nocache() could be done in usbcore rather than in ehci-hcd.c. I see. OTOH, ehci-hcd, does need to access its own memory space, right ? But if you're talking about something like ehci-hcd calling a certain API where it will ioremap() the resource and return the virtual address, then it might work just fine. > > > There's no good reason to dump all this information into more platform > > > resources. There already are fields for these things in the hcd > > > structure. The code to populate those fields could be shared among all > > > the ehci-<arch> drivers. > > > > my point is that the very existence of those fields in struct hc_driver > > (You mean struct usb_hcd.) (yes, sorry :-) > > > > > No, the runtime PM support _is_ present, but it is in usbcore instead > > > > > in the host controller drivers. > > > > > > > > We can use device hierarchy for that, no ? Instead of adding methods to > > > > struct hc_driver, implement things on dev_pm_ops. > > > > > > I don't understand. We _are_ using the device hierarchy for this. The > > > dev_pm_ops methods invoke the callbacks in hc_driver. I don't see any > > > > why does dev_pm_ops need to directly call hc_diver->bus_suspend() when > > you can add another dev_pm_ops to struct ehci-hcd.c ?? > > I think you're mixing up two different layers of the device model. > The bus_suspend and bus_resume routines are used for power management > of the root hub, whereas the dev_pm_ops methods are used for power > management of the controller device itself. > > Thus the bus_suspend and bus_resume affect things like the downstream > USB ports and the frame counter, whereas (in the PCI driver, for > example) the dev_pm_ops methods put the entire controller into D0 or > D3. Hmm, you're right. But couldn't we make it so that ehci-hcd's dev_pm_ops handle the bus states and PCI, if it's is a parent device to ehci-hcd, puts the controller to D0 or D3 ? I mean, an EHCI controller has many ports and each port can have many children devices. So, when all children of a specific port are suspended, that port goes to USB Bus Suspend, and when all ports are in suspend state, PCI driver's runtime_suspend will get called, and that will put the controller to D0 or D3. Does that work ? What I mean is that ehci-hcd.c::runtime_suspend() method will handle USB ports and frame counter and pci.c::runtime_suspend() would handle controller states. Does that work ? > > > way around this, because the dev_pm_ops methods are part of usbcore, > > > not duplicated in each host controller driver. > > > > I think PM needs to be very granular. What I mean by that is that we go > > from the very generic implementation to the very HW specific going up > > the device hierarchy. > > > > So, usbcore should care about USB devices and interfaces, ehci-hcd.c > > should care about specific bus states and ehci-omap.c should care about > > clocks, context save&restore and the OMAP Architecture code > > (arch/arm/*omap*) should care about clock domains, power domains, > > voltage domains and so on. > > That's essentially the way it works now. not so much, if usbcore needs to call specific function pointers by itself, rather than allowing PM framework do that IMO. > > This is what I wanted to see. Rather than making usbcore call > > hc_driver->bus_suspend, let the PM layer handle all the trickery for > > you. ehci-hcd's dev_pm_ops::ehci_suspend() will only be called when all > > its children are suspended anyway. Likewise, ehci-hcd's > > dev_pm_ops::ehci_resume() will be called before resuming any child. > > > > IMHO, the current implementation is the one going through all hops and > > loops, in a sense, reimplementing things which are generic on the PM > > framework. > > It sounds like you don't have a complete picture of how USB power > management is implemented. Almost all of the things you're asking for > are things we already do. The one exception is something we can't do > at all, because root hubs have to use the same dev_pm_ops method > pointers as any other hub. that's a good point. I had missed that :-p -- balbi
Attachment:
signature.asc
Description: Digital signature
