Hi Paul,
I tested your patch and implemented the prepare callback only.
This is working correctly and the callback is correctly called
to enable clocks right before loading code into memory.
Regarding unprepare, I have no usecase for this since clocks
disabling and resetting is done in rproc_stop. Thus, I did not
tested this part. Did you add it to be called after the subdevices
unprepare call ? If not, what is the use case for this ?
ie, what are you doing in unprepare that you don't do in stop ?
Obviously it should work but I can't say I tested it completely !
Regards,
Clément
----- On 16 Dec, 2019, at 09:42, Clément Leger cleger@xxxxxxxxx wrote:
> Hi Paul
>
> ----- On 14 Dec, 2019, at 23:30, Paul Cercueil paul@xxxxxxxxxxxxxxx wrote:
>
>> Hi Fabien,
>>
>>
>> Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE
>> <fabien.dessenne@xxxxxx> a écrit :
>>> Hi Paul
>>>
>>>
>>> On 10/12/2019 5:40 PM, Paul Cercueil wrote:
>>>> The .prepare() callback is called before the firmware is loaded to
>>>> memory. This is useful for instance in the case where some setup is
>>>> required for the memory to be accessible.
>>>
>>>
>>> I am trying to figure out what king of 'setup' may be required. From
>>> the
>>> ingenic driver I understand that you need to enable clocks to allow
>>> some
>>> memory access.
>>>
>>> Instead of adding this new ops, why not enabling clocks in probe()?
>>
>> Enabling the clocks in the probe means that the clocks will be
>> unconditionally enabled until the driver is removed, even if the remote
>> processor end up being unused. That would be a waste of power.
>
> We have the same kind of "problem" for k1c remoteproc driver (not yet
> upstream, depends on new arch). We need to enable clocks to load code
> into remote processor memory and currently we do that in probe.
> However, as you stated, we would like to enable them as late as possible
> (just before loading code) to avoid wasting power unnecessarily. So the
> "prepare" callback totally makes sense.
>
> Regards,
>
> Clément
>
>>
>> Cheers,
>> -Paul
>>
>>
>>>
>>> BR
>>>
>>> Fabien
>>>
>>>
>>>>
>>>> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
>>>> ---
>>>>
>>>> Notes:
>>>> v2-v4: No change
>>>>
>>>> drivers/remoteproc/remoteproc_core.c | 16 +++++++++++++++-
>>>> include/linux/remoteproc.h | 4 ++++
>>>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 0a9fc7fdd1c3..3ea5f675a148 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1299,11 +1299,19 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>> struct device *dev = &rproc->dev;
>>>> int ret;
>>>>
>>>> + if (rproc->ops->prepare) {
>>>> + ret = rproc->ops->prepare(rproc);
>>>> + if (ret) {
>>>> + dev_err(dev, "Failed to prepare rproc: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> /* load the ELF segments to memory */
>>>> ret = rproc_load_segments(rproc, fw);
>>>> if (ret) {
>>>> dev_err(dev, "Failed to load program segments: %d\n", ret);
>>>> - return ret;
>>>> + goto unprepare_rproc;
>>>> }
>>>>
>>>> /*
>>>> @@ -1354,6 +1362,9 @@ static int rproc_start(struct rproc *rproc,
>>>> const struct firmware *fw)
>>>> rproc_unprepare_subdevices(rproc);
>>>> reset_table_ptr:
>>>> rproc->table_ptr = rproc->cached_table;
>>>> +unprepare_rproc:
>>>> + if (rproc->ops->unprepare)
>>>> + rproc->ops->unprepare(rproc);
>>>>
>>>> return ret;
>>>> }
>>>> @@ -1483,6 +1494,9 @@ static int rproc_stop(struct rproc *rproc,
>>>> bool crashed)
>>>>
>>>> rproc->state = RPROC_OFFLINE;
>>>>
>>>> + if (rproc->ops->unprepare)
>>>> + rproc->ops->unprepare(rproc);
>>>> +
>>>> dev_info(dev, "stopped remote processor %s\n", rproc->name);
>>>>
>>>> return 0;
>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>> index 5f201f0c86c3..a6272d1ba384 100644
>>>> --- a/include/linux/remoteproc.h
>>>> +++ b/include/linux/remoteproc.h
>>>> @@ -355,6 +355,8 @@ enum rsc_handling_status {
>>>>
>>>> /**
>>>> * struct rproc_ops - platform-specific device handlers
>>>> + * @prepare: prepare the device for power up (before the firmware
>>>> is loaded)
>>>> + * @unprepare: unprepare the device after it is stopped
>>>> * @start: power on the device and boot it
>>>> * @stop: power off the device
>>>> * @kick: kick a virtqueue (virtqueue id given as a parameter)
>>>> @@ -371,6 +373,8 @@ enum rsc_handling_status {
>>>> * @get_boot_addr: get boot address to entry point specified in
>>>> firmware
>>>> */
>>>> struct rproc_ops {
>>>> + int (*prepare)(struct rproc *rproc);
>>>> + void (*unprepare)(struct rproc *rproc);
>>>> int (*start)(struct rproc *rproc);
>>>> int (*stop)(struct rproc *rproc);
> > >> void (*kick)(struct rproc *rproc, int vqid);
