[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

Subsystems: real time clock (rtc) subsystem, the rest

STALE4103d

5 messages, 4 authors, 2015-05-10 · open the first message on its own page

[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

From: Lothar Waßmann <hidden>
Date: 2012-08-18 08:10:08

If snvs_rtc_enable() happens to be descheduled for a period longer
than the timout period, a timeout may be indicated even if the RTC
status has changed meanwhile. Force a status read at the end of the
timeout period to prevent bogus timeout errors.


Signed-off-by: Lothar Wa?mann <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

 Compile tested only due to lack of HW.

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..dc921b2 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(int enable, void __iomem *addr)
+{
+	u32 lpcr = readl(addr + SNVS_LPCR);
+
+	return !!enable ^ !(lpcr & SNVS_LPCR_SRTC_ENV);
+}
+
 static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1);

@@ -99,18 +106,15 @@ static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	while (1) {
-		lpcr = readl(data->ioaddr + SNVS_LPCR);
+		if (snvs_rtc_enable_done(enable, data->ioaddr))
+			break;
 
-		if (enable) {
-			if (lpcr & SNVS_LPCR_SRTC_ENV)
-				break;
-		} else {
-			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+		if (time_after(jiffies, timeout)) {
+			if (snvs_rtc_enable_done(enable, data->ioaddr))
 				break;
+			else
+				return -ETIMEDOUT;
 		}
-
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 	}
 
 	return 0;

-- 
1.7.2.5

[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

From: Shawn Guo <hidden>
Date: 2012-08-18 12:46:54

On Sat, Aug 18, 2012 at 10:10:08AM +0200, Lothar Wa?mann wrote:

If snvs_rtc_enable() happens to be descheduled for a period longer
than the timout period, a timeout may be indicated even if the RTC
status has changed meanwhile. Force a status read at the end of the
timeout period to prevent bogus timeout errors.


Signed-off-by: Lothar Wa?mann <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

 Compile tested only due to lack of HW.

Tested-by: Shawn Guo <redacted>

A small nitpick below.

quoted hunk

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..dc921b2 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(int enable, void __iomem *addr)

Can we rename parameter "addr" to "ioaddr" and put it before "enable",
also change "enable" to bool?

Regards,
Shawn

quoted hunk

+{
+	u32 lpcr = readl(addr + SNVS_LPCR);
+
+	return !!enable ^ !(lpcr & SNVS_LPCR_SRTC_ENV);
+}
+
 static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1);

@@ -99,18 +106,15 @@ static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	while (1) {
-		lpcr = readl(data->ioaddr + SNVS_LPCR);
+		if (snvs_rtc_enable_done(enable, data->ioaddr))
+			break;
 
-		if (enable) {
-			if (lpcr & SNVS_LPCR_SRTC_ENV)
-				break;
-		} else {
-			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+		if (time_after(jiffies, timeout)) {
+			if (snvs_rtc_enable_done(enable, data->ioaddr))
 				break;
+			else
+				return -ETIMEDOUT;
 		}
-
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 	}
 
 	return 0;

-- 
1.7.2.5

[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

From: Lothar Waßmann <hidden>
Date: 2012-08-18 13:14:04

Shawn Guo writes:

On Sat, Aug 18, 2012 at 10:10:08AM +0200, Lothar Wa?mann wrote:

quoted

If snvs_rtc_enable() happens to be descheduled for a period longer
than the timout period, a timeout may be indicated even if the RTC
status has changed meanwhile. Force a status read at the end of the
timeout period to prevent bogus timeout errors.


Signed-off-by: Lothar Wa?mann <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

 Compile tested only due to lack of HW.

Tested-by: Shawn Guo <redacted>

A small nitpick below.

quoted

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..dc921b2 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(int enable, void __iomem *addr)

Can we rename parameter "addr" to "ioaddr" and put it before "enable",
also change "enable" to bool?

OK. The resulting assembler code of the patch below is identical to
the previous version of this patch.


Signed-off-by: Lothar Wa?mann <redacted>
Tested-by: Shawn Guo <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..a757cb9 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(void __iomem *ioaddr, bool enable)
+{
+	u32 lpcr = readl(ioaddr + SNVS_LPCR);
+
+	return enable ^ !(lpcr & SNVS_LPCR_SRTC_ENV);
+}
+
 static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1);

@@ -99,18 +106,15 @@ static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	while (1) {
-		lpcr = readl(data->ioaddr + SNVS_LPCR);
+		if (snvs_rtc_enable_done(data->ioaddr, enable))
+			break;
 
-		if (enable) {
-			if (lpcr & SNVS_LPCR_SRTC_ENV)
-				break;
-		} else {
-			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+		if (time_after(jiffies, timeout)) {
+			if (snvs_rtc_enable_done(data->ioaddr, enable))
 				break;
+			else
+				return -ETIMEDOUT;
 		}
-
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 	}
 
 	return 0;

-- 
1.7.2.5


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

From: akpm@linux-foundation.org (Andrew Morton)
Date: 2012-09-18 23:07:40

On Sat, 18 Aug 2012 10:10:08 +0200
Lothar Wa__mann [off-list ref] wrote:

If snvs_rtc_enable() happens to be descheduled for a period longer
than the timout period, a timeout may be indicated even if the RTC
status has changed meanwhile. Force a status read at the end of the
timeout period to prevent bogus timeout errors.

(delayed, sorry)

quoted hunk

--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(int enable, void __iomem *addr)
+{
+	u32 lpcr = readl(addr + SNVS_LPCR);
+
+	return !!enable ^ !(lpcr & SNVS_LPCR_SRTC_ENV);
+}
+
 static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1);

@@ -99,18 +106,15 @@ static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	while (1) {
-		lpcr = readl(data->ioaddr + SNVS_LPCR);
+		if (snvs_rtc_enable_done(enable, data->ioaddr))
+			break;
 
-		if (enable) {
-			if (lpcr & SNVS_LPCR_SRTC_ENV)
-				break;
-		} else {
-			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+		if (time_after(jiffies, timeout)) {
+			if (snvs_rtc_enable_done(enable, data->ioaddr))
 				break;
+			else
+				return -ETIMEDOUT;
 		}
-
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 	}
 
 	return 0;

I think this will have been addressed by
http://ozlabs.org/~akpm/mmots/broken-out/rtc-snvs-add-freescale-rtc-snvs-driver-fix.patch?

[PATCH] rtc: snvs: improve timeout handling in snvs_rtc_enable()

From: Alexandre Belloni <hidden>
Date: 2015-05-10 21:42:44

Hi Shawn, Lothar,

On 18/08/2012 at 15:14:04 +0200, Lothar Wa?mann wrote :

Shawn Guo writes:

quoted

On Sat, Aug 18, 2012 at 10:10:08AM +0200, Lothar Wa?mann wrote:

quoted

If snvs_rtc_enable() happens to be descheduled for a period longer
than the timout period, a timeout may be indicated even if the RTC
status has changed meanwhile. Force a status read at the end of the
timeout period to prevent bogus timeout errors.


Signed-off-by: Lothar Wa?mann <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

 Compile tested only due to lack of HW.

Tested-by: Shawn Guo <redacted>

It seems that patch never made it mainline. Can you check if it is still
needed?

quoted hunk

quoted

A small nitpick below.

quoted

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..dc921b2 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(int enable, void __iomem *addr)

Can we rename parameter "addr" to "ioaddr" and put it before "enable",
also change "enable" to bool?

OK. The resulting assembler code of the patch below is identical to
the previous version of this patch.


Signed-off-by: Lothar Wa?mann <redacted>
Tested-by: Shawn Guo <redacted>
---
 drivers/rtc/rtc-snvs.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index 912f116..a757cb9 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c

@@ -81,6 +81,13 @@ static void rtc_write_sync_lp(void __iomem *ioaddr)
 	}
 }
 
+static int snvs_rtc_enable_done(void __iomem *ioaddr, bool enable)
+{
+	u32 lpcr = readl(ioaddr + SNVS_LPCR);
+
+	return enable ^ !(lpcr & SNVS_LPCR_SRTC_ENV);
+}
+
 static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1);

@@ -99,18 +106,15 @@ static int snvs_rtc_enable(struct snvs_rtc_data *data, bool enable)
 	spin_unlock_irqrestore(&data->lock, flags);
 
 	while (1) {
-		lpcr = readl(data->ioaddr + SNVS_LPCR);
+		if (snvs_rtc_enable_done(data->ioaddr, enable))
+			break;
 
-		if (enable) {
-			if (lpcr & SNVS_LPCR_SRTC_ENV)
-				break;
-		} else {
-			if (!(lpcr & SNVS_LPCR_SRTC_ENV))
+		if (time_after(jiffies, timeout)) {
+			if (snvs_rtc_enable_done(data->ioaddr, enable))
 				break;
+			else
+				return -ETIMEDOUT;
 		}
-
-		if (time_after(jiffies, timeout))
-			return -ETIMEDOUT;
 	}
 
 	return 0;

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help