[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]

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,
+	},
+		/* terminator */
+	}
+};
+MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
+#endif
+
 static const struct at91_rtc_config *
 at91_rtc_get_config(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
+
+	if (pdev->dev.of_node) {
+		match = of_match_node(at91_rtc_dt_ids, pdev->dev.of_node);
+		if (!match)
+			return NULL;
+		return (const struct at91_rtc_config *)match->data;
+	}
+
 	return &at91rm9200_config;
 }
 
@@ -403,6 +425,7 @@ static struct platform_driver at91_rtc_driver = {
 	.driver		= {
 		.name	= "at91_rtc",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(at91_rtc_dt_ids),
 		.pm	= at91_rtc_pm_ptr,
 	},
 };
-- 
1.8.1.5

Re: [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 3/5] rtc-at91rm9200: refactor interrupt-register handling

[Johan Hovold]

Add accessors for the interrupt register.

This will allow us to easily add a shadow interrupt-mask register to
use on SoCs where the interrupt-mask register cannot be used.
---
 drivers/rtc/rtc-at91rm9200.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 67260f9..cb4462d 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -50,6 +50,21 @@ static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 
+static void at91_rtc_write_ier(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IER, mask);
+}
+
+static void at91_rtc_write_idr(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IDR, mask);
+}
+
+static u32 at91_rtc_read_imr(void)
+{
+	return at91_rtc_read(AT91_RTC_IMR);
+}
+
 /*
  * Decode time/date into rtc_time structure
  */
@@ -113,9 +128,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_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
+	at91_rtc_write_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_write_idr(AT91_RTC_ACKUPD);
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -147,7 +162,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_read(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,7 +187,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_write_idr(AT91_RTC_ALARM);
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -185,7 +200,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	}
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -201,9 +216,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	} else
-		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
+		at91_rtc_write_idr(AT91_RTC_ALARM);
 
 	return 0;
 }
@@ -212,7 +227,7 @@ 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);
+	unsigned long imr = at91_rtc_read_imr();
 
 	seq_printf(seq, "update_IRQ\t: %s\n",
 			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
@@ -232,7 +247,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_read(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);
@@ -327,7 +342,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_MR, 0);		/* 24 hour mode */
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 
@@ -365,7 +380,7 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 	struct rtc_device *rtc = platform_get_drvdata(pdev);
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 	free_irq(irq, pdev);
@@ -387,13 +402,13 @@ 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_imr = at91_rtc_read(AT91_RTC_IMR)
+	at91_rtc_imr = at91_rtc_read_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_imr);
+			at91_rtc_write_idr(at91_rtc_imr);
 	}
 	return 0;
 }
@@ -404,7 +419,7 @@ static int at91_rtc_resume(struct device *dev)
 		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
 		else
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
+			at91_rtc_write_ier(at91_rtc_imr);
 	}
 	return 0;
 }
-- 
1.8.1.5

[RFC 4/5] rtc-at91rm9200: add shadow interrupt mask

[Johan Hovold]

Add shadow interrupt-mask register which can be used on SoCs where the
actual hardware register is broken.

Note that some care needs to be taken to make sure the shadow mask
corresponds to the actual hardware state. The added overhead is not an
issue for the non-broken SoCs due to the relatively infrequent
interrupt-mask updates. We do, however, only use the shadow mask value
as a fall-back when it actually needed as there is still a theoretical
possibility that the mask is incorrect (see the code for details).
---
 drivers/rtc/rtc-at91rm9200.c | 49 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index cb4462d..2921866 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -42,6 +42,7 @@
 #define AT91_RTC_EPOCH		1900UL	/* just like arch/arm/common/rtctime.c */
 
 struct at91_rtc_config {
+	bool use_shadow_imr;
 };
 
 static const struct at91_rtc_config *at91_rtc_config;
