On 10/18/2016 05:46 PM, Ulf Hansson wrote:
> + Heiko
>
> On 12 October 2016 at 04:50, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
>>
>> Hi Jaehoon and Ulf,
>>
>> 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!!
>
Yes..I will apply on my repository..i'm testing on dwmmc-testing branch..
Actually, i found the some blocking issue but it's not relevant to these patches.
after checking the stress test until today..(eMMC/SD/SDIO)..I will do.
Entire patches looks good to me..thanks for Shawn's effort! :)
>
> 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.
Right..it doesn't need to uses "keep-power-in-suspend" for eMMC/SD.
>
> 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);
> }
There is no place to set this in dw_mmc controller..Maybe it should be located in other vendor driver.
Almost all cards that used SDIO might be WiFi..Of course..there might be other cards.
e.g) In exnyos, use the sdio as WiFi card(broadcom chipset) then broadcom driver also used..
When broadcom driver initialized or suspending time, it should be set to this flag..
As i mentioned..it's not normal case.
>
> 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?
Need to check :)
>
> 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?
It's reasonable..but it also needs to check on real case and stress test.
Best Regards,
Jaehoon Chung
>
> 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
>
>
>
--
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
