[PATCH 0/2] introduce sunxi hwspinlock

[PATCH 0/2] introduce sunxi hwspinlock

[fuyao]

From: fuyao <fuyao@allwinnertech.com>

this series add hwspinlock of sunxi. it provides hardware assistance for
synchronization between the multiple processors in the system.
(Or1k, Cortex-A7, Cortex-A53, Xtensa)

fuyao (2):
  dt-bindings: hwlock: add sunxi hwlock
  hwspinlock: add SUNXI implementation

 .../bindings/hwlock/sunxi,hwspinlock.yaml     |  46 ++++
 MAINTAINERS                                   |   6 +
 drivers/hwspinlock/Kconfig                    |  10 +
 drivers/hwspinlock/Makefile                   |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c         | 205 ++++++++++++++++++
 5 files changed, 268 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
 create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c

-- 
2.29.2

[PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock

[fuyao]

From: fuyao <fuyao@allwinnertech.com>

SUNXI hwspinlock binding DT schema format

Signed-off-by: fuyao <fuyao@allwinnertech.com>
---
 .../bindings/hwlock/sunxi,hwspinlock.yaml     | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml

diff --git a/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
new file mode 100644
index 0000000000000..68ce93b6d2bcb
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwlock/sunxi,hwspinlock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SUNXI HwSpinlock for SUNXI
+
+maintainers:
+  - fuyao <fuyao@allwinnertech.com>
+
+properties:
+  compatible:
+    enum:
+      - allwinner,h3-hwspinlock,  # for h3-hwspinlock
+      - allwinner,h6-hwspinlock,  # for h6-hwspinlock
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+
+additionalProperties: false
+
+examples:
+
+  - |
+    hwspinlock: spinlock@1c17000 {
+        compatible = "allwinner,h3-hwspinlock";
+        reg = <0x0 0x01c17000 0x0 0x1000>;
+        clocks = <&ccu CLK_BUS_SPINLOCK>;
+        resets = <&ccu RST_BUS_SPINLOCK>;
+    };
+
+  - |
+    hwspinlock: spinlock@3004000 {
+        compatible = "allwinner,h6-hwspinlock";
+        reg = <0x0 0x03004000 0x0 0x1000>;
+        clocks = <&ccu CLK_BUS_SPINLOCK>;
+        resets = <&ccu RST_BUS_SPINLOCK>;
+    };
+
-- 
2.29.2

[PATCH 2/2] hwspinlock: add SUNXI implementation

[fuyao]

From: fuyao <fuyao@allwinnertech.com>

Add hwspinlock support for the SUNXI Hardware Spinlock device.

The Hardware Spinlock device on SUNXI provides hardware assistance
for synchronization between the multiple processors in the system
(Cortex-A7, or1k, Xtensa DSP, Cortex-A53)

Signed-off-by: fuyao <fuyao@allwinnertech.com>
---
 MAINTAINERS                           |   6 +
 drivers/hwspinlock/Kconfig            |  10 ++
 drivers/hwspinlock/Makefile           |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++
 4 files changed, 222 insertions(+)
 create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e451dcce054f0..68d25574432d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -737,6 +737,12 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/staging/media/sunxi/cedrus/
 
+ALLWINNER HWSPINLOCK DRIVER
+M:	fuyao <fuyao@allwinnertech.com>
+S:	Maintained
+F:	drivers/hwspinlock/sunxi_hwspinlock.c
+F:      Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
+
 ALPHA PORT
 M:	Richard Henderson <rth@twiddle.net>
 M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 32cd26352f381..4d0d516dcb544 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -55,6 +55,16 @@ config HWSPINLOCK_STM32
 
 	  If unsure, say N.
 
+config HWSPINLOCK_SUNXI
+	tristate "SUNXI Hardware Spinlock device"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  Say y here to support the SUNXI Hardware Semaphore functionality, which
+	  provides a synchronisation mechanism for the various processor on the
+	  SoC.
+
+	  If unsure, say N.
+
 config HSEM_U8500
 	tristate "STE Hardware Semaphore functionality"
 	depends on ARCH_U8500 || COMPILE_TEST
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index ed053e3f02be4..839a053205f73 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
+obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
new file mode 100644
index 0000000000000..2c3dc148c9b72
--- /dev/null
+++ b/drivers/hwspinlock/sunxi_hwspinlock.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SUNXI hardware spinlock driver
+ *
+ * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/bitops.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/hwspinlock.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/err.h>
+#include <linux/reset.h>
+
+#include "hwspinlock_internal.h"
+
+/* hardware spinlock register list */
+#define	LOCK_SYS_STATUS_REG             (0x0000)
+#define	LOCK_STATUS_REG                 (0x0010)
+#define LOCK_BASE_OFFSET                (0x0100)
+#define LOCK_BASE_ID                    (0)
+
+/* Possible values of SPINLOCK_LOCK_REG */
+#define SPINLOCK_NOTTAKEN               (0)     /* free */
+#define SPINLOCK_TAKEN                  (1)     /* locked */
+
+struct sunxi_spinlock_config {
+	int nspin;
+};
+
+static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	/* attempt to acquire the lock by reading its value */
+	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
+}
+
+static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	/* release the lock by writing 0 to it */
+	writel(SPINLOCK_NOTTAKEN, lock_addr);
+}
+
+/*
+ * relax the SUNXI interconnect while spinning on it.
+ *
+ * The specs recommended that the retry delay time will be
+ * just over half of the time that a requester would be
+ * expected to hold the lock.
+ *
+ * in sunxi spinlock time less then 200 cycles
+ *
+ * The number below is taken from an hardware specs example,
+ * obviously it is somewhat arbitrary.
+ */
+static void sunxi_hwspinlock_relax(struct hwspinlock *lock)
+{
+	ndelay(50);
+}
+
+static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
+	.trylock = sunxi_hwspinlock_trylock,
+	.unlock = sunxi_hwspinlock_unlock,
+	.relax = sunxi_hwspinlock_relax,
+};
+
+struct sunxi_hwspinlock_device {
+	struct hwspinlock_device *bank;
+	struct clk *bus_clk;
+	struct reset_control *reset;
+};
+
+static void sunxi_hwspinlock_clk_init(struct platform_device *pdev,
+				      struct sunxi_hwspinlock_device *private)
+{
+	private->bus_clk = devm_clk_get(&pdev->dev, NULL);
+	private->reset = devm_reset_control_get(&pdev->dev, NULL);
+
+	if (private->reset)
+		reset_control_deassert(private->reset);
+	if (private->bus_clk)
+		clk_prepare_enable(private->bus_clk);
+}
+
+static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private)
+{
+	if (private->reset)
+		reset_control_assert(private->reset);
+	if (private->bus_clk)
+		clk_disable(private->bus_clk);
+}
+
+static const struct sunxi_spinlock_config spin_ver_1 = {
+	.nspin = 32,
+};
+
+static const struct of_device_id sunxi_hwspinlock_of_match[] = {
+	{
+		.compatible = "allwinner,h3-hwspinlock",
+		.data = &spin_ver_1,
+	},
+	{
+		.compatible = "allwinner,h6-hwspinlock",
+		.data = &spin_ver_1,
+	},
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match);
+
+static int sunxi_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct sunxi_hwspinlock_device *private;
+	struct hwspinlock_device *bank;
+	struct hwspinlock *hwlock;
+	const struct sunxi_spinlock_config *config;
+	const struct of_device_id *match;
+	void __iomem *iobase;
+	int num_locks, i, ret;
+
+	iobase = devm_platform_ioremap_resource(pdev, 0);
+	if (PTR_ERR(iobase))
+		return PTR_ERR(iobase);
+
+	match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match),
+				&pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	config = match->data;
+	num_locks = config->nspin;
+
+	private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
+	if (!private)
+		return -ENOMEM;
+
+	bank = devm_kzalloc(&pdev->dev,
+			    sizeof(*bank) + num_locks * sizeof(*hwlock),
+			    GFP_KERNEL);
+	if (!bank)
+		return -ENOMEM;
+
+	private->bank = bank;
+	sunxi_hwspinlock_clk_init(pdev, private);
+
+	platform_set_drvdata(pdev, private);
+
+	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
+		hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i;
+
+	ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops,
+					LOCK_BASE_ID, num_locks);
+	if (ret)
+		return ret;
+
+
+	return 0;
+}
+
+static int sunxi_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev);
+
+	sunxi_hwspinlock_clk_dinit(private);
+
+	return 0;
+}
+
+static struct platform_driver sunxi_hwspinlock_driver = {
+	.probe		= sunxi_hwspinlock_probe,
+	.remove		= sunxi_hwspinlock_remove,
+	.driver		= {
+		.name	= "sunxi-hwspinlock",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(sunxi_hwspinlock_of_match),
+	},
+};
+
+module_platform_driver(sunxi_hwspinlock_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI");
+MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>");
-- 
2.29.2

Re: [PATCH 0/2] introduce sunxi hwspinlock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi!

On Thu, Nov 19, 2020 at 02:44:51PM +0800, fuyao@allwinnertech.com wrote:
> From: fuyao <fuyao@allwinnertech.com>
> 
> this series add hwspinlock of sunxi. it provides hardware assistance for
> synchronization between the multiple processors in the system.
> (Or1k, Cortex-A7, Cortex-A53, Xtensa)

Xtensa? Which SoC has an Xtensa core?

Unfortunately, there's been a submission of the same driver earlier this week:
https://lore.kernel.org/lkml/cover.1605693132.git.wilken.gottwalt@posteo.net/

It would be great if you could point out whatever issue there is with
that patch series (it looks like the retry delay could be useful for
example).

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] hwspinlock: add SUNXI implementation

