[PATCH 1/2] rtc: zynqmp: re-use rtc_time64_to_tm operation

[PATCH 1/2] rtc: zynqmp: re-use rtc_time64_to_tm operation

[Jean-Francois Dagenais]

This allows a subsequent commit to spin_unlock sooner.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 drivers/rtc/rtc-zynqmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 2c762757fb54..cb78900ec1f5 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -94,7 +94,7 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		 * RTC has updated the CURRENT_TIME with the time written into
 		 * SET_TIME_WRITE register.
 		 */
-		rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
+		read_time = readl(xrtcdev->reg_base + RTC_CUR_TM);
 	} else {
 		/*
 		 * Time written in SET_TIME_WRITE has not yet updated into
@@ -104,8 +104,8 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		 * reading.
 		 */
 		read_time = readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1;
-		rtc_time64_to_tm(read_time, tm);
 	}
+	rtc_time64_to_tm(read_time, tm);
 
 	return 0;
 }
-- 
2.23.0

[PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Jean-Francois Dagenais]

When reaching the read_time implementation before 1 second has elapsed
since power on, SEC bit from status register is not yet set. read_time
incorrectly assumes that there is a pending set_time not yet reflected
in CURRENT_TIME and proceeds to use SET_TIME_READ (RTC_SET_TM_RD in the
code). This register contains garbage at this moment and this is
returned as the current time.

Here we switch to using the SEC interrupt to signal the pending set time
operation. The old ISR is renamed xlnx_rtc_alarm_interrupt and a new one
xlnx_rtc_sec_interrupt is created to handle disabling the SEC interrupt
once the pending write is known to be complete.

read_time now detects whether a pending write exists or not using the
interrupt mask (!interrupt enabled). The interrupt is only enabled when
actually performing a set_time.

Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Tested-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com>
---
 drivers/rtc/rtc-zynqmp.c | 69 +++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index cb78900ec1f5..9fec89c4f573 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/rtc.h>
+#include <linux/spinlock.h>
 
 /* RTC Registers */
 #define RTC_SET_TM_WR		0x00
@@ -45,6 +46,7 @@ struct xlnx_rtc_dev {
 	int			alarm_irq;
 	int			sec_irq;
 	int			calibval;
+	spinlock_t		lock;
 };
 
 static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
@@ -59,6 +61,8 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	 */
 	new_time = rtc_tm_to_time64(tm) + 1;
 
+	spin_lock_irq(&xrtcdev->lock);
+
 	/*
 	 * Writing into calibration register will clear the Tick Counter and
 	 * force the next second to be signaled exactly in 1 second period
@@ -68,32 +72,32 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 	writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
 
-	/*
-	 * Clear the rtc interrupt status register after setting the
-	 * time. During a read_time function, the code should read the
-	 * RTC_INT_STATUS register and if bit 0 is still 0, it means
-	 * that one second has not elapsed yet since RTC was set and
-	 * the current time should be read from SET_TIME_READ register;
-	 * otherwise, CURRENT_TIME register is read to report the time
-	 */
+	/* Clear sec interrupt status */
 	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_STS);
 
+	/* Now enable sec interrupt. This ensures our isr will get called, but
+	 * also signals the read_time function that a SET time is pending, in
+	 * which case time should be read from SET_TIME_READ instead of
+	 * CURRENT_TIME registers.
+	 */
+	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
+
+	spin_unlock_irq(&xrtcdev->lock);
+
 	return 0;
 }
 
 static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
-	u32 status;
+	u32 int_mask;
 	unsigned long read_time;
 	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
 
-	status = readl(xrtcdev->reg_base + RTC_INT_STS);
+	spin_lock_irq(&xrtcdev->lock);
+	int_mask = readl(xrtcdev->reg_base + RTC_INT_MASK);
 
