Skip to contentSkip to conversation

rtc: armada38x: drop redundant initialization

PATCH New Baruch Siach 3 participants 6 messages Jun 21, 2018 – Jul 12, 2018

Subsystems (2)
Real time clock (RTC) Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support
lore
Share & tools
lore

Baruch Siach

author
#0 lore raw

PATCH 1/2 rtc: armada38x: drop redundant initialization

The 'ret' variable in these functions is set unconditionally below.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/rtc/rtc-armada38x.c +2 -2
index 1e4978c96ffd..4d62a54fd5d6 100644
@@ -229,7 +229,7 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	int ret = 0;
+	int ret;
 	unsigned long time, flags;
 
 	ret = rtc_tm_to_time(tm, &time);

@@ -272,7 +272,7 @@ static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u32 reg = ALARM_REG(RTC_ALARM1, rtc->data->alarm);
 	u32 reg_irq = ALARM_REG(RTC_IRQ1_CONF, rtc->data->alarm);
 	unsigned long time, flags;
-	int ret = 0;
+	int ret;
 
 	ret = rtc_tm_to_time(&alrm->time, &time);
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 2/2 rtc: armada38x: reset after rtc power loss

When the RTC block looses power it needs a reset sequence to make it usable again. Otherwise, writes to the time register have no effect.

This reset sequence combines information from the mvebu_rtc driver in the Marvell provided U-Boot, and the SolidRun provided U-Boot repo.

Tested on the Armada 388 based SolidRun Clearfog Base.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
drivers/rtc/rtc-armada38x.c +23 -0
index 4d62a54fd5d6..0b50276dce6d 100644
@@ -30,6 +30,8 @@
 #define RTC_IRQ_FREQ_1HZ	    BIT(2)
 #define RTC_CCR		    0x18
 #define RTC_CCR_MODE		    BIT(15)
+#define RTC_CONF_TEST	    0x1C
+#define RTC_NOMINAL_TIMING	    BIT(13)
 
 #define RTC_TIME	    0xC
 #define RTC_ALARM1	    0x10

@@ -75,6 +77,7 @@ struct armada38x_rtc {
 	void __iomem	    *regs_soc;
 	spinlock_t	    lock;
 	int		    irq;
+	bool		    initialized;
 	struct value_to_freq *val_to_freq;
 	struct armada38x_rtc_data *data;
 };

@@ -226,6 +229,23 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static void armada38x_rtc_reset(struct armada38x_rtc *rtc)
+{
+	u32 reg;
+
+	reg = rtc->data->read_rtc_reg(rtc, RTC_CONF_TEST);
+	/* If bits [7:0] are non-zero, assume RTC was uninitialized */
+	if (reg & 0xff) {
+		rtc_delayed_write(0, rtc, RTC_CONF_TEST);
+		msleep(500); /* Oscillator startup time */
+		rtc_delayed_write(0, rtc, RTC_TIME);
+		rtc_delayed_write(SOC_RTC_ALARM1 | SOC_RTC_ALARM2, rtc,
+				  RTC_STATUS);
+		rtc_delayed_write(RTC_NOMINAL_TIMING, rtc, RTC_CCR);
+	}
+	rtc->initialized = true;
+}
+
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);

@@ -237,6 +257,9 @@ static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	if (ret)
 		goto out;
 
+	if (!rtc->initialized)
+		armada38x_rtc_reset(rtc);
+
 	spin_lock_irqsave(&rtc->lock, flags);
 	rtc_delayed_write(time, rtc, RTC_TIME);
 	spin_unlock_irqrestore(&rtc->lock, flags);
Signature · 1 line
2.17.1
maintainer
#2 lore raw

PATCH 2/2 rtc: armada38x: reset after rtc power loss

Hi Baruch,

Quoted 7 lines · Baruch Siach

When the RTC block looses power it needs a reset sequence to make it usable again. Otherwise, writes to the time register have no effect.

This reset sequence combines information from the mvebu_rtc driver in the Marvell provided U-Boot, and the SolidRun provided U-Boot repo.

Tested on the Armada 388 based SolidRun Clearfog Base.

It also matches my notes to reset the RTC on Armada 8K:
devmem2 0xf428401c w 0
devmem2 0xf4284018 w 0x2000

Acked-by Gregory CLEMENT <[email protected]>

Thanks,

Gregory

Quoted 64 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
drivers/rtc/rtc-armada38x.c +23 -0
index 4d62a54fd5d6..0b50276dce6d 100644
@@ -30,6 +30,8 @@
 #define RTC_IRQ_FREQ_1HZ	    BIT(2)
 #define RTC_CCR		    0x18
 #define RTC_CCR_MODE		    BIT(15)
