[PATCH 0/3] rtc: pcf8563: Fix unhandled interrupt storm

[PATCH 0/3] rtc: pcf8563: Fix unhandled interrupt storm

[Chen-Yu Tsai]

From: Chen-Yu Tsai <wens@csie.org>

Hi everyone,

While bringing up my Pine H64, I encountered an interrupt storm from the
pcf8563 RTC. The RTC chip's interrupt line is shared with the PMIC, and
was not properly added to the device tree. Also, the driver was using an
trigger method incompatible with the PMIC, preventing the interrupt line
from being shared. Last, the driver only clears and masks the alarm
interrupt, while leaving the timer interrupt untouched. This is a
problem if previous systems left the timer interrupt enabled, and there
was an interrupt pending.

This patch set fixes all three issues, one per patch.

Please have a look.

Chen-Yu Tsai (3):
  rtc: pcf8563: Fix interrupt trigger method
  rtc: pcf8563: Clear event flags and disable interrupts before
    requesting irq
  arm64: dts: allwinner: h6: Pine H64: Add interrupt line for RTC

 .../arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts |  2 ++
 drivers/rtc/rtc-pcf8563.c                           | 13 ++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH 1/3] rtc: pcf8563: Fix interrupt trigger method

[Chen-Yu Tsai]

From: Chen-Yu Tsai <wens@csie.org>

The PCF8563 datasheet says the interrupt line is active low and stays
active until the events are cleared, i.e. a level trigger interrupt.

Fix the flags used to request the interrupt.

Fixes: ede3e9d47cca ("drivers/rtc/rtc-pcf8563.c: add alarm support")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---

Not sure if this would cause issues for other platforms. Ideally we'd
take the flags from the device tree, but it seems not all platforms
support this.

