On Sat, 21 Nov 2020 23:19:00 -0600
Bjorn Andersson <bjorn.anders...@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.gottw...@posteo.net>");
> > --
> > 2.29.2
> >
