diff mbox series

[V2,1/4] drivers: qcom: rpmh-rsc: simplify TCS locking

Message ID 20190722215340.3071-1-ilina@codeaurora.org (mailing list archive)
State Not Applicable, archived
Headers show
Series [V2,1/4] drivers: qcom: rpmh-rsc: simplify TCS locking | expand

Commit Message

Lina Iyer July 22, 2019, 9:53 p.m. UTC
From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>

The tcs->lock was introduced to serialize access with in TCS group. But,
drv->lock is still needed to synchronize core aspects of the
communication. This puts the drv->lock in the critical and high latency
path of sending a request. drv->lock provides the all necessary
synchronization. So remove locking around TCS group and simply use the
drv->lock instead.

Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
[ilina: split patch into multiple files, update commit text]
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v2:
    - Split the patches into multiple
	- Optimzation to remove reundant TCS access
	- Split the rpmh library changes into its own patch
	- Remove locks in IRQ handler
    - Update commit text
    - Remove fixes in commit text
---
 drivers/soc/qcom/rpmh-internal.h |  2 --
 drivers/soc/qcom/rpmh-rsc.c      | 32 ++++++++++++--------------------
 2 files changed, 12 insertions(+), 22 deletions(-)

Comments

Stephen Boyd July 23, 2019, 6:22 p.m. UTC | #1
Quoting Lina Iyer (2019-07-22 14:53:37)
> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
> 
> The tcs->lock was introduced to serialize access with in TCS group. But,
> drv->lock is still needed to synchronize core aspects of the
> communication. This puts the drv->lock in the critical and high latency
> path of sending a request. drv->lock provides the all necessary
> synchronization. So remove locking around TCS group and simply use the
> drv->lock instead.

This doesn't talk about removing the irq saving and restoring though.
Can you keep irq saving and restoring in this patch and then remove that
in the next patch with reasoning? It probably isn't safe if the lock is
taken in interrupt context anyway.

> 
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> [ilina: split patch into multiple files, update commit text]
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index a7bbbb67991c..969d5030860e 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..5ede8d6de3ad 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
>  {
>         int m;
>         struct tcs_group *tcs;
> +       int ret = 0;
>  
>         tcs = get_tcs_of_type(drv, type);
>  
> -       spin_lock(&tcs->lock);
> -       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> -               spin_unlock(&tcs->lock);
> -               return 0;
> -       }
> +       spin_lock(&drv->lock);
> +       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
> +               goto done_invalidate;
>  
>         for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>                 if (!tcs_is_free(drv, m)) {
> -                       spin_unlock(&tcs->lock);
> -                       return -EAGAIN;
> +                       ret = -EAGAIN;
> +                       goto done_invalidate;
>                 }
>                 write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
>                 write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
>         }
>         bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> -       spin_unlock(&tcs->lock);
>  
> +done_invalidate:
> +       spin_unlock(&drv->lock);
>         return 0;

return ret now?

>  }
>  
> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>  {
>         struct tcs_group *tcs;
>         int tcs_id;
> -       unsigned long flags;
>         int ret;
>  
>         tcs = get_tcs_for_msg(drv, msg);
>         if (IS_ERR(tcs))
>                 return PTR_ERR(tcs);
>  
> -       spin_lock_irqsave(&tcs->lock, flags);
>         spin_lock(&drv->lock);
>         /*
>          * The h/w does not like if we send a request to the same address,
>          * when one is already in-flight or being processed.
>          */
>         ret = check_for_req_inflight(drv, tcs, msg);
> -       if (ret) {
> -               spin_unlock(&drv->lock);
> +       if (ret)
>                 goto done_write;
> -       }
>  
>         tcs_id = find_free_tcs(tcs);
>         if (tcs_id < 0) {
>                 ret = tcs_id;
> -               spin_unlock(&drv->lock);
>                 goto done_write;
>         }
>  
>         tcs->req[tcs_id - tcs->offset] = msg;
>         set_bit(tcs_id, drv->tcs_in_use);
> -       spin_unlock(&drv->lock);
>  
>         __tcs_buffer_write(drv, tcs_id, 0, msg);
>         __tcs_trigger(drv, tcs_id);
>  
>  done_write:
> -       spin_unlock_irqrestore(&tcs->lock, flags);
> +       spin_unlock(&drv->lock);
>         return ret;
>  }
>  
> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>  {
>         struct tcs_group *tcs;
>         int tcs_id = 0, cmd_id = 0;
> -       unsigned long flags;
>         int ret;
>  
>         tcs = get_tcs_for_msg(drv, msg);
>         if (IS_ERR(tcs))
>                 return PTR_ERR(tcs);
>  
> -       spin_lock_irqsave(&tcs->lock, flags);
> +       spin_lock(&drv->lock);
>         /* find the TCS id and the command in the TCS to write to */
>         ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
>         if (!ret)
>                 __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> -       spin_unlock_irqrestore(&tcs->lock, flags);
> +       spin_unlock(&drv->lock);
>  

These ones, just leave them doing the irq save restore for now?
Lina Iyer July 23, 2019, 7:21 p.m. UTC | #2
On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-07-22 14:53:37)
>> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
>>
>> The tcs->lock was introduced to serialize access with in TCS group. But,
>> drv->lock is still needed to synchronize core aspects of the
>> communication. This puts the drv->lock in the critical and high latency
>> path of sending a request. drv->lock provides the all necessary
>> synchronization. So remove locking around TCS group and simply use the
>> drv->lock instead.
>
>This doesn't talk about removing the irq saving and restoring though.
You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
we were only removing the tcs->lock.

