[PATCH 0/3] Add SNVS clock gating and clock support to rtc-snvs driver

[PATCH 0/3] Add SNVS clock gating and clock support to rtc-snvs driver

[Sanchayan Maity]

The following series of patches:
(1) Enable clock gating for the SNVS peripheral
(2) Adds SNVS devicetree node for the VF610 platform
(3) Add clock enable and disable support for the 
SNVS peripheral which is required by RTC. 

I would like to ask whether the solution i am
opting for in these set of patches is ok? Is there
another more appropriate solution? I couldn't find
any other piece of code which was enabling the clock
gating for the SNVS peripheral explicitly. Is this 
expected to be taken care of by uboot?

Sanchayan Maity (3):
  ARM: imx: clk-vf610: Add SNVS clock
  ARM: dts: vf610: Add SNVS node
  drivers/rtc/rtc-snvs: Add clock support

 arch/arm/boot/dts/vf610.dtsi            |   15 ++++++++++
 arch/arm/mach-imx/clk-vf610.c           |    1 +
 drivers/rtc/rtc-snvs.c                  |   48 +++++++++++++++++++++++++------
 include/dt-bindings/clock/vf610-clock.h |    3 +-
 4 files changed, 57 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 0/3] Add SNVS clock gating and clock support to rtc-snvs driver

[Sanchayan Maity]

The following series of patches:
(1) Enable clock gating for the SNVS peripheral
(2) Adds SNVS devicetree node for the VF610 platform
(3) Add clock enable and disable support for the 
SNVS peripheral which is required by RTC. 

I would like to ask whether the solution i am
opting for in these set of patches is ok? Is there
another more appropriate solution? I couldn't find
any other piece of code which was enabling the clock
gating for the SNVS peripheral explicitly. Is this 
expected to be taken care of by uboot?

Sanchayan Maity (3):
  ARM: imx: clk-vf610: Add SNVS clock
  ARM: dts: vf610: Add SNVS node
  drivers/rtc/rtc-snvs: Add clock support

 arch/arm/boot/dts/vf610.dtsi            |   15 ++++++++++
 arch/arm/mach-imx/clk-vf610.c           |    1 +
 drivers/rtc/rtc-snvs.c                  |   48 +++++++++++++++++++++++++------
 include/dt-bindings/clock/vf610-clock.h |    3 +-
 4 files changed, 57 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] ARM: imx: clk-vf610: Add SNVS clock

[Sanchayan Maity]

This patch adds the SNVS clock gating which is
required by the SNVS peripheral.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/mach-imx/clk-vf610.c           |    1 +
 include/dt-bindings/clock/vf610-clock.h |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/clk-vf610.c b/arch/arm/mach-imx/clk-vf610.c
index a178184..2a3a759f 100644
--- a/arch/arm/mach-imx/clk-vf610.c
+++ b/arch/arm/mach-imx/clk-vf610.c
@@ -318,6 +318,7 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)
 	clk[VF610_CLK_DMAMUX1] = imx_clk_gate2("dmamux1", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(5));
 	clk[VF610_CLK_DMAMUX2] = imx_clk_gate2("dmamux2", "platform_bus", CCM_CCGR6, CCM_CCGRx_CGn(1));
 	clk[VF610_CLK_DMAMUX3] = imx_clk_gate2("dmamux3", "platform_bus", CCM_CCGR6, CCM_CCGRx_CGn(2));
+	clk[VF610_CLK_SNVS] = imx_clk_gate2("snvs", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));
 
 	imx_check_clocks(clk, ARRAY_SIZE(clk));
 
diff --git a/include/dt-bindings/clock/vf610-clock.h b/include/dt-bindings/clock/vf610-clock.h
index d6b56b2..358eab9 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -169,6 +169,7 @@
 #define VF610_CLK_PLL7_MAIN		156
 #define VF610_CLK_USBPHY0		157
 #define VF610_CLK_USBPHY1		158
-#define VF610_CLK_END			159
+#define VF610_CLK_SNVS			159
+#define VF610_CLK_END			160
 
 #endif /* __DT_BINDINGS_CLOCK_VF610_H */
