linux-kernel - Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1576362603.3.5@crapouillou.net>
Date:   Sat, 14 Dec 2019 23:30:03 +0100
From:   Paul Cercueil <paul@...pouillou.net>
To:     Fabien DESSENNE <fabien.dessenne@...com>
Cc:     Ohad Ben-Cohen <ohad@...ery.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, od@...c.me,
        linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/5] remoteproc: Add prepare/unprepare callbacks

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.

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);