linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] hwspinlock: add sun6i hardware spinlock support
@ 2020-12-23 11:34 Wilken Gottwalt
  2020-12-23 11:34 ` [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock Wilken Gottwalt
  2020-12-23 11:35 ` [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
  0 siblings, 2 replies; 13+ messages in thread
From: Wilken Gottwalt @ 2020-12-23 11:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

Most of the Allwinner sun6i compatible devices contain a spinlock unit
which can be used to sync access to devices shared between the ARM cores
and the embedded companion core. According to the datasheets at least 32
spinlocks are supported. The implementation supports 32, 64, 128 and 256
spinlock setups, but there is no known SoC yet, which implements more
than 32 spinlocks.

This driver adds support for this hardware spinlock unit to Linux
including all 4 possible setups. The driver reports the found setup via
debugfs. It can be build as a builtin and normal module by using the
HWSPINLOCK_SUN8I symbol.

This driver is the first step to enable hwspinlock support in Linux, but
also requires support in the firmware of the companion core. This patch
provides the driver and binding documentation but is not yet included
into the sunxi files. Also not every sunxi seem to have support for this
hardware, but it can be found in sun6i, sun8i, sun9i and sun50i devices.

The spinlock hardware has two ways to figure out if a lock is taken. The
lock can simply be readi, or bits of a 32bit wide status register can be
checked. The status register only supports the first 32 locks and may
not cover bigger spinlock setups. Therefore reading/writing a specific
spinlock is used in the driver for checking the status of a lock.

The status register is now free for debugging/testing purposes and can
completely bypass the Linux hwspinlock ABI. This status register will be
used in some additional kernel modules to test the hwspinlock driver.

Testing the driver.

To run all tests it is necessary to take locks on the companion core and
show on the Linux side that the locks were taken by an external event.
This can be achived by using an Allwinner SoC which includes an OpenRisc
companion core and can use the free crust firmware. For this the crust
firmware needs to be changed to take and release spinlocks (a simple
MMIO operation on the hwlock registers), which is currently not
supported by the current crust firmware. The necessary crust fork can
be found here https://github.com/wgottwalt/crust (hwspinlock branch).
It is also necessary to build u-boot with support for this crust/SCP
firmware. This u-boot fork can be found here
https://github.com/crust-firmware/u-boot (crust branch).

To test this driver it is also necessary to pick a device that is fully
supported by the crust firmware. For this the H5 based Friendlyarm
NanoPi NEO2 was used. It is fully supported by u-boot (and the fork),
the crust firmware (via H5 target) and current Linux kernels. In the
crust fork it is necessary to go into debug menu of "make nconfig" and
select the hwspinlock test loop. This debug option enables a loop that
goes through the first 32 spinlocks. It takes/releases a lock one after
another using the timeout functions (and hw timers) of the crust
firmware. A timeout can be set in the debug menu.

Test 1:

This test was done using a mainline u-boot and a crust enabled u-boot.
For this a simple second kernel module was used, which can be found here
https://github.com/wgottwalt/sunxi_hwspinlock/tree/main/test.

Using mainline u-boot it shows that the Linux side correctly takes a
lock, tries to recursively take a lock again (which does not happen) and
releases a lock. This is done for all 32 locks several times.

# modprobe sun6i_hwspinlock_test
[  472.182172] [init]--- SUN6I HWSPINLOCK DRIVER TEST ---
[  472.187491] [run ]--- testing locks 0 to 31 ---
[  472.192058] [test] testing lock 0
[  472.195371] [test]+++ attempt #0 succeded
[  472.199394] [test]+++ attempt #1 succeded
[  472.203411] [test]+++ attempt #2 succeded
[  472.207433] [test] testing lock 1
[  472.210755] [test]+++ attempt #0 succeded
...
[  472.665684] [test]+++ attempt #2 succeded
[  472.669704] [test] testing lock 31
[  472.673117] [test]+++ attempt #0 succeded
[  472.677137] [test]+++ attempt #1 succeded
[  472.681152] [test]+++ attempt #2 succeded

If the same test is done with the hwspinlock loop enabled crust firmware
and the crust enabled u-boot fork, the Linux test kernel module hits the
one lock taken by the crust firmware.

# modprobe sun6i_hwspinlock_test
...
[  945.871840] [test]+++ attempt #1 succeded
[  945.875854] [test]+++ attempt #2 succeded
[  945.879875] [test] testing lock 18
[  945.883273] [test]+++ attempt #0 succeded
[  945.887293] [test]+++ attempt #1 succeded
[  945.891310] [test]+++ attempt #2 succeded
[  945.895329] [test] testing lock 19
[  945.898738] [test] taking lock attempt #0 failed (-16)
[  945.903886] [run ]--- testing specific lock 19 failed (-14) ---
[  945.909811] [test] testing lock 20
[  945.913224] [test]+++ attempt #0 succeded
[  945.917245] [test]+++ attempt #1 succeded
[  945.921265] [test]+++ attempt #2 succeded
[  945.925281] [test] testing lock 21
[  945.928694] [test]+++ attempt #0 succeded
[  945.932709] [test]+++ attempt #1 succeded
...

Test 2:

This is a more complex test which uses the status register to bypass the
Linux hwspinlock ABI. For this to work a slightly modified driver is
used. It can be found here
https://github.com/wgottwalt/sunxi_hwspinlock/tree/main/modified.
This modified driver splits the 4K memory range into two and leaves the
status register untouched. It can now be used by another test kernel
module, which can be found here
https://github.com/wgottwalt/sunxi_hwspinlock/tree/main/test2.
It is also necessary to change the device tree entries to get both
kernel modules working in parallel.

hwspinlock-mod@1c18000 {
        compatible = "allwinner,sun6i-a31-hwspinlock-mod";
        reg = <0x01c18000 0x4 0x01c18100 0x400>;
        clocks = <&ccu CLK_BUS_SPINLOCK>;
        clock-names = "ahb";
        resets = <&ccu RST_BUS_SPINLOCK>;
        reset-names = "ahb";
        status = "okay";
};

hwspinlock-stat@1c18010 {
        compatible = "allwinner,sun6i-a31-hwspinlock-stat";
        reg = <0x01c18010 0x4>;
        status = "okay";
};

The extended test kernel module supports four different modes to test
the hwspinlocks. Two of them are sufficient to show the spinlock
mechanism working.

Test 2 Mode 1:

This test reads and prints the status register continuously. The crust
firmware and the test are set to a hwspinlock timeout of one second. The
test kernel module code runs a bit slower because of more code executed
and all the printing. Because of that you can see how one lock is missed
completely between entry 8 and 9.

# modprobe sun6i_hwspinlock_test2 mode=1 loops=10
[  179.611838] [init]--- SUN6I HWSPINLOCK DRIVER ENHANCED TEST ---
[  179.618114] [sreg] 10000000_00000000_00000000_00000000
[  180.643989] [sreg] 01000000_00000000_00000000_00000000
[  181.668006] [sreg] 00100000_00000000_00000000_00000000
[  182.691985] [sreg] 00010000_00000000_00000000_00000000
[  183.715985] [sreg] 00001000_00000000_00000000_00000000
[  184.739987] [sreg] 00000100_00000000_00000000_00000000
[  185.763986] [sreg] 00000010_00000000_00000000_00000000
[  186.787985] [sreg] 00000001_00000000_00000000_00000000
[  187.811985] [sreg] 00000000_01000000_00000000_00000000
[  188.835985] [sreg] 00000000_00100000_00000000_00000000

Test 2 Mode 3:

This test combines the Linux hwspinlock ABI approach from the first test
and the status register access. The "after" reads show the locks taken
by both sides until the Linux hwspinlock driver tries to take the lock
taken by the crust firmware.

# modprobe sun6i_hwspinlock_test2 mode=3
[  454.734821] [init]--- SUN6I HWSPINLOCK DRIVER ENHANCED TEST ---
...
[  455.897153] [test]+++ attempt #1 succeded
[  455.901178] [sreg] before take 00000000_00000000_00000000_00100000
[  455.907372] [sreg] after take 00000000_00000000_00000000_01100000
[  455.913478] [sreg] after recursive take 00000000_00000000_00000000_01100000
[  455.920455] [sreg] after untake 00000000_00000000_00000000_00100000
[  455.926721] [test]+++ attempt #2 succeded
[  455.930741] [test] testing lock 26
[  455.934157] [sreg] before take 00000000_00000000_00000000_00100000
[  455.940345] [test] taking lock attempt #0 failed (-16)
[  455.945492] [run ]--- testing specific lock 26 failed (-14) ---
[  455.951419] [test] testing lock 27
[  455.954836] [sreg] before take 00000000_00000000_00000000_00100000
[  455.961034] [sreg] after take 00000000_00000000_00000000_00110000
[  455.967135] [sreg] after recursive take 00000000_00000000_00000000_00110000
[  455.974109] [sreg] after untake 00000000_00000000_00000000_00100000
[  455.980375] [test]+++ attempt #0 succeded
[  455.984400] [sreg] before take 00000000_00000000_00000000_00100000

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

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

Changes in v5:
  - changed symbols to earliest known supported SoC (A31)
  - simplified dt documentation
  - fixed several types
  - updated test description
  - changed init back to classic probe/remove callbacks

Changes in v4:
  - changed binding from sun8i-hwspinlock to sun8i-a33-hwspinlock
  - fixed several issues in the dt documentation
  - further simplified driver
  - fixed an add_action_and_reset_* function issue
  - fixed some typos

Changes in v3:
  - moved test description to cover letter
  - changed name and symbols from sunxi to sun8i
  - improved driver description
  - further simplified driver
  - fully switched to devm_* and devm_add_action_* functions

Changes in v2:
  - redone coverletter
  - fixed ranges in the device tree description
  - added suggestions from Bjorn Andersson and Maxime Ripard to the driver
  - provided better driver and test description

Wilken Gottwalt (2):
  dt-bindings: hwlock: add sun6i_hwspinlock
  hwspinlock: add sun6i hardware spinlock support

 .../bindings/hwlock/sun6i-a31-hwspinlock.yaml |  44 ++++
 MAINTAINERS                                   |   6 +
 drivers/hwspinlock/Kconfig                    |   9 +
 drivers/hwspinlock/Makefile                   |   1 +
 drivers/hwspinlock/sun6i_hwspinlock.c         | 214 ++++++++++++++++++
 5 files changed, 274 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
 create mode 100644 drivers/hwspinlock/sun6i_hwspinlock.c

-- 
2.29.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2020-12-23 11:34 [PATCH v5 0/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
@ 2020-12-23 11:34 ` Wilken Gottwalt
  2020-12-23 22:49   ` Rob Herring
  2020-12-23 11:35 ` [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
  1 sibling, 1 reply; 13+ messages in thread
From: Wilken Gottwalt @ 2020-12-23 11:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
compatible SoCs.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
Changes in v5:
  - changed binding to earliest known supported SoC sun6i-a31
  - dropped unnecessary entries

Changes in v4:
  - changed binding to sun8i-a33-hwpinlock
  - added changes suggested by Maxime Ripard

Changes in v3:
  - changed symbols from sunxi to sun8i

Changes in v2:
  - fixed memory ranges
---
 .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml

diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
new file mode 100644
index 000000000000..481c5c995ad7
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
+
+maintainers:
+  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
+
+description:
+  The hardware unit provides semaphores between the ARM cores and the embedded
+  companion core on the SoC.
+
+properties:
+  compatible:
+    const: allwinner,sun6i-a31-hwspinlock
+
+    reg:
+      maxItems: 1
+
+    clocks:
+      maxItems: 1
+
+    resets:
+      maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    hwspinlock@1c18000 {
+      compatible = "allwinner,sun6i-a31-hwspinlock";
+      reg = <0x01c18000 0x1000>;
+      clocks = <&ccu CLK_BUS_SPINLOCK>;
+      resets = <&ccu RST_BUS_SPINLOCK>;
+    };
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2020-12-23 11:34 [PATCH v5 0/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
  2020-12-23 11:34 ` [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock Wilken Gottwalt
@ 2020-12-23 11:35 ` Wilken Gottwalt
  2021-01-06 10:15   ` Maxime Ripard
  2021-01-07 17:43   ` Bjorn Andersson
  1 sibling, 2 replies; 13+ messages in thread
From: Wilken Gottwalt @ 2020-12-23 11:35 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
most of the sun6i compatible SoCs.

This unit provides at least 32 spinlocks in hardware. The implementation
supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
reading a register and released by writing a 0 to it. This driver
supports all 4 spinlock setups, but for now only the first setup (32
locks) seem to exist in available devices. This spinlock unit is shared
between all ARM cores and the embedded companion core. All of them can
take/release a lock with a single cycle operation. It can be used to
sync access to devices shared by the ARM cores and the companion core.

There are two ways to check if a lock is taken. The first way is to read
a lock. If a 0 is returned, the lock was free and is taken now. If an 1
is returned, the caller has to try again. Which means the lock is taken.
The second way is to read a 32bit wide status register where every bit
represents one of the 32 first locks. According to the datasheets this
status register supports only the 32 first locks. This is the reason the
first way (lock read/write) approach is used to be able to cover all 256
locks in future devices. The driver also reports the amount of supported
locks via debugfs.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
Changes in v5:
  - changed symbols to the earliest known supported SoC (sun6i/a31)
  - changed init back to classic probe/remove callbacks

Changes in v4:
  - further simplified driver
  - fixed an add_action_and_reset_ function issue
  - changed bindings from sun8i-hwspinlock to sun8i-a33-hwspinlock

Changes in v3:
  - moved test description to cover letter
  - changed name and symbols from sunxi to sun8i
  - improved driver description
  - further simplified driver
  - fully switched to devm_* and devm_add_action_* functions

Changes in v2:
  - added suggestions from Bjorn Andersson and Maxime Ripard
  - provided better driver and test description
---
 MAINTAINERS                           |   6 +
 drivers/hwspinlock/Kconfig            |   9 ++
 drivers/hwspinlock/Makefile           |   1 +
 drivers/hwspinlock/sun6i_hwspinlock.c | 214 ++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 drivers/hwspinlock/sun6i_hwspinlock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ad0e34bf8453..0842b2a3ea89 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/sun6i-a31-hwspinlock.yaml
+F:	drivers/hwspinlock/sun6i_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..56ecc1aa3166 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -55,6 +55,15 @@ config HWSPINLOCK_STM32
 
 	  If unsure, say N.
 
+config HWSPINLOCK_SUN6I
+	tristate "SUN6I Hardware Spinlock device"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  Say y here to support the SUN6I Hardware Spinlock device which can be
+	  found in most of the sun6i compatible Allwinner 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..83ec4f03decc 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_SUN6I)		+= sun6i_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
diff --git a/drivers/hwspinlock/sun6i_hwspinlock.c b/drivers/hwspinlock/sun6i_hwspinlock.c
new file mode 100644
index 000000000000..ba56eed818e7
--- /dev/null
+++ b/drivers/hwspinlock/sun6i_hwspinlock.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * sun6i_hwspinlock.c - hardware spinlock driver for sun6i compatible Allwinner SoCs
+ * Copyright (C) 2020 Wilken Gottwalt <wilken.gottwalt@posteo.net>
+ */
+
+#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		"sun6i_hwspinlock"
+
+#define SPINLOCK_BASE_ID	0 /* there is only one hwspinlock device per SoC */
+#define SPINLOCK_SYSSTATUS_REG	0x0000
+#define SPINLOCK_LOCK_REGN	0x0100
+#define SPINLOCK_NOTTAKEN	0
+
+struct sun6i_hwspinlock_data {
+	struct hwspinlock_device *bank;
+	struct reset_control *reset;
+	struct clk *ahb_clk;
+	struct dentry *debugfs;
+	int nlocks;
+};
+
+#ifdef CONFIG_DEBUG_FS
+
+static int hwlocks_supported_show(struct seq_file *seqf, void *unused)
+{
+	struct sun6i_hwspinlock_data *priv = seqf->private;
+
+	seq_printf(seqf, "%d\n", priv->nlocks);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(hwlocks_supported);
+
+static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
+{
+	priv->debugfs = debugfs_create_dir(DRIVER_NAME, NULL);
+	debugfs_create_file("supported", 0444, priv->debugfs, priv, &hwlocks_supported_fops);
+}
+
+#else
+
+static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
+{
+}
+
+#endif
+
+static int sun6i_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
+}
+
+static void sun6i_hwspinlock_unlock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	writel(SPINLOCK_NOTTAKEN, lock_addr);
+}
+
+static const struct hwspinlock_ops sun6i_hwspinlock_ops = {
+	.trylock	= sun6i_hwspinlock_trylock,
+	.unlock		= sun6i_hwspinlock_unlock,
+};
+
+static int sun6i_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct sun6i_hwspinlock_data *priv;
+	struct hwspinlock *hwlock;
+	void __iomem *io_base;
+	u32 num_banks;
+	int err, i;
+
+	io_base = devm_platform_ioremap_resource(pdev, SPINLOCK_BASE_ID);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(priv->ahb_clk)) {
+		err = PTR_ERR(priv->ahb_clk);
+		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
+		return err;
+	}
+
+	priv->reset = devm_reset_control_get(&pdev->dev, "ahb");
+	if (IS_ERR(priv->reset))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset),
+				     "unable to get reset control\n");
+
+	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_clk);
+	if (err) {
+		dev_err(&pdev->dev, "unable to prepare AHB clk (%d)\n", err);
+		goto clk_fail;
+	}
+
+	/*
+	 * bit 28 and 29 represents the hwspinlock setup
+	 *
+	 * every datasheet (A64, A80, A83T, H3, H5, H6 ...) says the default value is 0x1 and 0x1
+	 * to 0x4 represent 32, 64, 128 and 256 locks
+	 * but later datasheets (H5, H6) say 00, 01, 10, 11 represent 32, 64, 128 and 256 locks,
+	 * but that would mean H5 and H6 have 64 locks, while their datasheets talk about 32 locks
+	 * all the time, not a single mentioning of 64 locks
+	 * the 0x4 value is also not representable by 2 bits alone, so some datasheets are not
+	 * correct
+	 * one thing have all in common, default value of the sysstatus register is 0x10000000,
+	 * which results in bit 28 being set
+	 * this is the reason 0x1 is considered being 32 locks and bit 30 is taken into account
+	 * verified on H2+ (datasheet 0x1 = 32 locks) and H5 (datasheet 01 = 64 locks)
+	 */
+	num_banks = readl(io_base + SPINLOCK_SYSSTATUS_REG) >> 28;
+	switch (num_banks) {
+	case 1 ... 4:
+		priv->nlocks = 1 << (4 + num_banks);
+		break;
+	default:
+		err = -EINVAL;
+		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
+		goto bank_fail;
+	}
+
+	priv->bank = devm_kzalloc(&pdev->dev, struct_size(priv->bank, lock, priv->nlocks),
+				  GFP_KERNEL);
+	if (!priv->bank) {
+		err = -ENOMEM;
+		goto bank_fail;
+	}
+
+	for (i = 0; i < priv->nlocks; ++i) {
+		hwlock = &priv->bank->lock[i];
+		hwlock->priv = io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
+	}
+
+	/* failure of debugfs is considered non-fatal */
+	sun6i_hwspinlock_debugfs_init(priv);
+	if (IS_ERR(priv->debugfs))
+		priv->debugfs = NULL;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_hwspin_lock_register(&pdev->dev, priv->bank, &sun6i_hwspinlock_ops,
+					 SPINLOCK_BASE_ID, priv->nlocks);
+bank_fail:
+	clk_disable_unprepare(priv->ahb_clk);
+clk_fail:
+	reset_control_assert(priv->reset);
+
+	return err;
+}
+
+static int sun6i_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct sun6i_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;
+	}
+
+	clk_disable_unprepare(priv->ahb_clk);
+	reset_control_assert(priv->reset);
+
+	return 0;
+}
+
+static const struct of_device_id sun6i_hwspinlock_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-hwspinlock", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sun6i_hwspinlock_ids);
+
+static struct platform_driver sun6i_hwspinlock_driver = {
+	.probe	= sun6i_hwspinlock_probe,
+	.remove	= sun6i_hwspinlock_remove,
+	.driver	= {
+		.name		= DRIVER_NAME,
+		.of_match_table	= sun6i_hwspinlock_ids,
+	},
+};
+module_platform_driver(sun6i_hwspinlock_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SUN6I hardware spinlock driver");
+MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2020-12-23 11:34 ` [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock Wilken Gottwalt
@ 2020-12-23 22:49   ` Rob Herring
  2021-01-06 10:14     ` Maxime Ripard
  2021-02-07  9:07     ` Wilken Gottwalt
  0 siblings, 2 replies; 13+ messages in thread
From: Rob Herring @ 2020-12-23 22:49 UTC (permalink / raw)
  To: Wilken Gottwalt
  Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
<wilken.gottwalt@posteo.net> wrote:
>
> Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> compatible SoCs.

Please resend to DT list so that automated checks run and it's in my
queue (PW). You need to run 'make dt_binding_check' as there are
several issues.

> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
> Changes in v5:
>   - changed binding to earliest known supported SoC sun6i-a31
>   - dropped unnecessary entries
>
> Changes in v4:
>   - changed binding to sun8i-a33-hwpinlock
>   - added changes suggested by Maxime Ripard
>
> Changes in v3:
>   - changed symbols from sunxi to sun8i
>
> Changes in v2:
>   - fixed memory ranges
> ---
>  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> new file mode 100644
> index 000000000000..481c5c995ad7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#

This will fail checks. Wrong filename.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> +
> +maintainers:
> +  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
> +
> +description:
> +  The hardware unit provides semaphores between the ARM cores and the embedded
> +  companion core on the SoC.
> +
> +properties:
> +  compatible:
> +    const: allwinner,sun6i-a31-hwspinlock
> +
> +    reg:
> +      maxItems: 1
> +
> +    clocks:
> +      maxItems: 1
> +
> +    resets:
> +      maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    hwspinlock@1c18000 {

hwlock@...

> +      compatible = "allwinner,sun6i-a31-hwspinlock";
> +      reg = <0x01c18000 0x1000>;
> +      clocks = <&ccu CLK_BUS_SPINLOCK>;
> +      resets = <&ccu RST_BUS_SPINLOCK>;

You need an include for these defines.

> +    };
> --
> 2.29.2
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2020-12-23 22:49   ` Rob Herring
@ 2021-01-06 10:14     ` Maxime Ripard
  2021-01-07  8:36       ` Wilken Gottwalt
  2021-02-07  9:07     ` Wilken Gottwalt
  1 sibling, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2021-01-06 10:14 UTC (permalink / raw)
  To: Rob Herring
  Cc: Wilken Gottwalt, linux-kernel, Ohad Ben-Cohen, Bjorn Andersson,
	Baolin Wang, Chen-Yu Tsai, Jernej Skrabec

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

Hi,

On Wed, Dec 23, 2020 at 03:49:19PM -0700, Rob Herring wrote:
> On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
> <wilken.gottwalt@posteo.net> wrote:
> >
> > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > compatible SoCs.
> 
> Please resend to DT list so that automated checks run and it's in my
> queue (PW). You need to run 'make dt_binding_check' as there are
> several issues.
> 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > ---
> > Changes in v5:
> >   - changed binding to earliest known supported SoC sun6i-a31
> >   - dropped unnecessary entries
> >
> > Changes in v4:
> >   - changed binding to sun8i-a33-hwpinlock
> >   - added changes suggested by Maxime Ripard
> >
> > Changes in v3:
> >   - changed symbols from sunxi to sun8i
> >
> > Changes in v2:
> >   - fixed memory ranges
> > ---
> >  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > new file mode 100644
> > index 000000000000..481c5c995ad7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
> 
> This will fail checks. Wrong filename.

Also, the name should be the whole compatible, so
"allwinner,sun6i-a31-hwspinlock.yaml".

> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> > +
> > +maintainers:
> > +  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > +
> > +description:
> > +  The hardware unit provides semaphores between the ARM cores and the embedded
> > +  companion core on the SoC.
> > +
> > +properties:
> > +  compatible:
> > +    const: allwinner,sun6i-a31-hwspinlock
> > +
> > +    reg:
> > +      maxItems: 1
> > +
> > +    clocks:
> > +      maxItems: 1
> > +
> > +    resets:
> > +      maxItems: 1

And this is not indented properly, it should be at the same level than
compatible, not under it. Doesn't the meta-schema catch this?

Maxime

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2020-12-23 11:35 ` [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
@ 2021-01-06 10:15   ` Maxime Ripard
  2021-01-07  8:43     ` Wilken Gottwalt
  2021-01-07 17:43   ` Bjorn Andersson
  1 sibling, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2021-01-06 10:15 UTC (permalink / raw)
  To: Wilken Gottwalt
  Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang,
	Rob Herring, Chen-Yu Tsai, Jernej Skrabec

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

On Wed, Dec 23, 2020 at 12:35:10PM +0100, Wilken Gottwalt wrote:
> Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> most of the sun6i compatible SoCs.
> 
> This unit provides at least 32 spinlocks in hardware. The implementation
> supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> reading a register and released by writing a 0 to it. This driver
> supports all 4 spinlock setups, but for now only the first setup (32
> locks) seem to exist in available devices. This spinlock unit is shared
> between all ARM cores and the embedded companion core. All of them can
> take/release a lock with a single cycle operation. It can be used to
> sync access to devices shared by the ARM cores and the companion core.
> 
> There are two ways to check if a lock is taken. The first way is to read
> a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> is returned, the caller has to try again. Which means the lock is taken.
> The second way is to read a 32bit wide status register where every bit
> represents one of the 32 first locks. According to the datasheets this
> status register supports only the 32 first locks. This is the reason the
> first way (lock read/write) approach is used to be able to cover all 256
> locks in future devices. The driver also reports the amount of supported
> locks via debugfs.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2021-01-06 10:14     ` Maxime Ripard
@ 2021-01-07  8:36       ` Wilken Gottwalt
  0 siblings, 0 replies; 13+ messages in thread
From: Wilken Gottwalt @ 2021-01-07  8:36 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Rob Herring, linux-kernel, Ohad Ben-Cohen, Bjorn Andersson,
	Baolin Wang, Chen-Yu Tsai, Jernej Skrabec

On Wed, 6 Jan 2021 11:14:36 +0100
Maxime Ripard <maxime@cerno.tech> wrote:

> Hi,
> 
> On Wed, Dec 23, 2020 at 03:49:19PM -0700, Rob Herring wrote:
> > On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
> > <wilken.gottwalt@posteo.net> wrote:
> > >
> > > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > > compatible SoCs.
> > 
> > Please resend to DT list so that automated checks run and it's in my
> > queue (PW). You need to run 'make dt_binding_check' as there are
> > several issues.
> > 
> > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > > ---
> > > Changes in v5:
> > >   - changed binding to earliest known supported SoC sun6i-a31
> > >   - dropped unnecessary entries
> > >
> > > Changes in v4:
> > >   - changed binding to sun8i-a33-hwpinlock
> > >   - added changes suggested by Maxime Ripard
> > >
> > > Changes in v3:
> > >   - changed symbols from sunxi to sun8i
> > >
> > > Changes in v2:
> > >   - fixed memory ranges
> > > ---
> > >  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml new file mode 100644
> > > index 000000000000..481c5c995ad7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > @@ -0,0 +1,44 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
> > 
> > This will fail checks. Wrong filename.
> 
> Also, the name should be the whole compatible, so
> "allwinner,sun6i-a31-hwspinlock.yaml".

Yes, I mixed it up a bit. I was a bit to tired this day. And changing the
names/symbols in the last minute didn't go well either.

> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> > > +
> > > +maintainers:
> > > +  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > > +
> > > +description:
> > > +  The hardware unit provides semaphores between the ARM cores and the embedded
> > > +  companion core on the SoC.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: allwinner,sun6i-a31-hwspinlock
> > > +
> > > +    reg:
> > > +      maxItems: 1
> > > +
> > > +    clocks:
> > > +      maxItems: 1
> > > +
> > > +    resets:
> > > +      maxItems: 1
> 
> And this is not indented properly, it should be at the same level than
> compatible, not under it. Doesn't the meta-schema catch this?

Hmm, yeah, this is odd. I need to check my scripts. The bindings stuff is
clearly not checked.

> Maxime


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2021-01-06 10:15   ` Maxime Ripard
@ 2021-01-07  8:43     ` Wilken Gottwalt
  2021-01-08  8:57       ` Maxime Ripard
  0 siblings, 1 reply; 13+ messages in thread
From: Wilken Gottwalt @ 2021-01-07  8:43 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang,
	Rob Herring, Chen-Yu Tsai, Jernej Skrabec

On Wed, 6 Jan 2021 11:15:42 +0100
Maxime Ripard <maxime@cerno.tech> wrote:

> On Wed, Dec 23, 2020 at 12:35:10PM +0100, Wilken Gottwalt wrote:
> > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> > most of the sun6i compatible SoCs.
> > 
> > This unit provides at least 32 spinlocks in hardware. The implementation
> > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > reading a register and released by writing a 0 to it. This driver
> > supports all 4 spinlock setups, but for now only the first setup (32
> > locks) seem to exist in available devices. This spinlock unit is shared
> > between all ARM cores and the embedded companion core. All of them can
> > take/release a lock with a single cycle operation. It can be used to
> > sync access to devices shared by the ARM cores and the companion core.
> > 
> > There are two ways to check if a lock is taken. The first way is to read
> > a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> > is returned, the caller has to try again. Which means the lock is taken.
> > The second way is to read a 32bit wide status register where every bit
> > represents one of the 32 first locks. According to the datasheets this
> > status register supports only the 32 first locks. This is the reason the
> > first way (lock read/write) approach is used to be able to cover all 256
> > locks in future devices. The driver also reports the amount of supported
> > locks via debugfs.
> > 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> 
> Reviewed-by: Maxime Ripard <mripard@kernel.org>

Does it mean the driver is okay and I only need to fix the bindings
documentation? If so, would you prefer an updated patch set or only the
documentation patch in a new version?

> Thanks!
> Maxime


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2020-12-23 11:35 ` [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
  2021-01-06 10:15   ` Maxime Ripard
@ 2021-01-07 17:43   ` Bjorn Andersson
  2021-01-08  8:50     ` Wilken Gottwalt
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2021-01-07 17:43 UTC (permalink / raw)
  To: Wilken Gottwalt
  Cc: linux-kernel, Ohad Ben-Cohen, Baolin Wang, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

On Wed 23 Dec 05:35 CST 2020, Wilken Gottwalt wrote:

> Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> most of the sun6i compatible SoCs.
> 
> This unit provides at least 32 spinlocks in hardware. The implementation
> supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> reading a register and released by writing a 0 to it. This driver
> supports all 4 spinlock setups, but for now only the first setup (32
> locks) seem to exist in available devices. This spinlock unit is shared
> between all ARM cores and the embedded companion core. All of them can
> take/release a lock with a single cycle operation. It can be used to
> sync access to devices shared by the ARM cores and the companion core.
> 
> There are two ways to check if a lock is taken. The first way is to read
> a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> is returned, the caller has to try again. Which means the lock is taken.
> The second way is to read a 32bit wide status register where every bit
> represents one of the 32 first locks. According to the datasheets this
> status register supports only the 32 first locks. This is the reason the
> first way (lock read/write) approach is used to be able to cover all 256
> locks in future devices. The driver also reports the amount of supported
> locks via debugfs.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
> Changes in v5:
>   - changed symbols to the earliest known supported SoC (sun6i/a31)
>   - changed init back to classic probe/remove callbacks
> 
> Changes in v4:
>   - further simplified driver
>   - fixed an add_action_and_reset_ function issue
>   - changed bindings from sun8i-hwspinlock to sun8i-a33-hwspinlock
> 
> Changes in v3:
>   - moved test description to cover letter
>   - changed name and symbols from sunxi to sun8i
>   - improved driver description
>   - further simplified driver
>   - fully switched to devm_* and devm_add_action_* functions
> 
> Changes in v2:
>   - added suggestions from Bjorn Andersson and Maxime Ripard
>   - provided better driver and test description
> ---
>  MAINTAINERS                           |   6 +
>  drivers/hwspinlock/Kconfig            |   9 ++
>  drivers/hwspinlock/Makefile           |   1 +
>  drivers/hwspinlock/sun6i_hwspinlock.c | 214 ++++++++++++++++++++++++++
>  4 files changed, 230 insertions(+)
>  create mode 100644 drivers/hwspinlock/sun6i_hwspinlock.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ad0e34bf8453..0842b2a3ea89 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/sun6i-a31-hwspinlock.yaml
> +F:	drivers/hwspinlock/sun6i_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..56ecc1aa3166 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -55,6 +55,15 @@ config HWSPINLOCK_STM32
>  
>  	  If unsure, say N.
>  
> +config HWSPINLOCK_SUN6I
> +	tristate "SUN6I Hardware Spinlock device"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  Say y here to support the SUN6I Hardware Spinlock device which can be
> +	  found in most of the sun6i compatible Allwinner 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..83ec4f03decc 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_SUN6I)		+= sun6i_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> diff --git a/drivers/hwspinlock/sun6i_hwspinlock.c b/drivers/hwspinlock/sun6i_hwspinlock.c
> new file mode 100644
> index 000000000000..ba56eed818e7
> --- /dev/null
> +++ b/drivers/hwspinlock/sun6i_hwspinlock.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * sun6i_hwspinlock.c - hardware spinlock driver for sun6i compatible Allwinner SoCs
> + * Copyright (C) 2020 Wilken Gottwalt <wilken.gottwalt@posteo.net>
> + */
> +
> +#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		"sun6i_hwspinlock"

Isn't this the same as KBUILD_MODNAME?

> +
> +#define SPINLOCK_BASE_ID	0 /* there is only one hwspinlock device per SoC */
> +#define SPINLOCK_SYSSTATUS_REG	0x0000
> +#define SPINLOCK_LOCK_REGN	0x0100
> +#define SPINLOCK_NOTTAKEN	0
> +
> +struct sun6i_hwspinlock_data {
> +	struct hwspinlock_device *bank;
> +	struct reset_control *reset;
> +	struct clk *ahb_clk;
> +	struct dentry *debugfs;
> +	int nlocks;
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int hwlocks_supported_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sun6i_hwspinlock_data *priv = seqf->private;
> +
> +	seq_printf(seqf, "%d\n", priv->nlocks);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(hwlocks_supported);
> +
> +static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
> +{
> +	priv->debugfs = debugfs_create_dir(DRIVER_NAME, NULL);
> +	debugfs_create_file("supported", 0444, priv->debugfs, priv, &hwlocks_supported_fops);
> +}
> +
> +#else
> +
> +static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
> +{
> +}
> +
> +#endif
> +
> +static int sun6i_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
> +}
> +
> +static void sun6i_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> +}
> +
> +static const struct hwspinlock_ops sun6i_hwspinlock_ops = {
> +	.trylock	= sun6i_hwspinlock_trylock,
> +	.unlock		= sun6i_hwspinlock_unlock,
> +};
> +
> +static int sun6i_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sun6i_hwspinlock_data *priv;
> +	struct hwspinlock *hwlock;
> +	void __iomem *io_base;
> +	u32 num_banks;
> +	int err, i;
> +
> +	io_base = devm_platform_ioremap_resource(pdev, SPINLOCK_BASE_ID);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(priv->ahb_clk)) {
> +		err = PTR_ERR(priv->ahb_clk);
> +		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
> +		return err;
> +	}
> +
> +	priv->reset = devm_reset_control_get(&pdev->dev, "ahb");
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset),
> +				     "unable to get reset control\n");
> +
> +	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_clk);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to prepare AHB clk (%d)\n", err);
> +		goto clk_fail;
> +	}
> +
> +	/*
> +	 * bit 28 and 29 represents the hwspinlock setup
> +	 *
> +	 * every datasheet (A64, A80, A83T, H3, H5, H6 ...) says the default value is 0x1 and 0x1
> +	 * to 0x4 represent 32, 64, 128 and 256 locks
> +	 * but later datasheets (H5, H6) say 00, 01, 10, 11 represent 32, 64, 128 and 256 locks,
> +	 * but that would mean H5 and H6 have 64 locks, while their datasheets talk about 32 locks
> +	 * all the time, not a single mentioning of 64 locks
> +	 * the 0x4 value is also not representable by 2 bits alone, so some datasheets are not
> +	 * correct
> +	 * one thing have all in common, default value of the sysstatus register is 0x10000000,
> +	 * which results in bit 28 being set
> +	 * this is the reason 0x1 is considered being 32 locks and bit 30 is taken into account
> +	 * verified on H2+ (datasheet 0x1 = 32 locks) and H5 (datasheet 01 = 64 locks)
> +	 */
> +	num_banks = readl(io_base + SPINLOCK_SYSSTATUS_REG) >> 28;
> +	switch (num_banks) {
> +	case 1 ... 4:
> +		priv->nlocks = 1 << (4 + num_banks);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> +		goto bank_fail;
> +	}
> +
> +	priv->bank = devm_kzalloc(&pdev->dev, struct_size(priv->bank, lock, priv->nlocks),
> +				  GFP_KERNEL);
> +	if (!priv->bank) {
> +		err = -ENOMEM;
> +		goto bank_fail;
> +	}
> +
> +	for (i = 0; i < priv->nlocks; ++i) {
> +		hwlock = &priv->bank->lock[i];
> +		hwlock->priv = io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> +	}
> +
> +	/* failure of debugfs is considered non-fatal */
> +	sun6i_hwspinlock_debugfs_init(priv);
> +	if (IS_ERR(priv->debugfs))
> +		priv->debugfs = NULL;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return devm_hwspin_lock_register(&pdev->dev, priv->bank, &sun6i_hwspinlock_ops,
> +					 SPINLOCK_BASE_ID, priv->nlocks);

If this fails you will leave the reset deasserted and the clocks
prepared. So please handle this as well.

Regards,
Bjorn

> +bank_fail:
> +	clk_disable_unprepare(priv->ahb_clk);
> +clk_fail:
> +	reset_control_assert(priv->reset);
> +
> +	return err;
> +}
> +
> +static int sun6i_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sun6i_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;
> +	}
> +
> +	clk_disable_unprepare(priv->ahb_clk);
> +	reset_control_assert(priv->reset);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sun6i_hwspinlock_ids[] = {
> +	{ .compatible = "allwinner,sun6i-a31-hwspinlock", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sun6i_hwspinlock_ids);
> +
> +static struct platform_driver sun6i_hwspinlock_driver = {
> +	.probe	= sun6i_hwspinlock_probe,
> +	.remove	= sun6i_hwspinlock_remove,
> +	.driver	= {
> +		.name		= DRIVER_NAME,
> +		.of_match_table	= sun6i_hwspinlock_ids,
> +	},
> +};
> +module_platform_driver(sun6i_hwspinlock_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SUN6I hardware spinlock driver");
> +MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2021-01-07 17:43   ` Bjorn Andersson
@ 2021-01-08  8:50     ` Wilken Gottwalt
  0 siblings, 0 replies; 13+ messages in thread
From: Wilken Gottwalt @ 2021-01-08  8:50 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: linux-kernel, Ohad Ben-Cohen, Baolin Wang, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

On Thu, 7 Jan 2021 11:43:48 -0600
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> On Wed 23 Dec 05:35 CST 2020, Wilken Gottwalt wrote:
> 
> > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> > most of the sun6i compatible SoCs.
> > 
> > This unit provides at least 32 spinlocks in hardware. The implementation
> > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > reading a register and released by writing a 0 to it. This driver
> > supports all 4 spinlock setups, but for now only the first setup (32
> > locks) seem to exist in available devices. This spinlock unit is shared
> > between all ARM cores and the embedded companion core. All of them can
> > take/release a lock with a single cycle operation. It can be used to
> > sync access to devices shared by the ARM cores and the companion core.
> > 
> > There are two ways to check if a lock is taken. The first way is to read
> > a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> > is returned, the caller has to try again. Which means the lock is taken.
> > The second way is to read a 32bit wide status register where every bit
> > represents one of the 32 first locks. According to the datasheets this
> > status register supports only the 32 first locks. This is the reason the
> > first way (lock read/write) approach is used to be able to cover all 256
> > locks in future devices. The driver also reports the amount of supported
> > locks via debugfs.
> > 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > ---
> > Changes in v5:
> >   - changed symbols to the earliest known supported SoC (sun6i/a31)
> >   - changed init back to classic probe/remove callbacks
> > 
> > Changes in v4:
> >   - further simplified driver
> >   - fixed an add_action_and_reset_ function issue
> >   - changed bindings from sun8i-hwspinlock to sun8i-a33-hwspinlock
> > 
> > Changes in v3:
> >   - moved test description to cover letter
> >   - changed name and symbols from sunxi to sun8i
> >   - improved driver description
> >   - further simplified driver
> >   - fully switched to devm_* and devm_add_action_* functions
> > 
> > Changes in v2:
> >   - added suggestions from Bjorn Andersson and Maxime Ripard
> >   - provided better driver and test description
> > ---
> >  MAINTAINERS                           |   6 +
> >  drivers/hwspinlock/Kconfig            |   9 ++
> >  drivers/hwspinlock/Makefile           |   1 +
> >  drivers/hwspinlock/sun6i_hwspinlock.c | 214 ++++++++++++++++++++++++++
> >  4 files changed, 230 insertions(+)
> >  create mode 100644 drivers/hwspinlock/sun6i_hwspinlock.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ad0e34bf8453..0842b2a3ea89 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/sun6i-a31-hwspinlock.yaml
> > +F:	drivers/hwspinlock/sun6i_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..56ecc1aa3166 100644
> > --- a/drivers/hwspinlock/Kconfig
> > +++ b/drivers/hwspinlock/Kconfig
> > @@ -55,6 +55,15 @@ config HWSPINLOCK_STM32
> >  
> >  	  If unsure, say N.
> >  
> > +config HWSPINLOCK_SUN6I
> > +	tristate "SUN6I Hardware Spinlock device"
> > +	depends on ARCH_SUNXI || COMPILE_TEST
> > +	help
> > +	  Say y here to support the SUN6I Hardware Spinlock device which can be
> > +	  found in most of the sun6i compatible Allwinner 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..83ec4f03decc 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_SUN6I)		+= sun6i_hwspinlock.o
> >  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> > diff --git a/drivers/hwspinlock/sun6i_hwspinlock.c b/drivers/hwspinlock/sun6i_hwspinlock.c
> > new file mode 100644
> > index 000000000000..ba56eed818e7
> > --- /dev/null
> > +++ b/drivers/hwspinlock/sun6i_hwspinlock.c
> > @@ -0,0 +1,214 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * sun6i_hwspinlock.c - hardware spinlock driver for sun6i compatible Allwinner SoCs
> > + * Copyright (C) 2020 Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > + */
> > +
> > +#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		"sun6i_hwspinlock"
> 
> Isn't this the same as KBUILD_MODNAME?

Yes it is, but what is the problem?

> > +
> > +#define SPINLOCK_BASE_ID	0 /* there is only one hwspinlock device per SoC */
> > +#define SPINLOCK_SYSSTATUS_REG	0x0000
> > +#define SPINLOCK_LOCK_REGN	0x0100
> > +#define SPINLOCK_NOTTAKEN	0
> > +
> > +struct sun6i_hwspinlock_data {
> > +	struct hwspinlock_device *bank;
> > +	struct reset_control *reset;
> > +	struct clk *ahb_clk;
> > +	struct dentry *debugfs;
> > +	int nlocks;
> > +};
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +static int hwlocks_supported_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sun6i_hwspinlock_data *priv = seqf->private;
> > +
> > +	seq_printf(seqf, "%d\n", priv->nlocks);
> > +
> > +	return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(hwlocks_supported);
> > +
> > +static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
> > +{
> > +	priv->debugfs = debugfs_create_dir(DRIVER_NAME, NULL);
> > +	debugfs_create_file("supported", 0444, priv->debugfs, priv, &hwlocks_supported_fops);
> > +}
> > +
> > +#else
> > +
> > +static void sun6i_hwspinlock_debugfs_init(struct sun6i_hwspinlock_data *priv)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +static int sun6i_hwspinlock_trylock(struct hwspinlock *lock)
> > +{
> > +	void __iomem *lock_addr = lock->priv;
> > +
> > +	return (readl(lock_addr) == SPINLOCK_NOTTAKEN);
> > +}
> > +
> > +static void sun6i_hwspinlock_unlock(struct hwspinlock *lock)
> > +{
> > +	void __iomem *lock_addr = lock->priv;
> > +
> > +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> > +}
> > +
> > +static const struct hwspinlock_ops sun6i_hwspinlock_ops = {
> > +	.trylock	= sun6i_hwspinlock_trylock,
> > +	.unlock		= sun6i_hwspinlock_unlock,
> > +};
> > +
> > +static int sun6i_hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +	struct sun6i_hwspinlock_data *priv;
> > +	struct hwspinlock *hwlock;
> > +	void __iomem *io_base;
> > +	u32 num_banks;
> > +	int err, i;
> > +
> > +	io_base = devm_platform_ioremap_resource(pdev, SPINLOCK_BASE_ID);
> > +	if (IS_ERR(io_base))
> > +		return PTR_ERR(io_base);
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> > +	if (IS_ERR(priv->ahb_clk)) {
> > +		err = PTR_ERR(priv->ahb_clk);
> > +		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
> > +		return err;
> > +	}
> > +
> > +	priv->reset = devm_reset_control_get(&pdev->dev, "ahb");
> > +	if (IS_ERR(priv->reset))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset),
> > +				     "unable to get reset control\n");
> > +
> > +	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_clk);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "unable to prepare AHB clk (%d)\n", err);
> > +		goto clk_fail;
> > +	}
> > +
> > +	/*
> > +	 * bit 28 and 29 represents the hwspinlock setup
> > +	 *
> > +	 * every datasheet (A64, A80, A83T, H3, H5, H6 ...) says the default value is 0x1 and
> > 0x1
> > +	 * to 0x4 represent 32, 64, 128 and 256 locks
> > +	 * but later datasheets (H5, H6) say 00, 01, 10, 11 represent 32, 64, 128 and 256
> > locks,
> > +	 * but that would mean H5 and H6 have 64 locks, while their datasheets talk about 32
> > locks
> > +	 * all the time, not a single mentioning of 64 locks
> > +	 * the 0x4 value is also not representable by 2 bits alone, so some datasheets are not
> > +	 * correct
> > +	 * one thing have all in common, default value of the sysstatus register is 0x10000000,
> > +	 * which results in bit 28 being set
> > +	 * this is the reason 0x1 is considered being 32 locks and bit 30 is taken into account
> > +	 * verified on H2+ (datasheet 0x1 = 32 locks) and H5 (datasheet 01 = 64 locks)
> > +	 */
> > +	num_banks = readl(io_base + SPINLOCK_SYSSTATUS_REG) >> 28;
> > +	switch (num_banks) {
> > +	case 1 ... 4:
> > +		priv->nlocks = 1 << (4 + num_banks);
> > +		break;
> > +	default:
> > +		err = -EINVAL;
> > +		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> > +		goto bank_fail;
> > +	}
> > +
> > +	priv->bank = devm_kzalloc(&pdev->dev, struct_size(priv->bank, lock, priv->nlocks),
> > +				  GFP_KERNEL);
> > +	if (!priv->bank) {
> > +		err = -ENOMEM;
> > +		goto bank_fail;
> > +	}
> > +
> > +	for (i = 0; i < priv->nlocks; ++i) {
> > +		hwlock = &priv->bank->lock[i];
> > +		hwlock->priv = io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> > +	}
> > +
> > +	/* failure of debugfs is considered non-fatal */
> > +	sun6i_hwspinlock_debugfs_init(priv);
> > +	if (IS_ERR(priv->debugfs))
> > +		priv->debugfs = NULL;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return devm_hwspin_lock_register(&pdev->dev, priv->bank, &sun6i_hwspinlock_ops,
> > +					 SPINLOCK_BASE_ID, priv->nlocks);
> 
> If this fails you will leave the reset deasserted and the clocks
> prepared. So please handle this as well.

