* [PATCH 0/2] introduce sunxi hwspinlock
@ 2020-11-19 6:44 fuyao
2020-11-19 6:44 ` [PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock fuyao
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: fuyao @ 2020-11-19 6:44 UTC (permalink / raw)
To: mripard, wens, linux-remoteproc, linux-kernel; +Cc: fuyao
From: fuyao <fuyao@allwinnertech.com>
this series add hwspinlock of sunxi. it provides hardware assistance for
synchronization between the multiple processors in the system.
(Or1k, Cortex-A7, Cortex-A53, Xtensa)
fuyao (2):
dt-bindings: hwlock: add sunxi hwlock
hwspinlock: add SUNXI implementation
.../bindings/hwlock/sunxi,hwspinlock.yaml | 46 ++++
MAINTAINERS | 6 +
drivers/hwspinlock/Kconfig | 10 +
drivers/hwspinlock/Makefile | 1 +
drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++
5 files changed, 268 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml
create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
--
2.29.2
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock 2020-11-19 6:44 [PATCH 0/2] introduce sunxi hwspinlock fuyao @ 2020-11-19 6:44 ` fuyao 2020-11-19 6:44 ` [PATCH 2/2] hwspinlock: add SUNXI implementation fuyao 2020-11-20 16:07 ` [PATCH 0/2] introduce sunxi hwspinlock Maxime Ripard 2 siblings, 0 replies; 7+ messages in thread From: fuyao @ 2020-11-19 6:44 UTC (permalink / raw) To: mripard, wens, linux-remoteproc, linux-kernel; +Cc: fuyao From: fuyao <fuyao@allwinnertech.com> SUNXI hwspinlock binding DT schema format Signed-off-by: fuyao <fuyao@allwinnertech.com> --- .../bindings/hwlock/sunxi,hwspinlock.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml diff --git a/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml b/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml new file mode 100644 index 0000000000000..68ce93b6d2bcb --- /dev/null +++ b/Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml @@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwlock/sunxi,hwspinlock.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SUNXI HwSpinlock for SUNXI + +maintainers: + - fuyao <fuyao@allwinnertech.com> + +properties: + compatible: + enum: + - allwinner,h3-hwspinlock, # for h3-hwspinlock + - allwinner,h6-hwspinlock, # for h6-hwspinlock + + reg: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - resets + +additionalProperties: false + +examples: + + - | + hwspinlock: spinlock@1c17000 { + compatible = "allwinner,h3-hwspinlock"; + reg = <0x0 0x01c17000 0x0 0x1000>; + clocks = <&ccu CLK_BUS_SPINLOCK>; + resets = <&ccu RST_BUS_SPINLOCK>; + }; + + - | + hwspinlock: spinlock@3004000 { + compatible = "allwinner,h6-hwspinlock"; + reg = <0x0 0x03004000 0x0 0x1000>; + clocks = <&ccu CLK_BUS_SPINLOCK>; + resets = <&ccu RST_BUS_SPINLOCK>; + }; + -- 2.29.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] hwspinlock: add SUNXI implementation 2020-11-19 6:44 [PATCH 0/2] introduce sunxi hwspinlock fuyao 2020-11-19 6:44 ` [PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock fuyao @ 2020-11-19 6:44 ` fuyao 2020-11-21 4:01 ` Bjorn Andersson 2020-11-20 16:07 ` [PATCH 0/2] introduce sunxi hwspinlock Maxime Ripard 2 siblings, 1 reply; 7+ messages in thread From: fuyao @ 2020-11-19 6:44 UTC (permalink / raw) To: mripard, wens, linux-remoteproc, linux-kernel; +Cc: fuyao From: fuyao <fuyao@allwinnertech.com> Add hwspinlock support for the SUNXI Hardware Spinlock device. The Hardware Spinlock device on SUNXI provides hardware assistance for synchronization between the multiple processors in the system (Cortex-A7, or1k, Xtensa DSP, Cortex-A53) Signed-off-by: fuyao <fuyao@allwinnertech.com> --- MAINTAINERS | 6 + drivers/hwspinlock/Kconfig | 10 ++ drivers/hwspinlock/Makefile | 1 + drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++ 4 files changed, 222 insertions(+) create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c diff --git a/MAINTAINERS b/MAINTAINERS index e451dcce054f0..68d25574432d0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -737,6 +737,12 @@ L: linux-media@vger.kernel.org S: Maintained F: drivers/staging/media/sunxi/cedrus/ +ALLWINNER HWSPINLOCK DRIVER +M: fuyao <fuyao@allwinnertech.com> +S: Maintained +F: drivers/hwspinlock/sunxi_hwspinlock.c +F: Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml + ALPHA PORT M: Richard Henderson <rth@twiddle.net> M: Ivan Kokshaysky <ink@jurassic.park.msu.ru> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig index 32cd26352f381..4d0d516dcb544 100644 --- a/drivers/hwspinlock/Kconfig +++ b/drivers/hwspinlock/Kconfig @@ -55,6 +55,16 @@ config HWSPINLOCK_STM32 If unsure, say N. +config HWSPINLOCK_SUNXI + tristate "SUNXI Hardware Spinlock device" + depends on ARCH_SUNXI || COMPILE_TEST + help + Say y here to support the SUNXI Hardware Semaphore functionality, which + provides a synchronisation mechanism for the various processor on the + SoC. + + If unsure, say N. + config HSEM_U8500 tristate "STE Hardware Semaphore functionality" depends on ARCH_U8500 || COMPILE_TEST diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile index ed053e3f02be4..839a053205f73 100644 --- a/drivers/hwspinlock/Makefile +++ b/drivers/hwspinlock/Makefile @@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF) += sirf_hwspinlock.o obj-$(CONFIG_HWSPINLOCK_SPRD) += sprd_hwspinlock.o obj-$(CONFIG_HWSPINLOCK_STM32) += stm32_hwspinlock.o obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o +obj-$(CONFIG_HWSPINLOCK_SUNXI) += sunxi_hwspinlock.o diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c new file mode 100644 index 0000000000000..2c3dc148c9b72 --- /dev/null +++ b/drivers/hwspinlock/sunxi_hwspinlock.c @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * SUNXI hardware spinlock driver + * + * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/bitops.h> +#include <linux/pm_runtime.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/hwspinlock.h> +#include <linux/clk.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/err.h> +#include <linux/reset.h> + +#include "hwspinlock_internal.h" + +/* hardware spinlock register list */ +#define LOCK_SYS_STATUS_REG (0x0000) +#define LOCK_STATUS_REG (0x0010) +#define LOCK_BASE_OFFSET (0x0100) +#define LOCK_BASE_ID (0) + +/* Possible values of SPINLOCK_LOCK_REG */ +#define SPINLOCK_NOTTAKEN (0) /* free */ +#define SPINLOCK_TAKEN (1) /* locked */ + +struct sunxi_spinlock_config { + int nspin; +}; + +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock) +{ + void __iomem *lock_addr = lock->priv; + + /* attempt to acquire the lock by reading its value */ + return (readl(lock_addr) == SPINLOCK_NOTTAKEN); +} + +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock) +{ + void __iomem *lock_addr = lock->priv; + + /* release the lock by writing 0 to it */ + writel(SPINLOCK_NOTTAKEN, lock_addr); +} + +/* + * relax the SUNXI interconnect while spinning on it. + * + * The specs recommended that the retry delay time will be + * just over half of the time that a requester would be + * expected to hold the lock. + * + * in sunxi spinlock time less then 200 cycles + * + * The number below is taken from an hardware specs example, + * obviously it is somewhat arbitrary. + */ +static void sunxi_hwspinlock_relax(struct hwspinlock *lock) +{ + ndelay(50); +} + +static const struct hwspinlock_ops sunxi_hwspinlock_ops = { + .trylock = sunxi_hwspinlock_trylock, + .unlock = sunxi_hwspinlock_unlock, + .relax = sunxi_hwspinlock_relax, +}; + +struct sunxi_hwspinlock_device { + struct hwspinlock_device *bank; + struct clk *bus_clk; + struct reset_control *reset; +}; + +static void sunxi_hwspinlock_clk_init(struct platform_device *pdev, + struct sunxi_hwspinlock_device *private) +{ + private->bus_clk = devm_clk_get(&pdev->dev, NULL); + private->reset = devm_reset_control_get(&pdev->dev, NULL); + + if (private->reset) + reset_control_deassert(private->reset); + if (private->bus_clk) + clk_prepare_enable(private->bus_clk); +} + +static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private) +{ + if (private->reset) + reset_control_assert(private->reset); + if (private->bus_clk) + clk_disable(private->bus_clk); +} + +static const struct sunxi_spinlock_config spin_ver_1 = { + .nspin = 32, +}; + +static const struct of_device_id sunxi_hwspinlock_of_match[] = { + { + .compatible = "allwinner,h3-hwspinlock", + .data = &spin_ver_1, + }, + { + .compatible = "allwinner,h6-hwspinlock", + .data = &spin_ver_1, + }, + { /* Sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match); + +static int sunxi_hwspinlock_probe(struct platform_device *pdev) +{ + struct sunxi_hwspinlock_device *private; + struct hwspinlock_device *bank; + struct hwspinlock *hwlock; + const struct sunxi_spinlock_config *config; + const struct of_device_id *match; + void __iomem *iobase; + int num_locks, i, ret; + + iobase = devm_platform_ioremap_resource(pdev, 0); + if (PTR_ERR(iobase)) + return PTR_ERR(iobase); + + match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match), + &pdev->dev); + if (!match) + return -ENODEV; + + config = match->data; + num_locks = config->nspin; + + private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL); + if (!private) + return -ENOMEM; + + bank = devm_kzalloc(&pdev->dev, + sizeof(*bank) + num_locks * sizeof(*hwlock), + GFP_KERNEL); + if (!bank) + return -ENOMEM; + + private->bank = bank; + sunxi_hwspinlock_clk_init(pdev, private); + + platform_set_drvdata(pdev, private); + + for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++) + hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i; + + ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops, + LOCK_BASE_ID, num_locks); + if (ret) + return ret; + + + return 0; +} + +static int sunxi_hwspinlock_remove(struct platform_device *pdev) +{ + struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev); + + sunxi_hwspinlock_clk_dinit(private); + + return 0; +} + +static struct platform_driver sunxi_hwspinlock_driver = { + .probe = sunxi_hwspinlock_probe, + .remove = sunxi_hwspinlock_remove, + .driver = { + .name = "sunxi-hwspinlock", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(sunxi_hwspinlock_of_match), + }, +}; + +module_platform_driver(sunxi_hwspinlock_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI"); +MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>"); -- 2.29.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hwspinlock: add SUNXI implementation 2020-11-19 6:44 ` [PATCH 2/2] hwspinlock: add SUNXI implementation fuyao @ 2020-11-21 4:01 ` Bjorn Andersson 2020-11-21 14:33 ` fuyao 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2020-11-21 4:01 UTC (permalink / raw) To: fuyao; +Cc: mripard, wens, linux-remoteproc, linux-kernel On Thu 19 Nov 00:44 CST 2020, fuyao@allwinnertech.com wrote: > From: fuyao <fuyao@allwinnertech.com> > > Add hwspinlock support for the SUNXI Hardware Spinlock device. > > The Hardware Spinlock device on SUNXI provides hardware assistance > for synchronization between the multiple processors in the system > (Cortex-A7, or1k, Xtensa DSP, Cortex-A53) > > Signed-off-by: fuyao <fuyao@allwinnertech.com> > --- > MAINTAINERS | 6 + > drivers/hwspinlock/Kconfig | 10 ++ > drivers/hwspinlock/Makefile | 1 + > drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++ > 4 files changed, 222 insertions(+) > create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index e451dcce054f0..68d25574432d0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -737,6 +737,12 @@ L: linux-media@vger.kernel.org > S: Maintained > F: drivers/staging/media/sunxi/cedrus/ > > +ALLWINNER HWSPINLOCK DRIVER > +M: fuyao <fuyao@allwinnertech.com> > +S: Maintained > +F: drivers/hwspinlock/sunxi_hwspinlock.c > +F: Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml > + > ALPHA PORT > M: Richard Henderson <rth@twiddle.net> > M: Ivan Kokshaysky <ink@jurassic.park.msu.ru> > diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig > index 32cd26352f381..4d0d516dcb544 100644 > --- a/drivers/hwspinlock/Kconfig > +++ b/drivers/hwspinlock/Kconfig > @@ -55,6 +55,16 @@ config HWSPINLOCK_STM32 > > If unsure, say N. > > +config HWSPINLOCK_SUNXI > + tristate "SUNXI Hardware Spinlock device" > + depends on ARCH_SUNXI || COMPILE_TEST > + help > + Say y here to support the SUNXI Hardware Semaphore functionality, which > + provides a synchronisation mechanism for the various processor on the > + SoC. > + > + If unsure, say N. > + > config HSEM_U8500 > tristate "STE Hardware Semaphore functionality" > depends on ARCH_U8500 || COMPILE_TEST > diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile > index ed053e3f02be4..839a053205f73 100644 > --- a/drivers/hwspinlock/Makefile > +++ b/drivers/hwspinlock/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF) += sirf_hwspinlock.o > obj-$(CONFIG_HWSPINLOCK_SPRD) += sprd_hwspinlock.o > obj-$(CONFIG_HWSPINLOCK_STM32) += stm32_hwspinlock.o > obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o > +obj-$(CONFIG_HWSPINLOCK_SUNXI) += sunxi_hwspinlock.o > diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c > new file mode 100644 > index 0000000000000..2c3dc148c9b72 > --- /dev/null > +++ b/drivers/hwspinlock/sunxi_hwspinlock.c > @@ -0,0 +1,205 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SUNXI hardware spinlock driver > + * > + * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com > + * Please remove the remainder of this comment, it's already covered by the SPDX header above. > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/device.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/bitops.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/hwspinlock.h> > +#include <linux/clk.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/err.h> > +#include <linux/reset.h> You don't need all of these. > + > +#include "hwspinlock_internal.h" > + > +/* hardware spinlock register list */ > +#define LOCK_SYS_STATUS_REG (0x0000) > +#define LOCK_STATUS_REG (0x0010) > +#define LOCK_BASE_OFFSET (0x0100) > +#define LOCK_BASE_ID (0) No need for the parenthesis on these, please drop them. > + > +/* Possible values of SPINLOCK_LOCK_REG */ > +#define SPINLOCK_NOTTAKEN (0) /* free */ > +#define SPINLOCK_TAKEN (1) /* locked */ > + > +struct sunxi_spinlock_config { > + int nspin; > +}; > + > +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock) > +{ > + void __iomem *lock_addr = lock->priv; > + > + /* attempt to acquire the lock by reading its value */ > + return (readl(lock_addr) == SPINLOCK_NOTTAKEN); Please drop the outer (). > +} > + > +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock) > +{ > + void __iomem *lock_addr = lock->priv; > + > + /* release the lock by writing 0 to it */ > + writel(SPINLOCK_NOTTAKEN, lock_addr); > +} > + > +/* > + * relax the SUNXI interconnect while spinning on it. > + * > + * The specs recommended that the retry delay time will be > + * just over half of the time that a requester would be > + * expected to hold the lock. > + * > + * in sunxi spinlock time less then 200 cycles > + * > + * The number below is taken from an hardware specs example, > + * obviously it is somewhat arbitrary. Thank you for the good explanation. > + */ > +static void sunxi_hwspinlock_relax(struct hwspinlock *lock) > +{ > + ndelay(50); > +} > + > +static const struct hwspinlock_ops sunxi_hwspinlock_ops = { > + .trylock = sunxi_hwspinlock_trylock, > + .unlock = sunxi_hwspinlock_unlock, > + .relax = sunxi_hwspinlock_relax, > +}; > + > +struct sunxi_hwspinlock_device { > + struct hwspinlock_device *bank; > + struct clk *bus_clk; > + struct reset_control *reset; > +}; > + > +static void sunxi_hwspinlock_clk_init(struct platform_device *pdev, > + struct sunxi_hwspinlock_device *private) > +{ > + private->bus_clk = devm_clk_get(&pdev->dev, NULL); > + private->reset = devm_reset_control_get(&pdev->dev, NULL); You should check the return value of these, e.g. for EPROBE_DEFER and if so return appropriately from sunxi_hwspinlock_probe(). So please move them to the probe function to make this easier and cleaner. > + > + if (private->reset) > + reset_control_deassert(private->reset); > + if (private->bus_clk) > + clk_prepare_enable(private->bus_clk); Both of these apis start with if (!argument) return; So there's no need for you to check for NULL before calling them. Also to make it clear that you want these to be deassered and prepare_enabled between probe and remvoe, move them into probe (and next function into remove). > +} > + > +static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private) > +{ > + if (private->reset) > + reset_control_assert(private->reset); > + if (private->bus_clk) > + clk_disable(private->bus_clk); > +} > + > +static const struct sunxi_spinlock_config spin_ver_1 = { > + .nspin = 32, > +}; > + > +static const struct of_device_id sunxi_hwspinlock_of_match[] = { > + { > + .compatible = "allwinner,h3-hwspinlock", > + .data = &spin_ver_1, If all cases comes with the same "data", then please just put nspin in a #define until you're going to support hardware that has some other number of locks. > + }, > + { > + .compatible = "allwinner,h6-hwspinlock", > + .data = &spin_ver_1, > + }, > + { /* Sentinel */ }, No need to spell out "/* Sentinel */", leave it emtpy and please drop the , at the end. > +}; > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match); Please move this down by sunxi_hwspinlock_driver and use device_get_match_data() instead of of_match_device(). > + > +static int sunxi_hwspinlock_probe(struct platform_device *pdev) > +{ > + struct sunxi_hwspinlock_device *private; > + struct hwspinlock_device *bank; > + struct hwspinlock *hwlock; > + const struct sunxi_spinlock_config *config; > + const struct of_device_id *match; > + void __iomem *iobase; > + int num_locks, i, ret; > + > + iobase = devm_platform_ioremap_resource(pdev, 0); > + if (PTR_ERR(iobase)) > + return PTR_ERR(iobase); > + > + match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match), > + &pdev->dev); > + if (!match) > + return -ENODEV; > + > + config = match->data; > + num_locks = config->nspin; > + > + private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL); > + if (!private) > + return -ENOMEM; > + > + bank = devm_kzalloc(&pdev->dev, > + sizeof(*bank) + num_locks * sizeof(*hwlock), > + GFP_KERNEL); > + if (!bank) > + return -ENOMEM; > + > + private->bank = bank; > + sunxi_hwspinlock_clk_init(pdev, private); > + > + platform_set_drvdata(pdev, private); > + > + for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++) > + hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i; > + > + ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops, > + LOCK_BASE_ID, num_locks); This returns 0 or -errno, so rather than returning ret if ret otherwise 0, just do: return devm_hwspin_lock_register(...) > + if (ret) > + return ret; > + > + > + return 0; > +} > + > +static int sunxi_hwspinlock_remove(struct platform_device *pdev) > +{ > + struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev); > + > + sunxi_hwspinlock_clk_dinit(private); > + > + return 0; > +} > + > +static struct platform_driver sunxi_hwspinlock_driver = { > + .probe = sunxi_hwspinlock_probe, > + .remove = sunxi_hwspinlock_remove, > + .driver = { > + .name = "sunxi-hwspinlock", > + .owner = THIS_MODULE, module_platform_driver() fills out .owner for you, so please remove this. > + .of_match_table = of_match_ptr(sunxi_hwspinlock_of_match), Please skip of_match_ptr(), it will just cause build warnings when compile tested without CONFIG_OF. Thank you, Bjorn > + }, > +}; > + > +module_platform_driver(sunxi_hwspinlock_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI"); > +MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>"); > -- > 2.29.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hwspinlock: add SUNXI implementation 2020-11-21 4:01 ` Bjorn Andersson @ 2020-11-21 14:33 ` fuyao 0 siblings, 0 replies; 7+ messages in thread From: fuyao @ 2020-11-21 14:33 UTC (permalink / raw) To: Bjorn Andersson; +Cc: mripard, wens, linux-remoteproc, linux-kernel On Fri, Nov 20, 2020 at 10:01:04PM -0600, Bjorn Andersson wrote: > On Thu 19 Nov 00:44 CST 2020, fuyao@allwinnertech.com wrote: > > > From: fuyao <fuyao@allwinnertech.com> > > > > Add hwspinlock support for the SUNXI Hardware Spinlock device. > > > > The Hardware Spinlock device on SUNXI provides hardware assistance > > for synchronization between the multiple processors in the system > > (Cortex-A7, or1k, Xtensa DSP, Cortex-A53) > > > > Signed-off-by: fuyao <fuyao@allwinnertech.com> > > --- > > MAINTAINERS | 6 + > > drivers/hwspinlock/Kconfig | 10 ++ > > drivers/hwspinlock/Makefile | 1 + > > drivers/hwspinlock/sunxi_hwspinlock.c | 205 ++++++++++++++++++++++++++ > > 4 files changed, 222 insertions(+) > > create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e451dcce054f0..68d25574432d0 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -737,6 +737,12 @@ L: linux-media@vger.kernel.org > > S: Maintained > > F: drivers/staging/media/sunxi/cedrus/ > > > > +ALLWINNER HWSPINLOCK DRIVER > > +M: fuyao <fuyao@allwinnertech.com> > > +S: Maintained > > +F: drivers/hwspinlock/sunxi_hwspinlock.c > > +F: Documentation/devicetree/bindings/hwlock/sunxi,hwspinlock.yaml > > + > > ALPHA PORT > > M: Richard Henderson <rth@twiddle.net> > > M: Ivan Kokshaysky <ink@jurassic.park.msu.ru> > > diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig > > index 32cd26352f381..4d0d516dcb544 100644 > > --- a/drivers/hwspinlock/Kconfig > > +++ b/drivers/hwspinlock/Kconfig > > @@ -55,6 +55,16 @@ config HWSPINLOCK_STM32 > > > > If unsure, say N. > > > > +config HWSPINLOCK_SUNXI > > + tristate "SUNXI Hardware Spinlock device" > > + depends on ARCH_SUNXI || COMPILE_TEST > > + help > > + Say y here to support the SUNXI Hardware Semaphore functionality, which > > + provides a synchronisation mechanism for the various processor on the > > + SoC. > > + > > + If unsure, say N. > > + > > config HSEM_U8500 > > tristate "STE Hardware Semaphore functionality" > > depends on ARCH_U8500 || COMPILE_TEST > > diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile > > index ed053e3f02be4..839a053205f73 100644 > > --- a/drivers/hwspinlock/Makefile > > +++ b/drivers/hwspinlock/Makefile > > @@ -10,3 +10,4 @@ obj-$(CONFIG_HWSPINLOCK_SIRF) += sirf_hwspinlock.o > > obj-$(CONFIG_HWSPINLOCK_SPRD) += sprd_hwspinlock.o > > obj-$(CONFIG_HWSPINLOCK_STM32) += stm32_hwspinlock.o > > obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o > > +obj-$(CONFIG_HWSPINLOCK_SUNXI) += sunxi_hwspinlock.o > > diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c > > new file mode 100644 > > index 0000000000000..2c3dc148c9b72 > > --- /dev/null > > +++ b/drivers/hwspinlock/sunxi_hwspinlock.c > > @@ -0,0 +1,205 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * SUNXI hardware spinlock driver > > + * > > + * Copyright (C) 2020 Allwinnertech - http://www.allwinnertech.com > > + * > > Please remove the remainder of this comment, it's already covered by the > SPDX header above. > > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * General Public License for more details. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > +#include <linux/module.h> > > +#include <linux/device.h> > > +#include <linux/delay.h> > > +#include <linux/io.h> > > +#include <linux/bitops.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > +#include <linux/hwspinlock.h> > > +#include <linux/clk.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_device.h> > > +#include <linux/err.h> > > +#include <linux/reset.h> > > You don't need all of these. > > > + > > +#include "hwspinlock_internal.h" > > + > > +/* hardware spinlock register list */ > > +#define LOCK_SYS_STATUS_REG (0x0000) > > +#define LOCK_STATUS_REG (0x0010) > > +#define LOCK_BASE_OFFSET (0x0100) > > +#define LOCK_BASE_ID (0) > > No need for the parenthesis on these, please drop them. > > > + > > +/* Possible values of SPINLOCK_LOCK_REG */ > > +#define SPINLOCK_NOTTAKEN (0) /* free */ > > +#define SPINLOCK_TAKEN (1) /* locked */ > > + > > +struct sunxi_spinlock_config { > > + int nspin; > > +}; > > + > > +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock) > > +{ > > + void __iomem *lock_addr = lock->priv; > > + > > + /* attempt to acquire the lock by reading its value */ > > + return (readl(lock_addr) == SPINLOCK_NOTTAKEN); > > Please drop the outer (). > > > +} > > + > > +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock) > > +{ > > + void __iomem *lock_addr = lock->priv; > > + > > + /* release the lock by writing 0 to it */ > > + writel(SPINLOCK_NOTTAKEN, lock_addr); > > +} > > + > > +/* > > + * relax the SUNXI interconnect while spinning on it. > > + * > > + * The specs recommended that the retry delay time will be > > + * just over half of the time that a requester would be > > + * expected to hold the lock. > > + * > > + * in sunxi spinlock time less then 200 cycles > > + * > > + * The number below is taken from an hardware specs example, > > + * obviously it is somewhat arbitrary. > > Thank you for the good explanation. > > > + */ > > +static void sunxi_hwspinlock_relax(struct hwspinlock *lock) > > +{ > > + ndelay(50); > > +} > > + > > +static const struct hwspinlock_ops sunxi_hwspinlock_ops = { > > + .trylock = sunxi_hwspinlock_trylock, > > + .unlock = sunxi_hwspinlock_unlock, > > + .relax = sunxi_hwspinlock_relax, > > +}; > > + > > +struct sunxi_hwspinlock_device { > > + struct hwspinlock_device *bank; > > + struct clk *bus_clk; > > + struct reset_control *reset; > > +}; > > + > > +static void sunxi_hwspinlock_clk_init(struct platform_device *pdev, > > + struct sunxi_hwspinlock_device *private) > > +{ > > + private->bus_clk = devm_clk_get(&pdev->dev, NULL); > > + private->reset = devm_reset_control_get(&pdev->dev, NULL); > > You should check the return value of these, e.g. for EPROBE_DEFER and if > so return appropriately from sunxi_hwspinlock_probe(). > > So please move them to the probe function to make this easier and > cleaner. > > > + > > + if (private->reset) > > + reset_control_deassert(private->reset); > > + if (private->bus_clk) > > + clk_prepare_enable(private->bus_clk); > > Both of these apis start with > > if (!argument) > return; > > So there's no need for you to check for NULL before calling them. Also > to make it clear that you want these to be deassered and prepare_enabled > between probe and remvoe, move them into probe (and next function into > remove). > > > +} > > + > > +static void sunxi_hwspinlock_clk_dinit(struct sunxi_hwspinlock_device *private) > > +{ > > + if (private->reset) > > + reset_control_assert(private->reset); > > + if (private->bus_clk) > > + clk_disable(private->bus_clk); > > +} > > + > > +static const struct sunxi_spinlock_config spin_ver_1 = { > > + .nspin = 32, > > +}; > > + > > +static const struct of_device_id sunxi_hwspinlock_of_match[] = { > > + { > > + .compatible = "allwinner,h3-hwspinlock", > > + .data = &spin_ver_1, > > If all cases comes with the same "data", then please just put nspin in a > #define until you're going to support hardware that has some other > number of locks. > > > + }, > > + { > > + .compatible = "allwinner,h6-hwspinlock", > > + .data = &spin_ver_1, > > + }, > > + { /* Sentinel */ }, > > No need to spell out "/* Sentinel */", leave it emtpy and please drop the , at > the end. > > > +}; > > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_of_match); > > Please move this down by sunxi_hwspinlock_driver and use > device_get_match_data() instead of of_match_device(). > > > + > > +static int sunxi_hwspinlock_probe(struct platform_device *pdev) > > +{ > > + struct sunxi_hwspinlock_device *private; > > + struct hwspinlock_device *bank; > > + struct hwspinlock *hwlock; > > + const struct sunxi_spinlock_config *config; > > + const struct of_device_id *match; > > + void __iomem *iobase; > > + int num_locks, i, ret; > > + > > + iobase = devm_platform_ioremap_resource(pdev, 0); > > + if (PTR_ERR(iobase)) > > + return PTR_ERR(iobase); > > + > > + match = of_match_device(of_match_ptr(sunxi_hwspinlock_of_match), > > + &pdev->dev); > > + if (!match) > > + return -ENODEV; > > + > > + config = match->data; > > + num_locks = config->nspin; > > + > > + private = devm_kzalloc(&pdev->dev, sizeof(*private), GFP_KERNEL); > > + if (!private) > > + return -ENOMEM; > > + > > + bank = devm_kzalloc(&pdev->dev, > > + sizeof(*bank) + num_locks * sizeof(*hwlock), > > + GFP_KERNEL); > > + if (!bank) > > + return -ENOMEM; > > + > > + private->bank = bank; > > + sunxi_hwspinlock_clk_init(pdev, private); > > + > > + platform_set_drvdata(pdev, private); > > + > > + for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++) > > + hwlock->priv = iobase + LOCK_BASE_OFFSET + sizeof(u32) * i; > > + > > + ret = devm_hwspin_lock_register(&pdev->dev, bank, &sunxi_hwspinlock_ops, > > + LOCK_BASE_ID, num_locks); > > This returns 0 or -errno, so rather than returning ret if ret otherwise > 0, just do: > > return devm_hwspin_lock_register(...) > > > + if (ret) > > + return ret; > > + > > + > > + return 0; > > +} > > + > > +static int sunxi_hwspinlock_remove(struct platform_device *pdev) > > +{ > > + struct sunxi_hwspinlock_device *private = platform_get_drvdata(pdev); > > + > > + sunxi_hwspinlock_clk_dinit(private); > > + > > + return 0; > > +} > > + > > +static struct platform_driver sunxi_hwspinlock_driver = { > > + .probe = sunxi_hwspinlock_probe, > > + .remove = sunxi_hwspinlock_remove, > > + .driver = { > > + .name = "sunxi-hwspinlock", > > + .owner = THIS_MODULE, > > module_platform_driver() fills out .owner for you, so please remove > this. > > > + .of_match_table = of_match_ptr(sunxi_hwspinlock_of_match), > > Please skip of_match_ptr(), it will just cause build warnings when > compile tested without CONFIG_OF. > > Thank you, > Bjorn > > > + }, > > +}; > > + > > +module_platform_driver(sunxi_hwspinlock_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Hardware spinlock driver for SUNXI"); > > +MODULE_AUTHOR("fuyao <fuyao@allwinnertech.com>"); Thanks for you review, I read it carefully, and learned a lot. Maxim tells that there is already the same submission, so this submission will be abandoned. thanks again. -- fuyao ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] introduce sunxi hwspinlock 2020-11-19 6:44 [PATCH 0/2] introduce sunxi hwspinlock fuyao 2020-11-19 6:44 ` [PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock fuyao 2020-11-19 6:44 ` [PATCH 2/2] hwspinlock: add SUNXI implementation fuyao @ 2020-11-20 16:07 ` Maxime Ripard 2020-11-21 14:08 ` fuyao 2 siblings, 1 reply; 7+ messages in thread From: Maxime Ripard @ 2020-11-20 16:07 UTC (permalink / raw) To: fuyao; +Cc: wens, linux-remoteproc, linux-kernel [-- Attachment #1: Type: text/plain, Size: 684 bytes --] Hi! On Thu, Nov 19, 2020 at 02:44:51PM +0800, fuyao@allwinnertech.com wrote: > From: fuyao <fuyao@allwinnertech.com> > > this series add hwspinlock of sunxi. it provides hardware assistance for > synchronization between the multiple processors in the system. > (Or1k, Cortex-A7, Cortex-A53, Xtensa) Xtensa? Which SoC has an Xtensa core? Unfortunately, there's been a submission of the same driver earlier this week: https://lore.kernel.org/lkml/cover.1605693132.git.wilken.gottwalt@posteo.net/ It would be great if you could point out whatever issue there is with that patch series (it looks like the retry delay could be useful for example). Thanks! Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] introduce sunxi hwspinlock 2020-11-20 16:07 ` [PATCH 0/2] introduce sunxi hwspinlock Maxime Ripard @ 2020-11-21 14:08 ` fuyao 0 siblings, 0 replies; 7+ messages in thread From: fuyao @ 2020-11-21 14:08 UTC (permalink / raw) To: Maxime Ripard; +Cc: wens, linux-remoteproc, linux-kernel On Fri, Nov 20, 2020 at 05:07:10PM +0100, Maxime Ripard wrote: > Hi! > > On Thu, Nov 19, 2020 at 02:44:51PM +0800, fuyao@allwinnertech.com wrote: > > From: fuyao <fuyao@allwinnertech.com> > > > > this series add hwspinlock of sunxi. it provides hardware assistance for > > synchronization between the multiple processors in the system. > > (Or1k, Cortex-A7, Cortex-A53, Xtensa) > > Xtensa? Which SoC has an Xtensa core? The new Soc named R329 use Xtensa core as the arisc role and audio. And the hwspinlock is the same as h3 and h6. Additional, The new RISC-V Soc also use the same hwspinlock. > > Unfortunately, there's been a submission of the same driver earlier this week: > https://lore.kernel.org/lkml/cover.1605693132.git.wilken.gottwalt@posteo.net/ > > It would be great if you could point out whatever issue there is with > that patch series (it looks like the retry delay could be useful for > example). I will test that submission with arisc next week. BTW, which sunxi board you used, I want to used the same board with you. ^-^ thanks -- fuyao ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-21 14:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-19 6:44 [PATCH 0/2] introduce sunxi hwspinlock fuyao 2020-11-19 6:44 ` [PATCH 1/2] dt-bindings: hwlock: add sunxi hwlock fuyao 2020-11-19 6:44 ` [PATCH 2/2] hwspinlock: add SUNXI implementation fuyao 2020-11-21 4:01 ` Bjorn Andersson 2020-11-21 14:33 ` fuyao 2020-11-20 16:07 ` [PATCH 0/2] introduce sunxi hwspinlock Maxime Ripard 2020-11-21 14:08 ` fuyao
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).