[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Lokesh Vutla]

Some IP blocks like RTC, needs an additional unlocking mechanism for
writing to its registers. This patch adds optional lock and unlock
function pointers to the IP block's hwmod data which gets executed
before and after writing into IP sysconfig register.
And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod data,
so that sysconfig registers are updated properly.

Tested on:
DRA7-evm: http://pastebin.ubuntu.com/11688889/
DRA72-evm: http://pastebin.ubuntu.com/11688901/
BeagleBoard-x15: http://pastebin.ubuntu.com/11688907/
BeagleBoneBlack: http://pastebin.ubuntu.com/11688923/ 
AM437x-gp-evm: http://pastebin.ubuntu.com/11689157/ (Used an out of tree patch to enable RTC)

Lokesh Vutla (3):
  ARM: OMAP2+: hwmod: add support for lock and unlock hooks
  ARM: DRA: hwmod: RTC: Add lock and unlock functions
  ARM: AMx3xx: RTC: Add lock and unlock functions

 arch/arm/mach-omap2/omap_hwmod.c                   | 13 ++++++
 arch/arm/mach-omap2/omap_hwmod.h                   |  6 +++
 .../mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c |  2 +
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c          |  2 +
 arch/arm/mach-omap2/omap_hwmod_reset.c             | 47 ++++++++++++++++++++++
 5 files changed, 70 insertions(+)

-- 
1.9.1

[PATCH 1/3] ARM: OMAP2+: hwmod: add support for lock and unlock hooks

[Lokesh Vutla]

Some IP blocks like RTC, needs an additional setting for writing to its
registers. This is to prevent any spurious writes from changing the
register values.

This patch adds optional lock and unlock function pointers to
the IP block's hwmod data. These unlock and lock function pointers
are called by hwmod code before and after writing sysconfig registers.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 13 +++++++++++++
 arch/arm/mach-omap2/omap_hwmod.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 752969f..44c916d3 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -299,7 +299,20 @@ static void _write_sysconfig(u32 v, struct omap_hwmod *oh)
 
 	/* Module might have lost context, always update cache and register */
 	oh->_sysc_cache = v;
+
+	/*
+	 * Some IP blocks (such as RTC) require unlocking of IP before
+	 * accessing its registers. If a function pointer is present
+	 * to unlock, then call it before accessing sysconfig and
+	 * call lock after writing sysconfig.
+	 */
+	if (oh->class->unlock)
+		oh->class->unlock(oh);
+
 	omap_hwmod_write(v, oh, oh->class->sysc->sysc_offs);
+
+	if (oh->class->lock)
+		oh->class->lock(oh);
 }
 
 /**
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 9611c91..44c7db9 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -570,6 +570,8 @@ struct omap_hwmod_omap4_prcm {
  * @pre_shutdown: ptr to fn to be executed immediately prior to device shutdown
  * @reset: ptr to fn to be executed in place of the standard hwmod reset fn
  * @enable_preprogram:  ptr to fn to be executed during device enable
+ * @lock: ptr to fn to be executed to lock IP registers
+ * @unlock: ptr to fn to be executed to unlock IP registers
  *
  * Represent the class of a OMAP hardware "modules" (e.g. timer,
  * smartreflex, gpio, uart...)
@@ -594,6 +596,8 @@ struct omap_hwmod_class {
 	int					(*pre_shutdown)(struct omap_hwmod *oh);
 	int					(*reset)(struct omap_hwmod *oh);
 	int					(*enable_preprogram)(struct omap_hwmod *oh);
+	void				(*lock)(struct omap_hwmod *oh);
+	void				(*unlock)(struct omap_hwmod *oh);
 };
 
 /**
-- 
1.9.1

[PATCH 1/3] ARM: OMAP2+: hwmod: add support for lock and unlock hooks

[Paul Walmsley]

On Wed, 10 Jun 2015, Lokesh Vutla wrote:

> Some IP blocks like RTC, needs an additional setting for writing to its
> registers. This is to prevent any spurious writes from changing the
> register values.
> 
> This patch adds optional lock and unlock function pointers to
> the IP block's hwmod data. These unlock and lock function pointers
> are called by hwmod code before and after writing sysconfig registers.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Thanks, queued for v4.3.

- Paul

[PATCH 2/3] ARM: DRA: hwmod: RTC: Add lock and unlock functions

[Lokesh Vutla]

RTC IP have kicker feature which prevents spurious writes to its registers.
In order to write into any of the RTC registers, KICK values has to be
written to KICK registers.

Introduce omap_hwmod_rtc_unlock/lock functions, which  writes into these
KICK registers inorder to lock and unlock RTC registers.
Also hook these functions to RTC hwmod.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.h          |  2 ++
 arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  2 ++
 arch/arm/mach-omap2/omap_hwmod_reset.c    | 47 +++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 44c7db9..04855ab 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -742,6 +742,8 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh);
  */
 
 extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh);
