| Message ID | 1399479649-3247-1-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Boris, > -----Original Message----- > From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com] > Sent: Wednesday, May 07, 2014 12:21 PM > To: Bryan Evenson > Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Boris BREZILLON > Subject: [PATCH] ARM: at91: fix rtc irq mask for sam9x5 SoCs > > The RTC IMR register is not reliable on sam9x5 SoCs, hence why me have to > mask all interrupts no matter what IMR claims about already masked irqs. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Reported-by: Bryan Evenson <bevenson@melinkcorp.com> > --- > Hello Bryan, > > Yet another patch for you ;-). > > As usual, could you tell me if it fixes your bug. > > BTW, thanks for your tests. > > Best Regards, > > Boris This fixes the issue on my system. I disconnected my system from the network to guarantee ntp didn't adjust the time. I power cycled my system 15 times with the RTC backup battery in place, and each time it booted without issue and the RTC kept the correct time. Before this patch my system would never complete booting with the RTC battery installed. I also used my previous test script for the previous patch to verify that I could not lockup access to the RTC. Tested-by: Bryan Evenson <bevenson@melinkcorp.com> Regards, Bryan > > arch/arm/mach-at91/sysirq_mask.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/arm/mach-at91/sysirq_mask.c b/arch/arm/mach- > at91/sysirq_mask.c > index 2ba694f..eb3d2a5 100644 > --- a/arch/arm/mach-at91/sysirq_mask.c > +++ b/arch/arm/mach-at91/sysirq_mask.c > @@ -37,12 +37,7 @@ void __init at91_sysirq_mask_rtc(u32 rtc_base) > if (!base) > return; > > - mask = readl_relaxed(base + AT91_RTC_IMR); > - if (mask) { > - pr_info("AT91: Disabling rtc irq\n"); > - writel_relaxed(mask, base + AT91_RTC_IDR); > - (void)readl_relaxed(base + AT91_RTC_IMR); /* flush */ > - } > + writel_relaxed(0x1f, base + AT91_RTC_IDR); > > iounmap(base); > } > -- > 1.8.3.2
Atmel actually has this issue in the Errata of the SAM9G25 and SAM9G35 datasheets which might be worth referencing in the description? >49.7.1 RTC: Interrupt Mask Register cannot be used >Interrupt Mask Register read always returns 0.
On Wed, May 07, 2014 at 06:20:49PM +0200, Boris BREZILLON wrote: > The RTC IMR register is not reliable on sam9x5 SoCs, hence why me have to > mask all interrupts no matter what IMR claims about already masked irqs. Crap, I totally forgot about this. Doug reported the problem off-list back in December, but it got lost somehow. Sorry. > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Reported-by: Bryan Evenson <bevenson@melinkcorp.com> > --- > Hello Bryan, > > Yet another patch for you ;-). > > As usual, could you tell me if it fixes your bug. > > BTW, thanks for your tests. > > Best Regards, > > Boris > > arch/arm/mach-at91/sysirq_mask.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/arm/mach-at91/sysirq_mask.c b/arch/arm/mach-at91/sysirq_mask.c > index 2ba694f..eb3d2a5 100644 > --- a/arch/arm/mach-at91/sysirq_mask.c > +++ b/arch/arm/mach-at91/sysirq_mask.c > @@ -37,12 +37,7 @@ void __init at91_sysirq_mask_rtc(u32 rtc_base) > if (!base) > return; > > - mask = readl_relaxed(base + AT91_RTC_IMR); > - if (mask) { > - pr_info("AT91: Disabling rtc irq\n"); > - writel_relaxed(mask, base + AT91_RTC_IDR); > - (void)readl_relaxed(base + AT91_RTC_IMR); /* flush */ > - } > + writel_relaxed(0x1f, base + AT91_RTC_IDR); I believe this is the right way to handle this hardware bug (IMR is always read as 0 on one particular SoC), but please document this in a comment. You should also keep the flush (read of IMR) regardless (to make sure the write has reached the peripheral), and remember to remove the now unused mask variable. > iounmap(base); > } Thanks, Johan
[ Sorry for the resend -- forgot to add Doug as CC. ] On Wed, May 07, 2014 at 06:20:49PM +0200, Boris BREZILLON wrote: > The RTC IMR register is not reliable on sam9x5 SoCs, hence why me have to > mask all interrupts no matter what IMR claims about already masked irqs. Crap, I totally forgot about this. Doug reported the problem off-list back in December, but it got lost somehow. Sorry. > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Reported-by: Bryan Evenson <bevenson@melinkcorp.com> > --- > Hello Bryan, > > Yet another patch for you ;-). > > As usual, could you tell me if it fixes your bug. > > BTW, thanks for your tests. > > Best Regards, > > Boris > > arch/arm/mach-at91/sysirq_mask.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/arm/mach-at91/sysirq_mask.c b/arch/arm/mach-at91/sysirq_mask.c > index 2ba694f..eb3d2a5 100644 > --- a/arch/arm/mach-at91/sysirq_mask.c > +++ b/arch/arm/mach-at91/sysirq_mask.c > @@ -37,12 +37,7 @@ void __init at91_sysirq_mask_rtc(u32 rtc_base) > if (!base) > return; > > - mask = readl_relaxed(base + AT91_RTC_IMR); > - if (mask) { > - pr_info("AT91: Disabling rtc irq\n"); > - writel_relaxed(mask, base + AT91_RTC_IDR); > - (void)readl_relaxed(base + AT91_RTC_IMR); /* flush */ > - } > + writel_relaxed(0x1f, base + AT91_RTC_IDR); I believe this is the right way to handle this hardware bug (IMR is always read as 0 on one particular SoC), but please document this in a comment. You should also keep the flush (read of IMR) regardless (to make sure the write has reached the peripheral), and remember to remove the now unused mask variable. > iounmap(base); > } Thanks, Johan
On 08/05/2014 05:10, Mark Roszko wrote: > Atmel actually has this issue in the Errata of the SAM9G25 and SAM9G35 > datasheets which might be worth referencing in the description? > >> 49.7.1 RTC: Interrupt Mask Register cannot be used >> Interrupt Mask Register read always returns 0. Sure, I'll quote atmel's datasheet and state that it only impacts sam9g25 and g35 SoCs. Thanks, Boris
On 08/05/2014 17:49, Johan Hovold wrote: > On Wed, May 07, 2014 at 06:20:49PM +0200, Boris BREZILLON wrote: >> The RTC IMR register is not reliable on sam9x5 SoCs, hence why me have to >> mask all interrupts no matter what IMR claims about already masked irqs. > Crap, I totally forgot about this. Doug reported the problem off-list > back in December, but it got lost somehow. Sorry. No problem. BTW, I started to work on a more generic solution to handle these muxed irqs issues (see https://lkml.org/lkml/2014/3/28/353). Could you take a look at it (I'm still not happy with the proposed DT bindings, but this can be discussed)? >> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> >> Reported-by: Bryan Evenson <bevenson@melinkcorp.com> >> --- >> Hello Bryan, >> >> Yet another patch for you ;-). >> >> As usual, could you tell me if it fixes your bug. >> >> BTW, thanks for your tests. >> >> Best Regards, >> >> Boris >> >> arch/arm/mach-at91/sysirq_mask.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/arch/arm/mach-at91/sysirq_mask.c b/arch/arm/mach-at91/sysirq_mask.c >> index 2ba694f..eb3d2a5 100644 >> --- a/arch/arm/mach-at91/sysirq_mask.c >> +++ b/arch/arm/mach-at91/sysirq_mask.c >> @@ -37,12 +37,7 @@ void __init at91_sysirq_mask_rtc(u32 rtc_base) >> if (!base) >> return; >> >> - mask = readl_relaxed(base + AT91_RTC_IMR); >> - if (mask) { >> - pr_info("AT91: Disabling rtc irq\n"); >> - writel_relaxed(mask, base + AT91_RTC_IDR); >> - (void)readl_relaxed(base + AT91_RTC_IMR); /* flush */ >> - } >> + writel_relaxed(0x1f, base + AT91_RTC_IDR); > I believe this is the right way to handle this hardware bug (IMR is > always read as 0 on one particular SoC), but please document this in a > comment. Sure, I'll quote atmel's datasheet describing the errata. > > You should also keep the flush (read of IMR) regardless (to make sure > the write has reached the peripheral), and remember to remove the now > unused mask variable. Does it has something to do with memory barriers ? If so, why not using writel instead of writel_relaxed ? If not, could you point out where it is described in the datasheet ? Best Regards, Boris > >> iounmap(base); >> } > Thanks, > Johan
On Thu, May 08, 2014 at 07:28:04PM +0200, Boris BREZILLON wrote: > > You should also keep the flush (read of IMR) regardless (to make sure > > the write has reached the peripheral), and remember to remove the now > > unused mask variable. > > Does it has something to do with memory barriers ? > If so, why not using writel instead of writel_relaxed ? You only need to use the non-relaxed version when synchronising with DMA operations. The read-back of a register on the same device is a common technique to make sure that preceding write has actually reached the peripheral (write posting or flushing). In this case, it is used to make (reasonably) sure that interrupts have actually been masked before returning. (In the general case, you'd even need to verify the read-back value to be certain that the device has changed its state.) Johan
diff --git a/arch/arm/mach-at91/sysirq_mask.c b/arch/arm/mach-at91/sysirq_mask.c index 2ba694f..eb3d2a5 100644 --- a/arch/arm/mach-at91/sysirq_mask.c +++ b/arch/arm/mach-at91/sysirq_mask.c @@ -37,12 +37,7 @@ void __init at91_sysirq_mask_rtc(u32 rtc_base) if (!base) return; - mask = readl_relaxed(base + AT91_RTC_IMR); - if (mask) { - pr_info("AT91: Disabling rtc irq\n"); - writel_relaxed(mask, base + AT91_RTC_IDR); - (void)readl_relaxed(base + AT91_RTC_IMR); /* flush */ - } + writel_relaxed(0x1f, base + AT91_RTC_IDR); iounmap(base); }
The RTC IMR register is not reliable on sam9x5 SoCs, hence why me have to mask all interrupts no matter what IMR claims about already masked irqs. Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> Reported-by: Bryan Evenson <bevenson@melinkcorp.com> --- Hello Bryan, Yet another patch for you ;-). As usual, could you tell me if it fixes your bug. BTW, thanks for your tests. Best Regards, Boris arch/arm/mach-at91/sysirq_mask.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)