[v2,07/17] remoteproc: Introduce function rproc_alloc_state_machine()
diff mbox series

Message ID 20200324214603.14979-8-mathieu.poirier@linaro.org
State New
Headers show
Series
  • remoteproc: Add support for synchronisation with MCU
Related show

Commit Message

Mathieu Poirier March 24, 2020, 9:45 p.m. UTC
Introducing new function rproc_alloc_state_machine() to allocate
the MCU synchronisation operations and position it as the central
remoteproc core allocation function.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 84 +++++++++++++++++++++++++---
 include/linux/remoteproc.h           |  5 ++
 2 files changed, 81 insertions(+), 8 deletions(-)

Comments

Loic PALLARDY March 27, 2020, 1:12 p.m. UTC | #1
> -----Original Message-----
> From: Mathieu Poirier <mathieu.poirier@linaro.org>
> Sent: mardi 24 mars 2020 22:46
> To: bjorn.andersson@linaro.org
> Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s-
> anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN
> <arnaud.pouliquen@st.com>; Fabien DESSENNE
> <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org
> Subject: [PATCH v2 07/17] remoteproc: Introduce function
> rproc_alloc_state_machine()
> 
> Introducing new function rproc_alloc_state_machine() to allocate
> the MCU synchronisation operations and position it as the central
> remoteproc core allocation function.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 84
> +++++++++++++++++++++++++---
>  include/linux/remoteproc.h           |  5 ++
>  2 files changed, 81 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 9da245734db6..02dbb826aa29 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1954,6 +1954,7 @@ static void rproc_type_release(struct device *dev)
> 
>  	kfree(rproc->firmware);
>  	kfree(rproc->ops);
> +	kfree(rproc->sync_ops);
>  	kfree(rproc);
>  }
> 
> @@ -2018,12 +2019,34 @@ static int rproc_alloc_ops(struct rproc *rproc,
> const struct rproc_ops *ops)
>  	return 0;
>  }
> 
> +static int rproc_alloc_sync_ops(struct rproc *rproc,
> +				const struct rproc_ops *sync_ops)
> +{
> +	/*
> +	 * Given the unlimited amount of possibilities when
> +	 * synchronising with an MCU, no constraints are imposed
> +	 * on sync_ops.
> +	 */
> +	rproc->sync_ops = kmemdup(sync_ops,
> +				  sizeof(*sync_ops), GFP_KERNEL);
> +	if (!rproc->sync_ops)
> +		return -ENOMEM;
Should we check a minimal set of functions in sync_ops to be required?
Or we should consider all pointers at NULL is ok ?

> +
> +	return 0;
> +}
> +
>  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
>  				 const struct rproc_ops *boot_ops,
> +				 const struct rproc_ops *sync_ops,
> +				 struct rproc_sync_states *sync_states,
sync_states not used in this patch, should be introduced in patch 8

Regards,
Loic