+void omap_hwmod_rtc_unlock(struct omap_hwmod *oh);
+void omap_hwmod_rtc_lock(struct omap_hwmod *oh);
 
 /*
  * Chip variant-specific hwmod init routines - XXX should be converted
diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
index 0e64c2f..983042f 100644
--- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
@@ -1548,6 +1548,8 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = {
 static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = {
 	.name	= "rtcss",
 	.sysc	= &dra7xx_rtcss_sysc,
+	.unlock	= &omap_hwmod_rtc_unlock,
+	.lock	= &omap_hwmod_rtc_lock,
 };
 
 /* rtcss */
diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c
index 65e186c..1fb106d 100644
--- a/arch/arm/mach-omap2/omap_hwmod_reset.c
+++ b/arch/arm/mach-omap2/omap_hwmod_reset.c
@@ -25,11 +25,20 @@
  */
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/delay.h>
 
 #include <sound/aess.h>
 
 #include "omap_hwmod.h"
 
+#define OMAP_RTC_STATUS_REG	0x44
+#define OMAP_RTC_KICK0_REG	0x6c
+#define OMAP_RTC_KICK1_REG	0x70
+
+#define OMAP_RTC_KICK0_VALUE	0x83E70B13
+#define OMAP_RTC_KICK1_VALUE	0x95A4F1E0
+#define OMAP_RTC_STATUS_BUSY	BIT(0)
+
 /**
  * omap_hwmod_aess_preprogram - enable AESS internal autogating
  * @oh: struct omap_hwmod *
@@ -51,3 +60,41 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh)
 
 	return 0;
 }
+
+static void omap_rtc_wait_not_busy(struct omap_hwmod *oh)
+{
+	int count;
+	u8 status;
+
+	/* BUSY may stay active for 1/32768 second (~30 usec) */
+	for (count = 0; count < 50; count++) {
+		status = omap_hwmod_read(oh, OMAP_RTC_STATUS_REG);
+		if (!(status & OMAP_RTC_STATUS_BUSY))
+			break;
+		udelay(1);
+	}
+	/* now we have ~15 usec to read/write various registers */
+}
+
+/**
+ * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism.
+ * @oh: struct omap_hwmod *
+ *
+ * RTC IP have kicker feature.  This prevents spurious writes to its registers.
+ * In order to write into any of the RTC registers, KICK values has te be
+ * written in respective KICK registers. This is needed for hwmod to write into
+ * sysconfig register.
+ */
+void omap_hwmod_rtc_unlock(struct omap_hwmod *oh)
+{
+	omap_rtc_wait_not_busy(oh);
+	omap_hwmod_write(OMAP_RTC_KICK0_VALUE, oh, OMAP_RTC_KICK0_REG);
+	omap_hwmod_write(OMAP_RTC_KICK1_VALUE, oh, OMAP_RTC_KICK1_REG);
+}
+
+void omap_hwmod_rtc_lock(struct omap_hwmod *oh)
+{
+	omap_rtc_wait_not_busy(oh);
+	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK0_REG);
+	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK1_REG);
+}
-- 
1.9.1

[PATCH 2/3] ARM: DRA: hwmod: RTC: Add lock and unlock functions

[Paul Walmsley]

Hi, 

some comments.

On Wed, 10 Jun 2015, Lokesh Vutla wrote:

> RTC IP have kicker feature which prevents spurious writes to its registers.
> In order to write into any of the RTC registers, KICK values has to be
> written to KICK registers.
> 
> Introduce omap_hwmod_rtc_unlock/lock functions, which  writes into these
> KICK registers inorder to lock and unlock RTC registers.
> Also hook these functions to RTC hwmod.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.h          |  2 ++
>  arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  2 ++
>  arch/arm/mach-omap2/omap_hwmod_reset.c    | 47 +++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 44c7db9..04855ab 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -742,6 +742,8 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh);
>   */
>  
>  extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh);
> +void omap_hwmod_rtc_unlock(struct omap_hwmod *oh);
> +void omap_hwmod_rtc_lock(struct omap_hwmod *oh);
>  
>  /*
>   * Chip variant-specific hwmod init routines - XXX should be converted
> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> index 0e64c2f..983042f 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> @@ -1548,6 +1548,8 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = {
>  static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = {
>  	.name	= "rtcss",
>  	.sysc	= &dra7xx_rtcss_sysc,
> +	.unlock	= &omap_hwmod_rtc_unlock,
> +	.lock	= &omap_hwmod_rtc_lock,
>  };
>  
>  /* rtcss */

