spi shell does not allow for device reconfiguration when spi_context_configured is used

[SureshotM6]

When using spi conf <device> <frequency> [<settings>] followed by spi transceive from the shell, SPI controller drivers that are using spi_context_configured (such as spi_ll_stm32.c) do not allow for subsequent reconfiguration with spi conf. This means that it is not possible to change clock frequencies, polarity, phase, etc. without a reboot and there is no indication that the new settings did not take effect (besides verifying with a logic analyzer). Found on a STM32 Nucleo board, but appears to be applicable to other platforms as well.

This issue is due to spi_shell.c using a static struct spi_config whose address never changes, and the spi_context_configured API is simply checking to see if the address of the last used struct spi_config changed. Since the address never changes, even if the contents change, spi_context_configured returns true and the new configuration is never applied.

I have not investigated the best way of fixing this, but here are a few ideas:

1. Have spi_shell.c perform a dummy / 0 byte SPI transaction with a different struct spi_config structure when spi conf is called. This will force spi_context_configured to return false since the address is different and force a reconfiguration on the next transaction. However, this may not work properly with all SPI controller drivers and would require testing.
2. Add a new SPI_OP_RECONFIG flag to spi_operation_t (used inside of struct spi_config) to force spi_context_configured to return false and always force reconfiguration when set. This would be set by spi conf and optionally cleared after spi transceive.
3. Enhance spi_context_configured to perform a better (albeit slower) check, such as hashing the structure.
4. Enhance spi shell to double buffer the struct spi_config structure, and swap which structure is used on a spi conf. This would be fairly complicated to implement correctly though, as the last used config would need to be tracked on a per-controller basis.
5. Utilize spi_release to clear the cached struct spi_config pointer in the context. This is not supported by all controller drivers, would change the semantics of spi_release, and would unnecessarily force reconfiguration in some cases.
6. Add a new API / syscall to clear the cached struct spi_config,

To Reproduce

1. spi conf spi@44002000 80000000
2. spi transceive aa <- transceives at ~80MHz, mode 0, MSB first as requested
3. spi conf spi@44002000 100000 ol
4. spi transceive aa <- transceives at ~80MHz, mode 0, MSB first; should be ~100kHz, mode 2, LSB first

Expected behavior

SPI transaction with spi transceive is made with requested settings from spi conf.

Impact

Since the spi shell is mainly for debugging and hardware bringup, this is minor. Given that there is no indication of a potential issue to the user though, they may be inclined to believe that their SPI peripheral is not working instead of the controller settings being incorrect.

Environment (please complete the following information):

• OS: Linux
• Toolchain: Zephyr SDK 16.9
• Commit SHA or Version used: v3.7.1

[github-actions]

Hi @SureshotM6! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[tbursztyka]

SPI shell is seldomly used, and only for quick testing. No need to make it too complicated: rotating on 2 spi_config would do. Basically, use one spi_config and after a call to conf and a subsequent call to transceive, next time another spi_config will need to be used, and again and again after each conf/transceive. Switch to one or the other every conf+transceive (basically the conf use another config, and a transceive acknowledge that next conf will use the other config. This is to avoid calling conf 2+ times in a row and possibly ending in the same issue of having the current config pointer set in spi_context)

[SureshotM6]

No need to make it too complicated: rotating on 2 spi_config would do.

Yeah, that's my suggestion #4 above.

Switch to one or the other every conf+transceive (basically the conf use another config, and a transceive acknowledge that next conf will use the other config.

The problem with this, as I mentioned above, is when you have multiple SPI controllers. Doing a conf+transceive on controller A, followed by a conf+transceive on controller B, and back to a conf+transceive on controller A will result in using the same spi_config pointer on A twice in a row. That is why I mentioned that #4 is complicated to implement correctly.

It does also seem like spi_config reconfiguration may be needed for other use cases, such as for devices which switch protocols or allow higher speeds after an initial configuration write. Suggestions #2, #3, and #6 above are more generic and could be used for those devices too. Requiring those drivers to implement multiple spi_config structs to work around this seems like a waste of memory and slightly hacky.

[tbursztyka]

Seriously, spi shell is really meant for quick testing, no need to handle all possible use cases to avoid any possibility of collision. I don't see why we should grow the API just for this shell module, and no let's not slow spi_context_configured.

Doing a conf+transceive on controller A, followed by a conf+transceive on controller B, and back to a conf+transceive on controller A will result in using the same spi_config pointer on A twice in a row. That is why I mentioned that #4 is complicated to implement correctly.

Yes you will have to add some logic in the shell to by-pass some uses cases like that (not all). For instance on second conf of controller A, the module could check what it has already configured in the past on the same controller and thus forcefully reuses the one used for B instead etc...