@@ -49,20 +50,66 @@ 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 DEFINE_SPINLOCK(at91_rtc_lock);
+static u32 at91_rtc_shadow_imr;
 
 static void at91_rtc_write_ier(u32 mask)
 {
+	unsigned long flags;
+
+	/*
+	 * Lock needed to make sure shadow mask is updated before interrupts
+	 * are enabled.
+	 */
+	spin_lock_irqsave(&at91_rtc_lock, flags);
+	at91_rtc_shadow_imr |= mask;
 	at91_rtc_write(AT91_RTC_IER, mask);
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static void at91_rtc_write_idr(u32 mask)
 {
+	unsigned long flags;
+
+	/*
+	 * Lock needed to make sure shadow mask is not updated before
+	 * interrupts have been disabled.
+	 */
+	spin_lock_irqsave(&at91_rtc_lock, flags);
 	at91_rtc_write(AT91_RTC_IDR, 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_shadow_imr &= ~mask;
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static u32 at91_rtc_read_imr(void)
 {
-	return at91_rtc_read(AT91_RTC_IMR);
+	unsigned long flags;
+	u32 mask;
+
+	if (at91_rtc_config->use_shadow_imr) {
+		/*
+		 * Lock not strictly necessary on UP.
+		 */
+		spin_lock_irqsave(&at91_rtc_lock, flags);
+		mask = at91_rtc_shadow_imr;
+		spin_unlock_irqrestore(&at91_rtc_lock, flags);
+	} else {
+		mask = at91_rtc_read(AT91_RTC_IMR);
+	}
+
+	return mask;
 }
 
 /*
-- 
1.8.1.5

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

[Johan Hovold]

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 */
 	}
 };
-- 
1.8.1.5

Re: [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@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

Re: [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

Re: [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

Re: [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

[RFC v2 0/4] rtc-at91rm9200: add support for at91sam9x5

[Johan Hovold]

This v2 of my version of the work-around for the buggy hardware
register on sam9x5, based on top of the revert of commit 0ef1594c017
("drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR") and the
DT-support patch in Andrew's queue.

If preferred, the generic configuration support added in the first patch
could easily be reduced to a single flag set based on the
compatible-property matching sam9x5.

Johan

v2
 - rebase on top of DT-support patch by Joachim Eastwood
 - add missing brace in DT-id table

Johan Hovold (4):
  rtc-at91rm9200: add configuration support
  rtc-at91rm9200: refactor interrupt-register handling
  rtc-at91rm9200: add shadow interrupt mask
  rtc-at91rm9200: add support for at91sam9x5

 .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   2 +-
 drivers/rtc/rtc-at91rm9200.c                       | 140 ++++++++++++++++++---
 2 files changed, 121 insertions(+), 21 deletions(-)

-- 
1.8.1.5

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

[Johan Hovold]

Add configuration support which can be used to implement SoC-specific
workarounds for broken hardware.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 79233d0..26e560c 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -42,6 +42,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;
@@ -250,6 +254,34 @@ 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 of_device_id at91_rtc_dt_ids[] = {
+	{
+		.compatible = "atmel,at91rm9200-rtc",
+		.data = &at91rm9200_config,
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
+
+static const struct at91_rtc_config *
+at91_rtc_get_config(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	if (pdev->dev.of_node) {
+		match = of_match_node(at91_rtc_dt_ids, pdev->dev.of_node);
+		if (!match)
+			return NULL;
+		return (const struct at91_rtc_config *)match->data;
+	}
+
+	return &at91rm9200_config;
+}
+
 static const struct rtc_class_ops at91_rtc_ops = {
 	.read_time	= at91_rtc_readtime,
 	.set_time	= at91_rtc_settime,
@@ -268,6 +300,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");
@@ -383,12 +419,6 @@ static const struct dev_pm_ops at91_rtc_pm = {
 #define at91_rtc_pm_ptr	NULL
 #endif
 
-static const struct of_device_id at91_rtc_dt_ids[] = {
-	{ .compatible = "atmel,at91rm9200-rtc" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
-
 static struct platform_driver at91_rtc_driver = {
 	.remove		= __exit_p(at91_rtc_remove),
 	.driver		= {
-- 
1.8.1.5

[RFC v2 2/4] rtc-at91rm9200: refactor interrupt-register handling

[Johan Hovold]

Add accessors for the interrupt register.

This will allow us to easily add a shadow interrupt-mask register to
use on SoCs where the interrupt-mask register cannot be used.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 26e560c..46576ff 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -51,6 +51,21 @@ static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 
+static void at91_rtc_write_ier(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IER, mask);
+}
+
+static void at91_rtc_write_idr(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IDR, mask);
+}
+
+static u32 at91_rtc_read_imr(void)
+{
+	return at91_rtc_read(AT91_RTC_IMR);
+}
+
 /*
  * Decode time/date into rtc_time structure
  */
@@ -114,9 +129,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_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
+	at91_rtc_write_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_write_idr(AT91_RTC_ACKUPD);
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -148,7 +163,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_read(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__,
@@ -173,7 +188,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_write_idr(AT91_RTC_ALARM);
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -186,7 +201,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	}
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -202,9 +217,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	} else
-		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
+		at91_rtc_write_idr(AT91_RTC_ALARM);
 
 	return 0;
 }
@@ -213,7 +228,7 @@ 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);
+	unsigned long imr = at91_rtc_read_imr();
 
 	seq_printf(seq, "update_IRQ\t: %s\n",
 			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
@@ -233,7 +248,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_read(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);
@@ -326,7 +341,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_MR, 0);		/* 24 hour mode */
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 
@@ -364,7 +379,7 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 	struct rtc_device *rtc = platform_get_drvdata(pdev);
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 	free_irq(irq, pdev);
@@ -386,13 +401,13 @@ 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_imr = at91_rtc_read(AT91_RTC_IMR)
+	at91_rtc_imr = at91_rtc_read_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_imr);
+			at91_rtc_write_idr(at91_rtc_imr);
 	}
 	return 0;
 }
@@ -403,7 +418,7 @@ static int at91_rtc_resume(struct device *dev)
 		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
 		else
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
+			at91_rtc_write_ier(at91_rtc_imr);
 	}
 	return 0;
 }
-- 
1.8.1.5

[RFC v2 3/4] rtc-at91rm9200: add shadow interrupt mask

[Johan Hovold]

Add shadow interrupt-mask register which can be used on SoCs where the
actual hardware register is broken.

Note that some care needs to be taken to make sure the shadow mask
corresponds to the actual hardware state. The added overhead is not an
issue for the non-broken SoCs due to the relatively infrequent
interrupt-mask updates. We do, however, only use the shadow mask value
as a fall-back when it actually needed as there is still a theoretical
possibility that the mask is incorrect (see the code for details).

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 50 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 46576ff..ac5f8ed 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -30,6 +30,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/spinlock.h>
 
 #include <asm/uaccess.h>
 
@@ -43,6 +44,7 @@
 #define AT91_RTC_EPOCH		1900UL	/* just like arch/arm/common/rtctime.c */
 
 struct at91_rtc_config {
+	bool use_shadow_imr;
 };
 
 static const struct at91_rtc_config *at91_rtc_config;
@@ -50,20 +52,66 @@ 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 DEFINE_SPINLOCK(at91_rtc_lock);
+static u32 at91_rtc_shadow_imr;
 
 static void at91_rtc_write_ier(u32 mask)
 {
+	unsigned long flags;
+
+	/*
+	 * Lock needed to make sure shadow mask is updated before interrupts
+	 * are enabled.
+	 */
+	spin_lock_irqsave(&at91_rtc_lock, flags);
+	at91_rtc_shadow_imr |= mask;
 	at91_rtc_write(AT91_RTC_IER, mask);
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static void at91_rtc_write_idr(u32 mask)
 {
+	unsigned long flags;
+
+	/*
+	 * Lock needed to make sure shadow mask is not updated before
+	 * interrupts have been disabled.
+	 */
+	spin_lock_irqsave(&at91_rtc_lock, flags);
 	at91_rtc_write(AT91_RTC_IDR, 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_shadow_imr &= ~mask;
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static u32 at91_rtc_read_imr(void)
 {
-	return at91_rtc_read(AT91_RTC_IMR);
+	unsigned long flags;
+	u32 mask;
+
+	if (at91_rtc_config->use_shadow_imr) {
+		/*
+		 * Lock not strictly necessary on UP.
+		 */
+		spin_lock_irqsave(&at91_rtc_lock, flags);
+		mask = at91_rtc_shadow_imr;
+		spin_unlock_irqrestore(&at91_rtc_lock, flags);
+	} else {
+		mask = at91_rtc_read(AT91_RTC_IMR);
+	}
+
+	return mask;
 }
 
 /*
-- 
1.8.1.5

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

[Johan Hovold]

Add support for the at91sam9x5-family which must use the shadow
interrupt mask due to a hardware issue.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt | 2 +-
 drivers/rtc/rtc-at91rm9200.c                                   | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
index 2a3feab..34c1505 100644
--- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
@@ -1,7 +1,7 @@
 Atmel AT91RM9200 Real Time Clock
 
 Required properties:
-- compatible: should be: "atmel,at91rm9200-rtc"
+- compatible: should be: "atmel,at91rm9200-rtc" or "atmel,at91sam9x5-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 ac5f8ed..7c19390 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -320,11 +320,18 @@ 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,
+};
+
 static const struct of_device_id at91_rtc_dt_ids[] = {
 	{
 		.compatible = "atmel,at91rm9200-rtc",
 		.data = &at91rm9200_config,
 	}, {
+		.compatible = "atmel,at91sam9x5-rtc",
+		.data = &at91sam9x5_config,
+	}, {
 		/* sentinel */
 	}
 };
-- 
1.8.1.5

[PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Johan Hovold]

This is an update of the shadow-interrupt-mask series against v3.10-rc2.

I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
affected. If not, then some probing mechanism as the one Doug suggested
could be implemented on top of (a subset of) these patches. What do you
say, Nicolas?

Note that the first patch (adding a missing OF compile guard) could be
applied straight away.

Thanks,
Johan


v3:
 - rebase against v3.10-rc2
 - remove some comments

v2:
 - rebase on top of DT-support patch by Joachim Eastwood
 - add missing brace in DT-id table


Johan Hovold (5):
  rtc-at91rm9200: add match-table compile guard
  rtc-at91rm9200: add configuration support
  rtc-at91rm9200: refactor interrupt-register handling
  rtc-at91rm9200: add shadow interrupt mask
  rtc-at91rm9200: use shadow IMR on at91sam9x5

 .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   2 +-
 arch/arm/boot/dts/at91sam9x5.dtsi                  |   2 +-
 drivers/rtc/rtc-at91rm9200.c                       | 131 +++++++++++++++++----
 3 files changed, 113 insertions(+), 22 deletions(-)

-- 
1.8.2.1

[PATCH v3 1/5] rtc-at91rm9200: add match-table compile guard

[Johan Hovold]

Add missing match-table compile guard.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 0eab77b..eeeb73f 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -383,11 +383,13 @@ static int at91_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(at91_rtc_pm_ops, at91_rtc_suspend, at91_rtc_resume);
 
+#ifdef CONFIG_OF
 static const struct of_device_id at91_rtc_dt_ids[] = {
 	{ .compatible = "atmel,at91rm9200-rtc" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
+#endif
 
 static struct platform_driver at91_rtc_driver = {
 	.remove		= __exit_p(at91_rtc_remove),
-- 
1.8.2.1

[PATCH v3 2/5] rtc-at91rm9200: add configuration support

[Johan Hovold]

Add configuration support which can be used to implement SoC-specific
workarounds for broken hardware.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 46 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index eeeb73f..ab2024b 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -42,6 +42,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;
@@ -250,6 +254,36 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	return IRQ_NONE;		/* not handled */
 }
 
+static const struct at91_rtc_config at91rm9200_config = {
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id at91_rtc_dt_ids[] = {
+	{
+		.compatible = "atmel,at91rm9200-rtc",
+		.data = &at91rm9200_config,
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
+#endif
+
+static const struct at91_rtc_config *
+at91_rtc_get_config(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	if (pdev->dev.of_node) {
+		match = of_match_node(at91_rtc_dt_ids, pdev->dev.of_node);
+		if (!match)
+			return NULL;
+		return (const struct at91_rtc_config *)match->data;
+	}
+
+	return &at91rm9200_config;
+}
+
 static const struct rtc_class_ops at91_rtc_ops = {
 	.read_time	= at91_rtc_readtime,
 	.set_time	= at91_rtc_settime,
@@ -268,6 +302,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");
@@ -383,14 +421,6 @@ static int at91_rtc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(at91_rtc_pm_ops, at91_rtc_suspend, at91_rtc_resume);
 
-#ifdef CONFIG_OF
-static const struct of_device_id at91_rtc_dt_ids[] = {
-	{ .compatible = "atmel,at91rm9200-rtc" },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, at91_rtc_dt_ids);
-#endif
-
 static struct platform_driver at91_rtc_driver = {
 	.remove		= __exit_p(at91_rtc_remove),
 	.driver		= {
-- 
1.8.2.1

[PATCH v3 3/5] rtc-at91rm9200: refactor interrupt-register handling

[Johan Hovold]

Add accessors for the interrupt register.

This will allow us to easily add a shadow interrupt-mask register to
use on SoCs where the interrupt-mask register cannot be used.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index ab2024b..9592d08 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -51,6 +51,21 @@ static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 
+static void at91_rtc_write_ier(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IER, mask);
+}
+
+static void at91_rtc_write_idr(u32 mask)
+{
+	at91_rtc_write(AT91_RTC_IDR, mask);
+}
+
+static u32 at91_rtc_read_imr(void)
+{
+	return at91_rtc_read(AT91_RTC_IMR);
+}
+
 /*
  * Decode time/date into rtc_time structure
  */
@@ -114,9 +129,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_write(AT91_RTC_IER, AT91_RTC_ACKUPD);
+	at91_rtc_write_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_write_idr(AT91_RTC_ACKUPD);
 
 	at91_rtc_write(AT91_RTC_TIMR,
 			  bin2bcd(tm->tm_sec) << 0
@@ -148,7 +163,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_read(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__,
@@ -173,7 +188,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_write_idr(AT91_RTC_ALARM);
 	at91_rtc_write(AT91_RTC_TIMALR,
 		  bin2bcd(tm.tm_sec) << 0
 		| bin2bcd(tm.tm_min) << 8
@@ -186,7 +201,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	}
 
 	dev_dbg(dev, "%s(): %4d-%02d-%02d %02d:%02d:%02d\n", __func__,
@@ -202,9 +217,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_write(AT91_RTC_IER, AT91_RTC_ALARM);
+		at91_rtc_write_ier(AT91_RTC_ALARM);
 	} else
-		at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
+		at91_rtc_write_idr(AT91_RTC_ALARM);
 
 	return 0;
 }
@@ -213,7 +228,7 @@ 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);
+	unsigned long imr = at91_rtc_read_imr();
 
 	seq_printf(seq, "update_IRQ\t: %s\n",
 			(imr & AT91_RTC_ACKUPD) ? "yes" : "no");
@@ -233,7 +248,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_read(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);
@@ -328,7 +343,7 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 	at91_rtc_write(AT91_RTC_MR, 0);		/* 24 hour mode */
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 
@@ -373,7 +388,7 @@ static int __exit at91_rtc_remove(struct platform_device *pdev)
 	struct rtc_device *rtc = platform_get_drvdata(pdev);
 
 	/* Disable all interrupts */
-	at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ACKUPD | AT91_RTC_ALARM |
+	at91_rtc_write_idr(AT91_RTC_ACKUPD | AT91_RTC_ALARM |
 					AT91_RTC_SECEV | AT91_RTC_TIMEV |
 					AT91_RTC_CALEV);
 	free_irq(irq, pdev);
@@ -396,13 +411,13 @@ 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_imr = at91_rtc_read(AT91_RTC_IMR)
+	at91_rtc_imr = at91_rtc_read_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_imr);
+			at91_rtc_write_idr(at91_rtc_imr);
 	}
 	return 0;
 }
@@ -413,7 +428,7 @@ static int at91_rtc_resume(struct device *dev)
 		if (device_may_wakeup(dev))
 			disable_irq_wake(irq);
 		else
-			at91_rtc_write(AT91_RTC_IER, at91_rtc_imr);
+			at91_rtc_write_ier(at91_rtc_imr);
 	}
 	return 0;
 }
-- 
1.8.2.1

[PATCH v3 4/5] rtc-at91rm9200: add shadow interrupt mask

[Johan Hovold]

Add shadow interrupt-mask register which can be used on SoCs where the
actual hardware register is broken.

Note that some care needs to be taken to make sure the shadow mask
corresponds to the actual hardware state. The added overhead is not an
issue for the non-broken SoCs due to the relatively infrequent
interrupt-mask updates. We do, however, only use the shadow mask value
as a fall-back when it actually needed as there is still a theoretical
possibility that the mask is incorrect (see the code for details).

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/rtc/rtc-at91rm9200.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 9592d08..205701e 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -30,6 +30,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/spinlock.h>
 
 #include <asm/uaccess.h>
 
@@ -43,6 +44,7 @@
 #define AT91_RTC_EPOCH		1900UL	/* just like arch/arm/common/rtctime.c */
 
 struct at91_rtc_config {
+	bool use_shadow_imr;
 };
 
 static const struct at91_rtc_config *at91_rtc_config;
@@ -50,20 +52,55 @@ 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 DEFINE_SPINLOCK(at91_rtc_lock);
+static u32 at91_rtc_shadow_imr;
 
 static void at91_rtc_write_ier(u32 mask)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&at91_rtc_lock, flags);
+	at91_rtc_shadow_imr |= mask;
 	at91_rtc_write(AT91_RTC_IER, mask);
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static void at91_rtc_write_idr(u32 mask)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&at91_rtc_lock, flags);
 	at91_rtc_write(AT91_RTC_IDR, 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_shadow_imr &= ~mask;
+	spin_unlock_irqrestore(&at91_rtc_lock, flags);
 }
 
 static u32 at91_rtc_read_imr(void)
 {
-	return at91_rtc_read(AT91_RTC_IMR);
+	unsigned long flags;
+	u32 mask;
+
+	if (at91_rtc_config->use_shadow_imr) {
+		spin_lock_irqsave(&at91_rtc_lock, flags);
+		mask = at91_rtc_shadow_imr;
+		spin_unlock_irqrestore(&at91_rtc_lock, flags);
+	} else {
+		mask = at91_rtc_read(AT91_RTC_IMR);
+	}
+
+	return mask;
 }
 
 /*
-- 
1.8.2.1

[PATCH v3 5/5] rtc-at91rm9200: use shadow IMR on at91sam9x5

[Johan Hovold]

Add support for the at91sam9x5-family which must use the shadow
interrupt mask due to a hardware issue (causing RTC_IMR to always be
zero).

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt | 2 +-
 arch/arm/boot/dts/at91sam9x5.dtsi                              | 2 +-
 drivers/rtc/rtc-at91rm9200.c                                   | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
index 2a3feab..34c1505 100644
--- a/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/atmel,at91rm9200-rtc.txt
@@ -1,7 +1,7 @@
 Atmel AT91RM9200 Real Time Clock
 
 Required properties:
-- compatible: should be: "atmel,at91rm9200-rtc"
+- compatible: should be: "atmel,at91rm9200-rtc" or "atmel,at91sam9x5-rtc"
 - reg: physical base address of the controller and length of memory mapped
   region.
 - interrupts: rtc alarm/event interrupt
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index 1145ac3..b5833d1f 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -643,7 +643,7 @@
 			};
 
 			rtc@fffffeb0 {
-				compatible = "atmel,at91rm9200-rtc";
+				compatible = "atmel,at91sam9x5-rtc";
 				reg = <0xfffffeb0 0x40>;
 				interrupts = <1 4 7>;
 				status = "disabled";
diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 205701e..47416e9 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -309,12 +309,19 @@ 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,
+};
+
 #ifdef 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,
+	}, {
 		/* sentinel */
 	}
 };
-- 
1.8.2.1

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Andrew Morton]

On Thu, 23 May 2013 10:38:50 +0200 Johan Hovold <jhovold@gmail.com> wrote:

> This is an update of the shadow-interrupt-mask series against v3.10-rc2.
> 
> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
> affected. If not, then some probing mechanism as the one Doug suggested
> could be implemented on top of (a subset of) these patches. What do you
> say, Nicolas?
> 
> Note that the first patch (adding a missing OF compile guard) could be
> applied straight away.

At this stage it is unclear to me how to proceed with patches 2-5.

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Robert Nelson]

