Add pine64 star64 platform #167
Conversation
|
It should be noted that due to there being no current documentation on the JH7110 timer, details such as the register map are taken from the JH7110 timer Linux kernel patch which can be found here: https://patchwork.kernel.org/project/linux-riscv/patch/20230627055313.252519-3-xingyu.wu@starfivetech.com/. |
|
It looks good, could you squash the commits? |
| * Stores the number of times the continuous counter timer has elapsed and started over. | ||
| * This allows us to count to a higher number than allowed by the hardware. | ||
| */ | ||
| uint32_t value_h; |
There was a problem hiding this comment.
Should this be declared volatile also, as it could be access concurrently, and we also want to prevent the compiler from doing some optimization that would move the accesses around.
There was a problem hiding this comment.
I'm confused, these drivers aren't multithreaded right? How could there be concurrent access?
There was a problem hiding this comment.
Driver ware not multi-threaded, but they have an ISR that could interrupt us any time and update this value. Using volatile might not prevent the race completely, but could makes the windows even smaller.
There was a problem hiding this comment.
I don't quite understand why volatile will help here, this is internal state that is just like a normal C variable?
|
|
||
| /* Include unhandled interrupt in value_h */ | ||
| if (timer->regs->intclr == 1) { | ||
| value_h += 1; |
There was a problem hiding this comment.
Should we read timer->regs->value again now, as the interrupt may have arrived after we read it before?
There was a problem hiding this comment.
Isn't that what line 68 is doing? I'm confused
|
|
||
| /* the timer value counts down from the load value */ | ||
| uint64_t value_l = (uint64_t)(STARFIVE_TIMER_MAX_TICKS - timer->regs->value); | ||
| uint64_t value_h = (uint64_t)timer->value_h; |
There was a problem hiding this comment.
Do we have to take into account concurrency around timer->value_h here (in case the ISR updates it while we are here) ? So better read high, lo, and high again. If high it differs, read low again.
There was a problem hiding this comment.
The time is always going to be changing because you have to consume time to read the register so would reading the registers again actually help?
This adds a timer and serial driver to libplatsupport. Co-developed-by: Ivan-Velickovic <i.velickovic@unsw.edu.au> Signed-off-by: Jimmy Brush <code@jimmah.com>
The interrupt is enabled on init. Nothing needs to be done in the IRQ handler as the next read will acknowledge the interrupt to the device. Signed-off-by: Jimmy Brush <code@jimmah.com>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
|
I've tried to address all the comments now, hopefully we can merge this. |
This adds a timer and serial driver to libplatsupport.
Co-developed with @Ivan-Velickovic.
Purpose: Ready to merge.
Context: Needed to allow sel4test to run on star64.
Testing performed: Tested with WIP sel4test for the star64 platform on pinetab-v hardware.