[RFC 1/5] rtc-at91rm9200: add configuration support

[RFC 1/5] rtc-at91rm9200: add configuration support

[Johan Hovold]

Add configuration support which can be used to implement SoC-specific
workarounds for broken hardware.
---
 drivers/rtc/rtc-at91rm9200.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 434ebc3..5bae0a1 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -40,6 +40,10 @@
 
 #define AT91_RTC_EPOCH		1900UL	/* just like arch/arm/common/rtctime.c */
 
+struct at91_rtc_config {
+};
+
+static const struct at91_rtc_config *at91_rtc_config;
 static DECLARE_COMPLETION(at91_rtc_updated);
 static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
@@ -248,6 +252,15 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	return IRQ_NONE;		/* not handled */
 }
 
+static const struct at91_rtc_config at91rm9200_config = {
+};
+
+static const struct at91_rtc_config *
+at91_rtc_get_config(struct platform_device *pdev)
+{
+	return &at91rm9200_config;
+}
+
 static const struct rtc_class_ops at91_rtc_ops = {
 	.read_time	= at91_rtc_readtime,
 	.set_time	= at91_rtc_settime,
@@ -266,6 +279,10 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	struct resource *regs;
 	int ret = 0;
 
+	at91_rtc_config = at91_rtc_get_config(pdev);
+	if (!at91_rtc_config)
+		return -ENODEV;
+
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!regs) {
 		dev_err(&pdev->dev, "no mmio resource defined\n");
-- 
1.8.1.5

[RFC 2/5] rtc-at91rm9200: add device-tree support

[Johan Hovold]

On Fri, Mar 29, 2013 at 05:03:46PM +0100, Johan Hovold wrote:
> Add device tree support.
> ---
>  drivers/rtc/rtc-at91rm9200.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> index 5bae0a1..67260f9 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -28,6 +28,7 @@
>  #include <linux/ioctl.h>
>  #include <linux/completion.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -255,9 +256,30 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
>  static const struct at91_rtc_config at91rm9200_config = {
>  };
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id at91_rtc_dt_ids[] = {
> +	{
> +		.compatible = "atmel,at91rm9200-rtc",
> +		.data = &at91rm9200_config,
>+	},

There's a missing brace here. Will fix after any further feedback.

> +		/* terminator */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
> +#endif

[RFC 5/5] rtc-at91rm9200: add support for at91sam9x5

[Douglas Gilbert]

On 13-03-29 12:03 PM, Johan Hovold wrote:
> Add support for the at91sam9x5-family which must use the shadow
> interrupt mask due to a hardware issue.
> ---
>   drivers/rtc/rtc-at91rm9200.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> index 2921866..f3e351f 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -318,12 +318,20 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
>   static const struct at91_rtc_config at91rm9200_config = {
>   };
>
> +static const struct at91_rtc_config at91sam9x5_config = {
> +	.use_shadow_imr = true,
> +};
> +
>   #if defined(CONFIG_OF)
>   static const struct of_device_id at91_rtc_dt_ids[] = {
>   	{
>   		.compatible = "atmel,at91rm9200-rtc",
>   		.data = &at91rm9200_config,
>   	},
> +	{
> +		.compatible = "atmel,at91sam9x5-rtc",
> +		.data = &at91sam9x5_config,
> +	},
>   		/* terminator */
>   	}
>   };
>

Johan,
Looks good.

Plus add something like this to at91sam9x5.dtsi after the
i2c at 2 entry (at the end):

         rtc {
                 compatible = "atmel,at91sam9x5-rtc";
                 reg = <0xfffffeb0 0x40>;
                 interrupts = <1 4 7>;
                 status = "disabled";
         };


and an "enabler" in ariag25.dts (and perhaps other members
of the 9x5 sub-family), also at the end:

         rtc {
                 status = "okay";
         };

My patches are in Robert Nelson's tree at:
    http://www.eewiki.net/display/linuxonarm/AT91SAM9x5
in the Linux kernel section. My RTC code amounts to the same
thing as you are proposing, without the safety code around
the IMR shadow.

I provide binaries based on that work to Aria G25 users
via a google group. No-one has complained about RTC not
working. SPI and I2C problems are on-going but gradually
being sorted. Hence I know people are using and testing
this code, other than me.

Doug Gilbert

[RFC PATCH] rtc: rtc-at91rm9200: manage IMR depending on revision

[Nicolas Ferre]

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi all,

The funny thing is that I was writing exactly the same code as Johan's
when he posted his series.

So, here is my single patch, with the comment about the readback stolen from
Johan's, but without the way to determine with IP is buggy and which one is
not...
After having dug the possibility to read the IP revision, I discovered that it
is not possible to use this information ("version" register offset changing
according to... IP version number: well done!).
In conclusion, I guess that the only way to determine if we need the workaround
is to use the DT.
One remark though: if we use the compatibility string for this purpose, I fear
that we would twist the meaning of this information: SoC using an
"atmel,at91sam9x5-rtc" compatible RTC will not necessarily be touched by the
"non responding IMR" bug: at91sam9n12 or upcoming sama5d3 are not affected for
instance, and we need to cling to "atmel,at91rm9200-rtc" for them...
I think that we can use this method for the moment and move to another
compatibility string later if it is needed.

Thanks for your help, best regards.

 drivers/rtc/rtc-at91rm9200.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/rtc/rtc-at91rm9200.h |  1 +
 2 files changed, 76 insertions(+)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 29b92e4..4960d42 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -25,6 +25,7 @@
 #include <linux/rtc.h>
 #include <linux/bcd.h>
 #include <linux/interrupt.h>
+#include <linux/spinlock.h>
 #include <linux/ioctl.h>
 #include <linux/completion.h>
 #include <linux/io.h>
@@ -47,6 +48,74 @@ static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 static u32 at91_rtc_imr;
+static DEFINE_SPINLOCK(lock);
+static void (*at91_rtc_set_irq)(u32 irq_mask);
+static void (*at91_rtc_clear_irq)(u32 irq_mask);
+static u32 (*at91_rtc_read_imr)(void);
+
+static inline unsigned int at91_rtc_get_version(void)
+{
+	return at91_rtc_read(AT91_RTC_VERSION) & 0x00000fff;
+}
+
+static void at91_rtc_set_irq_simple(u32 irq_mask)
+{
+	at91_rtc_write(AT91_RTC_IER, irq_mask);
+}
+
+static void at91_rtc_clear_irq_simple(u32 irq_mask)
+{
+	at91_rtc_write(AT91_RTC_IDR, irq_mask);
+}
+
+static u32 at91_rtc_read_imr_simple(void)
+{
+	return at91_rtc_read(AT91_RTC_IMR);
+}
+
+static void at91_rtc_set_irq_brokenimr(u32 irq_mask)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lock, flags);
+	at91_rtc_imr |= irq_mask;
+	at91_rtc_write(AT91_RTC_IER, irq_mask);
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static void at91_rtc_clear_irq_brokenimr(u32 irq_mask)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lock, flags);
+	at91_rtc_write(AT91_RTC_IDR, irq_mask);
+	/*
+	 * Register read back (of any RTC-register) needed to make sure
+	 * IDR-register write has reached the peripheral before updating
+	 * shadow mask.
+	 *
+	 * Note that there is still a possibility that the mask is updated
+	 * before interrupts have actually been disabled in hardware. The only
+	 * way to be certain would be to poll the IMR-register, which is is
+	 * the very register we are trying to emulate. The register read back
+	 * is a reasonable heuristic.
+	 */
+	at91_rtc_read(AT91_RTC_SR);
+	at91_rtc_imr &= ~irq_mask;
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static u32 at91_rtc_read_imr_brokenimr(void)
+{
+	unsigned long flags;
+	u32 shadow_imr;
+
+	spin_lock_irqsave(&lock, flags);
+	shadow_imr = at91_rtc_imr;
+	spin_unlock_irqrestore(&lock, flags);
+
+	return shadow_imr;
+}
 
 /*
  * Decode time/date into rtc_time structure
@@ -300,6 +369,12 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 	at91_rtc_imr = 0;
+	spin_lock_init(&lock);
+
+	/* Choose IMR access functions */
+	at91_rtc_set_irq = at91_rtc_set_irq_simple;
+	at91_rtc_clear_irq = at91_rtc_clear_irq_simple;
+	at91_rtc_read_imr = at91_rtc_read_imr_simple;
 
 	ret = request_irq(irq, at91_rtc_interrupt,
 				IRQF_SHARED,
diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
index 5f940b6..da1945e 100644
--- a/drivers/rtc/rtc-at91rm9200.h
+++ b/drivers/rtc/rtc-at91rm9200.h
@@ -64,6 +64,7 @@
 #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
 #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
 #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
+#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
 
 #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
 #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
-- 
1.8.0

[RFC PATCH] rtc: rtc-at91rm9200: manage IMR depending on revision

[Douglas Gilbert]

On 13-04-02 09:06 AM, Nicolas Ferre wrote:
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Hi all,
>
> The funny thing is that I was writing exactly the same code as Johan's
> when he posted his series.
>
> So, here is my single patch, with the comment about the readback stolen from
> Johan's, but without the way to determine with IP is buggy and which one is
> not...
> After having dug the possibility to read the IP revision, I discovered that it
> is not possible to use this information ("version" register offset changing
> according to... IP version number: well done!).
> In conclusion, I guess that the only way to determine if we need the workaround
> is to use the DT.
> One remark though: if we use the compatibility string for this purpose, I fear
> that we would twist the meaning of this information: SoC using an
> "atmel,at91sam9x5-rtc" compatible RTC will not necessarily be touched by the
> "non responding IMR" bug: at91sam9n12 or upcoming sama5d3 are not affected for
> instance, and we need to cling to "atmel,at91rm9200-rtc" for them...
> I think that we can use this method for the moment and move to another
> compatibility string later if it is needed.

Rather than have so many people working on rtc-at91rm9200.c,
how about someone bring its "RTT" sibling into the DT
world. I'm talking about drivers/rtc/rtc-at91sam9.c ...

Doug Gilbert

[RFC PATCH] rtc: rtc-at91rm9200: manage IMR depending on revision

[Nicolas Ferre]

On 04/02/2013 05:32 PM, Douglas Gilbert :
> On 13-04-02 09:06 AM, Nicolas Ferre wrote:
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> Hi all,
>>
>> The funny thing is that I was writing exactly the same code as Johan's
>> when he posted his series.
>>
>> So, here is my single patch, with the comment about the readback
>> stolen from
>> Johan's, but without the way to determine with IP is buggy and which
>> one is
>> not...
>> After having dug the possibility to read the IP revision, I discovered
>> that it
>> is not possible to use this information ("version" register offset
>> changing
>> according to... IP version number: well done!).
>> In conclusion, I guess that the only way to determine if we need the
>> workaround
>> is to use the DT.
>> One remark though: if we use the compatibility string for this
>> purpose, I fear
>> that we would twist the meaning of this information: SoC using an
>> "atmel,at91sam9x5-rtc" compatible RTC will not necessarily be touched
>> by the
>> "non responding IMR" bug: at91sam9n12 or upcoming sama5d3 are not
>> affected for
>> instance, and we need to cling to "atmel,at91rm9200-rtc" for them...
>> I think that we can use this method for the moment and move to another
>> compatibility string later if it is needed.
> 
> Rather than have so many people working on rtc-at91rm9200.c,
> how about someone bring its "RTT" sibling into the DT
> world. I'm talking about drivers/rtc/rtc-at91sam9.c ...

I am currently trying to fix the issue that I have created by pushing a
boggus fix to Andrew's patch series (and "stable" incidentally).
So I am trying to find the best way to address this and:
- a correct
- a smallest possible
path or patch series (I admit that I prefer a single patch).

So, I am still posting rtc-at91rm9200.c patches and hoping from a
feedback. Once we have a good solution I will try to include it in 3.9
and the stable trees affected.

Best regards,
-- 
Nicolas Ferre

[RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision

[Nicolas Ferre]

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi again,

Here is my latest revision of this fix. It depends on the patch that is already
in Andrew's patch stack: "drivers-rtc-rtc-at91rm9200c-add-dt-support.patch".

I now use a different compatibility string to figure out what is the IP
revision that has the "boggus IMR" error. I think this way to handle it
is much simpler than the "config" structure one from Johan. The small number of
line changed and the "single patch" nature of it make me think that it will be
easier to send upstream and in the "stable" trees...

Please give feedback, but moreover, I would like to know if you (Johan and Douglas)
agree to give your "Signed-off-by" line because this patch is certainly
inspired by your comments, code and reviews.

Thank you for your help. Best regards,

 .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   3 +-
 drivers/rtc/rtc-at91rm9200.c                       | 126 ++++++++++++++++-----
 drivers/rtc/rtc-at91rm9200.h                       |   1 +
 3 files changed, 101 insertions(+), 29 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
index 2a3feab..9b87053 100644
--- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
@@ -1,7 +1,8 @@
 Atmel AT91RM9200 Real Time Clock
 
 Required properties:
-- compatible: should be: "atmel,at91rm9200-rtc"
+- compatible: should be: "atmel,at91rm9200-rtc", "atmel,at91sam9x5-rtc" or
+                         "atmel,at91sam9n12-rtc".
 - reg: physical base address of the controller and length of memory mapped
   region.
 - interrupts: rtc alarm/event interrupt
diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 29b92e4..f6b2ee3 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -25,6 +25,7 @@
 #include <linux/rtc.h>
 #include <linux/bcd.h>
 #include <linux/interrupt.h>
+#include <linux/spinlock.h>
 #include <linux/ioctl.h>
 #include <linux/completion.h>
 #include <linux/io.h>
@@ -47,6 +48,69 @@ static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 static u32 at91_rtc_imr;
+static DEFINE_SPINLOCK(lock);
+static void (*at91_rtc_set_irq)(u32 irq_mask);
+static void (*at91_rtc_clear_irq)(u32 irq_mask);
+static u32 (*at91_rtc_read_imr)(void);
+
+static void at91_rtc_set_irq_simple(u32 irq_mask)
+{
+	at91_rtc_write(AT91_RTC_IER, irq_mask);
+}
+
+static void at91_rtc_clear_irq_simple(u32 irq_mask)
+{
+	at91_rtc_write(AT91_RTC_IDR, irq_mask);
+}
+
+static u32 at91_rtc_read_imr_simple(void)
+{
+	return at91_rtc_read(AT91_RTC_IMR);
+}
+
+static void at91_rtc_set_irq_brokenimr(u32 irq_mask)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lock, flags);
+	at91_rtc_imr |= irq_mask;
+	at91_rtc_write(AT91_RTC_IER, irq_mask);
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static void at91_rtc_clear_irq_brokenimr(u32 irq_mask)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lock, flags);
+	at91_rtc_write(AT91_RTC_IDR, irq_mask);
+	/*
+	 * Register read back (of any RTC-register) needed to make sure
+	 * IDR-register write has reached the peripheral before updating
+	 * shadow mask.
+	 *
+	 * Note that there is still a possibility that the mask is updated
+	 * before interrupts have actually been disabled in hardware. The only
+	 * way to be certain would be to poll the IMR-register, which is is
+	 * the very register we are trying to emulate. The register read back
+	 * is a reasonable heuristic.
+	 */
+	at91_rtc_read(AT91_RTC_SR);
+	at91_rtc_imr &= ~irq_mask;
+	spin_unlock_irqrestore(&lock, flags);
+}
+
+static u32 at91_rtc_read_imr_brokenimr(void)
+{
+	unsigned long flags;
+	u32 shadow_imr;
+
+	spin_lock_irqsave(&lock, flags);
+	shadow_imr = at91_rtc_imr;
+	spin_unlock_irqrestore(&lock, flags);
+
+	return shadow_imr;
+}
 
 /*
  * Decode time/date into rtc_time structure
@@ -111,11 +175,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 	cr = at91_rtc_read(AT91_RTC_CR);
 	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
 
-	at91_rtc_imr |= AT91_RTC_ACKUPD;
-	at91_rtc_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
+	at91_rtc_set_irq(AT91_RTC_ACKUPD);
 	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD);
-	at91_rtc_imr &= ~AT91_RTC_ACKUPD;
+	at91_rtc_clear_irq(AT91_RTC_ACKUPD);
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -147,7 +209,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
 	tm->tm_year = at91_alarm_year - 1900;
 
-	alrm->enabled = (at91_rtc_imr & AT91_RTC_ALARM)
+	alrm->enabled = (at91_rtc_read_imr() & AT91_RTC_ALARM)
 			? 1 : 0;
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -172,8 +234,7 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm.tm_min = alrm->time.tm_min;
 	tm.tm_sec = alrm->time.tm_sec;
 
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-	at91_rtc_imr &= ~AT91_RTC_ALARM;
+	at91_rtc_clear_irq(AT91_RTC_ALARM);
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -186,8 +247,7 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	if (alrm->enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
-		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_set_irq(AT91_RTC_ALARM);
 	}
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -203,11 +263,9 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 
 	if (enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
-		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_set_irq(AT91_RTC_ALARM);
 	} else {
-		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-		at91_rtc_imr &= ~AT91_RTC_ALARM;
+		at91_rtc_clear_irq(AT91_RTC_ALARM);
 	}
 
 	return 0;
@@ -217,10 +275,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
  */
 static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
 {
+	unsigned long imr = at91_rtc_read_imr();
+
 	seq_printf(seq, "update_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_ACKUPD) ? "yes" : "no");
+			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
 	seq_printf(seq, "periodic_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_SECEV) ? "yes" : "no");
+			(imr & AT91_RTC_SECEV) ? "yes" : "no");
 
 	return 0;
 }
@@ -235,7 +295,7 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	unsigned int rtsr;
 	unsigned long events = 0;
 
-	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_imr;
+	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read_imr();
 	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
 		if (rtsr & AT91_RTC_ALARM)
 			events |= (RTC_AF | RTC_IRQF);
@@ -301,6 +361,18 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 					AT91_RTC_CALEV);
 	at91_rtc_imr = 0;
 
