[PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Bjorn Andersson]

Add binding documentation for the Qualcomm Hardware Mutex.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

I think the conclusion on the dt binding discussion for hwspinlocks was that
we're down to having the #hwlock-cells intact. So this version includes that,
but non of the other previously discussed properties.

Changes since v5:
- Extracted the dt binding documentation into a separate patch
- Moved the driver to consume a syscon
- Dropped previously suggested generic hwlock dt bindings

 .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt

diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
new file mode 100644
index 0000000..28ade7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
@@ -0,0 +1,39 @@
+Qualcomm Hardware Mutex Block:
+
+The hardware block provides mutexes utilized between different processors on
+the SoC as part of the communication protocol used by these processors.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,sfpb-mutex",
+		    "qcom,tcsr-mutex"
+
+- syscon:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: one cell containing:
+		    syscon phandle
+		    offset of the hwmutex block within the syscon
+		    stride of the hwmutex registers
+
+- #hwlock-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: must be 1, the specified cell represent the lock id
+		    (hwlock standard property, see hwlock.txt)
+
+Example:
+
+        tcsr: syscon@1a400000 {
+		compatible = "qcom,tcsr-msm8974", "syscon";
+		reg = <0xfd484000 0x2000>;
+	};
+
+	hwlock@fd484000 {
+		compatible = "qcom,tcsr-mutex";
+		syscon = <&tcsr 0 0x80>;
+
+		#hwlock-cells = <1>;
+	};
-- 
1.8.2.2

[PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
SoCs.

Based on initial effort by Kumar Gala <galak@codeaurora.org>

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

As Andy Gross introduced the tcsr syscon we can no longer just ioremap the
memory directly, so rework the driver to run ontop of syscon.

Changes since v5:
- Dropped all but hwspinlock specific dt bindings
- Hardcoded the number of locks (there is 32)
- Rework to sit ontop of syscon

Changes since v4:
- Aligned with devicetree support in hwlock framework and hence depends on [1]

Changes since v3:
- Reverted back to getting stride from of_match, per Kumars request

Changes since v2:
- MODULE_DEVICE_TABLE
- Changed prefix to qcom
- Cleaned up includes
- Rely on reg and num-locks to figure out stride, instead of of_match data

Changes since v1:
- Added the pm_runtime calls needed to be able to boot a kernel with
  pm_runtime and this driver, patch from Courtney.
- Added sfpb-mutex compatible, for re-use of the driver in family A platforms.
- Updated formatting of DT binding documentation, while adding the extra
  compatible.
- Dropped Stephen Boyds Reviewed-by due to these changes.

 drivers/hwspinlock/Kconfig           |  11 +++
 drivers/hwspinlock/Makefile          |   1 +
 drivers/hwspinlock/qcom_hwspinlock.c | 181 +++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/hwspinlock/qcom_hwspinlock.c

diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 3612cb5..762216d 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -18,6 +18,17 @@ config HWSPINLOCK_OMAP
 
 	  If unsure, say N.
 
+config HWSPINLOCK_QCOM
+	tristate "Qualcomm Hardware Spinlock device"
+	depends on ARCH_QCOM
+	select HWSPINLOCK
+	help
+	  Say y here to support the Qualcomm Hardware Mutex functionality, which
+	  provides a synchronisation mechanism for the various processors on
+	  the SoC.
+
+	  If unsure, say N.
+
 config HSEM_U8500
 	tristate "STE Hardware Semaphore functionality"
 	depends on ARCH_U8500
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index 93eb64b..68f95d9 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -4,4 +4,5 @@
 
 obj-$(CONFIG_HWSPINLOCK)		+= hwspinlock_core.o
 obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
+obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
new file mode 100644
index 0000000..93b62e0
--- /dev/null
+++ b/drivers/hwspinlock/qcom_hwspinlock.c
@@ -0,0 +1,181 @@
+/*
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2015, Sony Mobile Communications AB
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/hwspinlock.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#include "hwspinlock_internal.h"
+
+#define QCOM_MUTEX_APPS_PROC_ID	1
+#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;
+
+	ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_read(field, &lock_owner);
+	if (ret)
+		return ret;
+
+	return lock_owner == QCOM_MUTEX_APPS_PROC_ID;
+}
+
+static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	struct regmap_field *field = lock->priv;
+	u32 lock_owner;
+	int ret;
+
+	ret = regmap_field_read(field, &lock_owner);
+	if (ret) {
+		pr_err("%s: unable to query spinlock owner\n", __func__);
+		return;
+	}
+
+	if (lock_owner != QCOM_MUTEX_APPS_PROC_ID) {
+		pr_err("%s: spinlock not owned by us (actual owner is %d)\n",
+				__func__, lock_owner);
+	}
+
+	ret = regmap_field_write(field, 0);
+	if (ret)
+		pr_err("%s: failed to unlock spinlock\n", __func__);
+}
+
+static const struct hwspinlock_ops qcom_hwspinlock_ops = {
+	.trylock	= qcom_hwspinlock_trylock,
+	.unlock		= qcom_hwspinlock_unlock,
+};
+
+static const struct of_device_id qcom_hwspinlock_of_match[] = {
+	{ .compatible = "qcom,sfpb-mutex" },
+	{ .compatible = "qcom,tcsr-mutex" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_hwspinlock_of_match);
+
+static int qcom_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct hwspinlock_device *bank;
+	struct device_node *syscon;
+	struct reg_field field;
+	struct regmap *regmap;
+	size_t array_size;
+	u32 stride;
+	u32 base;
+	int ret;
+	int i;
+
+	syscon = of_parse_phandle(pdev->dev.of_node, "syscon", 0);
+	if (!syscon) {
+		dev_err(&pdev->dev, "no syscon property\n");
+		return -ENODEV;
+	}
+
+	regmap = syscon_node_to_regmap(syscon);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 1, &base);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no offset in syscon\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "syscon", 2, &stride);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no stride syscon\n");
+		return -EINVAL;
+	}
+
+	array_size = QCOM_MUTEX_NUM_LOCKS * sizeof(struct hwspinlock);
+	bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
+	if (!bank)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, bank);
+
+	for (i = 0; i < QCOM_MUTEX_NUM_LOCKS; i++) {
+		field.reg = base + i * stride;
+		field.lsb = 0;
+		field.msb = 32;
+
+		bank->lock[i].priv = devm_regmap_field_alloc(&pdev->dev,
+							     regmap, field);
+	}
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = hwspin_lock_register(bank, &pdev->dev, &qcom_hwspinlock_ops,
+				   0, QCOM_MUTEX_NUM_LOCKS);
+	if (ret)
+		pm_runtime_disable(&pdev->dev);
+
+	return ret;
+}
+
+static int qcom_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct hwspinlock_device *bank = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = hwspin_lock_unregister(bank);
+	if (ret) {
+		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
+		return ret;
+	}
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static struct platform_driver qcom_hwspinlock_driver = {
+	.probe		= qcom_hwspinlock_probe,
+	.remove		= qcom_hwspinlock_remove,
+	.driver		= {
+		.name	= "qcom_hwspinlock",
+		.of_match_table = qcom_hwspinlock_of_match,
+	},
+};
+
+static int __init qcom_hwspinlock_init(void)
+{
+	return platform_driver_register(&qcom_hwspinlock_driver);
+}
+/* board init code might need to reserve hwspinlocks for predefined purposes */
+postcore_initcall(qcom_hwspinlock_init);
+
+static void __exit qcom_hwspinlock_exit(void)
+{
+	platform_driver_unregister(&qcom_hwspinlock_driver);
+}
+module_exit(qcom_hwspinlock_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver for Qualcomm SoCs");
-- 
1.8.2.2

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Andy Gross]

On Fri, Feb 27, 2015 at 02:30:16PM -0800, Bjorn Andersson wrote:
> Add binding documentation for the Qualcomm Hardware Mutex.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---

Looks fine.

Reviewed-by: Andy Gross <agross@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Andy Gross]