-	if (status & RTC_INT_SEC) {
-		/*
-		 * RTC has updated the CURRENT_TIME with the time written into
-		 * SET_TIME_WRITE register.
-		 */
+	if (int_mask & RTC_INT_SEC) {
+		/* No set_time pending, use the CURRENT_TIME register */
 		read_time = readl(xrtcdev->reg_base + RTC_CUR_TM);
 	} else {
 		/*
@@ -105,6 +109,9 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		 */
 		read_time = readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1;
 	}
+
+	spin_unlock_irq(&xrtcdev->lock);
+
 	rtc_time64_to_tm(read_time, tm);
 
 	return 0;
@@ -173,14 +180,36 @@ static const struct rtc_class_ops xlnx_rtc_ops = {
 	.alarm_irq_enable = xlnx_rtc_alarm_irq_enable,
 };
 
-static irqreturn_t xlnx_rtc_interrupt(int irq, void *id)
+static irqreturn_t xlnx_rtc_sec_interrupt(int irq, void *id)
+{
+	struct xlnx_rtc_dev *xrtcdev = (struct xlnx_rtc_dev *)id;
+	unsigned int status;
+
+	status = readl(xrtcdev->reg_base + RTC_INT_STS);
+	/* Check if interrupt asserted */
+	if (!(status & RTC_INT_SEC))
+		return IRQ_NONE;
+
+	/* Clear RTC_INT_SEC interrupt only */
+	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_STS);
+
+	spin_lock(&xrtcdev->lock);
+
+	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_DIS);
+
+	spin_unlock(&xrtcdev->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t xlnx_rtc_alarm_interrupt(int irq, void *id)
 {
 	struct xlnx_rtc_dev *xrtcdev = (struct xlnx_rtc_dev *)id;
 	unsigned int status;
 
 	status = readl(xrtcdev->reg_base + RTC_INT_STS);
 	/* Check if interrupt asserted */
-	if (!(status & (RTC_INT_SEC | RTC_INT_ALRM)))
+	if (!(status & RTC_INT_ALRM))
 		return IRQ_NONE;
 
 	/* Clear RTC_INT_ALRM interrupt only */
@@ -202,6 +231,8 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
 	if (!xrtcdev)
 		return -ENOMEM;
 
+	spin_lock_init(&xrtcdev->lock);
+
 	platform_set_drvdata(pdev, xrtcdev);
 
 	xrtcdev->rtc = devm_rtc_allocate_device(&pdev->dev);
@@ -221,7 +252,7 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
 	if (xrtcdev->alarm_irq < 0)
 		return xrtcdev->alarm_irq;
 	ret = devm_request_irq(&pdev->dev, xrtcdev->alarm_irq,
-			       xlnx_rtc_interrupt, 0,
+			       xlnx_rtc_alarm_interrupt, 0,
 			       dev_name(&pdev->dev), xrtcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "request irq failed\n");
@@ -232,7 +263,7 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
 	if (xrtcdev->sec_irq < 0)
 		return xrtcdev->sec_irq;
 	ret = devm_request_irq(&pdev->dev, xrtcdev->sec_irq,
-			       xlnx_rtc_interrupt, 0,
+			       xlnx_rtc_sec_interrupt, 0,
 			       dev_name(&pdev->dev), xrtcdev);
 	if (ret) {
 		dev_err(&pdev->dev, "request irq failed\n");
-- 
2.23.0

Re: [PATCH 1/2] rtc: zynqmp: re-use rtc_time64_to_tm operation

[Michal Simek]

On 28. 11. 19 2:56, Jean-Francois Dagenais wrote:
> This allows a subsequent commit to spin_unlock sooner.
> 
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
>   drivers/rtc/rtc-zynqmp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> index 2c762757fb54..cb78900ec1f5 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -94,7 +94,7 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   		 * RTC has updated the CURRENT_TIME with the time written into
>   		 * SET_TIME_WRITE register.
>   		 */
> -		rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> +		read_time = readl(xrtcdev->reg_base + RTC_CUR_TM);
>   	} else {
>   		/*
>   		 * Time written in SET_TIME_WRITE has not yet updated into
> @@ -104,8 +104,8 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   		 * reading.
>   		 */
>   		read_time = readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1;
> -		rtc_time64_to_tm(read_time, tm);
>   	}
> +	rtc_time64_to_tm(read_time, tm);
>   
>   	return 0;
>   }
> 

Reviewed-by: Michal Simek <michal.simek@xilinx.com>

M

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Michal Simek]

On 28. 11. 19 2:56, Jean-Francois Dagenais wrote:
> When reaching the read_time implementation before 1 second has elapsed
> since power on, SEC bit from status register is not yet set. read_time
> incorrectly assumes that there is a pending set_time not yet reflected
> in CURRENT_TIME and proceeds to use SET_TIME_READ (RTC_SET_TM_RD in the
> code). This register contains garbage at this moment and this is
> returned as the current time.

How did you test this?

