| Message ID | 1433867020-7746-3-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Tue, Jun 9, 2015 at 9:23 AM, Lina Iyer <lina.iyer@linaro.org> wrote: > Hwspinlocks are widely used between processors in an SoC, and also > between elevation levels within in the same processor. QCOM SoC's use > hwspinlock to serialize entry into a low power mode when the context > switches from Linux to secure monitor. > > Lock #7 has been assigned for this purpose. In order to differentiate > between one cpu core holding a lock while another cpu is contending for > the same lock, the proc id written into the lock is (128 + cpu id). This > makes it unique value among the cpu cores and therefore when a core > locks the hwspinlock, other cores would wait for the lock to be released > since they would have a different proc id. This value is specific for > the lock #7 only. > > Declare lock #7 as raw capable, so the hwspinlock framework would not > enfore acquiring a s/w spinlock before acquiring the hwspinlock. > Hi Lina, Very sorry for slacking off and missing v1 of this. I'm puzzled to the concept of using the hwspinlock framework for lock-only locks. The patch your proposed is rather clean and as long as there's no lock-debugging added to the framework it would work... Blindly declaring lock #7 as special on all Qualcomm hwspinlocks I do however not like at all. There's nothing in either the SFPB nor TCSR mutex hardware that dictates this fact, it's a system configuration fact. As such this "requirement" should be described in the device tree. The puzzling part of the value to be written is strongly cpuidle implementation defined makes me wonder if it belong in this driver at all. At least this should be configured/flagged by some devicetree property. "qcom,lock-by-cpu-id-locks = <7, ...>"? The other alternative to these patches would be to just consume the syscon in cpuidle and opencode the locking there. It isolates the cpuidle specifics of this to the original place and it isn't using only one side of the hwspinlock framework... Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10 2015 at 11:33 -0600, Bjorn Andersson wrote: >On Tue, Jun 9, 2015 at 9:23 AM, Lina Iyer <lina.iyer@linaro.org> wrote: >> Hwspinlocks are widely used between processors in an SoC, and also >> between elevation levels within in the same processor. QCOM SoC's use >> hwspinlock to serialize entry into a low power mode when the context >> switches from Linux to secure monitor. >> >> Lock #7 has been assigned for this purpose. In order to differentiate >> between one cpu core holding a lock while another cpu is contending for >> the same lock, the proc id written into the lock is (128 + cpu id). This >> makes it unique value among the cpu cores and therefore when a core >> locks the hwspinlock, other cores would wait for the lock to be released >> since they would have a different proc id. This value is specific for >> the lock #7 only. >> >> Declare lock #7 as raw capable, so the hwspinlock framework would not >> enfore acquiring a s/w spinlock before acquiring the hwspinlock. >> > >Hi Lina, > >Very sorry for slacking off and missing v1 of this. > No worries. Thanks for reviewing. >I'm puzzled to the concept of using the hwspinlock framework for >lock-only locks. The patch your proposed is rather clean and as long >as there's no lock-debugging added to the framework it would work... > > >Blindly declaring lock #7 as special on all Qualcomm hwspinlocks I do >however not like at all. There's nothing in either the SFPB nor TCSR >mutex hardware that dictates this fact, it's a system configuration >fact. As such this "requirement" should be described in the device >tree. > Its not a mutable entity, but sure. >The puzzling part of the value to be written is strongly cpuidle >implementation defined makes me wonder if it belong in this driver at >all. > >At least this should be configured/flagged by some devicetree >property. "qcom,lock-by-cpu-id-locks = <7, ...>"? > Okay. > >The other alternative to these patches would be to just consume the >syscon in cpuidle and opencode the locking there. It isolates the >cpuidle specifics of this to the original place and it isn't using >only one side of the hwspinlock framework... > Well, ultimately a hwspinlock is just a writel, so that is a possibility, if we want. But it is a hwspinlock, therefore the use of the framework seems appropriate, even amidst the unique behavior of the lock. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c index 93b62e0..59278b0 100644 --- a/drivers/hwspinlock/qcom_hwspinlock.c +++ b/drivers/hwspinlock/qcom_hwspinlock.c @@ -25,16 +25,26 @@ #include "hwspinlock_internal.h" -#define QCOM_MUTEX_APPS_PROC_ID 1 -#define QCOM_MUTEX_NUM_LOCKS 32 +#define QCOM_MUTEX_APPS_PROC_ID 1 +#define QCOM_MUTEX_CPUIDLE_OFFSET 128 +#define QCOM_CPUIDLE_LOCK 7 +#define QCOM_MUTEX_NUM_LOCKS 32 + +static inline u32 __qcom_get_proc_id(struct hwspinlock *lock) +{ + return hwspin_lock_get_id(lock) == QCOM_CPUIDLE_LOCK ? + (QCOM_MUTEX_CPUIDLE_OFFSET + smp_processor_id()) : + QCOM_MUTEX_APPS_PROC_ID; +} static int qcom_hwspinlock_trylock(struct hwspinlock *lock) { struct regmap_field *field = lock->priv; u32 lock_owner; int ret; + u32 proc_id = __qcom_get_proc_id(lock); - ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID); + ret = regmap_field_write(field, proc_id); if (ret) return ret; @@ -42,7 +52,7 @@ static int qcom_hwspinlock_trylock(struct hwspinlock *lock) if (ret) return ret; - return lock_owner == QCOM_MUTEX_APPS_PROC_ID; + return lock_owner == proc_id; } static void qcom_hwspinlock_unlock(struct hwspinlock *lock) @@ -57,7 +67,7 @@ static void qcom_hwspinlock_unlock(struct hwspinlock *lock) return; } - if (lock_owner != QCOM_MUTEX_APPS_PROC_ID) { + if (lock_owner != __qcom_get_proc_id(lock)) { pr_err("%s: spinlock not owned by us (actual owner is %d)\n", __func__, lock_owner); } @@ -129,6 +139,8 @@ static int qcom_hwspinlock_probe(struct platform_device *pdev) regmap, field); } + bank->lock[QCOM_CPUIDLE_LOCK].hwcaps = HWL_CAP_ALLOW_RAW; + pm_runtime_enable(&pdev->dev); ret = hwspin_lock_register(bank, &pdev->dev, &qcom_hwspinlock_ops,
Hwspinlocks are widely used between processors in an SoC, and also between elevation levels within in the same processor. QCOM SoC's use hwspinlock to serialize entry into a low power mode when the context switches from Linux to secure monitor. Lock #7 has been assigned for this purpose. In order to differentiate between one cpu core holding a lock while another cpu is contending for the same lock, the proc id written into the lock is (128 + cpu id). This makes it unique value among the cpu cores and therefore when a core locks the hwspinlock, other cores would wait for the lock to be released since they would have a different proc id. This value is specific for the lock #7 only. Declare lock #7 as raw capable, so the hwspinlock framework would not enfore acquiring a s/w spinlock before acquiring the hwspinlock. Cc: Jeffrey Hugo <jhugo@codeaurora.org> Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com> Cc: Andy Gross <agross@codeaurora.org> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- drivers/hwspinlock/qcom_hwspinlock.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)