From: Ionela Voinescu <ionela.voinescu@arm.com> To: Viresh Kumar <viresh.kumar@linaro.org> 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: Fri, 9 Jul 2021 09:57:04 +0100 [thread overview] Message-ID: <20210709085704.GA10180@arm.com> (raw) In-Reply-To: <20210708075353.ivsuc4y47i6bbhgz@vireshk-i7> Hi Viresh, 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. > 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. > return 0; > } > > -------------------------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. Many thanks, Ionela. > diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c > index 3a653d54bee0..c1e62ea4cf01 100644 > --- a/drivers/clk/hisilicon/clk-hi3660-stub.c > +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c > @@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate, > stub_clk->msg[0], stub_clk->msg[1]); > > mbox_send_message(stub_clk_chan.mbox, stub_clk->msg); > - mbox_client_txdone(stub_clk_chan.mbox, 0); > > stub_clk->rate = rate; > return 0; > @@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev) > /* Use mailbox client without blocking */ > stub_clk_chan.cl.dev = dev; > stub_clk_chan.cl.tx_done = NULL; > - stub_clk_chan.cl.tx_block = false; > + stub_clk_chan.cl.tx_block = true; > stub_clk_chan.cl.knows_txdone = false; > > /* Allocate mailbox channel */ > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c > index 395ddc250828..8f6b787c0aba 100644 > --- a/drivers/mailbox/hi3660-mailbox.c > +++ b/drivers/mailbox/hi3660-mailbox.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2017-2018 HiSilicon Limited. > +// Copyright (c) 2017-2018 Hisilicon Limited. > // Copyright (c) 2017-2018 Linaro Limited. > > #include <linux/bitops.h> > @@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox) > return container_of(mbox, struct hi3660_mbox, controller); > } > > -static int hi3660_mbox_check_state(struct mbox_chan *chan) > +static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan) > { > unsigned long ch = (unsigned long)chan->con_priv; > struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > @@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan) > > /* Mailbox is ready to use */ > if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY) > - return 0; > + return true; > > /* Wait for acknowledge from remote */ > ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG, > val, (val & MBOX_STATE_ACK), 1000, 300000); > if (ret) { > dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__); > - return ret; > + return false; > } > > /* clear ack state, mailbox will get back to ready state */ > writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG); > > - return 0; > + return true; > } > > static int hi3660_mbox_unlock(struct mbox_chan *chan) > @@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > unsigned int i; > int ret; > > - ret = hi3660_mbox_check_state(chan); > - if (ret) > - return ret; > - > /* Clear mask for destination interrupt */ > writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG); > > @@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > static const struct mbox_chan_ops hi3660_mbox_ops = { > .startup = hi3660_mbox_startup, > .send_data = hi3660_mbox_send_data, > + .last_tx_done = hi3660_mbox_last_tx_done, > }; > > static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller, > @@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev) > mbox->controller.num_chans = MBOX_CHAN_MAX; > mbox->controller.ops = &hi3660_mbox_ops; > mbox->controller.of_xlate = hi3660_mbox_xlate; > + mbox->controller.txdone_poll = true; > > /* Initialize mailbox channel data */ > chan = mbox->chan; > > -- > viresh
WARNING: multiple messages have this Message-ID (diff)
From: Ionela Voinescu <ionela.voinescu@arm.com> To: Viresh Kumar <viresh.kumar@linaro.org> 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: Fri, 9 Jul 2021 09:57:04 +0100 [thread overview] Message-ID: <20210709085704.GA10180@arm.com> (raw) In-Reply-To: <20210708075353.ivsuc4y47i6bbhgz@vireshk-i7> Hi Viresh, 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. > 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. > return 0; > } > > -------------------------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. Many thanks, Ionela. > diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c > index 3a653d54bee0..c1e62ea4cf01 100644 > --- a/drivers/clk/hisilicon/clk-hi3660-stub.c > +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c > @@ -89,7 +89,6 @@ static int hi3660_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate, > stub_clk->msg[0], stub_clk->msg[1]); > > mbox_send_message(stub_clk_chan.mbox, stub_clk->msg); > - mbox_client_txdone(stub_clk_chan.mbox, 0); > > stub_clk->rate = rate; > return 0; > @@ -131,7 +130,7 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev) > /* Use mailbox client without blocking */ > stub_clk_chan.cl.dev = dev; > stub_clk_chan.cl.tx_done = NULL; > - stub_clk_chan.cl.tx_block = false; > + stub_clk_chan.cl.tx_block = true; > stub_clk_chan.cl.knows_txdone = false; > > /* Allocate mailbox channel */ > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c > index 395ddc250828..8f6b787c0aba 100644 > --- a/drivers/mailbox/hi3660-mailbox.c > +++ b/drivers/mailbox/hi3660-mailbox.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0 > -// Copyright (c) 2017-2018 HiSilicon Limited. > +// Copyright (c) 2017-2018 Hisilicon Limited. > // Copyright (c) 2017-2018 Linaro Limited. > > #include <linux/bitops.h> > @@ -83,7 +83,7 @@ static struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox) > return container_of(mbox, struct hi3660_mbox, controller); > } > > -static int hi3660_mbox_check_state(struct mbox_chan *chan) > +static bool hi3660_mbox_last_tx_done(struct mbox_chan *chan) > { > unsigned long ch = (unsigned long)chan->con_priv; > struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox); > @@ -94,20 +94,20 @@ static int hi3660_mbox_check_state(struct mbox_chan *chan) > > /* Mailbox is ready to use */ > if (readl(base + MBOX_MODE_REG) & MBOX_STATE_READY) > - return 0; > + return true; > > /* Wait for acknowledge from remote */ > ret = readx_poll_timeout_atomic(readl, base + MBOX_MODE_REG, > val, (val & MBOX_STATE_ACK), 1000, 300000); > if (ret) { > dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__); > - return ret; > + return false; > } > > /* clear ack state, mailbox will get back to ready state */ > writel(BIT(mchan->ack_irq), base + MBOX_ICLR_REG); > > - return 0; > + return true; > } > > static int hi3660_mbox_unlock(struct mbox_chan *chan) > @@ -182,10 +182,6 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > unsigned int i; > int ret; > > - ret = hi3660_mbox_check_state(chan); > - if (ret) > - return ret; > - > /* Clear mask for destination interrupt */ > writel_relaxed(~BIT(mchan->dst_irq), base + MBOX_IMASK_REG); > > @@ -207,6 +203,7 @@ static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg) > static const struct mbox_chan_ops hi3660_mbox_ops = { > .startup = hi3660_mbox_startup, > .send_data = hi3660_mbox_send_data, > + .last_tx_done = hi3660_mbox_last_tx_done, > }; > > static struct mbox_chan *hi3660_mbox_xlate(struct mbox_controller *controller, > @@ -259,6 +256,7 @@ static int hi3660_mbox_probe(struct platform_device *pdev) > mbox->controller.num_chans = MBOX_CHAN_MAX; > mbox->controller.ops = &hi3660_mbox_ops; > mbox->controller.of_xlate = hi3660_mbox_xlate; > + mbox->controller.txdone_poll = true; > > /* Initialize mailbox channel data */ > chan = mbox->chan; > > -- > 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-09 8:57 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 [this message] 2021-07-09 8:57 ` Ionela Voinescu 2021-07-12 4:14 ` Viresh Kumar 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=20210709085704.GA10180@arm.com \ --to=ionela.voinescu@arm.com \ --cc=digetx@gmail.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=viresh.kumar@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.