Add support for SiFive Premier P550 #190

Ivan-Velickovic · 2025-02-11T05:14:35Z

Draft since I have to write the timer driver. The serial driver is exactly the same as the one in #167.

Ivan-Velickovic · 2025-03-25T06:31:14Z

I believe this is ready for review now.

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>

lsf37

Looks good from my side.

Longer-term we should try to figure out a mechanism where we can test compilation for new platforms before they are merged. For now, I think we still have to merge first and then see if anything breaks once we can add it to CI.

Indanz · 2025-07-08T09:30:27Z

libplatsupport/mach_include/eswin/platsupport/mach/timer.h

+#include <stdint.h>
+#include <stdbool.h>
+
+/* These values correspond to the DTS node 'soc/timer@0x51840000' */


Please add a comment to say what timer peripheral is being implemented, and if there are multiple, which one is used. Both the datasheet names and the compat strings would be handy, but either is fine.

Indanz · 2025-07-08T09:31:28Z

libplatsupport/mach_include/eswin/platsupport/mach/timer.h

+#define ESWIN_TIMER_IRQ 0x159
+
+/* This is 24MHz */
+#define ESWIN_TIMER_TICKS_PER_SECOND 0x16e3600


Then why not write 24000000 or (24 * 1000 * 1000)?

Indanz · 2025-07-08T09:36:35Z

libplatsupport/mach_include/eswin/platsupport/mach/timer.h

+#define ESWIN_TIMER_ENABLED 0x1
+#define ESWIN_TIMER_DISABLED 0x0
+#define ESWIN_TIMER_MODE_FREE_RUNNING 0x0
+#define ESWIN_TIMER_MODE_USER_DEFINED (1 << 1)
+#define ESWIN_TIMER_IRQ_UNMASK 0x0


Please do (0 << n) so you at least document the bit index that defines this. But it's weird to have defines for 0, you can just say in a comment above the init what the result will be. I'd probably make defines for all settings (with comments behind them for what it means when it's not set) and then one define for the chosen cfg.

Indanz · 2025-07-08T10:00:22Z

libplatsupport/src/mach/eswin/timer.c

+    assert(timer);
+    assert(timer->regs);
+
+    timer->regs->ctrl &= 0xfffffffe;


Suggested change

timer->regs->ctrl &= 0xfffffffe;

timer->regs->ctrl &= ~0x1;

Or set it to just 0 and init it on enable.

Indanz · 2025-07-08T10:07:23Z

libplatsupport/src/mach/eswin/timer.c

+    uint64_t value_subsecond_ns =
+        (value_subsecond_ticks * NS_IN_S) / ESWIN_TIMER_TICKS_PER_SECOND;


One line please. If it doesn't fit, consider dropping the value_ prefix.

Doing the calculation this way is more exact compared to what I did for the imx93 timer driver.

Indanz · 2025-07-08T10:40:36Z

libplatsupport/src/mach/eswin/ltimer.c

+                                      handle_irq_wrapper,
+                                      &timers->callback_data);


I really don't understand the point of handle_irq_wrapper, but everyone is using it.

Indanz · 2025-07-08T10:42:49Z

libplatsupport/src/mach/eswin/ltimer.c

+                                      irqs[0],
+                                      handle_irq_wrapper,
+                                      &timers->callback_data);
+    assert(irq_id >= 0);


Also, you need to store irq_id to use it in destroy.

Indanz · 2025-07-08T10:47:39Z

libplatsupport/src/mach/eswin/serial.c

+
+int uart_getchar(ps_chardevice_t *d)
+{
+    while ((*REG_PTR(d->vaddr, UART_LSR) & BIT(0)));


Would be nice to have a define for bit 0, like you did for THRE.

Indanz · 2025-07-08T10:50:53Z

libplatsupport/src/mach/eswin/serial.c

+        uart_putchar(d, '\r');
+    }
+
+    while ((*REG_PTR(d->vaddr, UART_LSR) & UART_LSR_THRE) == 0);


