Hi Jaehoon,
On 2016/10/21 9:56, Jaehoon Chung wrote:
Hi Shawn,
On 10/18/2016 08:35 PM, Shawn Lin wrote:
在 2016/10/18 16:46, Ulf Hansson 写道:
+ Heiko
On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
Hi Jaehoon and Ulf,
I applied this on my repository. If there are any issue, let me know.
And minor issue can be fixed..step by step.
I was checking your repository and there is no doubt for them.
Thanks a lot!
Best Regards,
Jaehoon Chung
This patch is gonna support runtime PM for dw_mmc.
It could support to disable ciu_clk by default and disable
biu_clk if the devices are non-removeable, or removeable
with gpio-base card detect.
Then I remove the system PM since the runtime PM actually
does the same thing as it. So I help migrate the dw_mmc variant
drivers to use runtime PM pairs and pm_runtime_force_*. Note
that I only enable runtime PM for dw_mmc-rockchip as I will
leave the decision to the owners of the corresponding drivers.
I just tested it on my RK3288 platform with linux-next to make
the runtime PM and system PM work fine for my emmc, sd card and
sdio. But I don't have hardware to help test other variant drivers.
But in theory it should work fine as I mentioned that the runtime
PM does the same thing as system PM except for disabling ciu_clk
aggressively which should not be related to the variant hosts.
As you could see that I just extend the slot-gpio a bit, so the
ideal way is Ulf could pick them up with Jaehoon's ack. :)
The mmc core change looks fine to me, so I will wait for a pull
request from Jaehoon.
Changes in v2:
- use struct device as argument for runtime callback
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly
- minor fix since I change the argument for dw_mci_runtime_*
- use dw_mci_runtime_* directly
- use dw_mci_runtime_* directly
Shawn Lin (9):
mmc: dw_mmc: add runtime PM callback
mmc: dw_mmc-rockchip: add runtime PM support
mmc: core: expose the capability of gpio card detect
mmc: dw_mmc: disable biu clk if possible
mmc: dw_mmc-k3: deploy runtime PM facilities
mmc: dw_mmc-exynos: deploy runtime PM facilities
mmc: dw_mmc-pci: deploy runtime PM facilities
mmc: dw_mmc-pltfm: deploy runtime PM facilities
mmc: dw_mmc: remove system PM callback
drivers/mmc/core/slot-gpio.c | 8 +++++++
drivers/mmc/host/dw_mmc-exynos.c | 24 +++++++++-----------
drivers/mmc/host/dw_mmc-k3.c | 39 ++++++++------------------------
drivers/mmc/host/dw_mmc-pci.c | 29 ++++++++----------------
drivers/mmc/host/dw_mmc-pltfm.c | 28 +++++++----------------
drivers/mmc/host/dw_mmc-rockchip.c | 42 +++++++++++++++++++++++++++++++---
drivers/mmc/host/dw_mmc.c | 46 +++++++++++++++++++++++++-------------
drivers/mmc/host/dw_mmc.h | 6 ++---
include/linux/mmc/slot-gpio.h | 1 +
9 files changed, 117 insertions(+), 106 deletions(-)
Overall these changes looks good to me, so I am ready to accept the PR
from Jaehoon!!
Although, highly related to this patchset, I am worried that there is
a misunderstanding on how MMC_PM_KEEP_POWER (DT binding
"keep-power-in-suspend") is being used for dw_mmc. Perhaps I am wrong,
but I would appreciate if you could elaborate a bit for my
understanding.
First, this cap is solely intended to be used for controllers which
may have SDIO cards attached, as it indicates those cards may be
configured to be powered on while the system enters suspend state. By
looking at some DTS files, for example
arch/arm64/boot/dts/rockchip/rk3368-orion-r68-meta.dts which uses it
for an eMMC slot, this is clearly being abused.
Indeed. In general, it should be copy-paste issues as folks maybe write
their dts referring to the exist dts there. So yes, I will do some cleanup work for them in prevent of further spread of abused code.
Anyway, the wrong DT configurations might not be a big deal, as in
dw_mci_resume(), it's not the capabilities bit that is checked but the
corresponding "pm_flag". This flag is set via calling
sdio_set_host_pm_flags(), but as that doesn't happen for an eMMC card
we should be fine, right!?
Now, what also do puzzles me, is exactly that piece of code in
dw_mci_resume() that is being executed *when* the pm_flag contains
MMC_PM_KEEP_POWER:
if (slot->mmc->pm_flags & MMC_PM_KEEP_POWER) {
dw_mci_set_ios(slot->mmc, &slot->mmc->ios);
dw_mci_setup_bus(slot, true);
}
So, in the system resume path, the above do makes sense as you need to
restore the registers etc for the dw_mmc controller to enable it to
operate the SDIO card. Such as bus width, clocks, etc.
Although, I would expect something similar would be needed in the new
runtime resume path as well. And in particular also for eMMC/SD cards,
as you need to restore the dw_mmc registers to be able to operate the
card again. Don't you?
yes, we do.
So in the end, perhaps you should *always* call dw_mci_set_ios() and
dw_mci_setup_bus() in dw_mci_resume() instead of conditionally check
for MMC_PM_KEEP_POWER? Or maybe only a subset of those functions?
Thanks for noticing this.
Personally, I realize there is no need to check MMC_PM_KEEP_POWER but
it could be highly related to the cost of S-2-R, I guess. I just checked
sdhci and saw the similar cases you mentioned at the first glance.
Maybe I'm wrong but I need more time to investigate this issue later.
There are still some on-going cleanup work for dw_mmc listed on my TODO
list, including bugfix, legacy/redundant code etc.. So I will check this one either. Maybe Jaehoon could also do some stree test on enxyos
platforms. :)
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
