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

From: Bjorn Andersson <bjorn@kryo.se>
To: Lina Iyer <lina.iyer@linaro.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Jeffrey Hugo <jhugo@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@sonymobile.com>,
	Andy Gross <agross@codeaurora.org>
Subject: Re: [PATCH RFC v2 2/2] hwspinlock: qcom: Lock #7 is special lock, uses dynamic proc_id
Date: Wed, 10 Jun 2015 10:33:06 -0700	[thread overview]
Message-ID: <CAJAp7Oi23Ho_og9+GSv3qDnWZ0fkf2JydHssPH06jBuE4i3EzA@mail.gmail.com> (raw)
In-Reply-To: <1433867020-7746-3-git-send-email-lina.iyer@linaro.org>

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