Date:   Mon, 16 Dec 2019 17:16:48 +0100 (CET)
From:   Clément Leger <cleger@...ray.eu>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     Fabien DESSENNE <fabien.dessenne@...com>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, od <od@...c.me>,
        linux-remoteproc <linux-remoteproc@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

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@...ray.eu wrote:

> Hi Paul
> 
> ----- On 14 Dec, 2019, at 23:30, Paul Cercueil paul@...pouillou.net wrote:
> 
>> Hi Fabien,
>> 
>> 
>> Le jeu., déc. 12, 2019 at 10:03, Fabien DESSENNE
>> <fabien.dessenne@...com> 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@...pouillou.net>
>>>>  ---
>>>> 
>>>>  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);

Powered by blists - more mailing lists