Is the DRA7xx the only chip that has this lock/unlock feature, or do other 
OMAP chips use the same RTC IP block?

> diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c
> index 65e186c..1fb106d 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_reset.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c
> @@ -25,11 +25,20 @@
>   */
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> +#include <linux/delay.h>
>  
>  #include <sound/aess.h>
>  
>  #include "omap_hwmod.h"
>  
> +#define OMAP_RTC_STATUS_REG	0x44
> +#define OMAP_RTC_KICK0_REG	0x6c
> +#define OMAP_RTC_KICK1_REG	0x70
> +
> +#define OMAP_RTC_KICK0_VALUE	0x83E70B13
> +#define OMAP_RTC_KICK1_VALUE	0x95A4F1E0
> +#define OMAP_RTC_STATUS_BUSY	BIT(0)
> +
>  /**
>   * omap_hwmod_aess_preprogram - enable AESS internal autogating
>   * @oh: struct omap_hwmod *
> @@ -51,3 +60,41 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh)
>  
>  	return 0;
>  }
> +
> +static void omap_rtc_wait_not_busy(struct omap_hwmod *oh)

This function is missing kerneldoc and desperately needs it.

> +{
> +	int count;
> +	u8 status;
> +
> +	/* BUSY may stay active for 1/32768 second (~30 usec) */
> +	for (count = 0; count < 50; count++) {

OK, I give up.  Where does 50 come from?  This should be moved into a 
macro with a comment,@the very least.

> +		status = omap_hwmod_read(oh, OMAP_RTC_STATUS_REG);
> +		if (!(status & OMAP_RTC_STATUS_BUSY))
> +			break;
> +		udelay(1);
> +	}
> +	/* now we have ~15 usec to read/write various registers */
> +}
> +
> +/**
> + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism.
> + * @oh: struct omap_hwmod *
> + *
> + * RTC IP have kicker feature.  This prevents spurious writes to its registers.
> + * In order to write into any of the RTC registers, KICK values has te be
> + * written in respective KICK registers. This is needed for hwmod to write into
> + * sysconfig register.
> + */
> +void omap_hwmod_rtc_unlock(struct omap_hwmod *oh)
> +{
> +	omap_rtc_wait_not_busy(oh);

What prevents the CPU from context-switching away after the BUSY bit test 
and not returning back here by the time the ~15 ?s interval has ended?  
Looks to me like this whole thing needs to run with interrupts disabled.

> +	omap_hwmod_write(OMAP_RTC_KICK0_VALUE, oh, OMAP_RTC_KICK0_REG);
> +	omap_hwmod_write(OMAP_RTC_KICK1_VALUE, oh, OMAP_RTC_KICK1_REG);
> +}
> +
> +void omap_hwmod_rtc_lock(struct omap_hwmod *oh)
> +{
> +	omap_rtc_wait_not_busy(oh);

Same comment as the above.

> +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK0_REG);
> +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK1_REG);
> +}
> -- 
> 1.9.1
> 


- Paul

[PATCH 2/3] ARM: DRA: hwmod: RTC: Add lock and unlock functions

[Lokesh Vutla]