-- 
1.7.9.5

[PATCH 1/3] ARM: imx: clk-vf610: Add SNVS clock

[Sanchayan Maity]

This patch adds the SNVS clock gating which is
required by the SNVS peripheral.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/mach-imx/clk-vf610.c           |    1 +
 include/dt-bindings/clock/vf610-clock.h |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/clk-vf610.c b/arch/arm/mach-imx/clk-vf610.c
index a178184..2a3a759f 100644
--- a/arch/arm/mach-imx/clk-vf610.c
+++ b/arch/arm/mach-imx/clk-vf610.c
@@ -318,6 +318,7 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)
 	clk[VF610_CLK_DMAMUX1] = imx_clk_gate2("dmamux1", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(5));
 	clk[VF610_CLK_DMAMUX2] = imx_clk_gate2("dmamux2", "platform_bus", CCM_CCGR6, CCM_CCGRx_CGn(1));
 	clk[VF610_CLK_DMAMUX3] = imx_clk_gate2("dmamux3", "platform_bus", CCM_CCGR6, CCM_CCGRx_CGn(2));
+	clk[VF610_CLK_SNVS] = imx_clk_gate2("snvs", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));
 
 	imx_check_clocks(clk, ARRAY_SIZE(clk));
 
diff --git a/include/dt-bindings/clock/vf610-clock.h b/include/dt-bindings/clock/vf610-clock.h
index d6b56b2..358eab9 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -169,6 +169,7 @@
 #define VF610_CLK_PLL7_MAIN		156
 #define VF610_CLK_USBPHY0		157
 #define VF610_CLK_USBPHY1		158
-#define VF610_CLK_END			159
+#define VF610_CLK_SNVS			159
+#define VF610_CLK_END			160
 
 #endif /* __DT_BINDINGS_CLOCK_VF610_H */
-- 
1.7.9.5

[PATCH 2/3] ARM: dts: vf610: Add SNVS node

[Sanchayan Maity]

This adds a devicetree node for RTC support
for the VF610 platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vf610.dtsi |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 4d2ec32..3c0c757 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -381,6 +381,21 @@
 					<&clks VF610_CLK_DMAMUX3>;
 			};
 
+			snvs0: snvs@400a7000 {
+				compatible = "fsl,sec-v4.0-mon", "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0x400a7000 0x2000>;
+
+				snvs-rtc-lp@34 {
+					compatible = "fsl,sec-v4.0-mon-rtc-lp";
+					reg = <0x34 0x58>;
+					interrupts = <0 100 IRQ_TYPE_LEVEL_HIGH>;
+					clocks = <&clks VF610_CLK_SNVS>;
+					clock-names = "snvs";
+				};
+			};
+
 			uart4: serial@400a9000 {
 				compatible = "fsl,vf610-lpuart";
 				reg = <0x400a9000 0x1000>;
-- 
1.7.9.5

[PATCH 2/3] ARM: dts: vf610: Add SNVS node

[Sanchayan Maity]

This adds a devicetree node for RTC support
for the VF610 platform.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vf610.dtsi |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 4d2ec32..3c0c757 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -381,6 +381,21 @@
 					<&clks VF610_CLK_DMAMUX3>;
 			};
 
+			snvs0: snvs at 400a7000 {
+				compatible = "fsl,sec-v4.0-mon", "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				ranges = <0 0x400a7000 0x2000>;
+
+				snvs-rtc-lp at 34 {
+					compatible = "fsl,sec-v4.0-mon-rtc-lp";
+					reg = <0x34 0x58>;
+					interrupts = <0 100 IRQ_TYPE_LEVEL_HIGH>;
+					clocks = <&clks VF610_CLK_SNVS>;
+					clock-names = "snvs";
+				};
+			};
+
 			uart4: serial at 400a9000 {
 				compatible = "fsl,vf610-lpuart";
 				reg = <0x400a9000 0x1000>;
-- 
1.7.9.5

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Sanchayan Maity]

This patch adds clock enable and disable support
for the SNVS peripheral which is required by the
SNVS RTC.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index fa384fe..f3e8f05 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/rtc.h>
+#include <linux/clk.h>
 
 /* These register offsets are relative to LP (Low Power) range */
 #define SNVS_LPCR		0x04
