[PATCH 0/2] hwspinlock: add sunxi hardware spinlock support

[PATCH 0/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

Most of the Allwinner SoCs contain a spinlock unit which can be used for
synchronization/locking between different subsystems. According to the
several Allwinner datasheets most of the sun8i and sun50i SoCs share the
same hardware with at least 32 (32bit wide) registers for 32 spinlocks.
The implemented spinlock hardware can support 32, 64, 128 or 256
registers.

This driver supports all four register bank sizes and also provides some
additional information via debugfs. The driver can be build by setting
the HWSPINLOCK_SUNXI symbol and can be also compiled as module.

This patch adds the driver for this hardware and updates the hwlock
documentation on how to use it. According to the datasheets the H2+, H3,
H5 and H6 SoCs share exactly the same spinlock hardware. But I'm pretty
sure that the whole sun8i family has the same hardware. The sun50i family
may be a different story. The H616 is missing the whole spinlock part in
the datasheets, so I assume this is a sun50i part that does not support
hwspinlocks.

The driver itself is not yet enabled in the devicetree files of the H2/H3,
H5 and H6 SoCs, because for now I'm only able to test the driver against
a H2+ device (OrangePi Zero).

This patch adds:
- hwspinlock driver sunxi_hwspinlock
- hwspinlock dt bindings documentation
- updates MAINTAINERS

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

Wilken Gottwalt (2):
  dt-bindings: hwlock: sunxi: add sunxi_hwspinlock documentation
  hwspinlock: add sunxi hardware spinlock support

 .../bindings/hwlock/sunxi-hwspinlock.yaml     |  64 ++++
 MAINTAINERS                                   |   6 +
 drivers/hwspinlock/Kconfig                    |   9 +
 drivers/hwspinlock/Makefile                   |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c         | 282 ++++++++++++++++++
 5 files changed, 362 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: sunxi: add sunxi_hwspinlock documentation

[Wilken Gottwalt]

Adds documentation on how to use/enable the sunxi_hwspinlock driver.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
 .../bindings/hwlock/sunxi-hwspinlock.yaml     | 64 +++++++++++++++++++
 1 file changed, 64 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 000000000000..773eaa6b33ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.yaml
@@ -0,0 +1,64 @@
+# 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 hardware spinlock for Allwinner based SoCs
+
+maintainers:
+  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
+
+properties:
+  compatible:
+    enum:
+      - allwinner,sun8i-hwspinlock
+      - allwinner,sun50i-hwspinlock
+
+    reg:          # 0x01C18000 (H2+, H3, H5), 0x03004000 (H6), length 0x400 (max 256 * 32bit)
+      maxItems: 1
+
+    clocks:       # phandle to the reference clock
+      maxItems: 1
+
+    clock-names:  # name of the bus ("ahb")
+      maxItems: 1
+
+    resets:       # phandle to the reset control
+      maxItems: 1
+
+    reset-names:  # name of the bus ("ahb")
+      maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+
+  - |
+    /* H2+ based OrangePi Zero */
+    hwspinlock: hwspinlock@1C18000 {
+      compatible = "allwinner,sun8i-hwspinlock";
+      reg = <0x01c18000 0x400>;
+      clocks = <&ccu CLK_BUS_SPINLOCK>;
+      clock-names = "ahb";
+      resets = <&ccu RST_BUS_SPINLOCK>;
+      reset-names = "ahb";
+    };
+
+    /* H6 based OrangePi 3 */
+    hwspinlock: hwspinlock@3004000 {
+      compatible = "allwinner,sun50i-hwspinlock";
+      reg = <0x03004000 0x400>;
+      clocks = <&ccu CLK_BUS_SPINLOCK>;
+      clock-names = "ahb";
+      resets = <&ccu RST_BUS_SPINLOCK>;
+      reset-names = "ahb";
+    };
-- 
2.29.2

[PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

Adds the sunxi_hwspinlock driver and updates makefiles/maintainers.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
 MAINTAINERS                           |   6 +
 drivers/hwspinlock/Kconfig            |   9 +
 drivers/hwspinlock/Makefile           |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c | 282 ++++++++++++++++++++++++++
 4 files changed, 298 insertions(+)
 create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5cc595ac7b28..7765da172f10 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -722,6 +722,12 @@ L:	linux-crypto@vger.kernel.org
 S:	Maintained
 F:	drivers/crypto/allwinner/
 
+ALLWINNER HARDWARE SPINLOCK SUPPORT
+M:	Wilken Gottwalt <wilken.gottwalt@posteo.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.yaml
+F:	drivers/hwspinlock/sunxi_hwspinlock.c
+
 ALLWINNER THERMAL DRIVER
 M:	Vasily Khoruzhick <anarsoul@gmail.com>
 M:	Yangtao Li <tiny.windzz@gmail.com>
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 32cd26352f38..27b6e22126f7 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -55,6 +55,15 @@ 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 Allwinner hardware mutex device available
+	  in the H2+, H3, H5 and H6 SoCs.
+
+	  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 ed053e3f02be..bf46bee95226 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -9,4 +9,5 @@ obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
+obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
new file mode 100644
index 000000000000..54e6ad3ac1c2
--- /dev/null
+++ b/drivers/hwspinlock/sunxi_hwspinlock.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * sunxi_hwspinlock.c - hardware spinlock driver for Allwinner SoCs
+ * Copyright (C) 2020 Wilken Gottwalt <wilken.gottwalt@posteo.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/errno.h>
+#include <linux/hwspinlock.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include "hwspinlock_internal.h"
+
+#define DRIVER_NAME		"sunxi_hwspinlock"
+
+#define SPINLOCK_BASE_ID	0 /* there is only one hwspinlock device in each SoC */
+#define SPINLOCK_SYSSTATUS_REG	0x0000
+#define SPINLOCK_STATUS_REG	0x0010
+#define SPINLOCK_LOCK_REGN	0x0100
+#define SPINLOCK_NOTTAKEN	0
+#define SPINLOCK_TAKEN		1
+
+struct sunxi_hwspinlock {
+	void __iomem *io_base;
+};
+
+struct sunxi_hwspinlock_data {
+	void __iomem *io_base;
+	struct hwspinlock_device *bank;
+	struct reset_control *reset;
+	struct clk *ahb_clock;
+	struct dentry *debugfs;
+	int nlocks;
+};
+
+#ifdef CONFIG_DEBUG_FS
+
+static int hwlocks_supported_show(struct seq_file *seqf, void *unused)
+{
+	struct sunxi_hwspinlock_data *priv = seqf->private;
+
+	seq_printf(seqf, "%d\n", priv->nlocks);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(hwlocks_supported);
+
+static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
+{
+	struct sunxi_hwspinlock_data *priv = seqf->private;
+	int inuse;
+
+	/* getting the status of only the main 32 spinlocks is supported */
+	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
+	seq_printf(seqf, "%d\n", inuse);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(hwlocks_inuse);
+
+static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
+{
+	char name[32];
+
+	scnprintf(name, sizeof(name), "%s", DRIVER_NAME);
+
+	priv->debugfs = debugfs_create_dir(name, NULL);
+	debugfs_create_file("hwlocks_supported", 0444, priv->debugfs, priv,
+			    &hwlocks_supported_fops);
+	debugfs_create_file("hwlocks_inuse", 0444, priv->debugfs, priv, &hwlocks_inuse_fops);
+}
+
+#else
+
+static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
+{
+}
+
+#endif
+
+static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	struct sunxi_hwspinlock_data *priv = lock->priv;
+	void __iomem *lock_addr = priv->io_base;
+
+	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
+}
+
+static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	struct sunxi_hwspinlock_data *priv = lock->priv;
+	void __iomem *lock_addr = priv->io_base;
+
+	writel(SPINLOCK_NOTTAKEN, lock_addr);
+}
+
+static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
+	.trylock	= sunxi_hwspinlock_trylock,
+	.unlock		= sunxi_hwspinlock_unlock,
+};
+
+static int sunxi_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct sunxi_hwspinlock_data *priv;
+	struct sunxi_hwspinlock *hwpriv;
+	struct hwspinlock *hwlock;
+	struct resource *res;
+	int num_banks, err, i;
+
+	if (!node)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->io_base)) {
+		err = PTR_ERR(priv->io_base);
+		dev_err(&pdev->dev, "unable to request MMIO (%d)\n", err);
+		return err;
+	}
+
+	priv->ahb_clock = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(priv->ahb_clock)) {
+		err = PTR_ERR(priv->ahb_clock);
+		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
+		return err;
+	}
+
+	priv->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");
+	if (IS_ERR(priv->reset)) {
+		err = PTR_ERR(priv->reset);
+		if (err == -EPROBE_DEFER)
+			return err;
+
+		dev_info(&pdev->dev, "no optional reset control available (%d)\n", err);
+		priv->reset = NULL;
+	}
+
+	if (priv->reset) {
+		err = reset_control_deassert(priv->reset);
+		if (err) {
+			dev_err(&pdev->dev, "deassert reset control failure (%d)\n", err);
+			return err;
+		}
+	}
+
+	err = clk_prepare_enable(priv->ahb_clock);
+	if (err) {
+		dev_err(&pdev->dev, "unable to prepare AHB clock (%d)\n", err);
+		goto reset_fail;
+	}
+
+	/* bit 28 and 29 hold the amount of spinlock banks */
+	num_banks = (readl(priv->io_base + SPINLOCK_SYSSTATUS_REG) >> 28) & 0x03;
+	switch (num_banks) {
+	case 1 ... 4:
+		/*
+		 * 32, 64, 128 and 256 spinlocks are supported by the hardware implementation,
+		 * though most only support 32 spinlocks
+		 */
+		priv->nlocks = 1 << (4 + num_banks);
+		break;
+	default:
+		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
+		err = -EINVAL;
+		goto fail;
+	}
+
+	/*
+	 * the Allwinner hwspinlock device uses 32bit registers for representing every single
+	 * spinlock, which is a real waste of space
+	 */
+	priv->bank = devm_kzalloc(&pdev->dev, priv->nlocks * sizeof(*hwlock) + sizeof(*priv->bank),
+				  GFP_KERNEL);
+	if (!priv->bank) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	for (i = 0, hwlock = &priv->bank->lock[0]; i < priv->nlocks; ++i, ++hwlock) {
+		hwlock->priv = devm_kzalloc(&pdev->dev, sizeof(struct sunxi_hwspinlock),
+					    GFP_KERNEL);
+		if (!hwlock->priv) {
+			err = -ENOMEM;
+			goto fail;
+		}
+
+		hwpriv = hwlock->priv;
+		hwpriv->io_base = priv->io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
+	}
+
+	err = hwspin_lock_register(priv->bank, &pdev->dev, &sunxi_hwspinlock_ops, SPINLOCK_BASE_ID,
+				   priv->nlocks);
+	if (err) {
+		dev_err(&pdev->dev, "unable to register hwspinlocks (%d)\n", err);
+		goto fail;
+	}
+
+	sunxi_hwspinlock_debugfs_init(priv);
+
+	dev_dbg(&pdev->dev, "SUNXI hardware spinlock driver enabled (%d locks)\n", priv->nlocks);
+
+	return 0;
+
+fail:
+	clk_disable_unprepare(priv->ahb_clock);
+
+reset_fail:
+	if (priv->reset)
+		reset_control_assert(priv->reset);
+
+	return err;
+}
+
+static int sunxi_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct sunxi_hwspinlock_data *priv = platform_get_drvdata(pdev);
+	int err;
+
+	debugfs_remove_recursive(priv->debugfs);
+
+	err = hwspin_lock_unregister(priv->bank);
+	if (err) {
+		dev_err(&pdev->dev, "unregister device failed (%d)\n", err);
+		return err;
+	}
+
+	if (priv->reset)
+		reset_control_assert(priv->reset);
+
+	clk_disable_unprepare(priv->ahb_clock);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_hwspinlock_ids[] = {
+	{ .compatible = "allwinner,sun8i-hwspinlock", },
+	{ .compatible = "allwinner,sun50i-hwspinlock", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
+
+static struct platform_driver sunxi_hwspinlock_driver = {
+	.probe	= sunxi_hwspinlock_probe,
+	.remove	= sunxi_hwspinlock_remove,
+	.driver	= {
+		.name		= DRIVER_NAME,
+		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),
+	},
+};
+
+static int __init sunxi_hwspinlock_init(void)
+{
+	return platform_driver_register(&sunxi_hwspinlock_driver);
+}
+postcore_initcall(sunxi_hwspinlock_init);
+
+static void __exit sunxi_hwspinlock_exit(void)
+{
+	platform_driver_unregister(&sunxi_hwspinlock_driver);
+}
+module_exit(sunxi_hwspinlock_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SUNXI hardware spinlock driver");
+MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
-- 
2.29.2

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Maxime Ripard]

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

Hi Wilken,

On Wed, Nov 18, 2020 at 11:02:40AM +0100, Wilken Gottwalt wrote:
> Adds the sunxi_hwspinlock driver and updates makefiles/maintainers.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

A more descriptive commit log would be welcome here, for example
containing on which SoC this driver can be used, and on which it was
tested.

This is the third attempt at that driver, and you can find the previous
versions here:
https://patchwork.kernel.org/project/linux-arm-kernel/cover/20200210170143.20007-1-nborisov@suse.com/
https://lore.kernel.org/patchwork/patch/706512/

Most of the comments on those series still apply to yours.

Most importantly, this hwspinlock is used to synchronize the ARM cores
and the ARISC. How did you test this driver?

Thanks!
Maxime

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

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

Hi Maxime,

On Wed, 18 Nov 2020 16:37:33 +0100
Maxime Ripard <maxime@cerno.tech> wrote:
> Hi Wilken,
> 
> On Wed, Nov 18, 2020 at 11:02:40AM +0100, Wilken Gottwalt wrote:
> > Adds the sunxi_hwspinlock driver and updates makefiles/maintainers.
> > 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> 
> A more descriptive commit log would be welcome here, for example
> containing on which SoC this driver can be used, and on which it was
> tested.

can you help me here a bit? I still try to figure out how to do patch sets
properly. Some kernel submitting documentation says everything goes into the
coverletter and other documentation only tells how to split the patches. So
what would be the right way? A quick example based on my patch set would be
really helpful.

> This is the third attempt at that driver, and you can find the previous
> versions here:
> https://patchwork.kernel.org/project/linux-arm-kernel/cover/20200210170143.20007-1-nborisov@suse.com/
> https://lore.kernel.org/patchwork/patch/706512/
> 
> Most of the comments on those series still apply to yours.

Oh, I wrote my driver 2-3 years ago and just prepared it for mainline. I
wasn't aware of other attempts. I really should have checked this. Though,
I really want to get to the point where this driver is good enough for
mainline. Hmmm, it is interesting how similar these drivers are. Looks like
the other developers also got inspired by the already existing hwspinlock
drivers. :D

> Most importantly, this hwspinlock is used to synchronize the ARM cores
> and the ARISC. How did you test this driver?

Yes, you are right, I should have mentioned this. I have a simple test kernel
module for this. But I must admit, testing the ARISC is very hard and I have
no real idea how to do it. Testing the hwspinlocks in general seems to work
with my test kernel module, but I'm not sure if this is really sufficient. I
can provide the code for it if you like. What would be the best way? Github?
Just mailing a patch?

The test module produces these results:

# insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
[   45.395672] [init] sunxi hwspinlock test driver start
[   45.400775] [init] start test locks
[   45.404263] [run ] testing 32 locks
[   45.407804] [test] testing lock 0 -----
[   45.411652] [test] taking lock attempt #0 succeded
[   45.416438] [test] try taken lock attempt #0
[   45.420735] [test] unlock/take attempt #0
[   45.424752] [test] taking lock attempt #1 succeded
[   45.429556] [test] try taken lock attempt #1
[   45.433823] [test] unlock/take attempt #1
[   45.437862] [test] testing lock 1 -----
[   45.441699] [test] taking lock attempt #0 succeded
[   45.446484] [test] try taken lock attempt #0
[   45.450768] [test] unlock/take attempt #0
[   45.454774] [test] taking lock attempt #1 succeded
[   45.459576] [test] try taken lock attempt #1
[   45.463843] [test] unlock/take attempt #1
.
.
.
[   46.309925] [test] testing lock 30 -----
[   46.313852] [test] taking lock attempt #0 succeded
[   46.318654] [test] try taken lock attempt #0
[   46.322920] [test] unlock/take attempt #0
[   46.326944] [test] taking lock attempt #1 succeded
[   46.331729] [test] try taken lock attempt #1
[   46.335994] [test] unlock/take attempt #1
[   46.340021] [test] testing lock 31 -----
[   46.343947] [test] taking lock attempt #0 succeded
[   46.348749] [test] try taken lock attempt #0
[   46.353016] [test] unlock/take attempt #0
[   46.357040] [test] taking lock attempt #1 succeded
[   46.361825] [test] try taken lock attempt #1
[   46.366090] [test] unlock/take attempt #1
[   46.370112] [init] end test locks

Greetings,
Wilken

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Maxime Ripard]

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

