Re: [RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision
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 >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision
On Tue, Apr 02, 2013 at 06:36:06PM +0200, Nicolas Ferre wrote: > Signed-off-by: Nicolas Ferre > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH v2] rtc: rtc-at91rm9200: manage IMR depending on revision
Signed-off-by: Nicolas Ferre
---
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
#include
#include
+#include
#include
#include
#include
@@ -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_RT
