| Message ID | 20200210170143.20007-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for hwspinlock on A64 SoC | expand |
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
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 >
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 > >
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
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 >
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
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>
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
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 >
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
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>");
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