Hi Paul,
On Thursday 16 July 2015 05:43 AM, Paul Walmsley wrote:
> Hi,
>
> some comments.
>
> On Wed, 10 Jun 2015, Lokesh Vutla wrote:
>
>> RTC IP have kicker feature which prevents spurious writes to its registers.
>> In order to write into any of the RTC registers, KICK values has to be
>> written to KICK registers.
>>
>> Introduce omap_hwmod_rtc_unlock/lock functions, which  writes into these
>> KICK registers inorder to lock and unlock RTC registers.
>> Also hook these functions to RTC hwmod.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>   arch/arm/mach-omap2/omap_hwmod.h          |  2 ++
>>   arch/arm/mach-omap2/omap_hwmod_7xx_data.c |  2 ++
>>   arch/arm/mach-omap2/omap_hwmod_reset.c    | 47 +++++++++++++++++++++++++++++++
>>   3 files changed, 51 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
>> index 44c7db9..04855ab 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.h
>> +++ b/arch/arm/mach-omap2/omap_hwmod.h
>> @@ -742,6 +742,8 @@ const char *omap_hwmod_get_main_clk(struct omap_hwmod *oh);
>>    */
>>
>>   extern int omap_hwmod_aess_preprogram(struct omap_hwmod *oh);
>> +void omap_hwmod_rtc_unlock(struct omap_hwmod *oh);
>> +void omap_hwmod_rtc_lock(struct omap_hwmod *oh);
>>
>>   /*
>>    * Chip variant-specific hwmod init routines - XXX should be converted
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> index 0e64c2f..983042f 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> @@ -1548,6 +1548,8 @@ static struct omap_hwmod_class_sysconfig dra7xx_rtcss_sysc = {
>>   static struct omap_hwmod_class dra7xx_rtcss_hwmod_class = {
>>   	.name	= "rtcss",
>>   	.sysc	= &dra7xx_rtcss_sysc,
>> +	.unlock	= &omap_hwmod_rtc_unlock,
>> +	.lock	= &omap_hwmod_rtc_lock,
>>   };
>>
>>   /* rtcss */
>
> Is the DRA7xx the only chip that has this lock/unlock feature, or do other
> OMAP chips use the same RTC IP block?
>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_reset.c b/arch/arm/mach-omap2/omap_hwmod_reset.c
>> index 65e186c..1fb106d 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_reset.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_reset.c
>> @@ -25,11 +25,20 @@
>>    */
>>   #include <linux/kernel.h>
>>   #include <linux/errno.h>
>> +#include <linux/delay.h>
>>
>>   #include <sound/aess.h>
>>
>>   #include "omap_hwmod.h"
>>
>> +#define OMAP_RTC_STATUS_REG	0x44
>> +#define OMAP_RTC_KICK0_REG	0x6c
>> +#define OMAP_RTC_KICK1_REG	0x70
>> +
>> +#define OMAP_RTC_KICK0_VALUE	0x83E70B13
>> +#define OMAP_RTC_KICK1_VALUE	0x95A4F1E0
>> +#define OMAP_RTC_STATUS_BUSY	BIT(0)
>> +
>>   /**
>>    * omap_hwmod_aess_preprogram - enable AESS internal autogating
>>    * @oh: struct omap_hwmod *
>> @@ -51,3 +60,41 @@ int omap_hwmod_aess_preprogram(struct omap_hwmod *oh)
>>
>>   	return 0;
>>   }
>> +
>> +static void omap_rtc_wait_not_busy(struct omap_hwmod *oh)
>
> This function is missing kerneldoc and desperately needs it.
I should have written it, sorry about that.
For updating certain RTC registers, the MPU must wait for the BUSY 
status to become zero. Once the BUSY flag is zero, there is a 15-?s 
access period in which the MPU can program.

>
>> +{
>> +	int count;
>> +	u8 status;
>> +
>> +	/* BUSY may stay active for 1/32768 second (~30 usec) */
>> +	for (count = 0; count < 50; count++) {
>
> OK, I give up.  Where does 50 come from?  This should be moved into a
> macro with a comment, at the very least.
>
>> +		status = omap_hwmod_read(oh, OMAP_RTC_STATUS_REG);
>> +		if (!(status & OMAP_RTC_STATUS_BUSY))
>> +			break;
>> +		udelay(1);
>> +	}
>> +	/* now we have ~15 usec to read/write various registers */
>> +}
>> +
>> +/**
>> + * omap_hwmod_rtc_unlock - Reset and unlock the Kicker mechanism.
>> + * @oh: struct omap_hwmod *
>> + *
>> + * RTC IP have kicker feature.  This prevents spurious writes to its registers.
>> + * In order to write into any of the RTC registers, KICK values has te be
>> + * written in respective KICK registers. This is needed for hwmod to write into
>> + * sysconfig register.
>> + */
>> +void omap_hwmod_rtc_unlock(struct omap_hwmod *oh)
>> +{
>> +	omap_rtc_wait_not_busy(oh);
>
> What prevents the CPU from context-switching away after the BUSY bit test
> and not returning back here by the time the ~15 ?s interval has ended?
> Looks to me like this whole thing needs to run with interrupts disabled.
Yes, you are correct. Ill update it and repost.

Thanks and regards,
Lokesh
>
>> +	omap_hwmod_write(OMAP_RTC_KICK0_VALUE, oh, OMAP_RTC_KICK0_REG);
>> +	omap_hwmod_write(OMAP_RTC_KICK1_VALUE, oh, OMAP_RTC_KICK1_REG);
>> +}
>> +
>> +void omap_hwmod_rtc_lock(struct omap_hwmod *oh)
>> +{
>> +	omap_rtc_wait_not_busy(oh);
>
> Same comment as the above.
>
>> +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK0_REG);
>> +	omap_hwmod_write(0x0, oh, OMAP_RTC_KICK1_REG);
>> +}
>> --
>> 1.9.1
>>
>
>
> - Paul
>

[PATCH 3/3] ARM: AMx3xx: hwmod: RTC: Add lock and unlock functions

[Lokesh Vutla]

RTC IP have kicker feature which prevents spurious writes to its registers.
In order to write into any of the RTC registers, KICK values has to be
written to KICK registers.

