diff mbox series

[v3,08/13] remoteproc: add prepare and unprepare ops

Message ID 1519921440-21356-9-git-send-email-loic.pallardy@st.com
State New
Headers show
Series remoteproc: add fixed memory region support | expand

Commit Message

Loic Pallardy March 1, 2018, 4:23 p.m. UTC 
On some SoC architecture, it is needed to enable HW like
clock, bus, regulator, memory region... before loading
co-processor firmware.

This patch introduces prepare and unprepare ops to execute
platform specific function before firmware loading and after
stop execution.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

---
 drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-
 include/linux/remoteproc.h           |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
1.9.1

Comments

Bjorn Andersson May 10, 2018, 12:52 a.m. UTC | #1 
On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> On some SoC architecture, it is needed to enable HW like

> clock, bus, regulator, memory region... before loading

> co-processor firmware.

> 

> This patch introduces prepare and unprepare ops to execute

> platform specific function before firmware loading and after

> stop execution.

> 

> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

> ---

>  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-

>  include/linux/remoteproc.h           |  4 ++++

>  2 files changed, 23 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

> index 7a500cb..0ebbc4f 100644

> --- a/drivers/remoteproc/remoteproc_core.c

> +++ b/drivers/remoteproc/remoteproc_core.c

> @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)

>  		return ret;

>  	}

>  

> +	/* Prepare rproc for firmware loading if needed */

> +	if (rproc->ops->prepare) {

> +		ret = rproc->ops->prepare(rproc);

> +		if (ret) {

> +			dev_err(dev, "can't prepare rproc %s: %d\n",

> +				rproc->name, ret);

> +			goto disable_iommu;

> +		}

> +	}


We do allow drivers to implement custom versions of parse_fw() - and
they can call the resource-table-parse-fw from the custom function.

So with the proposed refactoring in patch 9 I would like for parse_fw()
to call back into the core to fill out the resource lists and then
before jumping to rproc_start() we loop over the allocator functions.

> +

>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);

>  

>  	/* load resource table */

>  	ret = rproc_load_rsc_table(rproc, fw);

>  	if (ret)

> -		goto disable_iommu;

> +		goto unprepare_rproc;

>  

>  	/* reset max_notifyid */

>  	rproc->max_notifyid = -1;

> @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)

>  	kfree(rproc->cached_table);

>  	rproc->cached_table = NULL;

>  	rproc->table_ptr = NULL;

> +unprepare_rproc:

> +	/* release HW resources if needed */

> +	if (rproc->ops->unprepare)

> +		rproc->ops->unprepare(rproc);

>  disable_iommu:

>  	rproc_disable_iommu(rproc);

>  	return ret;

> @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)

>  	/* clean up all acquired resources */

>  	rproc_resource_cleanup(rproc);

>  

> +	/* release HW resources if needed */

> +	if (rproc->ops->unprepare)

> +		rproc->ops->unprepare(rproc);


And this would then be handled by the rproc_resource_cleanup() function,
looping over all resources and calling release().

Regards,
Bjorn
Loic Pallardy May 14, 2018, 3:03 p.m. UTC | #2 
> -----Original Message-----

> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]

> Sent: Thursday, May 10, 2018 2:53 AM

> To: Loic PALLARDY <loic.pallardy@st.com>

> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-

> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;

> benjamin.gaignard@linaro.org

> Subject: Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops

> 

> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

> 

> > On some SoC architecture, it is needed to enable HW like

> > clock, bus, regulator, memory region... before loading

> > co-processor firmware.

> >

> > This patch introduces prepare and unprepare ops to execute

> > platform specific function before firmware loading and after

> > stop execution.

> >

> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

> > ---

> >  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-

> >  include/linux/remoteproc.h           |  4 ++++

> >  2 files changed, 23 insertions(+), 1 deletion(-)

> >

> > diff --git a/drivers/remoteproc/remoteproc_core.c

> b/drivers/remoteproc/remoteproc_core.c

> > index 7a500cb..0ebbc4f 100644

> > --- a/drivers/remoteproc/remoteproc_core.c

> > +++ b/drivers/remoteproc/remoteproc_core.c

> > @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc,

> const struct firmware *fw)

