From: Wilken Gottwalt <wilken.gottwalt@posteo.net>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: linux-kernel@vger.kernel.org, Ohad Ben-Cohen <ohad@wizery.com>,
Baolin Wang <baolin.wang7@gmail.com>,
Rob Herring <robh+dt@kernel.org>,
Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
Jernej Skrabec <jernej.skrabec@siol.net>
Subject: Re: [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support
Date: Mon, 23 Nov 2020 19:17:12 +0100 [thread overview]
Message-ID: <20201123191712.72484b19@monster.powergraphx.local> (raw)
In-Reply-To: <20201122051900.GH807@yoga>
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.
> > + 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 <wilken.gottwalt@posteo.net>");
> > --
> > 2.29.2
> >
next prev parent reply other threads:[~2020-11-23 18:17 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-18 10:01 [PATCH 0/2] hwspinlock: add sunxi hardware spinlock support Wilken Gottwalt
2020-11-18 10:01 ` [PATCH 1/2] dt-bindings: hwlock: sunxi: add sunxi_hwspinlock documentation Wilken Gottwalt
2020-11-18 10:02 ` [PATCH 2/2] hwspinlock: add sunxi hardware spinlock support Wilken Gottwalt
2020-11-18 15:37 ` Maxime Ripard
2020-11-18 19:36 ` Wilken Gottwalt
2020-11-19 7:15 ` Maxime Ripard
2020-11-19 10:13 ` Wilken Gottwalt
2020-11-20 16:42 ` Maxime Ripard
2020-11-21 12:22 ` fuyao
2020-11-21 16:44 ` Maxime Ripard
2020-11-23 18:32 ` Wilken Gottwalt
2020-11-24 3:35 ` Samuel Holland
2020-11-24 14:28 ` Maxime Ripard
2020-11-26 13:10 ` Wilken Gottwalt
2020-11-22 5:19 ` Bjorn Andersson
2020-11-23 18:17 ` Wilken Gottwalt [this message]
2020-11-24 14:54 ` Bjorn Andersson
2020-11-26 13:31 ` Wilken Gottwalt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201123191712.72484b19@monster.powergraphx.local \
--to=wilken.gottwalt@posteo.net \
--cc=baolin.wang7@gmail.com \
--cc=bjorn.andersson@linaro.org \
--cc=jernej.skrabec@siol.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mripard@kernel.org \
--cc=ohad@wizery.com \
--cc=robh+dt@kernel.org \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).