[PATCH 0/3] Add support for hwspinlock on A64 SoC

[PATCH 0/3] Add support for hwspinlock on A64 SoC

[Nikolay Borisov]

Here are a couple of patches that add support for hwspinlock ip block in the A64
SoC. I tested the driver on a pine64-lts and it seems to work - clock gating is
disabled and soft reset is de-asserted.

Nikolay Borisov (3):
  hwspinlock: sunxi: Implement support for Allwinner's A64 SoC
  arm64: dts: allwinner: a64: Add hwspinlock node
  dt-bindings: hwlock: Document A64 hwspinlock bindings

 .../bindings/hwlock/sunxi-hwspinlock.txt      |  27 +++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |   9 +
 drivers/hwspinlock/Kconfig                    |   9 +
 drivers/hwspinlock/Makefile                   |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c         | 181 ++++++++++++++++++
 5 files changed, 227 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
 create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c

--
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov]

Based on the datasheet this implements support for the hwspinlock IP
block.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 drivers/hwspinlock/Kconfig            |   9 ++
 drivers/hwspinlock/Makefile           |   1 +
 drivers/hwspinlock/sunxi_hwspinlock.c | 181 ++++++++++++++++++++++++++
 3 files changed, 191 insertions(+)
 create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c

diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 37740e992cfa..ebc1ea48ef16 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -68,3 +68,12 @@ config HSEM_U8500
 	  SoC.
 
 	  If unsure, say N.
+
+config HWSPINLOCK_SUNXI
+	tristate "Allwinner Hardware Spinlock device"
+	depends on ARCH_SUNXI
+	depends on HWSPINLOCK
+	help
+	  Say y here to support the SUNXI Hardware Spinlock device.
+
+	  If unsure, say N.
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index ed053e3f02be..6be5ebef906e 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
+obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o
 obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
 obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
new file mode 100644
index 000000000000..8e5685357fbf
--- /dev/null
+++ b/drivers/hwspinlock/sunxi_hwspinlock.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Nikolay Borisov <nborisov@suse.com> */
+
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/hwspinlock.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "hwspinlock_internal.h"
+
+/* Spinlock register offsets */
+#define LOCK_BASE_OFFSET		0x0100
+
+#define SPINLOCK_NUMLOCKS_BIT_OFFSET	(28)
+/* Possible values of SPINLOCK_LOCK_REG */
+#define SPINLOCK_NOTTAKEN		(0)	/* free */
+#define SPINLOCK_TAKEN			(1)	/* locked */
+
+struct sunxi_hwspinlock {
+	struct clk *clk;
+	struct reset_control *reset;
+	struct hwspinlock_device bank;
+};
+
+static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
+{
+	void __iomem *lock_addr = lock->priv;
+
+	/* attempt to acquire the lock by reading its value */
+	return (SPINLOCK_NOTTAKEN == readl(lock_addr));
+}
+
+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);
+}
+
+static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
+	.trylock	= sunxi_hwspinlock_trylock,
+	.unlock		= sunxi_hwspinlock_unlock,
+};
+
+static int sunxi_get_num_locks(void __iomem *io_base)
+{
+	int i = readl(io_base);
+	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
+
+	switch (i) {
+	case 0x1: return 32;
+	case 0x2: return 64;
+	case 0x3: return 128;
+	case 0x4: return 256;
+	}
+
+	return 0;
+}
+
+static int sunxi_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct sunxi_hwspinlock *hw;
+	void __iomem *io_base;
+	struct resource *res;
+	struct clk *clk;
+	struct reset_control *reset;
+	int i, ret, num_locks;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	io_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	/*
+	 * make sure the module is enabled and clocked before reading
+	 * the module SYSSTATUS register
+	 */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot enable clock\n");
+		return ret;
+	}
+
+	/* Disable soft reset */
+        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
+        if (IS_ERR(reset)) {
+                ret = PTR_ERR(reset);
+                goto out_declock;
+        }
+        reset_control_deassert(reset);
+
+	num_locks = sunxi_get_num_locks(io_base);
+	if (!num_locks) {
+		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
+		ret = -EINVAL;
+		goto out_reset;
+	}
+
+	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
+			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);
+	if (!hw) {
+		ret = -ENOMEM;
+		goto out_reset;
+	}
+
+	hw->clk = clk;
+	hw->reset = reset;
+
+	io_base += LOCK_BASE_OFFSET;
+	for (i = 0; i < num_locks; i++)
+		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
+
+	platform_set_drvdata(pdev, hw);
+
+	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
+				   0, num_locks);
+
+	if (!ret)
+		return ret;
+out_reset:
+	reset_control_assert(reset);
+out_declock:
+	clk_disable_unprepare(clk);
+	return ret;
+}
+
+static int sunxi_hwspinlock_remove(struct platform_device *pdev)
+{
+	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = hwspin_lock_unregister(&hw->bank);
+	if (ret)
+		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
+
+	reset_control_assert(hw->reset);
+	clk_disable_unprepare(hw->clk);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_hwpinlock_ids[] = {
+	{ .compatible = "allwinner,sun50i-a64-hwspinlock", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sunxi_hwpinlock_ids);
+
+static struct platform_driver sunxi_hwspinlock_driver = {
+	.probe		= sunxi_hwspinlock_probe,
+	.remove		= sunxi_hwspinlock_remove,
+	.driver		= {
+		.name	= "sunxi_hwspinlock",
+		.of_match_table = sunxi_hwpinlock_ids,
+	},
+};
+
+static int __init sunxi_hwspinlock_init(void)
+{
+	return platform_driver_register(&sunxi_hwspinlock_driver);
+}
+/* board init code might need to reserve hwspinlocks for predefined purposes */
+postcore_initcall(sunxi_hwspinlock_init);
+
+static void __exit sunxi_hwspinlock_exit(void)
+{
+	platform_driver_unregister(&sunxi_hwspinlock_driver);
+}
+module_exit(sunxi_hwspinlock_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver for sunxi SoCs");
+MODULE_AUTHOR("Nikolay Borisov <nborisov@suse.com>");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Nikolay Borisov]

Add a node describing the hwspinlock on A64.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 27e48234f1c2..01de89fc09cc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -1182,5 +1182,14 @@
 			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&osc24M>;
 		};
+
+		hwspinlock: spinlock@1c18000 {
+			compatible = "allwinner,sun50i-a64-hwspinlock";
+			reg = <0x01c18000 0x1000>;
+			clocks = <&ccu CLK_BUS_SPINLOCK>;
+			resets = <&ccu RST_BUS_SPINLOCK>;
+			#hwlock-cells = <1>;
+			status = "disabled";
+		};
 	};
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] dt-bindings: hwlock: Document A64 hwspinlock bindings