> >  		return ret;

> >  	}

> >

> > +	/* Prepare rproc for firmware loading if needed */

> > +	if (rproc->ops->prepare) {

> > +		ret = rproc->ops->prepare(rproc);

> > +		if (ret) {

> > +			dev_err(dev, "can't prepare rproc %s: %d\n",

> > +				rproc->name, ret);

> > +			goto disable_iommu;

> > +		}

> > +	}

> 

> We do allow drivers to implement custom versions of parse_fw() - and

> they can call the resource-table-parse-fw from the custom function.

> 

> So with the proposed refactoring in patch 9 I would like for parse_fw()

> to call back into the core to fill out the resource lists and then

> before jumping to rproc_start() we loop over the allocator functions.


Agree
Regards,
Loic
> 

> > +

> >  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);

> >

> >  	/* load resource table */

> >  	ret = rproc_load_rsc_table(rproc, fw);

> >  	if (ret)

> > -		goto disable_iommu;

> > +		goto unprepare_rproc;

> >

> >  	/* reset max_notifyid */

> >  	rproc->max_notifyid = -1;

> > @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc,

> const struct firmware *fw)

> >  	kfree(rproc->cached_table);

> >  	rproc->cached_table = NULL;

> >  	rproc->table_ptr = NULL;

> > +unprepare_rproc:

> > +	/* release HW resources if needed */

> > +	if (rproc->ops->unprepare)

> > +		rproc->ops->unprepare(rproc);

> >  disable_iommu:

> >  	rproc_disable_iommu(rproc);

> >  	return ret;

> > @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)

> >  	/* clean up all acquired resources */

> >  	rproc_resource_cleanup(rproc);

> >

> > +	/* release HW resources if needed */

> > +	if (rproc->ops->unprepare)

> > +		rproc->ops->unprepare(rproc);

> 

> And this would then be handled by the rproc_resource_cleanup() function,

> looping over all resources and calling release().

> 

> Regards,

> Bjorn
Suman Anna Oct. 24, 2018, 3:12 a.m. UTC | #3 
Hi Bjorn,

> >> -----Original Message-----

>> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]

>> Sent: Thursday, May 10, 2018 2:53 AM

>> To: Loic PALLARDY <loic.pallardy@st.com>

>> Cc: ohad@wizery.com; linux-remoteproc@vger.kernel.org; linux-

>> kernel@vger.kernel.org; Arnaud POULIQUEN <arnaud.pouliquen@st.com>;

>> benjamin.gaignard@linaro.org

>> Subject: Re: [PATCH v3 08/13] remoteproc: add prepare and unprepare ops

>>

>> On Thu 01 Mar 08:23 PST 2018, Loic Pallardy wrote:

>>

>>> On some SoC architecture, it is needed to enable HW like

>>> clock, bus, regulator, memory region... before loading

>>> co-processor firmware.

>>>

>>> This patch introduces prepare and unprepare ops to execute

>>> platform specific function before firmware loading and after

>>> stop execution.

>>>

>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

>>> ---

>>>  drivers/remoteproc/remoteproc_core.c | 20 +++++++++++++++++++-

>>>  include/linux/remoteproc.h           |  4 ++++

>>>  2 files changed, 23 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/remoteproc/remoteproc_core.c

>> b/drivers/remoteproc/remoteproc_core.c

>>> index 7a500cb..0ebbc4f 100644

>>> --- a/drivers/remoteproc/remoteproc_core.c

>>> +++ b/drivers/remoteproc/remoteproc_core.c

>>> @@ -1058,12 +1058,22 @@ static int rproc_fw_boot(struct rproc *rproc,

>> const struct firmware *fw)

>>>  		return ret;

>>>  	}

>>>

>>> +	/* Prepare rproc for firmware loading if needed */

>>> +	if (rproc->ops->prepare) {

>>> +		ret = rproc->ops->prepare(rproc);

>>> +		if (ret) {

>>> +			dev_err(dev, "can't prepare rproc %s: %d\n",

>>> +				rproc->name, ret);

>>> +			goto disable_iommu;

>>> +		}

>>> +	}

>>

>> We do allow drivers to implement custom versions of parse_fw() - and

