-
Notifications
You must be signed in to change notification settings - Fork 156
cortex_m: Enable fpu on startup #749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
28ca874
2472054
05f1849
d5126e1
05ee0ad
516a9aa
a2dfbaf
3559290
e73819a
d1d115d
dd877fe
07e7e53
f82fc72
88e9574
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,8 +33,25 @@ pub const InterruptOptions = microzig.utilities.GenerateInterruptOptions(&.{ | |
| .{ .InterruptEnum = Interrupt, .HandlerFn = Handler }, | ||
| }); | ||
|
|
||
| /// Allowable `platform` options for microzig.options. | ||
| pub const CPU_Options = core.CPU_Options; | ||
| /// Allowable `cpu` options for microzig.options. | ||
| pub const CPU_Options = struct { | ||
| /// If true, interrupt vectors are moved to RAM so handlers can be set at runtime. | ||
| /// | ||
| /// NOTE: Not supported on cortex_m0. | ||
| ram_vector_table: bool = false, | ||
|
|
||
| /// If true, the Cortex-M interrupts will be initialized with a more verbose variant | ||
| /// of the interrupt handlers which print the interrupt name. | ||
| /// | ||
| /// NOTE: This option is enabled in debug builds by default. | ||
| verbose_unhandled_irq: bool = (builtin.mode == .Debug), | ||
|
|
||
| /// If true, the FPU will be enabled in the startup code. | ||
| /// | ||
| /// NOTE: This option is enabled by default if hard float is enabled. | ||
| /// NOTE: Not supported on cortex_m0, cortex_m0plus and cortex_m3. | ||
| enable_fpu: bool = builtin.abi.float() == .hard, | ||
| }; | ||
|
|
||
| /// External Interrupts | ||
| /// These are the interrupts generated by the NVIC. | ||
|
|
@@ -643,6 +660,32 @@ pub const atomic = struct { | |
| } | ||
| }; | ||
|
|
||
| /// Enables the FPU. | ||
| /// | ||
| /// NOTE: This function is automatically called on cpu startup if the cpu has | ||
| /// an fpu and hard float is enabled. HAL's also call this in the startup of | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit HALs |
||
| /// other cores. | ||
| pub inline fn enable_fpu() void { | ||
| switch (cortex_m) { | ||
| inline .cortex_m0, | ||
| .cortex_m0plus, | ||
| .cortex_m3, | ||
| => |flavour| @compileError("FPU not supported on " ++ @tagName(flavour)), | ||
| else => {}, | ||
| } | ||
|
|
||
| // Taken from the rust crate cortex-m-rt. | ||
| asm volatile ( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented this using raw assembly since we might be missing some register definitions for FPU in some cortex-m variants and also it allows being embedded in naked functions.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no clobbers? I get there's a dsb and isb, but couldn't the compile still use cached registers, especially since this is inline? |
||
| \\ldr r0, =0xE000ED88 | ||
| \\ldr r1, =(0b1111 << 20) | ||
| \\ldr r2, [r0] | ||
| \\orr r2, r2, r1 | ||
| \\str r2, [r0] | ||
| \\dsb | ||
| \\isb | ||
| ); | ||
| } | ||
|
|
||
| /// The RAM vector table used. You can swap interrupt handlers at runtime here. | ||
| /// Available when using a RAM vector table or a RAM image. | ||
| pub var ram_vector_table: VectorTable align(256) = if (using_ram_vector_table or is_ram_image) | ||
|
|
@@ -654,7 +697,7 @@ else | |
| pub const startup_logic = struct { | ||
| extern fn microzig_main() noreturn; | ||
|
|
||
| pub fn ram_image_start() linksection("microzig_ram_start") callconv(.naked) void { | ||
| pub fn ram_image_start() linksection("microzig_ram_start") callconv(.naked) noreturn { | ||
| const eos = comptime microzig.utilities.get_end_of_stack(); | ||
| asm volatile ( | ||
| \\ | ||
|
|
@@ -672,6 +715,11 @@ pub const startup_logic = struct { | |
| microzig.utilities.initialize_system_memories(.auto); | ||
|
|
||
| if (using_ram_vector_table or is_ram_image) { | ||
| switch (cortex_m) { | ||
| .cortex_m0 => @compileError("RAM image and RAM vector table are not supported on cortex_m0"), | ||
| else => {}, | ||
| } | ||
|
|
||
| asm volatile ( | ||
| \\ | ||
| // Set VTOR to point to ram table | ||
|
|
@@ -684,6 +732,25 @@ pub const startup_logic = struct { | |
| : .{ .memory = true, .r0 = true, .r1 = true }); | ||
| } | ||
|
|
||
| if (fpu_present and microzig.options.cpu.enable_fpu) { | ||
| enable_fpu(); | ||
tact1m4n3 marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that this change makes microzig have more "batteries included" by default, but let's allow the user to disable the FPU at build time, with a flag named
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add this but isn't it a footgun? You could achieve the same by switching to soft float in the rp2350_arm target in build.zig and you don't risk crashing.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the option |
||
| } else if (!fpu_present and microzig.options.cpu.enable_fpu) { | ||
| @compileError( | ||
| \\FPU enable requested though the chip doesn't appear to have an | ||
| \\FPU. If your chip does have an FPU please add the `fpu_present` | ||
| \\equal to `true` property to your chip file, either manually or via | ||
| \\patches. If you want to use patches, you can use something like | ||
| \\this: | ||
| \\``` | ||
| \\.{ .set_device_property = .{ | ||
| \\ .device_name = "CHIP_NAME", | ||
| \\ .key = "fpu_present", | ||
| \\ .value = "true" | ||
| \\} }, | ||
| \\``` | ||
| ); | ||
| } | ||
|
|
||
| if (@hasField(types.peripherals.SystemControlBlock, "SHCSR")) { | ||
| // Enable distinction between MemFault, BusFault and UsageFault: | ||
| peripherals.scb.SHCSR.modify(.{ | ||
|
|
@@ -694,7 +761,9 @@ pub const startup_logic = struct { | |
| enable_fault_irq(); | ||
| } | ||
|
|
||
| microzig_main(); | ||
| // If the compiler gets too aggressive with inlining we might get some | ||
| // floating point operations before the FPU is enabled. | ||
| @call(.never_inline, microzig_main, .{}); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NP: instead can we use the noinline from the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imo it's ok that calls inside microzig_main get inlined because the FPU is already enabled when execution gets there.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would bubble up the hf store to the startup function and you end up with the same issue IIUC.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't understand what you are referring to. Can you elaborate?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry I think I was not clear. I though of flagging the microzig_main as noinline for every CPU not only arm. Just to simplify this @call other arch could suffer the same issue. It might also deserve a small explanation why we don't want microzig_main not to be inlined ? Feel free to discard this comment if you don't think it worth the effort.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely worth a comment to explain.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How can this be achieved? Idk of a way to make calls to microzig_main never inline (since it's always referenced as extern). But I will mark this PR as draft for now. I want to do a bit of refactoring.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the comment. Is it ok?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great comment, I was unsure why this was an issue but that makes sense to me! |
||
| } | ||
|
|
||
| // Validate that the VectorTable type has all the fault handlers that the CPU expects | ||
|
|
@@ -970,10 +1039,17 @@ const scb_base = scs_base + core.scb_base_offset; | |
| const mpu_base = scs_base + 0x0D90; | ||
| const fpu_base = scs_base + 0x0F34; | ||
|
|
||
| const properties = microzig.chip.properties; | ||
| // TODO: will have to standardize this with regz code generation | ||
| const mpu_present = @hasDecl(properties, "cpu.mpuPresent") and std.mem.eql(u8, properties.@"cpu.mpuPresent", "true"); | ||
| const fpu_present = @hasDecl(properties, "cpu.fpuPresent") and std.mem.eql(u8, properties.@"cpu.fpuPresent", "true"); | ||
| const mpu_present = @hasDecl(microzig.chip, "properties") and | ||
| @hasDecl(microzig.chip.properties, "mpu_present") and | ||
| is_property_true(microzig.chip.properties.mpu_present); | ||
| const fpu_present = @hasDecl(microzig.chip, "properties") and | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes code generated by regz doesn't include a properties declaration => if you used in the past fpu_present or mpu_present you got a compilation error. |
||
| @hasDecl(microzig.chip.properties, "fpu_present") and | ||
| is_property_true(microzig.chip.properties.fpu_present); | ||
|
|
||
| fn is_property_true(value: []const u8) bool { | ||
| return std.mem.eql(u8, value, "true") or std.mem.eql(u8, value, "1"); | ||
| } | ||
|
|
||
| const core = blk: { | ||
| break :blk switch (cortex_m) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made common CPU variant between cortex m flavours to be easier to add options. I also added compilation errors if for instance you try to use a ram vector table on cortex m0 or if you try to enable the FPU on the cortex ms that don't support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think of this