On Wed, Nov 18, 2020 at 08:36:24PM +0100, Wilken Gottwalt wrote:
> On Wed, 18 Nov 2020 16:37:33 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
> > Hi Wilken,
> > 
> > On Wed, Nov 18, 2020 at 11:02:40AM +0100, Wilken Gottwalt wrote:
> > > Adds the sunxi_hwspinlock driver and updates makefiles/maintainers.
> > > 
> > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > 
> > A more descriptive commit log would be welcome here, for example
> > containing on which SoC this driver can be used, and on which it was
> > tested.
> 
> can you help me here a bit? I still try to figure out how to do patch sets
> properly. Some kernel submitting documentation says everything goes into the
> coverletter and other documentation only tells how to split the patches. So
> what would be the right way? A quick example based on my patch set would be
> really helpful.

I mean, the split between your patches and so on is good, you got that right

The thing I wanted better details on is the commit log itself, so the
message attached to that patch.

> > This is the third attempt at that driver, and you can find the previous
> > versions here:
> > https://patchwork.kernel.org/project/linux-arm-kernel/cover/20200210170143.20007-1-nborisov@suse.com/
> > https://lore.kernel.org/patchwork/patch/706512/
> > 
> > Most of the comments on those series still apply to yours.
> 
> Oh, I wrote my driver 2-3 years ago and just prepared it for mainline. I
> wasn't aware of other attempts. I really should have checked this. Though,
> I really want to get to the point where this driver is good enough for
> mainline. Hmmm, it is interesting how similar these drivers are. Looks like
> the other developers also got inspired by the already existing hwspinlock
> drivers. :D

