On Wednesday 08 June 2005 5:02 pm, Todd Poynor wrote:
> Suspend/resume for the Intel XScale PXA27x OHCI host controller, from
> Nicolas Pitre and myself, based on the TI OMAP OHCI driver code.
Too bad all those pm_message_t changes broke that, then. :(
It's no longer possible to have driver suspend() methods do
anything intelligent based on the target suspend level,
including basics like leaving clocks active during STR level
suspend but turning them off in deeper suspend states; or
kicking in the appropriate wakeup triggering mechanism.
I've been meaning to rip out all that now-broken code, but
haven't made time to figure out something sensible to do
instead. I suspect the only workable answer for now is to
give up and assume that "suspend" means "power down" ... the
only way those pm_message_t changes really make sense seems
to be to assume you're running a laptop, and want to snapshot
to disk then power the whole thing off. With no possibility
of supporting wakeup events other than a power switch, of
course.
> This platform power cycles the device on certain system suspend modes,
> all of which currently arrive as state 3, so the code to react to host
> controller power cycle is performed for a resume from any state. I know
> that sort of thing is being hashed out on another list...
It is? I'd almost given up on system PM transitions or pm_message_t
ever making sense, actually. Maybe I should check out some of the
recent postings on the linux-pm list; maybe some will make sense for
systems where there's no persistent swapspace for "swsusp" to manage,
or where "suspend" is really a suspend state, with wakeup events.
(Ben had some good ideas...)
> If CONFIG_USB_SUSPEND=y then "already suspended/resumed" debug messages
> are generated, but that's true for OMAP as well, so I figured this is a
> work in progress.
>
> If this should go to the ARM patch system instead please clue me in,
> thanks.
No, USB patches can come through the USB list; I'm reasonably sure
Russell wants to avoid worrying about anything except ARM-core patches
and a few pet driver subsystems.
Comments below. I think the main point I'd make now is that the
pm_message_t parameter effectively became useless and so it'd
probably be best to ignore it until there's some new work to
un-break the PM framework ... just always turn off/reset the
controller during suspend. Yes that reduces functionality,
but that seemed to be a goal of pm_message_t: remove things
that don't fit the snapshot-laptop-and-power-off model.
- Dave
> Signed-off-by: Todd Poynor <[EMAIL PROTECTED]>
>
> Index: linux-2.6.12-rc4/drivers/usb/host/ohci-pxa27x.c
> ===================================================================
> --- linux-2.6.12-rc4.orig/drivers/usb/host/ohci-pxa27x.c 2005-06-08
> 22:42:48.000000000 +0000
> +++ linux-2.6.12-rc4/drivers/usb/host/ohci-pxa27x.c 2005-06-08
> 22:51:14.000000000 +0000
> @@ -310,6 +310,7 @@
> .hub_suspend = ohci_hub_suspend,
> .hub_resume = ohci_hub_resume,
> #endif
> + .start_port_reset = ohci_start_port_reset,
> };
>
> /*-------------------------------------------------------------------------*/
> @@ -337,32 +338,83 @@
> return 0;
> }
>
> +
> +#ifdef CONFIG_PM
> +
> static int ohci_hcd_pxa27x_drv_suspend(struct device *dev, pm_message_t
> state, u32 level)
I prefer to call these "message" not "state", since they've
stopped being even vaguely meaningful as states. Originally
the "state" made sense as a target ACPI state number, so that
for example S4 would powerdown and S3 wouldn't necessarily.
Specifially, PM_SUSPEND_DISK (4) and PM_SUSPEND_MEM (3) would
get passed. I originally thought the pm_message_t stuff was
going to formalize that stuff rather than come up with some
new thing and removing the only truly useful parameter data.
> {
> -// struct platform_device *pdev = to_platform_device(dev);
> -// struct usb_hcd *hcd = dev_get_drvdata(dev);
> - printk("%s: not implemented yet\n", __FUNCTION__);
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ohci_hcd *ohci = hcd_to_ohci(dev_get_drvdata(dev));
> + int status = -EINVAL;
>
> - return 0;
> + if (level != SUSPEND_POWER_DOWN)
> + return 0;
> + if (state <= dev->power.power_state)
> + return 0;
Doesn't "sparse" choke on stuff like this lately? Regardless,
the "power state" is no longer directly comprehensible in terms
of the suspend parameter, which is a "message". It's probably
not even worth looking at that parameter; there's only one
value it can legally have in a suspend() method.
> + dev_dbg(dev, "suspend to %d\n", state);
> + down(&ohci_to_hcd(ohci)->self.root_hub->serialize);
> + status = ohci_hub_suspend(ohci_to_hcd(ohci));
> + if (status == 0) {
> + if (state >= 4) {
> + pxa27x_stop_hc(pdev);
> + ohci_to_hcd(ohci)->self.root_hub->state =
> + USB_STATE_SUSPENDED;
> + state = 4;
And this is the class of functionality that's been broken by
those pm_message_t changes. PM_SUSPEND_DISK == 4, which worked
with this; and PM_SUSPEND_MEM == 3 with the other state.
But today, "pm_message_t" is just a fancy and useless boolean.
I've not had time to come up with a better idea than just
going back to the way things were before pm_message_t changes
started to roll in, but meanwhile I think the only solution
allowed by current pm_message_t stuff is to reset and power
down the HC in suspend() methods. That means no wakeup event
support here, no matter what kind of state the system is in.
(Though at least for OMAP 1710 ES2.0 it's clear how to do
OHCI wakeup events even from deep sleep. I don't know what
the PXA 27x does in such cases.)
> + }
> + ohci_to_hcd(ohci)->state = HC_STATE_SUSPENDED;
> + dev->power.power_state = state;
> + }
> + up(&ohci_to_hcd(ohci)->self.root_hub->serialize);
> + return status;
> }
>
> static int ohci_hcd_pxa27x_drv_resume(struct device *dev, u32 level)
> {
> -// struct platform_device *pdev = to_platform_device(dev);
> -// struct usb_hcd *hcd = dev_get_drvdata(dev);
> - printk("%s: not implemented yet\n", __FUNCTION__);
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ohci_hcd *ohci = hcd_to_ohci(dev_get_drvdata(dev));
> + int status = 0;
>
> - return 0;
> + if (level != RESUME_POWER_ON)
> + return 0;
> +
> + switch (dev->power.power_state) {
> + case 0:
> + break;
> + case 4:
Again, state 4 (PM_SUSPEND_DISK) isn't worth testing for any
longer, given the pm_message_t changes that broke things.
> + if (time_before(jiffies, ohci->next_statechange))
> + msleep(5);
> + ohci->next_statechange = jiffies;
> + /* FALLTHROUGH */
> + default:
> + dev_dbg(dev, "resume from %d\n", dev->power.power_state);
> + pxa27x_start_hc(pdev);
> +#ifdef CONFIG_USB_SUSPEND
> + /* get extra cleanup even if remote wakeup isn't in use */
> + status = usb_resume_device(ohci_to_hcd(ohci)->self.root_hub);
This stuff's messy, but last I looked it was still needed.
> +#else
> + down(&ohci_to_hcd(ohci)->self.root_hub->serialize);
> + status = ohci_hub_resume(ohci_to_hcd(ohci));
> + up(&ohci_to_hcd(ohci)->self.root_hub->serialize);
> +#endif
> + if (status == 0)
> + dev->power.power_state = 0;
> + break;
> + }
> + return status;
> }
>
> +#endif
>
> static struct device_driver ohci_hcd_pxa27x_driver = {
> .name = "pxa27x-ohci",
> .bus = &platform_bus_type,
> .probe = ohci_hcd_pxa27x_drv_probe,
> .remove = ohci_hcd_pxa27x_drv_remove,
> +#ifdef CONFIG_PM
> .suspend = ohci_hcd_pxa27x_drv_suspend,
> - .resume = ohci_hcd_pxa27x_drv_resume,
> + .resume = ohci_hcd_pxa27x_drv_resume,
> +#endif
> };
>
> static int __init ohci_hcd_pxa27x_init (void)
>
>
>
-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games. How far can you shotput
a projector? How fast can you ride your desk chair down the office luge track?
If you want to score the big prize, get to know the little guy.
Play to win an NEC 61" plasma display: http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
