| Message ID | 1347569157-19262-1-git-send-email-khilman@deeprootsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kevin, On Fri, Sep 14, 2012 at 2:15 AM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > From: Kevin Hilman <khilman@ti.com> > > On some platforms, bootloaders are known to do some interesting RTC > programming. Without going into the obscurities as to why this may be > the case, suffice it to say the the driver should not make any > assumptions about the state of the RTC when the driver loads. In > particular, the driver probe should be sure that all interrupts are > disabled until otherwise programmed. > > This was discovered when finding bursty I2C traffic every second on > Overo platforms. This I2C overhead was keeping the SoC from hitting > deep power states. The cause was found to be the RTC firing every > second on the I2C-connected TWL PMIC. > > Special thanks to Felipe Balbi for suggesting to look for a rogue > driver as the source of the I2C traffic rather than the I2C driver > itself. > > Special thanks to Steve Sakoman for helping track down the source of > the continuous RTC interrups on the Overo boards. > Tested that the continuous interrupt issue after doing a i2c mm on omap4sdp. This patch solves the issue. thanks, > Cc: Felipe Balbi <balbi@ti.com> > Cc: Steve Sakoman <steve@sakoman.com> > Signed-off-by: Kevin Hilman <khilman@ti.com> > --- > Patch applies to v3.6-rc5 > > drivers/rtc/rtc-twl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c > index c5d06fe..9277d94 100644 > --- a/drivers/rtc/rtc-twl.c > +++ b/drivers/rtc/rtc-twl.c > @@ -495,6 +495,11 @@ static int __devinit twl_rtc_probe(struct platform_device *pdev) > if (ret < 0) > goto out1; > > + /* ensure interrupts are disabled, bootloaders can be strange */ > + ret = twl_rtc_write_u8(0, REG_RTC_INTERRUPTS_REG); > + if (ret < 0) > + dev_warn(&pdev->dev, "unable to disable interrupt\n"); > + Now that it is always 0 can the below read be removed as it is redundant now. > /* init cached IRQ enable bits */ > ret = twl_rtc_read_u8(&rtc_irq_bits, REG_RTC_INTERRUPTS_REG); > if (ret < 0) > -- > 1.7.9.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Shubhrajyoti Datta <omaplinuxkernel@gmail.com> writes: > Hi Kevin, > > On Fri, Sep 14, 2012 at 2:15 AM, Kevin Hilman > <khilman@deeprootsystems.com> wrote: >> From: Kevin Hilman <khilman@ti.com> >> >> On some platforms, bootloaders are known to do some interesting RTC >> programming. Without going into the obscurities as to why this may be >> the case, suffice it to say the the driver should not make any >> assumptions about the state of the RTC when the driver loads. In >> particular, the driver probe should be sure that all interrupts are >> disabled until otherwise programmed. >> >> This was discovered when finding bursty I2C traffic every second on >> Overo platforms. This I2C overhead was keeping the SoC from hitting >> deep power states. The cause was found to be the RTC firing every >> second on the I2C-connected TWL PMIC. >> >> Special thanks to Felipe Balbi for suggesting to look for a rogue >> driver as the source of the I2C traffic rather than the I2C driver >> itself. >> >> Special thanks to Steve Sakoman for helping track down the source of >> the continuous RTC interrups on the Overo boards. >> > > Tested that the continuous interrupt issue after doing a i2c mm on omap4sdp. > This patch solves the issue. > thanks, > >> Cc: Felipe Balbi <balbi@ti.com> >> Cc: Steve Sakoman <steve@sakoman.com> >> Signed-off-by: Kevin Hilman <khilman@ti.com> >> --- >> Patch applies to v3.6-rc5 >> >> drivers/rtc/rtc-twl.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c >> index c5d06fe..9277d94 100644 >> --- a/drivers/rtc/rtc-twl.c >> +++ b/drivers/rtc/rtc-twl.c >> @@ -495,6 +495,11 @@ static int __devinit twl_rtc_probe(struct platform_device *pdev) >> if (ret < 0) >> goto out1; >> >> + /* ensure interrupts are disabled, bootloaders can be strange */ >> + ret = twl_rtc_write_u8(0, REG_RTC_INTERRUPTS_REG); >> + if (ret < 0) >> + dev_warn(&pdev->dev, "unable to disable interrupt\n"); >> + > Now that it is always 0 can the below read be removed as it is redundant now. Possibly, but I don't know this HW well enough to know if there are any persistent bits in that register on any of the various derivations of this PMIC. Since this read-back value is used throughout the driver, I decided not to mess with it when doing this targetted fix. Kevin
On Thu, Sep 13, 2012 at 1:45 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > From: Kevin Hilman <khilman@ti.com> > > On some platforms, bootloaders are known to do some interesting RTC > programming. Without going into the obscurities as to why this may be > the case, suffice it to say the the driver should not make any > assumptions about the state of the RTC when the driver loads. In > particular, the driver probe should be sure that all interrupts are > disabled until otherwise programmed. > > This was discovered when finding bursty I2C traffic every second on > Overo platforms. This I2C overhead was keeping the SoC from hitting > deep power states. The cause was found to be the RTC firing every > second on the I2C-connected TWL PMIC. > > Special thanks to Felipe Balbi for suggesting to look for a rogue > driver as the source of the I2C traffic rather than the I2C driver > itself. > > Special thanks to Steve Sakoman for helping track down the source of > the continuous RTC interrups on the Overo boards. > > Cc: Felipe Balbi <balbi@ti.com> > Cc: Steve Sakoman <steve@sakoman.com> > Signed-off-by: Kevin Hilman <khilman@ti.com> Tested on Overo/Tobi, and I no longer see the 1/second interrupts. Also verified that alarm interrupts still work using rtcwake. Tested-by: Steve Sakoman <steve@sakoman.com>
On Fri, Sep 14, 2012 at 7:37 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Shubhrajyoti Datta <omaplinuxkernel@gmail.com> writes: > >> Hi Kevin, >> >> On Fri, Sep 14, 2012 at 2:15 AM, Kevin Hilman >> <khilman@deeprootsystems.com> wrote: >>> From: Kevin Hilman <khilman@ti.com> >>> >>> On some platforms, bootloaders are known to do some interesting RTC >>> programming. Without going into the obscurities as to why this may be >>> the case, suffice it to say the the driver should not make any >>> assumptions about the state of the RTC when the driver loads. In >>> particular, the driver probe should be sure that all interrupts are >>> disabled until otherwise programmed. >>> >>> This was discovered when finding bursty I2C traffic every second on >>> Overo platforms. This I2C overhead was keeping the SoC from hitting >>> deep power states. The cause was found to be the RTC firing every >>> second on the I2C-connected TWL PMIC. >>> >>> Special thanks to Felipe Balbi for suggesting to look for a rogue >>> driver as the source of the I2C traffic rather than the I2C driver >>> itself. >>> >>> Special thanks to Steve Sakoman for helping track down the source of >>> the continuous RTC interrups on the Overo boards. >>> >> >> Tested that the continuous interrupt issue after doing a i2c mm on omap4sdp. >> This patch solves the issue. >> thanks, >> >>> Cc: Felipe Balbi <balbi@ti.com> >>> Cc: Steve Sakoman <steve@sakoman.com> >>> Signed-off-by: Kevin Hilman <khilman@ti.com> >>> --- >>> Patch applies to v3.6-rc5 >>> >>> drivers/rtc/rtc-twl.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c >>> index c5d06fe..9277d94 100644 >>> --- a/drivers/rtc/rtc-twl.c >>> +++ b/drivers/rtc/rtc-twl.c >>> @@ -495,6 +495,11 @@ static int __devinit twl_rtc_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto out1; >>> >>> + /* ensure interrupts are disabled, bootloaders can be strange */ >>> + ret = twl_rtc_write_u8(0, REG_RTC_INTERRUPTS_REG); >>> + if (ret < 0) >>> + dev_warn(&pdev->dev, "unable to disable interrupt\n"); >>> + >> Now that it is always 0 can the below read be removed as it is redundant now. > > Possibly, but I don't know this HW well enough to know if there are any > persistent bits in that register on any of the various derivations of > this PMIC. Since this read-back value is used throughout the driver, I > decided not to mess with it when doing this targetted fix. Indeed makes sense. > > Kevin >
On Fri, 14 Sep 2012 08:33:42 -0700
Steve Sakoman <steve@sakoman.com> wrote:
> Tested-by: Steve Sakoman <steve@sakoman.com>
Thanks.
Given the tested-by's that are rolling in, I will assume that people
are hitting this problem in 3.5 and perhaps earlier kernels, so I
scheduled the fix for 3.6, with a -stable backport.
I might have been wrong about that - in future, please do make these
issues clear in the patch changelog?
On Fri, Sep 14, 2012 at 12:20 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > Given the tested-by's that are rolling in, I will assume that people > are hitting this problem in 3.5 and perhaps earlier kernels, so I > scheduled the fix for 3.6, with a -stable backport. Yes, I just checked an Overo setup here that is running 3.2 and it has the same issue. So your plan for 3.6 and -stable is a good one. Steve
diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c index c5d06fe..9277d94 100644 --- a/drivers/rtc/rtc-twl.c +++ b/drivers/rtc/rtc-twl.c @@ -495,6 +495,11 @@ static int __devinit twl_rtc_probe(struct platform_device *pdev) if (ret < 0) goto out1; + /* ensure interrupts are disabled, bootloaders can be strange */ + ret = twl_rtc_write_u8(0, REG_RTC_INTERRUPTS_REG); + if (ret < 0) + dev_warn(&pdev->dev, "unable to disable interrupt\n"); + /* init cached IRQ enable bits */ ret = twl_rtc_read_u8(&rtc_irq_bits, REG_RTC_INTERRUPTS_REG); if (ret < 0)