[PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

From: Kumar Gala <galak@codeaurora.org>

Add driver for Qualcomm Hardware Mutex block that exists on newer
Qualcomm SoCs.

Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Eric Holmberg <eholmber@codeaurora.org>
Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Kumar Gala <galak@codeaurora.org>
[bjorn: added pm_runtime calls, from Courtney,
	added sfpb-mutex compatible,
	updated DT binding documentation formatting,
	based stride on resource size instead of hardcoded values,
	replaced msm prefix with qcom,
	cleaned up includes]
Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

We need this driver to add support for the shared memory manager, so I'm
reviving Kumars patch from a year ago, with some additional sprinkles on top.

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.

 .../devicetree/bindings/hwlock/qcom-hwspinlock.txt |   35 +++++
 drivers/hwspinlock/Kconfig                         |   11 ++
 drivers/hwspinlock/Makefile                        |    1 +
 drivers/hwspinlock/qcom_hwspinlock.c               |  147 ++++++++++++++++++++
 4 files changed, 194 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
 create mode 100644 drivers/hwspinlock/qcom_hwspinlock.c

diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
new file mode 100644
index 0000000..27c7c80
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
@@ -0,0 +1,35 @@
+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"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address and size of the mutex registers
+
+- reg-names:
+	Usage: required
+	Value type: <string>
+	Definition: must be "mutex-base"
+
+- qcom,num-locks:
+	Usage: required
+	Value type: <u32>
+	Definition: the number of locks/mutex available in this block
+
+Example:
+
+	hwlock@fd484000 {
+		compatible = "qcom,tcsr-mutex";
+		reg = <0xfd484000 0x1000>;
+		reg-names = "mutex-base";
+		qcom,num-locks = <32>;
+	};
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 3612cb5..af4c7e6 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -8,6 +8,17 @@ config HWSPINLOCK
 
 menu "Hardware Spinlock drivers"
 
+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 HWSPINLOCK_OMAP
 	tristate "OMAP Hardware Spinlock device"
 	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index 93eb64b..f3bff48 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_HWSPINLOCK)		+= hwspinlock_core.o
+obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_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..a9e5fa4
--- /dev/null
+++ b/drivers/hwspinlock/qcom_hwspinlock.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014, 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/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include "hwspinlock_internal.h"
+
+#define SPINLOCK_ID_APPS_PROC	1
+#define BASE_ID			0
+
+static int qcom_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
+
+	return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
+}
+
+static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+	u32 lock_owner;
+
+	lock_owner = readl_relaxed(lock_addr);
+	if (lock_owner != SPINLOCK_ID_APPS_PROC) {
+		pr_err("%s: spinlock not owned by us (actual owner is %d)\n",
+				__func__, lock_owner);
+	}
+
+	writel_relaxed(0, lock_addr);
+}
+
+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 device_node *node = pdev->dev.of_node;
+	struct hwspinlock_device *bank;
+	struct hwspinlock *hwlock;
+	struct resource *res;
+	void __iomem *iobase;
+	size_t array_size;
+	long stride, i;
+	u32 num_locks;
+	int ret;
+
+	ret = of_property_read_u32(node, "qcom,num-locks", &num_locks);
+	if (ret || num_locks == 0)
+		return -ENODEV;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mutex-base");
+	iobase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(iobase))
+		return PTR_ERR(iobase);
+
+	array_size = num_locks * sizeof(*hwlock);
+	bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
+	if (!bank)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, bank);
+
+	stride = (long)resource_size(res) / num_locks;
+	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
+		hwlock->priv = iobase + i * stride;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = hwspin_lock_register(bank, &pdev->dev, &qcom_hwspinlock_ops,
+						BASE_ID, 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");
+MODULE_AUTHOR("Kumar Gala <galak@codeaurora.org>");
+MODULE_AUTHOR("Jeffrey Hugo <jhugo@codeaurora.org>");
+MODULE_AUTHOR("Eric Holmberg <eholmber@codeaurora.org>");
-- 
1.7.9.5

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Jeffrey Hugo]

