mgmt: ec_host_cmd: add spi STM32 backend #57287

niedzwiecki-dawid · 2023-04-26T12:16:47Z

PR adds support for SPI STM32 backend.

include/zephyr/mgmt/ec_host_cmd/ec_host_cmd.h

subsys/mgmt/ec_host_cmd/Kconfig

subsys/mgmt/ec_host_cmd/ec_host_cmd_handler.c

nordicjm · 2023-04-26T13:31:43Z

doc/services/device_mgmt/ec_host_cmd.rst

@@ -36,13 +36,43 @@ one backend layer.
 .. image:: ec_host_cmd_shi.png
   :align: center

+Another case is SPI. Unfortunately, the current SPI API can't be used to handle the host commands


@tbursztyka this seems like a problem which is causing big duplication, are there plans to address this?

We are discussing the problem here

niedzwiecki-dawid · 2023-05-11T11:33:05Z

My plan is to add working implementation (SPI backend including driver) as a reference. In the long term, I would like to try extend the SPI API to handle host command communication flow. However, it won't be easy and requires changing/extending SPI drivers.

fabiobaltieri

Hey how about splitting this in two PRs, one for the first three patches and a dedicated one for the SPI backend? If you expect the backend conversation to drag a long, it may make things a bit easier.

subsys/mgmt/ec_host_cmd/Kconfig

subsys/mgmt/ec_host_cmd/ec_host_cmd_handler.c

niedzwiecki-dawid · 2023-05-12T13:27:05Z

Hey how about splitting this in two PRs, one for the first three patches and a dedicated one for the SPI backend? If you expect the backend conversation to drag a long, it may make things a bit easier.

Makes sense :-)

The PR with HC improvements. I don't know if the split can be done better.

I will rebase the SPI support once the improvement are merged.

Let continue review of the SPI STM32 backend here.

keith-zephyr · 2023-05-19T17:26:24Z

I haven't yet done a full review, but I will do so once the status = "disabled" vs. updating the compatible property issue is resolved.

niedzwiecki-dawid · 2023-05-22T13:41:22Z

I haven't yet done a full review, but I will do so once the status = "disabled" vs. updating the compatible property issue is resolved.

Thanks a lot for this suggestion. I'm really open to another ones to make this code less dirty.

erwango · 2023-05-22T14:02:55Z

doc/services/device_mgmt/ec_host_cmd.rst

+           /* Change the compatible string to use the Host Command version of the
+            * STM32 SPI driver
+            */
+           compatible = "st,stm32-spi-host-cmd";


I'd suggest to handle this slightly differently and use:

compatible = "st,stm32h7-spi", "st,stm32-spi-fifo", "st,stm32-spi-host-cmd";

This way, you keep the benefit of using "st,stm32h7-spi" and "st,stm32-spi-fifo" which are currently used to specify variants of STM32 SPI HW block.

I acknowledge this is less easy to use, but this will ease the implementation of this driver on multiple STM32 targets.
You can think or other ways to do it of course, but I think it would be easier for maintenance if you stay in sync with "main" STM32 SPI driver on that regard.

Please tell me if I understand it correctly.

The "st,stm32-spi" compatible string is always use for STM32 SPI block. The "st,stm32h7-spi" can be added as a second compatible string to specify variant of the SPI block that works a bit differently. "st,stm32-spi-fifo" is the same case.