@@ -39,6 +40,7 @@ struct snvs_rtc_data {
 	void __iomem *ioaddr;
 	int irq;
 	spinlock_t lock;
+	struct clk *clk;
 };
 
 static u32 rtc_read_lp_counter(void __iomem *ioaddr)
@@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	if (data->irq < 0)
 		return data->irq;
 
+	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
+			       IRQF_SHARED, "rtc alarm", &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+			data->irq, ret);
+		return ret;
+	}
+
+	data->clk = devm_clk_get(&pdev->dev, "snvs");
+	if (IS_ERR(data->clk)) {
+		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
+						PTR_ERR(data->clk));
+		ret = PTR_ERR(data->clk);
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, data);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Could not prepare or enable the snvs clock\n");
+		return ret;
+	}
+
 	spin_lock_init(&data->lock);
 
 	/* Initialize glitch detect */
@@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, true);
 
-	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
-			       IRQF_SHARED, "rtc alarm", &pdev->dev);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
-			data->irq, ret);
-		return ret;
-	}
-
 	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 					&snvs_rtc_ops, THIS_MODULE);
 	if (IS_ERR(data->rtc)) {
 		ret = PTR_ERR(data->rtc);
 		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
-		return ret;
+		goto error_rtc_device_register;
 	}
 
 	return 0;
+
+error_rtc_device_register:
+	clk_disable_unprepare(data->clk);
+
+	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
 	if (device_may_wakeup(dev))
 		enable_irq_wake(data->irq);
 
+	clk_disable_unprepare(data->clk);
+
 	return 0;
 }
 
 static int snvs_rtc_resume(struct device *dev)
 {
+	int ret;
+
 	struct snvs_rtc_data *data = dev_get_drvdata(dev);
 
 	if (device_may_wakeup(dev))
 		disable_irq_wake(data->irq);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 #endif
-- 
1.7.9.5

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Sanchayan Maity]

This patch adds clock enable and disable support
for the SNVS peripheral which is required by the
SNVS RTC.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index fa384fe..f3e8f05 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/rtc.h>
+#include <linux/clk.h>
 
 /* These register offsets are relative to LP (Low Power) range */
 #define SNVS_LPCR		0x04
@@ -39,6 +40,7 @@ struct snvs_rtc_data {
 	void __iomem *ioaddr;
 	int irq;
 	spinlock_t lock;
+	struct clk *clk;
 };
 
 static u32 rtc_read_lp_counter(void __iomem *ioaddr)
@@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	if (data->irq < 0)
 		return data->irq;
 
+	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
+			       IRQF_SHARED, "rtc alarm", &pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+			data->irq, ret);
+		return ret;
+	}
+
+	data->clk = devm_clk_get(&pdev->dev, "snvs");
+	if (IS_ERR(data->clk)) {
+		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
+						PTR_ERR(data->clk));
+		ret = PTR_ERR(data->clk);
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, data);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Could not prepare or enable the snvs clock\n");
+		return ret;
+	}
+
 	spin_lock_init(&data->lock);
 
 	/* Initialize glitch detect */
@@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, true);
 
-	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
-			       IRQF_SHARED, "rtc alarm", &pdev->dev);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
-			data->irq, ret);
-		return ret;
-	}
-
 	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
 					&snvs_rtc_ops, THIS_MODULE);
 	if (IS_ERR(data->rtc)) {
 		ret = PTR_ERR(data->rtc);
 		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
-		return ret;
+		goto error_rtc_device_register;
 	}
 
 	return 0;
+
+error_rtc_device_register:
+	clk_disable_unprepare(data->clk);
+
+	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
 	if (device_may_wakeup(dev))
 		enable_irq_wake(data->irq);
 
