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

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

[Sanchayan Maity]

The first version of the patches were send quiet a 
while back. 
https://lkml.org/lkml/2014/9/26/492

The reason for sending this v2 of the patches late was to
prevent any changes midway to the VF dts patches being tried
with the below commit.
https://lkml.org/lkml/2014/10/22/605

Changes since v1:
The clock enable and disable functions for the SNVS are
now optional and only enable/disable it, if it has been
defined in the device tree node. Instead of failing and 
returning error as in the earlier set of patches, this
makes sure that the driver does not break for the other
i.MX devices for which the clock has not been defined and
enabled elsewhere.

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

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

-- 
1.7.9.5

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

[Sanchayan Maity]

This patch adds support for clock gating of
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 5937dde..bbf4785 100644
--- a/arch/arm/mach-imx/clk-vf610.c
+++ b/arch/arm/mach-imx/clk-vf610.c
@@ -379,6 +379,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-rtc", "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 801c0ac..979d24a 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -192,6 +192,7 @@
 #define VF610_PLL5_BYPASS		179
 #define VF610_PLL6_BYPASS		180
 #define VF610_PLL7_BYPASS		181
-#define VF610_CLK_END			182
+#define VF610_CLK_SNVS			182
+#define VF610_CLK_END			183
 
 #endif /* __DT_BINDINGS_CLOCK_VF610_H */
-- 
1.7.9.5

Re: [[PATCHv2] 1/3] ARM: imx: clk-vf610: Add clock for SNVS

[Shawn Guo]

On Fri, Nov 07, 2014 at 06:34:26PM +0530, Sanchayan Maity wrote:
> This patch adds support for clock gating of
> 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 5937dde..bbf4785 100644
> --- a/arch/arm/mach-imx/clk-vf610.c
> +++ b/arch/arm/mach-imx/clk-vf610.c
> @@ -379,6 +379,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-rtc", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));

Stefan,

Would you confirm this register bits is the gating for SNVS clock?  I
cannot find it in my Vybrid Reference Manual.

Shawn

>  
>  	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 801c0ac..979d24a 100644
> --- a/include/dt-bindings/clock/vf610-clock.h
> +++ b/include/dt-bindings/clock/vf610-clock.h
> @@ -192,6 +192,7 @@
>  #define VF610_PLL5_BYPASS		179
>  #define VF610_PLL6_BYPASS		180
>  #define VF610_PLL7_BYPASS		181
> -#define VF610_CLK_END			182
> +#define VF610_CLK_SNVS			182
> +#define VF610_CLK_END			183
>  
>  #endif /* __DT_BINDINGS_CLOCK_VF610_H */
> -- 
> 1.7.9.5
> 

Re: [[PATCHv2] 1/3] ARM: imx: clk-vf610: Add clock for SNVS

[Stefan Agner]

On 2014-11-11 15:49, Shawn Guo wrote:
> On Fri, Nov 07, 2014 at 06:34:26PM +0530, Sanchayan Maity wrote:
>> This patch adds support for clock gating of
>> 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 5937dde..bbf4785 100644
>> --- a/arch/arm/mach-imx/clk-vf610.c
>> +++ b/arch/arm/mach-imx/clk-vf610.c
>> @@ -379,6 +379,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-rtc", "ipg_bus", CCM_CCGR6, CCM_CCGRx_CGn(7));
> 
> Stefan,
> 
> Would you confirm this register bits is the gating for SNVS clock?  I
> cannot find it in my Vybrid Reference Manual.

The RTC is part of the secure stuff. The clock gate bits are correct.

 
>>
>>  	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 801c0ac..979d24a 100644
>> --- a/include/dt-bindings/clock/vf610-clock.h
>> +++ b/include/dt-bindings/clock/vf610-clock.h
>> @@ -192,6 +192,7 @@
>>  #define VF610_PLL5_BYPASS		179
>>  #define VF610_PLL6_BYPASS		180
>>  #define VF610_PLL7_BYPASS		181
>> -#define VF610_CLK_END			182
>> +#define VF610_CLK_SNVS			182
>> +#define VF610_CLK_END			183
>>
>>  #endif /* __DT_BINDINGS_CLOCK_VF610_H */
>> --
>> 1.7.9.5
>>

