Re: [PATCH RFC v2 2/2] hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id

[Lina Iyer]

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@xxxxxxxxxx> 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-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/