+	clk_disable_unprepare(data->clk);
+
 	return 0;
 }
 
 static int snvs_rtc_resume(struct device *dev)
 {
+	int ret;
+
 	struct snvs_rtc_data *data = dev_get_drvdata(dev);
 
 	if (device_may_wakeup(dev))
 		disable_irq_wake(data->irq);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 #endif
-- 
1.7.9.5

Re: [PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Shawn Guo]

On Fri, Sep 26, 2014 at 06:44:01PM +0530, Sanchayan Maity wrote:
> This patch adds clock enable and disable support
> for the SNVS peripheral which is required by the
> SNVS RTC.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index fa384fe..f3e8f05 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/rtc.h>
> +#include <linux/clk.h>
>  
>  /* These register offsets are relative to LP (Low Power) range */
>  #define SNVS_LPCR		0x04
> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>  	void __iomem *ioaddr;
>  	int irq;
>  	spinlock_t lock;
> +	struct clk *clk;
>  };
>  
>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +			data->irq, ret);
> +		return ret;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> +						PTR_ERR(data->clk));
> +		ret = PTR_ERR(data->clk);
> +		return ret;
> +	}

No. This will break all i.MX6 devices running a DTB without clock
definition.

Shawn

> +
>  	platform_set_drvdata(pdev, data);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Could not prepare or enable the snvs clock\n");
> +		return ret;
> +	}
> +
>  	spin_lock_init(&data->lock);
>  
>  	/* Initialize glitch detect */
> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, true);
>  
> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> -			data->irq, ret);
> -		return ret;
> -	}
> -
>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  					&snvs_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(data->rtc)) {
>  		ret = PTR_ERR(data->rtc);
>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
> -		return ret;
> +		goto error_rtc_device_register;
>  	}
>  
>  	return 0;
> +
> +error_rtc_device_register:
> +	clk_disable_unprepare(data->clk);
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(data->irq);
>  
> +	clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
>  static int snvs_rtc_resume(struct device *dev)
>  {
> +	int ret;
> +
>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(data->irq);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  #endif
> -- 
> 1.7.9.5
> 

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Shawn Guo]

On Fri, Sep 26, 2014 at 06:44:01PM +0530, Sanchayan Maity wrote:
> This patch adds clock enable and disable support
> for the SNVS peripheral which is required by the
> SNVS RTC.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index fa384fe..f3e8f05 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/rtc.h>
> +#include <linux/clk.h>
>  
>  /* These register offsets are relative to LP (Low Power) range */
>  #define SNVS_LPCR		0x04
> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>  	void __iomem *ioaddr;
>  	int irq;
>  	spinlock_t lock;
> +	struct clk *clk;
>  };
>  
>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +			data->irq, ret);
> +		return ret;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> +						PTR_ERR(data->clk));
> +		ret = PTR_ERR(data->clk);
> +		return ret;
> +	}

No. This will break all i.MX6 devices running a DTB without clock
definition.

Shawn

