Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Tue, Jul 6, 2010 at 8:42 PM, Nicolas Pitre n...@fluxnic.net wrote: Another function pair would be needed instead, which would do almost like the suspend/resume code is already doing. Something like: /* * Indicate to the core SDIO layer that we're not requiring that the * function remain powered. If all functions for the card are in the * same no power state, then the host controller can remove power from * the card. Note: the function driver must preserve hardware states if * necessary. */ int sdio_release_power(struct sdio_func *func); /* * Indicate to the core SDIO layer that we want power back for this * SDIO function. The power may or may not actually have been removed * since last call to sdio_release_power(), so the function driver must * not assume any preserved state at the hardware level and re-perform * all the necessary hardware config. This function returns 0 when * power is actually restored, or some error code if this cannot be * achieved. One error reason might be that the card is no longer * available on the bus (was removed while powered down and card * detection didn't trigger yet). */ int sdio_claim_power(struct sdio_func *func); That's it. When the network interface is down and the hardware is not needed, you call sdio_release_power(). When the request to activate the network interface is received, you call sdio_claim_power() and configure the hardware appropriately. If sdio_claim_power() returns an error, then you just return an error to the network request, and eventually the driver's remove method will be called if this is indeed because the card was removed. After playing with this a little, I am getting convinced that the way to go here is to add runtime pm support to MMC/SDIO. It just fits perfectly into this model: Runtime PM already takes care of usage count per device (so it would tell if an SDIO function is idle), takes into account parent-child relationships (so if all SDIO functions are idle, it would automatically send an idle request to the card device), doesn't break existing drivers (drivers needs to explicitly enable it), play along nicely with the system suspend/resume, etc.. I'm going to split the patches to two: - first half would be basic wl1271 support for omap/zoom. it's pretty straight forward and can probably already go thru. - second half would be adding runtime PM to SDIO/MMC. I'll post some initial patches to set off a discussion. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Ohad, On 07/08/2010 11:10 PM, ext Ohad Ben-Cohen wrote: Hi Nicolas and Roger, On Tue, Jul 6, 2010 at 8:42 PM, Nicolas Pitren...@fluxnic.net wrote: On Tue, 6 Jul 2010, Roger Quadros wrote: If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then the SDIO/MMC core should tackle it, just like it deals with supply for slots with removable cards. ... Another function pair would be needed instead, which would do almost like the suspend/resume code is already doing. Something like: Thanks a lot for your review and comments, and for taking the time to present your approach. you're welcome :) I like it ! It'd allow us to lose the software (or fake if you want ;) card detect mechanism, which is something that should have been added to each platform we wanted to support. We would only need to make it possible to deliver board-specific data to the function driver (e.g., in the case of the wl1271, we need irq and board_ref_clock data). That would require some board-level platform-data configuration, which will be specific to the controller to which the device is hardwired to. This data should propagate through the host controller to the SDIO core so it would eventually be why should platform data go through the host controller? You are already creating a new platform device for wl1271_sdio just to pass platform_data so the platform data for that can be passed directly from board files to this platform device. It doesn't need to go via host controller. I think this case is specific to wl1271. Ideally wl1271 should have used the SDIO's in-band interrupt delivery mechanism and SDIO Configuration space to provide this information to make it truly plug n play. br, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Roger, On Fri, Jul 9, 2010 at 11:12 AM, Roger Quadros roger.quad...@nokia.com wrote: You are already creating a new platform device for wl1271_sdio just to pass platform_data With the new approach, I can remove this platform device altogether, and completely rely on the sdio function device for driver probing (as it was before). I just need to add this little support of delivering small bits of platform data - the mechanism should be simple and generic, and would considerably simplify the driver. I think this case is specific to wl1271. Yes, it is. There's an hw issue with the wl1271's in-band interrupt, so we can't use it (the issue is fixed in the wl1281 btw). But we also have to deliver the reference clock info to the driver, which is board-specific data, and would be relevant to all wl12xx devices. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Fri, Jul 9, 2010 at 11:32 AM, Ohad Ben-Cohen o...@wizery.com wrote: Hi Roger, On Fri, Jul 9, 2010 at 11:12 AM, Roger Quadros roger.quad...@nokia.com wrote: You are already creating a new platform device for wl1271_sdio just to pass platform_data With the new approach, I can remove this platform device altogether, and completely rely on the sdio function device for driver probing (as it was before). I just need to add this little support of delivering small bits of platform data - the mechanism should be simple and generic, and would considerably simplify the driver. I think this case is specific to wl1271. Yes, it is. wl1251 also has this issue. In fact it's even worse there as it's missing all SDIO registers and cannot be probed at all.. Those devices are not plug n play at all. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On 07/07/2010 04:52 PM, ext Nicolas Pitre wrote: On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 08:42 PM, ext Nicolas Pitre wrote: On Tue, 6 Jul 2010, Roger Quadros wrote: OK, this is how I see it. - Treat the non-removable card as non-removable. So no need to do card detect emulation. - Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator framework to define this regulator supply. Even though you mention that it is not actually a supply, it fits well in the fixed supply framework. - When the host controller is enumerated, the mmc core will power up the slot, find the sdio card, and probe the function driver (i.e. wl1271_sdio). - if interface is not in use, the function driver must release the sdio host, and this should eventually disable the vmmc supply. - Whenever the wlan interface must be brought up, wl1271_sdio, can claim the sdio host. this will cause the vmmc supply to be enabled, for as long as the interface is up. Does this address all issues? This is mostly all good, except that claiming/releasing the SDIO host is about access to the bus. It must be claimed right before doing any IO, and released right after that, even when the card is expected to remain powered. This is not the proper place to hook power control. Agreed, but is it so that SDIO power may be removed between a host_release and claim? This appears so from omap_hsmmc host controller. No, it is not because a host is not claimed that power should be dropped. The host claim/release is meant to provide exclusive access to the card that's all. If the OMAP controller is dropping power to the card upon host-disable() then it is wrong. AFAICS only the OMAP controller is playing such games at the moment and I suspect the semantics might not be all right. Shutting down the _controller_ when it is idle might be a good thing, but not power to the _card_. Only the function driver might know when it is fine to lose power. I completely agree with you Nicolas. omap_hsmmc needs to be fixed so that it does not control MMC/SDIO slot supply voltage. It should deal with only the mmc controller power savings and not the card/function power savings. regards, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Nicolas and Roger, On Tue, Jul 6, 2010 at 8:42 PM, Nicolas Pitre n...@fluxnic.net wrote: On Tue, 6 Jul 2010, Roger Quadros wrote: If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then the SDIO/MMC core should tackle it, just like it deals with supply for slots with removable cards. ... Another function pair would be needed instead, which would do almost like the suspend/resume code is already doing. Something like: Thanks a lot for your review and comments, and for taking the time to present your approach. I like it ! It'd allow us to lose the software (or fake if you want ;) card detect mechanism, which is something that should have been added to each platform we wanted to support. We would only need to make it possible to deliver board-specific data to the function driver (e.g., in the case of the wl1271, we need irq and board_ref_clock data). That would require some board-level platform-data configuration, which will be specific to the controller to which the device is hardwired to. This data should propagate through the host controller to the SDIO core so it would eventually be accessible by the function driver (e.g. via func-dev.pdata). We'll adapt and post follow-up patches. Thanks again, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote:
Nicolas Pitre wrote:
On Tue, 6 Jul 2010, Roger Quadros wrote:
On 07/06/2010 03:53 PM, ext Ohad Ben-Cohen wrote:
Hi Roger,
On Tue, Jul 6, 2010 at 1:35 PM, Roger Quadrosroger.quad...@nokia.com
wrote:
My point is that shouldn't this be handled by SDIO core?
Care to explain what you mean / give a code example ?
If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then
the SDIO/MMC core should tackle it, just like it deals with supply for slots
with removable cards.
Exact.
You need card detect events in order to trigger card sdio function
initialization and removals.
Why would you trigger function initialization and removal? Just to turn
off power? That's a bit like pulling off the battery from your laptop
when you want to suspend it. There is a better way to go about things.
Please share any alternative approach you may be thinking on.
OK, this is how I see it.
- Treat the non-removable card as non-removable. So no need to do card detect
emulation.
- Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator
framework to define this regulator supply. Even though you mention that it
is not actually a supply, it fits well in the fixed supply framework.
- When the host controller is enumerated, the mmc core will power up the slot,
find the sdio card, and probe the function driver (i.e. wl1271_sdio).
- if interface is not in use, the function driver must release the sdio host,
and this should eventually disable the vmmc supply.
- Whenever the wlan interface must be brought up, wl1271_sdio, can claim the
sdio host. this will cause the vmmc supply to be enabled, for as long as the
interface is up.
Does this address all issues?
This is mostly all good, except that claiming/releasing the SDIO host is
about access to the bus. It must be claimed right before doing any IO,
and released right after that, even when the card is expected to remain
powered. This is not the proper place to hook power control.
Another function pair would be needed instead, which would do almost
like the suspend/resume code is already doing. Something like:
/*
* Indicate to the core SDIO layer that we're not requiring that the
* function remain powered. If all functions for the card are in the
* same no power state, then the host controller can remove power from
* the card. Note: the function driver must preserve hardware states if
* necessary.
*/
int sdio_release_power(struct sdio_func *func);
/*
* Indicate to the core SDIO layer that we want power back for this
* SDIO function. The power may or may not actually have been removed
* since last call to sdio_release_power(), so the function driver must
* not assume any preserved state at the hardware level and re-perform
* all the necessary hardware config. This function returns 0 when
* power is actually restored, or some error code if this cannot be
* achieved. One error reason might be that the card is no longer
* available on the bus (was removed while powered down and card
* detection didn't trigger yet).
*/
int sdio_claim_power(struct sdio_func *func);
That's it. When the network interface is down and the hardware is not
needed, you call sdio_release_power(). When the request to activate the
network interface is received, you call sdio_claim_power() and configure
the hardware appropriately. If sdio_claim_power() returns an error,
then you just return an error to the network request, and eventually the
driver's remove method will be called if this is indeed because the card
was removed.
In the core SDIO code, this is almost identical to a suspend/resume
request, except that the request comes from the function driver instead
of the core MMC code.
For eMMC in omap_hsmmc, this is all done via claim_host / release_host
which call -enable() / -disable() methods. omap_hsmmc makes use of
mmc_power_restore_host() which calls host-bus_ops-power_restore()
which is not implemented for SDIO, but for MMC and SD it reinitializes
the card.
Shouldn't the power control intelligence (i.e. when to turn power ON/OFF) lie
with the bus drivers?
Set omap2_hsmmc_info mmc[x] {.nonremovable=true, .power_saving=true} and
implement host-bus_ops-power_restore() for SDIO, then the power will
go off 9 seconds after sdio_release_host() is called. Then tweak omap_hsmmc
so that it doesn't wait 9 seconds for the SDIO case
is the wl1271 supposed to be used only with omap_hsmmc? We need to have a
solution that works neatly irrespective of which host controller is being used.
regards,
-roger
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 08:42 PM, ext Nicolas Pitre wrote: On Tue, 6 Jul 2010, Roger Quadros wrote: OK, this is how I see it. - Treat the non-removable card as non-removable. So no need to do card detect emulation. - Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator framework to define this regulator supply. Even though you mention that it is not actually a supply, it fits well in the fixed supply framework. - When the host controller is enumerated, the mmc core will power up the slot, find the sdio card, and probe the function driver (i.e. wl1271_sdio). - if interface is not in use, the function driver must release the sdio host, and this should eventually disable the vmmc supply. - Whenever the wlan interface must be brought up, wl1271_sdio, can claim the sdio host. this will cause the vmmc supply to be enabled, for as long as the interface is up. Does this address all issues? This is mostly all good, except that claiming/releasing the SDIO host is about access to the bus. It must be claimed right before doing any IO, and released right after that, even when the card is expected to remain powered. This is not the proper place to hook power control. Agreed, but is it so that SDIO power may be removed between a host_release and claim? This appears so from omap_hsmmc host controller. No, it is not because a host is not claimed that power should be dropped. The host claim/release is meant to provide exclusive access to the card that's all. If the OMAP controller is dropping power to the card upon host-disable() then it is wrong. AFAICS only the OMAP controller is playing such games at the moment and I suspect the semantics might not be all right. Shutting down the _controller_ when it is idle might be a good thing, but not power to the _card_. Only the function driver might know when it is fine to lose power. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote: For eMMC in omap_hsmmc, this is all done via claim_host / release_host which call -enable() / -disable() methods. omap_hsmmc makes use of mmc_power_restore_host() which calls host-bus_ops-power_restore() which is not implemented for SDIO, but for MMC and SD it reinitializes the card. This is IMHO a really bad design. The power control decision has to come from the top, not from the bottom. And certainly not with a U-turn dependency the omap_hsmmc is using. I regret to say this, but the omap_hsmmc driver is becoming a total mess. The host controller driver has to be a dumb interface serving requests from the hardware used by the upper layer stack, not the place where decisions such as power handling should be made. Think of it like an ethernet driver. No ethernet driver in Linux is telling the IP stack when to shut down. Shouldn't the power control intelligence (i.e. when to turn power ON/OFF) lie with the bus drivers? Absolutely! And in the SDIO case that should lie with each function drivers. Please let's stop this omap_hsmmc madness. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 11/15] wireless: wl1271: introduce platform device support
-Original Message- From: Nicolas Pitre [mailto:n...@fluxnic.net] Sent: Wednesday, July 07, 2010 9:03 AM To: Roger Quadros Cc: Hunter Adrian (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wirel...@vger.kernel.org; linux-...@vger.kernel.org; linux- o...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; li...@arm.linux.org.uk; Chikkature Rajashekar Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); a...@linux-foundation.org; San Mehat Subject: Re: [PATCH 11/15] wireless: wl1271: introduce platform device support On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote: For eMMC in omap_hsmmc, this is all done via claim_host / release_host which call -enable() / -disable() methods. omap_hsmmc makes use of mmc_power_restore_host() which calls host-bus_ops-power_restore() which is not implemented for SDIO, but for MMC and SD it reinitializes the card. This is IMHO a really bad design. The power control decision has to come from the top, not from the bottom. And certainly not with a U-turn dependency the omap_hsmmc is using. I regret to say this, but the omap_hsmmc driver is becoming a total mess. The host controller driver has to be a dumb interface serving requests from the hardware used by the upper layer stack, not the place where decisions such as power handling should be made. Think of it like an ethernet driver. No ethernet driver in Linux is telling the IP stack when to shut down. The point is that MMC/SD core files were patched to provide this kind of a support. Any controller driver can use that framework today, right?. As an example omap_hsmmc driver was patched and it works fine. Why blame the controller driver for using a support provided by core files? Regards, Madhu Shouldn't the power control intelligence (i.e. when to turn power ON/OFF) lie with the bus drivers? Absolutely! And in the SDIO case that should lie with each function drivers. Please let's stop this omap_hsmmc madness. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Wed, 7 Jul 2010, Madhusudhan wrote: -Original Message- From: Nicolas Pitre [mailto:n...@fluxnic.net] Sent: Wednesday, July 07, 2010 9:03 AM To: Roger Quadros Cc: Hunter Adrian (Nokia-MS/Helsinki); Ohad Ben-Cohen; linux- wirel...@vger.kernel.org; linux-...@vger.kernel.org; linux- o...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; li...@arm.linux.org.uk; Chikkature Rajashekar Madhusudhan; Coelho Luciano (Nokia-MS/Helsinki); a...@linux-foundation.org; San Mehat Subject: Re: [PATCH 11/15] wireless: wl1271: introduce platform device support On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote: For eMMC in omap_hsmmc, this is all done via claim_host / release_host which call -enable() / -disable() methods. omap_hsmmc makes use of mmc_power_restore_host() which calls host-bus_ops-power_restore() which is not implemented for SDIO, but for MMC and SD it reinitializes the card. This is IMHO a really bad design. The power control decision has to come from the top, not from the bottom. And certainly not with a U-turn dependency the omap_hsmmc is using. I regret to say this, but the omap_hsmmc driver is becoming a total mess. The host controller driver has to be a dumb interface serving requests from the hardware used by the upper layer stack, not the place where decisions such as power handling should be made. Think of it like an ethernet driver. No ethernet driver in Linux is telling the IP stack when to shut down. The point is that MMC/SD core files were patched to provide this kind of a support. Any controller driver can use that framework today, right?. As an example omap_hsmmc driver was patched and it works fine. It is not because you are twisting the layers and customizing the core stack around your own controller that it is good software design. And the presence of the mmc_power_restore_host() code doesn't mean it is sane. My point is that no one should ever use that, not even omap_hsmmc. Proof: it works so fine that now you must come up with yet another contorted hack piled on top called fake^H^H^H^Hsoftware insert/remove events to support power handling with SDIO cards. This MMC_CAP_DISABLE is ridiculous. Why would this have to be a host capability? This should be a core feature that should be _transparent_ to all hosts with no changes to any of the host drivers. When the core code knows that the card can be shut down after some idle period, it should do so with the *existing* API calls, namely the set_ios method. In the SDIO case it would be a simple matter of mapping the sdio_release_power() onto that. Instead, some contorted power management support was sneaked in, which is misdesigned to the point of breaking proper SDIO support for which more misdesigned features are now needed to work around the former ones. Now the OMAP driver is becoming a stack of its own, making decisions that clearly should be made at a higher level of abstraction. To me this denotes some laziness from the involved developers who didn't take the time to design something sensible and generic in the core code, but rather hacked a quick solution specific to omap_hmmc.c. Of course the former would require a greater understanding of common code and some additional effort to make the solution truly generic. Instead, the easy solution was taken which is to stuff magic behaviors in a host driver while other people are not paying as much attention to it than core code. Needless to say that I'm not impressed at all. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Nicolas Pitre wrote: On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote: For eMMC in omap_hsmmc, this is all done via claim_host / release_host which call -enable() / -disable() methods. omap_hsmmc makes use of mmc_power_restore_host() which calls host-bus_ops-power_restore() which is not implemented for SDIO, but for MMC and SD it reinitializes the card. This is IMHO a really bad design. The power control decision has to come from the top, not from the bottom. And certainly not with a U-turn dependency the omap_hsmmc is using. The power control decision does come from the top via mmc_claim_host / mmc_release_host. I regret to say this, but the omap_hsmmc driver is becoming a total mess. The host controller driver has to be a dumb interface serving requests from the hardware used by the upper layer stack, not the place where decisions such as power handling should be made. Think of it like an ethernet driver. No ethernet driver in Linux is telling the IP stack when to shut down. The omap_hsmmc driver does not tell the core to shut down the upper layers. Instead the core tells the omap_hsmmc driver that it is disabled i.e. not currently being used so it can start taking steps to save power. That is sensible because not even the upper layers know when the next activity will be. Shouldn't the power control intelligence (i.e. when to turn power ON/OFF) lie with the bus drivers? Absolutely! And in the SDIO case that should lie with each function drivers. Please let's stop this omap_hsmmc madness. You are dealing with a trivial case - turn off the power when not in use. We have 3 power saving actions: enable DPS, put the card to sleep, and finally power it off. Each increases the latency to start up again and so must be staggered. With DPS-enabled the host controller can be powered off at any time. In that case the controller registers must be restored. There are numerous entry points to the driver. Checking whether to restore registers at every entry point is error prone and messy. Instead we require that the core tells the driver when to enable and disable. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Wed, 7 Jul 2010, Adrian Hunter wrote: Nicolas Pitre wrote: On Wed, 7 Jul 2010, Roger Quadros wrote: On 07/06/2010 10:51 PM, Hunter Adrian (Nokia-MS/Helsinki) wrote: For eMMC in omap_hsmmc, this is all done via claim_host / release_host which call -enable() / -disable() methods. omap_hsmmc makes use of mmc_power_restore_host() which calls host-bus_ops-power_restore() which is not implemented for SDIO, but for MMC and SD it reinitializes the card. This is IMHO a really bad design. The power control decision has to come from the top, not from the bottom. And certainly not with a U-turn dependency the omap_hsmmc is using. The power control decision does come from the top via mmc_claim_host / mmc_release_host. NO!!! THIS IS NOT ABOUT POWER! To the risk of repeating myself, mmc_claim_host and mmc_release_host are about getting exclusive access to the host controller. This should not have any relationship with power. If anything, you might infer some idleness of the host through that, but only to save power at the host controller level, but not at the card level. I regret to say this, but the omap_hsmmc driver is becoming a total mess. The host controller driver has to be a dumb interface serving requests from the hardware used by the upper layer stack, not the place where decisions such as power handling should be made. Think of it like an ethernet driver. No ethernet driver in Linux is telling the IP stack when to shut down. The omap_hsmmc driver does not tell the core to shut down the upper layers. What is this call to mmc_power_save_host() and mmc_power_restore_host() then? Instead the core tells the omap_hsmmc driver that it is disabled i.e. not currently being used so it can start taking steps to save power. That's not how I see things implemented right now. That is sensible because not even the upper layers know when the next activity will be. I don't agree with you. The upper layer, even if it cannot predict exactly when the next activity will occur, still has a hell of a better visibility on what is going on. In the context of an MMC/SD card, the upper layer knows about the block subsystem and it can look for dirty blocks that might be flushed in a near future. In the context of a SDIO card, it is the SDIO function driver that knows when it is important to preserve power to the card and when it is not. All the host controller driver must do is to simply and dumbly process requests from the core MMC code, including power control requests. Shouldn't the power control intelligence (i.e. when to turn power ON/OFF) lie with the bus drivers? Absolutely! And in the SDIO case that should lie with each function drivers. Please let's stop this omap_hsmmc madness. You are dealing with a trivial case - turn off the power when not in use. And apparently this cannot be implemented sanely on OMAP without faking a card removal. We have 3 power saving actions: enable DPS, put the card to sleep, and finally power it off. Each increases the latency to start up again and so must be staggered. With DPS-enabled the host controller can be powered off at any time. In that case the controller registers must be restored. You could implement the first one based on some idle period. The core code probably doesn't need to know as this should be transparent to the upper layer. But the other two definitely should be handled by the core code. There are numerous entry points to the driver. Checking whether to restore registers at every entry point is error prone and messy. Please give me something else than this lame excuse. There is effectively only _one_ main entry point, namely the request method of the mmc_host_ops structure. You might need to poke at the hardware when the set_ios or the enable_sdio_irq methods are called, and if the controller is latent then you'd only have to update the register cache. This is certainly not what I would call numerous. Instead we require that the core tells the driver when to enable and disable. No you don't. You are abusing the mmc_claim_host interface with power management issues. Those power issues are then guessed in the host controller driver, and then it eventually calls back into the core to tell the upper layer to shut off. What I'm telling you is that: 1) If you want to save power after some idle period you don't need host-ops-enable and host-ops-disable at all. Simply keep a timer alive in your host driver and reset it when a new request comes in. The code for mmc_host_enable() looks rather redundant, and fishy to me with its flag to prevent recursion, so this all could be removed. 2) Putting the card to sleep and/or removing power to it must be completely managed by the core code and certainly not from the host controller driver. The fact that mmc_power_save_host() is currently called from a host
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi, On 07/06/2010 03:37 AM, ext Ohad Ben-Cohen wrote: From: Ohad Ben-Cohenoh...@ti.com Introduce a platform device support which is decoupled from the life cycle of the sdio function. The platform device objective is to deliver board-specific values (like irq, ref_clock, and software card-detect emulation control). The driver can then dynamically create (or destroy) the sdio function whenever the wlan interface is brought up (or down), as part of the power() operation. Is this the right approach? Why should you emulate a card disconnect event when the wlan interface is brought down? Physically the card is never disconnected. So I don't see why we should fake it. Could you please explain why you need to do this? br, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Roger, On Tue, Jul 6, 2010 at 11:53 AM, Roger Quadros roger.quad...@nokia.com wrote: Could you please explain why you need to do this? To minimize power consumption when the wlan device is not in use, we would like to keep the device powered off as long as the interface is down, and only power it on when the user brings up the interface. Whenever the chip is powered on, it must be initialized and reconfigured by mmc_attach_sdio, and the wl1271 driver have to wait for that phase to successfully complete (essentially waiting for the sdio_driver's probe function to be called). To make sure this SDIO init step occurs correctly every time we toggle the device's power back on, and to prevent potential races, we also have to make sure that the sdio function is removed every time we power off the chip (the driver waits for the sdio_driver's remove function to be called). That's why we let the mmc layer think that the card is removed: physically it is still there, but for all practical purposes it is really removed, because once you power off the chip, you must reinitialize it again next time you power it on, as if the card was really removed and re-inserted. Thanks, Ohad. br, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Ohad, On 07/06/2010 12:30 PM, ext Ohad Ben-Cohen wrote: Hi Roger, On Tue, Jul 6, 2010 at 11:53 AM, Roger Quadrosroger.quad...@nokia.com wrote: Could you please explain why you need to do this? To minimize power consumption when the wlan device is not in use, we would like to keep the device powered off as long as the interface is down, and only power it on when the user brings up the interface. Agreed. Whenever the chip is powered on, it must be initialized and reconfigured by mmc_attach_sdio, and the wl1271 driver have to wait for that phase to successfully complete (essentially waiting for the sdio_driver's probe function to be called). To make sure this SDIO init step occurs correctly every time we toggle the device's power back on, and to prevent potential races, we also have to make sure that the sdio function is removed every time we power off the chip (the driver waits for the sdio_driver's remove function to be called). That's why we let the mmc layer think that the card is removed: physically it is still there, but for all practical purposes it is really removed, because once you power off the chip, you must reinitialize it again next time you power it on, as if the card was really removed and re-inserted. My point is that shouldn't this be handled by SDIO core? If there are no users for the SDIO function and the card, doesn't the SDIO core power down the slot, and take care of re-initialization when it is powered up? -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Hi Roger, On Tue, Jul 6, 2010 at 1:35 PM, Roger Quadros roger.quad...@nokia.com wrote: My point is that shouldn't this be handled by SDIO core? Care to explain what you mean / give a code example ? If there are no users for the SDIO function and the card, doesn't the SDIO core power down the slot and take care of re-initialization when it is powered up? You need card detect events in order to trigger card sdio function initialization and removals. Please share any alternative approach you may be thinking on. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On 07/06/2010 03:53 PM, ext Ohad Ben-Cohen wrote: Hi Roger, On Tue, Jul 6, 2010 at 1:35 PM, Roger Quadrosroger.quad...@nokia.com wrote: My point is that shouldn't this be handled by SDIO core? Care to explain what you mean / give a code example ? If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then the SDIO/MMC core should tackle it, just like it deals with supply for slots with removable cards. see mmc_regulator_set_ocr() mmc_power_up() mmc_set_ios() in drivers/mmc/core/core.c and omap_hsmmc_set_ios() in drivers/mmc/host/omap_hsmmc.c If there are no users for the SDIO function and the card, doesn't the SDIO core power down the slot and take care of re-initialization when it is powered up? You need card detect events in order to trigger card sdio function initialization and removals. Please share any alternative approach you may be thinking on. OK, this is how I see it. - Treat the non-removable card as non-removable. So no need to do card detect emulation. - Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator framework to define this regulator supply. Even though you mention that it is not actually a supply, it fits well in the fixed supply framework. - When the host controller is enumerated, the mmc core will power up the slot, find the sdio card, and probe the function driver (i.e. wl1271_sdio). - if interface is not in use, the function driver must release the sdio host, and this should eventually disable the vmmc supply. - Whenever the wlan interface must be brought up, wl1271_sdio, can claim the sdio host. this will cause the vmmc supply to be enabled, for as long as the interface is up. Does this address all issues? regards, -roger -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
On Tue, 6 Jul 2010, Roger Quadros wrote: On 07/06/2010 03:53 PM, ext Ohad Ben-Cohen wrote: Hi Roger, On Tue, Jul 6, 2010 at 1:35 PM, Roger Quadrosroger.quad...@nokia.com wrote: My point is that shouldn't this be handled by SDIO core? Care to explain what you mean / give a code example ? If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then the SDIO/MMC core should tackle it, just like it deals with supply for slots with removable cards. Exact. You need card detect events in order to trigger card sdio function initialization and removals. Why would you trigger function initialization and removal? Just to turn off power? That's a bit like pulling off the battery from your laptop when you want to suspend it. There is a better way to go about things. Please share any alternative approach you may be thinking on. OK, this is how I see it. - Treat the non-removable card as non-removable. So no need to do card detect emulation. - Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator framework to define this regulator supply. Even though you mention that it is not actually a supply, it fits well in the fixed supply framework. - When the host controller is enumerated, the mmc core will power up the slot, find the sdio card, and probe the function driver (i.e. wl1271_sdio). - if interface is not in use, the function driver must release the sdio host, and this should eventually disable the vmmc supply. - Whenever the wlan interface must be brought up, wl1271_sdio, can claim the sdio host. this will cause the vmmc supply to be enabled, for as long as the interface is up. Does this address all issues? This is mostly all good, except that claiming/releasing the SDIO host is about access to the bus. It must be claimed right before doing any IO, and released right after that, even when the card is expected to remain powered. This is not the proper place to hook power control. Another function pair would be needed instead, which would do almost like the suspend/resume code is already doing. Something like: /* * Indicate to the core SDIO layer that we're not requiring that the * function remain powered. If all functions for the card are in the * same no power state, then the host controller can remove power from * the card. Note: the function driver must preserve hardware states if * necessary. */ int sdio_release_power(struct sdio_func *func); /* * Indicate to the core SDIO layer that we want power back for this * SDIO function. The power may or may not actually have been removed * since last call to sdio_release_power(), so the function driver must * not assume any preserved state at the hardware level and re-perform * all the necessary hardware config. This function returns 0 when * power is actually restored, or some error code if this cannot be * achieved. One error reason might be that the card is no longer * available on the bus (was removed while powered down and card * detection didn't trigger yet). */ int sdio_claim_power(struct sdio_func *func); That's it. When the network interface is down and the hardware is not needed, you call sdio_release_power(). When the request to activate the network interface is received, you call sdio_claim_power() and configure the hardware appropriately. If sdio_claim_power() returns an error, then you just return an error to the network request, and eventually the driver's remove method will be called if this is indeed because the card was removed. In the core SDIO code, this is almost identical to a suspend/resume request, except that the request comes from the function driver instead of the core MMC code. Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/15] wireless: wl1271: introduce platform device support
Nicolas Pitre wrote:
On Tue, 6 Jul 2010, Roger Quadros wrote:
On 07/06/2010 03:53 PM, ext Ohad Ben-Cohen wrote:
Hi Roger,
On Tue, Jul 6, 2010 at 1:35 PM, Roger Quadrosroger.quad...@nokia.com
wrote:
My point is that shouldn't this be handled by SDIO core?
Care to explain what you mean / give a code example ?
If the Power enable GPIO can be treated as SDIO slot supply (i.e. vmmc), then
the SDIO/MMC core should tackle it, just like it deals with supply for slots
with removable cards.
Exact.
You need card detect events in order to trigger card sdio function
initialization and removals.
Why would you trigger function initialization and removal? Just to turn
off power? That's a bit like pulling off the battery from your laptop
when you want to suspend it. There is a better way to go about things.
Please share any alternative approach you may be thinking on.
OK, this is how I see it.
- Treat the non-removable card as non-removable. So no need to do card detect
emulation.
- Treat the GPIO power enable on wl1271 as VMMC supply. Use fixed regulator
framework to define this regulator supply. Even though you mention that it
is not actually a supply, it fits well in the fixed supply framework.
- When the host controller is enumerated, the mmc core will power up the slot,
find the sdio card, and probe the function driver (i.e. wl1271_sdio).
- if interface is not in use, the function driver must release the sdio host,
and this should eventually disable the vmmc supply.
- Whenever the wlan interface must be brought up, wl1271_sdio, can claim the
sdio host. this will cause the vmmc supply to be enabled, for as long as the
interface is up.
Does this address all issues?
This is mostly all good, except that claiming/releasing the SDIO host is
about access to the bus. It must be claimed right before doing any IO,
and released right after that, even when the card is expected to remain
powered. This is not the proper place to hook power control.
Another function pair would be needed instead, which would do almost
like the suspend/resume code is already doing. Something like:
/*
* Indicate to the core SDIO layer that we're not requiring that the
* function remain powered. If all functions for the card are in the
* same no power state, then the host controller can remove power from
* the card. Note: the function driver must preserve hardware states if
* necessary.
*/
int sdio_release_power(struct sdio_func *func);
/*
* Indicate to the core SDIO layer that we want power back for this
* SDIO function. The power may or may not actually have been removed
* since last call to sdio_release_power(), so the function driver must
* not assume any preserved state at the hardware level and re-perform
* all the necessary hardware config. This function returns 0 when
* power is actually restored, or some error code if this cannot be
* achieved. One error reason might be that the card is no longer
* available on the bus (was removed while powered down and card
* detection didn't trigger yet).
*/
int sdio_claim_power(struct sdio_func *func);
That's it. When the network interface is down and the hardware is not
needed, you call sdio_release_power(). When the request to activate the
network interface is received, you call sdio_claim_power() and configure
the hardware appropriately. If sdio_claim_power() returns an error,
then you just return an error to the network request, and eventually the
driver's remove method will be called if this is indeed because the card
was removed.
In the core SDIO code, this is almost identical to a suspend/resume
request, except that the request comes from the function driver instead
of the core MMC code.
For eMMC in omap_hsmmc, this is all done via claim_host / release_host
which call -enable() / -disable() methods. omap_hsmmc makes use of
mmc_power_restore_host() which calls host-bus_ops-power_restore()
which is not implemented for SDIO, but for MMC and SD it reinitializes
the card.
Set omap2_hsmmc_info mmc[x] {.nonremovable=true, .power_saving=true} and
implement host-bus_ops-power_restore() for SDIO, then the power will
go off 9 seconds after sdio_release_host() is called. Then tweak omap_hsmmc
so that it doesn't wait 9 seconds for the SDIO case
Nicolas
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/15] wireless: wl1271: introduce platform device support
From: Ohad Ben-Cohen oh...@ti.com
Introduce a platform device support which is decoupled
from the life cycle of the sdio function.
The platform device objective is to deliver board-specific
values (like irq, ref_clock, and software card-detect
emulation control).
The driver can then dynamically create (or destroy)
the sdio function whenever the wlan interface is
brought up (or down), as part of the power() operation.
Signed-off-by: Ohad Ben-Cohen oh...@ti.com
---
drivers/net/wireless/wl12xx/Kconfig |1 +
drivers/net/wireless/wl12xx/wl1271.h |1 +
drivers/net/wireless/wl12xx/wl1271_sdio.c | 185 +++--
3 files changed, 150 insertions(+), 37 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/Kconfig
b/drivers/net/wireless/wl12xx/Kconfig
index 337fc7b..8fb7b5a 100644
--- a/drivers/net/wireless/wl12xx/Kconfig
+++ b/drivers/net/wireless/wl12xx/Kconfig
@@ -66,6 +66,7 @@ config WL1271_SPI
config WL1271_SDIO
tristate TI wl1271 SDIO support
depends on WL1271 MMC ARM
+ select MMC_EMBEDDED_SDIO
---help---
This module adds support for the SDIO interface of adapters using
TI wl1271 chipset. Select this if your platform is using
diff --git a/drivers/net/wireless/wl12xx/wl1271.h
b/drivers/net/wireless/wl12xx/wl1271.h
index 5250361..2df57cc 100644
--- a/drivers/net/wireless/wl12xx/wl1271.h
+++ b/drivers/net/wireless/wl12xx/wl1271.h
@@ -352,6 +352,7 @@ struct wl1271 {
bool mac80211_registered;
void *if_priv;
+ void *if_plat_priv;
struct wl1271_if_operations *if_ops;
diff --git a/drivers/net/wireless/wl12xx/wl1271_sdio.c
b/drivers/net/wireless/wl12xx/wl1271_sdio.c
index 571c6b9..96b8fc3 100644
--- a/drivers/net/wireless/wl12xx/wl1271_sdio.c
+++ b/drivers/net/wireless/wl12xx/wl1271_sdio.c
@@ -28,6 +28,10 @@
#include linux/mmc/sdio_func.h
#include linux/mmc/sdio_ids.h
#include linux/mmc/card.h
+#include linux/mmc/host.h
+#include linux/completion.h
+#include linux/wl12xx.h
+#include linux/platform_device.h
#include plat/gpio.h
#include wl1271.h
@@ -36,6 +40,7 @@
#define RX71_WL1271_IRQ_GPIO 42
+static DECLARE_COMPLETION(wl1271_sdio_ready);
static const struct sdio_device_id wl1271_devices[] = {
{ SDIO_DEVICE(SDIO_VENDOR_ID_TI, SDIO_DEVICE_ID_TI_WL1271) },
@@ -50,7 +55,9 @@ static inline struct sdio_func *wl_to_func(struct wl1271 *wl)
static struct device *wl1271_sdio_wl_to_dev(struct wl1271 *wl)
{
- return (wl_to_func(wl)-dev);
+ struct platform_device *pdev = wl-if_plat_priv;
+
+ return pdev-dev;
}
static irqreturn_t wl1271_irq(int irq, void *cookie)
@@ -146,21 +153,44 @@ static void wl1271_sdio_raw_write(struct wl1271 *wl, int
addr, void *buf,
static int wl1271_sdio_set_power(struct wl1271 *wl, bool enable)
{
- struct sdio_func *func = wl_to_func(wl);
+ int ret = 0, timeleft;
- /* Let the SDIO stack handle wlan_enable control, so we
-* keep host claimed while wlan is in use to keep wl1271
-* alive.
-*/
if (enable) {
- sdio_claim_host(func);
- sdio_enable_func(func);
+ /* save our wl struct as private mmc data */
+ wl-set_embedded_data(wl);
+
+ INIT_COMPLETION(wl1271_sdio_ready);
+ /* 1271 Power Up Sequence */
+ wl-set_power(true);
+ mdelay(15);
+ wl-set_power(false);
+ mdelay(1);
+ wl-set_power(true);
+ mdelay(70);
+
+ /* emulate card detect event */
+ wl-set_carddetect(true);
+
} else {
- sdio_disable_func(func);
- sdio_release_host(func);
+ wl-set_embedded_data(NULL);
+
+ INIT_COMPLETION(wl1271_sdio_ready);
+
+ wl-set_power(false);
+ mdelay(10);
+
+ wl-set_carddetect(false);
}
- return 0;
+ timeleft = wait_for_completion_interruptible_timeout(wl1271_sdio_ready,
+ msecs_to_jiffies(5000));
+ if (timeleft = 0) {
+ wl1271_error(%s: unsuccessful SDIO %s (%d), __func__,
+ enable ? init : deinit, timeleft);
+ ret = (timeleft == 0 ? -EAGAIN : timeleft);
+ }
+
+ return ret;
}
static struct wl1271_if_operations sdio_ops = {
@@ -174,30 +204,97 @@ static struct wl1271_if_operations sdio_ops = {
.disable_irq= wl1271_sdio_disable_interrupts
};
-static int __devinit wl1271_probe(struct sdio_func *func,
+static int wl1271_sdio_probe(struct sdio_func *func,
const struct sdio_device_id *id)
{
+ struct wl1271 *wl = mmc_get_embedded_data(func-card-host);
+
+ /* 2nd func is WLAN, make sure wl context is received */
+ if (func-num != 0x02 || !wl)
+ return -ENODEV;
