[PATCH V2 0/3] add iwdg2 support for stm32mp157c

[PATCH V2 0/3] add iwdg2 support for stm32mp157c

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

This patch series updates stm32_iwdg driver to manage config
by compatible. stm32mp1 config requires a pclk clock.

Ludovic Barre (3):
  watchdog: stm32: add pclk feature for stm32mp1
  ARM: dts: stm32: add iwdg2 support for stm32mp157c
  ARM: dts: stm32: add iwdg2 support for stm32mp157c-ed1

 .../devicetree/bindings/watchdog/st,stm32-iwdg.txt |  21 +++-
 arch/arm/boot/dts/stm32mp157c-ed1.dts              |   5 +
 arch/arm/boot/dts/stm32mp157c.dtsi                 |   8 ++
 drivers/watchdog/stm32_iwdg.c                      | 132 ++++++++++++++-------
 4 files changed, 117 insertions(+), 49 deletions(-)

-- 
2.7.4

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

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

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@st.com>
---
 .../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;
+};
+
 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;
+	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);
-- 
2.7.4

[PATCH V2 2/3] ARM: dts: stm32: add iwdg2 support for stm32mp157c

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

This patch adds independent watchdog support for stm32mp157c.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 arch/arm/boot/dts/stm32mp157c.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 7d17538..95cc166 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -784,6 +784,14 @@
 			status = "disabled";
 		};
 
+		iwdg2: watchdog@5a002000 {
+			compatible = "st,stm32mp1-iwdg";
+			reg = <0x5a002000 0x400>;
+			clocks = <&rcc IWDG2>, <&rcc CK_LSI>;
+			clock-names = "pclk", "lsi";
+			status = "disabled";
+		};
+
 		usbphyc: usbphyc@5a006000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
2.7.4

[PATCH V2 3/3] ARM: dts: stm32: add iwdg2 support for stm32mp157c-ed1

[Ludovic Barre]

From: Ludovic Barre <ludovic.barre@st.com>

This patch activates independent watchdog support for
stm32mp157c board.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 arch/arm/boot/dts/stm32mp157c-ed1.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp157c-ed1.dts b/arch/arm/boot/dts/stm32mp157c-ed1.dts
index ae33653..8af263a 100644
--- a/arch/arm/boot/dts/stm32mp157c-ed1.dts
+++ b/arch/arm/boot/dts/stm32mp157c-ed1.dts
@@ -68,6 +68,11 @@
 	status = "okay";
 };
 
+&iwdg2 {
+	timeout-sec = <32>;
+	status = "okay";
+};
+
 &uart4 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart4_pins_a>;
-- 
2.7.4

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

[Guenter Roeck]

On 06/20/2018 12:53 AM, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
> 
> 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@st.com>
> ---
>   .../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 ...

>   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);
> 

Re: [PATCH V2 0/3] add iwdg2 support for stm32mp157c

[Alexandre Torgue]

Hi Ludovic

On 06/20/2018 09:53 AM, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
> 
> This patch series updates stm32_iwdg driver to manage config
> by compatible. stm32mp1 config requires a pclk clock.
> 
> Ludovic Barre (3):
>    watchdog: stm32: add pclk feature for stm32mp1
>    ARM: dts: stm32: add iwdg2 support for stm32mp157c
>    ARM: dts: stm32: add iwdg2 support for stm32mp157c-ed1
> 
>   .../devicetree/bindings/watchdog/st,stm32-iwdg.txt |  21 +++-
>   arch/arm/boot/dts/stm32mp157c-ed1.dts              |   5 +
>   arch/arm/boot/dts/stm32mp157c.dtsi                 |   8 ++
>   drivers/watchdog/stm32_iwdg.c                      | 132 ++++++++++++++-------
>   4 files changed, 117 insertions(+), 49 deletions(-)
> 

STM32 DT patches looks good to me. I will take them as soon as driver 
part is acked.

Regards
Alex

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

[Ludovic BARRE]

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@st.com>
>>
>> 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@st.com>
>> ---
>>   .../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);
>>
> 

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

[Guenter Roeck]

On 06/20/2018 06:24 AM, Ludovic BARRE wrote:
> 
> 
> 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@st.com>
>>>
>>> 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@st.com>
>>> ---
>>>   .../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
> ...

Just use true/false directly. There is no need for those defines.

If you want the reader to understand what is defined, I would be ok with

#define HAS_PCLK    true

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 */ }

Guenter

> 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);
>>>
>>
> 

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

[Ludovic BARRE]

On 06/20/2018 03:29 PM, Guenter Roeck wrote:
> On 06/20/2018 06:24 AM, Ludovic BARRE wrote:
>>
>>
>> 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@st.com>
>>>>
>>>> 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@st.com>
>>>> ---
>>>>   .../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
>> ...
> 
> Just use true/false directly. There is no need for those defines.
> 
> If you want the reader to understand what is defined, I would be ok with
> 
> #define HAS_PCLK    true
> 
> 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 */ }
> 
> Guenter

Ok, thanks

> 
>> 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);
>>>>
>>>
>>
>