> +
>  	platform_set_drvdata(pdev, data);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Could not prepare or enable the snvs clock\n");
> +		return ret;
> +	}
> +
>  	spin_lock_init(&data->lock);
>  
>  	/* Initialize glitch detect */
> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, true);
>  
> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> -			data->irq, ret);
> -		return ret;
> -	}
> -
>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  					&snvs_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(data->rtc)) {
>  		ret = PTR_ERR(data->rtc);
>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
> -		return ret;
> +		goto error_rtc_device_register;
>  	}
>  
>  	return 0;
> +
> +error_rtc_device_register:
> +	clk_disable_unprepare(data->clk);
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(data->irq);
>  
> +	clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
>  static int snvs_rtc_resume(struct device *dev)
>  {
> +	int ret;
> +
>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(data->irq);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  #endif
> -- 
> 1.7.9.5
> 

Re: [PATCH 2/3] ARM: dts: vf610: Add SNVS node

[Shawn Guo]

On Fri, Sep 26, 2014 at 06:44:00PM +0530, Sanchayan Maity wrote:
> This adds a devicetree node for RTC support
> for the VF610 platform.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vf610.dtsi |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
> index 4d2ec32..3c0c757 100644
> --- a/arch/arm/boot/dts/vf610.dtsi
> +++ b/arch/arm/boot/dts/vf610.dtsi
> @@ -381,6 +381,21 @@
>  					<&clks VF610_CLK_DMAMUX3>;
>  			};
>  
> +			snvs0: snvs@400a7000 {
> +				compatible = "fsl,sec-v4.0-mon", "simple-bus";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0 0x400a7000 0x2000>;
> +
> +				snvs-rtc-lp@34 {
> +					compatible = "fsl,sec-v4.0-mon-rtc-lp";
> +					reg = <0x34 0x58>;
> +					interrupts = <0 100 IRQ_TYPE_LEVEL_HIGH>;
> +					clocks = <&clks VF610_CLK_SNVS>;
> +					clock-names = "snvs";

The name should describe the clock usage inside the block, so "snvs" is
not a good description.

Shawn

> +				};
> +			};
> +
>  			uart4: serial@400a9000 {
>  				compatible = "fsl,vf610-lpuart";
>  				reg = <0x400a9000 0x1000>;
> -- 
> 1.7.9.5
> 

[PATCH 2/3] ARM: dts: vf610: Add SNVS node

[Shawn Guo]

On Fri, Sep 26, 2014 at 06:44:00PM +0530, Sanchayan Maity wrote:
> This adds a devicetree node for RTC support
> for the VF610 platform.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vf610.dtsi |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
> index 4d2ec32..3c0c757 100644
> --- a/arch/arm/boot/dts/vf610.dtsi
> +++ b/arch/arm/boot/dts/vf610.dtsi
> @@ -381,6 +381,21 @@
>  					<&clks VF610_CLK_DMAMUX3>;
>  			};
>  
> +			snvs0: snvs at 400a7000 {
> +				compatible = "fsl,sec-v4.0-mon", "simple-bus";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				ranges = <0 0x400a7000 0x2000>;
> +
> +				snvs-rtc-lp at 34 {
> +					compatible = "fsl,sec-v4.0-mon-rtc-lp";
> +					reg = <0x34 0x58>;
> +					interrupts = <0 100 IRQ_TYPE_LEVEL_HIGH>;
> +					clocks = <&clks VF610_CLK_SNVS>;
> +					clock-names = "snvs";

The name should describe the clock usage inside the block, so "snvs" is
not a good description.

Shawn

> +				};
> +			};
> +
>  			uart4: serial at 400a9000 {
>  				compatible = "fsl,vf610-lpuart";
>  				reg = <0x400a9000 0x1000>;
> -- 
> 1.7.9.5
> 

Re: [PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Stefan Agner]

Am 2014-09-28 05:56, schrieb Shawn Guo:
> On Fri, Sep 26, 2014 at 06:44:01PM +0530, Sanchayan Maity wrote:
>> This patch adds clock enable and disable support
>> for the SNVS peripheral which is required by the
>> SNVS RTC.
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>> index fa384fe..f3e8f05 100644
>> --- a/drivers/rtc/rtc-snvs.c
>> +++ b/drivers/rtc/rtc-snvs.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/rtc.h>
>> +#include <linux/clk.h>
>>
>>  /* These register offsets are relative to LP (Low Power) range */
>>  #define SNVS_LPCR		0x04
>> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>>  	void __iomem *ioaddr;
>>  	int irq;
>>  	spinlock_t lock;
>> +	struct clk *clk;
>>  };
>>
>>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
>> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>
>> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> +			data->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> +						PTR_ERR(data->clk));
>> +		ret = PTR_ERR(data->clk);
>> +		return ret;
>> +	}
> 
> No. This will break all i.MX6 devices running a DTB without clock
> definition.
> 

Shawn, do you know how this works in i.MX6? Is there no clock gating for
that module or is that clock just missing as of now?

But then, I guess even there is a clock on i.MX6 too, we would still
need to make the clock optional.

--
Stefan


