Hi, On Mon, Jul 16, 2018 at 12:11 PM, Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> wrote: > HI Rafael, > > > > On 7/16/2018 2:21 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam >> <vivek.gautam@xxxxxxxxxxxxxx> wrote: [cut] >>>> Although, given the PM >>>> subsystem internals, the suspend function wouldn't be called on SMMU >>>> implementation needed power control (since they would have runtime PM >>>> enabled) and on others, it would be called but do nothing (since no >>>> clocks). >>>> >>>>> Honestly, I just don't know. :-) >>>>> >>>>> It just looks odd the way it is done. I think the clock should be >>>>> gated during system-wide suspend too, because the system can spend >>>>> much more time in a sleep state than in the working state, on average. >>>>> >>>>> And note that you cannot rely on runtime PM to always do it for you, >>>>> because it may be disabled at a client device or even blocked by user >>>>> space via power/control in sysfs and that shouldn't matter for >>>>> system-wide PM. >>>> >>>> User space blocking runtime PM through sysfs is a good point. I'm not >>>> 100% sure how the PM subsystem deals with that in case of system-wide >>>> suspend. I guess for consistency and safety, we should have the >>>> suspend callback. >>> >>> Will add the following suspend callback (same as >>> arm_smmu_runtime_suspend): >>> >>> static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) >>> { >>> struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> >>> clk_bulk_disable(smmu->num_clks, smmu->clks); >>> >>> return 0; >>> } >> >> I think you also need to check if the clock has already been disabled >> by runtime PM. Otherwise you may end up disabling it twice in a row. > > > Should I rather call a pm_runtime_put() in suspend callback? That wouldn't work as runtime PM may be effectively disabled by user space via sysfs. That's one of the reasons why you need the extra system-wide suspend callback in the first place. :-) > Or an expanded form something similar to: > https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/slimbus/qcom-ctrl.c#L695 Yes, you can do something like that, but be careful to make sure that the state of the device after system-wide resume is consistent with its runtime PM status in all cases. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html