[PATCH 11/15] wireless: wl1271: introduce platform device support

[Nicolas Pitre]

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 driver and not from the core code or function 
   drivers is completely backward.  Contrary to the suspend/resume 
   requests which come from the machine bus where the host controller is 
   attached (yes, this concept applies to you even if you don't have 
   PCI), the possibility for shutting down power to the card when not in 
   use must come from the so called "MMC bus driver" like the MMC 
   block driver, or the SDIO function driver.  Putting a SDIO card to 
   sleep is card dependent anyway, and that knowledge has to live in the 
   SDIO function driver for that card, and certainly not in the host 
   driver nor in the platform code.

3) And yet that does _not_ require any new capability or method to be 
   implemented in the host controller driver as mmc_set_ios() can do 
   power handling already.

And incidentally, implementing 3 and 4 would suddenly work for all hosts 
without any change to the existing host controller drivers and prevent a 
whole lot of functionality duplications.


Nicolas