Re: [PATCH] Lock 7 is cpuidle specific, use non-generic value for locking

[Lina Iyer]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/