+#define RTC_CONF_TEST	    0x1C
+#define RTC_NOMINAL_TIMING	    BIT(13)
 
 #define RTC_TIME	    0xC
 #define RTC_ALARM1	    0x10

@@ -75,6 +77,7 @@ struct armada38x_rtc {
 	void __iomem	    *regs_soc;
 	spinlock_t	    lock;
 	int		    irq;
+	bool		    initialized;
 	struct value_to_freq *val_to_freq;
 	struct armada38x_rtc_data *data;
 };

@@ -226,6 +229,23 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static void armada38x_rtc_reset(struct armada38x_rtc *rtc)
+{
+	u32 reg;
+
+	reg = rtc->data->read_rtc_reg(rtc, RTC_CONF_TEST);
+	/* If bits [7:0] are non-zero, assume RTC was uninitialized */
+	if (reg & 0xff) {
+		rtc_delayed_write(0, rtc, RTC_CONF_TEST);
+		msleep(500); /* Oscillator startup time */
+		rtc_delayed_write(0, rtc, RTC_TIME);
+		rtc_delayed_write(SOC_RTC_ALARM1 | SOC_RTC_ALARM2, rtc,
+				  RTC_STATUS);
+		rtc_delayed_write(RTC_NOMINAL_TIMING, rtc, RTC_CCR);
+	}
+	rtc->initialized = true;
+}
+
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);

@@ -237,6 +257,9 @@ static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	if (ret)
 		goto out;
 
+	if (!rtc->initialized)
+		armada38x_rtc_reset(rtc);
+
 	spin_lock_irqsave(&rtc->lock, flags);
 	rtc_delayed_write(time, rtc, RTC_TIME);
 	spin_unlock_irqrestore(&rtc->lock, flags);
Signature · 1 line
2.17.1
Signature · 3 lines
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
maintainer Applied
#3 lore raw

PATCH 2/2 rtc: armada38x: reset after rtc power loss

In reply to Baruch Siach
Quoted 6 lines · Baruch Siach

When the RTC block looses power it needs a reset sequence to make it usable again. Otherwise, writes to the time register have no effect.

This reset sequence combines information from the mvebu_rtc driver in the Marvell provided U-Boot, and the SolidRun provided U-Boot repo.

Quoted 7 lines

Tested on the Armada 388 based SolidRun Clearfog Base.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Applied, thanks.

Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer
#4 lore raw

PATCH 1/2 rtc: armada38x: drop redundant initialization

In reply to Baruch Siach
Quoted 2 lines · Baruch Siach

The 'ret' variable in these functions is set unconditionally below.

Hum, what is the actual benefit of this patch?

Quoted 30 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/rtc/rtc-armada38x.c +2 -2
index 1e4978c96ffd..4d62a54fd5d6 100644
@@ -229,7 +229,7 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	int ret = 0;
+	int ret;
 	unsigned long time, flags;
 
 	ret = rtc_tm_to_time(tm, &time);

@@ -272,7 +272,7 @@ static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u32 reg = ALARM_REG(RTC_ALARM1, rtc->data->alarm);
 	u32 reg_irq = ALARM_REG(RTC_IRQ1_CONF, rtc->data->alarm);
 	unsigned long time, flags;
-	int ret = 0;
+	int ret;
 
 	ret = rtc_tm_to_time(&alrm->time, &time);
Signature · 1 line
2.17.1
Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author
#5 lore raw

Hi Alexandre,

Quoted 4 lines · Alexandre Belloni
Quoted 1 line · Baruch Siach

The 'ret' variable in these functions is set unconditionally below.

Hum, what is the actual benefit of this patch?

It is just a small cleanup to reduces somewhat the strain on the reader of the code. ret is not different from other variables in the same functions that are only initialized when needed.

baruch

Quoted 29 lines
Quoted 29 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/rtc/rtc-armada38x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/rtc/rtc-armada38x.c +2 -2
index 1e4978c96ffd..4d62a54fd5d6 100644
@@ -229,7 +229,7 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
-	int ret = 0;
+	int ret;
 	unsigned long time, flags;
 
 	ret = rtc_tm_to_time(tm, &time);

@@ -272,7 +272,7 @@ static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u32 reg = ALARM_REG(RTC_ALARM1, rtc->data->alarm);
 	u32 reg_irq = ALARM_REG(RTC_IRQ1_CONF, rtc->data->alarm);
 	unsigned long time, flags;
-	int ret = 0;
+	int ret;
 
 	ret = rtc_tm_to_time(&alrm->time, &time);
Signature · 1 line
2.17.1
Signature · 3 lines
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -