lib: bm_spi_mngr: sample: port SPI transaction manager to NCS bare metal, add sample and board configs

[PizzaAllTheWay]

Port the legacy nRF5 SDK nrf_spi_mngr library to NCS bare metal as bm_spi_mngr. The library serializes SPIM work on a single hardware instance by queuing transactions in a FIFO and running them back-to-back.

Main changes from the original:

• nrfx_spim instead of nrf_drv_spi.
• Zephyr ring buffer instead of nrf_queue, with irq_lock around shared queue access.
• All public APIs return int instead of ret_code_t.
• Error codes follow the <errno.h> (negative errno) scheme. nrfx error codes are propagated as-is since they already use the same convention.
• bm_spi_mngr_perform asserts its idle hook is not called from ISR context, as a precaution against silent deadlocks.

Both the non-blocking (schedule) and blocking (perform) APIs are preserved, as is per-transaction SPIM reconfiguration.

Also adds:

• A sample under samples/peripherals/spi_mngr that demonstrates non-blocking read, page program, and sector erase against the on-board external NOR flash. Only supported on the nRF54L15 DK and nRF54LM20 DK, as the other supported development kits do not have on-board external memory, so it would be hard to showcase SPI Manager there without external hardware.
• BOARD_EXTERNAL_MEMORY_* macros in the affected board-config.h files so the sample picks up the on-board flash automatically.

[github-actions]

You can find the documentation preview for this PR here.

[eivindj-nordic]

Please have a look through the documentation, header and source code to align the format with the rest of the NCS BM repo, avoid typedefs, hungarian notation etc.

[eivindj-nordic]

This belongs in the header.

[PizzaAllTheWay]

Oki. I rewrote API documentation to align with other API docs

[sondrep]

I notice that irq_lock() is called in both this function and the queue_pop() function, is it necessary to do it both functions? queue_pop() is not called any other places.

[PizzaAllTheWay]

Good point, I removed the irq_lock() from queue_pop(). Added a comment saying the caller must hold the lock.

[sondrep]

In Board Configurator v1.2.0 the external storage is referred to as External memory.

Suggested change

	Before flashing the sample, you must enable **external storage** using the `Board Configurator`_ app in `nRF Connect for Desktop`_, and then write the configuration to the board.
	Before flashing the sample, you must enable **External memory** using the `Board Configurator`_ app in `nRF Connect for Desktop`_, and then write the configuration to the board.

[PizzaAllTheWay]

fixed

[asilz]

Header represents the length better since address is 3 bytes and opcode is 1 byte

Suggested change

	#define MX25_CMD_ADDR_LEN 4U
	#define MX25_CMD_HEADER_LEN 4U

[PizzaAllTheWay]

fixed

[asilz]

Currently sending extra 0x00 bytes after the command

Suggested change

	static uint8_t read_tx[MX25_CMD_ADDR_LEN + BLOCK_LEN] = {
	static uint8_t read_tx[] = {

[PizzaAllTheWay]

fixed

[sondrep]

Is it strictly necessary for this struct to be volatile? It is only touched by spi_internal_transaction_cb() and bm_spi_mngr_perform(), so I don't see why it needs to be volatile, unless there is a problem with the memory consistency without it.

[PizzaAllTheWay]

Yes, it needs to be there, and you are correct about memory consistency. My initial thinking was honestly that the compiler would just optimize it out either way, so it wouldn't matter much. The case I had in mind is the blocking bm_spi_mngr_perform(), which spins here:

while (cb_data.transaction_in_progress) {
    if (user_function) {
        user_function();
    }
}

Nothing inside bm_spi_mngr_perform() ever writes false to that flag, it's cleared from the SPIM interrupt callback. So in theory the compiler could read it once, cache it, and turn this into an infinite loop, ie:

while (true) {
    if (user_function) {
        user_function();
    }
}

I actually tested it with a blocking call and it did seem to work the same with and without volatile. So one would assume that volatile is not needed here. However, that's just because of the circumstances: the loop calls an opaque user_function() and the address of cb_data gets passed off into another function, so the compiler can't prove nothing touches the flag and ends up re-reading it each iteration anyway.

The problem is that user_function is optional, passing NULL is perfectly valid (and the __ASSERT_NO_MSG(user_function == NULL || !k_is_in_isr()) explicitly allows it). For example, a blocking erase with no idle hook:

return bm_spi_mngr_perform(&spi_mgr, NULL, erase_xfers, ARRAY_SIZE(erase_xfers), NULL);

With user_function == NULL, that accidental unintended safety net is gone. The loop body collapses to nothing the compiler can see touching cb_data:

while (cb_data.transaction_in_progress) {
    /* if (user_function) -> NULL, dead code, removed */
}

Now the compiler is free to cache the flag and produce a genuine infinite loop => deadlock, even though the ISR clears it in memory. So this isn't a theoretical edge case, it's a fully supported way of calling the function.

volatile forces a fresh read of the flag every iteration regardless of whether user_function is set, so the ISR write is always observed and the loop exits. It also keeps us aligned with the old nRF5 SDK and makes it clear to the reader that this variable is touched from an interrupt and needs special attention and care.

[sondrep]

Should the asserts be handled like other files in lib, e.g.

Suggested change

	__ASSERT_NO_MSG(p_default_spim_config != NULL);
	if (p_default_spim_config == NULL) {
	return -EFAULT;
	}

[PizzaAllTheWay]

Fixed, replaced the user input asserts in the public API (bm_spi_mngr_init, bm_spi_mngr_schedule, bm_spi_mngr_perform) with guard clauses returning -EFAULT/-EINVAL, and updated the header @retval docs to match. Kept __ASSERT_NO_MSG for the internal checks since those guard library invariants/misuse, not user input.

[PizzaAllTheWay]

Please have a look through the documentation, header and source code to align the format with the rest of the NCS BM repo, avoid typedefs, hungarian notation etc.

Done. I converted all struct typedefs to named struct tags (keeping the function-pointer typedefs with _t, the same way other headers do it), removed the p_ Hungarian prefixes throughout, and rewrote the API docs to match other API doc style. Let me know if I have missed anything.