omap_hwmod_rtc_unlock/lock functions writes into these KICK registers
inorder to lock and unlock RTC registers.
This patch hooks omap_hwmod_rtc_unlock/lock functions into RTC hwmod,
so that SYSCONFIG register is updated properly.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
index cabc569..2d92958 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
@@ -904,6 +904,8 @@ static struct omap_hwmod_class_sysconfig am33xx_rtc_sysc = {
 static struct omap_hwmod_class am33xx_rtc_hwmod_class = {
 	.name		= "rtc",
 	.sysc		= &am33xx_rtc_sysc,
+	.unlock		= &omap_hwmod_rtc_unlock,
+	.lock		= &omap_hwmod_rtc_lock,
 };
 
 struct omap_hwmod am33xx_rtc_hwmod = {
-- 
1.9.1

[PATCH 3/3] ARM: AMx3xx: hwmod: RTC: Add lock and unlock functions

[Paul Walmsley]

Hi

On Wed, 10 Jun 2015, Lokesh Vutla wrote:

> RTC IP have kicker feature which prevents spurious writes to its registers.
> In order to write into any of the RTC registers, KICK values has to be
> written to KICK registers.
> 
> omap_hwmod_rtc_unlock/lock functions writes into these KICK registers
> inorder to lock and unlock RTC registers.
> This patch hooks omap_hwmod_rtc_unlock/lock functions into RTC hwmod,
> so that SYSCONFIG register is updated properly.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
> index cabc569..2d92958 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
> @@ -904,6 +904,8 @@ static struct omap_hwmod_class_sysconfig am33xx_rtc_sysc = {
>  static struct omap_hwmod_class am33xx_rtc_hwmod_class = {
>  	.name		= "rtc",
>  	.sysc		= &am33xx_rtc_sysc,
> +	.unlock		= &omap_hwmod_rtc_unlock,
> +	.lock		= &omap_hwmod_rtc_lock,
>  };
>  
>  struct omap_hwmod am33xx_rtc_hwmod = {

The DRA7xx integration from the previous patch should either be combined 
with this patch, or moved into a separate patch like this one (your 
preference).


- Paul

[PATCH 3/3] ARM: AMx3xx: hwmod: RTC: Add lock and unlock functions

[Lokesh Vutla]

Hi Paul,
On Thursday 16 July 2015 05:44 AM, Paul Walmsley wrote:
> Hi
>
> On Wed, 10 Jun 2015, Lokesh Vutla wrote:
>
>> RTC IP have kicker feature which prevents spurious writes to its registers.
>> In order to write into any of the RTC registers, KICK values has to be
>> written to KICK registers.
>>
>> omap_hwmod_rtc_unlock/lock functions writes into these KICK registers
>> inorder to lock and unlock RTC registers.
>> This patch hooks omap_hwmod_rtc_unlock/lock functions into RTC hwmod,
>> so that SYSCONFIG register is updated properly.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>   arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
>> index cabc569..2d92958 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c
>> @@ -904,6 +904,8 @@ static struct omap_hwmod_class_sysconfig am33xx_rtc_sysc = {
>>   static struct omap_hwmod_class am33xx_rtc_hwmod_class = {
>>   	.name		= "rtc",
>>   	.sysc		= &am33xx_rtc_sysc,
>> +	.unlock		= &omap_hwmod_rtc_unlock,
>> +	.lock		= &omap_hwmod_rtc_lock,
>>   };
>>
>>   struct omap_hwmod am33xx_rtc_hwmod = {
>
> The DRA7xx integration from the previous patch should either be combined
> with this patch, or moved into a separate patch like this one (your
> preference).
Ill make a separate patch for DRA7xx integration and repost it.

Thanks and regards,
Lokesh
>
>
> - Paul
>

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Lokesh Vutla]

Hi,
On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
> Some IP blocks like RTC, needs an additional unlocking mechanism for
> writing to its registers. This patch adds optional lock and unlock
> function pointers to the IP block's hwmod data which gets executed
> before and after writing into IP sysconfig register.
> And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod data,
> so that sysconfig registers are updated properly.
ping on this series.

Thanks and regards,
Lokesh
> 
> Tested on:
> DRA7-evm: http://pastebin.ubuntu.com/11688889/
> DRA72-evm: http://pastebin.ubuntu.com/11688901/
> BeagleBoard-x15: http://pastebin.ubuntu.com/11688907/
> BeagleBoneBlack: http://pastebin.ubuntu.com/11688923/ 
> AM437x-gp-evm: http://pastebin.ubuntu.com/11689157/ (Used an out of tree patch to enable RTC)
> 
> Lokesh Vutla (3):
>   ARM: OMAP2+: hwmod: add support for lock and unlock hooks
>   ARM: DRA: hwmod: RTC: Add lock and unlock functions
>   ARM: AMx3xx: RTC: Add lock and unlock functions
> 
>  arch/arm/mach-omap2/omap_hwmod.c                   | 13 ++++++
>  arch/arm/mach-omap2/omap_hwmod.h                   |  6 +++
>  .../mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c |  2 +
>  arch/arm/mach-omap2/omap_hwmod_7xx_data.c          |  2 +
>  arch/arm/mach-omap2/omap_hwmod_reset.c             | 47 ++++++++++++++++++++++
>  5 files changed, 70 insertions(+)
> 

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Tero Kristo]

On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
> Hi,
> On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
>> Some IP blocks like RTC, needs an additional unlocking mechanism for
>> writing to its registers. This patch adds optional lock and unlock
>> function pointers to the IP block's hwmod data which gets executed
>> before and after writing into IP sysconfig register.
>> And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod data,
>> so that sysconfig registers are updated properly.
> ping on this series.
>
> Thanks and regards,
> Lokesh

This looks kind of hackish to have the unlock + lock functionality copy 
pasted to both driver and hwmod.

It is also racy, as there is no locking in place to avoid concurrent 
access to the lock/unlock registers across hwmod+driver.

Can we avoid these issues somehow?

-Tero

>>
>> Tested on:
>> DRA7-evm: http://pastebin.ubuntu.com/11688889/
>> DRA72-evm: http://pastebin.ubuntu.com/11688901/
>> BeagleBoard-x15: http://pastebin.ubuntu.com/11688907/
>> BeagleBoneBlack: http://pastebin.ubuntu.com/11688923/
>> AM437x-gp-evm: http://pastebin.ubuntu.com/11689157/ (Used an out of tree patch to enable RTC)
>>
>> Lokesh Vutla (3):
>>    ARM: OMAP2+: hwmod: add support for lock and unlock hooks
>>    ARM: DRA: hwmod: RTC: Add lock and unlock functions
>>    ARM: AMx3xx: RTC: Add lock and unlock functions
>>
>>   arch/arm/mach-omap2/omap_hwmod.c                   | 13 ++++++
>>   arch/arm/mach-omap2/omap_hwmod.h                   |  6 +++
>>   .../mach-omap2/omap_hwmod_33xx_43xx_ipblock_data.c |  2 +
>>   arch/arm/mach-omap2/omap_hwmod_7xx_data.c          |  2 +
>>   arch/arm/mach-omap2/omap_hwmod_reset.c             | 47 ++++++++++++++++++++++
>>   5 files changed, 70 insertions(+)
>>
>

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Paul Walmsley]

On Tue, 14 Jul 2015, Tero Kristo wrote:

> On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
> > Hi,
> > On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
> > > Some IP blocks like RTC, needs an additional unlocking mechanism for
> > > writing to its registers. This patch adds optional lock and unlock
> > > function pointers to the IP block's hwmod data which gets executed
> > > before and after writing into IP sysconfig register.
> > > And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod data,
> > > so that sysconfig registers are updated properly.
> > ping on this series.
> > 
> > Thanks and regards,
> > Lokesh
> 

[...]

> It is also racy, as there is no locking in place to avoid concurrent access to
> the lock/unlock registers across hwmod+driver.

I don't see the race.  Where is it?


- Paul

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Tero Kristo]

On 07/16/2015 03:15 AM, Paul Walmsley wrote:
> On Tue, 14 Jul 2015, Tero Kristo wrote:
>
>> On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
>>> Hi,
>>> On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
>>>> Some IP blocks like RTC, needs an additional unlocking mechanism for
>>>> writing to its registers. This patch adds optional lock and unlock
>>>> function pointers to the IP block's hwmod data which gets executed
>>>> before and after writing into IP sysconfig register.
>>>> And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod data,
>>>> so that sysconfig registers are updated properly.
>>> ping on this series.
>>>
>>> Thanks and regards,
>>> Lokesh
>>
>
> [...]
>
>> It is also racy, as there is no locking in place to avoid concurrent access to
>> the lock/unlock registers across hwmod+driver.
>
> I don't see the race.  Where is it?

See drivers/rtc/rtc-omap.c, am3352_rtc_unlock and am3352_rtc_lock.

That code is accessing the exact same registers.

-Tero

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Paul Walmsley]

On Thu, 16 Jul 2015, Tero Kristo wrote:

> On 07/16/2015 03:15 AM, Paul Walmsley wrote:
> > On Tue, 14 Jul 2015, Tero Kristo wrote:
> > 
> > > On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
> > > > Hi,
> > > > On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
> > > > > Some IP blocks like RTC, needs an additional unlocking mechanism for
> > > > > writing to its registers. This patch adds optional lock and unlock
> > > > > function pointers to the IP block's hwmod data which gets executed
> > > > > before and after writing into IP sysconfig register.
> > > > > And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod
> > > > > data,
> > > > > so that sysconfig registers are updated properly.
> > > > ping on this series.
> > > > 
> > > > Thanks and regards,
> > > > Lokesh
> > > 
> > 
> > [...]
> > 
> > > It is also racy, as there is no locking in place to avoid concurrent
> > > access to
> > > the lock/unlock registers across hwmod+driver.
> > 
> > I don't see the race.  Where is it?
> 
> See drivers/rtc/rtc-omap.c, am3352_rtc_unlock and am3352_rtc_lock.
> 
> That code is accessing the exact same registers.

I guess my question is, when is it possible that code could race with the 
hwmod code for the same device?


- Paul

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Tero Kristo]

