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@xxxxxxxxxx>
>>>> ---
>>>> 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
>>>
>>