>Can you keep irq saving and restoring in this patch and then remove that
>in the next patch with reasoning? It probably isn't safe if the lock is
>taken in interrupt context anyway.
>
Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
been changed by this patch.

>>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
>> [ilina: split patch into multiple files, update commit text]
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>
>> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>> index a7bbbb67991c..969d5030860e 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index e278fc11fe5c..5ede8d6de3ad 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
>>  {
>>         int m;
>>         struct tcs_group *tcs;
>> +       int ret = 0;
>>
>>         tcs = get_tcs_of_type(drv, type);
>>
>> -       spin_lock(&tcs->lock);
>> -       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>> -               spin_unlock(&tcs->lock);
>> -               return 0;
>> -       }
>> +       spin_lock(&drv->lock);
>> +       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
>> +               goto done_invalidate;
>>
>>         for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>>                 if (!tcs_is_free(drv, m)) {
>> -                       spin_unlock(&tcs->lock);
>> -                       return -EAGAIN;
>> +                       ret = -EAGAIN;
>> +                       goto done_invalidate;
>>                 }
>>                 write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
>>                 write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
>>         }
>>         bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
>> -       spin_unlock(&tcs->lock);
>>
>> +done_invalidate:
>> +       spin_unlock(&drv->lock);
>>         return 0;
>
>return ret now?
>
Yes, will do.
>>  }
>>
>> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>>  {
>>         struct tcs_group *tcs;
>>         int tcs_id;
>> -       unsigned long flags;
>>         int ret;
>>
>>         tcs = get_tcs_for_msg(drv, msg);
>>         if (IS_ERR(tcs))
>>                 return PTR_ERR(tcs);
>>
>> -       spin_lock_irqsave(&tcs->lock, flags);
>>         spin_lock(&drv->lock);
>>         /*
>>          * The h/w does not like if we send a request to the same address,
>>          * when one is already in-flight or being processed.
>>          */
>>         ret = check_for_req_inflight(drv, tcs, msg);
>> -       if (ret) {
>> -               spin_unlock(&drv->lock);
>> +       if (ret)
>>                 goto done_write;
>> -       }
>>
>>         tcs_id = find_free_tcs(tcs);
>>         if (tcs_id < 0) {
>>                 ret = tcs_id;
>> -               spin_unlock(&drv->lock);
>>                 goto done_write;
>>         }
>>
>>         tcs->req[tcs_id - tcs->offset] = msg;
>>         set_bit(tcs_id, drv->tcs_in_use);
>> -       spin_unlock(&drv->lock);
>>
>>         __tcs_buffer_write(drv, tcs_id, 0, msg);
>>         __tcs_trigger(drv, tcs_id);
>>
>>  done_write:
>> -       spin_unlock_irqrestore(&tcs->lock, flags);
>> +       spin_unlock(&drv->lock);
>>         return ret;
>>  }
>>
>> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>>  {
>>         struct tcs_group *tcs;
>>         int tcs_id = 0, cmd_id = 0;
>> -       unsigned long flags;
>>         int ret;
>>
>>         tcs = get_tcs_for_msg(drv, msg);
>>         if (IS_ERR(tcs))
>>                 return PTR_ERR(tcs);
>>
>> -       spin_lock_irqsave(&tcs->lock, flags);
>> +       spin_lock(&drv->lock);
>>         /* find the TCS id and the command in the TCS to write to */
>>         ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
>>         if (!ret)
>>                 __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
>> -       spin_unlock_irqrestore(&tcs->lock, flags);
>> +       spin_unlock(&drv->lock);
>>
>
>These ones, just leave them doing the irq save restore for now?
>
drv->lock ??

--Lina
Stephen Boyd July 23, 2019, 8:18 p.m. UTC | #3
Quoting Lina Iyer (2019-07-23 12:21:59)
> On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2019-07-22 14:53:37)
> >> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
> >>
> >> The tcs->lock was introduced to serialize access with in TCS group. But,
> >> drv->lock is still needed to synchronize core aspects of the
> >> communication. This puts the drv->lock in the critical and high latency
> >> path of sending a request. drv->lock provides the all necessary
> >> synchronization. So remove locking around TCS group and simply use the
> >> drv->lock instead.
> >
> >This doesn't talk about removing the irq saving and restoring though.
> You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
> we were only removing the tcs->lock.