>> +
>>  	platform_set_drvdata(pdev, data);
>>
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"Could not prepare or enable the snvs clock\n");
>> +		return ret;
>> +	}
>> +
>>  	spin_lock_init(&data->lock);
>>
>>  	/* Initialize glitch detect */
>> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>
>>  	device_init_wakeup(&pdev->dev, true);
>>
>> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> -			data->irq, ret);
>> -		return ret;
>> -	}
>> -
>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>>  					&snvs_rtc_ops, THIS_MODULE);
>>  	if (IS_ERR(data->rtc)) {
>>  		ret = PTR_ERR(data->rtc);
>>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
>> -		return ret;
>> +		goto error_rtc_device_register;
>>  	}
>>
>>  	return 0;
>> +
>> +error_rtc_device_register:
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>>  }
>>
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>>  	if (device_may_wakeup(dev))
>>  		enable_irq_wake(data->irq);
>>
>> +	clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>
>>  static int snvs_rtc_resume(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>>
>>  	if (device_may_wakeup(dev))
>>  		disable_irq_wake(data->irq);
>>
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		return ret;
>> +
>>  	return 0;
>>  }
>>  #endif
>> --
>> 1.7.9.5
>>

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Stefan Agner]

Am 2014-09-28 05:56, schrieb Shawn Guo:
> On Fri, Sep 26, 2014 at 06:44:01PM +0530, Sanchayan Maity wrote:
>> This patch adds clock enable and disable support
>> for the SNVS peripheral which is required by the
>> SNVS RTC.
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>> index fa384fe..f3e8f05 100644
>> --- a/drivers/rtc/rtc-snvs.c
>> +++ b/drivers/rtc/rtc-snvs.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/rtc.h>
>> +#include <linux/clk.h>
>>
>>  /* These register offsets are relative to LP (Low Power) range */
>>  #define SNVS_LPCR		0x04
>> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>>  	void __iomem *ioaddr;
>>  	int irq;
>>  	spinlock_t lock;
>> +	struct clk *clk;
>>  };
>>
>>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
>> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>
>> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> +			data->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> +						PTR_ERR(data->clk));
>> +		ret = PTR_ERR(data->clk);
>> +		return ret;
>> +	}
> 
> No. This will break all i.MX6 devices running a DTB without clock
> definition.
> 

Shawn, do you know how this works in i.MX6? Is there no clock gating for
that module or is that clock just missing as of now?

But then, I guess even there is a clock on i.MX6 too, we would still
need to make the clock optional.

--
Stefan


>> +
>>  	platform_set_drvdata(pdev, data);
>>
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"Could not prepare or enable the snvs clock\n");
>> +		return ret;
>> +	}
>> +
>>  	spin_lock_init(&data->lock);
>>
>>  	/* Initialize glitch detect */
>> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>
>>  	device_init_wakeup(&pdev->dev, true);
>>
>> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> -			data->irq, ret);
>> -		return ret;
>> -	}
>> -
>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>>  					&snvs_rtc_ops, THIS_MODULE);
>>  	if (IS_ERR(data->rtc)) {
>>  		ret = PTR_ERR(data->rtc);
>>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
>> -		return ret;
>> +		goto error_rtc_device_register;
>>  	}
>>
>>  	return 0;
>> +
>> +error_rtc_device_register:
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>>  }
>>
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>>  	if (device_may_wakeup(dev))
>>  		enable_irq_wake(data->irq);
>>
>> +	clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>
>>  static int snvs_rtc_resume(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>>
>>  	if (device_may_wakeup(dev))
>>  		disable_irq_wake(data->irq);
>>
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		return ret;
>> +
>>  	return 0;
>>  }
>>  #endif
>> --
>> 1.7.9.5
>>

Re: [PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Stefan Agner]

Am 2014-09-26 15:14, schrieb Sanchayan Maity:
> This patch adds clock enable and disable support
> for the SNVS peripheral which is required by the
> SNVS RTC.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index fa384fe..f3e8f05 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/rtc.h>
> +#include <linux/clk.h>
>  
>  /* These register offsets are relative to LP (Low Power) range */
>  #define SNVS_LPCR		0x04
> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>  	void __iomem *ioaddr;
>  	int irq;
>  	spinlock_t lock;
> +	struct clk *clk;
>  };
>  
>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +			data->irq, ret);
> +		return ret;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> +						PTR_ERR(data->clk));
> +		ret = PTR_ERR(data->clk);
> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, data);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Could not prepare or enable the snvs clock\n");
> +		return ret;
> +	}
> +
>  	spin_lock_init(&data->lock);
>  
>  	/* Initialize glitch detect */
> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, true);
>  
> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> -			data->irq, ret);
> -		return ret;
> -	}
> -

Is there a reason why you moved the irq request before enabling/getting
the clocks? AFAIK the irq is quite independent from the clock of that
IP, so you could that leave here. Try to make as few changes to the code
as possible to archive your goal.

>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  					&snvs_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(data->rtc)) {
>  		ret = PTR_ERR(data->rtc);
>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
> -		return ret;
> +		goto error_rtc_device_register;
>  	}
>  
>  	return 0;
> +
> +error_rtc_device_register:
> +	clk_disable_unprepare(data->clk);
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(data->irq);
>  
> +	clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
>  static int snvs_rtc_resume(struct device *dev)
>  {
> +	int ret;
> +
>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(data->irq);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  #endif

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Stefan Agner]

Am 2014-09-26 15:14, schrieb Sanchayan Maity:
> This patch adds clock enable and disable support
> for the SNVS peripheral which is required by the
> SNVS RTC.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index fa384fe..f3e8f05 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/rtc.h>
> +#include <linux/clk.h>
>  
>  /* These register offsets are relative to LP (Low Power) range */
>  #define SNVS_LPCR		0x04
> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>  	void __iomem *ioaddr;
>  	int irq;
>  	spinlock_t lock;
> +	struct clk *clk;
>  };
>  
>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +			data->irq, ret);
> +		return ret;
> +	}
> +
> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
> +						PTR_ERR(data->clk));
> +		ret = PTR_ERR(data->clk);
> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, data);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Could not prepare or enable the snvs clock\n");
> +		return ret;
> +	}
> +
>  	spin_lock_init(&data->lock);
>  
>  	/* Initialize glitch detect */
> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, true);
>  
> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> -			data->irq, ret);
> -		return ret;
> -	}
> -

Is there a reason why you moved the irq request before enabling/getting
the clocks? AFAIK the irq is quite independent from the clock of that
IP, so you could that leave here. Try to make as few changes to the code
as possible to archive your goal.

>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>  					&snvs_rtc_ops, THIS_MODULE);
>  	if (IS_ERR(data->rtc)) {
>  		ret = PTR_ERR(data->rtc);
>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
> -		return ret;
> +		goto error_rtc_device_register;
>  	}
>  
>  	return 0;
> +
> +error_rtc_device_register:
> +	clk_disable_unprepare(data->clk);
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(data->irq);
>  
> +	clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
>  static int snvs_rtc_resume(struct device *dev)
>  {
> +	int ret;
> +
>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(data->irq);
>  
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  #endif

Re: [PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Sanchayan Maity]

>> This patch adds clock enable and disable support
>> for the SNVS peripheral which is required by the
>> SNVS RTC.
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>> index fa384fe..f3e8f05 100644
>> --- a/drivers/rtc/rtc-snvs.c
>> +++ b/drivers/rtc/rtc-snvs.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/rtc.h>
>> +#include <linux/clk.h>
>>  
>>  /* These register offsets are relative to LP (Low Power) range */
>>  #define SNVS_LPCR		0x04
>> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>>  	void __iomem *ioaddr;
>>  	int irq;
>>  	spinlock_t lock;
>> +	struct clk *clk;
>>  };
>>  
>>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
>> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>  
>> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> +			data->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> +						PTR_ERR(data->clk));
>> +		ret = PTR_ERR(data->clk);
>> +		return ret;
>> +	}
>> +
>>  	platform_set_drvdata(pdev, data);
>>  
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"Could not prepare or enable the snvs clock\n");
>> +		return ret;
>> +	}
>> +
>>  	spin_lock_init(&data->lock);
>>  
>>  	/* Initialize glitch detect */
>> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  
>>  	device_init_wakeup(&pdev->dev, true);
>>  
>> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> -			data->irq, ret);
>> -		return ret;
>> -	}
>> -
> 
> Is there a reason why you moved the irq request before enabling/getting
> the clocks? AFAIK the irq is quite independent from the clock of that
> IP, so you could that leave here. Try to make as few changes to the code
> as possible to archive your goal.