+	/* Choose IMR access functions */
+	at91_rtc_set_irq = at91_rtc_set_irq_simple;
+	at91_rtc_clear_irq = at91_rtc_clear_irq_simple;
+	at91_rtc_read_imr = at91_rtc_read_imr_simple;
+
+	if (pdev->dev.of_node
+	    && of_device_is_compatible(pdev->dev.of_node, "atmel,at91sam9x5-rtc")) {
+		at91_rtc_set_irq = at91_rtc_set_irq_brokenimr;
+		at91_rtc_clear_irq = at91_rtc_clear_irq_brokenimr;
+		at91_rtc_read_imr = at91_rtc_read_imr_brokenimr;
+	}
+
 	ret = request_irq(irq, at91_rtc_interrupt,
 				IRQF_SHARED,
 				"at91_rtc", pdev);
@@ -359,27 +431,23 @@ static int at91_rtc_suspend(struct device *dev)
 	/* this IRQ is shared with DBGU and other hardware which isn't
 	 * necessarily doing PM like we are...
 	 */
-	at91_rtc_bkpimr = at91_rtc_imr & (AT91_RTC_ALARM|AT91_RTC_SECEV);
+	at91_rtc_bkpimr = at91_rtc_read_imr() & (AT91_RTC_ALARM|AT91_RTC_SECEV);
 	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+		if (device_may_wakeup(dev))
 			enable_irq_wake(irq);