Yes drv->lock wasn't an irqsave/restore variant because it was a
spinlock inside of an obviously already irqsaved region of code because
the tcs->lock was outside the drv->lock and that was saving the irq
flags.

> 
> >Can you keep irq saving and restoring in this patch and then remove that
> >in the next patch with reasoning? It probably isn't safe if the lock is
> >taken in interrupt context anyway.
> >
> Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
> been changed by this patch.

It needs to be changed to maintain the irqsaving/restoring of the code.

> >> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >>  {
> >>         struct tcs_group *tcs;
> >>         int tcs_id;
> >> -       unsigned long flags;
> >>         int ret;
> >>
> >>         tcs = get_tcs_for_msg(drv, msg);
> >>         if (IS_ERR(tcs))
> >>                 return PTR_ERR(tcs);
> >>
> >> -       spin_lock_irqsave(&tcs->lock, flags);
> >>         spin_lock(&drv->lock);
> >>         /*
> >>          * The h/w does not like if we send a request to the same address,
> >>          * when one is already in-flight or being processed.
> >>          */
> >>         ret = check_for_req_inflight(drv, tcs, msg);
> >> -       if (ret) {
> >> -               spin_unlock(&drv->lock);
> >> +       if (ret)
> >>                 goto done_write;
> >> -       }
> >>
> >>         tcs_id = find_free_tcs(tcs);
> >>         if (tcs_id < 0) {
> >>                 ret = tcs_id;
> >> -               spin_unlock(&drv->lock);
> >>                 goto done_write;
> >>         }
> >>
> >>         tcs->req[tcs_id - tcs->offset] = msg;
> >>         set_bit(tcs_id, drv->tcs_in_use);
> >> -       spin_unlock(&drv->lock);
> >>
> >>         __tcs_buffer_write(drv, tcs_id, 0, msg);
> >>         __tcs_trigger(drv, tcs_id);
> >>
> >>  done_write:
> >> -       spin_unlock_irqrestore(&tcs->lock, flags);
> >> +       spin_unlock(&drv->lock);
> >>         return ret;
> >>  }
> >>
> >> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >>  {
> >>         struct tcs_group *tcs;
> >>         int tcs_id = 0, cmd_id = 0;
> >> -       unsigned long flags;
> >>         int ret;
> >>
> >>         tcs = get_tcs_for_msg(drv, msg);
> >>         if (IS_ERR(tcs))
> >>                 return PTR_ERR(tcs);
> >>
> >> -       spin_lock_irqsave(&tcs->lock, flags);
> >> +       spin_lock(&drv->lock);
> >>         /* find the TCS id and the command in the TCS to write to */
> >>         ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
> >>         if (!ret)
> >>                 __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> >> -       spin_unlock_irqrestore(&tcs->lock, flags);
> >> +       spin_unlock(&drv->lock);
> >>
> >
> >These ones, just leave them doing the irq save restore for now?
> >
> drv->lock ??
> 

Yes, it should have irq save/restore still.
Lina Iyer July 24, 2019, 2:54 p.m. UTC | #4
On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-07-23 12:21:59)
>> On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2019-07-22 14:53:37)
>> >> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
>> >>
>> >> The tcs->lock was introduced to serialize access with in TCS group. But,
>> >> drv->lock is still needed to synchronize core aspects of the
>> >> communication. This puts the drv->lock in the critical and high latency
>> >> path of sending a request. drv->lock provides the all necessary
>> >> synchronization. So remove locking around TCS group and simply use the
>> >> drv->lock instead.
>> >
>> >This doesn't talk about removing the irq saving and restoring though.
>> You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
>> we were only removing the tcs->lock.
>
>Yes drv->lock wasn't an irqsave/restore variant because it was a
>spinlock inside of an obviously already irqsaved region of code because
>the tcs->lock was outside the drv->lock and that was saving the irq
>flags.
>
Oh, right.
>>
>> >Can you keep irq saving and restoring in this patch and then remove that
>> >in the next patch with reasoning? It probably isn't safe if the lock is
>> >taken in interrupt context anyway.
>> >
>> Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
>> been changed by this patch.
>
>It needs to be changed to maintain the irqsaving/restoring of the code.
>
May be I should club this with the following patch. Instead of adding
irqsave and restore to drv->lock and then remvoing them again in the
following patch.