Re: [[PATCHv2] 1/3] ARM: imx: clk-vf610: Add clock for SNVS

[Stefan Agner]

On 2014-11-07 14:04, Sanchayan Maity wrote:
> This patch adds support for clock gating of
> 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 5937dde..bbf4785 100644
> --- a/arch/arm/mach-imx/clk-vf610.c
> +++ b/arch/arm/mach-imx/clk-vf610.c
> @@ -379,6 +379,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-rtc", "ipg_bus",
> CCM_CCGR6, CCM_CCGRx_CGn(7));

Just noticed that imx_clk_gate2 always sets both gates. This means, the
RTC will be gated when going into low power modes. We might need to use
a different gating when we want to use the RTC as wakeup source. But
this would be part of the suspend/resume patchset...

>  
>  	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 801c0ac..979d24a 100644
> --- a/include/dt-bindings/clock/vf610-clock.h
> +++ b/include/dt-bindings/clock/vf610-clock.h
> @@ -192,6 +192,7 @@
>  #define VF610_PLL5_BYPASS		179
>  #define VF610_PLL6_BYPASS		180
>  #define VF610_PLL7_BYPASS		181
> -#define VF610_CLK_END			182
> +#define VF610_CLK_SNVS			182
> +#define VF610_CLK_END			183
>  
>  #endif /* __DT_BINDINGS_CLOCK_VF610_H */

Acked-By: Stefan Agner <stefan@agner.ch>

--
Stefan

[[PATCHv2] 2/3] ARM: dts: vfxxx: Add SNVS node

[Sanchayan Maity]

This patch adds a devicetree node for the Secure
Non-Volatile Storage (SNVS) on the VF610 platform.
The SNVS block also has a Real Time Counter (RTC).

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

diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 4e70833..bc131b0 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -338,6 +338,21 @@
 				status = "disabled";
 			};
 
+			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-rtc";
+				};
+			};
+
 			uart4: serial@400a9000 {
 				compatible = "fsl,vf610-lpuart";
 				reg = <0x400a9000 0x1000>;
-- 
1.7.9.5

Re: [[PATCHv2] 2/3] ARM: dts: vfxxx: Add SNVS node

[Stefan Agner]

On 2014-11-07 14:04, Sanchayan Maity wrote:
> This patch adds a devicetree node for the Secure
> Non-Volatile Storage (SNVS) on the VF610 platform.
> The SNVS block also has a Real Time Counter (RTC).
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vfxxx.dtsi |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
> index 4e70833..bc131b0 100644
> --- a/arch/arm/boot/dts/vfxxx.dtsi
> +++ b/arch/arm/boot/dts/vfxxx.dtsi
> @@ -338,6 +338,21 @@
>  				status = "disabled";
>  			};
>  
> +			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>;

With the new device tree structure, the interrupt field needs to be part
of the vf500.dtsi now. The vfxxx.dtsi will be the parent also for the
Cortex-M4 device tree, which uses a different interrupt controller.

While at it, use the GIC_SPI macro for the first cell (so this will be
blend into the other interrupt definitions).

> +					clocks = <&clks VF610_CLK_SNVS>;
> +					clock-names = "snvs-rtc";
> +				};
> +			};
> +
>  			uart4: serial@400a9000 {
>  				compatible = "fsl,vf610-lpuart";
>  				reg = <0x400a9000 0x1000>;

Re: [[PATCHv2] 2/3] ARM: dts: vfxxx: Add SNVS node

[Sanchayan Maity]

Hello,

On Wednesday 12 November 2014 02:50 AM, Stefan Agner wrote:
> On 2014-11-07 14:04, Sanchayan Maity wrote:
>> This patch adds a devicetree node for the Secure
>> Non-Volatile Storage (SNVS) on the VF610 platform.
>> The SNVS block also has a Real Time Counter (RTC).
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  arch/arm/boot/dts/vfxxx.dtsi |   15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
>> index 4e70833..bc131b0 100644
>> --- a/arch/arm/boot/dts/vfxxx.dtsi
>> +++ b/arch/arm/boot/dts/vfxxx.dtsi
>> @@ -338,6 +338,21 @@
>>  				status = "disabled";
>>  			};
>>  
>> +			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>;
> 
> With the new device tree structure, the interrupt field needs to be part
> of the vf500.dtsi now. The vfxxx.dtsi will be the parent also for the
> Cortex-M4 device tree, which uses a different interrupt controller.
> 
> While at it, use the GIC_SPI macro for the first cell (so this will be
> blend into the other interrupt definitions).