On Wed, May 29, 2013 at 3:33 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 23 May 2013 10:38:50 +0200 Johan Hovold <jhovold@gmail.com> wrote:
>
>> This is an update of the shadow-interrupt-mask series against v3.10-rc2.
>>
>> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
>> affected. If not, then some probing mechanism as the one Doug suggested
>> could be implemented on top of (a subset of) these patches. What do you
>> say, Nicolas?
>>
>> Note that the first patch (adding a missing OF compile guard) could be
>> applied straight away.
>
> At this stage it is unclear to me how to proceed with patches 2-5.

fyi:

A version of these patches had been applied once before:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0ef1594c017521ea89278e80fe3f80dafb17abde

But due to a few issues it was later reverted:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e24b0bfa2f0446ffaad2661040be23668133aef8

Regards,

-- 
Robert Nelson
http://www.rcn-ee.com/

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Douglas Gilbert]

On 13-05-29 04:41 PM, Robert Nelson wrote:
> On Wed, May 29, 2013 at 3:33 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Thu, 23 May 2013 10:38:50 +0200 Johan Hovold <jhovold@gmail.com> wrote:
>>
>>> This is an update of the shadow-interrupt-mask series against v3.10-rc2.
>>>
>>> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
>>> affected. If not, then some probing mechanism as the one Doug suggested
>>> could be implemented on top of (a subset of) these patches. What do you
>>> say, Nicolas?
>>>
>>> Note that the first patch (adding a missing OF compile guard) could be
>>> applied straight away.
>>
>> At this stage it is unclear to me how to proceed with patches 2-5.
>
> fyi:
>
> A version of these patches had been applied once before:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0ef1594c017521ea89278e80fe3f80dafb17abde
>
> But due to a few issues it was later reverted:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e24b0bfa2f0446ffaad2661040be23668133aef8

Strange life of a patch. Mine was the original, Johan Hovold
objected and had it reverted. Johan then presented his first
patch then v2. They got lost in the weeds.

My hardware was still broken and this bug caused collateral
damage. My original patch no longer applied to lk 3.10.0-rc1
so I rewrote it, borrowing some of Johan's ideas and doing a
probe time check for the broken RTC_IMR. That patch was
presented about a week ago:
   http://marc.info/?l=linux-arm-kernel&m=136917492531478&w=2
The top of that post gives some more background.

That prompted Johan to produce v3 of his patch which is the
subject of this thread. I was hoping that Nicolas Ferre would
comment or ack one of these patches. Still waiting.


I have a copy of the original, publicly released manual for
the at91sam9g25 (a member of the at91sam9x5 family) marked
"11032A–ATARM–27-Jul-11". It contains the following:
   Errata
   49.3.1
     RTC: Interrupt Mask Register cannot be used
          Interrupt Mask Register reading always returns 0.

Both Rev B and Rev C of that manual drop that particular
erratum. My g25 SoC-based subsystems come from an Atmel
partner and still have the RTC IMR bug.

Doug Gilbert

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Nicolas Ferre]

On 30/05/2013 01:22, Douglas Gilbert :
> On 13-05-29 04:41 PM, Robert Nelson wrote:
>> On Wed, May 29, 2013 at 3:33 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>>> On Thu, 23 May 2013 10:38:50 +0200 Johan Hovold <jhovold@gmail.com>
>>> wrote:
>>>
>>>> This is an update of the shadow-interrupt-mask series against
>>>> v3.10-rc2.
>>>>
>>>> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
>>>> affected. If not, then some probing mechanism as the one Doug suggested
>>>> could be implemented on top of (a subset of) these patches. What do you
>>>> say, Nicolas?
>>>>
>>>> Note that the first patch (adding a missing OF compile guard) could be
>>>> applied straight away.
>>>
>>> At this stage it is unclear to me how to proceed with patches 2-5.
>>
>> fyi:
>>
>> A version of these patches had been applied once before:
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0ef1594c017521ea89278e80fe3f80dafb17abde
>>
>>
>> But due to a few issues it was later reverted:
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e24b0bfa2f0446ffaad2661040be23668133aef8
>>
>
> Strange life of a patch. Mine was the original, Johan Hovold
> objected and had it reverted. Johan then presented his first
> patch then v2. They got lost in the weeds.

