Hi Ulf,
Sorry for the delayed response.
The patches looks good to me except one concern mentioned below.
On 3/21/2013 3:28 AM, Ulf Hansson wrote:
If any driver wants to implement this then the runtime PM code would be
refactored again. So I guess we might want to think about this now itself?
Refactored, no.
It is just a new feature that needs to be added, should be a rather
simple patch. Since this kind of code has not been upstreamed for your
host driver I did not consider it in this initial step. Do you want me
to create an additional patch on top of this patchset? I can help out
if you like.
What about SD cards? For SD cards the runtime PM is not doing any advantage
but instead waste cpu cycles with a timer interrupt and running noop runtime
PM callbacks? I guess allowing to power off cards in such cases would have
decent power savings.
We will waste some cpu cycles, true.
Do you think that will have bad impact on performance? In that case
why do we even bother doing runtime PM in host drivers and in many
other places in the kernel? Of course we could optmize the code and
only enable runtime PM if there are a corresponding runtime pm
callbacks implemented in the bus_ops, but is it needed?
Well.. my point here is that runtime PM framework unnecessarily wakeup
processor (if idle) every "x" secs without doing any useful work. If
that is agreeable then I am okay with not having the optimization.
Please have a look at below thread to find the answers to your questions:
http://thread.gmane.org/gmane.linux.kernel.mmc/19444/focus=19443
Thanks a lot. I have missed this discussion :(
I have some comments on the possible solutions:
"In mmc bus_ops runtime callback, do a pm_runtime_get_sync("host plf
device"), and vice verse in the runtime resume callback. This will
prevent the host driver from entering runtime power save sate while
for example doing BKOPS, thus preventing your host driver from doing
mmc_suspend_host while BKOPS is running"
[Sujit] In addition, probably we can allow host to turn off the clocks while
carrying out BKOPS. But, how can we know whether card is done with BKOPS and
is idle so that we can call mmc_suspend_host()?
We are then going into details about how to implement IDLE BKOPS,
which is a bit out of scope for this patch, but let me try to comment
anyway.
The initial patch for BKOPS could skip your consideration, and just
check for BKOPS complete once runtime suspend callback gets called.
This will then be rather simple to implement and work for all cases
but yours. I realize that a new blk request will be needed to move out
from BKOPS state then.
The next step could be to schedule a timer/work when issuing BKOPS,
that polls for completion. I belive it should be rather simple to
extend the runtime pm callbacks with this support, right?
Thanks for the details. It looks clear to me now.
"Move mmc_suspend|resume_host from your host runtime callbacks, into
the bus_ops runtime callbacks. Typically, when no BKOPS is needed
mmc_suspend_host can be done."
[Sujit] Doesn't it look like we are establishing parent child relationship
here? If the card has nothing to do, suspend the host?
Well, the naming of these functions are not correct. It is not the
host that actually gets suspended, it is the card.
Right now these functions happens to be called when a host enters
suspend though, which indeed is also kind of strange. It would make
more sense to handle card suspend from the mmc/sdio bus instead; but
let's leave that for a separate discussion. :-)
I agree.
I also assume that if your host driver runtime pm callbacks calls
mmc_suspend|resume_host, your host driver system suspend|resume
callbacks must not - otherwise you will have races! Instead upper
layers like a power domain, must force your device into runtime
suspend when entering system suspend and vice verse when doing system
resume. These issues exists with or without my patches.
Possibly, the races can be avoided using pm_runtime_suspended() check,
but I am not sure if it is the clean way.
--
Regards,
Sujit
--
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