OK. Will fix this and send out a v3. My bad I missed the M4 changes.

> 
>> +					clocks = <&clks VF610_CLK_SNVS>;
>> +					clock-names = "snvs-rtc";
>> +				};
>> +			};
>> +
>>  			uart4: serial@400a9000 {
>>  				compatible = "fsl,vf610-lpuart";
>>  				reg = <0x400a9000 0x1000>;
> 

Regards,
Sanchayan.

[[PATCHv2] 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 for using the
RTC within the SNVS block.

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

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index fa384fe..d4a6512 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,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	if (data->irq < 0)
 		return data->irq;
 
+	data->clk = devm_clk_get(&pdev->dev, "snvs-rtc");
+	if (IS_ERR(data->clk)) {
+		data->clk = NULL;
+	} else {
+		ret = clk_prepare_enable(data->clk);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Could not prepare or enable the snvs clock\n");
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, data);
 
 	spin_lock_init(&data->lock);
@@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
 			data->irq, ret);
-		return ret;
+		goto error_rtc_device_register;
 	}
 
 	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
@@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	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:
+	if (data->clk)
+		clk_disable_unprepare(data->clk);
+
+	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev)
 	if (device_may_wakeup(dev))
 		enable_irq_wake(data->irq);
 
+	if (data->clk)
+		clk_disable_unprepare(data->clk);
+
 	return 0;
 }
 
 static int snvs_rtc_resume(struct device *dev)
 {
 	struct snvs_rtc_data *data = dev_get_drvdata(dev);
+	int ret;
 
 	if (device_may_wakeup(dev))
 		disable_irq_wake(data->irq);
 
+	if (data->clk) {
+		ret = clk_prepare_enable(data->clk);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 #endif
-- 
1.7.9.5

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

[Stefan Agner]

On 2014-11-07 14:04, Sanchayan Maity wrote:
> This patch adds clock enable and disable support for
> the SNVS peripheral, which is required for using the
> RTC within the SNVS block.

What happens if the device tree node is there while this patch is not
applied? I guess the driver would load, but since the clocks of the
peripheral are not enabled the first register access would lead to bus
error or similar. If this is the case, this would break bisectability.
You should move the device tree patch to the end.

Other than that
Acked-by: Stefan Agner <stefan@agner.ch>

--
Stefan

> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  drivers/rtc/rtc-snvs.c |   34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index fa384fe..d4a6512 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,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> +	data->clk = devm_clk_get(&pdev->dev, "snvs-rtc");
> +	if (IS_ERR(data->clk)) {
> +		data->clk = NULL;
> +	} else {
> +		ret = clk_prepare_enable(data->clk);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"Could not prepare or enable the snvs clock\n");
> +			return ret;
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, data);
>  
>  	spin_lock_init(&data->lock);
> @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>  			data->irq, ret);
> -		return ret;
> +		goto error_rtc_device_register;
>  	}
>  
>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>  	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:
> +	if (data->clk)
> +		clk_disable_unprepare(data->clk);
> +
> +	return ret;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev)
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(data->irq);
>  
> +	if (data->clk)
> +		clk_disable_unprepare(data->clk);
> +
>  	return 0;
>  }
>  
>  static int snvs_rtc_resume(struct device *dev)
>  {
>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
> +	int ret;
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(data->irq);
>  
> +	if (data->clk) {
> +		ret = clk_prepare_enable(data->clk);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  #endif

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

[Sanchayan Maity]

On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
> On 2014-11-07 14:04, Sanchayan Maity wrote:
>> This patch adds clock enable and disable support for
>> the SNVS peripheral, which is required for using the
>> RTC within the SNVS block.
> 
> What happens if the device tree node is there while this patch is not
> applied? I guess the driver would load, but since the clocks of the
> peripheral are not enabled the first register access would lead to bus
> error or similar. If this is the case, this would break bisectability.
> You should move the device tree patch to the end.
> 

If the DT node was present with this patch not applied, the driver would 
still crash with a bus error, since the clocks for SNVS are not explicitly 
enabled anywhere else. I had not thought about any problems which might 
occur later with git bisect. Will move this to the end with v3.

Thanks.

Regards,
Sanchayan.

> Other than that
> Acked-by: Stefan Agner <stefan@agner.ch>
> 
> --
> Stefan
> 
>>
>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> ---
>>  drivers/rtc/rtc-snvs.c |   34 ++++++++++++++++++++++++++++++++--
>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>> index fa384fe..d4a6512 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,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (data->irq < 0)
>>  		return data->irq;
>>  
>> +	data->clk = devm_clk_get(&pdev->dev, "snvs-rtc");
>> +	if (IS_ERR(data->clk)) {
>> +		data->clk = NULL;
>> +	} else {
>> +		ret = clk_prepare_enable(data->clk);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"Could not prepare or enable the snvs clock\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>>  	platform_set_drvdata(pdev, data);
>>  
>>  	spin_lock_init(&data->lock);
>> @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>>  			data->irq, ret);
>> -		return ret;
>> +		goto error_rtc_device_register;
>>  	}
>>  
>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>> @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>  	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:
>> +	if (data->clk)
>> +		clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>>  }
>>  
>>  #ifdef CONFIG_PM_SLEEP
>> @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev)
>>  	if (device_may_wakeup(dev))
>>  		enable_irq_wake(data->irq);
>>  
>> +	if (data->clk)
>> +		clk_disable_unprepare(data->clk);
>> +
>>  	return 0;
>>  }
>>  
>>  static int snvs_rtc_resume(struct device *dev)
>>  {
>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>> +	int ret;
>>  
>>  	if (device_may_wakeup(dev))
>>  		disable_irq_wake(data->irq);
>>  
>> +	if (data->clk) {
>> +		ret = clk_prepare_enable(data->clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  #endif
> 

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

[Shawn Guo]

On Wed, Nov 12, 2014 at 02:26:51PM +0530, Sanchayan Maity wrote:
> On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
> > On 2014-11-07 14:04, Sanchayan Maity wrote:
> >> This patch adds clock enable and disable support for
> >> the SNVS peripheral, which is required for using the
> >> RTC within the SNVS block.
> > 
> > What happens if the device tree node is there while this patch is not
> > applied? I guess the driver would load, but since the clocks of the
> > peripheral are not enabled the first register access would lead to bus
> > error or similar. If this is the case, this would break bisectability.
> > You should move the device tree patch to the end.
> > 
> 
> If the DT node was present with this patch not applied, the driver would 
> still crash with a bus error, since the clocks for SNVS are not explicitly 
> enabled anywhere else. I had not thought about any problems which might 
> occur later with git bisect. Will move this to the end with v3.

Since changes on rtc-snvs.c will need to go through RTC subsystem tree,
that means I cannot apply DTS changes until the driver patch gets
mainlined and appears on my tree.

Shawn

> 
> Thanks.
> 
> Regards,
> Sanchayan.
> 
> > Other than that
> > Acked-by: Stefan Agner <stefan@agner.ch>
> > 
> > --
> > Stefan
> > 
> >>
> >> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> >> ---
> >>  drivers/rtc/rtc-snvs.c |   34 ++++++++++++++++++++++++++++++++--
> >>  1 file changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> >> index fa384fe..d4a6512 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,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev)
> >>  	if (data->irq < 0)
> >>  		return data->irq;
> >>  
> >> +	data->clk = devm_clk_get(&pdev->dev, "snvs-rtc");
> >> +	if (IS_ERR(data->clk)) {
> >> +		data->clk = NULL;
> >> +	} else {
> >> +		ret = clk_prepare_enable(data->clk);
> >> +		if (ret) {
> >> +			dev_err(&pdev->dev,
> >> +				"Could not prepare or enable the snvs clock\n");
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >>  	platform_set_drvdata(pdev, data);
> >>  
> >>  	spin_lock_init(&data->lock);
> >> @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev)
> >>  	if (ret) {
> >>  		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> >>  			data->irq, ret);
> >> -		return ret;
> >> +		goto error_rtc_device_register;
> >>  	}
> >>  
> >>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> >> @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev)
> >>  	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:
> >> +	if (data->clk)
> >> +		clk_disable_unprepare(data->clk);
> >> +
> >> +	return ret;
> >>  }
> >>  
> >>  #ifdef CONFIG_PM_SLEEP
> >> @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev)
> >>  	if (device_may_wakeup(dev))
> >>  		enable_irq_wake(data->irq);
> >>  
> >> +	if (data->clk)
> >> +		clk_disable_unprepare(data->clk);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >>  static int snvs_rtc_resume(struct device *dev)
> >>  {
> >>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
> >> +	int ret;
> >>  
> >>  	if (device_may_wakeup(dev))
> >>  		disable_irq_wake(data->irq);
> >>  
> >> +	if (data->clk) {
> >> +		ret = clk_prepare_enable(data->clk);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >>  #endif
> > 

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

[Sanchayan Maity]

On Wednesday 12 November 2014 04:17 PM, Shawn Guo wrote:
> On Wed, Nov 12, 2014 at 02:26:51PM +0530, Sanchayan Maity wrote:
>> On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
>>> On 2014-11-07 14:04, Sanchayan Maity wrote:
>>>> This patch adds clock enable and disable support for
>>>> the SNVS peripheral, which is required for using the
>>>> RTC within the SNVS block.
>>>
>>> What happens if the device tree node is there while this patch is not
>>> applied? I guess the driver would load, but since the clocks of the
>>> peripheral are not enabled the first register access would lead to bus
>>> error or similar. If this is the case, this would break bisectability.
>>> You should move the device tree patch to the end.
>>>
>>
>> If the DT node was present with this patch not applied, the driver would 
>> still crash with a bus error, since the clocks for SNVS are not explicitly 
>> enabled anywhere else. I had not thought about any problems which might 
>> occur later with git bisect. Will move this to the end with v3.
> 
> Since changes on rtc-snvs.c will need to go through RTC subsystem tree,
> that means I cannot apply DTS changes until the driver patch gets
> mainlined and appears on my tree.
> 
> Shawn

Shawn

So the approach in this patch is OK and acceptable? 

Since the DT changes cannot be applied by you, till this goes through the 
RTC subsystem tree, gets mainlined and appears on your tree, I will send out 
this patch by itself then and send the DT changes later once this gets
mainlined?

I did not CC Alessandro Zummo, the maintainer of RTC subsystem by mistake.
Once you ACK/OK this i will send out this patch in isolation then or just 
CC Alessandro Zummo.

- Sanchayan

> 
>>
>> Thanks.
>>
>> Regards,
>> Sanchayan.
>>
>>> Other than that
>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>
>>> --
>>> Stefan
>>>
>>>>
>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>> ---
>>>>  drivers/rtc/rtc-snvs.c |   34 ++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
>>>> index fa384fe..d4a6512 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,6 +262,18 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>>>  	if (data->irq < 0)
>>>>  		return data->irq;
>>>>  
>>>> +	data->clk = devm_clk_get(&pdev->dev, "snvs-rtc");
>>>> +	if (IS_ERR(data->clk)) {
>>>> +		data->clk = NULL;
>>>> +	} else {
>>>> +		ret = clk_prepare_enable(data->clk);
>>>> +		if (ret) {
>>>> +			dev_err(&pdev->dev,
>>>> +				"Could not prepare or enable the snvs clock\n");
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	platform_set_drvdata(pdev, data);
>>>>  
>>>>  	spin_lock_init(&data->lock);
>>>> @@ -280,7 +294,7 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>>>  	if (ret) {
>>>>  		dev_err(&pdev->dev, "failed to request irq %d: %d\n",
>>>>  			data->irq, ret);
>>>> -		return ret;
>>>> +		goto error_rtc_device_register;
>>>>  	}
>>>>  
>>>>  	data->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
>>>> @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev)
>>>>  	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:
>>>> +	if (data->clk)
>>>> +		clk_disable_unprepare(data->clk);
>>>> +
>>>> +	return ret;
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_PM_SLEEP
>>>> @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev)
>>>>  	if (device_may_wakeup(dev))
>>>>  		enable_irq_wake(data->irq);
>>>>  
>>>> +	if (data->clk)
>>>> +		clk_disable_unprepare(data->clk);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>>  static int snvs_rtc_resume(struct device *dev)
>>>>  {
>>>>  	struct snvs_rtc_data *data = dev_get_drvdata(dev);
>>>> +	int ret;
>>>>  
>>>>  	if (device_may_wakeup(dev))
>>>>  		disable_irq_wake(data->irq);
>>>>  
>>>> +	if (data->clk) {
>>>> +		ret = clk_prepare_enable(data->clk);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  #endif
>>>

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

[Shawn Guo]

On Thu, Nov 13, 2014 at 11:03:09AM +0530, Sanchayan Maity wrote:
> On Wednesday 12 November 2014 04:17 PM, Shawn Guo wrote:
> > On Wed, Nov 12, 2014 at 02:26:51PM +0530, Sanchayan Maity wrote:
> >> On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
> >>> On 2014-11-07 14:04, Sanchayan Maity wrote:
> >>>> This patch adds clock enable and disable support for
> >>>> the SNVS peripheral, which is required for using the
> >>>> RTC within the SNVS block.
> >>>
> >>> What happens if the device tree node is there while this patch is not
> >>> applied? I guess the driver would load, but since the clocks of the
> >>> peripheral are not enabled the first register access would lead to bus
> >>> error or similar. If this is the case, this would break bisectability.
> >>> You should move the device tree patch to the end.
> >>>
> >>
> >> If the DT node was present with this patch not applied, the driver would 
> >> still crash with a bus error, since the clocks for SNVS are not explicitly 
> >> enabled anywhere else. I had not thought about any problems which might 
> >> occur later with git bisect. Will move this to the end with v3.
> > 
> > Since changes on rtc-snvs.c will need to go through RTC subsystem tree,
> > that means I cannot apply DTS changes until the driver patch gets
> > mainlined and appears on my tree.
> > 
> > Shawn
> 
> Shawn
> 
> So the approach in this patch is OK and acceptable? 

Yes, I'm fine with it.

Shawn

> 
> Since the DT changes cannot be applied by you, till this goes through the 
> RTC subsystem tree, gets mainlined and appears on your tree, I will send out 
> this patch by itself then and send the DT changes later once this gets
> mainlined?
> 
> I did not CC Alessandro Zummo, the maintainer of RTC subsystem by mistake.
> Once you ACK/OK this i will send out this patch in isolation then or just 
> CC Alessandro Zummo.

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

[Sanchayan Maity]

On Wednesday 19 November 2014 06:47 AM, Shawn Guo wrote:
> On Thu, Nov 13, 2014 at 11:03:09AM +0530, Sanchayan Maity wrote:
>> On Wednesday 12 November 2014 04:17 PM, Shawn Guo wrote:
>>> On Wed, Nov 12, 2014 at 02:26:51PM +0530, Sanchayan Maity wrote:
>>>> On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
>>>>> On 2014-11-07 14:04, Sanchayan Maity wrote:
>>>>>> This patch adds clock enable and disable support for
>>>>>> the SNVS peripheral, which is required for using the
>>>>>> RTC within the SNVS block.
>>>>>
>>>>> What happens if the device tree node is there while this patch is not
>>>>> applied? I guess the driver would load, but since the clocks of the
>>>>> peripheral are not enabled the first register access would lead to bus
>>>>> error or similar. If this is the case, this would break bisectability.
>>>>> You should move the device tree patch to the end.
>>>>>
>>>>
>>>> If the DT node was present with this patch not applied, the driver would 
>>>> still crash with a bus error, since the clocks for SNVS are not explicitly 
>>>> enabled anywhere else. I had not thought about any problems which might 
>>>> occur later with git bisect. Will move this to the end with v3.
>>>
>>> Since changes on rtc-snvs.c will need to go through RTC subsystem tree,
>>> that means I cannot apply DTS changes until the driver patch gets
>>> mainlined and appears on my tree.
>>>
>>> Shawn
>>
>> Shawn
>>
>> So the approach in this patch is OK and acceptable? 
> 
> Yes, I'm fine with it.
> 
> Shawn


Hello Alessandro, 

Sorry I missed adding you while sending this patch set.

Can you take this third patch, in this series through your RTC subsytem
tree? Or should I resend this third patch separately?

Regards,
Sanchayan.

> 
>>
>> Since the DT changes cannot be applied by you, till this goes through the 
>> RTC subsystem tree, gets mainlined and appears on your tree, I will send out 
>> this patch by itself then and send the DT changes later once this gets
>> mainlined?
>>
>> I did not CC Alessandro Zummo, the maintainer of RTC subsystem by mistake.
>> Once you ACK/OK this i will send out this patch in isolation then or just 
>> CC Alessandro Zummo.

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

[Sanchayan Maity]

On Wednesday 19 November 2014 10:19 AM, Sanchayan Maity wrote:
> On Wednesday 19 November 2014 06:47 AM, Shawn Guo wrote:
>> On Thu, Nov 13, 2014 at 11:03:09AM +0530, Sanchayan Maity wrote:
>>> On Wednesday 12 November 2014 04:17 PM, Shawn Guo wrote:
>>>> On Wed, Nov 12, 2014 at 02:26:51PM +0530, Sanchayan Maity wrote:
>>>>> On Wednesday 12 November 2014 02:57 AM, Stefan Agner wrote:
>>>>>> On 2014-11-07 14:04, Sanchayan Maity wrote:
>>>>>>> This patch adds clock enable and disable support for
>>>>>>> the SNVS peripheral, which is required for using the
>>>>>>> RTC within the SNVS block.
>>>>>>
>>>>>> What happens if the device tree node is there while this patch is not
>>>>>> applied? I guess the driver would load, but since the clocks of the
>>>>>> peripheral are not enabled the first register access would lead to bus
>>>>>> error or similar. If this is the case, this would break bisectability.
>>>>>> You should move the device tree patch to the end.
>>>>>>
>>>>>
>>>>> If the DT node was present with this patch not applied, the driver would 
>>>>> still crash with a bus error, since the clocks for SNVS are not explicitly 
>>>>> enabled anywhere else. I had not thought about any problems which might 
>>>>> occur later with git bisect. Will move this to the end with v3.
>>>>
>>>> Since changes on rtc-snvs.c will need to go through RTC subsystem tree,
>>>> that means I cannot apply DTS changes until the driver patch gets
>>>> mainlined and appears on my tree.
>>>>
>>>> Shawn
>>>
>>> Shawn
>>>
>>> So the approach in this patch is OK and acceptable? 
>>
>> Yes, I'm fine with it.
>>
>> Shawn
> 
> 
> Hello Alessandro, 
> 
> Sorry I missed adding you while sending this patch set.
> 
> Can you take this third patch, in this series through your RTC subsytem
> tree? Or should I resend this third patch separately?

Ping? Any inputs? Should I resend or is this Ok?
> 
> Regards,
> Sanchayan.
> 
>>
>>>
>>> Since the DT changes cannot be applied by you, till this goes through the 
>>> RTC subsystem tree, gets mainlined and appears on your tree, I will send out 
>>> this patch by itself then and send the DT changes later once this gets
>>> mainlined?
>>>
>>> I did not CC Alessandro Zummo, the maintainer of RTC subsystem by mistake.
>>> Once you ACK/OK this i will send out this patch in isolation then or just 
>>> CC Alessandro Zummo.