Re: [PATCH v3 03/14] remoteproc: Add new operation and flags for synchronistation
From: Bjorn Andersson <hidden>
Date: 2020-05-06 00:22:10
Also in:
linux-remoteproc, lkml
On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote:
Add a new sync_ops to support use cases where the remoteproc core is synchronising with the remote processor. Exactly when to use the synchronisation operations is directed by the flags in structure rproc_sync_flags.
I'm sorry, but no matter how many times I read these patches I have to translate "synchronising" to "remote controlled", and given the number of comments clarifying this makes me feel that we could perhaps come up with a better name?
quoted hunk ↗ jump to hunk
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- include/linux/remoteproc.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index ac4082f12e8b..ceb3b2bba824 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h@@ -353,6 +353,23 @@ enum rsc_handling_status { RSC_IGNORED = 1, }; +/** + * struct rproc_sync_flags - platform specific flags indicating which + * rproc_ops to use at specific times during + * the rproc lifecycle. + * @on_init: true if synchronising with the remote processor at + * initialisation time + * @after_stop: true if synchronising with the remote processor after it was + * stopped from the cmmand line + * @after_crash: true if synchronising with the remote processor after + * it has crashed + */ +struct rproc_sync_flags { + bool on_init;
This indirectly splits the RPROC_OFFLINE state in an "offline" and "already-booted" state. Wouldn't it be clearer to represent this with a new RPROC_ALREADY_BOOTED state?
+ bool after_stop;
What does it mean when this is true? That Linux can shut the remote core down, but someone else will start it?
+ bool after_crash;
Similarly what is the expected steps to be taken by the core when this is true? Should rproc_report_crash() simply stop/start the subdevices and upon one of the ops somehow tell the remote controller that it can proceed with the recovery?
quoted hunk ↗ jump to hunk
+}; + /** * struct rproc_ops - platform-specific device handlers * @start: power on the device and boot it@@ -459,6 +476,9 @@ struct rproc_dump_segment { * @firmware: name of firmware file to be loaded * @priv: private data which belongs to the platform-specific rproc module * @ops: platform-specific start/stop rproc handlers + * @sync_ops: platform-specific start/stop rproc handlers when + * synchronising with a remote processor. + * @sync_flags: Determine the rproc_ops to choose in specific states. * @dev: virtual device for refcounting and common remoteproc behavior * @power: refcount of users who need this rproc powered up * @state: state of the device@@ -482,6 +502,7 @@ struct rproc_dump_segment { * @table_sz: size of @cached_table * @has_iommu: flag to indicate if remote processor is behind an MMU * @auto_boot: flag to indicate if remote processor should be auto-started + * @sync_with_rproc: true if currently synchronising with the rproc * @dump_segments: list of segments in the firmware * @nb_vdev: number of vdev currently handled by rproc */@@ -492,6 +513,8 @@ struct rproc { const char *firmware; void *priv; struct rproc_ops *ops; + struct rproc_ops *sync_ops;
Do we really need two rproc_ops, given that both are coming from the platform driver and the sync_flags will define which one to look at? Can't the platform driver just provide an ops table that works with the flags it passes? Regards, Bjorn
quoted hunk ↗ jump to hunk
+ struct rproc_sync_flags sync_flags; struct device dev; atomic_t power; unsigned int state;@@ -515,6 +538,7 @@ struct rproc { size_t table_sz; bool has_iommu; bool auto_boot; + bool sync_with_rproc; struct list_head dump_segments; int nb_vdev; u8 elf_class;-- 2.20.1