So, in this case it is better just to change the "st,stm32-spi" string to "st,stm32-spi-host-cmd" and use the "additional" ones ("st,stm32h7-spi" and `"st,stm32-spi-fifo") as usual, because it doesn't cause building the standard driver anyway.

Is that correct? Thanks in advice.

This is fully correct

Fyi, spi_ll_stm32.c file is built in based on CONFIG_SPI_STM32=y, which is selected based on st,stm32-spi compat being enabled:

menuconfig SPI_STM32 bool "STM32 MCU SPI controller driver" default y depends on DT_HAS_ST_STM32_SPI_ENABLED

Great, working on the change.

The example in the docs is still correct, right?

erwango · 2023-05-23T08:51:50Z

dts/bindings/spi/st,stm32h7-spi-host-cmd.yaml

I don't think this file is required anymore

Needed devicetree macros e.g. DT_N_S_soc_S_spi_40013000_P_pinctrl_0_FOREACH_PROP_ELEM are not generated without this file for compatible = "st,stm32-spi-host-cmd";.

Sounds weird, there's likely an error somewhere

I will recheck it.

I've checked it in the devicetree python scripts. It looks like the property defines e.g. DT_N_S_soc_S_spi_40013000_P_pinctrl_0_IDX_0 are not generated for nodes that does't match any binding file. I think the reason is simple - the script doesn't know the property type. Only defines for default properties(status, compatible, reg and interrupts) are generated. So in my opinion, the binding file is required here.

subsys/mgmt/ec_host_cmd/backends/Kconfig

subsys/mgmt/ec_host_cmd/backends/ec_host_cmd_backend_spi_stm32.c

doc/services/device_mgmt/ec_host_cmd.rst

Add support for SPI host command backend for STM32 chips family. Unfortunately, the current SPI API can't be used to handle the host commands communication. The main issues are unknown command size sent by the host(the SPI transaction sends/receives specific number of bytes) and need to constant sending status byte(the SPI module is enabled and disabled per transaction). Thus the SPI backend includes basic SPI STM32 driver adjusted to host command specification. Signed-off-by: Dawid Niedzwiecki <[email protected]>

niedzwiecki-dawid · 2023-08-11T04:10:02Z

@erwango do you still request any change?

fabiobaltieri · 2023-08-11T11:19:17Z

@erwango do you still request any change?

I think Erwan is off until next week.

niedzwiecki-dawid · 2023-08-21T10:10:45Z

@erwango friendly ping :-)

niedzwiecki-dawid requested review from nashif, carlescufi, galak, MaureenHelm and tbursztyka as code owners April 26, 2023 12:16

zephyrbot added area: mcumgr platform: STM32 ST Micro STM32 area: SPI SPI bus area: ec_host_cmd labels Apr 26, 2023

zephyrbot requested review from ABOSTM, de-nordic, erwango, FRASTM, gautierg-st, GeorgeCGV, nordicjm and teburd April 26, 2023 12:17

zephyrbot assigned niedzwiecki-dawid Apr 26, 2023

niedzwiecki-dawid force-pushed the hc_spi branch 2 times, most recently from f0a129f to f8a6729 Compare April 26, 2023 13:21

nordicjm reviewed Apr 26, 2023

View reviewed changes

de-nordic removed their request for review April 26, 2023 15:20

niedzwiecki-dawid force-pushed the hc_spi branch from f8a6729 to 317a33d Compare April 27, 2023 05:21

zephyrbot requested a review from de-nordic April 27, 2023 05:22

niedzwiecki-dawid requested review from keith-zephyr and duda-patryk April 27, 2023 06:25

niedzwiecki-dawid requested a review from fabiobaltieri May 9, 2023 05:34

fabiobaltieri reviewed May 12, 2023

View reviewed changes

subsys/mgmt/ec_host_cmd/Kconfig Outdated Show resolved Hide resolved

subsys/mgmt/ec_host_cmd/ec_host_cmd_handler.c Outdated Show resolved Hide resolved

subsys/mgmt/ec_host_cmd/ec_host_cmd_handler.c Outdated Show resolved Hide resolved

niedzwiecki-dawid force-pushed the hc_spi branch from c98b0a6 to fa8611e Compare May 22, 2023 12:25

zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label May 22, 2023

niedzwiecki-dawid force-pushed the hc_spi branch from fa8611e to 10bdae8 Compare May 22, 2023 12:42

erwango requested changes May 22, 2023

View reviewed changes

niedzwiecki-dawid force-pushed the hc_spi branch from 10bdae8 to 6294bd6 Compare May 23, 2023 08:40

erwango reviewed May 23, 2023

View reviewed changes

erwango requested changes May 23, 2023

View reviewed changes

subsys/mgmt/ec_host_cmd/backends/Kconfig Outdated Show resolved Hide resolved

keith-zephyr reviewed May 23, 2023

View reviewed changes

subsys/mgmt/ec_host_cmd/backends/ec_host_cmd_backend_spi_stm32.c Outdated Show resolved Hide resolved

niedzwiecki-dawid force-pushed the hc_spi branch 2 times, most recently from a8a4964 to 2e536e2 Compare May 25, 2023 15:15

erwango self-requested a review May 25, 2023 15:32

fabiobaltieri reviewed Jun 12, 2023

View reviewed changes

niedzwiecki-dawid force-pushed the hc_spi branch 2 times, most recently from ac1ae77 to 568351e Compare June 12, 2023 14:07

fabiobaltieri previously approved these changes Jun 12, 2023

View reviewed changes

de-nordic removed their request for review June 21, 2023 11:14

keith-zephyr reviewed Aug 3, 2023

View reviewed changes

doc/services/device_mgmt/ec_host_cmd.rst Outdated Show resolved Hide resolved

doc/services/device_mgmt/ec_host_cmd.rst Outdated Show resolved Hide resolved

doc/services/device_mgmt/ec_host_cmd.rst Show resolved Hide resolved

carlescufi dismissed fabiobaltieri’s stale review via 5884b48 August 4, 2023 08:47

niedzwiecki-dawid force-pushed the hc_spi branch from 568351e to 5884b48 Compare August 4, 2023 08:47

keith-zephyr approved these changes Aug 4, 2023

View reviewed changes

fabiobaltieri approved these changes Aug 4, 2023

View reviewed changes

nordicjm mentioned this pull request Aug 10, 2023

Define guidelines for improved four eyes principle #61358

Closed

erwango approved these changes Aug 21, 2023

View reviewed changes

carlescufi merged commit 63af3c0 into zephyrproject-rtos:main Aug 21, 2023
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgmt: ec_host_cmd: add spi STM32 backend #57287

mgmt: ec_host_cmd: add spi STM32 backend #57287

niedzwiecki-dawid commented Apr 26, 2023 •

edited

Loading

nordicjm Apr 26, 2023

niedzwiecki-dawid Apr 26, 2023

niedzwiecki-dawid commented May 11, 2023

fabiobaltieri left a comment

niedzwiecki-dawid commented May 12, 2023 •

edited

Loading

keith-zephyr commented May 19, 2023

niedzwiecki-dawid commented May 22, 2023

erwango May 22, 2023

niedzwiecki-dawid May 23, 2023

erwango May 23, 2023

niedzwiecki-dawid May 23, 2023

niedzwiecki-dawid May 23, 2023

erwango May 23, 2023

niedzwiecki-dawid May 23, 2023

erwango May 23, 2023

niedzwiecki-dawid May 23, 2023

niedzwiecki-dawid May 25, 2023 •

edited

Loading

niedzwiecki-dawid commented Aug 11, 2023

fabiobaltieri commented Aug 11, 2023

niedzwiecki-dawid commented Aug 21, 2023

mgmt: ec_host_cmd: add spi STM32 backend #57287

mgmt: ec_host_cmd: add spi STM32 backend #57287

Conversation

niedzwiecki-dawid commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented May 11, 2023

fabiobaltieri left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented May 12, 2023 • edited Loading

keith-zephyr commented May 19, 2023

niedzwiecki-dawid commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedzwiecki-dawid May 25, 2023 • edited Loading

Choose a reason for hiding this comment

niedzwiecki-dawid commented Aug 11, 2023

fabiobaltieri commented Aug 11, 2023

niedzwiecki-dawid commented Aug 21, 2023

niedzwiecki-dawid commented Apr 26, 2023 •

edited

Loading

niedzwiecki-dawid commented May 12, 2023 •

edited

Loading

niedzwiecki-dawid May 25, 2023 •

edited

Loading