[Nikolay Borisov]

Add binding for the hwspinlock found on Allwinner A64 SoC.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 .../bindings/hwlock/sunxi-hwspinlock.txt      | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt

diff --git a/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
new file mode 100644
index 000000000000..af1fbf8fc9f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
@@ -0,0 +1,27 @@
+Allwinner HwSpinlock for A64 SoC
+=========================================
+
+Required properties:
+- compatible:		Should be "allwinner,sun50i-a64-hwspinlock"
+- reg:			Contains the hwspinlock module register address space
+			(base address and length)
+- resets:  phandle to the reset control for the hwspinlock
+- clocks:  phandle to the clock feeding the hwspinlock
+- #hwlock-cells:	Should be 1. The A64 hwspinlock users will use a
+			0-indexed relative hwlock number as the argument
+			specifier value for requesting a specific hwspinlock
+			within a hwspinlock bank.
+
+Please look at the generic hwlock binding for usage information for consumers,
+"Documentation/devicetree/bindings/hwlock/hwlock.txt"
+
+Example:
+
+1. Allwinner A64
+hwspinlock: spinlock@1c18000 {
+	compatible = "allwinner,sun50i-a64-hwspinlock";
+	reg = <0x01c18000 0x1000>;
+	clocks = <&ccu CLK_BUS_SPINLOCK>;
+	resets = <&ccu RST_BUS_SPINLOCK>;
+	#hwlock-cells = <1>;
+};
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Bjorn Andersson]

On Mon 10 Feb 09:01 PST 2020, Nikolay Borisov wrote:
[..]
> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> new file mode 100644
> index 000000000000..8e5685357fbf
> --- /dev/null
> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Nikolay Borisov <nborisov@suse.com> */
> +
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>

Please sort these.

> +
> +#include "hwspinlock_internal.h"
> +
> +/* Spinlock register offsets */
> +#define LOCK_BASE_OFFSET		0x0100
> +
> +#define SPINLOCK_NUMLOCKS_BIT_OFFSET	(28)
> +/* Possible values of SPINLOCK_LOCK_REG */
> +#define SPINLOCK_NOTTAKEN		(0)	/* free */
> +#define SPINLOCK_TAKEN			(1)	/* locked */
> +
> +struct sunxi_hwspinlock {
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	struct hwspinlock_device bank;
> +};
> +
> +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* attempt to acquire the lock by reading its value */
> +	return (SPINLOCK_NOTTAKEN == readl(lock_addr));

Please drop the parenthesis and flip the expression around, i.e.
variable == constant.

> +}
> +
> +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);
> +}
> +
> +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> +	.trylock	= sunxi_hwspinlock_trylock,
> +	.unlock		= sunxi_hwspinlock_unlock,
> +};
> +
> +static int sunxi_get_num_locks(void __iomem *io_base)
> +{
> +	int i = readl(io_base);
> +	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;

Please make i u32.

> +
> +	switch (i) {
> +	case 0x1: return 32;
> +	case 0x2: return 64;
> +	case 0x3: return 128;
> +	case 0x4: return 256;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw;
> +	void __iomem *io_base;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	int i, ret, num_locks;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	io_base = devm_ioremap_resource(&pdev->dev, res);

Please use devm_platform_ioremap_resource()

> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	/*
> +	 * make sure the module is enabled and clocked before reading
> +	 * the module SYSSTATUS register
> +	 */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot enable clock\n");
> +		return ret;
> +	}
> +
> +	/* Disable soft reset */
> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +        if (IS_ERR(reset)) {
> +                ret = PTR_ERR(reset);
> +                goto out_declock;
> +        }
> +        reset_control_deassert(reset);

Indentation of this chunk looks off.

> +
> +	num_locks = sunxi_get_num_locks(io_base);
> +	if (!num_locks) {
> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
> +		ret = -EINVAL;
> +		goto out_reset;
> +	}
> +
> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);

Please use struct_size() to calculate the size here.

> +	if (!hw) {
> +		ret = -ENOMEM;
> +		goto out_reset;
> +	}
> +
> +	hw->clk = clk;
> +	hw->reset = reset;
> +
> +	io_base += LOCK_BASE_OFFSET;
> +	for (i = 0; i < num_locks; i++)
> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> +
> +	platform_set_drvdata(pdev, hw);
> +
> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> +				   0, num_locks);

People will likely send patches to replace this with
devm_hwspin_lock_register(), but that will create an invalid ordering
between the clock disable, reset assert and the hwspinlock
unregistration.

You could deal with this using devm_add_action() and
devm_add_action_or_reset() for the clock and reset above. That will save
us future churn, would clean up your error handling and you could drop
the remove function completely.

> +
> +	if (!ret)
> +		return ret;
> +out_reset:
> +	reset_control_assert(reset);
> +out_declock:
> +	clk_disable_unprepare(clk);
> +	return ret;
> +}
> +
> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = hwspin_lock_unregister(&hw->bank);
> +	if (ret)
> +		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
> +
> +	reset_control_assert(hw->reset);
> +	clk_disable_unprepare(hw->clk);
> +
> +	return 0;
> +}
> +

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] dt-bindings: hwlock: Document A64 hwspinlock bindings

[Bjorn Andersson]

On Mon 10 Feb 09:01 PST 2020, Nikolay Borisov wrote:

> Add binding for the hwspinlock found on Allwinner A64 SoC.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

This does look good, but could you please rewrite this in YAML instead?

We will need review feedback from the devicetree guys, so please include
Rob and devicetree@ on your next post (i.e. run get_maintainers.pl).

Regards,
jorn

> ---
>  .../bindings/hwlock/sunxi-hwspinlock.txt      | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
> new file mode 100644
> index 000000000000..af1fbf8fc9f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/sunxi-hwspinlock.txt
> @@ -0,0 +1,27 @@
> +Allwinner HwSpinlock for A64 SoC
> +=========================================
> +
> +Required properties:
> +- compatible:		Should be "allwinner,sun50i-a64-hwspinlock"
> +- reg:			Contains the hwspinlock module register address space
> +			(base address and length)
> +- resets:  phandle to the reset control for the hwspinlock
> +- clocks:  phandle to the clock feeding the hwspinlock
> +- #hwlock-cells:	Should be 1. The A64 hwspinlock users will use a
> +			0-indexed relative hwlock number as the argument
> +			specifier value for requesting a specific hwspinlock
> +			within a hwspinlock bank.
> +
> +Please look at the generic hwlock binding for usage information for consumers,
> +"Documentation/devicetree/bindings/hwlock/hwlock.txt"
> +
> +Example:
> +
> +1. Allwinner A64
> +hwspinlock: spinlock@1c18000 {
> +	compatible = "allwinner,sun50i-a64-hwspinlock";
> +	reg = <0x01c18000 0x1000>;
> +	clocks = <&ccu CLK_BUS_SPINLOCK>;
> +	resets = <&ccu RST_BUS_SPINLOCK>;
> +	#hwlock-cells = <1>;
> +};
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov]

On 10.02.20 г. 20:57 ч., Bjorn Andersson wrote:
> On Mon 10 Feb 09:01 PST 2020, Nikolay Borisov wrote:
> [..]
>> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
>> new file mode 100644
>> index 000000000000..8e5685357fbf
>> --- /dev/null
>> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Nikolay Borisov <nborisov@suse.com> */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/reset.h>
>> +#include <linux/hwspinlock.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
> 
> Please sort these.

alphabetically or reverse christmas tree?

> 
>> +

<snip>
>> +	hw->clk = clk;
>> +	hw->reset = reset;
>> +
>> +	io_base += LOCK_BASE_OFFSET;
>> +	for (i = 0; i < num_locks; i++)
>> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
>> +
>> +	platform_set_drvdata(pdev, hw);
>> +
>> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
>> +				   0, num_locks);
> 
> People will likely send patches to replace this with
> devm_hwspin_lock_register(), but that will create an invalid ordering
> between the clock disable, reset assert and the hwspinlock
> unregistration.
> 
> You could deal with this using devm_add_action() and
> devm_add_action_or_reset() for the clock and reset above. That will save
> us future churn, would clean up your error handling and you could drop
> the remove function completely.

This is my first rodeo in device driver land so I'm learning. It looks
like what you want here is similar to what there is in
sprd_hwspinlock.c. Will rework it.

> 
>> +
>> +	if (!ret)
>> +		return ret;
>> +out_reset:
>> +	reset_control_assert(reset);
>> +out_declock:
>> +	clk_disable_unprepare(clk);
>> +	return ret;
>> +}
>> +
>> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
>> +	int ret;
>> +
>> +	ret = hwspin_lock_unregister(&hw->bank);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
>> +
>> +	reset_control_assert(hw->reset);
>> +	clk_disable_unprepare(hw->clk);
>> +
>> +	return 0;
>> +}
>> +
> 
> Regards,
> Bjorn
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Bjorn Andersson]

On Mon 10 Feb 11:06 PST 2020, Nikolay Borisov wrote:

> 
> 
> On 10.02.20 ??. 20:57 ??., Bjorn Andersson wrote:
> > On Mon 10 Feb 09:01 PST 2020, Nikolay Borisov wrote:
> > [..]
> >> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> >> new file mode 100644
> >> index 000000000000..8e5685357fbf
> >> --- /dev/null
> >> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> >> @@ -0,0 +1,181 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Author: Nikolay Borisov <nborisov@suse.com> */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/reset.h>
> >> +#include <linux/hwspinlock.h>
> >> +#include <linux/io.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> > 
> > Please sort these.
> 
> alphabetically or reverse christmas tree?
> 

Alphabetically please.

> > 
> >> +
> 
> <snip>
> >> +	hw->clk = clk;
> >> +	hw->reset = reset;
> >> +
> >> +	io_base += LOCK_BASE_OFFSET;
> >> +	for (i = 0; i < num_locks; i++)
> >> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> >> +
> >> +	platform_set_drvdata(pdev, hw);
> >> +
> >> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> >> +				   0, num_locks);
> > 
> > People will likely send patches to replace this with
> > devm_hwspin_lock_register(), but that will create an invalid ordering
> > between the clock disable, reset assert and the hwspinlock
> > unregistration.
> > 
> > You could deal with this using devm_add_action() and
> > devm_add_action_or_reset() for the clock and reset above. That will save
> > us future churn, would clean up your error handling and you could drop
> > the remove function completely.
> 
> This is my first rodeo in device driver land so I'm learning. It looks
> like what you want here is similar to what there is in
> sprd_hwspinlock.c. Will rework it.
> 