> 
> Here we switch to using the SEC interrupt to signal the pending set time
> operation. The old ISR is renamed xlnx_rtc_alarm_interrupt and a new one
> xlnx_rtc_sec_interrupt is created to handle disabling the SEC interrupt
> once the pending write is known to be complete.
> 
> read_time now detects whether a pending write exists or not using the
> interrupt mask (!interrupt enabled). The interrupt is only enabled when
> actually performing a set_time.
> 
> Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> Tested-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com>
> ---
>   drivers/rtc/rtc-zynqmp.c | 69 +++++++++++++++++++++++++++++-----------
>   1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
> index cb78900ec1f5..9fec89c4f573 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -13,6 +13,7 @@
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
>   #include <linux/rtc.h>
> +#include <linux/spinlock.h>
>   
>   /* RTC Registers */
>   #define RTC_SET_TM_WR		0x00
> @@ -45,6 +46,7 @@ struct xlnx_rtc_dev {
>   	int			alarm_irq;
>   	int			sec_irq;
>   	int			calibval;
> +	spinlock_t		lock;
>   };
>   
>   static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
> @@ -59,6 +61,8 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>   	 */
>   	new_time = rtc_tm_to_time64(tm) + 1;
>   
> +	spin_lock_irq(&xrtcdev->lock);
> +
>   	/*
>   	 * Writing into calibration register will clear the Tick Counter and
>   	 * force the next second to be signaled exactly in 1 second period
> @@ -68,32 +72,32 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>   
>   	writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
>   
> -	/*
> -	 * Clear the rtc interrupt status register after setting the
> -	 * time. During a read_time function, the code should read the
> -	 * RTC_INT_STATUS register and if bit 0 is still 0, it means
> -	 * that one second has not elapsed yet since RTC was set and
> -	 * the current time should be read from SET_TIME_READ register;
> -	 * otherwise, CURRENT_TIME register is read to report the time
> -	 */
> +	/* Clear sec interrupt status */
>   	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_STS);
>   
> +	/* Now enable sec interrupt. This ensures our isr will get called, but
> +	 * also signals the read_time function that a SET time is pending, in
> +	 * which case time should be read from SET_TIME_READ instead of
> +	 * CURRENT_TIME registers.
> +	 */
> +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
> +
> +	spin_unlock_irq(&xrtcdev->lock);
> +
>   	return 0;
>   }
>   
>   static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   {
> -	u32 status;
> +	u32 int_mask;
>   	unsigned long read_time;
>   	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>   
> -	status = readl(xrtcdev->reg_base + RTC_INT_STS);
> +	spin_lock_irq(&xrtcdev->lock);
> +	int_mask = readl(xrtcdev->reg_base + RTC_INT_MASK);
>   
> -	if (status & RTC_INT_SEC) {
> -		/*
> -		 * RTC has updated the CURRENT_TIME with the time written into
> -		 * SET_TIME_WRITE register.
> -		 */
> +	if (int_mask & RTC_INT_SEC) {
> +		/* No set_time pending, use the CURRENT_TIME register */
>   		read_time = readl(xrtcdev->reg_base + RTC_CUR_TM);
>   	} else {
>   		/*
> @@ -105,6 +109,9 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   		 */
>   		read_time = readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1;
>   	}
> +
> +	spin_unlock_irq(&xrtcdev->lock);
> +
>   	rtc_time64_to_tm(read_time, tm);
>   
>   	return 0;
> @@ -173,14 +180,36 @@ static const struct rtc_class_ops xlnx_rtc_ops = {
>   	.alarm_irq_enable = xlnx_rtc_alarm_irq_enable,
>   };
>   
> -static irqreturn_t xlnx_rtc_interrupt(int irq, void *id)
> +static irqreturn_t xlnx_rtc_sec_interrupt(int irq, void *id)
> +{
> +	struct xlnx_rtc_dev *xrtcdev = (struct xlnx_rtc_dev *)id;
> +	unsigned int status;
> +
> +	status = readl(xrtcdev->reg_base + RTC_INT_STS);
> +	/* Check if interrupt asserted */
> +	if (!(status & RTC_INT_SEC))
> +		return IRQ_NONE;
> +
> +	/* Clear RTC_INT_SEC interrupt only */
> +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_STS);
> +
> +	spin_lock(&xrtcdev->lock);
> +
> +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_DIS);
> +
> +	spin_unlock(&xrtcdev->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t xlnx_rtc_alarm_interrupt(int irq, void *id)
>   {
>   	struct xlnx_rtc_dev *xrtcdev = (struct xlnx_rtc_dev *)id;
>   	unsigned int status;
>   
>   	status = readl(xrtcdev->reg_base + RTC_INT_STS);
>   	/* Check if interrupt asserted */
> -	if (!(status & (RTC_INT_SEC | RTC_INT_ALRM)))
> +	if (!(status & RTC_INT_ALRM))
>   		return IRQ_NONE;
>   
>   	/* Clear RTC_INT_ALRM interrupt only */
> @@ -202,6 +231,8 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
>   	if (!xrtcdev)
>   		return -ENOMEM;
>   
> +	spin_lock_init(&xrtcdev->lock);
> +
>   	platform_set_drvdata(pdev, xrtcdev);
>   
>   	xrtcdev->rtc = devm_rtc_allocate_device(&pdev->dev);
> @@ -221,7 +252,7 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
>   	if (xrtcdev->alarm_irq < 0)
>   		return xrtcdev->alarm_irq;
>   	ret = devm_request_irq(&pdev->dev, xrtcdev->alarm_irq,
> -			       xlnx_rtc_interrupt, 0,
> +			       xlnx_rtc_alarm_interrupt, 0,
>   			       dev_name(&pdev->dev), xrtcdev);
>   	if (ret) {
>   		dev_err(&pdev->dev, "request irq failed\n");
> @@ -232,7 +263,7 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
>   	if (xrtcdev->sec_irq < 0)
>   		return xrtcdev->sec_irq;
>   	ret = devm_request_irq(&pdev->dev, xrtcdev->sec_irq,
> -			       xlnx_rtc_interrupt, 0,
> +			       xlnx_rtc_sec_interrupt, 0,
>   			       dev_name(&pdev->dev), xrtcdev);
>   	if (ret) {
>   		dev_err(&pdev->dev, "request irq failed\n");
> 

Srinivas: can you please take a look at this patch?

Thanks,
Michal

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Jean-Francois Dagenais]

Hi Michal, all,

> On Nov 28, 2019, at 03:19, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> in CURRENT_TIME and proceeds to use SET_TIME_READ (RTC_SET_TM_RD in the
>> code). This register contains garbage at this moment and this is
>> returned as the current time.
> 
> How did you test this?

Remember the linux trampoline code... jumping from FSBL to the kernel without
u-boot? Well, got it working and it speeds our boot substantially. This means we
now reach rtc-zynqmp.c's probe within one second of power on. When we boot the
board without a psbatt, and no internet connection (which means
systemd-timesyncd cannot fix the bunkers date), we were ending up with a date in
the very distant future (>50 years into the future). Aside from the date in our
UI, all our SSL connections were failing because of certificate dates. This got
our attention.

We drilled down quite a lot to find the real root cause. We used JTAG with xsct
with our board simply powered on but not booted (bootselect SD without an SD
card). This means no FSBL/psu_init code has run.

Exhibit A:

xsct% rrd rtc
     set_time_write                  set_time_read: 00000000
        calib_write                     calib_read: 00000000
       current_time: 00000000                alarm: 00000000
     rtc_int_status:       00         rtc_int_mask:       03
         rtc_int_en                    rtc_int_dis
         addr_error:       00  addr_error_int_mask:       01
  addr_error_int_en             addr_error_int_dis
            control: 01000000           safety_chk: 00000000


Then we enable the RTC:

xsct% rwr rtc control 0x81000000

The counter now counts, set_time_read and calib_read are garbage:
xsct% rrd rtc
     set_time_write                  set_time_read: fffe6bff
        calib_write                     calib_read: 000f7fff
       current_time: 00000002                alarm: 00000000
     rtc_int_status:       01         rtc_int_mask:       03
         rtc_int_en                    rtc_int_dis
         addr_error:       00  addr_error_int_mask:       01
  addr_error_int_en             addr_error_int_dis
            control: 81000000           safety_chk: 00000000
xsct% rrd rtc
     set_time_write                  set_time_read: fffe6bff
        calib_write                     calib_read: 000f7fff
       current_time: 00000005                alarm: 00000000
     rtc_int_status:       01         rtc_int_mask:       03
         rtc_int_en                    rtc_int_dis
         addr_error:       00  addr_error_int_mask:       01
  addr_error_int_en             addr_error_int_dis
            control: 81000000           safety_chk: 00000000
xsct% rrd rtc
     set_time_write                  set_time_read: fffe6bff
        calib_write                     calib_read: 000f7fff
       current_time: 00000008                alarm: 00000000
     rtc_int_status:       01         rtc_int_mask:       03
         rtc_int_en                    rtc_int_dis
         addr_error:       00  addr_error_int_mask:       01
  addr_error_int_en             addr_error_int_dis
            control: 81000000           safety_chk: 00000000

We tested further, inserted a psbatt, enabled the RTC, but never touched
set_time_write. Powered our board on and off. The observations is that that
set_time_read and calib_read registers are not initialized to 0x0 upon power on.
Once set though, they keep their values as long as the system is powered on or
psbatt keeps the RTC alive. This is a contradiction of the register POR values
specification.

So for rtc-zynqmp.c, it means that using the set_time_read register is a bad
idea unless the code can definitely say that the set_time_write has been written
to. And this is exactly what our patch does, by use of the seconds interrupt
being enabled which signals the read_time() function that there is a pending
write, and therefore the set_time_read can reliably be read.

Hope this helps!

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Alexandre Belloni]

On 28/11/2019 11:04:19-0500, Jean-Francois Dagenais wrote:
> Hi Michal, all,
> 
> > On Nov 28, 2019, at 03:19, Michal Simek <michal.simek@xilinx.com> wrote:
> > 
> >> in CURRENT_TIME and proceeds to use SET_TIME_READ (RTC_SET_TM_RD in the
> >> code). This register contains garbage at this moment and this is
> >> returned as the current time.
> > 
> > How did you test this?
> 
> Remember the linux trampoline code... jumping from FSBL to the kernel without
> u-boot? Well, got it working and it speeds our boot substantially. This means we
> now reach rtc-zynqmp.c's probe within one second of power on. When we boot the
> board without a psbatt, and no internet connection (which means
> systemd-timesyncd cannot fix the bunkers date), we were ending up with a date in
> the very distant future (>50 years into the future). Aside from the date in our
> UI, all our SSL connections were failing because of certificate dates. This got
> our attention.
> 
> We drilled down quite a lot to find the real root cause. We used JTAG with xsct
> with our board simply powered on but not booted (bootselect SD without an SD
> card). This means no FSBL/psu_init code has run.
> 
> Exhibit A:
> 
> xsct% rrd rtc
>      set_time_write                  set_time_read: 00000000
>         calib_write                     calib_read: 00000000
>        current_time: 00000000                alarm: 00000000
>      rtc_int_status:       00         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 01000000           safety_chk: 00000000
> 
> 
> Then we enable the RTC:
> 
> xsct% rwr rtc control 0x81000000
> 

Doesn't that enable battery switchover instead of simply enabling the
rtc, I though you didn't have a battery.

Or does that mean that your previous read of control returning bit 24
set is also bogus?

I ask because the simpler solution would simply to return -EINVAL in
xlnx_rtc_read_time when you detect a power on condition.

> The counter now counts, set_time_read and calib_read are garbage:
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000002                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000005                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> xsct% rrd rtc
>      set_time_write                  set_time_read: fffe6bff
>         calib_write                     calib_read: 000f7fff
>        current_time: 00000008                alarm: 00000000
>      rtc_int_status:       01         rtc_int_mask:       03
>          rtc_int_en                    rtc_int_dis
>          addr_error:       00  addr_error_int_mask:       01
>   addr_error_int_en             addr_error_int_dis
>             control: 81000000           safety_chk: 00000000
> 
> We tested further, inserted a psbatt, enabled the RTC, but never touched
> set_time_write. Powered our board on and off. The observations is that that
> set_time_read and calib_read registers are not initialized to 0x0 upon power on.
> Once set though, they keep their values as long as the system is powered on or
> psbatt keeps the RTC alive. This is a contradiction of the register POR values
> specification.
> 
> So for rtc-zynqmp.c, it means that using the set_time_read register is a bad
> idea unless the code can definitely say that the set_time_write has been written
> to. And this is exactly what our patch does, by use of the seconds interrupt
> being enabled which signals the read_time() function that there is a pending
> write, and therefore the set_time_read can reliably be read.
> 
> Hope this helps!
> 

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

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Jean-Francois Dagenais]

Bonjour Alexandre!

> On Nov 28, 2019, at 12:29, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
>> Then we enable the RTC:
>> 
>> xsct% rwr rtc control 0x81000000
>> 
> 
> Doesn't that enable battery switchover instead of simply enabling the
> rtc, I though you didn't have a battery.

In our tests, psbatt present only means that the RTC preserves it's state and
registers when the platform is powered off. Before toggling bit 31, the counter
(current_time) doesn't move despite the platform being on (psbatt present or
not). This indicates that effectively, the bit turns on the RTC.

> 
> Or does that mean that your previous read of control returning bit 24
> set is also bogus?

https://www.xilinx.com/html_docs/registers/ug1087/rtc___control.html#
Bits 30:28 are reserved, and not 0x0 as this reference page suggests, so who knows
what the bits mean... well someone at Xilinx knows!

> 
> I ask because the simpler solution would simply to return -EINVAL in
> xlnx_rtc_read_time when you detect a power on condition.

That could also work, but how would we determine that? Oh, perhaps by looking at
the Battery_Enable (31) bit. But then we would need to keep it disabled during
probe so that the time_read could test the bit to see if current_time is valid,
and then after this, when would we enable it? Perhaps at the first call to
set_time_write? This is all interesting but for now we have run out of time to
investigate further. Unless someone comes up with a ready made alternative
solution, we here are actually going to go into production with our patch. It
works well.

Cheers!

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Alexandre Belloni]