Ok. Will fix this, if the patch gets accepted in some form. Seems separating the
patch for SNVS node addition to the VF610 dts will be better. 

> 
>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>>  					&snvs_rtc_ops, THIS_MODULE);
>>  	if (IS_ERR(data->rtc)) {
>>  		ret = PTR_ERR(data->rtc);
>>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
>> -		return ret;
>> +		goto error_rtc_device_register;
>>  	}
>>  
>>  	return 0;
>> +
>> +error_rtc_device_register:
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>>  }
>>  
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>>  	if (device_may_wakeup(dev))
>>  		enable_irq_wake(data->irq);
>>  
>> +	clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>  
>>  static int snvs_rtc_resume(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>>  
>>  	if (device_may_wakeup(dev))
>>  		disable_irq_wake(data->irq);
>>  
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		return ret;
>> +
>>  	return 0;
>>  }
>>  #endif

[PATCH 3/3] drivers/rtc/rtc-snvs: Add clock support

[Sanchayan Maity]

>> This patch adds clock enable and disable support
>> for the SNVS peripheral which is required by the
>> SNVS RTC.
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  drivers/rtc/rtc-snvs.c |   48 +++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>> index fa384fe..f3e8f05 100644
>> --- a/drivers/rtc/rtc-snvs.c
>> +++ b/drivers/rtc/rtc-snvs.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/rtc.h>
>> +#include <linux/clk.h>
>>  
>>  /* These register offsets are relative to LP (Low Power) range */
>>  #define SNVS_LPCR		0x04
>> @@ -39,6 +40,7 @@ struct snvs_rtc_data {
>>  	void __iomem *ioaddr;
>>  	int irq;
>>  	spinlock_t lock;
>> +	struct clk *clk;
>>  };
>>  
>>  static u32 rtc_read_lp_counter(void __iomem *ioaddr)
>> @@ -260,8 +262,31 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>  
>> +	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> +			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> +			data->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "snvs");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>> +						PTR_ERR(data->clk));
>> +		ret = PTR_ERR(data->clk);
>> +		return ret;
>> +	}
>> +
>>  	platform_set_drvdata(pdev, data);
>>  
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +			"Could not prepare or enable the snvs clock\n");
>> +		return ret;
>> +	}
>> +
>>  	spin_lock_init(&data->lock);
>>  
>>  	/* Initialize glitch detect */
>> @@ -275,23 +300,20 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  
>>  	device_init_wakeup(&pdev->dev, true);
>>  
>> -	ret = devm_request_irq(&pdev->dev, data->irq, snvs_rtc_irq_handler,
>> -			       IRQF_SHARED, "rtc alarm", &pdev->dev);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>> -			data->irq, ret);
>> -		return ret;
>> -	}
>> -
> 
> Is there a reason why you moved the irq request before enabling/getting
> the clocks? AFAIK the irq is quite independent from the clock of that
> IP, so you could that leave here. Try to make as few changes to the code
> as possible to archive your goal.

Ok. Will fix this, if the patch gets accepted in some form. Seems separating the
patch for SNVS node addition to the VF610 dts will be better. 

> 
>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>>  					&snvs_rtc_ops, THIS_MODULE);
>>  	if (IS_ERR(data->rtc)) {
>>  		ret = PTR_ERR(data->rtc);
>>  		dev_err(&pdev->dev, "failed to register rtc: %d\n", ret);
>> -		return ret;
>> +		goto error_rtc_device_register;
>>  	}
>>  
>>  	return 0;
>> +
>> +error_rtc_device_register:
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>>  }
>>  
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -302,16 +324,24 @@ static int snvs_rtc_suspend(struct device *dev)
>>  	if (device_may_wakeup(dev))
>>  		enable_irq_wake(data->irq);
>>  
>> +	clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>  
>>  static int snvs_rtc_resume(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>>  
>>  	if (device_may_wakeup(dev))
>>  		disable_irq_wake(data->irq);
>>  
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret)
>> +		return ret;
>> +
>>  	return 0;
>>  }
>>  #endif