[Bjorn Andersson]

On Thu 19 Nov 00:44 CST 2020, fuyao@allwinnertech.com wrote:

> From: fuyao <fuyao@allwinnertech.com>
> 
> Add hwspinlock support for the SUNXI Hardware Spinlock device.
> 
> The Hardware Spinlock device on SUNXI provides hardware assistance
> for synchronization between the multiple processors in the system
> (Cortex-A7, or1k, Xtensa DSP, Cortex-A53)
> 
> Signed-off-by: fuyao <fuyao@allwinnertech.com>
> ---
>  MAINTAINERS                           |   6 +
>  drivers/hwspinlock/Kconfig            |  10 ++
>  drivers/hwspinlock/Makefile           |   1 +
>  drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++
>  4 files changed, 222 insertions(+)
>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e451dcce054f0..68d25574432d0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -737,6 +737,12 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/staging/media/sunxi/cedrus/
>  
> +ALLWINNER HWSPINLOCK DRIVER
> +M:	fuyao <fuyao@allwinnertech.com>
> +S:	Maintained
> +F:	drivers/hwspinlock/sunxi_hwspinlock.c
> +F:      Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
> +
>  ALPHA PORT
>  M:	Richard Henderson <rth@twiddle.net>
>  M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 32cd26352f381..4d0d516dcb544 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -55,6 +55,16 @@ config HWSPINLOCK_STM32
>  
>  	  If unsure, say N.
>  
> +config HWSPINLOCK_SUNXI
> +	tristate "SUNXI Hardware Spinlock device"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  Say y here to support the SUNXI Hardware Semaphore functionality, which
> +	  provides a synchronisation mechanism for the various processor on the
> +	  SoC.
> +
> +	  If unsure, say N.
> +
>  config HSEM_U8500
>  	tristate "STE Hardware Semaphore functionality"
>  	depends on ARCH_U8500 || COMPILE_TEST
> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index ed053e3f02be4..839a053205f73 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> +obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> new file mode 100644
> index 0000000000000..2c3dc148c9b72
> --- /dev/null
> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SUNXI hardware spinlock driver
> + *
> + * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com
> + *

