[v2,14/17] remoteproc: Refactor function rproc_trigger_recovery()
diff mbox series

Message ID 20200324214603.14979-15-mathieu.poirier@linaro.org
State New
Headers show
Series
  • remoteproc: Add support for synchronisation with MCU
Related show

Commit Message

Mathieu Poirier March 24, 2020, 9:46 p.m. UTC
Refactor function rproc_trigger_recovery() in order to avoid
reloading the fw image when synchronising with an MCU rather than
booting it.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Suman Anna March 31, 2020, 9:52 p.m. UTC | #1
Hi Mathieu,

On 3/24/20 4:46 PM, Mathieu Poirier wrote:
> Refactor function rproc_trigger_recovery() in order to avoid
> reloading the fw image when synchronising with an MCU rather than
> booting it.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index d3c4d7e6ca25..dbb0a8467205 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1661,7 +1661,7 @@ static void rproc_coredump(struct rproc *rproc)
>   */
>  int rproc_trigger_recovery(struct rproc *rproc)
>  {
> -	const struct firmware *firmware_p;
> +	const struct firmware *firmware_p = NULL;
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> @@ -1678,14 +1678,16 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  	/* generate coredump */
>  	rproc_coredump(rproc);
>  
> -	/* load firmware */
> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> -	if (ret < 0) {
> -		dev_err(dev, "request_firmware failed: %d\n", ret);
> -		goto unlock_mutex;
> +	/* load firmware if need be */
> +	if (!rproc_sync_with_mcu(rproc)) {
> +		ret = request_firmware(&firmware_p, rproc->firmware, dev);
> +		if (ret < 0) {
> +			dev_err(dev, "request_firmware failed: %d\n", ret);
> +			goto unlock_mutex;
> +		}

So, I am trying to understand the need for the flag around
RPROC_SYNC_STATE_CRASHED. Can you explain what all usecases that is
covering?

In anycase, you should probably combine this piece with the flag change
for STATE_CRASHED on the last patch.

regards
Suman

>  	}
>  
> -	/* boot the remote processor up again */
> +	/* boot up or synchronise with the remote processor again */
>  	ret = rproc_start(rproc, firmware_p);
>  
>  	release_firmware(firmware_p);
>
Mathieu Poirier April 2, 2020, 8:35 p.m. UTC | #2
On Tue, Mar 31, 2020 at 04:52:12PM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 3/24/20 4:46 PM, Mathieu Poirier wrote:
> > Refactor function rproc_trigger_recovery() in order to avoid
> > reloading the fw image when synchronising with an MCU rather than
> > booting it.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index d3c4d7e6ca25..dbb0a8467205 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1661,7 +1661,7 @@ static void rproc_coredump(struct rproc *rproc)
> >   */
> >  int rproc_trigger_recovery(struct rproc *rproc)
> >  {
> > -	const struct firmware *firmware_p;
> > +	const struct firmware *firmware_p = NULL;
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> >  
> > @@ -1678,14 +1678,16 @@ int rproc_trigger_recovery(struct rproc *rproc)
> >  	/* generate coredump */
> >  	rproc_coredump(rproc);
> >  
> > -	/* load firmware */
> > -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > -	if (ret < 0) {
> > -		dev_err(dev, "request_firmware failed: %d\n", ret);
> > -		goto unlock_mutex;
> > +	/* load firmware if need be */
> > +	if (!rproc_sync_with_mcu(rproc)) {
> > +		ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > +		if (ret < 0) {
> > +			dev_err(dev, "request_firmware failed: %d\n", ret);
> > +			goto unlock_mutex;
> > +		}
> 
> So, I am trying to understand the need for the flag around
> RPROC_SYNC_STATE_CRASHED. Can you explain what all usecases that is
> covering?

There could scenarios where another entity is in charge of the entire MCU
lifecycle.  That entity could be able to recognise the MCU has crashed and
automatically boot it again, in which case all the remoteproc core needs to do
is synchronise with it.

But it could also be that another entity has booted the MCU when the system
started but if the MCU crashes or is manually stopped, then the AP becomes the
MCU lifecycle. 

> 
> In anycase, you should probably combine this piece with the flag change
> for STATE_CRASHED on the last patch.

Sure.

> 
> regards
> Suman
> 
> >  	}
> >  
> > -	/* boot the remote processor up again */
> > +	/* boot up or synchronise with the remote processor again */
> >  	ret = rproc_start(rproc, firmware_p);
> >  
> >  	release_firmware(firmware_p);
> > 
>
Suman Anna April 9, 2020, 7:02 p.m. UTC | #3
On 4/2/20 3:35 PM, Mathieu Poirier wrote:
> On Tue, Mar 31, 2020 at 04:52:12PM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 3/24/20 4:46 PM, Mathieu Poirier wrote:
>>> Refactor function rproc_trigger_recovery() in order to avoid
>>> reloading the fw image when synchronising with an MCU rather than
>>> booting it.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index d3c4d7e6ca25..dbb0a8467205 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -1661,7 +1661,7 @@ static void rproc_coredump(struct rproc *rproc)
>>>   */
>>>  int rproc_trigger_recovery(struct rproc *rproc)
>>>  {
>>> -	const struct firmware *firmware_p;
>>> +	const struct firmware *firmware_p = NULL;
>>>  	struct device *dev = &rproc->dev;
>>>  	int ret;
>>>  
>>> @@ -1678,14 +1678,16 @@ int rproc_trigger_recovery(struct rproc *rproc)
>>>  	/* generate coredump */
>>>  	rproc_coredump(rproc);
>>>  
>>> -	/* load firmware */
>>> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "request_firmware failed: %d\n", ret);
>>> -		goto unlock_mutex;
>>> +	/* load firmware if need be */
>>> +	if (!rproc_sync_with_mcu(rproc)) {
>>> +		ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "request_firmware failed: %d\n", ret);
>>> +			goto unlock_mutex;
>>> +		}
>>
>> So, I am trying to understand the need for the flag around
>> RPROC_SYNC_STATE_CRASHED. Can you explain what all usecases that is
>> covering?
> 
> There could scenarios where another entity is in charge of the entire MCU
> lifecycle.  That entity could be able to recognise the MCU has crashed and
> automatically boot it again, in which case all the remoteproc core needs to do
> is synchronise with it.

So, Linux won't be responsible for error recovery in that case, and
wondering why this function would even be called in such a scenario?

> 
> But it could also be that another entity has booted the MCU when the system
> started but if the MCU crashes or is manually stopped, then the AP becomes the
> MCU lifecycle.

Yeah, this is more of a standard early-boot by bootloader scenario which
should be satisfied by just the on_init state.

I mostly still trying to understand the usecase here.

regards
Suman


> 
>>
>> In anycase, you should probably combine this piece with the flag change
>> for STATE_CRASHED on the last patch.
> 
> Sure.
> 
>>
>> regards
>> Suman
>>
>>>  	}
>>>  
>>> -	/* boot the remote processor up again */
>>> +	/* boot up or synchronise with the remote processor again */
>>>  	ret = rproc_start(rproc, firmware_p);
>>>  
>>>  	release_firmware(firmware_p);
>>>
>>

Patch
diff mbox series

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index d3c4d7e6ca25..dbb0a8467205 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1661,7 +1661,7 @@  static void rproc_coredump(struct rproc *rproc)
  */
 int rproc_trigger_recovery(struct rproc *rproc)
 {
-	const struct firmware *firmware_p;
+	const struct firmware *firmware_p = NULL;
 	struct device *dev = &rproc->dev;
 	int ret;
 
@@ -1678,14 +1678,16 @@  int rproc_trigger_recovery(struct rproc *rproc)
 	/* generate coredump */
 	rproc_coredump(rproc);
 
-	/* load firmware */
-	ret = request_firmware(&firmware_p, rproc->firmware, dev);
-	if (ret < 0) {
-		dev_err(dev, "request_firmware failed: %d\n", ret);
-		goto unlock_mutex;
+	/* load firmware if need be */
+	if (!rproc_sync_with_mcu(rproc)) {
+		ret = request_firmware(&firmware_p, rproc->firmware, dev);
+		if (ret < 0) {
+			dev_err(dev, "request_firmware failed: %d\n", ret);
+			goto unlock_mutex;
+		}
 	}
 
-	/* boot the remote processor up again */
+	/* boot up or synchronise with the remote processor again */
 	ret = rproc_start(rproc, firmware_p);
 
 	release_firmware(firmware_p);