Re: [PATCH v2 2/5] drivers/rtc/rtc-armada38x: Add a new RTC driver for recent mvebu SoCs

Hi Arnaud,
[...]

quoted

+/*
+ * According to the datasheet, we need to wait for 5us only between
+ * two consecutive writes
+ */
+static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
+{
+	writel(val, rtc->regs + offset);
+	udelay(5);
+}

The thing I do not get is how you can guarantee that an undelayed
writel() in a function will not be followed less than 5µs later by
another writel() in another function. For instance:

  1) a call to set_alarm() followed by a call to alarm_irq_enable().
  2) some writel() or even rtc_udelayed_write() w/ sth asynchronous
    occuring like your interrupt handler just after that.

I guess it may be possible to make assumptions on the fact that the
amount of code before a writel() or rtc_udelayed_write() may prevent
such scenario but it's difficult to tell, all the more w/ a spinlock
before the writel() in irq_enable().

Considering the description of the bug, why not doing the following:

 1) implement rtc_delayed_write() a bit differently:

    static inline void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
    {
	udelay(5);
	writel(val, rtc->regs + offset);
    }

 2) remove all writel() in the driver and use rtc_delayed_write()
    everywhere.

All writes being under the protection of your spinlock, this will
guarantee the 5µs delay in all cases.

You're right. I tried avoiding loosing microseconds here and there, but there
is too many case where it can fail. So let's us it everywhere.

[...]

quoted

+static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct armada38x_rtc *rtc = dev_get_drvdata(dev);
+	int ret = 0;
+	unsigned long time, flags;
+
+	ret = rtc_tm_to_time(tm, &time);
+
+	if (ret)
+		goto out;
+	/*
+	 * Setting the RTC time not always succeeds. According to the
+	 * errata we need to first write on the status register and
+	 * then wait for 100ms before writing to the time register to be
+	 * sure that the data will be taken into account.
+	 */
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	writel(0, rtc->regs + RTC_STATUS);
+
+	spin_unlock_irqrestore(&rtc->lock, flags);
+
+	msleep(100);
+
+	spin_lock_irqsave(&rtc->lock, flags);
+
+	writel(time, rtc->regs + RTC_TIME);

probably not critical (it's rtc clock and not system clock) but you still
introduce a 100ms shift when setting time here.

I am aware of this shift but the granularity is the second, so we can't do
better, we have to deal with the limitation of the hardware. :(

As for the two writel() not being done w/o releasing the spinlock, it
looks a bit weird but it seems it's ok for other functions manipulating
RTC_STATUS reg. 

Nonetheless, in the reverse direction, if a writel() occurs somewhere
(e.g. in the alarm irq handler) during your msleep(), you may do your
final writel() w/o having enforced the expected 100ms delay.

Actually I don't know if it is problematic if a writel occur in an other register
during the 100ms. I still wait for an answer for it.


Thanks,

Gregory




-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html