[PATCHv2] watchdog: xilinx: Add clock support

[PATCHv2] watchdog: xilinx: Add clock support

[Shubhrajyoti Datta]

Add support for the clock. Currently we enable
at probe and relinquish at remove.

Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
v2:
fix spaces and error path.

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  3 +++
 drivers/watchdog/of_xilinx_wdt.c                   | 23 ++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
index 6d63782..b0a9fa3 100644
--- a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
@@ -5,6 +5,8 @@ Required properties:
 - compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
 			  "xlnx,xps-timebase-wdt-1.01.a".
 - reg			: Physical base address and size
+- clocks		: Input clock specifier. Refer to common clock
+			  bindings.
 
 Optional properties:
 - clock-frequency	: Frequency of clock in Hz
@@ -17,6 +19,7 @@ Example:
 axi-timebase-wdt@40100000 {
 	clock-frequency = <50000000>;
 	compatible = "xlnx,xps-timebase-wdt-1.00.a";
+	clocks = <&clkc 15>;
 	reg = <0x40100000 0x10000>;
 	xlnx,wdt-enable-once = <0x0>;
 	xlnx,wdt-interval = <0x1b>;
diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index b2e1b4c..a225e43 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -10,6 +10,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -45,6 +46,7 @@ struct xwdt_device {
 	u32 wdt_interval;
 	spinlock_t spinlock;
 	struct watchdog_device xilinx_wdt_wdd;
+	struct clk		*clk;
 };
 
 static int xilinx_wdt_start(struct watchdog_device *wdd)
@@ -194,17 +196,29 @@ static int xwdt_probe(struct platform_device *pdev)
 
 	spin_lock_init(&xdev->spinlock);
 	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
+	xdev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(xdev->clk)) {
+		dev_err(&pdev->dev, "input clock not found\n");
+		rc = PTR_ERR(xdev->clk);
+		return rc;
+	}
+
+	rc = clk_prepare_enable(xdev->clk);
+	if (rc) {
+		dev_err(&pdev->dev, "unable to enable clock\n");
+		return rc;
+	}
 
 	rc = xwdt_selftest(xdev);
 	if (rc == XWT_TIMER_FAILED) {
 		dev_err(&pdev->dev, "SelfTest routine error\n");
-		return rc;
+		goto err_clk_disable;
 	}
 
 	rc = watchdog_register_device(xilinx_wdt_wdd);
 	if (rc) {
 		dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n", rc);
-		return rc;
+		goto err_clk_disable;
 	}
 
 	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
@@ -213,6 +227,10 @@ static int xwdt_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, xdev);
 
 	return 0;
+err_clk_disable:
+	clk_disable_unprepare(xdev->clk);
+
+	return rc;
 }
 
 static int xwdt_remove(struct platform_device *pdev)
@@ -220,6 +238,7 @@ static int xwdt_remove(struct platform_device *pdev)
 	struct xwdt_device *xdev = platform_get_drvdata(pdev);
 
 	watchdog_unregister_device(&xdev->xilinx_wdt_wdd);
+	clk_disable_unprepare(xdev->clk);
 
 	return 0;
 }
-- 
2.1.1

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

Re: [PATCHv2] watchdog: xilinx: Add clock support

[Guenter Roeck]

On 08/02/2016 09:28 PM, Shubhrajyoti Datta wrote:
> Add support for the clock. Currently we enable
> at probe and relinquish at remove.
>
> Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

Patch itself looks good.

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

Do you have a separate patch to add the clock to virtex440-ml507.dts ?

Not that it matters much, I guess - virtex440-ml507.dts says
		compatible = "xlnx,xps-timebase-wdt-1.00.b";
which isn't supported by of_xilinx_wdt.c, and not documented in
of-xilinx-wdt.txt. Is that an oversight or on purpose ?

Thanks,
Guenter

