[PATCH 0/3] repair RTC subsys (for i.MX)

[PATCH 0/3] repair RTC subsys (for i.MX)

[Wolfram Sang]

The recent updates to the RTC subsystem (removing UIE interrupts and use
alarms instead) introduced two problems for i.MX (and the subsys in
general, I'd think):

a) because registering the rtc now calls get_alarm(), the requirement
has been added for a lot of drivers that drvdata is properly set up
_before_ registering. rtc-mxc did not do that (probably bad; but as said
like a lot of other rtc-drivers currently) and oopsed.

b) the callbacks to the rtc-core for update_irqs have been removed, but
irq-handlers are still there, now being unused cruft. In case of
rtc-mxc, this is only one if-block, but for rtc-mc13xxx.c this is a
seperate handler.

>From a glimpse, most platform drivers seem to have at least one of these
problems now :( John, am I correct or am I missing something?

Kind regards,

   Wolfram

Wolfram Sang (3):
  rtc: mc13xxx: remove UIE signaling
  rtc: mxc: fix crash on boot
  rtc: mxc: remove UIE signaling

 drivers/rtc/rtc-mc13xxx.c |   23 -----------------------
 drivers/rtc/rtc-mxc.c     |   22 +++++++++++-----------
 2 files changed, 11 insertions(+), 34 deletions(-)

-- 
1.7.2.5

[PATCH 1/3] rtc: mc13xxx: remove UIE signaling

[Wolfram Sang]

The RTC core handles it since 6610e08 (RTC: Rework RTC code to use
timerqueue for events). So far, only the callbacks to the RTC core have
been removed, but not the handlers. Do this now.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/rtc/rtc-mc13xxx.c |   23 -----------------------
 1 files changed, 0 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/rtc-mc13xxx.c b/drivers/rtc/rtc-mc13xxx.c
index c5ac037..d4e2078 100644
--- a/drivers/rtc/rtc-mc13xxx.c
+++ b/drivers/rtc/rtc-mc13xxx.c
@@ -268,20 +268,6 @@ static irqreturn_t mc13xxx_rtc_alarm_handler(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t mc13xxx_rtc_update_handler(int irq, void *dev)
-{
-	struct mc13xxx_rtc *priv = dev;
-	struct mc13xxx *mc13xxx = priv->mc13xxx;
-
-	dev_dbg(&priv->rtc->dev, "1HZ\n");
-
-	rtc_update_irq(priv->rtc, 1, RTC_IRQF | RTC_UF);
-
-	mc13xxx_irq_ack(mc13xxx, irq);
-
-	return IRQ_HANDLED;
-}
-
 static int mc13xxx_rtc_alarm_irq_enable(struct device *dev,
 		unsigned int enabled)
 {
@@ -339,11 +325,6 @@ static int __devinit mc13xxx_rtc_probe(struct platform_device *pdev)
 
 	priv->valid = !rtcrst_pending;
 
-	ret = mc13xxx_irq_request_nounmask(mc13xxx, MC13XXX_IRQ_1HZ,
-			mc13xxx_rtc_update_handler, DRIVER_NAME, priv);
-	if (ret)
-		goto err_update_irq_request;
-
 	ret = mc13xxx_irq_request_nounmask(mc13xxx, MC13XXX_IRQ_TODA,
 			mc13xxx_rtc_alarm_handler, DRIVER_NAME, priv);
 	if (ret)
@@ -357,9 +338,6 @@ static int __devinit mc13xxx_rtc_probe(struct platform_device *pdev)
 		mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_TODA, priv);
 err_alarm_irq_request:
 
-		mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_1HZ, priv);
-err_update_irq_request:
-
 err_reset_irq_status:
 
 		mc13xxx_irq_free(mc13xxx, MC13XXX_IRQ_RTCRST, priv);
@@ -383,7 +361,6 @@ static int __devexit mc13xxx_rtc_remove(struct platform_device *pdev)
 	rtc_device_unregister(priv->rtc);
 
 	mc13xxx_irq_free(priv->mc13xxx, MC13XXX_IRQ_TODA, priv);
-	mc13xxx_irq_free(priv->mc13xxx, MC13XXX_IRQ_1HZ, priv);
 	mc13xxx_irq_free(priv->mc13xxx, MC13XXX_IRQ_RTCRST, priv);
 
 	mc13xxx_unlock(priv->mc13xxx);
-- 
1.7.2.5

Re: [1/3] rtc: mc13xxx: remove UIE signaling

[Alexandre Belloni]

On 04/05/2011 at 17:31:26 +0200, Wolfram Sang wrote :
> The RTC core handles it since 6610e08 (RTC: Rework RTC code to use
> timerqueue for events). So far, only the callbacks to the RTC core have
> been removed, but not the handlers. Do this now.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/rtc/rtc-mc13xxx.c |   23 -----------------------
>  1 files changed, 0 insertions(+), 23 deletions(-)
> 

Almost five years ago, you sent that patch...

Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [1/3] rtc: mc13xxx: remove UIE signaling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Wed, Apr 20, 2016 at 04:37:51PM +0200, Alexandre Belloni wrote:
> On 04/05/2011 at 17:31:26 +0200, Wolfram Sang wrote :
> > The RTC core handles it since 6610e08 (RTC: Rework RTC code to use
> > timerqueue for events). So far, only the callbacks to the RTC core have
> > been removed, but not the handlers. Do this now.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > ---
> >  drivers/rtc/rtc-mc13xxx.c |   23 -----------------------
> >  1 files changed, 0 insertions(+), 23 deletions(-)
> > 
> 
> Almost five years ago, you sent that patch...

Wow, I almost forgot about it ;)))

