LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Maulik Shah <mkshah@codeaurora.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Evan Green <evgreen@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Andy Gross <agross@kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Lina Iyer <ilina@codeaurora.org>,
	lsrao@codeaurora.org, Taniya Das <tdas@codeaurora.org>,
	Odelu Kukatla <okukatla@codeaurora.org>,
	Kiran Gunda <kgunda@codeaurora.org>,
	Sibi Sankar <sibis@codeaurora.org>
Subject: Re: [PATCH v13 5/5] drivers: qcom: Update rpmh clients to use start and end transactions
Date: Mon, 9 Mar 2020 16:44:27 -0700
Message-ID: <CAD=FV=UugityQX+TG2c41dyaaCrhYe534UgXxm0G0igLz-9LSw@mail.gmail.com> (raw)
In-Reply-To: <1583746236-13325-6-git-send-email-mkshah@codeaurora.org>

Hi,

On Mon, Mar 9, 2020 at 2:31 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Update all rpmh clients to start using rpmh_start_transaction() and
> rpmh_end_transaction().
>
> Cc: Taniya Das <tdas@codeaurora.org>
> Cc: Odelu Kukatla <okukatla@codeaurora.org>
> Cc: Kiran Gunda <kgunda@codeaurora.org>
> Cc: Sibi Sankar <sibis@codeaurora.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-rpmh.c             | 21 ++++++++++++++-------
>  drivers/interconnect/qcom/bcm-voter.c   | 13 +++++++++----
>  drivers/regulator/qcom-rpmh-regulator.c |  4 ++++
>  drivers/soc/qcom/rpmhpd.c               | 11 +++++++++--

This needs to be 4 separate patches since the change to each subsystem
will go through a different maintainer.

Also: it'll be a lot easier to land this if you make the new
rpmh_start_transaction() and rpmh_end_transaction() calls _optional_
for now, especially since they are just a speed optimization and not
for correctness.  That is, if a driver makes a call to rpmh_write(),
rpmh_write_async(), rpmh_write_batch(), or rpmh_invalidate() without
doing rpmh_start_transaction() then it should still work--just flush
right away.  Since you have rpmh_start_transaction() refcounted that's
as simple as making a call to rpmh_start_transaction() at the
beginning of all public calls and rpmh_end_transaction() at the end.
If there was already a refcount then no harm done.  If there wasn't
you'll get a flush at the end.

Once you make the call optional, you can actually leave changing the
callers until after your series lands.  Then you don't end up
bothering all the other maintainers with the back-and-forth.

Once all callers are updated you can make the call required.  ...or
(as noted below) maybe we should just keep it optional...

One last note here: you have a regulator change here but aren't
sending it to the regulator maintainer.  That won't work.  You also
have an interconnect change without sending it to the interconnect
maintainer.


>  4 files changed, 36 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 12bd871..16f68d4 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -154,22 +154,27 @@ static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>         cmd_state = c->aggr_state;
>         on_val = c->res_on_val;
>
> +       rpmh_start_transaction(c->dev);
> +
>         for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
>                 if (has_state_changed(c, state)) {
>                         if (cmd_state & BIT(state))
>                                 cmd.data = on_val;
>
>                         ret = rpmh_write_async(c->dev, state, &cmd, 1);
> -                       if (ret) {
> -                               dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> -                                       !state ? "sleep" :
> -                                       state == RPMH_WAKE_ONLY_STATE   ?
> -                                       "wake" : "active", c->res_name, ret);
> -                               return ret;
> -                       }
> +                       if (ret)
> +                               break;
>                 }
>         }
>
> +       ret |= rpmh_end_transaction(c->dev);

You can't do this.  "ret" is an integer and you're munging two error
codes into one int.  I don't think there is any clever way to do this,
but probably this would be fine (the compiler should optimize):

if (ret)
  rpmh_end_transaction(c->dev);
else
  ret = rpmh_end_transaction(c->dev);

...or just leave the "dev_err" and "return ret" where they were and
call rpmh_end_transaction() above without looking at the return value.


> +       if (ret) {
> +               dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> +                       !state ? "sleep" : state == RPMH_WAKE_ONLY_STATE ?
> +                       "wake" : "active", c->res_name, ret);
> +               return ret;
> +       }

Technically the error message above is now misleading if the
"end_transaction" failed.  Namely it will blame things on the active
only state whereas that wasn't the problem.


> +
>         c->last_sent_aggr_state = c->aggr_state;
>         c->peer->last_sent_aggr_state =  c->last_sent_aggr_state;
>
> @@ -267,7 +272,9 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>         cmd.addr = c->res_addr;
>         cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
>
> +       rpmh_start_transaction(c->dev);
>         ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
> +       ret |= rpmh_end_transaction(c->dev);

Again, no |=

Also: one argument for keeping start_transaction and end_transaction
optional long term is that you could completely eliminate this change.


