From: Viresh Kumar <viresh.kumar@linaro.org> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Kevin Wangtao <kevin.wangtao@hisilicon.com>, Leo Yan <leo.yan@linaro.org>, Jassi Brar <jaswinder.singh@linaro.org>, zhongkaihua@huawei.com, Dmitry Osipenko <digetx@gmail.com>, Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>, Stephen Boyd <sboyd@kernel.org>, linux-pm@vger.kernel.org, Vincent Guittot <vincent.guittot@linaro.org>, Rafael Wysocki <rjw@rjwysocki.net>, Sibi Sankar <sibis@codeaurora.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 03/13] opp: Keep track of currently programmed OPP Date: Mon, 12 Jul 2021 09:44:08 +0530 [thread overview] Message-ID: <20210712041408.5dgtwcp55pgt7twn@vireshk-i7> (raw) In-Reply-To: <20210709085704.GA10180@arm.com> On 09-07-21, 09:57, Ionela Voinescu wrote: > On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote: > > On 07-07-21, 11:24, Ionela Voinescu wrote: > > > Now comes the interesting part: what seems to fix it is a call to > > > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically > > > happened before this patch, as _find_current_opp() was always called. > > > I do not need to do anything with the returned frequency. > > > > Wow, thanks for narrowing it down this far :) > > > > I had a quick look and this is what I think is the problem here. > > > > This platform uses mailbox API to send its frequency change requests to another > > processor. And the way it is written currently, I don't see any guarantee > > whatsoever which say > > > > "once clk_set_rate() returns, the frequency would have already changed". > > > > I think what was strange to me was that the frequency never seems to > change, there isn't just a delay in the new frequency taking effect, as > I would expect in these cases. Or if there is a delay, that's quite large > - at least a second. No idea on what the firmware is doing behind the scene :) > > And this may exactly be the thing you are able to hit, luckily because of this > > patchset :) > > > > As a quick way of checking if that is right or not, this may make it work: > > > > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c > > index 395ddc250828..9856c1c84dcf 100644 > > --- a/drivers/mailbox/hi3660-mailbox.c > > +++ b/drivers/mailbox/hi3660-mailbox.c > > @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > > > > /* Trigger data transferring */ > > writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG); > > + > > + hi3660_mbox_check_state(chan); > > + > > I gave this a try an it does work for me. Good, so that kind of proves what I was suspecting. The mailbox driver looks buggy here. > > -------------------------8<------------------------- > > > > As a proper fix, something like this (not even compile tested) is required I > > believe as I don't see the clients would know if the transfer is over. Cc'ing > > mailbox guys to see what can be done. > > > > I'll give this a try as well when there is consensus. I might even try to > review it, if the time allows. Sure, lets see what the platform guys think about this first. Kevin, Kaihua ? -- viresh
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Kevin Wangtao <kevin.wangtao@hisilicon.com>, Leo Yan <leo.yan@linaro.org>, Jassi Brar <jaswinder.singh@linaro.org>, zhongkaihua@huawei.com, Dmitry Osipenko <digetx@gmail.com>, Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>, Stephen Boyd <sboyd@kernel.org>, linux-pm@vger.kernel.org, Vincent Guittot <vincent.guittot@linaro.org>, Rafael Wysocki <rjw@rjwysocki.net>, Sibi Sankar <sibis@codeaurora.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 03/13] opp: Keep track of currently programmed OPP Date: Mon, 12 Jul 2021 09:44:08 +0530 [thread overview] Message-ID: <20210712041408.5dgtwcp55pgt7twn@vireshk-i7> (raw) In-Reply-To: <20210709085704.GA10180@arm.com> On 09-07-21, 09:57, Ionela Voinescu wrote: > On Thursday 08 Jul 2021 at 13:23:53 (+0530), Viresh Kumar wrote: > > On 07-07-21, 11:24, Ionela Voinescu wrote: > > > Now comes the interesting part: what seems to fix it is a call to > > > clk_get_rate(opp_table->clk) in _set_opp(), which is what basically > > > happened before this patch, as _find_current_opp() was always called. > > > I do not need to do anything with the returned frequency. > > > > Wow, thanks for narrowing it down this far :) > > > > I had a quick look and this is what I think is the problem here. > > > > This platform uses mailbox API to send its frequency change requests to another > > processor. And the way it is written currently, I don't see any guarantee > > whatsoever which say > > > > "once clk_set_rate() returns, the frequency would have already changed". > > > > I think what was strange to me was that the frequency never seems to > change, there isn't just a delay in the new frequency taking effect, as > I would expect in these cases. Or if there is a delay, that's quite large > - at least a second. No idea on what the firmware is doing behind the scene :) > > And this may exactly be the thing you are able to hit, luckily because of this > > patchset :) > > > > As a quick way of checking if that is right or not, this may make it work: > > > > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c > > index 395ddc250828..9856c1c84dcf 100644 > > --- a/drivers/mailbox/hi3660-mailbox.c > > +++ b/drivers/mailbox/hi3660-mailbox.c > > @@ -201,6 +201,9 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > > > > /* Trigger data transferring */ > > writel(BIT(mchan->ack_irq), base + MBOX_SEND_REG); > > + > > + hi3660_mbox_check_state(chan); > > + > > I gave this a try an it does work for me. Good, so that kind of proves what I was suspecting. The mailbox driver looks buggy here. > > -------------------------8<------------------------- > > > > As a proper fix, something like this (not even compile tested) is required I > > believe as I don't see the clients would know if the transfer is over. Cc'ing > > mailbox guys to see what can be done. > > > > I'll give this a try as well when there is consensus. I might even try to > review it, if the time allows. Sure, lets see what the platform guys think about this first. Kevin, Kaihua ? -- viresh _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-12 4:14 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-21 11:17 [PATCH 00/13] opp: Implement dev_pm_opp_set_opp() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 01/13] opp: Rename _opp_set_rate_zero() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 02/13] opp: No need to check clk for errors Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 03/13] opp: Keep track of currently programmed OPP Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 21:41 ` Dmitry Osipenko 2021-01-21 21:41 ` Dmitry Osipenko 2021-01-22 4:45 ` Viresh Kumar 2021-01-22 4:45 ` Viresh Kumar 2021-01-22 14:31 ` Dmitry Osipenko 2021-01-22 14:31 ` Dmitry Osipenko 2021-01-25 3:12 ` Viresh Kumar 2021-01-25 3:12 ` Viresh Kumar 2021-01-27 16:31 ` Akhil P Oommen 2021-01-28 4:14 ` Viresh Kumar 2021-01-28 4:14 ` Viresh Kumar 2021-07-07 10:24 ` Ionela Voinescu 2021-07-07 10:24 ` Ionela Voinescu 2021-07-08 7:53 ` Viresh Kumar 2021-07-08 7:53 ` Viresh Kumar 2021-07-09 8:57 ` Ionela Voinescu 2021-07-09 8:57 ` Ionela Voinescu 2021-07-12 4:14 ` Viresh Kumar [this message] 2021-07-12 4:14 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 04/13] opp: Split _set_opp() out of dev_pm_opp_set_rate() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 05/13] opp: Allow _set_opp() to work for non-freq devices Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 06/13] opp: Allow _generic_set_opp_regulator() " Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 07/13] opp: Allow _generic_set_opp_clk_only() " Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 20:26 ` Dmitry Osipenko 2021-01-21 20:26 ` Dmitry Osipenko 2021-01-22 4:35 ` Viresh Kumar 2021-01-22 4:35 ` Viresh Kumar 2021-01-25 21:09 ` Dmitry Osipenko 2021-01-25 21:09 ` Dmitry Osipenko 2021-01-27 6:58 ` Viresh Kumar 2021-01-27 6:58 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 08/13] opp: Update parameters of _set_opp_custom() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 09/13] opp: Implement dev_pm_opp_set_opp() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 10/13] cpufreq: qcom: Migrate to dev_pm_opp_set_opp() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 11/13] devfreq: tegra30: " Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 21:36 ` Dmitry Osipenko 2021-01-21 21:36 ` Dmitry Osipenko 2021-01-22 6:26 ` Viresh Kumar 2021-01-22 6:26 ` Viresh Kumar 2021-01-22 15:28 ` Dmitry Osipenko 2021-01-22 15:28 ` Dmitry Osipenko 2021-01-25 3:14 ` Viresh Kumar 2021-01-25 3:14 ` Viresh Kumar 2021-01-25 16:00 ` Dmitry Osipenko 2021-01-25 16:00 ` Dmitry Osipenko 2021-01-27 9:10 ` [PATCH V2 " Viresh Kumar 2021-01-27 10:02 ` Viresh Kumar 2021-01-27 15:58 ` Dmitry Osipenko 2021-01-28 7:01 ` Viresh Kumar 2021-02-01 0:21 ` Chanwoo Choi 2021-02-01 19:57 ` Dmitry Osipenko 2021-01-21 11:17 ` [PATCH 12/13] drm: msm: " Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar 2021-01-21 11:17 ` [PATCH 13/13] opp: Remove dev_pm_opp_set_bw() Viresh Kumar 2021-01-21 11:17 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210712041408.5dgtwcp55pgt7twn@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=digetx@gmail.com \ --cc=ionela.voinescu@arm.com \ --cc=jaswinder.singh@linaro.org \ --cc=kevin.wangtao@hisilicon.com \ --cc=leo.yan@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=nm@ti.com \ --cc=rjw@rjwysocki.net \ --cc=sboyd@kernel.org \ --cc=sibis@codeaurora.org \ --cc=vincent.guittot@linaro.org \ --cc=vireshk@kernel.org \ --cc=zhongkaihua@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.