On 07/16/2015 01:13 PM, Paul Walmsley wrote:
> On Thu, 16 Jul 2015, Tero Kristo wrote:
>
>> On 07/16/2015 03:15 AM, Paul Walmsley wrote:
>>> On Tue, 14 Jul 2015, Tero Kristo wrote:
>>>
>>>> On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
>>>>> Hi,
>>>>> On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
>>>>>> Some IP blocks like RTC, needs an additional unlocking mechanism for
>>>>>> writing to its registers. This patch adds optional lock and unlock
>>>>>> function pointers to the IP block's hwmod data which gets executed
>>>>>> before and after writing into IP sysconfig register.
>>>>>> And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod
>>>>>> data,
>>>>>> so that sysconfig registers are updated properly.
>>>>> ping on this series.
>>>>>
>>>>> Thanks and regards,
>>>>> Lokesh
>>>>
>>>
>>> [...]
>>>
>>>> It is also racy, as there is no locking in place to avoid concurrent
>>>> access to
>>>> the lock/unlock registers across hwmod+driver.
>>>
>>> I don't see the race.  Where is it?
>>
>> See drivers/rtc/rtc-omap.c, am3352_rtc_unlock and am3352_rtc_lock.
>>
>> That code is accessing the exact same registers.
>
> I guess my question is, when is it possible that code could race with the
> hwmod code for the same device?