Yeah, it looks like you all got the same inspiration :)

> > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > and the ARISC. How did you test this driver?
> 
> Yes, you are right, I should have mentioned this. I have a simple test kernel
> module for this. But I must admit, testing the ARISC is very hard and I have
> no real idea how to do it. Testing the hwspinlocks in general seems to work
> with my test kernel module, but I'm not sure if this is really sufficient. I
> can provide the code for it if you like. What would be the best way? Github?
> Just mailing a patch?
> 
> The test module produces these results:
> 
> # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> [   45.395672] [init] sunxi hwspinlock test driver start
> [   45.400775] [init] start test locks
> [   45.404263] [run ] testing 32 locks
> [   45.407804] [test] testing lock 0 -----
> [   45.411652] [test] taking lock attempt #0 succeded
> [   45.416438] [test] try taken lock attempt #0
> [   45.420735] [test] unlock/take attempt #0
> [   45.424752] [test] taking lock attempt #1 succeded
> [   45.429556] [test] try taken lock attempt #1
> [   45.433823] [test] unlock/take attempt #1
> [   45.437862] [test] testing lock 1 -----
> [   45.441699] [test] taking lock attempt #0 succeded
> [   45.446484] [test] try taken lock attempt #0
> [   45.450768] [test] unlock/take attempt #0
> [   45.454774] [test] taking lock attempt #1 succeded
> [   45.459576] [test] try taken lock attempt #1
> [   45.463843] [test] unlock/take attempt #1
> .
> .
> .
> [   46.309925] [test] testing lock 30 -----
> [   46.313852] [test] taking lock attempt #0 succeded
> [   46.318654] [test] try taken lock attempt #0
> [   46.322920] [test] unlock/take attempt #0
> [   46.326944] [test] taking lock attempt #1 succeded
> [   46.331729] [test] try taken lock attempt #1
> [   46.335994] [test] unlock/take attempt #1
> [   46.340021] [test] testing lock 31 -----
> [   46.343947] [test] taking lock attempt #0 succeded
> [   46.348749] [test] try taken lock attempt #0
> [   46.353016] [test] unlock/take attempt #0
> [   46.357040] [test] taking lock attempt #1 succeded
> [   46.361825] [test] try taken lock attempt #1
> [   46.366090] [test] unlock/take attempt #1
> [   46.370112] [init] end test locks

That doesn't really test for contention though, and dealing with
contention is mostly what this hardware is about. Could you make a small
test with crust to see if when the arisc has taken the lock, the ARM
cores can't take it?

Maxime

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

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

On Thu, 19 Nov 2020 08:15:23 +0100
Maxime Ripard <maxime@cerno.tech> wrote:
> > can you help me here a bit? I still try to figure out how to do patch sets
> > properly. Some kernel submitting documentation says everything goes into the
> > coverletter and other documentation only tells how to split the patches. So
> > what would be the right way? A quick example based on my patch set would be
> > really helpful.
> 
> I mean, the split between your patches and so on is good, you got that right
> 
> The thing I wanted better details on is the commit log itself, so the
> message attached to that patch.

Ah yes, I think I got it now. So basically add a nice summary of the coverletter
there.

> > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > and the ARISC. How did you test this driver?
> > 
> > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > module for this. But I must admit, testing the ARISC is very hard and I have
> > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > with my test kernel module, but I'm not sure if this is really sufficient. I
> > can provide the code for it if you like. What would be the best way? Github?
> > Just mailing a patch?
> > 
> > The test module produces these results:
> > 
> > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> > [   45.395672] [init] sunxi hwspinlock test driver start
> > [   45.400775] [init] start test locks
> > [   45.404263] [run ] testing 32 locks
> > [   45.407804] [test] testing lock 0 -----
> > [   45.411652] [test] taking lock attempt #0 succeded
> > [   45.416438] [test] try taken lock attempt #0
> > [   45.420735] [test] unlock/take attempt #0
> > [   45.424752] [test] taking lock attempt #1 succeded
> > [   45.429556] [test] try taken lock attempt #1
> > [   45.433823] [test] unlock/take attempt #1
> > [   45.437862] [test] testing lock 1 -----
> 
> That doesn't really test for contention though, and dealing with
> contention is mostly what this hardware is about. Could you make a small
> test with crust to see if when the arisc has taken the lock, the ARM
> cores can't take it?

So the best solution would be to write a bare metal program that runs on the
arisc and can be triggered from the linux side (the test kernel module) to take
a spinlock ... or at least take spinlocks periodically for a while and watch it
on the linux side. Okay, I think I can do this. Though, I have to dig through
all this new stuff first.

Greetings,
Wilken

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Maxime Ripard]

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

Hi,

On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> On Thu, 19 Nov 2020 08:15:23 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
> > > can you help me here a bit? I still try to figure out how to do patch sets
> > > properly. Some kernel submitting documentation says everything goes into the
> > > coverletter and other documentation only tells how to split the patches. So
> > > what would be the right way? A quick example based on my patch set would be
> > > really helpful.
> > 
> > I mean, the split between your patches and so on is good, you got that right
> > 
> > The thing I wanted better details on is the commit log itself, so the
> > message attached to that patch.
> 
> Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> there.

Yes, a bit more context as well. Eventually, this should be the
motivation on why this patch is useful. So what it can be used for, what
are the challenges, how it was tested, etc.

The cover letter is usually here more to provide some meta-context: what
you expect from the maintainers / reviewers if it's an RFC, if there's
any feature missing or that could be added later on, etc.

> > > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > > and the ARISC. How did you test this driver?
> > > 
> > > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > > module for this. But I must admit, testing the ARISC is very hard and I have
> > > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > > with my test kernel module, but I'm not sure if this is really sufficient. I
> > > can provide the code for it if you like. What would be the best way? Github?
> > > Just mailing a patch?
> > > 
> > > The test module produces these results:
> > > 
> > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> > > [   45.395672] [init] sunxi hwspinlock test driver start
> > > [   45.400775] [init] start test locks
> > > [   45.404263] [run ] testing 32 locks
> > > [   45.407804] [test] testing lock 0 -----
> > > [   45.411652] [test] taking lock attempt #0 succeded
> > > [   45.416438] [test] try taken lock attempt #0
> > > [   45.420735] [test] unlock/take attempt #0
> > > [   45.424752] [test] taking lock attempt #1 succeded
> > > [   45.429556] [test] try taken lock attempt #1
> > > [   45.433823] [test] unlock/take attempt #1
> > > [   45.437862] [test] testing lock 1 -----
> > 
> > That doesn't really test for contention though, and dealing with
> > contention is mostly what this hardware is about. Could you make a small
> > test with crust to see if when the arisc has taken the lock, the ARM
> > cores can't take it?
> 
> So the best solution would be to write a bare metal program that runs on the
> arisc and can be triggered from the linux side (the test kernel module) to take
> a spinlock ... or at least take spinlocks periodically for a while and watch it
> on the linux side. Okay, I think I can do this. Though, I have to dig through
> all this new stuff first.

It doesn't have to be super complicated, just a loop that takes a lock,
sleeps for some time, and releases the lock should be enough to at least
validate that the lock is actually working

Maxime

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

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[fuyao]

On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> > On Thu, 19 Nov 2020 08:15:23 +0100
> > Maxime Ripard <maxime@cerno.tech> wrote:
> > > > can you help me here a bit? I still try to figure out how to do patch sets
> > > > properly. Some kernel submitting documentation says everything goes into the
> > > > coverletter and other documentation only tells how to split the patches. So
> > > > what would be the right way? A quick example based on my patch set would be
> > > > really helpful.
> > > 
> > > I mean, the split between your patches and so on is good, you got that right
> > > 
> > > The thing I wanted better details on is the commit log itself, so the
> > > message attached to that patch.
> > 
> > Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> > there.
> 
> Yes, a bit more context as well. Eventually, this should be the
> motivation on why this patch is useful. So what it can be used for, what
> are the challenges, how it was tested, etc.
> 
> The cover letter is usually here more to provide some meta-context: what
> you expect from the maintainers / reviewers if it's an RFC, if there's
> any feature missing or that could be added later on, etc.
> 
> > > > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > > > and the ARISC. How did you test this driver?
> > > > 
> > > > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > > > module for this. But I must admit, testing the ARISC is very hard and I have
> > > > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > > > with my test kernel module, but I'm not sure if this is really sufficient. I
> > > > can provide the code for it if you like. What would be the best way? Github?
> > > > Just mailing a patch?
> > > > 
> > > > The test module produces these results:
> > > > 
> > > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> > > > [   45.395672] [init] sunxi hwspinlock test driver start
> > > > [   45.400775] [init] start test locks
> > > > [   45.404263] [run ] testing 32 locks
> > > > [   45.407804] [test] testing lock 0 -----
> > > > [   45.411652] [test] taking lock attempt #0 succeded
> > > > [   45.416438] [test] try taken lock attempt #0
> > > > [   45.420735] [test] unlock/take attempt #0
> > > > [   45.424752] [test] taking lock attempt #1 succeded
> > > > [   45.429556] [test] try taken lock attempt #1
> > > > [   45.433823] [test] unlock/take attempt #1
> > > > [   45.437862] [test] testing lock 1 -----
> > > 
> > > That doesn't really test for contention though, and dealing with
> > > contention is mostly what this hardware is about. Could you make a small
> > > test with crust to see if when the arisc has taken the lock, the ARM
> > > cores can't take it?
> > 
> > So the best solution would be to write a bare metal program that runs on the
> > arisc and can be triggered from the linux side (the test kernel module) to take
> > a spinlock ... or at least take spinlocks periodically for a while and watch it
> > on the linux side. Okay, I think I can do this. Though, I have to dig through
> > all this new stuff first.
> 
> It doesn't have to be super complicated, just a loop that takes a lock,
> sleeps for some time, and releases the lock should be enough to at least
> validate that the lock is actually working
> 
I think the difficulty is the bare metal program in arsic has little
documentation.