No, they were not lost. No patch is ever lost and this thread is the proof.

> My hardware was still broken and this bug caused collateral
> damage. My original patch no longer applied to lk 3.10.0-rc1
> so I rewrote it, borrowing some of Johan's ideas and doing a
> probe time check for the broken RTC_IMR. That patch was
> presented about a week ago:
>    http://marc.info/?l=linux-arm-kernel&m=136917492531478&w=2
> The top of that post gives some more background.
>
> That prompted Johan to produce v3 of his patch which is the
> subject of this thread. I was hoping that Nicolas Ferre would
> comment or ack one of these patches. Still waiting.

Sure that all this did not progressed at the speed you expected. I 
understand that. But even if I did not answered in a timely manner, that 
does not mean that I didn't considered it and marked it as "things to be 
done before next merge window"...

So, today, too late, I gave my "Acked-by". Sorry for the delay. Let's 
still monitor the progress of this series upstream.

> I have a copy of the original, publicly released manual for
> the at91sam9g25 (a member of the at91sam9x5 family) marked
> "11032A–ATARM–27-Jul-11". It contains the following:
>    Errata
>    49.3.1
>      RTC: Interrupt Mask Register cannot be used
>           Interrupt Mask Register reading always returns 0.
>
> Both Rev B and Rev C of that manual drop that particular
> erratum. My g25 SoC-based subsystems come from an Atmel
> partner and still have the RTC IMR bug.

