Add driver for Qualcomm MSM Hardware Mutex block that exists on newer MSM
SoC (MSM8974, etc).
CC: Jeffrey Hugo <[email protected]>
CC: Eric Holmberg <[email protected]>
Signed-off-by: Kumar Gala <[email protected]>
---
.../devicetree/bindings/arm/msm/tcsr-mutex.txt | 20 +++
drivers/hwspinlock/Kconfig | 11 ++
drivers/hwspinlock/Makefile | 1 +
drivers/hwspinlock/msm_hwspinlock.c | 150 +++++++++++++++++++++
4 files changed, 182 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
create mode 100644 drivers/hwspinlock/msm_hwspinlock.c
diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
new file mode 100644
index 0000000..ddd6889
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
@@ -0,0 +1,20 @@
+Qualcomm MSM 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.
+
+Required properties:
+- compatible: should be "qcom,tcsr-mutex"
+- reg: Should contain registers location and length of mutex registers
+- reg-names:
+ "mutex-base" - string to identify mutex registers
+- qcom,num-locks: the number of locks/mutexes supported
+
+Example:
+
+ qcom,tcsr-mutex@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 70637d2..192e939 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -8,6 +8,17 @@ config HWSPINLOCK
menu "Hardware Spinlock drivers"
+config HWSPINLOCK_MSM
+ tristate "MSM Hardware Spinlock device"
+ depends on ARCH_MSM
+ select HWSPINLOCK
+ help
+ Say y here to support the MSM 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
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index 93eb64b..4074c56 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -3,5 +3,6 @@
#
obj-$(CONFIG_HWSPINLOCK) += hwspinlock_core.o
+obj-$(CONFIG_HWSPINLOCK_MSM) += msm_hwspinlock.o
obj-$(CONFIG_HWSPINLOCK_OMAP) += omap_hwspinlock.o
obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o
diff --git a/drivers/hwspinlock/msm_hwspinlock.c b/drivers/hwspinlock/msm_hwspinlock.c
new file mode 100644
index 0000000..c7d80c5
--- /dev/null
+++ b/drivers/hwspinlock/msm_hwspinlock.c
@@ -0,0 +1,150 @@
+/*
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ *
+ * 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/err.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/hwspinlock.h>
+
+#include <asm/io.h>
+
+#include "hwspinlock_internal.h"
+
+#define SPINLOCK_ID_APPS_PROC 1
+#define BASE_ID 0
+
+static int msm_hwspinlock_trylock(struct hwspinlock *lock)
+{
+ void __iomem *lock_addr = lock->priv;
+
+ writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
+ smp_mb();
+ return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
+}
+
+static void msm_hwspinlock_unlock(struct hwspinlock *lock)
+{
+ int lock_owner;
+ void __iomem *lock_addr = lock->priv;
+
+ lock_owner = readl_relaxed(lock_addr);
+ if (lock_owner != SPINLOCK_ID_APPS_PROC) {
+ pr_err("%s: spinlock not owned by Apps (actual owner is %d)\n",
+ __func__, lock_owner);
+ }
+
+ writel_relaxed(0, lock_addr);
+ smp_mb();
+}
+
+static const struct hwspinlock_ops msm_hwspinlock_ops = {
+ .trylock = msm_hwspinlock_trylock,
+ .unlock = msm_hwspinlock_unlock,
+};
+
+static struct of_device_id msm_hwspinlock_of_match[];
+static int msm_hwspinlock_probe(struct platform_device *pdev)
+{
+ int ret, i, stride;
+ u32 num_locks;
+ struct hwspinlock_device *bank;
+ struct hwspinlock *hwlock;
+ struct resource *res;
+ void __iomem *iobase;
+ struct device_node *node = pdev->dev.of_node;
+ const struct of_device_id *match;
+
+ match = of_match_device(msm_hwspinlock_of_match, &pdev->dev);
+ if (!match)
+ return -EINVAL;
+
+ 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);
+
+ bank = devm_kzalloc(&pdev->dev,
+ sizeof(*bank) + num_locks * sizeof(*hwlock), GFP_KERNEL);
+ if (!bank)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, bank);
+
+ for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
+ hwlock->priv = iobase + i * stride;
+
+ ret = hwspin_lock_register(bank, &pdev->dev, &msm_hwspinlock_ops,
+ BASE_ID, num_locks);
+ return ret;
+}
+
+static int msm_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;
+ }
+
+ return 0;
+}
+
+static struct of_device_id msm_hwspinlock_of_match[] = {
+ {
+ .compatible = "qcom,tcsr-mutex",
+ .data = (void *)0x80, /* register stride */
+ },
+ { },
+};
+
+static struct platform_driver msm_hwspinlock_driver = {
+ .probe = msm_hwspinlock_probe,
+ .remove = msm_hwspinlock_remove,
+ .driver = {
+ .name = "msm_hwspinlock",
+ .owner = THIS_MODULE,
+ .of_match_table = msm_hwspinlock_of_match,
+ },
+};
+
+static int __init msm_hwspinlock_init(void)
+{
+ return platform_driver_register(&msm_hwspinlock_driver);
+}
+/* board init code might need to reserve hwspinlocks for predefined purposes */
+postcore_initcall(msm_hwspinlock_init);
+
+static void __exit msm_hwspinlock_exit(void)
+{
+ platform_driver_unregister(&msm_hwspinlock_driver);
+}
+module_exit(msm_hwspinlock_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver for MSM");
+MODULE_AUTHOR("Kumar Gala <[email protected]>");
+MODULE_AUTHOR("Jeffrey Hugo <[email protected]>");
+MODULE_AUTHOR("Eric Holmberg <[email protected]>");
--
1.8.2.1
On Jul 29, 2013, at 3:11 PM, Kumar Gala wrote:
> Add driver for Qualcomm MSM Hardware Mutex block that exists on newer MSM
> SoC (MSM8974, etc).
>
> CC: Jeffrey Hugo <[email protected]>
> CC: Eric Holmberg <[email protected]>
> Signed-off-by: Kumar Gala <[email protected]>
> ---
[ had old device tree mailing list ]
> .../devicetree/bindings/arm/msm/tcsr-mutex.txt | 20 +++
> drivers/hwspinlock/Kconfig | 11 ++
> drivers/hwspinlock/Makefile | 1 +
> drivers/hwspinlock/msm_hwspinlock.c | 150 +++++++++++++++++++++
> 4 files changed, 182 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
> create mode 100644 drivers/hwspinlock/msm_hwspinlock.c
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
> new file mode 100644
> index 0000000..ddd6889
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
> @@ -0,0 +1,20 @@
> +Qualcomm MSM 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.
> +
> +Required properties:
> +- compatible: should be "qcom,tcsr-mutex"
> +- reg: Should contain registers location and length of mutex registers
> +- reg-names:
> + "mutex-base" - string to identify mutex registers
> +- qcom,num-locks: the number of locks/mutexes supported
> +
> +Example:
> +
> + qcom,tcsr-mutex@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 70637d2..192e939 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -8,6 +8,17 @@ config HWSPINLOCK
>
> menu "Hardware Spinlock drivers"
>
> +config HWSPINLOCK_MSM
> + tristate "MSM Hardware Spinlock device"
> + depends on ARCH_MSM
> + select HWSPINLOCK
> + help
> + Say y here to support the MSM 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
> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index 93eb64b..4074c56 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -3,5 +3,6 @@
> #
>
> obj-$(CONFIG_HWSPINLOCK) += hwspinlock_core.o
> +obj-$(CONFIG_HWSPINLOCK_MSM) += msm_hwspinlock.o
> obj-$(CONFIG_HWSPINLOCK_OMAP) += omap_hwspinlock.o
> obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o
> diff --git a/drivers/hwspinlock/msm_hwspinlock.c b/drivers/hwspinlock/msm_hwspinlock.c
> new file mode 100644
> index 0000000..c7d80c5
> --- /dev/null
> +++ b/drivers/hwspinlock/msm_hwspinlock.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * 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/err.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/hwspinlock.h>
> +
> +#include <asm/io.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +#define SPINLOCK_ID_APPS_PROC 1
> +#define BASE_ID 0
> +
> +static int msm_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> + void __iomem *lock_addr = lock->priv;
> +
> + writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
> + smp_mb();
> + return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
> +}
> +
> +static void msm_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> + int lock_owner;
> + void __iomem *lock_addr = lock->priv;
> +
> + lock_owner = readl_relaxed(lock_addr);
> + if (lock_owner != SPINLOCK_ID_APPS_PROC) {
> + pr_err("%s: spinlock not owned by Apps (actual owner is %d)\n",
> + __func__, lock_owner);
> + }
> +
> + writel_relaxed(0, lock_addr);
> + smp_mb();
> +}
> +
> +static const struct hwspinlock_ops msm_hwspinlock_ops = {
> + .trylock = msm_hwspinlock_trylock,
> + .unlock = msm_hwspinlock_unlock,
> +};
> +
> +static struct of_device_id msm_hwspinlock_of_match[];
> +static int msm_hwspinlock_probe(struct platform_device *pdev)
> +{
> + int ret, i, stride;
> + u32 num_locks;
> + struct hwspinlock_device *bank;
> + struct hwspinlock *hwlock;
> + struct resource *res;
> + void __iomem *iobase;
> + struct device_node *node = pdev->dev.of_node;
> + const struct of_device_id *match;
> +
> + match = of_match_device(msm_hwspinlock_of_match, &pdev->dev);
> + if (!match)
> + return -EINVAL;
> +
> + 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);
> +
> + bank = devm_kzalloc(&pdev->dev,
> + sizeof(*bank) + num_locks * sizeof(*hwlock), GFP_KERNEL);
> + if (!bank)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, bank);
> +
> + for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
> + hwlock->priv = iobase + i * stride;
> +
> + ret = hwspin_lock_register(bank, &pdev->dev, &msm_hwspinlock_ops,
> + BASE_ID, num_locks);
> + return ret;
> +}
> +
> +static int msm_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;
> + }
> +
> + return 0;
> +}
> +
> +static struct of_device_id msm_hwspinlock_of_match[] = {
> + {
> + .compatible = "qcom,tcsr-mutex",
> + .data = (void *)0x80, /* register stride */
> + },
> + { },
> +};
> +
> +static struct platform_driver msm_hwspinlock_driver = {
> + .probe = msm_hwspinlock_probe,
> + .remove = msm_hwspinlock_remove,
> + .driver = {
> + .name = "msm_hwspinlock",
> + .owner = THIS_MODULE,
> + .of_match_table = msm_hwspinlock_of_match,
> + },
> +};
> +
> +static int __init msm_hwspinlock_init(void)
> +{
> + return platform_driver_register(&msm_hwspinlock_driver);
> +}
> +/* board init code might need to reserve hwspinlocks for predefined purposes */
> +postcore_initcall(msm_hwspinlock_init);
> +
> +static void __exit msm_hwspinlock_exit(void)
> +{
> + platform_driver_unregister(&msm_hwspinlock_driver);
> +}
> +module_exit(msm_hwspinlock_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Hardware spinlock driver for MSM");
> +MODULE_AUTHOR("Kumar Gala <[email protected]>");
> +MODULE_AUTHOR("Jeffrey Hugo <[email protected]>");
> +MODULE_AUTHOR("Eric Holmberg <[email protected]>");
> --
> 1.8.2.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to [email protected]
> 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
On 07/29, Kumar Gala wrote:
> > diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
> > new file mode 100644
> > index 0000000..ddd6889
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
Maybe this should go under a new hwspinlock directory?
> > @@ -0,0 +1,20 @@
> > +Qualcomm MSM 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.
> > +
> > +Required properties:
> > +- compatible: should be "qcom,tcsr-mutex"
> > +- reg: Should contain registers location and length of mutex registers
> > +- reg-names:
> > + "mutex-base" - string to identify mutex registers
> > +- qcom,num-locks: the number of locks/mutexes supported
> > +
> > +Example:
> > +
> > + qcom,tcsr-mutex@fd484000 {
Maybe it should be hw-mutex@fd484000?
> > + compatible = "qcom,tcsr-mutex";
> > + reg = <0xfd484000 0x1000>;
> > + reg-names = "mutex-base";
> > + qcom,num-locks = <32>;
> > + };
> > diff --git a/drivers/hwspinlock/msm_hwspinlock.c b/drivers/hwspinlock/msm_hwspinlock.c
> > new file mode 100644
> > index 0000000..c7d80c5
> > --- /dev/null
> > +++ b/drivers/hwspinlock/msm_hwspinlock.c
> > @@ -0,0 +1,150 @@
> > +/*
> > + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> > + *
> > + * 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/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/hwspinlock.h>
> > +
> > +#include <asm/io.h>
<linux/io.h> please.
> > +
> > +#include "hwspinlock_internal.h"
> > +
> > +#define SPINLOCK_ID_APPS_PROC 1
> > +#define BASE_ID 0
> > +
> > +static int msm_hwspinlock_trylock(struct hwspinlock *lock)
> > +{
> > + void __iomem *lock_addr = lock->priv;
> > +
> > + writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
> > + smp_mb();
Are you sure you don't want mb() instead? What is this barrier
for? Comment please.
> > + return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
> > +}
> > +
> > +static void msm_hwspinlock_unlock(struct hwspinlock *lock)
> > +{
> > + int lock_owner;
> > + void __iomem *lock_addr = lock->priv;
> > +
> > + lock_owner = readl_relaxed(lock_addr);
> > + if (lock_owner != SPINLOCK_ID_APPS_PROC) {
> > + pr_err("%s: spinlock not owned by Apps (actual owner is %d)\n",
> > + __func__, lock_owner);
> > + }
> > +
> > + writel_relaxed(0, lock_addr);
> > + smp_mb();
Same here. What is smp_mb() for?
> > +}
> > +
> > +static const struct hwspinlock_ops msm_hwspinlock_ops = {
> > + .trylock = msm_hwspinlock_trylock,
> > + .unlock = msm_hwspinlock_unlock,
> > +};
> > +
> > +static struct of_device_id msm_hwspinlock_of_match[];
Why not just put the match table here then? Also, can it be
const?
> > +static int msm_hwspinlock_probe(struct platform_device *pdev)
> > +{
> > + int ret, i, stride;
> > + u32 num_locks;
> > + struct hwspinlock_device *bank;
> > + struct hwspinlock *hwlock;
> > + struct resource *res;
> > + void __iomem *iobase;
> > + struct device_node *node = pdev->dev.of_node;
> > + const struct of_device_id *match;
> > +
> > + match = of_match_device(msm_hwspinlock_of_match, &pdev->dev);
> > + if (!match)
> > + return -EINVAL;
> > +
> > + ret = of_property_read_u32(node, "qcom,num-locks", &num_locks);
> > + if (ret || (num_locks == 0))
Drop useless ().
> > + 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);
> > +
> > + bank = devm_kzalloc(&pdev->dev,
> > + sizeof(*bank) + num_locks * sizeof(*hwlock), GFP_KERNEL);
> > + if (!bank)
> > + return -ENOMEM;
> > +
Style Nit: Maybe we could grow a local variable to get this to be
one line.
size_t array_size;
array_size = num_lock * sizeof(*hwlock);
bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
> On 07/29, Kumar Gala wrote:
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>> new file mode 100644
>>> index 0000000..ddd6889
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>
> Maybe this should go under a new hwspinlock directory?
Will look for Ohad to comment on this.
>
>>> @@ -0,0 +1,20 @@
>>> +Qualcomm MSM 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.
>>> +
>>> +Required properties:
>>> +- compatible: should be "qcom,tcsr-mutex"
>>> +- reg: Should contain registers location and length of mutex registers
>>> +- reg-names:
>>> + "mutex-base" - string to identify mutex registers
>>> +- qcom,num-locks: the number of locks/mutexes supported
>>> +
>>> +Example:
>>> +
>>> + qcom,tcsr-mutex@fd484000 {
>
> Maybe it should be hw-mutex@fd484000?
again, will look for Ohad to make some comment on this.
>>> + compatible = "qcom,tcsr-mutex";
>>> + reg = <0xfd484000 0x1000>;
>>> + reg-names = "mutex-base";
>>> + qcom,num-locks = <32>;
>>> + };
>>> diff --git a/drivers/hwspinlock/msm_hwspinlock.c b/drivers/hwspinlock/msm_hwspinlock.c
>>> new file mode 100644
>>> index 0000000..c7d80c5
>>> --- /dev/null
>>> +++ b/drivers/hwspinlock/msm_hwspinlock.c
>>> @@ -0,0 +1,150 @@
>>> +/*
>>> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
>>> + *
>>> + * 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/err.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/hwspinlock.h>
>>> +
>>> +#include <asm/io.h>
>
> <linux/io.h> please.
will change, I choice <asm/io.h> since *_relaxed are arm specific.
>>> +
>>> +#include "hwspinlock_internal.h"
>>> +
>>> +#define SPINLOCK_ID_APPS_PROC 1
>>> +#define BASE_ID 0
>>> +
>>> +static int msm_hwspinlock_trylock(struct hwspinlock *lock)
>>> +{
>>> + void __iomem *lock_addr = lock->priv;
>>> +
>>> + writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
>>> + smp_mb();
>
> Are you sure you don't want mb() instead? What is this barrier
> for? Comment please.
Hopefully, Eric or Jeff can comment.
>>> + return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
>>> +}
>>> +
>>> +static void msm_hwspinlock_unlock(struct hwspinlock *lock)
>>> +{
>>> + int lock_owner;
>>> + void __iomem *lock_addr = lock->priv;
>>> +
>>> + lock_owner = readl_relaxed(lock_addr);
>>> + if (lock_owner != SPINLOCK_ID_APPS_PROC) {
>>> + pr_err("%s: spinlock not owned by Apps (actual owner is %d)\n",
>>> + __func__, lock_owner);
>>> + }
>>> +
>>> + writel_relaxed(0, lock_addr);
>>> + smp_mb();
>
> Same here. What is smp_mb() for?
Hopefully, Eric or Jeff can comment.
>
>>> +}
>>> +
>>> +static const struct hwspinlock_ops msm_hwspinlock_ops = {
>>> + .trylock = msm_hwspinlock_trylock,
>>> + .unlock = msm_hwspinlock_unlock,
>>> +};
>>> +
>>> +static struct of_device_id msm_hwspinlock_of_match[];
>
> Why not just put the match table here then? Also, can it be
> const?
Will change and make const
>
>>> +static int msm_hwspinlock_probe(struct platform_device *pdev)
>>> +{
>>> + int ret, i, stride;
>>> + u32 num_locks;
>>> + struct hwspinlock_device *bank;
>>> + struct hwspinlock *hwlock;
>>> + struct resource *res;
>>> + void __iomem *iobase;
>>> + struct device_node *node = pdev->dev.of_node;
>>> + const struct of_device_id *match;
>>> +
>>> + match = of_match_device(msm_hwspinlock_of_match, &pdev->dev);
>>> + if (!match)
>>> + return -EINVAL;
>>> +
>>> + ret = of_property_read_u32(node, "qcom,num-locks", &num_locks);
>>> + if (ret || (num_locks == 0))
>
> Drop useless ().
done
>
>>> + 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);
>>> +
>>> + bank = devm_kzalloc(&pdev->dev,
>>> + sizeof(*bank) + num_locks * sizeof(*hwlock), GFP_KERNEL);
>>> + if (!bank)
>>> + return -ENOMEM;
>>> +
>
> Style Nit: Maybe we could grow a local variable to get this to be
> one line.
>
> size_t array_size;
>
> array_size = num_lock * sizeof(*hwlock);
> bank = devm_kzalloc(&pdev->dev, sizeof(*bank) + array_size, GFP_KERNEL);
done
- k
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to [email protected]
> 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
On 07/29, Kumar Gala wrote:
>
> On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
>
> > On 07/29, Kumar Gala wrote:
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/hwspinlock.h>
> >>> +
> >>> +#include <asm/io.h>
> >
> > <linux/io.h> please.
>
> will change, I choice <asm/io.h> since *_relaxed are arm specific.
>
I thought readl_relaxed() was universal for HAS_IOMEM arches.
Unfortunately writel_relaxed() isn't defined on all arches.
Luckily this driver depends on ARCH_MSM so it isn't a problem.
Perhaps it should just depend on HAS_IOMEM so we can use it on
arm64 and/or hexagon in the future without having to change the
Kconfig. That would require introducing writel_relaxed() into the
other arches that don't have it right now.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Jul 29, 2013, at 4:54 PM, Kumar Gala wrote:
>
> On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
>
>> On 07/29, Kumar Gala wrote:
>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>>> new file mode 100644
>>>> index 0000000..ddd6889
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>
>> Maybe this should go under a new hwspinlock directory?
>
> Will look for Ohad to comment on this.
>
>>
>>>> @@ -0,0 +1,20 @@
>>>> +Qualcomm MSM 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.
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be "qcom,tcsr-mutex"
>>>> +- reg: Should contain registers location and length of mutex registers
>>>> +- reg-names:
>>>> + "mutex-base" - string to identify mutex registers
>>>> +- qcom,num-locks: the number of locks/mutexes supported
>>>> +
>>>> +Example:
>>>> +
>>>> + qcom,tcsr-mutex@fd484000 {
>>
>> Maybe it should be hw-mutex@fd484000?
>
> again, will look for Ohad to make some comment on this.
>
>>>> + compatible = "qcom,tcsr-mutex";
>>>> + reg = <0xfd484000 0x1000>;
>>>> + reg-names = "mutex-base";
>>>> + qcom,num-locks = <32>;
>>>> + };
Ohad, ping.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
+ Grant
On Thu, Aug 1, 2013 at 5:10 PM, Kumar Gala <[email protected]> wrote:
>> On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
>>> On 07/29, Kumar Gala wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>>>> new file mode 100644
>>>>> index 0000000..ddd6889
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>>
>>> Maybe this should go under a new hwspinlock directory?
>>
>> Will look for Ohad to comment on this.
>>
>>>
>>>>> @@ -0,0 +1,20 @@
>>>>> +Qualcomm MSM 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.
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: should be "qcom,tcsr-mutex"
>>>>> +- reg: Should contain registers location and length of mutex registers
>>>>> +- reg-names:
>>>>> + "mutex-base" - string to identify mutex registers
>>>>> +- qcom,num-locks: the number of locks/mutexes supported
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> + qcom,tcsr-mutex@fd484000 {
>>>
>>> Maybe it should be hw-mutex@fd484000?
>>
>> again, will look for Ohad to make some comment on this.
>>
>>>>> + compatible = "qcom,tcsr-mutex";
>>>>> + reg = <0xfd484000 0x1000>;
>>>>> + reg-names = "mutex-base";
>>>>> + qcom,num-locks = <32>;
>>>>> + };
>
> Ohad, ping.
I'd prefer a DT maintainer to take a look at your two open questions
above, especially if I'm to merge a new file into the devicetree
Documentation folder.
Grant, any chance you have a moment to take a look?
Otherwise, Stephen - do we have your Ack here? I was happy to see your
review but not sure what's the latest status.
Thanks,
Ohad.
On 08/10/13 12:11, Ohad Ben-Cohen wrote:
> Otherwise, Stephen - do we have your Ack here? I was happy to see your
> review but not sure what's the latest status.
The smp_mb() should be removed. Otherwise I'm willing to accept that we
only build this driver on ARCH_MSM for now. We can fix that in the
future to be available to hexagon if they ever want it. After that it
just needs a DT review/ack so I'll be happy to add a reviewed-by on v3.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Aug 12, 2013, at 11:30 AM, Stephen Boyd wrote:
> On 08/10/13 12:11, Ohad Ben-Cohen wrote:
>> Otherwise, Stephen - do we have your Ack here? I was happy to see your
>> review but not sure what's the latest status.
>
> The smp_mb() should be removed. Otherwise I'm willing to accept that we
> only build this driver on ARCH_MSM for now. We can fix that in the
> future to be available to hexagon if they ever want it. After that it
> just needs a DT review/ack so I'll be happy to add a reviewed-by on v3.
Yeah, waiting on our DT resolution before posting v3 w/various changes like removing the smp_mb() and a few other things.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Aug 10, 2013, at 2:11 PM, Ohad Ben-Cohen wrote:
> + Grant
>
> On Thu, Aug 1, 2013 at 5:10 PM, Kumar Gala <[email protected]> wrote:
>>> On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
>>>> On 07/29, Kumar Gala wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..ddd6889
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>>>>
>>>> Maybe this should go under a new hwspinlock directory?
>>>
>>> Will look for Ohad to comment on this.
>>>
>>>>
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +Qualcomm MSM 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.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should be "qcom,tcsr-mutex"
>>>>>> +- reg: Should contain registers location and length of mutex registers
>>>>>> +- reg-names:
>>>>>> + "mutex-base" - string to identify mutex registers
>>>>>> +- qcom,num-locks: the number of locks/mutexes supported
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> + qcom,tcsr-mutex@fd484000 {
>>>>
>>>> Maybe it should be hw-mutex@fd484000?
>>>
>>> again, will look for Ohad to make some comment on this.
>>>
>>>>>> + compatible = "qcom,tcsr-mutex";
>>>>>> + reg = <0xfd484000 0x1000>;
>>>>>> + reg-names = "mutex-base";
>>>>>> + qcom,num-locks = <32>;
>>>>>> + };
>>
>> Ohad, ping.
>
> I'd prefer a DT maintainer to take a look at your two open questions
> above, especially if I'm to merge a new file into the devicetree
> Documentation folder.
>
> Grant, any chance you have a moment to take a look?
>
> Otherwise, Stephen - do we have your Ack here? I was happy to see your
> review but not sure what's the latest status.
>
> Thanks,
> Ohad.
> --
So I think I'd ask you to recommend a name, should we just us 'hwspinlock'. The general view from ePAPR and dts is the node name should be a bit more generic (like ethernet or pci). So what would you suggest?
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Mon, Aug 12, 2013 at 8:24 PM, Kumar Gala <[email protected]> wrote:
> So I think I'd ask you to recommend a name, should we just us 'hwspinlock'. The general view from ePAPR and dts is the node name should be a bit more generic (like ethernet or pci). So what would you suggest?
How about 'hwlock' ?
This should be generic enough to cover all instances of hardware locks
(we already know hwspinlock isn't generic enough, as there are some
mutex-like hardware locks which doesn't require busy looping).
On Aug 13, 2013, at 1:01 AM, Ohad Ben-Cohen wrote:
> On Mon, Aug 12, 2013 at 8:24 PM, Kumar Gala <[email protected]> wrote:
>> So I think I'd ask you to recommend a name, should we just us 'hwspinlock'. The general view from ePAPR and dts is the node name should be a bit more generic (like ethernet or pci). So what would you suggest?
>
> How about 'hwlock' ?
>
> This should be generic enough to cover all instances of hardware locks
> (we already know hwspinlock isn't generic enough, as there are some
> mutex-like hardware locks which doesn't require busy looping).
hwlock sounds good to me, I'll go with it.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation