Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
On Sat, Mar 06, 2021 at 12:05:22PM +0100, Wilken Gottwalt wrote: > On Tue, 2 Mar 2021 18:20:02 +0100 > Maxime Ripard wrote: > > > Hi, > > > > On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote: > > > On Mon, 1 Mar 2021 14:13:05 +0100 > > > Maxime Ripard wrote: > > > > > > > On Sat, Feb 27, 2021 at 02:03:54PM +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 > > > > > > Nope, I had to replace the devm_hwspin_lock_register function by the > > > hwspin_lock_register function because like Bjorn pointed out that it can > > > fail and needs to handled correctly. And having a devm_* function does not > > > play well with the non-devm clock/reset functions and winding back if an > > > error occurs. It also messes with the call order in the remove function. > > > So > > > I went back to the classic way where I have full control over the call > > > order. > > > > If you're talking about the clock and reset line reassertion, I don't > > really see what the trouble is. Sure, it's not going to be in the exact > > same order in remove, but it's still going to execute in the proper > > order (ie, clock disable, then reset disable, then clock put and reset > > put). And you can use devm_add_action if you want to handle things > > automatically. > > See, in v5 zje result of devm_hwspin_lock_register was returned directly. The > remove callback or the bank_fail/clk_fail labels would not run, if the > registering > fails. In v6 it is fixed. Yeah, and it's indeed an issue... > + platform_set_drvdata(pdev, priv); > + > + return devm_hwspin_lock_register(>dev, priv->bank, > _hwspinlock_ops, > + SPINLOCK_BASE_ID, priv->nlocks); > +bank_fail: > + clk_disable_unprepare(priv->ahb_clk); > +clk_fail: > + reset_control_assert(priv->reset); > + > + return err; > +} > > So, is v6 fine for you even if it uses a more classic approach? ... but completely getting rid of the devm_ calls isn't a solution, if that's what you're calling the more classic approach. if (devm_hwspin_lock_register(..)) goto bank_fail return 0; works, and using devm_add_action like I suggested works as well to fix the issue you mentioned above Maxime signature.asc Description: PGP signature
Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
On Tue, 2 Mar 2021 18:20:02 +0100 Maxime Ripard wrote: > Hi, > > On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote: > > On Mon, 1 Mar 2021 14:13:05 +0100 > > Maxime Ripard wrote: > > > > > On Sat, Feb 27, 2021 at 02:03:54PM +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 > > > > Nope, I had to replace the devm_hwspin_lock_register function by the > > hwspin_lock_register function because like Bjorn pointed out that it can > > fail and needs to handled correctly. And having a devm_* function does not > > play well with the non-devm clock/reset functions and winding back if an > > error occurs. It also messes with the call order in the remove function. So > > I went back to the classic way where I have full control over the call > > order. > > If you're talking about the clock and reset line reassertion, I don't > really see what the trouble is. Sure, it's not going to be in the exact > same order in remove, but it's still going to execute in the proper > order (ie, clock disable, then reset disable, then clock put and reset > put). And you can use devm_add_action if you want to handle things > automatically. See, in v5 zje result of devm_hwspin_lock_register was returned directly. The remove callback or the bank_fail/clk_fail labels would not run, if the registering fails. In v6 it is fixed. + platform_set_drvdata(pdev, priv); + + return devm_hwspin_lock_register(>dev, priv->bank, _hwspinlock_ops, +SPINLOCK_BASE_ID, priv->nlocks); +bank_fail: + clk_disable_unprepare(priv->ahb_clk); +clk_fail: + reset_control_assert(priv->reset); + + return err; +} So, is v6 fine for you even if it uses a more classic approach? greetings, Will
Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
Hi, On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote: > On Mon, 1 Mar 2021 14:13:05 +0100 > Maxime Ripard wrote: > > > On Sat, Feb 27, 2021 at 02:03:54PM +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 > > Nope, I had to replace the devm_hwspin_lock_register function by the > hwspin_lock_register function because like Bjorn pointed out that it can > fail and needs to handled correctly. And having a devm_* function does not > play well with the non-devm clock/reset functions and winding back if an > error occurs. It also messes with the call order in the remove function. So > I went back to the classic way where I have full control over the call order. If you're talking about the clock and reset line reassertion, I don't really see what the trouble is. Sure, it's not going to be in the exact same order in remove, but it's still going to execute in the proper order (ie, clock disable, then reset disable, then clock put and reset put). And you can use devm_add_action if you want to handle things automatically. Maxime signature.asc Description: PGP signature
Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
On Mon, 1 Mar 2021 14:13:05 +0100 Maxime Ripard wrote: > On Sat, Feb 27, 2021 at 02:03:54PM +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 Nope, I had to replace the devm_hwspin_lock_register function by the hwspin_lock_register function because like Bjorn pointed out that it can fail and needs to handled correctly. And having a devm_* function does not play well with the non-devm clock/reset functions and winding back if an error occurs. It also messes with the call order in the remove function. So I went back to the classic way where I have full control over the call order. > Didn't I review this one already? > Maxime
Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
On Sat, Feb 27, 2021 at 02:03:54PM +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 Didn't I review this one already? Maxime
[PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support
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
---
Changes in v6:
- changed probe/remove function to use classic functions to better
handle failure situations
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 | 217 ++
4 files changed, 233 insertions(+)
create mode 100644 drivers/hwspinlock/sun6i_hwspinlock.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d92f85ca831d..d5821c52c502 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -734,6 +734,12 @@ L: linux-cry...@vger.kernel.org
S: Maintained
F: drivers/crypto/allwinner/
+ALLWINNER HARDWARE SPINLOCK SUPPORT
+M: Wilken Gottwalt
+S: Maintained
+F: Documentation/devicetree/bindings/hwlock/allwinner,sun6i-hwspinlock.yaml
+F: drivers/hwspinlock/sun6i_hwspinlock.c
+
ALLWINNER THERMAL DRIVER
M: Vasily Khoruzhick
M: Yangtao Li
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 ..98193c75d81b
--- /dev/null
+++ b/drivers/hwspinlock/sun6i_hwspinlock.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * sun6i_hwspinlock.c - hardware spinlock driver for sun6i compatible
Allwinner SoCs
+ * Copyright (C) 2020 Wilken Gottwalt
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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 0x
+#define SPINLOCK_LOCK_REGN 0x0100
+#define SPINLOCK_NOTTAKEN 0
+
+struct sun6i_hwspinlock_data {
+