---
 drivers/rtc/rtc-pcf8563.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 3efc86c25d27..e358313466f1 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -605,7 +605,7 @@ static int pcf8563_probe(struct i2c_client *client,
 	if (client->irq > 0) {
 		err = devm_request_threaded_irq(&client->dev, client->irq,
 				NULL, pcf8563_irq,
-				IRQF_SHARED|IRQF_ONESHOT|IRQF_TRIGGER_FALLING,
+				IRQF_SHARED | IRQF_ONESHOT | IRQF_TRIGGER_LOW,
 				pcf8563_driver.driver.name, client);
 		if (err) {
 			dev_err(&client->dev, "unable to request IRQ %d\n",
-- 
2.20.1

Re: [PATCH 1/3] rtc: pcf8563: Fix interrupt trigger method

[Alexandre Belloni]

On 04/06/2019 12:23:35+0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
> 
> The PCF8563 datasheet says the interrupt line is active low and stays
> active until the events are cleared, i.e. a level trigger interrupt.
> 
> Fix the flags used to request the interrupt.
> 
> Fixes: ede3e9d47cca ("drivers/rtc/rtc-pcf8563.c: add alarm support")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> 
> Not sure if this would cause issues for other platforms. Ideally we'd
> take the flags from the device tree, but it seems not all platforms
> support this.
> 
> ---
>  drivers/rtc/rtc-pcf8563.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 2/3] rtc: pcf8563: Clear event flags and disable interrupts before requesting irq

[Chen-Yu Tsai]

From: Chen-Yu Tsai <wens@csie.org>

Besides the alarm, the PCF8563 also has a timer triggered interrupt.
In cases where the previous system left the timer and interrupts on,
or somehow the bits got enabled, the interrupt would keep triggering
as the kernel doesn't know about it.

Clear both the alarm and timer event flags, and disable the interrupts,
before requesting the interrupt line.

Fixes: ede3e9d47cca ("drivers/rtc/rtc-pcf8563.c: add alarm support")
Fixes: a45d528aab8b ("rtc: pcf8563: clear expired alarm at boot time")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/rtc/rtc-pcf8563.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index e358313466f1..d8adf69b6697 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -563,7 +563,6 @@ static int pcf8563_probe(struct i2c_client *client,
 	struct pcf8563 *pcf8563;
 	int err;
 	unsigned char buf;
-	unsigned char alm_pending;
 
 	dev_dbg(&client->dev, "%s\n", __func__);
 
@@ -587,13 +586,13 @@ static int pcf8563_probe(struct i2c_client *client,
 		return err;
 	}
 
-	err = pcf8563_get_alarm_mode(client, NULL, &alm_pending);
-	if (err) {
-		dev_err(&client->dev, "%s: read error\n", __func__);
+	/* Clear flags and disable interrupts */
+	buf = 0;
+	err = pcf8563_write_block_data(client, PCF8563_REG_ST2, 1, &buf);
+	if (err < 0) {
+		dev_err(&client->dev, "%s: write error\n", __func__);
 		return err;
 	}
-	if (alm_pending)
-		pcf8563_set_alarm_mode(client, 0);
 
 	pcf8563->rtc = devm_rtc_device_register(&client->dev,
 				pcf8563_driver.driver.name,
-- 
2.20.1

Re: [PATCH 2/3] rtc: pcf8563: Clear event flags and disable interrupts before requesting irq

[Alexandre Belloni]

On 04/06/2019 12:23:36+0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
> 
> Besides the alarm, the PCF8563 also has a timer triggered interrupt.
> In cases where the previous system left the timer and interrupts on,
> or somehow the bits got enabled, the interrupt would keep triggering
> as the kernel doesn't know about it.
> 
> Clear both the alarm and timer event flags, and disable the interrupts,
> before requesting the interrupt line.
> 
> Fixes: ede3e9d47cca ("drivers/rtc/rtc-pcf8563.c: add alarm support")
> Fixes: a45d528aab8b ("rtc: pcf8563: clear expired alarm at boot time")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/rtc/rtc-pcf8563.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 3/3] arm64: dts: allwinner: h6: Pine H64: Add interrupt line for RTC

[Chen-Yu Tsai]

From: Chen-Yu Tsai <wens@csie.org>

The external PCF8563 RTC chip's interrupt line is connected to the NMI
line on the SoC.

Add the interrupt line to the device tree.

Fixes: 17ebc33afc35 ("arm64: allwinner: h6: add PCF8563 RTC on Pine H64 board")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
index 9e464d40cbff..189834518391 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
@@ -249,6 +249,8 @@
 	pcf8563: rtc@51 {
 		compatible = "nxp,pcf8563";
 		reg = <0x51>;
+		interrupt-parent = <&r_intc>;
+		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
 		#clock-cells = <0>;
 	};
 };
-- 
2.20.1

Re: [PATCH 0/3] rtc: pcf8563: Fix unhandled interrupt storm

[Alexandre Belloni]

On 04/06/2019 12:23:34+0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
> 
> Hi everyone,
> 
> While bringing up my Pine H64, I encountered an interrupt storm from the
> pcf8563 RTC. The RTC chip's interrupt line is shared with the PMIC, and
> was not properly added to the device tree. Also, the driver was using an
> trigger method incompatible with the PMIC, preventing the interrupt line
> from being shared. Last, the driver only clears and masks the alarm
> interrupt, while leaving the timer interrupt untouched. This is a
> problem if previous systems left the timer interrupt enabled, and there
> was an interrupt pending.
> 
> This patch set fixes all three issues, one per patch.
> 
> Please have a look.
> 

I don't have that particular RTC so I can't test but the interrupt
handling in pcf8563_irq seems problematic too. I guess the RTC will only
trigger once per second because the call to pcf8563_set_alarm_mode will
explicitely leave the alarm enabled. The core doesn't really care but it
doesn't really expect the alarm to stay enabled. i.e. It will ensure the
alarm is enabled again after setting it when necessary. I think it would
be safer to simply clear both AIE and AF here. Could you test?

> Chen-Yu Tsai (3):
>   rtc: pcf8563: Fix interrupt trigger method
>   rtc: pcf8563: Clear event flags and disable interrupts before
>     requesting irq
>   arm64: dts: allwinner: h6: Pine H64: Add interrupt line for RTC
> 
>  .../arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts |  2 ++
>  drivers/rtc/rtc-pcf8563.c                           | 13 ++++++-------
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] rtc: pcf8563: Fix unhandled interrupt storm

[Chen-Yu Tsai]

On Fri, Jun 21, 2019 at 12:22 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 04/06/2019 12:23:34+0800, Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > Hi everyone,
> >
> > While bringing up my Pine H64, I encountered an interrupt storm from the
> > pcf8563 RTC. The RTC chip's interrupt line is shared with the PMIC, and
> > was not properly added to the device tree. Also, the driver was using an
> > trigger method incompatible with the PMIC, preventing the interrupt line
> > from being shared. Last, the driver only clears and masks the alarm
> > interrupt, while leaving the timer interrupt untouched. This is a
> > problem if previous systems left the timer interrupt enabled, and there
> > was an interrupt pending.
> >
> > This patch set fixes all three issues, one per patch.
> >
> > Please have a look.
> >
>
> I don't have that particular RTC so I can't test but the interrupt
> handling in pcf8563_irq seems problematic too. I guess the RTC will only
> trigger once per second because the call to pcf8563_set_alarm_mode will
> explicitely leave the alarm enabled. The core doesn't really care but it
> doesn't really expect the alarm to stay enabled. i.e. It will ensure the
> alarm is enabled again after setting it when necessary. I think it would
> be safer to simply clear both AIE and AF here. Could you test?

Yeah, that bit looked weird to me as well. And actually the alarm doesn't
go down to the second, only the minute.

Is there a test program I can use to test the alarms?

Thanks
ChenYu

> > Chen-Yu Tsai (3):
> >   rtc: pcf8563: Fix interrupt trigger method
> >   rtc: pcf8563: Clear event flags and disable interrupts before
> >     requesting irq
> >   arm64: dts: allwinner: h6: Pine H64: Add interrupt line for RTC
> >
> >  .../arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts |  2 ++
> >  drivers/rtc/rtc-pcf8563.c                           | 13 ++++++-------
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > --
> > 2.20.1
> >
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH 0/3] rtc: pcf8563: Fix unhandled interrupt storm

[Alexandre Belloni]

On 24/06/2019 18:34:29+0800, Chen-Yu Tsai wrote:
> On Fri, Jun 21, 2019 at 12:22 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > On 04/06/2019 12:23:34+0800, Chen-Yu Tsai wrote:
> > > From: Chen-Yu Tsai <wens@csie.org>
> > >
> > > Hi everyone,
> > >
> > > While bringing up my Pine H64, I encountered an interrupt storm from the
> > > pcf8563 RTC. The RTC chip's interrupt line is shared with the PMIC, and
> > > was not properly added to the device tree. Also, the driver was using an
> > > trigger method incompatible with the PMIC, preventing the interrupt line
> > > from being shared. Last, the driver only clears and masks the alarm
> > > interrupt, while leaving the timer interrupt untouched. This is a
> > > problem if previous systems left the timer interrupt enabled, and there
> > > was an interrupt pending.
> > >
> > > This patch set fixes all three issues, one per patch.
> > >
> > > Please have a look.
> > >
> >
> > I don't have that particular RTC so I can't test but the interrupt
> > handling in pcf8563_irq seems problematic too. I guess the RTC will only
> > trigger once per second because the call to pcf8563_set_alarm_mode will
> > explicitely leave the alarm enabled. The core doesn't really care but it
> > doesn't really expect the alarm to stay enabled. i.e. It will ensure the
> > alarm is enabled again after setting it when necessary. I think it would
> > be safer to simply clear both AIE and AF here. Could you test?
> 
> Yeah, that bit looked weird to me as well. And actually the alarm doesn't
> go down to the second, only the minute.
> 
> Is there a test program I can use to test the alarms?
> 

Sure, tools/testing/selftests/rtc/rtctest.c if you use a recent enough
version, it will test minute boundaries.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com