> ---
> v2:
> fix spaces and error path.
>
>   .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  3 +++
>   drivers/watchdog/of_xilinx_wdt.c                   | 23 ++++++++++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> index 6d63782..b0a9fa3 100644
> --- a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> @@ -5,6 +5,8 @@ Required properties:
>   - compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
>   			  "xlnx,xps-timebase-wdt-1.01.a".
>   - reg			: Physical base address and size
> +- clocks		: Input clock specifier. Refer to common clock
> +			  bindings.
>
>   Optional properties:
>   - clock-frequency	: Frequency of clock in Hz
> @@ -17,6 +19,7 @@ Example:
>   axi-timebase-wdt@40100000 {
>   	clock-frequency = <50000000>;
>   	compatible = "xlnx,xps-timebase-wdt-1.00.a";
> +	clocks = <&clkc 15>;
>   	reg = <0x40100000 0x10000>;
>   	xlnx,wdt-enable-once = <0x0>;
>   	xlnx,wdt-interval = <0x1b>;
> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
> index b2e1b4c..a225e43 100644
> --- a/drivers/watchdog/of_xilinx_wdt.c
> +++ b/drivers/watchdog/of_xilinx_wdt.c
> @@ -10,6 +10,7 @@
>    * 2 of the License, or (at your option) any later version.
>    */
>
> +#include <linux/clk.h>
>   #include <linux/err.h>
>   #include <linux/module.h>
>   #include <linux/types.h>
> @@ -45,6 +46,7 @@ struct xwdt_device {
>   	u32 wdt_interval;
>   	spinlock_t spinlock;
>   	struct watchdog_device xilinx_wdt_wdd;
> +	struct clk		*clk;
>   };
>
>   static int xilinx_wdt_start(struct watchdog_device *wdd)
> @@ -194,17 +196,29 @@ static int xwdt_probe(struct platform_device *pdev)
>
>   	spin_lock_init(&xdev->spinlock);
>   	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
> +	xdev->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(xdev->clk)) {
> +		dev_err(&pdev->dev, "input clock not found\n");
> +		rc = PTR_ERR(xdev->clk);
> +		return rc;
> +	}
> +
> +	rc = clk_prepare_enable(xdev->clk);
> +	if (rc) {
> +		dev_err(&pdev->dev, "unable to enable clock\n");
> +		return rc;
> +	}
>
>   	rc = xwdt_selftest(xdev);
>   	if (rc == XWT_TIMER_FAILED) {
>   		dev_err(&pdev->dev, "SelfTest routine error\n");
> -		return rc;
> +		goto err_clk_disable;
>   	}
>
>   	rc = watchdog_register_device(xilinx_wdt_wdd);
>   	if (rc) {
>   		dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n", rc);
> -		return rc;
> +		goto err_clk_disable;
>   	}
>
>   	dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout %ds\n",
> @@ -213,6 +227,10 @@ static int xwdt_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, xdev);
>
>   	return 0;
> +err_clk_disable:
> +	clk_disable_unprepare(xdev->clk);
> +
> +	return rc;
>   }
>
>   static int xwdt_remove(struct platform_device *pdev)
> @@ -220,6 +238,7 @@ static int xwdt_remove(struct platform_device *pdev)
>   	struct xwdt_device *xdev = platform_get_drvdata(pdev);
>
>   	watchdog_unregister_device(&xdev->xilinx_wdt_wdd);
> +	clk_disable_unprepare(xdev->clk);
>
>   	return 0;
>   }
>

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

Re: [PATCHv2] watchdog: xilinx: Add clock support

[Michal Simek]

Hi Guenter,

On 3.8.2016 18:23, Guenter Roeck wrote:
> On 08/02/2016 09:28 PM, Shubhrajyoti Datta wrote:
>> Add support for the clock. Currently we enable
>> at probe and relinquish at remove.
>>
>> Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 
> Patch itself looks good.
> 
> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> 
> Do you have a separate patch to add the clock to virtex440-ml507.dts ?
> 
> Not that it matters much, I guess - virtex440-ml507.dts says
>         compatible = "xlnx,xps-timebase-wdt-1.00.b";
> which isn't supported by of_xilinx_wdt.c, and not documented in
> of-xilinx-wdt.txt. Is that an oversight or on purpose ?

We had an internal discussion about ppc405 and ppc440 support and truth
is Xilinx is not doing any testing on these cpus and the part of that is
that none is keeping stuff up2date.

I tried to remove xilinx ppc support from U-Boot because I didn't see
activity there but there were people who invested their time to do
necessary changes to keep it alive. I think kernel is just a same case.
I am happy to remove these xilinx ppc4xx dtses from the kernel but I
expect that there will be others who want to have it there.
And this is the group who should keep these dtses up2date and test them
regularly.

Thanks,
Michal

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

Re: [PATCHv2] watchdog: xilinx: Add clock support

[Rob Herring]

On Wed, Aug 03, 2016 at 09:58:38AM +0530, Shubhrajyoti Datta wrote:
> Add support for the clock. Currently we enable
> at probe and relinquish at remove.
> 
> Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> fix spaces and error path.
> 
>  .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  3 +++
>  drivers/watchdog/of_xilinx_wdt.c                   | 23 ++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> index 6d63782..b0a9fa3 100644
> --- a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
> @@ -5,6 +5,8 @@ Required properties:
>  - compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
>  			  "xlnx,xps-timebase-wdt-1.01.a".
>  - reg			: Physical base address and size
> +- clocks		: Input clock specifier. Refer to common clock
> +			  bindings.

You can't add new required properties. Doing so breaks compatibility 
with old DTs.

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

Re: [PATCHv2] watchdog: xilinx: Add clock support

[Shubhrajyoti Datta]

On Thu, Aug 4, 2016 at 10:59 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Aug 03, 2016 at 09:58:38AM +0530, Shubhrajyoti Datta wrote:
>> Add support for the clock. Currently we enable
>> at probe and relinquish at remove.
>>
>> Acked-by: Sören Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> ---
>> v2:
>> fix spaces and error path.
>>
>>  .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  3 +++
>>  drivers/watchdog/of_xilinx_wdt.c                   | 23 ++++++++++++++++++++--
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> index 6d63782..b0a9fa3 100644
>> --- a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> @@ -5,6 +5,8 @@ Required properties:
>>  - compatible         : Should be "xlnx,xps-timebase-wdt-1.00.a" or
>>                         "xlnx,xps-timebase-wdt-1.01.a".
>>  - reg                        : Physical base address and size
>> +- clocks             : Input clock specifier. Refer to common clock
>> +                       bindings.
>
> You can't add new required properties. Doing so breaks compatibility
> with old DTs.
Will resend fixing the compatibility with old dts.

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