| Message ID | 1438792366-2737-9-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 05 Aug 09:32 PDT 2015, Lina Iyer 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. > As I was thinking about this, I came to the conclusion that my argument that it's system configuration and not a property of the block that lock #7 is special is actually biting myself. Just as #7 is system configuration, so is the fact that 1 is the lock value for all other locks. I've been meaning to answer you, but I haven't come to a good conclusion in this matter. I think that both of these properties should be possible to express in DT, but I don't know how. Perhaps we should just list each lock that we expose as a subnode in DT with a property to indicate the lock value - with the possibility of indicating cpu_id. Something like: tcsr-mutex { compatible = "qcom,tcsr-mutex"; syscon = <&tcsr_mutex_block 0 0x80>; #hwlock-cells = <1>; #address-cells = <1>; #size-cells = <0>; smem-lock@3 { reg = <3>; qcom,lock-value = <1>; }; scm-lock@7 { reg = <7>; qcom,lock-value-from-cpu-id; qcom,lock-raw; }; }; As we don't reference most of these locks anyways I don't think this would still be reasonable. And if reg is an array it's quite compact. > Declare lock #7 as raw capable, so the hwspinlock framework would not > enfore acquiring a s/w spinlock before acquiring the hwspinlock. > I'm fine with this part! Sorry for the extremely slow response, but this took some time to process... Regards, Bjorn
On Wed, Aug 12 2015 at 22:30 -0600, Bjorn Andersson wrote: >On Wed 05 Aug 09:32 PDT 2015, Lina Iyer 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. >> > >As I was thinking about this, I came to the conclusion that my argument >that it's system configuration and not a property of the block that lock >#7 is special is actually biting myself. > >Just as #7 is system configuration, so is the fact that 1 is the lock >value for all other locks. > >I've been meaning to answer you, but I haven't come to a good conclusion >in this matter. I think that both of these properties should be possible >to express in DT, but I don't know how. > I thought about that. These are s/w imposed behavior. As a far as the h/w is concerned, you could just write a non-zero value and the lock is considered acquired. So placing that in DT may not be appropriate. > >Perhaps we should just list each lock that we expose as a subnode in DT >with a property to indicate the lock value - with the possibility of >indicating cpu_id. > >Something like: > >tcsr-mutex { > compatible = "qcom,tcsr-mutex"; > syscon = <&tcsr_mutex_block 0 0x80>; > > #hwlock-cells = <1>; > #address-cells = <1>; > #size-cells = <0>; > > smem-lock@3 { > reg = <3>; > qcom,lock-value = <1>; > }; > > scm-lock@7 { > reg = <7>; > qcom,lock-value-from-cpu-id; > qcom,lock-raw; > }; >}; > >As we don't reference most of these locks anyways I don't think this >would still be reasonable. And if reg is an array it's quite compact. > But, looking at the DT, its not evident what lock-value-from-cpu-id, would mean. It is an implementation detail of Linux (as a result of firmware). May be better to just hide it in the hwspinlock driver. I am not sure about lock-raw, but I would think its not QCOM specific. Imagine the case, when hwspinlock framework does not s/w spinlock around the hwspinlock, we wouldnt have this property. The reason why we spinlock around the hwspinlock is because we dont have a good way to generate a unique value for the hwspinlock, so multiple threads acquiring the same locks could safely be assured that they have acquired the lock. While convenient and probably more effecient to just have s/w spinlocks around the hwspinlock, the problem here is it is mandated across all locks. To me it seems OK to explicity call out lock #7 as unique entity in the bank that is compatible with a raw hwspinlock in the DT. But the value that lock #7 uses is an implementation detail and should rest wth the drivers. Thanks, Lina
diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c index c752447..573f0dd 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(-)