Probably better to check whether the buffer is not full, if there is a queue larger than one byte in the device.

(Ideally printf implementations would wait till the buffer is empty before writing a new line and keep transmitting characters while the buffer has space once they started. That way you can share a UART with multiple users like a Linux VM or other cores pretty well, without it becoming a mess. But oh well.)

The NS16650a/DesignWare APB Uart gateware used by the p550 is terrible and (a) the ns16650a doesn't provide a standard TX FIFO not full bit, so practically no software uses the one in the snps,dw-apb-uart (Linux definitely doesn't); and (b) as far as we can tell the TFNF bit also remains as 0 on the P550: au-ts/sddf#443.

See also the copious documentation I wrote about this: https://github.com/au-ts/sddf/blob/904ef0019d0404cb107dcca355126ae9c20cc0be/drivers/serial/ns16550a/uart.c#L43-L87, especially the linked Ultimate PutChar. Linux et al use this implementation where they check "if empty" then write a full FIFO sized without reading any registers, which has issues with sharing drivers as you mentioned.

Oh well, never mind then.

Reading the linked pdf, you can initially poll TEMT to see whether any data is queued and hence there is printing in progress, and poll THRE before queueing consequent bytes. That would avoid the interleaved output if everyone would do this.

Anyway, with the above scheme THRE is used as buffer not empty already, so the code as it is is fine.

Indanz · 2025-07-08T10:53:47Z

libplatsupport/src/mach/eswin/serial.c

+    dev->ioops      = *ops;
+    dev->flags      = SERIAL_AUTO_CR;
+
+    *REG_PTR(dev->vaddr, 0x8) = 1;


Defines for this would be nice.

Indanz · 2025-07-08T10:55:54Z

I see I have been too slow. Please fix issues in additional PR.

Ivan-Velickovic · 2025-07-10T00:21:23Z

@Indanz thanks for the review, I won't get to it this week probably but I do intend on fixing everything you brought up.

Ivan-Velickovic force-pushed the p550 branch 2 times, most recently from d0dc528 to aacc64b Compare February 17, 2025 13:08

Ivan-Velickovic marked this pull request as ready for review March 25, 2025 06:31

Ivan-Velickovic force-pushed the p550 branch from aacc64b to 073eb13 Compare March 25, 2025 06:31

Ivan-Velickovic added 2 commits April 1, 2025 13:02

libplatsupport: add serial support for HiFive P550

c4a2ee6

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>

libplatsupport: add ltimer for ESWIN

e34070d

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>

Ivan-Velickovic force-pushed the p550 branch from 073eb13 to e34070d Compare April 1, 2025 02:02

Ivan-Velickovic and others added 2 commits April 1, 2025 15:59

libplatsupport: introduce eswin mach

44dec8c

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>

Merge branch 'master' into p550

a90b7f5

lsf37 approved these changes Jul 8, 2025

View reviewed changes

lsf37 merged commit 1ba6640 into seL4:master Jul 8, 2025
23 of 24 checks passed

Indanz reviewed Jul 8, 2025

View reviewed changes

akifejaz mentioned this pull request Nov 3, 2025

Add Support for Banana Pi BPI-F3 #202

Merged

		uint64_t value_subsecond_ns =
		(value_subsecond_ticks * NS_IN_S) / ESWIN_TIMER_TICKS_PER_SECOND;

Add support for SiFive Premier P550 #190

Add support for SiFive Premier P550 #190

Uh oh!

Conversation

Ivan-Velickovic commented Feb 11, 2025

Uh oh!

Ivan-Velickovic commented Mar 25, 2025

Uh oh!

lsf37 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Indanz Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Indanz commented Jul 8, 2025

Uh oh!

Ivan-Velickovic commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Indanz Jul 8, 2025 •

edited

Loading