Re: [RFC 2/5] hwspinlock: Add Realtek RTD1195 SB2

From: Fabien DESSENNE
Date: Tue Dec 03 2019 - 03:46:41 EST


Hi Andreas


Few comments below.

BR

Fabien


On 02/12/2019 11:05 PM, Andreas FÃrber wrote:
> Implement a driver for hardware semaphores found in RTD1195 SoC.
> It allows for both per-register instances (SB2_HD_SEM) as well as
> contiguous register ranges (SB2_HD_SEM_NEW[0-7]).
>
> While these registers are part of the SB2 syscon, this implementation
> does not use syscon, to allow assigning one as SB2 syscon's hwlock.
>
> Cc: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>
> Signed-off-by: Andreas FÃrber <afaerber@xxxxxxx>
> ---
> drivers/hwspinlock/Kconfig | 11 ++++
> drivers/hwspinlock/Makefile | 1 +
> drivers/hwspinlock/rtd1195_sb2_sem.c | 101 +++++++++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+)
> create mode 100644 drivers/hwspinlock/rtd1195_sb2_sem.c
>
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 37740e992cfa..9c26c436d399 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -28,6 +28,17 @@ config HWSPINLOCK_QCOM
>
> If unsure, say N.
>
> +config HWSPINLOCK_RTD1195_SB2
> + tristate "Realtek RTD1195 SB2 Hardware Spinlock device"
> + depends on HWSPINLOCK
> + depends on ARCH_REALTEK || COMPILE_TEST
> + default ARCH_REALTEK
> + help
> + Say y here to support the Realtek Hardware Semaphore functionality,
> + found on the RTD1195 and later SoC families.
> +
> + If unsure, say N.
> +
> config HWSPINLOCK_SIRF
> tristate "SIRF Hardware Spinlock device"
> depends on HWSPINLOCK
> diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
> index ed053e3f02be..d5bd59412468 100644
> --- a/drivers/hwspinlock/Makefile
> +++ b/drivers/hwspinlock/Makefile
> @@ -6,6 +6,7 @@
> obj-$(CONFIG_HWSPINLOCK) += hwspinlock_core.o
> obj-$(CONFIG_HWSPINLOCK_OMAP) += omap_hwspinlock.o
> obj-$(CONFIG_HWSPINLOCK_QCOM) += qcom_hwspinlock.o
> +obj-$(CONFIG_HWSPINLOCK_RTD1195_SB2) += rtd1195_sb2_sem.o
> obj-$(CONFIG_HWSPINLOCK_SIRF) += sirf_hwspinlock.o
> obj-$(CONFIG_HWSPINLOCK_SPRD) += sprd_hwspinlock.o
> obj-$(CONFIG_HWSPINLOCK_STM32) += stm32_hwspinlock.o
> diff --git a/drivers/hwspinlock/rtd1195_sb2_sem.c b/drivers/hwspinlock/rtd1195_sb2_sem.c
> new file mode 100644
> index 000000000000..ae925d057874
> --- /dev/null
> +++ b/drivers/hwspinlock/rtd1195_sb2_sem.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * RTD1195 SB2 hardware semaphore
> + *
> + * Copyright (c) 2019 Andreas FÃrber
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/idr.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#include "hwspinlock_internal.h"
> +
> +struct rtd1195_sb2_sem {
> + struct platform_device *pdev;
> + void __iomem *base;
> + int base_id;
The pdev, base and base_id members of this struct are set in .probe()
but never read anywhere: you may remove them and keep only lockdev
> + struct hwspinlock_device lockdev;
> +};
> +
> +static DEFINE_IDR(rtd1195_sb2_sem_idr);
> +
> +static int rtd1195_sb2_sem_trylock(struct hwspinlock *lock)
> +{
> + void __iomem *reg = (void __iomem *)lock->priv;
> +
> + return readl_relaxed(reg) & BIT(0);
> +}
> +
> +static void rtd1195_sb2_sem_unlock(struct hwspinlock *lock)
> +{
> + void __iomem *reg = (void __iomem *)lock->priv;
> +
> + writel_relaxed(0, reg);
> +}
> +
> +static const struct hwspinlock_ops rtd1195_sb2_sem_hwspinlock_ops = {
> + .trylock = rtd1195_sb2_sem_trylock,
> + .unlock = rtd1195_sb2_sem_unlock,
> +};
> +
> +static int rtd1195_sb2_sem_probe(struct platform_device *pdev)
> +{
> + struct rtd1195_sb2_sem *sem;
> + struct hwspinlock *lock;
> + struct resource *res;
> + int i, num;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENOMEM;
> +
> + num = resource_size(res) / 4;
> +
> + sem = devm_kzalloc(&pdev->dev, sizeof(*sem) + num * sizeof(*lock),
> + GFP_KERNEL);
> + if (!sem)
> + return -ENOMEM;
> +
> + sem->pdev = pdev;
> +
> + sem->base = of_iomap(pdev->dev.of_node, 0);
You may use devm_ioremap_resource() here.
> + if (!sem->base)
> + return -ENOMEM;
> +
> + for (i = 0; i < num; i++) {
> + lock = &sem->lockdev.lock[i];
> + lock->priv = sem->base + i * 4;
> + }
> +
> + platform_set_drvdata(pdev, sem);
> +
> + sem->base_id = idr_alloc(&rtd1195_sb2_sem_idr, sem, 0, 0, GFP_KERNEL);
> +
> + return devm_hwspin_lock_register(&pdev->dev, &sem->lockdev,
> + &rtd1195_sb2_sem_hwspinlock_ops, sem->base_id, num);
> +}
> +
> +static const struct of_device_id rtd1195_sb2_sem_dt_ids[] = {
> + { .compatible = "realtek,rtd1195-sb2-sem" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rtd1195_sb2_sem_dt_ids);
> +
> +static struct platform_driver rtd1195_sb2_sem_platform_driver = {
> + .driver = {
> + .name = "rtd1195-sb2-sem",
> + .of_match_table = rtd1195_sb2_sem_dt_ids,
> + },
> + .probe = rtd1195_sb2_sem_probe,
> +};
> +module_platform_driver(rtd1195_sb2_sem_platform_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Hardware spinlock driver for Realtek RTD1195 SB2");
> +MODULE_AUTHOR("Andreas FÃrber <afaerber@xxxxxxx>");