| Message ID | 20200424200135.28825-13-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: Add support for synchronisaton with rproc | expand |
On 4/24/20 10:01 PM, Mathieu Poirier wrote: > Introducting function rproc_set_state_machine() to add > operations and a set of flags to use when synchronising with > a remote processor. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ > drivers/remoteproc/remoteproc_internal.h | 6 +++ > include/linux/remoteproc.h | 3 ++ > 3 files changed, 63 insertions(+) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 48afa1f80a8f..5c48714e8702 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) > } > EXPORT_SYMBOL(devm_rproc_add); > > +/** > + * rproc_set_state_machine() - Set a synchronisation ops and set of flags > + * to use with a remote processor > + * @rproc: The remote processor to work with > + * @sync_ops: The operations to use when synchronising with a remote > + * processor > + * @sync_flags: The flags to use when deciding if the remoteproc core > + * should be synchronising with a remote processor > + * > + * Returns 0 on success, an error code otherwise. > + */ > +int rproc_set_state_machine(struct rproc *rproc, > + const struct rproc_ops *sync_ops, > + struct rproc_sync_flags sync_flags) So this API should be called by platform driver only in case of synchronization support, right? In this case i would rename it as there is also a state machine in "normal" boot proposal: rproc_set_sync_machine or rproc_set_sync_state_machine > +{ > + if (!rproc || !sync_ops) > + return -EINVAL; > + > + /* > + * No point in going further if we never have to synchronise with > + * the remote processor. > + */ > + if (!sync_flags.on_init && > + !sync_flags.after_stop && !sync_flags.after_crash) > + return 0; > + > + /* > + * Refuse to go further if remoteproc operations have been allocated > + * but they will never be used. > + */ > + if (rproc->ops && sync_flags.on_init && > + sync_flags.after_stop && sync_flags.after_crash) > + return -EINVAL; > + > + /* > + * Don't allow users to set this more than once to avoid situations > + * where the remote processor can't be recovered. > + */ > + if (rproc->sync_ops) > + return -EINVAL; > + > + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); > + if (!rproc->sync_ops) > + return -ENOMEM; > + > + rproc->sync_flags = sync_flags; > + /* Tell the core what to do when initialising */ > + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); Is there a use case where sync_flags.on_init is false and other flags are true? Look like on_init is useless and should not be exposed to the platform driver. Or comments are missing to explain the usage of it vs the other flags. Regards, Arnaud > + > + return 0; > +} > +EXPORT_SYMBOL(rproc_set_state_machine); > + > /** > * rproc_type_release() - release a remote processor instance > * @dev: the rproc's device > @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) > kfree_const(rproc->firmware); > kfree_const(rproc->name); > kfree(rproc->ops); > + kfree(rproc->sync_ops); > kfree(rproc); > } > > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > index 7dcc0a26892b..c1a293a37c78 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -27,6 +27,8 @@ struct rproc_debug_trace { > /* > * enum rproc_sync_states - remote processsor sync states > * > + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core > + * is initialising. > * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core > * has shutdown (rproc_shutdown()) the > * remote processor. > @@ -39,6 +41,7 @@ struct rproc_debug_trace { > * operation to use. > */ > enum rproc_sync_states { > + RPROC_SYNC_STATE_INIT, > RPROC_SYNC_STATE_SHUTDOWN, > RPROC_SYNC_STATE_CRASHED, > }; > @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, > enum rproc_sync_states state) > { > switch (state) { > + case RPROC_SYNC_STATE_INIT: > + rproc->sync_with_rproc = rproc->sync_flags.on_init; > + break; > case RPROC_SYNC_STATE_SHUTDOWN: > rproc->sync_with_rproc = rproc->sync_flags.after_stop; > break; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index ceb3b2bba824..a75ed92b3de6 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); > struct rproc *rproc_alloc(struct device *dev, const char *name, > const struct rproc_ops *ops, > const char *firmware, int len); > +int rproc_set_state_machine(struct rproc *rproc, > + const struct rproc_ops *sync_ops, > + struct rproc_sync_flags sync_flags); > void rproc_put(struct rproc *rproc); > int rproc_add(struct rproc *rproc); > int rproc_del(struct rproc *rproc); >
On 4/29/20 11:22 AM, Arnaud POULIQUEN wrote: > > > On 4/24/20 10:01 PM, Mathieu Poirier wrote: >> Introducting function rproc_set_state_machine() to add >> operations and a set of flags to use when synchronising with >> a remote processor. >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ >> drivers/remoteproc/remoteproc_internal.h | 6 +++ >> include/linux/remoteproc.h | 3 ++ >> 3 files changed, 63 insertions(+) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 48afa1f80a8f..5c48714e8702 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) >> } >> EXPORT_SYMBOL(devm_rproc_add); >> >> +/** >> + * rproc_set_state_machine() - Set a synchronisation ops and set of flags >> + * to use with a remote processor >> + * @rproc: The remote processor to work with >> + * @sync_ops: The operations to use when synchronising with a remote >> + * processor >> + * @sync_flags: The flags to use when deciding if the remoteproc core >> + * should be synchronising with a remote processor >> + * >> + * Returns 0 on success, an error code otherwise. >> + */ >> +int rproc_set_state_machine(struct rproc *rproc, >> + const struct rproc_ops *sync_ops, >> + struct rproc_sync_flags sync_flags) > > So this API should be called by platform driver only in case of synchronization > support, right? > In this case i would rename it as there is also a state machine in "normal" boot > proposal: rproc_set_sync_machine or rproc_set_sync_state_machine > Reviewing the stm32 series, i wonder if sync_flags should be a pointer to a const structure as the platform driver should not update it during the rproc live cycle. Then IMO, using a pointer to the structure instead of the structure seems more in line with the rest of the remoteproc API. >> +{ >> + if (!rproc || !sync_ops) >> + return -EINVAL; >> + >> + /* >> + * No point in going further if we never have to synchronise with >> + * the remote processor. >> + */ >> + if (!sync_flags.on_init && >> + !sync_flags.after_stop && !sync_flags.after_crash) >> + return 0; >> + >> + /* >> + * Refuse to go further if remoteproc operations have been allocated >> + * but they will never be used. >> + */ >> + if (rproc->ops && sync_flags.on_init && >> + sync_flags.after_stop && sync_flags.after_crash) >> + return -EINVAL; >> + >> + /* >> + * Don't allow users to set this more than once to avoid situations >> + * where the remote processor can't be recovered. >> + */ >> + if (rproc->sync_ops) >> + return -EINVAL; >> + >> + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); >> + if (!rproc->sync_ops) >> + return -ENOMEM; >> + >> + rproc->sync_flags = sync_flags; >> + /* Tell the core what to do when initialising */ >> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); > > Is there a use case where sync_flags.on_init is false and other flags are true? > > Look like on_init is useless and should not be exposed to the platform driver. > Or comments are missing to explain the usage of it vs the other flags. > > Regards, > Arnaud > >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(rproc_set_state_machine); >> + >> /** >> * rproc_type_release() - release a remote processor instance >> * @dev: the rproc's device >> @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) >> kfree_const(rproc->firmware); >> kfree_const(rproc->name); >> kfree(rproc->ops); >> + kfree(rproc->sync_ops); >> kfree(rproc); >> } >> >> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h >> index 7dcc0a26892b..c1a293a37c78 100644 >> --- a/drivers/remoteproc/remoteproc_internal.h >> +++ b/drivers/remoteproc/remoteproc_internal.h >> @@ -27,6 +27,8 @@ struct rproc_debug_trace { >> /* >> * enum rproc_sync_states - remote processsor sync states >> * >> + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core >> + * is initialising. >> * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core >> * has shutdown (rproc_shutdown()) the >> * remote processor. >> @@ -39,6 +41,7 @@ struct rproc_debug_trace { >> * operation to use. >> */ >> enum rproc_sync_states { >> + RPROC_SYNC_STATE_INIT, >> RPROC_SYNC_STATE_SHUTDOWN, >> RPROC_SYNC_STATE_CRASHED, >> }; >> @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, >> enum rproc_sync_states state) >> { >> switch (state) { >> + case RPROC_SYNC_STATE_INIT: >> + rproc->sync_with_rproc = rproc->sync_flags.on_init; >> + break; >> case RPROC_SYNC_STATE_SHUTDOWN: >> rproc->sync_with_rproc = rproc->sync_flags.after_stop; >> break; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index ceb3b2bba824..a75ed92b3de6 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); >> struct rproc *rproc_alloc(struct device *dev, const char *name, >> const struct rproc_ops *ops, >> const char *firmware, int len); >> +int rproc_set_state_machine(struct rproc *rproc, >> + const struct rproc_ops *sync_ops, >> + struct rproc_sync_flags sync_flags); >> void rproc_put(struct rproc *rproc); >> int rproc_add(struct rproc *rproc); >> int rproc_del(struct rproc *rproc); >>
On Wed, Apr 29, 2020 at 11:22:28AM +0200, Arnaud POULIQUEN wrote: > > > On 4/24/20 10:01 PM, Mathieu Poirier wrote: > > Introducting function rproc_set_state_machine() to add > > operations and a set of flags to use when synchronising with > > a remote processor. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ > > drivers/remoteproc/remoteproc_internal.h | 6 +++ > > include/linux/remoteproc.h | 3 ++ > > 3 files changed, 63 insertions(+) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 48afa1f80a8f..5c48714e8702 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) > > } > > EXPORT_SYMBOL(devm_rproc_add); > > > > +/** > > + * rproc_set_state_machine() - Set a synchronisation ops and set of flags > > + * to use with a remote processor > > + * @rproc: The remote processor to work with > > + * @sync_ops: The operations to use when synchronising with a remote > > + * processor > > + * @sync_flags: The flags to use when deciding if the remoteproc core > > + * should be synchronising with a remote processor > > + * > > + * Returns 0 on success, an error code otherwise. > > + */ > > +int rproc_set_state_machine(struct rproc *rproc, > > + const struct rproc_ops *sync_ops, > > + struct rproc_sync_flags sync_flags) > > So this API should be called by platform driver only in case of synchronization > support, right? Correct > In this case i would rename it as there is also a state machine in "normal" boot > proposal: rproc_set_sync_machine or rproc_set_sync_state_machine That is a valid observation - rproc_set_sync_state_machine() sounds descriptive enough for me. > > > +{ > > + if (!rproc || !sync_ops) > > + return -EINVAL; > > + > > + /* > > + * No point in going further if we never have to synchronise with > > + * the remote processor. > > + */ > > + if (!sync_flags.on_init && > > + !sync_flags.after_stop && !sync_flags.after_crash) > > + return 0; > > + > > + /* > > + * Refuse to go further if remoteproc operations have been allocated > > + * but they will never be used. > > + */ > > + if (rproc->ops && sync_flags.on_init && > > + sync_flags.after_stop && sync_flags.after_crash) > > + return -EINVAL; > > + > > + /* > > + * Don't allow users to set this more than once to avoid situations > > + * where the remote processor can't be recovered. > > + */ > > + if (rproc->sync_ops) > > + return -EINVAL; > > + > > + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); > > + if (!rproc->sync_ops) > > + return -ENOMEM; > > + > > + rproc->sync_flags = sync_flags; > > + /* Tell the core what to do when initialising */ > > + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); > > Is there a use case where sync_flags.on_init is false and other flags are true? I haven't seen one yet, which doesn't mean it doesn't exist or won't in the future. I wanted to make this as flexible as possible. I started with the idea of making synchronisation at initialisation time implicit if rproc_set_state_machine() is called but I know it is only a matter of time before people come up with some exotic use case where .on_init is false. > > Look like on_init is useless and should not be exposed to the platform driver. > Or comments are missing to explain the usage of it vs the other flags. Comments added in remoteproc_internal.h and the new section in Documentation/remoteproc.txt aren't sufficient? Can you give me a hint as to what you think is missing? > > Regards, > Arnaud > > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(rproc_set_state_machine); > > + > > /** > > * rproc_type_release() - release a remote processor instance > > * @dev: the rproc's device > > @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) > > kfree_const(rproc->firmware); > > kfree_const(rproc->name); > > kfree(rproc->ops); > > + kfree(rproc->sync_ops); > > kfree(rproc); > > } > > > > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > > index 7dcc0a26892b..c1a293a37c78 100644 > > --- a/drivers/remoteproc/remoteproc_internal.h > > +++ b/drivers/remoteproc/remoteproc_internal.h > > @@ -27,6 +27,8 @@ struct rproc_debug_trace { > > /* > > * enum rproc_sync_states - remote processsor sync states > > * > > + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core > > + * is initialising. > > * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core > > * has shutdown (rproc_shutdown()) the > > * remote processor. > > @@ -39,6 +41,7 @@ struct rproc_debug_trace { > > * operation to use. > > */ > > enum rproc_sync_states { > > + RPROC_SYNC_STATE_INIT, > > RPROC_SYNC_STATE_SHUTDOWN, > > RPROC_SYNC_STATE_CRASHED, > > }; > > @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, > > enum rproc_sync_states state) > > { > > switch (state) { > > + case RPROC_SYNC_STATE_INIT: > > + rproc->sync_with_rproc = rproc->sync_flags.on_init; > > + break; > > case RPROC_SYNC_STATE_SHUTDOWN: > > rproc->sync_with_rproc = rproc->sync_flags.after_stop; > > break; > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > index ceb3b2bba824..a75ed92b3de6 100644 > > --- a/include/linux/remoteproc.h > > +++ b/include/linux/remoteproc.h > > @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); > > struct rproc *rproc_alloc(struct device *dev, const char *name, > > const struct rproc_ops *ops, > > const char *firmware, int len); > > +int rproc_set_state_machine(struct rproc *rproc, > > + const struct rproc_ops *sync_ops, > > + struct rproc_sync_flags sync_flags); > > void rproc_put(struct rproc *rproc); > > int rproc_add(struct rproc *rproc); > > int rproc_del(struct rproc *rproc); > >
On Wed, Apr 29, 2020 at 04:38:54PM +0200, Arnaud POULIQUEN wrote: > > > On 4/29/20 11:22 AM, Arnaud POULIQUEN wrote: > > > > > > On 4/24/20 10:01 PM, Mathieu Poirier wrote: > >> Introducting function rproc_set_state_machine() to add > >> operations and a set of flags to use when synchronising with > >> a remote processor. > >> > >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >> --- > >> drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ > >> drivers/remoteproc/remoteproc_internal.h | 6 +++ > >> include/linux/remoteproc.h | 3 ++ > >> 3 files changed, 63 insertions(+) > >> > >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > >> index 48afa1f80a8f..5c48714e8702 100644 > >> --- a/drivers/remoteproc/remoteproc_core.c > >> +++ b/drivers/remoteproc/remoteproc_core.c > >> @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) > >> } > >> EXPORT_SYMBOL(devm_rproc_add); > >> > >> +/** > >> + * rproc_set_state_machine() - Set a synchronisation ops and set of flags > >> + * to use with a remote processor > >> + * @rproc: The remote processor to work with > >> + * @sync_ops: The operations to use when synchronising with a remote > >> + * processor > >> + * @sync_flags: The flags to use when deciding if the remoteproc core > >> + * should be synchronising with a remote processor > >> + * > >> + * Returns 0 on success, an error code otherwise. > >> + */ > >> +int rproc_set_state_machine(struct rproc *rproc, > >> + const struct rproc_ops *sync_ops, > >> + struct rproc_sync_flags sync_flags) > > > > So this API should be called by platform driver only in case of synchronization > > support, right? > > In this case i would rename it as there is also a state machine in "normal" boot > > proposal: rproc_set_sync_machine or rproc_set_sync_state_machine > > > > Reviewing the stm32 series, i wonder if sync_flags should be a pointer to a const structure > as the platform driver should not update it during the rproc live cycle. > Then IMO, using a pointer to the structure instead of the structure seems more > in line with the rest of the remoteproc API. Humm... If we do make sync_flags constant then the platform drivers can't modify the values dynamically, as I did in the stm32 series. This is something Loic had asked for. Moreover function rproc_set_state_machine() can't be called twice so updating the sync_flags can't happen. > > >> +{ > >> + if (!rproc || !sync_ops) > >> + return -EINVAL; > >> + > >> + /* > >> + * No point in going further if we never have to synchronise with > >> + * the remote processor. > >> + */ > >> + if (!sync_flags.on_init && > >> + !sync_flags.after_stop && !sync_flags.after_crash) > >> + return 0; > >> + > >> + /* > >> + * Refuse to go further if remoteproc operations have been allocated > >> + * but they will never be used. > >> + */ > >> + if (rproc->ops && sync_flags.on_init && > >> + sync_flags.after_stop && sync_flags.after_crash) > >> + return -EINVAL; > >> + > >> + /* > >> + * Don't allow users to set this more than once to avoid situations > >> + * where the remote processor can't be recovered. > >> + */ > >> + if (rproc->sync_ops) > >> + return -EINVAL; > >> + > >> + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); > >> + if (!rproc->sync_ops) > >> + return -ENOMEM; > >> + > >> + rproc->sync_flags = sync_flags; > >> + /* Tell the core what to do when initialising */ > >> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); > > > > Is there a use case where sync_flags.on_init is false and other flags are true? > > > > Look like on_init is useless and should not be exposed to the platform driver. > > Or comments are missing to explain the usage of it vs the other flags. > > > > Regards, > > Arnaud > > > >> + > >> + return 0; > >> +} > >> +EXPORT_SYMBOL(rproc_set_state_machine); > >> + > >> /** > >> * rproc_type_release() - release a remote processor instance > >> * @dev: the rproc's device > >> @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) > >> kfree_const(rproc->firmware); > >> kfree_const(rproc->name); > >> kfree(rproc->ops); > >> + kfree(rproc->sync_ops); > >> kfree(rproc); > >> } > >> > >> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > >> index 7dcc0a26892b..c1a293a37c78 100644 > >> --- a/drivers/remoteproc/remoteproc_internal.h > >> +++ b/drivers/remoteproc/remoteproc_internal.h > >> @@ -27,6 +27,8 @@ struct rproc_debug_trace { > >> /* > >> * enum rproc_sync_states - remote processsor sync states > >> * > >> + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core > >> + * is initialising. > >> * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core > >> * has shutdown (rproc_shutdown()) the > >> * remote processor. > >> @@ -39,6 +41,7 @@ struct rproc_debug_trace { > >> * operation to use. > >> */ > >> enum rproc_sync_states { > >> + RPROC_SYNC_STATE_INIT, > >> RPROC_SYNC_STATE_SHUTDOWN, > >> RPROC_SYNC_STATE_CRASHED, > >> }; > >> @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, > >> enum rproc_sync_states state) > >> { > >> switch (state) { > >> + case RPROC_SYNC_STATE_INIT: > >> + rproc->sync_with_rproc = rproc->sync_flags.on_init; > >> + break; > >> case RPROC_SYNC_STATE_SHUTDOWN: > >> rproc->sync_with_rproc = rproc->sync_flags.after_stop; > >> break; > >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >> index ceb3b2bba824..a75ed92b3de6 100644 > >> --- a/include/linux/remoteproc.h > >> +++ b/include/linux/remoteproc.h > >> @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); > >> struct rproc *rproc_alloc(struct device *dev, const char *name, > >> const struct rproc_ops *ops, > >> const char *firmware, int len); > >> +int rproc_set_state_machine(struct rproc *rproc, > >> + const struct rproc_ops *sync_ops, > >> + struct rproc_sync_flags sync_flags); > >> void rproc_put(struct rproc *rproc); > >> int rproc_add(struct rproc *rproc); > >> int rproc_del(struct rproc *rproc); > >>
On 4/30/20 10:42 PM, Mathieu Poirier wrote: > On Wed, Apr 29, 2020 at 11:22:28AM +0200, Arnaud POULIQUEN wrote: >> >> >> On 4/24/20 10:01 PM, Mathieu Poirier wrote: >>> Introducting function rproc_set_state_machine() to add >>> operations and a set of flags to use when synchronising with >>> a remote processor. >>> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>> --- >>> drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ >>> drivers/remoteproc/remoteproc_internal.h | 6 +++ >>> include/linux/remoteproc.h | 3 ++ >>> 3 files changed, 63 insertions(+) >>> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >>> index 48afa1f80a8f..5c48714e8702 100644 >>> --- a/drivers/remoteproc/remoteproc_core.c >>> +++ b/drivers/remoteproc/remoteproc_core.c >>> @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) >>> } >>> EXPORT_SYMBOL(devm_rproc_add); >>> >>> +/** >>> + * rproc_set_state_machine() - Set a synchronisation ops and set of flags >>> + * to use with a remote processor >>> + * @rproc: The remote processor to work with >>> + * @sync_ops: The operations to use when synchronising with a remote >>> + * processor >>> + * @sync_flags: The flags to use when deciding if the remoteproc core >>> + * should be synchronising with a remote processor >>> + * >>> + * Returns 0 on success, an error code otherwise. >>> + */ >>> +int rproc_set_state_machine(struct rproc *rproc, >>> + const struct rproc_ops *sync_ops, >>> + struct rproc_sync_flags sync_flags) >> >> So this API should be called by platform driver only in case of synchronization >> support, right? > > Correct > >> In this case i would rename it as there is also a state machine in "normal" boot >> proposal: rproc_set_sync_machine or rproc_set_sync_state_machine > > That is a valid observation - rproc_set_sync_state_machine() sounds descriptive > enough for me. > >> >>> +{ >>> + if (!rproc || !sync_ops) >>> + return -EINVAL; >>> + >>> + /* >>> + * No point in going further if we never have to synchronise with >>> + * the remote processor. >>> + */ >>> + if (!sync_flags.on_init && >>> + !sync_flags.after_stop && !sync_flags.after_crash) >>> + return 0; >>> + >>> + /* >>> + * Refuse to go further if remoteproc operations have been allocated >>> + * but they will never be used. >>> + */ >>> + if (rproc->ops && sync_flags.on_init && >>> + sync_flags.after_stop && sync_flags.after_crash) >>> + return -EINVAL; >>> + >>> + /* >>> + * Don't allow users to set this more than once to avoid situations >>> + * where the remote processor can't be recovered. >>> + */ >>> + if (rproc->sync_ops) >>> + return -EINVAL; >>> + >>> + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); >>> + if (!rproc->sync_ops) >>> + return -ENOMEM; >>> + >>> + rproc->sync_flags = sync_flags; >>> + /* Tell the core what to do when initialising */ >>> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); >> >> Is there a use case where sync_flags.on_init is false and other flags are true? > > I haven't seen one yet, which doesn't mean it doesn't exist or won't in the > future. I wanted to make this as flexible as possible. I started with the idea > of making synchronisation at initialisation time implicit if > rproc_set_state_machine() is called but I know it is only a matter of time > before people come up with some exotic use case where .on_init is false. So having on_init false but after_crash && after_stop true, means loading the firmware on first start, and the synchronize with it, right? Yes probably could be an exotic valid use case. :) > >> >> Look like on_init is useless and should not be exposed to the platform driver. >> Or comments are missing to explain the usage of it vs the other flags. > > Comments added in remoteproc_internal.h and the new section in > Documentation/remoteproc.txt aren't sufficient? Can you give me a hint as to > what you think is missing? IMO something is quite confusing... On one side on_init can be set to false. But on the other side the flag is set by call rproc_set_state_machine. In Documentation/remoteproc.txt rproc_set_state_machine description is: "This function should be called for cases where the remote processor has been started by another entity, be it a boot loader or trusted environment, and the remoteproc core is to synchronise with the remote processor rather then boot it." So how on_init could be false if "the remote processor has been started by another entity"? Regards, Arnaud > >> >> Regards, >> Arnaud >> >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(rproc_set_state_machine); >>> + >>> /** >>> * rproc_type_release() - release a remote processor instance >>> * @dev: the rproc's device >>> @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) >>> kfree_const(rproc->firmware); >>> kfree_const(rproc->name); >>> kfree(rproc->ops); >>> + kfree(rproc->sync_ops); >>> kfree(rproc); >>> } >>> >>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h >>> index 7dcc0a26892b..c1a293a37c78 100644 >>> --- a/drivers/remoteproc/remoteproc_internal.h >>> +++ b/drivers/remoteproc/remoteproc_internal.h >>> @@ -27,6 +27,8 @@ struct rproc_debug_trace { >>> /* >>> * enum rproc_sync_states - remote processsor sync states >>> * >>> + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core >>> + * is initialising. >>> * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core >>> * has shutdown (rproc_shutdown()) the >>> * remote processor. >>> @@ -39,6 +41,7 @@ struct rproc_debug_trace { >>> * operation to use. >>> */ >>> enum rproc_sync_states { >>> + RPROC_SYNC_STATE_INIT, >>> RPROC_SYNC_STATE_SHUTDOWN, >>> RPROC_SYNC_STATE_CRASHED, >>> }; >>> @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, >>> enum rproc_sync_states state) >>> { >>> switch (state) { >>> + case RPROC_SYNC_STATE_INIT: >>> + rproc->sync_with_rproc = rproc->sync_flags.on_init; >>> + break; >>> case RPROC_SYNC_STATE_SHUTDOWN: >>> rproc->sync_with_rproc = rproc->sync_flags.after_stop; >>> break; >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>> index ceb3b2bba824..a75ed92b3de6 100644 >>> --- a/include/linux/remoteproc.h >>> +++ b/include/linux/remoteproc.h >>> @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); >>> struct rproc *rproc_alloc(struct device *dev, const char *name, >>> const struct rproc_ops *ops, >>> const char *firmware, int len); >>> +int rproc_set_state_machine(struct rproc *rproc, >>> + const struct rproc_ops *sync_ops, >>> + struct rproc_sync_flags sync_flags); >>> void rproc_put(struct rproc *rproc); >>> int rproc_add(struct rproc *rproc); >>> int rproc_del(struct rproc *rproc); >>>
On 4/30/20 10:51 PM, Mathieu Poirier wrote: > On Wed, Apr 29, 2020 at 04:38:54PM +0200, Arnaud POULIQUEN wrote: >> >> >> On 4/29/20 11:22 AM, Arnaud POULIQUEN wrote: >>> >>> >>> On 4/24/20 10:01 PM, Mathieu Poirier wrote: >>>> Introducting function rproc_set_state_machine() to add >>>> operations and a set of flags to use when synchronising with >>>> a remote processor. >>>> >>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>> --- >>>> drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ >>>> drivers/remoteproc/remoteproc_internal.h | 6 +++ >>>> include/linux/remoteproc.h | 3 ++ >>>> 3 files changed, 63 insertions(+) >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >>>> index 48afa1f80a8f..5c48714e8702 100644 >>>> --- a/drivers/remoteproc/remoteproc_core.c >>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>> @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) >>>> } >>>> EXPORT_SYMBOL(devm_rproc_add); >>>> >>>> +/** >>>> + * rproc_set_state_machine() - Set a synchronisation ops and set of flags >>>> + * to use with a remote processor >>>> + * @rproc: The remote processor to work with >>>> + * @sync_ops: The operations to use when synchronising with a remote >>>> + * processor >>>> + * @sync_flags: The flags to use when deciding if the remoteproc core >>>> + * should be synchronising with a remote processor >>>> + * >>>> + * Returns 0 on success, an error code otherwise. >>>> + */ >>>> +int rproc_set_state_machine(struct rproc *rproc, >>>> + const struct rproc_ops *sync_ops, >>>> + struct rproc_sync_flags sync_flags) >>> >>> So this API should be called by platform driver only in case of synchronization >>> support, right? >>> In this case i would rename it as there is also a state machine in "normal" boot >>> proposal: rproc_set_sync_machine or rproc_set_sync_state_machine >>> >> >> Reviewing the stm32 series, i wonder if sync_flags should be a pointer to a const structure >> as the platform driver should not update it during the rproc live cycle. >> Then IMO, using a pointer to the structure instead of the structure seems more >> in line with the rest of the remoteproc API. > > Humm... If we do make sync_flags constant then the platform drivers can't modify > the values dynamically, as I did in the stm32 series. This is something Loic > had asked for. > > Moreover function rproc_set_state_machine() can't be called twice so updating > the sync_flags can't happen. You are right, make it constant is not a good idea. Regards, Arnaud > >> >>>> +{ >>>> + if (!rproc || !sync_ops) >>>> + return -EINVAL; >>>> + >>>> + /* >>>> + * No point in going further if we never have to synchronise with >>>> + * the remote processor. >>>> + */ >>>> + if (!sync_flags.on_init && >>>> + !sync_flags.after_stop && !sync_flags.after_crash) >>>> + return 0; >>>> + >>>> + /* >>>> + * Refuse to go further if remoteproc operations have been allocated >>>> + * but they will never be used. >>>> + */ >>>> + if (rproc->ops && sync_flags.on_init && >>>> + sync_flags.after_stop && sync_flags.after_crash) >>>> + return -EINVAL; >>>> + >>>> + /* >>>> + * Don't allow users to set this more than once to avoid situations >>>> + * where the remote processor can't be recovered. >>>> + */ >>>> + if (rproc->sync_ops) >>>> + return -EINVAL; >>>> + >>>> + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); >>>> + if (!rproc->sync_ops) >>>> + return -ENOMEM; >>>> + >>>> + rproc->sync_flags = sync_flags; >>>> + /* Tell the core what to do when initialising */ >>>> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); >>> >>> Is there a use case where sync_flags.on_init is false and other flags are true? >>> >>> Look like on_init is useless and should not be exposed to the platform driver. >>> Or comments are missing to explain the usage of it vs the other flags. >>> >>> Regards, >>> Arnaud >>> >>>> + >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(rproc_set_state_machine); >>>> + >>>> /** >>>> * rproc_type_release() - release a remote processor instance >>>> * @dev: the rproc's device >>>> @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) >>>> kfree_const(rproc->firmware); >>>> kfree_const(rproc->name); >>>> kfree(rproc->ops); >>>> + kfree(rproc->sync_ops); >>>> kfree(rproc); >>>> } >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h >>>> index 7dcc0a26892b..c1a293a37c78 100644 >>>> --- a/drivers/remoteproc/remoteproc_internal.h >>>> +++ b/drivers/remoteproc/remoteproc_internal.h >>>> @@ -27,6 +27,8 @@ struct rproc_debug_trace { >>>> /* >>>> * enum rproc_sync_states - remote processsor sync states >>>> * >>>> + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core >>>> + * is initialising. >>>> * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core >>>> * has shutdown (rproc_shutdown()) the >>>> * remote processor. >>>> @@ -39,6 +41,7 @@ struct rproc_debug_trace { >>>> * operation to use. >>>> */ >>>> enum rproc_sync_states { >>>> + RPROC_SYNC_STATE_INIT, >>>> RPROC_SYNC_STATE_SHUTDOWN, >>>> RPROC_SYNC_STATE_CRASHED, >>>> }; >>>> @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, >>>> enum rproc_sync_states state) >>>> { >>>> switch (state) { >>>> + case RPROC_SYNC_STATE_INIT: >>>> + rproc->sync_with_rproc = rproc->sync_flags.on_init; >>>> + break; >>>> case RPROC_SYNC_STATE_SHUTDOWN: >>>> rproc->sync_with_rproc = rproc->sync_flags.after_stop; >>>> break; >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>> index ceb3b2bba824..a75ed92b3de6 100644 >>>> --- a/include/linux/remoteproc.h >>>> +++ b/include/linux/remoteproc.h >>>> @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); >>>> struct rproc *rproc_alloc(struct device *dev, const char *name, >>>> const struct rproc_ops *ops, >>>> const char *firmware, int len); >>>> +int rproc_set_state_machine(struct rproc *rproc, >>>> + const struct rproc_ops *sync_ops, >>>> + struct rproc_sync_flags sync_flags); >>>> void rproc_put(struct rproc *rproc); >>>> int rproc_add(struct rproc *rproc); >>>> int rproc_del(struct rproc *rproc); >>>>
On Mon, May 04, 2020 at 01:57:59PM +0200, Arnaud POULIQUEN wrote: > > > On 4/30/20 10:42 PM, Mathieu Poirier wrote: > > On Wed, Apr 29, 2020 at 11:22:28AM +0200, Arnaud POULIQUEN wrote: > >> > >> > >> On 4/24/20 10:01 PM, Mathieu Poirier wrote: > >>> Introducting function rproc_set_state_machine() to add > >>> operations and a set of flags to use when synchronising with > >>> a remote processor. > >>> > >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >>> --- > >>> drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ > >>> drivers/remoteproc/remoteproc_internal.h | 6 +++ > >>> include/linux/remoteproc.h | 3 ++ > >>> 3 files changed, 63 insertions(+) > >>> > >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > >>> index 48afa1f80a8f..5c48714e8702 100644 > >>> --- a/drivers/remoteproc/remoteproc_core.c > >>> +++ b/drivers/remoteproc/remoteproc_core.c > >>> @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) > >>> } > >>> EXPORT_SYMBOL(devm_rproc_add); > >>> > >>> +/** > >>> + * rproc_set_state_machine() - Set a synchronisation ops and set of flags > >>> + * to use with a remote processor > >>> + * @rproc: The remote processor to work with > >>> + * @sync_ops: The operations to use when synchronising with a remote > >>> + * processor > >>> + * @sync_flags: The flags to use when deciding if the remoteproc core > >>> + * should be synchronising with a remote processor > >>> + * > >>> + * Returns 0 on success, an error code otherwise. > >>> + */ > >>> +int rproc_set_state_machine(struct rproc *rproc, > >>> + const struct rproc_ops *sync_ops, > >>> + struct rproc_sync_flags sync_flags) > >> > >> So this API should be called by platform driver only in case of synchronization > >> support, right? > > > > Correct > > > >> In this case i would rename it as there is also a state machine in "normal" boot > >> proposal: rproc_set_sync_machine or rproc_set_sync_state_machine > > > > That is a valid observation - rproc_set_sync_state_machine() sounds descriptive > > enough for me. > > > >> > >>> +{ > >>> + if (!rproc || !sync_ops) > >>> + return -EINVAL; > >>> + > >>> + /* > >>> + * No point in going further if we never have to synchronise with > >>> + * the remote processor. > >>> + */ > >>> + if (!sync_flags.on_init && > >>> + !sync_flags.after_stop && !sync_flags.after_crash) > >>> + return 0; > >>> + > >>> + /* > >>> + * Refuse to go further if remoteproc operations have been allocated > >>> + * but they will never be used. > >>> + */ > >>> + if (rproc->ops && sync_flags.on_init && > >>> + sync_flags.after_stop && sync_flags.after_crash) > >>> + return -EINVAL; > >>> + > >>> + /* > >>> + * Don't allow users to set this more than once to avoid situations > >>> + * where the remote processor can't be recovered. > >>> + */ > >>> + if (rproc->sync_ops) > >>> + return -EINVAL; > >>> + > >>> + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); > >>> + if (!rproc->sync_ops) > >>> + return -ENOMEM; > >>> + > >>> + rproc->sync_flags = sync_flags; > >>> + /* Tell the core what to do when initialising */ > >>> + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); > >> > >> Is there a use case where sync_flags.on_init is false and other flags are true? > > > > I haven't seen one yet, which doesn't mean it doesn't exist or won't in the > > future. I wanted to make this as flexible as possible. I started with the idea > > of making synchronisation at initialisation time implicit if > > rproc_set_state_machine() is called but I know it is only a matter of time > > before people come up with some exotic use case where .on_init is false. > > So having on_init false but after_crash && after_stop true, means loading the > firmware on first start, and the synchronize with it, right? > Yes probably could be an exotic valid use case. :) > > > > >> > >> Look like on_init is useless and should not be exposed to the platform driver. > >> Or comments are missing to explain the usage of it vs the other flags. > > > > Comments added in remoteproc_internal.h and the new section in > > Documentation/remoteproc.txt aren't sufficient? Can you give me a hint as to > > what you think is missing? > > IMO something is quite confusing... > On one side on_init can be set to false. > But on the other side the flag is set by call rproc_set_state_machine. > In Documentation/remoteproc.txt rproc_set_state_machine description is: > > "This function should be called for cases where the remote processor has > been started by another entity, be it a boot loader or trusted environment, > and the remoteproc core is to synchronise with the remote processor rather > then boot it." > > So how on_init could be false if "the remote processor has > been started by another entity"? I see your point and I think it is a question of documentation. I will rephrase this to be more accurate. > > Regards, > Arnaud > > > > >> > >> Regards, > >> Arnaud > >> > >>> + > >>> + return 0; > >>> +} > >>> +EXPORT_SYMBOL(rproc_set_state_machine); > >>> + > >>> /** > >>> * rproc_type_release() - release a remote processor instance > >>> * @dev: the rproc's device > >>> @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) > >>> kfree_const(rproc->firmware); > >>> kfree_const(rproc->name); > >>> kfree(rproc->ops); > >>> + kfree(rproc->sync_ops); > >>> kfree(rproc); > >>> } > >>> > >>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > >>> index 7dcc0a26892b..c1a293a37c78 100644 > >>> --- a/drivers/remoteproc/remoteproc_internal.h > >>> +++ b/drivers/remoteproc/remoteproc_internal.h > >>> @@ -27,6 +27,8 @@ struct rproc_debug_trace { > >>> /* > >>> * enum rproc_sync_states - remote processsor sync states > >>> * > >>> + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core > >>> + * is initialising. > >>> * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core > >>> * has shutdown (rproc_shutdown()) the > >>> * remote processor. > >>> @@ -39,6 +41,7 @@ struct rproc_debug_trace { > >>> * operation to use. > >>> */ > >>> enum rproc_sync_states { > >>> + RPROC_SYNC_STATE_INIT, > >>> RPROC_SYNC_STATE_SHUTDOWN, > >>> RPROC_SYNC_STATE_CRASHED, > >>> }; > >>> @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, > >>> enum rproc_sync_states state) > >>> { > >>> switch (state) { > >>> + case RPROC_SYNC_STATE_INIT: > >>> + rproc->sync_with_rproc = rproc->sync_flags.on_init; > >>> + break; > >>> case RPROC_SYNC_STATE_SHUTDOWN: > >>> rproc->sync_with_rproc = rproc->sync_flags.after_stop; > >>> break; > >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >>> index ceb3b2bba824..a75ed92b3de6 100644 > >>> --- a/include/linux/remoteproc.h > >>> +++ b/include/linux/remoteproc.h > >>> @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); > >>> struct rproc *rproc_alloc(struct device *dev, const char *name, > >>> const struct rproc_ops *ops, > >>> const char *firmware, int len); > >>> +int rproc_set_state_machine(struct rproc *rproc, > >>> + const struct rproc_ops *sync_ops, > >>> + struct rproc_sync_flags sync_flags); > >>> void rproc_put(struct rproc *rproc); > >>> int rproc_add(struct rproc *rproc); > >>> int rproc_del(struct rproc *rproc); > >>>
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 48afa1f80a8f..5c48714e8702 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2065,6 +2065,59 @@ int devm_rproc_add(struct device *dev, struct rproc *rproc) } EXPORT_SYMBOL(devm_rproc_add); +/** + * rproc_set_state_machine() - Set a synchronisation ops and set of flags + * to use with a remote processor + * @rproc: The remote processor to work with + * @sync_ops: The operations to use when synchronising with a remote + * processor + * @sync_flags: The flags to use when deciding if the remoteproc core + * should be synchronising with a remote processor + * + * Returns 0 on success, an error code otherwise. + */ +int rproc_set_state_machine(struct rproc *rproc, + const struct rproc_ops *sync_ops, + struct rproc_sync_flags sync_flags) +{ + if (!rproc || !sync_ops) + return -EINVAL; + + /* + * No point in going further if we never have to synchronise with + * the remote processor. + */ + if (!sync_flags.on_init && + !sync_flags.after_stop && !sync_flags.after_crash) + return 0; + + /* + * Refuse to go further if remoteproc operations have been allocated + * but they will never be used. + */ + if (rproc->ops && sync_flags.on_init && + sync_flags.after_stop && sync_flags.after_crash) + return -EINVAL; + + /* + * Don't allow users to set this more than once to avoid situations + * where the remote processor can't be recovered. + */ + if (rproc->sync_ops) + return -EINVAL; + + rproc->sync_ops = kmemdup(sync_ops, sizeof(*sync_ops), GFP_KERNEL); + if (!rproc->sync_ops) + return -ENOMEM; + + rproc->sync_flags = sync_flags; + /* Tell the core what to do when initialising */ + rproc_set_sync_flag(rproc, RPROC_SYNC_STATE_INIT); + + return 0; +} +EXPORT_SYMBOL(rproc_set_state_machine); + /** * rproc_type_release() - release a remote processor instance * @dev: the rproc's device @@ -2088,6 +2141,7 @@ static void rproc_type_release(struct device *dev) kfree_const(rproc->firmware); kfree_const(rproc->name); kfree(rproc->ops); + kfree(rproc->sync_ops); kfree(rproc); } diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h index 7dcc0a26892b..c1a293a37c78 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -27,6 +27,8 @@ struct rproc_debug_trace { /* * enum rproc_sync_states - remote processsor sync states * + * @RPROC_SYNC_STATE_INIT state to use when the remoteproc core + * is initialising. * @RPROC_SYNC_STATE_SHUTDOWN state to use after the remoteproc core * has shutdown (rproc_shutdown()) the * remote processor. @@ -39,6 +41,7 @@ struct rproc_debug_trace { * operation to use. */ enum rproc_sync_states { + RPROC_SYNC_STATE_INIT, RPROC_SYNC_STATE_SHUTDOWN, RPROC_SYNC_STATE_CRASHED, }; @@ -47,6 +50,9 @@ static inline void rproc_set_sync_flag(struct rproc *rproc, enum rproc_sync_states state) { switch (state) { + case RPROC_SYNC_STATE_INIT: + rproc->sync_with_rproc = rproc->sync_flags.on_init; + break; case RPROC_SYNC_STATE_SHUTDOWN: rproc->sync_with_rproc = rproc->sync_flags.after_stop; break; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index ceb3b2bba824..a75ed92b3de6 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -619,6 +619,9 @@ struct rproc *rproc_get_by_child(struct device *dev); struct rproc *rproc_alloc(struct device *dev, const char *name, const struct rproc_ops *ops, const char *firmware, int len); +int rproc_set_state_machine(struct rproc *rproc, + const struct rproc_ops *sync_ops, + struct rproc_sync_flags sync_flags); void rproc_put(struct rproc *rproc); int rproc_add(struct rproc *rproc); int rproc_del(struct rproc *rproc);
Introducting function rproc_set_state_machine() to add operations and a set of flags to use when synchronising with a remote processor. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++++++ drivers/remoteproc/remoteproc_internal.h | 6 +++ include/linux/remoteproc.h | 3 ++ 3 files changed, 63 insertions(+)