Messages in this thread | | | Subject | Re: [Patch v6 5/7] slimbus: qcom: Add runtime-pm support using clock-pause feature | From | Srinivas Kandagatla <> | Date | Sat, 7 Oct 2017 11:25:12 +0100 |
| |
Thanks for the review comments
On 07/10/17 09:22, Jonathan Neuschäfer wrote: > Hi, some more trivial comments below. > > On Fri, Oct 06, 2017 at 05:51:34PM +0200, srinivas.kandagatla@linaro.org wrote: >> From: Sagar Dharia <sdharia@codeaurora.org> >> >> Slimbus HW mandates that clock-pause sequence has to be executed >> before disabling relevant interface and core clocks. >> Runtime-PM's autosuspend feature is used here to enter/exit low >> power mode for Qualcomm's Slimbus controller. Autosuspend feature >> enables driver to avoid changing power-modes too frequently since >> entering clock-pause is an expensive sequence >> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- > [...] >> +static int msm_clk_pause_wakeup(struct slim_controller *ctrl) >> +{ >> + struct msm_slim_ctrl *dev = slim_get_ctrldata(ctrl); >> + >> + clk_prepare_enable(dev->hclk); >> + clk_prepare_enable(dev->rclk); >> + enable_irq(dev->irq); >> + >> + writel_relaxed(1, dev->base + FRM_WAKEUP); >> + /* Make sure framer wakeup write goes through before ISR fires */ >> + mb(); >> + /** > > This isn't really a kerneldoc comment. Yep I agree will fix all such instances.
> >> + * HW Workaround: Currently, slave is reporting lost-sync messages >> + * after slimbus comes out of clock pause. >> + * Transaction with slave fail before slave reports that message >> + * Give some time for that report to come >> + * Slimbus wakes up in clock gear 10 at 24.576MHz. With each superframe >> + * being 250 usecs, we wait for 5-10 superframes here to ensure >> + * we get the message >> + */ >> + usleep_range(1250, 2500); >> + return 0; >> +} > [...] >> +#ifdef CONFIG_PM_SLEEP >> +static int msm_slim_suspend(struct device *dev) >> +{ >> + int ret = 0; >> + >> + if (!pm_runtime_enabled(dev) || >> + (!pm_runtime_suspended(dev))) { >> + dev_dbg(dev, "system suspend"); >> + ret = msm_slim_runtime_suspend(dev); >> + } >> + if (ret == -EISCONN) { >> + /** > > ditto. > Also, it looks misindented. Yep. will fix this too.
> >> + * If the clock pause failed due to active channels, there is >> + * a possibility that some audio stream is active during suspend. >> + * (e.g. modem usecase during suspend) >> + * We dont want to return suspend failure in that case so that >> + * display and relevant components can still go to suspend. >> + * If there is some other error, then it should prevent >> + * system level suspend >> + */ >> + ret = 0; >> + } >> + return ret; >> +} > > > Thanks, > Jonathan Neuschäfer >
| |