We already talked about this Douglas. Why are you saying this again. So, 
to summarize:

1/ each and every at91sam9x5 family SoC have and will probably always 
have this IMR bug (including 9g25 which is part of the family).

2/ you kindly reported the errata disappearing in the documentation. It 
is an error with document appearance which you probably noted. I have 
made the necessary actions to correct this. But here again, you have to 
be patient waiting for the datasheet's next revision.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Nicolas Ferre]

On 29/05/2013 22:41, Robert Nelson :
> On Wed, May 29, 2013 at 3:33 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Thu, 23 May 2013 10:38:50 +0200 Johan Hovold <jhovold@gmail.com> wrote:
>>
>>> This is an update of the shadow-interrupt-mask series against v3.10-rc2.
>>>
>>> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
>>> affected. If not, then some probing mechanism as the one Doug suggested
>>> could be implemented on top of (a subset of) these patches. What do you
>>> say, Nicolas?
>>>
>>> Note that the first patch (adding a missing OF compile guard) could be
>>> applied straight away.
>>
>> At this stage it is unclear to me how to proceed with patches 2-5.
>
> fyi:
>
> A version of these patches had been applied once before:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0ef1594c017521ea89278e80fe3f80dafb17abde
>
> But due to a few issues it was later reverted:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e24b0bfa2f0446ffaad2661040be23668133aef8