the arisc is usually used for standby, so don't provide detailed, and
start it with boot0 or uboot.

maybe we can run it use remoteproc to start it.

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Maxime Ripard]

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

On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
> On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> > > On Thu, 19 Nov 2020 08:15:23 +0100
> > > Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > can you help me here a bit? I still try to figure out how to do patch sets
> > > > > properly. Some kernel submitting documentation says everything goes into the
> > > > > coverletter and other documentation only tells how to split the patches. So
> > > > > what would be the right way? A quick example based on my patch set would be
> > > > > really helpful.
> > > > 
> > > > I mean, the split between your patches and so on is good, you got that right
> > > > 
> > > > The thing I wanted better details on is the commit log itself, so the
> > > > message attached to that patch.
> > > 
> > > Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> > > there.
> > 
> > Yes, a bit more context as well. Eventually, this should be the
> > motivation on why this patch is useful. So what it can be used for, what
> > are the challenges, how it was tested, etc.
> > 
> > The cover letter is usually here more to provide some meta-context: what
> > you expect from the maintainers / reviewers if it's an RFC, if there's
> > any feature missing or that could be added later on, etc.
> > 
> > > > > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > > > > and the ARISC. How did you test this driver?
> > > > > 
> > > > > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > > > > module for this. But I must admit, testing the ARISC is very hard and I have
> > > > > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > > > > with my test kernel module, but I'm not sure if this is really sufficient. I
> > > > > can provide the code for it if you like. What would be the best way? Github?
> > > > > Just mailing a patch?
> > > > > 
> > > > > The test module produces these results:
> > > > > 
> > > > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> > > > > [   45.395672] [init] sunxi hwspinlock test driver start
> > > > > [   45.400775] [init] start test locks
> > > > > [   45.404263] [run ] testing 32 locks
> > > > > [   45.407804] [test] testing lock 0 -----
> > > > > [   45.411652] [test] taking lock attempt #0 succeded
> > > > > [   45.416438] [test] try taken lock attempt #0
> > > > > [   45.420735] [test] unlock/take attempt #0
> > > > > [   45.424752] [test] taking lock attempt #1 succeded
> > > > > [   45.429556] [test] try taken lock attempt #1
> > > > > [   45.433823] [test] unlock/take attempt #1
> > > > > [   45.437862] [test] testing lock 1 -----
> > > > 
> > > > That doesn't really test for contention though, and dealing with
> > > > contention is mostly what this hardware is about. Could you make a small
> > > > test with crust to see if when the arisc has taken the lock, the ARM
> > > > cores can't take it?
> > > 
> > > So the best solution would be to write a bare metal program that runs on the
> > > arisc and can be triggered from the linux side (the test kernel module) to take
> > > a spinlock ... or at least take spinlocks periodically for a while and watch it
> > > on the linux side. Okay, I think I can do this. Though, I have to dig through
> > > all this new stuff first.
> > 
> > It doesn't have to be super complicated, just a loop that takes a lock,
> > sleeps for some time, and releases the lock should be enough to at least
> > validate that the lock is actually working
> >
>
> I think the difficulty is the bare metal program in arsic has little
> documentation.

crust has mostly figured it out:
https://github.com/crust-firmware/crust

Maxime

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

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Bjorn Andersson]

On Wed 18 Nov 04:02 CST 2020, Wilken Gottwalt wrote:

> Adds the sunxi_hwspinlock driver and updates makefiles/maintainers.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
>  MAINTAINERS                           |   6 +
>  drivers/hwspinlock/Kconfig            |   9 +
>  drivers/hwspinlock/Makefile           |   1 +
>  drivers/hwspinlock/sunxi_hwspinlock.c | 282 ++++++++++++++++++++++++++
>  4 files changed, 298 insertions(+)
>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5cc595ac7b28..7765da172f10 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -722,6 +722,12 @@ L:	linux-crypto@vger.kernel.org
>  S:	Maintained
>  F:	drivers/crypto/allwinner/
>  
> +ALLWINNER HARDWARE SPINLOCK SUPPORT
> +M:	Wilken Gottwalt <wilken.gottwalt@posteo.net>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.yaml
> +F:	drivers/hwspinlock/sunxi_hwspinlock.c
> +
>  ALLWINNER THERMAL DRIVER
>  M:	Vasily Khoruzhick <anarsoul@gmail.com>
>  M:	Yangtao Li <tiny.windzz@gmail.com>
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 32cd26352f38..27b6e22126f7 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -55,6 +55,15 @@ 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 Allwinner hardware mutex device available
> +	  in the H2+, H3, H5 and H6 SoCs.
> +
> +	  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 ed053e3f02be..bf46bee95226 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
> +obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> new file mode 100644
> index 000000000000..54e6ad3ac1c2
> --- /dev/null
> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * sunxi_hwspinlock.c - hardware spinlock driver for Allwinner SoCs
> + * Copyright (C) 2020 Wilken Gottwalt <wilken.gottwalt@posteo.net>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/errno.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +#define DRIVER_NAME		"sunxi_hwspinlock"
> +
> +#define SPINLOCK_BASE_ID	0 /* there is only one hwspinlock device in each SoC */
> +#define SPINLOCK_SYSSTATUS_REG	0x0000
> +#define SPINLOCK_STATUS_REG	0x0010
> +#define SPINLOCK_LOCK_REGN	0x0100
> +#define SPINLOCK_NOTTAKEN	0
> +#define SPINLOCK_TAKEN		1
> +
> +struct sunxi_hwspinlock {
> +	void __iomem *io_base;
> +};
> +
> +struct sunxi_hwspinlock_data {
> +	void __iomem *io_base;
> +	struct hwspinlock_device *bank;
> +	struct reset_control *reset;
> +	struct clk *ahb_clock;
> +	struct dentry *debugfs;
> +	int nlocks;
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int hwlocks_supported_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sunxi_hwspinlock_data *priv = seqf->private;
> +
> +	seq_printf(seqf, "%d\n", priv->nlocks);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(hwlocks_supported);
> +
> +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sunxi_hwspinlock_data *priv = seqf->private;
> +	int inuse;
> +
> +	/* getting the status of only the main 32 spinlocks is supported */
> +	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));

So this returns how many of the locks are taken? How is that useful?

> +	seq_printf(seqf, "%d\n", inuse);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(hwlocks_inuse);
> +
> +static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
> +{
> +	char name[32];
> +
> +	scnprintf(name, sizeof(name), "%s", DRIVER_NAME);

Why not just pass DRIVER_NAME directly to debugfs_create_dir()?

> +
> +	priv->debugfs = debugfs_create_dir(name, NULL);
> +	debugfs_create_file("hwlocks_supported", 0444, priv->debugfs, priv,
> +			    &hwlocks_supported_fops);
> +	debugfs_create_file("hwlocks_inuse", 0444, priv->debugfs, priv, &hwlocks_inuse_fops);
> +}
> +
> +#else
> +
> +static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
> +{
> +}
> +
> +#endif
> +
> +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	struct sunxi_hwspinlock_data *priv = lock->priv;
> +	void __iomem *lock_addr = priv->io_base;
> +
> +	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
> +}
> +
> +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	struct sunxi_hwspinlock_data *priv = lock->priv;
> +	void __iomem *lock_addr = priv->io_base;
> +
> +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> +}
> +
> +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> +	.trylock	= sunxi_hwspinlock_trylock,
> +	.unlock		= sunxi_hwspinlock_unlock,
> +};
> +
> +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;

I don't see a need for this, or the check to fail if it's NULL. Please
remove it.

> +	struct sunxi_hwspinlock_data *priv;
> +	struct sunxi_hwspinlock *hwpriv;
> +	struct hwspinlock *hwlock;
> +	struct resource *res;
> +	int num_banks, err, i;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->io_base = devm_ioremap_resource(&pdev->dev, res);

Please use devm_platform_ioremap_resource() instead.

