On Thu, 1 Sep 2011, Felipe Balbi wrote:
> > > - usbcore <--> MUSB <--> Architcture
> > > + usbcore <--> usb_host <--> MUSB <--> architecture
> >
> > Your diagram is not a good description of the existing stack. We
> > _already_ have an intermediate glue layer; the true picture before and
> > after looks like this:
> >
> > - usbcore <--> usb_hcd <--> MUSB <--> architecture
> > + usbcore <--> usb_host <--> MUSB <--> architecture
> >
> > The main differences are renaming the structure and embedding a struct
> > device within what used to called struct usb_hcd.
> >
> > What you're talking about, by contrast, would look like this (using
> > OMAP as an example):
> >
> > usbcore <--> usb_hcd <--> MUSB <--> architecture
> > - usbcore <--> usb_hcd <--> ehci_omap <--> architecture
> > + usbcore <--> usb_hcd <--> ehci_platform <--> ehci_omap <--> architecture
>
> ehci_platform doesn't exist in my books. and ehci_omap == architecture
> code.
Earlier you called it just "ehci". On Aug. 31 you wrote:
> let udev do its thing:
>
> - arch code will add a platform_device to the driver model, e.g.:
> platform_device_register(&omap_ehci_device);
>
> - based on the device add uevent, udev will load the correct module (in
> this case ehci-omap.ko)
>
> - ehci-omap.ko instantiates and add EHCI-core platform_device:
> ehci = platform_device_alloc("ehci", -1);
> platform_device_add(ehci);
>
> - based on the device add uevent, udev will load the correct module (in
> this case ehci-hcd.ko)
The end result of this would be:
usbcore <--> usb_hcd <--> ehci <--> ehci_omap <--> omap-usb-host
> Just look into the host stack and you should be able to see all the
> problems I have listed:
>
> . too many exported functions
What exported functions?
$ cd drivers/usb/host
$ grep -l EXPORT *.c
octeon2-common.c
pci-quirks.c
sl811-hcd.c
$
None of those seem particularly bad. And your proposal wouldn't
remove them.
> . lots of ifdefs
How would you feel if those #ifdefs were moved from ehci-hcd.c into
a .h file? Many header files are littered with #ifdefs.
> . prevents a dristro approach for embedded
> . duplicated struct hc_driver all over
Those two are real problems.
> . every architecture needs to add some ?hci-<arch>.c which does
> essentially the same.
In principle, some architectures could share this code. But how much
of it really is the same? Most of the commonality lies in the probe
routines. The drivers locate the necessary resources and register
their hcd structures.
Now, locating resources is clearly platform-specific. Registering
the hcd and defining the hc_driver structure are the only truly
redundant parts.
> . ?hci-hcd stacks are PCI centric
In what way? The only PCI-centric part of ehci-hcd is ehci-pci.c,
which is pretty understandable. Likewise for ohci-hcd and uhci-hcd.
True, xhci-hcd is currently very PCI-centric. That's because until
very recently there was no non-PCI xHCI hardware available.
Aside from xhci-hcd, AFAICT the only PCI-centric part in the entire USB
stack is the pci_suspend and pci_resume fields in usb_hcd, and they can
be eliminated easily enough. (hcd-pci.c doesn't count; it's only
common routines shared among [eoux]hcd-pci.c. Likewise for
pci-quirks.c -- neither of those files gets compiled if CONFIG_PCI is
disabled.)
> . ?hci-hcd PM handling is coupled with the underlying architecture
Are you talking about the bus_suspend/bus_resume methods? The methods
themselves are in the common code; only the method pointers are
duplicated in the hc_driver structures.
> . single module available for a particular system, making "hybrid
> systems" (let's say, and ARM-SoC with internal EHCI and PCIe
> controller with discrete EHCI component) very difficult
That's not right. If the kernel config defines both PCI and a
platform driver, then both ehci-pci.c and the appropriate platform glue
file will be #included in ehci-hcd.c. The resulting module will be
able to drive both the internal EHCI and the external PCIe controller.
> . it's difficult to use linux-next properly today (you would have to
> rebuild kernel for each architecture to compile test all
> ehci-<arch>.c files.
Another genuine problem.
> . lack of pm_runtime on ?hci-hcd (it could be used for e.g.
> bus_suspend/bus_resume)
No, the runtime PM support _is_ present, but it is in usbcore instead
in the host controller drivers.
> . ARCH needs to know about EHCI and EHCI needs to know about ARCH (not
> very object orientation friendly)
> . It doesn't really model the underlying HW
It's possible to go too far in modelling the hardware. We don't need
to introduce a data structure for every transistor, or even for every
IP component.
> > Now, there's nothing wrong with any of this. I agree with your goals
> > (insofar as I am aware of them). But none of what you're saying is
> > inconsistent with expanding the usb_hcd structure into its own device
> > layer.
>
> I'm yet to see that. I don't think adding such a layer for all the hosts
> will solve the problem. Like I said, spliting usb_hcd to its own device,
> is tackling the problem at the wrong spot.
Well, we don't have to do that.
> I have no problems, as of today, with how EHCI stack talks to usbcore.
> What I see as wrong is how ehci-<arch>.c is talking to EHCI stack.
Here's a different approach which addresses all the problems you listed
above -- and does so without adding extra layers to the device model.
It's a lot like what you're suggesting, but without a bunch of
extraneous stuff.
Make each ehci-<arch>.c into a standalone module. Remove all the
duplicated hc_drivers structures from these files, put a single
publicly-available ehci_hc_driver in ehci-hcd.c, and make ehci-hcd a
separate module containing just the core routines.
To make this work, we have to find a way to handle all the individual
differences in the various hc_driver structures. Most of the
differences lie in the product_desc and reset fields.
We can get rid of product_desc altogether, and in its place use the
parent controller's driver name. I'm not sure exactly what would be
needed for the reset routines, but it mostly looks like we just need to
refactor some of the startup code.
A few drivers, like ehci-tegra.c, will be more troublesome. For such
cases it might be better to have a private hc_driver structure with
most of the entries copied from the public ehci_hc_driver.
How does that sound?
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
