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 at rock-chips.com> 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 at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best Regards
Shawn Lin
