I'll try to keep this making sense, but I'm going to have to reply to things slightly out of order.
On Thu, Aug 09, 2007 at 11:27:02AM -0400, Alan Stern wrote:
> On Thu, 9 Aug 2007, Zephaniah E. Hull wrote:
>
> > OHCI isn't coming back on the OLPC after resume.
> >
> > After a bit of testing, the problem seems to have come down to two points.
> >
> > The first is that ohci_pci_resume is not forcing the root hub to be resumed
> > properly, that's a fairly trivial and obviously correct patch.
>
> Can you be more specific? What exactly goes wrong?
The first problem is pretty noticeable, ohci_rh_resume never gets called.
The chain roughly goes usb_resume_both -> usb_resume_device ->
generic_resume (some indirection here) -> hcd_bus_resume ->
ohci_rh_resume (some indirection here too).
usb_resume_device has an interesting check, if udev->state !=
USB_STATE_SUSPENED or if udev->reset_resume is false, then it silently
decides not to pass things down the chain, without a failure. (Note that
udev is usb_device, the joy of confusing naming.)
This is quite possibly a bug, since a few lines down in the same
function it checks for a quirk, USB_QUIRK_RESET_RESUME, and sets
reset_resume to 1 if it's there. This code has no impact, since it's
never reached if reset_resume isn't already 1.
There are only two other places that can set a usb_device struct's
reset_resume field to 1.
mark_children_for_reset_resume, which takes a hub argument and sets all
the children, but I'm fairly sure (not completely, but fairly) that this
won't ever do a root hub.
And then there is usb_root_hub_lost_power, which sets the device's
reset_resume to 1 and also sets 'rhdev->dev.power.prev_state.event =
PM_EVENT_ON'.
EHCI calls usb_root_hub_lost_power from the pci's resume function, very
much as I modified OHCI to do in the patch below, though IIRC it jumped
through some hoops to try and check if the power was actually cut during
suspend or not.
Setting hcd->state is just further assurance that ohci_rh_resume even
gets called.
>
> Note that ohci_pci_resume() isn't _supposed_ to force the root hub to
> be resumed. ohci_rh_resume() runs at a later time, after
> ohci_pci_resume().
>
> It would be appropriate to detect loss of VBUS power in
> ohci_pci_resume() and set the controller's state accordingly, as the
> comment in that routine indicates. But I don't know the best way of
> doing so.
>
> > The second is trickier, ohci_rh_resume is getting called in a state that it
> > thinks is an indication that it's coming back from a SUSPEND where it did
> > not
> > lose power.
>
> You mean ohci->regs->control doesn't contain OHCI_USB_RESET in the
> appropriate bits? What does it contain? And why?
ohci->hc_control & OHCI_CTRL_HCFS == OHCI_USB_SUSPEND, and I honestly
don't have the foggiest clue how or why it has that after coming back
from the chip being powered off.
My best guess is that somewhere else in the resume path we're resetting
it, but that's only a guess and I have no idea why anything would do
that.
Infact, grepping through the source tree, the only thing setting
OHCI_USB_SUSPEND is ohci_rh_suspend, which would have happened prior to
turning off the device.
My guess is that something is blindly restoring from ohci->hc_control
without first reading in the value.
>
> > I've patched it, and hopefully there won't be any false positives, but I
> > don't
> > have another machine to do suspend/resume testing on, so while it works for
> > me,
> > I can't guarantee that it won't cause problems for others.
> >
> > In any case, USB 1.1 devices directly plugged into the machine didn't work
> > after resume before this, and do now.
> >
> > Signed-off-by: Zephaniah E. Hull <[EMAIL PROTECTED]>
> >
> > diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
> > index bb9cc59..3660dcc 100644
> > --- a/drivers/usb/host/ohci-hub.c
> > +++ b/drivers/usb/host/ohci-hub.c
> > @@ -165,6 +165,12 @@ static int ohci_rh_resume (struct ohci_hcd *ohci)
> > }
> > } else switch (ohci->hc_control & OHCI_CTRL_HCFS) {
> > case OHCI_USB_SUSPEND:
> > + /* FIXME: we should try to detect loss of VBUS better. */
> > + if (!autostopped) {
> > + ohci_dbg (ohci, "resume root hub -- lost power\n");
> > + status = -EBUSY;
> > + break;
> > + }
>
> This is definitely not the right thing to do. We should be in the
> default (reset) case, not here. The correct approach is to find out
> why we aren't and fix it up.
>
> And this certainly _will_ generate false positives on other machines.
Agreed.
>
> > ohci->hc_control &= ~(OHCI_CTRL_HCFS|OHCI_SCHED_ENABLES);
> > ohci->hc_control |= OHCI_USB_RESUME;
> > ohci_writel (ohci, ohci->hc_control, &ohci->regs->control);
> > diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
> > index 48de318..ae1ecb2 100644
> > --- a/drivers/usb/host/ohci-pci.c
> > +++ b/drivers/usb/host/ohci-pci.c
> > @@ -282,7 +282,9 @@ static int ohci_pci_resume (struct usb_hcd *hcd)
> > prepare_for_handover(hcd);
> >
> > /* Force the PM core to resume the root hub */
> > - hcd_to_bus(hcd)->root_hub->dev.power.prev_state.event = PM_EVENT_ON;
> > + usb_root_hub_lost_power(hcd->self.root_hub);
> > +
> > + hcd->state = HC_STATE_SUSPENDED;
>
> Again, the wrong thing to do. For one thing, there's no reason to call
> usb_root_hub_lost_power() here since it will get called later on in
> ohci_rh_resume(). For another, there's no reason to set hcd->state to
> HC_STATE_SUSPENDED, since usb_hcd_pci_resume() in
> drivers/usb/core/hcd-pci.c has already verified that it has that value.
Agreed on the latter, the state set can be removed.
But the usb_root_hub_lost_power call must happen there, otherwise, as
noted above, ohci_rh_resume can never be called.
--
1024D/E65A7801 Zephaniah E. Hull <[EMAIL PROTECTED]>
92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
CCs of replies from mailing lists are requested.
signature.asc
Description: Digital signature
------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