On 29/11/2019 09:45:39-0500, Jean-Francois Dagenais wrote:
> > Or does that mean that your previous read of control returning bit 24
> > set is also bogus?
> 
> https://www.xilinx.com/html_docs/registers/ug1087/rtc___control.html#
> Bits 30:28 are reserved, and not 0x0 as this reference page suggests, so who knows
> what the bits mean... well someone at Xilinx knows!
> 

According to this link, bit 24 is Crystal Enable which I would think
would also enable the RTC.

> > 
> > I ask because the simpler solution would simply to return -EINVAL in
> > xlnx_rtc_read_time when you detect a power on condition.
> 
> That could also work, but how would we determine that? Oh, perhaps by looking at
> the Battery_Enable (31) bit. But then we would need to keep it disabled during
> probe so that the time_read could test the bit to see if current_time is valid,
> and then after this, when would we enable it? Perhaps at the first call to
> set_time_write? This is all interesting but for now we have run out of time to
> investigate further. Unless someone comes up with a ready made alternative
> solution, we here are actually going to go into production with our patch. It
> works well.

Yes, this is exactly what should be done. leave the RTC in PON state
until userspace sets the time. Until then, -EINVAL must be returned by
.read_time.

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

Re: [PATCH 2/2] rtc: zynqmp: fix invalid read_time before 1 second