Exactly like that, forgot that we had an example of this in the sprd
driver. That will ensure that we rely solely on devres to unroll the
resources in a suitable order.

Regards,
Bjorn

> > 
> >> +
> >> +	if (!ret)
> >> +		return ret;
> >> +out_reset:
> >> +	reset_control_assert(reset);
> >> +out_declock:
> >> +	clk_disable_unprepare(clk);
> >> +	return ret;
> >> +}
> >> +
> >> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> >> +{
> >> +	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
> >> +	int ret;
> >> +
> >> +	ret = hwspin_lock_unregister(&hw->bank);
> >> +	if (ret)
> >> +		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
> >> +
> >> +	reset_control_assert(hw->reset);
> >> +	clk_disable_unprepare(hw->clk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> > 
> > Regards,
> > Bjorn
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 7954 bytes --]

Hi,

On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
> Based on the datasheet this implements support for the hwspinlock IP
> block.

How was this tested?

There's also a lot of checkpatch issues, make sure you fix those.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  drivers/hwspinlock/Kconfig            |   9 ++
>  drivers/hwspinlock/Makefile           |   1 +
>  drivers/hwspinlock/sunxi_hwspinlock.c | 181 ++++++++++++++++++++++++++
>  3 files changed, 191 insertions(+)
>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
>
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 37740e992cfa..ebc1ea48ef16 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -68,3 +68,12 @@ config HSEM_U8500
>  	  SoC.
>
>  	  If unsure, say N.
> +
> +config HWSPINLOCK_SUNXI
> +	tristate "Allwinner Hardware Spinlock device"
> +	depends on ARCH_SUNXI
> +	depends on HWSPINLOCK
> +	help
> +	  Say y here to support the SUNXI Hardware Spinlock device.
> +
> +	  If unsure, say N.

sunxi doesn't really mean anything though, the A10 is also part of the
sunxi family and doesn't have that IP. Similarly, nothing prevents a
future SoC from changing that design. The first SoC that used it was
the A33 iirc, so let's just use sun8i.

> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index ed053e3f02be..6be5ebef906e 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o
> +obj-$(CONFIG_HWSPINLOCK_SUNXI)		+= sunxi_hwspinlock.o

Ditto for the name of the file (and basically every symbol in your
driver.

>  obj-$(CONFIG_HWSPINLOCK_STM32)		+= stm32_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> new file mode 100644
> index 000000000000..8e5685357fbf
> --- /dev/null
> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Nikolay Borisov <nborisov@suse.com> */
> +
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +/* Spinlock register offsets */
> +#define LOCK_BASE_OFFSET		0x0100
> +
> +#define SPINLOCK_NUMLOCKS_BIT_OFFSET	(28)
> +/* Possible values of SPINLOCK_LOCK_REG */
> +#define SPINLOCK_NOTTAKEN		(0)	/* free */
> +#define SPINLOCK_TAKEN			(1)	/* locked */
> +
> +struct sunxi_hwspinlock {
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	struct hwspinlock_device bank;
> +};
> +
> +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* attempt to acquire the lock by reading its value */
> +	return (SPINLOCK_NOTTAKEN == readl(lock_addr));
> +}
> +
> +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);
> +}
> +
> +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> +	.trylock	= sunxi_hwspinlock_trylock,
> +	.unlock		= sunxi_hwspinlock_unlock,
> +};
> +
> +static int sunxi_get_num_locks(void __iomem *io_base)
> +{
> +	int i = readl(io_base);

readl returns a u32.

> +	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;

And you probably want to mask the value to avoid other bitfields in
the register from messing with that value. FIELD_GET will help you to
do so.

> +	switch (i) {
> +	case 0x1: return 32;
> +	case 0x2: return 64;
> +	case 0x3: return 128;
> +	case 0x4: return 256;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw;
> +	void __iomem *io_base;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	int i, ret, num_locks;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	io_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	/*
> +	 * make sure the module is enabled and clocked before reading
> +	 * the module SYSSTATUS register
> +	 */

You don't define that register anywhere?

> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot enable clock\n");
> +		return ret;
> +	}

Can't we do that with runtime_pm?

> +	/* Disable soft reset */
> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +        if (IS_ERR(reset)) {
> +                ret = PTR_ERR(reset);
> +                goto out_declock;
> +        }
> +        reset_control_deassert(reset);

We might have the same issue than the mailbox driver where the
firmware will need to access the block at any time, so we can't really
toggle the reset line as we want.

> +	num_locks = sunxi_get_num_locks(io_base);
> +	if (!num_locks) {
> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
> +		ret = -EINVAL;
> +		goto out_reset;
> +	}
> +
> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);
> +	if (!hw) {
> +		ret = -ENOMEM;
> +		goto out_reset;
> +	}

That looks rather convoluted (especially since the variable length
array is at the second level), and can be made more obvious by:

- Removing the hwspinlock_device from sunxi_hwspinlock and allocating
  both separately.

- And then allocate the hwspinlock_device separately with struct_size

> +	hw->clk = clk;
> +	hw->reset = reset;

Why not using the structure directly instead of having temporary
variables?

> +	io_base += LOCK_BASE_OFFSET;
> +	for (i = 0; i < num_locks; i++)
> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);

Using a define for the registers offset would be nice here.

> +	platform_set_drvdata(pdev, hw);
> +
> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> +				   0, num_locks);
> +
> +	if (!ret)
> +		return ret;

That's a slightly weird construct too, since in pretty much all the
driver you return early on error and here you return early on
success. Just return ret if there's an error just like you're doing
everywhere else, and return 0 after that.

> +out_reset:
> +	reset_control_assert(reset);
> +out_declock:
> +	clk_disable_unprepare(clk);
> +	return ret;
> +}
> +
> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = hwspin_lock_unregister(&hw->bank);
> +	if (ret)
> +		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
> +
> +	reset_control_assert(hw->reset);
> +	clk_disable_unprepare(hw->clk);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_hwpinlock_ids[] = {
> +	{ .compatible = "allwinner,sun50i-a64-hwspinlock", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_hwpinlock_ids);
> +
> +static struct platform_driver sunxi_hwspinlock_driver = {
> +	.probe		= sunxi_hwspinlock_probe,
> +	.remove		= sunxi_hwspinlock_remove,
> +	.driver		= {
> +		.name	= "sunxi_hwspinlock",
> +		.of_match_table = sunxi_hwpinlock_ids,
> +	},
> +};
> +
> +static int __init sunxi_hwspinlock_init(void)
> +{
> +	return platform_driver_register(&sunxi_hwspinlock_driver);
> +}
> +/* board init code might need to reserve hwspinlocks for predefined purposes */
> +postcore_initcall(sunxi_hwspinlock_init);

We don't have any board init code. Can't we just use a regular
module_platform_driver call here?

Thanks!
Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]

Hi,

On Mon, Feb 10, 2020 at 07:01:42PM +0200, Nikolay Borisov wrote:
> Add a node describing the hwspinlock on A64.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 27e48234f1c2..01de89fc09cc 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -1182,5 +1182,14 @@
>  			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&osc24M>;
>  		};
> +
> +		hwspinlock: spinlock@1c18000 {

Nodes are supposed to be ordered by ascending physical address

> +			compatible = "allwinner,sun50i-a64-hwspinlock";
> +			reg = <0x01c18000 0x1000>;
> +			clocks = <&ccu CLK_BUS_SPINLOCK>;
> +			resets = <&ccu RST_BUS_SPINLOCK>;
> +			#hwlock-cells = <1>;
> +			status = "disabled";

Is there a reason to disable it?

Also, my understanding was that hwspinlocks were meant to be used by
client drivers, so surely we must tie them to other device nodes too,
right?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov]

On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
> Hi,
> 
> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
>> Based on the datasheet this implements support for the hwspinlock IP
>> block.
> 
> How was this tested?

I tested it on my pine64 lts e.g. loading the driver and reading the
reset/clock/sysstatus registers to ensure everything is unmasked and has
expected values.

> 
> There's also a lot of checkpatch issues, make sure you fix those.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  drivers/hwspinlock/Kconfig            |   9 ++
>>  drivers/hwspinlock/Makefile           |   1 +
>>  drivers/hwspinlock/sunxi_hwspinlock.c | 181 ++++++++++++++++++++++++++
>>  3 files changed, 191 insertions(+)
>>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
>>
>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>> index 37740e992cfa..ebc1ea48ef16 100644
>> --- a/drivers/hwspinlock/Kconfig
>> +++ b/drivers/hwspinlock/Kconfig
>> @@ -68,3 +68,12 @@ config HSEM_U8500
>>  	  SoC.
>>
>>  	  If unsure, say N.
>> +
>> +config HWSPINLOCK_SUNXI
>> +	tristate "Allwinner Hardware Spinlock device"
>> +	depends on ARCH_SUNXI
>> +	depends on HWSPINLOCK
>> +	help
>> +	  Say y here to support the SUNXI Hardware Spinlock device.
>> +
>> +	  If unsure, say N.
> 
> sunxi doesn't really mean anything though, the A10 is also part of the
> sunxi family and doesn't have that IP. Similarly, nothing prevents a
> future SoC from changing that design. The first SoC that used it was
> the A33 iirc, so let's just use sun8i.

Fair enough, I will use the same for the symbols as well. TBH the
nomenclature is quite confusing in allwinner land...

Actually since this driver will initially support A64 shouldn't the
symbols really be prefixed with sun50i, since looking at
https://linux-sunxi.org/Allwinner_SoC_Family sun8i pertains to 32 bit A7
cores?

> 
<snip>
>> +
>> +	/*
>> +	 * make sure the module is enabled and clocked before reading
>> +	 * the module SYSSTATUS register
>> +	 */
> 
> You don't define that register anywhere?
> 
>> +	clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Cannot enable clock\n");
>> +		return ret;
>> +	}
> 
> Can't we do that with runtime_pm?

Probably can but I'm new to device driver development so I don't
understand all the implications of using the pm framework. I saw that
other drivers did this but atm it's terra incognita to me.

> 
>> +	/* Disable soft reset */
>> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> +        if (IS_ERR(reset)) {
>> +                ret = PTR_ERR(reset);
>> +                goto out_declock;
>> +        }
>> +        reset_control_deassert(reset);
> 
> We might have the same issue than the mailbox driver where the
> firmware will need to access the block at any time, so we can't really
> toggle the reset line as we want.

What should we do then ?

> 
>> +	num_locks = sunxi_get_num_locks(io_base);
>> +	if (!num_locks) {
>> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
>> +		ret = -EINVAL;
>> +		goto out_reset;
>> +	}
>> +
>> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
>> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);
>> +	if (!hw) {
>> +		ret = -ENOMEM;
>> +		goto out_reset;
>> +	}
> 
> That looks rather convoluted (especially since the variable length
> array is at the second level), and can be made more obvious by:
> 
> - Removing the hwspinlock_device from sunxi_hwspinlock and allocating
>   both separately.
> 
> - And then allocate the hwspinlock_device separately with struct_size

Actually this can be allocated via struct_size e.g. struct_size(hw,
bank.lock, num_locks).

> 
>> +	hw->clk = clk;
>> +	hw->reset = reset;
> 
> Why not using the structure directly instead of having temporary
> variables?

Because I use the clk/reset before allocating the devmem.

> 
>> +	io_base += LOCK_BASE_OFFSET;
>> +	for (i = 0; i < num_locks; i++)
>> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> 
> Using a define for the registers offset would be nice here. 

you mean something like:

# define LOCK(X) (x * sizeof(u32))

I think this brings more needless indirection than helps but if you
insist I won't mind.

> 
>> +	platform_set_drvdata(pdev, hw);
>> +
>> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
>> +				   0, num_locks);
>> +
>> +	if (!ret)
>> +		return ret;
> 
> That's a slightly weird construct too, since in pretty much all the
> driver you return early on error and here you return early on
> success. Just return ret if there's an error just like you're doing
> everywhere else, and return 0 after that.
> 
>> +out_reset:
>> +	reset_control_assert(reset);
>> +out_declock:
>> +	clk_disable_unprepare(clk);
>> +	return ret;
>> +}
>> +

<snip>

>> +/* board init code might need to reserve hwspinlocks for predefined purposes */
>> +postcore_initcall(sunxi_hwspinlock_init);
> 
> We don't have any board init code. Can't we just use a regular
> module_platform_driver call here?

Perhaps we can, I will test on my pine64.
> 
> Thanks!
> Maxime
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Nikolay Borisov]

On 11.02.20 г. 9:55 ч., Maxime Ripard wrote:
> Hi,
> 
> On Mon, Feb 10, 2020 at 07:01:42PM +0200, Nikolay Borisov wrote:
>> Add a node describing the hwspinlock on A64.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> index 27e48234f1c2..01de89fc09cc 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -1182,5 +1182,14 @@
>>  			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>>  			clocks = <&osc24M>;
>>  		};
>> +
>> +		hwspinlock: spinlock@1c18000 {
> 
> Nodes are supposed to be ordered by ascending physical address
> 
>> +			compatible = "allwinner,sun50i-a64-hwspinlock";
>> +			reg = <0x01c18000 0x1000>;
>> +			clocks = <&ccu CLK_BUS_SPINLOCK>;
>> +			resets = <&ccu RST_BUS_SPINLOCK>;
>> +			#hwlock-cells = <1>;
>> +			status = "disabled";
> 
> Is there a reason to disable it?

I wondered about that - on the one hand we can safely leave it always
enabled, on the other hand all devices are are disabled in the dtsi.

> 
> Also, my understanding was that hwspinlocks were meant to be used by
> client drivers, so surely we must tie them to other device nodes too,
> right?

Perhaps but at this point I'm not sure to which device specifically.

> 
> Maxime
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5336 bytes --]

On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
> >> Based on the datasheet this implements support for the hwspinlock IP
> >> block.
> >
> > How was this tested?
>
> I tested it on my pine64 lts e.g. loading the driver and reading the
> reset/clock/sysstatus registers to ensure everything is unmasked and has
> expected values.

Isn't the point of hwspinlocks that it's shared between the ARISC core
and the ARM cores. How did you test that the lock was actually taken
on the other side just by using the ARM cores?

> >
> > There's also a lot of checkpatch issues, make sure you fix those.
> >
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>  drivers/hwspinlock/Kconfig            |   9 ++
> >>  drivers/hwspinlock/Makefile           |   1 +
> >>  drivers/hwspinlock/sunxi_hwspinlock.c | 181 ++++++++++++++++++++++++++
> >>  3 files changed, 191 insertions(+)
> >>  create mode 100644 drivers/hwspinlock/sunxi_hwspinlock.c
> >>
> >> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> >> index 37740e992cfa..ebc1ea48ef16 100644
> >> --- a/drivers/hwspinlock/Kconfig
> >> +++ b/drivers/hwspinlock/Kconfig
> >> @@ -68,3 +68,12 @@ config HSEM_U8500
> >>  	  SoC.
> >>
> >>  	  If unsure, say N.
> >> +
> >> +config HWSPINLOCK_SUNXI
> >> +	tristate "Allwinner Hardware Spinlock device"
> >> +	depends on ARCH_SUNXI
> >> +	depends on HWSPINLOCK
> >> +	help
> >> +	  Say y here to support the SUNXI Hardware Spinlock device.
> >> +
> >> +	  If unsure, say N.
> >
> > sunxi doesn't really mean anything though, the A10 is also part of the
> > sunxi family and doesn't have that IP. Similarly, nothing prevents a
> > future SoC from changing that design. The first SoC that used it was
> > the A33 iirc, so let's just use sun8i.
>
> Fair enough, I will use the same for the symbols as well. TBH the
> nomenclature is quite confusing in allwinner land...
>
> Actually since this driver will initially support A64 shouldn't the
> symbols really be prefixed with sun50i, since looking at
> https://linux-sunxi.org/Allwinner_SoC_Family sun8i pertains to 32 bit A7
> cores?

Not really, the design is the same and we don't want to rename
everything.

> >
> <snip>
> >> +
> >> +	/*
> >> +	 * make sure the module is enabled and clocked before reading
> >> +	 * the module SYSSTATUS register
> >> +	 */
> >
> > You don't define that register anywhere?
> >
> >> +	clk = devm_clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(clk))
> >> +		return PTR_ERR(clk);
> >> +
> >> +	ret = clk_prepare_enable(clk);
> >> +	if (ret) {
> >> +		dev_err(&pdev->dev, "Cannot enable clock\n");
> >> +		return ret;
> >> +	}
> >
> > Can't we do that with runtime_pm?
>
> Probably can but I'm new to device driver development so I don't
> understand all the implications of using the pm framework. I saw that
> other drivers did this but atm it's terra incognita to me.
>
> >
> >> +	/* Disable soft reset */
> >> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
> >> +        if (IS_ERR(reset)) {
> >> +                ret = PTR_ERR(reset);
> >> +                goto out_declock;
> >> +        }
> >> +        reset_control_deassert(reset);
> >
> > We might have the same issue than the mailbox driver where the
> > firmware will need to access the block at any time, so we can't really
> > toggle the reset line as we want.
>
> What should we do then ?

Unless you put a firmware on the ARISC core and see how it interacts
between the two, you can't really do anything.