Please remove the remainder of this comment, it's already covered by the
SPDX header above.

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * 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/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/bitops.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/err.h>
> +#include <linux/reset.h>

You don't need all of these.

> +
> +#include "hwspinlock_internal.h"
> +
> +/* hardware spinlock register list */
> +#define	LOCK_SYS_STATUS_REG             (0x0000)
> +#define	LOCK_STATUS_REG                 (0x0010)
> +#define LOCK_BASE_OFFSET                (0x0100)
> +#define LOCK_BASE_ID                    (0)

No need for the parenthesis on these, please drop them.

> +
> +/* Possible values of SPINLOCK_LOCK_REG */
> +#define SPINLOCK_NOTTAKEN               (0)     /* free */
> +#define SPINLOCK_TAKEN                  (1)     /* locked */
> +
> +struct sunxi_spinlock_config {
> +	int nspin;
> +};
> +
> +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* attempt to acquire the lock by reading its value */
> +	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);

Please drop the outer ().

> +}
> +
> +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* release the lock by writing 0 to it */
> +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> +}
> +
> +/*
> + * relax the SUNXI interconnect while spinning on it.
> + *
> + * The specs recommended that the retry delay time will be
> + * just over half of the time that a requester would be
> + * expected to hold the lock.
> + *
> + * in sunxi spinlock time less then 200 cycles
> + *
> + * The number below is taken from an hardware specs example,
> + * obviously it is somewhat arbitrary.

Thank you for the good explanation.

> + */
> +static void sunxi_hwspinlock_relax(struct hwspinlock *lock)
> +{
> +	ndelay(50);
> +}
> +
> +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> +	.trylock = sunxi_hwspinlock_trylock,
> +	.unlock = sunxi_hwspinlock_unlock,
> +	.relax = sunxi_hwspinlock_relax,
> +};
> +
> +struct sunxi_hwspinlock_device {
> +	struct hwspinlock_device *bank;
> +	struct clk *bus_clk;
> +	struct reset_control *reset;
> +};
> +
> +static void sunxi_hwspinlock_clk_init(struct platform_device *pdev,
> +				      struct sunxi_hwspinlock_device *private)
> +{
> +	private->bus_clk = devm_clk_get(&pdev->dev, NULL);
> +	private->reset = devm_reset_control_get(&pdev->dev, NULL);

You should check the return value of these, e.g. for EPROBE_DEFER and if
so return appropriately from sunxi_hwspinlock_probe().

So please move them to the probe function to make this easier and
cleaner.

> +
> +	if (private->reset)
> +		reset_control_deassert(private->reset);
> +	if (private->bus_clk)
> +		clk_prepare_enable(private->bus_clk);

Both of these apis start with

	if (!argument)
		return;

So there's no need for you to check for NULL before calling them. Also
to make it clear that you want these to be deassered and prepare_enabled
between probe and remvoe, move them into probe (and next function into
remove).

> +}
> +
> +static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private)
> +{
> +	if (private->reset)
> +		reset_control_assert(private->reset);
> +	if (private->bus_clk)
> +		clk_disable(private->bus_clk);
> +}
> +
> +static const struct sunxi_spinlock_config spin_ver_1 = {
> +	.nspin = 32,
> +};
> +
> +static const struct of_device_id sunxi_hwspinlock_of_match[] = {
> +	{
> +		.compatible = "allwinner,h3-hwspinlock",
> +		.data = &spin_ver_1,

If all cases comes with the same "data", then please just put nspin in a
#define until you're going to support hardware that has some other
number of locks.

> +	},
> +	{
> +		.compatible = "allwinner,h6-hwspinlock",
> +		.data = &spin_ver_1,
> +	},
> +	{ /* Sentinel */ },

No need to spell out "/* Sentinel */", leave it emtpy and please drop the , at
the end.

> +};
> +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match);

Please move this down by sunxi_hwspinlock_driver and use
device_get_match_data() instead of of_match_device().

> +
> +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock_device *private;
> +	struct hwspinlock_device *bank;
> +	struct hwspinlock *hwlock;
> +	const struct sunxi_spinlock_config *config;
> +	const struct of_device_id *match;
> +	void __iomem *iobase;
> +	int num_locks, i, ret;
> +
> +	iobase = devm_platform_ioremap_resource(pdev, 0);
> +	if (PTR_ERR(iobase))
> +		return PTR_ERR(iobase);
> +
> +	match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match),
> +				&pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	config = match->data;
> +	num_locks = config->nspin;
> +
> +	private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
> +	if (!private)
> +		return -ENOMEM;
> +
> +	bank = devm_kzalloc(&pdev->dev,
> +			    sizeof(*bank) + num_locks * sizeof(*hwlock),
> +			    GFP_KERNEL);
> +	if (!bank)
> +		return -ENOMEM;
> +
> +	private->bank = bank;
> +	sunxi_hwspinlock_clk_init(pdev, private);
> +
> +	platform_set_drvdata(pdev, private);
> +
> +	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
> +		hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i;
> +
> +	ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops,
> +					LOCK_BASE_ID, num_locks);

This returns 0 or -errno, so rather than returning ret if ret otherwise
0, just do:

	return devm_hwspin_lock_register(...)

