Re: [PATCH V2 1/3] watchdog: stm32: add pclk feature for stm32mp1

Ludovic BARRE <ludovic.barre@xxxxxx> · Wed, 20 Jun 2018 15:24:07 +0200

On 06/20/2018 11:19 AM, Guenter Roeck wrote:
On 06/20/2018 12:53 AM, Ludovic Barre wrote:
From: Ludovic Barre <ludovic.barre@xxxxxx>

This patch adds config data to manage specific properties by
compatible. Adds stm32mp1 config which requires pclk clock.

Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
---
  .../devicetree/bindings/watchdog/st,stm32-iwdg.txt |  21 +++-
  drivers/watchdog/stm32_iwdg.c                      | 132 
++++++++++++++-------
  2 files changed, 104 insertions(+), 49 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt 
b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
index cc13b10a..f07f6d89 100644
--- a/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
+++ b/Documentation/devicetree/bindings/watchdog/st,stm32-iwdg.txt
@@ -2,18 +2,31 @@ STM32 Independent WatchDoG (IWDG)
  ---------------------------------
  Required properties:
-- compatible: "st,stm32-iwdg"
-- reg: physical base address and length of the registers set for the 
device
-- clocks: must contain a single entry describing the clock input
+- compatible: Should be either "st,stm32-iwdg" or "st,stm32mp1-iwdg"
+- reg: Physical base address and length of the registers set for the 
device
+- clocks: Reference to the clock entry lsi. Additional pclk clock entry
+  is required only for st,stm32mp1-iwdg.
+- clock-names: Name of the clocks used.
+  "lsi" for st,stm32-iwdg
+  "pclk", "lsi" for st,stm32mp1-iwdg
  Optional Properties:
  - timeout-sec: Watchdog timeout value in seconds.
-Example:
+Examples:
  iwdg: watchdog@40003000 {
      compatible = "st,stm32-iwdg";
      reg = <0x40003000 0x400>;
      clocks = <&clk_lsi>;
+    clock-names = "lsi";
+    timeout-sec = <32>;
+};
+
+iwdg: iwdg@5a002000 {
+    compatible = "st,stm32mp1-iwdg";
+    reg = <0x5a002000 0x400>;
+    clocks = <&rcc IWDG2>, <&clk_lsi>;
+    clock-names = "pclk", "lsi";
      timeout-sec = <32>;
  };
diff --git a/drivers/watchdog/stm32_iwdg.c 
b/drivers/watchdog/stm32_iwdg.c
index c97ad56..fc96670 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,17 @@
  #define TIMEOUT_US    100000
  #define SLEEP_US    1000
+struct stm32_iwdg_config {
+    bool has_pclk;
+};
+

This data structure is unnecessary. Just assign the boolean directly to 
.data
and ...


Ok, I send a v3, with boolean directly to .data
like:

#define NO_PCLK		false
#define HAS_PCLK	true
...
static const struct of_device_id stm32_iwdg_of_match[] = {
	{ .compatible = "st,stm32-iwdg", .data = (void *) NO_PCLK },
	{ .compatible = "st,stm32mp1-iwdg", .data = (void *) HAS_PCLK },
	{ /* end node */ }
};

Note:
V3, because I sent my original version with V2
(it's mistake)

  struct stm32_iwdg {
-    struct watchdog_device    wdd;
-    void __iomem        *regs;
-    struct clk        *clk;
-    unsigned int        rate;
+    struct watchdog_device        wdd;
+    void __iomem            *regs;
+    struct stm32_iwdg_config    *config;

declare bool has_pclk here.

+    struct clk            *clk_lsi;
+    struct clk            *clk_pclk;
+    unsigned int            rate;
  };
  static inline u32 reg_read(void __iomem *base, u32 reg)
@@ -133,6 +140,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");
+    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->config->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 +192,50 @@ static const struct watchdog_ops stm32_iwdg_ops = {
      .set_timeout    = stm32_iwdg_set_timeout,
  };
+static const struct stm32_iwdg_config stm32_iwdg_cfg = {
+    .has_pclk = false,
+};
+
+static const struct stm32_iwdg_config stm32mp1_iwdg_cfg = {
+    .has_pclk = true,
+};
+
+static const struct of_device_id stm32_iwdg_of_match[] = {
+    { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_cfg },
+    { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_cfg },
+    { /* 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 || !match->data)
+        return -ENODEV;
+
+    wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+    if (!wdt)
+        return -ENOMEM;
+
+    wdt->config = (struct stm32_iwdg_config *)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 +263,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 +274,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);


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






