[v2,06/17] remoteproc: Introduce function rproc_alloc_internals()
diff mbox series

Message ID 20200324214603.14979-7-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
In preparation to allocate the synchronisation operation and state
machine, spin off a new function in order to keep rproc_alloc() as
clean as possible.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Comments

Loic PALLARDY March 27, 2020, 11:10 a.m. UTC | #1
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 06/17] remoteproc: Introduce function
> rproc_alloc_internals()
> 
> In preparation to allocate the synchronisation operation and state
> machine, spin off a new function in order to keep rproc_alloc() as
> clean as possible.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
> -
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index ee277bc5556c..9da245734db6 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
> const struct rproc_ops *ops)
>  	return 0;
>  }
> 
> +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> +				 const struct rproc_ops *boot_ops,
> +				 const char *firmware, int len)

len argument is not used in the patch nor in the following, maybe removed from my pov.

Regards,
Loic
> +{
> +	int ret;
> +
> +	/* We have a boot_ops so allocate firmware name and operations */
> +	if (boot_ops) {
> +		ret = rproc_alloc_firmware(rproc, name, firmware);
> +		if (ret)
> +			return ret;
> +
> +		ret = rproc_alloc_ops(rproc, boot_ops);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> char *name,
>  	rproc->dev.class = &rproc_class;
>  	rproc->dev.driver_data = rproc;
> 
> -	if (rproc_alloc_firmware(rproc, name, firmware))
> -		goto out;
> -
> -	if (rproc_alloc_ops(rproc, ops))
> +	if (rproc_alloc_internals(rproc, name, ops,
> +				  firmware, len))
>  		goto out;
> 
>  	/* Assign a unique device index and name */
> --
> 2.20.1
Suman Anna March 30, 2020, 8:38 p.m. UTC | #2
Hi Mathieu,

On 3/27/20 6:10 AM, Loic PALLARDY wrote:
> Hi Mathieu,
> 
>>
>> In preparation to allocate the synchronisation operation and state
>> machine, spin off a new function in order to keep rproc_alloc() as
>> clean as possible.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
>> -
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index ee277bc5556c..9da245734db6 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
>> const struct rproc_ops *ops)
>>  	return 0;
>>  }
>>
>> +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
>> +				 const struct rproc_ops *boot_ops,
>> +				 const char *firmware, int len)
> 
> len argument is not used in the patch nor in the following, maybe removed from my pov.

Indeed.

> 
> Regards,
> Loic

>> +{
>> +	int ret;
>> +
>> +	/* We have a boot_ops so allocate firmware name and operations */
>> +	if (boot_ops) {
>> +		ret = rproc_alloc_firmware(rproc, name, firmware);
>> +		if (ret)
>> +			return ret;

So, can you explain why firmware allocation now becomes conditional on
this boot_ops?

Perhaps, continue to call this as ops following the field name in struct
rproc.

regards
Suman

>> +
>> +		ret = rproc_alloc_ops(rproc, boot_ops);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * rproc_alloc() - allocate a remote processor handle
>>   * @dev: the underlying device
>> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
>> char *name,
>>  	rproc->dev.class = &rproc_class;
>>  	rproc->dev.driver_data = rproc;
>>
>> -	if (rproc_alloc_firmware(rproc, name, firmware))
>> -		goto out;
>> -
>> -	if (rproc_alloc_ops(rproc, ops))
>> +	if (rproc_alloc_internals(rproc, name, ops,
>> +				  firmware, len))
>>  		goto out;
>>
>>  	/* Assign a unique device index and name */
>> --
>> 2.20.1
>
Mathieu Poirier March 30, 2020, 11:07 p.m. UTC | #3
On Fri, Mar 27, 2020 at 11:10:20AM +0000, Loic PALLARDY 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 06/17] remoteproc: Introduce function
> > rproc_alloc_internals()
> > 
> > In preparation to allocate the synchronisation operation and state
> > machine, spin off a new function in order to keep rproc_alloc() as
> > clean as possible.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
> > -
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > index ee277bc5556c..9da245734db6 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
> > const struct rproc_ops *ops)
> >  	return 0;
> >  }
> > 
> > +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> > +				 const struct rproc_ops *boot_ops,
> > +				 const char *firmware, int len)
> 
> len argument is not used in the patch nor in the following, maybe removed from my pov.

