2021-10-11 16:35:35

by Fabien Dessenne

[permalink] [raw]
Subject: [PATCH v2] hwspinlock: stm32: enable clock at probe

Set the clock during probe and keep its control during suspend / resume
operations.
This fixes an issue when CONFIG_PM is not set and where the clock is
never enabled.

Make use of devm_ functions to simplify the code.

Signed-off-by: Fabien Dessenne <[email protected]>
---
Changes since v1:
- Call platform_set_drvdata() before pm_runtime_...() calls
---
drivers/hwspinlock/stm32_hwspinlock.c | 58 +++++++++++++++++----------
1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/hwspinlock/stm32_hwspinlock.c b/drivers/hwspinlock/stm32_hwspinlock.c
index 3ad0ce0da4d9..5bd11a7fab65 100644
--- a/drivers/hwspinlock/stm32_hwspinlock.c
+++ b/drivers/hwspinlock/stm32_hwspinlock.c
@@ -54,8 +54,23 @@ static const struct hwspinlock_ops stm32_hwspinlock_ops = {
.relax = stm32_hwspinlock_relax,
};

+static void stm32_hwspinlock_disable_clk(void *data)
+{
+ struct platform_device *pdev = data;
+ struct stm32_hwspinlock *hw = platform_get_drvdata(pdev);
+ struct device *dev = &pdev->dev;
+
+ pm_runtime_get_sync(dev);
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_put_noidle(dev);
+
+ clk_disable_unprepare(hw->clk);
+}
+
static int stm32_hwspinlock_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct stm32_hwspinlock *hw;
void __iomem *io_base;
size_t array_size;
@@ -66,41 +81,43 @@ static int stm32_hwspinlock_probe(struct platform_device *pdev)
return PTR_ERR(io_base);

array_size = STM32_MUTEX_NUM_LOCKS * sizeof(struct hwspinlock);
- hw = devm_kzalloc(&pdev->dev, sizeof(*hw) + array_size, GFP_KERNEL);
+ hw = devm_kzalloc(dev, sizeof(*hw) + array_size, GFP_KERNEL);
if (!hw)
return -ENOMEM;

- hw->clk = devm_clk_get(&pdev->dev, "hsem");
+ hw->clk = devm_clk_get(dev, "hsem");
if (IS_ERR(hw->clk))
return PTR_ERR(hw->clk);

- for (i = 0; i < STM32_MUTEX_NUM_LOCKS; i++)
- hw->bank.lock[i].priv = io_base + i * sizeof(u32);
+ ret = clk_prepare_enable(hw->clk);
+ if (ret) {
+ dev_err(dev, "Failed to prepare_enable clock\n");
+ return ret;
+ }

platform_set_drvdata(pdev, hw);
- pm_runtime_enable(&pdev->dev);

- ret = hwspin_lock_register(&hw->bank, &pdev->dev, &stm32_hwspinlock_ops,
- 0, STM32_MUTEX_NUM_LOCKS);
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_runtime_put(dev);

- if (ret)
- pm_runtime_disable(&pdev->dev);
+ ret = devm_add_action_or_reset(dev, stm32_hwspinlock_disable_clk, pdev);
+ if (ret) {
+ dev_err(dev, "Failed to register action\n");
+ return ret;
+ }

- return ret;
-}
+ for (i = 0; i < STM32_MUTEX_NUM_LOCKS; i++)
+ hw->bank.lock[i].priv = io_base + i * sizeof(u32);

-static int stm32_hwspinlock_remove(struct platform_device *pdev)
-{
- struct stm32_hwspinlock *hw = platform_get_drvdata(pdev);
- int ret;
+ ret = devm_hwspin_lock_register(dev, &hw->bank, &stm32_hwspinlock_ops,
+ 0, STM32_MUTEX_NUM_LOCKS);

- ret = hwspin_lock_unregister(&hw->bank);
if (ret)
- dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
-
- pm_runtime_disable(&pdev->dev);
+ dev_err(dev, "Failed to register hwspinlock\n");

- return 0;
+ return ret;
}

static int __maybe_unused stm32_hwspinlock_runtime_suspend(struct device *dev)
@@ -135,7 +152,6 @@ MODULE_DEVICE_TABLE(of, stm32_hwpinlock_ids);

