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? |
|
|
||
| static void uart_handle_irq(ps_chardevice_t *dev) | ||
| { | ||
| /* This UART driver is not interrupt driven, there is nothing to do here. */ |
There was a problem hiding this comment.
Since the UARTs are used as output only, interrupt support does not matter practically. One day, some generic seL4 console demo (tetris, nethack ...) would be nice to get read interrupts added also...
There was a problem hiding this comment.
IIUC based on other examples, to support this the only thing I need to do is to enable the interrupt generation on the device on init, and ack the interrupt to the device in this function?
There was a problem hiding this comment.
I will add that.
But it's not clear to me how a consumer of this API would effectively deal with interrupts. It looks like they are supposed to register and wait for the interrupt themselves, and then tell the driver when the interrupt occurred, without getting any further information about the interrupt from the driver.
On this device there could be various reasons for the interrupt, if we were to enable interrupts for more than on read, and I don't see a way for a client to determine why it was generated.
The design seems at odds with the timer API, which registers its own IRQ's and generates callbacks.
There was a problem hiding this comment.
If I remember correctly, uart_handle_irq() is just the abstraction function to ack the interrupt. The lib's user must still attach to the interrupt and call the uart_read() wrapper to drain the FIFO first. An existing app that does this should work out of the box on the Pine64 then.
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>
| * 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.
| uint64_t value_h = (uint64_t)timer->value_h; | ||
|
|
||
| /* Include unhandled interrupt in value_h */ | ||
| if (timer->regs->intclr == 1) { |
There was a problem hiding this comment.
How is the register intclr defined, are we really checking the whole register value here, or just the bit 0, because the other bits are reserved? I could not find any document that describes the timer resisters, seem this was never published and this is just taken from some other code?
There was a problem hiding this comment.
I couldn't find any documentation either other than what was with some code. I did test that this was set as shown when there was an unhandled interrupt. I can add the mask to be defensive.
There was a problem hiding this comment.
Well, without documentation, it's a well-educated guess that it's following the usual way, where just bit 0 matters. Bitght be safer to add a comment, that it's unclear how exactly this register works and give the source where this code is copied from.
There was a problem hiding this comment.
To be clear, I didn't copy the source, just used it as documentation.
|
|
||
| /* 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?
|
|
||
| /* 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.
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>
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.