Re: [PATCH V4 2/4] watchdog: stm32: add pclk feature for stm32mp1

From: Guenter Roeck
Date: Fri Jun 22 2018 - 09:40:39 EST

On 06/22/2018 02:15 AM, Ludovic BARRE wrote:


On 06/21/2018 06:53 PM, Guenter Roeck wrote:
On Thu, Jun 21, 2018 at 11:02:15AM +0200, Ludovic Barre wrote:
From: Ludovic Barre <ludovic.barre@xxxxxx>

This patch adds compatible data to manage pclk clock by
compatible. Adds stm32mp1 support which requires pclk clock.

Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
---
 drivers/watchdog/stm32_iwdg.c | 116 +++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 42 deletions(-)

diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index c97ad56..e00e3b3 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -11,12 +11,13 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.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/watchdog.h>
@@ -54,11 +55,15 @@
 #define TIMEOUT_US 100000
 #define SLEEP_US 1000
+#define HAS_PCLKÂÂÂ true
+
 struct stm32_iwdg {
ÂÂÂÂÂ struct watchdog_deviceÂÂÂ wdd;
ÂÂÂÂÂ void __iomemÂÂÂÂÂÂÂ *regs;
-ÂÂÂ struct clkÂÂÂÂÂÂÂ *clk;
+ÂÂÂ struct clkÂÂÂÂÂÂÂ *clk_lsi;
+ÂÂÂ struct clkÂÂÂÂÂÂÂ *clk_pclk;
ÂÂÂÂÂ unsigned intÂÂÂÂÂÂÂ rate;
+ÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ has_pclk;
 };
 static inline u32 reg_read(void __iomem *base, u32 reg)
@@ -133,6 +138,44 @@ static int stm32_iwdg_set_timeout(struct watchdog_device *wdd,
ÂÂÂÂÂ return 0;
 }
