From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH] Lock 7 is cpuidle specific, use non-generic value for locking Date: Thu, 12 Mar 2015 14:48:59 -0600 Message-ID: <20150312204859.GD497@linaro.org> References: <1425076217-10415-2-git-send-email-bjorn.andersson@sonymobile.com> <1426189108-35488-1-git-send-email-lina.iyer@linaro.org> <5501F889.1020800@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Content-Disposition: inline In-Reply-To: <5501F889.1020800@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd Cc: bjorn.andersson@sonymobile.com, linux-arm-msm@vger.kernel.org, jhugo@codeaurora.org, agross@codeaurora.org, linux-kernel@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org On Thu, Mar 12 2015 at 14:35 -0600, Stephen Boyd wrote: >On 03/12/15 12:38, Lina Iyer wrote: >> --- > >sign off? > :) I was just hacking it to make it easier to understand. Sure. >> drivers/hwspinlock/qcom_hwspinlock.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c >> index 93b62e0..7642524 100644 >> --- a/drivers/hwspinlock/qcom_hwspinlock.c >> +++ b/drivers/hwspinlock/qcom_hwspinlock.c >> @@ -25,16 +25,23 @@ >> >> #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 int qcom_hwspinlock_trylock(struct hwspinlock *lock) >> { >> struct regmap_field *field = lock->priv; >> u32 lock_owner; >> int ret; >> + u32 proc_id; >> >> - ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID); >> + proc_id = hwspin_lock_get_id(lock) == QCOM_CPUIDLE_LOCK ? >> + QCOM_MUTEX_CPUIDLE_OFFSET + smp_processor_id(): > >So we assume that the caller will always be the CPU that is locking the >lock? Also, do we assume that the remote side knows our CPU scheme here? >smp_processor_id() returns the logical CPU and not the physical CPU >number so hopefully the remote side doesn't care about logical CPU >numbers being written to the lock value. The remote side (SCM) doesnt care the value written. We use 128+cpu to be unique in Linux(128 is to make sure it doesnt clash with predefined values used across by other processors. > >Perhaps it would be better to have a way to tell the hwspinlock >framework what value we want written to the lock value. > That would be good, if there is value in that for other platforms, I will gladly make the change. Thoughts? >> + QCOM_MUTEX_APPS_PROC_ID; >> + >> + ret = regmap_field_write(field, proc_id); >> if (ret) >> return ret; >> >> @@ -42,7 +49,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) > >The unlock path checks proc_id so we need to update the path there too. > Good point. I missed it. >-- >Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >a Linux Foundation Collaborative Project >