Message-ID: <20170516191801.GA12920@tuxbook>
Date:   Tue, 16 May 2017 12:18:01 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     ohad@...ery.com, linux-kernel@...r.kernel.org,
        linux-remoteproc@...r.kernel.org, broonie@...nel.org,
        baolin.wang@...eadtrum.com
Subject: Re: [PATCH] hwspinlock: sprd: Add hardware spinlock driver

On Tue 16 May 00:54 PDT 2017, Baolin Wang wrote:

> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index 6b59cb5a..03c442f 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_HWSPINLOCK_OMAP)		+= omap_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_QCOM)		+= qcom_hwspinlock.o
>  obj-$(CONFIG_HWSPINLOCK_SIRF)		+= sirf_hwspinlock.o
>  obj-$(CONFIG_HSEM_U8500)		+= u8500_hsem.o
> +obj-$(CONFIG_HWSPINLOCK_SPRD)		+= sprd_hwspinlock.o

Please move this one line up, to keep the alphabetical sort order
(please make sure to update the order in Kconfig accordingly).

> diff --git a/drivers/hwspinlock/sprd_hwspinlock.c b/drivers/hwspinlock/sprd_hwspinlock.c
> new file mode 100644
> index 0000000..898738f
> --- /dev/null
> +++ b/drivers/hwspinlock/sprd_hwspinlock.c
> @@ -0,0 +1,243 @@
> +/*
> + * Spreadtrum hardware spinlock driver
> + * Copyright (C) 2017 Spreadtrum  - http://www.spreadtrum.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>

I don't see a need for spinlock.h

> +
> +#include "hwspinlock_internal.h"
> +
> +/* hwspinlock registers definition */
> +#define HWSPINLOCK_RECCTRL		0x4
> +#define HWSPINLOCK_TTLSTS		0x8
> +#define HWSPINLOCK_FLAG0		0x10
> +#define HWSPINLOCK_FLAG1		0x14
> +#define HWSPINLOCK_FLAG2		0x18
> +#define HWSPINLOCK_FLAG3		0x1c

These flags are unused.

> +#define HWSPINLOCK_MASTERID(_X_)	(0x80 + 0x4 * (_X_))
> +#define HWSPINLOCK_TOKEN(_X_)		(0x800 + 0x4 * (_X_))
> +#define HWSPINLOCK_VERID		0xFFC

verid is unused.

> +
> +/* untoken lock value */

untaken? Perhaps "unlocked value" instead?

> +#define HWSPINLOCK_NOTTAKEN		0x55aa10c5
> +/* bits definition of RECCTRL reg */
> +#define HWSPINLOCK_ID			0x0

id is unused.

> +#define HWSPINLOCK_USER_BITS		0x1
> +
> +/* hwspinlock number */
> +#define SPRD_HWLOCKS_NUM		32
> +
> +struct sprd_hwspinlock_dev {
> +	void __iomem *base;
> +	struct clk *clk;
> +	unsigned char status[SPRD_HWLOCKS_NUM];
> +	struct hwspinlock_device bank;
> +};
> +
> +static const struct of_device_id sprd_hwspinlock_of_match[] = {
> +	{ .compatible = "sprd,hwspinlock-r3p0",},
> +	{ /* sentinel */ }
> +};

Please move this next to the definition of sprd_hwspinlock_driver and
add the missing MODULE_DEVICE_TABLE(of, sprd_hwspinlock_of_match);

> +
> +static struct sprd_hwspinlock_dev *sprd_lock_to_dev(struct hwspinlock *lock)
> +{
> +	struct hwspinlock_device *hwbank;
> +	unsigned int lock_id = hwlock_to_id(lock);
> +
> +	hwbank = container_of(lock, struct hwspinlock_device, lock[lock_id]);
> +	return container_of(hwbank, struct sprd_hwspinlock_dev, bank);

As you platform_set_drvdata the sprd_hwspinlock_dev in probe you can
implement this with function with

return dev_get_drvdata(lock->bank->dev);

> +}
> +
> +/* set the hardware spinlock record type */
> +static void sprd_set_hwspinlock_record(struct sprd_hwspinlock_dev *sprd_hwlock,
> +				       unsigned int type)
> +{
> +	writel_relaxed(type, sprd_hwlock->base + HWSPINLOCK_RECCTRL);

Please use writel() and please inline this write into the probe function.

> +}
> +
> +/* get the hardware spinlock master/user id */
> +static unsigned int sprd_get_hwspinlock_id(struct sprd_hwspinlock_dev *sprd_hwlock,
> +					   unsigned int lock_id)
> +{
> +	return readl_relaxed(sprd_hwlock->base + HWSPINLOCK_MASTERID(lock_id));

Please use readl() and please inline this function in
sprd_hwspinlock_trylock()

> +}
> +
> +/* record the hardware spinlock status */
> +static int sprd_record_hwspinlock_sts(struct hwspinlock *lock)

The hwlock->status is not read by anyone, so please remove this
function.