On 9/2/2014 2:04 PM, Bjorn Andersson wrote:
> From: Kumar Gala <galak@codeaurora.org>
>
> Add driver for Qualcomm Hardware Mutex block that exists on newer
> Qualcomm SoCs.
>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Eric Holmberg <eholmber@codeaurora.org>
> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> [bjorn: added pm_runtime calls, from Courtney,
> 	added sfpb-mutex compatible,
> 	updated DT binding documentation formatting,
> 	based stride on resource size instead of hardcoded values,
> 	replaced msm prefix with qcom,
> 	cleaned up includes]
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
<snip>

For what its worth, this looks good to me.

Jeffrey Hugo
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation.

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Kumar Gala]

On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:

> From: Kumar Gala <galak@codeaurora.org>
> 
> Add driver for Qualcomm Hardware Mutex block that exists on newer
> Qualcomm SoCs.
> 
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Eric Holmberg <eholmber@codeaurora.org>
> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Kumar Gala <galak@codeaurora.org>
> [bjorn: added pm_runtime calls, from Courtney,
> 	added sfpb-mutex compatible,
> 	updated DT binding documentation formatting,
> 	based stride on resource size instead of hardcoded values,
> 	replaced msm prefix with qcom,
> 	cleaned up includes]
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
> 
> We need this driver to add support for the shared memory manager, so I'm
> reviving Kumars patch from a year ago, with some additional sprinkles on top.
> 
> 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

I know Jeff prefers this method of computing stride, but I’m not a fan as there isn’t a reason one could adjust qcom,num-locks in the dt for some reason and leave regs alone.

> 
> 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.
> 
> .../devicetree/bindings/hwlock/qcom-hwspinlock.txt |   35 +++++
> drivers/hwspinlock/Kconfig                         |   11 ++
> drivers/hwspinlock/Makefile                        |    1 +
> drivers/hwspinlock/qcom_hwspinlock.c               |  147 ++++++++++++++++++++
> 4 files changed, 194 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> create mode 100644 drivers/hwspinlock/qcom_hwspinlock.c
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> new file mode 100644
> index 0000000..27c7c80
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> @@ -0,0 +1,35 @@
> +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”

I dont get the purpose of having different compatible strings if there is no difference in the code between them.

> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: base address and size of the mutex registers
> +
> +- reg-names:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be "mutex-base"
> +
> +- qcom,num-locks:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: the number of locks/mutex available in this block
> +
> +Example:
> +
> +	hwlock@fd484000 {
> +		compatible = "qcom,tcsr-mutex";
> +		reg = <0xfd484000 0x1000>;
> +		reg-names = "mutex-base";
> +		qcom,num-locks = <32>;
> +	};
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 3612cb5..af4c7e6 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -8,6 +8,17 @@ config HWSPINLOCK
> 
> menu "Hardware Spinlock drivers"
> 
> +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 HWSPINLOCK_OMAP
> 	tristate "OMAP Hardware Spinlock device"
> 	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index 93eb64b..f3bff48 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -3,5 +3,6 @@
> #
> 
> obj-$(CONFIG_HWSPINLOCK)		+= hwspinlock_core.o
> +obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
> obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_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..a9e5fa4
> --- /dev/null
> +++ b/drivers/hwspinlock/qcom_hwspinlock.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2014, 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/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +#define SPINLOCK_ID_APPS_PROC	1
> +#define BASE_ID			0
> +
> +static int qcom_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
> +
> +	return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
> +}
> +
> +static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +	u32 lock_owner;
> +
> +	lock_owner = readl_relaxed(lock_addr);
> +	if (lock_owner != SPINLOCK_ID_APPS_PROC) {
> +		pr_err("%s: spinlock not owned by us (actual owner is %d)\n",
> +				__func__, lock_owner);
> +	}
> +
> +	writel_relaxed(0, lock_addr);
> +}
> +
> +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 device_node *node = pdev->dev.of_node;
> +	struct hwspinlock_device *bank;
> +	struct hwspinlock *hwlock;
> +	struct resource *res;
> +	void __iomem *iobase;
> +	size_t array_size;
> +	long stride, i;
> +	u32 num_locks;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "qcom,num-locks", &num_locks);
> +	if (ret || num_locks == 0)
> +		return -ENODEV;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mutex-base");
> +	iobase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(iobase))
> +		return PTR_ERR(iobase);
> +
> +	array_size = num_locks * sizeof(*hwlock);
> +	bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
> +	if (!bank)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, bank);
> +
> +	stride = (long)resource_size(res) / num_locks;
> +	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
> +		hwlock->priv = iobase + i * stride;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = hwspin_lock_register(bank, &pdev->dev, &qcom_hwspinlock_ops,
> +						BASE_ID, 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");
> +MODULE_AUTHOR("Kumar Gala <galak@codeaurora.org>");
> +MODULE_AUTHOR("Jeffrey Hugo <jhugo@codeaurora.org>");
> +MODULE_AUTHOR("Eric Holmberg <eholmber@codeaurora.org>");
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Wed 03 Sep 05:49 PDT 2014, Kumar Gala wrote:

> 
> On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> 
> > 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
> 
> I know Jeff prefers this method of computing stride, but I’m not a fan as
> there isn’t a reason one could adjust qcom,num-locks in the dt for some
> reason and leave regs alone.
> 

All the current platform it's 32 consecutive mutexes with either 4 or 128 byte
stride, so encoding it as data either way works fine. The hardware you're
trying to describe with your dt is the addresses that spans your mutex
registers and how many there are. So from the HW/dts pov I don't see why you
would like to do this.

Then looking in the caf code, there is a limit of max 8 mutexes. So apparently
there is some sort of usecase, I just don't know what or if it's valid from a
dt pov.


Going to that future awesome SoCs where it's still called tcsr-mutex, but with
a stride of 4096 bytes makes me wonder; is that really a consecutive 128kb with
nothing else in-between that we can ioremap?
I.e. can we really reuse this driver straight off for that SoC?

> > diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> > +- compatible:
> > +	Usage: required
> > +	Value type: <string>
> > +	Definition: must be one of:
> > +		    "qcom,sfpb-mutex",
> > +		    "qcom,tcsr-mutex”
> 
> I dont get the purpose of having different compatible strings if there is no
> difference in the code between them.
> 

The semantics are the same, but there are no mutex registers in the tcsr block
in e.g 8960, so the name is just missleading. I assume that's why you didn't
follow caf and used the compatible "sfpb" in the first place?

Regards,
Bjorn

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Kumar Gala]

On Sep 3, 2014, at 9:55 AM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:

> On Wed 03 Sep 05:49 PDT 2014, Kumar Gala wrote:
> 
>> 
>> On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
>> 
>>> 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
>> 
>> I know Jeff prefers this method of computing stride, but I’m not a fan as
>> there isn’t a reason one could adjust qcom,num-locks in the dt for some
>> reason and leave regs alone.
>> 
> 
> All the current platform it's 32 consecutive mutexes with either 4 or 128 byte
> stride, so encoding it as data either way works fine. The hardware you're
> trying to describe with your dt is the addresses that spans your mutex
> registers and how many there are. So from the HW/dts pov I don't see why you
> would like to do this.
> 
> Then looking in the caf code, there is a limit of max 8 mutexes. So apparently
> there is some sort of usecase, I just don't know what or if it's valid from a
> dt pov.

I believe not all the mutexes are meant for the cores running linux.  However, I think we just expect linux to play nice and not touch anything it isn’t using explicitly.

> Going to that future awesome SoCs where it's still called tcsr-mutex, but with
> a stride of 4096 bytes makes me wonder; is that really a consecutive 128kb with
> nothing else in-between that we can ioremap?

think 64-bit machines with more address space to burn and wanting to separate resources to use MMUs for protection.

> I.e. can we really reuse this driver straight off for that SoC?

I dont see why not.

>>> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
>>> +- compatible:
>>> +	Usage: required
>>> +	Value type: <string>
>>> +	Definition: must be one of:
>>> +		    "qcom,sfpb-mutex",
>>> +		    "qcom,tcsr-mutex”
>> 
>> I dont get the purpose of having different compatible strings if there is no
>> difference in the code between them.
>> 
> 
> The semantics are the same, but there are no mutex registers in the tcsr block
> in e.g 8960, so the name is just missleading. I assume that's why you didn't
> follow caf and used the compatible "sfpb" in the first place?

What do you expect the 8960 dt node to look like?  I’m not 100% against ‘sfpb’