This new revision doesn't have the issue encountered by first version.

Andrew,

The review of this patch series was in my TODO list for some time...

Today, I magically took time to review it ;-)
The patch series is good and I (even if it is too late) here is my:

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

I do not know if the series can be stacked for inclusion in 3.10-rc but 
the resolution of this bug can help a lot (as Douglas is saying in 
subsequent email...).

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Andrew Morton]

On Thu, 30 May 2013 09:50:27 +0200 Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> The review of this patch series was in my TODO list for some time...
> 
> Today, I magically took time to review it ;-)
> The patch series is good and I (even if it is too late) here is my:
> 
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> I do not know if the series can be stacked for inclusion in 3.10-rc but 
> the resolution of this bug can help a lot (as Douglas is saying in 
> subsequent email...).

We can do that, but looking through the discussion and changelogs I
can't seem to find a usable description of what impact the bug (and its
fix) have upon end-users.

A nicely packaged description of that impact would help grease the
wheels, please.

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Douglas Gilbert]

On 13-05-30 03:36 PM, Andrew Morton wrote:
> On Thu, 30 May 2013 09:50:27 +0200 Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>
>> The review of this patch series was in my TODO list for some time...
>>
>> Today, I magically took time to review it ;-)
>> The patch series is good and I (even if it is too late) here is my:
>>
>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>
>> I do not know if the series can be stacked for inclusion in 3.10-rc but
>> the resolution of this bug can help a lot (as Douglas is saying in
>> subsequent email...).
>
> We can do that, but looking through the discussion and changelogs I
> can't seem to find a usable description of what impact the bug (and its
> fix) have upon end-users.
>
> A nicely packaged description of that impact would help grease the
> wheels, please.

