LinuxLists.cc - [PATCH 0/2] hwspinlock: add sunxi hardware spinlock support

2020-11-18 10:03:30

Subject: [PATCH 0/2] hwspinlock: add sunxi hardware spinlock support

by Wilken Gottwalt

[permalink] [raw]

Subject: Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

On Sat, 21 Nov 2020 23:19:00 -0600
Bjorn Andersson <[email protected]> wrote:
> > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > +{
> > + struct sunxi_hwspinlock_data *priv = seqf->private;
> > + int inuse;
> > +
> > + /* getting the status of only the main 32 spinlocks is supported */
> > + inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
>
> So this returns how many of the locks are taken? How is that useful?

It is a way to see if locks were taken from linux or the arisc core without
touching the actual hwspinlock abi or the locks. So it is a nice way to debug
hwspinlocks, hence it is part of debugfs.

> > + seq_printf(seqf, "%d\n", inuse);
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(hwlocks_inuse);
> > +
> > +static void sunxi_hwspinlock_debugfs_init(struct sunxi_hwspinlock_data *priv)
> > +{
> > + char name[32];
> > +
> > + scnprintf(name, sizeof(name), "%s", DRIVER_NAME);
>
> Why not just pass DRIVER_NAME directly to debugfs_create_dir()?

Uuuh, you're right, that wasn't very clever. I think I changed the name creation
to something simpler and and just missed most obvious one.

> > +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *node = pdev->dev.of_node;
>
> I don't see a need for this, or the check to fail if it's NULL. Please
> remove it.

Yeah, will remove it.

> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->io_base = devm_ioremap_resource(&pdev->dev, res);
>
> Please use devm_platform_ioremap_resource() instead.

Hmm, so there is a platform version of it, too. Will change it.

> > + priv->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");
> > + if (IS_ERR(priv->reset)) { > + err = PTR_ERR(priv->reset);
> > + if (err == -EPROBE_DEFER)
> > + return err;
> > +
> > + dev_info(&pdev->dev, "no optional reset control available (%d)\n", err);
>
> In the even that no "ahb" reset is specified priv->reset is NULL, as
> such if you get here there's something wrong - so it's better to fail
> here.
>
> And you can use the convenient dev_err_probe() function to deal with the
> EPROBE_DEFER.
>
> > + priv->reset = NULL;
> > + }
> > +
> > + if (priv->reset) {
>
> It's perfectly fine to call reset_control_deassert(NULL); so you can
> omit this check.

Also will update these.

> > + /* bit 28 and 29 hold the amount of spinlock banks */
> > + num_banks = (readl(priv->io_base + SPINLOCK_SYSSTATUS_REG) >> 28) & 0x03;
> > + switch (num_banks) {
> > + case 1 ... 4:
>
> But the comment above says...and num_banks was & 0x3, so how can it be ...4?
>
> > + /*
> > + * 32, 64, 128 and 256 spinlocks are supported by the hardware implementation,
> > + * though most only support 32 spinlocks
> > + */
> > + priv->nlocks = 1 << (4 + num_banks);
> > + break;
> > + default:
>
> Given the mask above I believe this would only happen if bits 28 and 29
> are both 0...
>
> Regardless I think that this would be better written as a
>
> if (num_banks == invalid) {
> ...
> goto fail;
> }
>
> priv->nlocks = ...;

This one is a really odd one I noticed right after I submitted the patch. I
added the & 0x03 after reading the datasheets again. But I think the datasheets
are not fully correct here. The datasheets say, 0-4 represent 0, 32, 64, 128 and
256 locks and at the same time say, bits 28/29 are used for this and bits 30/31
are reserved. But you can't represent 5 values with 2 bits. So I'm pretty sure
these reserved bits are also used for it, at least bit 30. I will change this to
something which is more clear. It's weird, the H3, H5 and H6 datasheet state
exactly the same issue.

> > + dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> > + err = -EINVAL;
> > + goto fail;
> > + }
> > +
> > + /*
> > + * the Allwinner hwspinlock device uses 32bit registers for representing every single
> > + * spinlock, which is a real waste of space
> > + */
>
> Might be a waste, but having them be the natural write size in hardware
> makes sense. I'm however not sure what this comment has to do with the
> following allocation.
>
> > + priv->bank = devm_kzalloc(&pdev->dev, priv->nlocks * sizeof(*hwlock) +
> > sizeof(*priv->bank),
>
> Consider using struct_size() here.
>
> > + GFP_KERNEL);
> > + if (!priv->bank) {
> > + err = -ENOMEM;
> > + goto fail;
>
> If you do this allocation before you start the clock and deassert the
> reset you can just return -ENOMEM here, instead of the goto.
>
> > + }
> > +
> > + for (i = 0, hwlock = &priv->bank->lock[0]; i < priv->nlocks; ++i, ++hwlock) {
> > + hwlock->priv = devm_kzalloc(&pdev->dev, sizeof(struct sunxi_hwspinlock),
> > + GFP_KERNEL);
>
> hwpriv = devm_kzalloc(&pdev->dev, sizeof(*hwpriv), GFP_KERNEL);
> if (!hwpriv)
> return -ENOMEM;
>
> hwpriv->io_base = ...;
> priv->bank->lock[i].priv = hwpriv;
>
>

You're right, I will update this.

> > + if (!hwlock->priv) {
> > + err = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + hwpriv = hwlock->priv;
> > + hwpriv->io_base = priv->io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> > + }
> > +
> > + err = hwspin_lock_register(priv->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> > SPINLOCK_BASE_ID,
> > + priv->nlocks);
> > + if (err) {
> > + dev_err(&pdev->dev, "unable to register hwspinlocks (%d)\n", err);
> > + goto fail;
> > + }
> > +
> > + sunxi_hwspinlock_debugfs_init(priv);
> > +
> > + dev_dbg(&pdev->dev, "SUNXI hardware spinlock driver enabled (%d locks)\n",
> > priv->nlocks); +
> > + return 0;
> > +
> > +fail:
> > + clk_disable_unprepare(priv->ahb_clock);
> > +
> > +reset_fail:
> > + if (priv->reset)
> > + reset_control_assert(priv->reset);
> > +
> > + return err;
> > +}
> > +
> > +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> > +{
> > + struct sunxi_hwspinlock_data *priv = platform_get_drvdata(pdev);
> > + int err;
> > +
> > + debugfs_remove_recursive(priv->debugfs);
> > +
> > + err = hwspin_lock_unregister(priv->bank);
> > + if (err) {
> > + dev_err(&pdev->dev, "unregister device failed (%d)\n", err);
> > + return err;
> > + }
> > +
> > + if (priv->reset)
> > + reset_control_assert(priv->reset);
> > +
> > + clk_disable_unprepare(priv->ahb_clock);
>
> By using devm_add_action_or_reset() to set up an "assert and unprepare"-
> function you can use devm_hwspin_lock_register(), you can drop the
> remove function and you can clean up your sunxi_hwspinlock_data (e.g.
> you no longer need a pointer to priv->bank).

I'm not very used to these devm_* functions yet, but will try to use these.

> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > + { .compatible = "allwinner,sun8i-hwspinlock", },
> > + { .compatible = "allwinner,sun50i-hwspinlock", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > +
> > +static struct platform_driver sunxi_hwspinlock_driver = {
> > + .probe = sunxi_hwspinlock_probe,
> > + .remove = sunxi_hwspinlock_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .of_match_table = of_match_ptr(sunxi_hwspinlock_ids),
>
> Please avoid of_match_ptr, as this will cause warnings about unused
> variables when COMPILE_TEST without OF.

So did you mean to leave it out completely?

Thanks for looking through this, this really helps a lot. :-)

greetings,
Wilken

> Regards,
> Bjorn
>
> > + },
> > +};
> > +
> > +static int __init sunxi_hwspinlock_init(void)
> > +{
> > + return platform_driver_register(&sunxi_hwspinlock_driver);
> > +}
> > +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");
> > +MODULE_DESCRIPTION("SUNXI hardware spinlock driver");
> > +MODULE_AUTHOR("Wilken Gottwalt <[email protected]>");
> > --
> > 2.29.2
> >

2020-11-23 18:37:39

by Wilken Gottwalt

[permalink] [raw]

Subject: Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

On Sat, 21 Nov 2020 17:44:18 +0100
Maxime Ripard <[email protected]> wrote:

> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
> > On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
> > > > On Thu, 19 Nov 2020 08:15:23 +0100
> > > > Maxime Ripard <[email protected]> wrote:
> > > > > > can you help me here a bit? I still try to figure out how to do patch sets
> > > > > > properly. Some kernel submitting documentation says everything goes into the
> > > > > > coverletter and other documentation only tells how to split the patches. So
> > > > > > what would be the right way? A quick example based on my patch set would be
> > > > > > really helpful.
> > > > >
> > > > > I mean, the split between your patches and so on is good, you got that right
> > > > >
> > > > > The thing I wanted better details on is the commit log itself, so the
> > > > > message attached to that patch.
> > > >
> > > > Ah yes, I think I got it now. So basically add a nice summary of the coverletter
> > > > there.
> > >
> > > Yes, a bit more context as well. Eventually, this should be the
> > > motivation on why this patch is useful. So what it can be used for, what
> > > are the challenges, how it was tested, etc.
> > >
> > > The cover letter is usually here more to provide some meta-context: what
> > > you expect from the maintainers / reviewers if it's an RFC, if there's
> > > any feature missing or that could be added later on, etc.
> > >
> > > > > > > Most importantly, this hwspinlock is used to synchronize the ARM cores
> > > > > > > and the ARISC. How did you test this driver?
> > > > > >
> > > > > > Yes, you are right, I should have mentioned this. I have a simple test kernel
> > > > > > module for this. But I must admit, testing the ARISC is very hard and I have
> > > > > > no real idea how to do it. Testing the hwspinlocks in general seems to work
> > > > > > with my test kernel module, but I'm not sure if this is really sufficient. I
> > > > > > can provide the code for it if you like. What would be the best way? Github?
> > > > > > Just mailing a patch?
> > > > > >
> > > > > > The test module produces these results:
> > > > > >
> > > > > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko
> > > > > > [ 45.395672] [init] sunxi hwspinlock test driver start
> > > > > > [ 45.400775] [init] start test locks
> > > > > > [ 45.404263] [run ] testing 32 locks
> > > > > > [ 45.407804] [test] testing lock 0 -----
> > > > > > [ 45.411652] [test] taking lock attempt #0 succeded
> > > > > > [ 45.416438] [test] try taken lock attempt #0
> > > > > > [ 45.420735] [test] unlock/take attempt #0
> > > > > > [ 45.424752] [test] taking lock attempt #1 succeded
> > > > > > [ 45.429556] [test] try taken lock attempt #1
> > > > > > [ 45.433823] [test] unlock/take attempt #1
> > > > > > [ 45.437862] [test] testing lock 1 -----
> > > > >
> > > > > That doesn't really test for contention though, and dealing with
> > > > > contention is mostly what this hardware is about. Could you make a small
> > > > > test with crust to see if when the arisc has taken the lock, the ARM
> > > > > cores can't take it?
> > > >
> > > > So the best solution would be to write a bare metal program that runs on the
> > > > arisc and can be triggered from the linux side (the test kernel module) to take
> > > > a spinlock ... or at least take spinlocks periodically for a while and watch it
> > > > on the linux side. Okay, I think I can do this. Though, I have to dig through
> > > > all this new stuff first.
> > >
> > > It doesn't have to be super complicated, just a loop that takes a lock,
> > > sleeps for some time, and releases the lock should be enough to at least
> > > validate that the lock is actually working
> > >
> >
> > I think the difficulty is the bare metal program in arsic has little
> > documentation.
>
> crust has mostly figured it out:
> https://github.com/crust-firmware/crust

I actually have serious trouble to get crust running. It compiles for H2+/H3, but
I can't figure out if it runs at all. I will switch to a H5 based device which is
confirmed to work. If I see this correctly crust is doing nothing with spinlocks
yet, so I may end up also working on crust, adding the spinlocks there too. Don't
know yet how long I will take to understand every detail, but I will report
progress.

Greetings,
Wilken

2020-11-24 03:52:10

by Samuel Holland

[permalink] [raw]

Subject: Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support

On 11/23/20 12:32 PM, Wilken Gottwalt wrote:
> On Sat, 21 Nov 2020 17:44:18 +0100
> Maxime Ripard <[email protected]> wrote:
>
>> On Sat, Nov 21, 2020 at 08:22:55PM +0800, fuyao wrote:
>>> On Fri, Nov 20, 2020 at 05:42:31PM +0100, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Thu, Nov 19, 2020 at 11:13:43AM +0100, Wilken Gottwalt wrote:
>>>>> On Thu, 19 Nov 2020 08:15:23 +0100
>>>>> Maxime Ripard <[email protected]> wrote:
>>>>>>> can you help me here a bit? I still try to figure out how to do patch sets
>>>>>>> properly. Some kernel submitting documentation says everything goes into the
>>>>>>> coverletter and other documentation only tells how to split the patches. So
>>>>>>> what would be the right way? A quick example based on my patch set would be
>>>>>>> really helpful.
>>>>>>
>>>>>> I mean, the split between your patches and so on is good, you got that right
>>>>>>
>>>>>> The thing I wanted better details on is the commit log itself, so the
>>>>>> message attached to that patch.
>>>>>
>>>>> Ah yes, I think I got it now. So basically add a nice summary of the coverletter
>>>>> there.
>>>>
>>>> Yes, a bit more context as well. Eventually, this should be the
>>>> motivation on why this patch is useful. So what it can be used for, what
>>>> are the challenges, how it was tested, etc.
>>>>
>>>> The cover letter is usually here more to provide some meta-context: what
>>>> you expect from the maintainers / reviewers if it's an RFC, if there's
>>>> any feature missing or that could be added later on, etc.
>>>>
>>>>>>>> Most importantly, this hwspinlock is used to synchronize the ARM cores
>>>>>>>> and the ARISC. How did you test this driver?
>>>>>>>
>>>>>>> Yes, you are right, I should have mentioned this. I have a simple test kernel
>>>>>>> module for this. But I must admit, testing the ARISC is very hard and I have
>>>>>>> no real idea how to do it. Testing the hwspinlocks in general seems to work
>>>>>>> with my test kernel module, but I'm not sure if this is really sufficient. I
>>>>>>> can provide the code for it if you like. What would be the best way? Github?
>>>>>>> Just mailing a patch?
>>>>>>>
>>>>>>> The test module produces these results:
>>>>>>>
>>>>>>> # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko
>>>>>>> [ 45.395672] [init] sunxi hwspinlock test driver start
>>>>>>> [ 45.400775] [init] start test locks
>>>>>>> [ 45.404263] [run ] testing 32 locks
>>>>>>> [ 45.407804] [test] testing lock 0 -----
>>>>>>> [ 45.411652] [test] taking lock attempt #0 succeded
>>>>>>> [ 45.416438] [test] try taken lock attempt #0
>>>>>>> [ 45.420735] [test] unlock/take attempt #0
>>>>>>> [ 45.424752] [test] taking lock attempt #1 succeded
>>>>>>> [ 45.429556] [test] try taken lock attempt #1
>>>>>>> [ 45.433823] [test] unlock/take attempt #1
>>>>>>> [ 45.437862] [test] testing lock 1 -----
>>>>>>
>>>>>> That doesn't really test for contention though, and dealing with
>>>>>> contention is mostly what this hardware is about. Could you make a small
>>>>>> test with crust to see if when the arisc has taken the lock, the ARM
>>>>>> cores can't take it?
>>>>>
>>>>> So the best solution would be to write a bare metal program that runs on the
>>>>> arisc and can be triggered from the linux side (the test kernel module) to take
>>>>> a spinlock ... or at least take spinlocks periodically for a while and watch it
>>>>> on the linux side. Okay, I think I can do this. Though, I have to dig through
>>>>> all this new stuff first.
>>>>
>>>> It doesn't have to be super complicated, just a loop that takes a lock,
>>>> sleeps for some time, and releases the lock should be enough to at least
>>>> validate that the lock is actually working
>>>>
>>>
>>> I think the difficulty is the bare metal program in arsic has little
>>> documentation.
>>
>> crust has mostly figured it out:
>> https://github.com/crust-firmware/crust
>
> I actually have serious trouble to get crust running. It compiles for H2+/H3, but
> I can't figure out if it runs at all. I will switch to a H5 based device which is

Crust does not yet support the H2+/H3 (it is active WIP). H5 should work
well.

> confirmed to work. If I see this correctly crust is doing nothing with spinlocks
> yet, so I may end up also working on crust, adding the spinlocks there too. Don't> know yet how long I will take to understand every detail, but I will
report
> progress.

Correct. There is currently no hwspinlock driver in crust. For testing,
you can poke MMIO from the main loop, near the call to scpi_poll() in
common/system.c. You can use the timeout.h functions for timing.

If you want to write a full driver, I would like to know how you expect
to use the hwspinlocks. Allocating the locks has to be coordinated among
all of the users: Linux, U-Boot, crust, any other ARISC firmware, etc.

> Greetings,
> Wilken

Cheers,
Samuel

2020-11-24 14:32:26

On Tue, 24 Nov 2020 08:54:25 -0600
Bjorn Andersson <[email protected]> wrote:

> On Mon 23 Nov 12:17 CST 2020, Wilken Gottwalt wrote:
>
> > On Sat, 21 Nov 2020 23:19:00 -0600
> > Bjorn Andersson <[email protected]> wrote:
> > > > +static int hwlocks_inuse_show(struct seq_file *seqf, void *unused)
> > > > +{
> > > > + struct sunxi_hwspinlock_data *priv = seqf->private;
> > > > + int inuse;
> > > > +
> > > > + /* getting the status of only the main 32 spinlocks is supported */
> > > > + inuse = hweight32(readl(priv->io_base + SPINLOCK_STATUS_REG));
> > >
> > > So this returns how many of the locks are taken? How is that useful?
> >
> > It is a way to see if locks were taken from linux or the arisc core without
> > touching the actual hwspinlock abi or the locks. So it is a nice way to debug
> > hwspinlocks, hence it is part of debugfs.
> >
>
> So in a scenario where two remote processors ping-pong the lock between
> them, this will always read 1 and you won't know why?

I know it is not perfect. I will change it to actually report which locks are
taken. And currently the crust firmware does not use the locks and on the
Linux side there are only a handful of driver/components using hwspinlocks,
and none of them are active in a kernel compiled for a H5. So it really is a
nice way to check/debug the hwspinlocks. I already have a simple test running
where crust sets a spinlock and I can see it on Linux without touching the
actuall locks thanks to this status register.

> > > > + seq_printf(seqf, "%d\n", inuse);
> [..]
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id sunxi_hwspinlock_ids[] = {
> > > > + { .compatible = "allwinner,sun8i-hwspinlock", },
> > > > + { .compatible = "allwinner,sun50i-hwspinlock", },
> > > > + {},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, sunxi_hwspinlock_ids);
> > > > +
> > > > +static struct platform_driver sunxi_hwspinlock_driver = {
> > > > + .probe = sunxi_hwspinlock_probe,
> > > > + .remove = sunxi_hwspinlock_remove,
> > > > + .driver = {
> > > > + .name = DRIVER_NAME,
> > > > + .of_match_table = of_match_ptr(sunxi_hwspinlock_ids),
> > >
> > > Please avoid of_match_ptr, as this will cause warnings about unused
> > > variables when COMPILE_TEST without OF.
> >
> > So did you mean to leave it out completely?
> >
>
> Yes, "worst case" is that you include the reference to
> sunxi_hwspinlock_ids on a build without CONFIG_OF and wasting a little
> bit of memory.
>
> Using of_match_ptr() with CONFIG_OF=n will result in NULL and as such
> we'll get a compile warning that nothing references sunxi_hwspinlock_ids
> - so then that will have to be marked __maybe_unused, or wrapped in an
> #if...
>
> So better just leave it as:
> .of_match_table = sunxi_hwspinlock_ids,

Thank you for the explanation. I really like to know the details and reasons
behind this.

greetings,
Wilken

> Regards,
> Bjorn