+static int stm32_iwdg_clk_init(struct platform_device *pdev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct stm32_iwdg *wdt)
+{
+ÂÂÂ u32 ret;
+
+ÂÂÂ wdt->clk_lsi = devm_clk_get(&pdev->dev, "lsi");

I just noticed a subtle difference: This used to be
ÂÂÂÂÂÂÂÂÂÂÂ devm_clk_get(&pdev->dev, NULL);
which would always get the first clock, no matter how it is named.
Can that cause problems with backward compatibility ?

You are right Guenter.

When there are multiple clocks, I prefer to use name interface.
I find it easier to use, and avoid misunderstanding.

Today, only one "dtsi" define the watchdog and it's disabled
(stm32f429.dtsi). I think it's good moment to move to clock named
(before it is used anymore).

But you are right I forgot to change stm32f429.dtsi.
If I add a commit for stm32f429.dtsi, it's Ok for you ?


Not really. You are imposing a personal preference on others,
and you would make stm32f429.dtsi inconsistent since it doesn't
use clock names for anything else. This in turn means that people
will have an endless source of irritation since they will need
a clock name for this node but not for others.

You will have to get the arm and DT maintainers to agree on this change.

Thanks,
Guenter

BR
Ludo


Thanks,
Guenter

+ÂÂÂ if (IS_ERR(wdt->clk_lsi)) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to get lsi clock\n");
+ÂÂÂÂÂÂÂ return PTR_ERR(wdt->clk_lsi);
+ÂÂÂ }
+
+ÂÂÂ /* optional peripheral clock */
+ÂÂÂ if (wdt->has_pclk) {
+ÂÂÂÂÂÂÂ wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk");
+ÂÂÂÂÂÂÂ if (IS_ERR(wdt->clk_pclk)) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to get pclk clock\n");
+ÂÂÂÂÂÂÂÂÂÂÂ return PTR_ERR(wdt->clk_pclk);
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ ret = clk_prepare_enable(wdt->clk_pclk);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to prepare pclk clock\n");
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ }
+
+ÂÂÂ ret = clk_prepare_enable(wdt->clk_lsi);
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to prepare lsi clock\n");
+ÂÂÂÂÂÂÂ clk_disable_unprepare(wdt->clk_pclk);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ wdt->rate = clk_get_rate(wdt->clk_lsi);
+
+ÂÂÂ return 0;
+}
+
 static const struct watchdog_info stm32_iwdg_info = {
ÂÂÂÂÂ .optionsÂÂÂ = WDIOF_SETTIMEOUT |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ WDIOF_MAGICCLOSE |
@@ -147,49 +190,42 @@ static const struct watchdog_ops stm32_iwdg_ops = {
ÂÂÂÂÂ .set_timeoutÂÂÂ = stm32_iwdg_set_timeout,
 };
+static const struct of_device_id stm32_iwdg_of_match[] = {
+ÂÂÂ { .compatible = "st,stm32-iwdg", .data = (void *)!HAS_PCLK },
+ÂÂÂ { .compatible = "st,stm32mp1-iwdg", .data = (void *)HAS_PCLK },
+ÂÂÂ { /* end node */ }
+};
+MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
+
 static int stm32_iwdg_probe(struct platform_device *pdev)
 {
ÂÂÂÂÂ struct watchdog_device *wdd;
+ÂÂÂ const struct of_device_id *match;
ÂÂÂÂÂ struct stm32_iwdg *wdt;
ÂÂÂÂÂ struct resource *res;
-ÂÂÂ void __iomem *regs;
-ÂÂÂ struct clk *clk;
ÂÂÂÂÂ int ret;
+ÂÂÂ match = of_match_device(stm32_iwdg_of_match, &pdev->dev);
+ÂÂÂ if (!match)
+ÂÂÂÂÂÂÂ return -ENODEV;
+
+ÂÂÂ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ÂÂÂ if (!wdt)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ wdt->has_pclk = match->data;
+
ÂÂÂÂÂ /* This is the timer base. */
ÂÂÂÂÂ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-ÂÂÂ regs = devm_ioremap_resource(&pdev->dev, res);
-ÂÂÂ if (IS_ERR(regs)) {
+ÂÂÂ wdt->regs = devm_ioremap_resource(&pdev->dev, res);
+ÂÂÂ if (IS_ERR(wdt->regs)) {
ÂÂÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Could not get resource\n");
-ÂÂÂÂÂÂÂ return PTR_ERR(regs);
+ÂÂÂÂÂÂÂ return PTR_ERR(wdt->regs);
ÂÂÂÂÂ }
-ÂÂÂ clk = devm_clk_get(&pdev->dev, NULL);
-ÂÂÂ if (IS_ERR(clk)) {
-ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to get clock\n");
-ÂÂÂÂÂÂÂ return PTR_ERR(clk);
-ÂÂÂ }
-
-ÂÂÂ ret = clk_prepare_enable(clk);
-ÂÂÂ if (ret) {
-ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "Unable to prepare clock %p\n", clk);
+ÂÂÂ ret = stm32_iwdg_clk_init(pdev, wdt);
+ÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂ return ret;
-ÂÂÂ }
-
-ÂÂÂ /*
-ÂÂÂÂ * Allocate our watchdog driver data, which has the
-ÂÂÂÂ * struct watchdog_device nested within it.
-ÂÂÂÂ */
-ÂÂÂ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
-ÂÂÂ if (!wdt) {
-ÂÂÂÂÂÂÂ ret = -ENOMEM;
-ÂÂÂÂÂÂÂ goto err;
-ÂÂÂ }
-
-ÂÂÂ /* Initialize struct stm32_iwdg. */
-ÂÂÂ wdt->regs = regs;
-ÂÂÂ wdt->clk = clk;
-ÂÂÂ wdt->rate = clk_get_rate(clk);
ÂÂÂÂÂ /* Initialize struct watchdog_device. */
ÂÂÂÂÂ wdd = &wdt->wdd;
@@ -217,7 +253,8 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
ÂÂÂÂÂ return 0;
 err:
-ÂÂÂ clk_disable_unprepare(clk);
+ÂÂÂ clk_disable_unprepare(wdt->clk_lsi);
+ÂÂÂ clk_disable_unprepare(wdt->clk_pclk);
ÂÂÂÂÂ return ret;
 }
@@ -227,23 +264,18 @@ static int stm32_iwdg_remove(struct platform_device *pdev)
ÂÂÂÂÂ struct stm32_iwdg *wdt = platform_get_drvdata(pdev);
ÂÂÂÂÂ watchdog_unregister_device(&wdt->wdd);
-ÂÂÂ clk_disable_unprepare(wdt->clk);
+ÂÂÂ clk_disable_unprepare(wdt->clk_lsi);
+ÂÂÂ clk_disable_unprepare(wdt->clk_pclk);
ÂÂÂÂÂ return 0;
 }
-static const struct of_device_id stm32_iwdg_of_match[] = {
-ÂÂÂ { .compatible = "st,stm32-iwdg" },
-ÂÂÂ { /* end node */ }
-};
-MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match);
-
 static struct platform_driver stm32_iwdg_driver = {
ÂÂÂÂÂ .probeÂÂÂÂÂÂÂ = stm32_iwdg_probe,
ÂÂÂÂÂ .removeÂÂÂÂÂÂÂ = stm32_iwdg_remove,
ÂÂÂÂÂ .driver = {
ÂÂÂÂÂÂÂÂÂ .nameÂÂÂ = "iwdg",
-ÂÂÂÂÂÂÂ .of_match_table = stm32_iwdg_of_match,
+ÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(stm32_iwdg_of_match),
ÂÂÂÂÂ },
 };
 module_platform_driver(stm32_iwdg_driver);
--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html