Hmm yea I think you are right, this only gets potentially called within 
pm_runtime_get/put_sync for RTC.

The current sequence is highly inefficient though, as we are doing 
multiple lock/unlock operations to the RTC from multiple sources. See 
following rtcwake trace on am43xx-gp-evm as an example.


/ # rtcwake -s 4 -m mem
[    7.425322] am3352_rtc_unlock
[    7.428330] am3352_rtc_lock
[    7.431139] am3352_rtc_unlock
[    7.434116] am3352_rtc_lock
wakeup from "mem" at Sat Jan  1 00:00:11 2000
[    7.448549] PM: Syncing filesystems ... done.
[    7.455425] Freezing user space processes ... (elapsed 0.001 seconds) 
done.
[    7.463738] Freezing remaining freezable tasks ... (elapsed 0.001 
seconds) do
ne.
[    7.472532] Suspending console(s) (use no_console_suspend to debug)
[    7.481878] am3352_rtc_unlock
[    7.481889] am3352_rtc_lock
[    7.482307] PM: suspend of devices complete after 2.713 msecs
[    7.483479] PM: late suspend of devices complete after 1.153 msecs
[    7.484727] omap_hwmod_rtc_unlock
[    7.484733] omap_hwmod_rtc_lock
[    7.485182] PM: noirq suspend of devices complete after 1.685 msecs
[    7.485190] Disabling non-boot CPUs ...
[    7.485199] PM: Successfully put all powerdomains to target state
[    7.485199] PM: Wakeup source RTC Alarm
[    7.499853] PM: noirq resume of devices complete after 14.558 msecs
[    7.500047] am3352_rtc_unlock
[    7.500052] am3352_rtc_lock
[    7.500123] am3352_rtc_unlock
[    7.500128] am3352_rtc_lock
[    7.501019] PM: early resume of devices complete after 0.809 msecs
[    7.501464] am3352_rtc_unlock
[    7.501472] am3352_rtc_lock
[    7.558046] PM: resume of devices complete after 57.007 msecs
[    7.638807] Restarting tasks ... done.
[    7.643173] am3352_rtc_unlock
[    7.646162] am3352_rtc_lock

But, I guess this is for some interested party to optimize if needed, 
and it is mostly an issue with the RTC driver itself.

-Tero