I debated over this one... It is either introduce the function signature as a
whole or incrementally as parameters are needed.  I'm fine with both and will
adopt the latter on the next revision.

> 
> Regards,
> Loic
> > +{
> > +	int ret;
> > +
> > +	/* We have a boot_ops so allocate firmware name and operations */
> > +	if (boot_ops) {
> > +		ret = rproc_alloc_firmware(rproc, name, firmware);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = rproc_alloc_ops(rproc, boot_ops);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * rproc_alloc() - allocate a remote processor handle
> >   * @dev: the underlying device
> > @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> > char *name,
> >  	rproc->dev.class = &rproc_class;
> >  	rproc->dev.driver_data = rproc;
> > 
> > -	if (rproc_alloc_firmware(rproc, name, firmware))
> > -		goto out;
> > -
> > -	if (rproc_alloc_ops(rproc, ops))
> > +	if (rproc_alloc_internals(rproc, name, ops,
> > +				  firmware, len))
> >  		goto out;
> > 
> >  	/* Assign a unique device index and name */
> > --
> > 2.20.1
>
Mathieu Poirier April 1, 2020, 8:29 p.m. UTC | #4
Hi Suman,

On Mon, Mar 30, 2020 at 03:38:14PM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 3/27/20 6:10 AM, Loic PALLARDY wrote:
> > Hi Mathieu,
> > 
> >>
> >> In preparation to allocate the synchronisation operation and state
> >> machine, spin off a new function in order to keep rproc_alloc() as
> >> clean as possible.
> >>
> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
> >> -
> >>  1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index ee277bc5556c..9da245734db6 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
> >> const struct rproc_ops *ops)
> >>  	return 0;
> >>  }
> >>
> >> +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
> >> +				 const struct rproc_ops *boot_ops,
> >> +				 const char *firmware, int len)
> > 
> > len argument is not used in the patch nor in the following, maybe removed from my pov.
> 
> Indeed.
> 
> > 
> > Regards,
> > Loic
> 
> >> +{
> >> +	int ret;
> >> +
> >> +	/* We have a boot_ops so allocate firmware name and operations */
> >> +	if (boot_ops) {
> >> +		ret = rproc_alloc_firmware(rproc, name, firmware);
> >> +		if (ret)
> >> +			return ret;
> 
> So, can you explain why firmware allocation now becomes conditional on
> this boot_ops?

There is no point in allocating a firmware name in a scenario where the
remoteproc core is only synchronising with the MCU.  As soon as a boot_ops (to
be renamed ops as per your comment below) is present I assume firmware loading
will be involved at some point.   Do you see a scenario where that wouldn't be
be case?

> 
> Perhaps, continue to call this as ops following the field name in struct
> rproc.

Ok

> 
> regards
> Suman
> 
> >> +
> >> +		ret = rproc_alloc_ops(rproc, boot_ops);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * rproc_alloc() - allocate a remote processor handle
> >>   * @dev: the underlying device
> >> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
> >> char *name,
> >>  	rproc->dev.class = &rproc_class;
> >>  	rproc->dev.driver_data = rproc;
> >>
> >> -	if (rproc_alloc_firmware(rproc, name, firmware))
> >> -		goto out;
> >> -
> >> -	if (rproc_alloc_ops(rproc, ops))
> >> +	if (rproc_alloc_internals(rproc, name, ops,
> >> +				  firmware, len))
> >>  		goto out;
> >>
> >>  	/* Assign a unique device index and name */
> >> --
> >> 2.20.1
> > 
>
Suman Anna April 9, 2020, 9:53 p.m. UTC | #5
On 4/1/20 3:29 PM, Mathieu Poirier wrote:
> Hi Suman,
> 
> On Mon, Mar 30, 2020 at 03:38:14PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 3/27/20 6:10 AM, Loic PALLARDY wrote:
>>> Hi Mathieu,
>>>
>>>>
>>>> In preparation to allocate the synchronisation operation and state
>>>> machine, spin off a new function in order to keep rproc_alloc() as
>>>> clean as possible.
>>>>
>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 26 ++++++++++++++++++++++---
>>>> -
>>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index ee277bc5556c..9da245734db6 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -2018,6 +2018,26 @@ static int rproc_alloc_ops(struct rproc *rproc,
>>>> const struct rproc_ops *ops)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +static int rproc_alloc_internals(struct rproc *rproc, const char *name,
>>>> +				 const struct rproc_ops *boot_ops,
>>>> +				 const char *firmware, int len)
>>>
>>> len argument is not used in the patch nor in the following, maybe removed from my pov.
>>
>> Indeed.
>>
>>>
>>> Regards,
>>> Loic
>>
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/* We have a boot_ops so allocate firmware name and operations */
>>>> +	if (boot_ops) {
>>>> +		ret = rproc_alloc_firmware(rproc, name, firmware);
>>>> +		if (ret)
>>>> +			return ret;
>>
>> So, can you explain why firmware allocation now becomes conditional on
>> this boot_ops?
> 
> There is no point in allocating a firmware name in a scenario where the
> remoteproc core is only synchronising with the MCU.  As soon as a boot_ops (to
> be renamed ops as per your comment below) is present I assume firmware loading
> will be involved at some point.   Do you see a scenario where that wouldn't be
> be case?

No. But that isn't until the whole sync stuff is introduced. As of this
patch, it it still code refactoring. And I would think that the cases
where only sync_ops will be used will be the minority.

regards
Suman

> 
>>
>> Perhaps, continue to call this as ops following the field name in struct
>> rproc.
> 
> Ok
> 
>>
>> regards
>> Suman
>>
>>>> +
>>>> +		ret = rproc_alloc_ops(rproc, boot_ops);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  /**
>>>>   * rproc_alloc() - allocate a remote processor handle
>>>>   * @dev: the underlying device
>>>> @@ -2064,10 +2084,8 @@ struct rproc *rproc_alloc(struct device *dev, const
>>>> char *name,
>>>>  	rproc->dev.class = &rproc_class;
>>>>  	rproc->dev.driver_data = rproc;
>>>>
>>>> -	if (rproc_alloc_firmware(rproc, name, firmware))
>>>> -		goto out;
>>>> -
>>>> -	if (rproc_alloc_ops(rproc, ops))
>>>> +	if (rproc_alloc_internals(rproc, name, ops,
>>>> +				  firmware, len))
>>>>  		goto out;
>>>>
>>>>  	/* Assign a unique device index and name */
>>>> --
>>>> 2.20.1
>>>
>>

Patch
diff mbox series

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ee277bc5556c..9da245734db6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2018,6 +2018,26 @@  static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
 	return 0;
 }
 
+static int rproc_alloc_internals(struct rproc *rproc, const char *name,
+				 const struct rproc_ops *boot_ops,
+				 const char *firmware, int len)
+{
+	int ret;
+
+	/* We have a boot_ops so allocate firmware name and operations */
+	if (boot_ops) {
+		ret = rproc_alloc_firmware(rproc, name, firmware);
+		if (ret)
+			return ret;
+
+		ret = rproc_alloc_ops(rproc, boot_ops);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * rproc_alloc() - allocate a remote processor handle
  * @dev: the underlying device
@@ -2064,10 +2084,8 @@  struct rproc *rproc_alloc(struct device *dev, const char *name,
 	rproc->dev.class = &rproc_class;
 	rproc->dev.driver_data = rproc;
 
-	if (rproc_alloc_firmware(rproc, name, firmware))
-		goto out;
-
-	if (rproc_alloc_ops(rproc, ops))
+	if (rproc_alloc_internals(rproc, name, ops,
+				  firmware, len))
 		goto out;
 
 	/* Assign a unique device index and name */