Uh, this is embarrassing. For some reason my mind always stick to the devm_
functions, but yes, prepare and deassert have no devm equivalent. I will fix
that.

> Regards,
> Bjorn
> 
> > +bank_fail:
> > +	clk_disable_unprepare(priv->ahb_clk);
> > +clk_fail:
> > +	reset_control_assert(priv->reset);
> > +
> > +	return err;
> > +}
> > +
> > +static int sun6i_hwspinlock_remove(struct platform_device *pdev)
> > +{
> > +	struct sun6i_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;
> > +	}
> > +
> > +	clk_disable_unprepare(priv->ahb_clk);
> > +	reset_control_assert(priv->reset);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id sun6i_hwspinlock_ids[] = {
> > +	{ .compatible = "allwinner,sun6i-a31-hwspinlock", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, sun6i_hwspinlock_ids);
> > +
> > +static struct platform_driver sun6i_hwspinlock_driver = {
> > +	.probe	= sun6i_hwspinlock_probe,
> > +	.remove	= sun6i_hwspinlock_remove,
> > +	.driver	= {
> > +		.name		= DRIVER_NAME,
> > +		.of_match_table	= sun6i_hwspinlock_ids,
> > +	},
> > +};
> > +module_platform_driver(sun6i_hwspinlock_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("SUN6I hardware spinlock driver");
> > +MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
> > -- 
> > 2.29.2
> > 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support
  2021-01-07  8:43     ` Wilken Gottwalt
@ 2021-01-08  8:57       ` Maxime Ripard
  0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2021-01-08  8:57 UTC (permalink / raw)
  To: Wilken Gottwalt
  Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang,
	Rob Herring, Chen-Yu Tsai, Jernej Skrabec

On Thu, Jan 07, 2021 at 09:43:14AM +0100, Wilken Gottwalt wrote:
> On Wed, 6 Jan 2021 11:15:42 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
> > On Wed, Dec 23, 2020 at 12:35:10PM +0100, Wilken Gottwalt wrote:
> > > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> > > most of the sun6i compatible SoCs.
> > > 
> > > This unit provides at least 32 spinlocks in hardware. The implementation
> > > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > > reading a register and released by writing a 0 to it. This driver
> > > supports all 4 spinlock setups, but for now only the first setup (32
> > > locks) seem to exist in available devices. This spinlock unit is shared
> > > between all ARM cores and the embedded companion core. All of them can
> > > take/release a lock with a single cycle operation. It can be used to
> > > sync access to devices shared by the ARM cores and the companion core.
> > > 
> > > There are two ways to check if a lock is taken. The first way is to read
> > > a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> > > is returned, the caller has to try again. Which means the lock is taken.
> > > The second way is to read a 32bit wide status register where every bit
> > > represents one of the 32 first locks. According to the datasheets this
> > > status register supports only the 32 first locks. This is the reason the
> > > first way (lock read/write) approach is used to be able to cover all 256
> > > locks in future devices. The driver also reports the amount of supported
> > > locks via debugfs.
> > > 
> > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > 
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 
> Does it mean the driver is okay and I only need to fix the bindings
> documentation? If so, would you prefer an updated patch set or only the
> documentation patch in a new version?

It means I'm fine with the driver patch but the binding require a new
version. Send a new version of the series with my reviewed-by added to
the driver commit

Maxime

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2020-12-23 22:49   ` Rob Herring
  2021-01-06 10:14     ` Maxime Ripard
@ 2021-02-07  9:07     ` Wilken Gottwalt
  2021-02-09 18:48       ` Maxime Ripard
  1 sibling, 1 reply; 13+ messages in thread
From: Wilken Gottwalt @ 2021-02-07  9:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, Baolin Wang,
	Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec

On Wed, 23 Dec 2020 15:49:19 -0700
Rob Herring <robh+dt@kernel.org> wrote:

> On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
> <wilken.gottwalt@posteo.net> wrote:
> >
> > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > compatible SoCs.
> 
> Please resend to DT list so that automated checks run and it's in my
> queue (PW). You need to run 'make dt_binding_check' as there are
> several issues.

Mentioning somewhere, that yamllint is required would have helped here a lot.
Without it I always ended up with that, what was quite misleading:

ERROR: dtschema minimum version is v2020.8.1
make[1]: *** [Documentation/devicetree/bindings/Makefile:12: check_dtschema_version] Error 1
make: *** [Makefile:1370: dt_binding_check] Error 2

> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > ---
> > Changes in v5:
> >   - changed binding to earliest known supported SoC sun6i-a31
> >   - dropped unnecessary entries
> >
> > Changes in v4:
> >   - changed binding to sun8i-a33-hwpinlock
> >   - added changes suggested by Maxime Ripard
> >
> > Changes in v3:
> >   - changed symbols from sunxi to sun8i
> >
> > Changes in v2:
> >   - fixed memory ranges
> > ---
> >  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml new file mode 100644
> > index 000000000000..481c5c995ad7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
> 
> This will fail checks. Wrong filename.
> 
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> > +
> > +maintainers:
> > +  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > +
> > +description:
> > +  The hardware unit provides semaphores between the ARM cores and the embedded
> > +  companion core on the SoC.
> > +
> > +properties:
> > +  compatible:
> > +    const: allwinner,sun6i-a31-hwspinlock
> > +
> > +    reg:
> > +      maxItems: 1
> > +
> > +    clocks:
> > +      maxItems: 1
> > +
> > +    resets:
> > +      maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - resets
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    hwspinlock@1c18000 {
> 
> hwlock@...

sprd, stm32 and omap using hwspinlock. Why is it okay there and not okay in
my version?

> > +      compatible = "allwinner,sun6i-a31-hwspinlock";
> > +      reg = <0x01c18000 0x1000>;
> > +      clocks = <&ccu CLK_BUS_SPINLOCK>;
> > +      resets = <&ccu RST_BUS_SPINLOCK>;
> 
> You need an include for these defines.

So I guess it is needed because I the clocks/resets are used, right? But why
is it not the case for the sprd example, which also uses clocks?

> > +    };
> > --
> > 2.29.2
> >


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock
  2021-02-07  9:07     ` Wilken Gottwalt
@ 2021-02-09 18:48       ` Maxime Ripard
  0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2021-02-09 18:48 UTC (permalink / raw)
  To: Wilken Gottwalt
  Cc: Rob Herring, linux-kernel, Ohad Ben-Cohen, Bjorn Andersson,
	Baolin Wang, Chen-Yu Tsai, Jernej Skrabec

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

On Sun, Feb 07, 2021 at 10:07:31AM +0100, Wilken Gottwalt wrote:
> On Wed, 23 Dec 2020 15:49:19 -0700
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
> > <wilken.gottwalt@posteo.net> wrote:
> > >
> > > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > > compatible SoCs.
> > 
> > Please resend to DT list so that automated checks run and it's in my
> > queue (PW). You need to run 'make dt_binding_check' as there are
> > several issues.
> 
> Mentioning somewhere, that yamllint is required would have helped here a lot.
> Without it I always ended up with that, what was quite misleading:
> 
> ERROR: dtschema minimum version is v2020.8.1
> make[1]: *** [Documentation/devicetree/bindings/Makefile:12: check_dtschema_version] Error 1
> make: *** [Makefile:1370: dt_binding_check] Error 2
> 
> > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > > ---
> > > Changes in v5:
> > >   - changed binding to earliest known supported SoC sun6i-a31
> > >   - dropped unnecessary entries
> > >
> > > Changes in v4:
> > >   - changed binding to sun8i-a33-hwpinlock
> > >   - added changes suggested by Maxime Ripard
> > >
> > > Changes in v3:
> > >   - changed symbols from sunxi to sun8i
> > >
> > > Changes in v2:
> > >   - fixed memory ranges
> > > ---
> > >  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml new file mode 100644
> > > index 000000000000..481c5c995ad7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > @@ -0,0 +1,44 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
> > 
> > This will fail checks. Wrong filename.
> > 
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> > > +
> > > +maintainers:
> > > +  - Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > > +
> > > +description:
> > > +  The hardware unit provides semaphores between the ARM cores and the embedded
> > > +  companion core on the SoC.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: allwinner,sun6i-a31-hwspinlock
> > > +
> > > +    reg:
> > > +      maxItems: 1
> > > +
> > > +    clocks:
> > > +      maxItems: 1
> > > +
> > > +    resets:
> > > +      maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    hwspinlock@1c18000 {
> > 
> > hwlock@...
> 
> sprd, stm32 and omap using hwspinlock. Why is it okay there and not okay in
> my version?

The spec mandates hwlock:

https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst#generic-names-recommendation

They probably introduced their nodes before it was standardized (or
didn't care).

> > > +      compatible = "allwinner,sun6i-a31-hwspinlock";
> > > +      reg = <0x01c18000 0x1000>;
> > > +      clocks = <&ccu CLK_BUS_SPINLOCK>;
> > > +      resets = <&ccu RST_BUS_SPINLOCK>;
> > 
> > You need an include for these defines.
> 
> So I guess it is needed because I the clocks/resets are used, right? But why
> is it not the case for the sprd example, which also uses clocks?

If you're talking about sprd,hwspinlock-r3p0, it doesn't use any define
(and thus doesn't need any include), and it's a binding in a text format
that wouldn't compile the example (this is only done for yaml bindings).
Even if it was wrong for them to do so, they wouldn't notice.

Maxime

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-02-09 21:01 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23 11:34 [PATCH v5 0/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
2020-12-23 11:34 ` [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock Wilken Gottwalt
2020-12-23 22:49   ` Rob Herring
2021-01-06 10:14     ` Maxime Ripard
2021-01-07  8:36       ` Wilken Gottwalt
2021-02-07  9:07     ` Wilken Gottwalt
2021-02-09 18:48       ` Maxime Ripard
2020-12-23 11:35 ` [PATCH v5 2/2] hwspinlock: add sun6i hardware spinlock support Wilken Gottwalt
2021-01-06 10:15   ` Maxime Ripard
2021-01-07  8:43     ` Wilken Gottwalt
2021-01-08  8:57       ` Maxime Ripard
2021-01-07 17:43   ` Bjorn Andersson
2021-01-08  8:50     ` Wilken Gottwalt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).