> >> +	num_locks = sunxi_get_num_locks(io_base);
> >> +	if (!num_locks) {
> >> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
> >> +		ret = -EINVAL;
> >> +		goto out_reset;
> >> +	}
> >> +
> >> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
> >> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);
> >> +	if (!hw) {
> >> +		ret = -ENOMEM;
> >> +		goto out_reset;
> >> +	}
> >
> > That looks rather convoluted (especially since the variable length
> > array is at the second level), and can be made more obvious by:
> >
> > - Removing the hwspinlock_device from sunxi_hwspinlock and allocating
> >   both separately.
> >
> > - And then allocate the hwspinlock_device separately with struct_size
>
> Actually this can be allocated via struct_size e.g. struct_size(hw,
> bank.lock, num_locks).
>
> >
> >> +	hw->clk = clk;
> >> +	hw->reset = reset;
> >
> > Why not using the structure directly instead of having temporary
> > variables?
>
> Because I use the clk/reset before allocating the devmem.

Then do it the other way around?

> >
> >> +	io_base += LOCK_BASE_OFFSET;
> >> +	for (i = 0; i < num_locks; i++)
> >> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> >
> > Using a define for the registers offset would be nice here.
>
> you mean something like:
>
> # define LOCK(X) (x * sizeof(u32))
>
> I think this brings more needless indirection than helps but if you
> insist I won't mind.

It's not more of an indirection than using a function, and you're
doing that all the time already :)

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: dts: allwinner: a64: Add hwspinlock node

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1877 bytes --]

On Tue, Feb 11, 2020 at 10:09:48AM +0200, Nikolay Borisov wrote:
>
>
> On 11.02.20 г. 9:55 ч., Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Feb 10, 2020 at 07:01:42PM +0200, Nikolay Borisov wrote:
> >> Add a node describing the hwspinlock on A64.
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> index 27e48234f1c2..01de89fc09cc 100644
> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> @@ -1182,5 +1182,14 @@
> >>  			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> >>  			clocks = <&osc24M>;
> >>  		};
> >> +
> >> +		hwspinlock: spinlock@1c18000 {
> >
> > Nodes are supposed to be ordered by ascending physical address
> >
> >> +			compatible = "allwinner,sun50i-a64-hwspinlock";
> >> +			reg = <0x01c18000 0x1000>;
> >> +			clocks = <&ccu CLK_BUS_SPINLOCK>;
> >> +			resets = <&ccu RST_BUS_SPINLOCK>;
> >> +			#hwlock-cells = <1>;
> >> +			status = "disabled";
> >
> > Is there a reason to disable it?
>
> I wondered about that - on the one hand we can safely leave it always
> enabled, on the other hand all devices are are disabled in the dtsi.

Not all of them, only the one that are connected to external pins. The
PMU or GPU aren't for example.

> > Also, my understanding was that hwspinlocks were meant to be used by
> > client drivers, so surely we must tie them to other device nodes too,
> > right?
>
> Perhaps but at this point I'm not sure to which device specifically.

All the one that are shared with the ARISC core and needs some
synchronisation with the firmware running there.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov]

On 11.02.20 г. 14:34 ч., Maxime Ripard wrote:
> On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
>> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
>>>> Based on the datasheet this implements support for the hwspinlock IP
>>>> block.
>>>
>>> How was this tested?
>>
>> I tested it on my pine64 lts e.g. loading the driver and reading the
>> reset/clock/sysstatus registers to ensure everything is unmasked and has
>> expected values.
> 
> Isn't the point of hwspinlocks that it's shared between the ARISC core
> and the ARM cores. How did you test that the lock was actually taken
> on the other side just by using the ARM cores?

I haven't. I'm really focuse don just enabling this on the linux side of
things. True, hw spinlocks are used to synchronize cpu running different
OS'es. It's still implementation defined which hwspinlock is used for
which component. Additionally if we assume the ARISC core uses spinlock
this means by the time linux is booted the spinlocks should already be
clocked and out of software reset so perhahps this is also redundant in
the driver?


<snip>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1917 bytes --]

On Tue, Feb 11, 2020 at 03:17:40PM +0200, Nikolay Borisov wrote:
>
>
> On 11.02.20 г. 14:34 ч., Maxime Ripard wrote:
> > On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
> >> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
> >>>> Based on the datasheet this implements support for the hwspinlock IP
> >>>> block.
> >>>
> >>> How was this tested?
> >>
> >> I tested it on my pine64 lts e.g. loading the driver and reading the
> >> reset/clock/sysstatus registers to ensure everything is unmasked and has
> >> expected values.
> >
> > Isn't the point of hwspinlocks that it's shared between the ARISC core
> > and the ARM cores. How did you test that the lock was actually taken
> > on the other side just by using the ARM cores?
>
> I haven't. I'm really focuse don just enabling this on the linux side of
> things. True, hw spinlocks are used to synchronize cpu running different
> OS'es.

I'm sorry but this driver hasn't been really tested then. The whole
point of it is to synchronise with something. If you tested without
that something, it's just like testing a network driver without having
anything connected on the network you're testing it on: it probably
looks like it's working, but you really can't tell.

> It's still implementation defined which hwspinlock is used for
> which component. Additionally if we assume the ARISC core uses spinlock
> this means by the time linux is booted the spinlocks should already be
> clocked and out of software reset so perhahps this is also redundant in
> the driver?

Linux also likes to disable the clocks no one is using, so in such a
situation, what would happen? Can the ARISC still use them, should we
maintain the enabled all the time?

This is exactly the kind of corner-cases that we need a test for.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov]