> +	if (ret)
> +		return ret;
> +
> +
> +	return 0;
> +}
> +
> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev);
> +
> +	sunxi_hwspinlock_clk_dinit(private);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver sunxi_hwspinlock_driver = {
> +	.probe		= sunxi_hwspinlock_probe,
> +	.remove		= sunxi_hwspinlock_remove,
> +	.driver		= {
> +		.name	= "sunxi-hwspinlock",
> +		.owner	= THIS_MODULE,

module_platform_driver() fills out .owner for you, so please remove
this.

> +		.of_match_table = of_match_ptr(sunxi_hwspinlock_of_match),

Please skip of_match_ptr(), it will just cause build warnings when
compile tested without CONFIG_OF.

Thank you,
Bjorn

> +	},
> +};
> +
> +module_platform_driver(sunxi_hwspinlock_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI");
> +MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>");
> -- 
> 2.29.2
> 

Re: [PATCH 0/2] introduce sunxi hwspinlock

[fuyao]

On Fri, Nov 20, 2020 at 05:07:10PM +0100, Maxime Ripard wrote:
> Hi!
> 
> On Thu, Nov 19, 2020 at 02:44:51PM +0800, fuyao@allwinnertech.com wrote:
> > From: fuyao <fuyao@allwinnertech.com>
> > 
> > this series add hwspinlock of sunxi. it provides hardware assistance for
> > synchronization between the multiple processors in the system.
> > (Or1k, Cortex-A7, Cortex-A53, Xtensa)
> 
> Xtensa? Which SoC has an Xtensa core?

The new Soc named R329 use Xtensa core as the arisc role and audio.
And the hwspinlock is the same as h3 and h6.

Additional, The new RISC-V Soc also use the same hwspinlock.

> 
> Unfortunately, there's been a submission of the same driver earlier this week:
> https://lore.kernel.org/lkml/cover.1605693132.git.wilken.gottwalt@posteo.net/
> 
> It would be great if you could point out whatever issue there is with
> that patch series (it looks like the retry delay could be useful for
> example).

I will test that submission with arisc next week.

BTW, which sunxi board you used, I want to used the same board with you.

^-^

thanks
-- 
fuyao

Re: [PATCH 2/2] hwspinlock: add SUNXI implementation

[fuyao]

On Fri, Nov 20, 2020 at 10:01:04PM -0600, Bjorn Andersson wrote:
> On Thu 19 Nov 00:44 CST 2020, fuyao@allwinnertech.com wrote:
> 
> > From: fuyao <fuyao@allwinnertech.com>
> > 
> > Add hwspinlock support for the SUNXI Hardware Spinlock device.
> > 
> > The Hardware Spinlock device on SUNXI provides hardware assistance
> > for synchronization between the multiple processors in the system
> > (Cortex-A7, or1k, Xtensa DSP, Cortex-A53)
> > 
> > Signed-off-by: fuyao <fuyao@allwinnertech.com>
> > ---
> >  MAINTAINERS                           |   6 +
> >  drivers/hwspinlock/Kconfig            |  10 ++
> >  drivers/hwspinlock/Makefile           |   1 +
> >  drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++
> >  4 files changed, 222 insertions(+)
> >  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e451dcce054f0..68d25574432d0 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -737,6 +737,12 @@ L:	linux-media@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/staging/media/sunxi/cedrus/
> >  
> > +ALLWINNER HWSPINLOCK DRIVER
> > +M:	fuyao <fuyao@allwinnertech.com>
> > +S:	Maintained
> > +F:	drivers/hwspinlock/sunxi_hwspinlock.c
> > +F:      Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
> > +
> >  ALPHA PORT
> >  M:	Richard Henderson <rth@twiddle.net>
> >  M:	Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> > diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> > index 32cd26352f381..4d0d516dcb544 100644
> > --- a/drivers/hwspinlock/Kconfig
> > +++ b/drivers/hwspinlock/Kconfig
> > @@ -55,6 +55,16 @@ config HWSPINLOCK_STM32
> >  
> >  	  If unsure, say N.
> >  
> > +config HWSPINLOCK_SUNXI
> > +	tristate "SUNXI Hardware Spinlock device"
> > +	depends on ARCH_SUNXI || COMPILE_TEST
> > +	help
> > +	  Say y here to support the SUNXI Hardware Semaphore functionality, which
> > +	  provides a synchronisation mechanism for the various processor on the
> > +	  SoC.
> > +
> > +	  If unsure, say N.
> > +
> >  config HSEM_U8500
> >  	tristate "STE Hardware Semaphore functionality"
> >  	depends on ARCH_U8500 || COMPILE_TEST
> > diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> > index ed053e3f02be4..839a053205f73 100644
> > --- a/drivers/hwspinlock/Makefile
> > +++ b/drivers/hwspinlock/Makefile
> > @@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
> >  obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
> >  obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
> >  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> > +obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
> > diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> > new file mode 100644
> > index 0000000000000..2c3dc148c9b72
> > --- /dev/null
> > +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SUNXI hardware spinlock driver
> > + *
> > + * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com
> > + *
> 
> Please remove the remainder of this comment, it's already covered by the
> SPDX header above.
> 
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * 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/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/bitops.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/hwspinlock.h>
> > +#include <linux/clk.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/err.h>
> > +#include <linux/reset.h>
> 
> You don't need all of these.
> 
> > +
> > +#include "hwspinlock_internal.h"
> > +
> > +/* hardware spinlock register list */
> > +#define	LOCK_SYS_STATUS_REG             (0x0000)
> > +#define	LOCK_STATUS_REG                 (0x0010)
> > +#define LOCK_BASE_OFFSET                (0x0100)
> > +#define LOCK_BASE_ID                    (0)
> 
> No need for the parenthesis on these, please drop them.
> 
> > +
> > +/* Possible values of SPINLOCK_LOCK_REG */
> > +#define SPINLOCK_NOTTAKEN               (0)     /* free */
> > +#define SPINLOCK_TAKEN                  (1)     /* locked */
> > +
> > +struct sunxi_spinlock_config {
> > +	int nspin;
> > +};
> > +
> > +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> > +{
> > +	void __iomem *lock_addr = lock->priv;
> > +
> > +	/* attempt to acquire the lock by reading its value */
> > +	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
> 
> Please drop the outer ().
> 
> > +}
> > +
> > +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
> > +{
> > +	void __iomem *lock_addr = lock->priv;
> > +
> > +	/* release the lock by writing 0 to it */
> > +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> > +}
> > +
> > +/*
> > + * relax the SUNXI interconnect while spinning on it.
> > + *
> > + * The specs recommended that the retry delay time will be
> > + * just over half of the time that a requester would be
> > + * expected to hold the lock.
> > + *
> > + * in sunxi spinlock time less then 200 cycles
> > + *
> > + * The number below is taken from an hardware specs example,
> > + * obviously it is somewhat arbitrary.
> 
> Thank you for the good explanation.
> 
> > + */
> > +static void sunxi_hwspinlock_relax(struct hwspinlock *lock)
> > +{
> > +	ndelay(50);
> > +}
> > +
> > +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> > +	.trylock = sunxi_hwspinlock_trylock,
> > +	.unlock = sunxi_hwspinlock_unlock,
> > +	.relax = sunxi_hwspinlock_relax,
> > +};
> > +
> > +struct sunxi_hwspinlock_device {
> > +	struct hwspinlock_device *bank;
> > +	struct clk *bus_clk;
> > +	struct reset_control *reset;
> > +};
> > +
> > +static void sunxi_hwspinlock_clk_init(struct platform_device *pdev,
> > +				      struct sunxi_hwspinlock_device *private)
> > +{
> > +	private->bus_clk = devm_clk_get(&pdev->dev, NULL);
> > +	private->reset = devm_reset_control_get(&pdev->dev, NULL);
> 
> You should check the return value of these, e.g. for EPROBE_DEFER and if
> so return appropriately from sunxi_hwspinlock_probe().
> 
> So please move them to the probe function to make this easier and
> cleaner.
> 
> > +
> > +	if (private->reset)
> > +		reset_control_deassert(private->reset);
> > +	if (private->bus_clk)
> > +		clk_prepare_enable(private->bus_clk);
> 
> Both of these apis start with
> 
> 	if (!argument)
> 		return;
> 
> So there's no need for you to check for NULL before calling them. Also
> to make it clear that you want these to be deassered and prepare_enabled
> between probe and remvoe, move them into probe (and next function into
> remove).
> 
> > +}
> > +
> > +static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private)
> > +{
> > +	if (private->reset)
> > +		reset_control_assert(private->reset);
> > +	if (private->bus_clk)
> > +		clk_disable(private->bus_clk);
> > +}
> > +
> > +static const struct sunxi_spinlock_config spin_ver_1 = {
> > +	.nspin = 32,
> > +};
> > +
> > +static const struct of_device_id sunxi_hwspinlock_of_match[] = {
> > +	{
> > +		.compatible = "allwinner,h3-hwspinlock",
> > +		.data = &spin_ver_1,
> 
> If all cases comes with the same "data", then please just put nspin in a
> #define until you're going to support hardware that has some other
> number of locks.
> 
> > +	},
> > +	{
> > +		.compatible = "allwinner,h6-hwspinlock",
> > +		.data = &spin_ver_1,
> > +	},
> > +	{ /* Sentinel */ },
> 
> No need to spell out "/* Sentinel */", leave it emtpy and please drop the , at
> the end.
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match);
> 
> Please move this down by sunxi_hwspinlock_driver and use
> device_get_match_data() instead of of_match_device().
> 
> > +
> > +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +	struct sunxi_hwspinlock_device *private;
> > +	struct hwspinlock_device *bank;
> > +	struct hwspinlock *hwlock;
> > +	const struct sunxi_spinlock_config *config;
> > +	const struct of_device_id *match;
> > +	void __iomem *iobase;
> > +	int num_locks, i, ret;
> > +
> > +	iobase = devm_platform_ioremap_resource(pdev, 0);
> > +	if (PTR_ERR(iobase))
> > +		return PTR_ERR(iobase);
> > +
> > +	match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match),
> > +				&pdev->dev);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	config = match->data;
> > +	num_locks = config->nspin;
> > +
> > +	private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL);
> > +	if (!private)
> > +		return -ENOMEM;
> > +
> > +	bank = devm_kzalloc(&pdev->dev,
> > +			    sizeof(*bank) + num_locks * sizeof(*hwlock),
> > +			    GFP_KERNEL);
> > +	if (!bank)
> > +		return -ENOMEM;
> > +
> > +	private->bank = bank;
> > +	sunxi_hwspinlock_clk_init(pdev, private);
> > +
> > +	platform_set_drvdata(pdev, private);
> > +
> > +	for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
> > +		hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i;
> > +
> > +	ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops,
> > +					LOCK_BASE_ID, num_locks);
> 
> This returns 0 or -errno, so rather than returning ret if ret otherwise
> 0, just do:
> 
> 	return devm_hwspin_lock_register(...)
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> > +{
> > +	struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev);
> > +
> > +	sunxi_hwspinlock_clk_dinit(private);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver sunxi_hwspinlock_driver = {
> > +	.probe		= sunxi_hwspinlock_probe,
> > +	.remove		= sunxi_hwspinlock_remove,
> > +	.driver		= {
> > +		.name	= "sunxi-hwspinlock",
> > +		.owner	= THIS_MODULE,
> 
> module_platform_driver() fills out .owner for you, so please remove
> this.
> 
> > +		.of_match_table = of_match_ptr(sunxi_hwspinlock_of_match),
> 
> Please skip of_match_ptr(), it will just cause build warnings when
> compile tested without CONFIG_OF.
> 
> Thank you,
> Bjorn
> 
> > +	},
> > +};
> > +
> > +module_platform_driver(sunxi_hwspinlock_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI");
> > +MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>");

Thanks for you review, I read it carefully, and learned a lot.

Maxim tells that there is already the same submission, so this
submission will be abandoned.

thanks again.

-- 
fuyao