Hi,
On Wed, Oct 05, 2011 at 10:44:58AM -0400, Alan Stern wrote:
> On Tue, 4 Oct 2011, Felipe Balbi wrote:
>
> > > This is all part of the same project. We want to merge as much of the
> > > separate code as possible for all the different EHCI drivers. That
> > > means we have to make them all as similar as possible. Which means we
> > > have to look into why there are few odd things sticking out, like clock
> > > pointers or PHY resources. Especially when it's questionable whether
> > > these things should be handled in the EHCI layer in the first place.
> >
> > another argument to split EHCI to its own platform_driver: by doing so,
> > the platform-specific details, such as clock, PHYs, PM, etc will be
> > phased out to a parent driver
>
> If you want to split some of the existing EHCI platform drivers in two,
> that's okay with me (although it seems inefficient). In most of the
> cases, though, it doesn't appear to be needed.
IMO the benefits outweigh the drawbacks and I have already put them down
in emails multiple times.
> > and (at least for PM) would all be handled
> > automagically by pm_runtime ;-)
>
> It already is.
>
> > > > > What extra data is it reasonable to expect a driver to want? Whatever
> > > > > that is, we can add it directly into the usb_hcd structure.
> > > >
> > > > I don't think we can come up with a truly generic solution for all hcs.
> > > > For all EHCIs, maybe, but not for all HCs.
> > >
> > > That's okay, then let's start with EHCI controllers. When that's
> > > finished we can move on to do the same with OHCI.
> >
> > sounds good then.
>
> But you still haven't answered my question. What extra fields should
> be added to the ehci_hcd structure to accomodate all the various
> platform drivers?
none, I guess. I think we should be removing fields, not adding more.
Maybe we just need an easy way to pass quirks to ehci-hcd.c
> The real problem here is that the programming model we're evolving is
> not a good match to the original model for the HCD layer. The original
> idea was that each HC driver has its own hc_driver structure, which
> specifies what that driver needs. Now we're trying to change things so
> that a bunch of different EHCI drivers all share a large amount of
> common data, including the hc_driver structure, but still have
> differing detailed requirements.
>
> I'm beginning to think the best answer might be to have each EHCI
> platform driver make a copy of the generic ehci_hc_driver structure and
> then overwrite some of the fields (such as hcd_priv_size) with its
> own specific values.
I think that's trully unnecessary. I just went through all ehci-<arch>.c
files trying to see what was different. Here's what I found out:
ehci-octeon.c, ehci-omap.c, ehci-s5p.c, ehci-vt8500.c, and
ehci-w90x900.c don't define anything of their own. They simply reuse
what ehci core provides. Those are the simple ones.
ehci-tegra.c has many proprietary fields: reset, shutdown,
map_urb_for_dma, unmap_urb_for_dma, hub_control, bus_suspend and
bus_resume.
I guess many of those are workaround for Silicon issues. Could be
handled as a quirk passed to ehci core.
tegra's ->reset field does:
ehci_halt()
ehci_init()
ehci_port_power()
ehci-xilinx.c has different reset and port_handed_over.
port_handed_over is actually abused on this driver just to issue a bunch
of warnings:
72 static int ehci_xilinx_port_handed_over(struct usb_hcd *hcd, int portnum)
73 {
74 dev_warn(hcd->self.controller, "port %d cannot be enabled\n", portnum);
75 if (hcd->has_tt) {
76 dev_warn(hcd->self.controller,
77 "Maybe you have connected a low speed device?\n");
78
79 dev_warn(hcd->self.controller,
80 "We do not support low speed devices\n");
81 } else {
82 dev_warn(hcd->self.controller,
83 "Maybe your device is not a high speed device?\n");
84 dev_warn(hcd->self.controller,
85 "The USB host controller does not support full speed "
86 "nor low speed devices\n");
87 dev_warn(hcd->self.controller,
88 "You can reconfigure the host controller to have "
89 "full speed support\n");
90 }
91
92 return 0;
93 }
apparently, that device can't even support port handoff feature, so it
shouldn't allow that to happen.
on its reset field what it does is:
ehci_halt();
ehci_init();
ehci_reset();
This seems to be a workaround for some Silicon issue because I have seen
a similar ->reset() on other drivers (see below).
ehci-ath97.c, ehci-atmel.c, ehci-au1xxx.c, ehci-cns3xxx.c, ehci-fsl.c,
ehci-grlib.c, ehci-ixp4xx.c, ehci-msm.c, ehci-mxc.c, ehci-orion.c,
ehci-ppc-of.c, ehci-ps3.c, and ehci-spear.c only have a different
->reset() callback.
Looking at ->reset field of those guys, we have:
ath97 (it's a synopsys core, OMAP also has a synopsys EHCI core):
ehci_reset()
ehci_init()
ehci_port_power()
atmel
ehci_halt()
ehci_init()
ehci_reset()
ehci_port_power()
au1xxx
ehci_init()
need_io_watchdog = 0 <- quirk ??
cns3xxx
handle a shared clock <- pm_runtime could help ??
ehci_reset()
ehci_init()
ehci_port_power()
fsl
ehci_halt()
ehci_init()
ehci_reset()
platform-specific handling
ehci_port_power()
grlib
ehci_halt()
ehci_init()
ehci_power_power()
ehci_reset()
ixp4xx
big_endian_desc = 1 <- convert to bitshifts ?
big_endian_mmio = 1 <- convert to bitshifts ?
ehci_reset()
ehci_init()
ehci_port_power()
msm
ehci_setup()
platform-specific handling
ehci_port_power()
mxc
ehci_halt()
ehci_init()
ehci_reset()
ehci_port_power()
orion
ehci_halt()
ehci_init()
ehci_reset()
ehci_port_power()
ppc_of
ehci_halt()
ehci_init()
ehci_reset()
ps3
big_endian_mmio = 1 <- convert to bitshifts ?
ehci_halt()
ehci_init()
ehci_reset()
spear
ehci_halt()
ehci_init()
ehci_reset()
ehci_port_power()
As you can see, many of these ->reset() are very similar and I believe
there is a reason for that. Most likely, those companies are sourcing
IPs from another company and they end up with the same quirky controller
on their platform. If that's really the case, the platforms which use
the same EHCI IP, should share the same ehci-$IP.c source code, at
least. Ideally, ehci core would allow us to pass quirks for those quirky
reset devices and it would all be handled by the ehci core, no need for
a ehci-$IP.c file in the long run.
That's why I have been saying that we should split ehci core to its own
platform driver/device. Then it's easier to remove the platform-specific
details to a parent driver and treat those later, after EHCI core is in
a better shape.
Much of that code is pure duplication, that's all. Even though they are
running on different SoCs, they might as well be the exact same IP.
What I'm trying to show here is that those different fields, are only
different because a particular IP is quirky, not because it's really
different.
See tegra's map/unmap urb callbacks, it's just allocating and freeing a
temporary bounce buffer. Tegra's shutdown is only ensuring a particular
clock is ON before shutdown, tegra's bus_suspend/bus_resume is touching
the PHY (probably trying to save more power) and so on...
--
balbi
Attachment:
signature.asc
Description: Digital signature