static struct platform_driver stm32_hwspinlock_driver = {
.probe = stm32_hwspinlock_probe,
- .remove = stm32_hwspinlock_remove,
.driver = {
.name = "stm32_hwspinlock",
.of_match_table = stm32_hwpinlock_ids,
--
2.25.1


2021-11-17 10:32:09

by Fabien Dessenne

[permalink] [raw]
Subject: Re: [PATCH v2] hwspinlock: stm32: enable clock at probe

Hi

Any comments on this patch?

BR

Fabien

On 11/10/2021 15:58, Fabien Dessenne wrote:
> Set the clock during probe and keep its control during suspend / resume
> operations.
> This fixes an issue when CONFIG_PM is not set and where the clock is
> never enabled.
>
> Make use of devm_ functions to simplify the code.
>
> Signed-off-by: Fabien Dessenne <[email protected]>
> ---
> Changes since v1:
> - Call platform_set_drvdata() before pm_runtime_...() calls
> ---
> drivers/hwspinlock/stm32_hwspinlock.c | 58 +++++++++++++++++----------
> 1 file changed, 37 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwspinlock/stm32_hwspinlock.c b/drivers/hwspinlock/stm32_hwspinlock.c
> index 3ad0ce0da4d9..5bd11a7fab65 100644
> --- a/drivers/hwspinlock/stm32_hwspinlock.c
> +++ b/drivers/hwspinlock/stm32_hwspinlock.c
> @@ -54,8 +54,23 @@ static const struct hwspinlock_ops stm32_hwspinlock_ops = {
> .relax = stm32_hwspinlock_relax,
> };
>
> +static void stm32_hwspinlock_disable_clk(void *data)
> +{
> + struct platform_device *pdev = data;
> + struct stm32_hwspinlock *hw = platform_get_drvdata(pdev);
> + struct device *dev = &pdev->dev;
> +
> + pm_runtime_get_sync(dev);
> + pm_runtime_disable(dev);
> + pm_runtime_set_suspended(dev);
> + pm_runtime_put_noidle(dev);
> +
> + clk_disable_unprepare(hw->clk);
> +}
> +
> static int stm32_hwspinlock_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> struct stm32_hwspinlock *hw;
> void __iomem *io_base;
> size_t array_size;
> @@ -66,41 +81,43 @@ static int stm32_hwspinlock_probe(struct platform_device *pdev)
> return PTR_ERR(io_base);
>
> array_size = STM32_MUTEX_NUM_LOCKS * sizeof(struct hwspinlock);
> - hw = devm_kzalloc(&pdev->dev, sizeof(*hw) + array_size, GFP_KERNEL);
> + hw = devm_kzalloc(dev, sizeof(*hw) + array_size, GFP_KERNEL);
> if (!hw)
> return -ENOMEM;
>
> - hw->clk = devm_clk_get(&pdev->dev, "hsem");
> + hw->clk = devm_clk_get(dev, "hsem");
> if (IS_ERR(hw->clk))
> return PTR_ERR(hw->clk);
>
> - for (i = 0; i < STM32_MUTEX_NUM_LOCKS; i++)
> - hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> + ret = clk_prepare_enable(hw->clk);
> + if (ret) {
> + dev_err(dev, "Failed to prepare_enable clock\n");
> + return ret;
> + }
>
> platform_set_drvdata(pdev, hw);
> - pm_runtime_enable(&pdev->dev);
>
> - ret = hwspin_lock_register(&hw->bank, &pdev->dev, &stm32_hwspinlock_ops,
> - 0, STM32_MUTEX_NUM_LOCKS);
> + pm_runtime_get_noresume(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> + pm_runtime_put(dev);
>
> - if (ret)
> - pm_runtime_disable(&pdev->dev);
> + ret = devm_add_action_or_reset(dev, stm32_hwspinlock_disable_clk, pdev);
> + if (ret) {
> + dev_err(dev, "Failed to register action\n");
> + return ret;
> + }
>
> - return ret;
> -}
> + for (i = 0; i < STM32_MUTEX_NUM_LOCKS; i++)
> + hw->bank.lock[i].priv = io_base + i * sizeof(u32);
>
> -static int stm32_hwspinlock_remove(struct platform_device *pdev)
> -{
> - struct stm32_hwspinlock *hw = platform_get_drvdata(pdev);
> - int ret;
> + ret = devm_hwspin_lock_register(dev, &hw->bank, &stm32_hwspinlock_ops,
> + 0, STM32_MUTEX_NUM_LOCKS);
>
> - ret = hwspin_lock_unregister(&hw->bank);
> if (ret)
> - dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
> -
> - pm_runtime_disable(&pdev->dev);
> + dev_err(dev, "Failed to register hwspinlock\n");
>
> - return 0;
> + return ret;
> }
>
> static int __maybe_unused stm32_hwspinlock_runtime_suspend(struct device *dev)
> @@ -135,7 +152,6 @@ MODULE_DEVICE_TABLE(of, stm32_hwpinlock_ids);
>
> static struct platform_driver stm32_hwspinlock_driver = {
> .probe = stm32_hwspinlock_probe,
> - .remove = stm32_hwspinlock_remove,
> .driver = {
> .name = "stm32_hwspinlock",
> .of_match_table = stm32_hwpinlock_ids,
>