On Fri, Feb 27, 2015 at 02:30:17PM -0800, Bjorn Andersson wrote:
> Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
> SoCs.
> 
> Based on initial effort by Kumar Gala <galak@codeaurora.org>
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

Reviewed-by: Andy Gross <agross@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

Hi Mark, Rob,

On Sat, Feb 28, 2015 at 12:30 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> Add binding documentation for the Qualcomm Hardware Mutex.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>
> I think the conclusion on the dt binding discussion for hwspinlocks was that
> we're down to having the #hwlock-cells intact. So this version includes that,
> but non of the other previously discussed properties.
>
> Changes since v5:
> - Extracted the dt binding documentation into a separate patch
> - Moved the driver to consume a syscon
> - Dropped previously suggested generic hwlock dt bindings
>
>  .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt

Could you please ack this one patch from Bjorn?

I'll need your Ack before I can this forward.

Thanks,
Ohad.

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Mark Rutland]

On Fri, Feb 27, 2015 at 10:30:16PM +0000, Bjorn Andersson wrote:
> Add binding documentation for the Qualcomm Hardware Mutex.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> I think the conclusion on the dt binding discussion for hwspinlocks was that
> we're down to having the #hwlock-cells intact. So this version includes that,
> but non of the other previously discussed properties.
> 
> Changes since v5:
> - Extracted the dt binding documentation into a separate patch
> - Moved the driver to consume a syscon

I'm a little confused here. Why are we now using a syscon? I thought the
set of registers for the block was well-defined.

If there's a link to some previous discussion on that point, it would be
helpful.

Mark.

> - Dropped previously suggested generic hwlock dt bindings
> 
>  .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> new file mode 100644
> index 0000000..28ade7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> @@ -0,0 +1,39 @@
> +Qualcomm Hardware Mutex Block:
> +
> +The hardware block provides mutexes utilized between different processors on
> +the SoC as part of the communication protocol used by these processors.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,sfpb-mutex",
> +		    "qcom,tcsr-mutex"
> +
> +- syscon:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: one cell containing:
> +		    syscon phandle
> +		    offset of the hwmutex block within the syscon
> +		    stride of the hwmutex registers
> +
> +- #hwlock-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 1, the specified cell represent the lock id
> +		    (hwlock standard property, see hwlock.txt)
> +
> +Example:
> +
> +        tcsr: syscon@1a400000 {
> +		compatible = "qcom,tcsr-msm8974", "syscon";
> +		reg = <0xfd484000 0x2000>;
> +	};
> +
> +	hwlock@fd484000 {
> +		compatible = "qcom,tcsr-mutex";
> +		syscon = <&tcsr 0 0x80>;
> +
> +		#hwlock-cells = <1>;
> +	};
> -- 
> 1.8.2.2
> 
> 

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Bjorn Andersson]

On Thu 12 Mar 02:51 PDT 2015, Mark Rutland wrote:

> On Fri, Feb 27, 2015 at 10:30:16PM +0000, Bjorn Andersson wrote:
> > Add binding documentation for the Qualcomm Hardware Mutex.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > ---
> > 
> > I think the conclusion on the dt binding discussion for hwspinlocks was that
> > we're down to having the #hwlock-cells intact. So this version includes that,
> > but non of the other previously discussed properties.
> > 
> > Changes since v5:
> > - Extracted the dt binding documentation into a separate patch
> > - Moved the driver to consume a syscon
> 
> I'm a little confused here. Why are we now using a syscon? I thought the
> set of registers for the block was well-defined.
> 

The sfpb-mutex registers make up a block (according to my
documentation), but after discussing tcsr with Andy Gross we concluded
that although the tcsr-mutex registers are layed out consecutively in
the tcsr, they are not alone in the block.

Further more Andy introduced the tcsr-syscon binding in his work to
support DMA on GSBI (uart/i2c/spi), so that's why I had to make this
change.

Preferrably this would have showed up before v6...

> If there's a link to some previous discussion on that point, it would be
> helpful.
> 

Unfortunately there isn't, as we discussed this mainly face to face a
few weeks back.

Regards,
Bjorn

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Lina Iyer]

On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
>Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
>SoCs.
>
>Based on initial effort by Kumar Gala <galak@codeaurora.org>
>
>Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>---
>

[...]

>+#include "hwspinlock_internal.h"
>+
>+#define QCOM_MUTEX_APPS_PROC_ID	1
Hi Bjorn,

Not all locks use 1 to indicate its locked. For example lock index 7 is
used by cpuidle driver between HLOS and SCM. It uses a write value of
(128 + smp_processor_id()) to lock.