>  				 const char *firmware, int len)
>  {
>  	int ret;
> 
> +	/* We need at least a boot or a sync ops. */
> +	if (!boot_ops && !sync_ops)
> +		return -EINVAL;
> +
>  	/* We have a boot_ops so allocate firmware name and operations */
>  	if (boot_ops) {
>  		ret = rproc_alloc_firmware(rproc, name, firmware);
> @@ -2035,14 +2058,23 @@ static int rproc_alloc_internals(struct rproc
> *rproc, const char *name,
>  			return ret;
>  	}
> 
> +	/* Allocate a sync_ops if need be */
> +	if (sync_ops) {
> +		ret = rproc_alloc_sync_ops(rproc, sync_ops);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
> 
>  /**
> - * rproc_alloc() - allocate a remote processor handle
> + * rproc_alloc_state_machine() - allocate a remote processor handle
>   * @dev: the underlying device
>   * @name: name of this remote processor
>   * @ops: platform-specific handlers (mainly start/stop)
> + * @sync_ops: platform-specific handlers for synchronising with MCU
> + * @sync_states: states in which @ops and @sync_ops are to be used
>   * @firmware: name of firmware file to load, can be NULL
>   * @len: length of private data needed by the rproc driver (in bytes)
>   *
> @@ -2061,13 +2093,15 @@ static int rproc_alloc_internals(struct rproc
> *rproc, const char *name,
>   * Note: _never_ directly deallocate @rproc, even if it was not registered
>   * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
>   */
> -struct rproc *rproc_alloc(struct device *dev, const char *name,
> -			  const struct rproc_ops *ops,
> -			  const char *firmware, int len)
> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> *name,
> +					const struct rproc_ops *ops,
> +					const struct rproc_ops *sync_ops,
> +					struct rproc_sync_states
> *sync_states,
> +					const char *firmware, int len)
>  {
>  	struct rproc *rproc;
> 
> -	if (!dev || !name || !ops)
> +	if (!dev || !name)
>  		return NULL;
> 
>  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> @@ -2084,8 +2118,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> char *name,
>  	rproc->dev.class = &rproc_class;
>  	rproc->dev.driver_data = rproc;
> 
> -	if (rproc_alloc_internals(rproc, name, ops,
> -				  firmware, len))
> +	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
> +				  sync_states, firmware, len))
>  		goto out;
> 
>  	/* Assign a unique device index and name */
> @@ -2119,7 +2153,41 @@ struct rproc *rproc_alloc(struct device *dev, const
> char *name,
>  	put_device(&rproc->dev);
>  	return NULL;
>  }
> -EXPORT_SYMBOL(rproc_alloc);
> +EXPORT_SYMBOL(rproc_alloc_state_machine);
> +
> +/**
> + * rproc_alloc() - allocate a remote processor handle
> + * @dev: the underlying device
> + * @name: name of this remote processor
> + * @ops: platform-specific handlers (mainly start/stop)
> + * @firmware: name of firmware file to load, can be NULL
> + * @len: length of private data needed by the rproc driver (in bytes)
> + *
> + * Allocates a new remote processor handle, but does not register
> + * it yet. if @firmware is NULL, a default name is used.
> + *
> + * This function should be used by rproc implementations during
> initialization
> + * of the remote processor.
> + *
> + * After creating an rproc handle using this function, and when ready,
> + * implementations should then call rproc_add() to complete
> + * the registration of the remote processor.
> + *
> + * On success the new rproc is returned, and on failure, NULL.
> + *
> + * Note: _never_ directly deallocate @rproc, even if it was not registered
> + * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
> + */
> +struct rproc *rproc_alloc(struct device *dev, const char *name,
> +			  const struct rproc_ops *ops,
> +			  const char *firmware, int len)
> +{
> +	if (!name && !firmware)
> +		return NULL;
> +
> +	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
> +					 firmware, len);
> +}
> 
>  /**
>   * rproc_free() - unroll rproc_alloc()
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index d115e47d702d..d1214487daac 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -611,6 +611,11 @@ 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);
> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> *name,
> +					const struct rproc_ops *ops,
> +					const struct rproc_ops *sync_ops,
> +					struct rproc_sync_states
> *sync_states,
> +					const char *firmware, int len);
>  void rproc_put(struct rproc *rproc);
>  int rproc_add(struct rproc *rproc);
>  int rproc_del(struct rproc *rproc);
> --
> 2.20.1
Suman Anna March 30, 2020, 11:10 p.m. UTC | #2
Hi Mathieu,
> 
>> -----Original Message-----
>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Sent: mardi 24 mars 2020 22:46
>> To: bjorn.andersson@linaro.org
>> Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s-
>> anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN
>> <arnaud.pouliquen@st.com>; Fabien DESSENNE
>> <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org
>> Subject: [PATCH v2 07/17] remoteproc: Introduce function
>> rproc_alloc_state_machine()
>>
>> Introducing new function rproc_alloc_state_machine() to allocate
>> the MCU synchronisation operations and position it as the central
>> remoteproc core allocation function.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 84
>> +++++++++++++++++++++++++---
>>  include/linux/remoteproc.h           |  5 ++
>>  2 files changed, 81 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index 9da245734db6..02dbb826aa29 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1954,6 +1954,7 @@ static void rproc_type_release(struct device *dev)
>>
>>  	kfree(rproc->firmware);
>>  	kfree(rproc->ops);
>> +	kfree(rproc->sync_ops);
>>  	kfree(rproc);
>>  }
>>
>> @@ -2018,12 +2019,34 @@ static int rproc_alloc_ops(struct rproc *rproc,
>> const struct rproc_ops *ops)
>>  	return 0;
>>  }
>>
>> +static int rproc_alloc_sync_ops(struct rproc *rproc,
>> +				const struct rproc_ops *sync_ops)
>> +{
>> +	/*
>> +	 * Given the unlimited amount of possibilities when
>> +	 * synchronising with an MCU, no constraints are imposed
>> +	 * on sync_ops.
>> +	 */
>> +	rproc->sync_ops = kmemdup(sync_ops,
>> +				  sizeof(*sync_ops), GFP_KERNEL);
>> +	if (!rproc->sync_ops)
>> +		return -ENOMEM;
> Should we check a minimal set of functions in sync_ops to be required?
> Or we should consider all pointers at NULL is ok ?
> 
>> +
>> +	return 0;
>> +}
>> +
>>  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
>>  				 const struct rproc_ops *boot_ops,
>> +				 const struct rproc_ops *sync_ops,
>> +				 struct rproc_sync_states *sync_states,
> sync_states not used in this patch, should be introduced in patch 8

+1

