[v3,12/14] remoteproc: Introducing function rproc_set_state_machine()
diff mbox series

Message ID 20200424200135.28825-13-mathieu.poirier@linaro.org
State New
Headers show
Series
  • remoteproc: Add support for synchronisaton with rproc
Related show

Commit Message

Mathieu Poirier April 24, 2020, 8:01 p.m. UTC
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(+)

Comments

Arnaud POULIQUEN April 29, 2020, 9:22 a.m. UTC | #1
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);
>
Arnaud POULIQUEN April 29, 2020, 2:38 p.m. UTC | #2
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);
>>
Mathieu Poirier April 30, 2020, 8:42 p.m. UTC | #3
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);
> >
Mathieu Poirier April 30, 2020, 8:51 p.m. UTC | #4
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);
> >>
Arnaud POULIQUEN May 4, 2020, 11:57 a.m. UTC | #5
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);
>>>
Arnaud POULIQUEN May 4, 2020, noon UTC | #6
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);
>>>>
Mathieu Poirier May 5, 2020, 9:43 p.m. UTC | #7
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);
> >>>

Patch
diff mbox series

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);