Hi Mathieu, On 3/27/20 12:20 PM, Loic PALLARDY wrote: > Hi Mathieu, > >> >> This is the second revision of this set that tries to address the >> problem of synchronising with an MCU with as much flexibility as >> possible. >> >> New in this revision is a fix for a couple of bugs I found while >> testing things. First with the helper macro in patch 09 and the >> suppression of a boot message when synchronising with an MCU >> in patch 12. I have completely removed what used to be patch 18, >> the example on how to use the new API. This will be the subject >> of an upcoming patchset. >> >> Tested on ST's mp157c platform. Applies on v5.6-rc7 to keep things >> simple. > > Thanks Mathieu for the 2 series. I tested on my STM32MP157-DK2 the different > synchronization use cases (on_init, after_stop, after_crash), mixing the values and > I succeed to start/stop/restart M4 coprocessor with or without reloading firmware > according to sync values. (I only applied the correction I proposed in patch 16 review > to allow to resync with a preloaded or an already running coprocessor. > > Regards, > Loic >> >> Comments would be much appreciated. Thank you for the enhanced series to implement the logic in remoteproc core. I have provided my comments on most of the patches. Overall, I can see my early-boot scenarios work with the series, and the slightly different userspace-loading support usecase would need some additional support. As I commented on patch 1 in v1, I would rather reuse the the generic "rproc" instead of adding a new "mcu" terminology to code.. Let's just stick with the rproc Another thing I would prefer (echoing my comments on patch 7) is to just use an API for modifying the sync states, that can be used between rproc_alloc() and rproc_add(). The state-machine really doesn't kick in until rproc_add() is invoked. The memory for the driver private rproc structure is allocated using rproc_alloc() normally, and most of the DT-parsing in platform drivers is generally done directly into this allocated memory. I see it a bit cumbersome having to maintain a separate structure, and then do a memcpy, especially given that the rproc_alloc_state_machine() logic requires that you detect the state before calling the rproc_alloc(). regards Suman >> >> Thanks, >> Mathieu >> >> Mathieu Poirier (17): >> remoteproc: Add new operation and state machine for MCU >> synchronisation >> remoteproc: Introduce function rproc_set_mcu_sync_state() >> remoteproc: Split firmware name allocation from rproc_alloc() >> remoteproc: Split rproc_ops allocation from rproc_alloc() >> remoteproc: Get rid of tedious error path >> remoteproc: Introduce function rproc_alloc_internals() >> remoteproc: Introduce function rproc_alloc_state_machine() >> remoteproc: Allocate synchronisation state machine >> remoteproc: Call the right core function based on synchronisation >> state >> remoteproc: Decouple firmware load and remoteproc booting >> remoteproc: Repurpose function rproc_trigger_auto_boot() >> remoteproc: Rename function rproc_fw_boot() >> remoteproc: Introducting new functions to start and stop an MCU >> remoteproc: Refactor function rproc_trigger_recovery() >> remoteproc: Correctly deal with MCU synchronisation when changing FW >> image >> remoteproc: Correctly deal with MCU synchronisation when changing >> state >> remoteproc: Make MCU synchronisation state changes on stop and crashed >> >> drivers/remoteproc/remoteproc_core.c | 387 ++++++++++++++++++----- >> drivers/remoteproc/remoteproc_debugfs.c | 31 ++ >> drivers/remoteproc/remoteproc_internal.h | 91 ++++-- >> drivers/remoteproc/remoteproc_sysfs.c | 57 +++- >> include/linux/remoteproc.h | 28 +- >> 5 files changed, 487 insertions(+), 107 deletions(-) >> >> -- >> 2.20.1 >