> 
> Regards,
> Loic
> 
>>  				 const char *firmware, int len)
>>  {
>>  	int ret;
>>
>> +	/* We need at least a boot or a sync ops. */
>> +	if (!boot_ops && !sync_ops)
>> +		return -EINVAL;
>> +
>>  	/* We have a boot_ops so allocate firmware name and operations */
>>  	if (boot_ops) {
>>  		ret = rproc_alloc_firmware(rproc, name, firmware);
>> @@ -2035,14 +2058,23 @@ static int rproc_alloc_internals(struct rproc
>> *rproc, const char *name,
>>  			return ret;
>>  	}
>>
>> +	/* Allocate a sync_ops if need be */
>> +	if (sync_ops) {
>> +		ret = rproc_alloc_sync_ops(rproc, sync_ops);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>
>>  /**
>> - * rproc_alloc() - allocate a remote processor handle
>> + * rproc_alloc_state_machine() - allocate a remote processor handle
>>   * @dev: the underlying device
>>   * @name: name of this remote processor
>>   * @ops: platform-specific handlers (mainly start/stop)
>> + * @sync_ops: platform-specific handlers for synchronising with MCU
>> + * @sync_states: states in which @ops and @sync_ops are to be used
>>   * @firmware: name of firmware file to load, can be NULL
>>   * @len: length of private data needed by the rproc driver (in bytes)
>>   *
>> @@ -2061,13 +2093,15 @@ static int rproc_alloc_internals(struct rproc
>> *rproc, const char *name,
>>   * Note: _never_ directly deallocate @rproc, even if it was not registered
>>   * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
>>   */
>> -struct rproc *rproc_alloc(struct device *dev, const char *name,
>> -			  const struct rproc_ops *ops,
>> -			  const char *firmware, int len)
>> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
>> *name,
>> +					const struct rproc_ops *ops,
>> +					const struct rproc_ops *sync_ops,
>> +					struct rproc_sync_states
>> *sync_states,
>> +					const char *firmware, int len)

Do you foresee the need for sync_ops to be present as long as the rproc
is registered? I am wondering if it is better to introduce an API where
you can set the ops at runtime rather than allocate it upfront, so that
once the initial handling is done, you can reset both the sync_states
and ops.


>>  {
>>  	struct rproc *rproc;
>>
>> -	if (!dev || !name || !ops)
>> +	if (!dev || !name)
>>  		return NULL;
>>
>>  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
>> @@ -2084,8 +2118,8 @@ struct rproc *rproc_alloc(struct device *dev, const
>> char *name,
>>  	rproc->dev.class = &rproc_class;
>>  	rproc->dev.driver_data = rproc;
>>
>> -	if (rproc_alloc_internals(rproc, name, ops,
>> -				  firmware, len))
>> +	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
>> +				  sync_states, firmware, len))
>>  		goto out;
>>
>>  	/* Assign a unique device index and name */
>> @@ -2119,7 +2153,41 @@ struct rproc *rproc_alloc(struct device *dev, const
>> char *name,
>>  	put_device(&rproc->dev);
>>  	return NULL;
>>  }
>> -EXPORT_SYMBOL(rproc_alloc);
>> +EXPORT_SYMBOL(rproc_alloc_state_machine);
>> +
>> +/**
>> + * rproc_alloc() - allocate a remote processor handle
>> + * @dev: the underlying device
>> + * @name: name of this remote processor
>> + * @ops: platform-specific handlers (mainly start/stop)
>> + * @firmware: name of firmware file to load, can be NULL
>> + * @len: length of private data needed by the rproc driver (in bytes)
>> + *
>> + * Allocates a new remote processor handle, but does not register
>> + * it yet. if @firmware is NULL, a default name is used.
>> + *
>> + * This function should be used by rproc implementations during
>> initialization
>> + * of the remote processor.
>> + *
>> + * After creating an rproc handle using this function, and when ready,
>> + * implementations should then call rproc_add() to complete
>> + * the registration of the remote processor.
>> + *
>> + * On success the new rproc is returned, and on failure, NULL.
>> + *
>> + * Note: _never_ directly deallocate @rproc, even if it was not registered
>> + * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
>> + */
>> +struct rproc *rproc_alloc(struct device *dev, const char *name,
>> +			  const struct rproc_ops *ops,
>> +			  const char *firmware, int len)
>> +{
>> +	if (!name && !firmware)

Retain the original checks on dev, name and ops from the previous
rproc_alloc(). A NULL firmware was perfectly valid before, and the name
is allocated using the default template.

>> +		return NULL;
>> +
>> +	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
>> +					 firmware, len);
>> +}

Missing the EXPORT_SYMBOL on rproc_alloc() -> it is an API used by modules.

regards
Suman

>>
>>  /**
>>   * rproc_free() - unroll rproc_alloc()
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index d115e47d702d..d1214487daac 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -611,6 +611,11 @@ 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);
>> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
>> *name,
>> +					const struct rproc_ops *ops,
>> +					const struct rproc_ops *sync_ops,
>> +					struct rproc_sync_states
>> *sync_states,
>> +					const char *firmware, int len);
>>  void rproc_put(struct rproc *rproc);
>>  int rproc_add(struct rproc *rproc);
>>  int rproc_del(struct rproc *rproc);
>> --
>> 2.20.1
>
Mathieu Poirier March 30, 2020, 11:13 p.m. UTC | #3
On Fri, Mar 27, 2020 at 01:12:11PM +0000, Loic PALLARDY wrote:
> 
> 
> > -----Original Message-----
> > From: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Sent: mardi 24 mars 2020 22:46
> > To: bjorn.andersson@linaro.org
> > Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s-
> > anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN
> > <arnaud.pouliquen@st.com>; Fabien DESSENNE
> > <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org
> > Subject: [PATCH v2 07/17] remoteproc: Introduce function
> > rproc_alloc_state_machine()
> > 
> > Introducing new function rproc_alloc_state_machine() to allocate
> > the MCU synchronisation operations and position it as the central
> > remoteproc core allocation function.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 84
> > +++++++++++++++++++++++++---
> >  include/linux/remoteproc.h           |  5 ++
> >  2 files changed, 81 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > index 9da245734db6..02dbb826aa29 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1954,6 +1954,7 @@ static void rproc_type_release(struct device *dev)
> > 
> >  	kfree(rproc->firmware);
> >  	kfree(rproc->ops);
> > +	kfree(rproc->sync_ops);
> >  	kfree(rproc);
> >  }
> > 
> > @@ -2018,12 +2019,34 @@ static int rproc_alloc_ops(struct rproc *rproc,
> > const struct rproc_ops *ops)
> >  	return 0;
> >  }
> > 
> > +static int rproc_alloc_sync_ops(struct rproc *rproc,
> > +				const struct rproc_ops *sync_ops)
> > +{
> > +	/*
> > +	 * Given the unlimited amount of possibilities when
> > +	 * synchronising with an MCU, no constraints are imposed
> > +	 * on sync_ops.
> > +	 */
> > +	rproc->sync_ops = kmemdup(sync_ops,
> > +				  sizeof(*sync_ops), GFP_KERNEL);
> > +	if (!rproc->sync_ops)
> > +		return -ENOMEM;
> Should we check a minimal set of functions in sync_ops to be required?
> Or we should consider all pointers at NULL is ok ?

I spent a fair amount of time thinking about that very question... The problem
is that requirements are all over the place and I did not want to introduce
constraints.  I am highly influenceable on this one - ideas welcomed.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> >  				 const struct rproc_ops *boot_ops,
> > +				 const struct rproc_ops *sync_ops,
> > +				 struct rproc_sync_states *sync_states,
> sync_states not used in this patch, should be introduced in patch 8

Very well.

> 
> Regards,
> Loic
> 
> >  				 const char *firmware, int len)
> >  {
> >  	int ret;
> > 
> > +	/* We need at least a boot or a sync ops. */
> > +	if (!boot_ops && !sync_ops)
> > +		return -EINVAL;
> > +
> >  	/* We have a boot_ops so allocate firmware name and operations */
> >  	if (boot_ops) {
> >  		ret = rproc_alloc_firmware(rproc, name, firmware);
> > @@ -2035,14 +2058,23 @@ static int rproc_alloc_internals(struct rproc
> > *rproc, const char *name,
> >  			return ret;
> >  	}
> > 
> > +	/* Allocate a sync_ops if need be */
> > +	if (sync_ops) {
> > +		ret = rproc_alloc_sync_ops(rproc, sync_ops);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> > 
> >  /**
> > - * rproc_alloc() - allocate a remote processor handle
> > + * rproc_alloc_state_machine() - allocate a remote processor handle
> >   * @dev: the underlying device
> >   * @name: name of this remote processor
> >   * @ops: platform-specific handlers (mainly start/stop)
> > + * @sync_ops: platform-specific handlers for synchronising with MCU
> > + * @sync_states: states in which @ops and @sync_ops are to be used
> >   * @firmware: name of firmware file to load, can be NULL
> >   * @len: length of private data needed by the rproc driver (in bytes)
> >   *
> > @@ -2061,13 +2093,15 @@ static int rproc_alloc_internals(struct rproc
> > *rproc, const char *name,
> >   * Note: _never_ directly deallocate @rproc, even if it was not registered
> >   * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
> >   */
> > -struct rproc *rproc_alloc(struct device *dev, const char *name,
> > -			  const struct rproc_ops *ops,
> > -			  const char *firmware, int len)
> > +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> > *name,
> > +					const struct rproc_ops *ops,
> > +					const struct rproc_ops *sync_ops,
> > +					struct rproc_sync_states
> > *sync_states,
> > +					const char *firmware, int len)
> >  {
> >  	struct rproc *rproc;
> > 
> > -	if (!dev || !name || !ops)
> > +	if (!dev || !name)
> >  		return NULL;
> > 
> >  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> > @@ -2084,8 +2118,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> > char *name,
> >  	rproc->dev.class = &rproc_class;
> >  	rproc->dev.driver_data = rproc;
> > 
> > -	if (rproc_alloc_internals(rproc, name, ops,
> > -				  firmware, len))
> > +	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
> > +				  sync_states, firmware, len))
> >  		goto out;
> > 
> >  	/* Assign a unique device index and name */
> > @@ -2119,7 +2153,41 @@ struct rproc *rproc_alloc(struct device *dev, const
> > char *name,
> >  	put_device(&rproc->dev);
> >  	return NULL;
> >  }
> > -EXPORT_SYMBOL(rproc_alloc);
> > +EXPORT_SYMBOL(rproc_alloc_state_machine);
> > +
> > +/**
> > + * rproc_alloc() - allocate a remote processor handle
> > + * @dev: the underlying device
> > + * @name: name of this remote processor
> > + * @ops: platform-specific handlers (mainly start/stop)
> > + * @firmware: name of firmware file to load, can be NULL
> > + * @len: length of private data needed by the rproc driver (in bytes)
> > + *
> > + * Allocates a new remote processor handle, but does not register
> > + * it yet. if @firmware is NULL, a default name is used.
> > + *
> > + * This function should be used by rproc implementations during
> > initialization
> > + * of the remote processor.
> > + *
> > + * After creating an rproc handle using this function, and when ready,
> > + * implementations should then call rproc_add() to complete
> > + * the registration of the remote processor.
> > + *
> > + * On success the new rproc is returned, and on failure, NULL.
> > + *
> > + * Note: _never_ directly deallocate @rproc, even if it was not registered
> > + * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
> > + */
> > +struct rproc *rproc_alloc(struct device *dev, const char *name,
> > +			  const struct rproc_ops *ops,
> > +			  const char *firmware, int len)
> > +{
> > +	if (!name && !firmware)
> > +		return NULL;
> > +
> > +	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
> > +					 firmware, len);
> > +}
> > 
> >  /**
> >   * rproc_free() - unroll rproc_alloc()
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index d115e47d702d..d1214487daac 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -611,6 +611,11 @@ 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);
> > +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> > *name,
> > +					const struct rproc_ops *ops,
> > +					const struct rproc_ops *sync_ops,
> > +					struct rproc_sync_states
> > *sync_states,
> > +					const char *firmware, int len);
> >  void rproc_put(struct rproc *rproc);
> >  int rproc_add(struct rproc *rproc);
> >  int rproc_del(struct rproc *rproc);
> > --
> > 2.20.1
>
Mathieu Poirier April 1, 2020, 8:41 p.m. UTC | #4
On Mon, Mar 30, 2020 at 06:10:51PM -0500, Suman Anna wrote:
> Hi Mathieu,
> > 
> >> -----Original Message-----
> >> From: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> Sent: mardi 24 mars 2020 22:46
> >> To: bjorn.andersson@linaro.org
> >> Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s-
> >> anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN
> >> <arnaud.pouliquen@st.com>; Fabien DESSENNE
> >> <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org
> >> Subject: [PATCH v2 07/17] remoteproc: Introduce function
> >> rproc_alloc_state_machine()
> >>
> >> Introducing new function rproc_alloc_state_machine() to allocate
> >> the MCU synchronisation operations and position it as the central
> >> remoteproc core allocation function.
> >>
> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c | 84
> >> +++++++++++++++++++++++++---
> >>  include/linux/remoteproc.h           |  5 ++
> >>  2 files changed, 81 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index 9da245734db6..02dbb826aa29 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1954,6 +1954,7 @@ static void rproc_type_release(struct device *dev)
> >>
> >>  	kfree(rproc->firmware);
> >>  	kfree(rproc->ops);
> >> +	kfree(rproc->sync_ops);
> >>  	kfree(rproc);
> >>  }
> >>
> >> @@ -2018,12 +2019,34 @@ static int rproc_alloc_ops(struct rproc *rproc,
> >> const struct rproc_ops *ops)
> >>  	return 0;
> >>  }
> >>
> >> +static int rproc_alloc_sync_ops(struct rproc *rproc,
> >> +				const struct rproc_ops *sync_ops)
> >> +{
> >> +	/*
> >> +	 * Given the unlimited amount of possibilities when
> >> +	 * synchronising with an MCU, no constraints are imposed
> >> +	 * on sync_ops.
> >> +	 */
> >> +	rproc->sync_ops = kmemdup(sync_ops,
> >> +				  sizeof(*sync_ops), GFP_KERNEL);
> >> +	if (!rproc->sync_ops)
> >> +		return -ENOMEM;
> > Should we check a minimal set of functions in sync_ops to be required?
> > Or we should consider all pointers at NULL is ok ?
> > 
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> >>  				 const struct rproc_ops *boot_ops,
> >> +				 const struct rproc_ops *sync_ops,
> >> +				 struct rproc_sync_states *sync_states,
> > sync_states not used in this patch, should be introduced in patch 8
> 
> +1

