Hi,
On Wed, Aug 31, 2011 at 10:53:01PM -0400, Alan Stern wrote:
> On Thu, 1 Sep 2011, Felipe Balbi wrote:
>
> > > I'm willing to go along with the idea of adding an extra device layer
> > > between the controller and the root hub. But I'm not so keen about
> > > making it a platform_device. What we need here is a general usb_host
> > > device that can be a new device_type on the USB bus, like we discussed
> > > earlier.
> >
> > I don't think this will be a good approach. You see, this will affect
> > the non-standard hosts too.
>
> To some extent, yes. I don't see anything wrong with that. You're
> proposing a significant change to part of the USB stack; it ought to
> affect the existing drivers.
>
> > Hosts like MUSB are completely non-standard, so they must provide their
> > own hc_driver. But they talk directly to usbcore.
> >
> > In the case of the standard hosts what we have is, for example:
> >
> > usbcore <--> EHCI Stack <--> Architecture
> >
> > What I'm proposing is to split the EHCI stack to its own
> > platform_driver/device. If you add a struct usb_host kinda thing, that
> > will affect MUSB too:
> >
> > - 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.
The only difference with my approach is that ehci-hcd would get a
clean module_init()/module_exit() and ehci-omap.c would have its own
module_init()/exit() too. In fact, ehci-omap.c could vanish and we would
only use the drivers/mfd/omap-usb-host.c driver. The big picture would
then be:
- usbcore <--> usb_hcd <--> ehci <--> ehci_omap <--> omap-usb-host
+ usbcore <--> usb_hcd <--> ehci <--> omap-usb-host
> This would be both inconsistent and more complicated.
I disagree, but that's fine.
> > Such change should not affect the non-standard host.
>
> Why not? A host controller driver is a host controller driver, whether
> it obeys some standard or not. In the non-standard case, where there
that's for sure, but we have always been talking about the standard
hosts here and how the standard stack can be easily re-used without
adding that ifdefery hell we have today.
> > I believe you still
> > didn't understand the problem which I'm trying to solve.
>
> Partly this is because you've never written a single, complete
> description of the problem. In various emails you have described
> various parts of it, but there was never a full picture including all
> the details and their relative importance.
how come ? I have always said that I want to get rid of the ifdefery
hell in ehci-hcd and have said that I want an easier way to re-use the
?hci-hcd stacks from an embedded perspective.
Just look into the host stack and you should be able to see all the
problems I have listed:
. too many exported functions
. lots of ifdefs
. prevents a dristro approach for embedded
. duplicated struct hc_driver all over
. every architecture needs to add some ?hci-<arch>.c which does
essentially the same.
. ?hci-hcd stacks are PCI centric
. ?hci-hcd PM handling is coupled with the underlying architecture
. 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
. 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.
. lack of pm_runtime on ?hci-hcd (it could be used for e.g.
bus_suspend/bus_resume)
. 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
. coupled with PCI
. coupled with PCI
. coupled with PCI
. coupled with PCI
. coupled with PCI
. coupled with PCI
/* add what you want here */
> > It's not about
> > how EHCI talks to usbcore, it's how architecture talks to EHCI.
> > Currently, we have lots of duplicated code on all archs (hc_driver,
> > reuse of ehci_* symbols, allocation of usb_hcd + ehci structures, etc
> > etc). And what I want is to pack all things which are mandated by the
> > EHCI specification into its own self-sufficient driver. Then
> > architecture code, handles only architecture-specific things.
>
> Even here you haven't given a complete description of what you want. I
> know this because earlier you said that you also want to be able to
> build all the platform drivers in a single distribution.
yes, of course. And can't you see that there ? Ok let me make it
clearer:
EHCI self-sufficient driver means a ehci-hcd.ko which can be loaded and
knows nothing about the underlying architecture.
architecture code, in this context, is all the ehci-<arch>.c files. They
should become their own driver (ehci-omap.ko, ehci-tegra.ko and so on)
and they should allocate platform_device for ehci-hcd.ko and pass
correct resources, set itself as a parent device, etc.
If we need to make a call into the architecture code, we should not
export a symbol. I think it's nicer to pass a function pointer via
platform_data and pass the child device's pointer as argument:
struct ehci_platform_data {
int (*reset)(struct device *child);
};
static int ehci_omap_reset(struct device *child)
{
struct ehci_omap *omap = dev_get_drvdata(child->parent);
do_ehci_omap_reset(omap);
return 0;
}
The amont of exported functions we have today is insane. The amount of
ifdef is insane. All the PCI-centric stuff is just a mess...
> 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.
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.
> > Like I have said before, no matter if it's PCI, OMAP, Tegra or whatever,
> > the EHCI specification remains the same. Meaning that the way you
> > enqueue an urb (except on quirky HW) is the same throughout all
> > IPs. Similarly, the way you suspend/resume the bus, control a port, get
> > a frame number, enable/disable an endpoint and so on is the same thing.
> > So if you know those things are all the same, why do you even bother
> > exporting that symbol ? It will be reused as is by most implementations.
> >
> > I really don't understand your reluctance to add a
> > platform_driver/device to ehci-hcd.
>
> I'm not against adding another driver layer to ehci-hcd; I'm just
> against making it a platform_driver. The platform bus is a sort of
> catch-all for devices that don't have any suitable bus of their own.
> But we do already have a perfectly good bus for these new devices to
> use. So why involve the platform layer?
there's no such bus today. The USB bus is composed of VBUS, Ground, D+
and D- signals (plus the SuperSpeed pairs) and the usb bus
representation in kernel is for devices attached behind those 4 (or 8)
USB signals. What you want to do is wrong, IMHO.
We want EHCI to have a "bus" to talk to the SoC or PCI-bridge. But by
using the usb bus, it will be like putting a EHCI core on those usb
signals.
> > it will also allow for re-factoring of bus
> > suspend/resume (we could probably implement those with pm_runtime; put
> > bus suspend on ->runtime_suspend and bus resume on ->runtime_resume).
>
> They already are there, although the code path is a little convoluted.
> It might be possible to refactor this as you suggest; I'm not sure.
It's definitely possible, let's assum ehci-hcd has it's own
platform_device/driver:
struct dev_pm_ops ehci_pm_ops = {
.runtime_resume = ehci_bus_resume,
.runtime_suspend = ehci_bus_suspend,
};
ehci-omap.c is a parent device of ehci-hcd which means ehci-omap's
runtime_* callbacks only be called after all its children are done with
their runtime_* operations, then we could:
struct dev_pm_ops ehci_omap_pm_ops = {
.runtime_resume = ehci_omap_restore_context_and_restart,
.runtime_suspend = ehci_omap_save_context_and_stop,
};
> Consider xHCI as an example. To what extent does it make sense to
> suspend/resume the low/full/high-speed bus independently of the
> SuperSpeed bus? I don't know enough about xHCI to say.
Me neither, but I guess it shouldn't be a problem to keep both buses
suspended and enable them whenever we see a new device. After we know
the kind of device, we switch off the unused bus. I mean, if we're
attached directly to a superspeed device, we can suspend the
low/full/high-speed side, if we're connected to a low/full/high-speed
device, we can suspend the superspeed side and if we're connected to a
hub, we keep both sides resumed.
> > There are other benefits which I could describe but, again as I said
> > before, those can be seen from MUSB (a bit) and DWC3 drivers.
> >
> > On top of all that, you add the fact the HW _does__have_ two entities in
> > order to build a functional EHCI controller (one entity will pack EHCI
> > rules and the other entity will pack integration context - be it PCI or
> > anything else). In short we _do_ have two "devices" which we could be
> > modelling in a better way, IMHO.
> >
> > > Ideally this could be done in a way that allows relatively easy
> > > conversion of the existing [eoux]hci drivers while also allowing all
> > > the other special-purpose host controller drivers to continue as they
> > > are with only minimal changes.
> >
> > there should be no changes needed on the non-standard hosts.
>
> Why not? They have integration contexts just as much as EHCI does.
but that's already in place, right ? See musb for instance (not the best
example, but let's try it out). We have musb_core.c, musb_gadget.c,
musb_gadget_ep0.c, musb_host.c, musb_virthub.c and musb_debugfs.c which
makes up the core IP driver. Then we have omap2430.c, da8xx.c, davinci.c
am35x.c, blackfin.c and so on which makes up the integration context
part.
> Anyway, the changes I'm talking about are minimal. Just how minimal
> it's hard to say without doing some serious thinking about the proposed
> new software design, but I bet they could be made pretty small.
Ok, I would need to see them. But as of now, I think it's unnecessary
:-p
--
balbi
Attachment:
signature.asc
Description: Digital signature
