sd8686 driver: cleanup power management code

[Tony Espy]

If the CONFIG_ENABLE_PM flag in the Makefile for the Marvell sd8686 driver is set to "y", the driver doesn't compile ( see first attachment ).

Closer examination of the patch ( 0019-marvell_8686_8688.patch ) reveals that the sd8688 driver also doesn't seem to properly implement power-management. The file wlan_sdio.c defines wlan_suspend() and wlan_resume(), however the functions don't do anything, and are commented out in the sdio_driver struct.

The sd8686 code attempts to register wlan_pm_suspend()/resume(), which in turn call the lower level sbi_suspend()/resume(), which in turn call sdio_suspend()/resume(). The sd8688 code only defines the wlan_suspend() and resume() methods...

One other troubling piece of code is in the sd8686 driver. If the function wlan_pm_suspend() ( in wlan_main.c ) if the given level is SUSPEND_DISABLE, the code rejects this level if the Adapteris not already in DeepSleep mode. In the past, working with other Wi-Fi drivers, this seems odd to me. I would think that one of the responsibilities of this function would be to put the Adapter ( ie. firmware ) into deepsleep mode.

[Feng Tang (feng-tang)]

This is a long story :)

For PM issue, you may need read the original code from Marvell first.

Marvell's code is against their own reference platform. Their SDIO device first connects to a SDIO-To-CardBus converter which connects to the host machine, so that the code will contains a folder which has their own SDIO stack for the converter/controller.

Their own controller driver provides some API with which the 8686/8688 driver can use to control the controller's clock/power. In short, their SDIO device need control the host controller to make the PM work, which is not acceptable to SD/SDIO kernel maintainer Pierre Ossman. I have discussed this with Marvell engineers, and there is no solution so far. So when I port Marvell's driver to mainstream SDIO stack, I just change the SDIO interface layer, skip the PM part, and let SD host controller driver dealing with PM stuff, like turnning on/off device during system's suspend/resume power cycle.

If you have specail PM request, you may need directly ask for Marvell's support, as I don't have their detailed internal PM document.

[Tony Espy (awe)]

Since Intel is providing these drivers as a basis for MID customers, power-management needs to be a priority.

If suspend() / resume() doesn't work, then the only choice is to unload the driver before suspend, and then reload it on wakeup. If the device is connected to the 'net via Wi-Fi before being suspended,
re-connection to the 'net on resume takes much longer if the driver has to be re-inserted on wakeup. Not an ideal solution for a device that's supposed to be always online.

I'm willing to help out if need be...

Perhaps we need a 3-way conversation setup with Marvell?

[Feng Tang (feng-tang)]

Actually, we don't need to unload/reload the driver during suspend cycle, while just keep the driver there.

When system wakeup, SD/SDIO stack will re-detect the SDIO device, and Wi-Fi driver probe function will be called again to init it. So in short, after system wakeup, the SDIO Wi-Fi is up and running. Network-manager should take care of the reconnection to previous AP.

[Feng Tang (feng-tang)]

Actually, we don't need to unload/reload the driver during suspend cycle, while just keep the driver there.

When system wakeup, SD/SDIO stack will re-detect the SDIO device, and Wi-Fi driver probe function will be called again to init it. So in short, after system wakeup, the SDIO Wi-Fi is up and running (Moblin image work this way). Network-manager should take care of the reconnection to previous AP.

[alek du (alek-du)]

This is still under discussing, will set right milestone and importance later.

[Pat McGowan (pat-mcgowan)]

We need this resolved by 4/25, Tony is tracking it

[Feng Tang (feng-tang)]

As I said before, Marvell reference code is against their own HW setup, and it uses a different SDIO stack than current mainline SDIO stack (maintained by Pierre Ossman)on moblin.

Marvell's original code has some PM APIs, which work only with their own SDIO stack, and these are the only PM hooks with Linux OS (it register one platform_device dedicated for PM stuff). But after the code be ported to against Pirre Ossman's mainline stack, SDIO stack will cover the suspend/resume part, and for run time power management, current driver provide some "iwpriv" command like "iwpriv wlan0 deepsleep X".

So user can ignore these old PM code under ENABLE macro.

[Tony Espy (awe)]

After taking some to checkout the SDIO code code, I see what you're referring to as far as lack of support for suspend/resume. Seems like a pretty big hole to me.

So I'm wondering, does this mean we need to remove the driver prior to suspend and then re-insert it on resume? Otherwise, I don't see how the driver will properly reset itself ( ie. association state, scan tables, RX buffers, etc... ).

Also, since turning on CONFIG_ENABLE_PM causes the driver to fail to build, don't you think we should remove this code?

[Feng Tang (feng-tang)]

In previous discussion, I have said

"Actually, we don't need to unload/reload the driver during suspend cycle, while just keep the driver there.

When system wakeup, SD/SDIO stack will re-detect the SDIO device, and Wi-Fi driver probe function will be called again to init it. So in short, after system wakeup, the SDIO Wi-Fi is up and running (Moblin image work this way). Network-manager should take care of the reconnection to previous AP.
"

There is NO need to remove and reload the driver before suspend and after resume. After system resume, the SD host controller is powered on and will detect the 8686 and init it as a new SDIO device, then in turn will call 8686 driver's probe function and load the firmware, till now 8686 is fully functional.

Red Flag is also using the 8686 code base from Moblin, and I don't see they remove/reload 8686 driver in suspend/resume cycle.

[Jing Wang (jing-j-wang)]

Feng, when will this bug be fixed, you think?

[Tony Espy (awe)]

I finally understand what's going on thank's to Feng's explanation, plus a bit of poking around the SDIO source in the kernel.

The SDIO subsystem basically simulates device removal in prep for the system going into S3. When the system resumes, the SDIO subsystem does the reverse and re-probes the driver.

I do think that the ifdef'd code should get removed from the driver source, but that's a lower priority bug.

I'm going to change the priority to "Low" and change the bug description.

[Feng Tang (feng-tang)]

PM code cleanup for 8686 is done, update has been integrated to molin.org tree.

[Pau Oliva (poliva)]

If it can be of any help, I have an HTC Shift which uses sd8686 and to properly suspend / resume, I need to unload/reload the driver.

See here how I had to implement it using pm-utils hooks:

http://pof.eslack.org/blog/2008/04/21/suspend-resume-in-htc-shift/

[Feng Tang (feng-tang)]

Oliva, thanks for the info.

But I really don't see the case that system need rmmod/insmod 8686 driver during S3/S4 cycle for Moblin kernel

[Tony Espy (awe)]

Per Feng's comment, marking this as "Fix Committed".