>> >> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>> >>  {
>> >>         struct tcs_group *tcs;
>> >>         int tcs_id;
>> >> -       unsigned long flags;
>> >>         int ret;
>> >>
>> >>         tcs = get_tcs_for_msg(drv, msg);
>> >>         if (IS_ERR(tcs))
>> >>                 return PTR_ERR(tcs);
>> >>
>> >> -       spin_lock_irqsave(&tcs->lock, flags);
>> >>         spin_lock(&drv->lock);
>> >>         /*
>> >>          * The h/w does not like if we send a request to the same address,
>> >>          * when one is already in-flight or being processed.
>> >>          */
>> >>         ret = check_for_req_inflight(drv, tcs, msg);
>> >> -       if (ret) {
>> >> -               spin_unlock(&drv->lock);
>> >> +       if (ret)
>> >>                 goto done_write;
>> >> -       }
>> >>
>> >>         tcs_id = find_free_tcs(tcs);
>> >>         if (tcs_id < 0) {
>> >>                 ret = tcs_id;
>> >> -               spin_unlock(&drv->lock);
>> >>                 goto done_write;
>> >>         }
>> >>
>> >>         tcs->req[tcs_id - tcs->offset] = msg;
>> >>         set_bit(tcs_id, drv->tcs_in_use);
>> >> -       spin_unlock(&drv->lock);
>> >>
>> >>         __tcs_buffer_write(drv, tcs_id, 0, msg);
>> >>         __tcs_trigger(drv, tcs_id);
>> >>
>> >>  done_write:
>> >> -       spin_unlock_irqrestore(&tcs->lock, flags);
>> >> +       spin_unlock(&drv->lock);
>> >>         return ret;
>> >>  }
>> >>
>> >> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>> >>  {
>> >>         struct tcs_group *tcs;
>> >>         int tcs_id = 0, cmd_id = 0;
>> >> -       unsigned long flags;
>> >>         int ret;
>> >>
>> >>         tcs = get_tcs_for_msg(drv, msg);
>> >>         if (IS_ERR(tcs))
>> >>                 return PTR_ERR(tcs);
>> >>
>> >> -       spin_lock_irqsave(&tcs->lock, flags);
>> >> +       spin_lock(&drv->lock);
>> >>         /* find the TCS id and the command in the TCS to write to */
>> >>         ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
>> >>         if (!ret)
>> >>                 __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
>> >> -       spin_unlock_irqrestore(&tcs->lock, flags);
>> >> +       spin_unlock(&drv->lock);
>> >>
>> >
>> >These ones, just leave them doing the irq save restore for now?
>> >
>> drv->lock ??
>>
>
>Yes, it should have irq save/restore still.
>
Stephen Boyd July 24, 2019, 6:32 p.m. UTC | #5
Quoting Lina Iyer (2019-07-24 07:54:52)
> On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2019-07-23 12:21:59)
> >> On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
> >> >Can you keep irq saving and restoring in this patch and then remove that
> >> >in the next patch with reasoning? It probably isn't safe if the lock is
> >> >taken in interrupt context anyway.
> >> >
> >> Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
> >> been changed by this patch.
> >
> >It needs to be changed to maintain the irqsaving/restoring of the code.
> >
> May be I should club this with the following patch. Instead of adding
> irqsave and restore to drv->lock and then remvoing them again in the
> following patch.
> 

