feat: add nxp mcxn947 port (and minor fix to derive) #845
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Lint Results
Found 52 issues on changed lines in 9 files:
- port/nxp/mcx/build.zig: 5 issues
- port/nxp/mcx/src/boards/frdm_mcxn947.zig: 2 issues
- port/nxp/mcx/src/mcxn947/hal/flexcomm.zig: 3 issues
- port/nxp/mcx/src/mcxn947/hal/flexcomm/LPI2c.zig: 20 issues
- port/nxp/mcx/src/mcxn947/hal/flexcomm/LPUart.zig: 15 issues
- port/nxp/mcx/src/mcxn947/hal/gpio.zig: 1 issue
- port/nxp/mcx/src/mcxn947/hal/pin.zig: 2 issues
- port/nxp/mcx/src/mcxn947/hal/port.zig: 2 issues
- port/nxp/mcx/src/mcxn947/hal/syscon.zig: 2 issues
ℹ️ Additional issues on unchanged lines
The following 2 issue(s) exist but are not on lines changed in this PR:
build-internals/build.zig:115: Suggestion: Rename `Cpu` to `CPU`, it _should_ be more in line with our [style guidelines](https://microzig.tech/docs/contributing/). This automation is not perfect so take it with a grain of salt.
core/src/mmio.zig:14: Suggestion: Rename `IntT` to `Int_T`, it _should_ be more in line with our [style guidelines](https://microzig.tech/docs/contributing/). This automation is not perfect so take it with a grain of salt.
| .abi = .eabi | ||
| }, | ||
| .chip = .{ | ||
| // TODO: handle other core |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| .name = "MCXN947_cm33_core0", | ||
| .register_definition = .{ .svd = mcux_soc_svd.path("MCXN947/MCXN947_cm33_core0.xml") }, | ||
| .memory_regions = &.{ | ||
| // TODO: not sure about the accesses |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| .register_definition = .{ .svd = mcux_soc_svd.path("MCXN947/MCXN947_cm33_core0.xml") }, | ||
| .memory_regions = &.{ | ||
| // TODO: not sure about the accesses | ||
| // TODO: ROM |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| .memory_regions = &.{ | ||
| // TODO: not sure about the accesses | ||
| // TODO: ROM | ||
| // TODO: secure vs non-secure |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| // .{ .tag = .ram, .offset = 0x13000000, .length = 256 * 1024, .access = .r, .name = "ROM" }, | ||
| } | ||
| }, | ||
| // TODO: not need that ? |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| /// Locking a pin prevent its config to be changed until the next system reset. | ||
| /// | ||
| /// Not all config options are available for all pins (this function currently do no checks). | ||
| // TODO: check if features are available for a given pin |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| /// | ||
| /// Not all port have 32 pins available (this function currently do no checks). | ||
| pub fn get_gpio(port: Port, pin: u5) gpio.GPIO { | ||
| // TODO: check unavailable pins |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| /// | ||
| /// Not all port have 32 pins available (this function currently do no checks). | ||
| pub fn get_pin(port: Port, pin: u5) Pin { | ||
| // TODO: check unavailable pins |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
|
|
||
| // This enum can be automatically generated using `generate.zig`, | ||
| // but some fields have been manually added for conveniance (e.g. PORT5, GPIO5) | ||
| // TODO: some fields are probably missing, add them |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
| // This enum can be automatically generated using `generate.zig`, | ||
| // but some fields have been manually added for conveniance (e.g. PORT5, GPIO5) | ||
| // TODO: some fields are probably missing, add them | ||
| // TODO: use u8 |
There was a problem hiding this comment.
TODO style comments need to have a linked microzig issue on the same line.
mattnite
left a comment
mattnite
left a comment
There was a problem hiding this comment.
Great work for your first go at this!
For the linter comments, ignore the ones regarding TODOs, but please fix the ones for naming conventions.
The other thing I need from this PR, is examples, we don't have a HIL yet for this stuff, but by having examples we at least guard your work from bit rot. So please try to make some that show usage of your HAL. Examples have a similar structure to ports, under the examples directory in the root of the project.
github-actions
bot
dismissed
their stale review
Updating with new lint results
There was a problem hiding this comment.
🔍 Lint Results
Found 9 issues on changed lines in 4 files:
- port/nxp/mcx/src/boards/frdm_mcxn947.zig: 1 issue
- port/nxp/mcx/src/mcxn947/hal/flexcomm.zig: 2 issues
- port/nxp/mcx/src/mcxn947/hal/flexcomm/LPI2c.zig: 3 issues
- port/nxp/mcx/src/mcxn947/hal/flexcomm/LPUart.zig: 3 issues
| /// pub const panic = board.panic; | ||
| /// ``` | ||
| pub const panic = std.debug.FullPanic(struct { | ||
| pub fn panicFn(message: []const u8, first_trace_address: ?usize) noreturn { |
There was a problem hiding this comment.
Please change to panic_fn, in MicroZig we use snake case for function names.
| pub const FlexComm = enum(u4) { | ||
| _, | ||
|
|
||
| pub const LPUart = @import("flexcomm/LPUart.zig").LPUart; |
There was a problem hiding this comment.
Suggestion: Rename LPUart to LP_UART, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
| _, | ||
|
|
||
| pub const LPUart = @import("flexcomm/LPUart.zig").LPUart; | ||
| pub const LPI2c = @import("flexcomm/LPI2c.zig").LPI2c; |
There was a problem hiding this comment.
Suggestion: Rename LPI2c to LPI_2c, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
| // TODO: 10 bit addressing | ||
| // TODO: better receive (with matching) | ||
| // TODO: SMBus | ||
| pub const LPI2c = enum(u4) { |
There was a problem hiding this comment.
Suggestion: Rename LPI2c to LPI_2c, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
| pub const LPI2c = enum(u4) { | ||
| _, | ||
|
|
||
| pub const LPI2cTy = *volatile chip.types.peripherals.LPI2C0; |
There was a problem hiding this comment.
Suggestion: Rename LPI2cTy to LPI_2cTy, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
|
|
||
|
|
||
| // Follows the linux kernel's approach | ||
| pub const I2cMsg = struct { |
There was a problem hiding this comment.
Suggestion: Rename I2cMsg to I2C_Msg, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
|
|
||
| const chip = microzig.chip; | ||
| const peripherals = chip.peripherals; | ||
| const Io = std.Io; |
There was a problem hiding this comment.
Suggestion: Rename Io to IO, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
|
|
||
| /// Uart interface. Currently only support 8 bit mode. | ||
| // TODO: 9 and 10 bit mode | ||
| pub const LPUart = enum(u4) { |
There was a problem hiding this comment.
Suggestion: Rename LPUart to LP_UART, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
| pub const LPUart = enum(u4) { | ||
| _, | ||
|
|
||
| pub const LPUartTy = *volatile chip.types.peripherals.LPUART0; |
There was a problem hiding this comment.
Suggestion: Rename LPUartTy to LP_UART_Ty, it should be more in line with our style guidelines. This automation is not perfect so take it with a grain of salt.
2 participants
This PR add support for NXP mcxn947 chip (mcxnx4x in theory, but only tested on mcxn947).
Please note that this is essentially my first time doing embedded. Also, I did not check much on other ports for conventions since I did not know if I would do a PR.
There is also a bunch of TODOs in the code.
What is included:
Target.derive(it initially did not copy all the settings and used the default value for many, it seems to be fixed on main now though)PortandGPIOtype (mostly taken from mcxa's port)PinandFlexCommtype for configurationIo.ReaderandIo.WriterinterfacesI2C_DeviceinterfaceI could not make the default linker script to work. I believe the issue comes from where the end of the stack is.
And I have no idea how to handle the second cpu core.
Edit: I have severely underestimated the number of TODOs I had left ...