Reading 64 bit MMIO

[thejpster]

I think there's an error in reading the 64 bit value in delays::get_system_timer. I would expect to read hi, read lo, read hi again and if different (because lo has wrapped since our first read) to read lo again (which can't now wrap because it is near zero).

In your code, if lo wraps after the if but before the read of lo, you get a bogus result.

[andre-richter]

This seems to be an interesting case.

For this code:

// Since it is MMIO, we must emit two separate 32 bit reads
let mut hi = self.SYSTMR_HI.get();
let mut lo = self.SYSTMR_LO.get();

// We have to repeat if high word changed during read.
if hi != self.SYSTMR_HI.get() {
    hi = self.SYSTMR_HI.get();
    lo = self.SYSTMR_LO.get();
}

I get this clippy hint:

Checking kernel8 v0.1.0 (file:///home/arichter/repos/rust-raspi3-tutorial/09_delays)
warning: `if _ { .. } else { .. }` is an expression
  --> src/delays.rs:72:9
   |
72 | /         let mut lo = self.SYSTMR_LO.get();
73 | |
74 | |         // We have to repeat if high word changed during read.
75 | |         if hi != self.SYSTMR_HI.get() {
76 | |             hi = self.SYSTMR_HI.get();
77 | |             lo = self.SYSTMR_LO.get();
78 | |         }
   | |_________^ help: it is more idiomatic to write: `let <mut> lo = if hi != self.SYSTMR_HI.get() { ..; self.SYSTMR_LO.get() } else { self.SYSTMR_LO.get() };`
   |
   = note: #[warn(useless_let_if_seq)] on by default
   = note: you might not need `mut` at all
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#useless_let_if_seq

If you blindly follow that hint, like I did without thinking apparently, you lose the first MMIO read of lo.
Clippy would need to understand that these are volatile operations to not emit the hint.

Do I make sense?

[andre-richter]

Filed an issue to get feedback from clippy people: rust-lang/rust-clippy#3043

[toothbrush7777777]

@thejpster, @andre-richter

Despite the Clippy lint being incorrect, reading the registers twice is not necessary.

3.1.2 64-bit timer read/write

The ARM has 32-bit registers and the main timer has a 64-bits wide changing value. There is the potential for reading the wrong value if the timer value changes between reading the LS and MS 32 bits. Therefore it has dedicated logic to give trouble free access to the 64-bit value.
When reading the 64-bit timer value the user must always read the LS 32 bits first. At the same time the LS-32 bits are read, internal a copy is made of the MS-32 bits into a timer-read-hold register. When reading the timer MS-32 bits actually the timer-read-hold register is read.
When writing the 64-bit timer value the user must always write the LS 32 bits first. The LS 32-bits are stored in a timer-write-hold register. When the user writes the MS 32-bit, the stored LS bits are written as well.
The timer pre-scaler register is set to zero when the timer value (MS) is written.

— BCM2836 ARM-local peripherals

[andre-richter]

Ah, nice catch, thanks.

Will rectify it.

[andre-richter]

Upon further review, I am not sure if your quoted documentation applies to the counter that is handled by the code here.
Unfortunately, the bad state of Raspi SoC documentation is not of help here, as so often.

The first source of uncertainty is that the QA sheet you quote is for the BCM2836, which is the 32-bit predecessor of the Raspi 3's 64-bit BCM2837. While we can assume that SoC peripherals stayed the same mostly while the processor was exchanged, we still cannot be 100% sure.

The second issue the makes me wonder is that in the QA sheet, Section 3.1.1 talks about a pre-scaler,
while Section 3.1.2 talks about 64-bit timer reads using the two 32-bit registers. This doesn't match with other documentation that is available.

In the BCM2387 reference I use, Section 12 - System Timer describes the 32-bit timer registers the tutorial code reads. However, no mention of a pre-scaler.

A pre-scaler in timing context is only mentioned in Section 14 - Timer (ARM side), which describes yet another timer peripheral. But here, we are not having two 32-bit HI and LO registers, but a single 32-bit counter register.

All in all, very confusing and I tend to not change the code for now until I find further evidence about the Raspi 3's System Timer having the timer-read-hold functionality.

Perhaps looking at Raspi3-related reference code that can be attributed to ARM employees in Linux or any other projects that use this timer will shed some light on it.

[xihan94]

ARMv8 supports reading core timer values architecturally. Maybe try reading CNTPCT_EL0?

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500d/ch10s03s01.html

[andre-richter]

The tutorial shows both ways for learning purposes: CNTPCT et al. registers and the MMIO timer.

[xihan94]

Ah. I didn’t read the code below. Sorry.

[andre-richter]

Closing this for now because the failing pattern is not present in the rewrite anymore.
May reopen when it needs be used again and this is still an issue.