[PATCH 0/3] ARM: OMAP2+: hwmod: RTC: Add lock and unlock hooks

[Lokesh Vutla]

Hi Tero,
On Thursday 16 July 2015 05:33 PM, Tero Kristo wrote:
> On 07/16/2015 01:13 PM, Paul Walmsley wrote:
>> On Thu, 16 Jul 2015, Tero Kristo wrote:
>>
>>> On 07/16/2015 03:15 AM, Paul Walmsley wrote:
>>>> On Tue, 14 Jul 2015, Tero Kristo wrote:
>>>>
>>>>> On 07/14/2015 01:09 PM, Lokesh Vutla wrote:
>>>>>> Hi,
>>>>>> On Wednesday 10 June 2015 02:56 PM, Lokesh Vutla wrote:
>>>>>>> Some IP blocks like RTC, needs an additional unlocking mechanism for
>>>>>>> writing to its registers. This patch adds optional lock and unlock
>>>>>>> function pointers to the IP block's hwmod data which gets executed
>>>>>>> before and after writing into IP sysconfig register.
>>>>>>> And also hook lock and unlock functions to AMx3xx, DRA7 RTC hwmod
>>>>>>> data,
>>>>>>> so that sysconfig registers are updated properly.
>>>>>> ping on this series.
>>>>>>
>>>>>> Thanks and regards,
>>>>>> Lokesh
>>>>>
>>>>
>>>> [...]
>>>>
>>>>> It is also racy, as there is no locking in place to avoid concurrent
>>>>> access to
>>>>> the lock/unlock registers across hwmod+driver.
>>>>
>>>> I don't see the race.  Where is it?
>>>
>>> See drivers/rtc/rtc-omap.c, am3352_rtc_unlock and am3352_rtc_lock.
>>>
>>> That code is accessing the exact same registers.
>>
>> I guess my question is, when is it possible that code could race with the
>> hwmod code for the same device?
>
> Hmm yea I think you are right, this only gets potentially called within
> pm_runtime_get/put_sync for RTC.
Yes, sysc is written from hwmod_init code and pm_runtime_get/put_sysnc.
And we write into rtc registers only after pm_runtime_get_sync and 
rtc_unlock.
There cannot be a race condition here.
>
> The current sequence is highly inefficient though, as we are doing
> multiple lock/unlock operations to the RTC from multiple sources. See
> following rtcwake trace on am43xx-gp-evm as an example.
Initially I had a patch which leaves rtc unlocked at hwmod_init instead 
of doing
unlock and lock for each set of register writes in the driver.
But it was rejected stating that deviates the purpose of locking mechanism.
So I updated the driver for adapting this locking and unlocking mechanism.

Thanks and regards,
Lokesh

>
>
> / # rtcwake -s 4 -m mem
> [    7.425322] am3352_rtc_unlock
> [    7.428330] am3352_rtc_lock
> [    7.431139] am3352_rtc_unlock
> [    7.434116] am3352_rtc_lock
> wakeup from "mem" at Sat Jan  1 00:00:11 2000
> [    7.448549] PM: Syncing filesystems ... done.
> [    7.455425] Freezing user space processes ... (elapsed 0.001 seconds)
> done.
> [    7.463738] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) do
> ne.
> [    7.472532] Suspending console(s) (use no_console_suspend to debug)
> [    7.481878] am3352_rtc_unlock
> [    7.481889] am3352_rtc_lock
> [    7.482307] PM: suspend of devices complete after 2.713 msecs
> [    7.483479] PM: late suspend of devices complete after 1.153 msecs
> [    7.484727] omap_hwmod_rtc_unlock
> [    7.484733] omap_hwmod_rtc_lock
> [    7.485182] PM: noirq suspend of devices complete after 1.685 msecs
> [    7.485190] Disabling non-boot CPUs ...
> [    7.485199] PM: Successfully put all powerdomains to target state
> [    7.485199] PM: Wakeup source RTC Alarm
> [    7.499853] PM: noirq resume of devices complete after 14.558 msecs
> [    7.500047] am3352_rtc_unlock
> [    7.500052] am3352_rtc_lock
> [    7.500123] am3352_rtc_unlock
> [    7.500128] am3352_rtc_lock
> [    7.501019] PM: early resume of devices complete after 0.809 msecs
> [    7.501464] am3352_rtc_unlock
> [    7.501472] am3352_rtc_lock
> [    7.558046] PM: resume of devices complete after 57.007 msecs
> [    7.638807] Restarting tasks ... done.
> [    7.643173] am3352_rtc_unlock
> [    7.646162] am3352_rtc_lock
>
> But, I guess this is for some interested party to optimize if needed,
> and it is mostly an issue with the RTC driver itself.
>
> -Tero
>