Messages in this thread | | | Date | Thu, 2 Nov 2017 12:53:59 +0000 | From | James Hogan <> | Subject | Re: [PATCH v6 5/5] MIPS: ranchu: Add Ranchu as a new generic-based board |
| |
On Thu, Nov 02, 2017 at 12:47:27PM +0000, Miodrag Dinic wrote: > > > +static __init uint64_t read_rtc_time(void __iomem *base) > > > +{ > > > + u64 time_low; > > > + u64 time_high; > > > + > > > + time_low = readl(base + GOLDFISH_TIMER_LOW); > > > + time_high = readl(base + GOLDFISH_TIMER_HIGH); > > > + > > > + return (time_high << 32) | time_low; > > > > What if high changes while reading this? > > > > E.g. > > TIMER_LOW 0x00000000 *0xffffffff* > > TIMER_HIGH *0x00000001* 0x00000000 > > > > You'd presumably get 0x00000001ffffffff. > > > > Perhaps it should read HIGH before too, and retry if it has changed. > > This was already discussed in some earlier posts. (https://patchwork.linux-mips.org/patch/16628/) > Reading the low value first actually latches the high value, > so it is safe to leave it this way. Here is the relevant RTC device > implementation in QEMU: > > static uint64_t goldfish_rtc_read(void *opaque, hwaddr offset, unsigned size) > { > struct rtc_state *s = (struct rtc_state *)opaque; > switch(offset) { > case TIMER_TIME_LOW: > s->now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->time_base; > return s->now_ns; > case TIMER_TIME_HIGH: > return s->now_ns >> 32;
Ah okay. In that case the side effect of reading low clearly isn't obvious enough reading the driver code and it needs a comment to clarify the hardware behaviour :-)
Cheers James
| |