[PATCH 1/2] rtc: armada38x: drop redundant initialization

[PATCH 1/2] rtc: armada38x: drop redundant initialization

[Baruch Siach]

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

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/rtc/rtc-armada38x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 1e4978c96ffd..4d62a54fd5d6 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -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);
 
-- 
2.17.1

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

[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.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 4d62a54fd5d6..0b50276dce6d 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -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);
-- 
2.17.1

Re: [PATCH 1/2] rtc: armada38x: drop redundant initialization

[Alexandre Belloni]

On 21/06/2018 20:40:22+0300, Baruch Siach wrote:
> The 'ret' variable in these functions is set unconditionally below.
> 

Hum, what is the actual benefit of this patch?

> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/rtc/rtc-armada38x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> index 1e4978c96ffd..4d62a54fd5d6 100644
> --- a/drivers/rtc/rtc-armada38x.c
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -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);
>  
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: armada38x: drop redundant initialization

[Baruch Siach]

Hi Alexandre,

On Thu, Jun 21, 2018 at 07:59:39PM +0200, Alexandre Belloni wrote:
> On 21/06/2018 20:40:22+0300, Baruch Siach wrote:
> > 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

> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  drivers/rtc/rtc-armada38x.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> > index 1e4978c96ffd..4d62a54fd5d6 100644
> > --- a/drivers/rtc/rtc-armada38x.c
> > +++ b/drivers/rtc/rtc-armada38x.c
> > @@ -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);
> >  
> > -- 
> > 2.17.1

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH 2/2] rtc: armada38x: reset after rtc power loss

[Gregory CLEMENT]

Hi Baruch,
 
 On jeu., juin 21 2018, Baruch Siach <baruch@tkos.co.il> wrote:

> 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 <gregory.clement@bootlin.com>

Thanks,

Gregory


>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
> index 4d62a54fd5d6..0b50276dce6d 100644
> --- a/drivers/rtc/rtc-armada38x.c
> +++ b/drivers/rtc/rtc-armada38x.c
> @@ -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);
> -- 
> 2.17.1
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 2/2] rtc: armada38x: reset after rtc power loss

[Alexandre Belloni]

On 21/06/2018 20:40:23+0300, Baruch Siach wrote:
> 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 <baruch@tkos.co.il>
> ---
>  drivers/rtc/rtc-armada38x.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com