[PATCH 1/3 v3] drivers: hwspinlock: add CSR atlas7 implementation

[PATCH 1/3 v3] drivers: hwspinlock: add CSR atlas7 implementation

[Barry Song]

From: Wei Chen <wei.chen@csr.com>

Add hwspinlock support for the CSR atlas7 SoC.

The Hardware Spinlock device on atlas7 provides hardware assistance
for synchronization between the multiple processors in the system
(dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).

Cc: Suman Anna <s-anna@ti.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Signed-off-by: Wei Chen <wei.chen@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 -v3:
 use #hwlock-cells and general hwspinlock dt-binding;
 drop relax();
 drop num-spinlocks in dts;
 re-order Kconfig and Makefile;
 other codingstyle issues.
 Thanks Suman, Bjorn and Ohad

 drivers/hwspinlock/Kconfig           |  12 ++++
 drivers/hwspinlock/Makefile          |   1 +
 drivers/hwspinlock/sirf_hwspinlock.c | 135 +++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 drivers/hwspinlock/sirf_hwspinlock.c

diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index b5b4f52..73a4016 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -30,6 +30,18 @@ config HWSPINLOCK_QCOM
 
 	  If unsure, say N.
 
+config HWSPINLOCK_SIRF
+	tristate "SIRF Hardware Spinlock device"
+	depends on ARCH_SIRF
+	select HWSPINLOCK
+	help
+	  Say y here to support the SIRF Hardware Spinlock device, which
+	  provides a synchronisation mechanism for the various processors
+	  on the SoC.
+
+	  It's safe to say n here if you're not interested in SIRF hardware
+	  spinlock or just want a bare minimum kernel.
+
 config HSEM_U8500
 	tristate "STE Hardware Semaphore functionality"
 	depends on ARCH_U8500
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index 68f95d9..6b59cb5a 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -5,4 +5,5 @@
 obj-$(CONFIG_HWSPINLOCK)		+= hwspinlock_core.o
 obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
+obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
diff --git a/drivers/hwspinlock/sirf_hwspinlock.c b/drivers/hwspinlock/sirf_hwspinlock.c
new file mode 100644
index 0000000..e7e5ba6
--- /dev/null
+++ b/drivers/hwspinlock/sirf_hwspinlock.c
@@ -0,0 +1,135 @@
+/*
+ * SIRF hardware spinlock driver
+ *
+ * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/hwspinlock.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include "hwspinlock_internal.h"
+
+struct sirf_hwspinlock {
+	void __iomem *io_base;
+	struct hwspinlock_device bank;
+};
+
+/* Number of Hardware Spinlocks*/
+#define	HW_SPINLOCK_NUMBER	30
+
+/* Hardware spinlock register offsets */
+#define HW_SPINLOCK_BASE	0x404
+#define HW_SPINLOCK_OFFSET(x)	(HW_SPINLOCK_BASE + 0x4 * (x))
+
+static int sirf_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	/* attempt to acquire the lock by reading value == 1 from it */
+	return !!readl(lock_addr);
+}
+
+static void sirf_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	/* release the lock by writing 0 to it */
+	writel(0, lock_addr);
+}
+
+static const struct hwspinlock_ops sirf_hwspinlock_ops = {
+	.trylock = sirf_hwspinlock_trylock,
+	.unlock = sirf_hwspinlock_unlock,
+};
+
+static int sirf_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct sirf_hwspinlock *hwspin;
+	struct hwspinlock *hwlock;
+	int idx, ret;
+
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	hwspin = devm_kzalloc(&pdev->dev, sizeof(*hwspin) +
+			sizeof(*hwlock) * HW_SPINLOCK_NUMBER, GFP_KERNEL);
+	if (!hwspin)
+		return -ENOMEM;
+
+	/* retrieve io base */
+	hwspin->io_base = of_iomap(pdev->dev.of_node, 0);
+	if (!hwspin->io_base)
+		ret = -ENOMEM;
+
+	for (idx = 0; idx < HW_SPINLOCK_NUMBER; idx++) {
+		hwlock = &hwspin->bank.lock[idx];
+		hwlock->priv = hwspin->io_base + HW_SPINLOCK_OFFSET(idx);
+	}
+
+	platform_set_drvdata(pdev, hwspin);
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
+				&sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);
+	if (ret)
+		goto reg_failed;
+
+	return 0;
+
+reg_failed:
+	pm_runtime_disable(&pdev->dev);
+	iounmap(hwspin->io_base);
+
+	return ret;
+}
+
+static int sirf_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct sirf_hwspinlock *hwspin = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = hwspin_lock_unregister(&hwspin->bank);
+	if (ret) {
+		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
+		return ret;
+	}
+
+	pm_runtime_disable(&pdev->dev);
+
+	iounmap(hwspin->io_base);
+
+	return 0;
+}
+
+static const struct of_device_id sirf_hwpinlock_ids[] = {
+	{ .compatible = "sirf,hwspinlock", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sirf_hwpinlock_ids);
+
+static struct platform_driver sirf_hwspinlock_driver = {
+	.probe = sirf_hwspinlock_probe,
+	.remove = sirf_hwspinlock_remove,
+	.driver = {
+		.name = "atlas7_hwspinlock",
+		.of_match_table = of_match_ptr(sirf_hwpinlock_ids),
+	},
+};
+
+module_platform_driver(sirf_hwspinlock_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("SIRF Hardware spinlock driver");
+MODULE_AUTHOR("Wei Chen <wei.chen@csr.com>");
-- 
2.3.5

[PATCH 2/3 v3] Documentation: dt: add the CSR atlas7 hwspinlock bindings document

[Barry Song]

From: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>

The Hardware Spinlock device on atlas7 provides hardware assistance
for synchronization between the multiple processors in the system
(dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
This patch adds the DT bindings information for this hwspinlock
module.

Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
Cc: Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org>
Signed-off-by: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 .../devicetree/bindings/hwlock/sirf,hwspinlock.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt

diff --git a/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
new file mode 100644
index 0000000..cc6d351
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
@@ -0,0 +1,25 @@
+SIRF Hardware spinlock device Binding
+-----------------------------------------------
+
+Required properties :
+- compatible : shall contain only one of the following:
+	"sirf,hwspinlock"
+
+- reg : the register address of hwspinlock
+
+Please look at the generic hwlock binding for usage information for consumers,
+"Documentation/devicetree/bindings/hwlock/hwlock.txt"
+
+Example of hwlock provider:
+	hwlock {
+		compatible = "sirf,hwspinlock";
+		reg = <0x13240000 0x00010000>;
+		#hwlock-cells = <1>;
+	};
+
+Example of hwlock users:
+	node {
+		...
+		hwlocks = <&hwlock 2>;
+		...
+	};
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 v3] Documentation: dt: add the CSR atlas7 hwspinlock bindings document

[Suman Anna]

Hi Barry,

On 05/19/2015 01:41 AM, Barry Song wrote:
> From: Wei Chen <wei.chen@csr.com>
> 
> The Hardware Spinlock device on atlas7 provides hardware assistance
> for synchronization between the multiple processors in the system
> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
> This patch adds the DT bindings information for this hwspinlock
> module.
> 
> Cc: Suman Anna <s-anna@ti.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Signed-off-by: Wei Chen <wei.chen@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
>  .../devicetree/bindings/hwlock/sirf,hwspinlock.txt | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
> new file mode 100644
> index 0000000..cc6d351
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
> @@ -0,0 +1,25 @@
> +SIRF Hardware spinlock device Binding
> +-----------------------------------------------
> +
> +Required properties :
> +- compatible : shall contain only one of the following:
> +	"sirf,hwspinlock"
> +
> +- reg : the register address of hwspinlock

Also suggest to document the value to be used for #hwlock-cells, the
generic hwlock binding does not mention that.

> +
> +Please look at the generic hwlock binding for usage information for consumers,
> +"Documentation/devicetree/bindings/hwlock/hwlock.txt"
> +

regards
Suman

Re: [PATCH 2/3 v3] Documentation: dt: add the CSR atlas7 hwspinlock bindings document

[Barry Song]

2015-05-23 6:44 GMT+08:00 Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>:
> Hi Barry,
>
> On 05/19/2015 01:41 AM, Barry Song wrote:
>> From: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
>>
>> The Hardware Spinlock device on atlas7 provides hardware assistance
>> for synchronization between the multiple processors in the system
>> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
>> This patch adds the DT bindings information for this hwspinlock
>> module.
>>
>> Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>> Cc: Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org>
>> Signed-off-by: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
>> Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
>> ---
>>  .../devicetree/bindings/hwlock/sirf,hwspinlock.txt | 25 ++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>> new file mode 100644
>> index 0000000..cc6d351
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>> @@ -0,0 +1,25 @@
>> +SIRF Hardware spinlock device Binding
>> +-----------------------------------------------
>> +
>> +Required properties :
>> +- compatible : shall contain only one of the following:
>> +     "sirf,hwspinlock"
>> +
>> +- reg : the register address of hwspinlock
>
> Also suggest to document the value to be used for #hwlock-cells, the
> generic hwlock binding does not mention that.

do you think whether we can put one line like?

#hwlock-cells : <1>, as required by generic hwspinlock binding.
>
>> +
>> +Please look at the generic hwlock binding for usage information for consumers,
>> +"Documentation/devicetree/bindings/hwlock/hwlock.txt"
>> +
>
> regards
> Suman
>

-barry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3 v3] Documentation: dt: add the CSR atlas7 hwspinlock bindings document

[Suman Anna]

On 05/25/2015 12:33 AM, Barry Song wrote:
> 2015-05-23 6:44 GMT+08:00 Suman Anna <s-anna@ti.com>:
>> Hi Barry,
>>
>> On 05/19/2015 01:41 AM, Barry Song wrote:
>>> From: Wei Chen <wei.chen@csr.com>
>>>
>>> The Hardware Spinlock device on atlas7 provides hardware assistance
>>> for synchronization between the multiple processors in the system
>>> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
>>> This patch adds the DT bindings information for this hwspinlock
>>> module.
>>>
>>> Cc: Suman Anna <s-anna@ti.com>
>>> Cc: Bjorn Andersson <bjorn@kryo.se>
>>> Signed-off-by: Wei Chen <wei.chen@csr.com>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>> ---
>>>  .../devicetree/bindings/hwlock/sirf,hwspinlock.txt | 25 ++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>>> new file mode 100644
>>> index 0000000..cc6d351
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwlock/sirf,hwspinlock.txt
>>> @@ -0,0 +1,25 @@
>>> +SIRF Hardware spinlock device Binding
>>> +-----------------------------------------------
>>> +
>>> +Required properties :
>>> +- compatible : shall contain only one of the following:
>>> +     "sirf,hwspinlock"
>>> +
>>> +- reg : the register address of hwspinlock
>>
>> Also suggest to document the value to be used for #hwlock-cells, the
>> generic hwlock binding does not mention that.
> 
> do you think whether we can put one line like?
> 
> #hwlock-cells : <1>, as required by generic hwspinlock binding.

The generic hwspinlock binding does not require a specific value, that
is something determined by the individual platform implementation. So,
something like "#hwlock-cells: Should be 1" is fine.

regards
Suman

>>
>>> +
>>> +Please look at the generic hwlock binding for usage information for consumers,
>>> +"Documentation/devicetree/bindings/hwlock/hwlock.txt"
>>> +
>>
>> regards
>> Suman
>>
> 
> -barry
> 

[PATCH 3/3 v3] ARM: dts: atlas7: use general dt-binding for hwspinlock

[Barry Song]

From: Wei Chen <Wei.Chen-kQvG35nSl+M@public.gmane.org>

This patch moves to use generic dt-binding for hwspinlock providers and
clients.
add #hwlock-cells for the provider and hwlocks for clients.

Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
Cc: Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org>
Signed-off-by: Wei Chen <Wei.Chen-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 arch/arm/boot/dts/atlas7.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/atlas7.dtsi b/arch/arm/boot/dts/atlas7.dtsi
index a753178..66d3f0e 100644
--- a/arch/arm/boot/dts/atlas7.dtsi
+++ b/arch/arm/boot/dts/atlas7.dtsi
@@ -84,17 +84,17 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 
-			hwspinlock {
+			hwlock: hwspinlock {
 				compatible = "sirf,hwspinlock";
 				reg = <0x13240000 0x00010000>;
-
-				num-spinlocks = <30>;
+				#hwlock-cells = <1>;
 			};
 
 			ns_m3_rproc@0 {
 				compatible = "sirf,ns2m30-rproc";
 				reg = <0x13240000 0x00010000>;
 				interrupts = <0 123 0>;
+				hwlocks = <&hwlock 0>, <&hwlock 1>;
 			};
 
 			ns_m3_rproc@1 {
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v3] drivers: hwspinlock: add CSR atlas7 implementation

[Suman Anna]

Hi Barry,

On 05/19/2015 01:41 AM, Barry Song wrote:
> From: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
> 
> Add hwspinlock support for the CSR atlas7 SoC.
> 
> The Hardware Spinlock device on atlas7 provides hardware assistance
> for synchronization between the multiple processors in the system
> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
> 
> Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> Cc: Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org>
> Signed-off-by: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
> Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
> ---
>  -v3:
>  use #hwlock-cells and general hwspinlock dt-binding;
>  drop relax();
>  drop num-spinlocks in dts;
>  re-order Kconfig and Makefile;
>  other codingstyle issues.
>  Thanks Suman, Bjorn and Ohad
> 
>  drivers/hwspinlock/Kconfig           |  12 ++++
>  drivers/hwspinlock/Makefile          |   1 +
>  drivers/hwspinlock/sirf_hwspinlock.c | 135 +++++++++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644 drivers/hwspinlock/sirf_hwspinlock.c
> 
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index b5b4f52..73a4016 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -30,6 +30,18 @@ config HWSPINLOCK_QCOM
>  
>  	  If unsure, say N.
>  
> +config HWSPINLOCK_SIRF
> +	tristate "SIRF Hardware Spinlock device"
> +	depends on ARCH_SIRF
> +	select HWSPINLOCK
> +	help
> +	  Say y here to support the SIRF Hardware Spinlock device, which
> +	  provides a synchronisation mechanism for the various processors
> +	  on the SoC.
> +
> +	  It's safe to say n here if you're not interested in SIRF hardware
> +	  spinlock or just want a bare minimum kernel.
> +
>  config HSEM_U8500
>  	tristate "STE Hardware Semaphore functionality"
>  	depends on ARCH_U8500
> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index 68f95d9..6b59cb5a 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -5,4 +5,5 @@
>  obj-$(CONFIG_HWSPINLOCK)		+= hwspinlock_core.o
>  obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
> +obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> diff --git a/drivers/hwspinlock/sirf_hwspinlock.c b/drivers/hwspinlock/sirf_hwspinlock.c
> new file mode 100644
> index 0000000..e7e5ba6
> --- /dev/null
> +++ b/drivers/hwspinlock/sirf_hwspinlock.c
> @@ -0,0 +1,135 @@
> +/*
> + * SIRF hardware spinlock driver
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.

Not sure on this, but 2015 is here and now..

> + *
> + * Licensed under GPLv2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +struct sirf_hwspinlock {
> +	void __iomem *io_base;
> +	struct hwspinlock_device bank;
> +};
> +
> +/* Number of Hardware Spinlocks*/
> +#define	HW_SPINLOCK_NUMBER	30
> +
> +/* Hardware spinlock register offsets */
> +#define HW_SPINLOCK_BASE	0x404
> +#define HW_SPINLOCK_OFFSET(x)	(HW_SPINLOCK_BASE + 0x4 * (x))
> +
> +static int sirf_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* attempt to acquire the lock by reading value == 1 from it */
> +	return !!readl(lock_addr);
> +}
> +
> +static void sirf_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* release the lock by writing 0 to it */
> +	writel(0, lock_addr);
> +}
> +
> +static const struct hwspinlock_ops sirf_hwspinlock_ops = {
> +	.trylock = sirf_hwspinlock_trylock,
> +	.unlock = sirf_hwspinlock_unlock,
> +};
> +
> +static int sirf_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sirf_hwspinlock *hwspin;
> +	struct hwspinlock *hwlock;
> +	int idx, ret;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENODEV;
> +
> +	hwspin = devm_kzalloc(&pdev->dev, sizeof(*hwspin) +
> +			sizeof(*hwlock) * HW_SPINLOCK_NUMBER, GFP_KERNEL);
> +	if (!hwspin)
> +		return -ENOMEM;
> +
> +	/* retrieve io base */
> +	hwspin->io_base = of_iomap(pdev->dev.of_node, 0);
> +	if (!hwspin->io_base)
> +		ret = -ENOMEM;

You are missing the bail out here.

> +
> +	for (idx = 0; idx < HW_SPINLOCK_NUMBER; idx++) {
> +		hwlock = &hwspin->bank.lock[idx];
> +		hwlock->priv = hwspin->io_base + HW_SPINLOCK_OFFSET(idx);
> +	}
> +
> +	platform_set_drvdata(pdev, hwspin);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
> +				&sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);

this is a checkpatch warning with the --strict option, not sure what
convention Ohad is following though. Rest looks good.

regards
Suman
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v3] drivers: hwspinlock: add CSR atlas7 implementation

[Barry Song]

2015-05-23 6:51 GMT+08:00 Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>:
> Hi Barry,
>
> On 05/19/2015 01:41 AM, Barry Song wrote:
>> From: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
>>
>> Add hwspinlock support for the CSR atlas7 SoC.
>>
>> The Hardware Spinlock device on atlas7 provides hardware assistance
>> for synchronization between the multiple processors in the system
>> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
>>
>> Cc: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>> Cc: Bjorn Andersson <bjorn-UYDU3/A3LUY@public.gmane.org>
>> Signed-off-by: Wei Chen <wei.chen-kQvG35nSl+M@public.gmane.org>
>> Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
>> ---
>>  -v3:
>>  use #hwlock-cells and general hwspinlock dt-binding;
>>  drop relax();
>>  drop num-spinlocks in dts;
>>  re-order Kconfig and Makefile;
>>  other codingstyle issues.
>>  Thanks Suman, Bjorn and Ohad
>>
>>  drivers/hwspinlock/Kconfig           |  12 ++++
>>  drivers/hwspinlock/Makefile          |   1 +
>>  drivers/hwspinlock/sirf_hwspinlock.c | 135 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 148 insertions(+)
>>  create mode 100644 drivers/hwspinlock/sirf_hwspinlock.c
>>
>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>> index b5b4f52..73a4016 100644
>> --- a/drivers/hwspinlock/Kconfig
>> +++ b/drivers/hwspinlock/Kconfig
>> @@ -30,6 +30,18 @@ config HWSPINLOCK_QCOM
>>
>>         If unsure, say N.
>>
>> +config HWSPINLOCK_SIRF
>> +     tristate "SIRF Hardware Spinlock device"
>> +     depends on ARCH_SIRF
>> +     select HWSPINLOCK
>> +     help
>> +       Say y here to support the SIRF Hardware Spinlock device, which
>> +       provides a synchronisation mechanism for the various processors
>> +       on the SoC.
>> +
>> +       It's safe to say n here if you're not interested in SIRF hardware
>> +       spinlock or just want a bare minimum kernel.
>> +
>>  config HSEM_U8500
>>       tristate "STE Hardware Semaphore functionality"
>>       depends on ARCH_U8500
>> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
>> index 68f95d9..6b59cb5a 100644
>> --- a/drivers/hwspinlock/Makefile
>> +++ b/drivers/hwspinlock/Makefile
>> @@ -5,4 +5,5 @@
>>  obj-$(CONFIG_HWSPINLOCK)             += hwspinlock_core.o
>>  obj-$(CONFIG_HWSPINLOCK_OMAP)                += omap_hwspinlock.o
>>  obj-$(CONFIG_HWSPINLOCK_QCOM)                += qcom_hwspinlock.o
>> +obj-$(CONFIG_HWSPINLOCK_SIRF)                += sirf_hwspinlock.o
>>  obj-$(CONFIG_HSEM_U8500)             += u8500_hsem.o
>> diff --git a/drivers/hwspinlock/sirf_hwspinlock.c b/drivers/hwspinlock/sirf_hwspinlock.c
>> new file mode 100644
>> index 0000000..e7e5ba6
>> --- /dev/null
>> +++ b/drivers/hwspinlock/sirf_hwspinlock.c
>> @@ -0,0 +1,135 @@
>> +/*
>> + * SIRF hardware spinlock driver
>> + *
>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>
> Not sure on this, but 2015 is here and now..
>
>> + *
>> + * Licensed under GPLv2.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/hwspinlock.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +
>> +#include "hwspinlock_internal.h"
>> +
>> +struct sirf_hwspinlock {
>> +     void __iomem *io_base;
>> +     struct hwspinlock_device bank;
>> +};
>> +
>> +/* Number of Hardware Spinlocks*/
>> +#define      HW_SPINLOCK_NUMBER      30
>> +
>> +/* Hardware spinlock register offsets */
>> +#define HW_SPINLOCK_BASE     0x404
>> +#define HW_SPINLOCK_OFFSET(x)        (HW_SPINLOCK_BASE + 0x4 * (x))
>> +
>> +static int sirf_hwspinlock_trylock(struct hwspinlock *lock)
>> +{
>> +     void __iomem *lock_addr = lock->priv;
>> +
>> +     /* attempt to acquire the lock by reading value == 1 from it */
>> +     return !!readl(lock_addr);
>> +}
>> +
>> +static void sirf_hwspinlock_unlock(struct hwspinlock *lock)
>> +{
>> +     void __iomem *lock_addr = lock->priv;
>> +
>> +     /* release the lock by writing 0 to it */
>> +     writel(0, lock_addr);
>> +}
>> +
>> +static const struct hwspinlock_ops sirf_hwspinlock_ops = {
>> +     .trylock = sirf_hwspinlock_trylock,
>> +     .unlock = sirf_hwspinlock_unlock,
>> +};
>> +
>> +static int sirf_hwspinlock_probe(struct platform_device *pdev)
>> +{
>> +     struct sirf_hwspinlock *hwspin;
>> +     struct hwspinlock *hwlock;
>> +     int idx, ret;
>> +
>> +     if (!pdev->dev.of_node)
>> +             return -ENODEV;
>> +
>> +     hwspin = devm_kzalloc(&pdev->dev, sizeof(*hwspin) +
>> +                     sizeof(*hwlock) * HW_SPINLOCK_NUMBER, GFP_KERNEL);
>> +     if (!hwspin)
>> +             return -ENOMEM;
>> +
>> +     /* retrieve io base */
>> +     hwspin->io_base = of_iomap(pdev->dev.of_node, 0);
>> +     if (!hwspin->io_base)
>> +             ret = -ENOMEM;
>
> You are missing the bail out here.

real. it should be "return -ENOMEM"

>
>> +
>> +     for (idx = 0; idx < HW_SPINLOCK_NUMBER; idx++) {
>> +             hwlock = &hwspin->bank.lock[idx];
>> +             hwlock->priv = hwspin->io_base + HW_SPINLOCK_OFFSET(idx);
>> +     }
>> +
>> +     platform_set_drvdata(pdev, hwspin);
>> +
>> +     pm_runtime_enable(&pdev->dev);
>> +
>> +     ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
>> +                             &sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);
>
> this is a checkpatch warning with the --strict option, not sure what
> convention Ohad is following though. Rest looks good.

do you mean this CHECK?

CHECK: Alignment should match open parenthesis
#87: FILE: drivers/hwspinlock/sirf_hwspinlock.c:87:
+ ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
+ &sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);

>
> regards
> Suman

-barry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v3] drivers: hwspinlock: add CSR atlas7 implementation

[Suman Anna]

On 05/25/2015 12:37 AM, Barry Song wrote:
> 2015-05-23 6:51 GMT+08:00 Suman Anna <s-anna@ti.com>:
>> Hi Barry,
>>
>> On 05/19/2015 01:41 AM, Barry Song wrote:
>>> From: Wei Chen <wei.chen@csr.com>
>>>
>>> Add hwspinlock support for the CSR atlas7 SoC.
>>>
>>> The Hardware Spinlock device on atlas7 provides hardware assistance
>>> for synchronization between the multiple processors in the system
>>> (dual Cortex-A7, CAN bus Cortex-M3 and audio DSP).
>>>
>>> Cc: Suman Anna <s-anna@ti.com>
>>> Cc: Bjorn Andersson <bjorn@kryo.se>
>>> Signed-off-by: Wei Chen <wei.chen@csr.com>
>>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>>> ---
>>>  -v3:
>>>  use #hwlock-cells and general hwspinlock dt-binding;
>>>  drop relax();
>>>  drop num-spinlocks in dts;
>>>  re-order Kconfig and Makefile;
>>>  other codingstyle issues.
>>>  Thanks Suman, Bjorn and Ohad
>>>
>>>  drivers/hwspinlock/Kconfig           |  12 ++++
>>>  drivers/hwspinlock/Makefile          |   1 +
>>>  drivers/hwspinlock/sirf_hwspinlock.c | 135 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 148 insertions(+)
>>>  create mode 100644 drivers/hwspinlock/sirf_hwspinlock.c
>>>
>>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>>> index b5b4f52..73a4016 100644
>>> --- a/drivers/hwspinlock/Kconfig
>>> +++ b/drivers/hwspinlock/Kconfig
>>> @@ -30,6 +30,18 @@ config HWSPINLOCK_QCOM
>>>
>>>         If unsure, say N.
>>>
>>> +config HWSPINLOCK_SIRF
>>> +     tristate "SIRF Hardware Spinlock device"
>>> +     depends on ARCH_SIRF
>>> +     select HWSPINLOCK
>>> +     help
>>> +       Say y here to support the SIRF Hardware Spinlock device, which
>>> +       provides a synchronisation mechanism for the various processors
>>> +       on the SoC.
>>> +
>>> +       It's safe to say n here if you're not interested in SIRF hardware
>>> +       spinlock or just want a bare minimum kernel.
>>> +
>>>  config HSEM_U8500
>>>       tristate "STE Hardware Semaphore functionality"
>>>       depends on ARCH_U8500
>>> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
>>> index 68f95d9..6b59cb5a 100644
>>> --- a/drivers/hwspinlock/Makefile
>>> +++ b/drivers/hwspinlock/Makefile
>>> @@ -5,4 +5,5 @@
>>>  obj-$(CONFIG_HWSPINLOCK)             += hwspinlock_core.o
>>>  obj-$(CONFIG_HWSPINLOCK_OMAP)                += omap_hwspinlock.o
>>>  obj-$(CONFIG_HWSPINLOCK_QCOM)                += qcom_hwspinlock.o
>>> +obj-$(CONFIG_HWSPINLOCK_SIRF)                += sirf_hwspinlock.o
>>>  obj-$(CONFIG_HSEM_U8500)             += u8500_hsem.o
>>> diff --git a/drivers/hwspinlock/sirf_hwspinlock.c b/drivers/hwspinlock/sirf_hwspinlock.c
>>> new file mode 100644
>>> index 0000000..e7e5ba6
>>> --- /dev/null
>>> +++ b/drivers/hwspinlock/sirf_hwspinlock.c
>>> @@ -0,0 +1,135 @@
>>> +/*
>>> + * SIRF hardware spinlock driver
>>> + *
>>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>>
>> Not sure on this, but 2015 is here and now..
>>
>>> + *
>>> + * Licensed under GPLv2.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/hwspinlock.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +
>>> +#include "hwspinlock_internal.h"
>>> +
>>> +struct sirf_hwspinlock {
>>> +     void __iomem *io_base;
>>> +     struct hwspinlock_device bank;
>>> +};
>>> +
>>> +/* Number of Hardware Spinlocks*/
>>> +#define      HW_SPINLOCK_NUMBER      30
>>> +
>>> +/* Hardware spinlock register offsets */
>>> +#define HW_SPINLOCK_BASE     0x404
>>> +#define HW_SPINLOCK_OFFSET(x)        (HW_SPINLOCK_BASE + 0x4 * (x))
>>> +
>>> +static int sirf_hwspinlock_trylock(struct hwspinlock *lock)
>>> +{
>>> +     void __iomem *lock_addr = lock->priv;
>>> +
>>> +     /* attempt to acquire the lock by reading value == 1 from it */
>>> +     return !!readl(lock_addr);
>>> +}
>>> +
>>> +static void sirf_hwspinlock_unlock(struct hwspinlock *lock)
>>> +{
>>> +     void __iomem *lock_addr = lock->priv;
>>> +
>>> +     /* release the lock by writing 0 to it */
>>> +     writel(0, lock_addr);
>>> +}
>>> +
>>> +static const struct hwspinlock_ops sirf_hwspinlock_ops = {
>>> +     .trylock = sirf_hwspinlock_trylock,
>>> +     .unlock = sirf_hwspinlock_unlock,
>>> +};
>>> +
>>> +static int sirf_hwspinlock_probe(struct platform_device *pdev)
>>> +{
>>> +     struct sirf_hwspinlock *hwspin;
>>> +     struct hwspinlock *hwlock;
>>> +     int idx, ret;
>>> +
>>> +     if (!pdev->dev.of_node)
>>> +             return -ENODEV;
>>> +
>>> +     hwspin = devm_kzalloc(&pdev->dev, sizeof(*hwspin) +
>>> +                     sizeof(*hwlock) * HW_SPINLOCK_NUMBER, GFP_KERNEL);
>>> +     if (!hwspin)
>>> +             return -ENOMEM;
>>> +
>>> +     /* retrieve io base */
>>> +     hwspin->io_base = of_iomap(pdev->dev.of_node, 0);
>>> +     if (!hwspin->io_base)
>>> +             ret = -ENOMEM;
>>
>> You are missing the bail out here.
> 
> real. it should be "return -ENOMEM"
> 
>>
>>> +
>>> +     for (idx = 0; idx < HW_SPINLOCK_NUMBER; idx++) {
>>> +             hwlock = &hwspin->bank.lock[idx];
>>> +             hwlock->priv = hwspin->io_base + HW_SPINLOCK_OFFSET(idx);
>>> +     }
>>> +
>>> +     platform_set_drvdata(pdev, hwspin);
>>> +
>>> +     pm_runtime_enable(&pdev->dev);
>>> +
>>> +     ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
>>> +                             &sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);
>>
>> this is a checkpatch warning with the --strict option, not sure what
>> convention Ohad is following though. Rest looks good.
> 
> do you mean this CHECK?
> 
> CHECK: Alignment should match open parenthesis
> #87: FILE: drivers/hwspinlock/sirf_hwspinlock.c:87:
> + ret = hwspin_lock_register(&hwspin->bank, &pdev->dev,
> + &sirf_hwspinlock_ops, 0, HW_SPINLOCK_NUMBER);

Yes.

regards
Suman