A cpu acquires the remote spin lock, and calls into SCM to terminate the
power down sequence while passing the state of the L2. The lock help
guarantee the last core to hold the spinlock to have the most up to date
value for the L2 flush flag.

>+#define QCOM_MUTEX_NUM_LOCKS	32

Also, talking to Jeff it seems like that out of the 32 locks defined
only 8 is accessible from Linux. So its unnecessary and probably
incorrect to assume that there are 32 locks available.

Thanks,
Lina

>+{
>+	struct regmap_field *field = lock->priv;
>+	u32 lock_owner;
>+	int ret;
>+
>+	ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID);
>+	if (ret)
>+		return ret;
>+
>+	ret = regmap_field_read(field, &lock_owner);
>+	if (ret)
>+		return ret;
>+
>+	return lock_owner == QCOM_MUTEX_APPS_PROC_ID;
>+}
>+

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

[Lina Iyer]

---
 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():
+			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)
-- 
2.1.0

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Andy Gross]

On Thu, Mar 12, 2015 at 01:31:50PM -0600, Lina Iyer wrote:
> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
> >SoCs.
> >
> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
> >
> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> >---
> >
> 
> [...]
> 
> >+#include "hwspinlock_internal.h"
> >+
> >+#define QCOM_MUTEX_APPS_PROC_ID	1
> Hi Bjorn,
> 
> Not all locks use 1 to indicate its locked. For example lock index 7 is
> used by cpuidle driver between HLOS and SCM. It uses a write value of
> (128 + smp_processor_id()) to lock.

So this was the lock_rlock_id API?

> 
> A cpu acquires the remote spin lock, and calls into SCM to terminate the
> power down sequence while passing the state of the L2. The lock help
> guarantee the last core to hold the spinlock to have the most up to date
> value for the L2 flush flag.
> 
> >+#define QCOM_MUTEX_NUM_LOCKS	32
> 
> Also, talking to Jeff it seems like that out of the 32 locks defined
> only 8 is accessible from Linux. So its unnecessary and probably
> incorrect to assume that there are 32 locks available.

Out of curiosity, this is a TZ thing?  If so, we'd expect issues if someone
decides to utilize resources that, while technically are present, are unusable
by that processor.  This is not that much different from someone misconfiguring
an EE on a DMA controller.

> >+{
> >+	struct regmap_field *field = lock->priv;
> >+	u32 lock_owner;
> >+	int ret;
> >+
> >+	ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID);
> >+	if (ret)
> >+		return ret;
> >+
> >+	ret = regmap_field_read(field, &lock_owner);
> >+	if (ret)
> >+		return ret;
> >+
> >+	return lock_owner == QCOM_MUTEX_APPS_PROC_ID;
> >+}
> >+
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Lina Iyer]

On Thu, Mar 12 2015 at 13:43 -0600, Andy Gross wrote:
>On Thu, Mar 12, 2015 at 01:31:50PM -0600, Lina Iyer wrote:
>> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
>> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
>> >SoCs.
>> >
>> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
>> >
>> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> >---
>> >
>>
>> [...]
>>
>> >+#include "hwspinlock_internal.h"
>> >+
>> >+#define QCOM_MUTEX_APPS_PROC_ID	1
>> Hi Bjorn,
>>
>> Not all locks use 1 to indicate its locked. For example lock index 7 is
>> used by cpuidle driver between HLOS and SCM. It uses a write value of
>> (128 + smp_processor_id()) to lock.
>
>So this was the lock_rlock_id API?
>
Correct.
>>
>> A cpu acquires the remote spin lock, and calls into SCM to terminate the
>> power down sequence while passing the state of the L2. The lock help
>> guarantee the last core to hold the spinlock to have the most up to date
>> value for the L2 flush flag.
>>
>> >+#define QCOM_MUTEX_NUM_LOCKS	32
>>
>> Also, talking to Jeff it seems like that out of the 32 locks defined
>> only 8 is accessible from Linux. So its unnecessary and probably
>> incorrect to assume that there are 32 locks available.
>
>Out of curiosity, this is a TZ thing?  If so, we'd expect issues if someone
>decides to utilize resources that, while technically are present, are unusable
>by that processor.  This is not that much different from someone misconfiguring
>an EE on a DMA controller.
>
Per Jeff, the protection unit doesnt generally allow access to locks > 8
and shouldnt be allowed and in some SoC's where they dont have the
protection, it might still be a bad idea. It would be safer to restrict to
8, than allow all 32 and hope somebody doesnt do the wrong thing.

>> >+{
>> >+	struct regmap_field *field = lock->priv;
>> >+	u32 lock_owner;
>> >+	int ret;
>> >+
>> >+	ret = regmap_field_write(field, QCOM_MUTEX_APPS_PROC_ID);
>> >+	if (ret)
>> >+		return ret;
>> >+
>> >+	ret = regmap_field_read(field, &lock_owner);
>> >+	if (ret)
>> >+		return ret;
>> >+
>> >+	return lock_owner == QCOM_MUTEX_APPS_PROC_ID;
>> >+}
>> >+
>>
>
>-- 
>Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

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

[Stephen Boyd]

On 03/12/15 12:38, Lina Iyer wrote:
> ---

sign off?

>  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.

Perhaps it would be better to have a way to tell the hwspinlock
framework what value we want written to the lock value.

> +			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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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
>

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

[Andy Gross]

On Thu, Mar 12, 2015 at 01:38:28PM -0600, Lina Iyer wrote:

<snip>

>  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():
> +			QCOM_MUTEX_APPS_PROC_ID;
> +
> +	ret = regmap_field_write(field, proc_id);

I think I'd rather have a qcom specific function and EXPORT_SYMBOL that to deal
with this special case.

>  	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)
> -- 
> 2.1.0
> 

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lina Iyer]

On Thu, Mar 12 2015 at 14:49 -0600, Andy Gross wrote:
>On Thu, Mar 12, 2015 at 01:38:28PM -0600, Lina Iyer wrote:
>
><snip>
>
>>  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():
>> +			QCOM_MUTEX_APPS_PROC_ID;
>> +
>> +	ret = regmap_field_write(field, proc_id);
>
>I think I'd rather have a qcom specific function and EXPORT_SYMBOL that to deal
>with this special case.
>
I was going back and forth between a function and inlining this here.
But Stephen just made a good point that this is needed for unlock as
well. A function would be good.


>>  	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)
>> --
>> 2.1.0
>>
>
>-- 
>Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

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

[Stephen Boyd]

On 03/12/15 13:48, Lina Iyer wrote:
> 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.
>
>

It looks like the remote side unlocks it too? It doesn't seem like this
will work with the framework very well. The framework has a kernel
spinlock attached to the hwspinlock so when we lock the hwspinlock we
also lock the kernel spinlock and we only release the kernel spinlock
when the kernel unlocks the hwspinlock. In this case it seems like
cpuidle wants to have it's own kernel spinlock and just use the trylock
loop part of __hwspin_lock_timeout() without taking any kernel side
locks. Plus it wants to write a specific value to the lock.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lina Iyer]

On Thu, Mar 12 2015 at 15:12 -0600, Stephen Boyd wrote:
>On 03/12/15 13:48, Lina Iyer wrote:
>> 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.
>>
>>
>
>It looks like the remote side unlocks it too? It doesn't seem like this
>will work with the framework very well. The framework has a kernel
>spinlock attached to the hwspinlock so when we lock the hwspinlock we
>also lock the kernel spinlock and we only release the kernel spinlock
>when the kernel unlocks the hwspinlock. In this case it seems like
>cpuidle wants to have it's own kernel spinlock and just use the trylock
>loop part of __hwspin_lock_timeout() without taking any kernel side
>locks. Plus it wants to write a specific value to the lock.
>
Right.
Just noticed that part of the hwspinlock. Yes SCM unlocks the
hwspinlock. So I cannot hold any lock in Linux. May need changes in the
hwspinlock framework. Seems like an additional flag in hwspinlock to not
lock any in the trylock path work work. Hmm....



>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Lina Iyer]

On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
>Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
>SoCs.
>
>Based on initial effort by Kumar Gala <galak@codeaurora.org>
>
>Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>+config HWSPINLOCK_QCOM
>+	tristate "Qualcomm Hardware Spinlock device"
>+	depends on ARCH_QCOM
>+	select HWSPINLOCK

select MFD_SYSCON as well, perhaps?
We seem to be dependent on it.

>+	help
>+	  Say y here to support the Qualcomm Hardware Mutex functionality, which
>+	  provides a synchronisation mechanism for the various processors on
>+	  the SoC.
>+
>+	  If unsure, say N.
>+

Thanks,
Lina

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

[Andy Gross]

On Thu, Mar 12, 2015 at 04:16:00PM -0600, Lina Iyer wrote:

<snip>

> >It looks like the remote side unlocks it too? It doesn't seem like this
> >will work with the framework very well. The framework has a kernel
> >spinlock attached to the hwspinlock so when we lock the hwspinlock we
> >also lock the kernel spinlock and we only release the kernel spinlock
> >when the kernel unlocks the hwspinlock. In this case it seems like
> >cpuidle wants to have it's own kernel spinlock and just use the trylock
> >loop part of __hwspin_lock_timeout() without taking any kernel side
> >locks. Plus it wants to write a specific value to the lock.
> >
> Right.
> Just noticed that part of the hwspinlock. Yes SCM unlocks the
> hwspinlock. So I cannot hold any lock in Linux. May need changes in the
> hwspinlock framework. Seems like an additional flag in hwspinlock to not
> lock any in the trylock path work work. Hmm....

Or a specific EXPORT function for this one usecase which is unlike anyone elses
usage.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lina Iyer]

On Fri, Mar 13 2015 at 14:02 -0600, Andy Gross wrote:
>On Thu, Mar 12, 2015 at 04:16:00PM -0600, Lina Iyer wrote:
>
><snip>
>
>> >It looks like the remote side unlocks it too? It doesn't seem like this
>> >will work with the framework very well. The framework has a kernel
>> >spinlock attached to the hwspinlock so when we lock the hwspinlock we
>> >also lock the kernel spinlock and we only release the kernel spinlock
>> >when the kernel unlocks the hwspinlock. In this case it seems like
>> >cpuidle wants to have it's own kernel spinlock and just use the trylock
>> >loop part of __hwspin_lock_timeout() without taking any kernel side
>> >locks. Plus it wants to write a specific value to the lock.
>> >
>> Right.
>> Just noticed that part of the hwspinlock. Yes SCM unlocks the
>> hwspinlock. So I cannot hold any lock in Linux. May need changes in the
>> hwspinlock framework. Seems like an additional flag in hwspinlock to not
>> lock any in the trylock path work work. Hmm....
>
>Or a specific EXPORT function for this one usecase which is unlike anyone elses
>usage.

I think, this can be handled well within the QCOM driver. I will submit
a patch for it and we can discuss then later. For now, this will work
fine as is.

Lina

>
>-- 
>Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Jeffrey Hugo]

On 2/27/2015 3:30 PM, Bjorn Andersson wrote:
> Add binding documentation for the Qualcomm Hardware Mutex.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---

Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Jeffrey Hugo]

On 2/27/2015 3:30 PM, Bjorn Andersson wrote:
> Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
> SoCs.
>
> Based on initial effort by Kumar Gala <galak@codeaurora.org>
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---

Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

I don't see any reason to hold up this patch.  We can come to a 
conclusion on the number of locks and how to handle the lock 7 special 
case as an independent follow up.

Thanks sticking with this and getting it finalized.

-- 
Jeffrey Hugo
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Thu 12 Mar 12:31 PDT 2015, Lina Iyer wrote:

> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
> >SoCs.
> >
> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
> >
> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> >---
> >
> 
> [...]
> 
> >+#include "hwspinlock_internal.h"
> >+
> >+#define QCOM_MUTEX_APPS_PROC_ID	1
> Hi Bjorn,
> 
> Not all locks use 1 to indicate its locked. For example lock index 7 is
> used by cpuidle driver between HLOS and SCM. It uses a write value of
> (128 + smp_processor_id()) to lock.
> 

In other words, it's a magic number that will make sure that not more
than one cpu enters TZ sleep code at a time.

> A cpu acquires the remote spin lock, and calls into SCM to terminate the
> power down sequence while passing the state of the L2. The lock help
> guarantee the last core to hold the spinlock to have the most up to date
> value for the L2 flush flag.
> 

Yeah, I remember having to dig out the deadlock related to the
introduction of that logic on my side (turned out to have an old TZ).

There's already mutual exclusion and reference counting within TZ to
make sure we're not turning off the caches unless this is the last core
going down.
I presume that the reason behind the hwmutex logic is to make sure that
with multiple cores racing to sleep only one of them will flush the
caches in Linux and will be the last entering TZ. Can you confirm this?

> >+#define QCOM_MUTEX_NUM_LOCKS	32
> 
> Also, talking to Jeff it seems like that out of the 32 locks defined
> only 8 is accessible from Linux. So its unnecessary and probably
> incorrect to assume that there are 32 locks available.
> 

The hardware block have 32 locks and the decision regarding which locks
this particular Linux system is allowed to access is configuration.

Regards,
Bjorn

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Thu 12 Mar 12:55 PDT 2015, Lina Iyer wrote:

> On Thu, Mar 12 2015 at 13:43 -0600, Andy Gross wrote:
> >On Thu, Mar 12, 2015 at 01:31:50PM -0600, Lina Iyer wrote:
> >> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
[..]
> >> Also, talking to Jeff it seems like that out of the 32 locks defined
> >> only 8 is accessible from Linux. So its unnecessary and probably
> >> incorrect to assume that there are 32 locks available.
> >
> >Out of curiosity, this is a TZ thing?  If so, we'd expect issues if someone
> >decides to utilize resources that, while technically are present, are unusable
> >by that processor.  This is not that much different from someone misconfiguring
> >an EE on a DMA controller.
> >
> Per Jeff, the protection unit doesnt generally allow access to locks > 8
> and shouldnt be allowed and in some SoC's where they dont have the
> protection, it might still be a bad idea. It would be safer to restrict to
> 8, than allow all 32 and hope somebody doesnt do the wrong thing.
> 

You have the same problem with all peripherals that are shared between
the various subsystems; e.g. the BLSPs used by the sensor DSP shouldn't
be touched by the Linux system.

But what if Linux runs on the sensor DSP?


The driver should support the hardware and the system configuration (DT)
should make sure the valid resources are accessed.

Regards,
Bjorn

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Thu 12 Mar 15:29 PDT 2015, Lina Iyer wrote:

> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
> >SoCs.
> >
> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
> >
> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> >+config HWSPINLOCK_QCOM
> >+	tristate "Qualcomm Hardware Spinlock device"
> >+	depends on ARCH_QCOM
> >+	select HWSPINLOCK
> 
> select MFD_SYSCON as well, perhaps?
> We seem to be dependent on it.
> 

Right, missed that. Thanks!

Will resend with that addition.

Regards,
Bjorn

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Lina Iyer]

On Wed, Mar 18 2015 at 09:56 -0600, Bjorn Andersson wrote:
>On Thu 12 Mar 12:31 PDT 2015, Lina Iyer wrote:
>
>> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
>> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
>> >SoCs.
>> >
>> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
>> >
>> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> >---
>> >
>>
>> [...]
>>
>> >+#include "hwspinlock_internal.h"
>> >+
>> >+#define QCOM_MUTEX_APPS_PROC_ID	1
>> Hi Bjorn,
>>
>> Not all locks use 1 to indicate its locked. For example lock index 7 is
>> used by cpuidle driver between HLOS and SCM. It uses a write value of
>> (128 + smp_processor_id()) to lock.
>>
>
>In other words, it's a magic number that will make sure that not more
>than one cpu enters TZ sleep code at a time.
>
Right, its a magic number of sorts.

>> A cpu acquires the remote spin lock, and calls into SCM to terminate the
>> power down sequence while passing the state of the L2. The lock help
>> guarantee the last core to hold the spinlock to have the most up to date
>> value for the L2 flush flag.
>>
>
>Yeah, I remember having to dig out the deadlock related to the
>introduction of that logic on my side (turned out to have an old TZ).
>
>There's already mutual exclusion and reference counting within TZ to
>make sure we're not turning off the caches unless this is the last core
>going down.

Yes, there is. But the perception of the last core in Linux and the last
core going down in TZ may be incorrect. Say for example, two cpus are
going down from linux - cpu0 & cpu1. cpu0 was the last core calling into
TZ from Linux and cpu1 had already done so. However, cpu1 started
handling an FIQ and therefore was blocked doing that while cpu0, went
through TZ. When each cpu calls into TZ, we provide the TZ with the L2
flush flag so as to allow TZ to also flush its secure lines before
powering the L2 down. The L2 flush flag that the cpu submits is its own
version of the system state. To get TZ to recognize the last valid l2
flush flag value from Linux, we need the hwmutex.

>I presume that the reason behind the hwmutex logic is to make sure that
>with multiple cores racing to sleep only one of them will flush the
>caches in Linux and will be the last entering TZ. Can you confirm this?
>
Its more for passing the flush flag than flushing the cache itself per-se.

>> >+#define QCOM_MUTEX_NUM_LOCKS	32
>>
>> Also, talking to Jeff it seems like that out of the 32 locks defined
>> only 8 is accessible from Linux. So its unnecessary and probably
>> incorrect to assume that there are 32 locks available.
>>
>
>The hardware block have 32 locks and the decision regarding which locks
>this particular Linux system is allowed to access is configuration.
>
Understood. But while the hardware may support it, it may be right for
Linux to be allowed to configure, giving a false sense of number of
locks.

>Regards,
>Bjorn

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Lina Iyer]

On Wed, Mar 18 2015 at 10:12 -0600, Bjorn Andersson wrote:
>On Thu 12 Mar 15:29 PDT 2015, Lina Iyer wrote:
>
>> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
>> >Add driver for Qualcomm Hardware Mutex block found in many Qualcomm
>> >SoCs.
>> >
>> >Based on initial effort by Kumar Gala <galak@codeaurora.org>
>> >
>> >Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> >+config HWSPINLOCK_QCOM
>> >+	tristate "Qualcomm Hardware Spinlock device"
Any reason, why this is tristate and not a bool?

>> >+	depends on ARCH_QCOM
>> >+	select HWSPINLOCK
>>
>> select MFD_SYSCON as well, perhaps?
>> We seem to be dependent on it.
>>
>
>Right, missed that. Thanks!
>
>Will resend with that addition.
>
>Regards,
>Bjorn
>

Re: [PATCH v6 2/2] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Wed 18 Mar 09:45 PDT 2015, Lina Iyer wrote:

> On Wed, Mar 18 2015 at 09:56 -0600, Bjorn Andersson wrote:
> >On Thu 12 Mar 12:31 PDT 2015, Lina Iyer wrote:
> >
> >> On Fri, Feb 27 2015 at 15:30 -0700, Bjorn Andersson wrote:
[..]
> >> >+#define QCOM_MUTEX_NUM_LOCKS	32
> >>
> >> Also, talking to Jeff it seems like that out of the 32 locks defined
> >> only 8 is accessible from Linux. So its unnecessary and probably
> >> incorrect to assume that there are 32 locks available.
> >>
> >
> >The hardware block have 32 locks and the decision regarding which locks
> >this particular Linux system is allowed to access is configuration.
> >
> Understood. But while the hardware may support it, it may be right for
> Linux to be allowed to configure, giving a false sense of number of
> locks.
> 

You're not just randomly allocating these locks, the "sense of number of
locks" is most likely carried in an Excel sheet within Qualcomm.

Obviously the number 8 is arbitrary and a change of it is a question of
"system configuration" and not a matter of changing the implementation
of this device driver.

Regards,
Bjorn

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Tim Bird]

On Thu, Mar 12, 2015 at 2:29 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Mark, Rob,
>
> On Sat, Feb 28, 2015 at 12:30 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>> Add binding documentation for the Qualcomm Hardware Mutex.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> ---
>>
>> I think the conclusion on the dt binding discussion for hwspinlocks was that
>> we're down to having the #hwlock-cells intact. So this version includes that,
>> but non of the other previously discussed properties.
>>
>> Changes since v5:
>> - Extracted the dt binding documentation into a separate patch
>> - Moved the driver to consume a syscon
>> - Dropped previously suggested generic hwlock dt bindings
>>
>>  .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
>
> Could you please ack this one patch from Bjorn?
>
> I'll need your Ack before I can this forward.

I didn't see an Ack from Mark or Rob.  But I did see a question from
Mark and response from Bjorn.

Ohad - did you take this or are you still waiting for something?

Who should I pester about this? :-)
 -- Tim

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Thu, Apr 2, 2015 at 12:32 AM, Tim Bird <tbird20d@gmail.com> wrote:
> I didn't see an Ack from Mark or Rob.  But I did see a question from
> Mark and response from Bjorn.
>
> Ohad - did you take this or are you still waiting for something?
>
> Who should I pester about this? :-)

Sorry, I can't take this without a DT ack.

Ohad.

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Tim Bird]

On Wed, Apr 1, 2015 at 9:40 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Thu, Apr 2, 2015 at 12:32 AM, Tim Bird <tbird20d@gmail.com> wrote:
>> I didn't see an Ack from Mark or Rob.  But I did see a question from
>> Mark and response from Bjorn.
>>
>> Ohad - did you take this or are you still waiting for something?
>>
>> Who should I pester about this? :-)
>
> Sorry, I can't take this without a DT ack.

Hmmm.

The policy seems to be:
     "For driver (not subsystem) bindings: If you are comfortable with the
     binding, and it hasn't received an Acked-by from the devicetree
     maintainers after a few weeks, go ahead and take it."

The syscon property is only relative to the qcom hwspinlock driver,
(unless I'm missing something) and both Qualcomm and Sony devs are
OK with it.  So while an ACK from the DT side would be nice, I don't
think it's required.  This is exactly the type of delay that is really
holding up a lot of out-of-tree code.

But, in case that doesn't convince you...

Mark or Rob (or someone, anyone, over on the DT side)...

Can you please ACK or NAK the following binding?   This particular driver
is at the bottom of a very large dependency tree for out-of-tree code
for Qualcomm.

Thanks,
  -- Tim

---

Add binding documentation for the Qualcomm Hardware Mutex.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

I think the conclusion on the dt binding discussion for hwspinlocks was that
we're down to having the #hwlock-cells intact. So this version includes that,
but none of the other previously discussed properties.

Changes since v5:
- Extracted the dt binding documentation into a separate patch
- Moved the driver to consume a syscon
- Dropped previously suggested generic hwlock dt bindings

 .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt

diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
new file mode 100644
index 0000000..28ade7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
@@ -0,0 +1,39 @@
+Qualcomm Hardware Mutex Block:
+
+The hardware block provides mutexes utilized between different processors on
+the SoC as part of the communication protocol used by these processors.
+
+- compatible:
+    Usage: required
+    Value type: <string>
+    Definition: must be one of:
+            "qcom,sfpb-mutex",
+            "qcom,tcsr-mutex"
+
+- syscon:
+    Usage: required
+    Value type: <prop-encoded-array>
+    Definition: one cell containing:
+            syscon phandle
+            offset of the hwmutex block within the syscon
+            stride of the hwmutex registers
+
+- #hwlock-cells:
+    Usage: required
+    Value type: <u32>
+    Definition: must be 1, the specified cell represent the lock id
+            (hwlock standard property, see hwlock.txt)
+
+Example:
+
+        tcsr: syscon@1a400000 {
+        compatible = "qcom,tcsr-msm8974", "syscon";
+        reg = <0xfd484000 0x2000>;
+    };
+
+    hwlock@fd484000 {
+        compatible = "qcom,tcsr-mutex";
+        syscon = <&tcsr 0 0x80>;
+
+        #hwlock-cells = <1>;
+    };
--

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Thu, Apr 2, 2015 at 9:11 PM, Tim Bird <tbird20d@gmail.com> wrote:
> On Wed, Apr 1, 2015 at 9:40 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Sorry, I can't take this without a DT ack.
>
> Hmmm.
>
> The policy seems to be:
>      "For driver (not subsystem) bindings: If you are comfortable with the
>      binding, and it hasn't received an Acked-by from the devicetree
>      maintainers after a few weeks, go ahead and take it."
>
> The syscon property is only relative to the qcom hwspinlock driver,
> (unless I'm missing something) and both Qualcomm and Sony devs are
> OK with it.  So while an ACK from the DT side would be nice, I don't
> think it's required.  This is exactly the type of delay that is really
> holding up a lot of out-of-tree code.

Sorry, I do prefer to make sure Mark is OK with this devicetree patch,
especially since it wasn't clear whether Mark is entirely comfortable
with it in his last response.

Thanks,
Ohad.

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Tim Bird]

On Fri, Apr 3, 2015 at 6:55 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Thu, Apr 2, 2015 at 9:11 PM, Tim Bird <tbird20d@gmail.com> wrote:
>> On Wed, Apr 1, 2015 at 9:40 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> Sorry, I can't take this without a DT ack.
>>
>> Hmmm.
>>
>> The policy seems to be:
>>      "For driver (not subsystem) bindings: If you are comfortable with the
>>      binding, and it hasn't received an Acked-by from the devicetree
>>      maintainers after a few weeks, go ahead and take it."
>>
>> The syscon property is only relative to the qcom hwspinlock driver,
>> (unless I'm missing something) and both Qualcomm and Sony devs are
>> OK with it.  So while an ACK from the DT side would be nice, I don't
>> think it's required.  This is exactly the type of delay that is really
>> holding up a lot of out-of-tree code.
>
> Sorry, I do prefer to make sure Mark is OK with this devicetree patch,
> especially since it wasn't clear whether Mark is entirely comfortable
> with it in his last response.

Just to be clear - do you personally have any objections to the patch?
Are we now stuck in limbo until such time as the device tree maintainers
get around to us?

I'm worried about this status because my understanding is that the
DT maintainers are hugely backlogged and let a lot of stuff drop on the
floor.  In particular, see slide 21 of the following presentation:
http://www.elinux.org/images/0/0a/The_Device_Tree_as_a_Stable_ABI-_A_Fairy_Tale%3F.pdf
That graph shows that the DT maintainers are falling way behind in
their reviews and ACKs.

The policy of not waiting for an ACK from device tree maintainers
was specifically created to help the situation we are now in, and yet
you seem to be unwilling to follow it.  This is extremely frustrating.

One idea we discussed at a recent meeting on mainlining was to submit
SoC-blocking items to drivers/staging.  Then, stuff is at least in the tree
and can be tested by others pending some approval.  Do you have any
opinion on that?

Is there any way to move ahead?  Or are we doomed to just sit around
and wait indefinitely? For the record, after what amount of time without
a DT ACK would you consider accepting this (if ever)?

Another idea I'm considering is to write our own hwspinlock layer
and become the maintainer of that, so we're not blocked by you.
At this point, the value of using your hwspinlock framework
is vastly outweighed by the negative effect is has on our mainlining
effort.

Regards,

 -- Tim Bird
Senior Software Engineer, Sony Mobile
Architecture Group Chair, CE Workgroup, Linux Foundation

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Mon, Apr 6, 2015 at 7:22 PM, Tim Bird <tbird20d@gmail.com> wrote:
> On Fri, Apr 3, 2015 at 6:55 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Thu, Apr 2, 2015 at 9:11 PM, Tim Bird <tbird20d@gmail.com> wrote:
>>> On Wed, Apr 1, 2015 at 9:40 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>>> Sorry, I can't take this without a DT ack.
>>>
>>> Hmmm.
>>>
>>> The policy seems to be:
>>>      "For driver (not subsystem) bindings: If you are comfortable with the
>>>      binding, and it hasn't received an Acked-by from the devicetree
>>>      maintainers after a few weeks, go ahead and take it."
>>>
>>> The syscon property is only relative to the qcom hwspinlock driver,
>>> (unless I'm missing something) and both Qualcomm and Sony devs are
>>> OK with it.  So while an ACK from the DT side would be nice, I don't
>>> think it's required.  This is exactly the type of delay that is really
>>> holding up a lot of out-of-tree code.
>>
>> Sorry, I do prefer to make sure Mark is OK with this devicetree patch,
>> especially since it wasn't clear whether Mark is entirely comfortable
>> with it in his last response.
>
> Just to be clear - do you personally have any objections to the patch?

No, but this patch is for a folder I don't maintain so I prefer
someone who does to take a look.

Mark did take a look, and said he's confused by this patch (see this thread).

Do you want me to ignore him and just send it to Linus anyway?

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Bjorn Andersson]

On Mon, Apr 6, 2015 at 9:31 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Mon, Apr 6, 2015 at 7:22 PM, Tim Bird <tbird20d@gmail.com> wrote:
>> On Fri, Apr 3, 2015 at 6:55 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> On Thu, Apr 2, 2015 at 9:11 PM, Tim Bird <tbird20d@gmail.com> wrote:
>>>> On Wed, Apr 1, 2015 at 9:40 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>>>> Sorry, I can't take this without a DT ack.
>>>>
>>>> Hmmm.
>>>>
>>>> The policy seems to be:
>>>>      "For driver (not subsystem) bindings: If you are comfortable with the
>>>>      binding, and it hasn't received an Acked-by from the devicetree
>>>>      maintainers after a few weeks, go ahead and take it."
>>>>
>>>> The syscon property is only relative to the qcom hwspinlock driver,
>>>> (unless I'm missing something) and both Qualcomm and Sony devs are
>>>> OK with it.  So while an ACK from the DT side would be nice, I don't
>>>> think it's required.  This is exactly the type of delay that is really
>>>> holding up a lot of out-of-tree code.
>>>
>>> Sorry, I do prefer to make sure Mark is OK with this devicetree patch,
>>> especially since it wasn't clear whether Mark is entirely comfortable
>>> with it in his last response.
>>
>> Just to be clear - do you personally have any objections to the patch?
>
> No, but this patch is for a folder I don't maintain so I prefer
> someone who does to take a look.
>
> Mark did take a look, and said he's confused by this patch (see this thread).
>
> Do you want me to ignore him and just send it to Linus anyway?

Ohad, Tim,

For this patch to be useful to us we also need Suman's DT patch, so we
should try to get them both in asap.

Based on the long discussion we had on one of the previous iterations
of Suman's DT binding, with the DT maintainers I believe that it would
be fine to move along and sent Suman's patches to Linus - without an
explicit Ack from the DT guys (or did I just miss one?).

Regarding this patch I agree with Ohad that it would be good if we
verify that Mark's question is answered before moving on. So I will
reach out to him to see if he has any remaining concerns.

Regards,
Bjorn

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Mon, Apr 6, 2015 at 7:48 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> Based on the long discussion we had on one of the previous iterations
> of Suman's DT binding, with the DT maintainers I believe that it would
> be fine to move along and sent Suman's patches to Linus - without an
> explicit Ack from the DT guys (or did I just miss one?).

Thanks Bjorn.

Mark, are you OK with the latest iteration from Suman? it would be
nice to get your +1 just to make sure we don't merge stuff you're
uncomfortable with.

Thanks!
Ohad.

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Mon, Apr 6, 2015 at 10:04 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Mark, are you OK with the latest iteration from Suman? it would be
> nice to get your +1 just to make sure we don't merge stuff you're
> uncomfortable with.

Quick update:

As Tim pointed out, we can move forward with the driver binding patch
according to the process described under II.2 of [1]. Both Bjorn and
myself would still prefer to make sure Mark is satisfied with the
response Bjorn sent to Mark's question, but we understand if Mark is
swamped and we eventually would proceed according to the DT's
submitting-patches guidance below. Tim, thanks for pointing that out
as I wasn't aware of this.

What we probably do need a DT ack on is the hwspinlock subsystem
binding submitted by Suman, again according to the process described
under II.2 of [1]: "Subsystem bindings (anything affecting more than a
single device): then getting a devicetree maintainer to review it is
required".

Mark and Rob: thanks so much for all your help so far as you have
substantially helped shaping the hwspinlock binding. Please let us
know if you are satisfied with Suman's latest iteration, still prefer
to take another look at it, or are too swamped. If the latter, then
maybe we can ask Kumar to take a look, as this seems to be blocking
Qualcomm's upstream roadmap.

Thanks,
Ohad.

[1] Documentation/devicetree/bindings/submitting-patches.txt

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Kumar Gala]

> On Feb 27, 2015, at 4:30 PM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote:
> 
> Add binding documentation for the Qualcomm Hardware Mutex.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> —
> 

Acked-by: Kumar Gala <galak@codeaurora.org>

- k


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Ohad Ben-Cohen]

On Tue, Apr 14, 2015 at 10:18 PM, Kumar Gala <galak@codeaurora.org> wrote:
>> On Feb 27, 2015, at 4:30 PM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote:
>>
>> Add binding documentation for the Qualcomm Hardware Mutex.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> —
>>
>
> Acked-by: Kumar Gala <galak@codeaurora.org>

Perfect, thanks a lot, Kumar.

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Rob Herring]

On Mon, Apr 13, 2015 at 5:23 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Mon, Apr 6, 2015 at 10:04 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> Mark, are you OK with the latest iteration from Suman? it would be
>> nice to get your +1 just to make sure we don't merge stuff you're
>> uncomfortable with.
>
> Quick update:
>
> As Tim pointed out, we can move forward with the driver binding patch
> according to the process described under II.2 of [1]. Both Bjorn and
> myself would still prefer to make sure Mark is satisfied with the
> response Bjorn sent to Mark's question, but we understand if Mark is
> swamped and we eventually would proceed according to the DT's
> submitting-patches guidance below. Tim, thanks for pointing that out
> as I wasn't aware of this.
>
> What we probably do need a DT ack on is the hwspinlock subsystem
> binding submitted by Suman, again according to the process described
> under II.2 of [1]: "Subsystem bindings (anything affecting more than a
> single device): then getting a devicetree maintainer to review it is
> required".

Right, you can't really merge this driver binding until the binding is
done and acked. Your binding would be referring to a binding doc that
doesn't exist in the tree if you merge this.

> Mark and Rob: thanks so much for all your help so far as you have
> substantially helped shaping the hwspinlock binding. Please let us
> know if you are satisfied with Suman's latest iteration, still prefer
> to take another look at it, or are too swamped. If the latter, then
> maybe we can ask Kumar to take a look, as this seems to be blocking
> Qualcomm's upstream roadmap.

While I think the binding looks fine, I'm not going to go around Mark
either. He's invested the most thought in this (of DT maintainers) and
should be the one to ack it. The part I'm not clear what the purpose
of "pool-hwlock" was.

Rob

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Mark Rutland]

On Fri, Feb 27, 2015 at 10:30:16PM +0000, Bjorn Andersson wrote:
> Add binding documentation for the Qualcomm Hardware Mutex.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> I think the conclusion on the dt binding discussion for hwspinlocks was that
> we're down to having the #hwlock-cells intact. So this version includes that,
> but non of the other previously discussed properties.
> 
> Changes since v5:
> - Extracted the dt binding documentation into a separate patch
> - Moved the driver to consume a syscon
> - Dropped previously suggested generic hwlock dt bindings
> 
>  .../devicetree/bindings/hwlock/qcom-hwspinlock.txt | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> new file mode 100644
> index 0000000..28ade7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> @@ -0,0 +1,39 @@
> +Qualcomm Hardware Mutex Block:
> +
> +The hardware block provides mutexes utilized between different processors on
> +the SoC as part of the communication protocol used by these processors.
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,sfpb-mutex",
> +		    "qcom,tcsr-mutex"
> +
> +- syscon:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: one cell containing:
> +		    syscon phandle
> +		    offset of the hwmutex block within the syscon
> +		    stride of the hwmutex registers
> +
> +- #hwlock-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: must be 1, the specified cell represent the lock id
> +		    (hwlock standard property, see hwlock.txt)

I guess that this is a linear ID, matching some ID in a data sheet? It
would be nice to have something a little more explicit as to what the
valid values of the cell are.

Otherwise this looks fine, and seems to pair fine with the generic
bindings. The nit above can be corrected with a followup.

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> +
> +Example:
> +
> +        tcsr: syscon@1a400000 {
> +		compatible = "qcom,tcsr-msm8974", "syscon";
> +		reg = <0xfd484000 0x2000>;
> +	};
> +
> +	hwlock@fd484000 {
> +		compatible = "qcom,tcsr-mutex";
> +		syscon = <&tcsr 0 0x80>;
> +
> +		#hwlock-cells = <1>;
> +	};
> -- 
> 1.8.2.2
> 
> 

Re: [PATCH v6 1/2] DT: hwspinlock: Add binding documentation for Qualcomm hwmutex

[Mark Rutland]

> The part I'm not clear what the purpose of "pool-hwlock" was.

One use-case was a set of hwlocks having no fixed purpose, and being
available for dynamic allocation between the OS and other entities (e.g.
some RTOS on another core).

The set of locks forming a reusable pool, and any information associated
with them (e.g. the logical IDs used by the other entity) are a property
of that interface rather than the hwlock provider.

So you'd describe those pools of locks in some interface-specific
manner, consuming hwlocks from a set of hwlock providers.

Hopefully that doesn't maek things less clear...

Mark.