I will do that.

> 
> > 
> > Regards,
> > Loic
> > 
> >>  				 const char *firmware, int len)
> >>  {
> >>  	int ret;
> >>
> >> +	/* We need at least a boot or a sync ops. */
> >> +	if (!boot_ops && !sync_ops)
> >> +		return -EINVAL;
> >> +
> >>  	/* We have a boot_ops so allocate firmware name and operations */
> >>  	if (boot_ops) {
> >>  		ret = rproc_alloc_firmware(rproc, name, firmware);
> >> @@ -2035,14 +2058,23 @@ static int rproc_alloc_internals(struct rproc
> >> *rproc, const char *name,
> >>  			return ret;
> >>  	}
> >>
> >> +	/* Allocate a sync_ops if need be */
> >> +	if (sync_ops) {
> >> +		ret = rproc_alloc_sync_ops(rproc, sync_ops);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >>
> >>  /**
> >> - * rproc_alloc() - allocate a remote processor handle
> >> + * rproc_alloc_state_machine() - allocate a remote processor handle
> >>   * @dev: the underlying device
> >>   * @name: name of this remote processor
> >>   * @ops: platform-specific handlers (mainly start/stop)
> >> + * @sync_ops: platform-specific handlers for synchronising with MCU
> >> + * @sync_states: states in which @ops and @sync_ops are to be used
> >>   * @firmware: name of firmware file to load, can be NULL
> >>   * @len: length of private data needed by the rproc driver (in bytes)
> >>   *
> >> @@ -2061,13 +2093,15 @@ static int rproc_alloc_internals(struct rproc
> >> *rproc, const char *name,
> >>   * Note: _never_ directly deallocate @rproc, even if it was not registered
> >>   * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
> >>   */
> >> -struct rproc *rproc_alloc(struct device *dev, const char *name,
> >> -			  const struct rproc_ops *ops,
> >> -			  const char *firmware, int len)
> >> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> >> *name,
> >> +					const struct rproc_ops *ops,
> >> +					const struct rproc_ops *sync_ops,
> >> +					struct rproc_sync_states
> >> *sync_states,
> >> +					const char *firmware, int len)
> 
> Do you foresee the need for sync_ops to be present as long as the rproc
> is registered? I am wondering if it is better to introduce an API where
> you can set the ops at runtime rather than allocate it upfront, so that
> once the initial handling is done, you can reset both the sync_states
> and ops.

That is something I spent a fair amount of time thinking about.  I decided to
proceed with an upfront allocation scheme and see if people would come up
with needs that would mandate a runtime allocation.  I am willing to provide the
API if there is a need for it but would rather not if someone "may" need it. 

> 
> 
> >>  {
> >>  	struct rproc *rproc;
> >>
> >> -	if (!dev || !name || !ops)
> >> +	if (!dev || !name)
> >>  		return NULL;
> >>
> >>  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> >> @@ -2084,8 +2118,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> >> char *name,
> >>  	rproc->dev.class = &rproc_class;
> >>  	rproc->dev.driver_data = rproc;
> >>
> >> -	if (rproc_alloc_internals(rproc, name, ops,
> >> -				  firmware, len))
> >> +	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
> >> +				  sync_states, firmware, len))
> >>  		goto out;
> >>
> >>  	/* Assign a unique device index and name */
> >> @@ -2119,7 +2153,41 @@ struct rproc *rproc_alloc(struct device *dev, const
> >> char *name,
> >>  	put_device(&rproc->dev);
> >>  	return NULL;
> >>  }
> >> -EXPORT_SYMBOL(rproc_alloc);
> >> +EXPORT_SYMBOL(rproc_alloc_state_machine);
> >> +
> >> +/**
> >> + * rproc_alloc() - allocate a remote processor handle
> >> + * @dev: the underlying device
> >> + * @name: name of this remote processor
> >> + * @ops: platform-specific handlers (mainly start/stop)
> >> + * @firmware: name of firmware file to load, can be NULL
> >> + * @len: length of private data needed by the rproc driver (in bytes)
> >> + *
> >> + * Allocates a new remote processor handle, but does not register
> >> + * it yet. if @firmware is NULL, a default name is used.
> >> + *
> >> + * This function should be used by rproc implementations during
> >> initialization
> >> + * of the remote processor.
> >> + *
> >> + * After creating an rproc handle using this function, and when ready,
> >> + * implementations should then call rproc_add() to complete
> >> + * the registration of the remote processor.
> >> + *
> >> + * On success the new rproc is returned, and on failure, NULL.
> >> + *
> >> + * Note: _never_ directly deallocate @rproc, even if it was not registered
> >> + * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
> >> + */
> >> +struct rproc *rproc_alloc(struct device *dev, const char *name,
> >> +			  const struct rproc_ops *ops,
> >> +			  const char *firmware, int len)
> >> +{
> >> +	if (!name && !firmware)
> 
> Retain the original checks on dev, name and ops from the previous
> rproc_alloc(). A NULL firmware was perfectly valid before, and the name
> is allocated using the default template.

Here firmware can be NULL, but we need a name in order to construct the default
one.  On the flip side ops can be NULL for scenarios where the remoteproc core
is never in charge of the MCU lifecycle.  As for dev, it is checked in
rproc_alloc_state_machine().

> 
> >> +		return NULL;
> >> +
> >> +	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
> >> +					 firmware, len);
> >> +}
> 
> Missing the EXPORT_SYMBOL on rproc_alloc() -> it is an API used by modules.