[Jean-Francois Dagenais]

> On Nov 29, 2019, at 10:14, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
>> Bits 30:28 are reserved, and not 0x0 as this reference page suggests, so who knows
>> what the bits mean... well someone at Xilinx knows!
>> 
> 
> According to this link, bit 24 is Crystal Enable which I would think
> would also enable the RTC.

Yes, my bad.

> 
>>> 
>>> I ask because the simpler solution would simply to return -EINVAL in
>>> xlnx_rtc_read_time when you detect a power on condition.
>> 
>> That could also work, but how would we determine that? Oh, perhaps by looking at
>> the Battery_Enable (31) bit. But then we would need to keep it disabled during
>> probe so that the time_read could test the bit to see if current_time is valid,
>> and then after this, when would we enable it? Perhaps at the first call to
>> set_time_write? This is all interesting but for now we have run out of time to
>> investigate further. Unless someone comes up with a ready made alternative
>> solution, we here are actually going to go into production with our patch. It
>> works well.
> 
> Yes, this is exactly what should be done. leave the RTC in PON state
> until userspace sets the time. Until then, -EINVAL must be returned by
> .read_time.

Mathieu Gallichand pointed out, from ug1085:

IMPORTANT: The control register must be programmed every time the LPD is powered
on. Otherwise, the value returned by reading the control register can be
different from the actual control settings stored in the BPD.

So according to this, reading the control register to get bit 31 status is not a
recommended way to determine whether current_time is valid or not.

Re: [PATCH 1/2] rtc: zynqmp: re-use rtc_time64_to_tm operation

[Alexandre Belloni]

On 27/11/2019 20:56:12-0500, Jean-Francois Dagenais wrote:
> This allows a subsequent commit to spin_unlock sooner.
> 
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
Applied, thanks.

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