> +	if (IS_ERR(priv->io_base)) {
> +		err = PTR_ERR(priv->io_base);
> +		dev_err(&pdev->dev, "unable to request MMIO (%d)\n", err);
> +		return err;
> +	}
> +
> +	priv->ahb_clock = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(priv->ahb_clock)) {
> +		err = PTR_ERR(priv->ahb_clock);
> +		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
> +		return err;
> +	}
> +
> +	priv->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");
> +	if (IS_ERR(priv->reset)) { > +		err = PTR_ERR(priv->reset);
> +		if (err == -EPROBE_DEFER)
> +			return err;
> +
> +		dev_info(&pdev->dev, "no optional reset control available (%d)\n", err);

In the even that no "ahb" reset is specified priv->reset is NULL, as
such if you get here there's something wrong - so it's better to fail
here.

And you can use the convenient dev_err_probe() function to deal with the
EPROBE_DEFER.

> +		priv->reset = NULL;
> +	}
> +
> +	if (priv->reset) {

It's perfectly fine to call reset_control_deassert(NULL); so you can
omit this check.

> +		err = reset_control_deassert(priv->reset);
> +		if (err) {
> +			dev_err(&pdev->dev, "deassert reset control failure (%d)\n", err);
> +			return err;
> +		}
> +	}
> +
> +	err = clk_prepare_enable(priv->ahb_clock);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to prepare AHB clock (%d)\n", err);
> +		goto reset_fail;
> +	}
> +
> +	/* bit 28 and 29 hold the amount of spinlock banks */
> +	num_banks = (readl(priv->io_base + SPINLOCK_SYSSTATUS_REG) >> 28) & 0x03;
> +	switch (num_banks) {
> +	case 1 ... 4:

But the comment above says...and num_banks was & 0x3, so how can it be ...4?

> +		/*
> +		 * 32, 64, 128 and 256 spinlocks are supported by the hardware implementation,
> +		 * though most only support 32 spinlocks
> +		 */
> +		priv->nlocks = 1 << (4 + num_banks);
> +		break;
> +	default:

Given the mask above I believe this would only happen if bits 28 and 29
are both 0...

Regardless I think that this would be better written as a

if (num_banks == invalid) {
	...
	goto fail;
}

priv->nlocks = ...;

> +		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> +		err = -EINVAL;
> +		goto fail;
> +	}
> +
> +	/*
> +	 * the Allwinner hwspinlock device uses 32bit registers for representing every single
> +	 * spinlock, which is a real waste of space
> +	 */

Might be a waste, but having them be the natural write size in hardware
makes sense. I'm however not sure what this comment has to do with the
following allocation.

> +	priv->bank = devm_kzalloc(&pdev->dev, priv->nlocks * sizeof(*hwlock) + sizeof(*priv->bank),

Consider using struct_size() here.

> +				  GFP_KERNEL);
> +	if (!priv->bank) {
> +		err = -ENOMEM;
> +		goto fail;

If you do this allocation before you start the clock and deassert the
reset you can just return -ENOMEM here, instead of the goto.

> +	}
> +
> +	for (i = 0, hwlock = &priv->bank->lock[0]; i < priv->nlocks; ++i, ++hwlock) {
> +		hwlock->priv = devm_kzalloc(&pdev->dev, sizeof(struct sunxi_hwspinlock),
> +					    GFP_KERNEL);

		hwpriv = devm_kzalloc(&pdev->dev, sizeof(*hwpriv), GFP_KERNEL);
		if (!hwpriv)
			return -ENOMEM;

		hwpriv->io_base = ...;
		priv->bank->lock[i].priv = hwpriv;


> +		if (!hwlock->priv) {
> +			err = -ENOMEM;
> +			goto fail;
> +		}
> +
> +		hwpriv = hwlock->priv;
> +		hwpriv->io_base = priv->io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> +	}
> +
> +	err = hwspin_lock_register(priv->bank, &pdev->dev, &sunxi_hwspinlock_ops, SPINLOCK_BASE_ID,
> +				   priv->nlocks);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to register hwspinlocks (%d)\n", err);
> +		goto fail;
> +	}
> +
> +	sunxi_hwspinlock_debugfs_init(priv);
> +
> +	dev_dbg(&pdev->dev, "SUNXI hardware spinlock driver enabled (%d locks)\n", priv->nlocks);
> +
> +	return 0;
> +
> +fail:
> +	clk_disable_unprepare(priv->ahb_clock);
> +
> +reset_fail:
> +	if (priv->reset)
> +		reset_control_assert(priv->reset);
> +
> +	return err;
> +}
> +
> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock_data *priv = platform_get_drvdata(pdev);
> +	int err;
> +
> +	debugfs_remove_recursive(priv->debugfs);
> +
> +	err = hwspin_lock_unregister(priv->bank);
> +	if (err) {
> +		dev_err(&pdev->dev, "unregister device failed (%d)\n", err);
> +		return err;
> +	}
> +
> +	if (priv->reset)
> +		reset_control_assert(priv->reset);
> +
> +	clk_disable_unprepare(priv->ahb_clock);