I’m feel like we we should use compat for stride, so we’d end up with something like:

qcom,sfpb-mutex: stride 4 bytes, base: 0x01200604, reset: 0x01200600
qcom,tcsr-mutex: stride 128 bytes, base: 0xFD484000, reset: 0xFD485380
qcom,tcsr-4k-mutex: stride 4k bytes, base: 0x740000, reset: 0x767000

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Bjorn Andersson]

On Wed 03 Sep 08:22 PDT 2014, Kumar Gala wrote:

> 
> On Sep 3, 2014, at 9:55 AM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> 
> > On Wed 03 Sep 05:49 PDT 2014, Kumar Gala wrote:
> > 
> >> 
> >> On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
> >> 
> >>> 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
> >> 
> >> I know Jeff prefers this method of computing stride, but I’m not a fan as
> >> there isn’t a reason one could adjust qcom,num-locks in the dt for some
> >> reason and leave regs alone.
> >> 
> > 
> > All the current platform it's 32 consecutive mutexes with either 4 or 128 byte
> > stride, so encoding it as data either way works fine. The hardware you're
> > trying to describe with your dt is the addresses that spans your mutex
> > registers and how many there are. So from the HW/dts pov I don't see why you
> > would like to do this.
> > 
> > Then looking in the caf code, there is a limit of max 8 mutexes. So apparently
> > there is some sort of usecase, I just don't know what or if it's valid from a
> > dt pov.
> 
> I believe not all the mutexes are meant for the cores running linux.
> However, I think we just expect linux to play nice and not touch anything it
> isn’t using explicitly.
> 

I would expect so too.

One problem I see is that it's not very robust relying on the relationship
between reg and num-locks. I consider this an implementation detail leaking
into the dt binding and it's not described enough currently...

> > Going to that future awesome SoCs where it's still called tcsr-mutex, but with
> > a stride of 4096 bytes makes me wonder; is that really a consecutive 128kb with
> > nothing else in-between that we can ioremap?
> 
> think 64-bit machines with more address space to burn and wanting to separate
> resources to use MMUs for protection.
> 

Makes sense, I just don't have any documentation verifying this.

> > I.e. can we really reuse this driver straight off for that SoC?
> 
> I dont see why not.
> 

As long as the space inbetween is just wasted, there is no issue.

> >>> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
> >>> +- compatible:
> >>> +	Usage: required
> >>> +	Value type: <string>
> >>> +	Definition: must be one of:
> >>> +		    "qcom,sfpb-mutex",
> >>> +		    "qcom,tcsr-mutex”
> >> 
> >> I dont get the purpose of having different compatible strings if there is no
> >> difference in the code between them.
> >> 
> > 
> > The semantics are the same, but there are no mutex registers in the tcsr block
> > in e.g 8960, so the name is just missleading. I assume that's why you didn't
> > follow caf and used the compatible "sfpb" in the first place?
> 
> What do you expect the 8960 dt node to look like?  I’m not 100% against ‘sfpb’
> 
> I’m feel like we we should use compat for stride, so we’d end up with something like:
> 
> qcom,sfpb-mutex: stride 4 bytes, base: 0x01200604, reset: 0x01200600
> qcom,tcsr-mutex: stride 128 bytes, base: 0xFD484000, reset: 0xFD485380
> qcom,tcsr-4k-mutex: stride 4k bytes, base: 0x740000, reset: 0x767000
> 

Maybe these are the best names for the 3 hw blocks after all.

The alternative would be either to encode the platform name in the compatible
or adding the stride as a separate property. The first is waste and the second
doesn't describe how hw is connected. So netiher are good alternatives.


I think your suggestion is reasonable and will move the stride back into
compat. It's the most robust solution.

Is the 4k block finalized (don't see it in 8994 docs)? Should I add it to the
driver now as well?

Thanks for your input!

Regards,
Bjorn

Re: [PATCH v3] hwspinlock: qcom: Add support for Qualcomm HW Mutex block

[Jeffrey Hugo]

On 9/3/2014 10:34 AM, Bjorn Andersson wrote:
> On Wed 03 Sep 08:22 PDT 2014, Kumar Gala wrote:
>
>>
>> On Sep 3, 2014, at 9:55 AM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
>>
>>> On Wed 03 Sep 05:49 PDT 2014, Kumar Gala wrote:
>>>
>>>>
>>>> On Sep 2, 2014, at 3:04 PM, Bjorn Andersson <Bjorn.Andersson@sonymobile.com> wrote:
>>>>
>>>>> 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
>>>>
>>>> I know Jeff prefers this method of computing stride, but I’m not a fan as
>>>> there isn’t a reason one could adjust qcom,num-locks in the dt for some
>>>> reason and leave regs alone.
>>>>
>>>
>>> All the current platform it's 32 consecutive mutexes with either 4 or 128 byte
>>> stride, so encoding it as data either way works fine. The hardware you're
>>> trying to describe with your dt is the addresses that spans your mutex
>>> registers and how many there are. So from the HW/dts pov I don't see why you
>>> would like to do this.
>>>
>>> Then looking in the caf code, there is a limit of max 8 mutexes. So apparently
>>> there is some sort of usecase, I just don't know what or if it's valid from a
>>> dt pov.
>>
>> I believe not all the mutexes are meant for the cores running linux.
>> However, I think we just expect linux to play nice and not touch anything it
>> isn’t using explicitly.
>>
>
> I would expect so too.

Currently, and I expect this to continue, the first 8 mutexes involve 
usecases which involve Linux.  The remaining implemented locks support 
usecases which do not involve the Linux cores.  Generally we expect 
Linux to play nice, however it is possible to have hardware memory 
protection units in the SoC configured to prevent the Linux cores from 
accessing those upper locks.  From the global SoC pov, Linux should only 
"see" the first 8.

>
> One problem I see is that it's not very robust relying on the relationship
> between reg and num-locks. I consider this an implementation detail leaking
> into the dt binding and it's not described enough currently...
>
>>> Going to that future awesome SoCs where it's still called tcsr-mutex, but with
>>> a stride of 4096 bytes makes me wonder; is that really a consecutive 128kb with
>>> nothing else in-between that we can ioremap?
>>
>> think 64-bit machines with more address space to burn and wanting to separate
>> resources to use MMUs for protection.
>>
>
> Makes sense, I just don't have any documentation verifying this.
>
>>> I.e. can we really reuse this driver straight off for that SoC?
>>
>> I dont see why not.
>>
>
> As long as the space inbetween is just wasted, there is no issue.

Have a look at the msm8916.dtsi in the downstream repository.  I think 
it'll help with your questions concerning the 0x1000 stride case.

>
>>>>> diff --git a/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/qcom-hwspinlock.txt
>>>>> +- compatible:
>>>>> +	Usage: required
>>>>> +	Value type: <string>
>>>>> +	Definition: must be one of:
>>>>> +		    "qcom,sfpb-mutex",
>>>>> +		    "qcom,tcsr-mutex”
>>>>
>>>> I dont get the purpose of having different compatible strings if there is no
>>>> difference in the code between them.
>>>>
>>>
>>> The semantics are the same, but there are no mutex registers in the tcsr block
>>> in e.g 8960, so the name is just missleading. I assume that's why you didn't
>>> follow caf and used the compatible "sfpb" in the first place?
>>
>> What do you expect the 8960 dt node to look like?  I’m not 100% against ‘sfpb’
>>
>> I’m feel like we we should use compat for stride, so we’d end up with something like:
>>
>> qcom,sfpb-mutex: stride 4 bytes, base: 0x01200604, reset: 0x01200600
>> qcom,tcsr-mutex: stride 128 bytes, base: 0xFD484000, reset: 0xFD485380
>> qcom,tcsr-4k-mutex: stride 4k bytes, base: 0x740000, reset: 0x767000
>>
>
> Maybe these are the best names for the 3 hw blocks after all.
>
> The alternative would be either to encode the platform name in the compatible
> or adding the stride as a separate property. The first is waste and the second
> doesn't describe how hw is connected. So netiher are good alternatives.
>
>
> I think your suggestion is reasonable and will move the stride back into
> compat. It's the most robust solution.
>
> Is the 4k block finalized (don't see it in 8994 docs)? Should I add it to the
> driver now as well?
>
> Thanks for your input!
>
> Regards,
> Bjorn
>


Jeffrey Hugo
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation.