lkml.org 
[lkml]   [2020]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-11-26 14:33    [W:0.042 / U:0.932 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site