-		} else {
-			at91_rtc_write(AT91_RTC_IDR, at91_rtc_bkpimr);
-			at91_rtc_imr &= ~at91_rtc_bkpimr;
-		}
-}
+		else
+			at91_rtc_clear_irq(at91_rtc_bkpimr);
+	}
 	return 0;
 }
 
 static int at91_rtc_resume(struct device *dev)
 {
 	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
-		} else {
-			at91_rtc_imr |= at91_rtc_bkpimr;
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_bkpimr);
-		}
+		else
+			at91_rtc_set_irq(at91_rtc_bkpimr);
 	}
 	return 0;
 }
@@ -397,6 +465,8 @@ static const struct dev_pm_ops at91_rtc_pm = {
 
 static const struct of_device_id at91_rtc_dt_ids[] = {
 	{ .compatible = "atmel,at91rm9200-rtc" },
+	{ .compatible = "atmel,at91sam9x5-rtc" },
+	{ .compatible = "atmel,at91sam9n12-rtc" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
index 5f940b6..da1945e 100644
--- a/drivers/rtc/rtc-at91rm9200.h
+++ b/drivers/rtc/rtc-at91rm9200.h
@@ -64,6 +64,7 @@
 #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
 #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
 #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
+#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
 
 #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
 #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
-- 
1.8.0

[RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision

[Johan Hovold]

On Tue, Apr 02, 2013 at 06:36:06PM +0200, Nicolas Ferre wrote:
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Hi again,
> 
> Here is my latest revision of this fix. It depends on the patch that is already
> in Andrew's patch stack: "drivers-rtc-rtc-at91rm9200c-add-dt-support.patch".

That is a problem, as the patch in Andrew's stack is not (and should
not) be marked for stable. Hence this patch cannot be applied to the
stable trees and it won't even apply to 3.9-rc.

But there's more: The offending patch introduced the races we have been
discussion while attempting to add support for the sam9x5 with the
broken hardware register. But that family cannot be used without
DT-support, which the driver currently does not support. Hence, we added
a workaround (and introduced a regression by mistake), while adding
support for a SoC which still could not use the driver. [ For example,
the sam9x5 RTC-base register address can only be supplied from DT. ]

I think the only reasonable thing to do is to revert the patch and add
whatever version of the work-around on top of the device-tree support
when that is added to the driver (hence, earliest v3.10).

> I now use a different compatibility string to figure out what is the IP
> revision that has the "boggus IMR" error. I think this way to handle it
> is much simpler than the "config" structure one from Johan.

I wouldn't say it's much simpler. My solution is only more generic, but
could of course also be reduced to "set a flag if compatible matches
sam9x5".

> The small number of line changed and the "single patch" nature of it
> make me think that it will be easier to send upstream and in the
> "stable" trees...

Unfortunately, the 130-line diff isn't very small. In fact, it violates
the stable-kernel guide line of <100 lines. And as noted above, it
depends on another patch which adds DT-support (which is a new feature
and not a fix).

But the fundamental problem remains: it does not fix anything which was
working before the first work-around patch introduced the regression. I
think this is a clear case where we need to revert.

> Please give feedback, but moreover, I would like to know if you (Johan and Douglas)
> agree to give your "Signed-off-by" line because this patch is certainly
> inspired by your comments, code and reviews.
> 
> Thank you for your help. Best regards,
> 
>  .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   3 +-
>  drivers/rtc/rtc-at91rm9200.c                       | 126 ++++++++++++++++-----
>  drivers/rtc/rtc-at91rm9200.h                       |   1 +
>  3 files changed, 101 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> index 2a3feab..9b87053 100644
> --- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> +++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> @@ -1,7 +1,8 @@
>  Atmel AT91RM9200 Real Time Clock
>  
>  Required properties:
> -- compatible: should be: "atmel,at91rm9200-rtc"
> +- compatible: should be: "atmel,at91rm9200-rtc", "atmel,at91sam9x5-rtc" or
> +                         "atmel,at91sam9n12-rtc".

Also at91sam9g45 and at91sam9rl use this driver. As seems to be the case
for other peripherals, I suggest we use "atmel,at91sam9x5-rtc" for
sam9x5 and "atmel,at91rm9200-rtc" for the other SoCs, that is, the least
(and first) common denominator.

Either way, there's not need to add at91sam9n12-rtc in this patch.

>  - reg: physical base address of the controller and length of memory mapped
>    region.
>  - interrupts: rtc alarm/event interrupt

I'll respond to this mail with a revert-patch, and an updated RFC-series
based on top of the DT-patch in Andrew's queue.

Thanks,
Johan

[PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR"

[Johan Hovold]

This reverts commit 0ef1594c017521ea89278e80fe3f80dafb17abde.

This patch introduced a few races which cannot be easily fixed with a
small follow-up patch. Furthermore, the SoC with the broken hardware
register, which this patch intended to add support for, can only be used
with device trees, which this driver currently does not support.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 50 +++++++++++++++++---------------------------
 drivers/rtc/rtc-at91rm9200.h |  1 +
 2 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 0a9f27e..434ebc3 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -44,7 +44,6 @@ static DECLARE_COMPLETION(at91_rtc_updated);
 static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
-static u32 at91_rtc_imr;
 
 /*
  * Decode time/date into rtc_time structure
@@ -109,11 +108,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 	cr = at91_rtc_read(AT91_RTC_CR);
 	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
 
-	at91_rtc_imr |= AT91_RTC_ACKUPD;
 	at91_rtc_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
 	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD);
-	at91_rtc_imr &= ~AT91_RTC_ACKUPD;
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -145,7 +142,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
 	tm->tm_year = at91_alarm_year - 1900;
 
-	alrm->enabled = (at91_rtc_imr & AT91_RTC_ALARM)
+	alrm->enabled = (at91_rtc_read(AT91_RTC_IMR) & AT91_RTC_ALARM)
 			? 1 : 0;
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -171,7 +168,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm.tm_sec = alrm->time.tm_sec;
 
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-	at91_rtc_imr &= ~AT91_RTC_ALARM;
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -184,7 +180,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	if (alrm->enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
 		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
 	}
 
@@ -201,12 +196,9 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 
 	if (enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
 		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
-	} else {
+	} else
 		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-		at91_rtc_imr &= ~AT91_RTC_ALARM;
-	}
 
 	return 0;
 }
@@ -215,10 +207,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
  */
 static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
 {
+	unsigned long imr = at91_rtc_read(AT91_RTC_IMR);
+
 	seq_printf(seq, "update_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_ACKUPD) ? "yes" : "no");
+			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
 	seq_printf(seq, "periodic_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_SECEV) ? "yes" : "no");
+			(imr & AT91_RTC_SECEV) ? "yes" : "no");
 
 	return 0;
 }
@@ -233,7 +227,7 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	unsigned int rtsr;
 	unsigned long events = 0;
 
-	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_imr;
+	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read(AT91_RTC_IMR);
 	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
 		if (rtsr & AT91_RTC_ALARM)
 			events |= (RTC_AF | RTC_IRQF);
@@ -297,7 +291,6 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
-	at91_rtc_imr = 0;
 
 	ret = request_irq(irq, at91_rtc_interrupt,
 				IRQF_SHARED,
@@ -336,7 +329,6 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
-	at91_rtc_imr = 0;
 	free_irq(irq, pdev);
 
 	rtc_device_unregister(rtc);
@@ -349,35 +341,31 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 
 /* AT91RM9200 RTC Power management control */
 
-static u32 at91_rtc_bkpimr;
-
+static u32 at91_rtc_imr;
 
 static int at91_rtc_suspend(struct device *dev)
 {
 	/* this IRQ is shared with DBGU and other hardware which isn't
 	 * necessarily doing PM like we are...
 	 */
-	at91_rtc_bkpimr = at91_rtc_imr & (AT91_RTC_ALARM|AT91_RTC_SECEV);
-	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+	at91_rtc_imr = at91_rtc_read(AT91_RTC_IMR)
+			& (AT91_RTC_ALARM|AT91_RTC_SECEV);
+	if (at91_rtc_imr) {
+		if (device_may_wakeup(dev))
 			enable_irq_wake(irq);
-		} else {
-			at91_rtc_write(AT91_RTC_IDR, at91_rtc_bkpimr);
-			at91_rtc_imr &= ~at91_rtc_bkpimr;
-		}
-}
+		else
+			at91_rtc_write(AT91_RTC_IDR, at91_rtc_imr);
+	}
 	return 0;
 }
 
 static int at91_rtc_resume(struct device *dev)
 {
-	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+	if (at91_rtc_imr) {
+		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
-		} else {
-			at91_rtc_imr |= at91_rtc_bkpimr;
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_bkpimr);
-		}
+		else
+			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
 	}
 	return 0;
 }
diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
index 5f940b6..da1945e 100644
--- a/drivers/rtc/rtc-at91rm9200.h
+++ b/drivers/rtc/rtc-at91rm9200.h
@@ -64,6 +64,7 @@
 #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
 #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
 #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
+#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
 
 #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
 #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
-- 
1.8.1.5

[PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR"

[Nicolas Ferre]

On 04/03/2013 11:54 AM, Johan Hovold :
> This reverts commit 0ef1594c017521ea89278e80fe3f80dafb17abde.
> 
> This patch introduced a few races which cannot be easily fixed with a
> small follow-up patch. Furthermore, the SoC with the broken hardware
> register, which this patch intended to add support for, can only be used
> with device trees, which this driver currently does not support.
> 
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>

Fair enough, after fighting to find a solution that can makes us move
forward... your strong arguments convinced me.

So, Andrew, please can you take this "revert" patch for 3.9-rc ?
And sorry for the noise.

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

(Andrew, I figured out that you are not in copy of the original email:
do I need to send it back to you or can you pick it up in patchwork?
https://patchwork.kernel.org/patch/2385921/
)

> ---
>  drivers/rtc/rtc-at91rm9200.c | 50 +++++++++++++++++---------------------------
>  drivers/rtc/rtc-at91rm9200.h |  1 +
>  2 files changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> index 0a9f27e..434ebc3 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -44,7 +44,6 @@ static DECLARE_COMPLETION(at91_rtc_updated);
>  static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
>  static void __iomem *at91_rtc_regs;
>  static int irq;
> -static u32 at91_rtc_imr;
>  
>  /*
>   * Decode time/date into rtc_time structure
> @@ -109,11 +108,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
>  	cr = at91_rtc_read(AT91_RTC_CR);
>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
>  
> -	at91_rtc_imr |= AT91_RTC_ACKUPD;
>  	at91_rtc_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
>  	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD);
> -	at91_rtc_imr &= ~AT91_RTC_ACKUPD;
>  
>  	at91_rtc_write(AT91_RTC_TIMR,
>  			  bin2bcd(tm->tm_sec) << 0
> @@ -145,7 +142,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
>  	tm->tm_year = at91_alarm_year - 1900;
>  
> -	alrm->enabled = (at91_rtc_imr & AT91_RTC_ALARM)
> +	alrm->enabled = (at91_rtc_read(AT91_RTC_IMR) & AT91_RTC_ALARM)
>  			? 1 : 0;
>  
>  	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
> @@ -171,7 +168,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	tm.tm_sec = alrm->time.tm_sec;
>  
>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
> -	at91_rtc_imr &= ~AT91_RTC_ALARM;
>  	at91_rtc_write(AT91_RTC_TIMALR,
>  		  bin2bcd(tm.tm_sec) << 0
>  		| bin2bcd(tm.tm_min) << 8
> @@ -184,7 +180,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>  
>  	if (alrm->enabled) {
>  		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
> -		at91_rtc_imr |= AT91_RTC_ALARM;
>  		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
>  	}
>  
> @@ -201,12 +196,9 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  
>  	if (enabled) {
>  		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
> -		at91_rtc_imr |= AT91_RTC_ALARM;
>  		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
> -	} else {
> +	} else
>  		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
> -		at91_rtc_imr &= ~AT91_RTC_ALARM;
> -	}
>  
>  	return 0;
>  }
> @@ -215,10 +207,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>   */
>  static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
>  {
> +	unsigned long imr = at91_rtc_read(AT91_RTC_IMR);
> +
>  	seq_printf(seq, "update_IRQ\t: %s\n",
> -			(at91_rtc_imr & AT91_RTC_ACKUPD) ? "yes" : "no");
> +			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
>  	seq_printf(seq, "periodic_IRQ\t: %s\n",
> -			(at91_rtc_imr & AT91_RTC_SECEV) ? "yes" : "no");
> +			(imr & AT91_RTC_SECEV) ? "yes" : "no");
>  
>  	return 0;
>  }
> @@ -233,7 +227,7 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
>  	unsigned int rtsr;
>  	unsigned long events = 0;
>  
> -	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_imr;
> +	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read(AT91_RTC_IMR);
>  	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
>  		if (rtsr & AT91_RTC_ALARM)
>  			events |= (RTC_AF | RTC_IRQF);
> @@ -297,7 +291,6 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
>  					AT91_RTC_SECEV | AT91_RTC_TIMEV |
>  					AT91_RTC_CALEV);
> -	at91_rtc_imr = 0;
>  
>  	ret = request_irq(irq, at91_rtc_interrupt,
>  				IRQF_SHARED,
> @@ -336,7 +329,6 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
>  					AT91_RTC_SECEV | AT91_RTC_TIMEV |
>  					AT91_RTC_CALEV);
> -	at91_rtc_imr = 0;
>  	free_irq(irq, pdev);
>  
>  	rtc_device_unregister(rtc);
> @@ -349,35 +341,31 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
>  
>  /* AT91RM9200 RTC Power management control */
>  
> -static u32 at91_rtc_bkpimr;
> -
> +static u32 at91_rtc_imr;
>  
>  static int at91_rtc_suspend(struct device *dev)
>  {
>  	/* this IRQ is shared with DBGU and other hardware which isn't
>  	 * necessarily doing PM like we are...
>  	 */
> -	at91_rtc_bkpimr = at91_rtc_imr & (AT91_RTC_ALARM|AT91_RTC_SECEV);
> -	if (at91_rtc_bkpimr) {
> -		if (device_may_wakeup(dev)) {
> +	at91_rtc_imr = at91_rtc_read(AT91_RTC_IMR)
> +			& (AT91_RTC_ALARM|AT91_RTC_SECEV);
> +	if (at91_rtc_imr) {
> +		if (device_may_wakeup(dev))
>  			enable_irq_wake(irq);
> -		} else {
> -			at91_rtc_write(AT91_RTC_IDR, at91_rtc_bkpimr);
> -			at91_rtc_imr &= ~at91_rtc_bkpimr;
> -		}
> -}
> +		else
> +			at91_rtc_write(AT91_RTC_IDR, at91_rtc_imr);
> +	}
>  	return 0;
>  }
>  
>  static int at91_rtc_resume(struct device *dev)
>  {
> -	if (at91_rtc_bkpimr) {
> -		if (device_may_wakeup(dev)) {
> +	if (at91_rtc_imr) {
> +		if (device_may_wakeup(dev))
>  			disable_irq_wake(irq);
> -		} else {
> -			at91_rtc_imr |= at91_rtc_bkpimr;
> -			at91_rtc_write(AT91_RTC_IER, at91_rtc_bkpimr);
> -		}
> +		else
> +			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
>  	}
>  	return 0;
>  }
> diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
> index 5f940b6..da1945e 100644
> --- a/drivers/rtc/rtc-at91rm9200.h
> +++ b/drivers/rtc/rtc-at91rm9200.h
> @@ -64,6 +64,7 @@
>  #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
>  #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
>  #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
> +#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
>  
>  #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
>  #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
> 


-- 
Nicolas Ferre

[PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR"

[Nicolas Ferre]

On 04/03/2013 12:18 PM, Nicolas Ferre :
> On 04/03/2013 11:54 AM, Johan Hovold :
>> This reverts commit 0ef1594c017521ea89278e80fe3f80dafb17abde.
>>
>> This patch introduced a few races which cannot be easily fixed with a
>> small follow-up patch. Furthermore, the SoC with the broken hardware
>> register, which this patch intended to add support for, can only be used
>> with device trees, which this driver currently does not support.
>>
>> Cc: stable <stable@vger.kernel.org>
>> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> 
> Fair enough, after fighting to find a solution that can makes us move
> forward... your strong arguments convinced me.
> 
> So, Andrew, please can you take this "revert" patch for 3.9-rc ?
> And sorry for the noise.
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> (Andrew, I figured out that you are not in copy of the original email:
> do I need to send it back to you or can you pick it up in patchwork?
> https://patchwork.kernel.org/patch/2385921/
> )

Andrew, ping?

Thanks, best regards,

>> ---
>>  drivers/rtc/rtc-at91rm9200.c | 50 +++++++++++++++++---------------------------
>>  drivers/rtc/rtc-at91rm9200.h |  1 +
>>  2 files changed, 20 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
>> index 0a9f27e..434ebc3 100644
>> --- a/drivers/rtc/rtc-at91rm9200.c
>> +++ b/drivers/rtc/rtc-at91rm9200.c
>> @@ -44,7 +44,6 @@ static DECLARE_COMPLETION(at91_rtc_updated);
>>  static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
>>  static void __iomem *at91_rtc_regs;
>>  static int irq;
>> -static u32 at91_rtc_imr;
>>  
>>  /*
>>   * Decode time/date into rtc_time structure
>> @@ -109,11 +108,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
>>  	cr = at91_rtc_read(AT91_RTC_CR);
>>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
>>  
>> -	at91_rtc_imr |= AT91_RTC_ACKUPD;
>>  	at91_rtc_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
>>  	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
>>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD);
>> -	at91_rtc_imr &= ~AT91_RTC_ACKUPD;
>>  
>>  	at91_rtc_write(AT91_RTC_TIMR,
>>  			  bin2bcd(tm->tm_sec) << 0
>> @@ -145,7 +142,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
>>  	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
>>  	tm->tm_year = at91_alarm_year - 1900;
>>  
>> -	alrm->enabled = (at91_rtc_imr & AT91_RTC_ALARM)
>> +	alrm->enabled = (at91_rtc_read(AT91_RTC_IMR) & AT91_RTC_ALARM)
>>  			? 1 : 0;
>>  
>>  	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
>> @@ -171,7 +168,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>>  	tm.tm_sec = alrm->time.tm_sec;
>>  
>>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
>> -	at91_rtc_imr &= ~AT91_RTC_ALARM;
>>  	at91_rtc_write(AT91_RTC_TIMALR,
>>  		  bin2bcd(tm.tm_sec) << 0
>>  		| bin2bcd(tm.tm_min) << 8
>> @@ -184,7 +180,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
>>  
>>  	if (alrm->enabled) {
>>  		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
>> -		at91_rtc_imr |= AT91_RTC_ALARM;
>>  		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
>>  	}
>>  
>> @@ -201,12 +196,9 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>>  
>>  	if (enabled) {
>>  		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
>> -		at91_rtc_imr |= AT91_RTC_ALARM;
>>  		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
>> -	} else {
>> +	} else
>>  		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
>> -		at91_rtc_imr &= ~AT91_RTC_ALARM;
>> -	}
>>  
>>  	return 0;
>>  }
>> @@ -215,10 +207,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>>   */
>>  static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
>>  {
>> +	unsigned long imr = at91_rtc_read(AT91_RTC_IMR);
>> +
>>  	seq_printf(seq, "update_IRQ\t: %s\n",
>> -			(at91_rtc_imr & AT91_RTC_ACKUPD) ? "yes" : "no");
>> +			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
>>  	seq_printf(seq, "periodic_IRQ\t: %s\n",
>> -			(at91_rtc_imr & AT91_RTC_SECEV) ? "yes" : "no");
>> +			(imr & AT91_RTC_SECEV) ? "yes" : "no");
>>  
>>  	return 0;
>>  }
>> @@ -233,7 +227,7 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
>>  	unsigned int rtsr;
>>  	unsigned long events = 0;
>>  
>> -	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_imr;
>> +	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read(AT91_RTC_IMR);
>>  	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
>>  		if (rtsr & AT91_RTC_ALARM)
>>  			events |= (RTC_AF | RTC_IRQF);
>> @@ -297,7 +291,6 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
>>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
>>  					AT91_RTC_SECEV | AT91_RTC_TIMEV |
>>  					AT91_RTC_CALEV);
>> -	at91_rtc_imr = 0;
>>  
>>  	ret = request_irq(irq, at91_rtc_interrupt,
>>  				IRQF_SHARED,
>> @@ -336,7 +329,6 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
>>  	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
>>  					AT91_RTC_SECEV | AT91_RTC_TIMEV |
>>  					AT91_RTC_CALEV);
>> -	at91_rtc_imr = 0;
>>  	free_irq(irq, pdev);
>>  
>>  	rtc_device_unregister(rtc);
>> @@ -349,35 +341,31 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
>>  
>>  /* AT91RM9200 RTC Power management control */
>>  
>> -static u32 at91_rtc_bkpimr;
>> -
>> +static u32 at91_rtc_imr;
>>  
>>  static int at91_rtc_suspend(struct device *dev)
>>  {
>>  	/* this IRQ is shared with DBGU and other hardware which isn't
>>  	 * necessarily doing PM like we are...
>>  	 */
>> -	at91_rtc_bkpimr = at91_rtc_imr & (AT91_RTC_ALARM|AT91_RTC_SECEV);
>> -	if (at91_rtc_bkpimr) {
>> -		if (device_may_wakeup(dev)) {
>> +	at91_rtc_imr = at91_rtc_read(AT91_RTC_IMR)
>> +			& (AT91_RTC_ALARM|AT91_RTC_SECEV);
>> +	if (at91_rtc_imr) {
>> +		if (device_may_wakeup(dev))
>>  			enable_irq_wake(irq);
>> -		} else {
>> -			at91_rtc_write(AT91_RTC_IDR, at91_rtc_bkpimr);
>> -			at91_rtc_imr &= ~at91_rtc_bkpimr;
>> -		}
>> -}
>> +		else
>> +			at91_rtc_write(AT91_RTC_IDR, at91_rtc_imr);
>> +	}
>>  	return 0;
>>  }
>>  
>>  static int at91_rtc_resume(struct device *dev)
>>  {
>> -	if (at91_rtc_bkpimr) {
>> -		if (device_may_wakeup(dev)) {
>> +	if (at91_rtc_imr) {
>> +		if (device_may_wakeup(dev))
>>  			disable_irq_wake(irq);
>> -		} else {
>> -			at91_rtc_imr |= at91_rtc_bkpimr;
>> -			at91_rtc_write(AT91_RTC_IER, at91_rtc_bkpimr);
>> -		}
>> +		else
>> +			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
>>  	}
>>  	return 0;
>>  }
>> diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
>> index 5f940b6..da1945e 100644
>> --- a/drivers/rtc/rtc-at91rm9200.h
>> +++ b/drivers/rtc/rtc-at91rm9200.h
>> @@ -64,6 +64,7 @@
>>  #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
>>  #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
>>  #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
>> +#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
>>  
>>  #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
>>  #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
>>
> 
> 


-- 
Nicolas Ferre

[PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR"

[Greg KH]

On Fri, Apr 05, 2013 at 04:14:35PM +0200, Nicolas Ferre wrote:
> On 04/03/2013 12:18 PM, Nicolas Ferre :
> > On 04/03/2013 11:54 AM, Johan Hovold :
> >> This reverts commit 0ef1594c017521ea89278e80fe3f80dafb17abde.
> >>
> >> This patch introduced a few races which cannot be easily fixed with a
> >> small follow-up patch. Furthermore, the SoC with the broken hardware
> >> register, which this patch intended to add support for, can only be used
> >> with device trees, which this driver currently does not support.
> >>
> >> Cc: stable <stable@vger.kernel.org>
> >> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > 
> > Fair enough, after fighting to find a solution that can makes us move
> > forward... your strong arguments convinced me.
> > 
> > So, Andrew, please can you take this "revert" patch for 3.9-rc ?
> > And sorry for the noise.
> > 
> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> > 
> > (Andrew, I figured out that you are not in copy of the original email:
> > do I need to send it back to you or can you pick it up in patchwork?
> > https://patchwork.kernel.org/patch/2385921/
> > )
> 
> Andrew, ping?

Andrew is on vacation for a few weeks, sorry.

Perhaps you should send this yourself?

greg k-h

[PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR"

[Nicolas Ferre]

From: Johan Hovold <jhovold@gmail.com>

This reverts commit 0ef1594.

This patch introduced a few races which cannot be easily fixed with a
small follow-up patch. Furthermore, the SoC with the broken hardware
register, which this patch intended to add support for, can only be used
with device trees, which this driver currently does not support.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <jhovold@gmail.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Linus,

As suggested by Greg, I send you directly this "revert" patch. I hope it will
join your tree before 3.9-final.

Here is the discussion that led to this "revert" patch.
https://lkml.org/lkml/2013/4/3/176

Thanks a lot, best regards,

 drivers/rtc/rtc-at91rm9200.c | 50 +++++++++++++++++---------------------------
 drivers/rtc/rtc-at91rm9200.h |  1 +
 2 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 0a9f27e..434ebc3 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -44,7 +44,6 @@ static DECLARE_COMPLETION(at91_rtc_updated);
 static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
-static u32 at91_rtc_imr;
 
 /*
  * Decode time/date into rtc_time structure
@@ -109,11 +108,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 	cr = at91_rtc_read(AT91_RTC_CR);
 	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
 
-	at91_rtc_imr |= AT91_RTC_ACKUPD;
 	at91_rtc_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
 	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD);
-	at91_rtc_imr &= ~AT91_RTC_ACKUPD;
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -145,7 +142,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
 	tm->tm_year = at91_alarm_year - 1900;
 
-	alrm->enabled = (at91_rtc_imr & AT91_RTC_ALARM)
+	alrm->enabled = (at91_rtc_read(AT91_RTC_IMR) & AT91_RTC_ALARM)
 			? 1 : 0;
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -171,7 +168,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	tm.tm_sec = alrm->time.tm_sec;
 
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-	at91_rtc_imr &= ~AT91_RTC_ALARM;
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -184,7 +180,6 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	if (alrm->enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
 		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
 	}
 
@@ -201,12 +196,9 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 
 	if (enabled) {
 		at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
-		at91_rtc_imr |= AT91_RTC_ALARM;
 		at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
-	} else {
+	} else
 		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
-		at91_rtc_imr &= ~AT91_RTC_ALARM;
-	}
 
 	return 0;
 }
@@ -215,10 +207,12 @@ static int at91_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
  */
 static int at91_rtc_proc(struct device *dev, struct seq_file *seq)
 {
+	unsigned long imr = at91_rtc_read(AT91_RTC_IMR);
+
 	seq_printf(seq, "update_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_ACKUPD) ? "yes" : "no");
+			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
 	seq_printf(seq, "periodic_IRQ\t: %s\n",
-			(at91_rtc_imr & AT91_RTC_SECEV) ? "yes" : "no");
+			(imr & AT91_RTC_SECEV) ? "yes" : "no");
 
 	return 0;
 }
@@ -233,7 +227,7 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	unsigned int rtsr;
 	unsigned long events = 0;
 
-	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_imr;
+	rtsr = at91_rtc_read(AT91_RTC_SR) & at91_rtc_read(AT91_RTC_IMR);
 	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
 		if (rtsr & AT91_RTC_ALARM)
 			events |= (RTC_AF | RTC_IRQF);
@@ -297,7 +291,6 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
-	at91_rtc_imr = 0;
 
 	ret = request_irq(irq, at91_rtc_interrupt,
 				IRQF_SHARED,
@@ -336,7 +329,6 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
-	at91_rtc_imr = 0;
 	free_irq(irq, pdev);
 
 	rtc_device_unregister(rtc);
@@ -349,35 +341,31 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 
 /* AT91RM9200 RTC Power management control */
 
-static u32 at91_rtc_bkpimr;
-
+static u32 at91_rtc_imr;
 
 static int at91_rtc_suspend(struct device *dev)
 {
 	/* this IRQ is shared with DBGU and other hardware which isn't
 	 * necessarily doing PM like we are...
 	 */
-	at91_rtc_bkpimr = at91_rtc_imr & (AT91_RTC_ALARM|AT91_RTC_SECEV);
-	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+	at91_rtc_imr = at91_rtc_read(AT91_RTC_IMR)
+			& (AT91_RTC_ALARM|AT91_RTC_SECEV);
+	if (at91_rtc_imr) {
+		if (device_may_wakeup(dev))
 			enable_irq_wake(irq);
-		} else {
-			at91_rtc_write(AT91_RTC_IDR, at91_rtc_bkpimr);
-			at91_rtc_imr &= ~at91_rtc_bkpimr;
-		}
-}
+		else
+			at91_rtc_write(AT91_RTC_IDR, at91_rtc_imr);
+	}
 	return 0;
 }
 
 static int at91_rtc_resume(struct device *dev)
 {
-	if (at91_rtc_bkpimr) {
-		if (device_may_wakeup(dev)) {
+	if (at91_rtc_imr) {
+		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
-		} else {
-			at91_rtc_imr |= at91_rtc_bkpimr;
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_bkpimr);
-		}
+		else
+			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
 	}
 	return 0;
 }
diff --git a/drivers/rtc/rtc-at91rm9200.h b/drivers/rtc/rtc-at91rm9200.h
index 5f940b6..da1945e 100644
--- a/drivers/rtc/rtc-at91rm9200.h
+++ b/drivers/rtc/rtc-at91rm9200.h
@@ -64,6 +64,7 @@
 #define	AT91_RTC_SCCR		0x1c			/* Status Clear Command Register */
 #define	AT91_RTC_IER		0x20			/* Interrupt Enable Register */
 #define	AT91_RTC_IDR		0x24			/* Interrupt Disable Register */
+#define	AT91_RTC_IMR		0x28			/* Interrupt Mask Register */
 
 #define	AT91_RTC_VER		0x2c			/* Valid Entry Register */
 #define		AT91_RTC_NVTIM		(1 <<  0)		/* Non valid Time */
-- 
1.8.0

[RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision

[Nicolas Ferre]

On 04/03/2013 11:51 AM, Johan Hovold :
> On Tue, Apr 02, 2013 at 06:36:06PM +0200, Nicolas Ferre wrote:
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> Hi again,
>>
>> Here is my latest revision of this fix. It depends on the patch that is already
>> in Andrew's patch stack: "drivers-rtc-rtc-at91rm9200c-add-dt-support.patch".
> 
> That is a problem, as the patch in Andrew's stack is not (and should
> not) be marked for stable. Hence this patch cannot be applied to the
> stable trees and it won't even apply to 3.9-rc.

My intentions were to tag both patches for "stable". You highlight that
it is not a good practice: I admit that you are right.


> But there's more: The offending patch introduced the races we have been
> discussion while attempting to add support for the sam9x5 with the
> broken hardware register. But that family cannot be used without
> DT-support, which the driver currently does not support. Hence, we added
> a workaround (and introduced a regression by mistake), while adding
> support for a SoC which still could not use the driver. [ For example,
> the sam9x5 RTC-base register address can only be supplied from DT. ]
> 
> I think the only reasonable thing to do is to revert the patch and add
> whatever version of the work-around on top of the device-tree support
> when that is added to the driver (hence, earliest v3.10).

Yes. Let's do this.


>> I now use a different compatibility string to figure out what is the IP
>> revision that has the "boggus IMR" error. I think this way to handle it
>> is much simpler than the "config" structure one from Johan.
> 
> I wouldn't say it's much simpler. My solution is only more generic, but
> could of course also be reduced to "set a flag if compatible matches
> sam9x5".

The advantage is precisely to avoid the need for a "flag". Only function
pointers that are changed in case of the compatible string matching.


>> The small number of line changed and the "single patch" nature of it
>> make me think that it will be easier to send upstream and in the
>> "stable" trees...
> 
> Unfortunately, the 130-line diff isn't very small. In fact, it violates
> the stable-kernel guide line of <100 lines. And as noted above, it
> depends on another patch which adds DT-support (which is a new feature
> and not a fix).
> 
> But the fundamental problem remains: it does not fix anything which was
> working before the first work-around patch introduced the regression. I
> think this is a clear case where we need to revert.

Okay.

>> Please give feedback, but moreover, I would like to know if you (Johan and Douglas)
>> agree to give your "Signed-off-by" line because this patch is certainly
>> inspired by your comments, code and reviews.
>>
>> Thank you for your help. Best regards,
>>
>>  .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   3 +-
>>  drivers/rtc/rtc-at91rm9200.c                       | 126 ++++++++++++++++-----
>>  drivers/rtc/rtc-at91rm9200.h                       |   1 +
>>  3 files changed, 101 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
>> index 2a3feab..9b87053 100644
>> --- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
>> +++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
>> @@ -1,7 +1,8 @@
>>  Atmel AT91RM9200 Real Time Clock
>>  
>>  Required properties:
>> -- compatible: should be: "atmel,at91rm9200-rtc"
>> +- compatible: should be: "atmel,at91rm9200-rtc", "atmel,at91sam9x5-rtc" or
>> +                         "atmel,at91sam9n12-rtc".
> 
> Also at91sam9g45 and at91sam9rl use this driver.

Yes, sure, I did not want to add every single user of the RTC...

> As seems to be the case
> for other peripherals, I suggest we use "atmel,at91sam9x5-rtc" for
> sam9x5 and "atmel,at91rm9200-rtc" for the other SoCs, that is, the least
> (and first) common denominator.

... I was just following the habit of naming the changes in peripheral
revision by it first use in a SoC:
at91rm9200-rtc: from rm9200 up to 9g45
at91sam9x5-rtc: sam9x5 only (with IMR issue)
at91sam9n12-rtc: fist SoC that corrects the IMR issue with a new IP
revision, until now and sama5d3 SoC



> Either way, there's not need to add at91sam9n12-rtc in this patch.
> 
>>  - reg: physical base address of the controller and length of memory mapped
>>    region.
>>  - interrupts: rtc alarm/event interrupt
> 
> I'll respond to this mail with a revert-patch, and an updated RFC-series
> based on top of the DT-patch in Andrew's queue.

Best regards,
-- 
Nicolas Ferre

[RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision

[Johan Hovold]

On Wed, Apr 03, 2013 at 12:37:47PM +0200, Nicolas Ferre wrote:
> On 04/03/2013 11:51 AM, Johan Hovold :
> > On Tue, Apr 02, 2013 at 06:36:06PM +0200, Nicolas Ferre wrote:

[...]

> >> I now use a different compatibility string to figure out what is the IP
> >> revision that has the "boggus IMR" error. I think this way to handle it
> >> is much simpler than the "config" structure one from Johan.
> > 
> > I wouldn't say it's much simpler. My solution is only more generic, but
> > could of course also be reduced to "set a flag if compatible matches
> > sam9x5".
> 
> The advantage is precisely to avoid the need for a "flag". Only function
> pointers that are changed in case of the compatible string matching.

Yeah, you could do it that way. The overhead is negligible in either
solution; mask updates are infrequent and the only difference when
retrieving the mask would be to first check the flag.

An advantage of using the config-struct would perhaps be that it is same
mechanism used in i2c-at91 and atmel_lcdfb (in the arm-soc tree) to deal
with SoC-quirks and is easily extended should need arise.

The diffs of both solutions are also of roughly the same size.

But I don't have any strong preference. You decide.

[...]

> >> diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> >> index 2a3feab..9b87053 100644
> >> --- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> >> +++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
> >> @@ -1,7 +1,8 @@
> >>  Atmel AT91RM9200 Real Time Clock
> >>  
> >>  Required properties:
> >> -- compatible: should be: "atmel,at91rm9200-rtc"
> >> +- compatible: should be: "atmel,at91rm9200-rtc", "atmel,at91sam9x5-rtc" or
> >> +                         "atmel,at91sam9n12-rtc".
> > 
> > Also at91sam9g45 and at91sam9rl use this driver.
> 
> Yes, sure, I did not want to add every single user of the RTC...
> 
> > As seems to be the case
> > for other peripherals, I suggest we use "atmel,at91sam9x5-rtc" for
> > sam9x5 and "atmel,at91rm9200-rtc" for the other SoCs, that is, the least
> > (and first) common denominator.
> 
> ... I was just following the habit of naming the changes in peripheral
> revision by it first use in a SoC:
> at91rm9200-rtc: from rm9200 up to 9g45
> at91sam9x5-rtc: sam9x5 only (with IMR issue)
> at91sam9n12-rtc: fist SoC that corrects the IMR issue with a new IP
> revision, until now and sama5d3 SoC

Ah, ok.
 
> > Either way, there's not need to add at91sam9n12-rtc in this patch.
> > 
> >>  - reg: physical base address of the controller and length of memory mapped
> >>    region.
> >>  - interrupts: rtc alarm/event interrupt
> > 
> > I'll respond to this mail with a revert-patch, and an updated RFC-series
> > based on top of the DT-patch in Andrew's queue.

Thanks,
Johan