> +{
> +	struct sprd_hwspinlock_dev *sprd_hwlock = sprd_lock_to_dev(lock);
> +	unsigned int lock_id = hwlock_to_id(lock);
> +	unsigned char status;
> +
> +	if (lock_id >= SPRD_HWLOCKS_NUM) {
> +		dev_err(sprd_hwlock->bank.dev, "lock id is out of the range\n");
> +		return -ENXIO;
> +	}
> +
> +	/* get the hardware spinlock status */
> +	status = !!(readl_relaxed(sprd_hwlock->base + HWSPINLOCK_TTLSTS) &
> +		    BIT(lock_id));
> +
> +	sprd_hwlock->status[lock_id] = status;
> +	return 0;
> +}
> +
> +/* try to lock the hardware spinlock */
> +static int sprd_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	struct sprd_hwspinlock_dev *sprd_hwlock = sprd_lock_to_dev(lock);
> +	void __iomem *addr = lock->priv;
> +
> +	if (!readl_relaxed(addr))
> +		goto locked;

Please use readl() and as sprd_record_hwspinlock_sts() doesn't seem to
be needed, return 1 here.

> +
> +	dev_warn(sprd_hwlock->bank.dev,
> +		 "hwspinlock [%d] lock failed and master/user id = %d!\n",
> +		 hwlock_to_id(lock),
> +		 sprd_get_hwspinlock_id(sprd_hwlock, hwlock_to_id(lock)));

Please use local variables, rather than calling these functions in the
parameter list.

> +	return 0;
> +
> +locked:
> +	sprd_record_hwspinlock_sts(lock);
> +	return 1;
> +}
> +
> +/* unlock the hardware spinlock */
> +static void sprd_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	writel_relaxed(HWSPINLOCK_NOTTAKEN, lock_addr);

Please use writel()

> +	sprd_record_hwspinlock_sts(lock);
> +}
> +
> +/* The specs recommended below number as the retry delay time */
> +static void sprd_hwspinlock_relax(struct hwspinlock *lock)
> +{
> +	ndelay(10);
> +}
> +
> +static const struct hwspinlock_ops sprd_hwspinlock_ops = {
> +	.trylock = sprd_hwspinlock_trylock,
> +	.unlock = sprd_hwspinlock_unlock,
> +	.relax = sprd_hwspinlock_relax,
> +};
> +
> +static int sprd_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sprd_hwspinlock_dev *sprd_hwlock;
> +	struct hwspinlock *lock;
> +	struct resource *res;
> +	int i, ret;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENODEV;
> +
> +	sprd_hwlock = devm_kzalloc(&pdev->dev,
> +				   sizeof(struct sprd_hwspinlock_dev) +
> +				   SPRD_HWLOCKS_NUM * sizeof(*lock),
> +				   GFP_KERNEL);
> +	if (!sprd_hwlock)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sprd_hwlock->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(sprd_hwlock->base))
> +		return PTR_ERR(sprd_hwlock->base);
> +
> +	sprd_hwlock->clk = of_clk_get_by_name(pdev->dev.of_node, "enable");

Please use devm_clk_get(&pdev->dev, "enable");

> +	if (IS_ERR(sprd_hwlock->clk)) {
> +		dev_err(&pdev->dev, "get hwspinlock clock failed!\n");
> +		return PTR_ERR(sprd_hwlock->clk);
> +	}
> +
> +	clk_prepare_enable(sprd_hwlock->clk);
> +
> +	/* set the hwspinlock to record user id to identify subsystems */
> +	sprd_set_hwspinlock_record(sprd_hwlock, HWSPINLOCK_USER_BITS);
> +
> +	for (i = 0; i < SPRD_HWLOCKS_NUM; i++) {
> +		lock = &sprd_hwlock->bank.lock[i];
> +		lock->priv = sprd_hwlock->base + HWSPINLOCK_TOKEN(i);
> +	}
> +
> +	platform_set_drvdata(pdev, sprd_hwlock);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = hwspin_lock_register(&sprd_hwlock->bank, &pdev->dev,
> +				   &sprd_hwspinlock_ops, 0, SPRD_HWLOCKS_NUM);
> +	if (ret) {
> +		dev_err(&pdev->dev, "hwspinlock register failed!\n");

All error paths of hwspin_lock_register() will cause a log print already.

> +		pm_runtime_disable(&pdev->dev);
> +		clk_disable_unprepare(sprd_hwlock->clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sprd_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sprd_hwspinlock_dev *sprd_hwlock = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = hwspin_lock_unregister(&sprd_hwlock->bank);
> +	if (ret) {
> +		dev_err(&pdev->dev, "hwspinlock unregister failed: %d\n", ret);

All errors in hwspin_lock_unregister() will cause log prints.

> +		return ret;

You don't want to return early from a platform_driver remove function,
the caller ignores this and you will just leak resources.

> +	}
> +
> +	pm_runtime_disable(&pdev->dev);
> +	clk_disable_unprepare(sprd_hwlock->clk);
> +	return 0;
> +}
> +
> +static struct platform_driver sprd_hwspinlock_driver = {
> +	.probe = sprd_hwspinlock_probe,
> +	.remove = sprd_hwspinlock_remove,
> +	.driver = {
> +		.name = "sprd_hwspinlock",
> +		.owner = THIS_MODULE,

No need to set .owner in platform_drivers anymore.

> +		.of_match_table = of_match_ptr(sprd_hwspinlock_of_match),
> +	},
> +};
> +

Regards,
Bjorn

Powered by blists - more mailing lists