By using devm_add_action_or_reset() to set up an "assert and unprepare"-
function you can use devm_hwspin_lock_register(), you can drop the
remove function and you can clean up your sunxi_hwspinlock_data (e.g.
you no longer need a pointer to priv->bank).

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> +	{ .compatible = "allwinner,sun50i-hwspinlock", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> +
> +static struct platform_driver sunxi_hwspinlock_driver = {
> +	.probe	= sunxi_hwspinlock_probe,
> +	.remove	= sunxi_hwspinlock_remove,
> +	.driver	= {
> +		.name		= DRIVER_NAME,
> +		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),

Please avoid of_match_ptr, as this will cause warnings about unused
variables when COMPILE_TEST without OF.

Regards,
Bjorn

> +	},
> +};
> +
> +static int __init sunxi_hwspinlock_init(void)
> +{
> +	return platform_driver_register(&sunxi_hwspinlock_driver);
> +}
> +postcore_initcall(sunxi_hwspinlock_init);
> +
> +static void __exit sunxi_hwspinlock_exit(void)
> +{
> +	platform_driver_unregister(&sunxi_hwspinlock_driver);
> +}
> +module_exit(sunxi_hwspinlock_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SUNXI hardware spinlock driver");
> +MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
> -- 
> 2.29.2
> 

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

On Sat, 21 Nov 2020 23:19:00 -0600
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sunxi_hwspinlock_data *priv = seqf->private;
> > +	int inuse;
> > +
> > +	/* getting the status of only the main 32 spinlocks is supported */
> > +	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
> 
> So this returns how many of the locks are taken? How is that useful?

It is a way to see if locks were taken from linux or the arisc core without
touching the actual hwspinlock abi or the locks. So it is a nice way to debug
hwspinlocks, hence it is part of debugfs.

> > +	seq_printf(seqf, "%d\n", inuse);
> > +
> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(hwlocks_inuse);
> > +
> > +static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
> > +{
> > +	char name[32];
> > +
> > +	scnprintf(name, sizeof(name), "%s", DRIVER_NAME);
> 
> Why not just pass DRIVER_NAME directly to debugfs_create_dir()?

Uuuh, you're right, that wasn't very clever. I think I changed the name creation
to something simpler and and just missed most obvious one.

> > +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *node = pdev->dev.of_node;
> 
> I don't see a need for this, or the check to fail if it's NULL. Please
> remove it.

Yeah, will remove it.

> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->io_base = devm_ioremap_resource(&pdev->dev, res);
> 
> Please use devm_platform_ioremap_resource() instead.

Hmm, so there is a platform version of it, too. Will change it.

> > +	priv->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");
> > +	if (IS_ERR(priv->reset)) { > +		err = PTR_ERR(priv->reset);
> > +		if (err == -EPROBE_DEFER)
> > +			return err;
> > +
> > +		dev_info(&pdev->dev, "no optional reset control available (%d)\n", err);
> 
> In the even that no "ahb" reset is specified priv->reset is NULL, as
> such if you get here there's something wrong - so it's better to fail
> here.
> 
> And you can use the convenient dev_err_probe() function to deal with the
> EPROBE_DEFER.
> 
> > +		priv->reset = NULL;
> > +	}
> > +
> > +	if (priv->reset) {
> 
> It's perfectly fine to call reset_control_deassert(NULL); so you can
> omit this check.

Also will update these.

> > +	/* bit 28 and 29 hold the amount of spinlock banks */
> > +	num_banks = (readl(priv->io_base + SPINLOCK_SYSSTATUS_REG) >> 28) & 0x03;
> > +	switch (num_banks) {
> > +	case 1 ... 4:
> 
> But the comment above says...and num_banks was & 0x3, so how can it be ...4?
>
> > +		/*
> > +		 * 32, 64, 128 and 256 spinlocks are supported by the hardware implementation,
> > +		 * though most only support 32 spinlocks
> > +		 */
> > +		priv->nlocks = 1 << (4 + num_banks);
> > +		break;
> > +	default:
> 
> Given the mask above I believe this would only happen if bits 28 and 29
> are both 0...
> 
> Regardless I think that this would be better written as a
> 
> if (num_banks == invalid) {
> 	...
> 	goto fail;
> }
> 
> priv->nlocks = ...;

This one is a really odd one I noticed right after I submitted the patch. I
added the & 0x03 after reading the datasheets again. But I think the datasheets
are not fully correct here. The datasheets say, 0-4 represent 0, 32, 64, 128 and
256 locks and at the same time say, bits 28/29 are used for this and bits 30/31
are reserved. But you can't represent 5 values with 2 bits. So I'm pretty sure
these reserved bits are also used for it, at least bit 30. I will change this to
something which is more clear. It's weird, the H3, H5 and H6 datasheet state
exactly the same issue.

> > +		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> > +		err = -EINVAL;
> > +		goto fail;
> > +	}
> > +
> > +	/*
> > +	 * the Allwinner hwspinlock device uses 32bit registers for representing every single
> > +	 * spinlock, which is a real waste of space
> > +	 */
> 
> Might be a waste, but having them be the natural write size in hardware
> makes sense. I'm however not sure what this comment has to do with the
> following allocation.
> 
> > +	priv->bank = devm_kzalloc(&pdev->dev, priv->nlocks * sizeof(*hwlock) +
> > sizeof(*priv->bank),
> 
> Consider using struct_size() here.
>
> > +				  GFP_KERNEL);
> > +	if (!priv->bank) {
> > +		err = -ENOMEM;
> > +		goto fail;
> 
> If you do this allocation before you start the clock and deassert the
> reset you can just return -ENOMEM here, instead of the goto.
> 
> > +	}
> > +
> > +	for (i = 0, hwlock = &priv->bank->lock[0]; i < priv->nlocks; ++i, ++hwlock) {
> > +		hwlock->priv = devm_kzalloc(&pdev->dev, sizeof(struct sunxi_hwspinlock),
> > +					    GFP_KERNEL);
> 
> 		hwpriv = devm_kzalloc(&pdev->dev, sizeof(*hwpriv), GFP_KERNEL);
> 		if (!hwpriv)
> 			return -ENOMEM;
> 
> 		hwpriv->io_base = ...;
> 		priv->bank->lock[i].priv = hwpriv;
> 
>

You're right, I will update this.

> > +		if (!hwlock->priv) {
> > +			err = -ENOMEM;
> > +			goto fail;
> > +		}
> > +
> > +		hwpriv = hwlock->priv;
> > +		hwpriv->io_base = priv->io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> > +	}
> > +
> > +	err = hwspin_lock_register(priv->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> > SPINLOCK_BASE_ID,
> > +				   priv->nlocks);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "unable to register hwspinlocks (%d)\n", err);
> > +		goto fail;
> > +	}
> > +
> > +	sunxi_hwspinlock_debugfs_init(priv);
> > +
> > +	dev_dbg(&pdev->dev, "SUNXI hardware spinlock driver enabled (%d locks)\n",
> > priv->nlocks); +
> > +	return 0;
> > +
> > +fail:
> > +	clk_disable_unprepare(priv->ahb_clock);
> > +
> > +reset_fail:
> > +	if (priv->reset)
> > +		reset_control_assert(priv->reset);
> > +
> > +	return err;
> > +}
> > +
> > +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> > +{
> > +	struct sunxi_hwspinlock_data *priv = platform_get_drvdata(pdev);
> > +	int err;
> > +
> > +	debugfs_remove_recursive(priv->debugfs);
> > +
> > +	err = hwspin_lock_unregister(priv->bank);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "unregister device failed (%d)\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (priv->reset)
> > +		reset_control_assert(priv->reset);
> > +
> > +	clk_disable_unprepare(priv->ahb_clock);
> 
> By using devm_add_action_or_reset() to set up an "assert and unprepare"-
> function you can use devm_hwspin_lock_register(), you can drop the
> remove function and you can clean up your sunxi_hwspinlock_data (e.g.
> you no longer need a pointer to priv->bank).

I'm not very used to these devm_* functions yet, but will try to use these.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> > +	{ .compatible = "allwinner,sun50i-hwspinlock", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > +
> > +static struct platform_driver sunxi_hwspinlock_driver = {
> > +	.probe	= sunxi_hwspinlock_probe,
> > +	.remove	= sunxi_hwspinlock_remove,
> > +	.driver	= {
> > +		.name		= DRIVER_NAME,
> > +		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),
> 
> Please avoid of_match_ptr, as this will cause warnings about unused
> variables when COMPILE_TEST without OF.

So did you mean to leave it out completely?

Thanks for looking through this, this really helps a lot. :-)

greetings,
Wilken

> Regards,
> Bjorn
> 
> > +	},
> > +};
> > +
> > +static int __init sunxi_hwspinlock_init(void)
> > +{
> > +	return platform_driver_register(&sunxi_hwspinlock_driver);
> > +}
> > +postcore_initcall(sunxi_hwspinlock_init);
> > +
> > +static void __exit sunxi_hwspinlock_exit(void)
> > +{
> > +	platform_driver_unregister(&sunxi_hwspinlock_driver);
> > +}
> > +module_exit(sunxi_hwspinlock_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("SUNXI hardware spinlock driver");
> > +MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
> > -- 
> > 2.29.2
> > 

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

On Sat, 21 Nov 2020 17:44:18 +0100
Maxime Ripard <maxime@cerno.tech> wrote:

> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
> > On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> > > Hi,
> > > 
> > > On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> > > > On Thu, 19 Nov 2020 08:15:23 +0100
> > > > Maxime Ripard <maxime@cerno.tech> wrote:
> > > > > > can you help me here a bit? I still try to figure out how to do patch sets
> > > > > > properly. Some kernel submitting documentation says everything goes into the
> > > > > > coverletter and other documentation only tells how to split the patches. So
> > > > > > what would be the right way? A quick example based on my patch set would be
> > > > > > really helpful.
> > > > > 
> > > > > I mean, the split between your patches and so on is good, you got that right
> > > > > 
> > > > > The thing I wanted better details on is the commit log itself, so the
> > > > > message attached to that patch.
> > > > 
> > > > Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> > > > there.
> > > 
> > > Yes, a bit more context as well. Eventually, this should be the
> > > motivation on why this patch is useful. So what it can be used for, what
> > > are the challenges, how it was tested, etc.
> > > 
> > > The cover letter is usually here more to provide some meta-context: what
> > > you expect from the maintainers / reviewers if it's an RFC, if there's
> > > any feature missing or that could be added later on, etc.
> > > 
> > > > > > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > > > > > and the ARISC. How did you test this driver?
> > > > > > 
> > > > > > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > > > > > module for this. But I must admit, testing the ARISC is very hard and I have
> > > > > > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > > > > > with my test kernel module, but I'm not sure if this is really sufficient. I
> > > > > > can provide the code for it if you like. What would be the best way? Github?
> > > > > > Just mailing a patch?
> > > > > > 
> > > > > > The test module produces these results:
> > > > > > 
> > > > > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> > > > > > [   45.395672] [init] sunxi hwspinlock test driver start
> > > > > > [   45.400775] [init] start test locks
> > > > > > [   45.404263] [run ] testing 32 locks
> > > > > > [   45.407804] [test] testing lock 0 -----
> > > > > > [   45.411652] [test] taking lock attempt #0 succeded
> > > > > > [   45.416438] [test] try taken lock attempt #0
> > > > > > [   45.420735] [test] unlock/take attempt #0
> > > > > > [   45.424752] [test] taking lock attempt #1 succeded
> > > > > > [   45.429556] [test] try taken lock attempt #1
> > > > > > [   45.433823] [test] unlock/take attempt #1
> > > > > > [   45.437862] [test] testing lock 1 -----
> > > > > 
> > > > > That doesn't really test for contention though, and dealing with
> > > > > contention is mostly what this hardware is about. Could you make a small
> > > > > test with crust to see if when the arisc has taken the lock, the ARM
> > > > > cores can't take it?
> > > > 
> > > > So the best solution would be to write a bare metal program that runs on the
> > > > arisc and can be triggered from the linux side (the test kernel module) to take
> > > > a spinlock ... or at least take spinlocks periodically for a while and watch it
> > > > on the linux side. Okay, I think I can do this. Though, I have to dig through
> > > > all this new stuff first.
> > > 
> > > It doesn't have to be super complicated, just a loop that takes a lock,
> > > sleeps for some time, and releases the lock should be enough to at least
> > > validate that the lock is actually working
> > >
> >
> > I think the difficulty is the bare metal program in arsic has little
> > documentation.
> 
> crust has mostly figured it out:
> https://github.com/crust-firmware/crust

I actually have serious trouble to get crust running. It compiles for H2+/H3, but
I can't figure out if it runs at all. I will switch to a H5 based device which is
confirmed to work. If I see this correctly crust is doing nothing with spinlocks
yet, so I may end up also working on crust, adding the spinlocks there too. Don't
know yet how long I will take to understand every detail, but I will report
progress.

Greetings,
Wilken

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Samuel Holland]

On 11/23/20 12:32 PM, Wilken Gottwalt wrote:
> On Sat, 21 Nov 2020 17:44:18 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
>> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
>>> On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
>>>>> On Thu, 19 Nov 2020 08:15:23 +0100
>>>>> Maxime Ripard <maxime@cerno.tech> wrote:
>>>>>>> can you help me here a bit? I still try to figure out how to do patch sets
>>>>>>> properly. Some kernel submitting documentation says everything goes into the
>>>>>>> coverletter and other documentation only tells how to split the patches. So
>>>>>>> what would be the right way? A quick example based on my patch set would be
>>>>>>> really helpful.
>>>>>>
>>>>>> I mean, the split between your patches and so on is good, you got that right
>>>>>>
>>>>>> The thing I wanted better details on is the commit log itself, so the
>>>>>> message attached to that patch.
>>>>>
>>>>> Ah yes, I think I got it now. So basically add a nice summary of the coverletter
>>>>> there.
>>>>
>>>> Yes, a bit more context as well. Eventually, this should be the
>>>> motivation on why this patch is useful. So what it can be used for, what
>>>> are the challenges, how it was tested, etc.
>>>>
>>>> The cover letter is usually here more to provide some meta-context: what
>>>> you expect from the maintainers / reviewers if it's an RFC, if there's
>>>> any feature missing or that could be added later on, etc.
>>>>
>>>>>>>> Most importantly, this hwspinlock is used to synchronize the ARM cores
>>>>>>>> and the ARISC. How did you test this driver?
>>>>>>>
>>>>>>> Yes, you are right, I should have mentioned this. I have a simple test kernel
>>>>>>> module for this. But I must admit, testing the ARISC is very hard and I have
>>>>>>> no real idea how to do it. Testing the hwspinlocks in general seems to work
>>>>>>> with my test kernel module, but I'm not sure if this is really sufficient. I
>>>>>>> can provide the code for it if you like. What would be the best way? Github?
>>>>>>> Just mailing a patch?
>>>>>>>
>>>>>>> The test module produces these results:
>>>>>>>
>>>>>>> # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
>>>>>>> [   45.395672] [init] sunxi hwspinlock test driver start
>>>>>>> [   45.400775] [init] start test locks
>>>>>>> [   45.404263] [run ] testing 32 locks
>>>>>>> [   45.407804] [test] testing lock 0 -----
>>>>>>> [   45.411652] [test] taking lock attempt #0 succeded
>>>>>>> [   45.416438] [test] try taken lock attempt #0
>>>>>>> [   45.420735] [test] unlock/take attempt #0
>>>>>>> [   45.424752] [test] taking lock attempt #1 succeded
>>>>>>> [   45.429556] [test] try taken lock attempt #1
>>>>>>> [   45.433823] [test] unlock/take attempt #1
>>>>>>> [   45.437862] [test] testing lock 1 -----
>>>>>>
>>>>>> That doesn't really test for contention though, and dealing with
>>>>>> contention is mostly what this hardware is about. Could you make a small
>>>>>> test with crust to see if when the arisc has taken the lock, the ARM
>>>>>> cores can't take it?
>>>>>
>>>>> So the best solution would be to write a bare metal program that runs on the
>>>>> arisc and can be triggered from the linux side (the test kernel module) to take
>>>>> a spinlock ... or at least take spinlocks periodically for a while and watch it
>>>>> on the linux side. Okay, I think I can do this. Though, I have to dig through
>>>>> all this new stuff first.
>>>>
>>>> It doesn't have to be super complicated, just a loop that takes a lock,
>>>> sleeps for some time, and releases the lock should be enough to at least
>>>> validate that the lock is actually working
>>>>
>>>
>>> I think the difficulty is the bare metal program in arsic has little
>>> documentation.
>>
>> crust has mostly figured it out:
>> https://github.com/crust-firmware/crust
> 
> I actually have serious trouble to get crust running. It compiles for H2+/H3, but
> I can't figure out if it runs at all. I will switch to a H5 based device which is

Crust does not yet support the H2+/H3 (it is active WIP). H5 should work
well.

> confirmed to work. If I see this correctly crust is doing nothing with spinlocks
> yet, so I may end up also working on crust, adding the spinlocks there too. Don't> know yet how long I will take to understand every detail, but I will
report
> progress.

Correct. There is currently no hwspinlock driver in crust. For testing,
you can poke MMIO from the main loop, near the call to scpi_poll() in
common/system.c. You can use the timeout.h functions for timing.

If you want to write a full driver, I would like to know how you expect
to use the hwspinlocks. Allocating the locks has to be coordinated among
all of the users: Linux, U-Boot, crust, any other ARISC firmware, etc.

> Greetings,
> Wilken

Cheers,
Samuel

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Maxime Ripard]

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

On Mon, Nov 23, 2020 at 09:35:52PM -0600, Samuel Holland wrote:
> On 11/23/20 12:32 PM, Wilken Gottwalt wrote:
> > On Sat, 21 Nov 2020 17:44:18 +0100
> > Maxime Ripard <maxime@cerno.tech> wrote:
> > 
> >> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
> >>> On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> >>>> Hi,
> >>>>
> >>>> On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> >>>>> On Thu, 19 Nov 2020 08:15:23 +0100
> >>>>> Maxime Ripard <maxime@cerno.tech> wrote:
> >>>>>>> can you help me here a bit? I still try to figure out how to do patch sets
> >>>>>>> properly. Some kernel submitting documentation says everything goes into the
> >>>>>>> coverletter and other documentation only tells how to split the patches. So
> >>>>>>> what would be the right way? A quick example based on my patch set would be
> >>>>>>> really helpful.
> >>>>>>
> >>>>>> I mean, the split between your patches and so on is good, you got that right
> >>>>>>
> >>>>>> The thing I wanted better details on is the commit log itself, so the
> >>>>>> message attached to that patch.
> >>>>>
> >>>>> Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> >>>>> there.
> >>>>
> >>>> Yes, a bit more context as well. Eventually, this should be the
> >>>> motivation on why this patch is useful. So what it can be used for, what
> >>>> are the challenges, how it was tested, etc.
> >>>>
> >>>> The cover letter is usually here more to provide some meta-context: what
> >>>> you expect from the maintainers / reviewers if it's an RFC, if there's
> >>>> any feature missing or that could be added later on, etc.
> >>>>
> >>>>>>>> Most importantly, this hwspinlock is used to synchronize the ARM cores
> >>>>>>>> and the ARISC. How did you test this driver?
> >>>>>>>
> >>>>>>> Yes, you are right, I should have mentioned this. I have a simple test kernel
> >>>>>>> module for this. But I must admit, testing the ARISC is very hard and I have
> >>>>>>> no real idea how to do it. Testing the hwspinlocks in general seems to work
> >>>>>>> with my test kernel module, but I'm not sure if this is really sufficient. I
> >>>>>>> can provide the code for it if you like. What would be the best way? Github?
> >>>>>>> Just mailing a patch?
> >>>>>>>
> >>>>>>> The test module produces these results:
> >>>>>>>
> >>>>>>> # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> >>>>>>> [   45.395672] [init] sunxi hwspinlock test driver start
> >>>>>>> [   45.400775] [init] start test locks
> >>>>>>> [   45.404263] [run ] testing 32 locks
> >>>>>>> [   45.407804] [test] testing lock 0 -----
> >>>>>>> [   45.411652] [test] taking lock attempt #0 succeded
> >>>>>>> [   45.416438] [test] try taken lock attempt #0
> >>>>>>> [   45.420735] [test] unlock/take attempt #0
> >>>>>>> [   45.424752] [test] taking lock attempt #1 succeded
> >>>>>>> [   45.429556] [test] try taken lock attempt #1
> >>>>>>> [   45.433823] [test] unlock/take attempt #1
> >>>>>>> [   45.437862] [test] testing lock 1 -----
> >>>>>>
> >>>>>> That doesn't really test for contention though, and dealing with
> >>>>>> contention is mostly what this hardware is about. Could you make a small
> >>>>>> test with crust to see if when the arisc has taken the lock, the ARM
> >>>>>> cores can't take it?
> >>>>>
> >>>>> So the best solution would be to write a bare metal program that runs on the
> >>>>> arisc and can be triggered from the linux side (the test kernel module) to take
> >>>>> a spinlock ... or at least take spinlocks periodically for a while and watch it
> >>>>> on the linux side. Okay, I think I can do this. Though, I have to dig through
> >>>>> all this new stuff first.
> >>>>
> >>>> It doesn't have to be super complicated, just a loop that takes a lock,
> >>>> sleeps for some time, and releases the lock should be enough to at least
> >>>> validate that the lock is actually working
> >>>>
> >>>
> >>> I think the difficulty is the bare metal program in arsic has little
> >>> documentation.
> >>
> >> crust has mostly figured it out:
> >> https://github.com/crust-firmware/crust
> > 
> > I actually have serious trouble to get crust running. It compiles for H2+/H3, but
> > I can't figure out if it runs at all. I will switch to a H5 based device which is
> 
> Crust does not yet support the H2+/H3 (it is active WIP). H5 should work
> well.
> 
> > confirmed to work. If I see this correctly crust is doing nothing
> > with spinlocks yet, so I may end up also working on crust, adding
> > the spinlocks there too. Don't know yet how long I will take to
> > understand every detail, but I will report progress.
> 
> Correct. There is currently no hwspinlock driver in crust. For testing,
> you can poke MMIO from the main loop, near the call to scpi_poll() in
> common/system.c. You can use the timeout.h functions for timing.

Yeah, that would be enough for me. We only really need to make sure that
the concurrency is properly handled to merge the driver

> If you want to write a full driver, I would like to know how you expect
> to use the hwspinlocks. Allocating the locks has to be coordinated among
> all of the users: Linux, U-Boot, crust, any other ARISC firmware, etc.

while that can come as a second step and I wouldn't make it a
requirement for Linux to merged the hwspinlock driver.

Maxime

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

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Bjorn Andersson]

On Mon 23 Nov 12:17 CST 2020, Wilken Gottwalt wrote:

> On Sat, 21 Nov 2020 23:19:00 -0600
> Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > > +{
> > > +	struct sunxi_hwspinlock_data *priv = seqf->private;
> > > +	int inuse;
> > > +
> > > +	/* getting the status of only the main 32 spinlocks is supported */
> > > +	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
> > 
> > So this returns how many of the locks are taken? How is that useful?
> 
> It is a way to see if locks were taken from linux or the arisc core without
> touching the actual hwspinlock abi or the locks. So it is a nice way to debug
> hwspinlocks, hence it is part of debugfs.
> 

So in a scenario where two remote processors ping-pong the lock between
them, this will always read 1 and you won't know why?

> > > +	seq_printf(seqf, "%d\n", inuse);
[..]
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > > +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> > > +	{ .compatible = "allwinner,sun50i-hwspinlock", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > > +
> > > +static struct platform_driver sunxi_hwspinlock_driver = {
> > > +	.probe	= sunxi_hwspinlock_probe,
> > > +	.remove	= sunxi_hwspinlock_remove,
> > > +	.driver	= {
> > > +		.name		= DRIVER_NAME,
> > > +		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),
> > 
> > Please avoid of_match_ptr, as this will cause warnings about unused
> > variables when COMPILE_TEST without OF.
> 
> So did you mean to leave it out completely?
> 

Yes, "worst case" is that you include the reference to
sunxi_hwspinlock_ids on a build without CONFIG_OF and wasting a little
bit of memory.

Using of_match_ptr() with CONFIG_OF=n will result in NULL and as such
we'll get a compile warning that nothing references sunxi_hwspinlock_ids
- so then that will have to be marked __maybe_unused, or wrapped in an
#if...

So better just leave it as:
	.of_match_table = sunxi_hwspinlock_ids,

Regards,
Bjorn

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

On Mon, 23 Nov 2020 21:35:52 -0600
Samuel Holland <samuel@sholland.org> wrote:

> On 11/23/20 12:32 PM, Wilken Gottwalt wrote:
> > On Sat, 21 Nov 2020 17:44:18 +0100
> > Maxime Ripard <maxime@cerno.tech> wrote:
> > 
> >> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
> >>> On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> >>>> Hi,
> >>>>
> >>>> On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> >>>>> On Thu, 19 Nov 2020 08:15:23 +0100
> >>>>> Maxime Ripard <maxime@cerno.tech> wrote:
> >>>>>>> can you help me here a bit? I still try to figure out how to do patch sets
> >>>>>>> properly. Some kernel submitting documentation says everything goes into the
> >>>>>>> coverletter and other documentation only tells how to split the patches. So
> >>>>>>> what would be the right way? A quick example based on my patch set would be
> >>>>>>> really helpful.
> >>>>>>
> >>>>>> I mean, the split between your patches and so on is good, you got that right
> >>>>>>
> >>>>>> The thing I wanted better details on is the commit log itself, so the
> >>>>>> message attached to that patch.
> >>>>>
> >>>>> Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> >>>>> there.
> >>>>
> >>>> Yes, a bit more context as well. Eventually, this should be the
> >>>> motivation on why this patch is useful. So what it can be used for, what
> >>>> are the challenges, how it was tested, etc.
> >>>>
> >>>> The cover letter is usually here more to provide some meta-context: what
> >>>> you expect from the maintainers / reviewers if it's an RFC, if there's
> >>>> any feature missing or that could be added later on, etc.
> >>>>
> >>>>>>>> Most importantly, this hwspinlock is used to synchronize the ARM cores
> >>>>>>>> and the ARISC. How did you test this driver?
> >>>>>>>
> >>>>>>> Yes, you are right, I should have mentioned this. I have a simple test kernel
> >>>>>>> module for this. But I must admit, testing the ARISC is very hard and I have
> >>>>>>> no real idea how to do it. Testing the hwspinlocks in general seems to work
> >>>>>>> with my test kernel module, but I'm not sure if this is really sufficient. I
> >>>>>>> can provide the code for it if you like. What would be the best way? Github?
> >>>>>>> Just mailing a patch?
> >>>>>>>
> >>>>>>> The test module produces these results:
> >>>>>>>
> >>>>>>> # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko 
> >>>>>>> [   45.395672] [init] sunxi hwspinlock test driver start
> >>>>>>> [   45.400775] [init] start test locks
> >>>>>>> [   45.404263] [run ] testing 32 locks
> >>>>>>> [   45.407804] [test] testing lock 0 -----
> >>>>>>> [   45.411652] [test] taking lock attempt #0 succeded
> >>>>>>> [   45.416438] [test] try taken lock attempt #0
> >>>>>>> [   45.420735] [test] unlock/take attempt #0
> >>>>>>> [   45.424752] [test] taking lock attempt #1 succeded
> >>>>>>> [   45.429556] [test] try taken lock attempt #1
> >>>>>>> [   45.433823] [test] unlock/take attempt #1
> >>>>>>> [   45.437862] [test] testing lock 1 -----
> >>>>>>
> >>>>>> That doesn't really test for contention though, and dealing with
> >>>>>> contention is mostly what this hardware is about. Could you make a small
> >>>>>> test with crust to see if when the arisc has taken the lock, the ARM
> >>>>>> cores can't take it?
> >>>>>
> >>>>> So the best solution would be to write a bare metal program that runs on the
> >>>>> arisc and can be triggered from the linux side (the test kernel module) to take
> >>>>> a spinlock ... or at least take spinlocks periodically for a while and watch it
> >>>>> on the linux side. Okay, I think I can do this. Though, I have to dig through
> >>>>> all this new stuff first.
> >>>>
> >>>> It doesn't have to be super complicated, just a loop that takes a lock,
> >>>> sleeps for some time, and releases the lock should be enough to at least
> >>>> validate that the lock is actually working
> >>>>
> >>>
> >>> I think the difficulty is the bare metal program in arsic has little
> >>> documentation.
> >>
> >> crust has mostly figured it out:
> >> https://github.com/crust-firmware/crust
> > 
> > I actually have serious trouble to get crust running. It compiles for H2+/H3, but
> > I can't figure out if it runs at all. I will switch to a H5 based device which is
> 
> Crust does not yet support the H2+/H3 (it is active WIP). H5 should work
> well.
> 
> > confirmed to work. If I see this correctly crust is doing nothing with spinlocks
> > yet, so I may end up also working on crust, adding the spinlocks there too. Don't> know yet how
> > long I will take to understand every detail, but I will
> report
> > progress.
> 
> Correct. There is currently no hwspinlock driver in crust. For testing,
> you can poke MMIO from the main loop, near the call to scpi_poll() in
> common/system.c. You can use the timeout.h functions for timing.

Thank you very much for the hint. I already have a very simple test running
were crust changes the state of the first spinlock every 250ms (with the high
timeout it is much easier to catch).
 
> If you want to write a full driver, I would like to know how you expect
> to use the hwspinlocks. Allocating the locks has to be coordinated among
> all of the users: Linux, U-Boot, crust, any other ARISC firmware, etc.

I will think about this if the Linux hwspinlock driver is in an acceptable
state and I can easily show, that it works. I want to create more complex
tests first.

Can I actualy print messages from crust to the debug uart while linux runs?
It doesn't matter if the messages get scrambled while both write to the uart.
It would be just nice to see this playing out in "realtime".

> > Greetings,
> > Wilken
> 
> Cheers,
> Samuel

Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

[Wilken Gottwalt]

On Tue, 24 Nov 2020 08:54:25 -0600
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> On Mon 23 Nov 12:17 CST 2020, Wilken Gottwalt wrote:
> 
> > On Sat, 21 Nov 2020 23:19:00 -0600
> > Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > > > +{
> > > > +	struct sunxi_hwspinlock_data *priv = seqf->private;
> > > > +	int inuse;
> > > > +
> > > > +	/* getting the status of only the main 32 spinlocks is supported */
> > > > +	inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
> > > 
> > > So this returns how many of the locks are taken? How is that useful?
> > 
> > It is a way to see if locks were taken from linux or the arisc core without
> > touching the actual hwspinlock abi or the locks. So it is a nice way to debug
> > hwspinlocks, hence it is part of debugfs.
> > 
> 
> So in a scenario where two remote processors ping-pong the lock between
> them, this will always read 1 and you won't know why?

I know it is not perfect. I will change it to actually report which locks are
taken. And currently the crust firmware does not use the locks and on the
Linux side there are only a handful of driver/components using hwspinlocks,
and none of them are active in a kernel compiled for a H5. So it really is a
nice way to check/debug the hwspinlocks. I already have a simple test running
where crust sets a spinlock and I can see it on Linux without touching the
actuall locks thanks to this status register.

> > > > +	seq_printf(seqf, "%d\n", inuse);
> [..]
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > > > +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> > > > +	{ .compatible = "allwinner,sun50i-hwspinlock", },
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > > > +
> > > > +static struct platform_driver sunxi_hwspinlock_driver = {
> > > > +	.probe	= sunxi_hwspinlock_probe,
> > > > +	.remove	= sunxi_hwspinlock_remove,
> > > > +	.driver	= {
> > > > +		.name		= DRIVER_NAME,
> > > > +		.of_match_table	= of_match_ptr(sunxi_hwspinlock_ids),
> > > 
> > > Please avoid of_match_ptr, as this will cause warnings about unused
> > > variables when COMPILE_TEST without OF.
> > 
> > So did you mean to leave it out completely?
> > 
> 
> Yes, "worst case" is that you include the reference to
> sunxi_hwspinlock_ids on a build without CONFIG_OF and wasting a little
> bit of memory.
> 
> Using of_match_ptr() with CONFIG_OF=n will result in NULL and as such
> we'll get a compile warning that nothing references sunxi_hwspinlock_ids
> - so then that will have to be marked __maybe_unused, or wrapped in an
> #if...
> 
> So better just leave it as:
> 	.of_match_table = sunxi_hwspinlock_ids,

Thank you for the explanation. I really like to know the details and reasons
behind this.

greetings,
Wilken

> Regards,
> Bjorn