diff mbox series

[v13,5/5] drivers: qcom: Update rpmh clients to use start and end transactions

Message ID 1583746236-13325-6-git-send-email-mkshah@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Invoke rpmh_flush for non OSI targets | expand

Commit Message

Maulik Shah March 9, 2020, 9:30 a.m. UTC
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 +++++++++--
 4 files changed, 36 insertions(+), 13 deletions(-)

Comments

Doug Anderson March 9, 2020, 11:44 p.m. UTC | #1
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
Maulik Shah March 10, 2020, 11:46 a.m. UTC | #2
On 3/10/2020 5:14 AM, Doug Anderson wrote:
> 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.
I will split to 4 changes, and send each one to its respective mailing lists and maintainer/reviewer.
>
> 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

yes, this is already taken care.

All the calls from driver will go through as it is and won't fail even without calling new APIs.
So they are already optional.

The comment in rpmh_start_transaction() is already saying if client "choose" to invoke this
then this must be ended by calling rpmh_end_transaction(). if client don't invoke
rpmh_start_transaction() in the first place then everything is expected work as if no change.


> --just flush
> right away.  

No, currently also in driver no one is calling rpmh_flush().

so nothing breaks with series and no point in adding changes to flush right away and then remove them in same series.

when the clients starts invoking new APIs, rpmh_flush() will start getting invoked for the first time in driver.

> 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.

We don't need to end up syncing up with all other maintainers. the calls are already optional.

These new changes (as they be split in to 4) can go in any order in various maintainer's trees,
once this series goes in rpmh driver.

>
> 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.
Done.
>
>
>>  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.
Done.
>
>
>> +       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.
>
Done.
>> +
>>         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 |=
Done.
>
> Also: one argument for keeping start_transaction and end_transaction
> optional long term is that you could completely eliminate this change.
its already optional as described above.
>
>>         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:
>
Done.
>>         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 |=.
Done.
> ...and again, if starting/ending was optional you wouldn't need this change.
>
its already optional as described above.
>> +
>>         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 |=.
Done.
>
> ...and again, if starting/ending was optional you wouldn't need this change.

its already optional as described above.

Thanks,
Maulik

>
>
> -Doug
Doug Anderson March 10, 2020, 3:46 p.m. UTC | #3
Hi,

On Tue, Mar 10, 2020 at 4:47 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
>
> On 3/10/2020 5:14 AM, Doug Anderson wrote:
> > 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.
> I will split to 4 changes, and send each one to its respective mailing lists and maintainer/reviewer.
> >
> > 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
>
> yes, this is already taken care.
>
> All the calls from driver will go through as it is and won't fail even without calling new APIs.
> So they are already optional.
>
> The comment in rpmh_start_transaction() is already saying if client "choose" to invoke this
> then this must be ended by calling rpmh_end_transaction(). if client don't invoke
> rpmh_start_transaction() in the first place then everything is expected work as if no change.

I think you may have misunderstood.  With your v13 it's not optional
because the flush won't happen unless the clients call
rpmh_start_transaction() and rpmh_end_transaction().  ...but the flush
is necessary for correctness, right?


> > --just flush
> > right away.
>
> No, currently also in driver no one is calling rpmh_flush().
>
> so nothing breaks with series and no point in adding changes to flush right away and then remove them in same series.
>
> when the clients starts invoking new APIs, rpmh_flush() will start getting invoked for the first time in driver.

I'm not saying to expose flush to clients.  I'm saying that you should
modify the implementation of the calls in rpmh.c.  AKA in rpmh.c you
have:

rpmh_write():
  start_transaction()
  ... the contents of rpmh_write() from your v13 ...
  end_transaction()

rpmh_write_async():
  start_transaction()
  ... the contents of rpmh_write_async() from your v13 ...
  end_transaction()

rpmh_write_batch()
  start_transaction()
  ... the contents of rpmh_write_batch() from your v13 ...
  end_transaction()

rpmh_invalidate()
  start_transaction()
  ... the contents of rpmh_invalidate() from your v13 ...
  end_transaction()


If a client calls rpmh_write() without anything else, they will get an
implicit flush.

If a client does this:

start_transaction()
rpmh_invalidate()
rpmh_write_batch()
rpmh_write_batch()
end_transaction()

That translates to:

start_transaction()
  start_transaction()
  ... the contents of rpmh_invalidate() from your v13 ...
  end_transaction()
  start_transaction()
  ... the contents of rpmh_write_batch() from your v13 ...
  end_transaction()
  start_transaction()
  ... the contents of rpmh_write_batch() from your v13 ...
  end_transaction()
end_transaction()

...then you'll get one flush at the end.  That's because the
start_transaction() embedded in rpmh_invalidate() and
rpmh_write_batch() will increase the usage count to 2 and then
decrease back to 1.  Since it won't be 0 you won't get flushes from
the embedded start/end.  You'll just get one flush at the end.


Now start_transaction() and end_transaction() are truly optional.  If
a client doesn't call them they'll get an implicit flush at the end of
every call.  If they do call start/end themselves they can postpone
the flush till the true end.

...and because it's truly optional, you can drop several of the
changes in your series.



-Doug
Doug Anderson March 11, 2020, 11:02 p.m. UTC | #4
Hi,

On Mon, Mar 9, 2020 at 2:31 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> --- 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);

One last note is that your code doesn't actually compile for me since
you need "voter->dev" here (and in the start case), not "voter-dev".

-Doug
diff mbox series

Patch

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);
+	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;
+	}
+
 	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);
 	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:
 	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);
+
 	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);
+
+	return ret;
 }
 
 static void to_active_sleep(struct rpmhpd *pd, unsigned int corner,