Messages in this thread | | | Date | Thu, 26 Nov 2020 14:31:43 +0100 | From | Wilken Gottwalt <> | Subject | Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support |
| |
On Tue, 24 Nov 2020 08:54:25 -0600 Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> On Mon 23 Nov 12:17 CST 2020, Wilken Gottwalt wrote: > > > On Sat, 21 Nov 2020 23:19:00 -0600 > > Bjorn Andersson <bjorn.andersson@linaro.org> 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
| |