<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 7/26/2018 9:00 PM, Robin Murphy wrote: </div> <blockquote type="cite" cite="mid:401d8509-b9ad-913b-334e-f4ac853472e3@arm.com">On 26/07/18 08:12, Vivek Gautam wrote: <blockquote type="cite">On Wed, Jul 25, 2018 at 11:46 PM, Vivek Gautam <a class="moz-txt-link-rfc2396E" href="mailto:vivek.gautam@codeaurora.org"><vivek.gautam@codeaurora.org></a> wrote: <blockquote type="cite">On Tue, Jul 24, 2018 at 8:51 PM, Robin Murphy <a class="moz-txt-link-rfc2396E" href="mailto:robin.murphy@arm.com"><robin.murphy@arm.com></a> wrote: <blockquote type="cite">On 19/07/18 11:15, Vivek Gautam wrote: <blockquote type="cite"> From: Sricharan R <a class="moz-txt-link-rfc2396E" href="mailto:sricharan@codeaurora.org"><sricharan@codeaurora.org></a> The smmu needs to be functional only when the respective master's using it are active. The device_link feature helps to track such functional dependencies, so that the iommu gets powered when the master device enables itself using pm_runtime. So by adapting the smmu driver for runtime pm, above said dependency can be addressed. This patch adds the pm runtime/sleep callbacks to the driver and also the functions to parse the smmu clocks from DT and enable them in resume/suspend. Also, while we enable the runtime pm add a pm sleep suspend callback that pushes devices to low power state by turning the clocks off in a system sleep. Also add corresponding clock enable path in resume callback. Signed-off-by: Sricharan R <a class="moz-txt-link-rfc2396E" href="mailto:sricharan@codeaurora.org"><sricharan@codeaurora.org></a> Signed-off-by: Archit Taneja <a class="moz-txt-link-rfc2396E" href="mailto:architt@codeaurora.org"><architt@codeaurora.org></a> [vivek: rework for clock and pm ops] Signed-off-by: Vivek Gautam <a class="moz-txt-link-rfc2396E" href="mailto:vivek.gautam@codeaurora.org"><vivek.gautam@codeaurora.org></a> Reviewed-by: Tomasz Figa <a class="moz-txt-link-rfc2396E" href="mailto:tfiga@chromium.org"><tfiga@chromium.org></a> --- Changes since v12: - Added pm sleep .suspend callback. This disables the clocks. - Added corresponding change to enable clocks in .resume pm sleep callback. drivers/iommu/arm-smmu.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index c73cfce1ccc0..9138a6fffe04 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c </blockquote> </blockquote> </blockquote> [snip] <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">platform_device *pdev) arm_smmu_device_remove(pdev); } +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) +{ + struct arm_smmu_device *smmu = dev_get_drvdata(dev); + + return clk_bulk_enable(smmu->num_clks, smmu->clks); </blockquote> If there's a power domain being automatically switched by genpd then we need a reset here because we may have lost state entirely. Since I remembered the otherwise-useless GPU SMMU on Juno is in a separate power domain, I gave it a poking via sysfs with some debug stuff to dump sCR0 in these callbacks, and the problem is clear: ... [ 4.625551] arm-smmu 2b400000.iommu: genpd_runtime_suspend() [ 4.631163] arm-smmu 2b400000.iommu: arm_smmu_runtime_suspend: 0x00201936 [ 4.637897] arm-smmu 2b400000.iommu: suspend latency exceeded, 6733980 ns [ 21.566983] arm-smmu 2b400000.iommu: genpd_runtime_resume() [ 21.584796] arm-smmu 2b400000.iommu: arm_smmu_runtime_resume: 0x00220101 [ 21.591452] arm-smmu 2b400000.iommu: resume latency exceeded, 6658020 ns ... </blockquote> Qualcomm SoCs have retention enabled for SMMU registers so they don't lose state. ... [ 256.013367] arm-smmu b40000.arm,smmu: arm_smmu_runtime_suspend SCR0 = 0x201e36 [ 256.013367] [ 256.019160] arm-smmu b40000.arm,smmu: arm_smmu_runtime_resume SCR0 = 0x201e36 [ 256.019160] [ 256.027368] arm-smmu b40000.arm,smmu: arm_smmu_runtime_suspend SCR0 = 0x201e36 [ 256.027368] [ 256.036786] arm-smmu b40000.arm,smmu: arm_smmu_runtime_resume SCR0 = 0x201e36 ... However after adding arm_smmu_device_reset() in runtime_resume() I observe some performance degradation when kill an instance of 'kmscube' and start it again. The launch time with arm_smmu_device_reset() in runtime_resume() change is more. Could this be because of frequent TLB invalidation and sync? </blockquote> </blockquote> Probably. Plus the reset procedure is a big chunk of MMIO accesses, which for a non-trivial SMMU configuration probably isn't negligible in itself. Unfortunately, unless you know for absolute certain that you don't need to do that, you do. <blockquote type="cite">Some more information that i gathered. On Qcom SoCs besides the registers retention, TCU invalidates TLB cache on a CX power collapse exit, which is the system wide suspend case. The arm-smmu software is not aware of this CX power collapse / auto-invalidation. So wouldn't doing an explicit TLB invalidations during runtime resume be detrimental to performance? </blockquote> Indeed it would be, but resuming with TLBs full of random valid-looking junk is even more so. <blockquote type="cite">I have one more doubt here - We do runtime power cycle around arm_smmu_map/unmap() too. Now during map/unmap we selectively do TLB maintenance (either tlb_sync or tlb_add_flush). But with runtime pm we want to do TLBIALL*. Is that a problem? </blockquote> It's technically redundant to do both, true, but as we've covered in previous rounds of discussion it's very difficult to know *which* one is sufficient at any given time, so in order to make progress for now I think we have to settle with doing both. </blockquote> Thanks Robin. I will respin the patches as Tomasz also suggested;<span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"> arm_smmu_runtime_resume() will look like:<br style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;"> <br style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;"> <span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"> if (pm_runtime_suspended(dev))<br style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;"> <span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"> return 0;<br style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;"> <span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"> return arm_smmu_runtime_resume(dev); and, <span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"><span style="color: rgb(34, 34, 34); font-family: arial, sans-serif; font-size: 12.8px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">arm_smmu_runtime_resume() will have arm_smmu_device_reset(). Best regards Vivek <blockquote type="cite" cite="mid:401d8509-b9ad-913b-334e-f4ac853472e3@arm.com"> Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to <a class="moz-txt-link-abbreviated" href="mailto:majordomo@vger.kernel.org">majordomo@vger.kernel.org</a> More majordomo info at <a class="moz-txt-link-freetext" href="http://vger.kernel.org/majordomo-info.html">http://vger.kernel.org/majordomo-info.html</a> </blockquote> </body> </html>