* [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" [not found] <20130403095150.GC27600@localhost> @ 2013-04-03 9:54 ` Johan Hovold 2013-04-03 10:18 ` Nicolas Ferre 0 siblings, 1 reply; 5+ messages in thread From: Johan Hovold @ 2013-04-03 9:54 UTC (permalink / raw) To: Nicolas Ferre Cc: Douglas Gilbert, Jean-Christophe PLAGNIOL-VILLARD, Ludovic Desroches, linux-arm-kernel, linux-kernel, Johan Hovold, stable 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" 2013-04-03 9:54 ` [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" Johan Hovold @ 2013-04-03 10:18 ` Nicolas Ferre 2013-04-05 14:14 ` Nicolas Ferre 0 siblings, 1 reply; 5+ messages in thread From: Nicolas Ferre @ 2013-04-03 10:18 UTC (permalink / raw) To: Johan Hovold, Andrew Morton Cc: Douglas Gilbert, Jean-Christophe PLAGNIOL-VILLARD, Ludovic Desroches, linux-arm-kernel, linux-kernel, stable 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" 2013-04-03 10:18 ` Nicolas Ferre @ 2013-04-05 14:14 ` Nicolas Ferre 2013-04-05 15:35 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Nicolas Ferre @ 2013-04-05 14:14 UTC (permalink / raw) To: Andrew Morton Cc: Johan Hovold, linux-kernel, stable, Ludovic Desroches, Douglas Gilbert, Jean-Christophe PLAGNIOL-VILLARD, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" 2013-04-05 14:14 ` Nicolas Ferre @ 2013-04-05 15:35 ` Greg KH 2013-04-05 16:16 ` Nicolas Ferre 0 siblings, 1 reply; 5+ messages in thread From: Greg KH @ 2013-04-05 15:35 UTC (permalink / raw) To: Nicolas Ferre Cc: Andrew Morton, Johan Hovold, linux-kernel, stable, Ludovic Desroches, Douglas Gilbert, Jean-Christophe PLAGNIOL-VILLARD, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" 2013-04-05 15:35 ` Greg KH @ 2013-04-05 16:16 ` Nicolas Ferre 0 siblings, 0 replies; 5+ messages in thread From: Nicolas Ferre @ 2013-04-05 16:16 UTC (permalink / raw) To: torvalds Cc: akpm, jhovold, linux-kernel, stable, dgilbert, linux-arm-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-05 16:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20130403095150.GC27600@localhost>
2013-04-03 9:54 ` [PATCH] Revert "drivers/rtc/rtc-at91rm9200.c: use a variable for storing IMR" Johan Hovold
2013-04-03 10:18 ` Nicolas Ferre
2013-04-05 14:14 ` Nicolas Ferre
2013-04-05 15:35 ` Greg KH
2013-04-05 16:16 ` Nicolas Ferre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).