>> they can call the resource-table-parse-fw from the custom function.

>>

>> So with the proposed refactoring in patch 9 I would like for parse_fw()

>> to call back into the core to fill out the resource lists and then

>> before jumping to rproc_start() we loop over the allocator functions.


I do like this patch and actually have a need for something similar for
supporting loading into internal memories for some R5F remote processors
on the latest TI SoCs. R5Fs have both resets and halt signals, and the
reset needs to be released to allow loading into TCMs, with the start
performing the halt. I am forced to do this between rproc_alloc() and
rproc_start() at the moment, but this really creates a mismatch between
my probe, start, stop and remove.

That said, I do agree with you that the way this was used with carveouts
should not have been used. Overloading the parse_fw to achieve above is
not right either. Please see my comments on Loic's v4 ST remoteproc
patch [1].

[1] https://patchwork.kernel.org/patch/10547153/

> 

> Agree

> Regards,

> Loic

>>

>>> +

>>>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);

>>>

>>>  	/* load resource table */

>>>  	ret = rproc_load_rsc_table(rproc, fw);

>>>  	if (ret)

>>> -		goto disable_iommu;

>>> +		goto unprepare_rproc;

>>>

>>>  	/* reset max_notifyid */

>>>  	rproc->max_notifyid = -1;

>>> @@ -1086,6 +1096,10 @@ static int rproc_fw_boot(struct rproc *rproc,

>> const struct firmware *fw)

>>>  	kfree(rproc->cached_table);

>>>  	rproc->cached_table = NULL;

>>>  	rproc->table_ptr = NULL;

>>> +unprepare_rproc:

>>> +	/* release HW resources if needed */

>>> +	if (rproc->ops->unprepare)

>>> +		rproc->ops->unprepare(rproc);

>>>  disable_iommu:

>>>  	rproc_disable_iommu(rproc);

>>>  	return ret;

>>> @@ -1331,6 +1345,10 @@ void rproc_shutdown(struct rproc *rproc)

>>>  	/* clean up all acquired resources */

>>>  	rproc_resource_cleanup(rproc);

>>>

>>> +	/* release HW resources if needed */

>>> +	if (rproc->ops->unprepare)

>>> +		rproc->ops->unprepare(rproc);

>>

>> And this would then be handled by the rproc_resource_cleanup() function,

>> looping over all resources and calling release().

>>

>> Regards,

>> Bjorn

> --

> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7a500cb..0ebbc4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1058,12 +1058,22 @@  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		return ret;
 	}
 
+	/* Prepare rproc for firmware loading if needed */
+	if (rproc->ops->prepare) {
+		ret = rproc->ops->prepare(rproc);
+		if (ret) {
+			dev_err(dev, "can't prepare rproc %s: %d\n",
+				rproc->name, ret);
+			goto disable_iommu;
+		}
+	}
+
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
 
 	/* load resource table */
 	ret = rproc_load_rsc_table(rproc, fw);
 	if (ret)
-		goto disable_iommu;
+		goto unprepare_rproc;
 
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
@@ -1086,6 +1096,10 @@  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
+unprepare_rproc:
+	/* release HW resources if needed */
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
 disable_iommu:
 	rproc_disable_iommu(rproc);
 	return ret;
@@ -1331,6 +1345,10 @@  void rproc_shutdown(struct rproc *rproc)
 	/* clean up all acquired resources */
 	rproc_resource_cleanup(rproc);
 
+	/* release HW resources if needed */
+	if (rproc->ops->unprepare)
+		rproc->ops->unprepare(rproc);
+
 	rproc_disable_iommu(rproc);
 
 	/* Free the copy of the resource table */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4aa30bd..dcfa601 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -333,6 +333,8 @@  struct rproc_mem_entry {
 
 /**
  * struct rproc_ops - platform-specific device handlers
+ * @prepare:	prepare device for code loading
+ * @unprepare:	unprepare device after stop
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
@@ -345,6 +347,8 @@  struct rproc_mem_entry {
  * @get_boot_addr:	get boot address to entry point specified in firmware
  */
 struct rproc_ops {
+	int (*prepare)(struct rproc *rproc);
+	int (*unprepare)(struct rproc *rproc);
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);