Of course yes, good catch.

> 
> regards
> Suman
> 
> >>
> >>  /**
> >>   * rproc_free() - unroll rproc_alloc()
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index d115e47d702d..d1214487daac 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -611,6 +611,11 @@ 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);
> >> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
> >> *name,
> >> +					const struct rproc_ops *ops,
> >> +					const struct rproc_ops *sync_ops,
> >> +					struct rproc_sync_states
> >> *sync_states,
> >> +					const char *firmware, int len);
> >>  void rproc_put(struct rproc *rproc);
> >>  int rproc_add(struct rproc *rproc);
> >>  int rproc_del(struct rproc *rproc);
> >> --
> >> 2.20.1
> > 
>
Suman Anna April 9, 2020, 6:35 p.m. UTC | #5
On 4/1/20 3:41 PM, Mathieu Poirier wrote:
> On Mon, Mar 30, 2020 at 06:10:51PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>>
>>>> -----Original Message-----
>>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Sent: mardi 24 mars 2020 22:46
>>>> To: bjorn.andersson@linaro.org
>>>> Cc: ohad@wizery.com; Loic PALLARDY <loic.pallardy@st.com>; s-
>>>> anna@ti.com; peng.fan@nxp.com; Arnaud POULIQUEN
>>>> <arnaud.pouliquen@st.com>; Fabien DESSENNE
>>>> <fabien.dessenne@st.com>; linux-remoteproc@vger.kernel.org
>>>> Subject: [PATCH v2 07/17] remoteproc: Introduce function
>>>> rproc_alloc_state_machine()
>>>>
>>>> Introducing new function rproc_alloc_state_machine() to allocate
>>>> the MCU synchronisation operations and position it as the central
>>>> remoteproc core allocation function.
>>>>
>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 84
>>>> +++++++++++++++++++++++++---
>>>>  include/linux/remoteproc.h           |  5 ++
>>>>  2 files changed, 81 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 9da245734db6..02dbb826aa29 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1954,6 +1954,7 @@ static void rproc_type_release(struct device *dev)
>>>>
>>>>  	kfree(rproc->firmware);
>>>>  	kfree(rproc->ops);
>>>> +	kfree(rproc->sync_ops);
>>>>  	kfree(rproc);
>>>>  }
>>>>
>>>> @@ -2018,12 +2019,34 @@ static int rproc_alloc_ops(struct rproc *rproc,
>>>> const struct rproc_ops *ops)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +static int rproc_alloc_sync_ops(struct rproc *rproc,
>>>> +				const struct rproc_ops *sync_ops)
>>>> +{
>>>> +	/*
>>>> +	 * Given the unlimited amount of possibilities when
>>>> +	 * synchronising with an MCU, no constraints are imposed
>>>> +	 * on sync_ops.
>>>> +	 */
>>>> +	rproc->sync_ops = kmemdup(sync_ops,
>>>> +				  sizeof(*sync_ops), GFP_KERNEL);
>>>> +	if (!rproc->sync_ops)
>>>> +		return -ENOMEM;
>>> Should we check a minimal set of functions in sync_ops to be required?
>>> Or we should consider all pointers at NULL is ok ?
>>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
>>>>  				 const struct rproc_ops *boot_ops,
>>>> +				 const struct rproc_ops *sync_ops,
>>>> +				 struct rproc_sync_states *sync_states,
>>> sync_states not used in this patch, should be introduced in patch 8
>>
>> +1
> 
> I will do that.
> 
>>
>>>
>>> Regards,
>>> Loic
>>>
>>>>  				 const char *firmware, int len)
>>>>  {
>>>>  	int ret;
>>>>
>>>> +	/* We need at least a boot or a sync ops. */
>>>> +	if (!boot_ops && !sync_ops)
>>>> +		return -EINVAL;
>>>> +
>>>>  	/* We have a boot_ops so allocate firmware name and operations */
>>>>  	if (boot_ops) {
>>>>  		ret = rproc_alloc_firmware(rproc, name, firmware);
>>>> @@ -2035,14 +2058,23 @@ static int rproc_alloc_internals(struct rproc
>>>> *rproc, const char *name,
>>>>  			return ret;
>>>>  	}
>>>>
>>>> +	/* Allocate a sync_ops if need be */
>>>> +	if (sync_ops) {
>>>> +		ret = rproc_alloc_sync_ops(rproc, sync_ops);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>  	return 0;
>>>>  }
>>>>
>>>>  /**
>>>> - * rproc_alloc() - allocate a remote processor handle
>>>> + * rproc_alloc_state_machine() - allocate a remote processor handle
>>>>   * @dev: the underlying device
>>>>   * @name: name of this remote processor
>>>>   * @ops: platform-specific handlers (mainly start/stop)
>>>> + * @sync_ops: platform-specific handlers for synchronising with MCU
>>>> + * @sync_states: states in which @ops and @sync_ops are to be used
>>>>   * @firmware: name of firmware file to load, can be NULL
>>>>   * @len: length of private data needed by the rproc driver (in bytes)
>>>>   *
>>>> @@ -2061,13 +2093,15 @@ static int rproc_alloc_internals(struct rproc
>>>> *rproc, const char *name,
>>>>   * Note: _never_ directly deallocate @rproc, even if it was not registered
>>>>   * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
>>>>   */
>>>> -struct rproc *rproc_alloc(struct device *dev, const char *name,
>>>> -			  const struct rproc_ops *ops,
>>>> -			  const char *firmware, int len)
>>>> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
>>>> *name,
>>>> +					const struct rproc_ops *ops,
>>>> +					const struct rproc_ops *sync_ops,
>>>> +					struct rproc_sync_states
>>>> *sync_states,
>>>> +					const char *firmware, int len)
>>
>> Do you foresee the need for sync_ops to be present as long as the rproc
>> is registered? I am wondering if it is better to introduce an API where
>> you can set the ops at runtime rather than allocate it upfront, so that
>> once the initial handling is done, you can reset both the sync_states
>> and ops.
> 
> That is something I spent a fair amount of time thinking about.  I decided to
> proceed with an upfront allocation scheme and see if people would come up
> with needs that would mandate a runtime allocation.  I am willing to provide the
> API if there is a need for it but would rather not if someone "may" need it. 

I think you will have a need of a variant of this atleast as you rework
the series for the comments on the cover-letter [1]

[1] https://patchwork.kernel.org/comment/23260081/

> 
>>
>>
>>>>  {
>>>>  	struct rproc *rproc;
>>>>
>>>> -	if (!dev || !name || !ops)
>>>> +	if (!dev || !name)
>>>>  		return NULL;
>>>>
>>>>  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
>>>> @@ -2084,8 +2118,8 @@ struct rproc *rproc_alloc(struct device *dev, const
>>>> char *name,
>>>>  	rproc->dev.class = &rproc_class;
>>>>  	rproc->dev.driver_data = rproc;
>>>>
>>>> -	if (rproc_alloc_internals(rproc, name, ops,
>>>> -				  firmware, len))
>>>> +	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
>>>> +				  sync_states, firmware, len))
>>>>  		goto out;
>>>>
>>>>  	/* Assign a unique device index and name */
>>>> @@ -2119,7 +2153,41 @@ struct rproc *rproc_alloc(struct device *dev, const
>>>> char *name,
>>>>  	put_device(&rproc->dev);
>>>>  	return NULL;
>>>>  }
>>>> -EXPORT_SYMBOL(rproc_alloc);
>>>> +EXPORT_SYMBOL(rproc_alloc_state_machine);
>>>> +
>>>> +/**
>>>> + * rproc_alloc() - allocate a remote processor handle
>>>> + * @dev: the underlying device
>>>> + * @name: name of this remote processor
>>>> + * @ops: platform-specific handlers (mainly start/stop)
>>>> + * @firmware: name of firmware file to load, can be NULL
>>>> + * @len: length of private data needed by the rproc driver (in bytes)
>>>> + *
>>>> + * Allocates a new remote processor handle, but does not register
>>>> + * it yet. if @firmware is NULL, a default name is used.
>>>> + *
>>>> + * This function should be used by rproc implementations during
>>>> initialization
>>>> + * of the remote processor.
>>>> + *
>>>> + * After creating an rproc handle using this function, and when ready,
>>>> + * implementations should then call rproc_add() to complete
>>>> + * the registration of the remote processor.
>>>> + *
>>>> + * On success the new rproc is returned, and on failure, NULL.
>>>> + *
>>>> + * Note: _never_ directly deallocate @rproc, even if it was not registered
>>>> + * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
>>>> + */
>>>> +struct rproc *rproc_alloc(struct device *dev, const char *name,
>>>> +			  const struct rproc_ops *ops,
>>>> +			  const char *firmware, int len)
>>>> +{
>>>> +	if (!name && !firmware)
>>
>> Retain the original checks on dev, name and ops from the previous
>> rproc_alloc(). A NULL firmware was perfectly valid before, and the name
>> is allocated using the default template.
> 
> Here firmware can be NULL, but we need a name in order to construct the default
> one.  On the flip side ops can be NULL for scenarios where the remoteproc core
> is never in charge of the MCU lifecycle.  As for dev, it is checked in
> rproc_alloc_state_machine().

So, rproc_alloc() continues to be the API for normal use-cases, and
anyone needing the sync lifecycle would be using
rproc_alloc_state_machine(). rproc_alloc() was originally checking for
dev, name, ops. You are checking for the first two at the beginning of
the new function. The ops check is now not done until
rproc_alloc_internals(). I don't see the advantage of the additional
conditional check here, it will just be simpler to check for NULL ops
here, or retain the original checks as they were for better readability
on the mandatory stuff on this function.

regards
Suman

> 
>>
>>>> +		return NULL;
>>>> +
>>>> +	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
>>>> +					 firmware, len);
>>>> +}
>>
>> Missing the EXPORT_SYMBOL on rproc_alloc() -> it is an API used by modules.
> 
> Of course yes, good catch.
> 
>>
>> regards
>> Suman
>>
>>>>
>>>>  /**
>>>>   * rproc_free() - unroll rproc_alloc()
>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>> index d115e47d702d..d1214487daac 100644
>>>> --- a/include/linux/remoteproc.h
>>>> +++ b/include/linux/remoteproc.h
>>>> @@ -611,6 +611,11 @@ 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);
>>>> +struct rproc *rproc_alloc_state_machine(struct device *dev, const char
>>>> *name,
>>>> +					const struct rproc_ops *ops,
>>>> +					const struct rproc_ops *sync_ops,
>>>> +					struct rproc_sync_states
>>>> *sync_states,
>>>> +					const char *firmware, int len);
>>>>  void rproc_put(struct rproc *rproc);
>>>>  int rproc_add(struct rproc *rproc);
>>>>  int rproc_del(struct rproc *rproc);
>>>> --
>>>> 2.20.1
>>>
>>

Patch
diff mbox series

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9da245734db6..02dbb826aa29 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1954,6 +1954,7 @@  static void rproc_type_release(struct device *dev)
 
 	kfree(rproc->firmware);
 	kfree(rproc->ops);
+	kfree(rproc->sync_ops);
 	kfree(rproc);
 }
 
@@ -2018,12 +2019,34 @@  static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
 	return 0;
 }
 
+static int rproc_alloc_sync_ops(struct rproc *rproc,
+				const struct rproc_ops *sync_ops)
+{
+	/*
+	 * Given the unlimited amount of possibilities when
+	 * synchronising with an MCU, no constraints are imposed
+	 * on sync_ops.
+	 */
+	rproc->sync_ops = kmemdup(sync_ops,
+				  sizeof(*sync_ops), GFP_KERNEL);
+	if (!rproc->sync_ops)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int rproc_alloc_internals(struct rproc *rproc, const char *name,
 				 const struct rproc_ops *boot_ops,
+				 const struct rproc_ops *sync_ops,
+				 struct rproc_sync_states *sync_states,
 				 const char *firmware, int len)
 {
 	int ret;
 
+	/* We need at least a boot or a sync ops. */
+	if (!boot_ops && !sync_ops)
+		return -EINVAL;
+
 	/* We have a boot_ops so allocate firmware name and operations */
 	if (boot_ops) {
 		ret = rproc_alloc_firmware(rproc, name, firmware);
@@ -2035,14 +2058,23 @@  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
 			return ret;
 	}
 
+	/* Allocate a sync_ops if need be */
+	if (sync_ops) {
+		ret = rproc_alloc_sync_ops(rproc, sync_ops);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
 /**
- * rproc_alloc() - allocate a remote processor handle
+ * rproc_alloc_state_machine() - allocate a remote processor handle
  * @dev: the underlying device
  * @name: name of this remote processor
  * @ops: platform-specific handlers (mainly start/stop)
+ * @sync_ops: platform-specific handlers for synchronising with MCU
+ * @sync_states: states in which @ops and @sync_ops are to be used
  * @firmware: name of firmware file to load, can be NULL
  * @len: length of private data needed by the rproc driver (in bytes)
  *
@@ -2061,13 +2093,15 @@  static int rproc_alloc_internals(struct rproc *rproc, const char *name,
  * Note: _never_ directly deallocate @rproc, even if it was not registered
  * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
  */
-struct rproc *rproc_alloc(struct device *dev, const char *name,
-			  const struct rproc_ops *ops,
-			  const char *firmware, int len)
+struct rproc *rproc_alloc_state_machine(struct device *dev, const char *name,
+					const struct rproc_ops *ops,
+					const struct rproc_ops *sync_ops,
+					struct rproc_sync_states *sync_states,
+					const char *firmware, int len)
 {
 	struct rproc *rproc;
 
-	if (!dev || !name || !ops)
+	if (!dev || !name)
 		return NULL;
 
 	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
@@ -2084,8 +2118,8 @@  struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->dev.class = &rproc_class;
 	rproc->dev.driver_data = rproc;
 
-	if (rproc_alloc_internals(rproc, name, ops,
-				  firmware, len))
+	if (rproc_alloc_internals(rproc, name, ops, sync_ops,
+				  sync_states, firmware, len))
 		goto out;
 
 	/* Assign a unique device index and name */
@@ -2119,7 +2153,41 @@  struct rproc *rproc_alloc(struct device *dev, const char *name,
 	put_device(&rproc->dev);
 	return NULL;
 }
-EXPORT_SYMBOL(rproc_alloc);
+EXPORT_SYMBOL(rproc_alloc_state_machine);
+
+/**
+ * rproc_alloc() - allocate a remote processor handle
+ * @dev: the underlying device
+ * @name: name of this remote processor
+ * @ops: platform-specific handlers (mainly start/stop)
+ * @firmware: name of firmware file to load, can be NULL
+ * @len: length of private data needed by the rproc driver (in bytes)
+ *
+ * Allocates a new remote processor handle, but does not register
+ * it yet. if @firmware is NULL, a default name is used.
+ *
+ * This function should be used by rproc implementations during initialization
+ * of the remote processor.
+ *
+ * After creating an rproc handle using this function, and when ready,
+ * implementations should then call rproc_add() to complete
+ * the registration of the remote processor.
+ *
+ * On success the new rproc is returned, and on failure, NULL.
+ *
+ * Note: _never_ directly deallocate @rproc, even if it was not registered
+ * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
+ */
+struct rproc *rproc_alloc(struct device *dev, const char *name,
+			  const struct rproc_ops *ops,
+			  const char *firmware, int len)
+{
+	if (!name && !firmware)
+		return NULL;
+
+	return rproc_alloc_state_machine(dev, name, ops, NULL, NULL,
+					 firmware, len);
+}
 
 /**
  * rproc_free() - unroll rproc_alloc()
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d115e47d702d..d1214487daac 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -611,6 +611,11 @@  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);
+struct rproc *rproc_alloc_state_machine(struct device *dev, const char *name,
+					const struct rproc_ops *ops,
+					const struct rproc_ops *sync_ops,
+					struct rproc_sync_states *sync_states,
+					const char *firmware, int len);
 void rproc_put(struct rproc *rproc);
 int rproc_add(struct rproc *rproc);
 int rproc_del(struct rproc *rproc);