I suspect that gets us back to v1 of this patch series? I'd prefer you
just keep the save/restore of irqs in this patch and then remove them
later. Or if the order can be the other way, where we remove grabbing
the lock in irq context comes first and then consolidate the locks into
one it might work.
Lina Iyer July 24, 2019, 7:36 p.m. UTC | #6
On Wed, Jul 24 2019 at 12:32 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-07-24 07:54:52)
>> On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2019-07-23 12:21:59)
>> >> On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
>> >> >Can you keep irq saving and restoring in this patch and then remove that
>> >> >in the next patch with reasoning? It probably isn't safe if the lock is
>> >> >taken in interrupt context anyway.
>> >> >
>> >> Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
>> >> been changed by this patch.
>> >
>> >It needs to be changed to maintain the irqsaving/restoring of the code.
>> >
>> May be I should club this with the following patch. Instead of adding
>> irqsave and restore to drv->lock and then remvoing them again in the
>> following patch.
>>
>
>I suspect that gets us back to v1 of this patch series? I'd prefer you
>just keep the save/restore of irqs in this patch and then remove them
>later. Or if the order can be the other way, where we remove grabbing
>the lock in irq context comes first and then consolidate the locks into
>one it might work.
>
Patches 1 and 3 need not be bundled. We can keep them separate to help
understand the change better.
This patch order - #2, #1, #3, #4 would work.

--Lina
diff mbox series

Patch

diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a7bbbb67991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -28,7 +28,6 @@  struct rsc_drv;
  * @offset:    start of the TCS group relative to the TCSes in the RSC
  * @num_tcs:   number of TCSes in this type
  * @ncpt:      number of commands in each TCS
- * @lock:      lock for synchronizing this TCS writes
  * @req:       requests that are sent from the TCS
  * @cmd_cache: flattened cache of cmds in sleep/wake TCS
  * @slots:     indicates which of @cmd_addr are occupied
@@ -40,7 +39,6 @@  struct tcs_group {
 	u32 offset;
 	int num_tcs;
 	int ncpt;
-	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
 	u32 *cmd_cache;
 	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..5ede8d6de3ad 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -106,26 +106,26 @@  static int tcs_invalidate(struct rsc_drv *drv, int type)
 {
 	int m;
 	struct tcs_group *tcs;
+	int ret = 0;
 
 	tcs = get_tcs_of_type(drv, type);
 
-	spin_lock(&tcs->lock);
-	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
-		spin_unlock(&tcs->lock);
-		return 0;
-	}
+	spin_lock(&drv->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+		goto done_invalidate;
 
 	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
 		if (!tcs_is_free(drv, m)) {
-			spin_unlock(&tcs->lock);
-			return -EAGAIN;
+			ret = -EAGAIN;
+			goto done_invalidate;
 		}
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
 		write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
 	}
 	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
-	spin_unlock(&tcs->lock);
 
+done_invalidate:
+	spin_unlock(&drv->lock);
 	return 0;
 }
 
@@ -349,41 +349,35 @@  static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 {
 	struct tcs_group *tcs;
 	int tcs_id;
-	unsigned long flags;
 	int ret;
 
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);
 
-	spin_lock_irqsave(&tcs->lock, flags);
 	spin_lock(&drv->lock);
 	/*
 	 * The h/w does not like if we send a request to the same address,
 	 * when one is already in-flight or being processed.
 	 */
 	ret = check_for_req_inflight(drv, tcs, msg);
-	if (ret) {
-		spin_unlock(&drv->lock);
+	if (ret)
 		goto done_write;
-	}
 
 	tcs_id = find_free_tcs(tcs);
 	if (tcs_id < 0) {
 		ret = tcs_id;
-		spin_unlock(&drv->lock);
 		goto done_write;
 	}
 
 	tcs->req[tcs_id - tcs->offset] = msg;
 	set_bit(tcs_id, drv->tcs_in_use);
-	spin_unlock(&drv->lock);
 
 	__tcs_buffer_write(drv, tcs_id, 0, msg);
 	__tcs_trigger(drv, tcs_id);
 
 done_write:
-	spin_unlock_irqrestore(&tcs->lock, flags);
+	spin_unlock(&drv->lock);
 	return ret;
 }
 
@@ -481,19 +475,18 @@  static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
 {
 	struct tcs_group *tcs;
 	int tcs_id = 0, cmd_id = 0;
-	unsigned long flags;
 	int ret;
 
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);
 
-	spin_lock_irqsave(&tcs->lock, flags);
+	spin_lock(&drv->lock);
 	/* find the TCS id and the command in the TCS to write to */
 	ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
 	if (!ret)
 		__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
-	spin_unlock_irqrestore(&tcs->lock, flags);
+	spin_unlock(&drv->lock);
 
 	return ret;
 }
@@ -584,7 +577,6 @@  static int rpmh_probe_tcs_config(struct platform_device *pdev,
 		tcs->type = tcs_cfg[i].type;
 		tcs->num_tcs = tcs_cfg[i].n;
 		tcs->ncpt = ncpt;
-		spin_lock_init(&tcs->lock);
 
 		if (!tcs->num_tcs || tcs->type == CONTROL_TCS)
 			continue;