On Wed, 3 Oct 2012, Felipe Balbi wrote:
> > There will be a few cases where the platform code needs to call a core
> > routine. So we'd still need to export a few routines, but not nearly
> > as many as I did here.
>
> I think there shouldn't be.. well, unless we need to apply some very
> specific workaround like OMAP's port suspend workaround where we need to
> switch a clock parent. In that case we need to add code to hub_control()
> and the best way is to do our workaround thing, then call the generic
> hub_control()
Specifically, I was thinking of ehci_suspend, ehci_resume, ehci_setup,
and ehci_port_power.
Adding another layer to the device hierarchy might allow us to avoid
exporting ehci_suspend and ehci_resume, but doing that would be a
major change visible to userspace and I'm not convinced it would be
for the best. (Also, there is the special call to ehci_pci_reinit in
ehci_pci_resume _after_ ehci_resume runs -- that would be difficult to
handle. Other platform drivers may have similar requirements.)
Some drivers need to change a few values in the ehci_hcd structure
after ehci_setup runs. Exporting it seems to be the easiest way to
accomplish this.
The port power stuff almost certainly isn't needed. The EHCI core
driver and usbcore are responsible for managing port power anyway; the
platform drivers shouldn't be concerned with it. I'm planning on
removing it from the platform drivers at some point -- but for now
they use it.
> > > > +#ifdef CONFIG_PCI
> > > > +
> > > > +/* For working around the MosChip frame-index-register bug */
> > > > +static unsigned ehci_read_frame_index(struct ehci_hcd *ehci);
> > > > +
> > > > +#else
> > > > +
> > > > +static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
> > > > +{
> > > > + return ehci_readl(ehci, &ehci->regs->frame_index);
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > See this is one example. What if two other different plaforms need to
> > > work around silicon bugs on different cases ? Will we start sprinkling
> > > ifdefs in this driver again ?
> >
> > This isn't as significant as it appears. The "#ifdef CONFIG_PCI" part
> > is just a minor optimization, because this particular quirk affects
> > only PCI controllers. We could get rid of the inline routine entirely
> > and use the out-of-line ehci_read_frame_index() routine on all
> > platforms.
>
> that'll prevent certain compiler optimizations to happen, no ? I mean,
> do you think GCC would still inline the cases where it's really just a
> register read (which is just one instruction on ARM) ?
No; you're right.
> > > I would much rather have something like:
> > >
> > > static const struct ehci_platform_data moschip_pci_platform_data
> > > __devinitconst = {
> > > .frame_index = moschip_pci_frame_index,
> > > [ ... ]
> > > };
> > >
> > > static int ehci_pci_probe(struct pci_device *pci)
> > > {
> > > struct platform_device *ehci;
> > >
> > > ehci = platform_device_alloc(....);
> > > [ ... ]
> > >
> > > /* check if MosChip */
> > > ret = platform_device_add_data(ehci, &moschip_pci_platform_data,
> > > sizeof(struct ehci_platform_data));
> > >
> > > [ ... ]
> > >
> > > return 0;
> > > }
> > >
> > > Then on ehci-core, instead of adding ifdefs all over, you could:
> > >
> > > static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
> > > {
> > > if (unlikely(ehci->ops->frame_index))
> > > return ehci->ops->frame_index(ehci_to_dev(ehci));
> > >
> > > return ehci_readl(ehci, &ehci->regs->frame_index);
> > > }
That also is more than a single instruction, because it involves a
test. But the scheme could be made to work easily enough for all
platforms, although I would do it slightly differently:
static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
{
if (unlikely(ehci->frame_index_bug))
return ehci_moschip_read_frame_index(ehci);
return ehci_readl(ehci, &ehci->regs->frame_index);
}
This, together with moving the ehci_info() etc. macros from ehci-dbg.c
to ehci.h are simple cleanups that can be done before the real
conversion.
Another such cleanup involves the Link Power Management code. I don't
know why ehci-lpm.c is always included in the build even though it
gets used only by ehci-pci.c and ehci-vt8500.c. On the other hand, it
doesn't seem to be at all system specific; LPM is part of the official
spec. The ehci_update_device routine in ehci-pci.c, for example,
would work on any system (although most systems don't implement LPM
and so would set ehci->has_lpm to 0). Done properly, we wouldn't need
to export any of the routines in ehci-lpm.c.
> > > I would rather see ehci-pci.c passing a "ops" structure with a set of
> > > function pointers to the core layer.
> >
> > As pointed out above, ehci_pci_hc_driver essentially _is_ such an "ops"
> > structure. I can think of a few ways to initialize these structures.
>
> not really, because that's something ehci core should be passing to
> usbcore, that's the language they talk. If we need to change the
> implementation of what of EHCI specification says, we need to do it
> between ehci core and ehci-$platform.
The parts of the hc_driver structure that are relevant to usbcore and
the EHCI spec are the same among all the platform drivers (with maybe
one or two notable exceptions). The parts that need to get overridden
are mostly like .product_desc and .reset.
> > One is like the code above. Another is the way the existing code does
> > it (which would require all those exports you hate).
>
> I "hate" those for a reason. My experience with the MUSB driver taught
> me that the more core functionality we expose, the more it will be
> abused by users. It also taught me a different way of doing things
> considering how the HW is actually layed out (a 'generic' IP with a
> 'wrapper' IP around it, so two HW entities), which turned out to give
> a lot of functionality for free (PM encapsulation, address space
> 'isolation'[1], easier to read code with proper split between 'generic'
> and company-specific details, etc).
>
> Look at drivers/usb/dwc3/ and you will clearly see what I mean. What
> core functionality is built into dwc3.ko and dwc3-$plat.ko is just a
> shim layer which prepares the 'wrapper' to make dwc3 work.
>
> [1] by isolation I mean that e.g. EHCI's registers shouldn't be accessed
> by ehci-omap.c (ideally)
I think almost all of these advantages should be realizable with the
library approach.
> > gleaned from a quick scan through the driver source files:
> >
> > Every driver has its own .product_desc string.
> >
> > ehci-fsl and ehci-spear override hcd_priv_size so that they
> > can add their own data alongside the ehci_hcd structure.
> > We should make this easy to do because some other drivers
> > could stand to use the same technique.
>
> this could also be done differently. If EHCI gets its own device, that
> won't be necessary and ehci-spear/fsl will be able to
> platform_set_drvdata(pdev, spear/fsl), which will give them a way to
> manage their own data without polluting EHCI's internal data.
True. But we already do this same sort of thing at a different level
-- consider how ehci_hcd is added on at the end of usb_hcd. hcd.c
acts as a library, with routines like usb_create_hcd that are called
by the hardware-specific drivers.
> > Most drivers override the .reset pointer. I hope that in
> > time this will become less common, but for now we have to
> > live with it.
>
> I looked into those already, most of them have one form of:
>
> ehci->caps = hcd->regs + offset; /* offset can be zero */
> hcd->has_tt = true;
> ehci_setup();
> ehci_port_power();
>
> a few of them will have some extra bits and pieces here and there. OMAP,
> for instance does a PHY reset, which shouldn't be done there anyway, it
> should be done by a PHY driver (drivers/usb/phy/) and ehci core should
> just call usb_phy_reset(); or something similar.
Right; it's those extra bits and pieces that make life interesting.
The ehci-platform driver already encapsulates some of them into a
single data structure. But I don't think we can eliminate completely
the need for a specialized .reset routine. At least, not yet.
> > ehci-pmcmsp overrides .irq. I'm not sure that this is
> > really necessary; it doesn't look like it.
>
> you're right here. As long as ehci properly checks the STS register, we
> should be fine, right ?
Yes, we should. After all, plenty of other drivers use ahared IRQ
lines with no trouble; why shouldn't ehci-pmcmsp?
> > This suggests each platform driver should define a static table with
> > entries for product_desc, hcd_priv_size, reset, pci_suspend,
> > pci_resume, and update_device. The EHCI core could then take this
> > static table and merge it with the generic ehci_hc_driver to create a
> > platform-specific hc_driver. Very little would need to be exported.
>
> It looks to me that update_device could be made generic. The underlying
> platform doesn't need to know about a new device being addressed, ehci
> core does. Am I missing something ?
That's right; see above.
> > Or if you prefer, I could leave pci_suspend and pci_resume out of the
> > table. ehci-pci could add them to the hc_driver just like in the code
> > above.
>
> Or maybe just convert pci_resume/pci_suspend into generic resume/suspend
> and let pci pass its own implementation there ?
I really should explain why they exist and how they work...
It's mostly historical. The PCI-specific common parts of the UHCI,
OHCI, and EHCI drivers were split out into hcd-pci.c. This was before
I started working on usbcore; apparently most of the work was done by
David Brownell about 10-12 years ago. hcd-pci.c was split out in
2.5.21; hcd.c was added in 2.5.2 -- and PCI PM support was already
present.
hcd-pci.c includes the PCI-specific suspend and resume routines. But
of course, there has to be controller-specific code as well. I don't
know the reason, but someone decided to implement it by having the core
functions invoke callback routines in the controller drivers. Maybe
this seemed logical at the time; there weren't any non-PCI host
controller drivers back then. At any rate, the .pci_suspend and
.pci_resume members of hc_driver are those callback pointers.
The alternative, of course, is for each controller driver to have its
own suspend/resume routines that would call the functions in hcd-pci.c.
Implementing this now would be awkward because it would mean setting up
a separate dev_pm_ops table for each driver (instead of sharing the
single table in hcd-pci.c) as well as duplicating all the code
involved in handling suspend vs. hibernation vs. runtime suspend.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
