[PATCH v2] ASoC: Intel: Skylake: Recover BXT FW on DSP boot timeout error

[PATCH v2] ASoC: Intel: Skylake: Recover BXT FW on DSP boot timeout error

[Pawel Harlozinski]

When DSP boots with timeout error try to reload firmware
to keep best user expeirience.

Signed-off-by: Szymon Mielczarek <szymonx.mielczarek@intel.com>
Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
---
 sound/soc/intel/skylake/bxt-sst.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
index 440bca7afbf1..dd0260557530 100644
--- a/sound/soc/intel/skylake/bxt-sst.c
+++ b/sound/soc/intel/skylake/bxt-sst.c
@@ -455,13 +455,19 @@ static int bxt_set_dsp_D0(struct sst_dsp *ctx, unsigned int core_id)
 	/* If core 1 was turned on for booting core 0, turn it off */
 		skl_dsp_core_power_down(ctx, SKL_DSP_CORE_MASK(1));
 		if (ret == 0) {
-			dev_err(ctx->dev, "%s: DSP boot timeout\n", __func__);
-			dev_err(ctx->dev, "Error code=0x%x: FW status=0x%x\n",
+			dev_warn(ctx->dev,
+				"DSP boot timeout: Error code=0x%x: FW status=0x%x\n",
 				sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE),
 				sst_dsp_shim_read(ctx, BXT_ADSP_FW_STATUS));
-			dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
-			ret = -EIO;
-			goto err;
+			dev_warn(ctx->dev,"Reloading FW\n");
+
+			ret = bxt_sst_init_fw(skl->dev, skl);
+			if (ret < 0) {
+				dev_err(ctx->dev, "Reload fw failed: %d\n", ret);
+				dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
+				ret = -EIO;
+				goto err;
+			}
 		}
 	}
 
@@ -486,7 +492,6 @@ static int bxt_set_dsp_D0(struct sst_dsp *ctx, unsigned int core_id)
 	if (core_id == SKL_DSP_CORE0_ID)
 		core_mask |= SKL_DSP_CORE_MASK(1);
 	skl_dsp_disable_core(ctx, core_mask);
-
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH v2] ASoC: Intel: Skylake: Recover BXT FW on DSP boot timeout error

[Cezary Rojewski]

On 2019-07-08 16:02, Pawel Harlozinski wrote:
> When DSP boots with timeout error try to reload firmware
> to keep best user expeirience.
> 
> Signed-off-by: Szymon Mielczarek <szymonx.mielczarek@intel.com>
> Signed-off-by: Pawel Harlozinski <pawel.harlozinski@linux.intel.com>
> ---
>   sound/soc/intel/skylake/bxt-sst.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c
> index 440bca7afbf1..dd0260557530 100644
> --- a/sound/soc/intel/skylake/bxt-sst.c
> +++ b/sound/soc/intel/skylake/bxt-sst.c
> @@ -455,13 +455,19 @@ static int bxt_set_dsp_D0(struct sst_dsp *ctx, unsigned int core_id)
>   	/* If core 1 was turned on for booting core 0, turn it off */
>   		skl_dsp_core_power_down(ctx, SKL_DSP_CORE_MASK(1));
>   		if (ret == 0) {
> -			dev_err(ctx->dev, "%s: DSP boot timeout\n", __func__);
> -			dev_err(ctx->dev, "Error code=0x%x: FW status=0x%x\n",
> +			dev_warn(ctx->dev,
> +				"DSP boot timeout: Error code=0x%x: FW status=0x%x\n",
>   				sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE),
>   				sst_dsp_shim_read(ctx, BXT_ADSP_FW_STATUS));
> -			dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
> -			ret = -EIO;
> -			goto err;
> +			dev_warn(ctx->dev,"Reloading FW\n");
> +
> +			ret = bxt_sst_init_fw(skl->dev, skl);
> +			if (ret < 0) {
> +				dev_err(ctx->dev, "Reload fw failed: %d\n", ret);
> +				dev_err(ctx->dev, "Failed to set core0 to D0 state\n");
> +				ret = -EIO;
> +				goto err;
> +			}

First, I'm sorry for the delay in response.

Now, where were we..
I'd combine there dump messages together - that is both dev_warns and 
both dev_errs. No need to spam 2 lines when there is single a message to 
communicate on each occasion.

Any reason for ignoring return code of bxt_sst_init_fw and overwriting 
it with -EIO? If the reason is _good_ then you can skip assignment an do 
if (bxt_sst_init_fw()) directly.

Apart from that, I'd like to see a FIXME comment or something of that 
sort. FW preparation is being updated in patches to come and if this fix 
cannot wait, it's good to mention it targets BXT platforms only, while 
SKL/ CNL descendants are left alone.

Czarek

>   		}
>   	}
>   
> @@ -486,7 +492,6 @@ static int bxt_set_dsp_D0(struct sst_dsp *ctx, unsigned int core_id)
>   	if (core_id == SKL_DSP_CORE0_ID)
>   		core_mask |= SKL_DSP_CORE_MASK(1);
>   	skl_dsp_disable_core(ctx, core_mask);
> -
>   	return ret;
>   }
>   
>