>         if (ret) {
>                 dev_err(c->dev, "set active state of %s failed: (%d)\n",
>                         c->res_name, ret);
> diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
> index 2adfde8..fbe18b2 100644
> --- a/drivers/interconnect/qcom/bcm-voter.c
> +++ b/drivers/interconnect/qcom/bcm-voter.c
> @@ -263,7 +263,9 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
>         tcs_list_gen(&voter->commit_list, QCOM_ICC_BUCKET_AMC, cmds, commit_idx);
>
>         if (!commit_idx[0])
> -               goto out;
> +               goto end;
> +
> +       rpmh_start_transaction(voter-dev);
>
>         ret = rpmh_invalidate(voter->dev);
>         if (ret) {
> @@ -312,12 +314,15 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
>         tcs_list_gen(&voter->commit_list, QCOM_ICC_BUCKET_SLEEP, cmds, commit_idx);
>
>         ret = rpmh_write_batch(voter->dev, RPMH_SLEEP_STATE, cmds, commit_idx);
> -       if (ret) {
> +       if (ret)
>                 pr_err("Error sending SLEEP RPMH requests (%d)\n", ret);
> -               goto out;
> -       }
>
>  out:
> +       ret = rpmh_end_transaction(voter-dev);
> +       if (ret)
> +               pr_err("Error ending rpmh transaction (%d)\n", ret);
> +
> +end:

Personally I don't think "out" and "end" are very descriptive.  My own
favorite is to name these types of labels based on what has been done
so far.  So:

exit_started_rpmh_transaction:
exit_constructed_list:


>         list_for_each_entry_safe(bcm, bcm_tmp, &voter->commit_list, list)
>                 list_del_init(&bcm->list);
>
> diff --git a/drivers/regulator/qcom-rpmh-regulator.c b/drivers/regulator/qcom-rpmh-regulator.c
> index c86ad40..f4b9176 100644
> --- a/drivers/regulator/qcom-rpmh-regulator.c
> +++ b/drivers/regulator/qcom-rpmh-regulator.c
> @@ -163,12 +163,16 @@ static int rpmh_regulator_send_request(struct rpmh_vreg *vreg,
>  {
>         int ret;
>
> +       rpmh_start_transaction(vreg->dev);
> +
>         if (wait_for_ack || vreg->always_wait_for_ack)
>                 ret = rpmh_write(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd, 1);
>         else
>                 ret = rpmh_write_async(vreg->dev, RPMH_ACTIVE_ONLY_STATE, cmd,
>                                         1);
>
> +       ret |= rpmh_end_transaction(vreg->dev);

Again, no |=.

...and again, if starting/ending was optional you wouldn't need this change.


> +
>         return ret;
>  }
>
> diff --git a/drivers/soc/qcom/rpmhpd.c b/drivers/soc/qcom/rpmhpd.c
> index 4d264d0..0e9d204 100644
> --- a/drivers/soc/qcom/rpmhpd.c
> +++ b/drivers/soc/qcom/rpmhpd.c
> @@ -193,19 +193,26 @@ static const struct of_device_id rpmhpd_match_table[] = {
>  static int rpmhpd_send_corner(struct rpmhpd *pd, int state,
>                               unsigned int corner, bool sync)
>  {
> +       int ret;
>         struct tcs_cmd cmd = {
>                 .addr = pd->addr,
>                 .data = corner,
>         };
>
> +       rpmh_start_transaction(pd->dev);
> +
>         /*
>          * Wait for an ack only when we are increasing the
>          * perf state of the power domain
>          */
>         if (sync)
> -               return rpmh_write(pd->dev, state, &cmd, 1);
> +               ret = rpmh_write(pd->dev, state, &cmd, 1);
>         else
> -               return rpmh_write_async(pd->dev, state, &cmd, 1);
> +               ret = rpmh_write_async(pd->dev, state, &cmd, 1);
> +
> +       ret |= rpmh_end_transaction(pd->dev);

Again, no |=.

...and again, if starting/ending was optional you wouldn't need this change.



-Doug

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09  9:30 [PATCH v13 0/4] Invoke rpmh_flush for non OSI targets Maulik Shah
2020-03-09  9:30 ` [PATCH v13 1/5] arm64: dts: qcom: sc7180: Add cpuidle low power states Maulik Shah
2020-03-09  9:30 ` [PATCH v13 2/5] soc: qcom: rpmh: Update dirty flag only when data changes Maulik Shah
2020-03-09 23:42   ` Doug Anderson
2020-03-13  8:53     ` Maulik Shah
2020-03-09  9:30 ` [PATCH v13 3/5] soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new data Maulik Shah
2020-03-09 23:42   ` Doug Anderson
2020-03-10 11:09     ` Maulik Shah
2020-03-09  9:30 ` [PATCH v13 4/5] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches Maulik Shah
2020-03-09 23:43   ` Doug Anderson
2020-03-10 11:19     ` Maulik Shah
2020-03-10 15:46       ` Doug Anderson
2020-03-11  6:36         ` Maulik Shah
2020-03-11 23:06           ` Doug Anderson
     [not found]             ` <5a5274ac-41f4-b06d-ff49-c00cef67aa7f@codeaurora.org>
2020-03-12 15:11               ` Doug Anderson
2020-03-25 17:15                 ` Maulik Shah
2020-03-26 18:08                   ` Doug Anderson
2020-03-09  9:30 ` [PATCH v13 5/5] drivers: qcom: Update rpmh clients to use start and end transactions Maulik Shah
2020-03-09 23:44   ` Doug Anderson [this message]
2020-03-10 11:46     ` Maulik Shah
2020-03-10 15:46       ` Doug Anderson
2020-03-11 23:02   ` Doug Anderson
2020-03-09 23:42 ` [PATCH v13 0/4] Invoke rpmh_flush for non OSI targets Doug Anderson
2020-03-10 11:49   ` Maulik Shah

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='CAD=FV=UugityQX+TG2c41dyaaCrhYe534UgXxm0G0igLz-9LSw@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=kgunda@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lsrao@codeaurora.org \
    --cc=mka@chromium.org \
    --cc=mkshah@codeaurora.org \
    --cc=okukatla@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --cc=sibis@codeaurora.org \
    --cc=swboyd@chromium.org \
    --cc=tdas@codeaurora.org \
    /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: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git