| Message ID | 20200324214603.14979-1-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | remoteproc: Add support for synchronisation with MCU | expand |
Hi Mathieu, > -----Original Message----- > From: Mathieu Poirier <mathieu.poirier@linaro.org> > Sent: mardi 24 mars 2020 22:46 > To: bjorn.andersson@linaro.org > Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s- > anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN > <arnaud.pouliquen@st.com>; Fabien DESSENNE > <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org > Subject: [PATCH v2 00/17] remoteproc: Add support for synchronisation with > MCU > > 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. > > 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
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 >
On Tue, Mar 31, 2020 at 05:51:44PM -0500, Suman Anna wrote: > 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 You got it. > > 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(). You raise an interesting point... As my work on the mp157c [1] proved, mandating to know if the core should sync before calling rproc_alloc_state_machine() requires a fair amount of refactoring. I will follow your recommendation for the next revision. Thanks, Mathieu [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261083 > > 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 > > >
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. Comments would be much appreciated. 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(-)