Thanks for picking it up.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] rtc: mxc: fix crash on boot

[Wolfram Sang]

Commit f44f7f96a20 ("RTC: Initialize kernel state from RTC") caused a
crash using the mxc-rtc driver.

The reason is that rtc_device_register() calls rtc_read_alarm() after
that change, when the driver does not have all driver data set up yet.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/rtc/rtc-mxc.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index 826ab64..d814417 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -418,14 +418,6 @@ static int __init mxc_rtc_probe(struct platform_device *pdev)
 		goto exit_put_clk;
 	}
 
-	rtc = rtc_device_register(pdev->name, &pdev->dev, &mxc_rtc_ops,
-				  THIS_MODULE);
-	if (IS_ERR(rtc)) {
-		ret = PTR_ERR(rtc);
-		goto exit_put_clk;
-	}
-
-	pdata->rtc = rtc;
 	platform_set_drvdata(pdev, pdata);
 
 	/* Configure and enable the RTC */
@@ -438,8 +430,19 @@ static int __init mxc_rtc_probe(struct platform_device *pdev)
 		pdata->irq = -1;
 	}
 
+	rtc = rtc_device_register(pdev->name, &pdev->dev, &mxc_rtc_ops,
+				  THIS_MODULE);
+	if (IS_ERR(rtc)) {
+		ret = PTR_ERR(rtc);
+		goto exit_clr_drvdata;
+	}
+
+	pdata->rtc = rtc;
+
 	return 0;
 
+exit_clr_drvdata:
+	platform_set_drvdata(pdev, NULL);
 exit_put_clk:
 	clk_disable(pdata->clk);
 	clk_put(pdata->clk);
-- 
1.7.2.5

[PATCH 3/3] rtc: mxc: remove UIE signaling

[Wolfram Sang]

The RTC core handles it since 6610e08 (RTC: Rework RTC code to use
timerqueue for events).

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/rtc/rtc-mxc.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index d814417..c996218 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -213,9 +213,6 @@ static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id)
 	if (status & RTC_ALM_BIT)
 		events |= (RTC_AF | RTC_IRQF);
 
-	if (status & RTC_1HZ_BIT)
-		events |= (RTC_UF | RTC_IRQF);
-
 	if (status & PIT_ALL_ON)
 		events |= (RTC_PF | RTC_IRQF);
 
-- 
1.7.2.5

Re: [3/3] rtc: mxc: remove UIE signaling

[Alexandre Belloni]

On 04/05/2011 at 17:31:28 +0200, Wolfram Sang wrote :
> The RTC core handles it since 6610e08 (RTC: Rework RTC code to use
> timerqueue for events).
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/rtc/rtc-mxc.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/3] repair RTC subsys (for i.MX)

[john stultz]

On Wed, 2011-05-04 at 17:31 +0200, Wolfram Sang wrote:
> The recent updates to the RTC subsystem (removing UIE interrupts and use
> alarms instead) introduced two problems for i.MX (and the subsys in
> general, I'd think):
> 
> a) because registering the rtc now calls get_alarm(), the requirement
> has been added for a lot of drivers that drvdata is properly set up
> _before_ registering. rtc-mxc did not do that (probably bad; but as said
> like a lot of other rtc-drivers currently) and oopsed.
> 
> b) the callbacks to the rtc-core for update_irqs have been removed, but
> irq-handlers are still there, now being unused cruft. In case of
> rtc-mxc, this is only one if-block, but for rtc-mc13xxx.c this is a
> seperate handler.
> 
> From a glimpse, most platform drivers seem to have at least one of these
> problems now :( John, am I correct or am I missing something?

I've tried to go through and clean up most of the b) issues, although
some have apparently slipped by. Please let me know of any others you
ran across.

And indeed we've hit a few of issue a) already, so I should probably run
through and do a full audit.

Your patches look fine to me. Do you intend to push them or should I
queue them up?

thanks
-john

Re: [rtc-linux] Re: [PATCH 0/3] repair RTC subsys (for i.MX)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

On Wed, May 04, 2011 at 03:30:59PM -0700, john stultz wrote:
> On Wed, 2011-05-04 at 17:31 +0200, Wolfram Sang wrote:
> > The recent updates to the RTC subsystem (removing UIE interrupts and use
> > alarms instead) introduced two problems for i.MX (and the subsys in
> > general, I'd think):
> > 
> > a) because registering the rtc now calls get_alarm(), the requirement
> > has been added for a lot of drivers that drvdata is properly set up
> > _before_ registering. rtc-mxc did not do that (probably bad; but as said
> > like a lot of other rtc-drivers currently) and oopsed.
> > 
> > b) the callbacks to the rtc-core for update_irqs have been removed, but
> > irq-handlers are still there, now being unused cruft. In case of
> > rtc-mxc, this is only one if-block, but for rtc-mc13xxx.c this is a
> > seperate handler.
> > 
> > From a glimpse, most platform drivers seem to have at least one of these
> > problems now :( John, am I correct or am I missing something?
> 
> I've tried to go through and clean up most of the b) issues, although
> some have apparently slipped by. Please let me know of any others you
> ran across.

I discovered it when working on the rtc-stmp3xxx driver. For this, I
will send a patch series in a few minutes, because there are more things
to be done there. Grepping for 'RTC_UF' should point to other areas of
interest.

> And indeed we've hit a few of issue a) already, so I should probably run
> through and do a full audit.

I hacked a q'n'd coccinelle-script pointing out potential candidates. It
found two more for which I will send patches, too. But I think a
seperate audit would be a good idea, just to be sure.

> Your patches look fine to me. Do you intend to push them or should I
> queue them up?

I'd say they should go via your tree.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]