How about this:

The members of Atmel's at91sam9x5 family (9x5) have
a broken RTC interrupt mask register (AT91_RTC_IMR).
It does not reflect enabled interrupts but instead
always returns zero.

The kernel's rtc-at91rm9200 driver handles the RTC
for the 9x5 family. Currently when the date/time is
set, an interrupt is generated and this driver neglects
to handle the interrupt. The kernel complains about the
un-handled interrupt and disables it henceforth. This
not only breaks the RTC function, but since that
interrupt is shared (Atmel's SYS interrupt) then other
things break as well (e.g. the debug port no longer
accepts characters).

Tested on the at91sam9g25. Bug confirmed by Atmel.

Edit as you please.

Doug Gilbert

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Nicolas Ferre]

On 31/05/2013 01:17, Douglas Gilbert :
> On 13-05-30 03:36 PM, Andrew Morton wrote:
>> On Thu, 30 May 2013 09:50:27 +0200 Nicolas Ferre
>> <nicolas.ferre@atmel.com> wrote:
>>
>>> The review of this patch series was in my TODO list for some time...
>>>
>>> Today, I magically took time to review it ;-)
>>> The patch series is good and I (even if it is too late) here is my:
>>>
>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>
>>> I do not know if the series can be stacked for inclusion in 3.10-rc but
>>> the resolution of this bug can help a lot (as Douglas is saying in
>>> subsequent email...).
>>
>> We can do that, but looking through the discussion and changelogs I
>> can't seem to find a usable description of what impact the bug (and its
>> fix) have upon end-users.
>>
>> A nicely packaged description of that impact would help grease the
>> wheels, please.
>
> How about this:
>
> The members of Atmel's at91sam9x5 family (9x5) have
> a broken RTC interrupt mask register (AT91_RTC_IMR).
> It does not reflect enabled interrupts but instead
> always returns zero.
>
> The kernel's rtc-at91rm9200 driver handles the RTC
> for the 9x5 family. Currently when the date/time is
> set, an interrupt is generated and this driver neglects
> to handle the interrupt. The kernel complains about the
> un-handled interrupt and disables it henceforth. This
> not only breaks the RTC function, but since that
> interrupt is shared (Atmel's SYS interrupt) then other
> things break as well (e.g. the debug port no longer
> accepts characters).
>
> Tested on the at91sam9g25. Bug confirmed by Atmel.

Absolutely. Thank you Douglas for the detailed description.

>
> Edit as you please.
>
> Doug Gilbert
>
>


-- 
Nicolas Ferre

Re: [PATCH v3 0/5] rtc-at91rm9200: add shadow interrupt mask

[Nicolas Ferre]

On 23/05/2013 10:38, Johan Hovold :
> This is an update of the shadow-interrupt-mask series against v3.10-rc2.
>
> I guess we need Atmel to confirm that all sam9x5 SoCs are indeed
> affected. If not, then some probing mechanism as the one Doug suggested
> could be implemented on top of (a subset of) these patches. What do you
> say, Nicolas?

No probing mechanism is needed: only sam9x5 are affected.

> Note that the first patch (adding a missing OF compile guard) could be
> applied straight away.
>
> Thanks,
> Johan
>
>
> v3:
>   - rebase against v3.10-rc2
>   - remove some comments

Thanks for rebasing this series.

Best regards,

> v2:
>   - rebase on top of DT-support patch by Joachim Eastwood
>   - add missing brace in DT-id table
>
>
> Johan Hovold (5):
>    rtc-at91rm9200: add match-table compile guard
>    rtc-at91rm9200: add configuration support
>    rtc-at91rm9200: refactor interrupt-register handling
>    rtc-at91rm9200: add shadow interrupt mask
>    rtc-at91rm9200: use shadow IMR on at91sam9x5
>
>   .../bindings/rtc/atmel,at91rm9200-rtc.txt          |   2 +-
>   arch/arm/boot/dts/at91sam9x5.dtsi                  |   2 +-
>   drivers/rtc/rtc-at91rm9200.c                       | 131 +++++++++++++++++----
>   3 files changed, 113 insertions(+), 22 deletions(-)
>


-- 
Nicolas Ferre

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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