On 12.02.20 г. 14:06 ч., Maxime Ripard wrote:
> On Tue, Feb 11, 2020 at 03:17:40PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 11.02.20 г. 14:34 ч., Maxime Ripard wrote:
>>> On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
>>>> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
>>>>>> Based on the datasheet this implements support for the hwspinlock IP
>>>>>> block.
>>>>>
>>>>> How was this tested?
>>>>
>>>> I tested it on my pine64 lts e.g. loading the driver and reading the
>>>> reset/clock/sysstatus registers to ensure everything is unmasked and has
>>>> expected values.
>>>
>>> Isn't the point of hwspinlocks that it's shared between the ARISC core
>>> and the ARM cores. How did you test that the lock was actually taken
>>> on the other side just by using the ARM cores?
>>
>> I haven't. I'm really focuse don just enabling this on the linux side of
>> things. True, hw spinlocks are used to synchronize cpu running different
>> OS'es.
> 
> I'm sorry but this driver hasn't been really tested then. The whole
> point of it is to synchronise with something. If you tested without

I disagree, the whole point is to expose the facility for other drivers
which, in turn, might need to synchronize with that other thing. I see
the hwspinlock driver as a dumb provider of the interface. The only
pertinent contention point I see is how should the clock/soft reset
registers be programmed considering the spinlocks might be accessed
outside of linux.

> that something, it's just like testing a network driver without having
> anything connected on the network you're testing it on: it probably
> looks like it's working, but you really can't tell.
> 
>> It's still implementation defined which hwspinlock is used for
>> which component. Additionally if we assume the ARISC core uses spinlock
>> this means by the time linux is booted the spinlocks should already be
>> clocked and out of software reset so perhahps this is also redundant in
>> the driver?
> 
> Linux also likes to disable the clocks no one is using, so in such a
> situation, what would happen? Can the ARISC still use them, should we
> maintain the enabled all the time?
> 
> This is exactly the kind of corner-cases that we need a test for.


Fair point BUT, and this is one big BUT. This other thing (the ARISC fw)
doesn't have a contract with the kernel on synchronization. So should
the test be performed against allwinner's binary blob or against some of
the open source alternatives ( I only saw it mentioned on the sunxi web
page but no links to such open source alternatives).


Furthermore, if we assume we should be compatible with the binary blob
it needs to be RE'd in order to understand which hw spinlocks it's using
for synchronization and I'm not aware of any such effort.

> 
> Maxime
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4238 bytes --]

On Wed, Feb 12, 2020 at 04:32:11PM +0200, Nikolay Borisov wrote:
>
>
> On 12.02.20 г. 14:06 ч., Maxime Ripard wrote:
> > On Tue, Feb 11, 2020 at 03:17:40PM +0200, Nikolay Borisov wrote:
> >>
> >>
> >> On 11.02.20 г. 14:34 ч., Maxime Ripard wrote:
> >>> On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
> >>>> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
> >>>>>> Based on the datasheet this implements support for the hwspinlock IP
> >>>>>> block.
> >>>>>
> >>>>> How was this tested?
> >>>>
> >>>> I tested it on my pine64 lts e.g. loading the driver and reading the
> >>>> reset/clock/sysstatus registers to ensure everything is unmasked and has
> >>>> expected values.
> >>>
> >>> Isn't the point of hwspinlocks that it's shared between the ARISC core
> >>> and the ARM cores. How did you test that the lock was actually taken
> >>> on the other side just by using the ARM cores?
> >>
> >> I haven't. I'm really focuse don just enabling this on the linux side of
> >> things. True, hw spinlocks are used to synchronize cpu running different
> >> OS'es.
> >
> > I'm sorry but this driver hasn't been really tested then. The whole
> > point of it is to synchronise with something. If you tested without
>
> I disagree, the whole point is to expose the facility for other drivers
> which, in turn, might need to synchronize with that other thing.

And you haven't tested that either.

> I see the hwspinlock driver as a dumb provider of the interface. The
> only pertinent contention point I see is how should the clock/soft
> reset registers be programmed considering the spinlocks might be
> accessed outside of linux.
>
> > that something, it's just like testing a network driver without having
> > anything connected on the network you're testing it on: it probably
> > looks like it's working, but you really can't tell.
> >
> >> It's still implementation defined which hwspinlock is used for
> >> which component. Additionally if we assume the ARISC core uses spinlock
> >> this means by the time linux is booted the spinlocks should already be
> >> clocked and out of software reset so perhahps this is also redundant in
> >> the driver?
> >
> > Linux also likes to disable the clocks no one is using, so in such a
> > situation, what would happen? Can the ARISC still use them, should we
> > maintain the enabled all the time?
> >
> > This is exactly the kind of corner-cases that we need a test for.
>
> Fair point BUT, and this is one big BUT. This other thing (the ARISC fw)
> doesn't have a contract with the kernel on synchronization.

You really need two things here:

 - A hwspinlock driver that works, and you can see that on the other
   side you have that lock reported as taken (either taking one on the
   ARISC and seeing that it's taken on the ARM core, or the other way
   around). You don't really need a contract for that to do that kind
   of test, you can hack your own.

 - Once that driver is working, indeed, we'll need to come up with
   that contract. It's not that hard, really, it's just assigning a
   number to every device that could be shared, and we're even in a
   position where we can probably force that, and every implementation
   would have to comply. But that's a second step.

> So should the test be performed against allwinner's binary blob or
> against some of the open source alternatives ( I only saw it
> mentioned on the sunxi web page but no links to such open source
> alternatives).
>
> Furthermore, if we assume we should be compatible with the binary blob
> it needs to be RE'd in order to understand which hw spinlocks it's using
> for synchronization and I'm not aware of any such effort.

You don't really need to care about the Allwinner firmware. If they
already have a list of devices they have assigned, it's just as good
as any and if it makes sense we can use it, but if they don't or if it
doesn't make sense, I'm totally fine ignoring that driver as well.

The best thing you should target right now is crust:
https://github.com/crust-firmware/crust

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel